Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] Throw on using both "id" and "migrate_from" option in hasher config#51670
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…on in hasher config
| ->beforeNormalization()->ifString()->then(fn ($v) => ['algorithm' =>$v])->end() | ||
| ->validate() | ||
| ->ifTrue(fn ($v) =>isset($v['migrate_from'],$v['id']) &&0 !==\count($v['migrate_from'])) | ||
| ->thenInvalid('You cannot use "migrate_from" when using a custom service id.') |
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.
As it was silently ignored before, it would be better to trigger a deprecation than throwing an error, to avoid breaking projects in 6.4
ogizanagiSep 18, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Since it would mean such projects are configured in a way the developers think it'll handle the password migration properly, but is not, making them aware of the issue the hard way during upgrade might be better?
The fix for removing this exception is straightforward enough, unless this config is hidden somehow in a dependency or whatever (but I would expect the issue would have been already spotted in such a case).
chalasr commentedSep 15, 2023
What issue could arise if we just make it work as a bugfix instead? |
ogizanagi commentedSep 18, 2023
@chalasr : Do you mean applying this patch to lower branches as well, or making the configuration work with custom hasher ids? |
chalasr commentedSep 18, 2023
@ogizanagi I mean making it work as it is the expected behavior to me. Even if we just didn’t think about it, if making it work doesn’t have much potential to break userland code then I think we should do it instead of deprecating to make it work later. |
ogizanagi commentedSep 18, 2023 • edited by nicolas-grekas
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by nicolas-grekas
Uh oh!
There was an error while loading.Please reload this page.
…th custom hasher service with security bundle config (ogizanagi)This PR was merged into the 5.4 branch.Discussion----------[SecurityBundle][PasswordHasher] Fix password migration with custom hasher service with security bundle config| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#51670 (alternative)| License | MIT| Doc PR | N/ASupersedes#51670 as a bug fix, so the security config properly uses a for the `migrate_from` when a `MigratingPasswordHasher` when a custom hasher service id is used:```yamlsecurity: password_hashers: legacy: md5 App\User\Identity: id: App\Security\CustomHasher migrate_from: - legacy```---> **Note**> 6.3 patch :6.3...ogizanagi:63-security-custom-hasher-migrateCommits-------4d65f30 [SecurityBundle][PasswordHasher] Fix password migration with custom hasher service with security bundle config
I stumbled upon this DX issue lately when trying to definea custom hasher service while handling password migration with a legacy one using
migrate_from.Such a config:
will not indicate any issue to the developper, while the
migrate_fromis completely ignored and will not wrap theCustomHasherinto aMigratingPasswordHasherwith thelegacyextra hasher ; which means the password migration will not work as expected.➜ This PR suggests to throw a configuration exception with a hint regarding this.
Perhaps it would be great to make this possible for Symfony 7.1?