Skip to content

Conversation

@JohnVillalovos
Copy link
Collaborator

To work better with PHP's ldap_connect() function, we have switched to
requiring a URI to specify the LDAP server(s).

This allows specifying multiple LDAP servers, if desired.

BREAKING CHANGE: LDAP plugin configuration now requires uri and no
longer supports host or port. Migrate to: 'uri' => 'ldap://host:389' or 'uri' => 'ldaps://host:636' For multiple
servers, provide a space-separated URI string.

Copilot AI review requested due to automatic review settings February 8, 2026 02:13
@JohnVillalovos JohnVillalovos marked this pull request as draft February 8, 2026 02:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the LDAP authentication plugin to configure LDAP servers via a required URI string (supporting multiple servers), aligning better with PHP LDAP connection semantics and removing support for legacy host/port.

Changes:

  • Replace LDAP plugin configuration from host/port to required uri (space-separated for multiple servers).
  • Remove vendored PEAR/Net_LDAP2 sources and rely on pear/net_ldap2 via Composer (documented + suggested in composer.json).
  • Update LDAP plugin runtime checks, tests, and documentation to reflect the new configuration and dependency model.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/Plugins/Authentication/Ldap/LdapTest.php Updates tests to use URI-based config and adds coverage for rejecting legacy host/port.
plugins/Authentication/Ldap/namespace.php Removes PEAR include-path bootstrapping; relies on normal loading paths.
plugins/Authentication/Ldap/LdapOptions.php Switches option construction to read/validate uri and rejects legacy host/port.
plugins/Authentication/Ldap/LdapConfigKeys.php Removes HOST/PORT keys and introduces URI config key metadata.
plugins/Authentication/Ldap/Ldap2Wrapper.php Uses PEAR::isError checks and adds a runtime guard for missing pear/net_ldap2.
plugins/Authentication/Ldap/Ldap.config.dist.php Updates distributed config template to document and default uri.
plugins/Authentication/Ldap/LDAP2/Util.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/SimpleFileSchemaCache.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/Search.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/SchemaCache.interface.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/Schema.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/RootDSE.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/LDIF.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/Filter.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
plugins/Authentication/Ldap/LDAP2/Entry.php Deletes vendored Net_LDAP2 implementation file (now external dependency).
lib/external/pear/license.txt Removes vendored PEAR license file (PEAR no longer shipped in-tree).
lib/external/pear/PEAR.php Removes vendored PEAR core (now external dependency if needed).
lib/external/pear/Config/Container/PHPArray.php Removes vendored PEAR Config component (no longer shipped in-tree).
lib/external/pear/Config/Container.php Removes vendored PEAR Config component (no longer shipped in-tree).
lib/external/pear/Config.php Removes vendored PEAR Config component (no longer shipped in-tree).
docs/source/LDAP-Authentication.rst Documents prerequisites (pear/net_ldap2) and the new uri configuration with examples.
docs/source/INSTALLATION.rst Updates installation guidance to prefer composer install --no-dev by default.
composer.json Adds pear/net_ldap2 as a suggested dependency for LDAP plugin usage.
.github/workflows/lint-and-analyse-php.yml Minor workflow comment cleanup.

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

Comment on lines +20 to +23
$loadedConfig = @require $configPath;
if (is_array($loadedConfig) && isset($loadedConfig['settings']['ldap']) && is_array($loadedConfig['settings']['ldap'])) {
$this->rawLdapSettings = $loadedConfig['settings']['ldap'];
}
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__construct() loads the config file via @require $configPath and then immediately calls Configuration::Instance()->Register($configPath, ...), which itself @requires the same file again. This double-executes any code in Ldap.config.php (especially problematic for legacy-style configs that mutate $conf or define constants), and makes config loading harder to reason about. Consider avoiding the pre-require and instead capturing the raw settings from the same load path used by Register() (e.g., refactor Register() to return the loaded array, or add a method to access the loaded settings for this config id).

Copilot uses AI. Check for mistakes.
Comment on lines 79 to 81
$message = 'Could not authenticate user against ldap %s: ' . $result->getMessage();
Log::Error($message, $username);
}
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log/error message text says "authenticate user against ldap %s" but the %s is filled with the username, not an LDAP endpoint. This makes the error misleading in logs. Update the message to refer to the user (or include the server/URI if that's what you intended).

Suggested change
$message = 'Could not authenticate user against ldap %s: ' . $result->getMessage();
Log::Error($message, $username);
}
Log::Error('Could not authenticate user %s against ldap: %s', $username, $result->getMessage());
}

Copilot uses AI. Check for mistakes.
To work better with PHP's `ldap_connect()` function, we have switched to
requiring a URI to specify the LDAP server(s).

This allows specifying multiple LDAP servers, if desired.

BREAKING CHANGE: LDAP plugin configuration now requires `uri` and no
longer supports `host` or `port`. Migrate to: `'uri' =>
'ldap://host:389'` or `'uri' => 'ldaps://host:636'` For multiple
servers, provide a space-separated URI string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant