Skip to content

Commit c272e48

Browse files
os-zhuangclaude
andauthored
fix(plugin-sharing): recompute sharing rules for predicate (multi) writes (#4779) (#5102)
`bindRuleHooks` located the rows to recompute from a single record id (`if (!id) return`), and `ObjectQL.update()` only populates `input.id` for a scalar `where.id`. A predicate write routes to `updateMany` and carries no id, so every bulk write skipped sharing-rule recompute entirely: records bulk-moved out of a rule's criteria kept the `sys_record_share` rows the rule had issued, and their recipients kept access the rules no longer implied. Fail-open on the authorization side; same family as #4757 and #4778. Keyed off the write's ROW SET instead of one id. `beforeUpdate`/`beforeDelete` resolve the affected rows from the predicate and stash them on the shared hook context (the before hook is where it must happen — the write is what makes those rows unfindable); the after hook acts on them. Per the maintainer's ruling (option C): - bounded set (<= RULE_RECOMPUTE_ROW_CAP = 1000) -> per-row `evaluateAllForRecord`, synchronous, diff-based so both directions are covered (out of the criteria revokes, into it grants); - unbounded set (over cap / `multi` with no `where` / failed resolve) -> synchronous set-based revoke of the object's rule grants, then asynchronous re-grant via `evaluateAllRulesForObject`. The write is never refused: that would leak an internal recompute bound out as a business limit on how many rows an admin may update. The asymmetry it trades on is that over-granting is a security incident while under-granting is an availability wobble, so the safety half is always synchronous and complete and only the expensive restoration half is deferred. The re-grant is in-process rather than routed through the OPTIONAL `IJobService`, which would make the guarantee composition-dependent; durability comes from the plugin's existing `kernel:bootstrapped` backfill, which re-runs the same idempotent reconcile. Also binds `afterDelete` and retires the deleted records' rule grants (the orphan noted at the tail of the issue). Nothing else could reach them: `evaluateRule` iterates records that still exist, so a grant whose record is gone outlived every reconcile path and every restart. New on SharingRuleService: revokeRuleGrantsForObject, revokeRuleGrantsForRecords, evaluateAllRulesForObject. Manual shares are never touched. Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t Co-authored-by: Claude <noreply@anthropic.com>
1 parent b40f81c commit c272e48

7 files changed

Lines changed: 1103 additions & 13 deletions

File tree

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
fix(plugin-sharing): recompute sharing rules for predicate (`multi`) writes — stale `sys_record_share` grants no longer survive a bulk update (#4779)
6+
7+
`bindRuleHooks` located the rows to recompute from a single record id:
8+
9+
```ts
10+
const id = String(data?.id ?? ctx?.input?.id ?? '');
11+
if (!id) return;
12+
```
13+
14+
`ObjectQL.update()` only populates `input.id` when `where.id` is a scalar. A
15+
predicate write (`multi: true`) routes to `updateMany`, leaves `input.id`
16+
undefined, and carries no id in its payload — so **every bulk write skipped
17+
sharing-rule recompute entirely**.
18+
19+
The consequence is a fail-open on the authorization side. A criteria-based rule
20+
materialises `sys_record_share` rows; an admin then bulk-updates those records
21+
out of the criteria (`{ where: { region: 'east' }, multi: true, data: { region:
22+
'west' } }`); nothing recomputes, the grant rows stay in the table, and the
23+
recipients keep the read/edit access the rule no longer implies. Same family as
24+
#4757 (`sys_attachment`) and #4778 (approval locks), but better hidden — a stale
25+
grant is indistinguishable from a legitimate one. The reverse direction (bulk
26+
update **into** a rule's criteria never granting) was broken too.
27+
28+
**What changes**
29+
30+
The hooks now key off the write's ROW SET instead of one id. `beforeUpdate` /
31+
`beforeDelete` resolve the affected rows from the predicate and stash them on
32+
the shared hook context (the `before` hook is where it must happen — the write
33+
is what makes those rows unfindable); the `after` hook acts on them:
34+
35+
- **Bounded set (≤ 1000 rows, `RULE_RECOMPUTE_ROW_CAP`)**`evaluateAllForRecord`
36+
per row, synchronously. Diff-based, so this covers both directions: rows moved
37+
out of a rule's criteria are revoked, rows moved in are granted.
38+
- **Unbounded set** (over the cap, `multi: true` with no `where` at all, or a
39+
resolve that failed) — every `source: 'rule'` grant on the object is revoked
40+
**synchronously** in one set-based statement, and the deserved grants are
41+
restored **asynchronously** by reconciling the object's rules.
42+
43+
**The write is never refused.** Refusing would turn an internal recompute bound
44+
into a business-visible limit on how many rows an admin may update, reported by
45+
a subsystem they never configured. The asymmetry it trades on instead:
46+
over-granting is a security incident, under-granting is an availability wobble.
47+
So the safety half is always synchronous and complete, and only the expensive
48+
restoration half is deferred.
49+
50+
**Operational note.** After a bulk write whose row set could not be bounded,
51+
recipients may briefly lose access to records they still qualify for, until the
52+
background re-grant finishes. It is logged with the object and the reason. The
53+
re-grant is in-process; if it is lost to a crash, the plugin's existing
54+
`kernel:bootstrapped` backfill re-runs the same idempotent reconcile on the next
55+
start, and any subsequent `sys_sharing_rule` write reconciles too.
56+
57+
**Also fixed:** the rule hooks now bind `afterDelete` and retire the deleted
58+
records' rule grants. Nothing else could: `evaluateRule` iterates records that
59+
still exist, so a grant whose record is gone was unreachable by every reconcile
60+
path and outlived restarts. Harmless only while record ids are never reused —
61+
an assumption nothing in the platform enforces.
62+
63+
New on `SharingRuleService`: `revokeRuleGrantsForObject`,
64+
`revokeRuleGrantsForRecords` and `evaluateAllRulesForObject`. Manual
65+
(`source: 'manual'`) shares are never touched by any of them.

0 commit comments

Comments
 (0)