Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Add hash_mapping option to PasswordType#42883
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
Uh oh!
There was an error while loading.Please reload this page.
yceruto left a comment
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.
I continue thinking that this hashing process does not belong to the Form flow (at least under the current possibilities it gives us) because this must be done after validation passed successfully (and not only validations linked to this password field itself but the entire form must be valid). Doing this before it causes security issues and doing this after it looks out of Form's responsibility to modify directly the underlying data without any standard Form's mechanism (data mapper, etc).
| publicstaticfunctiongetSubscribedEvents() | ||
| { | ||
| return [ | ||
| FormEvents::SUBMIT => ['hashPassword', -256], |
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.
ifhash_mapping is referring to the same property path as the mapped field then we are hashing the plain password before validating it, which looks wrong.
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.
Also, if something goes wrong during plain password validation you can end up with a wrong hashed password (from#42807 (review)in the DB if your object is a Doctrine entity and you flush changes, but also potentially when serializing the user in the session if the edited user is the currently authenticated user)
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.
The purpose of this PR is to store the password hash in a different property.
I did the change sohash_mapping cannot refer to itself.
And thePasswordType field should be unmapped. Maybe I should force it ifhash_mapping is used?
Also, if some validation constraints are put on the entity password property (the hashed one), for example aNotBlank, the hash needs to be populated before the validation to work.
And regarding your second comment, if something goes wrong during the validation, "the issue" you described is not really specific to this feature.
All others fields of the form are also already populate in the entity before validation. The same behavior already happens even if we don't use this new feature.
| { | ||
| $resolver->setDefaults([ | ||
| 'always_empty' =>true, | ||
| 'hash_mapping' =>null, |
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.
In this new approachhash_mapping must be different fromproperty_path to avoid the issue explained above.
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.
Yes you are right, and this is also the reason we cannot use the previous PR.
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.
Fixed
599f112 to300928cCompare| { | ||
| if ($options['hash_mapping']) { | ||
| if ($options['hash_mapping'] ==$builder->getName()) { | ||
| thrownewInvalidConfigurationException('The hash_mapping property must be a different.'); |
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.
Not sure if I had to useInvalidConfigurationException orInvalidOptionsException
300928c to4c67975CompareSeb33300 commentedSep 29, 2021
I created the documentation PR. |
Seb33300 commentedMay 1, 2022
Closed for#46224 |
…e` (Seb33300)This PR was merged into the 6.2 branch.Discussion----------[Form] Add `hash_property_path` option to `PasswordType`| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets |Fix#29066| License | MIT| Doc PR |symfony/symfony-docs#15872Same as#42883 but using a Form Extension and rebased to 6.1 & tests.This PR adds a new `hash_mapping` option to `PasswordType`.The `hash_mapping` option can be set with a property path where we want to set the hashed password.The `hash_mapping` option can only be used on unmapped fields to minimize plain password leak.Commits-------7065dfe [Form] Add hash_mapping option to PasswordType
Uh oh!
There was an error while loading.Please reload this page.
Following#42807 with a different implementation.
This PR adds a new
hash_mappingoption toPasswordType.The
hash_mappingoption can be set with a property path where we want to store the hashed password.Some questions I have:
Coreto a dedicatedPasswordHasherextension? (same asCsrf,Validator, ...)PasswordAuthenticatedUserInterfaceentities (is it possible withPasswordHashercomponent?)I am submitting this PR to get feedback for now, and if they are positive, I will continue the work to add tests.