[Ldap] Deprecate '{username}' parameter use in favour of '{user_identifier}' in LDAP configuration

[ktherage]

Q	A
Branch?	6.2
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

This PR aims to fix a missing forward compatibility as done in src/Symfony/Component/Ldap/Security/LdapUserProvider.php at line 80 when authenticating with LDAP and using the following configuration :

security:
// ...
    firewalls:œ
    // ...
        main:
            form_login_ldap:
                // ...
                query_string: '(whatever={user_identifier})'
                dn_string: 'uid={user_identifier},dc=example,dc=com'

instead of :

security:
// ...
    firewalls:
    // ...
        main:
            form_login_ldap:
                // ...
                query_string: '(whatever={username})'
                dn_string: 'uid={username},dc=example,dc=com'

maybe related to : #40403

[carsonbot]

Hey!

I think @Jayfrown has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

[Jayfrown]

lgtm

[nicolas-grekas]

It looks like this would benefit from triggering a deprecation when {username} is used. This also means this should target 6.2.

[wouterj]

I agree with @nicolas-grekas.

There is no "forward compatibility" here, as we don't replace {user_identifier} in Symfony 6.0:

symfony/src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php

Line 87 in e0ad2ee

$query = str_replace('{username}', $username, $ldapBadge->getQueryString());

It would be a good idea to change this parameter name though. That would mean replacing both (like you introduce this PR) and trigger a deprecation when {username} is used. This would then need to go in the current development version (6.2), so we can remove {username} in 7.0.

Status: needs work

[ktherage]

@nicolas-grekas / @wouterj,

I made some changes based on your returns.
As it seems centralised in LdapBadge in 6.2, I've added the deprecations on LdapBadge's construction and replaced {username} parameter by {user_identifier} if used.

This will allow a smoother change when droping deprecation notice IMO.

[wouterj]

I like your proposal of doing the cleaning up in the LdapBadge. I've left one comment in a place that is not directly dependent on LdapBadge, but other than that this is ready to go imho!

[nicolas-grekas]

(I just pushed the last changes)

[fabpot]

Thank you @ktherage.