Skip to content

fix(2fa): show low recovery-codes warning after email OTP sign-in #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
romanetar wants to merge 4 commits into
base: feat/mfa-phase1---migrations--and--interfaces
Choose a base branch
from
Open

fix(2fa): show low recovery-codes warning after email OTP sign-in #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
786a87f
fix(2fa): show low recovery-codes warning after email OTP sign-in
romanetar Nov 4, 2026
155cfcf
fix(2fa): do not warn about 0 recovery codes after email OTP sign-in
romanetar Nov 4, 2026
e502683
fix(2fa): make the recovery-codes status read best-effort after verif…
romanetar Nov 4, 2026
5ee7c6a
fix(2fa): keep the low recovery-codes warning accessible and its redi…
romanetar Nov 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions app/Http/Controllers/UserController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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->recoveryCodesStatusOrEmpty($user)
));
} catch (ValidationException $ex) {
Log::warning($ex);
return $this->error412($ex->getMessages());
Expand Down Expand Up @@ -905,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);
Expand Down Expand Up @@ -1004,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<string,int> 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');
Expand Down
80 changes: 56 additions & 24 deletions resources/js/login/login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -464,9 +465,12 @@ 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()). 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;
window.location.href =
(response && response.redirect_url) || window.location.href;
this.proceedAfterVerifiedLogin(response, { warnWhenNoneLeft: false });
},
(error) => {
this.handleMfaError(error, "twofactor");
Expand Down Expand Up @@ -567,32 +571,51 @@ 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;
// 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");
},
);
}

/**
* 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, { warnWhenNoneLeft }) {
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";
Comment thread
romanetar marked this conversation as resolved.
Resolved

const isLow =
typeof remaining === "number" &&
remaining < threshold &&
(warnWhenNoneLeft || remaining > 0);

if (isLow && !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;
Expand Down Expand Up @@ -865,11 +888,16 @@ 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
}
/>
)}
</Typography>
{showTwoFactorForm && (
{showTwoFactorForm && !this.state.lowRecoveryCodesWarning && (
<TwoFactorForm
otpCode={this.state.twoFactorCode}
otpError={this.state.errors.twofactor}
Expand Down Expand Up @@ -897,8 +925,9 @@ class LoginPage extends React.Component {
onCancel={this.resetToPasswordFlow}
/>
)}
{showRecoveryForm && this.state.lowRecoveryCodesWarning && (
{this.state.lowRecoveryCodesWarning && (
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
<div
role="alert"
className={recoveryCodesStyles.low_code_warning}
data-testid="low-recovery-codes-warning"
>
Expand All @@ -909,6 +938,9 @@ class LoginPage extends React.Component {
locked out.
</Typography>
<Button
// The verify form that held focus unmounts when the warning
// appears; move keyboard focus to the only way forward.
autoFocus
variant="contained"
color="primary"
onClick={this.onContinueAfterLowRecoveryCodes}
Expand Down
63 changes: 63 additions & 0 deletions tests/TwoFactorLoginFlowTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -668,10 +669,31 @@ public function testSuccessfulOTPVerificationCompletesLogin(): void
$this->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);
Expand Down Expand Up @@ -1066,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
Expand Down
Loading
Loading