fix(approvals)!: 审批请求按参与者可见,而不是整个租户可见 (#3590)

.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');
+  });
+});