diff --git a/next-env.d.ts b/next-env.d.ts index a419cbe..ce4e94a 100644 --- a/next-env.d.ts +++ b/next-env.d.ts @@ -1,7 +1,7 @@ /// /// -import "./.next/dev/types/routes.d.ts"; -import "./.next/dev/types/root-params.d.ts"; +import "./.next/types/routes.d.ts"; +import "./.next/types/root-params.d.ts"; // NOTE: This file should not be edited // see https://nextjs.org/docs/app/api-reference/config/typescript for more information. diff --git a/src/app/(public)/login/__tests__/sign-in-action-wiring.test.tsx b/src/app/(public)/login/__tests__/sign-in-action-wiring.test.tsx new file mode 100644 index 0000000..5e5c54a --- /dev/null +++ b/src/app/(public)/login/__tests__/sign-in-action-wiring.test.tsx @@ -0,0 +1,118 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * RUK-290 — the two `"use server"` wrappers in `login/page.tsx` actually forward + * `rememberMe`. SPEC §3.2 site 2. + * + * **Why this file exists.** Ship review mutated both wrappers to pass a literal + * `rememberMe: false` and the entire gate stayed green — 219 contract tests, + * 1466 unit tests and `tsc` all silent — while the checkbox stopped working on + * both sign-in methods at once. That is the widest silent failure in the change + * and, until this file, the only one nothing covered. + * + * SPEC §3.2 lists site 2 as "not silent" because widening a signature breaks its + * callers. True for *adding* the parameter; useless against *bypassing* it + * afterwards, which is the shape a regression actually takes. `false` satisfies + * `boolean`, so the compiler has nothing to say. + * + * Nothing else reaches these wrappers: the four `LoginPage` component tests pass + * inert stubs (`otpSignInAction={async () => ({})}`) because they are testing + * the component, not the page that composes it. The chain is proven from the + * checkbox down to `credentialsSignInAction`, and from `credentialsSignInAction` + * to the wire — this is the joint between those two halves. + * + * The page is an async server component, so it is invoked as a function and its + * returned element inspected for the props it hands down. The wrappers are then + * called directly, which is what a submitting form does. + */ + +/** The shape `credentialsSignInAction` receives; only `rememberMe` is asserted. */ +type SignInInput = { + kind: "otp" | "password"; + email: string; + code?: string; + password?: string; + rememberMe: boolean; + next?: string; +}; + +const credentialsSignInAction = vi.fn<(input: SignInInput) => Promise<{ error?: string }>>(); + +vi.mock("@/server/auth/built-in-sign-in-actions", () => ({ + credentialsSignInAction: (input: SignInInput) => credentialsSignInAction(input), + requestOtpAction: vi.fn(async () => ({})), + changeEmailAction: vi.fn(async () => {}), +})); + +vi.mock("@/server/auth/password-reset-actions", () => ({ + requestPasswordResetAction: vi.fn(async () => ({})), + confirmPasswordResetAction: vi.fn(async () => ({})), + abandonPasswordResetAction: vi.fn(async () => {}), +})); + +vi.mock("@/server/auth/otp-nonce-cookie", () => ({ + readPasswordResetBinding: vi.fn(async () => undefined), +})); + +vi.mock("@/server/auth/auth-config", () => ({ signIn: vi.fn(async () => {}) })); + +vi.mock("@/server/backend/auth/resolve-auth-providers", () => ({ + resolveAuthProviders: vi.fn(async () => ({ ok: true, methods: [] })), +})); + +// The page renders ; the element's props are the subject here, so +// the component itself is replaced by a marker that keeps them inspectable. +vi.mock("@/features/auth/login-page", () => ({ LoginPage: () => null })); + +const Page = (await import("@/app/(public)/login/page")).default; + +type SignInProps = { + otpSignInAction: (email: string, code: string, rememberMe: boolean) => Promise<{ error?: string }>; + passwordSignInAction: (email: string, password: string, rememberMe: boolean) => Promise<{ error?: string }>; +}; + +/** Invokes the server component and returns the props it hands to LoginPage. */ +async function signInProps(): Promise { + const element = await Page({ searchParams: Promise.resolve({}) }); + return (element as unknown as { props: SignInProps }).props; +} + +beforeEach(() => { + credentialsSignInAction.mockReset(); + credentialsSignInAction.mockResolvedValue({}); +}); + +/** What the action actually received, read from the mock rather than the input. */ +function actionArg(): SignInInput { + const [first] = credentialsSignInAction.mock.calls; + expect(first).toBeDefined(); + return first![0]; +} + +describe("login page — the sign-in wrappers forward the remember-me choice", () => { + it.each([true, false])("passes rememberMe %s through the password wrapper", async (choice) => { + const { passwordSignInAction } = await signInProps(); + + await passwordSignInAction("admin@example.test", "hunter2", choice); + + expect(actionArg()).toMatchObject({ kind: "password", rememberMe: choice }); + }); + + it.each([true, false])("passes rememberMe %s through the OTP wrapper", async (choice) => { + const { otpSignInAction } = await signInProps(); + + await otpSignInAction("admin@example.test", "123456", choice); + + expect(actionArg()).toMatchObject({ kind: "otp", rememberMe: choice }); + }); + + it("still forwards the credentials alongside the flag", async () => { + // Guards against a wrapper that satisfies the assertions above by passing + // the flag and dropping something else. + const { passwordSignInAction } = await signInProps(); + + await passwordSignInAction("admin@example.test", "hunter2", true); + + expect(actionArg()).toMatchObject({ email: "admin@example.test", password: "hunter2" }); + }); +}); diff --git a/src/app/(public)/login/page.tsx b/src/app/(public)/login/page.tsx index 0ecc280..21d4534 100644 --- a/src/app/(public)/login/page.tsx +++ b/src/app/(public)/login/page.tsx @@ -63,14 +63,14 @@ export default async function Page({ * server, and the sanitized `redirectTo` is closed over here so a client can * never supply a destination of its own and route around `safeNext`. */ - async function otpSignInAction(email: string, code: string) { + async function otpSignInAction(email: string, code: string, rememberMe: boolean) { "use server"; - return credentialsSignInAction({ kind: "otp", email, code, next: redirectTo }); + return credentialsSignInAction({ kind: "otp", email, code, rememberMe, next: redirectTo }); } - async function passwordSignInAction(email: string, password: string) { + async function passwordSignInAction(email: string, password: string, rememberMe: boolean) { "use server"; - return credentialsSignInAction({ kind: "password", email, password, next: redirectTo }); + return credentialsSignInAction({ kind: "password", email, password, rememberMe, next: redirectTo }); } return ( diff --git a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx index 65baa21..5876912 100644 --- a/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx +++ b/src/features/auth/__tests__/accept-invite-no-query-provider.test.tsx @@ -6,6 +6,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { AcceptInvitePage } from "../accept-invite-page"; import { LoginPage } from "../login-page"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); const noopAccept = vi.fn(async () => {}); diff --git a/src/features/auth/__tests__/login-page-methods.test.tsx b/src/features/auth/__tests__/login-page-methods.test.tsx index e078bfd..3137d44 100644 --- a/src/features/auth/__tests__/login-page-methods.test.tsx +++ b/src/features/auth/__tests__/login-page-methods.test.tsx @@ -5,6 +5,18 @@ import { afterEach, describe, expect, it } from "vitest"; import { LoginPage } from "@/features/auth/login-page"; import type { SignInMethod } from "@/domain/auth/sign-in-method"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + /** * RUK-288 AC-1 / AC-2 / AC-11 — the login page is drawn from the backend's * method list, and cannot lock anyone out when that list is unavailable. diff --git a/src/features/auth/__tests__/login-page-reset.test.tsx b/src/features/auth/__tests__/login-page-reset.test.tsx index 6a158cb..63fea29 100644 --- a/src/features/auth/__tests__/login-page-reset.test.tsx +++ b/src/features/auth/__tests__/login-page-reset.test.tsx @@ -5,6 +5,18 @@ import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/re import { LoginPage } from "@/features/auth/login-page"; import type { SignInMethod } from "@/domain/auth/sign-in-method"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); const PASSWORD_METHOD: SignInMethod = { diff --git a/src/features/auth/__tests__/login-page.test.tsx b/src/features/auth/__tests__/login-page.test.tsx index 36293b2..07aab0e 100644 --- a/src/features/auth/__tests__/login-page.test.tsx +++ b/src/features/auth/__tests__/login-page.test.tsx @@ -4,6 +4,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { LoginPage } from "../login-page"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + // This config has no global testing-library auto-cleanup, so unmount between // tests to keep the document free of stale renders. afterEach(() => cleanup()); diff --git a/src/features/auth/__tests__/otp-sign-in-flow.test.tsx b/src/features/auth/__tests__/otp-sign-in-flow.test.tsx index ee41076..180786a 100644 --- a/src/features/auth/__tests__/otp-sign-in-flow.test.tsx +++ b/src/features/auth/__tests__/otp-sign-in-flow.test.tsx @@ -4,6 +4,18 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { OtpSignInFlow } from "@/features/auth/otp-sign-in-flow"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + /** * RUK-288 AC-4 / AC-5 / AC-6 — the two-step code flow and its state table. */ @@ -28,14 +40,35 @@ function setup(overrides: Partial[0]> = {}) { return { requestCode, submitCode, onChangeEmail }; } -async function reachCodeStep(requestCode?: () => Promise<{ error?: string }>) { - const handles = setup(requestCode ? { requestCode: vi.fn(requestCode) } : {}); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); +/** + * Renders the flow and walks it to step two, the starting point of almost every + * test below. Takes the same overrides as `setup` so a test that needs its own + * `submitCode` does not have to re-copy the four-line walk — a copy that, being + * setup rather than assertion, tended to drift. + * + * `address` is a parameter because the "change email" tests walk this path twice + * with two different addresses. + */ +async function reachCodeStep( + overrides: Partial[0]> = {}, + address = "someone@example.test", +) { + const handles = setup(overrides); + await enterAddress(address); + return handles; +} + +/** The step-one half on its own: for a second pass through an existing render. */ +async function enterAddress(address: string) { + fireEvent.change(screen.getByLabelText("Email code"), { target: { value: address } }); fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); await waitFor(() => expect(screen.getByLabelText("Enter the 6-digit code")).toBeDefined()); - return handles; +} + +/** Types a code into step two and submits it. */ +function submitCodeValue(code: string) { + fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: code } }); + fireEvent.click(screen.getByRole("button", { name: "Sign in" })); } describe("step one — asking for a code", () => { @@ -54,7 +87,7 @@ describe("step one — asking for a code", () => { it("looks identical whether or not the address has an account", async () => { // The backend answers 202 for both, deliberately. If this component ever // branched on the outcome it would leak exactly what that 202 hides. - const { requestCode } = await reachCodeStep(async () => ({})); + const { requestCode } = await reachCodeStep({ requestCode: vi.fn(async () => ({})) }); expect(requestCode).toHaveBeenCalledTimes(1); expect(screen.queryByRole("alert")).toBeNull(); @@ -91,18 +124,9 @@ describe("step two — entering the code", () => { describe("AC-4 — a lost binding is not a wrong code", () => { it("tells the user to request a new code, never that the code is wrong", async () => { - const submitCode = vi.fn(async () => ({ error: "otp_session_mismatch" })); - setup({ submitCode }); + await reachCodeStep({ submitCode: vi.fn(async () => ({ error: "otp_session_mismatch" })) }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); - fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { - target: { value: "123456" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + submitCodeValue("123456"); const alert = await screen.findByRole("alert"); expect(alert.textContent).toContain("can't be checked in this browser"); @@ -115,18 +139,9 @@ describe("AC-4 — a lost binding is not a wrong code", () => { // The binding is gone, so step two is a dead end: "Sign in" would fire more // doomed calls, and the residual cooldown greys out the very button the // message tells the user to press. - const submitCode = vi.fn(async () => ({ error: "otp_session_mismatch" })); - setup({ submitCode }); + await reachCodeStep({ submitCode: vi.fn(async () => ({ error: "otp_session_mismatch" })) }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); - fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { - target: { value: "123456" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + submitCodeValue("123456"); await waitFor(() => expect(screen.getByLabelText("Email code")).toBeDefined()); expect(screen.queryByLabelText("Enter the 6-digit code")).toBeNull(); @@ -135,17 +150,9 @@ describe("AC-4 — a lost binding is not a wrong code", () => { }); it("reports a wrong code distinctly, and keeps the user on step two", async () => { - const submitCode = vi.fn(async () => ({ error: "otp_verification_failed" })); - setup({ submitCode }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); - fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { - target: { value: "000000" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + await reachCodeStep({ submitCode: vi.fn(async () => ({ error: "otp_verification_failed" })) }); + + submitCodeValue("000000"); const alert = await screen.findByRole("alert"); expect(alert.textContent).toContain("isn't valid"); @@ -208,17 +215,8 @@ describe("expiry wins over a wrong code", () => { it("shows the expired message rather than re-check-your-code", async () => { // Telling someone to re-check a code that can no longer work is a dead end. - const submitCode = vi.fn(async () => ({ error: "otp_verification_failed" })); - setup({ submitCode }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); - fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { - target: { value: "000000" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + await reachCodeStep({ submitCode: vi.fn(async () => ({ error: "otp_verification_failed" })) }); + submitCodeValue("000000"); await screen.findByRole("alert"); await vi.advanceTimersByTimeAsync(300_000); @@ -233,12 +231,7 @@ describe("a second submit while one is in flight is ignored", () => { // code, so an impatient double-click would otherwise burn two of them. let resolveSubmit: (v: { error?: string }) => void = () => {}; const submitCode = vi.fn(() => new Promise<{ error?: string }>((resolve) => (resolveSubmit = resolve))); - setup({ submitCode }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); + await reachCodeStep({ submitCode }); fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: "123456" }, }); @@ -261,12 +254,7 @@ describe("the double-submit guard is synchronous, not state-based", () => { // awaiting between them is what distinguishes the ref from the state. let resolveSubmit: (v: { error?: string }) => void = () => {}; const submitCode = vi.fn(() => new Promise<{ error?: string }>((resolve) => (resolveSubmit = resolve))); - setup({ submitCode }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); + await reachCodeStep({ submitCode }); fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: "123456" }, }); @@ -351,33 +339,100 @@ describe("§6.9 — a successful resend restarts the flow", () => { describe("§6.6 — the address from step one is the one verified", () => { it("submits the code against the address the code was sent to", async () => { - const submitCode = vi.fn(async () => ({})); - setup({ submitCode }); + const { submitCode } = await reachCodeStep({ submitCode: vi.fn(async () => ({})) }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); - fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { - target: { value: "123456" }, + submitCodeValue("123456"); + + await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456", false)); + }); + + it("sends the remember-me choice with the code", async () => { + // RUK-290. The box lives on the CODE step because that is the request that + // mints the session; the address step issues no token. + const { submitCode } = await reachCodeStep({ submitCode: vi.fn(async () => ({})) }); + + fireEvent.click(screen.getByLabelText("Keep me signed in")); + submitCodeValue("123456"); + + await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456", true)); + }); + + it("does not offer the remember-me box on the address step", () => { + // It would attach the choice to a request that issues no token. + setup({}); + + expect(screen.queryByLabelText("Keep me signed in")).toBeNull(); + }); + + it("survives a wrong code, so the remaining attempts stay usable", async () => { + // The binding is deliberately kept alive after a wrong code (RUK-288), and + // the box must be kept with it — otherwise five attempts means re-ticking + // it five times. Correct today only because nothing resets it in the error + // branch; this test is what stops someone "fixing" that. + const { submitCode } = await reachCodeStep({ + submitCode: vi.fn(async () => ({ error: "otp_verification_failed" })), }); - fireEvent.click(screen.getByRole("button", { name: "Sign in" })); - await waitFor(() => expect(submitCode).toHaveBeenCalledWith("someone@example.test", "123456")); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + submitCodeValue("000000"); + await waitFor(() => expect(submitCode).toHaveBeenCalled()); + + // Still on the code step, still ticked — one render throughout, so this + // asserts the component's real state rather than a fresh mount's default. + expect(screen.getByLabelText("Enter the 6-digit code")).toBeDefined(); + expect(screen.getByLabelText("Keep me signed in").getAttribute("data-state")).toBe("checked"); + }); + + it("forgets the remember-me choice when the user changes address", async () => { + // `backToEmail()` resets the step IN PLACE — the component is not + // unmounted — so nothing clears this for us. Without the explicit reset the + // next person to sign in from this browser inherits a long-session choice + // they never made. + // + // Stays in ONE render on purpose: tearing down and re-rendering would + // destroy the state under test and the assertion would pass no matter what + // `backToEmail` does. + const { submitCode } = await reachCodeStep(); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + expect(screen.getByLabelText("Keep me signed in").getAttribute("data-state")).toBe("checked"); + + fireEvent.click(screen.getByRole("button", { name: "Change email" })); + await waitFor(() => expect(screen.queryByLabelText("Enter the 6-digit code")).toBeNull()); + + // Same component instance, second address. + await enterAddress("another@example.test"); + submitCodeValue("123456"); + + // Asserted on what the submit handler receives, not on the checkbox's own + // state: the value reaching the backend is the thing that matters. + await waitFor(() => expect(submitCode).toHaveBeenCalledWith("another@example.test", "123456", false)); + }); + + it("forgets the choice when a lost binding sends the flow back to step one", async () => { + // The second in-place return to the address step. Same hazard, different + // branch — and a branch nothing else in this file exercises with the box. + // First submit loses the binding, second succeeds — hence the explicit + // return type, so `{}` on the happy path is not narrowed away. + const submitCode = + vi.fn<(email: string, code: string, remember: boolean) => Promise<{ error?: string }>>(); + submitCode.mockResolvedValueOnce({ error: "otp_session_mismatch" }).mockResolvedValue({}); + await reachCodeStep({ submitCode }); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + submitCodeValue("123456"); + + // The lost binding drops the flow back to the address step. + await waitFor(() => expect(screen.queryByLabelText("Enter the 6-digit code")).toBeNull()); + + // Same component instance, walked to step two a second time. + await enterAddress("someone@example.test"); + submitCodeValue("654321"); + + await waitFor(() => expect(submitCode).toHaveBeenLastCalledWith("someone@example.test", "654321", false)); }); it("refuses a short code locally rather than spending a backend attempt", async () => { // Only five attempts exist per code; a 3-digit submit must not burn one. - const submitCode = vi.fn(async () => ({})); - await reachCodeStep(); - cleanup(); - setup({ submitCode }); - fireEvent.change(screen.getByLabelText("Email code"), { - target: { value: "someone@example.test" }, - }); - fireEvent.click(screen.getByRole("button", { name: "Email me a code" })); - await waitFor(() => screen.getByLabelText("Enter the 6-digit code")); + const { submitCode } = await reachCodeStep({ submitCode: vi.fn(async () => ({})) }); fireEvent.change(screen.getByLabelText("Enter the 6-digit code"), { target: { value: "123" }, }); diff --git a/src/features/auth/__tests__/password-sign-in-form.test.tsx b/src/features/auth/__tests__/password-sign-in-form.test.tsx index 24f0614..90503fe 100644 --- a/src/features/auth/__tests__/password-sign-in-form.test.tsx +++ b/src/features/auth/__tests__/password-sign-in-form.test.tsx @@ -4,6 +4,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { PasswordSignInForm } from "@/features/auth/password-sign-in-form"; +// jsdom implements no ResizeObserver, and Radix's Checkbox measures itself via +// `useSize`. Without this every test in the file dies on render rather than on +// an assertion. Inline rather than a shared helper: `src/features/**` may not +// import `@/shared/testing/**` (eslint no-restricted-imports), and that +// boundary is worth more than deduplicating six lines. Same stub as +// `src/features/settings/__tests__/timezone-card.test.tsx`. +globalThis.ResizeObserver ??= class { + observe() {} + unobserve() {} + disconnect() {} +} as unknown as typeof ResizeObserver; + afterEach(() => cleanup()); function setup(submit = vi.fn(async () => ({}) as { error?: string })) { @@ -21,7 +33,27 @@ describe("password sign-in form", () => { fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); fireEvent.click(screen.getByRole("button", { name: "Sign in" })); - await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2")); + await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2", false)); + }); + + it("sends the remember-me choice with the credentials", async () => { + // RUK-290. The box is a session-length REQUEST; the whole point is that it + // arrives. Asserted on the submit handler rather than on the checkbox's own + // state, because a checkbox that toggles but is never read looks identical. + const submit = setup(); + + fireEvent.change(screen.getByLabelText("Email"), { target: { value: "admin@example.test" } }); + fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); + fireEvent.click(screen.getByLabelText("Keep me signed in")); + fireEvent.click(screen.getByRole("button", { name: "Sign in" })); + + await waitFor(() => expect(submit).toHaveBeenCalledWith("admin@example.test", "hunter2", true)); + }); + + it("starts unticked, so a shared computer is the default", () => { + setup(); + + expect(screen.getByLabelText("Keep me signed in").getAttribute("data-state")).toBe("unchecked"); }); it("stays disabled until both fields are filled", () => { diff --git a/src/features/auth/login-page.tsx b/src/features/auth/login-page.tsx index 5365a05..0a065b1 100644 --- a/src/features/auth/login-page.tsx +++ b/src/features/auth/login-page.tsx @@ -35,9 +35,15 @@ export interface LoginPageProps { signInAction: (providerId: string) => Promise; /** Step one of the OTP flow: mails a code and binds it to this browser. */ requestOtpAction: (email: string) => Promise<{ error?: string }>; - /** Step two, and the password form: establishes the session. */ - otpSignInAction: (email: string, code: string) => Promise<{ error?: string }>; - passwordSignInAction: (email: string, password: string) => Promise<{ error?: string }>; + /** + * Step two, and the password form: establishes the session. + * + * `rememberMe` is required rather than optional (RUK-290). Optional would let + * a caller omit it and feed `undefined` into a chain typed `boolean`, with no + * compiler error anywhere along the way. + */ + otpSignInAction: (email: string, code: string, rememberMe: boolean) => Promise<{ error?: string }>; + passwordSignInAction: (email: string, password: string, rememberMe: boolean) => Promise<{ error?: string }>; /** Abandons the current OTP flow so another address can be used. */ changeEmailAction: () => Promise; /** Step one of the password reset (RUK-289): mails a code, binds this browser. */ diff --git a/src/features/auth/otp-sign-in-flow.tsx b/src/features/auth/otp-sign-in-flow.tsx index a9adf6c..d50bc4f 100644 --- a/src/features/auth/otp-sign-in-flow.tsx +++ b/src/features/auth/otp-sign-in-flow.tsx @@ -3,6 +3,7 @@ import { useCallback, useState } from "react"; import { Button } from "@/shared/ui/shadcn/button"; +import { Checkbox } from "@/shared/ui/shadcn/checkbox"; import { Input } from "@/shared/ui/shadcn/input"; import { Label } from "@/shared/ui/shadcn/label"; import { isWellFormedOtpCode } from "@/domain/auth/sign-in-method"; @@ -22,7 +23,11 @@ type Step = "email" | "code"; export interface OtpSignInFlowProps { label: string; requestCode: (email: string) => Promise<{ error?: string }>; - submitCode: (email: string, code: string) => Promise<{ error?: string }>; + /** + * `rememberMe` is a required third argument (RUK-290) — optional would let a + * caller pass `undefined` into a chain typed `boolean` with no compiler error. + */ + submitCode: (email: string, code: string, rememberMe: boolean) => Promise<{ error?: string }>; onChangeEmail: () => Promise; } @@ -30,6 +35,9 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: const [step, setStep] = useState("email"); const [email, setEmail] = useState(""); const [code, setCode] = useState(""); + // Belongs to step two: this is the request that mints the session. Asking on + // the address step would attach the choice to a request that issues no token. + const [rememberMe, setRememberMe] = useState(false); const [error, setError] = useState(); const [pending, setPending] = useState(false); @@ -69,7 +77,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: const result = await timers.guard(async () => { setPending(true); setError(undefined); - const outcome = await submitCode(email, code.trim()); + const outcome = await submitCode(email, code.trim(), rememberMe); setPending(false); return outcome; }); @@ -83,6 +91,10 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: // is the primary action and nothing is throttled. setStep("email"); setCode(""); + // Reset explicitly: the step changes in place, the component is not + // unmounted, so nothing clears this for us. Returning to step one is a + // fresh sign-in and must not silently inherit the previous choice. + setRememberMe(false); timers.reset(); } setError(result.error); @@ -92,6 +104,7 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }: await onChangeEmail(); setStep("email"); setCode(""); + setRememberMe(false); setError(undefined); timers.reset(); } @@ -158,9 +171,23 @@ export function OtpSignInFlow({ label, requestCode, submitCode, onChangeEmail }:

)} {!expired ? ( - + <> +
+ setRememberMe(checked === true)} + /> + {/* Names no duration on purpose — see the password form. */} + +
+ + ) : null}