Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/form.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\Form\Extension\Core\Type\ColorType;
use Symfony\Component\Form\Extension\Core\Type\FileType;
use Symfony\Component\Form\Extension\Core\Type\FormType;
use Symfony\Component\Form\Extension\Core\Type\PasswordHasherExtension;
use Symfony\Component\Form\Extension\Core\Type\SubmitType;
use Symfony\Component\Form\Extension\Core\Type\TransformationFailureExtension;
use Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension;
Expand Down Expand Up @@ -113,6 +114,13 @@
->args([service('translator')->ignoreOnInvalid()])
->tag('form.type')

->set('form.type_extension.form.password_hasher', PasswordHasherExtension::class)
->args([
service('security.password_hasher')->ignoreOnInvalid(),
service('form.property_accessor'),
])
->tag('form.type_extension', ['extended-type' => FormType::class])

->set('form.type_extension.form.transformation_failure_handling', TransformationFailureExtension::class)
->args([service('translator')->ignoreOnInvalid()])
->tag('form.type_extension', ['extended-type' => FormType::class])
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* Deprecate calling `FormErrorIterator::children()` if the current element is not iterable.
* Allow to pass `TranslatableMessage` objects to the `help` option
* Added a `hash_password` option to `PasswordType`

5.3
---
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface;
use Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory;
use Symfony\Component\Form\ChoiceList\Factory\PropertyAccessDecorator;
use Symfony\Component\Form\Extension\Core\Type\PasswordHasherExtension;
use Symfony\Component\Form\Extension\Core\Type\TransformationFailureExtension;
use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Contracts\Translation\TranslatorInterface;
Expand All @@ -31,12 +33,14 @@ class CoreExtension extends AbstractExtension
private $propertyAccessor;
private $choiceListFactory;
private $translator;
private $passwordHasher;

public function __construct(PropertyAccessorInterface $propertyAccessor = null, ChoiceListFactoryInterface $choiceListFactory = null, TranslatorInterface $translator = null)
public function __construct(PropertyAccessorInterface $propertyAccessor = null, ChoiceListFactoryInterface $choiceListFactory = null, TranslatorInterface $translator = null, UserPasswordHasherInterface $passwordHasher = null)
{
$this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
$this->choiceListFactory = $choiceListFactory ?? new CachingFactoryDecorator(new PropertyAccessDecorator(new DefaultChoiceListFactory(), $this->propertyAccessor));
$this->translator = $translator;
$this->passwordHasher = $passwordHasher;
}

protected function loadTypes()
Expand Down Expand Up @@ -83,6 +87,7 @@ protected function loadTypes()
protected function loadTypeExtensions()
{
return [
new PasswordHasherExtension($this->passwordHasher, $this->propertyAccessor),
new TransformationFailureExtension($this->translator),
];
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Core\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Form\Extension\Core\Type\RepeatedType;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;

/**
* @author Sébastien Alfaiate <s.alfaiate@webarea.fr>
*/
class PasswordHasherListener implements EventSubscriberInterface
{
private $passwordHasher;
private $propertyAccessor;

/** @var FormInterface[][] */
private static $passwordTypes = [];
Copy link
Member

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

Copy link
Member

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.


public function __construct(UserPasswordHasherInterface $passwordHasher = null, PropertyAccessorInterface $propertyAccessor = null)
{
$this->passwordHasher = $passwordHasher;
$this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor();
}

public static function getSubscribedEvents()
{
return [
FormEvents::POST_SUBMIT => ['hashPasswords', -2048],
];
}

public function registerPasswordType(FormEvent $event)
{
$form = $event->getForm();
$parentForm = $form->getParent();
$rootName = $form->getRoot()->getName();
Copy link
Member

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.


if (!isset(self::$passwordTypes[$form->getName()])) {
self::$passwordTypes[$rootName] = [];
}

if ($parentForm && $parentForm->getConfig()->getType()->getInnerType() instanceof RepeatedType) {
if ('first' == $form->getName()) {
self::$passwordTypes[$rootName][] = $parentForm;
}
} else {
self::$passwordTypes[$rootName][] = $form;
}
}

public function hashPasswords(FormEvent $event)
{
$form = $event->getForm();

if ($form->isRoot() && $form->isValid() && isset(self::$passwordTypes[$form->getName()])) {
foreach (self::$passwordTypes[$form->getName()] as $passwordType) {
if (($user = $passwordType->getParent()->getData()) && ($user instanceof PasswordAuthenticatedUserInterface)) {
$this->propertyAccessor->setValue(
$user,
$passwordType->getPropertyPath(),
Copy link
Member

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.

$this->passwordHasher->hashPassword($user, $passwordType->getData())
);
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\Extension\Core\EventListener\PasswordHasherListener;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;

/**
* @author Sébastien Alfaiate <s.alfaiate@webarea.fr>
*/
class PasswordHasherExtension extends AbstractTypeExtension
{
private $passwordHasher;
private $propertyAccessor;

public function __construct(UserPasswordHasherInterface $passwordHasher = null, PropertyAccessorInterface $propertyAccessor = null)
{
$this->passwordHasher = $passwordHasher;
$this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor();
}

/**
* {@inheritdoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder->addEventSubscriber(new PasswordHasherListener($this->passwordHasher, $this->propertyAccessor));
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

  1. When the event is triggered on a password form field, if it is eligible, it is stored in a static property
  2. Loop on the static property to hash the passwords

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 the PasswordType, and then, another event only for the root form that will not require to recurse the whole form tree.

}

/**
* {@inheritdoc}
*/
public static function getExtendedTypes(): iterable
{
return [FormType::class];
}
}
14 changes: 14 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/PasswordType.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,26 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\EventListener\PasswordHasherListener;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\Options;
use Symfony\Component\OptionsResolver\OptionsResolver;

class PasswordType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
if ($options['hash_password']) {
$builder->addEventListener(FormEvents::POST_SUBMIT, [new PasswordHasherListener(), 'registerPasswordType']);
}
}

/**
* {@inheritdoc}
*/
Expand All @@ -36,6 +49,7 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults([
'always_empty' => true,
'hash_password' => false,
'trim' => false,
'invalid_message' => function (Options $options, $previousValue) {
return ($options['legacy_error_messages'] ?? true)
Expand Down