diff --git a/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx b/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx index 23592a35..884b5019 100644 --- a/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx +++ b/clients/web/src/test/core/react/useClientSettingsDraft.test.tsx @@ -6,7 +6,8 @@ * persist, synchronous flush-on-close, and reset-on-close behaviors. */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, render, renderHook } from "@testing-library/react"; +import { useEffect } from "react"; import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft"; interface SettingsShape { @@ -422,4 +423,43 @@ describe("useClientSettingsDraft", () => { }); expect(result.current.flush).toBe(flushAfterMount); }); + it("never commits the previous session's draft after reopening", () => { + // Same reasoning as the `useSettingsDraft` counterpart: the regression is + // a committed frame, not an end state. `rerender()` flushes the old + // seeding effect before any assertion on `result.current`, so an + // effect-seeded implementation passes those too. Record every commit from + // inside the tree instead, and assert the previous session's draft is + // never committed while the new one is open. + const commits: Array<{ opened: boolean; text: string | null }> = []; + let nextInitial: SettingsShape = { text: "first-session", rows: [] }; + + function Harness({ opened }: { opened: boolean }) { + const { draft } = useClientSettingsDraft({ + opened, + resolveInitial: () => nextInitial, + onPersist: vi.fn(), + onError: vi.fn(), + }); + useEffect(() => { + commits.push({ opened, text: draft?.text ?? null }); + }); + return null; + } + + const { rerender } = render(); + expect(commits).toContainEqual({ opened: true, text: "first-session" }); + + rerender(); + + // Reopen against different saved config — the modal must never paint the + // values from the session that just closed. + nextInitial = { text: "second-session", rows: [] }; + commits.length = 0; + rerender(); + + expect(commits.length).toBeGreaterThan(0); + for (const commit of commits) { + expect(commit).toEqual({ opened: true, text: "second-session" }); + } + }); }); diff --git a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx index 65c1780a..d346677d 100644 --- a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx +++ b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx @@ -189,7 +189,7 @@ describe("useEmaIdpLoginState", () => { ).not.toHaveBeenCalled(); expect(result.current.loginState).toBe("none"); }); - it("swallows a session-read rejection and leaves loginState unchanged", async () => { + it("a failed read for a new issuer reports none, not the old issuer's state", async () => { const exp = Math.floor(Date.now() / 1000) + 3600; const payload = btoa(JSON.stringify({ exp })) .replace(/\+/g, "-") @@ -209,9 +209,10 @@ describe("useEmaIdpLoginState", () => { expect(result.current.loginState).toBe("logged_in"); }); - // The storage backend goes away. The effect-triggered refresh must not - // surface an unhandled rejection, and must leave the last state we - // actually read in place rather than falsely reporting signed-out. + // The storage backend goes away *and* the issuer changes. The rejection + // must not surface as an unhandled promise, and the new issuer must not + // inherit the previous one's "logged_in" — nobody has authenticated + // against it, and a read that never succeeded cannot say otherwise. const unhandled = vi.fn(); process.on("unhandledRejection", unhandled); try { @@ -230,6 +231,113 @@ describe("useEmaIdpLoginState", () => { "https://other-idp.test", ); expect(unhandled).not.toHaveBeenCalled(); + expect(result.current.loginState).toBe("none"); + }); + + it("keeps the last answer when a re-read of the same issuer fails", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + vi.mocked(storage.getIdpSession).mockResolvedValue({ + idToken: `h.${payload}.s`, + }); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + await waitFor(() => { + expect(result.current.loginState).toBe("logged_in"); + }); + + // A transient storage outage is not evidence that the session ended, so + // the issuer we are still looking at keeps the answer we actually got. + vi.mocked(storage.getIdpSession).mockRejectedValue( + new Error("storage unreachable"), + ); + await act(async () => { + await result.current.refresh().catch(() => {}); + }); + expect(result.current.loginState).toBe("logged_in"); }); + + it("ignores an overtaken read that resolves last for the same issuer", async () => { + const jwt = (secondsFromNow: number) => { + const payload = btoa( + JSON.stringify({ exp: Math.floor(Date.now() / 1000) + secondsFromNow }), + ) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + return `h.${payload}.s`; + }; + + // Two reads for the *same* issuer overlap, and the older one resolves + // last. Issuer keying cannot tell them apart — both carry this issuer — + // so only the read token stops the superseded answer from committing. + let releaseFirst: () => void = () => {}; + const first = new Promise<{ idToken: string }>((resolve) => { + releaseFirst = () => resolve({ idToken: jwt(3600) }); + }); + vi.mocked(storage.getIdpSession) + .mockReturnValueOnce(first) + .mockResolvedValue({ idToken: jwt(-3600) }); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + // Second read starts and finishes while the first is still pending. + await act(async () => { + await result.current.refresh(); + }); + expect(result.current.loginState).toBe("expired"); + + await act(async () => { + releaseFirst(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(result.current.loginState).toBe("expired"); + }); + + it("a logout supersedes a read that was already in flight", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + + let releaseRead: () => void = () => {}; + const pending = new Promise<{ idToken: string }>((resolve) => { + releaseRead = () => resolve({ idToken: `h.${payload}.s` }); + }); + vi.mocked(storage.getIdpSession).mockReturnValue(pending); + + const { result } = renderHook(() => + useEmaIdpLoginState(storage, "https://idp.test", true), + ); + + act(() => { + result.current.logout(); + }); + // Let the clear settle on its own before releasing the read, so the read + // is unambiguously the *last* writer — without the token it would land on + // top of the clear and report the cleared session as logged in. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + expect(result.current.loginState).toBe("none"); + + await act(async () => { + releaseRead(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(storage.clearIdpSession).toHaveBeenCalledWith("https://idp.test"); + expect(result.current.loginState).toBe("none"); + }); }); diff --git a/clients/web/src/test/core/react/useSettingsDraft.test.tsx b/clients/web/src/test/core/react/useSettingsDraft.test.tsx index aa34f202..451a166e 100644 --- a/clients/web/src/test/core/react/useSettingsDraft.test.tsx +++ b/clients/web/src/test/core/react/useSettingsDraft.test.tsx @@ -6,7 +6,8 @@ * appear to eat keystrokes. The PUT itself debounces. */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { act, renderHook } from "@testing-library/react"; +import { act, render, renderHook } from "@testing-library/react"; +import { useEffect } from "react"; import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft"; interface SettingsShape { @@ -375,4 +376,46 @@ describe("useSettingsDraft", () => { }); expect(result.current.flush).toBe(flushAfterMount); }); + + it("never commits the previous target's draft for a new target", () => { + // The regression this pins is a *frame*, not an end state, so it cannot + // be observed through `result.current`: RTL's `act` flushes passive + // effects before the assertion runs, so an effect-seeded implementation + // reaches the same final value and passes any check made after the + // rerender settles. What distinguishes them is what was *committed* on + // the way there — an effect renders once with the previous target's draft + // and paints it before correcting itself. + // + // So record every commit from inside the tree and assert no committed + // frame ever paired "beta" with alpha's draft. + const commits: Array<{ targetId: string; text: string | null }> = []; + const SETTINGS: Record = { + alpha: { text: "alpha-value", rows: [] }, + beta: { text: "beta-value", rows: [] }, + }; + + function Harness({ targetId }: { targetId: string }) { + const { draft } = useSettingsDraft({ + targetId, + resolveInitial: (id) => SETTINGS[id] ?? EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }); + useEffect(() => { + commits.push({ targetId, text: draft?.text ?? null }); + }); + return null; + } + + const { rerender } = render(); + expect(commits).toContainEqual({ targetId: "alpha", text: "alpha-value" }); + + commits.length = 0; + rerender(); + + expect(commits.length).toBeGreaterThan(0); + for (const commit of commits) { + expect(commit).toEqual({ targetId: "beta", text: "beta-value" }); + } + }); }); diff --git a/core/react/useEmaIdpLoginState.ts b/core/react/useEmaIdpLoginState.ts index d03d9853..5af72c1c 100644 --- a/core/react/useEmaIdpLoginState.ts +++ b/core/react/useEmaIdpLoginState.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import type { OAuthStorage } from "../auth/storage.js"; import { clearEmaIdpSession, @@ -13,6 +13,21 @@ export interface UseEmaIdpLoginStateResult { logout: () => void; } +/** + * What a completed read told us, and *which issuer* it was told about. + * + * The pair is stored together rather than as a bare `EmaIdpLoginState` + * because a login state is only ever meaningful for the issuer it was read + * for. With a bare value, changing issuer carries the previous issuer's + * answer over to the new one until a fresh read lands — and if that read + * fails, indefinitely, so the settings surface reports "logged in" for an + * IdP nobody has authenticated against. + */ +interface ReadResult { + issuer: string; + value: EmaIdpLoginState; +} + /** * Read and clear EMA IdP session state for Client Settings UX. * Pass `active: true` while the settings surface is open to refresh on open. @@ -23,48 +38,67 @@ export function useEmaIdpLoginState( active: boolean, ): UseEmaIdpLoginStateResult { const normalizedIssuer = issuer ? normalizeIdpIssuer(issuer) : ""; - const [loginState, setLoginState] = useState("none"); + const [read, setRead] = useState({ issuer: "", value: "none" }); + + /** + * Monotonic read token — the same device `useInitialConfig` uses. Reads are + * unordered, so an issuer flipped A → B → A can have the first A read + * resolve last; a completion may commit only while it still holds the + * current token. `logout` claims one too, so a read already in flight when + * the session is cleared cannot land on top of the clear. + */ + const generation = useRef(0); + + // Derived rather than stored: with no issuer there is nothing to be logged + // in to, and a result read for some *other* issuer says nothing about this + // one. A mismatch therefore reports "none" instead of the previous issuer's + // answer, which is also what makes the failure path below safe. + const loginState: EmaIdpLoginState = + normalizedIssuer && read.issuer === normalizedIssuer ? read.value : "none"; const refresh = useCallback(async () => { - if (!normalizedIssuer) { - setLoginState("none"); - return; - } - setLoginState(await getEmaIdpLoginState(storage, normalizedIssuer)); + const mine = ++generation.current; + if (!normalizedIssuer) return; + const value = await getEmaIdpLoginState(storage, normalizedIssuer); + if (generation.current !== mine) return; + setRead({ issuer: normalizedIssuer, value }); }, [storage, normalizedIssuer]); useEffect(() => { if (!active) return; // Reading the persisted IdP session *is* the external system this effect - // exists to synchronize with, and the read is asynchronous: the - // `loginState` commit happens in a continuation after - // `getEmaIdpLoginState` resolves. `set-state-in-effect` follows the call - // into `refresh` without modelling the `await`, so it reports every - // "fetch on open" effect alike — verified against a minimal hook whose - // only setState is after an `await`. The one commit here that *is* - // synchronous is the no-issuer reset to "none", which settles in a single - // extra render rather than cascading. - // eslint-disable-next-line react-hooks/set-state-in-effect -- async read of an external store + // exists to synchronize with, and no commit happens here: `refresh` + // returns before its first `await` when there is no issuer, and otherwise + // commits in a continuation. That is also why this needs no + // `set-state-in-effect` suppression, where `useServers` and + // `useInitialConfig` do — deriving `loginState` from an issuer-keyed read + // took the last synchronous setState out of the path. refresh().catch(() => { // `refresh` does not own its failures: `getEmaIdpLoginState` awaits // `storage.getIdpSession`, which rejects when the backend is // unreachable. Swallow it here so it does not surface as an unhandled - // rejection, and leave `loginState` untouched — the same choice `logout` - // makes, so the UI keeps reflecting the last state we actually read - // rather than falsely reporting signed-out. + // rejection, and commit nothing — the issuer keying above is what makes + // that safe. A failed read for a *new* issuer reports "none", because + // no result for that issuer exists; a failed re-read of the issuer + // already on screen keeps the last answer we actually got, which is the + // right reading of a transient storage outage. }); }, [active, refresh]); const logout = useCallback(() => { if (!normalizedIssuer) return; + // Supersede any read in flight: it was started before the session was + // cleared, so its answer is already wrong by the time it resolves. + const mine = ++generation.current; void clearEmaIdpSession(storage, normalizedIssuer) .then(() => { - setLoginState("none"); + if (generation.current !== mine) return; + setRead({ issuer: normalizedIssuer, value: "none" }); }) .catch(() => { // Clearing the persisted IdP session failed (e.g. the storage backend // is unreachable). Swallow the rejection so it does not surface as an - // unhandled promise, and leave loginState unchanged so the UI keeps + // unhandled promise, and leave the last read in place so the UI keeps // reflecting the still-present session rather than falsely showing // signed-out. });