8000 [PasswordHasher] UserPasswordHasher only calls getSalt when method exists by dbrumann · Pull Request #41755 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PasswordHasher] UserPasswordHasher only calls getSalt when method exists #41755

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
Merged
merged 1 commit into from
Jun 21, 2021
Merged

[PasswordHasher] UserPasswordHasher only calls getSalt when method exists #41755

merged 1 commit into from
Jun 21, 2021

Conversation

dbrumann
Copy link
Contributor
Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #41753
License MIT

@dbrumann dbrumann requested a review from chalasr as a code owner June 20, 2021 12:40
@carsonbot carsonbot added this to the 5.3 milestone Jun 20, 2021
@chalasr
Copy link
Member
chalasr commented Jun 20, 2021

Thanks for the perfect report and the patch, Denis.
Could you please check other places where the very same deprecation notice is triggered?
I suspect that at least CheckCredentialsListener and PasswordMigratingListener have the same issue (might be a few more)

@carsonbot carsonbot changed the title UserPasswordHasher only calls getSalt when method exists [PasswordHasher] UserPasswordHasher only calls getSalt when method exists Jun 20, 2021
@derrabus
Copy link
Member

If a user class already implements PasswordAuthenticatedUserInterface, but not LegacyPasswordAuthenticatedUserInterface: do we still want to check if getSalt() exists?

@chalasr
Copy link
Member
chalasr commented Jun 20, 2021 •
edited
Loading
8000

Very good question.
When introducing the PasswordHasher contracts, I decided to not bind them to UserInterface to allow for greater flexibility for both developers and us (maintainers, for potential future evolutions).

But, the current code (before this patch) assumes that if your user class is not implementing LegacyPasswordHasherInterface, it still inevitably has the getSalt() method anyway because it is probably implementing UserInterface which enforces having this method until 6.0.

This patch basically removes that assumption. And actually, because UserPasswordHasher is located in password-hasher and not in security-core, it makes sense to me not to tie it to UserInterface.
So this method_exists() check is just a way to not check for $user instanceof UserInterface.
Yet it won't be needed at all in 6.0+, but having it now allows for non-UserInterface implementations to be compatible with this service, while they get a meaningful notice telling them to implement the new interface.

We could consider this a too edge case for supporting it.
Personally, I think it does not harm so I'm in favor of doing this change.

@dbrumann
Copy link
Contributor Author

Thanks for the perfect report and the patch, Denis.
Could you please check other places where the very same deprecation notice is triggered?
I suspect that at least CheckCredentialsListener and PasswordMigratingListener have the same issue (might be a few more)

Sure thing, but probably not today.

@dbrumann
Copy link
Contributor Author

@chalasr as far as I can tell, the other places do not (yet) need this change, because all occurrences I could find where getting the user via UserPassportInterface which will always return a UserInterface which still has a getSalt() method.

@chalasr
Copy link
Member
chalasr commented Jun 20, 2021

Thanks for confirming.

@derrabus
Copy link
Member

Good catch, thanks Denis.

@derrabus derrabus merged commit 19cd5b4 into symfony:5.3 Jun 21, 2021
@dbrumann dbrumann deleted the issue_41753 branch June 21, 2021 21:42
@fabpot fabpot mentioned this pull request Jun 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@chalasr chalasr chalasr approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug PasswordHasher Status: Reviewed
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
6 participants
@dbrumann @chalasr @derrabus @nicolas-grekas @stof @carsonbot
0