From 6fc3d1009dafcc891f6e444b24b42c60cb076863 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 13:20:51 +0000 Subject: [PATCH 1/3] =?UTF-8?q?test(plugin-approvals):=20pin=20the=20busin?= =?UTF-8?q?ess-unit=20MEMBER=20org=20screen=20(#14946)=20=E2=80=94=20red?= =?UTF-8?q?=20against=20the=20unmodified=20service?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .../business-unit-member-org-screen.test.ts | 268 ++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts diff --git a/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts new file mode 100644 index 0000000000..97d3a4b9ea --- /dev/null +++ b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +/** + * #14946 — the expanded BUSINESS-UNIT MEMBERS are screened to the directory + * organization, with a STRICT equality. + * + * `expandBusinessUnitUsers` screens the `sys_business_unit` rows with + * `businessUnitOrgScope` — null-inclusive since #3807, because a seeded unit + * carries `organization_id = null` by construction and is admitted on purpose. + * The `sys_business_unit_member` read that follows carried NO organization + * predicate at all, under `SYSTEM_CTX`, which carries no tenant either. A + * seeded unit id exists identically in every tenant, so tenant A's request + * resolved that unit and then collected EVERY tenant's membership rows off it. + * + * Why the member screen is strict where the unit screen is not — measured on + * this tree, not inherited from the sibling card: + * + * - `sys_business_unit_member` declares no `organization_id` + * (`packages/platform-objects/src/identity/sys-business-unit-member.object.ts`); + * the column is INJECTED (`applySystemFields`, `injected-system-columns.ts`) + * and the tenancy census lists the object `reach: "in"` with + * `tenantField: "organization_id"`; + * - REST / session writes fill it (`SqlDriver.injectTenantOnInsert`); seed + * replay does not (`seed-loader.ts` withholds its `fallbackOrgId` from + * every `sys_` object); elevated system-context writes do not either + * (`unclassified` in `PLATFORM_OBJECT_TENANCY`, tracked as #14570). + * + * ⇒ a NULL there means UNKNOWN tenancy, not "platform-global", and routing + * approval authority over tenant A's record to an identity of unknown + * tenancy is the same cross-tenant hole by the other door. The screen fails + * CLOSED, exactly as `plugin-sharing`'s `memberScope` does for the same rows. + * + * Every anchor unit below is SEEDED (`organization_id: null`) unless a case + * says otherwise. That is load-bearing: on an org-stamped unit the assertions + * would hold even if the member screen were deleted, because the unit screen + * would answer first. Anchoring on a seeded unit is what makes each case a pin + * on the MEMBER screen. + * + * B1 — THE LEAK: a seeded unit with two tenants' membership rows resolves + * ONLY the request organization's users, at both depths of the walk. + * B2 — THE CONTROL: an org-stamped unit still routes its own members — + * strict, not broken — and still drops the other tenant's row. + * B3 — THE DECLARED COST: org-less membership rows (seed replay, elevated + * writes) do NOT route when the request carries an organization; the + * slot falls to the literal and the #3807 warning fires, so the empty + * slate is loud rather than silent. + * B4 — a request carrying no organization is untouched: every member + * routes and the read carries no organization predicate. + * B5 — THE SHAPE: the member read carries a strict `organization_id` + * equality and no `$or` null arm, in ONE read for the whole subtree. + * B6 — the `expression` / `resolveAs: 'department'` call site is closed too. + */ +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; + } + // A row that simply omits the column is `undefined`, which a strict + // `organization_id: 'org_a'` equality must NOT match — the same answer + // a SQL `organization_id = ?` gives an unstamped row. + if (rv !== v) return false; + } + return true; + } + return { + _tables: tables, + /** Every `find` anyone made, with its options — pins the predicate SHAPE (B5). */ + _finds: [] as Array<{ object: string; options: any }>, + async find(object: string, options?: any) { + this._finds.push({ object, options }); + 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() {}, + }; +} + +/** See the identical note in `team-approver-org-screen.test.ts` (#10230). */ +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 ORG_B = 'org_b'; +const CTX_A = { userId: 'u_sub', organizationId: ORG_A, positions: [], permissions: [] } as any; +/** No organization anywhere on the request — the single-org / embedded stack. */ +const CTX_NO_ORG = { userId: 'u_sub', positions: [], permissions: [] } as any; +const DEPT_SEEDED = { type: 'department', value: 'bu_seeded' }; + +/** + * The org chart the card describes: a SEEDED unit tree (`organization_id: + * null`, which `businessUnitOrgScope` admits by design) — the same unit ids + * in every tenant. + */ +const SEEDED_TREE = [ + { id: 'bu_seeded', organization_id: null, active: true }, + { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true }, +]; + +/** + * Two tenants' membership rows on that shared tree, at both depths — the + * shape the REST/session write path produces on a real multi-tenant + * deployment, since it stamps `organization_id` from the caller's tenant. + */ +const TWO_TENANT_MEMBERS = [ + { id: 'bm_a', business_unit_id: 'bu_seeded', user_id: 'u_a', organization_id: ORG_A }, + { id: 'bm_b', business_unit_id: 'bu_seeded', user_id: 'u_b', organization_id: ORG_B }, + { id: 'bm_a_child', business_unit_id: 'bu_seeded_child', user_id: 'u_a_child', organization_id: ORG_A }, + { id: 'bm_b_child', business_unit_id: 'bu_seeded_child', user_id: 'u_b_child', organization_id: ORG_B }, +]; + +function input(approvers: any[], configExtra: Record = {}, extra: 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 }, + ...extra, + }; +} + +describe('#14946 business-unit MEMBER org screen', () => { + let engine: ReturnType; + let svc: ApprovalService; + let warnings: Array<[any, any]>; + let n = 0; + + beforeEach(() => { + engine = makeFakeEngine(); + warnings = []; + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(new Date('2026-01-15T10:00:00Z').getTime() + (n++) * 1000) }, + logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) } as any, + }); + engine._tables['sys_business_unit'] = SEEDED_TREE.map(r => ({ ...r })); + engine._tables['sys_business_unit_member'] = TWO_TENANT_MEMBERS.map(r => ({ ...r })); + }); + + const memberReads = () => engine._finds.filter(f => f.object === 'sys_business_unit_member'); + + it('B1 — THE LEAK: a seeded unit resolves ONLY the request organization\'s members, at both depths', async () => { + const req = opened(await svc.openNodeRequest(input([DEPT_SEEDED]), CTX_A)); + console.log('[PROBE B1] org_a request, seeded unit, org_a+org_b members -> pending_approvers =', + JSON.stringify(req.pending_approvers)); + expect([...(req.pending_approvers ?? [])].sort()).toEqual(['u_a', 'u_a_child']); + expect(req.pending_approvers).not.toContain('u_b'); + expect(req.pending_approvers).not.toContain('u_b_child'); + }); + + it('B2 — THE CONTROL: an org-stamped unit still routes its own members, and still drops the other tenant\'s', async () => { + // The unit screen admits this unit outright, so anything dropped here is + // the MEMBER screen's doing — and anything kept proves it is strict, not + // "refuses everything". + engine._tables['sys_business_unit'] = [{ id: 'bu_mine', organization_id: ORG_A, active: true }]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u_a', organization_id: ORG_A }, + { id: 'bm2', business_unit_id: 'bu_mine', user_id: 'u_b', organization_id: ORG_B }, + ]; + const req = opened(await svc.openNodeRequest(input([{ type: 'department', value: 'bu_mine' }]), CTX_A)); + console.log('[PROBE B2] org_a request, org_a unit, org_a+org_b members -> pending_approvers =', + JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_a']); + }); + + it('B3 — THE DECLARED COST: org-less membership rows do NOT route when the request carries an organization, and the empty slate is LOUD', async () => { + // Seed replay and elevated system writes both leave `organization_id` + // NULL (#14570). Unknown tenancy is not "this organization": the slot + // falls to the literal, which #3807's warning already reports. + engine._tables['sys_business_unit_member'] = [ + { id: 'bm_seeded', business_unit_id: 'bu_seeded', user_id: 'u_seeded', organization_id: null }, + { id: 'bm_unstamped', business_unit_id: 'bu_seeded_child', user_id: 'u_unstamped' }, + ]; + const req = opened(await svc.openNodeRequest(input([DEPT_SEEDED]), CTX_A)); + console.log('[PROBE B3] org_a request, seeded unit, org-less members -> pending_approvers =', + JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['department:bu_seeded']); + const loud = warnings.find(([msg]) => String(msg).includes("approver 'department:bu_seeded' expanded to nobody")); + expect(loud).toBeDefined(); + expect(loud?.[1]).toMatchObject({ type: 'department', value: 'bu_seeded', organizationId: ORG_A }); + }); + + it('B4 — a request carrying no organization is untouched: every member routes, no organization predicate', async () => { + const req = opened(await svc.openNodeRequest(input([DEPT_SEEDED]), CTX_NO_ORG)); + console.log('[PROBE B4] org-less request -> pending_approvers =', JSON.stringify(req.pending_approvers)); + expect([...(req.pending_approvers ?? [])].sort()).toEqual(['u_a', 'u_a_child', 'u_b', 'u_b_child']); + const reads = memberReads(); + expect(reads.length).toBe(1); + const where = reads[0].options?.where ?? reads[0].options?.filter ?? {}; + expect(where).not.toHaveProperty('organization_id'); + expect(where).not.toHaveProperty('$or'); + }); + + it('B5 — THE SHAPE: one member read for the whole subtree, carrying a strict equality and no null arm', async () => { + await svc.openNodeRequest(input([DEPT_SEEDED]), CTX_A); + const reads = memberReads(); + expect(reads.length).toBe(1); + const where = reads[0].options?.where ?? reads[0].options?.filter ?? {}; + console.log('[PROBE B5] sys_business_unit_member where =', JSON.stringify(where)); + expect(where.organization_id).toBe(ORG_A); + // ⛔ Not `businessUnitOrgScope`'s `$or: [{organization_id}, {organization_id: null}]`. + // A null arm here would re-admit every org-less row — the B3 population — + // and re-open the hole for the elevated-write case. + expect(where).not.toHaveProperty('$or'); + expect([...(where.business_unit_id?.$in ?? [])].sort()).toEqual(['bu_seeded', 'bu_seeded_child']); + }); + + it('B6 — the `expression` / `resolveAs: \'department\'` call site is closed too', async () => { + const req = opened(await svc.openNodeRequest(input( + [{ type: 'expression', value: 'vars.picked', resolveAs: 'department' }], + { behavior: 'unanimous' }, + { variables: { picked: ['bu_seeded'] } }, + ), CTX_A)); + console.log('[PROBE B6] expression/resolveAs department -> pending_approvers =', + JSON.stringify(req.pending_approvers)); + expect([...(req.pending_approvers ?? [])].sort()).toEqual(['u_a', 'u_a_child']); + expect(req.pending_approvers).not.toContain('u_b'); + expect(req.pending_approvers).not.toContain('u_b_child'); + }); +}); From fe370ce48be031b43fcc5a16a4d35268963a9878 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 13:30:19 +0000 Subject: [PATCH 2/3] fix(plugin-approvals): screen expanded business-unit members to the directory organization (#14946) `expandBusinessUnitUsers` screened the unit rows with the null-inclusive `businessUnitOrgScope` (#3807) but read `sys_business_unit_member` with no organization predicate, under SYSTEM_CTX which carries no tenant. A seeded unit id exists in every tenant, so tenant A's department approver resolved tenant B's members. The member read now carries a strict organization_id equality (`businessUnitMemberScope`): the column is injected and only the session write path fills it, so NULL means unknown tenancy, not global. Existing fixtures that pinned org-less membership rows on stamped or seeded units are re-anchored onto stamped rows; the org-less case is pinned on its own (B3) as the declared, loud cost. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- .changeset/approvals-bu-member-org-screen.md | 9 +++ .../src/approval-service.test.ts | 24 +++++--- .../plugin-approvals/src/approval-service.ts | 61 ++++++++++++++++++- 3 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 .changeset/approvals-bu-member-org-screen.md diff --git a/.changeset/approvals-bu-member-org-screen.md b/.changeset/approvals-bu-member-org-screen.md new file mode 100644 index 0000000000..61d6e389c1 --- /dev/null +++ b/.changeset/approvals-bu-member-org-screen.md @@ -0,0 +1,9 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +Fix: a `department` approver on a seeded business unit no longer routes the approval to another organization's members. + +`ApprovalService.expandBusinessUnitUsers` screened the `sys_business_unit` rows with the null-inclusive tenant predicate (#3807 — a seeded unit carries no organization and is admitted on purpose) but read `sys_business_unit_member` with no organization predicate at all, under a system context that carries no tenant either. A seeded unit id exists identically in every tenant, so a `department:` approver on tenant A's request resolved the shared unit and then collected every tenant's membership rows hanging off it — approval authority over A's record, routed to B's users. The member read now carries a strict `organization_id` equality against the directory organization the approver resolves in: the same screen `plugin-sharing` applies to these rows, and the same posture this package already takes for `sys_team_member` and `sys_user_position`. + +The screen is strict rather than null-inclusive on purpose. `sys_business_unit_member.organization_id` is filled by REST/session writes but left NULL by seed replay and by elevated system-context writes (tracked in #14570), so a NULL on a membership row means unknown tenancy, not "platform-global", and routing fails closed on it. Declared cost: on a deployment whose membership rows (not merely its units) were seeded or system-written, a `department` approver on a request that carries an organization now expands to nobody — the slot falls to the `department:` literal, the existing `expanded to nobody` warning (#3807) names it, and `onEmptyApprovers` governs the request as for any unstaffed target. The repair is to stamp those membership rows. A request that carries no organization is unchanged, and so is every unit-level screen. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index ebc706d637..8ddb4febba 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -429,10 +429,12 @@ describe('ApprovalService (node era)', () => { { id: 'd1', active: true, organization_id: 't1' }, { id: 'd2', active: true, organization_id: 't1' }, ]; + // #14946: membership rows are stamped (the session write path fills + // `organization_id`); the member read is now strictly screened to it. engine._tables['sys_business_unit_member'] = [ - { id: 'm1', business_unit_id: 'd1', user_id: 'u2' }, - { id: 'm2', business_unit_id: 'd1', user_id: 'u3' }, - { id: 'm3', business_unit_id: 'd2', user_id: 'u4' }, + { id: 'm1', business_unit_id: 'd1', user_id: 'u2', organization_id: 't1' }, + { id: 'm2', business_unit_id: 'd1', user_id: 'u3', organization_id: 't1' }, + { id: 'm3', business_unit_id: 'd2', user_id: 'u4', organization_id: 't1' }, ]; const req = await svc.openNodeRequest( exprInput('vars.picked_departments', { @@ -913,8 +915,8 @@ describe('ApprovalService (node era)', () => { { id: 'bu2', parent_business_unit_id: 'bu1', organization_id: 't1', active: true }, ]; engine._tables['sys_business_unit_member'] = [ - { id: 'bm1', business_unit_id: 'bu1', user_id: 'u5' }, - { id: 'bm2', business_unit_id: 'bu2', user_id: 'u6' }, + { id: 'bm1', business_unit_id: 'bu1', user_id: 'u5', organization_id: 't1' }, + { id: 'bm2', business_unit_id: 'bu2', user_id: 'u6', organization_id: 't1' }, ]; const req = await svc.openNodeRequest(positionInput({ config: { @@ -944,9 +946,13 @@ describe('ApprovalService (node era)', () => { { id: 'bu_seeded', organization_id: null, active: true }, { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true }, ]; + // #14946: the SEEDED rows are the units; the membership rows are stamped, + // as the session write path leaves them. An org-less membership row on a + // seeded unit is pinned on its own in + // `business-unit-member-org-screen.test.ts` (B3) — it does NOT route. engine._tables['sys_business_unit_member'] = [ - { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5' }, - { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6' }, + { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5', organization_id: 't1' }, + { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6', organization_id: 't1' }, ]; const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); // Both the seed check AND the subtree descent must see the null-org rows. @@ -973,8 +979,8 @@ describe('ApprovalService (node era)', () => { { id: 'bu_theirs', parent_business_unit_id: 'bu_seeded', organization_id: 't2', active: true }, ]; engine._tables['sys_business_unit_member'] = [ - { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u5' }, - { id: 'bm2', business_unit_id: 'bu_theirs', user_id: 'intruder' }, + { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u5', organization_id: 't1' }, + { id: 'bm2', business_unit_id: 'bu_theirs', user_id: 'intruder', organization_id: 't2' }, ]; const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX); expect(req.pending_approvers).toEqual(['u5']); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index aed7ecacea..73fe256215 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1734,7 +1734,60 @@ export class ApprovalService implements IApprovalService { return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] }; } - /** Recursive department — walks `sys_business_unit.parent_business_unit_id`. */ + /** + * Tenant scope for the `sys_business_unit_member` read — a STRICT equality, + * deliberately NOT {@link businessUnitOrgScope} (#14946). + * + * The two screens answer different questions. The UNIT is the anchor the + * approver NAMES, and a seeded unit carries `organization_id = null` by + * construction (a seed cannot know the id the runtime mints at boot), so + * #3807 admits the null there on purpose. The MEMBER rows are the SET BEING + * ROUTED TO — enumerated by the platform, never named by anyone — and a + * seeded unit id exists identically in every tenant. Before this screen the + * member read carried no organization predicate at all, under + * {@link SYSTEM_CTX} which carries no tenant either, so tenant A's request + * resolved the shared unit and then collected EVERY tenant's membership rows + * hanging off it: approval authority over A's record, routed to B's users. + * + * Why the null arm is NOT copied here — measured on this tree: + * - `sys_business_unit_member` declares no `organization_id`; the column + * is injected (`applySystemFields`) and the tenancy census lists it in; + * - REST / session writes fill it (`SqlDriver.injectTenantOnInsert`); + * - seed replay does NOT (`seed-loader.ts` withholds its `fallbackOrgId` + * from every `sys_` object), and elevated system-context writes do NOT + * (`unclassified` in `PLATFORM_OBJECT_TENANCY`, tracked as #14570). + * So a NULL on a member row means UNKNOWN tenancy, not "platform-global", + * and unknown tenancy is not a member of this organization. This is the + * ruling `plugin-sharing`'s `memberScope` already applies to the same rows + * (#14547 / #14949), and the posture this file already takes for + * `sys_team_member` and `sys_user_position`. + * + * The cost is declared, not hidden: an organization whose MEMBERSHIP rows + * were seeded or system-written expands to nobody even on a unit it can + * see. That is not silent — the graph-type fallback in `expandApprover` + * warns `expanded to nobody` (#3807) and `onEmptyApprovers` governs the + * request as for any unstaffed target — and the repair is to stamp the + * membership rows, never to widen this screen. ⛔ Do not "unify" the two + * screens: one method serving both re-opens whichever half it does not + * implement. + */ + private businessUnitMemberScope( + filter: Record, + organizationId?: string | null, + ): Record { + if (!organizationId) return filter; + return { ...filter, organization_id: organizationId }; + } + + /** + * Recursive department — walks `sys_business_unit.parent_business_unit_id`. + * + * Two tenant screens, and they are different on purpose: the UNIT rows + * (seed check and descent) go through the null-inclusive + * {@link businessUnitOrgScope}; the MEMBER read goes through the strict + * {@link businessUnitMemberScope}. `organizationId` is the DIRECTORY + * organization the approver resolves in (ADR-0105 D9), for both. + */ private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise { if (!businessUnitId) return []; // Seed sanity check: skip if dept doesn't exist or is inactive within tenant. @@ -1769,7 +1822,11 @@ export class ApprovalService implements IApprovalService { let rows: any[] = []; try { rows = await this.engine.find('sys_business_unit_member', { - where: { business_unit_id: { $in: Array.from(seen) } }, + // #14946: tenant-screened — {@link businessUnitMemberScope} is STRICT + // on purpose and is not {@link businessUnitOrgScope}. The units above + // proved their tenancy (or are seeded); these rows have not, and the + // shared seeded unit id is exactly where other tenants' rows sit. + where: this.businessUnitMemberScope({ business_unit_id: { $in: Array.from(seen) } }, organizationId), fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, From 16dbdf55bc418b2a17bfe6266e290acc16d7a252 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 13:45:24 +0000 Subject: [PATCH 3/3] test(plugin-approvals): judge the #14946 fake engine's find bound by presence; re-anchor the system-context page; ledger the new pin file - the fake engine's `find` no longer reads `this` (the objectql-double-limit probe calls it unbound) and applies the caller's bound after the filter, by presence, instead of a default page of 1000 - content/docs/permissions/system-context.mdx: six approval-service.ts line anchors re-aimed by +57, the net insertion of the businessUnitMemberScope docblock above them (check-system-context-census --fix) - engine-double-contract ledger learns business-unit-member-org-screen.test.ts Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ --- content/docs/permissions/system-context.mdx | 2 +- .../src/business-unit-member-org-screen.test.ts | 11 +++++++---- scripts/engine-double-contract.pinned.json | 10 ++++++++++ 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 1c5ffe7baf..6ab73fea81 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -145,7 +145,7 @@ The largest single consumer — **17 of the 106 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3248`, `:3396`, `:3564`, `:3635`, `:3824`, `:3864` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3305`, `:3453`, `:3621`, `:3692`, `:3881`, `:3921` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` | diff --git a/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts index 97d3a4b9ea..b0b3a1e994 100644 --- a/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts +++ b/packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts @@ -76,14 +76,17 @@ function makeFakeEngine() { } return true; } + /** Every `find` anyone made, with its options — pins the predicate SHAPE (B5). */ + const finds: Array<{ object: string; options: any }> = []; return { _tables: tables, - /** Every `find` anyone made, with its options — pins the predicate SHAPE (B5). */ - _finds: [] as Array<{ object: string; options: any }>, + _finds: finds, async find(object: string, options?: any) { - this._finds.push({ object, options }); + finds.push({ object, options }); const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); - return rows.slice(0, options?.limit ?? 1000); + // The caller's bound, applied AFTER the filter and by PRESENCE — the + // shape `check:objectql-double-limit` holds every ObjectQL double to. + return typeof options?.limit === 'number' ? rows.slice(0, options.limit) : rows; }, async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, async update(object: string, data: any, options?: any) { diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index b3739ef73c..ac2327e9b6 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2131,6 +2131,16 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/business-unit-member-org-screen.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", "verb": "delete",