Skip to content

refactor(ldap)!: migrate to symfony/ldap#1324

Draft
JohnVillalovos wants to merge 1 commit intodevelopfrom
jlvillal/symfony_ldap
Draft

refactor(ldap)!: migrate to symfony/ldap#1324
JohnVillalovos wants to merge 1 commit intodevelopfrom
jlvillal/symfony_ldap

Conversation

@JohnVillalovos
Copy link
Copy Markdown
Collaborator

Switch the LDAP authentication plugin from the pear/net_ldap2 library to symfony/ldap. This removes the deprecated Net_LDAP2-based implementation and replaces it with a new SymfonyLdapWrapper.

Key changes:

  • Add symfony/ldap to composer.json requirements
  • Replace Ldap2Wrapper with SymfonyLdapWrapper using the Symfony LDAP client
  • Replace LdapOptions::Ldap2Config() with GetConnectionConfig() returning a structured array with a connectionString instead of a host array
  • Update LdapUser to use Symfony\Component\Ldap\Entry instead of Net_LDAP2_Entry
  • Add type declarations throughout LdapOptions, LdapUser, and LdapTest
  • Add GetUserIdAttribute() validation to prevent LDAP injection via attribute name

BREAKING CHANGE: pear/net_ldap2 is no longer used; remove it from your installation. The 'uri' config key is required (host/port are rejected).

Copilot AI review requested due to automatic review settings April 9, 2026 05:04
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/symfony_ldap branch from a465a88 to 9f5a5ce Compare April 9, 2026 05:08
Copy link
Copy Markdown
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

Migrates the LDAP authentication plugin from the deprecated pear/net_ldap2 implementation to symfony/ldap, updating configuration handling, LDAP entry usage, and documentation accordingly.

Changes:

  • Replace Ldap2Wrapper with a new SymfonyLdapWrapper built on Symfony’s ext-ldap adapter.
  • Refactor LDAP connection configuration to use a required uri connection string (reject legacy host/port) and add user-id attribute validation.
  • Update LDAP user mapping to use Symfony\Component\Ldap\Entry, and adjust docs/composer dependency.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Plugins/Authentication/Ldap/LdapTest.php Updates tests to Symfony LDAP Entry and new options API; adds new validation test.
plugins/Authentication/Ldap/SymfonyLdapWrapper.php Introduces the new Symfony-based LDAP wrapper for connect/auth/search/group handling.
plugins/Authentication/Ldap/namespace.php Switches plugin includes from Ldap2Wrapper to SymfonyLdapWrapper.
plugins/Authentication/Ldap/LdapUser.php Migrates LDAP entry handling from Net_LDAP2 entries to Symfony Entry.
plugins/Authentication/Ldap/LdapOptions.php Replaces Ldap2Config() with GetConnectionConfig() and enforces uri-based configuration.
plugins/Authentication/Ldap/Ldap2Wrapper.php Removes the deprecated Net_LDAP2-based wrapper.
plugins/Authentication/Ldap/Ldap.php Wires the plugin to use SymfonyLdapWrapper by default.
docs/source/LDAP-Authentication.rst Updates installation and configuration docs to reference symfony/ldap.
composer.json Replaces pear/net_ldap2 dependency with symfony/ldap.

Comment on lines 5 to 7
require_once(ROOT_DIR . 'plugins/Authentication/Ldap/namespace.php');
use Symfony\Component\Ldap\Entry;

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

use Symfony\Component\Ldap\Entry; is placed after a require_once statement. In PHP, use imports must appear before any non-declare code at the top level, otherwise this will cause a parse error. Move the use statement above the require_once, or instantiate via the fully-qualified class name instead.

Suggested change
require_once(ROOT_DIR . 'plugins/Authentication/Ldap/namespace.php');
use Symfony\Component\Ldap\Entry;
use Symfony\Component\Ldap\Entry;
require_once(ROOT_DIR . 'plugins/Authentication/Ldap/namespace.php');

Copilot uses AI. Check for mistakes.
$options = new LdapOptions();

$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('Invalid LDAP attribute name for user.id.attribute');
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

This test expects the exception message to equal Invalid LDAP attribute name for user.id.attribute, but LdapOptions::GetUserIdAttribute() throws Invalid LDAP attribute name for user.id.attribute: '<value>' (includes the invalid attribute). Update the expectation to match the full message, or use a regex-based matcher.

Suggested change
$this->expectExceptionMessage('Invalid LDAP attribute name for user.id.attribute');
$this->expectExceptionMessage("Invalid LDAP attribute name for user.id.attribute: 'uid)(|(cn=*))'");

Copilot uses AI. Check for mistakes.
Comment thread tests/Plugins/Authentication/Ldap/LdapTest.php
@JohnVillalovos JohnVillalovos marked this pull request as draft April 9, 2026 05:22
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/symfony_ldap branch 3 times, most recently from 798eec4 to b73e329 Compare April 11, 2026 14:00
Switch the LDAP authentication plugin from the pear/net_ldap2 library to
symfony/ldap. This removes the deprecated Net_LDAP2-based implementation
and replaces it with a new SymfonyLdapWrapper.

Key changes:
- Add symfony/ldap to composer.json requirements
- Replace Ldap2Wrapper with SymfonyLdapWrapper using the Symfony LDAP client
- Replace LdapOptions::Ldap2Config() with GetConnectionConfig() returning a
  structured array with a connectionString instead of a host array
- Update LdapUser to use Symfony\Component\Ldap\Entry instead of Net_LDAP2_Entry
- Add type declarations throughout LdapOptions, LdapUser, and LdapTest
- Add GetUserIdAttribute() validation to prevent LDAP injection via attribute name

BREAKING CHANGE: pear/net_ldap2 is no longer used; remove it from your
installation. The 'uri' config key is required (host/port are rejected).
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/symfony_ldap branch from b73e329 to d919870 Compare April 11, 2026 14:07
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.

2 participants