[Form] Add hash_password option to PasswordType #42807

Seb33300 · 2021-08-31T10:46:40Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #29066
License	MIT
Doc PR	TODO

This PR adds a new hash_password option to PasswordType.

When hash_password is set to true the form will return a hashed password (using the security.yml config) 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_empty option 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.

stof · 2021-08-31T13:06:05Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+        $data = $event->getData();
+
+        if ($form->isRoot() && $form->isValid() && $data instanceof PasswordAuthenticatedUserInterface) {
+            foreach ($form->all() as $field) {


This works only if the password field is a child of the root form, which is weird to me

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.

Totally forgot about the subforms. I updated the code to support them with recursive function.

yceruto · 2021-08-31T19:21:33Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+        $data = $event->getData();
+
+        if ($form->isRoot() && $form->isValid() && $data instanceof PasswordAuthenticatedUserInterface) {
+            foreach ($form->all() as $field) {


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.

jderusse · 2021-09-01T06:05:55Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+                $this->propertyAccessor->setValue(
+                    $data,
+                    $field->getPropertyPath(),
+                    $this->passwordHasher->hashPassword($form->getData(), $field->getData())


$this->passwordHasher might be null

stof · 2021-09-01T08:53:27Z

src/Symfony/Component/Form/Extension/Core/Type/PasswordHasherExtension.php

+     */
+    public function buildForm(FormBuilderInterface $builder, array $options)
+    {
+        $builder->addEventSubscriber(new PasswordHasherListener($this->passwordHasher, $this->propertyAccessor));


What is the performance impact of adding this recursing listener on all 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.

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.

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).

I see. I have an idea to prevent recursion on the whole form.
I will give a try on it.

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

I think I can make it more clean. I got more inspiration during the night XD
I will try to do the changes today.

I fixed this using an event listener in the PasswordType, and then, another event only for the root form that will not require to recurse the whole form tree.

Seb33300 · 2021-09-02T06:54:43Z

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 Core to a separate PasswordHasher extension?
I mean like Csrf, Validation, etc... (see screenshot)

stof

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 null or the empty string (and then later with the hash in your listener, which is even broken if the data is null because 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.

stof · 2021-09-03T17:02:21Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+    {
+        $form = $event->getForm();
+        $parentForm = $form->getParent();
+        $rootName = $form->getRoot()->getName();


Be careful. $form->getRoot()->getName() is not a unique identifier of the root form over the life of the PHP process.

stof · 2021-09-03T17:03:31Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+    private $propertyAccessor;
+
+    /** @var FormInterface[][] */
+    private static $passwordTypes = [];


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...)

stof · 2021-09-03T17:04:08Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+    private $propertyAccessor;
+
+    /** @var FormInterface[][] */
+    private static $passwordTypes = [];


The name is quite confusing, because it does not contains types at all.

stof · 2021-09-03T17:04:40Z

src/Symfony/Component/Form/Extension/Core/EventListener/PasswordHasherListener.php

+                if (($user = $passwordType->getParent()->getData()) && ($user instanceof PasswordAuthenticatedUserInterface)) {
+                    $this->propertyAccessor->setValue(
+                        $user,
+                        $passwordType->getPropertyPath(),


be careful about unmapped fields, which won't work here.

Seb33300 · 2021-09-03T17:51:46Z

I may give another try in a different way.

Probably something like an option hash_mapping for PasswordType where we can specify the property we want to use to store the hashed password.

So the plain password can be an unmapped PasswordType.

[Form] Add hash_password option to PasswordType

1f9d26b

Seb33300 requested review from xabbuh and yceruto as code owners August 31, 2021 10:46

carsonbot added Status: Needs Review Feature Form labels Aug 31, 2021

stof reviewed Aug 31, 2021

View reviewed changes

yceruto requested changes Aug 31, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 31, 2021

Seb33300 added 3 commits September 1, 2021 11:12

Add recursive

259760a

Fix event priority (trigger after TransformationFailureListener)

b8f094e

Check if property is writable

eb537f0

Seb33300 mentioned this pull request Sep 1, 2021

Auto Password Encoder #29066

Closed

jderusse reviewed Sep 1, 2021

View reviewed changes

Seb33300 mentioned this pull request Sep 1, 2021

[Security][DX] Added SecurityPasswordType #35654

Closed

stof reviewed Sep 1, 2021

View reviewed changes

Seb33300 added 5 commits September 1, 2021 16:29

Use compound option instead of count

2b97313

Remove recursive function

a934274

Fix type

40c0bb6

Add listener on PasswordType

c433009

Fix static analysis

eb0a1b8

Seb33300 added 2 commits September 2, 2021 13:58

Fix typehint

f7d5dce

Fix typehint

2b4d948

stof requested changes Sep 3, 2021

View reviewed changes

Seb33300 closed this Sep 3, 2021

Seb33300 mentioned this pull request Sep 4, 2021

[Form] Add hash_mapping option to PasswordType #42883

Closed

Seb33300 deleted the hash-password-type branch April 26, 2024 09:26

Uh oh!

[Form] Add hash_password option to PasswordType #42807

[Form] Add hash_password option to PasswordType #42807

Uh oh!

Conversation

Seb33300 commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seb33300 commented Sep 2, 2021

Uh oh!

stof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seb33300 commented Sep 3, 2021

Uh oh!

Uh oh!

Seb33300 commented Aug 31, 2021 •

edited

Loading