[Form] Add hash_mapping option to PasswordType #42883
Conversation
There was a problem hiding this comment.
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).
| public static function getSubscribedEvents() | ||
| { | ||
| return [ | ||
| FormEvents::SUBMIT => ['hashPassword', -256], |
There was a problem hiding this comment.
if hash_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.
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.
The purpose of this PR is to store the password hash in a different property.
I did the change so hash_mapping cannot refer to itself.
And the PasswordType field should be unmapped. Maybe I should force it if hash_mapping is used?
Also, if some validation constraints are put on the entity password property (the hashed one), for example a NotBlank, 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.
| @@ -36,6 +60,7 @@ public function configureOptions(OptionsResolver $resolver) | |||
| { | |||
| $resolver->setDefaults([ | |||
| 'always_empty' => true, | |||
| 'hash_mapping' => null, | |||
There was a problem hiding this comment.
In this new approach hash_mapping must be different from property_path to avoid the issue explained above.
There was a problem hiding this comment.
Yes you are right, and this is also the reason we cannot use the previous PR.
Seb33300
changed the title
Seb33300
force-pushed
the
password-hash-mapping
branch
2 times, most recently
from
599f112 to
300928c
Compare
| { | ||
| if ($options['hash_mapping']) { | ||
| if ($options['hash_mapping'] == $builder->getName()) { | ||
| throw new InvalidConfigurationException('The hash_mapping property must be a different.'); |
There was a problem hiding this comment.
Not sure if I had to use InvalidConfigurationException or InvalidOptionsException
Seb33300
force-pushed
the
password-hash-mapping
branch
from
300928c to
4c67975
Compare
|
I created the documentation PR. |
|
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#15872 Same 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
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 as Csrf, 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.