fix(plugin-sharing): the business_unit recipient expands exactly one unit, not the whole subtree (#7807)

.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.

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

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

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

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<string, string[]>;
  expandUsers?: Map<string, string[]>;
+ /**
+ * [#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<string, string[]>;
  head?: Map<string, string | null>;
 };
 
@@ -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<boolean> {
+ 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<string[]> {
+ 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<string[]> {
  if (!businessUnitId) return [];
  const cached = this.cache.expandUsers!.get(businessUnitId);