From c8e3a669acabd42021643f6c1f3f6ffd4d09eb0f Mon Sep 17 00:00:00 2001 From: Stephan Vock Date: Fri, 17 Nov 2023 13:22:57 +0000 Subject: [PATCH] Two-Factor: extract code validation into constraint and add test to enable two-factor --- .env.test | 1 + config/services_test.yaml | 6 ++ src/Controller/UserController.php | 35 +++------ src/Form/Type/EnableTwoFactorAuthType.php | 15 +++- .../Validation/TwoFactorCodeConstraint.php | 26 +++++++ .../TwoFactorCodeConstraintValidator.php | 34 ++++++++ tests/Controller/UserControllerTest.php | 77 +++++++++++++++++++ tests/Mock/TotpAuthenticatorStub.php | 42 ++++++++++ 8 files changed, 209 insertions(+), 27 deletions(-) create mode 100644 src/Form/Validation/TwoFactorCodeConstraint.php create mode 100644 src/Form/Validation/TwoFactorCodeConstraintValidator.php create mode 100644 tests/Controller/UserControllerTest.php create mode 100644 tests/Mock/TotpAuthenticatorStub.php diff --git a/.env.test b/.env.test index 9497b948e..66778117e 100644 --- a/.env.test +++ b/.env.test @@ -6,3 +6,4 @@ PANTHER_APP_ENV=panther DATABASE_URL="mysql://root@127.0.0.1:3306/packagist?serverVersion=8.0.28" MAILER_DSN=null://null REDIS_URL=redis://localhost/14 +APP_MAILER_FROM_EMAIL=packagist@example.org diff --git a/config/services_test.yaml b/config/services_test.yaml index f7254eeee..75538b822 100644 --- a/config/services_test.yaml +++ b/config/services_test.yaml @@ -13,3 +13,9 @@ services: Algolia\AlgoliaSearch\SearchClient: public: true factory: ['Algolia\AlgoliaSearch\SearchClient', create] + + # stub to replace 2FA code generation + App\Tests\Mock\TotpAuthenticatorStub: + arguments: + $totpFactory: '@scheb_two_factor.security.totp_factory' + Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpAuthenticatorInterface: '@App\Tests\Mock\TotpAuthenticatorStub' diff --git a/src/Controller/UserController.php b/src/Controller/UserController.php index 1e376def7..f2cca764a 100644 --- a/src/Controller/UserController.php +++ b/src/Controller/UserController.php @@ -265,39 +265,28 @@ public function enableTwoFactorAuthAction(Request $req, #[VarName('name')] User throw $this->createAccessDeniedException('You cannot change this user\'s two-factor authentication settings'); } - $enableRequest = new EnableTwoFactorRequest(); - $form = $this->createForm(EnableTwoFactorAuthType::class, $enableRequest) - ->handleRequest($req); - - $secret = (string) $req->getSession()->get('2fa_secret'); - if (!$form->isSubmitted() || '' === $secret) { - $secret = $authenticator->generateSecret(); - $req->getSession()->set('2fa_secret', $secret); - } - + $secret = (string) $req->getSession()->get('2fa_secret', $authenticator->generateSecret()); // Temporarily store this code on the user, as we'll need it there to generate the // QR code and to check the confirmation code. We won't actually save this change // until we've confirmed the code $user->setTotpSecret($secret); - if ($form->isSubmitted()) { - // Validate the code using the secret that was submitted in the form - if (!$authenticator->checkCode($user, $enableRequest->getCode() ?? '')) { - $form->get('code')->addError(new FormError('Invalid authenticator code')); - } + $enableRequest = new EnableTwoFactorRequest(); + $form = $this->createForm(EnableTwoFactorAuthType::class, $enableRequest, ['user' => $user]) + ->handleRequest($req); - if ($form->isValid()) { - $req->getSession()->remove('2fa_secret'); - $authManager->enableTwoFactorAuth($user, $secret); - $backupCode = $authManager->generateAndSaveNewBackupCode($user); + if ($form->isSubmitted() && $form->isValid()) { + $req->getSession()->remove('2fa_secret'); + $authManager->enableTwoFactorAuth($user, $secret); + $backupCode = $authManager->generateAndSaveNewBackupCode($user); - $this->addFlash('success', 'Two-factor authentication has been enabled.'); - $req->getSession()->set('backup_code', $backupCode); + $this->addFlash('success', 'Two-factor authentication has been enabled.'); + $req->getSession()->set('backup_code', $backupCode); - return $this->redirectToRoute('user_2fa_confirm', ['name' => $user->getUsername()]); - } + return $this->redirectToRoute('user_2fa_confirm', ['name' => $user->getUsername()]); } + $req->getSession()->set('2fa_secret', $secret); $qrContent = $authenticator->getQRContent($user); $qrCode = Builder::create() diff --git a/src/Form/Type/EnableTwoFactorAuthType.php b/src/Form/Type/EnableTwoFactorAuthType.php index ce4aa638d..360ae4042 100644 --- a/src/Form/Type/EnableTwoFactorAuthType.php +++ b/src/Form/Type/EnableTwoFactorAuthType.php @@ -12,7 +12,9 @@ namespace App\Form\Type; +use App\Entity\User; use App\Form\Model\EnableTwoFactorRequest; +use App\Form\Validation\TwoFactorCodeConstraint; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; @@ -25,13 +27,18 @@ class EnableTwoFactorAuthType extends AbstractType { public function buildForm(FormBuilderInterface $builder, array $options): void { - $builder->add('code', TextType::class); + $builder->add('code', TextType::class, [ + 'constraints' => [new TwoFactorCodeConstraint($options['user'])] + ]); } public function configureOptions(OptionsResolver $resolver): void { - $resolver->setDefaults([ - 'data_class' => EnableTwoFactorRequest::class, - ]); + $resolver + ->setDefaults([ + 'data_class' => EnableTwoFactorRequest::class, + 'user' => null, + ]) + ->setAllowedTypes('user', User::class); } } diff --git a/src/Form/Validation/TwoFactorCodeConstraint.php b/src/Form/Validation/TwoFactorCodeConstraint.php new file mode 100644 index 000000000..1e7b1dfca --- /dev/null +++ b/src/Form/Validation/TwoFactorCodeConstraint.php @@ -0,0 +1,26 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Form\Validation; + +use App\Entity\User; +use Symfony\Component\Validator\Constraint; + +class TwoFactorCodeConstraint extends Constraint +{ + public function __construct( + public readonly User $user, + public readonly string $message = 'Invalid authenticator code', + ) { + parent::__construct(); + } +} diff --git a/src/Form/Validation/TwoFactorCodeConstraintValidator.php b/src/Form/Validation/TwoFactorCodeConstraintValidator.php new file mode 100644 index 000000000..8e5d67499 --- /dev/null +++ b/src/Form/Validation/TwoFactorCodeConstraintValidator.php @@ -0,0 +1,34 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Form\Validation; + +use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpAuthenticatorInterface; +use Symfony\Component\Validator\Constraint; +use Symfony\Component\Validator\ConstraintValidator; + +class TwoFactorCodeConstraintValidator extends ConstraintValidator +{ + public function __construct( + private readonly TotpAuthenticatorInterface $totpAuthenticator, + ) {} + + /** + * @param TwoFactorCodeConstraint $constraint + */ + public function validate(mixed $value, Constraint $constraint): void + { + if (!$this->totpAuthenticator->checkCode($constraint->user, (string) $value)) { + $this->context->addViolation($constraint->message); + } + } +} diff --git a/tests/Controller/UserControllerTest.php b/tests/Controller/UserControllerTest.php new file mode 100644 index 000000000..b85396cd4 --- /dev/null +++ b/tests/Controller/UserControllerTest.php @@ -0,0 +1,77 @@ + + * 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 Symfony\Bundle\FrameworkBundle\KernelBrowser; +use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; + +class UserControllerTest 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 testEnableTwoFactoCode(): void + { + $user = new User; + $user->setEnabled(true); + $user->setUsername('test'); + $user->setEmail('test@example.org'); + $user->setPassword('testtest'); + $user->setApiToken('token'); + + $em = static::getContainer()->get(ManagerRegistry::class)->getManager(); + $em->persist($user); + $em->flush(); + + $this->client->loginUser($user); + + $crawler = $this->client->request('GET', sprintf('/users/%s/2fa/enable', $user->getUsername())); + $form = $crawler->selectButton('Enable Two-Factor Authentication')->form(); + $form->setValues([ + 'enable_two_factor_auth[code]' => 123456, + ]); + + $crawler = $this->client->submit($form); + $this->assertResponseStatusCodeSame(422); + + $form = $crawler->selectButton('Enable Two-Factor Authentication')->form(); + $form->setValues([ + 'enable_two_factor_auth[code]' => TotpAuthenticatorStub::MOCKED_VALID_CODE, + ]); + + $this->client->submit($form); + $this->assertResponseStatusCodeSame(302); + + $em->clear(); + $this->assertTrue($em->getRepository(User::class)->find($user->getId())->isTotpAuthenticationEnabled()); + } +} diff --git a/tests/Mock/TotpAuthenticatorStub.php b/tests/Mock/TotpAuthenticatorStub.php new file mode 100644 index 000000000..d402d32bf --- /dev/null +++ b/tests/Mock/TotpAuthenticatorStub.php @@ -0,0 +1,42 @@ + + * Nils Adermann + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace App\Tests\Mock; + +use Scheb\TwoFactorBundle\Model\Totp\TwoFactorInterface; +use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpAuthenticatorInterface; +use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpFactory; +use ParagonIE\ConstantTime\Base32; + +class TotpAuthenticatorStub implements TotpAuthenticatorInterface +{ + public const MOCKED_VALID_CODE = '999999'; + + public function __construct( + private readonly TotpFactory $totpFactory, + ) {} + + public function checkCode(TwoFactorInterface $user, string $code): bool + { + return $code === self::MOCKED_VALID_CODE; + } + + public function getQRContent(TwoFactorInterface $user): string + { + return $this->totpFactory->createTotpForUser($user)->getProvisioningUri(); + } + + public function generateSecret(): string + { + return Base32::encodeUpperUnpadded(random_bytes(32)); + } +}