From 6c8c3ff09ed27a2ec9114d7236a1bfd1397309d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 12:57:19 +0000 Subject: [PATCH 1/6] fix(plugin-sharing): recompute BU-tree sharing-rule grants on business-unit graph writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bindRuleHooks` registers recompute hooks scoped to each rule's own `object_name` and nothing bound one on `sys_business_unit` or `sys_business_unit_member`, so a rule whose recipient resolves through the business-unit graph only moved its materialised `sys_record_share` grants when the shared RECORD was next written. A business unit moved OUT of a shared subtree therefore kept its members' read access until that happened — a revocation with no bound in time. Adds `bu-tree-recompute.ts`, binding afterInsert/afterUpdate/afterDelete on both BU tables, and `revokeRuleGrantsForRetiredRecipients` on `SharingRuleService` — the recipient-axis twin of the existing record-axis revokes. Applies the #4779 split on the new axis and reuses its queue: the revoke is synchronous and complete (no record scan; cost does not grow with how many records a rule matches), the re-grant is queued on the shared `ruleRegrantQueue`, coalesced per rule. Co-Authored-By: Claude --- .../plugin-sharing/src/bu-tree-recompute.ts | 318 ++++++++++++++++++ packages/plugins/plugin-sharing/src/index.ts | 10 + .../plugin-sharing/src/sharing-plugin.ts | 17 + .../src/sharing-rule-service.ts | 80 +++++ 4 files changed, 425 insertions(+) create mode 100644 packages/plugins/plugin-sharing/src/bu-tree-recompute.ts 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 3a3e2d19a8..8282809c2d 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -487,6 +487,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 ───────────────────────────────────────────────────── /** From 95d506f42787c2f2293d346b17ae0f1a972a9306 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 13:22:42 +0000 Subject: [PATCH 2/6] test(plugin-sharing): pin the #7729 BU-graph revocation matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduces the QA matrix end to end over an in-memory engine that dispatches hooks on write: inSubtree grants, re-parent OUT revokes, re-parent back IN re-grants, membership add/remove moves the same way — every `(no touch)` cell asserted against a write log proving the shared record was never touched. The security pin blocks the asynchronous re-grant queue with a gate before re-parenting, so a grant that is gone while the gate is shut can only have been withdrawn on the write path. Non-regression: a `user` / `team` / `position` / `queue` rule is not recomputed AT ALL by a BU write, an inactive BU-tree rule is not woken, and a BU rename or an `is_primary` flip skips the recompute entirely. Co-Authored-By: Claude --- .../src/bu-tree-recompute.test.ts | 446 ++++++++++++++++++ 1 file changed, 446 insertions(+) create mode 100644 packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts 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..0b3a06d0d9 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -0,0 +1,446 @@ +// 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 } from 'vitest'; +import { assertEngineDeleteDispatch } 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; + if (Array.isArray(f.$or)) return f.$or.some((x: any) => matches(row, x)); + if (Array.isArray(f.$and)) return f.$and.every((x: any) => matches(row, x)); + 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; + }, + async update(o: string, idOrData: any, dataOrOpts?: any) { + const data = typeof idOrData === 'object' ? idOrData : dataOrOpts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const t = ensure(o); + const i = t.findIndex((r) => r.id === id); + if (i >= 0) t[i] = { ...t[i], ...data }; + writes.push({ op: 'update', object: o }); + await dispatch('afterUpdate', o, { result: t[i], input: { id, data } }); + return t[i]; + }, + 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 + }); + + afterEach(async () => { + // 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. + let release!: () => void; + const gate = new Promise((r) => { release = 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 + + release(); + 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; + let revokeSpy: ReturnType; + let evaluateSpy: ReturnType; + + 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] as any).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'); + 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); + }); +}); From 5543ef7699807ceb33b57bc4d21c34debf2f1af4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:45:24 +0000 Subject: [PATCH 3/6] test(plugin-sharing): pin the #7729 fake engine to ObjectQL's update dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:engine-double-contract` caught the new double's `update()` accepting call shapes `ObjectQL.update` refuses. Routes it through the producer's own `assertEngineUpdateDispatch` alongside the delete half, and seeds the row the `is_primary` narrowing case operates on — under the corrected dispatch a write to a non-existent id fires no hook at all, so that case would have passed without testing the narrowing. Also releases the re-grant queue gate from `afterEach` rather than inline. The queue is module-scoped and shared by the file: a gate left shut by a FAILING assertion blocked the chain and turned one clear failure into 15 timeouts, measured during this fix's own ablation. Adds the changeset. Co-Authored-By: Claude --- .changeset/bu-tree-share-recompute.md | 11 +++++ .../src/bu-tree-recompute.test.ts | 49 ++++++++++++++----- 2 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 .changeset/bu-tree-share-recompute.md 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/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts index 0b3a06d0d9..ebd9d50682 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -24,7 +24,7 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +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'; @@ -118,15 +118,22 @@ function makeEngine() { await dispatch('afterInsert', o, { result: row, input: { data } }); return row; }, - async update(o: string, idOrData: any, dataOrOpts?: any) { - const data = typeof idOrData === 'object' ? idOrData : dataOrOpts; - const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + // 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 i = t.findIndex((r) => r.id === id); - if (i >= 0) t[i] = { ...t[i], ...data }; + 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 }); - await dispatch('afterUpdate', o, { result: t[i], input: { id, data } }); - return t[i]; + 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); @@ -199,7 +206,22 @@ describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () 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(); }); @@ -216,8 +238,7 @@ describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () // 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. - let release!: () => void; - const gate = new Promise((r) => { release = r; }); + const gate = new Promise((r) => { releaseGate = r; }); ruleRegrantQueue.enqueue(() => gate); await engine.update('sys_business_unit', { @@ -228,7 +249,8 @@ describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () expect(grantsFor('priya')).toBe(0); // was 1 before this fix expect(sharedRecordWrites()).toBe(0); // …and nobody touched the record - release(); + releaseGate!(); + releaseGate = null; await ruleRegrantQueue.whenIdle(); expect(grantsFor('priya')).toBe(0); // still 0 once the queue drains }, @@ -383,6 +405,11 @@ describe('#7729 non-regression — rules that do not read the BU tree are left a 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(); From 060617d12d75ba62dcdbeafec909b99ee1abb400 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:25:26 +0000 Subject: [PATCH 4/6] test(plugin-sharing): conjoin the new fake's $or with its sibling field keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the looseness #7760 removed from `sharing-rule.test.ts`'s fake one commit earlier: the matcher returned on `$or` alone and dropped every sibling key, so `listRules`'s `{object_name, active, $or:[…org scope…]}` would have matched the whole table here while driver-sql and driver-memory conjoin them. Dormant in this file today (its reads all run under a system context, which carries no org and so composes no `$or`), fixed anyway — a double looser than the contract it stands in for is how a green suite ships a broken filter. Co-Authored-By: Claude --- .../plugin-sharing/src/bu-tree-recompute.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 ebd9d50682..ec82a2b215 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -47,8 +47,14 @@ const RULE = 'share_new_inquiries_with_field_ops'; function matches(row: Row, f: any): boolean { if (!f || typeof f !== 'object') return true; - if (Array.isArray(f.$or)) return f.$or.some((x: any) => matches(row, x)); - if (Array.isArray(f.$and)) return f.$and.every((x: any) => matches(row, x)); + // [#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]; From 8f5b14bd95ff3142fa1d3003af2bc79c235fa4f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:31:34 +0000 Subject: [PATCH 5/6] docs(permissions): add the business-unit write as the fourth withdrawal moment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sharing-rules.mdx` enumerated withdrawal as happening at exactly THREE moments, in a definitive table. This change adds a fourth — a `sys_business_unit` / `sys_business_unit_member` write — so an author reasoning about revocation timing from that table was reading an incomplete list, on the one topic the change is about. The new row states the asymmetry rather than flattening it: recipients the rule no longer reaches are revoked before the write returns, while the grant direction is queued and coalesced per rule. It also scopes itself to the two recipient kinds whose expansion reads the BU graph, so nobody infers that every rule now recomputes on every business-unit write. Deliberately unchanged, both checked rather than assumed: - The "always recoverable from the API surface" paragraph below the table. Its subject is an over-granting RULE recovered by switching it off or deleting it — the rule-write axis, which this change does not touch — so it stays exactly true, and its "not on the next time somebody happens to touch the record" phrasing is the same line the new row echoes. - The recipient table's `business_unit` row ("exactly that business unit (no subtree)"). That states the DECLARED contract, matching the spec enum and the ADR-0105 lint red-line. The runtime diverges by walking the subtree for both BU kinds, which is filed as #7807 — rewriting the doc to match would document an over-grant as intended behaviour. The new row is consistent with it either way: a unit-only expansion still reads `sys_business_unit` for its own active flag and `sys_business_unit_member` for its members, so it needs the recompute under both readings. `sharing-service.mdx` says nothing about recipient kinds or the BU graph; absent is not wrong, so it is left alone. Co-Authored-By: Claude --- content/docs/permissions/sharing-rules.mdx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 From 189635e07994b69392a13c5ec77153030dc760fd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 16:24:45 +0000 Subject: [PATCH 6/6] test(plugin-sharing): type the #7729 spies from the service methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:type-check-debt` went red: plugin-sharing's TEST_DEBT records 3 raw tsc errors and the new guard took it to 5. src/bu-tree-recompute.test.ts(388,38): error TS7006: Parameter 'c' implicitly has an 'any' type. src/bu-tree-recompute.test.ts(389,40): error TS7006: Parameter 'c' implicitly has an 'any' type. Cause: the spies were declared `ReturnType`, the unparameterised spelling, which erases the signature — so `mock.calls` degrades to an implicit `any` per element. Declaring them as `MockInstance` derives the real types, which fixes both errors and lets the `as any` cast on the first assertion go: `c[0].id` is now the checked argument type rather than an unchecked cast. The ledger entry is NOT raised. It is a shrink-only ratchet (#5278) and these errors are hours old; the measured count is back to exactly 3, and its composition again matches the entry's note verbatim (TS6133 x2, TS18048 x1). Why local verification missed it: this package's tsconfig excludes `**/*.test.ts`, so `pnpm --filter @objectstack/plugin-sharing typecheck` never compiles its own tests — only `check:type-check-debt` measures that layer. Co-Authored-By: Claude --- .../plugin-sharing/src/bu-tree-recompute.test.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) 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 ec82a2b215..b7cfd275e4 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.test.ts @@ -23,7 +23,7 @@ * record was never touched. */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +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'; @@ -333,8 +333,15 @@ describe('#7729 business-unit graph writes recompute BU-tree sharing rules', () describe('#7729 non-regression — rules that do not read the BU tree are left alone', () => { let engine: ReturnType; let rules: SharingRuleService; - let revokeSpy: ReturnType; - let evaluateSpy: ReturnType; + // 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(); @@ -385,7 +392,7 @@ describe('#7729 non-regression — rules that do not read the BU tree are left a 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] as any).id)).toEqual(['srule_bu']); + expect(revokeSpy.mock.calls.map((c) => c[0].id)).toEqual(['srule_bu']); expect(evaluateSpy.mock.calls.map((c) => c[0])).toEqual(['srule_bu']); });