From 40db293567504adb8b90f24a3634086c3713d8a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 04:33:00 +0000 Subject: [PATCH] fix(plugin-sharing): recompute sharing rules for predicate (multi) writes (#4779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bindRuleHooks` located the rows to recompute from a single record id (`if (!id) return`), and `ObjectQL.update()` only populates `input.id` for a scalar `where.id`. A predicate write routes to `updateMany` and carries no id, so every bulk write skipped sharing-rule recompute entirely: records bulk-moved out of a rule's criteria kept the `sys_record_share` rows the rule had issued, and their recipients kept access the rules no longer implied. Fail-open on the authorization side; same family as #4757 and #4778. Keyed off the write's ROW SET instead of one id. `beforeUpdate`/`beforeDelete` resolve the affected rows from the predicate and stash them on the shared hook context (the before hook is where it must happen — the write is what makes those rows unfindable); the after hook acts on them. Per the maintainer's ruling (option C): - bounded set (<= RULE_RECOMPUTE_ROW_CAP = 1000) -> per-row `evaluateAllForRecord`, synchronous, diff-based so both directions are covered (out of the criteria revokes, into it grants); - unbounded set (over cap / `multi` with no `where` / failed resolve) -> synchronous set-based revoke of the object's rule grants, then asynchronous re-grant via `evaluateAllRulesForObject`. The write is never refused: that would leak an internal recompute bound out as a business limit on how many rows an admin may update. The asymmetry it trades on is that over-granting is a security incident while under-granting is an availability wobble, so the safety half is always synchronous and complete and only the expensive restoration half is deferred. The re-grant is in-process rather than routed through the OPTIONAL `IJobService`, which would make the guarantee composition-dependent; durability comes from the plugin's existing `kernel:bootstrapped` backfill, which re-runs the same idempotent reconcile. Also binds `afterDelete` and retires the deleted records' rule grants (the orphan noted at the tail of the issue). Nothing else could reach them: `evaluateRule` iterates records that still exist, so a grant whose record is gone outlived every reconcile path and every restart. New on SharingRuleService: revokeRuleGrantsForObject, revokeRuleGrantsForRecords, evaluateAllRulesForObject. Manual shares are never touched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t --- .../sharing-rule-bulk-write-recompute.md | 65 +++ .../plugin-sharing/src/bulk-recompute.test.ts | 513 ++++++++++++++++++ .../plugin-sharing/src/bulk-recompute.ts | 258 +++++++++ packages/plugins/plugin-sharing/src/index.ts | 10 + .../plugins/plugin-sharing/src/rule-hooks.ts | 160 +++++- .../plugin-sharing/src/rule-rebind.test.ts | 21 +- .../src/sharing-rule-service.ts | 89 +++ 7 files changed, 1103 insertions(+), 13 deletions(-) create mode 100644 .changeset/sharing-rule-bulk-write-recompute.md create mode 100644 packages/plugins/plugin-sharing/src/bulk-recompute.test.ts create mode 100644 packages/plugins/plugin-sharing/src/bulk-recompute.ts diff --git a/.changeset/sharing-rule-bulk-write-recompute.md b/.changeset/sharing-rule-bulk-write-recompute.md new file mode 100644 index 0000000000..4bb00c50b6 --- /dev/null +++ b/.changeset/sharing-rule-bulk-write-recompute.md @@ -0,0 +1,65 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): recompute sharing rules for predicate (`multi`) writes — stale `sys_record_share` grants no longer survive a bulk update (#4779) + +`bindRuleHooks` located the rows to recompute from a single record id: + +```ts +const id = String(data?.id ?? ctx?.input?.id ?? ''); +if (!id) return; +``` + +`ObjectQL.update()` only populates `input.id` when `where.id` is a scalar. A +predicate write (`multi: true`) routes to `updateMany`, leaves `input.id` +undefined, and carries no id in its payload — so **every bulk write skipped +sharing-rule recompute entirely**. + +The consequence is a fail-open on the authorization side. A criteria-based rule +materialises `sys_record_share` rows; an admin then bulk-updates those records +out of the criteria (`{ where: { region: 'east' }, multi: true, data: { region: +'west' } }`); nothing recomputes, the grant rows stay in the table, and the +recipients keep the read/edit access the rule no longer implies. Same family as +#4757 (`sys_attachment`) and #4778 (approval locks), but better hidden — a stale +grant is indistinguishable from a legitimate one. The reverse direction (bulk +update **into** a rule's criteria never granting) was broken too. + +**What changes** + +The hooks now key off the write's ROW SET instead of one id. `beforeUpdate` / +`beforeDelete` resolve the affected rows from the predicate and stash them on +the shared hook context (the `before` hook is where it must happen — the write +is what makes those rows unfindable); the `after` hook acts on them: + +- **Bounded set (≤ 1000 rows, `RULE_RECOMPUTE_ROW_CAP`)** — `evaluateAllForRecord` + per row, synchronously. Diff-based, so this covers both directions: rows moved + out of a rule's criteria are revoked, rows moved in are granted. +- **Unbounded set** (over the cap, `multi: true` with no `where` at all, or a + resolve that failed) — every `source: 'rule'` grant on the object is revoked + **synchronously** in one set-based statement, and the deserved grants are + restored **asynchronously** by reconciling the object's rules. + +**The write is never refused.** Refusing would turn an internal recompute bound +into a business-visible limit on how many rows an admin may update, reported by +a subsystem they never configured. The asymmetry it trades on instead: +over-granting is a security incident, under-granting is an availability wobble. +So the safety half is always synchronous and complete, and only the expensive +restoration half is deferred. + +**Operational note.** After a bulk write whose row set could not be bounded, +recipients may briefly lose access to records they still qualify for, until the +background re-grant finishes. It is logged with the object and the reason. The +re-grant is in-process; if it is lost to a crash, the plugin's existing +`kernel:bootstrapped` backfill re-runs the same idempotent reconcile on the next +start, and any subsequent `sys_sharing_rule` write reconciles too. + +**Also fixed:** the rule hooks now bind `afterDelete` and retire the deleted +records' rule grants. Nothing else could: `evaluateRule` iterates records that +still exist, so a grant whose record is gone was unreachable by every reconcile +path and outlived restarts. Harmless only while record ids are never reused — +an assumption nothing in the platform enforces. + +New on `SharingRuleService`: `revokeRuleGrantsForObject`, +`revokeRuleGrantsForRecords` and `evaluateAllRulesForObject`. Manual +(`source: 'manual'`) shares are never touched by any of them. diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts new file mode 100644 index 0000000000..23b2e8aa6d --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts @@ -0,0 +1,513 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4779] Sharing-rule recompute for predicate (`multi: true`) writes. + * + * The hooks used to open with `if (!id) return`, and `ObjectQL.update()` only + * populates `input.id` for a scalar `where.id`. So a bulk write recomputed + * nothing: records bulk-updated OUT of a sharing rule's criteria kept every + * `sys_record_share` row the rule had issued, and the recipients kept read and + * edit access the rules no longer implied — a fail-open on the authorization + * side (same family as #4757 / #4778). + * + * Maintainer ruling (2026-08-04), implemented here as option C: + * + * - bounded row set (≤ `RULE_RECOMPUTE_ROW_CAP`) → per-row recompute, + * synchronous, both directions; + * - unbounded row set → synchronous set-based revoke of the object's rule + * grants + asynchronous re-grant. The write is never refused. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; +import { + bindRuleHooks, + unbindAllRuleHooks, + ruleRegrantQueue, + SHARING_RULE_HOOK_PACKAGE, +} from './rule-hooks.js'; +import { + RULE_RECOMPUTE_ROW_CAP, + RuleRegrantQueue, + resolveAffectedRows, + idsFromHookInput, +} from './bulk-recompute.js'; + +interface Row { [k: string]: any } + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** A non-system session — the hooks deliberately skip `isSystem` writes. */ +const ADMIN_SESSION = { isSystem: false, userId: 'admin' }; + +type HookEntry = { event: string; handler: (ctx: any) => any; options: Row }; + +/** + * A fake ObjectQL engine that also reproduces the part of the write pipeline + * this fix depends on: `before*` and `after*` hooks of one write share ONE + * `HookContext` instance (the real engine mutates `ctx.event` in place rather + * than building a second context), and a predicate update leaves `input.id` + * undefined. + */ +function makeEngine() { + const tables: Record = {}; + const hooks: HookEntry[] = []; + const ensure = (n: string) => (tables[n] ??= []); + /** Every `delete` call this engine saw, for asserting set-based revokes. */ + const deleteCalls: Array<{ object: string; options: any }> = []; + + 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' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (v != null && typeof v === 'object' && '$ne' in (v as any)) { + if (rv === (v as any).$ne) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + + const engine = { + _tables: tables, + _deleteCalls: deleteCalls, + /** Set to make the predicate resolve throw (the 'resolve-failed' branch). */ + failFindOn: null as string | null, + getSchema() { return undefined; }, + async find(o: string, opts?: any) { + if (engine.failFindOn === o) throw new Error(`boom: ${o} unavailable`); + 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); 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 }; + return t[i]; + }, + async delete(o: string, opts?: any) { + // Pinned to `ObjectQLEngine.delete`'s own dispatch predicate (#4434, + // #4550): a predicate-shaped delete without `multi: true` is the one + // shape a real server answers 500 to. The set-based revokes this issue + // adds are exactly that shape, so a fake that accepted them would prove + // nothing about production. + assertEngineDeleteDispatch(opts); + deleteCalls.push({ object: o, options: 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); + return { ok: true }; + }, + 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); }, + + async fire(event: string, object: string, ctx: any) { + for (const h of [...hooks]) { + if (h.event === event && h.options.object === object) await h.handler(ctx); + } + }, + + /** + * A predicate update, run the way the engine runs one: seed a hook context + * with NO `input.id` (the #4779 precondition), fire `beforeUpdate`, mutate + * the matched rows, then fire `afterUpdate` on the SAME context object. + */ + async simulateBulkUpdate(object: string, where: any, data: Row, session: any = ADMIN_SESSION) { + const ctx: any = { + object, + event: 'beforeUpdate', + input: { id: undefined, data, options: { where, multi: true } }, + session, + }; + await engine.fire('beforeUpdate', object, ctx); + const t = ensure(object); + let affected = 0; + for (let i = 0; i < t.length; i++) { + if (where != null && !matches(t[i], where)) continue; + t[i] = { ...t[i], ...data }; + affected++; + } + ctx.event = 'afterUpdate'; + ctx.result = affected; + await engine.fire('afterUpdate', object, ctx); + return affected; + }, + + /** A single-id update — `input.id` is populated, as the engine does. */ + async simulateUpdateById(object: string, id: string, data: Row, session: any = ADMIN_SESSION) { + const ctx: any = { + object, + event: 'beforeUpdate', + input: { id, data, options: { where: { id } } }, + session, + }; + await engine.fire('beforeUpdate', object, ctx); + const t = ensure(object); + const i = t.findIndex((r) => r.id === id); + if (i >= 0) t[i] = { ...t[i], ...data }; + ctx.event = 'afterUpdate'; + ctx.result = t[i]; + await engine.fire('afterUpdate', object, ctx); + }, + + async simulateBulkDelete(object: string, where: any, session: any = ADMIN_SESSION) { + const ctx: any = { + object, + event: 'beforeDelete', + input: { id: undefined, options: { where, multi: true } }, + session, + }; + await engine.fire('beforeDelete', object, ctx); + const t = ensure(object); + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + ctx.event = 'afterDelete'; + await engine.fire('afterDelete', object, ctx); + }, + }; + return engine; +} + +type Engine = ReturnType; + +/** `sys_record_share` rows this rule has materialised, by record. */ +function ruleShares(engine: Engine): Array<{ record_id: string; recipient_id: string }> { + return (engine._tables.sys_record_share ?? []) + .filter((r) => r.source === 'rule') + .map((r) => ({ record_id: String(r.record_id), recipient_id: String(r.recipient_id) })); +} + +function makeStack(engine: Engine, logger: any) { + const sharing = new SharingService({ engine: engine as any }); + const rules = new SharingRuleService({ engine: engine as any, sharing, logger }); + return { sharing, rules }; +} + +describe('#4779 predicate (multi) writes recompute sharing rules', () => { + let engine: Engine; + let rules: SharingRuleService; + let logger: any; + + /** Seed `count` opportunity rows in `region`, all owned by `boss`. */ + const seed = (count: number, region: string, from = 0) => { + for (let i = from; i < from + count; i++) { + engine._tables.opportunity.push({ id: `opp${i}`, region, owner_id: 'boss', stage: 'open' }); + } + }; + + beforeEach(async () => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + engine._tables.opportunity = []; + engine._tables.sys_record_share = []; + engine._tables.sys_sharing_rule = [{ + id: 'srule_east', + name: 'east_to_alice', + label: 'East → Alice', + object_name: 'opportunity', + criteria_json: JSON.stringify({ region: 'east' }), + recipient_type: 'user', + recipient_id: 'alice', + access_level: 'edit', + active: true, + }]; + ({ rules } = makeStack(engine, logger)); + const ruleRows = await rules.listRules({ activeOnly: true }, SYS); + bindRuleHooks(engine as any, rules, ruleRows, logger); + }); + + it('binds the before/after pair a row-set recompute needs', () => { + const bound = engine.boundFor(SHARING_RULE_HOOK_PACKAGE).map((h) => h.event).sort(); + expect(bound).toEqual([ + 'afterDelete', 'afterInsert', 'afterUpdate', 'beforeDelete', 'beforeUpdate', + ]); + }); + + /** + * THE REPRO. Revert-proof: with `if (!id) return` restored, `evaluateAllForRecord` + * is never called for a predicate write, so the two grants survive and the + * final expectation reads `2`, not `0`. + */ + it('revokes grants when a bulk update moves records OUT of the criteria', async () => { + seed(2, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(2); + + await engine.simulateBulkUpdate('opportunity', { region: 'east' }, { region: 'west' }); + + expect(ruleShares(engine)).toEqual([]); + }); + + it('grants when a bulk update moves records INTO the criteria (the reverse direction)', async () => { + seed(3, 'west'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toEqual([]); + + await engine.simulateBulkUpdate('opportunity', { region: 'west' }, { region: 'east' }); + + expect(ruleShares(engine).map((s) => s.record_id).sort()).toEqual(['opp0', 'opp1', 'opp2']); + expect(new Set(ruleShares(engine).map((s) => s.recipient_id))).toEqual(new Set(['alice'])); + }); + + it('still recomputes a single-id update (no regression on the path that worked)', async () => { + seed(1, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(1); + + await engine.simulateUpdateById('opportunity', 'opp0', { region: 'west' }); + + expect(ruleShares(engine)).toEqual([]); + }); + + it('leaves system-context bulk writes to the boot backfill, as before', async () => { + seed(2, 'east'); + await rules.evaluateRule('srule_east', SYS); + + await engine.simulateBulkUpdate( + 'opportunity', { region: 'east' }, { region: 'west' }, { isSystem: true }, + ); + + // Untouched by the hooks — `kernel:bootstrapped`'s backfill owns seeds. + expect(ruleShares(engine)).toHaveLength(2); + }); + + it('does not touch manual shares — only rule-materialised ones', async () => { + seed(1, 'east'); + await rules.evaluateRule('srule_east', SYS); + engine._tables.sys_record_share.push({ + id: 'shr_manual', object_name: 'opportunity', record_id: 'opp0', + recipient_type: 'user', recipient_id: 'carol', access_level: 'read', source: 'manual', + }); + + await engine.simulateBulkUpdate('opportunity', { region: 'east' }, { region: 'west' }); + + const remaining = engine._tables.sys_record_share; + expect(remaining).toHaveLength(1); + expect(remaining[0].id).toBe('shr_manual'); + }); + + describe('unbounded row sets — synchronous revoke, asynchronous re-grant', () => { + it('over the cap: revokes set-based before the write returns, then re-grants', async () => { + // 1001 rows match the WRITE's predicate (stage), which is over the cap; + // only two of them match the RULE's criteria (region). + seed(RULE_RECOMPUTE_ROW_CAP + 1, 'west'); + engine._tables.opportunity[0].region = 'east'; + engine._tables.opportunity[1].region = 'east'; + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(2); + + await engine.simulateBulkUpdate('opportunity', { stage: 'open' }, { stage: 'won' }); + + // Synchronous half: the grants are gone the moment the write returns, + // and they went in ONE set-based statement rather than 1001. + expect(ruleShares(engine)).toEqual([]); + const revokes = engine._deleteCalls.filter((c) => c.object === 'sys_record_share'); + expect(revokes).toHaveLength(1); + expect(revokes[0].options).toMatchObject({ + where: { source: 'rule', object_name: 'opportunity' }, + multi: true, + }); + + // Asynchronous half: the two rows still in `east` get their grant back. + await ruleRegrantQueue.whenIdle(); + expect(ruleShares(engine).map((s) => s.record_id).sort()).toEqual(['opp0', 'opp1']); + }); + + it('over the cap: the write is NOT refused (option A was the fallback, not the ruling)', async () => { + seed(RULE_RECOMPUTE_ROW_CAP + 1, 'east'); + await expect( + engine.simulateBulkUpdate('opportunity', { stage: 'open' }, { stage: 'won' }), + ).resolves.toBe(RULE_RECOMPUTE_ROW_CAP + 1); + expect(engine._tables.opportunity.every((r) => r.stage === 'won')).toBe(true); + await ruleRegrantQueue.whenIdle(); + }); + + it('an unscoped multi write (no predicate at all) takes the unbounded path', async () => { + seed(2, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(2); + + await engine.simulateBulkUpdate('opportunity', null, { stage: 'won' }); + + expect(ruleShares(engine)).toEqual([]); + await ruleRegrantQueue.whenIdle(); + // Still `east` — the re-grant restores what is still deserved. + expect(ruleShares(engine)).toHaveLength(2); + }); + + it('a failed resolve revokes rather than silently recomputing nothing', async () => { + seed(2, 'east'); + await rules.evaluateRule('srule_east', SYS); + engine.failFindOn = 'opportunity'; + + await engine.simulateBulkUpdate('opportunity', { region: 'east' }, { region: 'west' }); + + // "The query failed" must never be read as "no rows matched" (#4757). + expect(ruleShares(engine)).toEqual([]); + engine.failFindOn = null; + await ruleRegrantQueue.whenIdle(); + }); + + it('warns loudly about the availability trade it just made', async () => { + seed(RULE_RECOMPUTE_ROW_CAP + 1, 'east'); + await engine.simulateBulkUpdate('opportunity', { stage: 'open' }, { stage: 'won' }); + await ruleRegrantQueue.whenIdle(); + + const warned = logger.warn.mock.calls.map((c: any[]) => String(c[0])).join('\n'); + expect(warned).toContain('re-granted in the background'); + expect(logger.warn.mock.calls.some((c: any[]) => c[1]?.reason === 'over-cap')).toBe(true); + }); + }); + + describe('afterDelete — the orphaned-grant tail of the issue', () => { + it('revokes the deleted records grants (nothing can ever reconcile them)', async () => { + seed(3, 'east'); + await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(3); + + await engine.simulateBulkDelete('opportunity', { id: { $in: ['opp0', 'opp1'] } }); + + expect(ruleShares(engine).map((s) => s.record_id)).toEqual(['opp2']); + }); + + it('revokes via a set-based, multi-declared delete', async () => { + seed(1, 'east'); + await rules.evaluateRule('srule_east', SYS); + engine._deleteCalls.length = 0; + + await engine.simulateBulkDelete('opportunity', { region: 'east' }); + + const revokes = engine._deleteCalls.filter((c) => c.object === 'sys_record_share'); + expect(revokes).toHaveLength(1); + expect(revokes[0].options).toMatchObject({ + where: { source: 'rule', object_name: 'opportunity', record_id: { $in: ['opp0'] } }, + multi: true, + }); + }); + }); + + it('unbindAllRuleHooks removes the whole enlarged set', () => { + expect(unbindAllRuleHooks(engine as any)).toBe(5); + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(0); + }); +}); + +describe('resolveAffectedRows', () => { + let engine: Engine; + beforeEach(() => { + engine = makeEngine(); + engine._tables.opportunity = [ + { id: 'a', region: 'east' }, { id: 'b', region: 'east' }, { id: 'c', region: 'west' }, + ]; + }); + + it('takes a scalar input.id without querying', async () => { + const spy = vi.spyOn(engine, 'find'); + const out = await resolveAffectedRows(engine, 'opportunity', { input: { id: 'a' } }); + expect(out).toEqual({ kind: 'rows', ids: ['a'] }); + expect(spy).not.toHaveBeenCalled(); + }); + + it('resolves a predicate into its row set', async () => { + const out = await resolveAffectedRows(engine, 'opportunity', { + input: { options: { where: { region: 'east' }, multi: true } }, + }); + expect(out).toEqual({ kind: 'rows', ids: ['a', 'b'] }); + }); + + it('reports a predicate matching nothing as an EMPTY row set, not as unbounded', async () => { + const out = await resolveAffectedRows(engine, 'opportunity', { + input: { options: { where: { region: 'north' }, multi: true } }, + }); + expect(out).toEqual({ kind: 'rows', ids: [] }); + }); + + it('reports a missing predicate as unbounded (the AST would cover the table)', async () => { + const out = await resolveAffectedRows(engine, 'opportunity', { + input: { options: { multi: true } }, + }); + expect(out).toEqual({ kind: 'unbounded', reason: 'no-predicate' }); + }); + + it('reports over-cap without paying for the full scan', async () => { + engine._tables.opportunity = Array.from( + { length: RULE_RECOMPUTE_ROW_CAP + 5 }, + (_, i) => ({ id: `r${i}`, region: 'east' }), + ); + const spy = vi.spyOn(engine, 'find'); + const out = await resolveAffectedRows(engine, 'opportunity', { + input: { options: { where: { region: 'east' }, multi: true } }, + }); + expect(out).toEqual({ kind: 'unbounded', reason: 'over-cap' }); + expect(spy.mock.calls[0][1].limit).toBe(RULE_RECOMPUTE_ROW_CAP + 1); + }); + + it('reports a failed resolve as unbounded, never as an empty set', async () => { + engine.failFindOn = 'opportunity'; + const out = await resolveAffectedRows(engine, 'opportunity', { + input: { options: { where: { region: 'east' }, multi: true } }, + }); + expect(out).toMatchObject({ kind: 'unbounded', reason: 'resolve-failed' }); + }); +}); + +describe('idsFromHookInput', () => { + it('accepts the shapes that name primary keys', () => { + expect(idsFromHookInput('a')).toEqual(['a']); + expect(idsFromHookInput(7)).toEqual(['7']); + expect(idsFromHookInput({ $in: ['a', 'b'] })).toEqual(['a', 'b']); + expect(idsFromHookInput(['a', 'b'])).toEqual(['a', 'b']); + }); + + it('rejects predicates that merely look like ids', () => { + expect(idsFromHookInput(undefined)).toBeNull(); + expect(idsFromHookInput(null)).toBeNull(); + expect(idsFromHookInput({ $ne: 'a' })).toBeNull(); + expect(idsFromHookInput({ $in: [] })).toBeNull(); + }); +}); + +describe('RuleRegrantQueue', () => { + it('serializes tasks — a bulk write never fans out concurrent reconciles', async () => { + const q = new RuleRegrantQueue(); + const order: string[] = []; + const slow = (tag: string, ms: number) => async () => { + order.push(`${tag}:start`); + await new Promise((r) => setTimeout(r, ms)); + order.push(`${tag}:end`); + }; + q.enqueue(slow('a', 12)); + q.enqueue(slow('b', 1)); + await q.whenIdle(); + expect(order).toEqual(['a:start', 'a:end', 'b:start', 'b:end']); + expect(q.pending).toBe(0); + }); + + it('absorbs a failing task — a detached rejection must not reach the host', async () => { + const q = new RuleRegrantQueue(); + const onError = vi.fn(); + q.enqueue(async () => { throw new Error('regrant exploded'); }, onError); + q.enqueue(async () => { /* the chain survives */ }); + await expect(q.whenIdle()).resolves.toBeUndefined(); + expect(onError).toHaveBeenCalledOnce(); + expect((onError.mock.calls[0][0] as Error).message).toBe('regrant exploded'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.ts new file mode 100644 index 0000000000..f8c677a149 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4779] Resolve WHICH rows a write touched, so sharing-rule recompute is + * driven by the write's row SET instead of by a single record id. + * + * ## The defect this exists to close + * + * `bindRuleHooks` used to open its handler with + * + * ```ts + * const id = String(data?.id ?? ctx?.input?.id ?? ''); + * if (!id) return; // ← every predicate write, silently + * ``` + * + * and `ObjectQL.update()` only populates `input.id` for a scalar `where.id`. + * A predicate (`multi: true`) update therefore recomputed NOTHING: an admin + * could bulk-move a thousand records out of a sharing rule's criteria and + * every `sys_record_share` row the rule had issued stayed in the table, + * answering. That is a fail-open on the AUTHORIZATION side — the same family + * as #4757 (`sys_attachment`) and #4778 (approval locks), just better hidden, + * because the stale grant looks exactly like a legitimate one. + * + * ## The two answers, and why there are exactly two + * + * A recompute needs the affected rows one at a time (`evaluateAllForRecord` + * runs a criteria probe, a recipient expansion and a grant diff PER ROW), so + * it needs a bound. A revoke does not: `sys_record_share` can be scoped by + * `object_name` in a single set-based delete. The maintainer's ruling on this + * issue (2026-08-04, option C) turns that asymmetry into the design — + * **over-granting is a security incident, under-granting is an availability + * wobble** — so the safety half is always synchronous and complete, and only + * the expensive restoration half is deferred: + * + * - {@link AffectedRows} `kind: 'rows'` — the write's row set is known and + * within {@link RULE_RECOMPUTE_ROW_CAP}. `afterUpdate` recomputes each row + * synchronously, which covers BOTH directions (a row bulk-updated INTO a + * rule's criteria is granted, one updated OUT of it is revoked) because + * `reconcileForRecord` is diff-based. + * - {@link AffectedRows} `kind: 'unbounded'` — the row set could not be + * bounded. Every rule-sourced grant on the object is revoked synchronously + * (one set-based statement, no cap to exceed), then re-granted + * asynchronously by reconciling the object's rules. + * + * The write is never refused (that was option A, the fallback): refusing would + * make an internal recompute bound into a business-visible limit on how many + * rows an admin may update, reported by a subsystem they never configured. + * + * ### Why the unbounded revoke is object-wide and not `record_id: {$in: […]}` + * + * Scoping the revoke to the affected ids would spare the untouched rows their + * brief outage — but it needs the full id list, and needing the full id list + * is exactly the bound this branch exists because we could not meet. A scoped + * revoke would therefore re-import the cap into the one operation that is + * free of it. The wider blast radius is deliberate and is in the safe + * direction: the extra rows lose access for as long as the re-grant takes, + * and no row keeps access it should have lost. + * + * ## Superset, never subset + * + * The predicate is resolved with SYSTEM_CTX, while the write itself also + * carries the sharing middleware's writable-rows filter (`buildWriteFilter`) + * composed into its AST. So the resolved set is a SUPERSET of the rows the + * write actually changed. That is the correct direction and is load-bearing: + * `evaluateAllForRecord` is idempotent, so recomputing an untouched row costs + * a query and changes nothing, whereas MISSING a changed row is the defect. + */ + +/** + * Rows a single write may recompute synchronously before the recompute is + * deferred. 1000 mirrors the fail-closed bound the sibling authorization + * guards already chose — `MULTI_DELETE_AUTH_LIMIT` in service-storage's + * attachment hooks (#4757) and #4630's `sys_comment` resolve — so the three + * guards in this family answer "how many rows may one write drag through a + * per-row pass?" with one number. + */ +export const RULE_RECOMPUTE_ROW_CAP = 1_000; + +/** Why a write's row set could not be enumerated. */ +export type UnboundedReason = + /** `multi: true` with no `where` at all — the AST covers the whole table. */ + | 'no-predicate' + /** The predicate matches more rows than {@link RULE_RECOMPUTE_ROW_CAP}. */ + | 'over-cap' + /** The resolve query itself failed; nothing at all is known about the set. */ + | 'resolve-failed'; + +/** The row set a write touched, as far as it can be known before it lands. */ +export type AffectedRows = + | { readonly kind: 'rows'; readonly ids: readonly string[] } + | { readonly kind: 'unbounded'; readonly reason: UnboundedReason; readonly detail?: string }; + +/** The slice of the ObjectQL engine this module reads. */ +export interface RecomputeEngine { + find(object: string, options?: any): Promise; +} + +interface MinimalLogger { + info?: (msg: any, ...rest: any[]) => void; + warn?: (msg: any, ...rest: any[]) => void; + error?: (msg: any, ...rest: any[]) => void; +} + +const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; + +/** + * Ids named directly by a hook's `input.id`. + * + * `delete()` accepts `{ id: { $in: [...] } }`, so a "single id" input can name + * many rows — the same shape `asIdList` handles in service-storage's + * attachment guard (#4757). Anything else (a predicate operator, an object) + * names no primary key and returns `null` so the caller falls through to the + * predicate resolve. + */ +export function idsFromHookInput(id: unknown): string[] | null { + if (typeof id === 'string' || typeof id === 'number' || typeof id === 'bigint') { + return [String(id)]; + } + if (Array.isArray(id)) { + const out = id.filter((v) => typeof v === 'string' || typeof v === 'number').map(String); + return out.length > 0 ? out : null; + } + if (id && typeof id === 'object' && Array.isArray((id as any).$in)) { + const out = (id as any).$in + .filter((v: unknown) => typeof v === 'string' || typeof v === 'number') + .map(String); + return out.length > 0 ? out : null; + } + return null; +} + +/** + * Resolve the rows a `beforeUpdate` / `beforeDelete` context is about to + * change, bounded by {@link RULE_RECOMPUTE_ROW_CAP}. + * + * Called from a BEFORE hook on purpose: an update that moves rows out of a + * rule's criteria makes them unfindable by that same predicate the moment it + * lands, and a delete removes them outright. The `after` hook then reads the + * stashed answer (the engine reuses one `HookContext` instance across the + * before/after pair of a write, which is the seam `primary-bu-projection.ts` + * already relies on). + */ +export async function resolveAffectedRows( + engine: RecomputeEngine, + objectName: string, + hookCtx: any, + logger?: MinimalLogger, +): Promise { + // 1. The write names its rows directly (scalar id, or delete's `$in` form). + const named = idsFromHookInput(hookCtx?.input?.id); + if (named) return { kind: 'rows', ids: named }; + + // 2. Some writes carry the id in the payload instead (insert, and upsert- + // shaped updates whose `data.id` the engine has not lifted yet). + const payloadId = (hookCtx?.input?.data as any)?.id; + const fromPayload = idsFromHookInput(payloadId); + if (fromPayload) return { kind: 'rows', ids: fromPayload }; + + // 3. Predicate write. No predicate at all means the AST is `{ object }` — + // literally every row. Enumerating "the whole table" is not a bound, so + // say so rather than resolving a partial page and treating it as the set. + // ("Nothing was queried" is not "nothing matched" — #4757's own lesson.) + const where = hookCtx?.input?.options?.where; + if (where === undefined || where === null) { + return { kind: 'unbounded', reason: 'no-predicate' }; + } + + try { + // CAP + 1 is the whole detection: one extra row is enough to know the set + // is over the cap, and reading the exact size would cost the scan this + // branch exists to avoid. + const rows = await engine.find(objectName, { + where, + fields: ['id'], + limit: RULE_RECOMPUTE_ROW_CAP + 1, + context: SYSTEM_CTX, + }); + const ids = (Array.isArray(rows) ? rows : []) + .map((r: any) => (r?.id == null ? '' : String(r.id))) + .filter(Boolean); + if (ids.length > RULE_RECOMPUTE_ROW_CAP) { + return { kind: 'unbounded', reason: 'over-cap' }; + } + return { kind: 'rows', ids }; + } catch (err: any) { + // The predicate could not be run, so the row set is unknown — which is a + // strictly weaker state than "matched nothing" and must not be read as it. + logger?.warn?.( + '[sharing-rule] could not resolve the rows a bulk write touches — every rule grant on the object ' + + 'will be revoked and re-granted asynchronously instead', + { object: objectName, error: err?.message }, + ); + return { kind: 'unbounded', reason: 'resolve-failed', detail: err?.message }; + } +} + +/** + * The asynchronous half of the ruling: re-grant after the synchronous revoke. + * + * Deliberately in-process and dependency-free rather than routed through + * `IJobService`. The job service is an OPTIONAL capability (`serve.ts`'s + * `CAPABILITY_PROVIDERS` lists `job` and `sharing` independently, and + * plugin-sharing does not depend on it), so routing through it would make the + * re-grant happen in some compositions and silently not in others — a + * declared-but-not-enforced guarantee, which is the shape AGENTS.md PD #10 + * exists to forbid. An in-process queue is present in every composition that + * has the hooks at all. + * + * Durability comes from the layer that already provides it: the plugin's + * `kernel:bootstrapped` pass (`backfillRuleGrants`) reconciles EVERY rule on + * every boot and `evaluateRule` is idempotent, so a re-grant lost to a crash + * is repaired by the next start. That is the compensating executor this + * design leans on; nothing here needs to survive the process. + * + * Tasks are serialized (never concurrent) so a bulk write cannot fan out into + * hundreds of parallel criteria queries. + */ +export class RuleRegrantQueue { + private chain: Promise = Promise.resolve(); + private depth = 0; + + /** Tasks enqueued and not yet settled. */ + get pending(): number { + return this.depth; + } + + /** + * Queue a re-grant. Returns immediately — the caller is a write hook and + * must not wait. Failures are logged, never rethrown: an unhandled rejection + * from a detached task would take down the host, and the failure mode here + * is under-sharing, which the next boot repairs. + */ + enqueue(task: () => Promise, onError?: (err: unknown) => void): void { + this.depth += 1; + this.chain = this.chain.then(async () => { + try { + await task(); + } catch (err) { + try { + onError?.(err); + } catch { + /* a logger that throws must not poison the chain */ + } + } finally { + this.depth -= 1; + } + }); + } + + /** + * Resolves once every task enqueued BEFORE this call has settled. The seam + * tests use to observe the asynchronous half deterministically; production + * code never awaits it. + */ + whenIdle(): Promise { + return this.chain; + } +} diff --git a/packages/plugins/plugin-sharing/src/index.ts b/packages/plugins/plugin-sharing/src/index.ts index 4022819062..eec30d137b 100644 --- a/packages/plugins/plugin-sharing/src/index.ts +++ b/packages/plugins/plugin-sharing/src/index.ts @@ -34,9 +34,19 @@ export { bindRuleHooks, unbindAllRuleHooks, bindRuleCriteriaGuard, + ruleRegrantQueue, SHARING_RULE_HOOK_PACKAGE, RULE_CRITERIA_GUARD_PACKAGE, } from './rule-hooks.js'; +export { + RULE_RECOMPUTE_ROW_CAP, + RuleRegrantQueue, + resolveAffectedRows, + idsFromHookInput, + type AffectedRows, + type UnboundedReason, + type RecomputeEngine, +} from './bulk-recompute.js'; export { parseCriteria, isMatchAllCriteria, diff --git a/packages/plugins/plugin-sharing/src/rule-hooks.ts b/packages/plugins/plugin-sharing/src/rule-hooks.ts index 1de75b4879..3b5aff9577 100644 --- a/packages/plugins/plugin-sharing/src/rule-hooks.ts +++ b/packages/plugins/plugin-sharing/src/rule-hooks.ts @@ -3,11 +3,31 @@ import type { SharingRuleService } from './sharing-rule-service.js'; import type { SharingRuleRow } from '@objectstack/spec/contracts'; import { isMatchAllCriteria, SharingCriteriaValidationError } from './rule-criteria.js'; +import { + RULE_RECOMPUTE_ROW_CAP, + RuleRegrantQueue, + resolveAffectedRows, + type AffectedRows, +} from './bulk-recompute.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; export const SHARING_RULE_HOOK_PACKAGE = 'plugin-sharing:rules'; +/** + * [#4779] Shared-`HookContext` key holding the row set the write is about to + * change, stashed by the `before` hook for the `after` hook to consume. + * + * The stash is necessary, not a convenience: an update that moves rows OUT of + * a rule's criteria makes them unfindable by the write's own predicate the + * instant it lands, and a delete removes them outright — so `afterUpdate` / + * `afterDelete` are structurally too late to ask "which rows was this?". + * `ObjectQL.update()` / `.delete()` reuse ONE `HookContext` instance across + * each before/after pair (they mutate `ctx.event` in place), which is the same + * seam `primary-bu-projection.ts`'s `__primaryBuUserId` rides on. + */ +const STASH_KEY = '__sharingAffectedRows'; + /** * Package id for the `sys_sharing_rule` DATA-change triggers that re-run the * bind (#2592). Deliberately distinct from {@link SHARING_RULE_HOOK_PACKAGE} @@ -29,6 +49,14 @@ interface MinimalEngine { packageId?: string; }): void; unregisterHooksByPackage(packageId: string): number; + /** + * [#4779] Needed to resolve a predicate write's row set in the `before` + * hook. Optional so a caller holding only the hook-registry surface still + * type-checks; absent, every predicate write is treated as unbounded — the + * safe direction (revoke everything, re-grant asynchronously), never the + * silent no-op this issue was filed for. + */ + find?(object: string, options?: any): Promise; } interface MinimalLogger { @@ -37,10 +65,43 @@ interface MinimalLogger { } /** - * Bind afterInsert/afterUpdate hooks for every distinct object_name in - * `rules`. Each hook calls `service.evaluateAllForRecord(object, id, …)` - * with SYSTEM_CTX so the evaluator can write `sys_record_share` rows - * without being blocked by its own enforcement. + * The in-process executor for the asynchronous re-grant half of #4779. + * + * Module-scoped on purpose: `bindRuleHooks` is called again on every rule + * rebind (`bindRuleRebindTriggers` unbinds and re-binds the whole package on + * each `sys_sharing_rule` write), and a per-call queue would let a rebind + * orphan re-grants that were still in flight. One queue per process keeps the + * chain — and therefore the serialization guarantee — continuous across + * rebinds. Exported for tests to await; production code never does. + */ +export const ruleRegrantQueue = new RuleRegrantQueue(); + +/** + * Bind the sharing-rule recompute hooks for every distinct object_name in + * `rules`. Everything runs with SYSTEM_CTX so the evaluator can write + * `sys_record_share` without being blocked by its own enforcement. + * + * Five hooks per object: + * + * - `afterInsert` — recompute the inserted row (unchanged behaviour). + * - `beforeUpdate` / `beforeDelete` — resolve the affected row set and stash + * it on the shared `HookContext` ({@link STASH_KEY}). Must be `before`: + * the write is what makes those rows unfindable. + * - `afterUpdate` — recompute per row when the set is bounded (which grants + * AND revokes, so a bulk update INTO a rule's criteria is covered as well + * as one out of it); otherwise revoke the object's rule grants set-based + * and queue the re-grant. + * - `afterDelete` — revoke the deleted rows' rule grants. Nothing can + * re-grant them: `evaluateRule` iterates records that still exist, so a + * grant whose record is gone is unreachable by every reconcile path and + * outlives restarts (the orphan noted at the tail of #4779). Harmless only + * while record ids are never reused — an assumption no gate enforces. + * + * [#4779] `if (!id) return` — the line these hooks used to open with — is + * gone. It read as a cheap guard and was in fact the whole defect: predicate + * (`multi: true`) writes never populate `input.id`, so every bulk write + * skipped recompute entirely and left stale `sys_record_share` rows granting + * access the rules no longer imply. * * Caller is responsible for invoking {@link unbindAllRuleHooks} before * re-binding when the rule set changes. @@ -57,19 +118,100 @@ export function bindRuleHooks( if (r.object_name) objects.add(r.object_name); } for (const objectName of objects) { - const handler = async (ctx: any) => { + const opts = { object: objectName, packageId: SHARING_RULE_HOOK_PACKAGE, priority: 180 }; + + /** Recompute one record; never throws (a hook must not fail the write). */ + const recomputeRow = async (id: string): Promise => { + await service.evaluateAllForRecord(objectName, id, SYSTEM_CTX as any); + }; + + /** + * The unbounded branch: revoke now, re-grant later. Ordered so that the + * process can die between the two and still be safe — the grants are + * already gone, and the `kernel:bootstrapped` backfill re-grants on the + * next start. + */ + const revokeThenQueueRegrant = async (reason: string): Promise => { + await service.revokeRuleGrantsForObject(objectName); + logger?.warn?.( + '[sharing-rule] a bulk write touched more rows than can be recomputed inline — every rule grant on ' + + 'this object was revoked and is being re-granted in the background; recipients may briefly lose ' + + 'access to records they still qualify for (a restart re-runs the same reconcile)', + { object: objectName, reason, cap: RULE_RECOMPUTE_ROW_CAP }, + ); + ruleRegrantQueue.enqueue( + () => service.evaluateAllRulesForObject(objectName).then(() => undefined), + (err: any) => logger?.warn?.( + '[sharing-rule] background re-grant failed — grants stay revoked until the next reconcile ' + + '(any sharing-rule write, or a restart)', + { object: objectName, error: err?.message }, + ), + ); + }; + + const stashAffectedRows = async (ctx: any) => { + if ((ctx?.session as any)?.isSystem) return; + try { + ctx[STASH_KEY] = typeof engine.find === 'function' + ? await resolveAffectedRows(engine as Required, objectName, ctx, logger) + : ({ kind: 'unbounded', reason: 'resolve-failed', detail: 'engine has no find()' } as AffectedRows); + } catch (err: any) { + // resolveAffectedRows already fails safe; this is the belt for a + // genuinely unexpected throw. Unknown must never degrade to "no rows". + ctx[STASH_KEY] = { kind: 'unbounded', reason: 'resolve-failed', detail: err?.message } as AffectedRows; + } + }; + + /** What the `after` hook should act on when no `before` hook ran. */ + const affectedFrom = (ctx: any): AffectedRows => + (ctx?.[STASH_KEY] as AffectedRows | undefined) + ?? ({ kind: 'unbounded', reason: 'resolve-failed', detail: 'no before-hook stash' } as AffectedRows); + + engine.registerHook('afterInsert', async (ctx: any) => { if ((ctx?.session as any)?.isSystem) return; try { const data = ctx?.result ?? ctx?.input?.data ?? {}; const id = String((data as any)?.id ?? ctx?.input?.id ?? ''); if (!id) return; - await service.evaluateAllForRecord(objectName, id, SYSTEM_CTX as any); + await recomputeRow(id); } catch (err: any) { logger?.warn?.('[sharing-rule] hook evaluation failed', { object: objectName, error: err?.message }); } - }; - engine.registerHook('afterInsert', handler, { object: objectName, packageId: SHARING_RULE_HOOK_PACKAGE, priority: 180 }); - engine.registerHook('afterUpdate', handler, { object: objectName, packageId: SHARING_RULE_HOOK_PACKAGE, priority: 180 }); + }, opts); + + engine.registerHook('beforeUpdate', stashAffectedRows, opts); + engine.registerHook('beforeDelete', stashAffectedRows, opts); + + engine.registerHook('afterUpdate', async (ctx: any) => { + if ((ctx?.session as any)?.isSystem) return; + try { + const affected = affectedFrom(ctx); + if (affected.kind === 'rows') { + for (const id of affected.ids) await recomputeRow(id); + return; + } + await revokeThenQueueRegrant(affected.reason); + } catch (err: any) { + logger?.warn?.('[sharing-rule] hook evaluation failed', { object: objectName, error: err?.message }); + } + }, opts); + + engine.registerHook('afterDelete', async (ctx: any) => { + if ((ctx?.session as any)?.isSystem) return; + try { + const affected = affectedFrom(ctx); + if (affected.kind === 'rows') { + await service.revokeRuleGrantsForRecords(objectName, affected.ids); + return; + } + // The deleted set is unknown, so which grants are orphaned is unknown + // too. Revoke them all and let the re-grant restore those whose record + // survived — the same trade the update path makes. + await revokeThenQueueRegrant(affected.reason); + } catch (err: any) { + logger?.warn?.('[sharing-rule] hook evaluation failed', { object: objectName, error: err?.message }); + } + }, opts); } logger?.info?.('[sharing-rule] hooks bound', { objects: Array.from(objects), ruleCount: rules.length }); } diff --git a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts index 85d42d42b5..3ace560ae4 100644 --- a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts +++ b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts @@ -23,6 +23,14 @@ import { type AnyRecord = Record; type HookEntry = { event: string; handler: (ctx: any) => any; options: AnyRecord }; +/** + * Lifecycle hooks `bindRuleHooks` registers per object. Named rather than + * inlined because these tests are about REBIND bookkeeping (bound → unbound → + * re-bound), not about which events the recompute needs — #4779 changed the + * second and must not look like it changed the first. + */ +const RULE_HOOKS_PER_OBJECT = 5; + function makeEngine() { const hooks: HookEntry[] = []; return { @@ -90,14 +98,19 @@ describe('SharingServicePlugin sys_sharing_rule data-change rebind (#2592)', () await engine.fire('afterInsert', 'sys_sharing_rule', { result: { id: 'r1' } }); const bound = engine.boundFor(SHARING_RULE_HOOK_PACKAGE); - expect(bound.map((h) => h.event).sort()).toEqual(['afterInsert', 'afterUpdate']); + // [#4779] Five, not two: predicate writes need a `before` hook to resolve + // their row set (the write is what makes those rows unfindable), and + // `afterDelete` retires the grants of records that no reconcile can reach. + expect(bound.map((h) => h.event).sort()).toEqual( + ['afterDelete', 'afterInsert', 'afterUpdate', 'beforeDelete', 'beforeUpdate'], + ); for (const h of bound) expect(h.options.object).toBe('project'); }); it('tears down hooks when the last rule for an object is deleted', async () => { rules = [{ name: 'r1', object_name: 'project', active: true }]; await engine.fire('afterInsert', 'sys_sharing_rule', {}); - expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(2); + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(RULE_HOOKS_PER_OBJECT); rules = []; await engine.fire('afterDelete', 'sys_sharing_rule', {}); @@ -116,7 +129,7 @@ describe('SharingServicePlugin sys_sharing_rule data-change rebind (#2592)', () it('keeps previous bindings and does not throw when listRules fails', async () => { rules = [{ name: 'r1', object_name: 'project', active: true }]; await engine.fire('afterInsert', 'sys_sharing_rule', {}); - expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(2); + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(RULE_HOOKS_PER_OBJECT); ruleService.listRules = vi.fn(async () => { throw new Error('db gone'); }); await expect( @@ -124,7 +137,7 @@ describe('SharingServicePlugin sys_sharing_rule data-change rebind (#2592)', () ).resolves.toBeUndefined(); // the write must not fail // The failed rebind ran before unbind — previous bindings intact. - expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(2); + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toHaveLength(RULE_HOOKS_PER_OBJECT); }); it('serializes overlapping rebinds so the newest rule snapshot wins', async () => { diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 03b62031a3..80f76b9039 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -378,6 +378,95 @@ export class SharingRuleService implements ISharingRuleService { return results; } + /** + * [#4779] Reconcile EVERY rule bound to `object` — the object-scoped twin of + * the `kernel:bootstrapped` backfill. + * + * This is the re-grant half of the ruling's option C: after a bulk write + * whose row set could not be bounded has had its grants revoked set-based, + * this pass puts back the grants that are still deserved. Per RULE rather + * than per row, deliberately — `evaluateRule` already diffs the whole + * matched set against the whole existing grant set in one pass, which is + * both cheaper than N per-row reconciles and the exact primitive the boot + * backfill uses, so the asynchronous repair and the restart repair are the + * same code path rather than two that must be kept agreeing. + * + * Inactive rules are included: `evaluateRule` purges their grants (#4433), + * so excluding them would leave withdrawal to the next restart. Best-effort + * per rule — one broken rule must not stop its siblings being restored. + */ + async evaluateAllRulesForObject(object: string): Promise { + if (!object) return 0; + const rules = await this.listRules({ object }, SYSTEM_CTX as any); + let reconciled = 0; + for (const rule of rules) { + try { + await this.evaluateRule(rule.id, SYSTEM_CTX as any); + reconciled += 1; + } catch (err: any) { + this.logger?.warn?.('[sharing-rule] object reconcile failed for rule', { + object, + rule: rule.name ?? rule.id, + error: err?.message, + }); + } + } + return reconciled; + } + + /** + * [#4779] Revoke every rule-materialised grant on `object`, set-based. + * + * The cheap, uncapped half of the ruling: one predicate delete over + * `sys_record_share`, whose cost does not grow with the number of records + * the triggering write touched. It is what lets a bulk write proceed + * without the recompute bound leaking out as a limit on how many rows an + * admin may change — the write lands, every grant that may have gone stale + * is gone before it returns, and {@link evaluateAllRulesForObject} puts + * back the deserved ones asynchronously. + * + * `multi: true` is required, not decorative: `ObjectQL.delete` refuses a + * predicate-shaped call that does not declare bulk intent + * (`resolveEngineDeleteDispatch`), which is precisely the shape that made + * every `DELETE /sharing/rules/:id` answer 500 in #4434. + * + * Only `source: 'rule'` rows are touched. A manual grant is a human's + * decision about one record and no rule evaluation would ever re-create it, + * so sweeping it here would destroy data this subsystem does not own. + */ + async revokeRuleGrantsForObject(object: string): Promise { + if (!object) return; + await this.engine.delete('sys_record_share', { + where: { source: 'rule', object_name: object }, + multi: true, + context: SYSTEM_CTX, + } as any); + } + + /** + * [#4779] Revoke the rule-materialised grants of a NAMED set of records — + * the delete path's revoke, where the rows are gone and no reconcile can + * ever reach them again. + * + * Chunked because the id set rides in an `$in`, and a single statement + * binding a thousand parameters is a portability trap (SQLite's default + * `SQLITE_MAX_VARIABLE_NUMBER` is 999 on older builds). Chunking keeps this + * O(ids/CHUNK) statements instead of O(ids), which is still set-based in + * the sense that matters. + */ + async revokeRuleGrantsForRecords(object: string, recordIds: readonly string[]): Promise { + if (!object || recordIds.length === 0) return; + const CHUNK = 200; + for (let i = 0; i < recordIds.length; i += CHUNK) { + const batch = recordIds.slice(i, i + CHUNK); + await this.engine.delete('sys_record_share', { + where: { source: 'rule', object_name: object, record_id: { $in: batch } }, + multi: true, + context: SYSTEM_CTX, + } as any); + } + } + // ── internals ───────────────────────────────────────────────────── /**