fix(sharing): 停用规则时撤回已物化授权,并修复 DELETE 500 (#4433, #4434)

.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.

content/docs/permissions/permissions-matrix.mdx

@@ -161,7 +161,7 @@ Sharing rules extend access beyond ownership and the depth axis. The declarative
 </Callout>
 
 <Callout type="info">
-**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)).
 </Callout>
 
 ### Configuration Example

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.
+
+<Callout type="info">
+  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.
+</Callout>
+
 ### There is no "share every record" rule
 
 The predicate is **mandatory on every authoring path**, whether you declare

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<typeof makeEngine>;
+  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<typeof makeEngine>;
 

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<typeof makeEngine>;
+  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');
+  });
+});