fix(ui): clear account-scoped query caches on sign-out (#11380)
Sign-out invalidated two query keys - the auth session and health - and left everything else in the cache. `invalidateQueries` also keeps serving the old data while it refetches, so it was insufficient even for the two it did touch. The company list was never touched at all, which is how one account's companies could still be in hand when the next account signed in. The self-hosted path now resets every account-scoped entry. Scoping is an allowlist of instance-scoped roots rather than a list of things to clear, so a query key added later is account-scoped unless someone deliberately says otherwise - forgetting this file fails closed. `health` is the only exemption. It carries deployment mode, bootstrap state and Cloud metadata, nothing account-scoped, and `useCloudInstance` observes it with `enabled: false` and leaves the fetch to CloudAccessGate. Dropping the entry would strand every such observer on `null` until the gate happened to refetch, flipping Cloud instances into their self-hosted rendering mid sign-out. It is refreshed in place instead. `resetQueries` rather than `removeQueries`: removal empties the cache without notifying the observers already subscribed, so a mounted `useQuery` keeps returning its last result until an unrelated re-render rebuilds it. CompanyProvider sits above the router and stays mounted across the whole sign-out and sign-in cycle, so that is the common case here rather than a corner one. Reset notifies them, so the old data leaves the cache and everything reading it. The cloud path is untouched: it is a top-level navigation, and the document reload builds a new QueryClient with nothing left to clear. This is the root cause behind the onboarding draft-ownership gate added in #11382. That gate stays, and its comment now says why: this fix covers the sign-out button, not the question. An account can change without it - a session lapsing server-side, a second account signing in on a warm tab, a caller supplying the company context from somewhere else - so the gate stays independent rather than deferring to this. ui typecheck clean; full ui suite 4014 pass, with only the timezone-dependent IssueProperties failure already present on master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
b38d6ddb81
commit
38752c4e5e
|
|
@ -496,11 +496,15 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", (
|
|||
});
|
||||
it("does not hand one account's draft to the next when a refetch fails after a switch", async () => {
|
||||
// The attack path in full. Account A onboards and leaves a draft naming
|
||||
// its company. A signs out — which does not clear the companies cache,
|
||||
// because `useSignOut` invalidates only the session and health queries.
|
||||
// B signs in and the companies refetch fails, so A's list is still in
|
||||
// its company. The account then changes without this component's company
|
||||
// cache being cleared, and the refetch fails, so A's list is still in
|
||||
// hand. A list that still contains A's company must not be read as proof
|
||||
// that B owns it.
|
||||
//
|
||||
// `useSignOut` now resets account-scoped caches, which closes the
|
||||
// sign-out-button route into this state (see its own regression test). The
|
||||
// gate is asserted here independently of that: it must hold for any route
|
||||
// that leaves a stale list behind, not only the one that has been fixed.
|
||||
window.localStorage.setItem(
|
||||
ONBOARDING_STORAGE_KEY,
|
||||
JSON.stringify({
|
||||
|
|
|
|||
|
|
@ -205,10 +205,15 @@ export function OnboardingWizard() {
|
|||
// trusts "not loading, no error" finds the old company id in them and hands
|
||||
// one account's onboarding draft to the next.
|
||||
//
|
||||
// Sign-out does not help: `useSignOut` never clears the company cache, and
|
||||
// on the self-hosted path there is no document reload to clear it either.
|
||||
// Even once that is fixed, an account change can skip the button entirely -
|
||||
// a session lapsing server-side, a second account signing in on a warm tab.
|
||||
// Sign-out is no longer the hole it was: `useSignOut` now resets every
|
||||
// account-scoped cache entry rather than invalidating two of them, so the
|
||||
// ordinary A-signs-out-then-B-signs-in path does not leave A's companies in
|
||||
// hand.
|
||||
//
|
||||
// That covers the button, not the question. An account can change without
|
||||
// it - a session lapsing server-side, a second account signing in on a warm
|
||||
// tab, a caller supplying the company context from somewhere else - so this
|
||||
// gate stays independent of that fix rather than deferring to it.
|
||||
//
|
||||
// So this asks for a list fetched *for this mount*, rather than reading the
|
||||
// one in hand. `isFetchedAfterMount` is the part that matters; `staleTime: 0`
|
||||
|
|
|
|||
|
|
@ -1,11 +1,12 @@
|
|||
// @vitest-environment jsdom
|
||||
|
||||
import { act } from "react";
|
||||
import { flushSync } from "react-dom";
|
||||
import { createRoot } from "react-dom/client";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { QueryClient, QueryClientProvider, useQuery } from "@tanstack/react-query";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { queryKeys } from "@/lib/queryKeys";
|
||||
import { useSignOut } from "./useSignOut";
|
||||
import { isAccountScopedQueryKey, useSignOut } from "./useSignOut";
|
||||
|
||||
const mockAuthApi = vi.hoisted(() => ({ signOut: vi.fn() }));
|
||||
const mockNavigateTopLevel = vi.hoisted(() => vi.fn());
|
||||
|
|
@ -75,13 +76,22 @@ describe("useSignOut", () => {
|
|||
flushSync(() => root.unmount());
|
||||
});
|
||||
|
||||
it("keeps self-hosted sign-out pending until the local request finishes, then invalidates caches", async () => {
|
||||
it("keeps self-hosted sign-out pending until the local request finishes, then clears account caches", async () => {
|
||||
let resolveSignOut: (() => void) | undefined;
|
||||
mockAuthApi.signOut.mockImplementation(() => new Promise<void>((resolve) => {
|
||||
resolveSignOut = resolve;
|
||||
}));
|
||||
queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" });
|
||||
queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } });
|
||||
queryClient.setQueryData(queryKeys.companies.all, {
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
queryClient.setQueryData(queryKeys.access.currentBoardAccess, {
|
||||
isInstanceAdmin: false,
|
||||
companyIds: ["company-a"],
|
||||
});
|
||||
queryClient.setQueryData(queryKeys.issues.list("company-a"), [{ id: "issue-a" }]);
|
||||
const onSignedOut = vi.fn();
|
||||
const root = renderHarness(onSignedOut);
|
||||
|
||||
|
|
@ -97,16 +107,101 @@ describe("useSignOut", () => {
|
|||
|
||||
expect(captured?.error).toBeNull();
|
||||
expect(onSignedOut).toHaveBeenCalledOnce();
|
||||
expect(queryClient.getQueryState(queryKeys.auth.session)?.isInvalidated).toBe(true);
|
||||
|
||||
// Account-scoped entries are gone outright, not merely marked stale. An
|
||||
// invalidated entry keeps serving its old value until a refetch succeeds,
|
||||
// which is the leak this guards.
|
||||
expect(queryClient.getQueryData(queryKeys.auth.session)).toBeUndefined();
|
||||
expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined();
|
||||
expect(queryClient.getQueryData(queryKeys.access.currentBoardAccess)).toBeUndefined();
|
||||
expect(queryClient.getQueryData(queryKeys.issues.list("company-a"))).toBeUndefined();
|
||||
|
||||
// Health describes the instance, not the account, and several observers
|
||||
// read it with `enabled: false`. It is refreshed in place.
|
||||
expect(queryClient.getQueryData(queryKeys.health)).toEqual({
|
||||
status: "ok",
|
||||
deploymentMode: "authenticated",
|
||||
});
|
||||
expect(queryClient.getQueryState(queryKeys.health)?.isInvalidated).toBe(true);
|
||||
|
||||
flushSync(() => root.unmount());
|
||||
});
|
||||
|
||||
it("exposes a stable error without closing chrome or invalidating caches", async () => {
|
||||
it("leaves the previous account nothing to read when the next account's refetch fails", async () => {
|
||||
// The end-to-end shape of the leak. Account A's company list is in cache,
|
||||
// A signs out, B signs in, and B's companies request fails.
|
||||
// `companiesListQueryOptions` sets `retry: false`, so that single failure
|
||||
// is final — and under `invalidateQueries` the observer would still be
|
||||
// holding A's list, which consumers read as "this account owns these
|
||||
// companies".
|
||||
mockAuthApi.signOut.mockResolvedValue(undefined);
|
||||
queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" });
|
||||
|
||||
const companiesQueryFn = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce({
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
})
|
||||
.mockRejectedValue(new Error("refetch failed for the new account"));
|
||||
|
||||
// A live observer, standing in for CompanyProvider — which sits above the
|
||||
// router and stays mounted across the whole sign-out/sign-in cycle, so it
|
||||
// never gets a fresh mount to clear it.
|
||||
let observed: unknown = "unset";
|
||||
function CompaniesObserver() {
|
||||
const { data } = useQuery({
|
||||
queryKey: queryKeys.companies.all,
|
||||
queryFn: companiesQueryFn,
|
||||
retry: false,
|
||||
});
|
||||
observed = data;
|
||||
return null;
|
||||
}
|
||||
|
||||
const root = createRoot(container);
|
||||
await act(async () => {
|
||||
root.render(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<Harness />
|
||||
<CompaniesObserver />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
await vi.waitFor(() =>
|
||||
expect(observed).toEqual({
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
captured?.mutate();
|
||||
// The sign-out resolves, its cache reset fires, and the refetch that
|
||||
// reset kicks off is the one that fails.
|
||||
await vi.waitFor(() => expect(captured?.isSuccess).toBe(true));
|
||||
await vi.waitFor(() => expect(companiesQueryFn).toHaveBeenCalledTimes(2));
|
||||
});
|
||||
|
||||
expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined();
|
||||
expect(observed).toBeUndefined();
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("exposes a stable error without closing chrome or clearing caches", async () => {
|
||||
mockAuthApi.signOut.mockRejectedValue(new Error("Sign-out request failed"));
|
||||
queryClient.setQueryData(queryKeys.health, { status: "ok", deploymentMode: "authenticated" });
|
||||
queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } });
|
||||
queryClient.setQueryData(queryKeys.companies.all, {
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
const onSignedOut = vi.fn();
|
||||
const root = renderHarness(onSignedOut);
|
||||
|
||||
|
|
@ -115,9 +210,62 @@ describe("useSignOut", () => {
|
|||
await vi.waitFor(() => expect(captured?.error?.message).toBe("Sign-out request failed"));
|
||||
expect(captured?.isPending).toBe(false);
|
||||
expect(onSignedOut).not.toHaveBeenCalled();
|
||||
// The account is still signed in, so its caches must survive intact —
|
||||
// a failed sign-out must not blank the app the user is still using.
|
||||
expect(queryClient.getQueryData(queryKeys.auth.session)).toEqual({ session: { id: "session-1" } });
|
||||
expect(queryClient.getQueryData(queryKeys.companies.all)).toEqual({
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
expect(queryClient.getQueryState(queryKeys.auth.session)?.isInvalidated).toBe(false);
|
||||
expect(queryClient.getQueryState(queryKeys.health)?.isInvalidated).toBe(false);
|
||||
|
||||
flushSync(() => root.unmount());
|
||||
});
|
||||
|
||||
it("leaves the Cloud cache to the document reload rather than clearing it locally", async () => {
|
||||
// The Cloud path never reaches onSuccess's clearing branch: it hands off to
|
||||
// a top-level navigation, and the resulting document load builds a new
|
||||
// QueryClient from scratch. Clearing here would only blank the UI during
|
||||
// the frames before that navigation commits.
|
||||
queryClient.setQueryData(queryKeys.health, {
|
||||
status: "ok",
|
||||
cloud: {
|
||||
managed: true,
|
||||
managedBy: "paperclip-cloud",
|
||||
stackSlug: "acme",
|
||||
cloudBaseUrl: "https://cloud.example.test",
|
||||
},
|
||||
});
|
||||
queryClient.setQueryData(queryKeys.companies.all, {
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
const root = renderHarness();
|
||||
|
||||
flushSync(() => captured?.mutate());
|
||||
|
||||
await vi.waitFor(() => expect(mockNavigateTopLevel).toHaveBeenCalledOnce());
|
||||
expect(queryClient.getQueryData(queryKeys.companies.all)).toEqual({
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
|
||||
flushSync(() => root.unmount());
|
||||
});
|
||||
});
|
||||
|
||||
describe("isAccountScopedQueryKey", () => {
|
||||
it("keeps only the instance-scoped health entry", () => {
|
||||
expect(isAccountScopedQueryKey(queryKeys.health)).toBe(false);
|
||||
});
|
||||
|
||||
it("treats every other key root as account-scoped, including ones added later", () => {
|
||||
expect(isAccountScopedQueryKey(queryKeys.companies.all)).toBe(true);
|
||||
expect(isAccountScopedQueryKey(queryKeys.auth.session)).toBe(true);
|
||||
expect(isAccountScopedQueryKey(queryKeys.access.currentBoardAccess)).toBe(true);
|
||||
expect(isAccountScopedQueryKey(queryKeys.issues.list("company-a"))).toBe(true);
|
||||
expect(isAccountScopedQueryKey(queryKeys.secrets.list("company-a"))).toBe(true);
|
||||
expect(isAccountScopedQueryKey(["some-key-nobody-has-written-yet"])).toBe(true);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -6,6 +6,27 @@ import { useCloudInstance } from "./useCloudInstance";
|
|||
|
||||
const CLOUD_SIGN_OUT_PATH = "/cloud/logout";
|
||||
|
||||
/**
|
||||
* Query-key roots that describe the *instance* rather than the account that was
|
||||
* signed in, and so are allowed to outlive a sign-out.
|
||||
*
|
||||
* `health` is the only one. It carries deployment mode, bootstrap state and
|
||||
* Cloud metadata — nothing account-scoped — and `useCloudInstance` observes it
|
||||
* with `enabled: false`, deliberately leaving the fetch to CloudAccessGate.
|
||||
* Dropping the entry would strand every such observer on `null` until the gate
|
||||
* happened to refetch, flipping Cloud instances into their self-hosted
|
||||
* rendering mid-sign-out. It is refreshed in place instead.
|
||||
*
|
||||
* Everything *not* listed here is treated as account-scoped and cleared. That
|
||||
* direction is the point: a query key added later is account-scoped unless
|
||||
* someone deliberately says otherwise, so forgetting this file fails closed.
|
||||
*/
|
||||
const INSTANCE_SCOPED_QUERY_ROOTS: readonly unknown[] = [queryKeys.health[0]];
|
||||
|
||||
export function isAccountScopedQueryKey(queryKey: readonly unknown[]): boolean {
|
||||
return !INSTANCE_SCOPED_QUERY_ROOTS.includes(queryKey[0]);
|
||||
}
|
||||
|
||||
interface UseSignOutOptions {
|
||||
onSignedOut?: () => void;
|
||||
}
|
||||
|
|
@ -14,8 +35,10 @@ interface UseSignOutOptions {
|
|||
* Owns the app-wide sign-out decision.
|
||||
*
|
||||
* Cloud-managed tenants must enter the harness-owned logout sequence without
|
||||
* first clearing the tenant session. Authenticated self-hosted instances keep
|
||||
* the local API flow and invalidate the auth-dependent caches afterward.
|
||||
* first clearing the tenant session; that path is a top-level navigation, so
|
||||
* the document reload it triggers builds a new QueryClient and there is nothing
|
||||
* left here to clear. Authenticated self-hosted instances keep the local API
|
||||
* flow and drop the account-scoped caches afterward.
|
||||
*/
|
||||
export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) {
|
||||
const cloud = useCloudInstance();
|
||||
|
|
@ -36,10 +59,34 @@ export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) {
|
|||
if (target === "cloud") return;
|
||||
|
||||
onSignedOut?.();
|
||||
await Promise.all([
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.auth.session }),
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.health }),
|
||||
]);
|
||||
|
||||
// Drop every account-scoped cache entry, rather than invalidating a
|
||||
// couple of them. `invalidateQueries` only marks an entry stale and goes
|
||||
// on serving the old value until a refetch succeeds — and the companies
|
||||
// query sets `retry: false`, so a single failed request is enough to
|
||||
// leave the previous account's company list readable for the whole of
|
||||
// the *next* account's session. Consumers that read a non-empty list as
|
||||
// authoritative (company auto-selection, the invite-landing "already a
|
||||
// member" check, the board-access gate) would then act on it.
|
||||
//
|
||||
// `resetQueries` rather than `removeQueries`: removal empties the cache
|
||||
// but does not notify the observers already subscribed to those entries,
|
||||
// so a mounted `useQuery` keeps returning its last result until some
|
||||
// unrelated re-render happens to rebuild the query. That is not a corner
|
||||
// case here — CompanyProvider sits above the router and stays mounted
|
||||
// across the whole sign-out/sign-in cycle. Reset notifies them, so the
|
||||
// old data is gone from the cache *and* from everything reading it.
|
||||
//
|
||||
// Not awaited: the refetches this kicks off are expected to 401 now that
|
||||
// the session is gone, and the sign-out button should not sit pending
|
||||
// while they fail.
|
||||
void queryClient.resetQueries({
|
||||
predicate: (query) => isAccountScopedQueryKey(query.queryKey),
|
||||
});
|
||||
|
||||
// Instance-scoped, so refreshed in place rather than dropped — see
|
||||
// INSTANCE_SCOPED_QUERY_ROOTS.
|
||||
await queryClient.invalidateQueries({ queryKey: queryKeys.health });
|
||||
},
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -101,17 +101,27 @@ describe("InstanceGeneralSettings sign-out", () => {
|
|||
expect(mockAuthApi.signOut).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps authenticated self-hosted sign-out local and invalidates auth caches", async () => {
|
||||
it("keeps authenticated self-hosted sign-out local and drops the account caches", async () => {
|
||||
const invalidateQueries = vi.spyOn(queryClient, "invalidateQueries");
|
||||
await renderPage(SELF_HOSTED_HEALTH);
|
||||
queryClient.setQueryData(queryKeys.auth.session, { session: { id: "session-1" } });
|
||||
queryClient.setQueryData(queryKeys.companies.all, {
|
||||
companies: [{ id: "company-a", name: "Account A Co" }],
|
||||
unauthorized: false,
|
||||
});
|
||||
|
||||
flushSync(() => signOutButton()?.click());
|
||||
|
||||
await vi.waitFor(() => expect(mockAuthApi.signOut).toHaveBeenCalledOnce());
|
||||
await vi.waitFor(() => expect(invalidateQueries).toHaveBeenCalledWith({
|
||||
queryKey: queryKeys.auth.session,
|
||||
}));
|
||||
// Account-scoped entries are cleared outright, not marked stale — a stale
|
||||
// entry keeps serving the previous account's data until a refetch succeeds.
|
||||
await vi.waitFor(() =>
|
||||
expect(queryClient.getQueryData(queryKeys.auth.session)).toBeUndefined(),
|
||||
);
|
||||
expect(queryClient.getQueryData(queryKeys.companies.all)).toBeUndefined();
|
||||
// Health describes the instance, so it is refreshed rather than dropped.
|
||||
expect(invalidateQueries).toHaveBeenCalledWith({ queryKey: queryKeys.health });
|
||||
expect(queryClient.getQueryData(queryKeys.health)).toEqual(SELF_HOSTED_HEALTH);
|
||||
expect(mockNavigateTopLevel).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue