Skip to content

[Security][DX] Added SecurityPasswordType #35654

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 4 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
10 changes: 10 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,15 @@
<argument type="service" id="translator"/>
<argument type="string">%validator.translation_domain%</argument>
</service>

<!-- 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" />
Copy link
Member

@yceruto yceruto Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</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"/>
Copy link
Member

@yceruto yceruto Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<tag name="form.type" alias="security_password" />
</service>
</services>
</container>
1 change: 1 addition & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ CHANGELOG
is deprecated. The method will be added to the interface in 6.0.
* Implementing the `FormConfigBuilderInterface` without implementing the `setIsEmptyCallback()` method
is deprecated. The method will be added to the interface in 6.0.
* Added `SecurityExtension` and `SecurityPasswordType`

5.0.0
-----
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?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\Security;

use Symfony\Component\Form\AbstractExtension;
use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;

/**
* Integrates the Security (Core) component with the Form library.
*
* @author Loïck Piera <pyrech@gmail.com>
*/
class SecurityExtension extends AbstractExtension
{
private $passwordEncoder;

public function __construct(UserPasswordEncoderInterface $passwordEncoder = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove the nullable definition.

{
$this->passwordEncoder = $passwordEncoder;
}

protected function loadTypes()
{
if (!$this->passwordEncoder) {
return [];
}

return [
new Type\SecurityPasswordType($this->passwordEncoder),
];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?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\Security\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Event\SubmitEvent;
use Symfony\Component\Form\Exception\InvalidConfigurationException;
use Symfony\Component\Form\Extension\Core\Type\PasswordType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface;
use Symfony\Component\Security\Core\User\UserInterface;

/**
* @author Loïck Piera <pyrech@gmail.com>
*/
class SecurityPasswordType extends AbstractType
{
private $passwordEncoder;

public function __construct(UserPasswordEncoderInterface $passwordEncoder)
{
$this->passwordEncoder = $passwordEncoder;
}

public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder->addEventListener(FormEvents::SUBMIT, function (SubmitEvent $event) {
$securityUser = $event->getForm()->getConfig()->getOption('security_user');

if (!$securityUser) {
$parentData = $event->getForm()->getParent()->getData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (!$parentData instanceof UserInterface) {
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

$plainPassword = $event->getData();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the statement should be in one line

'null',
UserInterface::class,
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

public function getParent()
{
return PasswordType::class;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?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\Tests\Extension\Security;

use PHPUnit\Framework\TestCase;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\FormType;
use Symfony\Component\Form\Extension\Security\SecurityExtension;
use Symfony\Component\Form\Extension\Security\Type\SecurityPasswordType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormFactoryBuilder;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Security\Core\User\User;

class SecurityExtensionTest extends TestCase
{
public function testSecurityPasswordTypeWorks()
{
$user = new User('email@example.com', 'previous_password');

$this->assertSame('previous_password', $user->getPassword());

$form = $this->getFormFactory()
->createBuilder(FormType::class, $user)
->add('password', SecurityPasswordType::class, [
'security_user' => $user,
])
->getForm()
;

$form->submit(['password' => 'new_password']);

$this->assertTrue($form->isSubmitted());
$this->assertTrue($form->isValid());
$this->assertSame('encoded_password', $user->getPassword());
}

public function testSecurityPasswordTypeDetectsUserObject()
{
$user = new User('email@example.com', 'previous_password');

$this->assertSame('previous_password', $user->getPassword());

$form = $this->getFormFactory()->create(PasswordResetType::class, $user);
$form->submit(['password' => 'new_password']);

$this->assertTrue($form->isSubmitted());
$this->assertTrue($form->isValid());
$this->assertSame('encoded_password', $user->getPassword());
}

public function testSecurityPasswordTypeDoesNotUpdatePasswordWhenEmpty()
{
$user = new User('email@example.com', 'previous_password');

$this->assertSame('previous_password', $user->getPassword());

$form = $this->getFormFactory()->create(PasswordResetType::class, $user);
$form->submit([]);

$this->assertTrue($form->isSubmitted());
$this->assertTrue($form->isValid());
$this->assertSame('previous_password', $user->getPassword());
}

private function getFormFactory()
{
$userPasswordEncoder = $this->getMockBuilder('Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface')->getMock();

$userPasswordEncoder
->method('encodePassword')
->willReturn('encoded_password');

$formFactoryBuilder = new FormFactoryBuilder();
$formFactoryBuilder->addExtension(new SecurityExtension($userPasswordEncoder));

return $formFactoryBuilder->getFormFactory();
}
}

class PasswordResetType extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder
->add('password', SecurityPasswordType::class)
;
}

public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('data_class', User::class);
}
}