fix(approvals)!: scope approval requests to their participants, not the tenant (#3590) (#3599)

* fix(approvals)!: scope approval requests to their participants, not the tenant (#3590)

getRequest / listRequests / countRequests deliberately query with SYSTEM_CTX to
bypass RLS — as their own comments say, the approver-visibility rule spans
identity forms RLS cannot model cleanly, so it has to be expressed in the
service. Only the TENANT half of that rule was ever written. The comment names
the participant half; the code never applied it. So any authenticated user could
read any approval request in their tenant: its payload snapshot, its full
decision history, and — once decision attachments derived their access from the
request (#3580) — its files.

`approverId` on listRequests is a FILTER, not authorization. Omitting it
returned the whole tenant.

A caller now sees a request when they participate in it: the submitter, a
current approver (via the normalized approver index, so every identity form the
write path recorded is covered), or someone who already acted on it — a past
approver whose slot has moved on, a commenter. Override admins keep the
unrestricted view the "all requests" console surface depends on; a tokenless
context sees nothing.

Keying on the concrete user id is sufficient rather than an approximation, and
that is worth stating because under-granting here would silently hide approvals
someone must act on: position/team/manager/field approvers are resolved to
concrete user ids at open time, and the `type:value` literal is only the
fallback for a spec that resolved to NOBODY — a slot no one can act on either
way, since can_act is a plain membership test over the resolved ids.

A write path's own result is NOT re-gated. Every operation echoes back the
request it just changed via getRequest; the operation authorized itself before
writing, and re-asking answers a settled question — wrongly, for a context that
carries no userId (a flow-driven resume, a service-to-service call), turning a
successful write into a null result. Four existing tests caught exactly that,
which is why the echo now goes through readBackRequest instead.

Verified in the browser on app-showcase: a non-admin approver's unfiltered list
drops from 3 requests to the 1 she is on, an admin still sees all 3, her
decision attachment still downloads (200, real PDF bytes — #3580 composes), and
a request she has nothing to do with reads back empty.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd

* docs(approvals): state who can see a request, now that it is participant-scoped

The drift check flagged approvals.mdx, and rightly: it documented `approverId`
as one filter among several and never said who may see what — so a reader would
reasonably assume omitting it returns the whole tenant, which is exactly what
this PR stops being true.

Also records that a decision's attachments inherit the same rule, since that is
the non-obvious half: the file is as readable as the decision, never more.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/approval-participant-visibility.md

@@ -0,0 +1,39 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals)!: an approval request is visible to its participants, not to the whole tenant (#3590)
+
+`getRequest` / `listRequests` / `countRequests` deliberately query with
+`SYSTEM_CTX` to bypass RLS — as the code comments say, the approver-visibility
+rule spans identity forms RLS cannot model cleanly, so it has to be expressed in
+the service. Only the **tenant** half of that rule was ever applied. The
+participant half was named in the comment and never written, so **any
+authenticated user could read any approval request in their tenant** — its
+payload snapshot, its full decision history, and (once decision attachments
+derived their access from the request, #3580) its files.
+
+`approverId` on `listRequests` is a *filter*, not authorization: omitting it
+returned the whole tenant.
+
+A caller now sees a request when they are a participant — the submitter, a
+current approver (via the normalized approver index, so every identity form the
+write path recorded is covered), or someone who has already acted on it (a past
+approver whose slot has moved on, a commenter). Admins with override authority
+keep the unrestricted view the "all requests" console surface depends on, and a
+tokenless context sees nothing.
+
+Keying on the concrete user id is sufficient rather than an approximation:
+position/team/manager/field approvers are resolved to concrete user ids at open
+time, and the `type:value` literal is only the fallback for a spec that resolved
+to *nobody* — a slot no one can act on either way. So this cannot hide a request
+from someone who could actually act on it.
+
+**A write path's own result is not re-gated.** Every operation echoes back the
+request it just changed; the operation already authorized itself, and re-asking
+would answer wrong for a context carrying no `userId` (a flow-driven resume, a
+service-to-service call), turning a successful write into `null`.
+
+Marked breaking because a client that listed requests without an `approverId`
+filter and expected the whole tenant will now receive only its own — which is
+the point.

content/docs/automation/approvals.mdx

@@ -244,6 +244,20 @@ a person's identities in one call.
 Other filters: `status`, `object`, `recordId`, `submitterId`, `q`, `limit`,
 `offset`.
 
+<Callout type="warn">
+**`approverId` is a filter, not authorization.** What you may see is decided
+separately: a request is visible to its **participants** — the submitter, a
+current approver, and anyone who has already acted on it (a past approver whose
+slot has moved on, a commenter). So omitting `approverId` returns *your*
+requests, not every request in the tenant. Admins with override authority
+(`admin_full_access`, or `organization_admin` within their org) see all of them
+— that is what the "all requests" view is for.
+
+The same rule governs a decision's files: an attachment is exactly as readable
+as the decision it hangs off, never more (`sys_approval_action` delegates that
+question back to this service via `fileAccessDelegate`).
+</Callout>
+
 <Callout type="warn">
 **Opening a request notifies nobody.** There is no built-in "you have an
 approval waiting" message today — an approver only discovers it by looking at

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -740,8 +740,10 @@ describe('ApprovalService (node era)', () => {
     expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false, can_override: false });
     const asSubmitter = await svc.getRequest(req.id, { userId: 'u1', tenantId: 't1' } as any);
     expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true, can_override: false });
-    const asOther = await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any);
-    expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false, can_override: false });
+    // #3590: a same-tenant stranger participates in nothing, so the request is
+    // not readable at all — previously it came back with an all-false viewer
+    // block, which meant every authenticated user could read every request.
+    expect(await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any)).toBeNull();
   });
 
   it('getRequest: viewer.can_act drops to false once the request is finalized', async () => {
@@ -1477,12 +1479,22 @@ describe('ApprovalService — admin override (#3424)', () => {
     const req = await svc.openNodeRequest(stuckInput(), CTX);
     const asAdmin = await svc.getRequest(req.id, TENANT_ADMIN);
     expect(asAdmin!.viewer).toMatchObject({ can_act: false, can_override: true });
-    const asMember = await svc.getRequest(req.id, MEMBER);
-    expect(asMember!.viewer!.can_override).toBe(false);
+    // Someone who CAN see the request but holds no override privilege — the
+    // submitter. `can_override` is about the privilege, not about access, so
+    // the check needs a participant rather than a stranger.
+    const asSubmitter = await svc.getRequest(req.id, CTX);
+    expect(asSubmitter!.viewer!.can_override).toBe(false);
     await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN);
     const after = await svc.getRequest(req.id, TENANT_ADMIN);
     expect(after!.viewer!.can_override).toBe(false);
   });
+
+  // #3590: a plain member who participates in nothing now sees nothing — a
+  // request is no longer readable merely because you are in the same tenant.
+  it('a non-participant member cannot read the request at all', async () => {
+    const req = await svc.openNodeRequest(stuckInput(), CTX);
+    expect(await svc.getRequest(req.id, MEMBER)).toBeNull();
+  });
 });
 
 describe('record-lock hook (node era)', () => {
@@ -2114,3 +2126,101 @@ describe('ApprovalService — authorizeFileRead delegate (ADR-0104 D3 wave 2)',
     expect((SysApprovalAction as any).fileAccessDelegate).toBe('approvals');
   });
 });
+
+// ── Participant visibility (#3590) ───────────────────────────────────
+//
+// getRequest/listRequests deliberately query with SYSTEM_CTX (the
+// approver-visibility rule spans identity forms RLS cannot model), but only
+// the TENANT half of that rule was ever applied — so any authenticated user
+// could read any request in their tenant, and after #3580 its decision
+// attachments too. These lock in the participant half.
+describe('ApprovalService — participant visibility (#3590)', () => {
+  const svcFor = (engine: any) => {
+    let n = 0;
+    return new ApprovalService({ engine, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
+  };
+  const asUser = (userId: string) => ({ userId, tenantId: 't1', positions: [], permissions: [] } as any);
+  const ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
+
+  it('the submitter and a pending approver can read it; a same-tenant stranger cannot', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX); // submitter u1, approver u9
+
+    expect(await svc.getRequest(req.id, asUser('u1'))).not.toBeNull();
+    expect(await svc.getRequest(req.id, asUser('u9'))).not.toBeNull();
+    expect(await svc.getRequest(req.id, asUser('u_stranger'))).toBeNull();
+  });
+
+  it('someone who already acted keeps access after their slot moves on', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
+
+    // u9 is no longer a pending approver, but the decision is theirs — the
+    // audit trail (and its attachments) must not vanish from under them.
+    expect(await svc.getRequest(req.id, asUser('u9'))).not.toBeNull();
+  });
+
+  it('an override admin keeps the unrestricted view the console depends on', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+
+    expect(await svc.getRequest(req.id, ADMIN)).not.toBeNull();
+  });
+
+  it('a tokenless context sees nothing — the gate fails closed', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+
+    expect(await svc.getRequest(req.id, { tenantId: 't1', positions: [], permissions: [] } as any)).toBeNull();
+  });
+
+  it('listRequests no longer returns the whole tenant when no approverId filter is passed', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const mine = await svc.openNodeRequest(openInput(['u9']), CTX);            // submitter u1
+    await svc.openNodeRequest(openInput(['u7'], { recordId: 'opp2', record: { id: 'opp2' } }), asUser('u_other')); // unrelated to u1
+
+    // The old behaviour: omit approverId and receive every request in the
+    // tenant. `approverId` is a filter, never authorization.
+    const seen = await svc.listRequests(undefined, asUser('u1'));
+    expect(seen.map(r => r.id)).toEqual([mine.id]);
+
+    const strangerSees = await svc.listRequests(undefined, asUser('u_stranger'));
+    expect(strangerSees).toEqual([]);
+
+    // The admin console still sees everything.
+    expect((await svc.listRequests(undefined, ADMIN)).length).toBeGreaterThanOrEqual(2);
+  });
+
+  it('countRequests agrees with the list it paginates', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    await svc.openNodeRequest(openInput(['u9']), CTX);
+    await svc.openNodeRequest(openInput(['u7'], { recordId: 'opp2', record: { id: 'opp2' } }), asUser('u_other'));
+
+    expect(await svc.countRequests(undefined, asUser('u_stranger'))).toBe(0);
+    expect(await svc.countRequests(undefined, asUser('u1'))).toBe(1);
+  });
+
+  it('a write path still echoes back its own result when the context has no userId', async () => {
+    const engine = makeFakeEngine();
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+
+    // Flow-driven resumes and service-to-service calls carry no userId. The
+    // operation authorized itself; re-gating the echo would turn a successful
+    // write into a null result.
+    const res = await svc.decideNode(
+      req.id,
+      { decision: 'approve', actorId: 'u9' },
+      { isSystem: false, positions: [], permissions: [] } as any,
+    );
+    expect(res.request).not.toBeNull();
+    expect(res.request.status).toBe('approved');
+  });
+});