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