Skip to content

Commit 13f533a

Browse files
os-warrenclaude
andauthored
fix(approvals): screen the manager approver to the request's organization (#10153) (#10334)
* test(approvals): measurement harness for the #10153 manager org screen (no fix) Pins the CURRENT behaviour so the premise and the tiering question are reproducible: the manager branch resolves across the organization boundary, the sibling position expansion is screened (and is not reject-everything), team is not screened either, and a sole cross-org manager approver under onEmptyApprovers: 'fail' opens today while a screened type in the identical shape throws NO_APPROVERS. No fix is implemented. The card tripped its re-tiering wire (Clause-2) and is handed back for dispatch at the required tier. Part of #10153 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(approvals): pin the #10153 harness fake engine to ObjectQL's write dispatch check:engine-double-contract flagged the harness double's delete()/update() as looser than the engine they stand in for. Route both through assertEngineDeleteDispatch / assertEngineUpdateDispatch and record the new pinned coverage in the retained ledger, as the gate's own remedy prescribes. Part of #10153 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * fix(approvals): screen the `manager` approver to the request's organization (#10153) `lookupManager` read `sys_user.manager_id` with no organization argument while every other graph-shaped approver expansion is handed the directory org. Since `sys_user` carries no `organization_id`, a `manager_id` crossing an organization boundary routed the approval to an out-of-tenant approver. The screen is a `sys_member` membership test, applied only when the fact is present and negative: a manager with membership rows, none in the request's organization, is dropped; absent membership rows, a failed read, or a request with no organization leave routing exactly as it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * fix(approvals): type the membership read and re-point the double ledger at the renamed pin file The #4918 ratchet grandfathers `approval-service.ts` for its EXISTING query-options erasures only, so the new `sys_member` read carries no `as any`. The engine-double ledger follows the harness file's rename — same two pinned doubles, no coverage lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(approvals): narrow the openNodeRequest union so the pins type-check in the hidden layer `plugin-approvals` excludes `**/*.test.ts` from its tsconfig, so `pnpm --filter @objectstack/plugin-approvals typecheck` never read this file and reported exit 0 over it. `check:type-check-debt --re-measure` did: the pins billed TEST_DEBT 21 raw TS2339/TS18048, all from reading `pending_approvers` straight off `ApprovalRequestRow | ApprovalNodeAutoOutcome`. Narrowed through an `opened()` helper that REFUSES the auto-approval arm rather than casting past it — every probe asserts something about an opened request, so an auto-approval reaching one is a wrong answer that must say so. Re-measured: the package is back to its frozen 348, contributing 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent adbcbfd commit 13f533a

4 files changed

Lines changed: 426 additions & 3 deletions

File tree

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
---
2+
"@objectstack/plugin-approvals": minor
3+
---
4+
5+
fix(approvals): screen the `manager` approver to the request's organization (#10153)
6+
7+
`expandApprovers` hands the directory organization to every graph-shaped
8+
approver expansion — `department`, `position`, `org_membership_level`. The
9+
`manager` branch did not: `lookupManager` read `sys_user.manager_id` under a
10+
system context and took no organization argument at all. `sys_user` is a global
11+
identity table with no `organization_id`, so nothing else on that path supplied
12+
the tenancy fact either. A `manager_id` crossing an organization boundary
13+
therefore routed the submission to an approver **in another organization** — an
14+
out-of-tenant person granted approval authority over the record.
15+
16+
The same column has been screened on the hierarchy side since cloud#1195. This
17+
brings the approvals consumer into line for the `manager` branch.
18+
19+
## What the screen is
20+
21+
`lookupManager(userId, organizationId)` now resolves the manager and then asks
22+
whether he is **provably outside** the request's organization:
23+
24+
| membership rows for the manager | result |
25+
|---|---|
26+
| some exist, none in the request's org | **screened out** — the slot falls through to the `manager:<value>` literal |
27+
| one is in the request's org | resolves, unchanged |
28+
| none exist at all | resolves, unchanged — the tenancy fact is absent, not negative |
29+
| the `sys_member` read failed | resolves, unchanged |
30+
| the request carries no organization | resolves, unchanged — and no read is performed |
31+
32+
The fail-open half is this file's ruled posture on addressing paths, stated
33+
twice already: `filterApproversWhoCanRead` refuses to empty a live slate on an
34+
infrastructure hiccup, and `expandPositionUsers` carries "a step routing to
35+
nobody is worse than one routing to a lapsed holder". A drop is logged with the
36+
manager's id, his organizations and the request's, so the fix ("repair the link"
37+
/ "grant the membership" / "retarget the step") is legible without a debugger.
38+
39+
## ⚠️ This moves one input from accepted to refused
40+
41+
A node whose **sole** approver is a cross-org `manager` and which is authored
42+
with the **non-default** `onEmptyApprovers: 'fail'` used to open successfully;
43+
it now throws `NO_APPROVERS`. Nothing new is thrown — a screened-out manager
44+
leaves only a `type:value` literal, which the pre-existing empty-slate test
45+
already classifies as empty, and `'fail'` already throws on empty. Every
46+
screened sibling has reached that same bucket since it was written.
47+
48+
**The default policy is unaffected**: `admin_rescue` still opens the request
49+
(decidable by a privileged admin) and warns, and `auto_approve` still
50+
auto-approves. Both directions and both policies are pinned in
51+
`manager-approver-org-screen.test.ts`.
52+
53+
## What this does NOT decide
54+
55+
- **#7497** (does approver routing imply record read visibility?) stays open.
56+
The screen reads `sys_member`, which looks like the D2 read filter beside it,
57+
and the code says at length why it is the *sibling* treatment instead: two of
58+
the three org-scoped expansions already screen on `sys_member.organization_id`,
59+
and `sys_user` offers no other tenancy fact. No reads are granted and no read
60+
screen is applied to any type that lacked one.
61+
- **`team`** is still unscreened — it is a sibling graph expansion that is not
62+
org-scoped either, tracked as #10230, and it touches this same file.
63+
- `APPROVER_ORG_SCOPED` is untouched. It answers ADR-0105 D9 *retargetability*
64+
(may an author write `organization:` on this type?), not screening, and
65+
`manager: false` remains correct.

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

Lines changed: 104 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -985,7 +985,12 @@ export class ApprovalService implements IApprovalService {
985985
} else if (type === 'manager' && record) {
986986
const subject = (record as any)[a.value] ?? (record as any).owner_id;
987987
if (subject) {
988-
const mgr = await this.lookupManager(String(subject));
988+
// #10153: the request's OWN organization, not `directoryOrg`. They are
989+
// provably equal on this branch (`manager` is not org-scoped, so a
990+
// `organization` declaration is refused above), and naming the request
991+
// org says what the screen asserts: tenancy of the request, never an
992+
// ADR-0105 D9 retarget this type does not have.
993+
const mgr = await this.lookupManager(String(subject), organizationId);
989994
if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions);
990995
}
991996
}
@@ -1339,16 +1344,112 @@ export class ApprovalService implements IApprovalService {
13391344
return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)));
13401345
}
13411346

1342-
private async lookupManager(userId: string): Promise<string | null> {
1347+
/**
1348+
* `sys_user.manager_id`, screened to the request's organization (#10153).
1349+
*
1350+
* Takes an organization argument for the same reason its siblings do
1351+
* ({@link expandPositionUsers}, {@link expandMembershipTierUsers}): an
1352+
* approver expansion answers "who, in THIS organization". Before #10153 this
1353+
* one did not ask, and it was the only expansion that did not — a
1354+
* `manager_id` pointing at a person in another organization routed that
1355+
* person an approval over a record they are not a tenant of.
1356+
*
1357+
* ⚠️ The screen reads `sys_member`, which LOOKS like the D2 read-visibility
1358+
* filter next to it ({@link filterApproversWhoCanRead}). It is not, and this
1359+
* comment exists so the next reader does not conclude that #7497 (does
1360+
* approver routing imply record read visibility?) was settled here. It was
1361+
* not. Two facts make this the SIBLING treatment rather than a
1362+
* read-visibility ruling:
1363+
*
1364+
* 1. Two of the three org-scoped expansions already screen on exactly this
1365+
* column — `expandMembershipTierUsers` filters `sys_member.organization_id`
1366+
* outright, and it is also the second limb of `expandPositionUsers`. So
1367+
* `sys_member.organization_id` is already this file's answer to "which
1368+
* organization is this person in", independent of what they may read.
1369+
* 2. `sys_user` carries no `organization_id` at all. It is a GLOBAL identity
1370+
* table, so a membership row is the only tenancy fact that exists for a
1371+
* user — there is no other read this screen could have been written with.
1372+
*
1373+
* This change grants no reads and applies no read screen to any type that
1374+
* lacks one today, so it decides nothing #7497 asks.
1375+
*/
1376+
private async lookupManager(userId: string, organizationId?: string | null): Promise<string | null> {
13431377
try {
13441378
const rows = await this.engine.find('sys_user', {
13451379
where: { id: userId }, fields: ['id', 'manager_id'], limit: 1, context: SYSTEM_CTX,
13461380
} as any);
13471381
const row: any = Array.isArray(rows) ? rows[0] : null;
1348-
return row?.manager_id ? String(row.manager_id) : null;
1382+
const managerId = row?.manager_id ? String(row.manager_id) : null;
1383+
if (!managerId) return null;
1384+
if (await this.managerIsProvablyOutsideOrg(managerId, organizationId)) return null;
1385+
return managerId;
13491386
} catch { return null; }
13501387
}
13511388

1389+
/**
1390+
* Is `managerId` PROVABLY a member of other organizations and not of
1391+
* `organizationId`? (#10153)
1392+
*
1393+
* "Provably" is the whole shape of this screen, and it is deliberate rather
1394+
* than a weaker version of "must prove membership":
1395+
*
1396+
* - membership rows exist for this user, none in the request's org
1397+
* ⇒ the tenancy fact is present and NEGATIVE ⇒ screen him out;
1398+
* - no membership rows at all, or the read failed
1399+
* ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was.
1400+
*
1401+
* The fail-open half is not timidity, it is this file's ruled posture on
1402+
* addressing paths, stated twice already: {@link filterApproversWhoCanRead}
1403+
* refuses to empty a live slate on an infrastructure hiccup, and
1404+
* {@link expandPositionUsers} carries "a step routing to nobody is worse than
1405+
* one routing to a lapsed holder". It is also load-bearing in practice — a
1406+
* stack that stamps an organization on its requests but does not materialize
1407+
* `sys_member` rows would otherwise lose every manager approver at once,
1408+
* which is a bigger behaviour change than the hole being closed. Measured:
1409+
* this repo's own `type:manager` out-of-office fixture is such a stack.
1410+
*
1411+
* Screening the MANAGER only, before OOO delegation, is deliberate too: the
1412+
* delegate arrives from `sys_approval_delegation`, whose rows already carry
1413+
* (and are already filtered by) an `organization_id` in
1414+
* {@link lookupActiveDelegation}. This card is about `sys_user.manager_id`.
1415+
*/
1416+
private async managerIsProvablyOutsideOrg(
1417+
managerId: string,
1418+
organizationId?: string | null,
1419+
): Promise<boolean> {
1420+
const requestOrg = organizationId ? String(organizationId) : '';
1421+
// No organization on the request ⇒ nothing to screen against, and no read.
1422+
// The ordinary single-organization / embedded stack costs nothing here.
1423+
if (!requestOrg) return false;
1424+
let rows: any[] = [];
1425+
try {
1426+
// No `as any` on this options bag — #4918's ratchet grandfathers this
1427+
// file for its EXISTING erasures only, and a NEW one must carry the
1428+
// declared type. `ApprovalEngine.find` already accepts it as written.
1429+
rows = await this.engine.find('sys_member', {
1430+
where: { user_id: managerId },
1431+
fields: ['user_id', 'organization_id'],
1432+
limit: 1000,
1433+
context: SYSTEM_CTX,
1434+
});
1435+
} catch { return false; } // membership unreadable — see the fail-open note above
1436+
const orgs = (rows ?? [])
1437+
.map((r: any) => String(r?.organization_id ?? ''))
1438+
.filter(Boolean);
1439+
if (!orgs.length) return false; // no tenancy fact recorded for this user
1440+
if (orgs.includes(requestOrg)) return false; // he is a member here — route as before
1441+
this.logger?.warn?.(
1442+
`[approvals] #10153: manager '${managerId}' was dropped from the approver slate — `
1443+
+ `'sys_user.manager_id' points across an organization boundary. He holds membership in `
1444+
+ `${orgs.length} organization(s), none of them the request's organization '${requestOrg}', `
1445+
+ `so routing this approval to him would put approval authority over the record outside its `
1446+
+ `tenant. Fix the 'manager_id' link, grant him a membership in this organization, or route `
1447+
+ `this step with an approver type that names someone in it.`,
1448+
{ managerId, requestOrganizationId: requestOrg, managerOrganizationIds: orgs },
1449+
);
1450+
return true;
1451+
}
1452+
13521453
/**
13531454
* Out-of-office auto-skip (#1322 M1). Given an individually-routed approver
13541455
* id, follow any active `sys_approval_delegation` chain and return the id the

0 commit comments

Comments
 (0)