From 4e70cd5cb0786c5082252559dc5a9ac8f8551367 Mon Sep 17 00:00:00 2001 From: Stephan Vock Date: Fri, 17 Nov 2023 15:33:35 +0000 Subject: [PATCH] Two-Factor: password reset for a user with 2FA enabled should ask for the 2FA code --- src/Controller/ResetPasswordController.php | 7 +- ...solvedTwoFactorCodeCredentialsListener.php | 29 +++++ src/Form/ResetPasswordFormType.php | 23 ++++ .../ResolvedTwoFactorCodeCredentials.php | 23 ++++ templates/reset_password/reset.html.twig | 3 + .../ResetPasswordControllerTest.php | 115 ++++++++++++++++++ 6 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 src/EventListener/ResolvedTwoFactorCodeCredentialsListener.php create mode 100644 src/Security/Passport/Badge/ResolvedTwoFactorCodeCredentials.php create mode 100644 tests/Controller/ResetPasswordControllerTest.php diff --git a/src/Controller/ResetPasswordController.php b/src/Controller/ResetPasswordController.php index 095adc843..32b214756 100644 --- a/src/Controller/ResetPasswordController.php +++ b/src/Controller/ResetPasswordController.php @@ -16,6 +16,7 @@ use App\Form\ResetPasswordFormType; use App\Form\ResetPasswordRequestFormType; use App\Security\BruteForceLoginFormAuthenticator; +use App\Security\Passport\Badge\ResolvedTwoFactorCodeCredentials; use App\Security\UserChecker; use Beelab\Recaptcha2Bundle\Recaptcha\RecaptchaException; use Beelab\Recaptcha2Bundle\Recaptcha\RecaptchaVerifier; @@ -98,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); + $form = $this->createForm(ResetPasswordFormType::class, null, ['user' => $user]); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { @@ -124,7 +125,9 @@ public function reset(Request $request, UserPasswordHasherInterface $passwordHas // skip authenticating if any pre-auth check does not pass } - if ($response = $userAuthenticator->authenticateUser($user, $authenticator, $request)) { + // A user resetting the password with 2FA enabled, should automatically be marked as 2FA complete + $badges = $user->isTotpAuthenticationEnabled() ? [new ResolvedTwoFactorCodeCredentials()] : []; + if ($response = $userAuthenticator->authenticateUser($user, $authenticator, $request, $badges)) { return $response; } diff --git a/src/EventListener/ResolvedTwoFactorCodeCredentialsListener.php b/src/EventListener/ResolvedTwoFactorCodeCredentialsListener.php new file mode 100644 index 000000000..df23d879f --- /dev/null +++ b/src/EventListener/ResolvedTwoFactorCodeCredentialsListener.php @@ -0,0 +1,29 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\EventListener; + +use App\Security\Passport\Badge\ResolvedTwoFactorCodeCredentials; +use Scheb\TwoFactorBundle\Security\Http\Authenticator\TwoFactorAuthenticator; +use Symfony\Component\EventDispatcher\Attribute\AsEventListener; +use Symfony\Component\Security\Http\Event\AuthenticationTokenCreatedEvent; + +#[AsEventListener(event: AuthenticationTokenCreatedEvent::class, method: 'onAuthenticationTokenCreated', priority: 512)] +class ResolvedTwoFactorCodeCredentialsListener +{ + public function onAuthenticationTokenCreated(AuthenticationTokenCreatedEvent $event): void + { + if ($event->getPassport()->getBadge(ResolvedTwoFactorCodeCredentials::class)) { + $event->getAuthenticatedToken()->setAttribute(TwoFactorAuthenticator::FLAG_2FA_COMPLETE, true); + } + } +} diff --git a/src/Form/ResetPasswordFormType.php b/src/Form/ResetPasswordFormType.php index 579e9b851..77f9b5816 100644 --- a/src/Form/ResetPasswordFormType.php +++ b/src/Form/ResetPasswordFormType.php @@ -12,13 +12,24 @@ namespace App\Form; +use App\Entity\User; +use App\Form\Validation\TwoFactorCodeConstraint; use App\Validator\Password; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\Extension\Core\Type\PasswordType; +use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\OptionsResolver\OptionsResolver; class ResetPasswordFormType extends AbstractType { + public function configureOptions(OptionsResolver $resolver): void + { + $resolver + ->setDefault('user', null) + ->setAllowedTypes('user', User::class); + } + public function buildForm(FormBuilderInterface $builder, array $options): void { $builder @@ -32,5 +43,17 @@ public function buildForm(FormBuilderInterface $builder, array $options): void ], ]) ; + + if ($options['user']->isTotpAuthenticationEnabled()) { + $builder + ->add('twoFactorCode', TextType::class, [ + 'label' => 'Two-Factor Code', + 'required' => true, + 'mapped' => false, + 'constraints' => [ + new TwoFactorCodeConstraint($options['user']), + ], + ]); + } } } diff --git a/src/Security/Passport/Badge/ResolvedTwoFactorCodeCredentials.php b/src/Security/Passport/Badge/ResolvedTwoFactorCodeCredentials.php new file mode 100644 index 000000000..51cba6f0c --- /dev/null +++ b/src/Security/Passport/Badge/ResolvedTwoFactorCodeCredentials.php @@ -0,0 +1,23 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Security\Passport\Badge; + +use Symfony\Component\Security\Http\Authenticator\Passport\Badge\BadgeInterface; + +class ResolvedTwoFactorCodeCredentials implements BadgeInterface +{ + public function isResolved(): bool + { + return true; + } +} diff --git a/templates/reset_password/reset.html.twig b/templates/reset_password/reset.html.twig index ab3b9cc58..ede4d6083 100644 --- a/templates/reset_password/reset.html.twig +++ b/templates/reset_password/reset.html.twig @@ -7,6 +7,9 @@ {{ form_start(resetForm) }} {{ form_row(resetForm.plainPassword) }} + {% if resetForm.twoFactorCode is defined %} + {{ form_row(resetForm.twoFactorCode) }} + {% endif %} {{ form_end(resetForm) }} {% endblock %} diff --git a/tests/Controller/ResetPasswordControllerTest.php b/tests/Controller/ResetPasswordControllerTest.php new file mode 100644 index 000000000..daec9416e --- /dev/null +++ b/tests/Controller/ResetPasswordControllerTest.php @@ -0,0 +1,115 @@ + + * 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\Tests\Mock\TotpAuthenticatorStub; +use Doctrine\DBAL\Connection; +use Doctrine\Persistence\ManagerRegistry; +use Scheb\TwoFactorBundle\Security\Http\Authenticator\TwoFactorAuthenticator; +use Symfony\Bundle\FrameworkBundle\KernelBrowser; +use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; +use Symfony\Component\DomCrawler\Crawler; +use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; + +class ResetPasswordControllerTest 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 testResetPassword(): void + { + $user = $this->setupUserWithPasswordResetRequest(false); + $oldPassword = $user->getPassword(); + + $crawler = $this->client->request('GET', '/reset-password/reset/' . $user->getConfirmationToken()); + $this->assertResponseStatusCodeSame(200); + + $this->submitPasswordResetFormAndAsserStatusCode($crawler, 'new-password', 302); + $this->assertUserHasNewPassword($user, $oldPassword); + } + + public function testResetPasswordWithTwoFactor(): void + { + $user = $this->setupUserWithPasswordResetRequest(true); + $oldPassword = $user->getPassword(); + + $crawler = $this->client->request('GET', '/reset-password/reset/' . $user->getConfirmationToken()); + $this->assertResponseStatusCodeSame(200); + + $this->submitPasswordResetFormAndAsserStatusCode($crawler, 'new-password', 422); + $this->submitPasswordResetFormAndAsserStatusCode($crawler, 'new-password', 302, TotpAuthenticatorStub::MOCKED_VALID_CODE); + $this->assertUserHasNewPassword($user, $oldPassword); + + $this->assertTrue(self::getContainer()->get(TokenStorageInterface::class)->getToken()?->getAttribute(TwoFactorAuthenticator::FLAG_2FA_COMPLETE)); + } + + private function setupUserWithPasswordResetRequest(bool $withTwoFactor): User + { + $user = new User; + $user->setEnabled(true); + $user->setUsername('test'); + $user->setEmail('test@example.org'); + $user->setPassword('testtest'); + $user->setApiToken('token'); + $user->initializeConfirmationToken(); + $user->setPasswordRequestedAt(new \DateTime()); + + if ($withTwoFactor) { + $user->setTotpSecret('secret'); + } + + $em = static::getContainer()->get(ManagerRegistry::class)->getManager(); + $em->persist($user); + $em->flush(); + + return $user; + } + + private function submitPasswordResetFormAndAsserStatusCode(Crawler $crawler, string $newPassword, int $expectedStatusCode, ?string $mfaCode = null): void + { + $form = $crawler->selectButton('Reset password')->form(); + $form->setValues(array_filter([ + 'reset_password_form[plainPassword]' => $newPassword, + 'reset_password_form[twoFactorCode]' => $mfaCode, + ])); + + $this->client->submit($form); + $this->assertResponseStatusCodeSame($expectedStatusCode); + } + + private function assertUserHasNewPassword(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->assertNotSame($oldPassword, $user->getPassword()); + } +}