Merge pull request #690 from MBombeck/work/sharing-client

Account sharing, the part people touch

src/__tests__/acting-account-boundary-guard.test.ts

@@ -81,6 +81,16 @@ describe("the acting-account carrier is read in one place", () => {
     // themselves call these two functions and never name the column, which is
     // what keeps this list from growing one entry per surface.
     "lib/sharing/acting-session.ts",
+    // v1.36.0 — the only thing that PUBLISHES it, and it publishes the stamp
+    // only after re-deciding it. The account payload has to say which record
+    // the browser is inside so the banner can name a person, but the value it
+    // prints is not the column: an entry reaches `active` solely by surviving
+    // the same live-grant pass that builds the switcher list, so a stamp left
+    // behind by a lapsed grant reads as "not switched" — the same answer the
+    // resolver will independently give the next delegated request. Takes the
+    // whole auth context for exactly this reason: had it taken the id, the
+    // route would name the column too, and the list would grow per surface.
+    "lib/sharing/account-access.ts",
   ].sort();
 
   it("no other file reads or writes the carrier column", () => {

src/__tests__/delegable-surface-guard.test.ts

@@ -252,13 +252,15 @@ const DELEGABLE_ROUTES: Record<string, string> = {};
  *
  * The intended members are the surfaces a switched session needs in order to
  * stay usable and to get back out: the account bootstrap payload, the switch
- * endpoint, logout, the native refresh route, the locale read. One of them
- * exists so far. The rest arrive as their own diffs here; naming them before
+ * endpoint, logout, the native refresh route, the locale read. Two of them
+ * exist so far. The rest arrive as their own diffs here; naming them before
  * they exist would freeze a guess.
  */
 const ACTOR_ROUTES: Record<string, string> = {
   "app/api/account/switch/route.ts":
     "The way back out. Every other route refuses under a switch, so if this one did too a browser could enter a record and never leave it. It reads and writes exactly one row — the caller's own session — renders nothing of the owner's, and grants nothing: the account it stamps is validated against a live grant first, and the stamp is re-checked on every request after.",
+  "app/api/auth/me/route.ts":
+    "The app shell reads it on every boot, including while a switch is on: the switcher, the banner naming whose record is open, and the route back out all bind this payload. Every field it returns is the caller's own — their preferences, their modules, their identity — and the one field about the switch says only which records they may open and which they are inside. It reads no row of the owner's, and its `accountAccess` block grants nothing: the resolver re-checks the grant on every delegated request regardless of what this payload said a moment ago.",
 };
 
 /**
@@ -266,7 +268,7 @@ const ACTOR_ROUTES: Record<string, string> = {
  * a formality by accident: every addition has to be counted here as well as
  * listed above, which is one more place a careless admission has to pass.
  */
-const FROZEN_ENTRY_COUNT = 1;
+const FROZEN_ENTRY_COUNT = 2;
 
 /**
  * The two surfaces that authenticate a Bearer token outside `requireAuth` —

src/__tests__/destructive-control-guard.test.ts

@@ -500,6 +500,40 @@ const REGISTRY: DestructiveEntry[] = [
     ],
   },
 
+  // ── Account sharing ─────────────────────────────────────────────────────
+  {
+    file: "components/settings/access/grants-given-card.tsx",
+    destroys:
+      "another account's standing access to this health record, and every browser session of theirs that was inside it",
+    recovery: "permanent",
+    confirm: ["ConfirmButton"],
+    undetected:
+      "No detector sees it. The DELETE goes out from `useRevokeGrant` in `lib/queries/use-account-grants.ts`, and `lib/` is an excluded root; the card itself only calls `.mutate(grant.id)` with a bare string, which no payload pattern matches, and the route path lives in the hook rather than here. Registered by hand.",
+  },
+  {
+    file: "components/settings/access/grants-received-card.tsx",
+    destroys:
+      "this account's own access to somebody else's record, handed back rather than withdrawn",
+    recovery: "permanent",
+    confirm: ["ConfirmButton"],
+    undetected:
+      "Same shape as the card above, and one step further out of reach: renouncing is a POST, so not even a DELETE goes out. The path (`/api/account/grants/{id}/renounce`) names an act the D2 vocabulary does not carry, and it lives in the hook regardless. Registered by hand.",
+  },
+  {
+    file: "components/settings/access/grant-invite-card.tsx",
+    destroys:
+      "nothing — it offers access, and the offer confers nothing until the other person accepts it",
+    recovery: "permanent",
+    confirm: [],
+    unconfirmed: [
+      {
+        control: "the invitation form",
+        reason:
+          "D3 flags this file for `expiresAt: null` in the invite payload, reading it as a field being blanked. It is not: null means the grant runs until somebody ends it, which is the common household case and the absence of an expiry rather than the removal of one. The form creates a pending row and destroys nothing, and gating a creation behind a confirmation dialog would teach people to click through the dialogs that do guard a loss.",
+      },
+    ],
+  },
+
   // ── Admin ───────────────────────────────────────────────────────────────
   {
     file: "components/admin/danger-zone-section.tsx",

src/__tests__/e2e-shared-session-guard.test.ts

@@ -0,0 +1,95 @@
+/**
+ * Structural guard on the E2E suite's session boundary.
+ *
+ * Account switching is stamped on the session row, not held in the tab. Every
+ * Playwright context opened from `STORAGE_STATE_PATH` carries the SAME session
+ * cookie, so a spec that switches that jar into another account's record
+ * switches it for every spec authenticating with it — and the suite runs
+ * `fullyParallel`. The failure lands in an unrelated file: a settings surface
+ * painting "Not part of shared access", an axe scan reading a banner its spec
+ * never asked for, a `waitForResponse` on a request the refused surface never
+ * sends. Nothing in the spec that fails is wrong, and nothing in its file
+ * explains it.
+ *
+ * So: a spec that switches accounts uses its own jar. This file holds that
+ * shape by matching the two things a switching spec cannot avoid naming — the
+ * menu entry it clicks and the endpoint that entry calls.
+ *
+ * It is a tripwire, not a proof. A future spec could switch through a control
+ * neither marker names, and this guard would not see it; the counter-check
+ * below is what stops the matchers from quietly matching nothing.
+ */
+import { describe, it, expect } from "vitest";
+import { readFileSync, globSync } from "node:fs";
+import { join, sep } from "node:path";
+
+const E2E = join(process.cwd(), "e2e");
+
+/**
+ * Markers that mean "this spec drives an account switch": the switcher entry
+ * a person clicks, and the endpoint that click posts to.
+ */
+const SWITCH_MARKERS = ["account-switcher-entry", "/api/account/switch"];
+
+/** The jar shared by every ordinary authenticated spec. */
+const SHARED_JAR = "STORAGE_STATE_PATH";
+
+function specFiles(): string[] {
+  return globSync("*.spec.ts", { cwd: E2E })
+    .map((p) => p.split(sep).join("/"))
+    .sort();
+}
+
+function read(rel: string): string {
+  return readFileSync(join(E2E, rel), "utf8");
+}
+
+/**
+ * The file with comments removed. This guard's own subject matter is worth
+ * explaining in prose inside the specs, and a naive substring match would be
+ * satisfied by that prose alone.
+ */
+function stripComments(src: string): string {
+  return src.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^\s*\/\/.*$/gm, "");
+}
+
+function switchingSpecs(): string[] {
+  return specFiles().filter((rel) => {
+    const src = stripComments(read(rel));
+    return SWITCH_MARKERS.some((marker) => src.includes(marker));
+  });
+}
+
+/**
+ * `STORAGE_STATE_PATH` as a whole word, so `OWNER_STORAGE_STATE_PATH` and
+ * `DELEGATE_STORAGE_STATE_PATH` — the isolated jars, which are the fix rather
+ * than the problem — do not read as a hit.
+ */
+function usesSharedJar(src: string): boolean {
+  return new RegExp(`(^|[^A-Z_])${SHARED_JAR}\\b`).test(src);
+}
+
+describe("e2e specs that switch accounts do not share a session", () => {
+  it("finds the specs that switch at all", () => {
+    // An empty match set would make every assertion below vacuous. The suite
+    // has a switching journey; if it ever has none, this guard has no subject
+    // and should be deleted rather than left passing.
+    expect(switchingSpecs().length).toBeGreaterThan(0);
+  });
+
+  it("none of them authenticates with the shared jar", () => {
+    const offenders = switchingSpecs().filter((rel) =>
+      usesSharedJar(stripComments(read(rel))),
+    );
+    expect(offenders).toEqual([]);
+  });
+
+  it("the shared jar is still what the ordinary specs read", () => {
+    // The other way this guard could go quietly green: the shared jar renamed
+    // or retired, leaving the matcher above looking for a string nothing uses.
+    const sharing = specFiles().filter((rel) =>
+      usesSharedJar(stripComments(read(rel))),
+    );
+    expect(sharing.length).toBeGreaterThan(10);
+  });
+});