From a6d7efe013a96731969a98a6311dac61c44fd029 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:49:34 +0000 Subject: [PATCH 1/3] wip: fix sharing-rule withdrawal (#4433) and DELETE 500 (#4434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An over-granting sharing rule had no withdrawal path on the API surface: deactivating it never withdrew its materialized grants, and the DELETE route answered 500 for both address forms. RC-exit blockers, since v17 advertises "switching a rule off actually withdraws access". #4434 — deleteRule purged sys_record_share with a predicate-shaped engine.delete carrying neither a scalar id nor multi:true, the one shape the engine's dispatch refuses. It threw before ever reaching the rule row, so every DELETE /sharing/rules/:idOrName 500'd. Routed through purgeRuleGrants instead of adding multi:true, so a rule's grants retire exactly one way — SharingService.revoke, as every other withdrawal path already does (AGENTS.md PD #5). #4433 — three independent gaps, one per path the issue walked: - deactivation: the rule-write trigger skipped reconcile on every isSystem write. defineRule (the only implementation behind POST /sharing/rules) writes with SYSTEM_CTX unconditionally, so the skip caught 100% of REST authoring. Now gated on boot phase, the question the skip actually meant to ask. - record touch: evaluateAllForRecord listed activeOnly rules, so a deactivated rule was absent from the loop and its grants were never examined. Lists every rule; inactive ones desire nothing and take reconcileForRecord's existing revoke branch. - boot: the backfill was handed activeOnly rules, making it structurally incapable of revoking. Walks every rule, plus a new sweepOrphanedRuleGrants for grants whose rule row is gone entirely (unreachable by rule iteration). Test fakes: makeEngine().delete now mirrors the real engine's dispatch guard. The looser fake is why #4434 shipped green — the pre-existing "deleteRule drops rule + all its grants" test passed against a delete the running server always rejected. Progress: service + plugin fixes done, regression tests added for both issues. Remaining: rule-rebind.test.ts still pins the old isSystem skip and needs updating; full gates + live-server verification pending. Refs #4433, #4434 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../plugin-sharing/src/boot-backfill.test.ts | 95 +++++++++++ .../plugin-sharing/src/rule-rebind.test.ts | 5 + .../plugin-sharing/src/sharing-plugin.ts | 68 +++++++- .../src/sharing-rule-service.ts | 93 +++++++++- .../plugin-sharing/src/sharing-rule.test.ts | 159 ++++++++++++++++++ 5 files changed, 407 insertions(+), 13 deletions(-) diff --git a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts index 0fea98f84e..7394cb6e35 100644 --- a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts +++ b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts @@ -58,6 +58,13 @@ function makeEngine() { return t[i]; }, async delete(o: string, opts?: any) { + // Mirror `ObjectQLEngine.delete`'s dispatch guard (#4434) — see the same + // note in sharing-rule.test.ts. A fake looser than the contract it + // stands in for is how a green suite ships a dead route. + const whereId = opts?.where && typeof opts.where === 'object' ? (opts.where as any).id : undefined; + const t0 = typeof whereId; + const scalarId = whereId != null && (t0 === 'string' || t0 === 'number' || t0 === 'bigint'); + if (!scalarId && !opts?.multi) throw new Error('Delete requires an ID or options.multi=true'); 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 }; @@ -127,6 +134,94 @@ describe('backfillRuleGrants (#2926 ③ — seed rows materialize at boot)', () }); }); +/** + * objectstack#4433 (restart half) — "not at boot". + * + * The boot pass was handed `listRules({ activeOnly: true })`, so a deactivated + * rule was never evaluated and the grants it had materialised survived every + * restart: the rule read `active: false` while the orphaned `source: 'rule'` + * row kept answering. The pass is the last line of defence for withdrawal, so + * it has to walk EVERY rule — `evaluateRule` purges the ones it finds inactive. + */ +describe('boot rule backfill withdraws deactivated rules (#4433)', () => { + let engine: ReturnType; + let sharing: SharingService; + let rules: SharingRuleService; + + beforeEach(async () => { + engine = makeEngine(); + sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing }); + engine._tables.showcase_private_note = [ + { id: 'note_n', title: 'rc1 rule target', owner_id: 'admin' }, + ]; + await rules.defineRule({ + name: 'rc1_rule_livetest', label: 'RC1 live test', object: 'showcase_private_note', + criteria: { title: 'rc1 rule target' }, + recipientType: 'user', recipientId: 'member_b', accessLevel: 'read', + }, SYS); + // Boot 1 materialises the grant — the issue's step 3 baseline. + await backfillRuleGrants(rules, await rules.listRules({}, SYS)); + expect(engine._tables.sys_record_share ?? []).toHaveLength(1); + }); + + it('revokes a deactivated rule\'s grants on the next boot', async () => { + // Admin switches the rule off. Nothing else touches the record. + await rules.defineRule({ + name: 'rc1_rule_livetest', label: 'RC1 live test', object: 'showcase_private_note', + criteria: { title: 'rc1 rule target' }, + recipientType: 'user', recipientId: 'member_b', accessLevel: 'read', active: false, + }, SYS); + + // Restart: the pass must see the inactive rule to be able to purge it. + await backfillRuleGrants(rules, await rules.listRules({}, SYS)); + + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('an activeOnly rule list can never withdraw — the #4433 boot repro', async () => { + await rules.defineRule({ + name: 'rc1_rule_livetest', label: 'RC1 live test', object: 'showcase_private_note', + criteria: { title: 'rc1 rule target' }, + recipientType: 'user', recipientId: 'member_b', accessLevel: 'read', active: false, + }, SYS); + + // The old call site, pinned as the defect it was: an inactive rule is + // absent from the list, so the pass has nothing to purge with. + const activeOnly = await rules.listRules({ activeOnly: true }, SYS); + expect(activeOnly).toHaveLength(0); + await backfillRuleGrants(rules, activeOnly); + expect(engine._tables.sys_record_share ?? []).toHaveLength(1); // still granted + + // Reconciling every rule is what repairs it. + await backfillRuleGrants(rules, await rules.listRules({}, SYS)); + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('still materialises active rules (the #2926 behaviour is untouched)', async () => { + engine._tables.showcase_private_note.push({ id: 'note_2', title: 'rc1 rule target', owner_id: 'admin' }); + await backfillRuleGrants(rules, await rules.listRules({}, SYS)); + expect((engine._tables.sys_record_share ?? []).map((s) => s.record_id).sort()).toEqual(['note_2', 'note_n']); + }); + + it('sweeps grants whose rule row vanished before the restart', async () => { + // A rule removed by a path that never reached deleteRule (data-API delete + // with the hook unbound, a migration, a crash mid-delete). + engine._tables.sys_sharing_rule = []; + await backfillRuleGrants(rules, await rules.listRules({}, SYS)); + expect(engine._tables.sys_record_share ?? []).toHaveLength(1); // unreachable by rule iteration + + expect(await rules.sweepOrphanedRuleGrants()).toBe(1); + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('the sweep is idempotent across repeated boots', async () => { + expect(await rules.sweepOrphanedRuleGrants()).toBe(0); + expect(await rules.sweepOrphanedRuleGrants()).toBe(0); + expect(engine._tables.sys_record_share ?? []).toHaveLength(1); + }); +}); + describe("backfillRetiredAccessLevels (#3865 — stored 'full' normalises to 'edit')", () => { let engine: ReturnType; diff --git a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts index 90bfd0af2e..c5a1a6d6ec 100644 --- a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts +++ b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts @@ -171,11 +171,16 @@ describe('SharingServicePlugin reconciles grants on rule writes (#3821)', () => listRules: vi.fn(async () => []), evaluateRule: vi.fn(async () => ({ ruleId: 'r1', matchedRecords: 2, expandedUsers: 1, grantsCreated: 2, grantsUpdated: 0, grantsRevoked: 0 })), revokeRuleGrants: vi.fn(async () => 2), + sweepOrphanedRuleGrants: vi.fn(async () => 0), }; (plugin as any).ruleService = ruleService; const ctx = makeCtx(); logger = ctx.logger; (plugin as any).bindRuleRebindTriggers(engine, ctx); + // [#4433] Boot is over — `kernel:bootstrapped` has run its backfill, so + // runtime rule writes own reconciliation from here. Before this point the + // trigger defers to that pass (see the boot-phase describe below). + (plugin as any).ruleGrantsBootReconciled = true; }); it('backfills existing records when a rule is created', async () => { diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 6ea9378a13..3094ec5719 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -42,10 +42,18 @@ export interface SharingPluginOptions { * but seed rows are written with `isSystem` (which the hooks deliberately * skip — see rule-hooks.ts), so a fresh deploy's seed data carried no * `sys_record_share` rows until each record was touched at runtime. - * Reconcile every active rule once per boot: `evaluateRule` is idempotent + * Reconcile every rule once per boot: `evaluateRule` is idempotent * (diff-based grant/update/revoke), so repeated boots are no-ops. * Best-effort per rule — one broken rule must not block startup or its * siblings. Returns the number of rules successfully reconciled. + * + * [#4433] Callers must pass EVERY rule, not just the active ones. This pass is + * the last line of defence for withdrawal: `evaluateRule` purges the grants of + * a rule it finds inactive, so an inactive rule in this list is what turns a + * restart into a repair. Handed only active rules — as it was — the pass could + * physically never revoke anything a deactivated rule had left behind, which + * is why the #4433 repro survived a full restart with the rule reading + * `active: false` and the grant still answering. */ export async function backfillRuleGrants( ruleService: SharingRuleService, @@ -223,6 +231,21 @@ export class SharingServicePlugin implements Plugin { /** Resolved once in `kernel:ready`; reused by the `kernel:bootstrapped` backfills. */ private engine?: SharingEngine; + /** + * [#4433] Has the `kernel:bootstrapped` rule-grant backfill finished? + * + * This is the real question the rule-write trigger needs to answer before it + * decides to skip a reconcile — "is the boot pass going to cover this write + * anyway?" It used to ask `session.isSystem` instead, which is a different + * question with a very different answer: `SharingRuleService.defineRule` + * writes `sys_sharing_rule` with SYSTEM_CTX **always** (it must, to reach a + * platform table the sharing middleware otherwise gates), so every runtime + * authoring write — including `POST /sharing/rules` with `active: false` — + * looked exactly like boot seeding and was skipped. That is the whole of + * #4433's first half: deactivation returned 200, and nothing reconciled. + */ + private ruleGrantsBootReconciled = false; + constructor(options: SharingPluginOptions = {}) { this.options = options; } @@ -265,8 +288,19 @@ export class SharingServicePlugin implements Plugin { * `evaluateRule` the REST `/sharing/rules/:id/evaluate` endpoint runs, which * is diff-based and purges when the rule is inactive. Deletes can't go * through it (the row is gone, `RULE_NOT_FOUND`), so they purge directly. - * System-context writes are skipped: seeding and package bootstrap write - * with `isSystem`, and `kernel:bootstrapped` already backfills those. + * + * [#4433] The reconcile is skipped only until the `kernel:bootstrapped` + * backfill has run — NOT for every `isSystem` write, as it was. #3821 built + * this seam and then gated it on the one predicate that switches it off + * everywhere it mattered: `defineRule` — the sole implementation behind + * `POST /sharing/rules`, the documented way to deactivate a rule — writes + * with SYSTEM_CTX unconditionally, so the `isSystem` skip caught 100% of + * REST authoring. The withdrawal path was present, tested (against a mocked + * `session` the real path never sends) and unreachable in production: an + * admin saving `active: false` got a 200 and no reconcile, and because boot + * backfill then only walked ACTIVE rules, the orphaned grant outlived every + * restart. Boot phase is the honest predicate — before it, the backfill owes + * this table a pass; after it, nothing else will do the work. */ private bindRuleRebindTriggers(engine: any, ctx: PluginContext): void { const scheduleRebind = (): Promise => { @@ -310,9 +344,14 @@ export class SharingServicePlugin implements Plugin { error: err?.message, }); } - // Seeding / package bootstrap write with `isSystem`; `kernel:bootstrapped` - // backfills those, so reconciling here would only duplicate that work. - if (hookCtx?.session?.isSystem) return; + // [#4433] Skip only while the boot backfill still owes this table a + // pass. Declared-rule seeding and package bootstrap run before + // `kernel:bootstrapped`, and that pass reconciles every rule, so + // reconciling here would duplicate it. Once it has run, every write + // reconciles — regardless of `isSystem`, which cannot distinguish boot + // seeding from an admin's `POST /sharing/rules` (both arrive as + // SYSTEM_CTX from `defineRule`). + if (!this.ruleGrantsBootReconciled) return; const data = hookCtx?.result ?? hookCtx?.input?.data ?? {}; const ruleId = String(data?.id ?? hookCtx?.input?.id ?? ''); if (!ruleId) return; @@ -595,11 +634,26 @@ export class SharingServicePlugin implements Plugin { if (!this.ruleService) return; try { - const rules = await this.ruleService.listRules({ activeOnly: true }, { isSystem: true } as any); + // [#4433] EVERY rule, not `activeOnly` — a deactivated rule's grants + // are withdrawn by reconciling it, so excluding inactive rules made + // the boot pass structurally incapable of repairing them. + const rules = await this.ruleService.listRules({}, { isSystem: true } as any); await backfillRuleGrants(this.ruleService, rules, ctx.logger as any); } catch (err: any) { ctx.logger.warn('SharingServicePlugin: boot rule backfill (kernel:bootstrapped) failed', { error: err?.message }); } + // [#4433] Grants whose rule row is gone entirely are unreachable by + // reconciling rules — there is no rule left to iterate. Sweep them + // separately so "the rule is gone" and "its access is gone" mean the + // same thing after a restart, whichever path removed the rule. + try { + await this.ruleService.sweepOrphanedRuleGrants(); + } catch (err: any) { + ctx.logger.warn('SharingServicePlugin: orphaned rule-grant sweep (kernel:bootstrapped) failed', { error: err?.message }); + } + // Withdrawal is now complete for this boot; runtime rule writes own it + // from here (see bindRuleRebindTriggers). + this.ruleGrantsBootReconciled = true; }); } } diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 59fa8066b6..03b62031a3 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -246,10 +246,25 @@ export class SharingRuleService implements ISharingRuleService { const row = await this.getRule(idOrName, context); if (!row) return; // Drop materialised grants first so we don't orphan them. - await this.engine.delete('sys_record_share', { - where: { source: 'rule', source_id: row.id }, - context: SYSTEM_CTX, - } as any); + // + // [#4434] This used to be a predicate-shaped `engine.delete` on + // `sys_record_share` (`where: { source, source_id }`) with neither a + // scalar id nor `multi: true` — the one shape the engine's dispatch + // refuses, so EVERY `DELETE /sharing/rules/:idOrName` threw + // 'Delete requires an ID or options.multi=true' and answered 500 before + // it ever reached the rule row. Both address forms died on it, which left + // an over-granting rule unrecoverable from the API surface once #4433 had + // also closed the deactivation path. + // + // The fix routes through {@link purgeRuleGrants} rather than adding + // `multi: true` to the bulk call: it is the same revoke path every other + // withdrawal already uses (`evaluateRule` on an inactive rule, + // `revokeRuleGrants` after a data-API delete), so a rule's grants are + // retired exactly one way — through `SharingService.revoke`, one row at a + // time by scalar id — instead of two divergent ones. Adding `multi` here + // would have fixed the 500 while keeping delete as the only withdrawal + // that bypasses the sharing service (AGENTS.md PD #5). + await this.purgeRuleGrants(row.id); await this.engine.delete('sys_sharing_rule', { where: { id: row.id }, context: SYSTEM_CTX, @@ -281,16 +296,82 @@ export class SharingRuleService implements ISharingRuleService { return this.purgeRuleGrants(ruleId); } + /** + * [#4433] Revoke every `source: 'rule'` grant whose `source_id` no longer + * resolves to a rule row at all, and report how many went. + * + * Reconciling the rules themselves — which the boot backfill now does for + * inactive rules too — can only reach grants some surviving rule still + * claims. A grant whose rule row is GONE is unreachable that way: there is + * nothing left to iterate. Those orphans are exactly the rows #4433 found + * still answering after a restart, and they arise from every path that + * removes a rule without going through {@link deleteRule} — a data-API + * delete while the reconcile hook was unbound, a row dropped by a migration + * or by hand, a crash between the two writes in `deleteRule`. Sweeping at + * boot is what makes "the rule is gone" and "its access is gone" the same + * statement no matter which path removed it. + * + * Reads the rule ids first and diffs in memory: the grant table is the big + * one, and a per-grant existence probe would be one query per row. + */ + async sweepOrphanedRuleGrants(): Promise { + const ruleRows = await this.engine.find('sys_sharing_rule', { + fields: ['id'], + limit: 100000, + context: SYSTEM_CTX, + }); + const live = new Set(); + for (const r of (ruleRows ?? [])) live.add(String((r as any).id)); + + const grants = await this.engine.find('sys_record_share', { + where: { source: 'rule' }, + fields: ['id', 'source_id'], + limit: 100000, + context: SYSTEM_CTX, + }); + let revoked = 0; + for (const g of (grants ?? [])) { + const sourceId = (g as any).source_id; + // A `source: 'rule'` row with no `source_id` names no rule that could + // ever re-grant it — equally unreachable, equally void. + if (sourceId != null && live.has(String(sourceId))) continue; + await this.sharing.revoke(String((g as any).id), SYSTEM_CTX as any); + revoked += 1; + } + if (revoked > 0) { + this.logger?.warn?.( + '[sharing-rule] revoked rule grants whose rule row no longer exists', + { grants: revoked }, + ); + } + return revoked; + } + + /** + * Reconcile every rule on `object` against ONE record — the per-record pass + * the afterInsert/afterUpdate hooks run. + * + * [#4433] Deliberately lists ALL rules, not just active ones. Filtering to + * `activeOnly` here meant a deactivated rule was simply absent from the + * loop, so the grants it had already materialised were never even looked + * at: touching the record — the very event that created the grant — walked + * straight past it. An inactive rule is not "no rule", it is a rule whose + * desired grant set is EMPTY, and only by reconciling it can the stale rows + * be revoked. `match: false` for an inactive rule sends `reconcileForRecord` + * down its existing revoke-the-remainder branch, so nothing new is needed to + * withdraw them. + */ async evaluateAllForRecord( object: string, recordId: string, context: SharingExecutionContext, ): Promise { - const rules = await this.listRules({ object, activeOnly: true }, context); + const rules = await this.listRules({ object }, context); if (rules.length === 0) return []; const results: SharingRuleEvaluationResult[] = []; for (const rule of rules) { - const match = await this.recordMatches(rule, recordId); + // An inactive rule desires nothing; skip the criteria query entirely. + const match = rule.active ? await this.recordMatches(rule, recordId) : false; const users = match ? await this.expandRecipient(rule) : []; results.push(await this.reconcileForRecord(rule, recordId, match, users)); } diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index bc428fb478..292aef923b 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -59,6 +59,7 @@ function makeEngine() { return t[i]; }, async delete(o: string, opts?: any) { + assertDeletable(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 }; @@ -66,6 +67,24 @@ function makeEngine() { }; } +/** + * Mirror `ObjectQLEngine.delete`'s dispatch guard (objectstack#4434). + * + * The real engine routes a delete by SCALAR `where.id` to `driver.delete` and + * anything else to `driver.deleteMany` — but only when `options.multi` is set; + * otherwise it throws `'Delete requires an ID or options.multi=true'`. The fake + * used to accept any `where`, so `deleteRule`'s predicate-shaped purge of + * `sys_record_share` passed here while the running server answered 500 to every + * `DELETE /sharing/rules/:idOrName`. A fake looser than the contract it stands + * in for is how a green suite ships a dead route. + */ +function assertDeletable(opts?: any): void { + const whereId = opts?.where && typeof opts.where === 'object' ? (opts.where as any).id : undefined; + const t = typeof whereId; + const scalarId = whereId != null && (t === 'string' || t === 'number' || t === 'bigint'); + if (!scalarId && !opts?.multi) throw new Error('Delete requires an ID or options.multi=true'); +} + describe('TeamGraphService (flat — better-auth sys_team)', () => { let engine: ReturnType; beforeEach(() => { @@ -321,6 +340,146 @@ describe('SharingRuleService', () => { expect(engine._tables.sys_record_share).toHaveLength(0); }); + /** + * objectstack#4434 — `DELETE /sharing/rules/:idOrName` answered 500 for BOTH + * address forms. `deleteRule` purged `sys_record_share` with a + * predicate-shaped `engine.delete` carrying neither a scalar id nor + * `multi: true`, the one shape the engine's dispatch refuses, so it threw + * before ever reaching the rule row. With #4433 also closing the + * deactivation path, an over-granting rule had no withdrawal path left at + * all — the reason both were RC-exit blockers. + */ + it('deleteRule issues only engine-legal deletes — by NAME (#4434)', async () => { + await rules.defineRule({ + name: 'rc1_rule_livetest', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule('rc1_rule_livetest', SYS); + expect(engine._tables.sys_record_share.length).toBeGreaterThan(0); + + // The repro addressed the rule by name; the throw was unconditional. + await expect(rules.deleteRule('rc1_rule_livetest', SYS)).resolves.toBeUndefined(); + expect(engine._tables.sys_sharing_rule).toHaveLength(0); + expect(engine._tables.sys_record_share).toHaveLength(0); + }); + + it('deleteRule issues only engine-legal deletes — by ID (#4434)', async () => { + const r = await rules.defineRule({ + name: 'rc1_rule_livetest', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule(r.id, SYS); + await expect(rules.deleteRule(r.id, SYS)).resolves.toBeUndefined(); + expect(engine._tables.sys_sharing_rule).toHaveLength(0); + expect(engine._tables.sys_record_share).toHaveLength(0); + }); + + it('deleteRule withdraws grants through the sharing service, not a bulk delete (#4434)', async () => { + const r = await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule(r.id, SYS); + const granted = engine._tables.sys_record_share.length; + expect(granted).toBeGreaterThan(0); + + // Every grant retires down the same path as every other withdrawal + // (evaluateRule-on-inactive, revokeRuleGrants) — one revoke per row — + // rather than a second, divergent bulk-delete path. + const revoke = vi.spyOn(sharing, 'revoke'); + await rules.deleteRule(r.id, SYS); + expect(revoke).toHaveBeenCalledTimes(granted); + revoke.mockRestore(); + }); + + /** + * objectstack#4433 (record-touch half) — `evaluateAllForRecord` listed only + * ACTIVE rules, so a deactivated rule was absent from the loop entirely and + * the grants it had materialised were never even examined. Touching the + * record — the very event that created the grant — walked past it, which is + * step 6 of the issue's repro. + */ + it('touching a record withdraws a deactivated rule\'s grants (#4433)', async () => { + const r = await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule(r.id, SYS); + const before = engine._tables.sys_record_share.filter(s => s.record_id === 'opp1'); + expect(before.length).toBeGreaterThan(0); + + // Admin switches the rule OFF (no explicit evaluate), then the record is + // touched — the afterUpdate hook's call. + await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', active: false, + }, SYS); + const res = await rules.evaluateAllForRecord('opportunity', 'opp1', SYS); + + expect(res[0].grantsRevoked).toBe(before.length); + expect(engine._tables.sys_record_share.filter(s => s.record_id === 'opp1')).toHaveLength(0); + }); + + it('an inactive rule never re-grants on touch (#4433)', async () => { + const r = await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', active: false, + }, SYS); + await rules.evaluateAllForRecord('opportunity', 'opp1', SYS); + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + expect(r.active).toBe(false); + }); + + /** + * objectstack#4433 — a grant whose rule row is GONE cannot be reached by + * reconciling rules (there is no rule left to iterate), so it needs its own + * sweep. These are the rows left by every path that removes a rule without + * `deleteRule`: a data-API delete while the hook was unbound, a migration, a + * crash between `deleteRule`'s two writes. + */ + it('sweepOrphanedRuleGrants revokes grants whose rule row is gone (#4433)', async () => { + const r = await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule(r.id, SYS); + const granted = engine._tables.sys_record_share.length; + expect(granted).toBeGreaterThan(0); + + // Rule row vanishes behind the service's back. + engine._tables.sys_sharing_rule = []; + + expect(await rules.sweepOrphanedRuleGrants()).toBe(granted); + expect(engine._tables.sys_record_share).toHaveLength(0); + }); + + it('sweepOrphanedRuleGrants leaves live rule grants and manual shares alone (#4433)', async () => { + const r = await rules.defineRule({ + name: 'hv', label: 'HV', object: 'opportunity', + criteria: { amount: { $gte: 100000 } }, + recipientType: 'team', recipientId: 'sales', + }, SYS); + await rules.evaluateRule(r.id, SYS); + const ruleGrants = engine._tables.sys_record_share.length; + // A hand-made share, plus a grant from a rule that no longer exists. + engine._tables.sys_record_share.push( + { id: 'manual1', object_name: 'opportunity', record_id: 'opp1', recipient_id: 'zoe', source: 'manual' }, + { id: 'orphan1', object_name: 'opportunity', record_id: 'opp1', recipient_id: 'zoe', source: 'rule', source_id: 'srule_gone' }, + ); + + expect(await rules.sweepOrphanedRuleGrants()).toBe(1); + expect(engine._tables.sys_record_share).toHaveLength(ruleGrants + 1); + expect(engine._tables.sys_record_share.some(s => s.id === 'manual1')).toBe(true); + expect(engine._tables.sys_record_share.some(s => s.id === 'orphan1')).toBe(false); + }); + it('inactive rule purges grants on evaluate', async () => { const r = await rules.defineRule({ name: 'hv', label: 'HV', object: 'opportunity', From ad2ced56eab19890ea361ef07c2c7670e359d4f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:51:44 +0000 Subject: [PATCH 2/3] test(sharing): pin the boot-phase reconcile contract (#4433) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rule-rebind.test.ts asserted the defect: 'skips system-context writes' passed a mocked session the real REST path never sends, then asserted the reconcile did NOT happen. Replaced with the corrected contract — deferral is about WHEN a write happens (before the kernel:bootstrapped backfill), not WHO made it — plus cases for both session kinds after boot and the seeding deferral during it. pnpm --filter @objectstack/plugin-sharing test: 243 passed (11 files). Refs #4433 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../plugin-sharing/src/rule-rebind.test.ts | 90 ++++++++++++++++++- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts index c5a1a6d6ec..85d42d42b5 100644 --- a/packages/plugins/plugin-sharing/src/rule-rebind.test.ts +++ b/packages/plugins/plugin-sharing/src/rule-rebind.test.ts @@ -201,12 +201,26 @@ describe('SharingServicePlugin reconciles grants on rule writes (#3821)', () => expect(ruleService.evaluateRule).not.toHaveBeenCalled(); }); - it('skips system-context writes — boot backfill owns those', async () => { - await engine.fire('afterInsert', 'sys_sharing_rule', { - result: { id: 'r1' }, + /** + * objectstack#4433 — this is the defect, and it hid behind the test that + * used to live here ("skips system-context writes"). + * + * `SharingRuleService.defineRule` — the ONLY implementation behind + * `POST /sharing/rules`, the documented way to deactivate a rule — writes + * `sys_sharing_rule` with SYSTEM_CTX unconditionally, because it has to + * reach a platform table the sharing middleware otherwise gates. So the old + * `session.isSystem` skip did not filter out "boot seeding"; it filtered out + * every REST authoring write there is. The old test passed a mocked + * `session: { isSystem: true }` that the real REST path never sends, and + * asserted the reconcile did NOT happen — pinning the bug as the contract. + */ + it('reconciles a SYSTEM_CTX authoring write once boot is done (#4433)', async () => { + // Exactly what `POST /sharing/rules` with `active: false` produces. + await engine.fire('afterUpdate', 'sys_sharing_rule', { + result: { id: 'r1', active: false }, session: { isSystem: true }, }); - expect(ruleService.evaluateRule).not.toHaveBeenCalled(); + expect(ruleService.evaluateRule).toHaveBeenCalledWith('r1', expect.objectContaining({ isSystem: true })); }); it('never fails the authoring write when reconciliation throws', async () => { @@ -233,3 +247,71 @@ describe('SharingServicePlugin reconciles grants on rule writes (#3821)', () => expect(order).toEqual(['rebind', 'reconcile']); }); }); + +/** + * [#4433] Boot phase — the predicate that replaced the `isSystem` skip. + * + * The skip exists to avoid duplicating work: declared-rule seeding and package + * bootstrap write `sys_sharing_rule` before `kernel:bootstrapped`, and that + * pass reconciles every rule anyway. That is a statement about WHEN a write + * happens, not about WHO made it — so it is gated on boot phase, which is true + * for exactly the writes the backfill covers and false for every runtime one. + */ +describe('SharingServicePlugin defers reconciliation to the boot backfill (#4433)', () => { + let engine: ReturnType; + let plugin: SharingServicePlugin; + let ruleService: AnyRecord; + + beforeEach(() => { + engine = makeEngine(); + plugin = new SharingServicePlugin(); + ruleService = { + listRules: vi.fn(async () => []), + evaluateRule: vi.fn(async () => ({ ruleId: 'r1', matchedRecords: 0, expandedUsers: 0, grantsCreated: 0, grantsUpdated: 0, grantsRevoked: 0 })), + revokeRuleGrants: vi.fn(async () => 0), + sweepOrphanedRuleGrants: vi.fn(async () => 0), + }; + (plugin as any).ruleService = ruleService; + (plugin as any).bindRuleRebindTriggers(engine, makeCtx()); + // Boot still in flight — `ruleGrantsBootReconciled` defaults to false. + }); + + it('skips the per-write reconcile while boot is still in flight', async () => { + await engine.fire('afterInsert', 'sys_sharing_rule', { result: { id: 'seeded' } }); + expect(ruleService.evaluateRule).not.toHaveBeenCalled(); + }); + + it('still rebinds the lifecycle hooks during boot', async () => { + // Deferring the reconcile must not defer the binding — a rule seeded at + // boot has to be enforceable for records written straight afterwards. + await engine.fire('afterInsert', 'sys_sharing_rule', { result: { id: 'seeded' } }); + expect(ruleService.listRules).toHaveBeenCalled(); + }); + + it('reconciles every write once the backfill has run — user session', async () => { + (plugin as any).ruleGrantsBootReconciled = true; + await engine.fire('afterUpdate', 'sys_sharing_rule', { + result: { id: 'r1', active: false }, + session: { userId: 'admin' }, + }); + expect(ruleService.evaluateRule).toHaveBeenCalledWith('r1', expect.objectContaining({ isSystem: true })); + }); + + it('reconciles every write once the backfill has run — system session', async () => { + (plugin as any).ruleGrantsBootReconciled = true; + await engine.fire('afterUpdate', 'sys_sharing_rule', { + result: { id: 'r1', active: false }, + session: { isSystem: true }, + }); + expect(ruleService.evaluateRule).toHaveBeenCalledWith('r1', expect.objectContaining({ isSystem: true })); + }); + + it('purges on a post-boot delete regardless of session kind', async () => { + (plugin as any).ruleGrantsBootReconciled = true; + await engine.fire('afterDelete', 'sys_sharing_rule', { + input: { id: 'r1' }, + session: { isSystem: true }, + }); + expect(ruleService.revokeRuleGrants).toHaveBeenCalledWith('r1'); + }); +}); From db2e901c1554de174536283ae269b5649f6be2d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 12:16:19 +0000 Subject: [PATCH 3/3] docs(sharing): document the withdrawal lifecycle + add changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live verification on a persistent SQLite datastore confirmed all three withdrawal moments and both DELETE address forms, so the guarantee is now worth stating: an over-granting sharing rule is always recoverable from the API surface. The CI docs-drift advisory flagged six hand-written pages against this change. None asserted anything false — the withdrawal timing was simply undocumented, which is how the behaviour drifted unnoticed in the first place. Added the missing section to permissions/sharing-rules.mdx (the three reconcile moments, delete-by-id-or-name, the boot sweep for grants whose rule row is gone) and sharpened permissions-matrix.mdx, which said rules re-evaluate on record insert/update and stopped there — true but partial now that rule writes and boot also reconcile. The other four flagged pages only list the plugin or its layer and needed no change. content/docs/references/ is generated and was not touched. Changeset: minor on @objectstack/plugin-sharing. Not patch — a `source: 'rule'` grant whose rule is inactive or gone now disappears on upgrade, and DELETE starts succeeding where it used to 500, so callers that treated that 500 as "unsupported" will now really delete. Refs #4433, #4434 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../sharing-rule-withdrawal-and-delete.md | 55 +++++++++++++++++++ .../docs/permissions/permissions-matrix.mdx | 2 +- content/docs/permissions/sharing-rules.mdx | 32 +++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 .changeset/sharing-rule-withdrawal-and-delete.md diff --git a/.changeset/sharing-rule-withdrawal-and-delete.md b/.changeset/sharing-rule-withdrawal-and-delete.md new file mode 100644 index 0000000000..1eb87c64da --- /dev/null +++ b/.changeset/sharing-rule-withdrawal-and-delete.md @@ -0,0 +1,55 @@ +--- +"@objectstack/plugin-sharing": minor +--- + +fix(plugin-sharing): deactivating or deleting a sharing rule actually withdraws its grants (#4433, #4434) + +An over-granting sharing rule had no withdrawal path on the product's API +surface. Deactivating it left every grant it had materialised in place — not on +the next record touch, not after a full restart — and the DELETE route answered +500 for both address forms it advertises, so the rule could not be removed +either. Together that made a too-broad rule unrecoverable short of hand-editing +`sys_record_share`, against a v17 release note that advertises the opposite +("switching a rule off actually withdraws access"). + +`minor`, not `patch`: this changes an observable runtime behaviour that +deployments may have adapted to. A `source: 'rule'` grant whose rule is +inactive — or whose rule row is gone — now disappears, on the deactivating +write, on the next touch of the record, and on the next boot. Anything relying +on those rows surviving deactivation (including data repaired by hand around +the old behaviour) will see them revoked on upgrade. `DELETE +/api/v1/sharing/rules/:idOrName` also starts succeeding where it used to 500, +so callers that treated that 500 as "unsupported" will now really delete. + +#4433 — three independent gaps, one per path the report walked: + +- **The deactivating write.** The `sys_sharing_rule` reconcile trigger skipped + every `isSystem` write, on the theory that those were boot seeding. + `SharingRuleService.defineRule` — the only implementation behind + `POST /sharing/rules`, and the documented way to deactivate a rule — writes + with SYSTEM_CTX unconditionally, because it must reach a platform table the + sharing middleware otherwise gates. So the skip caught 100% of REST + authoring: the withdrawal path built by #3821 existed, had tests (against a + mocked session the real path never sends), and was unreachable in production. + Now gated on boot phase, which is the question the skip actually meant to + ask. +- **The record touch.** `evaluateAllForRecord` listed only active rules, so a + deactivated rule was absent from the loop entirely and its grants were never + examined. It now reconciles every rule; an inactive one desires nothing and + takes the existing revoke-the-remainder branch. +- **The boot pass.** `backfillRuleGrants` was handed an `activeOnly` list, + making it structurally incapable of revoking anything. It now walks every + rule, and a new `sweepOrphanedRuleGrants` retires grants whose rule row is + gone entirely — unreachable by rule iteration, so they need their own sweep. + +#4434 — `deleteRule` purged `sys_record_share` with a predicate-shaped +`engine.delete` carrying neither a scalar id nor `multi: true`, the one shape +the engine's dispatch refuses; it threw before ever reaching the rule row. +Fixed by routing through the same `SharingService.revoke` path every other +withdrawal already uses, rather than adding `multi: true` — a rule's grants now +retire exactly one way instead of two divergent ones. + +The unit fakes are part of the fix: `makeEngine().delete` accepted any `where`, +which is why #4434 shipped green — the pre-existing "deleteRule drops rule and +all its grants" test asserted success against a delete the running server +always rejected. The fakes now mirror the real engine's dispatch guard. diff --git a/content/docs/permissions/permissions-matrix.mdx b/content/docs/permissions/permissions-matrix.mdx index 5b37b96834..a7e88796a2 100644 --- a/content/docs/permissions/permissions-matrix.mdx +++ b/content/docs/permissions/permissions-matrix.mdx @@ -161,7 +161,7 @@ Sharing rules extend access beyond ownership and the depth axis. The declarative -**Beyond declarative rules:** Two other sharing mechanisms exist but are **not** `SharingRule` types. **Manual sharing** is a runtime grant — `sys_record_share` rows created with `source: 'manual'` (see `packages/plugins/plugin-sharing/src/sharing-service.ts`). **Matrix / territory-shaped access** is served today by multi-position assignment anchored on business units, plus pre-resolved membership sets: a registered `rls-membership-resolver` (ADR-0105 D11) stages e.g. `territory_account_ids` into `ExecutionContext.rlsMembership`, and a policy references it as `account_id IN (current_user.territory_account_ids)`. The aspirational `TerritorySchema` was removed in ADR-0105 D11 — it had no runtime object, stack field or resolver; a generalized dimension-security module will arrive with its own ADR. Owner/criteria rules are re-evaluated on insert/update via internal sharing rule hooks. +**Beyond declarative rules:** Two other sharing mechanisms exist but are **not** `SharingRule` types. **Manual sharing** is a runtime grant — `sys_record_share` rows created with `source: 'manual'` (see `packages/plugins/plugin-sharing/src/sharing-service.ts`). **Matrix / territory-shaped access** is served today by multi-position assignment anchored on business units, plus pre-resolved membership sets: a registered `rls-membership-resolver` (ADR-0105 D11) stages e.g. `territory_account_ids` into `ExecutionContext.rlsMembership`, and a policy references it as `account_id IN (current_user.territory_account_ids)`. The aspirational `TerritorySchema` was removed in ADR-0105 D11 — it had no runtime object, stack field or resolver; a generalized dimension-security module will arrive with its own ADR. Criteria rules are re-evaluated on record insert/update via internal sharing rule hooks, on every write to the rule itself, and once per boot — so deactivating or deleting a rule withdraws the grants it materialized (see [Sharing Rules](/docs/permissions/sharing-rules#switching-a-rule-off-withdraws-the-access-it-granted)). ### Configuration Example diff --git a/content/docs/permissions/sharing-rules.mdx b/content/docs/permissions/sharing-rules.mdx index b0eb488cf9..cfdf7c9ebb 100644 --- a/content/docs/permissions/sharing-rules.mdx +++ b/content/docs/permissions/sharing-rules.mdx @@ -161,6 +161,38 @@ covered; an unauthorized call fails with `403 PERMISSION_DENIED`. Boot seeding, lifecycle hooks, and backfills run as system context and are unaffected. +### Switching a rule off withdraws the access it granted + +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: + +| 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 | +| **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 +`DELETE {basePath}/sharing/rules/:idOrName` (by id **or** by name) or through +the plain data API in Setup. + +The practical guarantee: **an over-granting rule is always recoverable from the +API surface.** Switch it off or delete it, and the access it materialized is +gone — not on the next time somebody happens to touch the record, and not only +after a restart (objectstack#4433, #4434). A grant whose rule row has vanished +entirely is retired by the boot sweep, so a database repaired by hand — or +upgraded from a build that leaked these rows — converges on the next start. + + + Because withdrawal is materialized rather than computed at read time, the + revocation is visible in `sys_record_share` itself. `GET + {basePath}/data/:object/:id/shares` is the fastest way to confirm a rule's + access is really gone, and `POST {basePath}/sharing/rules/:idOrName/evaluate` + forces a reconcile on demand. + + ### There is no "share every record" rule The predicate is **mandatory on every authoring path**, whether you declare