From 45ee70acb720e50a10bc2dbe44f18bad57e37f55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 13 Sep 2026 14:49:56 +0200 Subject: [PATCH] fix(settings): list record content only where the grant manages it The anamnesis page writes through requireRecordAuth("manage", "profile"), which refuses a grant whose sections do not reach profile. The Settings list and the navigation entry checked only the level and the record kind, so a MANAGE grant scoped away from profile would be offered a page whose saves are refused. Record-content destinations now name the section they write, and are listed only when the active record's manageable sections include it. A destination without a named section is never listed. The shell and the navigation read the same predicate. MANAGE grants are whole-record today, so managed profiles and adult MANAGE shares list what they did before. Refs #939 --- .../managed-record-navigation.test.tsx | 7 + .../__tests__/nav-model-shared-record.test.ts | 122 ++++++++++++++++-- .../layout/__tests__/sidebar-nav.test.tsx | 4 +- src/components/layout/sidebar-nav.tsx | 10 +- src/components/layout/top-bar.tsx | 9 +- .../__tests__/settings-shell.test.tsx | 45 ++++++- src/lib/record-settings/classification.ts | 54 +++++++- 7 files changed, 224 insertions(+), 27 deletions(-) diff --git a/src/__tests__/managed-record-navigation.test.tsx b/src/__tests__/managed-record-navigation.test.tsx index 5f8fd08d7..8653150cf 100644 --- a/src/__tests__/managed-record-navigation.test.tsx +++ b/src/__tests__/managed-record-navigation.test.tsx @@ -3,6 +3,7 @@ import { renderToStaticMarkup } from "react-dom/server"; import { I18nProvider } from "@/lib/i18n/context"; import { classifySettingsDestination } from "@/lib/record-settings"; +import { delegatedDomains } from "@/lib/sharing/domain-write-support"; const pathnameRef = { value: "/settings/security" }; const activeRecordRef = { value: true }; @@ -31,6 +32,12 @@ vi.mock("@/hooks/use-auth", () => ({ recordKind: recordRef.value.kind, sections: null, canWrite: true, + // Bound as the server publishes it for a whole-record grant. + manageableDomains: delegatedDomains( + recordRef.value.level as "read" | "write" | "manage", + null, + "manage", + ), }, accounts: [], canSwitch: true, diff --git a/src/components/layout/__tests__/nav-model-shared-record.test.ts b/src/components/layout/__tests__/nav-model-shared-record.test.ts index 5b2a43239..f9c6005f9 100644 --- a/src/components/layout/__tests__/nav-model-shared-record.test.ts +++ b/src/components/layout/__tests__/nav-model-shared-record.test.ts @@ -22,15 +22,26 @@ * MANAGE case and the parity case go red (2); * - keep Notifications in the switched tail beside the remapped Settings * entry → "never offers Notifications under a switch" goes red, with the - * two "offers Settings" cases and the parity case (4). + * two "offers Settings" cases and the parity case (4); + * - drop the manageable-section check from + * `isSettingsDestinationListedForRecord` → "offers no Settings entry to a + * MANAGE share whose sections do not include profile" and "keeps a + * managed profile's guardian configuration …" go red (2). */ import { describe, expect, it } from "vitest"; import { SETTINGS_SECTIONS } from "@/components/settings/settings-shell"; import { + RECORD_CONTENT_WRITE_DOMAINS, + SETTINGS_DESTINATION_INVENTORY, isSettingsDestinationListedForRecord, type SettingsRecordContext, } from "@/lib/record-settings/classification"; +import { + DOMAIN_WRITE_SUPPORT, + delegatedDomains, +} from "@/lib/sharing/domain-write-support"; +import type { AccountAccessLevel } from "@/lib/sharing/account-access-view"; import { BOTTOM_NAV_PRIMARY_SLOT_HREFS, @@ -131,14 +142,29 @@ describe("a module the RECORD does not track", () => { }); describe("the utility tail under a switch", () => { - const MANAGED_AT_MANAGE: SettingsRecordContext = { - recordKind: "managed", - level: "manage", - }; - const SHARED_AT_MANAGE: SettingsRecordContext = { - recordKind: "shared", - level: "manage", - }; + /** + * The context as the server publishes it for a whole-record grant: the + * manageable list is the real `delegatedDomains` answer, not a literal, so + * a change to the write-support table reaches these cases. + */ + const context = ( + recordKind: "managed" | "shared", + level: AccountAccessLevel | null, + sections: SettingsRecordContext["manageableDomains"] | null = null, + ): SettingsRecordContext => ({ + recordKind, + level, + manageableDomains: + level === null + ? [] + : delegatedDomains( + level, + sections === null ? null : [...sections], + "manage", + ), + }); + const MANAGED_AT_MANAGE = context("managed", "manage"); + const SHARED_AT_MANAGE = context("shared", "manage"); const hrefs = (record: SettingsRecordContext | null) => visibleUtilityDestinations({ record }).map((d) => d.href); @@ -175,7 +201,7 @@ describe("the utility tail under a switch", () => { it("offers nothing to a READ or WRITE share, where the shell lists nothing", () => { for (const recordKind of ["shared", "managed"] as const) { for (const level of ["read", "write"] as const) { - expect(hrefs({ recordKind, level })).toEqual([]); + expect(hrefs(context(recordKind, level))).toEqual([]); } } }); @@ -184,7 +210,7 @@ describe("the utility tail under a switch", () => { // `resolveRecordCapabilities` answers `recordKind: "shared", level: null` // for both; a Settings entry there would lead into a context nobody has // proven. - expect(hrefs({ recordKind: "shared", level: null })).toEqual([]); + expect(hrefs(context("shared", null))).toEqual([]); }); it("never offers Notifications under a switch", () => { @@ -192,11 +218,81 @@ describe("the utility tail under a switch", () => { // under a switch at every level and for every record kind. for (const recordKind of ["shared", "managed"] as const) { for (const level of ["read", "write", "manage", null] as const) { - expect(hrefs({ recordKind, level })).not.toContain("/notifications"); + expect(hrefs(context(recordKind, level))).not.toContain( + "/notifications", + ); } } }); + /** + * The anamnesis forms post to `requireRecordAuth("manage", "profile")`, + * which refuses a grant whose sections do not reach `profile`. A MANAGE + * grant is whole-record today, so these contexts are synthetic; they pin + * that the list stays true if a scoped one ever reaches the client. + */ + it("offers no Settings entry to a MANAGE share whose sections do not include profile", () => { + const scoped = context("shared", "manage", ["labs"]); + // Non-zero: the scoped grant still manages something, so the empty tail + // is the domain check and not a grant that manages nothing. + expect(scoped.manageableDomains).toEqual(["labs"]); + expect(hrefs(scoped)).toEqual([]); + // And the shell lists nothing for it either. + expect( + SETTINGS_SECTIONS.filter((section) => + isSettingsDestinationListedForRecord(section.slug, scoped), + ), + ).toEqual([]); + }); + + it("lands a MANAGE share that manages profile on anamnesis, as the shell lists it", () => { + const scoped = context("shared", "manage", ["profile"]); + expect(hrefs(scoped)).toEqual(["/settings/anamnesis"]); + expect( + SETTINGS_SECTIONS.filter((section) => + isSettingsDestinationListedForRecord(section.slug, scoped), + ).map((section) => section.slug), + ).toEqual(["anamnesis"]); + }); + + it("keeps a managed profile's guardian configuration independent of the manageable sections", () => { + // Guardian destinations resolve `requireGuardianAuth`, not a section, so + // only the record-content page depends on `profile`. + const scoped = context("managed", "manage", ["labs"]); + const listed = SETTINGS_SECTIONS.filter((section) => + isSettingsDestinationListedForRecord(section.slug, scoped), + ).map((section) => section.slug); + expect(listed).toContain("modules"); + expect(listed).not.toContain("anamnesis"); + expect(hrefs(scoped)).toEqual(["/settings/account"]); + // The whole-record guardian grant is unchanged: anamnesis stays listed. + expect( + SETTINGS_SECTIONS.filter((section) => + isSettingsDestinationListedForRecord(section.slug, MANAGED_AT_MANAGE), + ).map((section) => section.slug), + ).toContain("anamnesis"); + }); + + it("names the managed section of every record-content destination", () => { + const recordContent = Object.entries(SETTINGS_DESTINATION_INVENTORY) + .filter(([, classification]) => classification.kind === "manage-writable") + .map(([slug]) => slug); + expect(recordContent.length).toBeGreaterThan(0); + const domains = RECORD_CONTENT_WRITE_DOMAINS as Partial< + Record + >; + for (const slug of recordContent) { + const domain = domains[slug]; + expect(domain, slug).toBeDefined(); + // A section with no MANAGE route could never satisfy the check. + expect( + DOMAIN_WRITE_SUPPORT[domain as keyof typeof DOMAIN_WRITE_SUPPORT] + .manage, + slug, + ).toBe(true); + } + }); + it("lands on the first section the Settings shell lists, for every record kind", () => { // The parity net. The navigation walks the slug registry, the shell walks // its own ordered section list; both filter with the same predicate, and @@ -205,6 +301,8 @@ describe("the utility tail under a switch", () => { const contexts: SettingsRecordContext[] = [ MANAGED_AT_MANAGE, SHARED_AT_MANAGE, + context("shared", "manage", ["profile"]), + context("managed", "manage", ["labs"]), ]; for (const record of contexts) { const firstListed = SETTINGS_SECTIONS.find((section) => diff --git a/src/components/layout/__tests__/sidebar-nav.test.tsx b/src/components/layout/__tests__/sidebar-nav.test.tsx index ea1c5d49c..176fdb67a 100644 --- a/src/components/layout/__tests__/sidebar-nav.test.tsx +++ b/src/components/layout/__tests__/sidebar-nav.test.tsx @@ -50,6 +50,7 @@ import { I18nProvider } from "@/lib/i18n/context"; import { SidebarNav } from "../sidebar-nav"; import { ADMIN_SECTIONS } from "@/components/admin/admin-shell"; import { visibleUtilityDestinations } from "../nav-model"; +import { delegatedDomains } from "@/lib/sharing/domain-write-support"; function render({ pathname = "/", @@ -293,8 +294,7 @@ describe(" inside somebody else's record (#939)", () => { canWrite: active.level !== "read", sections: null, writableDomains: [], - manageableDomains: - active.level === "manage" ? ["measurements"] : [], + manageableDomains: delegatedDomains(active.level, null, "manage"), }, } : undefined; diff --git a/src/components/layout/sidebar-nav.tsx b/src/components/layout/sidebar-nav.tsx index 90edb5afd..4d8dfc467 100644 --- a/src/components/layout/sidebar-nav.tsx +++ b/src/components/layout/sidebar-nav.tsx @@ -316,6 +316,10 @@ export function SidebarNav() { recordKind, level, } = useRecordCapabilities(); + // The sections the grant may manage, bound from the server-resolved entry. + // The Settings entry needs them: a record-content page is offered only when + // the grant manages the section its forms write. + const manageableDomains = user?.accountAccess?.active?.manageableDomains; const visibleNavItems = useMemo( () => visibleNavDestinations(user?.modules, mounted, sharedRecord, sections), @@ -367,9 +371,11 @@ export function SidebarNav() { const footerUtilityItems = useMemo( () => visibleUtilityDestinations({ - record: sharedRecord ? { recordKind, level } : null, + record: sharedRecord + ? { recordKind, level, manageableDomains: manageableDomains ?? [] } + : null, }).filter((d) => d.href !== "/notifications"), - [sharedRecord, recordKind, level], + [sharedRecord, recordKind, level, manageableDomains], ); function isUtilityActive(href: string) { diff --git a/src/components/layout/top-bar.tsx b/src/components/layout/top-bar.tsx index fccdb8be1..f2aa2aaf8 100644 --- a/src/components/layout/top-bar.tsx +++ b/src/components/layout/top-bar.tsx @@ -54,7 +54,14 @@ export function TopBar() { level, } = useRecordCapabilities(); const utilityItems = visibleUtilityDestinations({ - record: sharedRecord ? { recordKind, level } : null, + record: sharedRecord + ? { + recordKind, + level, + manageableDomains: + user?.accountAccess?.active?.manageableDomains ?? [], + } + : null, }); const logout = useLogout(); const { theme, setTheme } = useTheme(); diff --git a/src/components/settings/__tests__/settings-shell.test.tsx b/src/components/settings/__tests__/settings-shell.test.tsx index 01ba0b83d..fc16d5808 100644 --- a/src/components/settings/__tests__/settings-shell.test.tsx +++ b/src/components/settings/__tests__/settings-shell.test.tsx @@ -25,7 +25,11 @@ const mockModulesRef: { value: Record | undefined } = { // server has already resolved which record is active, so it applies on the SSR // pass and is assertable from a static render. const mockActiveRecordRef: { - value: { recordKind: "managed" | "shared"; level: string } | null; + value: { + recordKind: "managed" | "shared"; + level: string; + manageableDomains: string[]; + } | null; } = { value: null }; vi.mock("@/hooks/use-auth", () => ({ useAuth: () => ({ @@ -48,6 +52,7 @@ vi.mock("@/hooks/use-auth", () => ({ })); import { I18nProvider } from "@/lib/i18n/context"; +import { delegatedDomains } from "@/lib/sharing/domain-write-support"; import { SETTINGS_SECTION_SLUGS, SETTINGS_SECTIONS, @@ -60,7 +65,11 @@ function renderShell(props: { pathname?: string; locale?: "en" | "de"; modules?: Record; - activeRecord?: { recordKind: "managed" | "shared"; level: string } | null; + activeRecord?: { + recordKind: "managed" | "shared"; + level: string; + manageableDomains: string[]; + } | null; }) { mockPathnameRef.value = props.pathname ?? "/settings/account"; mockModulesRef.value = props.modules; @@ -358,7 +367,11 @@ describe("", () => { it("offers the Modules destination to a guardian on a managed record", () => { const html = renderShell({ active: "account", - activeRecord: { recordKind: "managed", level: "manage" }, + activeRecord: { + recordKind: "managed", + level: "manage", + manageableDomains: delegatedDomains("manage", null, "manage"), + }, }); expect(html).toContain('href="/settings/modules"'); // Its own configuration is not the record's, and stays away. @@ -372,12 +385,36 @@ describe("", () => { // here — both are `managed-guardian`, which an adult record never opens. const html = renderShell({ active: "account", - activeRecord: { recordKind: "shared", level: "manage" }, + activeRecord: { + recordKind: "shared", + level: "manage", + manageableDomains: delegatedDomains("manage", null, "manage"), + }, }); expect(html).not.toContain('href="/settings/modules"'); expect(html).not.toContain('href="/settings/integrations"'); expect(html).toContain('href="/settings/anamnesis"'); }); + + it("lists nothing for a MANAGE grant that does not manage profile", () => { + // The anamnesis forms write through `requireRecordAuth("manage", + // "profile")`, which refuses a grant whose sections miss `profile`, so + // the shell does not list the page. The navigation reads the same + // predicate and offers no Settings entry either. + const html = renderShell({ + active: "account", + activeRecord: { + recordKind: "shared", + level: "manage", + manageableDomains: ["labs"], + }, + }); + expect(html).not.toContain('href="/settings/anamnesis"'); + expect(html).not.toContain('href="/settings/modules"'); + // Non-zero: the shell rendered, and still names the active section in + // its heading, which it resolves from the unfiltered list. + expect(html).toContain('id="settings-section-account-title"'); + }); }); it("falls back to `account` when the pathname doesn't match a known slug", () => { diff --git a/src/lib/record-settings/classification.ts b/src/lib/record-settings/classification.ts index 6c606479a..fbeda64b2 100644 --- a/src/lib/record-settings/classification.ts +++ b/src/lib/record-settings/classification.ts @@ -6,6 +6,7 @@ import type { AccountAccessLevel, AccountRecordKind, } from "@/lib/sharing/account-access-view"; +import type { ShareDomain } from "@/lib/sharing/scope"; export type SettingsDestinationKind = | "personal" @@ -146,14 +147,44 @@ export function isManageDelegateSettingsDestination( return classifySettingsDestination(destination).kind === "manage-writable"; } +/** + * The section each record-content destination writes to. + * + * A `manage-writable` destination is a page of forms whose routes resolve + * `requireRecordAuth("manage",
)`, and that call refuses a grant + * whose sections do not reach the section (`grantCoversDomain`). Level alone + * therefore does not say the page works: the grant must manage that section. + * The anamnesis forms post to `/api/allergies` and `/api/family-history`, both + * declared on `profile`. + * + * A `manage-writable` destination missing here is never listed, so a new one + * cannot appear in a shared record until somebody names what it writes. + */ +export const RECORD_CONTENT_WRITE_DOMAINS = { + anamnesis: "profile", +} as const satisfies Partial>; + /** * The record a Settings listing is drawn for: the server-resolved kind of the - * record on screen and the level of the grant that opened it. `level` is null - * in one's own record and in a context the client could not prove. + * record on screen, the level of the grant that opened it, and the sections + * that grant may manage. `level` is null in one's own record and in a context + * the client could not prove. `manageableDomains` is bound from + * `accountAccess.active.manageableDomains`, never derived here. */ export interface SettingsRecordContext { recordKind: AccountRecordKind; level: AccountAccessLevel | null; + manageableDomains: readonly ShareDomain[]; +} + +function managesRecordContentSection( + destination: string, + record: SettingsRecordContext, +): boolean { + const section = ( + RECORD_CONTENT_WRITE_DOMAINS as Partial> + )[destination]; + return section !== undefined && record.manageableDomains.includes(section); } /** @@ -171,6 +202,15 @@ export interface SettingsRecordContext { * thresholds and notification routing stay with the owner. * * Every other context lists nothing. * + * A record-content destination is listed only when the grant manages the + * section it writes ({@link RECORD_CONTENT_WRITE_DOMAINS}). Today a MANAGE + * grant is whole-record at every layer (`inviteGrant` refuses a scope on a + * MANAGE invitation, no endpoint raises a live grant, and the account-access + * schema rejects a manage entry with sections), so this changes nothing for a + * managed profile or an adult MANAGE share as they exist. It keeps the list + * true if a scoped MANAGE grant ever reaches the client, rather than offering a + * page whose writes answer 403. + * * Every destination on either list needs MANAGE, so the level is checked once * for both record kinds. A guardian grant is always MANAGE today, which is * exactly why the check was easy to leave out, and a managed entry that ever @@ -184,11 +224,13 @@ export function isSettingsDestinationListedForRecord( ): boolean { if (record.level !== "manage") return false; const kind = classifySettingsDestination(destination).kind; - if (record.recordKind === "managed") { - return kind === "managed-guardian" || kind === "manage-writable"; + if (kind === "manage-writable") { + return ( + (record.recordKind === "managed" || record.recordKind === "shared") && + managesRecordContentSection(destination, record) + ); } - if (record.recordKind === "shared") return kind === "manage-writable"; - return false; + return record.recordKind === "managed" && kind === "managed-guardian"; } /**