8000 [Ldap] Make LdapUserProvider::refreshUser() actually reload the user entry by Mathieudewet · Pull Request #33246 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Ldap] Make LdapUserProvider::refreshUser() actually reload the user entry #33246

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[Ldap] Make LdapUserProvider::refreshUser() actually reload the user entry #33246

wants to merge 3 commits into from

Conversation

Mathieudewet
Copy link
Contributor
@Mathieudewet Mathieudewet commented Aug 19, 2019 •
edited
Loading
Q A
Branch? 4.4
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass?
Fixed tickets
License MIT
Doc PR

@Mathieudewet Mathieudewet force-pushed the fix/ldap-refresh-user branch 2 times, most recently from 301500a to 5472601 Compare August 19, 2019 18:43
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 19, 2019
@nicolas-grekas
Copy link
Member

I'm sorry but I miss the point of this PR: why would an LDAP user provider have to know anything about a Doctrine ORM?

@Mathieudewet Mathieudewet force-pushed the fix/ldap-refresh-user branch 3 times, most recently from 7ee1af0 to 75a3d59 Compare August 20, 2019 06:32
@Mathieudewet Mathieudewet force-pushed the fix/ldap-refresh-user branch from 75a3d59 to 7de44eb Compare August 20, 2019 07:20
@Simperfit
Copy link
Contributor

I don't think that's a right fix either :/

@chalasr chalasr removed the Bug label Aug 20, 2019
@chalasr
Copy link
Member
chalasr commented Aug 20, 2019 •
edited
Loading

LGTM now that the doctrine-related code has been reverted, deauthentication should be triggered if the user has changed on the ldap server.
Related #18401 (comment) from @csarrazi

The refreshUser method actually only refreshes the user from the session, without checking whether he has changed or not. This could change at a later time.

@chalasr
Copy link
Member
chalasr commented Aug 20, 2019

@Mathieudewet Can you add a test and a note in the Ldap component's CHANGELOG file?

@chalasr chalasr changed the title Fix refresh method of LdapUserProvider [Ldap] Make LdapUserProvider::refreshUser() actually reload the user entry Aug 20, 2019
@Mathieudewet Mathieudewet force-pushed the fix/ldap-refresh-user branch from bc48b5c to d786f99 Compare August 21, 2019 06:15
@Mathieudewet Mathieudewet force-pushed the fix/ldap-refresh-user branch from d786f99 to c847af7 Compare August 21, 2019 06:19
@csarrazi
Copy link
Contributor

Note that this will break for session-based mechanisms. Stateless authentication modes (Basic / Digest) will be able to work, as the user never has to be refreshed, but any other authentication mode might break, if you are using the authenticated user credentials in order to fetch the user information (bind w/ authenticated user vs bind w/ search user). The only way this can work is with a search user.

👎 on this.

@chalasr
Copy link
Member
chalasr commented Aug 24, 2019

Thank you for reviewing @csarrazi.
Closing as explained.

@chalasr chalasr closed this Aug 24, 2019
@Mathieudewet Mathieudewet deleted the fix/ldap-refresh-user branch August 26, 2019 09:00
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Status: Needs Work
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@Mathieudewet @nicolas-grekas @Simperfit @chalasr @csarrazi @carsonbot
0