Skip to content

Conversation

Seb33300
Copy link
Contributor

@Seb33300 Seb33300 commented Feb 21, 2023

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

When using the new hash_property_path option to hash password submitted by forms, we should skip hashing if the submitted password is empty.

  • Because empty passwords are not allowed and saving an empty password hash will prevent the user to login his account:

    $presentedPassword = $badge->getPassword();
    if ('' === $presentedPassword) {
    throw new BadCredentialsException('The presented password cannot be empty.');
    }

  • Because a common use case when creating a user profile form is to ignore the "new password" input if it's left blank.


$form = $this->factory
->createBuilder('Symfony\Component\Form\Extension\Core\Type\FormType', $user)
->add('plainPassword', 'Symfony\Component\Form\Extension\Core\Type\PasswordType', [
Copy link
Member

Choose a reason for hiding this comment

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

let's use ::class even if some other test cases don't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -37,6 +37,10 @@ public function __construct(

public function registerPassword(FormEvent $event)
{
if ('' === $event->getData()) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we account for null too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@nicolas-grekas
Copy link
Member

Thank you @Seb33300.

@nicolas-grekas nicolas-grekas merged commit 49092e3 into symfony:6.2 Feb 21, 2023
@fabpot fabpot mentioned this pull request Feb 28, 2023
@Seb33300 Seb33300 deleted the form-empty-password branch September 27, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants