diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 886b24a3..9f7da04e 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->recoveryCodesStatusOrEmpty($user) + )); } catch (ValidationException $ex) { Log::warning($ex); return $this->error412($ex->getMessages()); @@ -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); @@ -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 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/resources/js/login/login.js b/resources/js/login/login.js index f85e6894..39add0ff 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,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"); @@ -567,25 +571,8 @@ 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"); @@ -593,6 +580,42 @@ 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, { 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"; + + 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; @@ -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 + } /> )} - {showTwoFactorForm && ( + {showTwoFactorForm && !this.state.lowRecoveryCodesWarning && ( )} - {showRecoveryForm && this.state.lowRecoveryCodesWarning && ( + {this.state.lowRecoveryCodesWarning && (
@@ -909,6 +938,9 @@ class LoginPage extends React.Component { locked out.