Recovery code login flow UI

.github/workflows/pull_request_frontend_tests.yml

@@ -102,7 +102,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

@@ -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!'

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/cancel**';
 
-// 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');
+  });
+});