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 <kevin@kdecherf.com>

migrations/Version20250413133131.php

@@ -0,0 +1,47 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Application\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Wallabag\Doctrine\WallabagMigration;
+
+/**
+ * Add boolean for two-step setup for google authenticator.
+ */
+final class Version20250413133131 extends WallabagMigration
+{
+    public function up(Schema $schema): void
+    {
+        $userTable = $schema->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');
+    }
+}

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);
     }
 
     /**

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

templates/Config/otp_app.html.twig

@@ -18,14 +18,14 @@
                                 <img data-controller="qrcode" data-qrcode-url-value="{{ qr_code|raw }}" class="hide-on-med-and-down" />
                             </p>
 
-                            <div data-controller="highlight">
+                            <div id="config_otp_app_secret">
                                 {{ 'config.otp.app.two_factor_code_description_5'|trans }} <pre><code>{{ secret }}</code></pre>
                             </div>
                         </li>
                         <li>
                             <p>{{ 'config.otp.app.two_factor_code_description_3'|trans }}</p>
 
-                            <div data-controller="highlight"><pre><code>{{ backupCodes|join("\n") }}</code></pre></div>
+                            <div><pre><code>{{ backupCodes|join("\n") }}</code></pre></div>
                         </li>
                         <li>
                             <p>{{ 'config.otp.app.two_factor_code_description_4'|trans }}</p>
@@ -36,7 +36,7 @@
                             </div>
                             {% endfor %}
 
-                            <form class="form" action="{{ path("config_otp_app_check") }}" method="post">
+                            <form class="form" action="{{ path("config_otp_app_check") }}" method="post" name="config_otp_app_check">
                                 <input type="hidden" name="token" value="{{ csrf_token('otp') }}" />
                                 <div class="card-content">
                                     <div class="row">

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()

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();
     }