OpenStackweb · smarcet · Aug 12, 2026 · Aug 12, 2026
diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
@@ -591,20 +591,10 @@ public function postLogin()
 
                     if ($flow == IAuthService::AuthenticationFlowPasswordless) {
 
-                        // Passwordless login is single-factor (email access only) and
-                        // must not be usable to satisfy MFA enforcement (SDS idp-mfa.md
-                        // §7.4 / Open Question #3).
-                        $existing_user = $this->auth_service->getUserByUsername($username);
-                        if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) {
-                            throw new AuthenticationException(
-                                "This account requires password and two-factor authentication. Please use the password login option."
-                            );
-                        }
-
                         $client = $this->resolveClientFromMemento();
 
                         $otpClaim = OAuth2OTP::fromParams($username, $connection, $password);
-                        $this->auth_service->loginWithOTP($otpClaim, $client);
+                        $this->auth_service->loginWithOTPEnforcing2FA($otpClaim, $client);
                         // A completed login must not leave the OTP screen restorable
                         // on a later refresh - same identity-leakage concern already
                         // fixed for the MFA flow's verify2FA()/verify2FARecovery().

diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php
@@ -271,38 +271,92 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $
         );
 
         // TX-C: resolve or create user, finalize, login
-        return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) {
+        return $this->tx_service->transaction(function () use ($otp, $client, $remember) {
+            $user = $this->resolveOTPUser($otp);
+            $this->finalizeRedemption($otp, $user, $client);
+            Auth::login($user, $remember);
+            Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
+            return $otp;
+        });
+    }
 
-            $user = $this->getUserByUsername($otp->getUserName());
+    /**
+     * @param OAuth2OTP $otpClaim
+     * @param Client|null $client
+     * @param bool $remember
+     * @return OAuth2OTP|null
+     * @throws Exception
+     */
+    public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP
+    {
+        Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName()));
 
-            if (is_null($user)) {
-                Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName()));
-                $user = $this->auth_user_service->registerUser(
-                    [
-                        'email' => $otp->getUserName(),
-                        'email_verified' => true,
-                        'send_email_verified_notice' => false,
-                        'active' => true,
-                    ],
-                    $otp
-                );
-            } else if ($user->isActive()) {
-                $user->verifyEmail(false);
-            }
+        $otp = $this->findAndValidateOTP(
+            $otpClaim->getValue(),
+            $otpClaim->getUserName(),
+            $otpClaim->getConnection(),
+            $otpClaim->getScope(),
+            $client
+        );
 
-            if (!$user->canLogin()) {
-                Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId()));
-                throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
+        // TX-C: resolve or create user, enforce 2FA, finalize, login
+        return $this->tx_service->transaction(function () use ($otp, $client, $remember) {
+            $user = $this->resolveOTPUser($otp);
+
+            // Passwordless login is single-factor (email access only) and must not
+            // be usable to satisfy MFA enforcement (SDS idp-mfa.md §7.4 / Open
+            // Question #3). Checked here - after the OTP is proven valid, before
+            // finalizeRedemption()/Auth::login() - so neither a guessed code nor a
+            // rejected valid code ever triggers a login side effect (redemption,
+            // Auth::login, the Login event / queued PostLoginUser job).
+            if ($user->shouldRequire2FA()) {
+                throw new AuthenticationException(
+                    "This account requires password and two-factor authentication. Please use the password login option."
+                );
             }
 
             $this->finalizeRedemption($otp, $user, $client);
-
             Auth::login($user, $remember);
-            Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId()));
+            Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA user %s logged in.", $user->getId()));
             return $otp;
         });
     }
 
+    /**
+     * Resolves the user for an already-validated passwordless OTP, auto-registering
+     * a brand-new email if needed. Does not finalize redemption or log in - callers
+     * decide that (and whether to enforce 2FA first).
+     * @param OAuth2OTP $otp
+     * @return User
+     * @throws AuthenticationException
+     */
+    private function resolveOTPUser(OAuth2OTP $otp): User
+    {
+        $user = $this->getUserByUsername($otp->getUserName());
+
+        if (is_null($user)) {
+            Log::debug(sprintf("AuthService::resolveOTPUser user %s does not exist; auto-registering.", $otp->getUserName()));
+            $user = $this->auth_user_service->registerUser(
+                [
+                    'email' => $otp->getUserName(),
+                    'email_verified' => true,
+                    'send_email_verified_notice' => false,
+                    'active' => true,
+                ],
+                $otp
+            );
+        } else if ($user->isActive()) {
+            $user->verifyEmail(false);
+        }
+
+        if (!$user->canLogin()) {
+            Log::warning(sprintf("AuthService::resolveOTPUser user %s cannot login (not active).", $user->getId()));
+            throw new AuthenticationException("We are sorry, your username or password does not match an existing record.");
+        }
+
+        return $user;
+    }
+
     /**
      * Verifies an OTP against an already-authenticated session user (MFA primitive).
      *

diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php
@@ -91,6 +91,18 @@ public function loginUser(User $user, bool $remember): void;
      */
     public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP;
 
+    /**
+     * Same as loginWithOTP(), but rejects the login when the resolved user has
+     * MFA enforced - passwordless (email-only) proof is not sufficient for an
+     * account that requires two-factor authentication.
+     * @param OAuth2OTP $otpClaim
+     * @param Client|null $client
+     * @param bool $remember
+     * @return OAuth2OTP|null
+     * @throws AuthenticationException
+     */
+    public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP;
+
 
     /**
      * @param string $username

diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php
@@ -193,6 +193,15 @@ public function testNonAdminWithoutMFALogsInNormally(): void
 
     public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void
     {
+        // No separate "no login side effect" assertions here on purpose: the
+        // DB-visible ones (OTP redemption, sibling revocation) are unreachable
+        // by construction, because the guard throws inside the AuthService
+        // transaction and DoctrineTransactionService rolls the whole closure
+        // back - moving the guard below finalizeRedemption() leaves the OTP
+        // un-redeemed all the same. The session-visible one (Auth::login() and
+        // the Login event that queues PostLoginUser) is already covered by the
+        // Auth::check() assertion below, which is what fails first if the guard
+        // is moved below Auth::login(). Both were verified by mutation.
         $this->emitOTP(self::ADMIN_EMAIL);
         $code = $this->latestOtpCode(self::ADMIN_EMAIL);
 
@@ -213,6 +222,56 @@ public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void
         $this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password');
     }
 
+    public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void
+    {
+        // Regression guard: the enforcement check used to run BEFORE the OTP was
+        // validated, so an attacker submitting a garbage/guessed code against an
+        // MFA-enforced account's email got the distinguishing "requires password
+        // and two-factor authentication" message without ever proving control of
+        // the inbox - an account-enumeration oracle. The check must now only be
+        // reachable after loginWithOTPEnforcing2FA() has proven the code valid,
+        // so an invalid code gets the same generic rejection for any account.
+        //
+        // Enumeration is about DISTINGUISHABILITY, not about one phrase: merely
+        // asserting the absence of "two-factor authentication" would still pass
+        // if some future enforced-only branch leaked a *differently* worded
+        // message. So the enforced account's rejection is compared byte-for-byte
+        // against a non-enforced control driven through the same endpoint with
+        // the same bad code.
+        $control_email = $this->createPlainUser();
+
+        $this->emitOTP(self::ADMIN_EMAIL);
+        $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code');
+
+        $this->assertFalse(Auth::check(), 'an invalid code must never authenticate');
+        $this->assertResponseStatus(302);
+        $enforced_notice = Session::get('flash_notice');
+        $this->assertNotNull($enforced_notice, 'the enforced account must get a flashed rejection');
+
+        // Cleared so the control's assertion cannot silently read the enforced
+        // account's leftover flash and compare a value against itself.
+        Session::forget('flash_notice');
+
+        $this->emitOTP($control_email);
+        $this->postLoginOTP($control_email, 'not-the-real-code');
+
+        $this->assertFalse(Auth::check(), 'an invalid code must never authenticate the control account either');
+        $this->assertResponseStatus(302);
+        $control_notice = Session::get('flash_notice');
+        $this->assertNotNull($control_notice, 'the control account must get a flashed rejection');
+
+        $this->assertSame(
+            $control_notice,
+            $enforced_notice,
+            'an invalid code must produce an identical rejection for an enforced and a non-enforced account - any difference is an enumeration oracle'
+        );
+        $this->assertStringNotContainsString(
+            'two-factor authentication',
+            $enforced_notice,
+            'an invalid code must not leak that the account is MFA-enforced'
+        );
+    }
+
     public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void
     {
         $email = $this->createPlainUser();