Observer verification: fix coverage bookkeeping, restart sessions when scope widens

docs/sharing.md

@@ -150,13 +150,17 @@ Authorization is enforced at `open()`: the method computes the caller's effectiv
 
 Because the role is recomputed from the graph on every `open()`, the live computation is the *sole* source of truth for access -- there is no eager cleanup whose bugs could grant access to an unreachable user. This is what makes lazy revocation safe: severing an edge is enough to deny access, even though the unreachable records linger in storage.
 
-### Terminating live sessions on revocation
+### Terminating live sessions on revocation or scope growth
 
-Authorization is only checked at `open()`, so a session that is *already* open is not re-checked per message. Without intervention, a collaborator who was just removed or downgraded could keep using their live session until something else disconnected them. To close this gap, `removeCollaborator`/`revokeShareLink` proactively restart the gadget's Overseer DO via `ctx.abort()` whenever the change actually removed or downgraded someone (i.e. the returned `AffectedCollaborator[]` is non-empty; pure no-op removals don't restart). Aborting forcibly disconnects every client; each reconnects and re-runs `open()`, which re-evaluates the now-changed permission graph -- sending removed users to the terminal access-denied page and handing downgraded users their reduced capability (the editor swaps to the `use` view automatically based on `metadata.role`). Since removals are rare (and DOs restart unpredictably anyway, so reconnects are already cheap), the disruption is acceptable.
+Authorization is only checked at `open()`, so a session that is *already* open is not re-checked per message. Without intervention, a collaborator who was just removed or downgraded could keep using their live session until something else disconnected them. To close this gap, `removeCollaborator`/`revokeShareLink` proactively restart the gadget's Overseer DO whenever the change actually removed or downgraded someone (i.e. the returned `AffectedCollaborator[]` is non-empty; pure no-op removals don't restart). Resetting the DO forcibly disconnects every client; each reconnects and re-runs `open()`, which re-evaluates the now-changed permission graph -- sending removed users to the terminal access-denied page and handing downgraded users their reduced capability (the editor swaps to the `use` view automatically based on `metadata.role`). Since removals are rare (and DOs restart unpredictably anyway, so reconnects are already cheap), the disruption is acceptable.
 
-Two precautions surround the abort (`OverseerImpl.scheduleRevocationRestart`): the severed edge is flushed with `ctx.storage.sync()` first (because `ctx.abort()` does not respect the output gate, a restart could otherwise come back with the change lost), and the abort is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO aborts, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect.
+Two precautions surround the reset (`OverseerImpl.scheduleAccessRestart`). The reset is delayed ~100ms so the triggering RPC's response reaches the caller -- typically the owner, who is also connected -- before their own connection drops. And because resetting the DO does not respect the output gate, storage is flushed with `ctx.storage.sync()` immediately before it, or a restart could come back with the change lost. The flush has to happen at the end rather than up front: the delay runs concurrently with *other* access mutations, and a revocation that has written its edge and is still awaiting `tearDownLostObservers` would otherwise be reset with its write still buffered and silently never happen, while its caller was told it succeeded.
 
-Note this is only needed for removals/downgrades. Granting or raising access never strands anyone, and `prohibitAllSharing` cannot strand a session either: an observation that would set that flag is *blocked* (rather than applied) if the gadget is already shared, so the flag only ever flips to true on a gadget with no other sessions to evict.
+The flush and the reset run together inside `ctx.blockConcurrencyWhile()`, throwing out of the callback to reset the object rather than calling `ctx.abort()` after the sync. This is what makes the pair airtight: a bare sync only narrows the window, since a request delivered once it resolves can still write a mutation the reset then discards. Nothing is delivered to the object for the duration of the block, so there is no moment between "everything is durable" and "the object is gone" at which anything can run. The `scheduler.wait()` stays outside the block -- blocking concurrency across the whole delay would stall every unrelated request for it. Concurrent triggers coalesce onto one timer rather than racing several resets. The disconnect reaches the browser through the existing `notifyClosed` plumbing: when the Overseer DO is reset, the per-session `notifyClosed` stub is disposed without being called, which `AuthenticatedApiImpl` treats as a lost connection and reacts to by killing the browser WebSocket, forcing a reconnect.
+
+Granting or raising access never strands anyone: a live session's capability is fixed at open, so a `use` collaborator promoted to `build` in the graph still holds `UseOverseerInterface` until they re-open, and nobody is newly excluded from anything. `prohibitAllSharing` cannot strand a session either: an observation that would set that flag is *blocked* (rather than applied) if the gadget is already shared, so the flag only ever flips to true on a gadget with no other sessions to evict.
+
+The same reset serves a second purpose, though, and there the trigger is a *grant*: observer verification (see docs/observers.md) also runs only at `open()`, so widening the set of gatekeepers a collaborator must be verified against leaves their live session holding access they were never verified for. `OverseerImpl.#restartIfSessionsAffected` restarts the workspace whenever that happens -- a connection is added, one is bound into a gadget, or a merge promotes such a binding -- so every client re-opens and re-runs `ensureObserver` at the new scope. It is a no-op unless a collaborator session of the affected role is live -- severing sessions is all a restart does -- so a solo workspace, or one whose collaborators are all disconnected, is never disturbed. See docs/observers.md, "Restarting when verification scope widens", for the full trigger list and the reasoning about what deliberately does *not* trigger it.
 
 ## Future work
 

packages/integration-tests/__tests__/external-message-verification.test.ts

@@ -0,0 +1,205 @@
+// Tests for the external-message authorization gate (authorizeCollaborator in overseer.ts):
+// receiveExternalMessage() must hold a collaborator to the same observer verification open()
+// applies -- non-interactively, since this path has no way to prompt for account configuration --
+// and must deny an insufficient role *before* verification runs.
+//
+// These live in their own file -- with their own harness, like every suite here -- so the suite
+// stays self-contained as the observer suites around it grow.
+
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import type { RpcStub } from "capnweb";
+import type { AuthenticatedApi, PublicApi } from "@gadgets/workshop-shared/api";
+import type {
+  SubmitExternalMessageResult,
+} from "@gadgets/workshop-shared/external-message-gateway";
+import {
+  startTestGatekeeperHarness, TEST_GATEKEEPER_WORKER, TEST_VENDOR_ID, type Harness,
+} from "../src/harness.js";
+import {
+  accountLabel, connect, listConnectedAccounts, MAX_OBSERVER_PROMPTS, nextUsernames,
+  ObserverConfigRecorder, signUp, stubFor, waitFor, type ConnectedAccount,
+} from "../src/rpc-client.js";
+import { NetworkInterceptor } from "../src/network-interceptor.js";
+
+// Reason text shaped like what a gatekeeper actually reports on a settled denial. Its appearance
+// in the gateway's reply below is what proves a live verification round trip happened.
+const DENIED_REASON = "You do not have access to this thing.";
+
+let harness: Harness;
+let interceptor: NetworkInterceptor;
+
+beforeAll(async () => {
+  interceptor = new NetworkInterceptor();
+  interceptor.install();
+  harness = await startTestGatekeeperHarness();
+});
+
+afterAll(async () => {
+  const unmocked = interceptor.getUnmockedCalls();
+  await harness?.server.close();
+  interceptor.uninstall();
+  interceptor.reset();
+  expect(unmocked).toEqual([]);
+});
+
+async function withSession<T>(body: (api: RpcStub<PublicApi>) => Promise<T>): Promise<T> {
+  const publicApi = connect(harness.url);
+  try {
+    return await body(publicApi);
+  } finally {
+    publicApi[Symbol.dispose]();
+  }
+}
+
+function thingUrl(name: string): string {
+  return `https://gadgets-test.example/things/${name}`;
+}
+
+async function provisionAccount(api: RpcStub<AuthenticatedApi>): Promise<ConnectedAccount> {
+  await api.provisionAmbientAccount(TEST_VENDOR_ID);
+  return waitFor("the test account to be provisioned", async () => {
+    const accounts = await listConnectedAccounts(api);
+    return accounts.find(a => a.vendorId === TEST_VENDOR_ID) ?? null;
+  });
+}
+
+/**
+ * Submit an external chat message as `callerEmail`, through the fixture worker's control surface
+ * (and so through the Workshop's real ExternalMessageGateway entrypoint).
+ */
+async function submitExternalMessage(input: {
+  callerEmail: string; gadgetKey: string; prompt: string;
+}): Promise<SubmitExternalMessageResult> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/submit-external-message",
+    { method: "POST", body: JSON.stringify({
+        chatKey: `chat-${input.gadgetKey}`, messageKey: crypto.randomUUID(),
+        gadgetTitle: input.gadgetKey, ...input }) });
+  if (res.status !== 200) {
+    throw new Error(`submit-external-message failed with ${res.status}: ${await res.text()}`);
+  }
+  return await res.json() as SubmitExternalMessageResult;
+}
+
+/** Tell the gatekeeper what to do the next time it's asked to admit `label` as an observer. */
+async function setVerifyOutcome(
+    label: string, outcome: { allow: true } | { allow: false; reason: string }): Promise<void> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/verify-outcome",
+    { method: "POST", body: JSON.stringify({ label, ...outcome }) });
+  if (res.status !== 204) {
+    throw new Error(`Setting the verify outcome failed with ${res.status}: ${await res.text()}`);
+  }
+}
+
+/** The workspace id behind an external gadgetKey -- the DO id the gateway derives from it. */
+async function externalGadgetId(gadgetKey: string): Promise<string> {
+  const res = await harness.fetchWorker(
+    TEST_GATEKEEPER_WORKER, "http://gatekeeper-test.test/control/external-gadget-id",
+    { method: "POST", body: JSON.stringify({ gadgetKey }) });
+  if (res.status !== 200) {
+    throw new Error(`external-gadget-id failed with ${res.status}: ${await res.text()}`);
+  }
+  return (await res.json() as { gadgetId: string }).gadgetId;
+}
+
+describe("external-message verification", () => {
+  it.concurrent("the external-message path verifies collaborators like open() does", async () => {
+    await withSession(async publicApi => {
+      const [alice, bob, carol] = nextUsernames("alice", "bob", "carol");
+      const aliceApi = await signUp(publicApi, alice);
+      const aliceAccount = await provisionAccount(aliceApi);
+      const gadgetKey = `external-${crypto.randomUUID()}`;
+
+      // Alice creates the workspace through the external channel. No test user has an AI model,
+      // so a submission that passes the authorization gate is rejected with the model message --
+      // which is what tells "passed the gate" apart from a gate denial below.
+      await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+
+      // Wire the workspace up over the web API: connect a Thing (an account-requiring connection,
+      // so collaborators must be observer-verified against it) and add Bob.
+      const gadgetId = await externalGadgetId(gadgetKey);
+      using overseer = await aliceApi.openGadget(gadgetId);
+      const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external"));
+      if (!gatekeeper) throw new Error("Failed to create the test connection");
+
+      const bobApi = await signUp(publicApi, bob);
+      const bobAccount = await provisionAccount(bobApi);
+      await overseer.addCollaborator(bob, "build");
+
+      // A stranger is turned away by role, before verification is ever attempted.
+      await signUp(publicApi, carol);
+      await expect(submitExternalMessage({ callerEmail: carol, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/do not have access/i) });
+
+      // Bob has build access but has never opened, so he was never observer-verified -- and this
+      // path has no configuration channel to fix that. The agent's reply could surface anything
+      // the workspace has already read, so the external path must refuse him rather than fall
+      // through to the model check.
+      await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/could not be verified/i) });
+
+      // Opening the workspace verifies him; the same submission now passes the gate and fails
+      // only on the missing AI model, exactly like the owner's did.
+      const callback = stubFor(
+          new ObserverConfigRecorder().alwaysChoose(bobAccount.id, MAX_OBSERVER_PROMPTS));
+      try {
+        (await bobApi.openGadget(gadgetId, undefined, callback))[Symbol.dispose]();
+      } finally {
+        callback[Symbol.dispose]();
+      }
+      await expect(submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+
+      // The gatekeeper now revokes Bob's underlying access. His persisted observer record is
+      // untouched, so only a live addObserver re-verification on this submission can notice --
+      // and the gatekeeper's own refusal reason appearing in the reply is the proof that round
+      // trip happened, since nothing persisted in the Workshop contains it. An implementation
+      // that merely checked the record would keep accepting him here.
+      await setVerifyOutcome(accountLabel(bobAccount), { allow: false, reason: DENIED_REASON });
+      const revoked = await submitExternalMessage({ callerEmail: bob, gadgetKey, prompt: "hi" });
+      if (revoked.accepted) throw new Error("The revoked submission was accepted");
+      expect(revoked.message).toMatch(/could not be verified/i);
+      expect(revoked.message).toContain(DENIED_REASON);
+    });
+  });
+
+  it.concurrent("the external-message path denies a use collaborator by role, not verification",
+      async () => {
+    await withSession(async publicApi => {
+      const [alice, dave] = nextUsernames("alice", "dave");
+      const aliceApi = await signUp(publicApi, alice);
+      const aliceAccount = await provisionAccount(aliceApi);
+      const gadgetKey = `external-use-${crypto.randomUUID()}`;
+
+      // Alice creates the workspace through the external channel (the AI-model rejection means
+      // her submission passed the gate), then binds its connection to a gadget so it falls in
+      // "use" verification scope.
+      await expect(submitExternalMessage({ callerEmail: alice, gadgetKey, prompt: "hello" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/AI model/i) });
+      const gadgetId = await externalGadgetId(gadgetKey);
+      using overseer = await aliceApi.openGadget(gadgetId);
+      const gatekeeper = await overseer.newGatekeeper(aliceAccount.id, thingUrl("external-use"));
+      if (!gatekeeper) throw new Error("Failed to create the test connection");
+      using gadget = await overseer.createGadget("Test Gadget", undefined, "TEST_GADGET");
+      await gadget.bind("TEST_THING", await gatekeeper.getId());
+
+      // Dave is in verification scope and unverified, but this path can never grant a "use"
+      // collaborator agent access, so his role is checked before verification runs: he gets the
+      // plain denial, not a verification failure he has no reason to go fix.
+      await signUp(publicApi, dave);
+      if (!await overseer.addCollaborator(dave, "use")) {
+        throw new Error(`Failed to share the gadget with ${dave}`);
+      }
+      await expect(submitExternalMessage({ callerEmail: dave, gadgetKey, prompt: "hi" }))
+          .resolves.toMatchObject({
+            accepted: false, message: expect.stringMatching(/do not have access/i) });
+    });
+  });
+});