Recovery code login flow UI (#151)

* Feature | Recovery code login flow UI (CU-86ba2zp4f)

Close the remaining gaps in the recovery-code MFA login mode.

- onBackToOtp / onUseRecovery clear recoveryCode and errors.recovery along
  with the mode switch, so an abandoned attempt is not re-shown when the
  user toggles back into recovery mode.
- Recovery field drops autoComplete="one-time-code": that hint makes the OS
  offer the e-mailed OTP in the recovery field, which is the wrong
  credential and the OTP/recovery confusion risk called out in the ticket.
  Added a format hint and copy that distinguishes it from the e-mailed code.

Tests:
- New recovery-code-form.test.js: autocomplete, format hint, disabled
  states, inline error, submit, back vs cancel, raw value handed to parent.
- login.mfa.test.js: mode switching + state cleanup, input normalization,
  empty/in-flight submits, success redirect, low-codes warning (and its
  already-dismissed variant), invalid/used code, mfa_session_expired,
  mfa_rate_limit.
- E2E TS-005 fixed (it filled a 16-char code that can never exist) and now
  asserts the dash never reaches the endpoint; new TS-009 back-to-OTP,
  TS-010 invalid/used code, TS-011 recovery rate limit, TS-012 recovery
  session expiry. CI seeds mfa-ts-009..012 for them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: seed mfa-ts-009..012 in the push front-end workflow too

The MFA e2e suite gives every TS-* test its own account because a real
login burns that user's own OTP rate-limit window. TS-009..TS-012 were
added with the seed loop widened only in pull_request_frontend_tests.yml,
so "Front End Tests On Push" logged in as users that do not exist and the
four new tests timed out waiting for the password step.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep the recovery code out of the request URL

verifyRecoveryCode() posted through postRawRequest(), which copies every
param onto the query string in addition to the body (base_actions.js:71).
That writes the recovery code - a credential that completes a login on its
own - into any access log along the path. #146 hit the same trap with
current_password and added the body-only postRawRequestFull() for it;
switch this call to it as well.

TS-005 asserted the code off the query string, which encoded the leak as
the expected contract. It now asserts the body carries the code and the
query string does not, so the fix cannot silently regress.

Also from review:
- the "all 8 tests" comment in the MFA spec had gone stale at 12 tests;
  reworded so it does not track a count, and it now states the seed loop
  lives in BOTH workflow files (the divergence that broke push CI).
- onBackToOtp()'s comment justified its reset with a clean-field-on-reentry
  guarantee that onUseRecovery() already provides; state the real reason,
  which is not holding an unspent credential in component state.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: romanetar

.github/workflows/pull_request_frontend_tests.yml

@@ -103,7 +103,7 @@ jobs:
           php artisan db:seed --force
           php artisan idp:create-super-admin test@test.com '1Qaz2wsx!'
           php artisan idp:create-raw-user e2e@test.com '1Qaz2wsx!'
-          for i in 001 002 003 004 005 006 007 008; do
+          for i in 001 002 003 004 005 006 007 008 009 010 011 012; do
             php artisan idp:create-super-admin "mfa-ts-$i@test.com" '1Qaz2wsx!'
           done
           php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!'

.github/workflows/push_frontend_tests.yml

@@ -104,7 +104,7 @@ jobs:
           php artisan db:seed --force
           php artisan idp:create-super-admin test@test.com '1Qaz2wsx!'
           php artisan idp:create-raw-user e2e@test.com '1Qaz2wsx!'
-          for i in 001 002 003 004 005 006 007 008; do
+          for i in 001 002 003 004 005 006 007 008 009 010 011 012; do
             php artisan idp:create-super-admin "mfa-ts-$i@test.com" '1Qaz2wsx!'
           done
           php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!'

resources/js/login/actions.js

@@ -1,4 +1,4 @@
-import {postRawRequest} from '../base_actions'
+import {postRawRequest, postRawRequestFull} from '../base_actions'
 
 export const verifyAccount = (email, token) => {
 
@@ -51,7 +51,11 @@ export const verifyRecoveryCode = (recoveryCode, token) => {
         recovery_code: recoveryCode
     };
 
-    return postRawRequest(window.RECOVERY_2FA_ENDPOINT)(params, {'X-CSRF-TOKEN': token});
+    // postRawRequestFull(), not postRawRequest(): the latter also copies every
+    // param onto the query string, which would write the recovery code - a
+    // credential that completes a login on its own - into every access log it
+    // passes through. Same reasoning as the current_password fix in #146.
+    return postRawRequestFull(window.RECOVERY_2FA_ENDPOINT)(params, {'X-CSRF-TOKEN': token});
 }
 
 export const cancelLogin = (token) => {

resources/js/login/components/recovery_code_form.js

@@ -34,7 +34,8 @@ const RecoveryCodeForm = ({
         <form onSubmit={handleSubmit} target="_self" className={styles.otp_form} data-testid="recovery-form">
             <div className={styles.subtitle}>Enter a recovery code</div>
             <p className={styles.info_message}>
-                Enter one of the recovery codes you saved when you enabled two-step verification.
+                This is not the code we e-mailed you. Enter one of the recovery codes you saved
+                when you enabled two-step verification.
             </p>
             <TextField
                 id="recovery_code"
@@ -46,7 +47,10 @@ const RecoveryCodeForm = ({
                 fullWidth
                 autoFocus={true}
                 label="Recovery code"
-                autoComplete="one-time-code"
+                helperText="8 characters, shown as ABCD-1234. The dash is optional."
+                // Deliberately not "one-time-code": that hint makes the OS offer the
+                // e-mailed OTP here, which is the wrong credential for this field.
+                autoComplete="off"
                 disabled={disableInput}
                 onChange={onRecoveryCodeChange}
                 error={!!recoveryError}
@@ -69,7 +73,8 @@ const RecoveryCodeForm = ({
             <div className={styles.footer_instructions}>
                 <hr className={styles.separator}/>
                 <div className={styles.box}>
-                    <Link href="#" onClick={handleBack} variant="body2" target="_self">
+                    <Link href="#" onClick={handleBack} variant="body2" target="_self"
+                          data-testid="back-to-otp-link">
                         Back to verification code
                     </Link>
                     {" · "}

resources/js/login/login.js

@@ -587,15 +587,21 @@ class LoginPage extends React.Component {
     this.setState({
       ...this.state,
       authFlow: FLOW.RECOVERY,
+      recoveryCode: "",
       errors: { ...this.state.errors, recovery: "" },
     });
   }
 
   onBackToOtp() {
+    // Drop the abandoned recovery code as we leave the mode so a credential the
+    // user chose not to spend is not kept in component state for the rest of the
+    // session. A clean field on re-entry is already guaranteed by onUseRecovery();
+    // this is about not holding the value, not about what the next render shows.
     this.setState({
       ...this.state,
       authFlow: FLOW.MFA,
-      errors: { ...this.state.errors, twofactor: "" },
+      recoveryCode: "",
+      errors: { ...this.state.errors, twofactor: "", recovery: "" },
     });
   }
 

tests/e2e/pages/LoginPage.ts

@@ -21,6 +21,8 @@ export class LoginPage {
   readonly useRecoveryLink: Locator;
   // Recovery code step
   readonly recoveryForm: Locator;
+  readonly recoveryCodeInput: Locator;
+  readonly backToOtpLink: Locator;
 
   constructor(page: Page) {
     this.page = page;
@@ -38,6 +40,8 @@ export class LoginPage {
     this.cancelLink = page.locator('[data-testid="cancel-link"]');
     this.useRecoveryLink = page.locator('[data-testid="use-recovery-link"]');
     this.recoveryForm = page.locator('[data-testid="recovery-form"]');
+    this.recoveryCodeInput = page.locator('#recovery_code');
+    this.backToOtpLink = page.locator('[data-testid="back-to-otp-link"]');
   }
 
   async goto() {

tests/e2e/tests/auth/login-mfa-flow.spec.ts

@@ -6,18 +6,27 @@ import type { Page } from '@playwright/test';
 // param onto the URL as a query string (in addition to the body), so the real
 // request is "<path>?otp_value=...&method=..." - a glob without the trailing
 // wildcard requires an exact end-of-string match and silently never fires.
+// The recovery endpoint is the exception: it posts through postRawRequestFull(),
+// which is body-only, so its URL carries no query string ('**' still matches).
 const VERIFY_URL   = '**/auth/login/2fa/verify**';
 const RESEND_URL   = '**/auth/login/2fa/resend**';
 const RECOVERY_URL = '**/auth/login/2fa/recovery**';
 const CANCEL_URL   = '**/auth/login/reset**';
 
-// Each TS-* test gets its own MFA-enforced super-admin (mfa-ts-NNN@test.com,
-// seeded by CI via idp:create-super-admin - see pull_request_frontend_tests.yml).
+// Each TS-* test gets its own MFA-enforced super-admin (mfa-ts-NNN@test.com),
+// seeded by CI via idp:create-super-admin in BOTH pull_request_frontend_tests.yml
+// and push_frontend_tests.yml - adding a TS-NNN here means widening the seed loop
+// in both, or the new test logs in as a user that does not exist.
 // A real login issues a real OTP challenge and counts against that user's own
 // two_factor.rate_limit.max_otp_requests window, so sharing one fixed account
-// across all 8 tests would exhaust the limit well before the suite finishes.
+// across the whole suite would exhaust the limit well before it finishes.
 const MFA_USER_PASSWORD = '1Qaz2wsx!';
 
+// Recovery codes are generated as 8 chars from [A-Z0-9] and shown as XXXX-XXXX
+// (RecoveryCodeService::regenerateCodesForUser), but hashed without the dash.
+const RECOVERY_CODE_AS_DISPLAYED = 'ABCD-1234';
+const RECOVERY_CODE_NORMALIZED   = 'ABCD1234';
+
 function mfaUserEmailFor(testTitle: string): string {
   const match = testTitle.match(/TS-(\d+)/);
   if (!match) {
@@ -127,7 +136,7 @@ test.describe('MFA Login Flow', () => {
     });
 
   // TS-005 ─────────────────────────────────────────────────────────────────
-  test('TS-005: use recovery code — recovery form shown and API called',
+  test('TS-005: use recovery code — recovery form shown and normalized code posted',
     async ({ loginPage, page }) => {
       await page.route(RECOVERY_URL, route =>
         route.fulfill({ status: 200, contentType: 'application/json', body: '{}' })
@@ -137,13 +146,21 @@ test.describe('MFA Login Flow', () => {
       await expect(loginPage.recoveryForm).toBeVisible();
       await expect(loginPage.twoFactorForm).not.toBeVisible();
 
-      await page.locator('#recovery_code').fill('ABCD-1234-EFGH-5678');
+      // Typed exactly as the code is displayed to the user (XXXX-XXXX); the
+      // separator is presentational only and must never reach the endpoint.
+      await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_AS_DISPLAYED);
+      await expect(loginPage.recoveryCodeInput).toHaveValue(RECOVERY_CODE_NORMALIZED);
 
-      const [response] = await Promise.all([
-        page.waitForResponse(RECOVERY_URL),
+      const [request] = await Promise.all([
+        page.waitForRequest(RECOVERY_URL),
         loginPage.verifyButton.click(),
       ]);
-      expect(response.status()).toBe(200);
+
+      // The code travels in the body only. verifyRecoveryCode() posts through
+      // postRawRequestFull() precisely so it never reaches the query string,
+      // where access logs would capture it - assert both halves of that.
+      expect(request.postDataJSON()).toMatchObject({ recovery_code: RECOVERY_CODE_NORMALIZED });
+      expect(new URL(request.url()).searchParams.get('recovery_code')).toBeNull();
     });
 
   // TS-006 ─────────────────────────────────────────────────────────────────
@@ -210,4 +227,97 @@ test.describe('MFA Login Flow', () => {
       await expect(loginPage.errorLabel).toBeVisible();
       await expect(loginPage.errorLabel).toContainText('Too many attempts');
     });
+
+  // TS-009 ─────────────────────────────────────────────────────────────────
+  test('TS-009: back to verification code — OTP mode restored with a clean recovery field',
+    async ({ loginPage }) => {
+      await loginPage.useRecoveryLink.click();
+      await expect(loginPage.recoveryForm).toBeVisible();
+
+      await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED);
+      await loginPage.backToOtpLink.click();
+
+      // The OTP flow must be intact — same form, still able to submit a code.
+      await expect(loginPage.twoFactorForm).toBeVisible();
+      await expect(loginPage.recoveryForm).not.toBeVisible();
+      await expect(loginPage.passwordForm).not.toBeVisible();
+
+      // Re-entering recovery mode must not resurrect the abandoned code.
+      await loginPage.useRecoveryLink.click();
+      await expect(loginPage.recoveryCodeInput).toHaveValue('');
+    });
+
+  // TS-010 ─────────────────────────────────────────────────────────────────
+  test('TS-010: invalid/used recovery code — inline error, still in the recovery form',
+    async ({ loginPage, page }) => {
+      // A used code fails exactly like an unknown one: the backend answers
+      // mfa_invalid_recovery for both (AbstractMFAChallengeStrategy).
+      await page.route(RECOVERY_URL, route =>
+        route.fulfill({
+          status: 401,
+          contentType: 'application/json',
+          body: JSON.stringify({ error_code: 'mfa_invalid_recovery' }),
+        })
+      );
+
+      await loginPage.useRecoveryLink.click();
+      await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED);
+      await loginPage.verifyButton.click();
+
+      await expect(loginPage.errorLabel).toBeVisible();
+      await expect(loginPage.errorLabel).toContainText('Invalid recovery code');
+      // The MFA flow must not be abandoned on a bad code.
+      await expect(loginPage.recoveryForm).toBeVisible();
+      await expect(loginPage.passwordForm).not.toBeVisible();
+    });
+
+  // TS-011 ─────────────────────────────────────────────────────────────────
+  test('TS-011: recovery rate limit — 429 inline error shown',
+    async ({ loginPage, page }) => {
+      await page.route(RECOVERY_URL, route =>
+        route.fulfill({
+          status: 429,
+          contentType: 'application/json',
+          body: JSON.stringify({
+            error_code: 'mfa_rate_limit',
+            error_message: 'Too many attempts. Please try again later.',
+          }),
+        })
+      );
+
+      await loginPage.useRecoveryLink.click();
+      await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED);
+      await loginPage.verifyButton.click();
+
+      await expect(loginPage.errorLabel).toBeVisible();
+      await expect(loginPage.errorLabel).toContainText('Too many attempts');
+      await expect(loginPage.recoveryForm).toBeVisible();
+    });
+
+  // TS-012 ─────────────────────────────────────────────────────────────────
+  test('TS-012: recovery session expired — back to password form with warning snackbar',
+    async ({ loginPage, page }) => {
+      await page.route(RECOVERY_URL, route =>
+        route.fulfill({
+          status: 401,
+          contentType: 'application/json',
+          body: JSON.stringify({ error_code: 'mfa_session_expired' }),
+        })
+      );
+      // resetToPasswordFlow() fires cancelLogin() in the background; absorb it.
+      await page.route(CANCEL_URL, route =>
+        route.fulfill({ status: 200, contentType: 'application/json', body: '{}' })
+      );
+
+      await loginPage.useRecoveryLink.click();
+      await loginPage.recoveryCodeInput.fill(RECOVERY_CODE_NORMALIZED);
+      await loginPage.verifyButton.click();
+
+      await expect(loginPage.recoveryForm).not.toBeVisible();
+      await expect(loginPage.passwordForm).toBeVisible();
+
+      const snackbar = page.locator('[role="alert"]');
+      await expect(snackbar).toBeVisible();
+      await expect(snackbar).toContainText('session has expired');
+    });
 });

tests/js/login/components/recovery-code-form.test.js

@@ -0,0 +1,81 @@
+import React from 'react';
+import { render, screen, fireEvent } from '@testing-library/react';
+import RecoveryCodeForm from '../../../../resources/js/login/components/recovery_code_form';
+
+const baseProps = {
+  recoveryCode: 'ABCD1234',
+  recoveryError: '',
+  disableInput: false,
+  onRecoveryCodeChange: jest.fn(),
+  onVerify: jest.fn(),
+  onBackToOtp: jest.fn(),
+  onCancel: jest.fn(),
+};
+
+describe('RecoveryCodeForm', () => {
+
+  beforeEach(() => jest.clearAllMocks());
+
+  it('does not advertise the field as a one-time-code slot', () => {
+    // "one-time-code" would make the OS offer the e-mailed OTP here — the
+    // wrong credential for this field, and the OTP/recovery confusion risk.
+    render(<RecoveryCodeForm {...baseProps} />);
+    expect(document.getElementById('recovery_code')).toHaveAttribute('autocomplete', 'off');
+  });
+
+  it('renders the expected-format hint', () => {
+    render(<RecoveryCodeForm {...baseProps} />);
+    expect(screen.getByText(/8 characters, shown as ABCD-1234/)).toBeInTheDocument();
+  });
+
+  it('VERIFY button is disabled when the code is empty', () => {
+    render(<RecoveryCodeForm {...baseProps} recoveryCode="" />);
+    expect(screen.getByTestId('verify-button')).toBeDisabled();
+  });
+
+  it('VERIFY button is disabled while a submit is in flight', () => {
+    render(<RecoveryCodeForm {...baseProps} disableInput={true} />);
+    expect(screen.getByTestId('verify-button')).toBeDisabled();
+  });
+
+  it('error paragraph renders when recoveryError is non-empty', () => {
+    const msg = 'Invalid recovery code. Please try again.';
+    render(<RecoveryCodeForm {...baseProps} recoveryError={msg} />);
+    expect(screen.getByTestId('error-label')).toHaveTextContent(msg);
+  });
+
+  it('submitting the form calls onVerify without navigating', () => {
+    render(<RecoveryCodeForm {...baseProps} />);
+    fireEvent.submit(screen.getByTestId('recovery-form'));
+    expect(baseProps.onVerify).toHaveBeenCalledTimes(1);
+  });
+
+  it('"Back to verification code" calls onBackToOtp', () => {
+    render(<RecoveryCodeForm {...baseProps} />);
+    fireEvent.click(screen.getByTestId('back-to-otp-link'));
+    expect(baseProps.onBackToOtp).toHaveBeenCalledTimes(1);
+    expect(baseProps.onCancel).not.toHaveBeenCalled();
+  });
+
+  it('"Cancel" calls onCancel', () => {
+    render(<RecoveryCodeForm {...baseProps} />);
+    fireEvent.click(screen.getByTestId('cancel-link'));
+    expect(baseProps.onCancel).toHaveBeenCalledTimes(1);
+    expect(baseProps.onBackToOtp).not.toHaveBeenCalled();
+  });
+
+  it('typing in the field reports the raw value to the parent', () => {
+    // Normalization lives in LoginPage.onRecoveryCodeChange(); the form must
+    // hand over the untouched event so that stays the single source of truth.
+    // Read the value inside the handler: the field is controlled, so React
+    // resets the DOM node back to the (unchanged) prop before the assertion runs.
+    let seen = null;
+    const onRecoveryCodeChange = jest.fn((ev) => { seen = ev.target.value; });
+
+    render(<RecoveryCodeForm {...baseProps} recoveryCode="" onRecoveryCodeChange={onRecoveryCodeChange} />);
+    fireEvent.change(document.getElementById('recovery_code'), { target: { value: 'abcd-1234' } });
+
+    expect(onRecoveryCodeChange).toHaveBeenCalledTimes(1);
+    expect(seen).toBe('abcd-1234');
+  });
+});