From f595a7fb8dfbca0278e11a55ba90afcad4fb76d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 13:19:20 +0000 Subject: [PATCH] fix(approvals): refuse cross-org targeting on directory-less approver types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `user`, `field` and `manager` return EARLY in `resolveApproverSpec` — they name a person outright instead of expanding a directory. D9's org resolution (#3824) sat AFTER those returns, so an `organization` declared on one of them never reached the check and was silently INERT. That is precisely the behaviour ADR-0105 D9 rules out and the authoring docs promise against ("`organization` on those is refused at runtime"). The `os lint` rule caught it at author time, but the runtime claim was false — and a flow stored before the lint existed, or assembled programmatically, got no signal. Resolution moves to the top of `resolveApproverSpec`, above every early return, so the refusal reaches all three types. The ordinary path is unchanged and still free: with no `organization` declared the resolver returns the request's organization without reading anything. Why the existing tests missed it: `approver-org-scope.test.ts` calls the resolver DIRECTLY, so it never traverses the early return, and the service-level integration test only covered `position`. Both gaps are closed — the new case opens a real request for each of the three directory-less types and asserts the refusal, plus one that a directory-less approver with NO declaration is untouched, so the guard cannot start costing the ordinary case. Found by cloud's group-posture dogfood on a real `group` boot. The two links that dogfood was written to check both hold: a cross-organization approver does reach her queue through the D2 union wall, and `decide()` does accept her. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL --- ...-0105-d9-refuse-on-directory-less-types.md | 26 +++++++++++++++++ .../plugin-approvals/src/approval-service.ts | 28 +++++++++++++++---- .../approver-cross-org.integration.test.ts | 27 ++++++++++++++++++ 3 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 .changeset/adr-0105-d9-refuse-on-directory-less-types.md diff --git a/.changeset/adr-0105-d9-refuse-on-directory-less-types.md b/.changeset/adr-0105-d9-refuse-on-directory-less-types.md new file mode 100644 index 0000000000..e585eabb21 --- /dev/null +++ b/.changeset/adr-0105-d9-refuse-on-directory-less-types.md @@ -0,0 +1,26 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): refuse `organization` on directory-less approver types instead +of silently ignoring it (ADR-0105 D9) + +`user`, `field` and `manager` return EARLY in `resolveApproverSpec` — they name +a person outright rather than expanding a directory. D9's org resolution was +placed after those returns, so an `organization` declared on one of them never +reached the check: it was silently INERT. + +That is the one behaviour ADR-0105 D9 rules out and the authoring docs +explicitly promise against ("`organization` on those is refused at runtime"). +The `os lint` rule caught it at author time, but the runtime claim was false — +and a stored flow that predates the lint, or one assembled programmatically, +got no signal at all. + +Resolution now happens at the top of `resolveApproverSpec`, above every early +return, so the refusal reaches all three types. The ordinary path is unchanged +and still costs nothing: with no `organization` declared the resolver returns +the request's organization without reading anything. + +Found by cloud's group-posture dogfood driving a real `group` boot — the +resolver's own unit tests could not see it, because they call the resolver +directly and never traverse the early return. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 1bebf99654..8e9c2e5046 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -784,6 +784,23 @@ export class ApprovalService implements IApprovalService { { deprecated: a.type, canonical: type }, ); } + // [ADR-0105 D9] WHERE this approver is looked up — the request's own + // organization unless the spec targets another one in the same group. + // + // Resolved HERE, above the `user` / `field` / `manager` early returns, + // because refusing a declaration on a directory-less type is one of the + // things this resolution DOES (those types name a person outright, so + // `organization` on them cannot narrow anything and an author who wrote it + // misunderstood the field). Resolving it after those returns made the + // refusal unreachable and the declaration silently inert — exactly the + // "ignored, not refused" behaviour ADR-0105 D9 rules out, and what the + // cloud group-posture dogfood caught. + // + // Costs nothing on the overwhelmingly common path: with no `organization` + // declared, the resolver returns the request org without reading anything. + const directoryOrg = await this.directoryOrgFor(a, organizationId); + const crossOrg = directoryOrg !== organizationId; + if (type === 'user') { return this.applyOooDelegation(String(a.value), now, organizationId, substitutions); } @@ -798,12 +815,11 @@ export class ApprovalService implements IApprovalService { } return out; } - // [ADR-0105 D9] WHERE this approver is looked up — the request's own - // organization unless the spec targets another one in the same group. - // Resolution failures propagate: they are routing bugs, and this call is - // OUTSIDE the swallowing try below on purpose (see the catch's comment). - const directoryOrg = await this.directoryOrgFor(a, organizationId); - const crossOrg = directoryOrg !== organizationId; + // `directoryOrg` / `crossOrg` were resolved at the TOP of this method, so + // the refusal reaches directory-less types too. Resolution failures + // propagate: they are routing bugs, and that call sits OUTSIDE the + // swallowing try below on purpose (see the catch's comment). + // // A cross-org slate is filtered to the people who can actually READ the // request (D2 union); same-org routing is untouched and does no extra read. const bounded = async (users: string[]): Promise => ( diff --git a/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts b/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts index 6f449d3405..f85f296448 100644 --- a/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts @@ -158,6 +158,33 @@ describe('ADR-0105 D9 — cross-org approver targeting through ApprovalService', expect(req.pending_approvers).toEqual(['position:cfo']); }); + // Regression: `user` / `field` / `manager` return EARLY in + // `resolveApproverSpec`, before the graph-expansion branch. D9's resolution + // originally sat after those returns, so the declaration on a directory-less + // type was silently INERT rather than refused — the one behaviour ADR-0105 D9 + // and the authoring docs both promise it is not. The resolver's own unit test + // could not see it (it calls the resolver directly); only a request opened + // through the service reaches the early return. Caught by cloud's + // group-posture dogfood. + it('refuses `organization` on a directory-less type — the early return must not skip the check', async () => { + for (const spec of [ + { type: 'user', value: 'u_cfo', organization: '$root' }, + { type: 'field', value: 'owner_id', organization: '$root' }, + { type: 'manager', organization: '$root' }, + ]) { + await expect( + svc.openNodeRequest(openInput([spec]), CTX), + `approver type '${spec.type}' must refuse a cross-org declaration`, + ).rejects.toThrow(/VALIDATION_FAILED.*no effect/); + } + }); + + it('a directory-less approver with NO declaration is untouched by D9', async () => { + // The guard must not cost the ordinary case anything. + const req = await svc.openNodeRequest(openInput([{ type: 'user', value: 'u_plant_mgr' }]), CTX); + expect(req.pending_approvers).toEqual(['u_plant_mgr']); + }); + it('refuses a target outside the group — loudly, and no request is created', async () => { engine._tables['sys_organization'].push({ id: 'o_rival', slug: 'rival-co', parent_organization_id: null }); await expect(