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/ResetPasswordController.php b/src/Controller/ResetPasswordController.php index 682bb54fd..03bb406d1 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()) { @@ -123,7 +124,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/Controller/UserController.php b/src/Controller/UserController.php index 1e376def7..5b6cf39a8 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/EventListener/ResolvedTwoFactorCodeCredentialsListener.php b/src/EventListener/ResolvedTwoFactorCodeCredentialsListener.php new file mode 100644 index 000000000..23fe0df92 --- /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; + +class ResolvedTwoFactorCodeCredentialsListener +{ + #[AsEventListener(event: AuthenticationTokenCreatedEvent::class, priority: 512)] + 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..05435fb06 100644 --- a/src/Form/ResetPasswordFormType.php +++ b/src/Form/ResetPasswordFormType.php @@ -12,13 +12,25 @@ namespace App\Form; +use App\Entity\User; use App\Validator\Password; +use App\Validator\TwoFactorCode; 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 + ->define('user') + ->allowedTypes(User::class) + ->required(); + } + public function buildForm(FormBuilderInterface $builder, array $options): void { $builder @@ -32,5 +44,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 TwoFactorCode($options['user']), + ], + ]); + } } } diff --git a/src/Form/Type/EnableTwoFactorAuthType.php b/src/Form/Type/EnableTwoFactorAuthType.php index ce4aa638d..a45dd5119 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\Validator\TwoFactorCode; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; @@ -25,13 +27,19 @@ class EnableTwoFactorAuthType extends AbstractType { public function buildForm(FormBuilderInterface $builder, array $options): void { - $builder->add('code', TextType::class); + $builder->add('code', TextType::class, [ + 'constraints' => [new TwoFactorCode($options['user'])] + ]); } public function configureOptions(OptionsResolver $resolver): void { - $resolver->setDefaults([ - 'data_class' => EnableTwoFactorRequest::class, - ]); + $resolver + ->setDefaults([ + 'data_class' => EnableTwoFactorRequest::class, + ]) + ->define('user') + ->allowedTypes(User::class) + ->required(); } } 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/src/Validator/TwoFactorCode.php b/src/Validator/TwoFactorCode.php new file mode 100644 index 000000000..1383ac069 --- /dev/null +++ b/src/Validator/TwoFactorCode.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\Validator; + +use App\Entity\User; +use Symfony\Component\Validator\Constraint; + +class TwoFactorCode extends Constraint +{ + public function __construct( + public readonly User $user, + public readonly string $message = 'Invalid authenticator code', + ) { + parent::__construct(); + } +} diff --git a/src/Validator/TwoFactorCodeValidator.php b/src/Validator/TwoFactorCodeValidator.php new file mode 100644 index 000000000..d592eb0a4 --- /dev/null +++ b/src/Validator/TwoFactorCodeValidator.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\Validator; + +use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpAuthenticatorInterface; +use Symfony\Component\Validator\Constraint; +use Symfony\Component\Validator\ConstraintValidator; + +class TwoFactorCodeValidator extends ConstraintValidator +{ + public function __construct( + private readonly TotpAuthenticatorInterface $totpAuthenticator, + ) {} + + /** + * @param TwoFactorCode $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/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()); + } +} 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)); + } +}