Avoid simple passwords (e.g. email as password) (#1439)

* add a simple test for the RegistrationController

* add a test to verify that simple passwords shouldnt be used

* dont allow to chose the same password as username, email or their canoncial variants on registration

* add a test for changing the password with prohibited password

* check for prohibited passwords on password change

* check for prohibited passwords on password reset change

src/Controller/ResetPasswordController.php

@@ -99,7 +99,7 @@ public function reset(Request $request, UserPasswordHasherInterface $passwordHas
  }
 
  // The token is valid; allow the user to change their password.
- $form = $this->createForm(ResetPasswordFormType::class, null, ['user' => $user]);
+ $form = $this->createForm(ResetPasswordFormType::class, $user);
  $form->handleRequest($request);
 
  if ($form->isSubmitted() && $form->isValid()) {

src/Form/ChangePasswordFormType.php

@@ -14,6 +14,7 @@
 
 use App\Entity\User;
 use App\Form\Type\InvisibleRecaptchaType;
+use App\Validator\NotProhibitedPassword;
 use App\Validator\Password;
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Core\Type\PasswordType;
@@ -55,6 +56,7 @@ public function configureOptions(OptionsResolver $resolver): void
  {
  $resolver->setDefaults([
  'data_class' => User::class,
+ 'constraints' => [new NotProhibitedPassword()],
  ]);
  }
 }

src/Form/RegistrationFormType.php

@@ -13,6 +13,7 @@
 namespace App\Form;
 
 use App\Entity\User;
+use App\Validator\NotProhibitedPassword;
 use App\Validator\Password;
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
@@ -53,6 +54,7 @@ public function configureOptions(OptionsResolver $resolver): void
  $resolver->setDefaults([
  'data_class' => User::class,
  'validation_groups' => ['Default', 'Registration'],
+ 'constraints' => [new NotProhibitedPassword()],
  ]);
  }
 }

src/Form/ResetPasswordFormType.php

@@ -13,6 +13,7 @@
 namespace App\Form;
 
 use App\Entity\User;
+use App\Validator\NotProhibitedPassword;
 use App\Validator\Password;
 use App\Validator\RateLimitingRecaptcha;
 use App\Validator\TwoFactorCode;
@@ -26,10 +27,10 @@ class ResetPasswordFormType extends AbstractType
 {
  public function configureOptions(OptionsResolver $resolver): void
  {
- $resolver
- ->define('user')
- ->allowedTypes(User::class)
-  ->required();
+ $resolver->setDefaults([
+ 'data_class' => User::class,
+ 'constraints' => [new NotProhibitedPassword()],
+ ]);
  }
 
  public function buildForm(FormBuilderInterface $builder, array $options): void
@@ -46,15 +47,15 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
  ])
  ;
 
- if ($options['user']->isTotpAuthenticationEnabled()) {
+ if ($options['data']->isTotpAuthenticationEnabled()) {
  $builder
  ->add('twoFactorCode', TextType::class, [
  'label' => 'Two-Factor Code',
  'required' => true,
  'mapped' => false,
  'constraints' => [
  new RateLimitingRecaptcha(),
- new TwoFactorCode($options['user']),
+ new TwoFactorCode($options['data']),
  ],
  ]);
  }

src/Validator/NotProhibitedPassword.php

@@ -0,0 +1,27 @@
+<?php declare(strict_types=1);
+
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <[email protected]>
+ * Nils Adermann <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Validator;
+
+use Attribute;
+use Symfony\Component\Validator\Constraint;
+
+#[Attribute(Attribute::TARGET_CLASS)]
+class NotProhibitedPassword extends Constraint
+{
+ public string $message = 'Password should not match your email or any of your names.';
+
+ public function getTargets(): string
+ {
+ return self::CLASS_CONSTRAINT;
+ }
+}

src/Validator/NotProhibitedPasswordValidator.php

@@ -0,0 +1,60 @@
+<?php declare(strict_types=1);
+
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <[email protected]>
+ * Nils Adermann <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Validator;
+
+use App\Entity\User;
+use Symfony\Component\Form\Form;
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+use Symfony\Component\Validator\Exception\UnexpectedTypeException;
+
+class NotProhibitedPasswordValidator extends ConstraintValidator
+{
+ public function validate(mixed $value, Constraint $constraint): void
+ {
+ if (!$constraint instanceof NotProhibitedPassword) {
+ throw new UnexpectedTypeException($constraint, NotProhibitedPassword::class);
+ }
+
+ if (!$value instanceof User) {
+ throw new UnexpectedTypeException($value, User::class);
+ }
+
+ $form = $this->context->getRoot();
+ if (!$form instanceof Form) {
+ throw new UnexpectedTypeException($form, Form::class);
+ }
+
+ $user = $value;
+ $password = $form->get('plainPassword')->getData();
+
+ $prohibitedPasswords = [
+ $user->getEmail(),
+ $user->getEmailCanonical(),
+ $user->getUsername(),
+ $user->getUsernameCanonical(),
+ ];
+
+ foreach ($prohibitedPasswords as $prohibitedPassword) {
+ if ($password === $prohibitedPassword) {
+ $this->context
+ ->buildViolation($constraint->message)
+ ->atPath('plainPassword')
+ ->addViolation();
+
+ return;
+ }
+ }
+
+ }
+}

tests/Controller/ChangePasswordControllerTest.php

@@ -0,0 +1,99 @@
+<?php declare(strict_types=1);
+
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <[email protected]>
+ * Nils Adermann <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Controller;
+
+use App\Entity\User;
+use App\Validator\NotProhibitedPassword;
+use Doctrine\DBAL\Connection;
+use Doctrine\Persistence\ManagerRegistry;
+use PHPUnit\Framework\Attributes\TestWith;
+use Symfony\Bundle\FrameworkBundle\KernelBrowser;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\DomCrawler\Crawler;
+use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface;
+
+class ChangePasswordControllerTest extends WebTestCase
+{
+ private KernelBrowser $client;
+
+ public function setUp(): void
+ {
+ $this->client = self::createClient();
+ $this->client->disableReboot(); // Prevent reboot between requests
+ static::getContainer()->get(Connection::class)->beginTransaction();
+
+ parent::setUp();
+ }
+
+ public function tearDown(): void
+ {
+ static::getContainer()->get(Connection::class)->rollBack();
+
+ parent::tearDown();
+ }
+
+ #[TestWith(['SuperSecret123', 'ok'])]
+ #[TestWith(['[email protected]', 'prohibited-password-error'])]
+ public function testChangePassword(string $newPassword, string $expectedResult): void
+ {
+ $user = new User;
+ $user->setEnabled(true);
+ $user->setUsername('test');
+ $user->setEmail('[email protected]');
+ $user->setPassword('testtest');
+ $user->setApiToken('token');
+ $user->setGithubId('123456');
+
+ $currentPassword = 'current-one-123';
+ $currentPasswordHash = self::getContainer()->get(UserPasswordHasherInterface::class)->hashPassword($user, $currentPassword);
+ $user->setPassword($currentPasswordHash);
+
+ $em = static::getContainer()->get(ManagerRegistry::class)->getManager();
+ $em->persist($user);
+ $em->flush();
+
+ $this->client->loginUser($user);
+
+ $crawler = $this->client->request('GET', '/profile/change-password');
+
+ $form = $crawler->selectButton('Change password')->form();
+ $crawler = $this->client->submit($form, [
+ 'change_password_form[current_password]' => $currentPassword,
+ 'change_password_form[plainPassword]' => $newPassword,
+ ]);
+
+ if ($expectedResult == 'ok') {
+ $this->assertResponseStatusCodeSame(302);
+
+ $em->clear();
+ $user = $em->getRepository(User::class)->find($user->getId());
+ $this->assertNotNull($user);
+ $this->assertNotSame($currentPasswordHash, $user->getPassword());
+ }
+
+ if ($expectedResult === 'prohibited-password-error') {
+ $this->assertResponseStatusCodeSame(422);
+ $this->assertFormError((new NotProhibitedPassword)->message, $crawler);
+ }
+ }
+
+ private function assertFormError(string $message, Crawler $crawler): void
+ {
+ $formCrawler = $crawler->filter('[name="change_password_form"]');
+ $this->assertCount(
+ 1,
+ $formCrawler->filter('.alert-danger:contains("' . $message . '")'),
+ $formCrawler->html()."\nShould find an .alert-danger within the form with the message: '$message'",
+ );
+ }
+}

tests/Controller/ProfileControllerTest.php

@@ -1,5 +1,15 @@
 <?php declare(strict_types=1);
 
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <[email protected]>
+ * Nils Adermann <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
 namespace App\Tests\Controller;
 
 use App\Entity\User;
@@ -21,6 +31,13 @@ public function setUp(): void
  parent::setUp();
  }
 
+ public function tearDown(): void
+ {
+ static::getContainer()->get(Connection::class)->rollBack();
+
+ parent::tearDown();
+ }
+
  public function testEditProfile(): void
  {
  $user = new User;

tests/Controller/RegistrationControllerTest.php

@@ -0,0 +1,95 @@
+<?php declare(strict_types=1);
+
+/*
+ * This file is part of Packagist.
+ *
+ * (c) Jordi Boggiano <[email protected]>
+ * Nils Adermann <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace App\Tests\Controller;
+
+use App\Entity\User;
+use App\Validator\NotProhibitedPassword;
+use Doctrine\DBAL\Connection;
+use Doctrine\Persistence\ManagerRegistry;
+use PHPUnit\Framework\Attributes\TestWith;
+use Symfony\Bundle\FrameworkBundle\KernelBrowser;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\DomCrawler\Crawler;
+
+class RegistrationControllerTest extends WebTestCase
+{
+ private KernelBrowser $client;
+
+ public function setUp(): void
+ {
+ $this->client = self::createClient();
+ $this->client->disableReboot(); // Prevent reboot between requests
+ static::getContainer()->get(Connection::class)->beginTransaction();
+
+ parent::setUp();
+ }
+
+ public function tearDown(): void
+ {
+ static::getContainer()->get(Connection::class)->rollBack();
+
+ parent::tearDown();
+ }
+
+ public function testRegisterWithoutOAuth(): void
+ {
+ $crawler = $this->client->request('GET', '/register/');
+ $this->assertResponseStatusCodeSame(200);
+
+ $form = $crawler->filter('[name="registration_form"]')->form();
+ $form->setValues([
+ 'registration_form[email]' => '[email protected]',
+ 'registration_form[username]' => 'max.example',
+ 'registration_form[plainPassword]' => 'SuperSecret123',
+ ]);
+
+ $this->client->submit($form);
+ $this->assertResponseStatusCodeSame(302);
+
+ $em = static::getContainer()->get(ManagerRegistry::class)->getManager();
+ $user = $em->getRepository(User::class)->findOneBy(['username' => 'max.example']);
+ $this->assertInstanceOf(User::class, $user);
+ $this->assertSame('[email protected]', $user->getEmailCanonical(), "user email should have been canonicalized");
+ }
+
+ #[TestWith(['max.example'])]
+ #[TestWith(['[email protected]'])]
+ #[TestWith(['[email protected]'])]
+ public function testRegisterWithTooSimplePasswords(string $password): void
+ {
+ $crawler = $this->client->request('GET', '/register/');
+ $this->assertResponseStatusCodeSame(200);
+
+ $form = $crawler->filter('[name="registration_form"]')->form();
+ $form->setValues([
+ 'registration_form[email]' => '[email protected]',
+ 'registration_form[username]' => 'max.example',
+ 'registration_form[plainPassword]' => $password,
+ ]);
+
+ $crawler = $this->client->submit($form);
+ $this->assertResponseStatusCodeSame(422, 'Should be invalid because password is the same as email or username');
+
+ $this->assertFormError((new NotProhibitedPassword)->message, $crawler);
+ }
+
+ private function assertFormError(string $message, Crawler $crawler): void
+ {
+ $formCrawler = $crawler->filter('[name="registration_form"]');
+ $this->assertCount(
+ 1,
+ $formCrawler->filter('.alert-danger:contains("' . $message . '")'),
+ $formCrawler->html()."\nShould find an .alert-danger within the form with the message: '$message'",
+ );
+ }
+}