Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.2fromktherage:ldap_forward_compat
Jul 29, 2022

Conversation

@ktherage
Copy link
Contributor

@ktheragektherage commentedJun 15, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?no
Deprecations?no
Tickets
LicenseMIT
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

@carsonbotcarsonbot added this to the5.4 milestoneJun 15, 2022
@carsonbotcarsonbot changed the title[LDAP] Fixing missing 'user_identifier' forward compatibility in CheckLdapCredentialsListener[Ldap] Fixing missing 'user_identifier' forward compatibility in CheckLdapCredentialsListenerJun 15, 2022
@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

Copy link
Contributor

@JayfrownJayfrown left a comment

Choose a reason for hiding this comment

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

lgtm

@nicolas-grekas
Copy link
Member

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

@nicolas-grekasnicolas-grekas modified the milestones:5.4,6.2Jun 20, 2022
@wouterj
Copy link
Member

I agree with@nicolas-grekas.

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

$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

@ktheragektherage changed the base branch from5.4 to6.2June 29, 2022 12:51
@ktherage
Copy link
ContributorAuthor

ktherage commentedJun 29, 2022•
edited
Loading

@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.

@ktheragektherage changed the title[Ldap] Fixing missing 'user_identifier' forward compatibility in CheckLdapCredentialsListener[Ldap] Deprecate '{username}' parameter use in favour of '{user_indentifier}' in LDAP configurationJun 29, 2022
Copy link
Member

@wouterjwouterj left a comment

Choose a reason for hiding this comment

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

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

@nicolas-grekasnicolas-grekasforce-pushed theldap_forward_compat branch 2 times, most recently from4efdadb to1f11b3eCompareJuly 28, 2022 12:40
@nicolas-grekasnicolas-grekas changed the title[Ldap] Deprecate '{username}' parameter use in favour of '{user_indentifier}' in LDAP configuration[Ldap] Deprecate '{username}' parameter use in favour of '{user_identifier}' in LDAP configurationJul 28, 2022
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I just pushed the last changes)

@fabpot
Copy link
Member

Thank you@ktherage.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@wouterjwouterjwouterj left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

+1 more reviewer

@JayfrownJayfrownJayfrown approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

6 participants

@ktherage@carsonbot@nicolas-grekas@wouterj@fabpot@Jayfrown
[8]ページ先頭
©2009-2025 Movatter.jp