diff --git a/.changeset/manager-approver-org-screen.md b/.changeset/manager-approver-org-screen.md new file mode 100644 index 0000000000..26301d1362 --- /dev/null +++ b/.changeset/manager-approver-org-screen.md @@ -0,0 +1,65 @@ +--- +"@objectstack/plugin-approvals": minor +--- + +fix(approvals): screen the `manager` approver to the request's organization (#10153) + +`expandApprovers` hands the directory organization to every graph-shaped +approver expansion — `department`, `position`, `org_membership_level`. The +`manager` branch did not: `lookupManager` read `sys_user.manager_id` under a +system context and took no organization argument at all. `sys_user` is a global +identity table with no `organization_id`, so nothing else on that path supplied +the tenancy fact either. A `manager_id` crossing an organization boundary +therefore routed the submission to an approver **in another organization** — an +out-of-tenant person granted approval authority over the record. + +The same column has been screened on the hierarchy side since cloud#1195. This +brings the approvals consumer into line for the `manager` branch. + +## What the screen is + +`lookupManager(userId, organizationId)` now resolves the manager and then asks +whether he is **provably outside** the request's organization: + +| membership rows for the manager | result | +|---|---| +| some exist, none in the request's org | **screened out** — the slot falls through to the `manager:` literal | +| one is in the request's org | resolves, unchanged | +| none exist at all | resolves, unchanged — the tenancy fact is absent, not negative | +| the `sys_member` read failed | resolves, unchanged | +| the request carries no organization | resolves, unchanged — and no read is performed | + +The fail-open half is this file's ruled posture on addressing paths, stated +twice already: `filterApproversWhoCanRead` refuses to empty a live slate on an +infrastructure hiccup, and `expandPositionUsers` carries "a step routing to +nobody is worse than one routing to a lapsed holder". A drop is logged with the +manager's id, his organizations and the request's, so the fix ("repair the link" +/ "grant the membership" / "retarget the step") is legible without a debugger. + +## ⚠️ This moves one input from accepted to refused + +A node whose **sole** approver is a cross-org `manager` and which is authored +with the **non-default** `onEmptyApprovers: 'fail'` used to open successfully; +it now throws `NO_APPROVERS`. Nothing new is thrown — a screened-out manager +leaves only a `type:value` literal, which the pre-existing empty-slate test +already classifies as empty, and `'fail'` already throws on empty. Every +screened sibling has reached that same bucket since it was written. + +**The default policy is unaffected**: `admin_rescue` still opens the request +(decidable by a privileged admin) and warns, and `auto_approve` still +auto-approves. Both directions and both policies are pinned in +`manager-approver-org-screen.test.ts`. + +## What this does NOT decide + +- **#7497** (does approver routing imply record read visibility?) stays open. + The screen reads `sys_member`, which looks like the D2 read filter beside it, + and the code says at length why it is the *sibling* treatment instead: two of + the three org-scoped expansions already screen on `sys_member.organization_id`, + and `sys_user` offers no other tenancy fact. No reads are granted and no read + screen is applied to any type that lacked one. +- **`team`** is still unscreened — it is a sibling graph expansion that is not + org-scoped either, tracked as #10230, and it touches this same file. +- `APPROVER_ORG_SCOPED` is untouched. It answers ADR-0105 D9 *retargetability* + (may an author write `organization:` on this type?), not screening, and + `manager: false` remains correct. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 4f0a054ee2..8e66838682 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -985,7 +985,12 @@ export class ApprovalService implements IApprovalService { } else if (type === 'manager' && record) { const subject = (record as any)[a.value] ?? (record as any).owner_id; if (subject) { - const mgr = await this.lookupManager(String(subject)); + // #10153: the request's OWN organization, not `directoryOrg`. They are + // provably equal on this branch (`manager` is not org-scoped, so a + // `organization` declaration is refused above), and naming the request + // org says what the screen asserts: tenancy of the request, never an + // ADR-0105 D9 retarget this type does not have. + const mgr = await this.lookupManager(String(subject), organizationId); if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions); } } @@ -1339,16 +1344,112 @@ export class ApprovalService implements IApprovalService { return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); } - private async lookupManager(userId: string): Promise { + /** + * `sys_user.manager_id`, screened to the request's organization (#10153). + * + * Takes an organization argument for the same reason its siblings do + * ({@link expandPositionUsers}, {@link expandMembershipTierUsers}): an + * approver expansion answers "who, in THIS organization". Before #10153 this + * one did not ask, and it was the only expansion that did not — a + * `manager_id` pointing at a person in another organization routed that + * person an approval over a record they are not a tenant of. + * + * ⚠️ The screen reads `sys_member`, which LOOKS like the D2 read-visibility + * filter next to it ({@link filterApproversWhoCanRead}). It is not, and this + * comment exists so the next reader does not conclude that #7497 (does + * approver routing imply record read visibility?) was settled here. It was + * not. Two facts make this the SIBLING treatment rather than a + * read-visibility ruling: + * + * 1. Two of the three org-scoped expansions already screen on exactly this + * column — `expandMembershipTierUsers` filters `sys_member.organization_id` + * outright, and it is also the second limb of `expandPositionUsers`. So + * `sys_member.organization_id` is already this file's answer to "which + * organization is this person in", independent of what they may read. + * 2. `sys_user` carries no `organization_id` at all. It is a GLOBAL identity + * table, so a membership row is the only tenancy fact that exists for a + * user — there is no other read this screen could have been written with. + * + * This change grants no reads and applies no read screen to any type that + * lacks one today, so it decides nothing #7497 asks. + */ + private async lookupManager(userId: string, organizationId?: string | null): Promise { try { const rows = await this.engine.find('sys_user', { where: { id: userId }, fields: ['id', 'manager_id'], limit: 1, context: SYSTEM_CTX, } as any); const row: any = Array.isArray(rows) ? rows[0] : null; - return row?.manager_id ? String(row.manager_id) : null; + const managerId = row?.manager_id ? String(row.manager_id) : null; + if (!managerId) return null; + if (await this.managerIsProvablyOutsideOrg(managerId, organizationId)) return null; + return managerId; } catch { return null; } } + /** + * Is `managerId` PROVABLY a member of other organizations and not of + * `organizationId`? (#10153) + * + * "Provably" is the whole shape of this screen, and it is deliberate rather + * than a weaker version of "must prove membership": + * + * - membership rows exist for this user, none in the request's org + * ⇒ the tenancy fact is present and NEGATIVE ⇒ screen him out; + * - no membership rows at all, or the read failed + * ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was. + * + * The fail-open half is not timidity, it is this file's ruled posture on + * addressing paths, stated twice already: {@link filterApproversWhoCanRead} + * refuses to empty a live slate on an infrastructure hiccup, and + * {@link expandPositionUsers} carries "a step routing to nobody is worse than + * one routing to a lapsed holder". It is also load-bearing in practice — a + * stack that stamps an organization on its requests but does not materialize + * `sys_member` rows would otherwise lose every manager approver at once, + * which is a bigger behaviour change than the hole being closed. Measured: + * this repo's own `type:manager` out-of-office fixture is such a stack. + * + * Screening the MANAGER only, before OOO delegation, is deliberate too: the + * delegate arrives from `sys_approval_delegation`, whose rows already carry + * (and are already filtered by) an `organization_id` in + * {@link lookupActiveDelegation}. This card is about `sys_user.manager_id`. + */ + private async managerIsProvablyOutsideOrg( + managerId: string, + organizationId?: string | null, + ): Promise { + const requestOrg = organizationId ? String(organizationId) : ''; + // No organization on the request ⇒ nothing to screen against, and no read. + // The ordinary single-organization / embedded stack costs nothing here. + if (!requestOrg) return false; + let rows: any[] = []; + try { + // No `as any` on this options bag — #4918's ratchet grandfathers this + // file for its EXISTING erasures only, and a NEW one must carry the + // declared type. `ApprovalEngine.find` already accepts it as written. + rows = await this.engine.find('sys_member', { + where: { user_id: managerId }, + fields: ['user_id', 'organization_id'], + limit: 1000, + context: SYSTEM_CTX, + }); + } catch { return false; } // membership unreadable — see the fail-open note above + const orgs = (rows ?? []) + .map((r: any) => String(r?.organization_id ?? '')) + .filter(Boolean); + if (!orgs.length) return false; // no tenancy fact recorded for this user + if (orgs.includes(requestOrg)) return false; // he is a member here — route as before + this.logger?.warn?.( + `[approvals] #10153: manager '${managerId}' was dropped from the approver slate — ` + + `'sys_user.manager_id' points across an organization boundary. He holds membership in ` + + `${orgs.length} organization(s), none of them the request's organization '${requestOrg}', ` + + `so routing this approval to him would put approval authority over the record outside its ` + + `tenant. Fix the 'manager_id' link, grant him a membership in this organization, or route ` + + `this step with an approver type that names someone in it.`, + { managerId, requestOrganizationId: requestOrg, managerOrganizationIds: orgs }, + ); + return true; + } + /** * Out-of-office auto-skip (#1322 M1). Given an individually-routed approver * id, follow any active `sys_approval_delegation` chain and return the id the diff --git a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts new file mode 100644 index 0000000000..af93008915 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts @@ -0,0 +1,247 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +/** + * #10153 — the `manager` approver is screened to the request's organization. + * + * This file began as round 1's MEASUREMENT HARNESS, which pinned the defective + * behaviour on purpose so the premise and the tiering question were + * reproducible rather than argued. `PREMISE A` and `CLAUSE-2 (a)` asserted + * today's cross-org resolution; the fix inverts both, exactly as that file's + * header instructed. The probes that describe the SIBLING treatment (`B`, + * `B2`, `C-b`) and the `team` gap (`W`) are unchanged, and they are what makes + * the inversion readable: the same tree, the same run, one screened type next + * to the newly screened one. + * + * What it pins, both directions and both policies: + * A — a manager whose only `sys_member` row is in ANOTHER organization no + * longer resolves into this organization's slate. + * A2 — a manager who IS a member here still resolves. Without this, a + * screen that rejected everything would pass A. + * A3 — a manager with NO membership row anywhere still resolves: the + * tenancy fact is absent, so routing is left exactly as it was. + * A4 — a request with no organization at all is untouched (no screen, and + * no read to perform it). + * B/B2 — the sibling `position` expansion IS screened, and its screen is not + * reject-everything. + * W — `team` is still NOT screened. #10230 owns that; this card did not + * touch it, and the pin says so out loud. + * C-a — THE ACCEPT-TO-REJECT FLIP. Under `onEmptyApprovers: 'fail'` a node + * whose sole approver is a cross-org `manager` used to OPEN; it now + * throws `NO_APPROVERS`. That throw is PRE-EXISTING code and a bare + * `Error`, not a minted ADR-0112 envelope — there is no `code` / + * `status` to assert here, and inventing one would be a fiction. + * C-a2 — the same node under the DEFAULT policy (`admin_rescue`) still + * OPENS. This is what confines the flip to one non-default policy. + * C-b — a screened sibling in the identical shape throws too (unchanged). + */ +import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import type { ApprovalRequestRow } from '@objectstack/spec/contracts'; +import { ApprovalService, type ApprovalNodeAutoOutcome } from './approval-service.js'; + +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + function matches(row: any, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { if (!(v as any[]).some(s => matches(row, s))) return false; continue; } + if (k === '$and') { if (!(v as any[]).every(s => matches(row, s))) return false; continue; } + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; continue; + } + if (v != null && typeof v === 'object' && '$ne' in (v as any)) { + if (rv === (v as any).$ne) return false; continue; + } + if (rv !== v) return false; + } + return true; + } + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + return rows.slice(0, options?.limit ?? 1000); + }, + async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, + async update(object: string, data: any, options?: any) { + // Pinned to ObjectQL.update's OWN dispatch predicate — a double looser + // than the engine it stands in for turns a green suite into no suite. + const dispatch = assertEngineUpdateDispatch(data, options); + const t = ensure(object); + if (dispatch.kind === 'multi') { + let n = 0; + for (let i = 0; i < t.length; i++) { + if (matches(t[i], options?.where)) { t[i] = { ...t[i], ...data }; n++; } + } + return { updated: n }; + } + const i = t.findIndex(r => r.id === dispatch.id); + if (i >= 0) t[i] = { ...t[i], ...data }; + return t[i]; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const t = ensure(object); + if (dispatch.kind === 'multi') { + const survivors = t.filter(r => !matches(r, options?.where)); + const deleted = t.length - survivors.length; + t.splice(0, t.length, ...survivors); + return { deleted }; + } + const i = t.findIndex(r => r.id === dispatch.id); + if (i >= 0) t.splice(i, 1); + return { id: dispatch.id }; + }, + registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, + }; +} + +/** + * `openNodeRequest` returns `ApprovalRequestRow | ApprovalNodeAutoOutcome` — the + * second arm is the `onEmptyApprovers: 'auto_approve'` exit, which opens no + * request at all. Narrow rather than read through the union: every probe below + * asserts something about an OPENED request, so an auto-approval reaching one of + * them is a wrong answer that must say so, not a property read off the wrong arm. + * (Left unnarrowed this file billed the package's TEST_DEBT ledger 21 raw TS2339 + * — invisible to `pnpm --filter @objectstack/plugin-approvals typecheck`, whose + * tsconfig excludes `**\/*.test.ts`, and caught only by `check:type-check-debt + * --re-measure`.) + */ +function opened(result: ApprovalRequestRow | ApprovalNodeAutoOutcome): ApprovalRequestRow { + if ('autoApproved' in result) { + throw new Error('expected an OPENED approval request, got an auto-approval outcome'); + } + return result; +} + +const ORG_A = 'org_a'; +const CTX_A = { userId: 'u_sub', organizationId: ORG_A, positions: [], permissions: [] } as any; + +function input(approvers: any[], configExtra: Record = {}) { + return { + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers, behavior: 'first_response' as const, lockRecord: false, ...configExtra }, + record: { id: 'opp1', owner_id: 'u_sub', amount: 100 }, + }; +} + +describe('#10153 manager approver org screen', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(new Date('2026-01-15T10:00:00Z').getTime() + (n++) * 1000) }, + }); + // Directory: submitter in org_a; his manager is a member of org_b ONLY. + engine._tables['sys_user'] = [ + { id: 'u_sub', manager_id: 'u_mgr_b' }, + { id: 'u_mgr_b', manager_id: null }, + ]; + engine._tables['sys_member'] = [ + { id: 'm1', user_id: 'u_sub', organization_id: ORG_A, role: 'member' }, + { id: 'm2', user_id: 'u_mgr_b', organization_id: 'org_b', role: 'member' }, + ]; + // Sibling directory: the only `cfo` holder is in org_b. + engine._tables['sys_user_position'] = [ + { id: 'p1', user_id: 'u_pos_b', position: 'cfo', organization_id: 'org_b' }, + ]; + }); + + // `ApprovalRequestRow` — the PUBLISHED contract type — declares no + // `organization_id`, though `openNodeRequest` stamps one on the row it writes + // and returns. Read the stamp off the persisted row rather than through a + // property the contract does not have. (Filed separately; not this card.) + const storedOrg = () => (engine._tables['sys_approval_request'] ?? [])[0]?.organization_id; + + // `{ type: 'manager' }` and `{ type: 'manager', value: 'owner_id' }` resolve + // through the same line (`record[a.value] ?? record.owner_id`); the explicit + // spelling is used from here on only so the unresolved fallback slot reads as + // `manager:owner_id` rather than `manager:undefined`. + const MGR = { type: 'manager', value: 'owner_id' }; + + it('A — a manager whose membership is in ANOTHER organization is screened OUT', async () => { + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); + console.log('[PROBE A] request org =', storedOrg(), 'pending_approvers =', JSON.stringify(req.pending_approvers)); + // Inverted from round 1, which measured ['u_mgr_b'] here. + expect(req.pending_approvers).toEqual(['manager:owner_id']); + expect((req.pending_approvers ?? []).some((x: string) => !x.includes(':'))).toBe(false); + }); + + it('A2 — a manager who IS a member of the request org still resolves', async () => { + engine._tables['sys_member'].push({ id: 'm3', user_id: 'u_mgr_b', organization_id: ORG_A, role: 'member' }); + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); + console.log('[PROBE A2] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('A3 — a manager with NO membership row anywhere is left alone (no tenancy fact)', async () => { + engine._tables['sys_member'] = engine._tables['sys_member'].filter((m: any) => m.user_id !== 'u_mgr_b'); + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); + console.log('[PROBE A3] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('A4 — a request carrying no organization is untouched by the screen', async () => { + const ctxNoOrg = { userId: 'u_sub', positions: [], permissions: [] } as any; + const req = opened(await svc.openNodeRequest(input([MGR]), ctxNoOrg)); + console.log('[PROBE A4] request org =', storedOrg(), 'pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('PREMISE B — sibling `position` IS screened to the request org (same tree)', async () => { + const req = opened(await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A)); + console.log('[PROBE B] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['position:cfo']); + }); + + it('PREMISE B2 — same-org `position` holder DOES resolve (screen is not reject-everything)', async () => { + engine._tables['sys_user_position'].push({ id: 'p2', user_id: 'u_pos_a', position: 'cfo', organization_id: ORG_A }); + const req = opened(await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A)); + console.log('[PROBE B2] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_pos_a']); + }); + + it('C-a — THE FLIP: sole cross-org `manager` + onEmptyApprovers:fail now THROWS', async () => { + // Round 1 measured this same call OPENING (status 'pending'). This is the + // accept-to-reject flip that re-graded the card `Clause-2: yes`, pinned so + // a reviewer reads it here rather than discovering it. + let err: any = null; + try { + await svc.openNodeRequest(input([MGR], { onEmptyApprovers: 'fail' }), CTX_A); + } catch (e) { err = e; } + console.log('[PROBE C-a] threw =', err ? String(err.message).slice(0, 90) : 'NOTHING'); + expect(err).toBeTruthy(); + expect(String(err.message)).toMatch(/^NO_APPROVERS:/); + }); + + it('C-a2 — the SAME node under the DEFAULT policy still opens (the flip is confined)', async () => { + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); // onEmptyApprovers absent => admin_rescue + console.log('[PROBE C-a2] status =', req.status, 'approvers =', JSON.stringify(req.pending_approvers)); + expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual(['manager:owner_id']); + }); + + it('CLAUSE-2 (b) — a SCREENED sibling in the same shape THROWS NO_APPROVERS', async () => { + let err: any = null; + try { + await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }], { onEmptyApprovers: 'fail' }), CTX_A); + } catch (e) { err = e; } + console.log('[PROBE C-b] threw =', err ? String(err.message).slice(0, 90) : 'NOTHING'); + expect(err).toBeTruthy(); + }); + + it('W — `team` is STILL not org-screened (#10230 owns it; this card did not touch it)', async () => { + engine._tables['sys_team'] = [{ id: 'team_b', name: 'B team', organization_id: 'org_b' }]; + engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_b', user_id: 'u_team_b' }]; + const req = opened(await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A)); + console.log('[PROBE W] org_a request, org_b team -> pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_team_b']); + }); +}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 30f010c403..14bd327590 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1016,6 +1016,16 @@ "verb": "delete", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/record-reader-visibility.test.ts", "verb": "delete",