From 8eb854f9d25e29f7028b92e8239e0267fe04efc3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 15:53:50 -0300 Subject: [PATCH 01/13] 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. --- app/Http/Controllers/UserController.php | 8 ++++++ tests/TwoFactorLoginFlowTest.php | 34 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 97a54674..7fc2e290 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -885,6 +885,14 @@ public function verify2FARecovery() 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) { diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index cff0ff0f..33dbb17b 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -33,6 +33,9 @@ use Auth\Repositories\IUserRecoveryCodeRepository; use LaravelDoctrine\ORM\Facades\EntityManager; use Models\OAuth2\Client; +use OAuth2\OAuth2Protocol; +use OAuth2\Requests\OAuth2RequestMemento; +use OAuth2\Services\IMementoOAuth2SerializerService; use Services\OAuth2\PrincipalService; use Strategies\ILoginStrategy; use Strategies\MFA\IMFAChallengeStrategy; @@ -873,6 +876,37 @@ public function testUsedRecoveryCodeFails(): void $this->assertFalse(Auth::check()); } + public function testRecoveryWithStaleOAuth2ClientFailsBeforeBurningCode(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERYSTALE789'; + $codeId = $this->createRecoveryCode($admin, $plain, false); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + // A pending OAuth2 authorization request whose client no longer exists + // (e.g. deleted mid-login). verify2FA() fails this via + // resolveClientFromMemento() BEFORE redeeming the OTP; recovery must + // apply the same guard instead of burning the single-use code and + // establishing a session for a doomed authorization request. + App::make(IMementoOAuth2SerializerService::class)->serialize( + OAuth2RequestMemento::buildFromState([ + OAuth2Protocol::OAuth2Protocol_ResponseType => OAuth2Protocol::OAuth2Protocol_ResponseType_Code, + OAuth2Protocol::OAuth2Protocol_ClientId => 'stale-client-' . uniqid(), + OAuth2Protocol::OAuth2Protocol_RedirectUri => 'https://client.invalid/callback', + ]) + ); + + $response = $this->recovery($plain); + + $this->assertResponseStatus(412); + $this->assertFalse(Auth::check(), 'no session must be established when the pending OAuth2 client cannot be resolved'); + + EntityManager::clear(); + $code = EntityManager::find(UserRecoveryCode::class, $codeId); + $this->assertFalse($code->isUsed(), 'the recovery code must NOT be burned when the pending OAuth2 request points at a non-existent client'); + } + // ------------------------------------------------------------------------- // resend // ------------------------------------------------------------------------- From 77264d5a37403b606e3f00fb3cfbd61d9d5ba7a4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 15:57:55 -0300 Subject: [PATCH 02/13] 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. --- app/Http/Controllers/UserController.php | 7 ++-- .../TwoFactorRateLimitMiddleware.php | 5 +-- app/libs/Auth/MFAConstants.php | 32 +++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 app/libs/Auth/MFAConstants.php diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 7fc2e290..50754efe 100644 --- a/app/Http/Controllers/UserController.php +++ b/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; @@ -799,7 +800,7 @@ 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. @@ -911,7 +912,7 @@ 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']); @@ -1029,7 +1030,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]); } /** diff --git a/app/Http/Middleware/TwoFactorRateLimitMiddleware.php b/app/Http/Middleware/TwoFactorRateLimitMiddleware.php index e197735b..26e8d7e9 100644 --- a/app/Http/Middleware/TwoFactorRateLimitMiddleware.php +++ b/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) diff --git a/app/libs/Auth/MFAConstants.php b/app/libs/Auth/MFAConstants.php new file mode 100644 index 00000000..4fa67abd --- /dev/null +++ b/app/libs/Auth/MFAConstants.php @@ -0,0 +1,32 @@ + Date: Tue, 11 Aug 2026 16:04:09 -0300 Subject: [PATCH 03/13] 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. --- app/Http/Controllers/UserController.php | 16 +++--- .../MFA/AbstractMFAChallengeStrategy.php | 12 ++--- app/Strategies/MFA/IMFAChallengeStrategy.php | 2 +- app/Strategies/MFA/MFAPendingState.php | 49 +++++++++++++++++++ tests/TwoFactorLoginFlowTest.php | 3 +- .../MFA/AbstractMFAChallengeStrategyTest.php | 6 +-- 6 files changed, 69 insertions(+), 19 deletions(-) create mode 100644 app/Strategies/MFA/MFAPendingState.php diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 50754efe..75e36f52 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -763,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(); @@ -785,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 @@ -804,7 +804,7 @@ public function verify2FA() } // 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 @@ -880,7 +880,7 @@ 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(); @@ -899,7 +899,7 @@ public function verify2FARecovery() } 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 { @@ -915,7 +915,7 @@ public function verify2FARecovery() 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(); @@ -978,13 +978,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, diff --git a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php index 95da987e..2ecce5ae 100644 --- a/app/Strategies/MFA/AbstractMFAChallengeStrategy.php +++ b/app/Strategies/MFA/AbstractMFAChallengeStrategy.php @@ -17,7 +17,7 @@ abstract class AbstractMFAChallengeStrategy implements IMFAChallengeStrategy 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 +31,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 diff --git a/app/Strategies/MFA/IMFAChallengeStrategy.php b/app/Strategies/MFA/IMFAChallengeStrategy.php index c395551d..ddc34dc7 100644 --- a/app/Strategies/MFA/IMFAChallengeStrategy.php +++ b/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; } diff --git a/app/Strategies/MFA/MFAPendingState.php b/app/Strategies/MFA/MFAPendingState.php new file mode 100644 index 00000000..1ff41451 --- /dev/null +++ b/app/Strategies/MFA/MFAPendingState.php @@ -0,0 +1,49 @@ +user_id; + } + + /** + * Unix timestamp of when the challenge was issued. + */ + public function getPendingAt(): int + { + return $this->pending_at; + } + + /** + * Whether the original login submission asked for a remembered session. + */ + public function shouldRemember(): bool + { + return $this->remember; + } +} diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 33dbb17b..1a980a05 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -40,6 +40,7 @@ use Strategies\ILoginStrategy; use Strategies\MFA\IMFAChallengeStrategy; use Strategies\MFA\MFAChallengeStrategyFactory; +use Strategies\MFA\MFAPendingState; use Utils\Services\IAuthService; /** @@ -541,7 +542,7 @@ public function resendChallenge(User $user, ?Client $client, bool $remember): ar return $this->inner->resendChallenge($user, $client, $remember); } - public function getPendingState(): ?array + public function getPendingState(): ?MFAPendingState { return $this->inner->getPendingState(); } diff --git a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php index afa2432b..211d8261 100644 --- a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -53,9 +53,9 @@ public function testGetPendingState_withValidSession_returnsState(): void $state = $this->strategy->getPendingState(); $this->assertNotNull($state); - $this->assertSame(42, $state['user_id']); - $this->assertTrue($state['remember']); - $this->assertArrayHasKey('pending_at', $state); + $this->assertSame(42, $state->getUserId()); + $this->assertTrue($state->shouldRemember()); + $this->assertGreaterThan(0, $state->getPendingAt()); } public function testGetPendingState_withExpiredSession_returnsNull(): void From e35415986b4d41eaa19590d31a2e8dc5d79a7586 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 16:11:29 -0300 Subject: [PATCH 04/13] 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). --- app/Http/Controllers/UserController.php | 23 ++++----- app/Services/Auth/IRecoveryCodeService.php | 6 +++ app/Services/Auth/RecoveryCodeService.php | 12 +++++ app/Services/Auth/RecoveryCodesStatus.php | 60 ++++++++++++++++++++++ tests/TwoFactorLoginFlowTest.php | 7 +++ 5 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 app/Services/Auth/RecoveryCodesStatus.php diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 75e36f52..fdd3b482 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -934,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()); @@ -1200,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()), @@ -1209,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) diff --git a/app/Services/Auth/IRecoveryCodeService.php b/app/Services/Auth/IRecoveryCodeService.php index 51df966b..ab6de16e 100644 --- a/app/Services/Auth/IRecoveryCodeService.php +++ b/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; } diff --git a/app/Services/Auth/RecoveryCodeService.php b/app/Services/Auth/RecoveryCodeService.php index 1abaeb9c..a626dd82 100644 --- a/app/Services/Auth/RecoveryCodeService.php +++ b/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) + ); + } } diff --git a/app/Services/Auth/RecoveryCodesStatus.php b/app/Services/Auth/RecoveryCodesStatus.php new file mode 100644 index 00000000..c99e9f2c --- /dev/null +++ b/app/Services/Auth/RecoveryCodesStatus.php @@ -0,0 +1,60 @@ +remaining; + } + + public function getTotal(): int + { + return $this->total; + } + + public function getLowThreshold(): int + { + return $this->low_threshold; + } + + /** + * @return array + */ + public function toArray(): array + { + return [ + 'recovery_codes_remaining' => $this->remaining, + 'recovery_codes_total' => $this->total, + 'recovery_codes_low_threshold' => $this->low_threshold, + ]; + } +} diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 1a980a05..08145d04 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -856,6 +856,13 @@ public function testRecoveryCodeLoginSucceeds(): void $this->assertIsString($payload['redirect_url'] ?? null); $this->assertTrue(Auth::check()); + // Wire contract consumed by login.js's low-recovery-codes warning + // (RecoveryCodesStatus::toArray()) - expected values come from config, + // not from re-deriving the service's own math. + $this->assertIsInt($payload['recovery_codes_remaining'] ?? null); + $this->assertSame((int) Config::get('auth.recovery_codes.count'), $payload['recovery_codes_total'] ?? null); + $this->assertSame((int) Config::get('auth.recovery_codes.low_threshold'), $payload['recovery_codes_low_threshold'] ?? null); + EntityManager::clear(); $code = EntityManager::find(UserRecoveryCode::class, $codeId); $this->assertTrue($code->isUsed(), 'the recovery code must be marked used'); From 472e2907b30edbe044983d383057e6e7c0a9369b Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 16:21:10 -0300 Subject: [PATCH 05/13] 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. --- tests/TwoFactorLoginFlowTest.php | 112 +++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 08145d04..1b9c733e 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -41,6 +41,7 @@ use Strategies\MFA\IMFAChallengeStrategy; use Strategies\MFA\MFAChallengeStrategyFactory; use Strategies\MFA\MFAPendingState; +use Illuminate\Support\Facades\URL; use Utils\Services\IAuthService; /** @@ -915,6 +916,117 @@ public function testRecoveryWithStaleOAuth2ClientFailsBeforeBurningCode(): void $this->assertFalse($code->isUsed(), 'the recovery code must NOT be burned when the pending OAuth2 request points at a non-existent client'); } + // ------------------------------------------------------------------------- + // full OIDC circuit: authorize -> login -> MFA -> consent -> auth code + // ------------------------------------------------------------------------- + + private const OIDC_CLIENT_ID = '.-_~87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x.openstack.client'; + private const OIDC_REDIRECT_URI = 'https://www.test.com/oauth2'; + + public function testFullOIDCFlowWithMFAChallengeAndConsentDeliversAuthCode(): void + { + $this->startOIDCFlowUpToChallenge(); + + $response = $this->verify($this->latestOtpCode(self::ADMIN_EMAIL)); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertTrue(Auth::check(), 'second factor verified - session must be established'); + $this->assertSame( + URL::action('OAuth2\OAuth2ProviderController@auth'), + $payload['redirect_url'] ?? null, + 'the XHR must be told to navigate back to the authorization endpoint so the pending OIDC request resumes' + ); + + $this->completeConsentAndGetAuthCode(); + } + + public function testFullOIDCFlowWithRecoveryCodeAndConsentDeliversAuthCode(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERYOIDC321'; + $this->createRecoveryCode($admin, $plain, false); + + $this->startOIDCFlowUpToChallenge(); + + $response = $this->recovery($plain); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertTrue(Auth::check(), 'recovery code verified - session must be established'); + $this->assertSame( + URL::action('OAuth2\OAuth2ProviderController@auth'), + $payload['redirect_url'] ?? null, + 'the XHR must be told to navigate back to the authorization endpoint so the pending OIDC request resumes' + ); + + $this->completeConsentAndGetAuthCode(); + } + + /** + * Starts an OIDC authorization-code request and walks it up to the MFA + * challenge: authorize -> redirected to login -> password accepted -> + * challenge issued, no session yet. The OAuth2 memento is serialized by + * the authorize endpoint, so the whole login leg runs under the + * OAuth2LoginStrategy, client-scoped OTP included. + */ + private function startOIDCFlowUpToChallenge(): void + { + $response = $this->action('POST', 'OAuth2\OAuth2ProviderController@auth', [ + 'client_id' => self::OIDC_CLIENT_ID, + 'redirect_uri' => self::OIDC_REDIRECT_URI, + 'response_type' => 'code', + 'scope' => 'openid profile email', + ]); + $this->assertResponseStatus(302); + $this->assertTrue( + str_contains($response->getTargetUrl(), '/login'), + 'an unauthenticated OIDC request must bounce to the login screen' + ); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $this->assertResponseStatus(302); + $this->assertFalse(Auth::check(), 'password alone must not establish a session while MFA is pending'); + } + + /** + * Walks the consent leg (first authorization for this client, so consent is + * required) and asserts the authorization code is delivered to the client's + * redirect_uri. + */ + private function completeConsentAndGetAuthCode(): void + { + // The top-level navigation the SPA performs with redirect_url: the auth + // endpoint rebuilds the authorization request from the session memento. + $response = $this->action('GET', 'OAuth2\OAuth2ProviderController@auth'); + $this->assertResponseStatus(302); + $this->assertSame( + URL::action('UserController@getConsent'), + $response->getTargetUrl(), + 'first authorization for this client must land on the consent screen' + ); + + $this->action('GET', 'UserController@getConsent'); + $this->assertResponseStatus(200); + + $this->action('POST', 'UserController@postConsent', [ + 'trust' => IAuthService::AuthorizationResponse_AllowOnce, + '_token' => Session::token(), + ]); + $this->assertResponseStatus(302); + + $response = $this->action('GET', 'OAuth2\OAuth2ProviderController@auth'); + $this->assertResponseStatus(302); + + $url = $response->getTargetUrl(); + $this->assertTrue( + str_starts_with($url, self::OIDC_REDIRECT_URI), + "the final hop must deliver to the client redirect_uri, got: {$url}" + ); + parse_str(parse_url($url, PHP_URL_QUERY) ?? '', $query); + $this->assertNotEmpty($query['code'] ?? null, 'an authorization code must be delivered to the client'); + } + // ------------------------------------------------------------------------- // resend // ------------------------------------------------------------------------- From b7d210c2d4ce8bbc7a0777684152dda48d8b6c51 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 16:37:20 -0300 Subject: [PATCH 06/13] test(oidc): repair OIDCProtocolTestCase login circuits (stale password + MFA gate) Two stacked breakages, both predating and unrelated to each individual test: - 021bee3d (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() (4864f50a / #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. --- tests/OIDCProtocolTestCase.php | 58 +++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/tests/OIDCProtocolTestCase.php b/tests/OIDCProtocolTestCase.php index fd95a2c7..58f2c68f 100644 --- a/tests/OIDCProtocolTestCase.php +++ b/tests/OIDCProtocolTestCase.php @@ -55,6 +55,12 @@ protected function prepareForTests():void parent::prepareForTests(); App::singleton(UtilsServiceCatalog::ServerConfigurationService, StubServerConfigurationService::class); $this->current_realm = Config::get('app.url'); + // This class exercises the OIDC/OAuth2 protocol, not the MFA gate: the + // seeded login user belongs to SuperAdminGroup (enforced by default), + // and every password login leg here would otherwise stop at the 2FA + // challenge. The gate itself is covered by TwoFactorLoginFlowTest, + // including the full authorize -> MFA -> consent -> code circuit. + Config::set('two_factor.enforced_groups', []); Session::start(); } @@ -126,7 +132,7 @@ public function testLoginWithTrailingSpace() $response = $this->action('POST', "UserController@postLogin", [ 'username' => ' sebastian@tipit.net ', - 'password' => ' 1qaz2wsx ', + 'password' => ' 1Qaz2wsx! ', '_token' => Session::token(), 'flow' => 'password', ] @@ -172,7 +178,7 @@ public function testConsentPrompt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -259,7 +265,7 @@ public function testConsentLogin() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -341,7 +347,7 @@ public function testAuthCode() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -426,7 +432,7 @@ public function testAuthCodeIDN() array ( 'username' => 'hei@やる.ca', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -539,7 +545,7 @@ public function testAuthCodeOpenIdScopeOnly() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -623,7 +629,7 @@ public function testMaxAge1AndWait2() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -687,7 +693,7 @@ public function testToken array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -849,7 +855,7 @@ public function testTokenSeveralScopes array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -997,7 +1003,7 @@ public function testGetRefreshTokenWithPromptSetToConsentLogin() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -1140,7 +1146,7 @@ public function testFlowNativeDisplay() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => $json_response['required_params_valid_values']["_token"] ) @@ -1259,7 +1265,7 @@ public function testGetRefreshTokenFromNativeAppNTimes($n = 5) array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -1457,7 +1463,7 @@ public function testTokenResponseModePost() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -1604,7 +1610,7 @@ public function testNativeClientBasicAuth() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -1743,7 +1749,7 @@ public function testClientAuthenticationClientSecretJwt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -1922,7 +1928,7 @@ public function testClientAuthenticationPrivateKeyJwt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2074,7 +2080,7 @@ public function testImplicitFlowTokenIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2155,7 +2161,7 @@ public function testImplicitFlowIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2240,7 +2246,7 @@ public function testImplicitFlowIdTokenMaxAge1000() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2350,7 +2356,7 @@ public function testImplicitFlowAccessToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2487,7 +2493,7 @@ public function testImplicitFlowResponseModePost() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2657,7 +2663,7 @@ public function testHybridFlowCodeIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2764,7 +2770,7 @@ public function testHybridFlowCodeIdTokenIdTokenHint() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -2985,7 +2991,7 @@ public function testHybridFlowCodeAccessToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -3103,7 +3109,7 @@ public function testHybridFlowCodeAccessTokenIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) @@ -3210,7 +3216,7 @@ public function testTryingAuthCodeTwice() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1qaz2wsx', + 'password' => '1Qaz2wsx!', 'flow' => 'password', '_token' => Session::token() ) From 6e16fb214a09207f3d6ebc9dff5d3d905675d13c Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 16:48:30 -0300 Subject: [PATCH 07/13] 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. --- app/libs/Auth/AuthService.php | 8 +++ tests/unit/AuthServiceLogoutTest.php | 79 ++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 tests/unit/AuthServiceLogoutTest.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 665956d7..f90af6cd 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -504,8 +504,16 @@ public function logout(bool $clear_security_ctx = true): void // Flush all session data and regenerate the session ID to ensure no stale // data survives (OAuth2 memento, OpenID auth context, authorization responses, etc.) + // The flush also wipes the session-backed security context, so when the + // caller asked to keep it (clear_security_ctx = false - the prompt=login + // re-authentication path, which needs the requested-user id to show the + // login hint on the login screen) it is captured first and re-saved + // after the session ID is regenerated. + $preserved_security_ctx = $clear_security_ctx ? null : $this->security_context_service->get(); Session::flush(); Session::regenerate(); + if (!is_null($preserved_security_ctx)) + $this->security_context_service->save($preserved_security_ctx); } public function invalidateSession(): void diff --git a/tests/unit/AuthServiceLogoutTest.php b/tests/unit/AuthServiceLogoutTest.php new file mode 100644 index 00000000..25366224 --- /dev/null +++ b/tests/unit/AuthServiceLogoutTest.php @@ -0,0 +1,79 @@ +save( + (new SecurityContext) + ->setRequestedUserId(self::REQUESTED_USER_ID) + ->setAuthTimeRequired(true) + ); + } + + public function testLogoutPreservingSecurityContext_survivesSessionFlush(): void + { + $this->saveSecurityContext(); + Session::put('unrelated_key', 'value'); + + App::make(IAuthService::class)->logout(false); + + $ctx = App::make(ISecurityContextService::class)->get(); + $this->assertSame( + self::REQUESTED_USER_ID, + $ctx->getRequestedUserId(), + 'logout(clear_security_ctx: false) must preserve the security context across the session flush' + ); + $this->assertTrue($ctx->isAuthTimeRequired()); + // The flush hardening itself must still hold for everything else. + $this->assertNull(Session::get('unrelated_key'), 'all other session data must still be flushed on logout'); + } + + public function testLogoutClearingSecurityContext_removesIt(): void + { + $this->saveSecurityContext(); + + App::make(IAuthService::class)->logout(true); + + $ctx = App::make(ISecurityContextService::class)->get(); + $this->assertNull($ctx->getRequestedUserId(), 'logout(clear_security_ctx: true) must clear the security context'); + } +} From e3d8b4107a7e79cdbdcc7794a3a66b93b369fb54 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 16:55:18 -0300 Subject: [PATCH 08/13] 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. --- tests/OIDCProtocolTestCase.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/OIDCProtocolTestCase.php b/tests/OIDCProtocolTestCase.php index 58f2c68f..98d2a2c7 100644 --- a/tests/OIDCProtocolTestCase.php +++ b/tests/OIDCProtocolTestCase.php @@ -1437,7 +1437,11 @@ public function testTokenResponseModePost() OAuth2Protocol::OfflineAccess_Scope), OAuth2Protocol::OAuth2Protocol_LoginHint => 'sebastian@tipit.net', OAuth2Protocol::OAuth2Protocol_Prompt => OAuth2Protocol::OAuth2Protocol_Prompt_Consent, - OAuth2Protocol::OAuth2Protocol_MaxAge => 1, + // 3200 like the sibling circuits: this 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 forces a re-login instead of the form post. + OAuth2Protocol::OAuth2Protocol_MaxAge => 3200, OAuth2Protocol::OAuth2Protocol_ResponseMode => OAuth2Protocol::OAuth2Protocol_ResponseMode_FormPost ); From 235d64916b3c1d0839cfc23d97f3997119ff3db5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 17:18:23 -0300 Subject: [PATCH 09/13] 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). --- tests/TwoFactorLoginFlowTest.php | 164 +++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 1b9c733e..bde41a26 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -963,6 +963,170 @@ public function testFullOIDCFlowWithRecoveryCodeAndConsentDeliversAuthCode(): vo $this->completeConsentAndGetAuthCode(); } + public function testOIDCFlowWrongRecoveryCodeThenCorrectCompletesCircuit(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERYRETRY111'; + $this->createRecoveryCode($admin, $plain, false); + + $this->startOIDCFlowUpToChallenge(); + + // Wrong code: rejected without killing the pending challenge or the + // OAuth2 memento - the user must be able to retry within the same flow. + $response = $this->recovery('WRONGCODE000'); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a rejected recovery code must not establish a session'); + + // Correct code on the retry: the same OIDC flow completes end to end. + $response = $this->recovery($plain); + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertTrue(Auth::check()); + $this->assertSame(URL::action('OAuth2\OAuth2ProviderController@auth'), $payload['redirect_url'] ?? null); + + $this->completeConsentAndGetAuthCode(); + } + + public function testOIDCFlowConsecutiveWrongRecoveryCodesHitRateLimit(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERYLIMIT222'; + $this->createRecoveryCode($admin, $plain, false); + + $this->startOIDCFlowUpToChallenge(); + + $max = (int) Config::get('two_factor.rate_limit.max_attempts'); + for ($i = 0; $i < $max; $i++) { + $response = $this->recovery('WRONGCODE' . $i); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + + // Threshold reached: even the CORRECT code is rejected while the + // window lasts, still without a session - brute-forcing recovery codes + // inside a pending OIDC flow cannot buy extra attempts. + $response = $this->recovery($plain); + $this->assertResponseStatus(429); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a rate-limited attempt must not establish a session even with a valid code'); + } + + public function testOIDCFlowBurnedRecoveryCodeFailsThenFreshCodeCompletesCircuit(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $burned = 'RECOVERYBURNED33'; + $fresh = 'RECOVERYFRESH444'; + $this->createRecoveryCode($admin, $burned, true); // already used + $freshId = $this->createRecoveryCode($admin, $fresh, false); + + $this->startOIDCFlowUpToChallenge(); + + // A burned (single-use, already redeemed) code is rejected like any + // other invalid code. + $response = $this->recovery($burned); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a burned recovery code must not establish a session'); + + // A fresh code still completes the same OIDC flow afterwards. + $response = $this->recovery($fresh); + $this->assertResponseStatus(200); + $this->assertTrue(Auth::check()); + + EntityManager::clear(); + $code = EntityManager::find(UserRecoveryCode::class, $freshId); + $this->assertTrue($code->isUsed(), 'the fresh recovery code must be marked used'); + + $this->completeConsentAndGetAuthCode(); + } + + public function testOIDCFlowWrongOTPThenCorrectCompletesCircuit(): void + { + $this->startOIDCFlowUpToChallenge(); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + // Wrong OTP: rejected without killing the pending challenge or the + // OAuth2 memento - the user must be able to retry within the same flow. + $response = $this->verify('000000'); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a rejected OTP must not establish a session'); + + // Correct OTP on the retry: the same OIDC flow completes end to end. + $response = $this->verify($code); + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertTrue(Auth::check()); + $this->assertSame(URL::action('OAuth2\OAuth2ProviderController@auth'), $payload['redirect_url'] ?? null); + + $this->completeConsentAndGetAuthCode(); + } + + public function testOIDCFlowConsecutiveWrongOTPsHitRateLimit(): void + { + $this->startOIDCFlowUpToChallenge(); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + $max = (int) Config::get('two_factor.rate_limit.max_attempts'); + for ($i = 0; $i < $max; $i++) { + $response = $this->verify('00000' . $i); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + + // Threshold reached: even the CORRECT code is rejected while the + // window lasts, still without a session - brute-forcing the OTP inside + // a pending OIDC flow cannot buy extra attempts. + $response = $this->verify($code); + $this->assertResponseStatus(429); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a rate-limited attempt must not establish a session even with a valid code'); + } + + public function testOIDCFlowRedeemedOTPFailsThenResendCompletesCircuit(): void + { + $this->startOIDCFlowUpToChallenge(); + $burned = $this->latestOtpCode(self::ADMIN_EMAIL); + + // Burn the issued OTP directly (single-use, already redeemed) - the + // OTP analog of an already-used recovery code. + /** @var IOAuth2OTPRepository $otpRepo */ + $otpRepo = App::make(IOAuth2OTPRepository::class); + $otp = $otpRepo->getByValue($burned); + $this->assertNotNull($otp); + $otp->redeem(); + EntityManager::persist($otp); + EntityManager::flush(); + + $response = $this->verify($burned); + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertFalse(Auth::check(), 'a redeemed OTP must not establish a session'); + + // Resend issues a fresh code; the same OIDC flow completes with it. + $this->resend(); + $this->assertResponseStatus(200); + $fresh = $this->latestOtpCode(self::ADMIN_EMAIL); + $this->assertNotSame($burned, $fresh); + + $response = $this->verify($fresh); + $this->assertResponseStatus(200); + $this->assertTrue(Auth::check()); + + $this->completeConsentAndGetAuthCode(); + } + /** * Starts an OIDC authorization-code request and walks it up to the MFA * challenge: authorize -> redirected to login -> password accepted -> From 667251667900547ba6cdc7fc894b0f2821ffd3d9 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 17:37:01 -0300 Subject: [PATCH 10/13] 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. --- tests/TwoFactorLoginFlowTest.php | 142 +++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index bde41a26..7af45bb4 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -916,6 +916,148 @@ public function testRecoveryWithStaleOAuth2ClientFailsBeforeBurningCode(): void $this->assertFalse($code->isUsed(), 'the recovery code must NOT be burned when the pending OAuth2 request points at a non-existent client'); } + // ------------------------------------------------------------------------- + // branch coverage: validator 412s, vanished pending user, expired session, + // stale OAuth2 client on verify2FA, audit failure on the FAILED-verify path + // ------------------------------------------------------------------------- + + public function testVerifyValidatorRejectsMalformedRequest(): void + { + $response = $this->action('POST', 'UserController@verify2FA', [ + 'method' => 'bogus-method', + '_token' => Session::token(), + ]); + + $this->assertResponseStatus(412); + $this->assertFalse(Auth::check()); + } + + public function testRecoveryValidatorRejectsMalformedRequest(): void + { + $response = $this->action('POST', 'UserController@verify2FARecovery', [ + '_token' => Session::token(), + ]); + + $this->assertResponseStatus(412); + $this->assertFalse(Auth::check()); + } + + public function testVerifyWithVanishedPendingUserFailsAsExpiredSession(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + // The pending user disappeared between challenge and verification + // (e.g. deleted account) - must clear the pending state, not 500. + Session::put('2fa_pending_user_id', PHP_INT_MAX); + + $response = $this->verify('123456'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertFalse(Auth::check()); + $this->assertNull(Session::get('2fa_pending_user_id'), 'the orphaned pending state must be cleared'); + } + + public function testRecoveryWithoutPendingChallengeFailsAsExpiredSession(): void + { + // No prior postLogin -> no pending state. + $response = $this->recovery('ANYCODE123'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + + public function testRecoveryWithVanishedPendingUserFailsAsExpiredSession(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + Session::put('2fa_pending_user_id', PHP_INT_MAX); + + $response = $this->recovery('ANYCODE123'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertFalse(Auth::check()); + $this->assertNull(Session::get('2fa_pending_user_id'), 'the orphaned pending state must be cleared'); + } + + public function testVerifyWithStaleOAuth2ClientFailsBeforeRedeemingOTP(): void + { + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $code = $this->latestOtpCode(self::ADMIN_EMAIL); + + // Same guard already proven for recovery: a pending OAuth2 request + // whose client no longer exists must fail BEFORE the OTP is redeemed. + App::make(IMementoOAuth2SerializerService::class)->serialize( + OAuth2RequestMemento::buildFromState([ + OAuth2Protocol::OAuth2Protocol_ResponseType => OAuth2Protocol::OAuth2Protocol_ResponseType_Code, + OAuth2Protocol::OAuth2Protocol_ClientId => 'stale-client-' . uniqid(), + OAuth2Protocol::OAuth2Protocol_RedirectUri => 'https://client.invalid/callback', + ]) + ); + + $response = $this->verify($code); + + $this->assertResponseStatus(412); + $this->assertFalse(Auth::check(), 'no session must be established when the pending OAuth2 client cannot be resolved'); + + /** @var IOAuth2OTPRepository $otpRepo */ + $otpRepo = App::make(IOAuth2OTPRepository::class); + EntityManager::clear(); + $otp = $otpRepo->getByValue($code); + $this->assertNotNull($otp); + $this->assertFalse($otp->isRedeemed(), 'the OTP must NOT be redeemed when the pending OAuth2 request points at a non-existent client'); + } + + public function testFailedVerifyAuditFailureStillReturnsClean401(): void + { + // Audit is best-effort on the FAILED path too: a failure emitting + // challenge_failed must not turn the clean 401 (whose error_code the + // rate-limit middleware keys on) into a 500. + $auditMock = \Mockery::mock(ITwoFactorAuditService::class); + $auditMock->shouldReceive('log') + ->andReturnUsing(function (User $user, string $eventType) { + if ($eventType === TwoFactorAuditLog::EventChallengeFailed) { + throw new \Exception('audit sink unavailable'); + } + }); + $this->app->instance(ITwoFactorAuditService::class, $auditMock); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->verify('000000'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + + public function testFailedRecoveryAuditFailureStillReturnsClean401(): void + { + $auditMock = \Mockery::mock(ITwoFactorAuditService::class); + $auditMock->shouldReceive('log') + ->andReturnUsing(function (User $user, string $eventType) { + if ($eventType === TwoFactorAuditLog::EventChallengeFailed) { + throw new \Exception('audit sink unavailable'); + } + }); + $this->app->instance(ITwoFactorAuditService::class, $auditMock); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + + $response = $this->recovery('WRONGCODE999'); + + $this->assertResponseStatus(401); + $payload = json_decode($response->getContent(), true); + $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertFalse(Auth::check()); + } + // ------------------------------------------------------------------------- // full OIDC circuit: authorize -> login -> MFA -> consent -> auth code // ------------------------------------------------------------------------- From 6f1bb9f580b1fd747d2d8ef62e022bf0a6d6890f Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 18:12:14 -0300 Subject: [PATCH 11/13] test(ci): actually run the protocol TestCase suites, stop hiding failure breadth Two changes to phpunit.xml: - The Application suite's 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). --- phpunit.xml | 10 +++++++++- tests/OAuth2ProtocolTestCase.php | 7 +++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/phpunit.xml b/phpunit.xml index 1f73569a..04e8bd17 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -3,7 +3,7 @@ backupGlobals="false" colors="true" processIsolation="false" - stopOnFailure="true" + stopOnFailure="false" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/11.2/phpunit.xsd" cacheDirectory=".phpunit.cache" backupStaticProperties="false" @@ -13,6 +13,14 @@ ./tests/ ./tests/OpenTelemetry/ ./tests/TestCase.php + + ./tests/OAuth2ProtocolTestCase.php + ./tests/OIDCProtocolTestCase.php + ./tests/OIDCPasswordlessTestCase.php + ./tests/OpenIdProtocolTestCase.php ./tests/OpenTelemetry/ diff --git a/tests/OAuth2ProtocolTestCase.php b/tests/OAuth2ProtocolTestCase.php index d0a72b9f..2e9604c2 100644 --- a/tests/OAuth2ProtocolTestCase.php +++ b/tests/OAuth2ProtocolTestCase.php @@ -459,6 +459,13 @@ public function testResourceServerIntrospectionNotValidIP() { $access_token = $this->testValidateToken(); + // The resource-server IP check became opt-in in #98 + // (oauth2.validate_resource_server_ip, default off) - this test is + // about the rejection itself, so turn the flag on. Set AFTER + // testValidateToken(): that helper introspects from resource server 1, + // whose registered IPs do include the test-request IP. + Config::set('oauth2.validate_resource_server_ip', true); + $client_id = 'resource.server.2.openstack.client'; $client_secret = '123456789123456789123456789123456789123456789'; //do token validation .... From 9e25bf67ac64d7a60a9b00076cb1fff7a0e359ac Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 18:18:13 -0300 Subject: [PATCH 12/13] 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. --- .../Auth/ITwoFactorRateLimitService.php | 14 +++- .../Auth/TwoFactorRateLimitService.php | 2 +- app/Strategies/ILoginStrategy.php | 5 +- .../MFA/AbstractMFAChallengeStrategy.php | 9 +-- app/libs/Auth/MFAConstants.php | 26 +++++-- tests/TwoFactorLoginFlowTest.php | 72 ++++++++++--------- .../MFA/AbstractMFAChallengeStrategyTest.php | 25 +++---- .../MFA/EmailOTPMFAChallengeStrategyTest.php | 7 +- 8 files changed, 96 insertions(+), 64 deletions(-) diff --git a/app/Services/Auth/ITwoFactorRateLimitService.php b/app/Services/Auth/ITwoFactorRateLimitService.php index 47f1c587..0f2a0da2 100644 --- a/app/Services/Auth/ITwoFactorRateLimitService.php +++ b/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 diff --git a/app/Services/Auth/TwoFactorRateLimitService.php b/app/Services/Auth/TwoFactorRateLimitService.php index 13967462..df461192 100644 --- a/app/Services/Auth/TwoFactorRateLimitService.php +++ b/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); } } diff --git a/app/Strategies/ILoginStrategy.php b/app/Strategies/ILoginStrategy.php index 5894bc4d..7d895329 100644 --- a/app/Strategies/ILoginStrategy.php +++ b/app/Strategies/ILoginStrategy.php @@ -1,4 +1,7 @@ getByEmailOrName(self::ADMIN_EMAIL); if ($admin) { $userId = $admin->getId(); + $prefix = ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX; foreach (['verify', 'recovery', 'resend'] as $action) { - Cache::forget("2fa_rate:{$action}:{$userId}"); + Cache::forget("{$prefix}{$action}:{$userId}"); // RateLimiter::hit() also writes a companion ":timer" key holding // the window's reset timestamp - must be cleared too, or a stale // timer from an earlier test leaks into a later one for this // same fixed subject (self::ADMIN_EMAIL's user id). - Cache::forget("2fa_rate:{$action}:{$userId}:timer"); + Cache::forget("{$prefix}{$action}:{$userId}:timer"); } } // otp is keyed by the (lowercased) submitted email, not a user id - // clear every literal email this test class submits to that action. + $otpPrefix = ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX . ITwoFactorRateLimitService::ActionOtp . ':'; foreach ([self::ADMIN_EMAIL, 'someone-else@example.com'] as $email) { - Cache::forget('2fa_rate:otp:' . strtolower($email)); - Cache::forget('2fa_rate:otp:' . strtolower($email) . ':timer'); + Cache::forget($otpPrefix . strtolower($email)); + Cache::forget($otpPrefix . strtolower($email) . ':timer'); } } @@ -343,7 +347,7 @@ public function testCancelClearsUIStateAndPendingChallenge(): void $response = $this->verify($code); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_SESSION_EXPIRED, $payload['error_code']); $this->assertFalse(Auth::check(), 'a cancelled challenge must never establish a session'); } @@ -431,7 +435,7 @@ public function testFailedOTPVerificationReturnsErrorAndIncrementsCounter(): voi $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code']); $this->assertFalse(Auth::check()); $this->assertSame(1, (int) Cache::get('2fa_rate:verify:' . $userId, 0), 'verify counter must increment on failure'); @@ -460,7 +464,7 @@ public function testOTPVerificationRejectsWrongCode(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code'], + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code'], 'verifyChallenge must load the stored OTP and reject a non-matching value'); $this->assertFalse(Auth::check()); } @@ -480,7 +484,7 @@ public function testOTPCodeRejectsReuseAfterSuccessfulVerification(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code'], + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code'], 'a reused OTP must be rejected because the redemption was committed by the AuthService transaction'); } @@ -506,7 +510,7 @@ public function testRecoveryCodeRejectsReuseAfterTransactionCommit(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code'], + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code'], 'recovery code reuse must be rejected because used_at was committed via the AuthService transaction'); } @@ -688,7 +692,7 @@ public function testExpiredMFASessionFails(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_SESSION_EXPIRED, $payload['error_code']); } // ------------------------------------------------------------------------- @@ -836,7 +840,7 @@ public function testDeviceTrustFailureDoesNotBlockLogin(): void $this->assertEquals(200, $response->getStatusCode(), 'a best-effort device-trust failure must not fail the login'); $this->assertTrue(Auth::check(), 'session must be established despite the device-trust failure'); - $this->assertNull(Session::get('2fa_pending_user_id'), 'pending MFA state must be cleared even when device-trust enrollment fails'); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID), 'pending MFA state must be cleared even when device-trust enrollment fails'); } // ------------------------------------------------------------------------- @@ -881,7 +885,7 @@ public function testUsedRecoveryCodeFails(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -948,15 +952,15 @@ public function testVerifyWithVanishedPendingUserFailsAsExpiredSession(): void // The pending user disappeared between challenge and verification // (e.g. deleted account) - must clear the pending state, not 500. - Session::put('2fa_pending_user_id', PHP_INT_MAX); + Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, PHP_INT_MAX); $response = $this->verify('123456'); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_SESSION_EXPIRED, $payload['error_code']); $this->assertFalse(Auth::check()); - $this->assertNull(Session::get('2fa_pending_user_id'), 'the orphaned pending state must be cleared'); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID), 'the orphaned pending state must be cleared'); } public function testRecoveryWithoutPendingChallengeFailsAsExpiredSession(): void @@ -966,7 +970,7 @@ public function testRecoveryWithoutPendingChallengeFailsAsExpiredSession(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_SESSION_EXPIRED, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -974,15 +978,15 @@ public function testRecoveryWithVanishedPendingUserFailsAsExpiredSession(): void { $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); - Session::put('2fa_pending_user_id', PHP_INT_MAX); + Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, PHP_INT_MAX); $response = $this->recovery('ANYCODE123'); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_session_expired', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_SESSION_EXPIRED, $payload['error_code']); $this->assertFalse(Auth::check()); - $this->assertNull(Session::get('2fa_pending_user_id'), 'the orphaned pending state must be cleared'); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID), 'the orphaned pending state must be cleared'); } public function testVerifyWithStaleOAuth2ClientFailsBeforeRedeemingOTP(): void @@ -1033,7 +1037,7 @@ public function testFailedVerifyAuditFailureStillReturnsClean401(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -1054,7 +1058,7 @@ public function testFailedRecoveryAuditFailureStillReturnsClean401(): void $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -1118,7 +1122,7 @@ public function testOIDCFlowWrongRecoveryCodeThenCorrectCompletesCircuit(): void $response = $this->recovery('WRONGCODE000'); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code']); $this->assertFalse(Auth::check(), 'a rejected recovery code must not establish a session'); // Correct code on the retry: the same OIDC flow completes end to end. @@ -1144,7 +1148,7 @@ public function testOIDCFlowConsecutiveWrongRecoveryCodesHitRateLimit(): void $response = $this->recovery('WRONGCODE' . $i); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -1154,7 +1158,7 @@ public function testOIDCFlowConsecutiveWrongRecoveryCodesHitRateLimit(): void $response = $this->recovery($plain); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); $this->assertFalse(Auth::check(), 'a rate-limited attempt must not establish a session even with a valid code'); } @@ -1173,7 +1177,7 @@ public function testOIDCFlowBurnedRecoveryCodeFailsThenFreshCodeCompletesCircuit $response = $this->recovery($burned); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_invalid_recovery', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_INVALID_RECOVERY, $payload['error_code']); $this->assertFalse(Auth::check(), 'a burned recovery code must not establish a session'); // A fresh code still completes the same OIDC flow afterwards. @@ -1198,7 +1202,7 @@ public function testOIDCFlowWrongOTPThenCorrectCompletesCircuit(): void $response = $this->verify('000000'); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code']); $this->assertFalse(Auth::check(), 'a rejected OTP must not establish a session'); // Correct OTP on the retry: the same OIDC flow completes end to end. @@ -1221,7 +1225,7 @@ public function testOIDCFlowConsecutiveWrongOTPsHitRateLimit(): void $response = $this->verify('00000' . $i); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code']); $this->assertFalse(Auth::check()); } @@ -1231,7 +1235,7 @@ public function testOIDCFlowConsecutiveWrongOTPsHitRateLimit(): void $response = $this->verify($code); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); $this->assertFalse(Auth::check(), 'a rate-limited attempt must not establish a session even with a valid code'); } @@ -1253,7 +1257,7 @@ public function testOIDCFlowRedeemedOTPFailsThenResendCompletesCircuit(): void $response = $this->verify($burned); $this->assertResponseStatus(401); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_verification_failed', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_VERIFICATION_FAILED, $payload['error_code']); $this->assertFalse(Auth::check(), 'a redeemed OTP must not establish a session'); // Resend issues a fresh code; the same OIDC flow completes with it. @@ -1365,7 +1369,7 @@ public function testVerifyRateLimitBlocksAfterThreshold(): void $response = $this->verify('bad-code-final'); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); $this->assertSame((string) $max, $response->headers->get('X-RateLimit-Limit')); $this->assertSame('0', $response->headers->get('X-RateLimit-Remaining')); $this->assertGreaterThan(0, (int) $response->headers->get('Retry-After')); @@ -1383,7 +1387,7 @@ public function testRecoveryRateLimitBlocksAfterThreshold(): void $response = $this->recovery('bad-recovery-final'); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); $this->assertSame((string) $max, $response->headers->get('X-RateLimit-Limit')); $this->assertSame('0', $response->headers->get('X-RateLimit-Remaining')); $this->assertGreaterThan(0, (int) $response->headers->get('Retry-After')); @@ -1401,7 +1405,7 @@ public function testResendRateLimitBlocksAfterThreshold(): void $response = $this->resend(); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); $this->assertSame((string) $max, $response->headers->get('X-RateLimit-Limit')); $this->assertSame('0', $response->headers->get('X-RateLimit-Remaining')); $this->assertGreaterThan(0, (int) $response->headers->get('Retry-After')); @@ -1439,7 +1443,7 @@ public function testOtpEmailRateLimitBlocksAfterThreshold(): void $response = $this->emitOTP(self::ADMIN_EMAIL); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); // A 429 must give the client a standard, machine-readable retry signal - // without these, callers have no way to know how long to back off. @@ -1473,7 +1477,7 @@ public function testOtpEmailRateLimitIsCaseInsensitive(): void $response = $this->emitOTP('SEBASTIAN@TIPIT.NET'); $this->assertResponseStatus(429); $payload = json_decode($response->getContent(), true); - $this->assertSame('mfa_rate_limit', $payload['error_code']); + $this->assertSame(MFAConstants::ERROR_CODE_RATE_LIMIT, $payload['error_code']); } // ------------------------------------------------------------------------- diff --git a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php index 211d8261..f9340fac 100644 --- a/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/AbstractMFAChallengeStrategyTest.php @@ -14,6 +14,7 @@ **/ use Auth\Exceptions\AuthenticationException; +use Auth\MFAConstants; use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\User; use Illuminate\Support\Facades\Hash; @@ -60,14 +61,14 @@ public function testGetPendingState_withValidSession_returnsState(): void public function testGetPendingState_withExpiredSession_returnsNull(): void { - Session::put('2fa_pending_user_id', 99); - Session::put('2fa_pending_at', time() - 301); - Session::put('2fa_remember', false); + Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, 99); + Session::put(MFAConstants::SESSION_KEY_PENDING_AT, time() - 301); + Session::put(MFAConstants::SESSION_KEY_REMEMBER, false); $state = $this->strategy->getPendingState(); $this->assertNull($state); - $this->assertNull(Session::get('2fa_pending_user_id')); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID)); } public function testGetPendingState_withMissingSession_returnsNull(): void @@ -79,17 +80,17 @@ public function testGetPendingState_withMissingSession_returnsNull(): void public function testClearPendingState_removesAllSessionKeys(): void { - Session::put('2fa_pending_user_id', 7); - Session::put('2fa_pending_at', time()); - Session::put('2fa_remember', true); - Session::put('2fa_recovery_attempts', 1); + Session::put(MFAConstants::SESSION_KEY_PENDING_USER_ID, 7); + Session::put(MFAConstants::SESSION_KEY_PENDING_AT, time()); + Session::put(MFAConstants::SESSION_KEY_REMEMBER, true); + Session::put(MFAConstants::SESSION_KEY_RECOVERY_ATTEMPTS, 1); $this->strategy->clearPendingState(); - $this->assertNull(Session::get('2fa_pending_user_id')); - $this->assertNull(Session::get('2fa_pending_at')); - $this->assertNull(Session::get('2fa_remember')); - $this->assertNull(Session::get('2fa_recovery_attempts')); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID)); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_PENDING_AT)); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_REMEMBER)); + $this->assertNull(Session::get(MFAConstants::SESSION_KEY_RECOVERY_ATTEMPTS)); } public function testVerifyRecoveryCode_withMatchingCode_marksAsUsed(): void diff --git a/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php b/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php index 3fefe025..bfbd664a 100644 --- a/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php +++ b/tests/unit/MFA/EmailOTPMFAChallengeStrategyTest.php @@ -13,6 +13,7 @@ * limitations under the License. **/ +use Auth\MFAConstants; use App\libs\OAuth2\Repositories\IOAuth2OTPRepository; use Auth\Repositories\IUserRecoveryCodeRepository; use Auth\User; @@ -87,8 +88,8 @@ public function testIssueChallenge_storesPendingStateAndReturnsOtpInfo(): void ['otp_length' => 6, 'otp_lifetime' => 120, 'otp_issued_at' => $issuedAt->getTimestamp()], $result ); - $this->assertSame(42, Session::get('2fa_pending_user_id')); - $this->assertTrue(Session::get('2fa_remember')); + $this->assertSame(42, Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID)); + $this->assertTrue(Session::get(MFAConstants::SESSION_KEY_REMEMBER)); } // ---------- resendChallenge ---------- @@ -115,7 +116,7 @@ public function testResendChallenge_delegatesToIssueChallenge(): void ['otp_length' => 6, 'otp_lifetime' => 120, 'otp_issued_at' => $issuedAt->getTimestamp()], $result ); - $this->assertSame(7, Session::get('2fa_pending_user_id')); + $this->assertSame(7, Session::get(MFAConstants::SESSION_KEY_PENDING_USER_ID)); } // ---------- verifyChallenge ---------- From 305b9bde9da8905c3f6cefc1467ff4ed8016f091 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 11 Aug 2026 21:34:14 -0300 Subject: [PATCH 13/13] 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. --- tests/OIDCProtocolTestCase.php | 55 ++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/tests/OIDCProtocolTestCase.php b/tests/OIDCProtocolTestCase.php index 98d2a2c7..eb0e64e8 100644 --- a/tests/OIDCProtocolTestCase.php +++ b/tests/OIDCProtocolTestCase.php @@ -45,6 +45,9 @@ */ final class OIDCProtocolTestCase extends OpenStackIDBaseTestCase { + // Seeded login user's password (database/seeds/TestSeeder.php). + private const SEED_PASSWORD = '1Qaz2wsx!'; + /** * @var string */ @@ -132,7 +135,7 @@ public function testLoginWithTrailingSpace() $response = $this->action('POST', "UserController@postLogin", [ 'username' => ' sebastian@tipit.net ', - 'password' => ' 1Qaz2wsx! ', + 'password' => ' ' . self::SEED_PASSWORD . ' ', '_token' => Session::token(), 'flow' => 'password', ] @@ -178,7 +181,7 @@ public function testConsentPrompt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -265,7 +268,7 @@ public function testConsentLogin() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -347,7 +350,7 @@ public function testAuthCode() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -432,7 +435,7 @@ public function testAuthCodeIDN() array ( 'username' => 'hei@やる.ca', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -545,7 +548,7 @@ public function testAuthCodeOpenIdScopeOnly() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -629,7 +632,7 @@ public function testMaxAge1AndWait2() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -693,7 +696,7 @@ public function testToken array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -855,7 +858,7 @@ public function testTokenSeveralScopes array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1003,7 +1006,7 @@ public function testGetRefreshTokenWithPromptSetToConsentLogin() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1146,7 +1149,7 @@ public function testFlowNativeDisplay() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => $json_response['required_params_valid_values']["_token"] ) @@ -1265,7 +1268,7 @@ public function testGetRefreshTokenFromNativeAppNTimes($n = 5) array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1467,7 +1470,7 @@ public function testTokenResponseModePost() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1614,7 +1617,7 @@ public function testNativeClientBasicAuth() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1753,7 +1756,7 @@ public function testClientAuthenticationClientSecretJwt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -1932,7 +1935,7 @@ public function testClientAuthenticationPrivateKeyJwt() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2084,7 +2087,7 @@ public function testImplicitFlowTokenIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2165,7 +2168,7 @@ public function testImplicitFlowIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2250,7 +2253,7 @@ public function testImplicitFlowIdTokenMaxAge1000() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2360,7 +2363,7 @@ public function testImplicitFlowAccessToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2497,7 +2500,7 @@ public function testImplicitFlowResponseModePost() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2667,7 +2670,7 @@ public function testHybridFlowCodeIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2774,7 +2777,7 @@ public function testHybridFlowCodeIdTokenIdTokenHint() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -2995,7 +2998,7 @@ public function testHybridFlowCodeAccessToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -3113,7 +3116,7 @@ public function testHybridFlowCodeAccessTokenIdToken() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() ) @@ -3220,7 +3223,7 @@ public function testTryingAuthCodeTwice() array ( 'username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD, 'flow' => 'password', '_token' => Session::token() )