-
-
Notifications
You must be signed in to change notification settings - Fork 9.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1f9d26b
259760a
b8f094e
eb537f0
2b97313
a934274
40c0bb6
c433009
eb0a1b8
f7d5dce
2b4d948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be careful. |
||
|
||
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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( I don't think it is possible to determine that we are on the root form from the builder. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I push a new version of the listener.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed this using an event listener in the |
||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public static function getExtendedTypes(): iterable | ||
{ | ||
return [FormType::class]; | ||
} | ||
} |
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...)