From 033b430833e2e2987e6fec195f2bc859258d6727 Mon Sep 17 00:00:00 2001 From: Philipp Scheit Date: Tue, 19 Mar 2024 16:10:10 +0100 Subject: [PATCH] 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 | 2 +- src/Form/ChangePasswordFormType.php | 2 + src/Form/RegistrationFormType.php | 2 + src/Form/ResetPasswordFormType.php | 13 +-- src/Validator/NotProhibitedPassword.php | 27 +++++ .../NotProhibitedPasswordValidator.php | 60 +++++++++++ .../ChangePasswordControllerTest.php | 99 +++++++++++++++++++ tests/Controller/ProfileControllerTest.php | 17 ++++ .../Controller/RegistrationControllerTest.php | 95 ++++++++++++++++++ .../ResetPasswordControllerTest.php | 22 +++++ 10 files changed, 332 insertions(+), 7 deletions(-) create mode 100644 src/Validator/NotProhibitedPassword.php create mode 100644 src/Validator/NotProhibitedPasswordValidator.php create mode 100644 tests/Controller/ChangePasswordControllerTest.php create mode 100644 tests/Controller/RegistrationControllerTest.php diff --git a/src/Controller/ResetPasswordController.php b/src/Controller/ResetPasswordController.php index f82eb699b..0448e3aea 100644 --- a/src/Controller/ResetPasswordController.php +++ b/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()) { diff --git a/src/Form/ChangePasswordFormType.php b/src/Form/ChangePasswordFormType.php index c9116bcff..61181d18b 100644 --- a/src/Form/ChangePasswordFormType.php +++ b/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()], ]); } } diff --git a/src/Form/RegistrationFormType.php b/src/Form/RegistrationFormType.php index a4945a36a..4600dc098 100644 --- a/src/Form/RegistrationFormType.php +++ b/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()], ]); } } diff --git a/src/Form/ResetPasswordFormType.php b/src/Form/ResetPasswordFormType.php index 83e8eb258..e9bcce8e4 100644 --- a/src/Form/ResetPasswordFormType.php +++ b/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,7 +47,7 @@ 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', @@ -54,7 +55,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'mapped' => false, 'constraints' => [ new RateLimitingRecaptcha(), - new TwoFactorCode($options['user']), + new TwoFactorCode($options['data']), ], ]); } diff --git a/src/Validator/NotProhibitedPassword.php b/src/Validator/NotProhibitedPassword.php new file mode 100644 index 000000000..e3d56f0ad --- /dev/null +++ b/src/Validator/NotProhibitedPassword.php @@ -0,0 +1,27 @@ + + * Nils Adermann + * + * 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; + } +} diff --git a/src/Validator/NotProhibitedPasswordValidator.php b/src/Validator/NotProhibitedPasswordValidator.php new file mode 100644 index 000000000..96d39bb7b --- /dev/null +++ b/src/Validator/NotProhibitedPasswordValidator.php @@ -0,0 +1,60 @@ + + * Nils Adermann + * + * 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; + } + } + + } +} diff --git a/tests/Controller/ChangePasswordControllerTest.php b/tests/Controller/ChangePasswordControllerTest.php new file mode 100644 index 000000000..7e1203011 --- /dev/null +++ b/tests/Controller/ChangePasswordControllerTest.php @@ -0,0 +1,99 @@ + + * Nils Adermann + * + * 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(['test@example.org', 'prohibited-password-error'])] + public function testChangePassword(string $newPassword, string $expectedResult): void + { + $user = new User; + $user->setEnabled(true); + $user->setUsername('test'); + $user->setEmail('test@example.org'); + $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'", + ); + } +} diff --git a/tests/Controller/ProfileControllerTest.php b/tests/Controller/ProfileControllerTest.php index 9bccedf40..1d9a98333 100644 --- a/tests/Controller/ProfileControllerTest.php +++ b/tests/Controller/ProfileControllerTest.php @@ -1,5 +1,15 @@ + * Nils Adermann + * + * 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; diff --git a/tests/Controller/RegistrationControllerTest.php b/tests/Controller/RegistrationControllerTest.php new file mode 100644 index 000000000..341f5c932 --- /dev/null +++ b/tests/Controller/RegistrationControllerTest.php @@ -0,0 +1,95 @@ + + * Nils Adermann + * + * 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]' => 'Max@Example.com', + '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('max@example.com', $user->getEmailCanonical(), "user email should have been canonicalized"); + } + + #[TestWith(['max.example'])] + #[TestWith(['max@example.com'])] + #[TestWith(['Max@Example.com'])] + 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]' => 'Max@Example.com', + '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'", + ); + } +} diff --git a/tests/Controller/ResetPasswordControllerTest.php b/tests/Controller/ResetPasswordControllerTest.php index daec9416e..c8f2ca3ed 100644 --- a/tests/Controller/ResetPasswordControllerTest.php +++ b/tests/Controller/ResetPasswordControllerTest.php @@ -69,6 +69,18 @@ public function testResetPasswordWithTwoFactor(): void $this->assertTrue(self::getContainer()->get(TokenStorageInterface::class)->getToken()?->getAttribute(TwoFactorAuthenticator::FLAG_2FA_COMPLETE)); } + public function testResetPasswordToProhibited(): void + { + $user = $this->setupUserWithPasswordResetRequest(false); + $oldPassword = $user->getPassword(); + + $crawler = $this->client->request('GET', '/reset-password/reset/' . $user->getConfirmationToken()); + $this->assertResponseStatusCodeSame(200); + + $this->submitPasswordResetFormAndAsserStatusCode($crawler, newPassword: $user->getEmail(), expectedStatusCode: 422); + $this->assertUserHasUnchangedPassword($user, $oldPassword); + } + private function setupUserWithPasswordResetRequest(bool $withTwoFactor): User { $user = new User; @@ -112,4 +124,14 @@ private function assertUserHasNewPassword(User $user, ?string $oldPassword): voi $this->assertNotNull($user); $this->assertNotSame($oldPassword, $user->getPassword()); } + + private function assertUserHasUnchangedPassword(User $user, ?string $oldPassword): void + { + $em = static::getContainer()->get(ManagerRegistry::class)->getManager(); + $em->clear(); + + $user = $em->getRepository(User::class)->find($user->getId()); + $this->assertNotNull($user); + $this->assertSame($oldPassword, $user->getPassword()); + } }