MFA hardening: OAuth2 memento guard on recovery, DTO refactors, full OIDC circuit tests (#152)

* fix(2fa): validate pending OAuth2 client before redeeming a recovery code

verify2FARecovery() skipped the resolveClientFromMemento() guard that
verify2FA() applies, so with a pending OAuth2 authorization request whose
client no longer exists the single-use recovery code was burned and an IDP
session established for an authorization request that could only fail at
the /oauth2/auth hop. Apply the same guard before redemption; recovery-code
checking itself stays client-agnostic.

* refactor(2fa): extract MFA error_code literals into MFAConstants

The error_code values emitted by UserController's MFA endpoints were
hardcoded strings, duplicated in TwoFactorRateLimitMiddleware::FAILURE_CODES
where a silent drift would break the rate-limit failure counting. Tests keep
asserting the literal wire values on purpose, pinning the contract.

* refactor(2fa): return a typed DTO from getPendingState() instead of an array

MFAPendingState (getUserId / getPendingAt / shouldRemember) replaces the
string-keyed array, so callers stop scattering 'user_id'/'remember' literals
and casts, and the shape is enforced by the type system instead of by
convention.

* refactor(2fa): serialize recovery-codes standing via a RecoveryCodesStatus DTO

verify2FARecovery() and getProfile() each hand-built the
recovery_codes_remaining/total/low_threshold payload with their own config()
reads and magic defaults. IRecoveryCodeService::getStatus() now returns a
RecoveryCodesStatus DTO whose toArray() owns the wire keys, so both call
sites merge the same serialized shape. Side effect: the recovery XHR
response now also carries recovery_codes_total (additive, ignored by the
SPA).

* test(2fa): prove the full OIDC consent circuit for verify2FA and verify2FARecovery

authorize -> login -> MFA challenge -> verify (OTP / recovery code) ->
redirect_url back to the authorization endpoint (rebuilt from the session
memento) -> consent screen -> AllowOnce -> authorization code delivered to
the client redirect_uri. Locks in that the XHR verify contract composes with
the interactive grant's memento round-trip.

Note: OIDCProtocolTestCase's password-login circuits (e.g. testAuthCode)
predate the MFA gate and post a wrong seed password - broken independently
of this change.

* test(oidc): repair OIDCProtocolTestCase login circuits (stale password + MFA gate)

Two stacked breakages, both predating and unrelated to each individual test:

- 021bee3 (jul 2024) changed the TestSeeder passwords from '1qaz2wsx' to
  '1Qaz2wsx!' without updating this class, so every password login leg has
  silently failed since - errorLogin() also answers 302, so the post-login
  assertion kept passing and tests died downstream instead.
- The MFA gate now challenges the seeded login user (SuperAdminGroup is in
  two_factor.enforced_groups), so even a correct password stops at the 2FA
  challenge. This class exercises the OIDC protocol, not the gate - enforced
  groups are cleared in prepareForTests(); the gate plus the full
  authorize -> MFA -> consent -> code circuit live in TwoFactorLoginFlowTest.

Result: 29 broken -> 3 (32/35 green). The 3 residuals have distinct
pre-existing causes: testConsentLogin and
testGetRefreshTokenWithPromptSetToConsentLogin lose the login hint because
AuthService::logout()'s Session::flush() (4864f50 / #118) wipes the
session-backed security context even when called with clear_security_ctx =
false (prompt=login path); testTokenResponseModePost uses max_age=1 and the
multi-request dance now takes longer than 1s, forcing a re-login.

* fix(auth): honor clear_security_ctx=false across logout()'s session flush

The Session::flush() hardening added in #118 wipes the whole session at the
end of logout(), including the session-backed security context - even when
the caller passed clear_security_ctx = false (the prompt=login
re-authentication path in InteractiveGrantType::mustAuthenticateUser()),
which broke the login-hint prefill on the login screen for prompt=login
OIDC requests. Capture the context before the flush and re-save it after
the session ID regenerate; everything else is still flushed, so the #118
hardening stands.

* test(oidc): raise testTokenResponseModePost max_age from 1 to 3200

The test exercises response_mode=form_post, not max_age expiry
(testMaxAge1AndWait2 owns that) - with max_age=1 the multi-request
login+consent dance takes longer than 1s and the final authorize hop forced
a re-login instead of delivering the form post. 3200 matches the sibling
circuits. OIDCProtocolTestCase is now fully green: 35/35.

* test(2fa): negative-path OIDC circuits for verify2FA and verify2FARecovery

Six tests inside a pending OIDC authorization-code flow, three per endpoint:
- wrong code then correct code: the rejection keeps the pending challenge
  and the OAuth2 memento alive, and the retry completes the full circuit
  (consent -> authorization code).
- consecutive wrong codes up to the rate-limit threshold: every attempt is
  401 without a session, and once the window closes even the CORRECT code
  answers 429 - brute-forcing inside a pending flow buys no extra attempts.
- burned single-use code (used recovery code / redeemed OTP): rejected like
  any invalid code, and the flow still completes afterwards with a fresh
  code (new recovery code / resent OTP).

* test(2fa): cover the error branches of verify2FA and verify2FARecovery

- validator 412s (malformed request, no otp_value / recovery_code)
- vanished pending user -> mfa_session_expired + pending state cleared
- recovery without a pending challenge -> mfa_session_expired
- stale OAuth2 client guard on verify2FA (parity with the recovery test):
  412 before the OTP is redeemed
- audit failure on the FAILED-verify path stays a clean 401 with the
  error_code the rate-limit middleware keys on, for both endpoints

verify2FA line coverage 82.3% -> 95.2%, verify2FARecovery 82.7% -> 94.2%;
the only uncovered lines left are the generic Exception -> 500 catches.

* test(ci): actually run the protocol TestCase suites, stop hiding failure breadth

Two changes to phpunit.xml:
- The Application suite's <directory> scan only picks up *Test.php (PHPUnit's
  default suffix), so the four concrete *TestCase.php protocol suites
  (OAuth2Protocol, OIDCProtocol, OIDCPasswordless, OpenIdProtocol - 93 tests)
  were NEVER executed by CI. That is how OIDCProtocolTestCase stayed broken
  for two years with green builds. They are now listed explicitly.
- stopOnFailure=false so a run reports every failure instead of dying on the
  first one.

Also fixes the one test the newly-wired suites surfaced:
testResourceServerIntrospectionNotValidIP expected an unconditional 400, but
the resource-server IP check became opt-in in #98
(oauth2.validate_resource_server_ip, default off) - the test now enables the
flag before asserting the rejection.

Full-suite evidence (523 tests): green except 8 pre-existing
environment-dependent Turnstile tests that need TEST_USER_EMAIL /
TEST_USER_PASSWORD and the Turnstile secrets CI injects (they pass in CI;
locally their markTestSkipped guard is defeated by a typed-property
TypeError when the env vars are absent).

* refactor(2fa): single home for every MFA string constant

MFAConstants now owns all of them:
- error codes: the existing three plus mfa_rate_limit and mfa_required.
  ITwoFactorRateLimitService::RATE_LIMIT_ERROR_CODE and
  ILoginStrategy::MFA_REQUIRED alias it, so consumers keep their names while
  the value is defined once.
- 2fa_* session keys: previously defined TWICE in production
  (AbstractMFAChallengeStrategy's private consts and
  ITwoFactorRateLimitService::PENDING_USER_SESSION_KEY) - both now alias
  MFAConstants.

Also promotes the rate-limit cache-key prefix ('2fa_rate:', previously a
sprintf literal in TwoFactorRateLimitService duplicated by the test flush
helper) to ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX.

All ~50 hardcoded literals across TwoFactorLoginFlowTest,
AbstractMFAChallengeStrategyTest and EmailOTPMFAChallengeStrategyTest now
reference the constants.

* test(oidc): extract the seeded password into a SEED_PASSWORD constant

The literal appeared at 26 call sites; a seed password change is now a
one-line edit, matching TwoFactorLoginFlowTest. The trailing-space login
test keeps its spacing explicit around the constant, since that spacing
is the subject under test. Suite re-run in idp-app: 35/35, 506 assertions.

Author: smarcet

app/Http/Controllers/UserController.php

@@ -28,6 +28,7 @@
 use App\Services\Auth\IUserService as AuthUserService;
 use Auth\Exceptions\AuthenticationException;
 use Auth\Exceptions\UnverifiedEmailMemberException;
+use Auth\MFAConstants;
 use Auth\User;
 use Exception;
 use Illuminate\Http\Request as LaravelRequest;
@@ -762,7 +763,7 @@ public function verify2FA()
                 return $this->mfaSessionExpired();
             }
 
-            $user = $this->auth_service->getUserById((int) $pending['user_id']);
+            $user = $this->auth_service->getUserById($pending->getUserId());
             if (is_null($user) || !$user->isTwoFactorMethodEnabled($method)) {
                 $strategy->clearPendingState();
                 return $this->mfaSessionExpired();
@@ -784,7 +785,7 @@ public function verify2FA()
             } catch (AuthenticationException $ex) {
                 Log::warning($ex);
                 // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity.
-                $userId = (int) $pending['user_id'];
+                $userId = $pending->getUserId();
                 $user = $this->auth_service->getUserById($userId) ?? $user;
                 // Best-effort: an audit-logging failure here must not turn a
                 // clean 401 into a 500 (which would also drop the error_code
@@ -799,11 +800,11 @@ public function verify2FA()
                 } catch (\Throwable $auditEx) {
                     Log::warning($auditEx);
                 }
-                return $this->unauthorized(['error_code' => 'mfa_verification_failed']);
+                return $this->unauthorized(['error_code' => MFAConstants::ERROR_CODE_VERIFICATION_FAILED]);
             }
 
             // Second factor verified: establish the session.
-            $this->auth_service->loginUser($user, (bool) $pending['remember']);
+            $this->auth_service->loginUser($user, $pending->shouldRemember());
 
             if ($trust_device) {
                 // Best-effort: the OTP is already redeemed and the session
@@ -879,18 +880,26 @@ public function verify2FARecovery()
                 return $this->mfaSessionExpired();
             }
 
-            $user = $this->auth_service->getUserById((int) $pending['user_id']);
+            $user = $this->auth_service->getUserById($pending->getUserId());
             if (is_null($user)) {
                 $strategy->clearPendingState();
                 return $this->mfaSessionExpired();
             }
 
+            // Same guard verify2FA() applies before redeeming: a pending OAuth2
+            // authorization request must still resolve to an existing client,
+            // or the single-use recovery code would be burned (and a session
+            // established) for an authorization request that can only fail at
+            // the /oauth2/auth hop. Recovery-code checking itself is
+            // client-agnostic, so the resolved client is not passed down.
+            $this->resolveClientFromMemento();
+
             try {
                 $this->auth_service->verifyMFARecoveryCode($user, $strategy, $recovery_code);
             } catch (AuthenticationException $ex) {
                 Log::warning($ex);
                 // Re-fetch user: the tx wrapper closed/reset the EM on failure, detaching the entity.
-                $userId = (int) $pending['user_id'];
+                $userId = $pending->getUserId();
                 $user = $this->auth_service->getUserById($userId) ?? $user;
                 // Best-effort: see verify2FA() for rationale.
                 try {
@@ -903,10 +912,10 @@ public function verify2FARecovery()
                 } catch (\Throwable $auditEx) {
                     Log::warning($auditEx);
                 }
-                return $this->unauthorized(['error_code' => 'mfa_invalid_recovery']);
+                return $this->unauthorized(['error_code' => MFAConstants::ERROR_CODE_INVALID_RECOVERY]);
             }
 
-            $this->auth_service->loginUser($user, (bool) $pending['remember']);
+            $this->auth_service->loginUser($user, $pending->shouldRemember());
             $strategy->clearPendingState();
             $this->clearMFAUISessionState();
 
@@ -925,16 +934,14 @@ public function verify2FARecovery()
             }
 
             // See verify2FA() for rationale: return the destination as data so a real
-            // top-level navigation (not this XHR) performs any cross-origin hop.
+            // top-level navigation (not this XHR) performs any cross-origin hop. The
+            // recovery-codes standing rides along so the login page can warn the user
+            // when they've just burned into their last few codes (see RecoveryCodesStatus).
             $redirect = $this->login_strategy->postLogin();
-            return $this->ok([
-                'redirect_url' => $redirect->getTargetUrl(),
-                // CU-86ba2zp66 / sds/idp-mfa.md §4.10.3, §4.11 step 5: the login page
-                // must be able to warn the user when they've just burned into their
-                // last few recovery codes, since it may be their only way back in.
-                'recovery_codes_remaining' => $this->recovery_code_service->countUnusedRecoveryCodes($user),
-                'recovery_codes_low_threshold' => (int) config('auth.recovery_codes.low_threshold', 3),
-            ]);
+            return $this->ok(array_merge(
+                ['redirect_url' => $redirect->getTargetUrl()],
+                $this->recovery_code_service->getStatus($user)->toArray()
+            ));
         } catch (ValidationException $ex) {
             Log::warning($ex);
             return $this->error412($ex->getMessages());
@@ -969,13 +976,13 @@ public function resend2FA()
                 return $this->mfaSessionExpired();
             }
 
-            $user = $this->auth_service->getUserById((int) $pending['user_id']);
+            $user = $this->auth_service->getUserById($pending->getUserId());
             if (is_null($user) || !$user->isTwoFactorMethodEnabled($method)) {
                 $strategy->clearPendingState();
                 return $this->mfaSessionExpired();
             }
 
-            $payload = $this->auth_service->resendMFAChallenge($user, $strategy, $this->resolveClientFromMemento(), (bool) $pending['remember']);
+            $payload = $this->auth_service->resendMFAChallenge($user, $strategy, $this->resolveClientFromMemento(), $pending->shouldRemember());
 
             // Keep the refresh-restorable session state in sync with the
             // fresh challenge (e.g. otp_lifetime countdown resets on resend,
@@ -1021,7 +1028,7 @@ public function resend2FA()
     private function mfaSessionExpired()
     {
         $this->clearMFAUISessionState();
-        return $this->unauthorized(['error_code' => 'mfa_session_expired']);
+        return $this->unauthorized(['error_code' => MFAConstants::ERROR_CODE_SESSION_EXPIRED]);
     }
 
     /**
@@ -1191,7 +1198,7 @@ public function getProfile()
                 $lang2Code[] = $lang;
         }
 
-        return View::make("profile", [
+        return View::make("profile", array_merge([
             'user' => json_encode(SerializerRegistry::getInstance()->getSerializer(
                 $user, SerializerRegistry::SerializerType_Private)->serialize()),
             "openid_url" => $this->server_configuration_service->getUserIdentityEndpointURL($user->getIdentifier()),
@@ -1200,10 +1207,7 @@ public function getProfile()
             'countries' => CountryList::getCountries(),
             'languages' => $lang2Code,
             'two_factor_enabled' => $user->shouldRequire2FA(),
-            'recovery_codes_remaining' => $this->recovery_code_service->countUnusedRecoveryCodes($user),
-            'recovery_codes_total' => (int)config('auth.recovery_codes.count', 10),
-            'recovery_codes_low_threshold' => (int)config('auth.recovery_codes.low_threshold', 3),
-        ]);
+        ], $this->recovery_code_service->getStatus($user)->toArray()));
     }
 
     public function deleteTrustedSite($id)

app/Http/Middleware/TwoFactorRateLimitMiddleware.php

@@ -13,6 +13,7 @@
  **/
 
 use App\Services\Auth\ITwoFactorRateLimitService;
+use Auth\MFAConstants;
 use Closure;
 use Illuminate\Cache\RateLimiting\Unlimited;
 use Illuminate\Support\Facades\Log;
@@ -41,8 +42,8 @@ final class TwoFactorRateLimitMiddleware
      * Response error_code values that count as a verification failure.
      */
     private const FAILURE_CODES = [
-        'mfa_verification_failed',
-        'mfa_invalid_recovery',
+        MFAConstants::ERROR_CODE_VERIFICATION_FAILED,
+        MFAConstants::ERROR_CODE_INVALID_RECOVERY,
     ];
 
     public function __construct(private readonly ITwoFactorRateLimitService $rate_limit_service)

app/Services/Auth/IRecoveryCodeService.php

@@ -61,4 +61,10 @@ public function enableTwoFactorAndGenerateCodes(User $user, string $method): arr
      * @return int count of unused recovery codes
      */
     public function countUnusedRecoveryCodes(User $user): int;
+
+    /**
+     * @param User $user
+     * @return RecoveryCodesStatus remaining/total/low-threshold standing for the user
+     */
+    public function getStatus(User $user): RecoveryCodesStatus;
 }

app/Services/Auth/ITwoFactorRateLimitService.php

@@ -2,6 +2,8 @@
 
 namespace App\Services\Auth;
 
+use Auth\MFAConstants;
+
 /**
  * Copyright 2026 OpenStack Foundation
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -38,14 +40,14 @@ interface ITwoFactorRateLimitService
     public const ActionResend   = 'resend';
     public const ActionOtp      = 'otp';
 
-    public const RATE_LIMIT_ERROR_CODE = 'mfa_rate_limit';
+    public const RATE_LIMIT_ERROR_CODE = MFAConstants::ERROR_CODE_RATE_LIMIT;
     public const RATE_LIMIT_MESSAGE    = 'Too many attempts. Please try again later.';
 
     /**
      * Session key holding the user id of the pending MFA challenge - the
      * subject the verify/recovery/resend named limiters throttle by.
      */
-    public const PENDING_USER_SESSION_KEY = '2fa_pending_user_id';
+    public const PENDING_USER_SESSION_KEY = MFAConstants::SESSION_KEY_PENDING_USER_ID;
 
     /**
      * Prefix applied to the Action* constants when registering/looking up
@@ -57,6 +59,14 @@ interface ITwoFactorRateLimitService
      */
     public const RATE_LIMITER_NAME_PREFIX = '2fa-rate:';
 
+    /**
+     * Prefix of the cache keys holding the per-subject attempt counters
+     * (and their companion ":timer" keys) - see cacheKey() in the
+     * implementation. Distinct from RATE_LIMITER_NAME_PREFIX (dash), which
+     * names the limiters, not the storage.
+     */
+    public const RATE_LIMIT_CACHE_KEY_PREFIX = '2fa_rate:';
+
     /**
      * @param string $action one of self::ActionVerify|ActionRecovery|ActionResend|ActionOtp
      * @param string|int $subject a user id for session-keyed actions, or a raw

app/Services/Auth/RecoveryCodeService.php

@@ -156,4 +156,16 @@ public function countUnusedRecoveryCodes(User $user): int
     {
         return count($this->repository->getUnusedByUser($user));
     }
+
+    /**
+     * @inheritDoc
+     */
+    public function getStatus(User $user): RecoveryCodesStatus
+    {
+        return new RecoveryCodesStatus(
+            $this->countUnusedRecoveryCodes($user),
+            (int) config('auth.recovery_codes.count', 10),
+            (int) config('auth.recovery_codes.low_threshold', 3)
+        );
+    }
 }

app/Services/Auth/RecoveryCodesStatus.php

@@ -0,0 +1,60 @@
+<?php
+namespace App\Services\Auth;
+/**
+ * Copyright 2026 OpenStack Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+
+/**
+ * Immutable snapshot of a user's recovery-codes standing, built by
+ * IRecoveryCodeService::getStatus(). toArray() owns the wire keys consumed by
+ * the login SPA (resources/js/login/login.js) and the profile page
+ * (resources/views/profile.blade.php) - CU-86ba2zp66 / sds/idp-mfa.md §4.10.3,
+ * §4.11 step 5: the UI must be able to warn the user when they've burned into
+ * their last few recovery codes, since those may be their only way back in.
+ *
+ * @package App\Services\Auth
+ */
+final class RecoveryCodesStatus
+{
+    public function __construct(
+        private readonly int $remaining,
+        private readonly int $total,
+        private readonly int $low_threshold,
+    ) {}
+
+    public function getRemaining(): int
+    {
+        return $this->remaining;
+    }
+
+    public function getTotal(): int
+    {
+        return $this->total;
+    }
+
+    public function getLowThreshold(): int
+    {
+        return $this->low_threshold;
+    }
+
+    /**
+     * @return array<string,int>
+     */
+    public function toArray(): array
+    {
+        return [
+            'recovery_codes_remaining'     => $this->remaining,
+            'recovery_codes_total'         => $this->total,
+            'recovery_codes_low_threshold' => $this->low_threshold,
+        ];
+    }
+}

app/Services/Auth/TwoFactorRateLimitService.php

@@ -94,6 +94,6 @@ private function limitsFor(string $action): array
      */
     private function cacheKey(string $action, string|int $subject): string
     {
-        return sprintf('2fa_rate:%s:%s', $action, $subject);
+        return sprintf('%s%s:%s', self::RATE_LIMIT_CACHE_KEY_PREFIX, $action, $subject);
     }
 }

app/Strategies/ILoginStrategy.php

@@ -1,4 +1,7 @@
 <?php namespace Strategies;
+
+use Auth\MFAConstants;
+
 /**
  * Interface ILoginStrategy
  * @package Strategies
@@ -9,7 +12,7 @@ interface ILoginStrategy
      * error_code returned by challengeRequired() when factor 1 passed but a
      * 2FA challenge is pending.
      */
-    const MFA_REQUIRED = 'mfa_required';
+    const MFA_REQUIRED = MFAConstants::ERROR_CODE_MFA_REQUIRED;
 
     /**
      * @return mixed

app/Strategies/MFA/AbstractMFAChallengeStrategy.php

@@ -1,6 +1,7 @@
 <?php namespace Strategies\MFA;
 
 use Auth\Exceptions\AuthenticationException;
+use Auth\MFAConstants;
 use Auth\Repositories\IUserRecoveryCodeRepository;
 use Auth\User;
 use Illuminate\Support\Facades\Hash;
@@ -10,14 +11,14 @@
 abstract class AbstractMFAChallengeStrategy implements IMFAChallengeStrategy
 {
     private const SESSION_TTL           = 300;
-    private const KEY_USER_ID           = '2fa_pending_user_id';
-    private const KEY_PENDING_AT        = '2fa_pending_at';
-    private const KEY_REMEMBER          = '2fa_remember';
-    private const KEY_RECOVERY_ATTEMPTS = '2fa_recovery_attempts';
+    private const KEY_USER_ID           = MFAConstants::SESSION_KEY_PENDING_USER_ID;
+    private const KEY_PENDING_AT        = MFAConstants::SESSION_KEY_PENDING_AT;
+    private const KEY_REMEMBER          = MFAConstants::SESSION_KEY_REMEMBER;
+    private const KEY_RECOVERY_ATTEMPTS = MFAConstants::SESSION_KEY_RECOVERY_ATTEMPTS;
 
     public function __construct(protected IUserRecoveryCodeRepository $recovery_code_repository) {}
 
-    public function getPendingState(): ?array
+    public function getPendingState(): ?MFAPendingState
     {
         $user_id    = Session::get(self::KEY_USER_ID);
         $pending_at = Session::get(self::KEY_PENDING_AT);
@@ -31,11 +32,11 @@ public function getPendingState(): ?array
             return null;
         }
 
-        return [
-            'user_id'    => $user_id,
-            'pending_at' => $pending_at,
-            'remember'   => Session::get(self::KEY_REMEMBER, false),
-        ];
+        return new MFAPendingState(
+            (int) $user_id,
+            (int) $pending_at,
+            (bool) Session::get(self::KEY_REMEMBER, false)
+        );
     }
 
     public function clearPendingState(): void

app/Strategies/MFA/IMFAChallengeStrategy.php

@@ -8,7 +8,7 @@ interface IMFAChallengeStrategy
     public function issueChallenge(User $user, ?Client $client, bool $remember): array;
     public function verifyChallenge(User $user, string $code, ?Client $client = null): void;
     public function resendChallenge(User $user, ?Client $client, bool $remember): array;
-    public function getPendingState(): ?array;
+    public function getPendingState(): ?MFAPendingState;
     public function clearPendingState(): void;
     public function verifyRecoveryCode(User $user, string $code): void;
 }