-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] add PasswordEncoderInterface::needsRehash() #31594
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
eae871b
to
7daff22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth an UPGRADE entry as not implementing them will break in 5.0
d91d302
to
25c197b
Compare
25c197b
to
e30c8c2
Compare
e30c8c2
to
50590dc
Compare
friendly ping @symfony/deciders |
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function needsRehash(string $encoded): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a check for BCrypt passwords too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's deprecated so I don't think we should improve it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with this. It's best practice to have the hash contain the algo, salt, and options and then determine based on this if a rehash is needed.
But going this way contradics the old way of doing that using a salt parameter and UserPasswordEncoderInterface
. So I think we should deprecate
- the salt paramter in
public function encodePassword($raw, $salt); public function getSalt(); - maybe also
symfony/src/Symfony/Component/Security/Core/Encoder/UserPasswordEncoderInterface.php
Line 21 in 0119d21
interface UserPasswordEncoderInterface
Thank you @nicolas-grekas. |
… (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Security] add PasswordEncoderInterface::needsRehash() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Split from #31153, with tests. Commits ------- 50590dc [Security] add PasswordEncoderInterface::needsRehash()
Split from #31153, with tests.