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
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..85d42d42b5 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 () => {
@@ -196,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 () => {
@@ -228,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');
+ });
+});
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',