diff --git a/SW.Bitween.Web/ClientApp/e2e/sign-out.spec.ts b/SW.Bitween.Web/ClientApp/e2e/sign-out.spec.ts new file mode 100644 index 00000000..60b11b84 --- /dev/null +++ b/SW.Bitween.Web/ClientApp/e2e/sign-out.spec.ts @@ -0,0 +1,151 @@ +import { expect, test, type Page } from "@playwright/test"; +import { ADMIN_EMAIL, ADMIN_PASSWORD } from "./helpers"; + +/** + * Signing out, and the four ways it used to go wrong. + * + * The session lives in four places — a row in the database, the HttpOnly refresh + * cookie, the Jwt in localStorage, and React's own copy. Only the last one decides + * what you see, and it used to be the one thing a sign-out could fail to clear: + * `signOut` awaited the server first, so any failure threw before the session was + * ended and left the whole app on screen with the Jwt already deleted. Every test + * here is a way that could happen. + */ + +const submitCredentials = async (page: Page) => { + await page.fill("#login-email", ADMIN_EMAIL); + await page.fill("#login-password", ADMIN_PASSWORD); + await page.getByRole("button", { name: /^Sign in$/ }).click(); +}; + +async function signIn(page: Page) { + await page.goto("login"); + await submitCredentials(page); + await page.waitForURL((u) => !u.pathname.endsWith("/login"), { timeout: 15000 }); +} + +const signOut = async (page: Page) => { + await page.getByRole("button", { name: "Account menu" }).click(); + await page.getByRole("button", { name: /Sign out/ }).click(); +}; + +/** The signed-in shell: present only while React holds a session. */ +const shell = (page: Page) => page.getByRole("button", { name: "Account menu" }); + +test("the login page appears without waiting for the server", async ({ page }) => { + await signIn(page); + await page.route("**/api/accounts/logout", async (r) => { + await new Promise((res) => setTimeout(res, 3000)); + await r.continue(); + }); + const started = Date.now(); + await signOut(page); + // Held for 3s on purpose: the session ends locally first, so nothing waits on it. + await page.waitForURL(/\/login/, { timeout: 2500 }); + expect(Date.now() - started).toBeLessThan(2500); +}); + +test("a refused sign-out still signs you out", async ({ page }) => { + await signIn(page); + await page.route("**/api/accounts/logout", (r) => r.fulfill({ status: 500, body: "boom" })); + await signOut(page); + await page.waitForURL(/\/login/, { timeout: 5000 }); + expect(await page.evaluate(() => localStorage.getItem("access_token"))).toBeNull(); +}); + +test("an unreachable backend still signs you out", async ({ page }) => { + await signIn(page); + await page.route("**/api/accounts/logout", (r) => r.abort("connectionrefused")); + await signOut(page); + await page.waitForURL(/\/login/, { timeout: 5000 }); +}); + +test("signing out in one tab ends the session in the other", async ({ page, context }) => { + await signIn(page); + const other = await context.newPage(); + await other.goto("https://localhost:7155/partners"); + await other.waitForTimeout(2000); + expect(await shell(other).count()).toBe(1); + + await signOut(page); + await page.waitForURL(/\/login/, { timeout: 5000 }); + // Both credentials are shared by every tab, so the session really has ended here + // too — this tab just never used to find out until someone pressed refresh. + await other.waitForURL(/\/login/, { timeout: 8000 }); +}); + +test("a sign-out that never answers does not block signing back in", async ({ page }) => { + await signIn(page); + await page.route("**/api/accounts/logout", () => { + /* never fulfilled: the request hangs rather than failing */ + }); + await signOut(page); + await page.waitForURL(/\/login/, { timeout: 5000 }); + + // A sign-in cancels the pending sign-out rather than waiting for it. Waiting was + // the obvious guard against the race in the next test, and would have deadlocked + // here for as long as the request hung. + await submitCredentials(page); + await page.waitForURL((u) => !u.pathname.endsWith("/login"), { timeout: 8000 }); + expect(await shell(page).count()).toBe(1); +}); + +test("a slow sign-out response cannot wipe the session that replaced it", async ({ page }) => { + await signIn(page); + // The logout response carries `Clear-Site-Data: "cookies", "storage"`, which the + // browser applies to the whole origin whenever it lands — including over a newer + // sign-in. Cancelling the request means the response never arrives. + await page.route("**/api/accounts/logout", async (r) => { + await new Promise((res) => setTimeout(res, 4000)); + await r.continue(); + }); + await signOut(page); + await page.waitForURL(/\/login/, { timeout: 5000 }); + await submitCredentials(page); + await page.waitForURL((u) => !u.pathname.endsWith("/login"), { timeout: 10000 }); + + await page.waitForTimeout(5000); // outlast the delayed response + expect(page.url()).not.toContain("/login"); + expect(await page.evaluate(() => localStorage.getItem("access_token"))).not.toBeNull(); + await page.goto("partners"); + await page.waitForTimeout(1500); + expect(await shell(page).count()).toBe(1); +}); + +test("a slow session read cannot flash the app back after a sign-out", async ({ page, context }) => { + await signIn(page); + + // Held open so it lands after the sign-out below. It returns 200 — the Jwt went + // out before the sign-out and is still valid — so its result must be discarded on + // arrival rather than trusted, or the app appears again over a dead session. + await page.route("**/api/accounts/profile", async (r) => { + await new Promise((res) => setTimeout(res, 4000)); + await r.continue(); + }); + const reloading = page.reload(); + await page.waitForTimeout(800); + + const other = await context.newPage(); + await other.goto("https://localhost:7155/partners"); + await other.waitForTimeout(1500); + await signOut(other); + await other.waitForURL(/\/login/, { timeout: 8000 }); + + let everSignedIn = false; + for (let i = 0; i < 60; i++) { + if (await shell(page).count()) everSignedIn = true; + await page.waitForTimeout(100); + } + await reloading.catch(() => {}); + expect(everSignedIn).toBe(false); + expect(page.url()).toContain("/login"); +}); + +test("a session that ended server-side does not need a refresh", async ({ page }) => { + await signIn(page); + // Exactly what a sign-out elsewhere leaves behind: no cookie, a useless Jwt. + await page.context().clearCookies(); + await page.evaluate(() => localStorage.setItem("access_token", "dead")); + await page.getByRole("link", { name: /^Partners$/ }).click(); + await page.waitForURL(/\/login/, { timeout: 10000 }); +}); diff --git a/SW.Bitween.Web/ClientApp/src/api/http/request.ts b/SW.Bitween.Web/ClientApp/src/api/http/request.ts index 67214946..415ef585 100644 --- a/SW.Bitween.Web/ClientApp/src/api/http/request.ts +++ b/SW.Bitween.Web/ClientApp/src/api/http/request.ts @@ -7,7 +7,22 @@ import { ApiRequestError } from "../types"; export const API_BASE = "/api"; /** The Jwt is kept in localStorage; the refresh token is an HttpOnly cookie JS never sees. */ -const TOKEN_KEY = "access_token"; +export const TOKEN_KEY = "access_token"; + +/** + * Told when a request proves the session is over — the Jwt was refused and no + * refresh cookie was left to replace it. + * + * A callback rather than a hook because this is plain module code that cannot + * reach React state. `SessionProvider` registers itself on mount; with nothing + * registered, the `UNAUTHENTICATED` error below is all that happens, which is + * what used to leave a dead session rendering the entire app until somebody + * pressed refresh. + */ +let sessionEndedListener: (() => void) | null = null; +export const onSessionEnded = (listener: (() => void) | null): void => { + sessionEndedListener = listener; +}; export const getToken = (): string | null => localStorage.getItem(TOKEN_KEY); export const setToken = (jwt: string): void => localStorage.setItem(TOKEN_KEY, jwt); @@ -88,6 +103,8 @@ export interface RequestOptions { body?: unknown; /** Internal: prevents the 401 → refresh → retry loop from recursing. */ _retried?: boolean; + /** Lets a caller cancel the request — only `logout` needs this, and says why. */ + signal?: AbortSignal; } /** @@ -111,12 +128,16 @@ export async function request(path: string, opts: RequestOptions = {}): Promi ...(token ? { Authorization: `Bearer ${token}` } : {}), }, body: sendJson ? JSON.stringify(opts.body ?? {}) : undefined, + ...(opts.signal ? { signal: opts.signal } : {}), }); if (res.status === 401 && !opts._retried) { const refreshed = await silentRefresh(); if (refreshed) return request(path, { ...opts, _retried: true }); clearToken(); + // Tell the app, not only the caller. Every page with a read in flight is about + // to render its own small error, and a page's error state cannot end a session. + sessionEndedListener?.(); throw new ApiRequestError("UNAUTHENTICATED", "Your session has ended. Please sign in again."); } diff --git a/SW.Bitween.Web/ClientApp/src/api/http/session.ts b/SW.Bitween.Web/ClientApp/src/api/http/session.ts index 88076776..7c4c4378 100644 --- a/SW.Bitween.Web/ClientApp/src/api/http/session.ts +++ b/SW.Bitween.Web/ClientApp/src/api/http/session.ts @@ -1,6 +1,6 @@ import { ApiRequestError, type Session, type User } from "../types"; import { getAppConfig } from "./appConfig"; -import { clearToken, get, getToken, post, setToken } from "./request"; +import { clearToken, get, getToken, post, request, setToken } from "./request"; /** GET /accounts/profile — camelCase ProfileModel. */ interface Profile { @@ -42,6 +42,26 @@ const buildSession = (profile: Profile): Session => { const loadSession = async (): Promise => buildSession(await get("/accounts/profile")); +/** + * The sign-out request that may still be in flight, so a sign-in can cancel it. + * + * Its response carries `Clear-Site-Data: "cookies", "storage"`, which the browser + * applies to the whole origin the moment it arrives — so landing *after* a fresh + * sign-in, it wipes that sign-in's Jwt and refresh cookie and throws the user back + * to the page they just left. The two genuinely can overlap now that signing out + * no longer blocks the UI: the login page appears at once, and a password manager + * can fill and submit it before the logout lands. + * + * Waiting for the logout first was the obvious guard and the wrong one — a request + * that hangs rather than fails would block sign-in for as long as it hung. Nothing + * in the response is worth waiting for: it was already sent, so the server deletes + * the refresh token either way, and only the header we don't want is discarded. + */ +let logoutInFlight: AbortController | null = null; + +/** A sign-in supersedes any sign-out still in flight. */ +const abortPendingLogout = () => logoutInFlight?.abort(); + export const sessionMethods = { async getSession(): Promise { // No stored Jwt → anonymous; don't probe the backend (an expired token still @@ -57,6 +77,7 @@ export const sessionMethods = { }, async login(email: string, password: string): Promise { + abortPendingLogout(); const { jwt } = await post("/accounts/login", { Username: email, Password: password, @@ -66,6 +87,7 @@ export const sessionMethods = { }, async loginWithMicrosoft(): Promise { + abortPendingLogout(); const cfg = await getAppConfig(); if (!cfg.msalClientId) throw new ApiRequestError("MS_NOT_CONFIGURED", "Microsoft sign-in isn't configured."); @@ -94,10 +116,24 @@ export const sessionMethods = { }, async logout(): Promise { + // Cleared first and unconditionally. This used to run in a `finally`, which + // deleted the Jwt and then let the error through — the worst pairing, because + // the caller aborted before it could end the session and the app carried on + // rendering as if signed in. Removing the key here is also what wakes the + // other tabs (see the `storage` listener in SessionContext). + clearToken(); + const controller = new AbortController(); + logoutInFlight = controller; try { - await post("/accounts/logout"); + await request("/accounts/logout", { method: "POST", signal: controller.signal }); + } catch { + // Swallowed on purpose: signing out must not depend on the server answering, + // and a sign-in aborting this is a normal outcome rather than a fault. Only + // the server can invalidate the refresh cookie — it is HttpOnly, so JS cannot + // touch it — so if this never lands the cookie outlives the sign-out. This + // browser has no Jwt to pair with it, and the next sign-in replaces it. } finally { - clearToken(); + if (logoutInFlight === controller) logoutInFlight = null; } }, diff --git a/SW.Bitween.Web/ClientApp/src/api/index.ts b/SW.Bitween.Web/ClientApp/src/api/index.ts index 31dadb7d..5e8fa302 100644 --- a/SW.Bitween.Web/ClientApp/src/api/index.ts +++ b/SW.Bitween.Web/ClientApp/src/api/index.ts @@ -12,4 +12,5 @@ export const api: ApiClient = httpClient; export { getAppConfig, resetAppConfig } from "./http/appConfig"; export type { AppConfig } from "./http/appConfig"; export { referencesGlobal, referencesPartnerProp } from "./http/references"; +export { TOKEN_KEY, onSessionEnded } from "./http/request"; export * from "./types"; diff --git a/SW.Bitween.Web/ClientApp/src/auth/SessionContext.tsx b/SW.Bitween.Web/ClientApp/src/auth/SessionContext.tsx index 8f49f237..18088f4e 100644 --- a/SW.Bitween.Web/ClientApp/src/auth/SessionContext.tsx +++ b/SW.Bitween.Web/ClientApp/src/auth/SessionContext.tsx @@ -4,11 +4,12 @@ import { useContext, useEffect, useMemo, + useRef, useState, type ReactNode, } from "react"; import { useQueryClient } from "@tanstack/react-query"; -import { api, type PermissionKey, type Session } from "../api"; +import { api, TOKEN_KEY, onSessionEnded, type PermissionKey, type Session } from "../api"; import { useIdleLogout } from "./useIdleLogout"; interface SessionContextValue { @@ -31,17 +32,30 @@ export function SessionProvider({ children }: { children: ReactNode }) { const [session, setSession] = useState(null); const [initializing, setInitializing] = useState(true); const queryClient = useQueryClient(); + /** + * Bumped whenever the session is authoritatively replaced or ended. + * + * `getSession()` cannot be cancelled, so a slow one can resolve *after* a + * sign-out and hand `setSession` the very session that was just discarded — + * signing the user back in. Each caller notes the generation it started under + * and drops its own result if that is no longer current. + */ + const generation = useRef(0); useEffect(() => { + const startedAt = generation.current; api .getSession() - .then(setSession) + .then((next) => { + if (generation.current === startedAt) setSession(next); + }) .finally(() => setInitializing(false)); }, []); const adoptSession = useCallback( (next: Session) => { // a different identity invalidates everything previously fetched + generation.current += 1; queryClient.clear(); setSession(next); }, @@ -64,15 +78,64 @@ export function SessionProvider({ children }: { children: ReactNode }) { }, [adoptSession]); const refresh = useCallback(async () => { - setSession(await api.getSession()); + const startedAt = generation.current; + const next = await api.getSession(); + if (generation.current === startedAt) setSession(next); }, []); - const signOut = useCallback(async () => { - await api.logout(); - queryClient.clear(); + /** Ends the session locally, then tells the server. Never the other way round. */ + const endSession = useCallback(() => { + generation.current += 1; setSession(null); + queryClient.clear(); }, [queryClient]); + const signOut = useCallback(async () => { + // The session ends here, whatever the server says. `await api.logout()` used to + // come first, so any failure — a backend restart, a blip, an unreachable host — + // threw before the two lines below ran: signed out everywhere except the one + // place that decides what you see, which then needed a manual refresh. Ending it + // first also means the login page appears immediately rather than after the + // round trip, which is what made signing out feel slow. + endSession(); + await api.logout(); + }, [endSession]); + + /** + * Ends the session here when it ends in another tab. + * + * Both credentials are shared by every tab of this origin — the Jwt in + * localStorage and the refresh cookie — so a sign-out in one tab really has ended + * the session in all of them; the others simply never found out and kept rendering + * the whole app over reads that could only fail. `storage` fires only in the + * *other* tabs, which is exactly the audience. A null key means the entire store + * was cleared, which the logout response's `Clear-Site-Data` does, so that counts + * too; a key with a new value is another tab signing *in*, which doesn't. + */ + useEffect(() => { + const onStorage = (e: StorageEvent) => { + if (e.key !== null && e.key !== TOKEN_KEY) return; + if (e.newValue) return; + endSession(); + }; + window.addEventListener("storage", onStorage); + return () => window.removeEventListener("storage", onStorage); + }, [endSession]); + + /** + * Ends the session here when a request proves it is already over. + * + * Covers what no `storage` event can: a Jwt and refresh cookie that both expired + * while a tab sat open, or an administrator disabling the account. `request()` + * already knew — it writes "Your session has ended" — but it could only throw that + * at whichever page happened to be asking, and a page's error state cannot sign + * anyone out. + */ + useEffect(() => { + onSessionEnded(endSession); + return () => onSessionEnded(null); + }, [endSession]); + // Finding #4 in the 9USRCraft pen test: an authenticated session stayed usable // indefinitely. Signs out after 30 minutes with no activity in any tab. useIdleLogout(!!session, signOut); diff --git a/SW.Bitween.Web/ClientApp/src/auth/useIdleLogout.ts b/SW.Bitween.Web/ClientApp/src/auth/useIdleLogout.ts index ba1130e3..53e60537 100644 --- a/SW.Bitween.Web/ClientApp/src/auth/useIdleLogout.ts +++ b/SW.Bitween.Web/ClientApp/src/auth/useIdleLogout.ts @@ -22,8 +22,6 @@ const writeSharedActivity = (ts: number) => { } }; -const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); - /** * Signs the user out after a period with no activity in any tab. Runs the normal * sign-out path, so an idle session ends exactly like clicking Sign out. @@ -67,20 +65,10 @@ export function useIdleLogout( window.clearInterval(interval); ACTIVITY_EVENTS.forEach((e) => window.removeEventListener(e, markActivity)); - try { - await signOutRef.current(); - } catch { - // Only the server call invalidates the refresh-token cookie (it is HttpOnly, - // so JS cannot clear it). Retry once before giving up. - await sleep(2000); - try { - await signOutRef.current(); - } catch { - // api.logout() clears the stored Jwt in a finally, so it is already gone by - // now; reloading is what drops the in-memory session and shows the login page. - window.location.reload(); - } - } + // No retry, and nothing to reload around: `signOut` ends the session before it + // calls the server and swallows a failed call, so this cannot leave the user + // looking at the app. It used to need both. + await signOutRef.current(); }, CHECK_INTERVAL_MS); return () => { diff --git a/SW.Bitween.Web/ClientApp/src/components/mapper/MappingEditorToolbar.tsx b/SW.Bitween.Web/ClientApp/src/components/mapper/MappingEditorToolbar.tsx index 4613ea6c..fe4c725b 100644 --- a/SW.Bitween.Web/ClientApp/src/components/mapper/MappingEditorToolbar.tsx +++ b/SW.Bitween.Web/ClientApp/src/components/mapper/MappingEditorToolbar.tsx @@ -97,7 +97,7 @@ const MappingEditorToolbar: React.FC = ({
{/* Back */} + + + +
- + @@ -356,7 +356,7 @@ export function RoleEditor() { onConfirm={async () => { await api.deleteRole(id!); void queryClient.invalidateQueries({ queryKey: keys.roles.all }); - navigate("/team/roles"); + navigate("/team/roles", { replace: true }); }} onClose={() => setConfirmingDelete(false)} /> diff --git a/SW.Bitween.Web/ClientApp/src/pages/work-groups/WorkGroupPage.tsx b/SW.Bitween.Web/ClientApp/src/pages/work-groups/WorkGroupPage.tsx index 20c2347f..d2cb7dc5 100644 --- a/SW.Bitween.Web/ClientApp/src/pages/work-groups/WorkGroupPage.tsx +++ b/SW.Bitween.Web/ClientApp/src/pages/work-groups/WorkGroupPage.tsx @@ -146,7 +146,7 @@ export function WorkGroupPage() { onConfirm={async () => { await api.deleteWorkGroup(groupId); void queryClient.invalidateQueries({ queryKey: keys.workGroups.all }); - navigate("/work-groups"); + navigate("/work-groups", { replace: true }); }} onClose={() => setDeleting(false)} />