diff --git a/apps/web/src/app.css b/apps/web/src/app.css index 83428f4f3..b75b4c4e4 100644 --- a/apps/web/src/app.css +++ b/apps/web/src/app.css @@ -1446,6 +1446,13 @@ select:disabled, color: var(--shell-panel-faint); } +.settings-nav-access-error { + margin: 0 0 0.3rem; + padding: 0 0.42rem; + font-size: 0.6875rem; + color: var(--shell-panel-faint); +} + /* Mock .ch rows in the sidebar: pad 0.32/0.42, square. */ .shell-ch-row { display: grid; diff --git a/apps/web/src/pages/settings-nav.tsx b/apps/web/src/pages/settings-nav.tsx index d689823c4..447de908e 100644 --- a/apps/web/src/pages/settings-nav.tsx +++ b/apps/web/src/pages/settings-nav.tsx @@ -40,6 +40,11 @@ export function SettingsNav({ return (

{group.label}

+ {group.accessProbeFailed === true && ( +

+ {SETTINGS_STRINGS.accessProbeFailedHint} +

+ )} {primary.map((section) => { const Icon = section.icon; return ( diff --git a/apps/web/src/pages/settings-page.tsx b/apps/web/src/pages/settings-page.tsx index ee1117a92..8049ca8f4 100644 --- a/apps/web/src/pages/settings-page.tsx +++ b/apps/web/src/pages/settings-page.tsx @@ -52,7 +52,9 @@ export function SettingsRoute({ // `sections` while its probe is still resolving, same as when it's // genuinely denied — wait for every gate to settle before treating a // miss as final, or a deep link to an about-to-be-allowed section would - // bounce away before its probe finishes. + // bounce away before its probe finishes. `error` is settled: the + // registry withholds the section and the nav shows a couldn't-check + // state rather than pretending the principal is unauthorized. const accessSettled = access.people !== "loading" && access.roles !== "loading" && diff --git a/apps/web/src/settings-access.ts b/apps/web/src/settings-access.ts index e52e3a0a8..aac860de0 100644 --- a/apps/web/src/settings-access.ts +++ b/apps/web/src/settings-access.ts @@ -4,28 +4,18 @@ // QueryClient — instead of each mount calling `@corbits/settings-ui`'s bare // `useTenancyAccess`, which fetches on every mount with no cache — means // two mounted consumers share one in-flight request and one cached result. -// The package stays free of TanStack Query: this only injects its own -// `evaluate` primitive into the app's cache, the package never imports it. +// The package stays free of TanStack Query: this only injects the +// package's probe into the app's cache, the package never imports Query. -import { evaluate } from "@corbits/settings-ui"; -import type { SectionAccess, TenancyAccess } from "@corbits/settings-ui"; -import { useQuery } from "@tanstack/react-query"; +import { + coalesceSectionAccess, + probeSectionAccess, +} from "@corbits/settings-ui"; +import type { TenancyAccess } from "@corbits/settings-ui"; +import { useQuery, useQueryClient } from "@tanstack/react-query"; import { tenantKeys } from "./query-client"; -async function probe( - tenantId: string, - principalId: string, - resource: string, -): Promise { - try { - const result = await evaluate(tenantId, principalId, resource, "read"); - return result.effect === "allow" ? "allowed" : "denied"; - } catch { - return "denied"; - } -} - const LOADING_ACCESS: TenancyAccess = { people: "loading", roles: "loading", @@ -33,28 +23,51 @@ const LOADING_ACCESS: TenancyAccess = { credentials: "loading", }; +function coalesceTenancyAccess( + previous: TenancyAccess | undefined, + next: TenancyAccess, +): TenancyAccess { + const prior = previous ?? LOADING_ACCESS; + return { + people: coalesceSectionAccess(prior.people, next.people), + roles: coalesceSectionAccess(prior.roles, next.roles), + grants: coalesceSectionAccess(prior.grants, next.grants), + credentials: coalesceSectionAccess(prior.credentials, next.credentials), + }; +} + /** One shared probe per (tenant, principal), not one per mounted consumer. * `null` ids report `loading` — the same "not shown yet, never disabled" - * contract `@corbits/settings-ui`'s own hook holds. */ + * contract `@corbits/settings-ui`'s own hook holds. A thrown evaluate + * (network/5xx) is `error`, not `denied`; a refetch failure keeps the + * last allow/deny so gated nav does not vanish as if unauthorized. */ export function useSettingsAccess( tenantId: string | null, principalId: string | null, ): TenancyAccess { + const queryClient = useQueryClient(); const enabled = tenantId !== null && principalId !== null; + const queryKey = + tenantId !== null && principalId !== null + ? tenantKeys.settingsAccess(tenantId, principalId) + : tenantKeys.settingsAccess("none", "none"); const query = useQuery({ - queryKey: - tenantId !== null && principalId !== null - ? tenantKeys.settingsAccess(tenantId, principalId) - : tenantKeys.settingsAccess("none", "none"), + queryKey, queryFn: async (): Promise => { if (tenantId === null || principalId === null) return LOADING_ACCESS; + const previous = queryClient.getQueryData(queryKey); const [people, roles, grants, credentials] = await Promise.all([ - probe(tenantId, principalId, "principal"), - probe(tenantId, principalId, "role"), - probe(tenantId, principalId, "grant"), - probe(tenantId, principalId, "credential"), + probeSectionAccess(tenantId, principalId, "principal"), + probeSectionAccess(tenantId, principalId, "role"), + probeSectionAccess(tenantId, principalId, "grant"), + probeSectionAccess(tenantId, principalId, "credential"), ]); - return { people, roles, grants, credentials }; + return coalesceTenancyAccess(previous, { + people, + roles, + grants, + credentials, + }); }, enabled, }); diff --git a/apps/web/test/settings-access.test.tsx b/apps/web/test/settings-access.test.tsx new file mode 100644 index 000000000..6103611ae --- /dev/null +++ b/apps/web/test/settings-access.test.tsx @@ -0,0 +1,108 @@ +// The app's cached settings-access probe must agree with +// `@corbits/settings-ui`'s mapping: evaluate effect !== allow is deny; +// a thrown probe is error, not deny (CL-6829). + +import { afterEach, describe, expect, test } from "bun:test"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import type { TenancyAccess } from "@corbits/settings-ui"; + +import { useSettingsAccess } from "../src/settings-access"; +import { + createTestQueryClient, + TestQueryProvider, +} from "./test-query-provider"; + +const realFetch = globalThis.fetch; + +let container: HTMLDivElement | null = null; +let root: Root | null = null; + +afterEach(() => { + globalThis.fetch = realFetch; + if (root !== null) { + act(() => { + root?.unmount(); + }); + root = null; + } + container?.remove(); + container = null; +}); + +const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); + +function AccessProbe({ + onAccess, +}: { + readonly onAccess: (access: TenancyAccess) => void; +}) { + const access = useSettingsAccess("tnt_1", "prn_1"); + onAccess(access); + return null; +} + +async function mountProbe(): Promise { + const seen: TenancyAccess[] = []; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + const client = createTestQueryClient(); + await act(async () => { + root?.render( + + seen.push(access)} /> + , + ); + }); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 20)); + }); + return seen; +} + +describe("useSettingsAccess", () => { + test("a deny effect is denied", async () => { + globalThis.fetch = (async () => + json({ effect: "deny", matchingGrants: [] })) as unknown as typeof fetch; + const seen = await mountProbe(); + const last = seen[seen.length - 1]; + expect(last).toEqual({ + people: "denied", + roles: "denied", + grants: "denied", + credentials: "denied", + }); + }); + + test("a 5xx is error, not denied", async () => { + globalThis.fetch = (async () => json({}, 500)) as unknown as typeof fetch; + const seen = await mountProbe(); + const last = seen[seen.length - 1]; + expect(last).toEqual({ + people: "error", + roles: "error", + grants: "error", + credentials: "error", + }); + }); + + test("a network failure is error, not denied", async () => { + globalThis.fetch = (() => + Promise.reject( + new TypeError("Failed to fetch"), + )) as unknown as typeof fetch; + const seen = await mountProbe(); + const last = seen[seen.length - 1]; + expect(last).toEqual({ + people: "error", + roles: "error", + grants: "error", + credentials: "error", + }); + }); +}); diff --git a/apps/web/test/settings-page.test.tsx b/apps/web/test/settings-page.test.tsx index a7ae94342..88dc1ba79 100644 --- a/apps/web/test/settings-page.test.tsx +++ b/apps/web/test/settings-page.test.tsx @@ -7,6 +7,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import { act } from "react"; import { createRoot, type Root } from "react-dom/client"; import { QueryClient } from "@tanstack/react-query"; +import { SETTINGS_STRINGS, type TenancyAccess } from "@corbits/settings-ui"; import type { PrincipalsPage } from "../src/api"; import { BenchProvider } from "../src/bench-context"; @@ -29,16 +30,23 @@ const principalsPage: PrincipalsPage = { nextCursor: null, }; -const deniedAccess = { +const deniedAccess: TenancyAccess = { people: "denied", roles: "denied", grants: "denied", credentials: "denied", -} as const; +}; + +const errorAccess: TenancyAccess = { + people: "error", + roles: "error", + grants: "error", + credentials: "error", +}; /** A client pre-seeded so both the bench and the settings-access probe are * already resolved on first render — deterministic, no real network. */ -function seededClient(): QueryClient { +function seededClient(access: TenancyAccess = deniedAccess): QueryClient { const client = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: 0, staleTime: Infinity }, @@ -47,7 +55,7 @@ function seededClient(): QueryClient { client.setQueryData(meKeys.principals, principalsPage); client.setQueryData( tenantKeys.settingsAccess("tenant_1", "principal_1"), - deniedAccess, + access, ); return client; } @@ -67,13 +75,17 @@ describe("SettingsRoute section-id redirect", () => { container = null; }); - async function mount(path: string, navigated: string[]) { + async function mount( + path: string, + navigated: string[], + access: TenancyAccess = deniedAccess, + ) { container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); await act(async () => { root?.render( - + navigated.push(to)} /> @@ -99,4 +111,23 @@ describe("SettingsRoute section-id redirect", () => { await mount("/settings/account", navigated); expect(navigated).toEqual([]); }); + + test("a probe failure is not presented as a gate deny", async () => { + const navigated: string[] = []; + await mount("/settings/account", navigated, errorAccess); + expect(container?.textContent).toContain( + SETTINGS_STRINGS.accessProbeFailedHint, + ); + expect(container?.textContent).not.toContain( + SETTINGS_STRINGS.peopleSectionTitle, + ); + }); + + test("a gate deny does not claim the access check failed", async () => { + const navigated: string[] = []; + await mount("/settings/account", navigated); + expect(container?.textContent).not.toContain( + SETTINGS_STRINGS.accessProbeFailedHint, + ); + }); }); diff --git a/packages/settings-ui/src/access.ts b/packages/settings-ui/src/access.ts index e956ef922..c397f38da 100644 --- a/packages/settings-ui/src/access.ts +++ b/packages/settings-ui/src/access.ts @@ -4,12 +4,16 @@ // There's no capability listing to read this off of, so this probes the // one grant-checked route that requires no grant of its own — // `evaluate` — for the resource each section is built on. +// A 200 whose effect is not `allow` is an authenticated deny. A thrown +// probe (network, 5xx) is `error`, never `denied` — collapsing those +// together made the gated nav vanish as if the principal were +// unauthorized (CL-6829). import { useEffect, useState } from "react"; import { evaluate } from "./tenancy-api"; -export type SectionAccess = "loading" | "allowed" | "denied"; +export type SectionAccess = "loading" | "allowed" | "denied" | "error"; export type TenancyAccess = { readonly people: SectionAccess; @@ -18,6 +22,35 @@ export type TenancyAccess = { readonly credentials: SectionAccess; }; +/** Maps one evaluate call to a nav gate. Catch is `error`, not `denied`. */ +export async function probeSectionAccess( + tenantId: string, + principalId: string, + resource: string, +): Promise { + try { + const result = await evaluate(tenantId, principalId, resource, "read"); + return result.effect === "allow" ? "allowed" : "denied"; + } catch { + // report-error-ignore: CL-6829 — a failed evaluate probe is the + // `error` nav state, not an unexpected exception to report. + return "error"; + } +} + +/** A failed probe must not clobber a prior allow/deny — only an unresolved + * gate becomes `error`. That keeps last-known nav and avoids flashing + * gated sections that a later successful deny would hide. */ +export function coalesceSectionAccess( + previous: SectionAccess, + next: SectionAccess, +): SectionAccess { + if (next === "error" && (previous === "allowed" || previous === "denied")) { + return previous; + } + return next; +} + function useResourceAccess( tenantId: string | null, principalId: string | null, @@ -32,14 +65,9 @@ function useResourceAccess( } let cancelled = false; setAccess("loading"); - evaluate(tenantId, principalId, resource, "read") - .then((result) => { - if (!cancelled) - setAccess(result.effect === "allow" ? "allowed" : "denied"); - }) - .catch(() => { - if (!cancelled) setAccess("denied"); - }); + void probeSectionAccess(tenantId, principalId, resource).then((next) => { + if (!cancelled) setAccess(next); + }); return () => { cancelled = true; }; @@ -49,8 +77,10 @@ function useResourceAccess( } /** One probe per section, run in parallel — a section stays out of the nav - * until its probe resolves `allowed`, so a slow or failed probe reads as - * "not shown yet", never as a visible-but-disabled tab. */ + * until its probe resolves `allowed`, so a slow probe reads as "not shown + * yet", never as a visible-but-disabled tab. A failed probe is `error`, + * not `denied`: the registry withholds the section but marks the group so + * a host can show a couldn't-check state instead of looking unauthorized. */ export function useTenancyAccess( tenantId: string | null, principalId: string | null, diff --git a/packages/settings-ui/src/index.ts b/packages/settings-ui/src/index.ts index a4260aaf2..5c8ee2aa5 100644 --- a/packages/settings-ui/src/index.ts +++ b/packages/settings-ui/src/index.ts @@ -70,7 +70,11 @@ export type { PrincipalLabel } from "./identity"; export { GRANT_RESOURCES, GRANT_ACTIONS } from "./resource-vocabulary"; export type { GrantResource, GrantAction } from "./resource-vocabulary"; -export { useTenancyAccess } from "./access"; +export { + useTenancyAccess, + probeSectionAccess, + coalesceSectionAccess, +} from "./access"; export type { SectionAccess, TenancyAccess } from "./access"; export { diff --git a/packages/settings-ui/src/section-registry.tsx b/packages/settings-ui/src/section-registry.tsx index f682394e0..ac34338fa 100644 --- a/packages/settings-ui/src/section-registry.tsx +++ b/packages/settings-ui/src/section-registry.tsx @@ -121,22 +121,33 @@ const SETTINGS_SECTION_GROUPS: readonly SettingsSectionGroupDef[] = [ /** * The Personal Settings / Shared Settings groups, with a section dropped entirely — never * rendered disabled — until its `access[gate]` probe resolves `allowed`. - * Both the settings stage and a host's own section nav (e.g. col2) should - * read from this single registry so they can never drift. + * Loading and authenticated deny both withhold the section. A probe `error` + * withholds too (so gated sections never flash then hide) but marks the + * group `accessProbeFailed` so a host can show a couldn't-check state + * instead of looking like unauthorized. Both the settings stage and a + * host's own section nav (e.g. col2) should read from this single registry + * so they can never drift. */ export function resolveSettingsSectionGroups( access: TenancyAccess, ): readonly SettingsSectionGroup[] { - return SETTINGS_SECTION_GROUPS.map((group) => ({ - id: group.id, - label: group.label, - sections: group.sections - .filter( - (section) => - section.gate === undefined || access[section.gate] === "allowed", - ) - .map(({ gate: _gate, ...section }) => section), - })); + return SETTINGS_SECTION_GROUPS.map((group) => { + const accessProbeFailed = group.sections.some( + (section) => + section.gate !== undefined && access[section.gate] === "error", + ); + return { + id: group.id, + label: group.label, + sections: group.sections + .filter( + (section) => + section.gate === undefined || access[section.gate] === "allowed", + ) + .map(({ gate: _gate, ...section }) => section), + ...(accessProbeFailed ? { accessProbeFailed: true as const } : {}), + }; + }); } /** diff --git a/packages/settings-ui/src/shell.tsx b/packages/settings-ui/src/shell.tsx index 24e376b91..647f1236d 100644 --- a/packages/settings-ui/src/shell.tsx +++ b/packages/settings-ui/src/shell.tsx @@ -50,6 +50,10 @@ export type SettingsSectionGroup = { readonly id: string; readonly label: string; readonly sections: readonly SettingsSection[]; + /** Set when a tenancy probe for a gated section in this group failed + * (network/5xx), so a host can show a couldn't-check state instead of + * treating the absent sections as an authenticated deny. */ + readonly accessProbeFailed?: true; }; export function flattenSettingsSections( diff --git a/packages/settings-ui/src/strings.ts b/packages/settings-ui/src/strings.ts index bdc1ac69e..a1ecf43a0 100644 --- a/packages/settings-ui/src/strings.ts +++ b/packages/settings-ui/src/strings.ts @@ -11,6 +11,8 @@ export const SETTINGS_STRINGS = { groupEveryoneLabel: "Shared Settings", advancedSectionsSummary: "Advanced", advancedSectionsHint: "Roles, granular permissions, and activity history.", + accessProbeFailedHint: + "Couldn't check access for some sections. They'll show up here once that succeeds.", notificationsSectionTitle: "Notifications", notificationsSectionDescription: "How activity reaches you.", diff --git a/packages/settings-ui/test/access.test.ts b/packages/settings-ui/test/access.test.ts new file mode 100644 index 000000000..bfe80e67e --- /dev/null +++ b/packages/settings-ui/test/access.test.ts @@ -0,0 +1,88 @@ +// ACL evaluate probes decide whether People/Roles/Grants/Credentials belong +// in the settings nav. A 200 with effect !== allow is an authenticated deny; +// a thrown probe (network, 5xx) is not — it must not collapse to "denied" +// or those sections vanish as if the principal were unauthorized (CL-6829). + +import { afterEach, describe, expect, test } from "bun:test"; + +import { coalesceSectionAccess, probeSectionAccess } from "../src/access"; + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); + +const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); + +function stubFetch( + respond: (path: string) => Response | Promise, +): void { + globalThis.fetch = ((input: RequestInfo | URL) => { + const path = + typeof input === "string" ? input : new URL(String(input)).pathname; + return Promise.resolve(respond(path)); + }) as typeof fetch; +} + +describe("probeSectionAccess", () => { + test("an allow effect is allowed", async () => { + stubFetch(() => json({ effect: "allow", matchingGrants: [] })); + await expect( + probeSectionAccess("tnt_1", "prn_1", "principal"), + ).resolves.toBe("allowed"); + }); + + test("a deny effect is denied", async () => { + stubFetch(() => json({ effect: "deny", matchingGrants: [] })); + await expect( + probeSectionAccess("tnt_1", "prn_1", "principal"), + ).resolves.toBe("denied"); + }); + + test("an ask effect is denied — only allow is allowed", async () => { + stubFetch(() => json({ effect: "ask", matchingGrants: [] })); + await expect(probeSectionAccess("tnt_1", "prn_1", "role")).resolves.toBe( + "denied", + ); + }); + + test("a 5xx is error, not denied", async () => { + stubFetch(() => json({}, 500)); + await expect(probeSectionAccess("tnt_1", "prn_1", "grant")).resolves.toBe( + "error", + ); + }); + + test("a network failure is error, not denied", async () => { + globalThis.fetch = (() => + Promise.reject( + new TypeError("Failed to fetch"), + )) as unknown as typeof fetch; + await expect( + probeSectionAccess("tnt_1", "prn_1", "credential"), + ).resolves.toBe("error"); + }); +}); + +describe("coalesceSectionAccess", () => { + test("a failed probe keeps a prior allow so gated nav does not vanish", () => { + expect(coalesceSectionAccess("allowed", "error")).toBe("allowed"); + }); + + test("a failed probe keeps a prior deny so gated nav does not flash", () => { + expect(coalesceSectionAccess("denied", "error")).toBe("denied"); + }); + + test("a failed first probe is error — there is no last-known to keep", () => { + expect(coalesceSectionAccess("loading", "error")).toBe("error"); + }); + + test("a successful deny replaces a prior allow", () => { + expect(coalesceSectionAccess("allowed", "denied")).toBe("denied"); + }); +}); diff --git a/packages/settings-ui/test/section-registry.test.tsx b/packages/settings-ui/test/section-registry.test.tsx index 70f63a12a..d81797bd7 100644 --- a/packages/settings-ui/test/section-registry.test.tsx +++ b/packages/settings-ui/test/section-registry.test.tsx @@ -64,10 +64,59 @@ describe("resolveSettingsSectionGroups", () => { grants: "allowed", credentials: "denied", }; - expect(ids(resolveSettingsSectionGroups(loading))).toEqual([ + const groups = resolveSettingsSectionGroups(loading); + expect(ids(groups)).toEqual([ { id: "account", sections: ["account"] }, { id: "everyone", sections: ["grants", "audit"] }, ]); + expect( + groups.find((group) => group.id === "everyone")?.accessProbeFailed, + ).toBe(undefined); + }); + + test("a probe error withholds gated sections but is not an authenticated deny", () => { + const errored: TenancyAccess = { + people: "error", + roles: "error", + grants: "error", + credentials: "error", + }; + const groups = resolveSettingsSectionGroups(errored); + expect(ids(groups)).toEqual([ + { id: "account", sections: ["account"] }, + { id: "everyone", sections: ["audit"] }, + ]); + expect( + groups.find((group) => group.id === "account")?.accessProbeFailed, + ).toBe(undefined); + expect( + groups.find((group) => group.id === "everyone")?.accessProbeFailed, + ).toBe(true); + expect( + resolveSettingsSectionGroups(denied).find( + (group) => group.id === "everyone", + )?.accessProbeFailed, + ).toBe(undefined); + }); + + test("a mixed error withholds only the failed gate and still flags the group", () => { + const mixed: TenancyAccess = { + people: "error", + roles: "allowed", + grants: "allowed", + credentials: "allowed", + }; + const groups = resolveSettingsSectionGroups(mixed); + expect(ids(groups)).toEqual([ + { id: "account", sections: ["account"] }, + { + id: "everyone", + sections: ["connections", "roles", "grants", "audit"], + }, + ]); + expect( + groups.find((group) => group.id === "everyone")?.accessProbeFailed, + ).toBe(true); }); test("Roles, Grants, and Audit are tucked under Advanced; Connections and People are not", () => { diff --git a/scripts/checks/report-error-baseline.txt b/scripts/checks/report-error-baseline.txt index dcefb0449..fe163890f 100644 --- a/scripts/checks/report-error-baseline.txt +++ b/scripts/checks/report-error-baseline.txt @@ -103,7 +103,6 @@ apps/web/src/session.ts 1 return { apps/web/src/session.ts 2 return { apps/web/src/session.ts 3 return { apps/web/src/session.ts 4 return { -apps/web/src/settings-access.ts 1 return "denied"; apps/web/src/shell/context-menu/items.tsx 1 toast("Couldn't copy the link"); apps/web/src/shell/routine-panel.tsx 1 setSaveState("error"); apps/web/src/shell/workbench-list.tsx 1 // Revert the optimistic title on failure; the list will refetch on