From b09224cac132724f5719e5671a5e9cedd6a4293b Mon Sep 17 00:00:00 2001 From: Kevin Decherf Date: Sun, 13 Apr 2025 16:12:13 +0200 Subject: [PATCH] Add a two-step setup of OTP Before this change, 2FA with OTP was enabled before the user was able to submit a code to validate the setup. Thus, this could lead to a situation where the user is locked out of her account if there was an issue setting up her application. Now we rely on a new boolean property that is set to true only after the user submits a valid code during the setup phase. Fixes #4867 Signed-off-by: Kevin Decherf --- migrations/Version20250413133131.php | 47 +++++++++++++++ src/Controller/ConfigController.php | 12 ++-- src/Entity/User.php | 12 +++- templates/Config/otp_app.html.twig | 6 +- tests/Controller/ConfigControllerTest.php | 63 ++++++++++++++++++--- tests/Controller/SecurityControllerTest.php | 2 + 6 files changed, 125 insertions(+), 17 deletions(-) create mode 100644 migrations/Version20250413133131.php diff --git a/migrations/Version20250413133131.php b/migrations/Version20250413133131.php new file mode 100644 index 000000000..db1fff2cf --- /dev/null +++ b/migrations/Version20250413133131.php @@ -0,0 +1,47 @@ +getTable($this->getTable('user')); + + $this->skipIf($userTable->hasColumn('google_authenticator'), 'It seems that you already played this migration.'); + + $userTable->addColumn('google_authenticator', 'boolean', [ + 'default' => false, + 'notnull' => true, + ]); + } + + /** + * Query to update data in user table, as it's not possible to perform this in the `up` method. + */ + public function postUp(Schema $schema): void + { + $this->skipIf(!$schema->getTable($this->getTable('user'))->hasColumn('google_authenticator'), 'Unable to update google_authenticator column'); + $this->connection->executeQuery( + 'UPDATE ' . $this->getTable('user') . ' SET google_authenticator = :googleAuthenticator WHERE googleAuthenticatorSecret IS NOT NULL AND googleAuthenticatorSecret <> :emptyString', + [ + 'googleAuthenticator' => true, + 'emptyString' => '', + ] + ); + } + + public function down(Schema $schema): void + { + $userTable = $schema->getTable($this->getTable('user')); + $userTable->dropColumn('google_authenticator'); + } +} diff --git a/src/Controller/ConfigController.php b/src/Controller/ConfigController.php index 39b4e48ad..fde92ddaa 100644 --- a/src/Controller/ConfigController.php +++ b/src/Controller/ConfigController.php @@ -313,6 +313,7 @@ class ConfigController extends AbstractController $user = $this->getUser(); $user->setGoogleAuthenticatorSecret(''); + $user->setGoogleAuthenticator(false); $user->setBackupCodes(null); $this->userManager->updateUser($user); @@ -354,11 +355,6 @@ class ConfigController extends AbstractController $this->userManager->updateUser($user); $this->entityManager->flush(); - $this->addFlash( - 'notice', - 'flashes.config.notice.otp_enabled' - ); - return $this->render('Config/otp_app.html.twig', [ 'backupCodes' => $backupCodes, 'qr_code' => $googleAuthenticator->getQRContent($user), @@ -408,6 +404,9 @@ class ConfigController extends AbstractController 'notice', 'flashes.config.notice.otp_enabled' ); + $user->setGoogleAuthenticator(true); + $this->userManager->updateUser($user); + $this->entityManager->flush(); return $this->redirect($this->generateUrl('config') . '#set3'); } @@ -421,8 +420,9 @@ class ConfigController extends AbstractController $user->setBackupCodes(null); $this->userManager->updateUser($user); + $this->entityManager->flush(); - return $this->redirect($this->generateUrl('config') . '#set3'); + return $this->redirect($this->generateUrl('config_otp_app'), 307); } /** diff --git a/src/Entity/User.php b/src/Entity/User.php index fc48d8737..f3668e787 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -147,6 +147,11 @@ class User extends BaseUser implements EmailTwoFactorInterface, GoogleTwoFactorI #[ORM\Column(name: 'googleAuthenticatorSecret', type: 'string', nullable: true)] private $googleAuthenticatorSecret; + // default value is explicitly set to false here to ensure that Doctrine + // does not complain about schema mapping mismatch + #[ORM\Column(name: 'google_authenticator', type: 'boolean', options: ['default' => false])] + private $googleAuthenticator = false; + /** * @var array */ @@ -264,6 +269,11 @@ class User extends BaseUser implements EmailTwoFactorInterface, GoogleTwoFactorI $this->emailTwoFactor = $emailTwoFactor; } + public function setGoogleAuthenticator(bool $googleAuthenticator): void + { + $this->googleAuthenticator = $googleAuthenticator; + } + /** * Used in the user config form to be "like" the email option. */ @@ -294,7 +304,7 @@ class User extends BaseUser implements EmailTwoFactorInterface, GoogleTwoFactorI public function isGoogleAuthenticatorEnabled(): bool { - return $this->googleAuthenticatorSecret ? true : false; + return $this->googleAuthenticator; } public function getGoogleAuthenticatorUsername(): string diff --git a/templates/Config/otp_app.html.twig b/templates/Config/otp_app.html.twig index 475d783b1..96df3955a 100644 --- a/templates/Config/otp_app.html.twig +++ b/templates/Config/otp_app.html.twig @@ -18,14 +18,14 @@

-
+
{{ 'config.otp.app.two_factor_code_description_5'|trans }} 
{{ secret }}

  • {{ 'config.otp.app.two_factor_code_description_3'|trans }}

    - 
    {{ backupCodes|join("\n") }}
    + 
    {{ backupCodes|join("\n") }}

  • {{ 'config.otp.app.two_factor_code_description_4'|trans }}

    @@ -36,7 +36,7 @@
    • {% endfor %} -
    +
    diff --git a/tests/Controller/ConfigControllerTest.php b/tests/Controller/ConfigControllerTest.php index f41f25c13..43014f7e1 100644 --- a/tests/Controller/ConfigControllerTest.php +++ b/tests/Controller/ConfigControllerTest.php @@ -3,6 +3,7 @@ namespace Tests\Wallabag\Controller; use Doctrine\ORM\EntityManagerInterface; +use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Google\GoogleAuthenticatorInterface; use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\HttpFoundation\Session\SessionInterface; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; @@ -1223,27 +1224,74 @@ class ConfigControllerTest extends WallabagTestCase public function testUserEnable2faGoogle() { + $googleAuthenticatorMock = $this->createMock(GoogleAuthenticatorInterface::class); + $googleAuthenticatorMock + ->method('generateSecret') + ->willReturn('DUMMYSECRET'); + $googleAuthenticatorMock + ->method('checkCode') + ->willReturnCallback(function ($user, $code) { + return '123456' === $code; + }); + $this->logInAs('admin'); $client = $this->getTestClient(); + $client->disableReboot(); // Disable reboot to keep the mock in the container + + // name::class notation does not work in this context + self::getContainer()->set('scheb_two_factor.security.google_authenticator', $googleAuthenticatorMock); $crawler = $client->request('GET', '/config'); - $form = $crawler->filter('form[name=config_otp_app]')->form(); - $client->submit($form); + $crawler = $client->submit($form); $this->assertSame(200, $client->getResponse()->getStatusCode()); - // restore user + $secret = $crawler->filter('div#config_otp_app_secret pre code')->innerText(); + $this->assertSame('DUMMYSECRET', $secret); + + $em = $this->getEntityManager(); + $user = $em + ->getRepository(User::class) + ->findOneByUsername('admin'); + // At this phase, the user should not have 2FA enabled + $this->assertFalse($user->isGoogleTwoFactor()); + + // First test: send invalid OTP code + $form = $crawler->filter('form[name=config_otp_app_check]')->form(); + $data = [ + '_auth_code' => '000000', + ]; + $client->submit($form, $data); + + $this->assertSame(307, $client->getResponse()->getStatusCode()); + $this->assertStringContainsString('flashes.config.notice.otp_code_invalid', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]); + + // Follow the redirect to the OTP check form again + $crawler = $client->followRedirect(); + + // Second test: send valid OTP code + $form = $crawler->filter('form[name=config_otp_app_check]')->form(); + $data = [ + '_auth_code' => '123456', + ]; + $client->submit($form, $data); + + $this->assertSame(302, $client->getResponse()->getStatusCode()); + $this->assertStringContainsString('flashes.config.notice.otp_enabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]); + $em = $this->getEntityManager(); $user = $em ->getRepository(User::class) ->findOneByUsername('admin'); $this->assertTrue($user->isGoogleTwoFactor()); - $this->assertGreaterThan(0, $user->getBackupCodes()); + $this->assertGreaterThan(0, \count($user->getBackupCodes())); - $user->setGoogleAuthenticatorSecret(false); - $user->setBackupCodes(null); + // Restore user + $user->setGoogleAuthenticatorSecret(''); + $user->setGoogleAuthenticator(false); + $user->setBackupCodes([]); $em->persist($user); $em->flush(); } @@ -1259,6 +1307,7 @@ class ConfigControllerTest extends WallabagTestCase ->findOneByUsername('admin'); $user->setGoogleAuthenticatorSecret('Google2FA'); + $user->setGoogleAuthenticator(true); $em->persist($user); $em->flush(); @@ -1271,7 +1320,6 @@ class ConfigControllerTest extends WallabagTestCase $this->assertStringContainsString('flashes.config.notice.otp_disabled', $client->getContainer()->get(SessionInterface::class)->getFlashBag()->get('notice')[0]); - // restore user $em = $this->getEntityManager(); $user = $em ->getRepository(User::class) @@ -1279,6 +1327,7 @@ class ConfigControllerTest extends WallabagTestCase $this->assertEmpty($user->getGoogleAuthenticatorSecret()); $this->assertEmpty($user->getBackupCodes()); + $this->assertFalse($user->isGoogleTwoFactor()); } public function testExportTaggingRule() diff --git a/tests/Controller/SecurityControllerTest.php b/tests/Controller/SecurityControllerTest.php index 3a90b1337..110b3ab7f 100644 --- a/tests/Controller/SecurityControllerTest.php +++ b/tests/Controller/SecurityControllerTest.php @@ -66,6 +66,7 @@ class SecurityControllerTest extends WallabagTestCase ->getRepository(User::class) ->findOneByUsername('admin'); $user->setGoogleAuthenticatorSecret('26LDIHYGHNELOQEM'); + $user->setGoogleAuthenticator(true); $em->persist($user); $em->flush(); @@ -78,6 +79,7 @@ class SecurityControllerTest extends WallabagTestCase ->getRepository(User::class) ->findOneByUsername('admin'); $user->setGoogleAuthenticatorSecret(null); + $user->setGoogleAuthenticator(false); $em->persist($user); $em->flush(); }