diff --git a/.changeset/bu-tree-share-recompute.md b/.changeset/bu-tree-share-recompute.md new file mode 100644 index 0000000000..47da12f087 --- /dev/null +++ b/.changeset/bu-tree-share-recompute.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +Recompute `unit_and_subordinates` / `business_unit` sharing-rule grants when the business-unit graph changes. + +**Security:** read access granted through a business-unit sharing rule is now withdrawn immediately when the business unit is moved out of the shared subtree, deactivated or deleted, or when the membership row is removed — previously it survived until the shared record happened to be written next, which put no bound at all on how long a revoked recipient kept reading the record. + +The sharing-rule recompute hooks were registered only on each rule's own object, and nothing was registered on `sys_business_unit` or `sys_business_unit_member`. A rule whose recipient resolves through the business-unit tree therefore never re-materialised its `sys_record_share` grants when the tree or a membership moved. Writes to both tables now drive a recompute: the withdrawal is synchronous and complete before the write returns (scoped by recipient, so it needs no scan of the records the rule matches), and the grant direction — a unit moved *into* a shared subtree — is queued on the existing re-grant queue. + +Only recipient kinds that actually read the business-unit graph are recomputed (`business_unit`, `unit_and_subordinates`); `user`, `team`, `position` and `queue` rules are untouched by a business-unit write. diff --git a/content/docs/permissions/sharing-rules.mdx b/content/docs/permissions/sharing-rules.mdx index cfdf7c9ebb..6326e3a03b 100644 --- a/content/docs/permissions/sharing-rules.mdx +++ b/content/docs/permissions/sharing-rules.mdx @@ -166,12 +166,13 @@ unaffected. A sharing rule's grants are **materialized** — evaluating a rule writes real `sys_record_share` rows with `source: 'rule'` and `source_id` set to the rule. Because those rows outlive the evaluation that produced them, withdrawal has to -be an explicit act, and it happens at **three** moments: +be an explicit act, and it happens at **four** moments: | When | What is reconciled | |:--|:--| | **The write that deactivates or edits the rule** | That rule's grants, immediately — deactivating with `active: false` (or `POST {basePath}/sharing/rules` with the same name) revokes them before the call returns | | **The next insert/update of a matching record** | That record's grants for every rule on the object — an inactive rule desires nothing, so its rows are revoked | +| **A `sys_business_unit` or `sys_business_unit_member` write** | The grants of rules whose recipients read the BU graph — `unit_and_subordinates` and `business_unit`, and only those two. Recipients the rule no longer reaches are revoked **before the write returns**; the grant direction (a unit moved *into* a shared subtree) is queued and coalesced per rule (objectstack#7729) | | **Every boot** | All rules, plus a sweep of `source: 'rule'` rows whose `source_id` no longer resolves to any rule | Deleting a rule withdraws its grants too, whether you delete it through diff --git a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts new file mode 100644 index 0000000000..b7cfd275e4 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -0,0 +1,486 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7729] Sharing-rule revocation on BUSINESS-UNIT GRAPH writes. + * + * The defect was timing, not resolution. Subtree expansion resolved correctly + * through three levels and was symmetric on placement; what never happened was + * the recompute. `bindRuleHooks` binds only on each rule's own `object_name`, + * so the materialised `sys_record_share` grants moved only when the shared + * RECORD was written. The QA matrix that filed this: + * + * inSubtree=1, reparentedOut(no touch)=1, reparentedOut(touched)=0, + * restored(no touch)=0, restored(touched)=1 + * + * The security column is `reparentedOut(no touch)=1` — after a business unit + * is moved OUT of a shared subtree its members keep read access, indefinitely, + * until somebody happens to write the shared record. + * + * These tests drive REAL `SharingService` / `SharingRuleService` instances over + * an in-memory engine that dispatches hooks on write, so what is measured is + * the whole path: BU write → hook → recipient re-expansion → grant diff. The + * two `(no touch)` cells are asserted with a write log proving the shared + * record was never touched. + */ + +import { describe, it, expect, beforeEach, afterEach, vi, type MockInstance } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; +import { ruleRegrantQueue } from './rule-hooks.js'; +import { + bindBusinessUnitTreeRecompute, + unbindBusinessUnitTreeRecompute, + writeCanChangeExpansion, + BU_TREE_RECOMPUTE_PACKAGE, + BU_TREE_RECIPIENT_TYPES, + BU_GRAPH_OBJECTS, +} from './bu-tree-recompute.js'; + +interface Row { [k: string]: any } +type HookEntry = { event: string; handler: (ctx: any) => any; options: Row }; + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; + +/** The rule under test — the showcase geometry the QA run exercised. */ +const RULE = 'share_new_inquiries_with_field_ops'; + +function matches(row: Row, f: any): boolean { + if (!f || typeof f !== 'object') return true; + // [#7676] A combinator is CONJOINED with its sibling field keys, never a + // short-circuit that returns before they are read — the looseness #7760 had + // to remove from `sharing-rule.test.ts`'s fake one commit before this one. + // `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. + 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; +} + +/** + * In-memory engine that dispatches lifecycle hooks on every write, so the + * hooks under test run exactly where they run in production. + */ +function makeEngine() { + const tables: Record = {}; + const hooks: HookEntry[] = []; + /** Every write, so a test can prove a record was NOT touched. */ + const writes: Array<{ op: string; object: string }> = []; + const ensure = (n: string) => (tables[n] ??= []); + + async function dispatch(event: string, object: string, ctx: Row): Promise { + const applicable = hooks + .filter((h) => { + if (h.event !== event) return false; + const o = h.options.object; + return Array.isArray(o) ? o.includes(object) : o === object; + }) + .sort((a, b) => (a.options.priority ?? 0) - (b.options.priority ?? 0)); + for (const h of applicable) await h.handler(ctx); + } + + return { + _tables: tables, + _writes: writes, + _hooks: hooks, + getSchema() { return undefined; }, + registerHook(event: string, handler: (ctx: any) => any, options: Row = {}) { + hooks.push({ event, handler, options }); + }, + unregisterHooksByPackage(packageId: string) { + let removed = 0; + for (let i = hooks.length - 1; i >= 0; i--) { + if (hooks[i].options.packageId === packageId) { hooks.splice(i, 1); removed++; } + } + return removed; + }, + boundFor(packageId: string) { + return hooks.filter((h) => h.options.packageId === packageId); + }, + /** Seed rows WITHOUT firing hooks — the boot-loader analog. */ + 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 = { ...data }; + ensure(o).push(row); + writes.push({ op: 'insert', object: o }); + await dispatch('afterInsert', o, { result: row, input: { data } }); + return row; + }, + // Both write verbs open with the PRODUCER's own dispatch predicate + // (#4550 / #5480 / #6277), never a hand-mirrored guard: a fixture that + // drifts to a call shape `ObjectQL` would refuse fails loudly here instead + // of collecting a green from gates 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); + writes.push({ op: 'update', object: o }); + for (const r of targets) { + await dispatch('afterUpdate', o, { result: r, input: { id: r.id, 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 ?? {}; + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + writes.push({ op: 'delete', object: o }); + await dispatch('afterDelete', o, { input: { id: opts?.id, options: opts } }); + return { ok: true }; + }, + }; +} + +describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () => { + let engine: ReturnType; + let sharing: SharingService; + let rules: SharingRuleService; + + /** How many `read` grants `userId` currently holds on `recordId`. */ + const grantsFor = (userId: string, recordId = 'inq_new'): number => + (engine._tables.sys_record_share ?? []).filter( + (r) => r.recipient_id === userId && r.record_id === recordId && r.source === 'rule', + ).length; + + /** Writes recorded against the SHARED RECORD's object — must stay at zero. */ + const sharedRecordWrites = (): number => + engine._writes.filter((w) => w.object === 'showcase_inquiry').length; + + beforeEach(async () => { + engine = makeEngine(); + sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing }); + + // Three-level BU tree, plus a sibling root to re-parent OUT into. + engine.seed('sys_business_unit', [ + { id: 'bu_root', name: 'Root', parent_business_unit_id: null, active: true }, + { id: 'bu_field_ops', name: 'Field Ops', parent_business_unit_id: 'bu_root', active: true }, + { id: 'bu_west', name: 'West', parent_business_unit_id: 'bu_field_ops', active: true }, + { id: 'bu_outside', name: 'Outside', parent_business_unit_id: 'bu_root', active: true }, + ]); + // The seed-data-thin trap #7729 records: a fresh boot seeds the tree and + // ZERO member rows, so a fixture that does not place someone explicitly + // measures nothing at all. `priya` sits at the THIRD level. + engine.seed('sys_business_unit_member', [ + { id: 'bum_priya', business_unit_id: 'bu_west', user_id: 'priya', is_primary: true }, + ]); + engine.seed('showcase_inquiry', [ + { id: 'inq_new', status: 'new', owner_id: 'wes' }, + { id: 'inq_won', status: 'won', owner_id: 'wes' }, + ]); + engine.seed('sys_sharing_rule', [{ + id: 'srule_field_ops', + organization_id: null, + name: RULE, + label: 'New inquiries → Field Ops subtree', + object_name: 'showcase_inquiry', + criteria_json: JSON.stringify({ status: 'new' }), + recipient_type: 'unit_and_subordinates', + recipient_id: 'bu_field_ops', + access_level: 'read', + active: true, + managed_by: 'package', + }]); + + bindBusinessUnitTreeRecompute(engine as any, rules, undefined); + + // Boot-backfill analog: materialise the rule once, the way + // `kernel:bootstrapped` does. This is the matrix's `inSubtree` cell. + await rules.evaluateRule(RULE, SYS); + engine._writes.length = 0; // only writes AFTER setup are interesting + }); + + /** + * Release for the queue gate the security pin installs. + * + * Held HERE rather than inside that test, because the queue is module-scoped + * and shared by every test in this file: a gate left shut by a FAILING + * assertion blocks the chain forever, and every later test then dies in + * `whenIdle()` with a timeout. Measured during this fix's own ablation — the + * one clear assertion failure arrived as 15 timeouts, most of them in tests + * the ablation should not have touched at all. Releasing unconditionally + * here keeps a real regression legible as the one test it actually is. + */ + let releaseGate: (() => void) | null = null; + + afterEach(async () => { + releaseGate?.(); + releaseGate = null; + // The re-grant queue is module-scoped; never leak a pending pass. + await ruleRegrantQueue.whenIdle(); + }); + + it('inSubtree=1 — a member three levels down inside the shared subtree reads the record', () => { + expect(grantsFor('priya')).toBe(1); + expect(grantsFor('priya', 'inq_won')).toBe(0); // criteria still bounds it + }); + + it( + 'SECURITY PIN — a BU re-parented OUT loses its members access with NO write to the shared record, ' + + 'and before the write returns', + async () => { + // Block the asynchronous re-grant queue so nothing it does can be + // mistaken for the synchronous revoke. If the grant is gone while this + // gate is shut, the revocation happened on the write path. + const gate = new Promise((r) => { releaseGate = r; }); + ruleRegrantQueue.enqueue(() => gate); + + await engine.update('sys_business_unit', { + id: 'bu_west', + parent_business_unit_id: 'bu_outside', + }); + + expect(grantsFor('priya')).toBe(0); // was 1 before this fix + expect(sharedRecordWrites()).toBe(0); // …and nobody touched the record + + releaseGate!(); + releaseGate = null; + await ruleRegrantQueue.whenIdle(); + expect(grantsFor('priya')).toBe(0); // still 0 once the queue drains + }, + ); + + it('restored(no touch)=1 — re-parenting the BU back IN re-grants without touching the record', async () => { + await engine.update('sys_business_unit', { id: 'bu_west', parent_business_unit_id: 'bu_outside' }); + expect(grantsFor('priya')).toBe(0); + + await engine.update('sys_business_unit', { id: 'bu_west', parent_business_unit_id: 'bu_field_ops' }); + await ruleRegrantQueue.whenIdle(); + + expect(grantsFor('priya')).toBe(1); // was 0 before this fix + expect(sharedRecordWrites()).toBe(0); + }); + + it('deactivating a BU in the middle of the subtree revokes the descendants below it', async () => { + await engine.update('sys_business_unit', { id: 'bu_west', active: false }); + expect(grantsFor('priya')).toBe(0); + expect(sharedRecordWrites()).toBe(0); + + await engine.update('sys_business_unit', { id: 'bu_west', active: true }); + await ruleRegrantQueue.whenIdle(); + expect(grantsFor('priya')).toBe(1); + }); + + it('deleting a BU out of the subtree revokes its members', async () => { + await engine.delete('sys_business_unit', { where: { id: 'bu_west' }, multi: true }); + expect(grantsFor('priya')).toBe(0); + expect(sharedRecordWrites()).toBe(0); + }); + + it('membership REMOVE revokes, membership ADD re-grants — both without touching the record', async () => { + await engine.delete('sys_business_unit_member', { where: { id: 'bum_priya' }, multi: true }); + expect(grantsFor('priya')).toBe(0); + expect(sharedRecordWrites()).toBe(0); + + await engine.insert('sys_business_unit_member', { + id: 'bum_priya2', business_unit_id: 'bu_west', user_id: 'priya', + }); + await ruleRegrantQueue.whenIdle(); + expect(grantsFor('priya')).toBe(1); + expect(sharedRecordWrites()).toBe(0); + }); + + it('moving a member between units inside the same subtree keeps access', async () => { + await engine.update('sys_business_unit_member', { + id: 'bum_priya', business_unit_id: 'bu_field_ops', + }); + await ruleRegrantQueue.whenIdle(); + expect(grantsFor('priya')).toBe(1); + }); + + it('moving a member OUT of the subtree entirely revokes, synchronously', async () => { + await engine.update('sys_business_unit_member', { + id: 'bum_priya', business_unit_id: 'bu_outside', + }); + expect(grantsFor('priya')).toBe(0); + await ruleRegrantQueue.whenIdle(); + expect(grantsFor('priya')).toBe(0); + }); + + it('covers `business_unit` recipients too — today they walk the same subtree resolver', 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); + expect(grantsFor('priya')).toBe(1); + + await engine.update('sys_business_unit_member', { id: 'bum_priya', business_unit_id: 'bu_outside' }); + expect(grantsFor('priya')).toBe(0); + }); +}); + +describe('#7729 non-regression — rules that do not read the BU tree are left alone', () => { + let engine: ReturnType; + let rules: SharingRuleService; + // Typed from the METHODS rather than as a bare `ReturnType`. + // That erased spelling drops the signature, so `mock.calls` degrades to an + // implicit `any` per element — invisible to `pnpm typecheck` here (this + // package's tsconfig excludes `*.test.ts`) and caught only by + // `check:type-check-debt`, whose ratchet may not be raised for new code. + // Deriving from the service also keeps the assertions honest: `c[0]` is the + // real argument type, so the `as any` this used to need is gone. + let revokeSpy: MockInstance; + let evaluateSpy: MockInstance; + + beforeEach(() => { + engine = makeEngine(); + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing }); + engine.seed('sys_business_unit', [ + { id: 'bu_root', name: 'Root', parent_business_unit_id: null, active: true }, + { id: 'bu_west', name: 'West', parent_business_unit_id: 'bu_root', active: true }, + ]); + revokeSpy = vi.spyOn(rules, 'revokeRuleGrantsForRetiredRecipients'); + evaluateSpy = vi.spyOn(rules, 'evaluateRule'); + bindBusinessUnitTreeRecompute(engine as any, rules, undefined); + }); + + afterEach(async () => { + await ruleRegrantQueue.whenIdle(); + vi.restoreAllMocks(); + }); + + const seedRule = (id: string, recipient_type: string, recipient_id: string) => { + engine.seed('sys_sharing_rule', [{ + id, organization_id: null, name: id, label: id, + object_name: 'showcase_inquiry', criteria_json: JSON.stringify({ status: 'new' }), + recipient_type, recipient_id, access_level: 'read', active: true, + }]); + }; + + it('a `user` / `team` / `position` / `queue` rule is never recomputed by a BU write', async () => { + seedRule('srule_user', 'user', 'priya'); + seedRule('srule_team', 'team', 'team_sales'); + seedRule('srule_position', 'position', 'sales_rep'); + seedRule('srule_queue', 'queue', 'q_triage'); + + await engine.update('sys_business_unit', { id: 'bu_west', parent_business_unit_id: null }); + await ruleRegrantQueue.whenIdle(); + + // Not "recomputed more cheaply" — not recomputed AT ALL. A fix that + // reconciled everything on every BU write would pass the security pin + // above and wreck the system. + expect(revokeSpy).not.toHaveBeenCalled(); + expect(evaluateSpy).not.toHaveBeenCalled(); + }); + + it('recomputes exactly the BU-tree rules when both kinds are present', async () => { + seedRule('srule_user', 'user', 'priya'); + seedRule('srule_bu', 'unit_and_subordinates', 'bu_root'); + + await engine.update('sys_business_unit', { id: 'bu_west', parent_business_unit_id: null }); + await ruleRegrantQueue.whenIdle(); + + expect(revokeSpy.mock.calls.map((c) => c[0].id)).toEqual(['srule_bu']); + expect(evaluateSpy.mock.calls.map((c) => c[0])).toEqual(['srule_bu']); + }); + + it('an INACTIVE BU-tree rule is not woken by a BU write (that axis is #4433s)', async () => { + engine.seed('sys_sharing_rule', [{ + id: 'srule_off', organization_id: null, name: 'srule_off', label: 'off', + object_name: 'showcase_inquiry', criteria_json: JSON.stringify({ status: 'new' }), + recipient_type: 'unit_and_subordinates', recipient_id: 'bu_root', + access_level: 'read', active: false, + }]); + await engine.update('sys_business_unit', { id: 'bu_west', parent_business_unit_id: null }); + await ruleRegrantQueue.whenIdle(); + expect(revokeSpy).not.toHaveBeenCalled(); + }); + + it('a BU update that cannot move the expansion (a rename) skips the recompute entirely', async () => { + seedRule('srule_bu', 'unit_and_subordinates', 'bu_root'); + await engine.update('sys_business_unit', { id: 'bu_west', name: 'Western Region' }); + await ruleRegrantQueue.whenIdle(); + expect(revokeSpy).not.toHaveBeenCalled(); + expect(evaluateSpy).not.toHaveBeenCalled(); + }); + + it('an `is_primary` flip on a member row skips it too — that column drives the projection, not sharing', async () => { + seedRule('srule_bu', 'unit_and_subordinates', 'bu_root'); + // The row must EXIST, or the hook never dispatches and this passes for the + // wrong reason — a phantom check of the narrowing rather than a check. + engine.seed('sys_business_unit_member', [ + { id: 'bum_x', business_unit_id: 'bu_west', user_id: 'priya', is_primary: true }, + ]); + await engine.update('sys_business_unit_member', { id: 'bum_x', is_primary: false }); + await ruleRegrantQueue.whenIdle(); + expect(revokeSpy).not.toHaveBeenCalled(); + }); +}); + +describe('#7729 binding + recipient-kind audit', () => { + it('binds afterInsert/afterUpdate/afterDelete on BOTH business-unit tables', () => { + const engine = makeEngine(); + bindBusinessUnitTreeRecompute(engine as any, { + listRules: async () => [], + revokeRuleGrantsForRetiredRecipients: async () => 0, + evaluateRule: async () => undefined, + }, undefined); + + const bound = engine.boundFor(BU_TREE_RECOMPUTE_PACKAGE); + expect(bound).toHaveLength(6); + for (const object of BU_GRAPH_OBJECTS) { + expect(bound.filter((h) => h.options.object === object).map((h) => h.event).sort()) + .toEqual(['afterDelete', 'afterInsert', 'afterUpdate']); + } + expect(unbindBusinessUnitTreeRecompute(engine as any)).toBe(6); + expect(engine.boundFor(BU_TREE_RECOMPUTE_PACKAGE)).toHaveLength(0); + }); + + it('binds under its OWN package id so a rule rebind cannot tear it down', () => { + const engine = makeEngine(); + bindBusinessUnitTreeRecompute(engine as any, { + listRules: async () => [], + revokeRuleGrantsForRetiredRecipients: async () => 0, + evaluateRule: async () => undefined, + }, undefined); + // What `unbindAllRuleHooks` removes. + engine.unregisterHooksByPackage('plugin-sharing:rules'); + expect(engine.boundFor(BU_TREE_RECOMPUTE_PACKAGE)).toHaveLength(6); + }); + + it( + 'pins the audited recipient kinds — exactly the two whose expansion reads the BU graph', + () => { + // Measured against `SharingRuleService.expandRecipient`: `user` is a + // literal id, `team` reads sys_team_member/sys_member/sys_user, + // `position` reads sys_user_position/sys_member, `queue` returns []. + expect([...BU_TREE_RECIPIENT_TYPES].sort()).toEqual(['business_unit', 'unit_and_subordinates']); + }, + ); + + it('treats an unreadable update payload as "might have changed" rather than "did not"', () => { + expect(writeCanChangeExpansion('sys_business_unit', 'afterUpdate', {})).toBe(true); + expect(writeCanChangeExpansion('sys_business_unit', 'afterUpdate', { input: { data: 'nope' } })).toBe(true); + expect(writeCanChangeExpansion('sys_business_unit', 'afterInsert', { input: { data: { name: 'x' } } })).toBe(true); + expect(writeCanChangeExpansion('sys_business_unit', 'afterDelete', { input: { data: { name: 'x' } } })).toBe(true); + expect(writeCanChangeExpansion('sys_business_unit', 'afterUpdate', { input: { data: { name: 'x' } } })).toBe(false); + expect( + writeCanChangeExpansion('sys_business_unit', 'afterUpdate', { input: { data: { parent_business_unit_id: 'b' } } }), + ).toBe(true); + expect( + writeCanChangeExpansion('sys_business_unit_member', 'afterUpdate', { input: { data: { user_id: 'u' } } }), + ).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts new file mode 100644 index 0000000000..c346990fc5 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts @@ -0,0 +1,318 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7729] Recompute sharing-rule grants when the BUSINESS-UNIT GRAPH moves. + * + * ## The defect + * + * `bindRuleHooks` registers its recompute hooks scoped to each rule's own + * `object_name`, and nothing anywhere registered one on `sys_business_unit` or + * `sys_business_unit_member`. So the materialised `sys_record_share` grants of + * a rule whose recipient resolves through the BU tree only moved when the + * SHARED RECORD was next written — a re-parent or a membership edit moved + * nothing at all. + * + * The measured matrix on the QA run this was extracted from: + * `inSubtree=1`, `reparentedOut(no touch)=1`, `reparentedOut(touched)=0`, + * `restored(no touch)=0`, `restored(touched)=1`. Read the second column: after + * a business unit is moved OUT of a shared subtree its members KEEP read + * access, and keep it until someone happens to write the shared record. That + * is a revocation with no bound in time, which is the security half of this + * issue; the grant direction (`restored(no touch)=0`) is the availability half + * and moves the same way here. + * + * ## Which recipient kinds this covers, measured rather than assumed + * + * `SharingRuleService.expandRecipient` is the one switch that decides, and it + * reads the BU tree for TWO of the six kinds: + * + * | recipient_type | resolver | reads the BU tree | + * |-------------------------|---------------------------------------------|---| + * | `user` | the literal id | no | + * | `team` | `TeamGraphService` (`sys_team_member`, `sys_member`, `sys_user`) | no | + * | `business_unit` | `BusinessUnitGraphService.expandUsers` | 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.) + * + * Everything else is deliberately NOT recomputed. That exclusion is a + * requirement, not an optimisation: a fix that recomputed every rule on every + * BU write would close the security hole and replace it with a load problem — + * see {@link BU_TREE_RECIPIENT_TYPES}. + * + * ## The design: synchronous revoke, asynchronous re-grant + * + * This is the #4779 ruling applied on a different axis, deliberately reusing + * its seam rather than inventing a second one — the safety half runs to + * completion before the write returns, the expensive restoration half is + * queued on the shared {@link ruleRegrantQueue}: + * + * - **Revoke (synchronous, complete).** + * `revokeRuleGrantsForRetiredRecipients` re-expands the rule's recipients + * and deletes the grants of everyone who dropped out, set-based. Cost is + * one grant query + one subtree walk + one member query + a chunked delete + * PER AFFECTED RULE — no record scan, so it does not grow with how many + * records the rule matches. A BU moved out of a shared subtree has lost its + * access by the time the re-parent returns: no window. + * - **Re-grant (asynchronous, coalesced).** A BU moved INTO a shared subtree + * needs new grants for up to (matched records × new members) pairs, which + * is exactly the fan-out that must not sit on a write path. It is queued as + * a full `evaluateRule` pass, serialized behind every other queued + * re-grant. Under-granting for the length of that queue is the wobble the + * ruling already accepts, and `kernel:bootstrapped`'s backfill repairs a + * re-grant lost to a crash. + * + * ## What this costs on a bulk BU write, stated rather than hidden + * + * A predicate write over the BU tree dispatches these hooks once per matched + * row, and the synchronous half runs on EVERY dispatch — it is not memoised + * per write, and must not be. Memoising it would mean judging the whole batch + * from the tree as it looked at the first row, which re-opens the hole for + * rows 2..N (their units would still read as in-subtree). The revoke is + * idempotent and converges, so the repeats are wasted work rather than wrong + * work, and each repeat is bounded by the per-rule cost above with no record + * scan in it. The `granted.size === 0` short-circuit inside the service + * absorbs the common case (nothing granted yet → nothing to walk). The + * asynchronous half IS coalesced per rule, so a thousand-row import collapses + * into one re-grant pass per rule instead of a thousand. + */ + +import type { SharingRuleRow, SharingRuleRecipientType } from '@objectstack/spec/contracts'; +import { ruleRegrantQueue } from './rule-hooks.js'; + +const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; + +/** + * Package id for the BU-graph recompute hooks. + * + * Distinct from `SHARING_RULE_HOOK_PACKAGE` for the same reason + * `RULE_REBIND_TRIGGER_PACKAGE` is: `unbindAllRuleHooks` runs on every rule + * rebind, and these hooks must survive it. They carry no rule snapshot to go + * stale — the handler reads `listRules()` live — so there is nothing for a + * rebind to refresh here. + */ +export const BU_TREE_RECOMPUTE_PACKAGE = 'plugin-sharing:bu-tree-recompute'; + +/** + * The recipient kinds whose expansion reads the business-unit graph. + * + * This set is the non-regression guarantee. A rule recipient that never reads + * the BU tree (`user` / `team` / `position` / `queue`) is not recomputed by a + * BU write at all — not more cheaply, not at all — so a deployment whose rules + * are all `user`-recipient pays one `sys_sharing_rule` read per BU write and + * nothing else. + */ +export const BU_TREE_RECIPIENT_TYPES: ReadonlySet = new Set([ + 'business_unit', + 'unit_and_subordinates', +]); + +/** The two tables whose writes can move a BU-tree recipient expansion. */ +export const BU_GRAPH_OBJECTS = ['sys_business_unit', 'sys_business_unit_member'] as const; + +/** + * Columns of `sys_business_unit` that `BusinessUnitGraphService.expandUsers` + * actually reads: `parent_business_unit_id` (the descent), `active` (a hard + * filter that stops descent and blanks an inactive seed) and + * `organization_id` (the tenant scope every BU query is predicated on). + * + * A patch touching none of them — a rename, a new `manager_user_id`, an icon — + * cannot change who the rule expands to, so it is skipped. Anything this + * cannot read confidently is treated as "might have" and recomputed. + */ +const BU_EXPANSION_FIELDS = ['parent_business_unit_id', 'active', 'organization_id'] as const; + +/** + * Columns of `sys_business_unit_member` the same expansion reads. `is_primary` + * is deliberately absent: it drives the `sys_user.primary_business_unit_id` + * projection (`primary-bu-projection.ts`, ADR-0057 D12), which no sharing-rule + * recipient expansion consults. + */ +const MEMBER_EXPANSION_FIELDS = ['business_unit_id', 'user_id'] as const; + +interface MinimalEngine { + registerHook( + event: string, + handler: (ctx: any) => any | Promise, + options?: { object?: string | string[]; priority?: number; packageId?: string }, + ): void; + unregisterHooksByPackage(packageId: string): number; +} + +interface MinimalLogger { + info?: (msg: any, ...rest: any[]) => void; + warn?: (msg: any, ...rest: any[]) => void; +} + +/** The slice of {@link SharingRuleService} this module drives. */ +export interface BuTreeRecomputeRuleService { + listRules( + filter: { object?: string; activeOnly?: boolean }, + context: any, + ): Promise; + revokeRuleGrantsForRetiredRecipients(rule: SharingRuleRow): Promise; + evaluateRule(idOrName: string, context: any): Promise; +} + +/** + * Rules with a re-grant queued and not yet started. + * + * Module-scoped for the reason {@link ruleRegrantQueue} is: the binding can be + * torn down and re-established, and a per-binding set would let that orphan + * the coalescing of re-grants still in flight. The entry is released BEFORE + * the pass runs, so a BU write that lands while a re-grant is executing queues + * a fresh pass rather than being swallowed by the one that is already past + * reading the tree. + */ +const pendingRegrants = new Set(); + +/** Test seam: are any coalesced re-grants outstanding? */ +export function pendingBuTreeRegrants(): number { + return pendingRegrants.size; +} + +/** + * Could this write have changed a BU-tree recipient expansion? + * + * Inserts and deletes always could. An update is judged on the columns its + * patch carries, and anything unreadable (a non-object payload, an absent one) + * answers `true` — "we cannot tell" must never collapse into "nothing + * changed", which is the direction that silently skips the revocation. + */ +export function writeCanChangeExpansion(objectName: string, event: string, hookCtx: any): boolean { + if (event !== 'afterUpdate') return true; + const data = hookCtx?.input?.data; + if (!data || typeof data !== 'object' || Array.isArray(data)) return true; + const fields = objectName === 'sys_business_unit' ? BU_EXPANSION_FIELDS : MEMBER_EXPANSION_FIELDS; + return fields.some((f) => Object.prototype.hasOwnProperty.call(data, f)); +} + +/** + * Bind the BU-graph recompute hooks. + * + * Six hooks: `afterInsert` / `afterUpdate` / `afterDelete` on each of + * {@link BU_GRAPH_OBJECTS}. + * + * **`after`, not `before`** — unlike the row-set stash in `bulk-recompute.ts`, + * nothing here needs the pre-write state. What the revoke needs is the tree as + * it is NOW, and the write is what makes that true. + * + * **System writes are NOT skipped**, which is the opposite of what + * `bindRuleHooks` does and is deliberate. Its skip is about grant + * MATERIALISATION, which the boot backfill re-does anyway. Here the payload is + * REVOCATION, and the realistic production trigger for a re-parent is an HRIS + * or directory sync — a system write. Skipping those would leave the hole open + * on the very path most likely to open it. `primary-bu-projection.ts` reached + * the same conclusion for the same table. + */ +export function bindBusinessUnitTreeRecompute( + engine: MinimalEngine, + service: BuTreeRecomputeRuleService, + logger?: MinimalLogger, +): void { + if (typeof engine.registerHook !== 'function') return; + if (typeof engine.unregisterHooksByPackage === 'function') { + engine.unregisterHooksByPackage(BU_TREE_RECOMPUTE_PACKAGE); + } + + /** + * Queue the re-grant half for one rule, at most one pass outstanding. + * + * Never awaited by the caller: this is the half whose cost scales with + * (matched records × recipients), and a write path must not carry it. + */ + const queueRegrant = (rule: SharingRuleRow): void => { + const ruleId = String(rule.id); + if (pendingRegrants.has(ruleId)) return; + pendingRegrants.add(ruleId); + ruleRegrantQueue.enqueue( + async () => { + // Released first: a BU write landing during this pass must be able to + // queue its own, since this one may already have read the tree. + pendingRegrants.delete(ruleId); + await service.evaluateRule(ruleId, SYSTEM_CTX as any); + }, + (err: any) => { + pendingRegrants.delete(ruleId); + logger?.warn?.( + '[sharing-rule] business-unit re-grant failed — recipients who moved INTO a shared subtree stay ' + + 'without access until the next reconcile (any sharing-rule write, or a restart); grants that ' + + 'were withdrawn stay withdrawn', + { rule: rule.name ?? ruleId, error: err?.message }, + ); + }, + ); + }; + + const onGraphWrite = (objectName: string, event: string) => async (hookCtx: any): Promise => { + try { + if (!writeCanChangeExpansion(objectName, event, hookCtx)) return; + const rules = await service.listRules({ activeOnly: true }, SYSTEM_CTX as any); + const affected = (rules ?? []).filter((r) => BU_TREE_RECIPIENT_TYPES.has(r.recipient_type)); + if (affected.length === 0) return; + for (const rule of affected) { + // Safety half — synchronous and complete. Failing one rule must not + // stop the next: a throw here would leave the remaining rules' stale + // grants in place, which is the direction this whole module exists to + // prevent. + try { + const retired = await service.revokeRuleGrantsForRetiredRecipients(rule); + if (retired > 0) { + logger?.info?.( + '[sharing-rule] business-unit graph changed — withdrew grants from recipients the rule no ' + + 'longer reaches', + { rule: rule.name ?? rule.id, object: objectName, event, recipientsRetired: retired }, + ); + } + } catch (err: any) { + logger?.warn?.( + '[sharing-rule] business-unit revocation failed — this rule may still grant access to ' + + 'recipients it no longer reaches until the next reconcile', + { rule: rule.name ?? rule.id, object: objectName, error: err?.message }, + ); + } + // Restoration half — queued, coalesced, never awaited. + queueRegrant(rule); + } + } catch (err: any) { + // A hook must not fail an operator's write. + logger?.warn?.('[sharing-rule] business-unit recompute failed', { + object: objectName, + event, + error: err?.message, + }); + } + }; + + for (const objectName of BU_GRAPH_OBJECTS) { + for (const event of ['afterInsert', 'afterUpdate', 'afterDelete']) { + engine.registerHook(event, onGraphWrite(objectName, event), { + object: objectName, + packageId: BU_TREE_RECOMPUTE_PACKAGE, + // Behind the primary-BU projection (150) so the denormalised + // `sys_user.primary_business_unit_id` is already in step, and behind + // the rule recompute's own 180 for the same reason it sits there. + priority: 190, + }); + } + } + + logger?.info?.('[sharing-rule] business-unit graph recompute hooks bound', { + objects: [...BU_GRAPH_OBJECTS], + recipientTypes: [...BU_TREE_RECIPIENT_TYPES], + }); +} + +export function unbindBusinessUnitTreeRecompute(engine: MinimalEngine): number { + if (typeof engine.unregisterHooksByPackage !== 'function') return 0; + return engine.unregisterHooksByPackage(BU_TREE_RECOMPUTE_PACKAGE); +} diff --git a/packages/plugins/plugin-sharing/src/index.ts b/packages/plugins/plugin-sharing/src/index.ts index ffc40803dd..64994c97e1 100644 --- a/packages/plugins/plugin-sharing/src/index.ts +++ b/packages/plugins/plugin-sharing/src/index.ts @@ -53,6 +53,16 @@ export { type UnboundedReason, type RecomputeEngine, } from './bulk-recompute.js'; +export { + bindBusinessUnitTreeRecompute, + unbindBusinessUnitTreeRecompute, + writeCanChangeExpansion, + pendingBuTreeRegrants, + BU_TREE_RECOMPUTE_PACKAGE, + BU_TREE_RECIPIENT_TYPES, + BU_GRAPH_OBJECTS, + type BuTreeRecomputeRuleService, +} from './bu-tree-recompute.js'; export { bindRecordShareCascade, unbindRecordShareCascade, diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index b12779396e..6cd915ef58 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -31,6 +31,7 @@ import { registerShareLinkRoutes } from './share-link-routes.js'; import { bindRuleHooks, unbindAllRuleHooks, bindRuleCriteriaGuard, RULE_REBIND_TRIGGER_PACKAGE } from './rule-hooks.js'; import { bindRuleProvenanceStamp, unbindRuleProvenanceStamp } from './sharing-rule-provenance.js'; import { bindPrimaryBuHooks, backfillPrimaryBu } from './primary-bu-projection.js'; +import { bindBusinessUnitTreeRecompute } from './bu-tree-recompute.js'; import { bindRecordShareCascade } from './record-share-cascade.js'; import { bootstrapDeclaredSharingRules } from './bootstrap-declared-sharing-rules.js'; @@ -577,6 +578,22 @@ export class SharingServicePlugin implements Plugin { bindRuleHooks(engine, this.ruleService, rules, ctx.logger as any); this.bindRuleRebindTriggers(engine, ctx); + // [#7729] A rule whose recipient resolves through the BUSINESS-UNIT + // graph changes audience when the graph moves, not when the shared + // record is written — and `bindRuleHooks` above binds only on each + // rule's own object, so a re-parent or a membership edit reached + // nothing. Left unbound, a business unit moved OUT of a shared + // subtree kept its members' read access until somebody happened to + // write the shared record: a revocation with no bound in time. + // + // Bound OUTSIDE the rebind seam on purpose. It carries no rule + // snapshot — the handler reads `listRules()` live on each BU write — + // so a runtime-authored rule is picked up without a rebind, and + // `unbindAllRuleHooks` (which every rebind calls) cannot tear it + // down. Inside `enforce`, unlike the primary-BU projection next to + // it: this one IS an access-control surface. + bindBusinessUnitTreeRecompute(engine, this.ruleService, ctx.logger as any); + // [#3896] Authoring a rule in Setup is a plain INSERT on // sys_sharing_rule — it bypasses defineRule's validation, so the // match-all criteria gate has to sit on the table itself too. diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 2f280894d8..030eb78d4a 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -540,6 +540,86 @@ export class SharingRuleService implements ISharingRuleService { } } + /** + * [#7729] Revoke this rule's grants whose RECIPIENT the rule no longer + * expands to — the recipient-axis twin of + * {@link revokeRuleGrantsForRecords}. + * + * ## Why a third revoke, and why on this axis + * + * The two revokes above are both scoped by RECORD, because the writes that + * drove them were writes to records. A business-unit re-parent or a + * membership edit touches no record at all: what changes is who + * {@link expandRecipient} resolves to, and therefore which of the rule's + * already-materialised grants have gone stale. Scoping that withdrawal by + * record would mean enumerating every record the rule matches — the very + * scan {@link RULE_RECOMPUTE_ROW_CAP} exists because we cannot afford on a + * write path. Scoping it by recipient needs no record scan at all: one query + * for the rule's granted recipients, one recipient expansion, and a + * chunked set-based delete of the difference. + * + * ## Cheap enough to be the SYNCHRONOUS half + * + * This is the safety half of the same split the #4779 ruling settled + * (over-granting is a security incident, under-granting is an availability + * wobble): complete and synchronous on the write path, with the expensive + * re-grant deferred to {@link evaluateRule} on the shared re-grant queue. + * A BU moved OUT of a shared subtree therefore loses its members' access + * before the write returns, rather than at the shared record's next write — + * which was unbounded in time, and is what #7729 was filed for. + * + * The `granted.size === 0` short-circuit is load-bearing, not an + * optimisation: it is what keeps boot-time BU seeding (thousands of member + * inserts against an empty `sys_record_share`) from paying a subtree walk + * per row. + * + * An INACTIVE rule expands to nobody, so every grant it still holds is + * stale — the same verdict {@link evaluateRule} reaches by a longer road + * (#4433), reached here without one. + * + * Deletes set-based rather than through `SharingService.revoke`, following + * {@link revokeRuleGrantsForObject} / {@link revokeRuleGrantsForRecords}: + * under a system context `revoke` is itself a scalar-id delete with no + * event and no audit trail, so per-row revocation would buy nothing and + * cost one statement per grant on a path whose whole justification is that + * it stays cheap. Chunked at 200 for the same `$in` portability reason. + * + * @returns how many RECIPIENTS were retired (not how many rows went). + */ + async revokeRuleGrantsForRetiredRecipients(rule: SharingRuleRow): Promise { + if (!rule?.id) return 0; + const existing = await this.engine.find('sys_record_share', { + where: { source: 'rule', source_id: rule.id }, + fields: ['id', 'recipient_id'], + limit: 100000, + context: SYSTEM_CTX, + }); + const granted = new Set(); + for (const row of (existing ?? [])) { + const rid = (row as any).recipient_id; + if (rid != null && rid !== '') granted.add(String(rid)); + } + if (granted.size === 0) return 0; + + const desired = rule.active ? new Set(await this.expandRecipient(rule)) : new Set(); + const stale = [...granted].filter((recipientId) => !desired.has(recipientId)); + if (stale.length === 0) return 0; + + const CHUNK = 200; + for (let i = 0; i < stale.length; i += CHUNK) { + await this.engine.delete('sys_record_share', { + where: { + source: 'rule', + source_id: rule.id, + recipient_id: { $in: stale.slice(i, i + CHUNK) }, + }, + multi: true, + context: SYSTEM_CTX, + } as any); + } + return stale.length; + } + // ── internals ───────────────────────────────────────────────────── /**