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); + }); });