[Security][DX] Added SecurityPasswordType #35654

pyrech · 2020-02-09T14:40:29Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #...
License	MIT
Doc PR	symfony/symfony-docs#...

Few days ago, I discussed with @lyrixx about how to simplify user password handling in EasyAdmin. Because currently if we want to make user's password administrable we have to create our own custom controller to interact with UserPasswordEncoder. For better DX we concluded that it would be better to add a native SecurityPasswordType in Symfony. So here we go:

This new form type inherits from PasswordType.
It automatically detects the user object from the parent form. This can be overridden with the option security_user.
If no data is submitted, the old password is kept.

I will see to add tests if the idea is accepted.

wouterj

I like this idea! I've left two minor comments.

src/Symfony/Bundle/SecurityBundle/Form/SecurityPasswordType.php

lyrixx · 2020-02-10T10:47:57Z

I would move everything in the Form component. We have already a system for extension

pyrech · 2020-02-16T22:42:01Z

I moved the form type to the form component in a new SecurityExtension 👍

I also added some tests. I'm unsure if I need to add a dev-dependency on symfony/security inside symfony/form component and if yes, on which version : 5.1 but not released, will this break the CI?

lyrixx · 2020-02-17T09:49:55Z

I also added some tests. I'm unsure if I need to add a dev-dependency on symfony/security inside symfony/form component and if yes, on which version : 5.1 but not released, will this break the CI?

Yes :) IHMO you can target 3.4 because it works with 3.4 :)

yceruto · 2020-02-19T18:01:39Z

src/Symfony/Component/Form/Extension/Security/Type/SecurityPasswordType.php

+    public function configureOptions(OptionsResolver $resolver)
+    {
+        $resolver->setDefault('security_user', null);
+        $resolver->setAllowedTypes('security_user', [


the statement should be in one line

yceruto · 2020-02-19T18:05:32Z

src/Symfony/Component/Form/Extension/Security/Type/SecurityPasswordType.php

+        $resolver->setAllowedTypes('security_user', [
+            'null',
+            UserInterface::class,
+        ]);


we've recently added a fluent interface to define options, let's use it.

yceruto · 2020-02-19T18:14:05Z

src/Symfony/Component/Form/Extension/Security/Type/SecurityPasswordType.php

+                    throw new InvalidConfigurationException(sprintf('You should either use "%s" inside a parent form where data is an instance of "%s" or specify the user in "security_user" option', self::class, UserInterface::class));
+                }
+
+                $securityUser = $parentData;


the aux variable can be avoided here. i.e. $securityUser = $event->getForm()->getParent()->getData()

yceruto · 2020-02-19T18:21:20Z

src/Symfony/Component/Form/Extension/Security/Type/SecurityPasswordType.php

+            $securityUser = $event->getForm()->getConfig()->getOption('security_user');
+
+            if (!$securityUser) {
+                $parentData = $event->getForm()->getParent()->getData();


the parent form cannot be null here, should we check?

yceruto · 2020-02-19T18:33:27Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

+        <!-- SecurityExtension -->
+        <service id="form.extension.security" class="Symfony\Component\Form\Extension\Security\SecurityExtension">
+            <argument type="service" id="security.user_password_encoder" on-invalid="ignore"/>
+            <tag name="form.type_extension" />


The target class isn't a form type extension, but a "form extension" which doesn't need to be registered as service.

yceruto · 2020-02-19T18:38:51Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

+            <tag name="form.type_extension" />
+        </service>
+        <service id="form.extension.security.type.security_password" class="Symfony\Component\Form\Extension\Security\Type\SecurityPasswordType">
+            <argument type="service" id="security.user_password_encoder" on-invalid="ignore"/>


This class would be pointless without the required password encoder, I'd suggest make the argument mandatory and remove the whole service definition if "security" isn't enabled. See form.type_extension.form.validator as sample.

yceruto · 2020-02-19T18:40:06Z

src/Symfony/Component/Form/Extension/Security/SecurityExtension.php

+{
+    private $passwordEncoder;
+
+    public function __construct(UserPasswordEncoderInterface $passwordEncoder = null)


Please, remove the nullable definition.

yceruto · 2020-02-19T19:07:23Z

I like the idea too!

I'm wondering if this implementation fits better in a new "password type" extension rather than creating a new type and enable this feature through a new option "auto_encode".

I don't like much the idea to use the user instance. Instead, I would do what @wouterj has suggested here #35654 (comment).

fabpot · 2020-08-11T07:12:12Z

@pyrech @lyrixx Have you read the comments from @yceruto? Any interest in resuming the work here?

yceruto

After a second review I realized that this password encoding cannot be done before validation (that happens in POST_SUBMIT event on the root form) otherwise constraint validators will validate the encoded password (which would be wrong) rather than the plain password. Let's think about the Length() constraint for instance.

But, also changing the form data in POST_SUBMIT event is currently impossible, so I don't think the proposal is feasible this way.

yceruto · 2020-08-12T03:29:18Z

src/Symfony/Component/Form/Extension/Security/Type/SecurityPasswordType.php

+
+            $plainPassword = $event->getData();
+
+            $event->setData($plainPassword ? $this->passwordEncoder->encodePassword($securityUser, $plainPassword) : $securityUser->getPassword());


The submitted data cannot be changed this way before validating the plain password.

In effect, if the validation step fails, the encoded password will be visible to the end user.

fabpot · 2020-09-01T11:55:15Z

@pyrech Can you have a look at the feedback?

pyrech · 2020-09-01T12:51:13Z

I agree with the reviews that the current implementation is not ideal at all. I don't know how to improve it enough though so we might just close this PR 👍

Seb33300 · 2021-09-01T06:07:17Z

I worked on a different approach here: #42807

carsonbot added Status: Needs Review Security DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Feb 9, 2020

wouterj reviewed Feb 9, 2020

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Form/SecurityPasswordType.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/SecurityBundle/Form/SecurityPasswordType.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Feb 9, 2020

pyrech requested a review from xabbuh as a code owner February 16, 2020 21:05

pyrech added 2 commits February 16, 2020 23:38

Add SecurityPasswordType

d9f4308

Remove required option override

8910840

pyrech added 2 commits February 17, 2020 00:17

Move to Form component

1782e14

Add tests

16bc5b1

yceruto requested changes Feb 19, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 19, 2020

yceruto requested changes Aug 12, 2020

View reviewed changes

fabpot closed this Sep 1, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

pyrech deleted the security-password-type branch January 7, 2023 18:17


		$plainPassword = $event->getData();

		$event->setData($plainPassword ? $this->passwordEncoder->encodePassword($securityUser, $plainPassword) : $securityUser->getPassword());

Uh oh!

[Security][DX] Added SecurityPasswordType #35654

[Security][DX] Added SecurityPasswordType #35654

Uh oh!

Conversation

pyrech commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lyrixx commented Feb 10, 2020

Uh oh!

pyrech commented Feb 16, 2020

Uh oh!

lyrixx commented Feb 17, 2020

Uh oh!

yceruto Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto commented Feb 19, 2020

Uh oh!

fabpot commented Aug 11, 2020

Uh oh!

yceruto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

yceruto Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 1, 2020

Uh oh!

pyrech commented Sep 1, 2020

Uh oh!

Seb33300 commented Sep 1, 2021

Uh oh!

Uh oh!

pyrech commented Feb 9, 2020 •

edited

Loading

yceruto Feb 19, 2020 •

edited

Loading

yceruto Feb 19, 2020 •

edited

Loading

yceruto left a comment •

edited

Loading