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_password option to PasswordType#42807
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
| $data = $event->getData(); | ||
| if ($form->isRoot() && $form->isValid() && $data instanceof PasswordAuthenticatedUserInterface) { | ||
| foreach ($form->all() as $field) { |
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.
This works only if the password field is a child of the root form, which is weird to me
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 agree with@stof, but even if you could work around it with some recursive procedure, it looks to me out of the Form component scope changing the underlying data after the data mapping & validation process. I'm 👎 for that reason.
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.
Totally forgot about the subforms. I updated the code to support them with recursive function.
| $data = $event->getData(); | ||
| if ($form->isRoot() && $form->isValid() && $data instanceof PasswordAuthenticatedUserInterface) { | ||
| foreach ($form->all() as $field) { |
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 agree with@stof, but even if you could work around it with some recursive procedure, it looks to me out of the Form component scope changing the underlying data after the data mapping & validation process. I'm 👎 for that reason.
| $this->propertyAccessor->setValue( | ||
| $data, | ||
| $field->getPropertyPath(), | ||
| $this->passwordHasher->hashPassword($form->getData(), $field->getData()) |
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.
$this->passwordHasher might be null
| */ | ||
| public function buildForm(FormBuilderInterface $builder, array $options) | ||
| { | ||
| $builder->addEventSubscriber(new PasswordHasherListener($this->passwordHasher, $this->propertyAccessor)); |
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.
What is the performance impact of adding this recursing listener onall forms, even when they don't rely on PasswordType at all and even less on the new option ? The design used in this PR seems wrong to me, as it breaks the encapsulation of types.
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 event is added to all forms but only the root form is concerned ($form->isRoot() condition in the listener):
https://github.com/symfony/symfony/pull/42807/files#diff-fd2fa2abcca2add3e709b18c862ca809369f50fe351e1b381761fe9249f32288R48-R52
I don't think it is possible to determine that we are on the root form from the builder.FormTypeCsrfExtension &FormTypeValidatorExtension are working in the same way. The event is added to all forms and the condition$form->isRoot() is in the listener to target the root form.
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.
but you still recurse in the whole form tree.
The CSRF extension applies on all forms, but there is 2 very important difference:
- it only performs work for the root form. It does not recurse in the whole form tree
- CSRF protection is a feature that actually makes useful work for all root forms (if the CSRF protection is not enabled in the options, the listener is not registered and so does not run).
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 see. I have an idea to prevent recursion on the whole form.
I will give a try on it.
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 push a new version of the listener.
- When the event is triggered on a password form field, if it is eligible, it is stored in a static property
- Loop on the static property to hash the passwords
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 think I can make it more clean. I got more inspiration during the night XD
I will try to do the changes today.
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 fixed this using an event listener in thePasswordType, and then, another event only for the root form that will not require to recurse the whole form tree.
Seb33300 commentedSep 2, 2021
I pushed a new version working without recursive function to fix@stof review. The last thing I am wondering is if I should move all of this out from the
|
stof 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.
this pattern of first setting the plain password inside the object in the property meant to store the password hash and then replacing it with the password hash after the validation has run is quite dangerous to me:
- it will be a mess to actually define validation rules, because you cannot put them in your object (otherwise, they will run on the password hash in other cases)
- if something stops before running the replacement, your object will keep the plain password in the field, which opens the door to leaking it (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).
- it makes it a lot harder to deal with an optional field, as it will overwrite the existing password hash with
nullor the empty string (and then later with the hash in your listener, which is even broken if the data isnullbecause of not submitting a value)
To me, we should not implement a feature that requires storing the plain password temporarily in the place expecting to contain the password hash. It makes it far too easy to break things.
| { | ||
| $form = $event->getForm(); | ||
| $parentForm = $form->getParent(); | ||
| $rootName = $form->getRoot()->getName(); |
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.
Be careful.$form->getRoot()->getName() is not a unique identifier of the root form over the life of the PHP process.
| private $propertyAccessor; | ||
| /** @var FormInterface[][] */ | ||
| private static $passwordTypes = []; |
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.
you never seem to remove things from this static property, which creates memory leaks (for instance for application servers than don't stop the PHP process at the end of each request processing, but this also includes testsuites...)
| private $propertyAccessor; | ||
| /** @var FormInterface[][] */ | ||
| private static $passwordTypes = []; |
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 name is quite confusing, because it does not contains types at all.
| if (($user = $passwordType->getParent()->getData()) && ($user instanceof PasswordAuthenticatedUserInterface)) { | ||
| $this->propertyAccessor->setValue( | ||
| $user, | ||
| $passwordType->getPropertyPath(), |
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.
be careful about unmapped fields, which won't work here.
Seb33300 commentedSep 3, 2021
I may give another try in a different way. Probably something like an option So the plain password can be an unmapped |
Uh oh!
There was an error while loading.Please reload this page.
This PR adds a new
hash_passwordoption toPasswordType.When
hash_passwordis set totruethe form will return a hashed password (using thesecurity.ymlconfig) in place of the plain password.The hash is generated on post submit AFTER form validation so that validators can validate constraints on the plain password.
And ONLY IF THE FORM IS VALID so that the password field can be filled with the plain password if the
always_emptyoption is disabled when validation fails.This option will make very easy to build registration & change password forms (No more unmapped plainPassword field or listener to setup on our projects).
For example, on EasyAdminBundle project, many developers are facing difficulties in setting up password hash:
EasyCorp/EasyAdminBundle#3349
If the feedback is positive I can continue the work to add tests and documentation.