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

Comments

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

Merged
derrabus merged 1 commit intosymfony:5.3from
dbrumann:issue_41753
Jun 21, 2021
Merged

[PasswordHasher] UserPasswordHasher only calls getSalt when method exists#41755
derrabus merged 1 commit intosymfony:5.3from
dbrumann:issue_41753

Conversation

@dbrumann
Copy link
Contributor
@dbrumann dbrumann commented Jun 20, 2021
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
derrabus commented Jun 20, 2021

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

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 $ 8000 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
dbrumann 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)

Sure thing, but probably not today.

@dbrumann
Copy link
Contributor Author
dbrumann commented Jun 20, 2021

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

stof
stof reviewed Jun 21, 2021
View reviewed changes
@derrabus
Copy link
Member
derrabus commented Jun 21, 2021

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