Fix logging to file and LDAP authentication #884

christiankreidl · 2025-11-12T17:35:11Z

Hi!

This pull request fixes:

logging to file app.log not working due to comparison of upper- and lower-case strings
LDAP authentication not working due to problems with new config file format
- ConfigKeys and config file missing prefix
- hard-coded string instead of ConfigKey used
ActiveDirectory authentication also fixed, but could not be tested ( ActiveDirectory.config.php incorrectly parsed #856 )
LDAP log message not an error thus changed to debug

Copilot

Pull Request Overview

This pull request fixes critical authentication and logging issues in the LibreBooking system, specifically addressing LDAP/ActiveDirectory authentication failures and logging configuration problems.

Fixes case-sensitivity bug in logging level comparisons that prevented file logging from working
Corrects LDAP/ActiveDirectory configuration structure to match the new nested config format
Replaces hardcoded config string with proper constant reference in LDAP options
Changes inappropriate error-level log message to debug level

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
lib/Common/Logging/Log.php	Adds `strtolower()` to normalize logging level comparisons, fixing file logging
plugins/Authentication/Ldap/LdapOptions.php	Replaces hardcoded string with `LdapConfigKeys::DEBUG_ENABLED` constant
plugins/Authentication/Ldap/LdapConfigKeys.php	Adds 'ldap.' prefix to all configuration keys for proper namespacing
plugins/Authentication/Ldap/Ldap.config.dist.php	Restructures config to use nested 'ldap' section with unprefixed keys
plugins/Authentication/ActiveDirectory/ActiveDirectoryConfigKeys.php	Adds 'activedirectory.' prefix to all configuration keys for proper namespacing
plugins/Authentication/ActiveDirectory/ActiveDirectory.config.dist.php	Restructures config to use nested 'activedirectory' section with unprefixed keys
plugins/Authentication/Ldap/Ldap2Wrapper.php	Changes uid attribute log message from Error to Debug level

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…omparison

…dingly

…UG instead of ERROR)

christiankreidl · 2025-11-13T09:17:44Z

fix: commit messages changed to fix linting errors

lucs7

@christiankreidl great PR thank you for fixing this.
I would suggest a slightly different solution for the log.php (see below).

for the issue with the Plugin Configuration requiringing he plugin name in the key, I will look into a more generalized solution, so I would suggest to remove commit 58a0ba9 for now from the PR

lucs7 · 2025-11-14T12:36:58Z

I created lucs7@7851a07 to fix the broken PluginConfig structure, which then will correctly apply the section a

@christiankreidl Can you please test if this works with the plugins
I added the rest of the suggestions as a PR in your repo
christiankreidl#1

I created two docker test env to run the ActiveDiretory and Ldap plugins for login, oth passing after some testing :)

…UG instead of ERROR)

Previous changes caused an error in the plugin config loading This commit fixes the structure and places each plugin config into its respective nested section which was existing for some but not all plugin configs

…ration loading and validation

…ation and improve error logging for invalid config values

…description

…y configuration

Admin Username in Active Directory configuration LDAP Host in LDAP configuration

christiankreidl · 2025-11-17T10:50:02Z

@christiankreidl Can you please test if this works with the plugins I added the rest of the suggestions as a PR in your repo christiankreidl#1

@lucs7 The above pull request was tested with LDAP and works. Thanks!

lucs7 · 2025-11-17T15:42:46Z

Can you merge the PR into your repo, this should update this PR too as far as I understand git ^^

Fix/ldap config

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-22T03:15:45Z

plugins/Authentication/Ldap/LdapConfigKeys.php

+ 'default' => '',
 'label' => 'Search Filter',
- 'description' => 'LDAP search filter (use %s for username)',
+ 'description' => 'Optional LDAP search filter for additional constraints (leave empty for default)',


[nitpick] The description change is misleading. The FILTER is indeed optional (empty by default), but the comment "leave empty for default" is confusing because there is no "default" filter - it's just not used. The actual user identification filter is constructed automatically using the user.id.attribute (see Ldap2Wrapper.php line 96 where it creates a filter from the uidAttribute).

Consider clarifying the description to:

'description' => 'Optional additional LDAP search filter to combine with the uid filter (leave empty if not needed)',

Suggested change

'description' => 'Optional LDAP search filter for additional constraints (leave empty for default)',

'description' => 'Optional additional LDAP search filter to combine with the uid filter (leave empty if not needed)',

Copilot · 2025-11-22T03:15:46Z

plugins/Authentication/Ldap/LdapConfigKeys.php

 'type' => 'string',
 'default' => 'sub',
 'label' => 'Search Scope',
 'description' => 'LDAP search scope (base, one, or sub)',


[nitpick] The removal of the 'choices' array for the SCOPE configuration is a regression. The choices provided useful UI hints and validation for the three valid LDAP search scope values ('base', 'one', 'sub'). Removing this makes the configuration less user-friendly and removes validation that could catch configuration errors.

Consider restoring the choices:

'choices' => [ 'base' => 'Base', 'one' => 'One Level', 'sub' => 'Subtree' ]

Suggested change

'description' => 'LDAP search scope (base, one, or sub)',

'description' => 'LDAP search scope (base, one, or sub)',

'choices' => [

'base' => 'Base',

'one' => 'One Level',

'sub' => 'Subtree'

],

Copilot · 2025-11-22T03:15:46Z

lib/Config/PluginConfigKeys.php

+ if ($section) {
+ if ("{$section}.{$configKey}" === $key) {
+ return $config;
+ }


The findByKey method is incomplete. When a config doesn't have a section, the method should still check if the key matches (without section prefix) and return the config. Currently, if $section is null or empty, the method will never return anything, causing all non-sectioned keys to not be found.

Add an else clause to handle configs without sections:

} else { // Config without section - match key directly if ($configKey === $key) { return $config; } }

Suggested change

}

}

} else {

// Config without section - match key directly

if ($configKey === $key) {

return $config;

}

Copilot · 2025-11-22T03:15:47Z

tests/Plugins/Authentication/PluginConfigLoadingTest.php

+
+ public function setUp(): void
+ {
+ parent::setup();


[nitpick] Method name case mismatch: The call to parent::setup() should be parent::setUp() to match the PHPUnit convention and the method signature defined on line 13. PHP method names are case-insensitive, but this inconsistency could cause confusion.

Suggested change

parent::setup();

parent::setUp();

lucs7 · 2025-11-22T16:49:08Z

Looks good to me 🥳

lucs7 · 2025-12-16T18:56:32Z

can be closed as #899 is taking it over

JohnVillalovos requested a review from Copilot November 12, 2025 17:39

Copilot started reviewing on behalf of JohnVillalovos November 12, 2025 17:40 View session

Copilot finished reviewing on behalf of JohnVillalovos November 12, 2025 17:41

Copilot AI reviewed Nov 12, 2025

View reviewed changes

christiankreidl added 4 commits November 13, 2025 10:02

fix: use lower-case log_level to fix case-sensitivity bug in string c…

0cd1b03

…omparison

fix: add prefix to ConfigKeys key value and edit example config accor…

58a0ba9

…dingly

fix: use ConfigKey instead of hard-coded name

95c1bbb

fix: informational log message changed to more appropriate level (DEB…

8d404c0

…UG instead of ERROR)

christiankreidl force-pushed the fix-ldap branch from f157f1c to 8d404c0 Compare November 13, 2025 09:13

JohnVillalovos mentioned this pull request Nov 13, 2025

Make container run as non-root LibreBooking/docker#124

Closed

lucs7 reviewed Nov 14, 2025

View reviewed changes

christiankreidl and others added 9 commits November 14, 2025 16:01

fix: use ConfigKey instead of hard-coded name

803c866

fix: informational log message changed to more appropriate level (DEB…

c986428

…UG instead of ERROR)

fix(ldap): Rename debug configuration key for consistency

b8a896b

fix(auth): Updated plugin configuration keys into nested structures

4e9d65e

Previous changes caused an error in the plugin config loading This commit fixes the structure and places each plugin config into its respective nested section which was existing for some but not all plugin configs

test(auth): Add comprehensive tests for authentication plugin configu…

d3e21f1

…ration loading and validation

fix(config): Preserve unknown subkeys in original structure for valid…

d9160bb

…ation and improve error logging for invalid config values

fix(ldap): Update default search filter to be optional with improved …

d4866ab

…description

docs(auth): Enhance description for Admin Username in Active Director…

ad60cfb

…y configuration

docs(auth): Enhance description for properties in plugins

903b198

Admin Username in Active Directory configuration LDAP Host in LDAP configuration

christiankreidl and others added 2 commits November 18, 2025 09:17

Merge pull request #1 from lucs7/fix/ldap-config

6ea9259

Fix/ldap config

Merge branch 'develop' into fix-ldap

35ba693

JohnVillalovos requested a review from Copilot November 22, 2025 03:10

Copilot started reviewing on behalf of JohnVillalovos November 22, 2025 03:10 View session

Copilot finished reviewing on behalf of JohnVillalovos November 22, 2025 03:14

Copilot AI reviewed Nov 22, 2025

View reviewed changes

JohnVillalovos closed this Dec 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix logging to file and LDAP authentication #884

Fix logging to file and LDAP authentication #884

Uh oh!

christiankreidl commented Nov 12, 2025

Copilot AI left a comment

christiankreidl commented Nov 13, 2025

lucs7 left a comment •

edited

Loading

lucs7 commented Nov 14, 2025 •

edited

Loading

christiankreidl commented Nov 17, 2025

lucs7 commented Nov 17, 2025

Copilot AI left a comment

Copilot AI Nov 22, 2025

Copilot AI Nov 22, 2025

Copilot AI Nov 22, 2025

Copilot AI Nov 22, 2025

lucs7 commented Nov 22, 2025

lucs7 commented Dec 16, 2025

Labels

3 participants

	'description' => 'Optional LDAP search filter for additional constraints (leave empty for default)',
	'description' => 'Optional additional LDAP search filter to combine with the uid filter (leave empty if not needed)',

- 'description' => 'LDAP search scope (base, one, or sub)',
+ 'description' => 'LDAP search scope (base, one, or sub)',
+ 'choices' => [
+ 'base' => 'Base',
+ 'one' => 'One Level',
+ 'sub' => 'Subtree'
+ ],

- }
+ }
+ } else {
+ // Config without section - match key directly
+ if ($configKey === $key) {
+ return $config;
+ }

Fix logging to file and LDAP authentication #884

Fix logging to file and LDAP authentication #884

Uh oh!

Conversation

christiankreidl commented Nov 12, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

christiankreidl commented Nov 13, 2025

lucs7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

lucs7 commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

christiankreidl commented Nov 17, 2025

lucs7 commented Nov 17, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

lucs7 commented Nov 22, 2025

lucs7 commented Dec 16, 2025

Labels

3 participants

lucs7 left a comment •

edited

Loading

lucs7 commented Nov 14, 2025 •

edited

Loading