From 786a87fb4f264b6ec8ac106826767cf3da23512f Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 4 Nov 2026 20:28:54 +0100 Subject: [PATCH 1/4] fix(2fa): show low recovery-codes warning after email OTP sign-in verify2FA now returns the recovery-codes standing (same payload as verify2FARecovery) and the login SPA shares one helper for both paths to show the low-codes warning before navigating to redirect_url. The warning block no longer depends on the recovery form so it renders after an OTP verification too. --- app/Http/Controllers/UserController.php | 8 +- resources/js/login/login.js | 58 +++++++------ tests/TwoFactorLoginFlowTest.php | 21 +++++ tests/js/login/login.mfa.test.js | 105 +++++++++++++++++++++++- 4 files changed, 167 insertions(+), 25 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 886b24a3..c37ce94e 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -806,8 +806,14 @@ public function verify2FA() // code with no way to recover it client-side. A real top-level navigation to // this URL lets the browser complete that chain natively instead - CORS never // applies to page navigations, only to XHR/fetch. + // + // The recovery-codes standing rides along (same as verify2FARecovery()) so + // the login page can warn the user when they are running low on codes. $redirect = $this->login_strategy->postLogin(); - return $this->ok(['redirect_url' => $redirect->getTargetUrl()]); + 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()); diff --git a/resources/js/login/login.js b/resources/js/login/login.js index f85e6894..c8387dfe 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -140,6 +140,7 @@ class LoginPage extends React.Component { this.onVerify2FA = this.onVerify2FA.bind(this); this.onResend2FA = this.onResend2FA.bind(this); this.onVerifyRecovery = this.onVerifyRecovery.bind(this); + this.proceedAfterVerifiedLogin = this.proceedAfterVerifiedLogin.bind(this); this.onContinueAfterLowRecoveryCodes = this.onContinueAfterLowRecoveryCodes.bind(this); this.onUseRecovery = this.onUseRecovery.bind(this); this.onBackToOtp = this.onBackToOtp.bind(this); @@ -464,9 +465,10 @@ class LoginPage extends React.Component { // data (never a redirect for this XHR to follow) - a real top-level navigation // to it lets the browser complete any further hop natively, cross-origin // included, which this XHR never could. + // The recovery-codes standing rides along: warn before navigating when + // the user is running low (see proceedAfterVerifiedLogin()). const { response } = payload; - window.location.href = - (response && response.redirect_url) || window.location.href; + this.proceedAfterVerifiedLogin(response); }, (error) => { this.handleMfaError(error, "twofactor"); @@ -567,25 +569,7 @@ class LoginPage extends React.Component { verifyRecoveryCode(recoveryCode, this.props.token).then( (payload) => { const { response } = payload; - const redirectUrl = - (response && response.redirect_url) || window.location.href; - const remaining = response && response.recovery_codes_remaining; - const threshold = - (response && response.recovery_codes_low_threshold) ?? - DEFAULT_RECOVERY_CODES_LOW_THRESHOLD; - const alreadyDismissed = - sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY) === "1"; - - if (typeof remaining === "number" && remaining < threshold && !alreadyDismissed) { - this.setState({ - ...this.state, - lowRecoveryCodesWarning: { remaining, redirectUrl }, - }); - return; - } - - // See onVerify2FA() for rationale on using a real top-level navigation. - window.location.href = redirectUrl; + this.proceedAfterVerifiedLogin(response); }, (error) => { this.handleMfaError(error, "recovery"); @@ -593,6 +577,34 @@ class LoginPage extends React.Component { ); } + /** + * Shared tail of every successful 2FA verification (email OTP and recovery + * code): show the "running low on recovery codes" warning once per browser + * session, otherwise navigate to the post-login destination. + */ + proceedAfterVerifiedLogin(response) { + const redirectUrl = + (response && response.redirect_url) || window.location.href; + const remaining = response && response.recovery_codes_remaining; + const threshold = + (response && response.recovery_codes_low_threshold) ?? + DEFAULT_RECOVERY_CODES_LOW_THRESHOLD; + const alreadyDismissed = + sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY) === "1"; + + if (typeof remaining === "number" && remaining < threshold && !alreadyDismissed) { + this.setState({ + ...this.state, + lowRecoveryCodesWarning: { remaining, redirectUrl }, + }); + return; + } + + // Real top-level navigation: the backend returns the destination as data so the + // browser completes any cross-origin hop natively (an XHR could not). + window.location.href = redirectUrl; + } + onContinueAfterLowRecoveryCodes() { sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, "1"); window.location.href = this.state.lowRecoveryCodesWarning.redirectUrl; @@ -869,7 +881,7 @@ class LoginPage extends React.Component { /> )} - {showTwoFactorForm && ( + {showTwoFactorForm && !this.state.lowRecoveryCodesWarning && ( )} - {showRecoveryForm && this.state.lowRecoveryCodesWarning && ( + {this.state.lowRecoveryCodesWarning && (
assertStringStartsWith('http', $payload['redirect_url']); $this->assertTrue(Auth::check()); + // Same recovery-codes standing as verify2FARecovery() so the login page can warn + // about low codes after an email OTP sign-in (RecoveryCodesStatus::toArray()). + $this->assertSame(0, $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); + $admin = $this->user(self::ADMIN_EMAIL); $this->assertGreaterThan(0, $this->countAudit($admin->getId(), TwoFactorAuditLog::EventChallengeSucceeded)); } + public function testOTPVerificationReportsUnusedRecoveryCodesRemaining(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $this->createRecoveryCode($admin, 'UNUSEDCODEONE', false); + $this->createRecoveryCode($admin, 'UNUSEDCODETWO', false); + $this->createRecoveryCode($admin, 'BURNEDCODE', true); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->verify($this->latestOtpCode(self::ADMIN_EMAIL)); + + $this->assertResponseStatus(200); + $payload = json_decode($response->getContent(), true); + $this->assertSame(2, $payload['recovery_codes_remaining'] ?? null, 'used codes must not be counted'); + } + public function testFailedOTPVerificationReturnsErrorAndIncrementsCounter(): void { $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); diff --git a/tests/js/login/login.mfa.test.js b/tests/js/login/login.mfa.test.js index 62524fb9..23bec7b6 100644 --- a/tests/js/login/login.mfa.test.js +++ b/tests/js/login/login.mfa.test.js @@ -22,7 +22,7 @@ delete window.location; window.location = { href: '', reload: jest.fn() }; import { LoginPage } from '../../../resources/js/login/login'; -import { verifyRecoveryCode, cancelLogin, resend2FA } from '../../../resources/js/login/actions'; +import { verify2FA, verifyRecoveryCode, cancelLogin, resend2FA } from '../../../resources/js/login/actions'; // ─── Minimal props that satisfy the LoginPage constructor ──────────────────── @@ -210,6 +210,109 @@ describe('LoginPage', () => { }); }); + // ─── Low recovery-codes warning after an email OTP sign-in ───────────────── + + describe('onVerify2FA — low recovery-codes warning', () => { + let inst; + + beforeEach(() => { + inst = makeInstance(); + inst.state = { ...inst.state, authFlow: FLOW.MFA, twoFactorCode: '123456' }; + window.location.href = ''; + verify2FA.mockReset(); + verifyRecoveryCode.mockReset(); + sessionStorage.clear(); + }); + + it('remaining below the threshold — shows the warning instead of navigating', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ + response: { + redirect_url: 'https://idp.test/authorize', + recovery_codes_remaining: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD - 1, + recovery_codes_total: 10, + recovery_codes_low_threshold: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, + }, + }), + ); + + inst.onVerify2FA(); + await flush(); + + expect(window.location.href).toBe(''); + expect(inst.state.lowRecoveryCodesWarning).toEqual({ + remaining: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD - 1, + redirectUrl: 'https://idp.test/authorize', + }); + }); + + it('remaining at the threshold — navigates directly', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ + response: { + redirect_url: 'https://idp.test/authorize', + recovery_codes_remaining: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, + recovery_codes_low_threshold: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, + }, + }), + ); + + inst.onVerify2FA(); + await flush(); + + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + + it('response without the standing — navigates directly', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ response: { redirect_url: 'https://idp.test/authorize' } }), + ); + + inst.onVerify2FA(); + await flush(); + + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + + it('dismissed warning is not shown again in the same session (OTP and recovery paths)', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ + response: { redirect_url: 'https://idp.test/authorize', recovery_codes_remaining: 1 }, + }), + ); + + inst.onVerify2FA(); + await flush(); + expect(inst.state.lowRecoveryCodesWarning).not.toBeNull(); + + inst.onContinueAfterLowRecoveryCodes(); + expect(window.location.href).toBe('https://idp.test/authorize'); + + // Same session, OTP path again. + window.location.href = ''; + inst.state = { ...inst.state, lowRecoveryCodesWarning: null, disableInput: false }; + inst.onVerify2FA(); + await flush(); + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + + // Same session, recovery path. + window.location.href = ''; + verifyRecoveryCode.mockReturnValue( + Promise.resolve({ + response: { redirect_url: 'https://idp.test/authorize', recovery_codes_remaining: 1 }, + }), + ); + inst.state = { ...inst.state, disableInput: false, recoveryCode: 'ABCD1234' }; + inst.onVerifyRecovery(); + await flush(); + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + }); + // ─── Recovery-code login flow (CU-86ba2zp4f) ─────────────────────────────── describe('recovery code flow', () => { From 155cfcf358a82e94f908894edead53b6d32583c8 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 4 Nov 2026 20:48:26 +0100 Subject: [PATCH 2/4] fix(2fa): do not warn about 0 recovery codes after email OTP sign-in A user who passes the email OTP challenge with no unused recovery codes most likely never generated any (codes are only created on enrollment or explicit regeneration), so "you have 0 recovery codes left" is misleading there. Only warn on the OTP path when 0 < remaining < threshold; the recovery-code path still warns at 0, since the user just burned their last code. --- resources/js/login/login.js | 21 ++++++++++++++----- tests/js/login/login.mfa.test.js | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/resources/js/login/login.js b/resources/js/login/login.js index c8387dfe..bb710b2a 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -466,9 +466,11 @@ class LoginPage extends React.Component { // to it lets the browser complete any further hop natively, cross-origin // included, which this XHR never could. // The recovery-codes standing rides along: warn before navigating when - // the user is running low (see proceedAfterVerifiedLogin()). + // the user is running low (see proceedAfterVerifiedLogin()). A user with + // no unused codes here most likely never generated any, so 0 is not + // "running low" on this path - that case belongs to enrollment, not here. const { response } = payload; - this.proceedAfterVerifiedLogin(response); + this.proceedAfterVerifiedLogin(response, { warnWhenNoneLeft: false }); }, (error) => { this.handleMfaError(error, "twofactor"); @@ -569,7 +571,8 @@ class LoginPage extends React.Component { verifyRecoveryCode(recoveryCode, this.props.token).then( (payload) => { const { response } = payload; - this.proceedAfterVerifiedLogin(response); + // The user just burned a code, so 0 left means they used their last one. + this.proceedAfterVerifiedLogin(response, { warnWhenNoneLeft: true }); }, (error) => { this.handleMfaError(error, "recovery"); @@ -581,8 +584,11 @@ class LoginPage extends React.Component { * Shared tail of every successful 2FA verification (email OTP and recovery * code): show the "running low on recovery codes" warning once per browser * session, otherwise navigate to the post-login destination. + * + * @param {object} response verification response (redirect_url + recovery-codes standing) + * @param {{warnWhenNoneLeft: boolean}} options whether 0 unused codes should warn */ - proceedAfterVerifiedLogin(response) { + proceedAfterVerifiedLogin(response, { warnWhenNoneLeft }) { const redirectUrl = (response && response.redirect_url) || window.location.href; const remaining = response && response.recovery_codes_remaining; @@ -592,7 +598,12 @@ class LoginPage extends React.Component { const alreadyDismissed = sessionStorage.getItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY) === "1"; - if (typeof remaining === "number" && remaining < threshold && !alreadyDismissed) { + const isLow = + typeof remaining === "number" && + remaining < threshold && + (warnWhenNoneLeft || remaining > 0); + + if (isLow && !alreadyDismissed) { this.setState({ ...this.state, lowRecoveryCodesWarning: { remaining, redirectUrl }, diff --git a/tests/js/login/login.mfa.test.js b/tests/js/login/login.mfa.test.js index 23bec7b6..c86b8ee0 100644 --- a/tests/js/login/login.mfa.test.js +++ b/tests/js/login/login.mfa.test.js @@ -264,6 +264,24 @@ describe('LoginPage', () => { expect(inst.state.lowRecoveryCodesWarning).toBeNull(); }); + it('no unused codes left — navigates directly (user never generated codes)', async () => { + verify2FA.mockReturnValue( + Promise.resolve({ + response: { + redirect_url: 'https://idp.test/authorize', + recovery_codes_remaining: 0, + recovery_codes_low_threshold: DEFAULT_RECOVERY_CODES_LOW_THRESHOLD, + }, + }), + ); + + inst.onVerify2FA(); + await flush(); + + expect(window.location.href).toBe('https://idp.test/authorize'); + expect(inst.state.lowRecoveryCodesWarning).toBeNull(); + }); + it('response without the standing — navigates directly', async () => { verify2FA.mockReturnValue( Promise.resolve({ response: { redirect_url: 'https://idp.test/authorize' } }), @@ -453,6 +471,24 @@ describe('LoginPage', () => { }); }); + it('valid code that was the last one — warns with 0 left', async () => { + verifyRecoveryCode.mockReturnValue( + Promise.resolve({ + response: { redirect_url: 'https://idp.test/authorize', recovery_codes_remaining: 0 }, + }), + ); + inst.state = { ...inst.state, recoveryCode: 'ABCD1234' }; + + inst.onVerifyRecovery(); + await flush(); + + expect(window.location.href).toBe(''); + expect(inst.state.lowRecoveryCodesWarning).toEqual({ + remaining: 0, + redirectUrl: 'https://idp.test/authorize', + }); + }); + it('low-code warning already dismissed this session — navigates straight through', async () => { sessionStorage.setItem(RECOVERY_CODES_LOW_WARNING_DISMISSED_KEY, '1'); verifyRecoveryCode.mockReturnValue( From e502683d7d9e3981fd8bca2c4e7609cc28a9e707 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 4 Nov 2026 20:51:59 +0100 Subject: [PATCH 3/4] fix(2fa): make the recovery-codes status read best-effort after verification verify2FA() and verify2FARecovery() read the recovery-codes standing after the second factor is redeemed, the session is established and postLogin() may have consumed the OAuth2 memento. A failure there turned the sign-in into a 500 without redirect_url. Log it and return redirect_url without the status fields instead; the login page already navigates directly when they are absent. --- app/Http/Controllers/UserController.php | 24 ++++++++++++-- tests/TwoFactorLoginFlowTest.php | 42 +++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index c37ce94e..9f7da04e 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -812,7 +812,7 @@ public function verify2FA() $redirect = $this->login_strategy->postLogin(); return $this->ok(array_merge( ['redirect_url' => $redirect->getTargetUrl()], - $this->recovery_code_service->getStatus($user)->toArray() + $this->recoveryCodesStatusOrEmpty($user) )); } catch (ValidationException $ex) { Log::warning($ex); @@ -911,7 +911,7 @@ public function verify2FARecovery() $redirect = $this->login_strategy->postLogin(); return $this->ok(array_merge( ['redirect_url' => $redirect->getTargetUrl()], - $this->recovery_code_service->getStatus($user)->toArray() + $this->recoveryCodesStatusOrEmpty($user) )); } catch (ValidationException $ex) { Log::warning($ex); @@ -1010,6 +1010,26 @@ private function mfaSessionExpired() * * @return void */ + /** + * Recovery-codes standing for the post-verification response, best-effort. + * Callers only reach this once the second factor is redeemed, the session is + * established and postLogin() may already have consumed the OAuth2 memento, + * so a failure here must not turn the sign-in into a 500 that drops + * redirect_url. The login page navigates directly when the fields are absent. + * + * @param User $user + * @return array RecoveryCodesStatus::toArray(), or [] on failure + */ + private function recoveryCodesStatusOrEmpty(User $user): array + { + try { + return $this->recovery_code_service->getStatus($user)->toArray(); + } catch (\Throwable $ex) { + Log::warning($ex); + return []; + } + } + private function clearMFAUISessionState(): void { Session::forget('flow'); diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index e852dedb..e5a557f4 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -18,6 +18,7 @@ use App\libs\Auth\Models\UserTrustedDevice; use App\Mail\OAuth2PasswordlessOTPMail; use App\Services\Auth\IDeviceTrustService; +use App\Services\Auth\IRecoveryCodeService; use App\Services\Auth\ITwoFactorAuditService; use App\Services\Auth\ITwoFactorRateLimitService; use Auth\AuthHelper; @@ -1087,6 +1088,47 @@ public function testRecoveryAuditFailureDoesNotBlockLogin(): void $this->assertTrue(Auth::check(), 'session must be established despite the audit failure'); } + public function testRecoveryCodesStatusFailureDoesNotBlockOTPLogin(): void + { + // The recovery-codes standing is best-effort: it is read after the OTP is + // redeemed and the session established, so a failure must NOT 500 the + // user - the response still carries redirect_url, just without the status. + $this->failRecoveryCodesStatus(); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->verify($this->latestOtpCode(self::ADMIN_EMAIL)); + + $this->assertEquals(200, $response->getStatusCode(), 'a recovery-codes status failure must not fail the login'); + $this->assertTrue(Auth::check(), 'session must be established despite the status failure'); + $payload = json_decode($response->getContent(), true); + $this->assertStringStartsWith('http', $payload['redirect_url'] ?? ''); + $this->assertArrayNotHasKey('recovery_codes_remaining', $payload); + } + + public function testRecoveryCodesStatusFailureDoesNotBlockRecoveryLogin(): void + { + $admin = $this->user(self::ADMIN_EMAIL); + $plain = 'RECOVERYSTATUSFAIL' . strtoupper(uniqid()); + $this->createRecoveryCode($admin, $plain, false); + $this->failRecoveryCodesStatus(); + + $this->postLogin(self::ADMIN_EMAIL, self::SEED_PASSWORD); + $response = $this->recovery($plain); + + $this->assertEquals(200, $response->getStatusCode(), 'a recovery-codes status failure must not fail the login'); + $this->assertTrue(Auth::check(), 'session must be established despite the status failure'); + $payload = json_decode($response->getContent(), true); + $this->assertStringStartsWith('http', $payload['redirect_url'] ?? ''); + $this->assertArrayNotHasKey('recovery_codes_remaining', $payload); + } + + private function failRecoveryCodesStatus(): void + { + $mock = \Mockery::mock(IRecoveryCodeService::class); + $mock->shouldReceive('getStatus')->andThrow(new \RuntimeException('recovery codes store unavailable')); + $this->app->instance(IRecoveryCodeService::class, $mock); + } + public function testDeviceTrustFailureDoesNotBlockLogin(): void { // Device-trust enrollment is best-effort: by the time it runs the OTP is From 5ee7c6a37a4341f2aafc0589a1554f8e5b826f90 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 11 Nov 2026 14:31:36 +0100 Subject: [PATCH 4/4] fix(2fa): keep the low recovery-codes warning accessible and its redirect intact Announce the warning (role=alert) and focus Continue when the verify form unmounts, and hide the account chip's delete action while the warning is pending: the sign-in already completed server-side, so removing the account there left the warning beside a fresh sign-in form and cancelled the OAuth2 request behind its redirect. --- resources/js/login/login.js | 11 ++++++- tests/js/login/login.mfa.test.js | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/resources/js/login/login.js b/resources/js/login/login.js index bb710b2a..39add0ff 100644 --- a/resources/js/login/login.js +++ b/resources/js/login/login.js @@ -888,7 +888,12 @@ class LoginPage extends React.Component { variant="outlined" className={styles.valid_user_name_chip} label={this.state.user_name} - onDelete={this.handleDelete} + // The sign-in already completed server-side while the low + // recovery-codes warning is pending, so switching accounts + // there would only strand the user beside a stale redirect. + onDelete={ + this.state.lowRecoveryCodesWarning ? undefined : this.handleDelete + } /> )} @@ -922,6 +927,7 @@ class LoginPage extends React.Component { )} {this.state.lowRecoveryCodesWarning && (
@@ -932,6 +938,9 @@ class LoginPage extends React.Component { locked out.