From 51b3d2050fbd10694822cc6078a3d773c504ddbf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 15:08:33 +0000 Subject: [PATCH] fix(plugin-sharing): business_unit recipient expands exactly one unit, not the subtree (#7807) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two business-unit recipient kinds were declared as two widths and enforced as one. `SharingRuleService.expandRecipient` routed both through the identical `BusinessUnitGraphService.expandUsers` call, whose first act is a BFS over `parent_business_unit_id`, so the branches differed only in their comments. A rule authored `recipient_type: 'business_unit'` — declared by the spec, the lint red-line table and ADR-0057 D5 as "exactly one business unit's members (no subtree)" — reached that unit plus every descendant unit's members. On a three-level tree a division-anchored rule silently granted to every department and office beneath it, and `unit_and_subordinates`, documented as the strictly wider grant of the pair, was not wider at all. Maintainer ruling 2026-08-12, direction 1: narrow the runtime to the declaration. `business_unit` now resolves through a new `expandUnitMembers()` — no descent — while `unit_and_subordinates` keeps the subtree walk unchanged. Both kinds survive; neither is merged or retired. `expandUsers()` keeps its contract meaning for the `bu:` approver prefix and org rollups. The narrow width caches separately, since one map keyed by unit id would let whichever width ran first answer for the other. Both widths are pinned as a pair on one three-level fixture, at the graph, the service and the REST level, because a change narrowing BOTH kinds would satisfy the headline while destroying the distinction. Verified by two ablations: with the narrowing reverted exactly the 8 narrow assertions go red and every wide one stays green; with both branches narrowed exactly the wide assertions go red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .../business-unit-recipient-exact-width.md | 67 +++++ .../src/bu-tree-recompute.test.ts | 9 +- .../plugin-sharing/src/bu-tree-recompute.ts | 21 +- .../src/business-unit-graph.test.ts | 77 +++++ .../plugin-sharing/src/business-unit-graph.ts | 106 ++++++- .../src/recipient-width.test.ts | 265 ++++++++++++++++++ .../src/sharing-rule-service.ts | 17 +- .../plugin-sharing/src/sharing-rule.test.ts | 23 +- .../dogfood/test/authz-conformance.matrix.ts | 3 +- ...wcase-bu-hierarchy-sharing.dogfood.test.ts | 87 +++++- 10 files changed, 638 insertions(+), 37 deletions(-) create mode 100644 .changeset/business-unit-recipient-exact-width.md create mode 100644 packages/plugins/plugin-sharing/src/recipient-width.test.ts diff --git a/.changeset/business-unit-recipient-exact-width.md b/.changeset/business-unit-recipient-exact-width.md new file mode 100644 index 0000000000..64b4235c84 --- /dev/null +++ b/.changeset/business-unit-recipient-exact-width.md @@ -0,0 +1,67 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): the `business_unit` sharing-rule recipient expands exactly one unit, not its whole subtree (#7807) + +⚠️ **This is an intentional over-grant fix, and it REDUCES visible rows for any +deployment that authored `business_unit` sharing rules.** Read the migration note +below before upgrading if you use that recipient kind. + +## What was wrong + +The two business-unit recipient kinds were **declared** as two different widths +and **enforced** as the same width. `SharingRuleService.expandRecipient` routed +both through the identical `BusinessUnitGraphService.expandUsers` call, whose +first act is a BFS over `parent_business_unit_id` — so the two branches differed +only in their comments. + +A rule authored as `recipient_type: 'business_unit'`, which the authoring spec +(`ShareRecipientType`), the org-axis lint red-line table and ADR-0057 D5 all +describe as *"exactly one business unit's members (no subtree)"*, in fact reached +that unit **plus every descendant unit's members**. On a three-level tree a rule +anchored at a division silently granted to every department and office beneath +it. + +Two consequences, and the second is why this is filed as security rather than +tidiness: the narrow spelling over-granted **silently**, which is the worst +failure shape for generated security metadata (an agent that asks for the narrow +grant should get the narrow grant); and `unit_and_subordinates`, documented as +the *strictly wider* grant of the pair, was not wider at all, leaving the +distinction the lint red-line draws unenforceable in practice. + +## What changed + +`business_unit` now resolves through a new +`BusinessUnitGraphService.expandUnitMembers()` — members whose +`business_unit_id` equals the named unit, with no descent. It keeps every other +guarantee the subtree walk had: an inactive or out-of-tenant anchor unit +contributes nobody, and an unreadable unit fails closed rather than granting. + +`unit_and_subordinates` is **unchanged** and keeps the subtree walk — it is the +kind whose declared semantics *is* the hierarchy widening (ADR-0057 D5). The two +kinds remain two kinds; neither is merged into the other or retired. +`expandUsers()` also keeps its meaning for the `bu:` approver prefix and org +rollups, which are subtree consumers by contract. + +Grant recomputation on business-unit graph writes (#7729) still covers both +recipient kinds, because a unit-only expansion still reads `sys_business_unit` +for its anchor's `active` flag and tenant scope and `sys_business_unit_member` +for its members. What changed there is blast radius, not coverage: re-parenting a +unit no longer moves a `business_unit` rule's recipients, while deactivating the +anchor or editing its membership still does. + +## Migration + +**In-tree cost is zero** — no shipped example app or seeded rule authors +`business_unit` (the showcase and CRM apps use `position` and +`unit_and_subordinates`), so nothing in this repository changes behaviour. + +**Out-of-tree deployments:** if you authored a `business_unit` rule and were +relying — knowingly or not — on it reaching descendant units, those descendant +members **lose the grants that rule materialised**. Grants are reconciled on the +next evaluation pass, so the reduction lands without any action on your part. + +If the subtree reach was what you actually wanted, change the rule's recipient to +`unit_and_subordinates`, which has always meant exactly that and is unaffected by +this release. If you wanted the narrow grant, you now have it. diff --git a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts index b7cfd275e4..95f837a3b1 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -319,7 +319,14 @@ describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () expect(grantsFor('priya')).toBe(0); }); - it('covers `business_unit` recipients too — today they walk the same subtree resolver', async () => { + // [#7807] narrowed `business_unit` to exactly one unit's members. This case + // still belongs here, and deliberately: a unit-only expansion still reads + // `sys_business_unit` (its own `active` flag, its tenant scope) and + // `sys_business_unit_member`, so a write to either table can still move who + // the rule reaches. The anchor below is the unit priya is a DIRECT member + // of, which is what makes this a test of RECOMPUTE COVERAGE rather than of + // the subtree walk. + it('covers `business_unit` recipients too — a unit-only expansion still reads both BU tables', async () => { engine._tables.sys_sharing_rule[0].recipient_type = 'business_unit'; engine._tables.sys_sharing_rule[0].recipient_id = 'bu_west'; await rules.evaluateRule(RULE, SYS); diff --git a/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts index c346990fc5..b00aa1c414 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts @@ -30,19 +30,22 @@ * |-------------------------|---------------------------------------------|---| * | `user` | the literal id | no | * | `team` | `TeamGraphService` (`sys_team_member`, `sys_member`, `sys_user`) | no | - * | `business_unit` | `BusinessUnitGraphService.expandUsers` | YES | + * | `business_unit` | `BusinessUnitGraphService.expandUnitMembers` | YES | * | `position` | `PositionGraphService` (`sys_user_position`, `sys_member`) | no | * | `unit_and_subordinates` | `BusinessUnitGraphService.expandUsers` | YES | * | `queue` | returns `[]` (no `sys_queue` yet) | no | * - * `business_unit` is in that set on the strength of what the code does today: - * `expandRecipient` routes it through the SAME `expandUsers` call as - * `unit_and_subordinates`, so it walks `descendants()` and is just as exposed - * to a re-parent. (The spec declares it as "exactly one business unit's - * members (no subtree)" — that divergence is a separate defect and is filed - * separately; covering the kind here is correct under either reading, since a - * unit-only expansion still reads `sys_business_unit` for its own `active` - * flag and `sys_business_unit_member` for its members.) + * `business_unit` stays in that set after #7807 narrowed it to exactly one + * unit's members. The divergence this file originally noted — `expandRecipient` + * routing it through the SAME subtree `expandUsers` call as + * `unit_and_subordinates`, against a spec declaring it as "exactly one business + * unit's members (no subtree)" — was resolved in favour of the declaration, and + * membership here was correct under either reading for the reason that fix + * confirmed: a unit-only expansion still reads `sys_business_unit` for its own + * `active` flag and tenant scope, and `sys_business_unit_member` for its + * members. What changed is the blast radius, not the coverage — a re-parent no + * longer moves a `business_unit` rule's recipients (its anchor's own membership + * is what moves them), while a deactivation or a membership edit still does. * * Everything else is deliberately NOT recomputed. That exclusion is a * requirement, not an optimisation: a fix that recomputed every rule on every diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts index d61546128c..1d4ec5ab3b 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.test.ts @@ -113,6 +113,83 @@ describe('BusinessUnitGraphService — subtree expansion', () => { }); }); +/** + * [#7807] The two widths, pinned as a PAIR on one three-level tree. + * + * A division ⊃ department ⊃ office tree is the floor for this: on a two-level + * fixture "exactly one unit" and "unit plus its children" can agree by + * accident, so a two-level pin cannot tell the fixed behaviour from the + * defect. Each assertion below names the width it guards, because a change + * that narrowed BOTH kinds would satisfy the `business_unit` half while + * destroying the distinction the spec draws between them. + */ +const DIV_UNITS: UnitRow[] = [ + { id: 'bu_div', organization_id: null, active: true }, + { id: 'bu_dept', parent_business_unit_id: 'bu_div', organization_id: null, active: true }, + { id: 'bu_office', parent_business_unit_id: 'bu_dept', organization_id: null, active: true }, +]; +const DIV_MEMBERS: MemberRow[] = [ + { business_unit_id: 'bu_div', user_id: 'u_div' }, + { business_unit_id: 'bu_dept', user_id: 'u_dept' }, + { business_unit_id: 'bu_office', user_id: 'u_office' }, +]; + +describe('BusinessUnitGraphService — the two widths are actually two widths (#7807)', () => { + const graph = () => new BusinessUnitGraphService({ engine: makeEngine(DIV_UNITS, DIV_MEMBERS) }); + + it('NARROW — expandUnitMembers returns only the named unit, three levels notwithstanding', async () => { + expect(await graph().expandUnitMembers('bu_div')).toEqual(['u_div']); + }); + + it('WIDE — expandUsers still returns the whole subtree (the control)', async () => { + expect((await graph().expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']); + }); + + it('the narrow width skips even a DIRECT child, not merely the grandchild', async () => { + const users = await graph().expandUnitMembers('bu_div'); + expect(users).not.toContain('u_dept'); + expect(users).not.toContain('u_office'); + }); + + it('a mid-tree unit expands to its own members only', async () => { + expect(await graph().expandUnitMembers('bu_dept')).toEqual(['u_dept']); + }); + + it('an inactive unit contributes nobody to the narrow width either', async () => { + const units = DIV_UNITS.map((u) => (u.id === 'bu_div' ? { ...u, active: false } : u)); + const g = new BusinessUnitGraphService({ engine: makeEngine(units, DIV_MEMBERS) }); + expect(await g.expandUnitMembers('bu_div')).toEqual([]); + }); + + it('an unknown unit expands to nobody rather than to everybody', async () => { + expect(await graph().expandUnitMembers('bu_nope')).toEqual([]); + expect(await graph().expandUnitMembers('')).toEqual([]); + }); + + it('the narrow width is org-predicated exactly like the wide one', async () => { + const g = new BusinessUnitGraphService({ + engine: makeEngine(DIV_UNITS, DIV_MEMBERS), + organizationId: 'org_a', + }); + // Seeded (null-org) units are not visible to an org-scoped rule — the + // same `[divergence]` posture the wide width holds below. + expect(await g.expandUnitMembers('bu_div')).toEqual([]); + }); + + it('the two widths do NOT share a cache entry for the same unit id', async () => { + // Both maps are keyed by BU id. One shared map would let whichever width + // ran first answer for the other — the over-grant returning through the + // cache door. + const g = graph(); + expect(await g.expandUnitMembers('bu_div')).toEqual(['u_div']); + expect((await g.expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']); + // …and in the opposite order, on a fresh instance. + const g2 = graph(); + expect((await g2.expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']); + expect(await g2.expandUnitMembers('bu_div')).toEqual(['u_div']); + }); +}); + describe('BusinessUnitGraphService — org scoping (#3807)', () => { it('an org-less rule (today’s materialized shape) expands seeded units fine', async () => { // `expandRecipient` passes `rule.organization_id ?? null`, and every diff --git a/packages/plugins/plugin-sharing/src/business-unit-graph.ts b/packages/plugins/plugin-sharing/src/business-unit-graph.ts index e5fac402fa..65ffa41a24 100644 --- a/packages/plugins/plugin-sharing/src/business-unit-graph.ts +++ b/packages/plugins/plugin-sharing/src/business-unit-graph.ts @@ -9,6 +9,15 @@ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; type DeptCache = { descendants?: Map; expandUsers?: Map; + /** + * [#7807] Members of exactly one unit — a SEPARATE map from + * {@link DeptCache.expandUsers} on purpose. Both are keyed by business-unit + * id but answer different questions, so sharing one map would let a narrow + * `business_unit` expansion be served a cached subtree answer (re-opening + * the over-grant this issue closed) or vice versa, depending only on which + * recipient kind happened to be evaluated first in the pass. + */ + unitMembers?: Map; head?: Map; }; @@ -34,6 +43,14 @@ export interface BusinessUnitGraphOptions { * `active` flag as a hard filter (inactive departments contribute no * members and stop BFS descent into their subtrees). * + * Two DIFFERENT widths live here, and keeping them distinct is the point + * (#7807): + * - {@link BusinessUnitGraphService.expandUsers} — the unit PLUS every + * descendant unit (the `IBusinessUnitGraphService` contract; drives the + * `unit_and_subordinates` recipient). + * - {@link BusinessUnitGraphService.expandUnitMembers} — exactly that one + * unit's members (drives the `business_unit` recipient). + * * Reuses {@link TeamGraphService.managerOf} for user-level manager * lookup so callers can use this single service in approval / sharing * pipelines. @@ -50,6 +67,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { this.cache = opts.cache ?? {}; this.cache.descendants ??= new Map(); this.cache.expandUsers ??= new Map(); + this.cache.unitMembers ??= new Map(); this.cache.head ??= new Map(); this.teamGraph = opts.teamGraph; } @@ -60,20 +78,7 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { if (cached) return cached; // Verify seed itself is active + within tenant scope. - let seedActive = true; - try { - const seedRows = await this.engine.find('sys_business_unit', { - where: this.orgScope({ id: businessUnitId }), - fields: ['id', 'active'], - limit: 1, - context: SYSTEM_CTX, - }); - const seedRow: any = Array.isArray(seedRows) ? seedRows[0] : null; - if (!seedRow) seedActive = false; - else if (seedRow.active === false) seedActive = false; - } catch { - seedActive = false; - } + const seedActive = await this.seedIsUsable(businessUnitId); if (!seedActive) { this.cache.descendants!.set(businessUnitId, []); return []; @@ -107,6 +112,79 @@ export class BusinessUnitGraphService implements IBusinessUnitGraphService { return out; } + /** + * Is the seed unit itself active and inside the tenant scope? + * + * Shared by both widths so they agree on what an unusable seed is: a unit + * that does not exist, sits in another organization, or carries + * `active: false` contributes NOBODY — it is never merely "expanded without + * its descendants". A read failure answers `false` (fail closed: an + * unreadable unit must not grant). + */ + private async seedIsUsable(businessUnitId: string): Promise { + try { + const seedRows = await this.engine.find('sys_business_unit', { + where: this.orgScope({ id: businessUnitId }), + fields: ['id', 'active'], + limit: 1, + context: SYSTEM_CTX, + }); + const seedRow: any = Array.isArray(seedRows) ? seedRows[0] : null; + if (!seedRow) return false; + if (seedRow.active === false) return false; + return true; + } catch { + return false; + } + } + + /** + * [#7807] Members of EXACTLY ONE business unit — no subtree descent. + * + * This is the enforcement of the `business_unit` sharing-rule recipient, + * which the spec (`ShareRecipientType`), the lint red-line table and + * ADR-0057 D5 all declare as "exactly one business unit's members (no + * subtree)". Until #7807 the runtime routed it through + * {@link BusinessUnitGraphService.expandUsers} instead, so a rule anchored + * at a division silently reached every department and office beneath it — + * an over-grant, and one that made the strictly-wider `unit_and_subordinates` + * kind not wider at all. + * + * Deliberately NOT a variant of `expandUsers`: that method is the + * `IBusinessUnitGraphService` contract's SUBTREE expansion ("all user ids in + * `businessUnitId` or any descendant business unit") and keeps that meaning + * for `unit_and_subordinates`, the `bu:` approver prefix and org rollups. + * The two widths are now two methods rather than one method and two + * comments. + */ + async expandUnitMembers(businessUnitId: string): Promise { + if (!businessUnitId) return []; + const cached = this.cache.unitMembers!.get(businessUnitId); + if (cached) return cached; + + if (!(await this.seedIsUsable(businessUnitId))) { + this.cache.unitMembers!.set(businessUnitId, []); + return []; + } + + let rows: any[] = []; + try { + rows = await this.engine.find('sys_business_unit_member', { + where: { business_unit_id: businessUnitId }, + fields: ['user_id'], + limit: 10000, + context: SYSTEM_CTX, + }); + } catch { + rows = []; + } + const users = Array.from( + new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)), + ); + this.cache.unitMembers!.set(businessUnitId, users); + return users; + } + async expandUsers(businessUnitId: string): Promise { if (!businessUnitId) return []; const cached = this.cache.expandUsers!.get(businessUnitId); diff --git a/packages/plugins/plugin-sharing/src/recipient-width.test.ts b/packages/plugins/plugin-sharing/src/recipient-width.test.ts new file mode 100644 index 0000000000..419f1765b4 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/recipient-width.test.ts @@ -0,0 +1,265 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7807] `business_unit` vs `unit_and_subordinates` — two DECLARED widths, + * pinned as two ENFORCED widths. + * + * The defect: `SharingRuleService.expandRecipient` routed both recipient kinds + * through the identical `BusinessUnitGraphService.expandUsers` call, whose + * first act is a `descendants()` BFS. So a rule authored as `business_unit` — + * declared by `ShareRecipientType`, by the lint red-line table and by + * ADR-0057 D5 as "exactly one business unit's members (no subtree)" — in fact + * reached that unit **plus every descendant unit's members**. An over-grant, + * and one that made `unit_and_subordinates` (the "strictly WIDER grant" of the + * pair) not wider at all. + * + * Maintainer ruling 2026-08-12, direction 1: narrow the runtime to match the + * declaration. The two kinds stay two kinds; neither is retired. + * + * ## Why this file asserts a PAIR, not a fix + * + * "`business_unit` got narrower" is only half the evidence. A change that + * narrowed BOTH kinds satisfies that half completely while destroying the + * distinction the ruling exists to preserve — so the wide kind is asserted on + * the SAME tree, the SAME fixture and the SAME call, as a control. Each + * `describe` below names which width it guards, so a regression in either + * direction fails as the width it actually broke. + * + * ## Why three levels + * + * A division ⊃ department ⊃ office tree is the floor. On a two-level fixture + * "exactly one unit" and "unit plus its children" can agree by accident, and + * the pin cannot tell the fixed behaviour from the defect it was written for. + * The narrow assertions therefore exclude the DIRECT child as well as the + * grandchild. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; + +interface Row { [k: string]: any } + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; + +const NARROW_RULE = 'share_note_with_division_only'; +const WIDE_RULE = 'share_note_with_division_subtree'; + +function matches(row: Row, f: any): boolean { + if (!f || typeof f !== 'object') return true; + // A combinator is CONJOINED with its sibling field keys, never a + // short-circuit that returns before they are read (#7676): `listRules` + // composes `{object_name, active, $or:[…org scope…]}`, and a matcher that + // returned on the `$or` alone would match the whole table here while + // driver-sql and driver-memory conjoin the two. A fake looser than the + // contract it stands in for is how a green suite ships a broken filter. + if (Array.isArray(f.$or) && !f.$or.some((x: any) => matches(row, x))) return false; + if (Array.isArray(f.$and) && !f.$and.every((x: any) => matches(row, x))) return false; + for (const [k, v] of Object.entries(f)) { + if (k === '$or' || k === '$and') continue; + const rv = row[k]; + if (v != null && typeof v === 'object' && !Array.isArray(v)) { + const op: any = v; + if ('$in' in op) { if (!op.$in.includes(rv)) return false; continue; } + // `descendants()` filters children with `active: { $ne: false }`, so an + // undefined `active` must PASS — the graph treats absent as active. + if ('$ne' in op) { if (rv === op.$ne) return false; continue; } + if ('$gte' in op) { if (!(rv >= op.$gte)) return false; continue; } + } + if (rv !== v) return false; + } + return true; +} + +function makeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + let seq = 0; + return { + _tables: tables, + getSchema() { return undefined; }, + seed(object: string, rows: Row[]) { ensure(object).push(...rows.map((r) => ({ ...r }))); }, + async find(o: string, opts?: any) { + const f = opts?.filter ?? opts?.where; + return ensure(o).filter((r) => matches(r, f)).slice(0, opts?.limit ?? 10000); + }, + async insert(o: string, data: any) { + const row = { id: data.id ?? `${o}_${++seq}`, ...data }; + ensure(o).push(row); + return row; + }, + // Both write verbs open with the PRODUCER's own dispatch predicate + // (#4550 / #5480 / #6277) rather than a hand-mirrored guard, so a fixture + // that drifts to a call shape `ObjectQL` would refuse fails loudly here + // instead of collecting a green from a check that never ran. + async update(o: string, data: any, options?: any) { + const verdict = assertEngineUpdateDispatch(data, options); + const t = ensure(o); + const targets = verdict.kind === 'by-id' + ? t.filter((r) => r.id === verdict.id) + : t.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return verdict.kind === 'by-id' ? (targets[0] ?? null) : targets.length; + }, + async delete(o: string, opts?: any) { + assertEngineDeleteDispatch(opts); + const t = ensure(o); + const where = opts?.where ?? (opts?.id != null ? { id: opts.id } : {}); + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + return { ok: true }; + }, + }; +} + +describe('#7807 recipient width — business_unit vs unit_and_subordinates', () => { + let engine: ReturnType; + let rules: SharingRuleService; + + /** Who currently holds a rule-materialised grant on `recordId`. */ + const granteesOf = (recordId: string): string[] => + (engine._tables.sys_record_share ?? []) + .filter((r) => r.record_id === recordId && r.source === 'rule') + .map((r) => String(r.recipient_id)) + .sort(); + + beforeEach(async () => { + engine = makeEngine(); + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing }); + + // Three levels: division ⊃ department ⊃ office, one member each, plus a + // unit OUTSIDE the division entirely so "narrow" cannot pass merely by + // granting nobody beyond the tree. + engine.seed('sys_business_unit', [ + { id: 'bu_div', name: 'Division', parent_business_unit_id: null, active: true }, + { id: 'bu_dept', name: 'Department', parent_business_unit_id: 'bu_div', active: true }, + { id: 'bu_office', name: 'Office', parent_business_unit_id: 'bu_dept', active: true }, + { id: 'bu_other', name: 'Elsewhere', parent_business_unit_id: null, active: true }, + ]); + engine.seed('sys_business_unit_member', [ + { id: 'bum_div', business_unit_id: 'bu_div', user_id: 'u_div' }, + { id: 'bum_dept', business_unit_id: 'bu_dept', user_id: 'u_dept' }, + { id: 'bum_office', business_unit_id: 'bu_office', user_id: 'u_office' }, + { id: 'bum_other', business_unit_id: 'bu_other', user_id: 'u_other' }, + ]); + + // Two records so the two rules cannot grant into each other's result. + engine.seed('showcase_private_note', [ + { id: 'note_narrow', tag: 'narrow', owner_id: 'author' }, + { id: 'note_wide', tag: 'wide', owner_id: 'author' }, + ]); + + // The SAME anchor unit for both rules — the whole point of the pair. + engine.seed('sys_sharing_rule', [ + { + id: 'srule_narrow', organization_id: null, name: NARROW_RULE, + label: 'Note → Division only', object_name: 'showcase_private_note', + criteria_json: JSON.stringify({ tag: 'narrow' }), + recipient_type: 'business_unit', recipient_id: 'bu_div', + access_level: 'read', active: true, managed_by: 'package', + }, + { + id: 'srule_wide', organization_id: null, name: WIDE_RULE, + label: 'Note → Division subtree', object_name: 'showcase_private_note', + criteria_json: JSON.stringify({ tag: 'wide' }), + recipient_type: 'unit_and_subordinates', recipient_id: 'bu_div', + access_level: 'read', active: true, managed_by: 'package', + }, + ]); + + await rules.evaluateRule(NARROW_RULE, SYS); + await rules.evaluateRule(WIDE_RULE, SYS); + }); + + describe('NARROW — `business_unit` reaches exactly one unit', () => { + it('grants the anchor unit\'s own members', () => { + expect(granteesOf('note_narrow')).toEqual(['u_div']); + }); + + it('does NOT reach the direct child unit (the over-grant this closed)', () => { + expect(granteesOf('note_narrow')).not.toContain('u_dept'); + }); + + it('does NOT reach the grandchild unit either', () => { + expect(granteesOf('note_narrow')).not.toContain('u_office'); + }); + + it('never reached outside the tree, before or after', () => { + expect(granteesOf('note_narrow')).not.toContain('u_other'); + }); + }); + + describe('WIDE — `unit_and_subordinates` still reaches the whole subtree (control)', () => { + it('grants the anchor unit AND every descendant unit, three levels down', () => { + expect(granteesOf('note_wide')).toEqual(['u_dept', 'u_div', 'u_office']); + }); + + it('stops at the tree boundary', () => { + expect(granteesOf('note_wide')).not.toContain('u_other'); + }); + }); + + describe('the pair, stated as one fact', () => { + it('the wider kind is STRICTLY wider — same anchor, same tree, same pass', () => { + const narrow = granteesOf('note_narrow'); + const wide = granteesOf('note_wide'); + // Strictly wider: narrow ⊂ wide, and the containment is proper. + expect(narrow.every((u) => wide.includes(u))).toBe(true); + expect(wide.length).toBeGreaterThan(narrow.length); + }); + }); + + describe('the narrowing rides the RECONCILE path too, not just first materialisation', () => { + it('re-running the narrow rule keeps it narrow (no drift back to the subtree)', async () => { + await rules.evaluateRule(NARROW_RULE, SYS); + expect(granteesOf('note_narrow')).toEqual(['u_div']); + }); + + it('a member joining a DESCENDANT unit does not widen the narrow rule', async () => { + await engine.insert('sys_business_unit_member', { + id: 'bum_late', business_unit_id: 'bu_dept', user_id: 'u_late', + }); + await rules.evaluateRule(NARROW_RULE, SYS); + expect(granteesOf('note_narrow')).toEqual(['u_div']); + + // …and the same join DOES widen the subtree rule, which is what proves + // the fixture is capable of expressing the difference at all. + await rules.evaluateRule(WIDE_RULE, SYS); + expect(granteesOf('note_wide')).toContain('u_late'); + }); + + it('a member joining the ANCHOR unit widens the narrow rule (it is not simply frozen)', async () => { + await engine.insert('sys_business_unit_member', { + id: 'bum_div2', business_unit_id: 'bu_div', user_id: 'u_div2', + }); + await rules.evaluateRule(NARROW_RULE, SYS); + expect(granteesOf('note_narrow')).toEqual(['u_div', 'u_div2']); + }); + + it('re-parenting a descendant OUT never mattered to the narrow rule', async () => { + await engine.update('sys_business_unit', { id: 'bu_dept', parent_business_unit_id: 'bu_other' }); + await rules.evaluateRule(NARROW_RULE, SYS); + expect(granteesOf('note_narrow')).toEqual(['u_div']); + + // The wide rule loses the moved subtree — the control moving under the + // same write, which is how we know the write took effect at all. + await rules.evaluateRule(WIDE_RULE, SYS); + expect(granteesOf('note_wide')).toEqual(['u_div']); + }); + }); + + describe('an inactive anchor grants nobody under either width', () => { + it('narrow: an inactive unit contributes no members', async () => { + await engine.update('sys_business_unit', { id: 'bu_div', active: false }); + await rules.evaluateRule(NARROW_RULE, SYS); + expect(granteesOf('note_narrow')).toEqual([]); + }); + + it('wide: an inactive seed still blanks the whole subtree', async () => { + await engine.update('sys_business_unit', { id: 'bu_div', active: false }); + await rules.evaluateRule(WIDE_RULE, SYS); + expect(granteesOf('note_wide')).toEqual([]); + }); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 4a50e83d27..91b04618ba 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -709,12 +709,24 @@ export class SharingRuleService implements ISharingRuleService { if (rule.recipient_type === 'user') return [rule.recipient_id]; if (rule.recipient_type === 'team') return team.expandUsers(rule.recipient_id); if (rule.recipient_type === 'business_unit') { + // [#7807] EXACTLY ONE unit's members — no subtree descent. The spec + // (`ShareRecipientType`), the lint red-line table and ADR-0057 D5 all + // declare this kind as "exactly one business unit's members (no + // subtree)"; this branch used to call `expandUsers`, the SAME subtree + // walk `unit_and_subordinates` below uses, so the two kinds differed + // only in their comments and a rule anchored at a division silently + // reached every department and office beneath it. + // + // ⛔ Do not "simplify" this back into a shared call with the branch + // below. The two widths are the contract: `unit_and_subordinates` is + // the strictly WIDER grant of the pair, and it is only wider while this + // one stays narrow. const dept = new BusinessUnitGraphService({ engine: this.engine, organizationId: rule.organization_id ?? null, teamGraph: team, }); - return dept.expandUsers(rule.recipient_id); + return dept.expandUnitMembers(rule.recipient_id); } if (rule.recipient_type === 'position') { // ADR-0090 D3 — positions are flat; expand holders via the platform @@ -732,6 +744,9 @@ export class SharingRuleService implements ISharingRuleService { // re-homed onto the BUSINESS-UNIT subtree: the unit named by // `recipient_id` plus every descendant unit's members. The former // position-tree walk queried a `parent` column that never existed. + // + // This is the WIDE half of the pair (#7807) and keeps the subtree walk + // unchanged — `expandUsers` is the contract's descendant expansion. const dept = new BusinessUnitGraphService({ engine: this.engine, organizationId: rule.organization_id ?? null, diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 78704802fc..50c85560f9 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -516,7 +516,14 @@ describe('SharingRuleService', () => { expect(active.map(r => r.name)).toEqual(['a']); }); - it('recipientType=business_unit expands via the BU graph (BFS)', async () => { + // [#7807] This pair replaces a single test that asserted + // `recipientType=business_unit` expanded "alice (emea_sales) + bob + // (emea_sales_uk descendant)" — i.e. it pinned the over-grant as if it were + // the contract. `business_unit` is declared as exactly one unit's members + // (no subtree); the subtree is `unit_and_subordinates`' own semantics + // (ADR-0057 D5). Both widths are asserted here on the SAME anchor so a + // change that collapsed them again fails as the width it broke. + it('recipientType=business_unit expands EXACTLY ONE unit — no subtree (#7807)', async () => { const r = await rules.defineRule({ name: 'dept_rule', label: 'Dept Rule', object: 'opportunity', criteria: { amount: { $gte: 100000 } }, @@ -524,6 +531,20 @@ describe('SharingRuleService', () => { }, SYS); const res = await rules.evaluateRule(r.id, SYS); expect(res.matchedRecords).toBe(2); // opp1, opp2 + expect(res.expandedUsers).toBe(1); // alice (emea_sales) — bob is a DESCENDANT + expect(res.grantsCreated).toBe(2); + expect(engine._tables.sys_record_share).toHaveLength(2); + expect(new Set(engine._tables.sys_record_share.map(s => s.recipient_id))).toEqual(new Set(['alice'])); + }); + + it('recipientType=unit_and_subordinates expands the whole subtree (the wider half)', async () => { + const r = await rules.defineRule({ + name: 'dept_subtree_rule', label: 'Dept Subtree Rule', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'unit_and_subordinates', recipientId: 'emea_sales', accessLevel: 'read', + }, SYS); + const res = await rules.evaluateRule(r.id, SYS); + expect(res.matchedRecords).toBe(2); // opp1, opp2 expect(res.expandedUsers).toBe(2); // alice (emea_sales) + bob (emea_sales_uk descendant) expect(res.grantsCreated).toBe(4); expect(engine._tables.sys_record_share).toHaveLength(4); diff --git a/packages/qa/dogfood/test/authz-conformance.matrix.ts b/packages/qa/dogfood/test/authz-conformance.matrix.ts index c886d48dbe..4477ed9092 100644 --- a/packages/qa/dogfood/test/authz-conformance.matrix.ts +++ b/packages/qa/dogfood/test/authz-conformance.matrix.ts @@ -201,7 +201,8 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [ { id: 'sharing-rules', summary: 'criteria sharing rules (recipients: user/team/position/unit_and_subordinates/business_unit)', state: 'enforced', enforcement: 'plugin-sharing/sharing-rule-service.ts (materialized into sys_record_share); every authorable recipient expands in expandRecipient. The never-enforced owner-type rules + group/guest recipients were removed from the authoring spec (ADR-0078; group renamed → team)', proof: 'showcase-bu-hierarchy-sharing.dogfood.test.ts' }, { id: 'hierarchy-widening', summary: 'hierarchy widening — a unit + its subordinate units gain access', state: 'enforced', - enforcement: 'plugin-sharing/business-unit-graph.ts BusinessUnitGraphService subtree (business_unit recipient) — ADR-0057 D5 re-homed off the never-existent sys_position.parent', proof: 'showcase-bu-hierarchy-sharing.dogfood.test.ts' }, + enforcement: 'plugin-sharing/business-unit-graph.ts BusinessUnitGraphService.expandUsers subtree (unit_and_subordinates recipient) — ADR-0057 D5 re-homed off the never-existent sys_position.parent. The narrower business_unit recipient resolves through expandUnitMembers (exactly one unit, no descent) and is pinned by the same proof file: #7807 narrowed the runtime to the declaration after both kinds shared one subtree walk', + proof: 'showcase-bu-hierarchy-sharing.dogfood.test.ts' }, { id: 'rls-compiler-fail-closed', summary: 'uncompilable RLS predicate is surfaced/denied, not dropped', state: 'enforced', enforcement: 'plugin-security/rls-compiler.ts compileFilter (drop + warn + RLS_DENY_FILTER) on the shape gate formula/rls-predicate.ts isSupportedRlsExpression — hoisted out of plugin-security in #4983 so lint/validate-rls-predicate-enforceability.ts can REJECT the same predicate at authoring time (ADR-0056 D4), from the one definition' }, { id: 'system-permissions', summary: 'systemPermissions / tab-app gating', state: 'enforced', diff --git a/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts b/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts index 98137decf6..dfe9ee1d57 100644 --- a/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts @@ -1,12 +1,23 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // // ADR-0057 D6 (reconciles ADR-0056 D6 / #2077 demo) — a sharing rule whose -// recipient is a BUSINESS UNIT widens access DOWN the hierarchy: the unit's -// members AND every subordinate (descendant) unit's members gain access via the -// `sys_business_unit` tree (BFS). This is the honest re-homing of the broken -// `unit_and_subordinates` (sys_position.parent never existed) onto the working -// business-unit tree. Proven end-to-end: the rule materialises sys_record_share -// rows; a non-owner in the unit subtree can then read a private record. +// recipient is `unit_and_subordinates` widens access DOWN the hierarchy: the +// unit's members AND every subordinate (descendant) unit's members gain access +// via the `sys_business_unit` tree (BFS). This is the honest re-homing of the +// broken `unit_and_subordinates` (sys_position.parent never existed) onto the +// working business-unit tree. Proven end-to-end: the rule materialises +// sys_record_share rows; a non-owner in the unit subtree can then read a +// private record. +// +// [#7807] The widening rule below used to be authored as `business_unit`, and +// it passed — because the runtime routed BOTH recipient kinds through the same +// subtree walk, so the narrow spelling silently over-granted. The maintainer +// ruled (2026-08-12) to narrow the runtime to the declaration, which makes the +// recipient kind load-bearing here rather than interchangeable: widening is +// `unit_and_subordinates`' own semantics. The second suite pins the other half +// end-to-end — a `business_unit` rule reaches the anchor unit and stops there — +// because a fix that narrowed BOTH kinds would satisfy the first suite's +// headline while destroying the distinction the spec draws. // // @proof: showcase-bu-hierarchy-sharing // @@ -31,6 +42,8 @@ describe('showcase: business-unit hierarchy sharing rule (ADR-0057 D6 / #2077)', let ql: any; let ownerTok: string, mgrTok: string, contribTok: string, outsiderTok: string; let noteId: string; + /** [#7807] The note shared with the narrow `business_unit` recipient. */ + let exactNoteId: string; beforeAll(async () => { // [#5491] The platform baseline no longer ships a `'*'` object grant, so a @@ -70,22 +83,46 @@ describe('showcase: business-unit hierarchy sharing rule (ADR-0057 D6 / #2077)', } // Define a sharing rule: share the BU-shared note with the PARENT business - // unit (and, via the tree, its descendants). Recipient = business_unit. - // The criteria is not optional — a rule without one would share every - // record of the object, which defineRule now rejects (#3896). + // unit AND, via the tree, its descendants. Recipient = + // `unit_and_subordinates` — the kind whose declared semantics IS the + // subtree (#7807). The criteria is not optional — a rule without one would + // share every record of the object, which defineRule now rejects (#3896). const rules: any = stack.kernel.getService('sharingRules'); await rules.defineRule({ name: 'share_notes_with_region', label: 'Notes → Region (BU subtree)', object: 'showcase_private_note', criteria: { title: 'BU-shared note' }, - recipientType: 'business_unit', + recipientType: 'unit_and_subordinates', recipientId: 'bu_h_parent', accessLevel: 'read', active: true, }, SYS); // Materialise grants for existing records. await rules.evaluateRule('share_notes_with_region', SYS); + + // [#7807] The narrow half, on the SAME tree and the SAME members: a second + // note shared with `business_unit` anchored at the SAME parent unit. A + // separate record is required — sharing both notes through one rule pair + // would let the subtree rule's grants answer for the narrow one. + const c2 = await stack.apiAs(ownerTok, 'POST', OBJ, { title: 'BU-exact note' }); + expect(c2.status, 'owner creates the exact-width note').toBeLessThan(300); + exactNoteId = (await c2.json())?.id ?? (await c2.json())?.record?.id; + if (!exactNoteId) { + const row = await ql.findOne('showcase_private_note', { where: { title: 'BU-exact note' }, context: SYS }); + exactNoteId = row?.id; + } + await rules.defineRule({ + name: 'share_notes_with_region_exact', + label: 'Notes → Region (exactly that unit)', + object: 'showcase_private_note', + criteria: { title: 'BU-exact note' }, + recipientType: 'business_unit', + recipientId: 'bu_h_parent', + accessLevel: 'read', + active: true, + }, SYS); + await rules.evaluateRule('share_notes_with_region_exact', SYS); }, 90_000); afterAll(async () => { await stack?.stop(); }); @@ -116,4 +153,34 @@ describe('showcase: business-unit hierarchy sharing rule (ADR-0057 D6 / #2077)', const r = await stack.apiAs(outsiderTok, 'GET', `${OBJ}/${noteId}`); expect(r.status, 'outsider stays denied').not.toBe(200); }); + + // ── [#7807] the narrow half of the pair ──────────────────────────────── + // + // Same tree, same members, same booted stack — only the recipient KIND + // differs. Before #7807 every assertion in this block failed: the + // `business_unit` rule walked the subtree exactly like the one above. + + it('materialises grants for the anchor unit ONLY (no subtree)', async () => { + const shares = await ql.find('sys_record_share', { + where: { object_name: 'showcase_private_note', record_id: exactNoteId, source: 'rule' }, + context: SYS, + }); + const recipients = (shares ?? []).map((s: any) => s.recipient_id); + const mgrId = (await ql.findOne('sys_user', { where: { email: 'bu-mgr@verify.test' }, context: SYS }))?.id; + const contribId = (await ql.findOne('sys_user', { where: { email: 'bu-contrib@verify.test' }, context: SYS }))?.id; + expect(recipients, 'anchor-unit member granted').toContain(mgrId); + expect(recipients, 'subordinate-unit member NOT granted — business_unit is exactly one unit') + .not.toContain(contribId); + }); + + it('a manager in the anchor unit can READ the exact-width note', async () => { + const r = await stack.apiAs(mgrTok, 'GET', `${OBJ}/${exactNoteId}`); + expect(r.status, 'anchor-unit member reads via the narrow BU share').toBe(200); + }); + + it('a contributor in a SUBORDINATE unit is DENIED the exact-width note', async () => { + const r = await stack.apiAs(contribTok, 'GET', `${OBJ}/${exactNoteId}`); + expect(r.status, 'subordinate-unit member denied — no subtree widening on business_unit') + .not.toBe(200); + }); });