diff --git a/.changeset/system-write-sharing-materialization.md b/.changeset/system-write-sharing-materialization.md new file mode 100644 index 0000000000..ebce8b0c45 --- /dev/null +++ b/.changeset/system-write-sharing-materialization.md @@ -0,0 +1,46 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): let system writes materialize sharing rules — drop the `isSystem` skips in `bindRuleHooks` (#13533) + +A criteria sharing rule declares a promise: `status == "approved"` means the +named recipients can see the record. `bindRuleHooks` did not keep that promise +when the platform was the writer. Its `afterInsert` and `afterUpdate` hooks +returned early on `ctx.session.isSystem`, so a system-context write that moved a +record INTO a rule's criteria materialized no `sys_record_share` row at all. + +The path that made this a real outage rather than a boot-time curiosity is +approval write-back. An approval node with `lockRecord: true` mirrors the +decision onto the subject record under a system context — that is the only write +that can land while the record is locked — so a manager approving a leave +request produced exactly the skip above. A teammate who relied on the rule could +not see the record, and nothing repaired it until somebody ran +`POST /api/v1/sharing/rules/:id/evaluate` or restarted the server. The failure +was invisible from a manager or admin view, which reads through the profile's +`viewAllRecords` grant and never consults a sharing rule at all. + +**What changes.** A system write now materializes exactly as a user write does — +grants on the way into a rule's criteria, revokes on the way out, per record, +synchronously with the write. Three early returns are gone: the two on +`afterInsert` / `afterUpdate`, and the one on the `beforeUpdate` / `beforeDelete` +row-set stash they depended on (without that stash an `after` hook reads the row +set as *unbounded*, which would have turned every single-row system update into +an object-wide revoke plus an asynchronous re-grant). The INFO line that +announced the skip — `[sharing-rule] sharing materialisation skipped for isSystem +writes; re-evaluate rules or restart to backfill` — is retired with it, along +with the `SYSTEM_WRITE_SKIP_NOTICE` constant, which was not exported from the +package entry point. Operationally this means seed- and import-time system writes +on rule-covered objects now pay per-record sharing evaluation at write time — the +cost a user write of the same shape has always paid, with the +`kernel:bootstrapped` backfill still reconciling behind it. + +**What does not change.** `afterDelete` still skips system writes, on separate +grounds: what it skips is revocation, and `record-share-cascade.ts` delivers that +on every sharing-capable object, stashing for system writes on its own account. +The `kernel:bootstrapped` boot backfill still runs and is still needed — it +reaches rows no hook saw, and it is the only pass that purges a deactivated +rule's grants. No new option, flag or declarative switch: the fix is the removal. + +No published export moves; `SYSTEM_WRITE_SKIP_NOTICE` was never re-exported from +the package entry point. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 6990eb7383..7e8762b2d9 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -9,9 +9,9 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a service self-write, a migration. This page is **the authority** for what that flag actually does. It exists -because the flag is not one concept: it is a single boolean read at **109 +because the flag is not one concept: it is a single boolean read at **106 distinct sites across 20 packages**, and knowing three of those behaviours gives -no hint that the other hundred-and-six exist. Every documented app-side bug +no hint that the other hundred-and-three exist. Every documented app-side bug traced to `isSystem` had the same shape — the metadata was complete and correct, and the gap was observable only by querying the resulting rows. @@ -124,18 +124,18 @@ that silently does not happen. ### 3. Sharing (`plugin-sharing`) -The largest single consumer — **20 of the 109 sites**. +The largest single consumer — **17 of the 106 sites**. | # | Behaviour when `isSystem` | What you get / what you lose | Anchor | |:--|:---|:---|:---| -| 30 | **Sharing-rule grant materialisation is skipped on all four record-write hooks** | Lose: **no `sys_record_share` rows are created**. A fully configured sharing rule grants **nothing** on seeded data until a rule is re-evaluated or the boot backfill runs. This is the behaviour that motivated #4707. Since #6783 the skip is no longer silent — it emits an INFO notice (rough edge 2) | `rule-hooks.ts:250`, `:274`, `:293`, `:322` | +| 30 | **Sharing-rule REVOCATION is skipped on the record-`afterDelete` hook** — and on that hook only | Lose: nothing permanently — the revoke is **delivered, but deferred on the unbounded shape**. The payload belongs to another subscriber: `record-share-cascade.ts` binds on every sharing-capable object and stashes for system writes on its own account (#5103). When the deleted ids are enumerable it revokes inline; when they are not — a predicate delete whose row set the stash could not resolve — it hands the reclaim to a queued background orphan sweep instead, so the share rows outlive the deleted records until that sweep runs, with the boot orphan sweep behind it. No surviving record loses access either way, and a restart re-runs the same sweep. This is one subscriber declining work another owns, not elevation silencing a consequence. ⚠️ **Grant MATERIALISATION no longer asks** — the `afterInsert` / `afterUpdate` skips, and the `before*` stash skip that fed them, were removed by the 2026-08-31 ruling on #13533; a system write materialises exactly as a user write does | `rule-hooks.ts:292` | | 31 | Sharing write verdict short-circuits to `allow` | Get: writes pass the sharing gate unconditionally | `plugin-sharing/src/sharing-service.ts:677` | | 32 | Record visibility / manage-shares checks return true | Get: no ownership or Modify-All requirement | `plugin-sharing/src/sharing-service.ts:943`, `:1030`, `:1787` | | 33 | `grant()` skips the enforcement + manage-shares assertions | Get: the rule evaluator can materialise through the public API. Note it is **not** a bare skip: the system branch asserts the grant is not *inert* instead (a grant on an object no verdict can consult is refused) | `plugin-sharing/src/sharing-service.ts:1238` | | 34 | `revoke()` deletes directly, **before** the non-manual-source guard | Get: the evaluator can revoke its own grants. Lose: the `CONFLICT` guard that warns a rule-materialised grant will be silently re-granted on the next reconcile | `plugin-sharing/src/sharing-service.ts:1476` (guard at `:1501`) | | 35 | `listShares()` skips the management gate | Get: full enumeration of who can see a record | `plugin-sharing/src/sharing-service.ts:1528` | -| 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1077` | -| 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link creation/resolution while the policy is off | `plugin-sharing/src/share-link-service.ts:449`, `:503`, `:507`, `:580`, `:610` | +| 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1088` | +| 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `plugin-sharing/src/share-link-service.ts:449`, `:503`, `:507`, `:580`, `:610` | | 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:47` | | 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:157`, `:382` | @@ -217,14 +217,32 @@ should recognise it instead of re-deriving it. (`plugin-security/src/claim-seed-ownership.ts`). Any *third* system write path gets neither. If you add one, stamp ownership yourself. -2. **Sharing materialisation is skipped, and now says so.** Row 30 produces +2. **Sharing materialisation is no longer skipped — the rough edge is closed, + and it is recorded here rather than deleted.** This entry used to describe "configured but inert": nine installed sharing rules, matching records, - correct positions — and `sys_record_share` empty. The boot backfill does - eventually fix it, so the behaviour is not wrong; it was undiscoverable. - The INFO notice tracked as #6783 **has since shipped** - (`rule-hooks.ts:274`, `:293` call `noteSystemWriteSkipped`), so the skip is - now observable — the earlier edition of this page said it was not, and that - sentence was already stale when the anchors were re-censused. + correct positions — and `sys_record_share` empty, repaired only by a + re-evaluation or a restart. The reading that made it a rough edge rather + than a defect was that the boot backfill eventually fixes it, so the + behaviour was correct and merely undiscoverable; #6783 shipped an INFO + notice on that reasoning. + + That reading did not survive contact with a runtime write. An approval + node's write-back is a system write — `lockRecord: true` means only a + platform write can land while the record is locked — and it happens long + after boot, so no backfill was coming: a manager approved a request and the + teammate who depended on the criteria rule `status == "approved"` could not + see it at all. The maintainer ruled on 2026-08-31 (#13533) that the skip was + a **bug**, because a sharing rule's declared semantics is a published + promise and `isSystem` names the operator, never a consequence that need not + happen. Both materialisation skips and the notice that announced them are + gone; row 30 is now the `afterDelete` skip alone, which survives on the + separate ground that another subscriber delivers that payload. + + ⚠️ The observability half of that reading is worth keeping in mind + independently: the only signal a builder ever had was one INFO line, and + nothing in the docs or the tests said "after approval, when does the team + see it?". A compensating path that exists but that nobody can be expected to + know about is not a compensating path. 3. **Strict write observability is inert under elevation.** Row 22: a caller that asked to be told loudly about dropped fields is told nothing, because @@ -251,7 +269,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are independent decisions, and a seed loader plausibly wants the first two but not the third. The concept is nevertheless **staying as one boolean**: -- **Shipped semantics.** `isSystem` is a published contract with 109 read sites +- **Shipped semantics.** `isSystem` is a published contract with 106 read sites in 20 packages. Splitting it is a breaking contract change across all of them. (The ruling was taken when the census read 80 sites in 18 packages; the count has grown, which strengthens rather than weakens the argument.) @@ -308,12 +326,12 @@ still holds equal to the census on every pull request: | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — | | — parsed as a declaration | 22 | ✅ | | — parsed as an object-literal / type key (producers and option objects) | 310 | — | -| — parsed as a property **read** | 115 | ✅ | +| — parsed as a property **read** | 112 | ✅ | | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ | | — the remainder: text inside comments and string literals | 358 | — | | Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ | -| Of those reads: reads of `ExecutionContext.isSystem` | **109** | ✅ | -| — behaviour-bearing (rows 1–61 above) | 105 | ✅ | +| Of those reads: reads of `ExecutionContext.isSystem` | **106** | ✅ | +| — behaviour-bearing (rows 1–61 above) | 102 | ✅ | | — carry the flag onward only (rows 62–65 above) | 4 | ✅ | | Packages containing at least one elevation read | **20** | ✅ | | Files containing at least one elevation read | 45 | ✅ | diff --git a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts index 9f52dc8d89..e3167282b8 100644 --- a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts +++ b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts @@ -3,12 +3,17 @@ /** * [#2926 ③] Boot backfill of sharing-rule grants. * - * Rule grants are materialized by write hooks, which deliberately skip - * `isSystem` writes (rule-hooks.ts) — so records created by the boot-time - * seed loader (always `isSystem`) never produced `sys_record_share` rows: - * demo data shipping with matching sharing rules was broken out of the box - * until an admin "touched" each record at runtime. `backfillRuleGrants` - * reconciles every active rule once per boot, idempotently. + * Rule grants are materialized by write hooks, and this pass reconciles every + * rule once per boot, idempotently, for the rows those hooks did not reach. + * + * [#13533] The original reason for that gap was that the hooks skipped + * `isSystem` writes, so boot-time seed rows (always `isSystem`) never produced + * `sys_record_share` rows and demo data shipped inert. That skip is gone — a + * system write now materialises like any other — but this pass is NOT + * obsolete, and the two remaining reasons are what these tests cover: rows + * written while a rule was inactive or before its hooks were bound are still + * unreached by any hook, and this is the only pass that PURGES a deactivated + * rule's grants (#4433), which no write-path hook can do. */ import { describe, it, expect, beforeEach, vi } from 'vitest'; diff --git a/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts index d78c6b69f0..361513e60e 100644 --- a/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts +++ b/packages/plugins/plugin-sharing/src/bu-tree-recompute.ts @@ -205,13 +205,17 @@ export function writeCanChangeExpansion(objectName: string, event: string, hookC * nothing here needs the pre-write state. What the revoke needs is the tree as * it is NOW, and the write is what makes that true. * - * **System writes are NOT skipped**, which is the opposite of what - * `bindRuleHooks` does and is deliberate. Its skip is about grant - * MATERIALISATION, which the boot backfill re-does anyway. Here the payload is - * REVOCATION, and the realistic production trigger for a re-parent is an HRIS - * or directory sync — a system write. Skipping those would leave the hole open - * on the very path most likely to open it. `primary-bu-projection.ts` reached - * the same conclusion for the same table. + * **System writes are NOT skipped**, and this file's hooks never carried an + * `isSystem` branch to skip them with. `bindRuleHooks` no longer skips them + * either: its `afterInsert` / `afterUpdate` materialisation skips, and the + * `before*` stash skip that fed them, were removed by the 2026-08-31 ruling on + * #13533, so a system write there materialises grants exactly as a user write + * does — the one skip it keeps is `afterDelete` revocation, which + * `record-share-cascade.ts` delivers instead. Here the payload is REVOCATION, + * and the realistic production trigger for a re-parent is an HRIS or directory + * sync — a system write. Skipping those would leave the hole open on the very + * path most likely to open it. `primary-bu-projection.ts` reached the same + * conclusion for the same table. */ export function bindBusinessUnitTreeRecompute( engine: MinimalEngine, diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts index 0c4a9064aa..7a339bcc76 100644 --- a/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.test.ts @@ -40,7 +40,11 @@ import { interface Row { [k: string]: any } const SYS = { isSystem: true, positions: [], permissions: [] } as any; -/** A non-system session — the hooks deliberately skip `isSystem` writes. */ +/** + * The default session for this suite: an interactive admin. [#13533] It is no + * longer the only session the hooks act on — a system write takes the same + * path — so this is a default, not a precondition. + */ const ADMIN_SESSION = { isSystem: false, userId: 'admin' }; type HookEntry = { event: string; handler: (ctx: any) => any; options: Row }; @@ -343,16 +347,37 @@ describe('#4779 predicate (multi) writes recompute sharing rules', () => { expect(ruleShares(engine)).toEqual([]); }); - it('leaves system-context bulk writes to the boot backfill, as before', async () => { + /** + * [#13533] REVERSED, not deleted. This test used to read: + * + * it('leaves system-context bulk writes to the boot backfill, as before') + * … expect(ruleShares(engine)).toHaveLength(2); // untouched by the hooks + * + * and it pinned a real behaviour: `bindRuleHooks` skipped `session.isSystem` + * on `afterUpdate` (and on the `before*` stash that fed it), so a system bulk + * write changed no grants and `kernel:bootstrapped`'s backfill owned the + * repair. The maintainer reversed that on 2026-08-31 (verbatim, untranslated: + * 「裁定:系统写参与逐记录共享物化 —— 删除 `plugin-sharing` 两个钩子里的 + * `isSystem` 跳过,⛔ 不加声明式开关、不以文档代修。」), on the ground that a + * rule's declared semantics is a published promise and `isSystem` names the + * OPERATOR, never a consequence that need not happen. + * + * So the same write is now expected to do what the identical user-context + * write two tests up does — and the assertion below is deliberately the same + * shape as that one, because "the same" is the whole content of the ruling. + */ + it('recomputes system-context bulk writes too — identically to a user write (#13533)', async () => { seed(2, 'east'); await rules.evaluateRule('srule_east', SYS); + expect(ruleShares(engine)).toHaveLength(2); await engine.simulateBulkUpdate( 'opportunity', { region: 'east' }, { region: 'west' }, { isSystem: true }, ); - // Untouched by the hooks — `kernel:bootstrapped`'s backfill owns seeds. - expect(ruleShares(engine)).toHaveLength(2); + // Moved OUT of the criteria, so the grants are revoked — exactly as the + // user-context twin above revokes them. + expect(ruleShares(engine)).toEqual([]); }); it('does not touch manual shares — only rule-materialised ones', async () => { diff --git a/packages/plugins/plugin-sharing/src/rule-hooks.ts b/packages/plugins/plugin-sharing/src/rule-hooks.ts index 027726718c..23baa61dae 100644 --- a/packages/plugins/plugin-sharing/src/rule-hooks.ts +++ b/packages/plugins/plugin-sharing/src/rule-hooks.ts @@ -29,32 +29,37 @@ export const RULE_REBIND_TRIGGER_PACKAGE = 'plugin-sharing:rule-rebind'; */ export const RULE_CRITERIA_GUARD_PACKAGE = 'plugin-sharing:rule-criteria-guard'; -/** - * [#6783] The one INFO line an `isSystem` write batch gets when it lands rows - * on an object that an ACTIVE sharing rule covers and materialises no grants. +/* + * RETIRED — the `isSystem` materialisation skip, and with it the INFO line that + * announced it (`SYSTEM_WRITE_SKIP_NOTICE`, #6783 / #4707 demand 3). + * + * The notice existed because the skip was silent: a fresh install could carry + * active rules, matching records and an empty `sys_record_share`, and nothing + * said so. Its wording named the two remedies — re-evaluate the rules, or + * restart to backfill — and both were real. What made it obsolete is that the + * skip it described is gone, so there is no longer a silent window to announce. * - * The wording after the tag is the maintainer's, verbatim (ruling on #4707, - * 2026-08-06, demand 3): it names the behaviour AND both remedies, because the - * whole defect being fixed is that neither was discoverable. hotcrm#640 is the - * specimen — a fresh install with 9 active rules, 9 matching accounts and an - * empty `sys_record_share`, where every visible layer said "configured" and - * nothing said "inert". The only way to learn the truth was to query the table, - * find it empty, and read this file. + * Maintainer ruling 2026-08-31 (verbatim, untranslated — an accepted decision + * is reversed only by a later one, AGENTS.md Prime Directive #13): * - * It is a statement about the WRITE PATH, not a claim that grants were owed: - * whether a given seeded row would have matched a rule's criteria is precisely - * the query the skip exists to avoid, so answering it here would cost the skip - * its reason to exist. Worded this way the line is true in both cases — it says - * materialisation did not run, and where the answer comes from when it does. + * 裁定:系统写参与逐记录共享物化 —— 删除 `plugin-sharing` 两个钩子里的 + * `isSystem` 跳过,⛔ 不加声明式开关、不以文档代修。 * - * INFO, deliberately, not `warn`: the behaviour is CORRECT (the - * `kernel:bootstrapped` backfill in `sharing-plugin.ts` reconciles every rule - * and `evaluateRule` is idempotent), so a warning would train operators to - * ignore a subsystem that is working as designed. + * The reasoning that binds here: a sharing rule's declared semantics + * (`status == "approved"` implies the team can see it) is a published promise, + * and a hook that skipped the write which satisfies it made declared different + * from enforced. `isSystem` says the platform is the OPERATOR; it never says + * the consequence need not happen. The specimen was an approval write-back — + * a `lockRecord: true` node mirrors the decision onto the record under a system + * context (`plugin-approvals` `approval-service.ts` `mirrorStatusField`), which + * is the ONLY write that can land while the record is locked — after which a + * teammate relying on the rule could not see the record until somebody ran + * `POST /api/v1/sharing/rules/:id/evaluate` or restarted the server. + * + * The boot backfill in `sharing-plugin.ts` is unaffected and still needed: it + * covers rows written while a rule was inactive or before its hooks were bound, + * and it is the only pass that purges a deactivated rule's grants. */ -export const SYSTEM_WRITE_SKIP_NOTICE = - '[sharing-rule] sharing materialisation skipped for isSystem writes; ' + - 're-evaluate rules or restart to backfill'; interface MinimalEngine { registerHook(event: string, handler: (ctx: any) => any | Promise, options?: { @@ -111,7 +116,10 @@ export const ruleRegrantQueue = new RuleRegrantQueue(); * - `afterInsert` — recompute the inserted row (unchanged behaviour). * - `beforeUpdate` / `beforeDelete` — resolve the affected row set and stash * it for the `after` half (`AFFECTED_ROWS_STASH_KEY`). Must be - * `before`: the write is what makes those rows unfindable. + * `before`: the write is what makes those rows unfindable. Runs for system + * writes too, and has to: `afterUpdate` now materialises for them, and with + * no stash `readAffectedRows` answers `unbounded` — which would send every + * single-row system update down the object-wide revoke branch below. * [#6966] The stash rides `HookContext.dispatch.scope`, the engine's * per-write scratch — NOT the context object. A predicate write dispatches * `before*` per row (#5574) and builds a fresh context for each, so the @@ -142,10 +150,12 @@ export const ruleRegrantQueue = new RuleRegrantQueue(); * skipped recompute entirely and left stale `sys_record_share` rows granting * access the rules no longer imply. * - * [#6783] The two skips that drop GRANT MATERIALISATION (`afterInsert`, - * `afterUpdate`) now emit {@link SYSTEM_WRITE_SKIP_NOTICE} once per object per - * binding generation. The skips themselves are unchanged — the behaviour is - * correct and the boot backfill heals it; only the silence was the defect. + * [#13533] GRANT MATERIALISATION no longer asks whether the writer is the + * platform. The `afterInsert` / `afterUpdate` skips on `session.isSystem` — and + * the `before*` stash skip they depended on — are gone, so a system write + * materialises exactly as a user write does. See the retirement note at the top + * of this file for the ruling and the specimen. `afterDelete` still skips, on + * its own separate grounds, spelled out at that hook. * * Caller is responsible for invoking {@link unbindAllRuleHooks} before * re-binding when the rule set changes. @@ -157,55 +167,12 @@ export function bindRuleHooks( logger?: MinimalLogger, ): void { const objects = new Set(); - /** Active rule names per object — the `rules:` field of the #6783 notice. */ - const activeRuleNames = new Map(); for (const r of rules) { if (r.active === false) continue; if (!r.object_name) continue; objects.add(r.object_name); - const named = activeRuleNames.get(r.object_name) ?? []; - named.push(String(r.name ?? r.id ?? '')); - activeRuleNames.set(r.object_name, named); } - /** - * [#6783] Objects whose current silent window has already been reported. - * - * Scoped to this binding generation on purpose. The signal being added is - * "materialisation did not run here", which is a property of the OBJECT and - * of the rule set bound to it — not of the row — so a seed batch of N rows - * must produce ONE line, never N. The failure mode being fixed is silence; - * trading it for a per-row flood would replace one defect with another, and - * an operator who scrolls past the line is exactly as uninformed as one who - * was never told. - * - * The latch re-arms with the binding: `bindRuleRebindTriggers` unbinds and - * re-binds this whole package on every `sys_sharing_rule` write, so a rule - * set that changed gets its own notice rather than inheriting the previous - * generation's silence. - */ - const notified = new Set(); - - /** - * Emit {@link SYSTEM_WRITE_SKIP_NOTICE} at most once per object per binding - * generation. Never throws: this runs on the write path ahead of the hooks' - * own `try`, and a logger that throws must not fail an operator's write. The - * latch is claimed BEFORE the log so a throwing logger cannot turn one - * suppressed line into one throw per row. - */ - const noteSystemWriteSkipped = (objectName: string): void => { - if (notified.has(objectName)) return; - notified.add(objectName); - try { - logger?.info?.(SYSTEM_WRITE_SKIP_NOTICE, { - object: objectName, - rules: activeRuleNames.get(objectName) ?? [], - }); - } catch { - /* a logger that throws must not fail the write */ - } - }; - for (const objectName of objects) { const opts = { object: objectName, packageId: SHARING_RULE_HOOK_PACKAGE, priority: 180 }; @@ -241,13 +208,19 @@ export function bindRuleHooks( /** * [#5103] Delegates to the shared stash: the record-delete cascade binds * its own `beforeDelete` on the same objects, and whichever of the two runs - * first resolves the row set for both. Still skips system writes — the - * recompute half deliberately leaves seeds to the boot backfill — but the - * skip is now only about *this* subscriber; the cascade stashes for system - * writes on its own account. + * first resolves the row set for both. + * + * [#13533] Unconditional. It used to skip system writes, on the reasoning + * that the recompute half left seeds to the boot backfill — which was true + * only while `afterUpdate` skipped them too. It no longer does, and an + * `after` hook with no stash does not read as "nothing changed": it reads + * as `unbounded` (`readAffectedRows`, deliberately), so the skip would have + * turned every single-row system update — the approval write-back the + * ruling is about — into an object-wide revoke plus an asynchronous + * re-grant. Resolving is also nearly free on that shape: a write that names + * its row short-circuits in `resolveAffectedRows` step 1 without querying. */ const stashAffectedRows = async (ctx: any) => { - if ((ctx?.session as any)?.isSystem) return; await stashAffectedRowsOnCtx(engine, objectName, ctx, logger); }; @@ -271,11 +244,6 @@ export function bindRuleHooks( ctx?.dispatch?.mode === 'per-row' && ctx.dispatch.index !== 0; engine.registerHook('afterInsert', async (ctx: any) => { - if ((ctx?.session as any)?.isSystem) { - // [#6783] The skip stays exactly as it was; it just stops being silent. - noteSystemWriteSkipped(objectName); - return; - } try { const data = ctx?.result ?? ctx?.input?.data ?? {}; const id = String((data as any)?.id ?? ctx?.input?.id ?? ''); @@ -290,13 +258,10 @@ export function bindRuleHooks( engine.registerHook('beforeDelete', stashAffectedRows, opts); engine.registerHook('afterUpdate', async (ctx: any) => { - if ((ctx?.session as any)?.isSystem) { - // [#6783] An `isSystem` update INTO a rule's criteria owes grants the - // same way an insert does, and `evaluateRule` is diff-based, so the - // notice's remedy is true for both directions of an update. - noteSystemWriteSkipped(objectName); - return; - } + // [#13533] An `isSystem` update INTO a rule's criteria owes grants the + // same way an insert does — that update IS the approval write-back — and + // `evaluateRule` is diff-based, so one out of the criteria revokes just + // as symmetrically. Neither direction asks who wrote the row any more. if (alreadyHandledThisWrite(ctx)) return; try { const affected = affectedFrom(ctx); @@ -311,14 +276,19 @@ export function bindRuleHooks( }, opts); engine.registerHook('afterDelete', async (ctx: any) => { - // [#6783] Deliberately silent, unlike the insert/update skips above. - // What a delete skips is REVOCATION, not materialisation, and the - // notice's remedy would be false here: `evaluateRule` iterates records - // that still exist, so no re-evaluation and no restart can reach a grant - // whose record is gone (the orphan named at the tail of #4779). That - // class is owned by `record-share-cascade.ts` — which stashes for system - // writes on its own account (#5103) — and by the boot orphan sweep, so - // an INFO line here would point an operator at a repair that cannot run. + // [#13533] The one system-write skip that SURVIVED the ruling, because + // what it skips is a different payload. The insert/update skips dropped + // MATERIALISATION — a grant the rule's own declared semantics promised, + // with nothing else on the write path to deliver it. A delete skips + // REVOCATION, and that consequence is delivered anyway: the general + // invariant "the record is gone, so no share on it can be valid" belongs + // to `record-share-cascade.ts`, which binds on every sharing-capable + // object and stashes for system writes on its own account (#5103), with + // the boot orphan sweep behind it. So this is not `isSystem` used as a + // blanket silencer; it is one subscriber declining work another + // subscriber owns. Removing it would double-revoke, and the rule-only + // trade below (revoke the object's grants, re-grant asynchronously) is + // one the cascade must never make on manual rows. if ((ctx?.session as any)?.isSystem) return; if (alreadyHandledThisWrite(ctx)) return; try { diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 1ad3d82f2a..2d57974679 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -133,9 +133,16 @@ export interface SharingPluginOptions { /** * [#2926 ③] Boot backfill: rule grants are materialized by the write hooks, - * 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. + * and this pass covers the rows those hooks did not reach. + * + * [#13533] It was originally written for seed rows, which are written with + * `isSystem` and which the hooks used to skip — so a fresh deploy's seed data + * carried no `sys_record_share` rows until each record was touched at runtime. + * That skip is gone (see the retirement note in `rule-hooks.ts`), and this pass + * is still owed: rows written while a rule was inactive or before its hooks + * were bound are unreached by any hook, and this is the only pass that PURGES + * a deactivated rule's grants (#4433). + * * 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 @@ -871,13 +878,17 @@ export class SharingServicePlugin implements Plugin { } }); - // [#2926 ③] Materialize sharing grants for rows already present at boot — - // notably SeedLoader-inserted seed records, whose write goes through the - // isSystem short-circuit in the rule hooks and therefore never produces a - // `sys_record_share`. Runs on `kernel:bootstrapped` — the anchor that fires - // after every `kernel:ready` handler (including the AppPlugin seed loader) - // has settled — so the reconcile sees the seeded rows. Idempotent: a runtime - // write that already materialized a grant is reconciled to the same state. + // [#2926 ③] Materialize sharing grants for rows already present at boot. + // [#13533] SeedLoader rows used to be the headline case, because the rule + // hooks short-circuited on `isSystem` and so never produced a + // `sys_record_share` for them; that skip is gone. What still lands here is + // every row no hook reached — written before its object's hooks were bound, + // or while its rule was inactive — plus the withdrawal half, which no write + // hook can do: this pass is handed EVERY rule, so an inactive one has its + // grants purged (#4433). Runs on `kernel:bootstrapped` — the anchor that + // fires after every `kernel:ready` handler (including the AppPlugin seed + // loader) has settled. Idempotent: a runtime write that already + // materialized a grant is reconciled to the same state. ctx.hook('kernel:bootstrapped', async () => { // [#3865] Normalise retired `access_level: 'full'` rows FIRST, so the // rule reconcile below materialises grants from already-canonical rules diff --git a/packages/plugins/plugin-sharing/src/system-write-materialisation.test.ts b/packages/plugins/plugin-sharing/src/system-write-materialisation.test.ts new file mode 100644 index 0000000000..c3ffb45ae5 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/system-write-materialisation.test.ts @@ -0,0 +1,539 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13533] A system write materialises sharing grants, exactly as a user write + * does. + * + * ## This file is the REVERSAL of `system-write-skip-notice.test.ts` (#6783) + * + * It is renamed rather than deleted, and every expectation the old file held is + * registered below with its new counterpart, because the old file was not + * wrong: it pinned a real, maintainer-ruled behaviour (#4707 demand 3, + * 2026-08-06 — an `isSystem` write batch that materialises zero grants says so, + * once). A later ruling reversed the behaviour it pinned, so its pins invert; + * silently dropping them would leave no record that the reversal happened. + * + * Maintainer ruling 2026-08-31 (verbatim, untranslated): + * + * 裁定:系统写参与逐记录共享物化 —— 删除 `plugin-sharing` 两个钩子里的 + * `isSystem` 跳过,⛔ 不加声明式开关、不以文档代修。 + * + * ### The reversal register — old pin, new pin + * + * | #6783 expectation (was) | #13533 expectation (is) | + * |---|---| + * | a system INSERT into the criteria grants nothing, and logs the notice once | it materialises the grant; there is no notice, and no notice constant to import | + * | a batch of N system inserts grants nothing, one line for the batch | every row that matches is materialised | + * | a system UPDATE into the criteria grants nothing, same one line | it materialises — this is the approval write-back, the specimen of the card | + * | the notice is INFO, never warn/error | no line at any level is owed for a materialising write | + * | the latch is per object, and re-arms on rebind | retired with the latch | + * | the notice survives a throwing log sink | retired with the notice | + * | a NON-system write materialises normally | UNCHANGED — kept below as the control that this change widened the population without moving the user path | + * | an active rule that legitimately matches nothing grants nothing | UNCHANGED — kept below as the over-materialisation control, now driven by a SYSTEM write | + * | an object whose only rule is inactive binds no hooks | UNCHANGED | + * | an `isSystem` DELETE is silent, because the notice's remedy cannot repair it | UNCHANGED in behaviour: `afterDelete` still skips system writes, on the separate ground that `record-share-cascade.ts` owns that payload | + * | the notice text is the maintainer's wording, verbatim | retired with the notice | + * + * ## What acceptance looks like, and why a grant row alone is not it + * + * The card's reproduction constraint is binding (triage, 2026-08-31): the defect + * is observable ONLY to a principal WITHOUT `viewAllRecords` who depends on the + * sharing rule. A manager or admin reads through the profile path, never through + * the rule, and sees the record either way — "the manager sees it" is true and + * is NOT a counter-proof. So the acceptance pins here end at + * `SharingService.buildReadFilter` for a plain member context, and then run that + * filter against the table: the assertion is that the teammate can now SEE the + * record, not merely that a row appeared in `sys_record_share`. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; +import { + bindRuleHooks, + unbindAllRuleHooks, + SHARING_RULE_HOOK_PACKAGE, +} from './rule-hooks.js'; + +interface Row { [k: string]: any } + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** What a seed run / package install / internal importer sends. */ +const SYSTEM_SESSION = { isSystem: true }; +/** What an interactive admin sends. */ +const ADMIN_SESSION = { isSystem: false, userId: 'admin' }; +/** + * What the approval write-back sends. `plugin-approvals` + * (`approval-service.ts` `mirrorStatusField`) writes the decision onto the + * subject record as `{ ...SYSTEM_CTX, userId: actorId }` — elevated, because a + * `lockRecord: true` node means only a platform write can land while the record + * is locked, but still carrying WHO decided so downstream cascades keep an + * identity. Both halves matter here: it is a system write, and it is a + * single-id write. + */ +const APPROVAL_WRITEBACK_SESSION = { isSystem: true, userId: 'manager' }; + +type HookEntry = { event: string; handler: (ctx: any) => any; options: Row }; + +/** + * A fake ObjectQL engine, pinned to the real engine's write dispatch on both + * destructive verbs (#4550 / #5480) so a double looser than the thing it + * replaces cannot turn this suite green on calls production refuses. + */ +function makeEngine() { + const tables: Record = {}; + const schemas: Record = {}; + const hooks: HookEntry[] = []; + const ensure = (n: string) => (tables[n] ??= []); + /** Every `find` this suite drove, so the census pin can count reads. */ + const finds: string[] = []; + + function matches(row: Row, f: any): boolean { + if (!f || typeof f !== 'object') return true; + // `$or` / `$and` are conjoined WITH their sibling keys, the way a real + // driver ANDs them — a short-circuiting `return` here would discard every + // sibling equality key in the same object. See #7620. + if (Array.isArray(f.$or) && !f.$or.some((x: any) => matches(row, x))) return false; + if (Array.isArray(f.$and) && !f.$and.every((x: any) => matches(row, x))) return false; + for (const [k, v] of Object.entries(f)) { + if (k === '$or' || k === '$and') continue; + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + + const engine = { + _tables: tables, + _schemas: schemas, + _finds: finds, + getSchema(name: string) { return schemas[name]; }, + async find(o: string, opts?: any) { + finds.push(o); + const f = opts?.filter ?? opts?.where; + return ensure(o).filter((r) => matches(r, f)).slice(0, opts?.limit ?? 10000); + }, + async insert(o: string, data: any) { const row = { ...data }; ensure(o).push(row); return row; }, + async update(o: string, data: any, options?: any) { + // Pinned to `ObjectQL.update`'s dispatch (#5480): a scalar `data.id` + // outranks `where`/`multi`, and a predicate update without `multi` is + // the shape a real server refuses. + assertEngineUpdateDispatch(data, options); + const t = ensure(o); const i = t.findIndex((r) => r.id === data?.id); + if (i >= 0) t[i] = { ...t[i], ...data }; + return t[i]; + }, + async delete(o: string, opts?: any) { + // Pinned to `ObjectQL.delete`'s dispatch (#4434 / #4550). + assertEngineDeleteDispatch(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 }; + }, + registerHook(event: string, handler: (ctx: any) => any, options: Row = {}) { + hooks.push({ event, handler, options }); + }, + unregisterHooksByPackage(packageId: string) { + let removed = 0; + for (let i = hooks.length - 1; i >= 0; i--) { + if (hooks[i].options.packageId === packageId) { hooks.splice(i, 1); removed++; } + } + return removed; + }, + boundFor(packageId: string) { return hooks.filter((h) => h.options.packageId === packageId); }, + + async fire(event: string, object: string, ctx: any) { + for (const h of [...hooks]) { + if (h.event === event && h.options.object === object) await h.handler(ctx); + } + }, + + /** One row insert, fired the way the engine fires `afterInsert`. */ + async simulateInsert(object: string, row: Row, session: any = ADMIN_SESSION) { + ensure(object).push({ ...row }); + await engine.fire('afterInsert', object, { + object, event: 'afterInsert', input: { data: row }, result: row, session, + }); + }, + + /** `count` rows in one pass — a seed batch, one hook fire per row. */ + async simulateInsertBatch(object: string, rows: Row[], session: any = SYSTEM_SESSION) { + for (const row of rows) await engine.simulateInsert(object, row, session); + }, + + /** + * A SINGLE-ID update — the approval write-back's shape, and the shape the + * `before` stash resolves without querying (`resolveAffectedRows` step 1). + */ + async simulateUpdate(object: string, id: string, data: Row, session: any = ADMIN_SESSION) { + const ctx: any = { + object, event: 'beforeUpdate', + input: { id, data: { ...data, id }, options: {} }, + session, + }; + await engine.fire('beforeUpdate', object, ctx); + const t = ensure(object); + const i = t.findIndex((r) => r.id === id); + if (i >= 0) t[i] = { ...t[i], ...data }; + ctx.event = 'afterUpdate'; + await engine.fire('afterUpdate', object, ctx); + }, + + /** A predicate update: no `input.id`, one shared ctx across before/after. */ + async simulateBulkUpdate(object: string, where: any, data: Row, session: any = ADMIN_SESSION) { + const ctx: any = { + object, event: 'beforeUpdate', + input: { id: undefined, data, options: { where, multi: true } }, + session, + }; + await engine.fire('beforeUpdate', object, ctx); + const t = ensure(object); + for (let i = 0; i < t.length; i++) { + if (where != null && !matches(t[i], where)) continue; + t[i] = { ...t[i], ...data }; + } + ctx.event = 'afterUpdate'; + await engine.fire('afterUpdate', object, ctx); + }, + + async simulateBulkDelete(object: string, where: any, session: any = ADMIN_SESSION) { + const ctx: any = { + object, event: 'beforeDelete', + input: { id: undefined, options: { where, multi: true } }, + session, + }; + await engine.fire('beforeDelete', object, ctx); + const t = ensure(object); + for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); + ctx.event = 'afterDelete'; + await engine.fire('afterDelete', object, ctx); + }, + }; + return engine; +} + +type Engine = ReturnType; + +/** Every `sys_record_share` row a rule materialised. */ +const ruleShares = (engine: Engine) => + (engine._tables.sys_record_share ?? []).filter((r) => r.source === 'rule'); + +const rule = (over: Row = {}): Row => ({ + id: 'srule_east', + name: 'east_to_alice', + label: 'East → Alice', + object_name: 'opportunity', + criteria_json: JSON.stringify({ region: 'east' }), + recipient_type: 'user', + recipient_id: 'alice', + access_level: 'edit', + active: true, + ...over, +}); + +describe('#13533 a system write materialises sharing grants', () => { + let engine: Engine; + let rules: SharingRuleService; + let logger: any; + + /** Bind the hooks against whatever is currently in `sys_sharing_rule`. */ + const bind = async () => { + const ruleRows = await rules.listRules({ activeOnly: true }, SYS); + unbindAllRuleHooks(engine as any); + bindRuleHooks(engine as any, rules, ruleRows, logger); + }; + + beforeEach(async () => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + engine._tables.opportunity = []; + engine._tables.sys_record_share = []; + engine._tables.sys_sharing_rule = [rule()]; + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger }); + await bind(); + }); + + // ── the reversal: what the #6783 pins asserted, inverted ────────────── + + it('a system INSERT into the criteria materialises the grant (was: skipped, and logged once)', async () => { + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, SYSTEM_SESSION); + + expect(ruleShares(engine).map((r) => r.record_id)).toEqual(['opp0']); + expect(ruleShares(engine).map((r) => r.recipient_id)).toEqual(['alice']); + }); + + it('a BATCH of system inserts materialises every matching row (was: zero grants, one line)', async () => { + await engine.simulateInsertBatch('opportunity', [ + { id: 'opp0', region: 'east', owner_id: 'boss' }, + { id: 'opp1', region: 'east', owner_id: 'boss' }, + { id: 'opp2', region: 'west', owner_id: 'boss' }, + { id: 'opp3', region: 'east', owner_id: 'boss' }, + ]); + + expect(engine._tables.opportunity).toHaveLength(4); + // `opp2` is `west`: outside the criteria, so it is correctly ungranted. + expect(ruleShares(engine).map((r) => r.record_id).sort()).toEqual(['opp0', 'opp1', 'opp3']); + }); + + it('a system bulk UPDATE into the criteria materialises (was: skipped, same one line)', async () => { + await engine.simulateInsertBatch('opportunity', [ + { id: 'opp0', region: 'west', owner_id: 'boss' }, + { id: 'opp1', region: 'west', owner_id: 'boss' }, + ]); + expect(ruleShares(engine)).toEqual([]); + + await engine.simulateBulkUpdate('opportunity', { region: 'west' }, { region: 'east' }, SYSTEM_SESSION); + + expect(ruleShares(engine).map((r) => r.record_id).sort()).toEqual(['opp0', 'opp1']); + }); + + it('a system bulk update OUT of the criteria revokes — the diff runs in both directions', async () => { + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, SYSTEM_SESSION); + expect(ruleShares(engine)).toHaveLength(1); + + await engine.simulateBulkUpdate('opportunity', { region: 'east' }, { region: 'west' }, SYSTEM_SESSION); + + expect(ruleShares(engine)).toEqual([]); + }); + + it('says nothing at any level about a materialising system write (was: one INFO notice)', async () => { + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, SYSTEM_SESSION); + + for (const level of ['info', 'warn', 'error'] as const) { + expect( + logger[level].mock.calls.filter((c: any[]) => String(c[0]).includes('materialisation skipped')), + ).toEqual([]); + } + // …and specifically not the retired wording, whichever sink it reached. + for (const level of ['info', 'warn', 'error'] as const) { + expect( + logger[level].mock.calls.filter((c: any[]) => String(c[0]).includes('restart to backfill')), + ).toEqual([]); + } + }); + + // ── the acceptance anchor: the card's own reproduction, in a unit ───── + + describe('the approval write-back, observed from a member WITHOUT viewAllRecords', () => { + /** + * The card's minimal path: an object whose `status` an approval node + * mirrors, a criteria rule `status == "approved"` naming a teammate, and a + * record owned by somebody else. + */ + const REP2 = { userId: 'rep2' } as any; + let sharing: SharingService; + + beforeEach(async () => { + engine._tables.crm_leave_request = []; + engine._schemas.crm_leave_request = { + name: 'crm_leave_request', + sharingModel: 'private', + fields: { owner_id: {}, status: {} }, + }; + engine._tables.sys_sharing_rule = [rule({ + id: 'srule_leave_approved', + name: 'leave_request_approved_team_sharing_sales_rep', + object_name: 'crm_leave_request', + criteria_json: JSON.stringify({ status: 'approved' }), + recipient_type: 'user', + recipient_id: 'rep2', + access_level: 'read', + })]; + sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger }); + await bind(); + + // rep1 submits; the record starts `pending` and is owned by rep1. + await engine.simulateInsert( + 'crm_leave_request', + { id: 'lr1', status: 'pending', owner_id: 'rep1' }, + { isSystem: false, userId: 'rep1' }, + ); + }); + + /** Does rep2's OWN read path admit `lr1`? */ + const rep2CanSee = async (): Promise => { + const filter = await sharing.buildReadFilter('crm_leave_request', REP2); + // A member context must never bypass — if it did, this whole assertion + // would be vacuous and would pass with the defect still present. + expect(filter).not.toBeNull(); + const visible = await engine.find('crm_leave_request', { where: filter }); + return visible.some((r: any) => r.id === 'lr1'); + }; + + it('cannot see a teammate PENDING record — the baseline the defect hid behind', async () => { + expect(await rep2CanSee()).toBe(false); + // Not because sharing is off: rep2 is simply not the owner and the rule's + // criteria is not satisfied yet. + expect(await sharing.buildReadFilter('crm_leave_request', REP2)).toEqual({ owner_id: 'rep2' }); + }); + + it('SEES it the moment the approval write-back lands — no evaluate, no restart', async () => { + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'approved' }, APPROVAL_WRITEBACK_SESSION, + ); + + expect(ruleShares(engine).map((r) => [r.record_id, r.recipient_id])).toEqual([['lr1', 'rep2']]); + expect(await rep2CanSee()).toBe(true); + // The grant is what widened the filter — the owner match is still there, + // so this is additive access, not a scope escalation. + expect(await sharing.buildReadFilter('crm_leave_request', REP2)).toEqual({ + $or: [{ owner_id: 'rep2' }, { id: { $in: ['lr1'] } }], + }); + }); + + it('the approver keeps seeing it either way — a manager view CANNOT observe this defect', async () => { + // Triage's binding note, pinned so a future reader cannot re-derive the + // wrong acceptance: a principal that bypasses sharing reads the record + // before AND after the write-back, so verifying the fix from a manager or + // admin perspective proves nothing at all. + const MANAGER = { isSystem: false, userId: 'manager', __readScope: 'org' } as any; + expect(await sharing.buildReadFilter('crm_leave_request', MANAGER)).toBeNull(); + + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'approved' }, APPROVAL_WRITEBACK_SESSION, + ); + + expect(await sharing.buildReadFilter('crm_leave_request', MANAGER)).toBeNull(); + }); + + it('a system write that does NOT satisfy the criteria grants nothing (over-materialisation control)', async () => { + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'rejected' }, APPROVAL_WRITEBACK_SESSION, + ); + + expect(ruleShares(engine)).toEqual([]); + expect(await rep2CanSee()).toBe(false); + }); + + it('a later system write out of the criteria revokes again — a recall takes the access back', async () => { + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'approved' }, APPROVAL_WRITEBACK_SESSION, + ); + expect(await rep2CanSee()).toBe(true); + + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'recalled' }, APPROVAL_WRITEBACK_SESSION, + ); + + expect(ruleShares(engine)).toEqual([]); + expect(await rep2CanSee()).toBe(false); + }); + + it('takes the BOUNDED per-record branch, never the object-wide revoke', async () => { + // The `before*` stash skip (`rule-hooks.ts`, removed with the other two) + // was load-bearing here: with no stash, `readAffectedRows` answers + // `unbounded`, and a single approval would have revoked every rule grant + // on the object and re-granted asynchronously. That branch announces + // itself with a `warn`; this pin is that the warn never fires. + await engine.simulateUpdate( + 'crm_leave_request', 'lr1', { status: 'approved' }, APPROVAL_WRITEBACK_SESSION, + ); + + expect( + logger.warn.mock.calls.filter((c: any[]) => String(c[0]).includes('more rows than can be recomputed')), + ).toEqual([]); + }); + }); + + // ── the controls the #6783 file already carried, kept unchanged ─────── + + it('a NON-system write still materialises — the user path did not move', async () => { + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, ADMIN_SESSION); + + expect(ruleShares(engine).map((r) => r.record_id)).toEqual(['opp0']); + }); + + it('an active rule that legitimately matches nothing still grants nothing', async () => { + // `west` is outside the criteria. Driven by a SYSTEM write now, which is + // the half that used to be untestable: the skip made every system write + // look identical to a legitimate non-match. + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'west', owner_id: 'boss' }, SYSTEM_SESSION); + + expect(ruleShares(engine)).toEqual([]); + }); + + it("an object whose only rule is inactive binds no hooks, so a system write is a no-op", async () => { + engine._tables.sys_sharing_rule = [rule({ active: false })]; + await bind(); + + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toEqual([]); + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east' }, SYSTEM_SESSION); + + expect(ruleShares(engine)).toEqual([]); + }); + + it('a system write on an object NO active rule covers is a no-op', async () => { + engine._tables.invoice = []; + await engine.simulateInsert('invoice', { id: 'inv0', region: 'east' }, SYSTEM_SESSION); + + expect(ruleShares(engine)).toEqual([]); + }); + + it('an isSystem DELETE is still skipped here — that payload belongs to the cascade', async () => { + // UNCHANGED by #13533, and deliberately so. What a delete skips is + // REVOCATION, which `record-share-cascade.ts` delivers on every + // sharing-capable object (stashing for system writes on its own account, + // #5103) with the boot orphan sweep behind it. Removing this skip would + // double-revoke and would let the rule-only "revoke the object, re-grant + // later" trade run on a payload the cascade must never make it on. + await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, ADMIN_SESSION); + expect(ruleShares(engine)).toHaveLength(1); + + await engine.simulateBulkDelete('opportunity', { region: 'east' }, SYSTEM_SESSION); + + // Still one rule grant: this subscriber did nothing, exactly as before. + expect(ruleShares(engine)).toHaveLength(1); + }); +}); + +/** + * [#13533] The bulk-path census, as an executable measurement. + * + * The ruling required the bulk paths to be measured before disposal, and + * forbade keeping the skip on unmeasured performance fear. What the removal + * costs a bulk system write is the same per-record pass a bulk USER write has + * always paid, so the number this pins is a comparison, not an absolute: the + * two populations do the same work per row. A future change that makes system + * writes cheaper OR more expensive than user writes moves this pin. + */ +describe('#13533 census: a system write costs exactly what the same user write costs', () => { + let engine: Engine; + let rules: SharingRuleService; + + const rows = (n: number): Row[] => + Array.from({ length: n }, (_, i) => ({ id: `opp${i}`, region: 'east', owner_id: 'boss' })); + + const seedCost = async (session: any, n: number): Promise => { + engine = makeEngine(); + engine._tables.opportunity = []; + engine._tables.sys_record_share = []; + engine._tables.sys_sharing_rule = [rule()]; + const sharing = new SharingService({ engine: engine as any }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger: undefined }); + const ruleRows = await rules.listRules({ activeOnly: true }, SYS); + unbindAllRuleHooks(engine as any); + bindRuleHooks(engine as any, rules, ruleRows, { warn: () => {} } as any); + engine._finds.length = 0; + await engine.simulateInsertBatch('opportunity', rows(n), session); + return engine._finds.length; + }; + + it('a 25-row system insert batch reads exactly what a 25-row admin batch reads', async () => { + const asSystem = await seedCost(SYSTEM_SESSION, 25); + const asAdmin = await seedCost(ADMIN_SESSION, 25); + + expect(asSystem).toBe(asAdmin); + // And it is per-record, linear in the batch — the shape the ruling asked to + // be measured. (Before the fix the system number was 0 and the grants were + // 0 with it, which is the defect, not a saving.) + const asSystemHalf = await seedCost(SYSTEM_SESSION, 5); + expect(asSystem).toBeGreaterThan(asSystemHalf); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/system-write-skip-notice.test.ts b/packages/plugins/plugin-sharing/src/system-write-skip-notice.test.ts deleted file mode 100644 index 7592770b2b..0000000000 --- a/packages/plugins/plugin-sharing/src/system-write-skip-notice.test.ts +++ /dev/null @@ -1,378 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * [#6783] "Configured but inert" stops being silent. - * - * Demand 3 of #4707, maintainer-ruled 2026-08-06. An `isSystem` write batch — - * a seed run, a package install, an internal importer — lands rows on an - * object an ACTIVE sharing rule covers, and the record-write hooks skip it, so - * ZERO `sys_record_share` rows are materialised. That skip is correct: the - * `kernel:bootstrapped` backfill reconciles every rule and `evaluateRule` is - * idempotent. What was wrong is that nothing said so. hotcrm#640: a fresh - * install with 9 active rules, 9 matching accounts, and an empty - * `sys_record_share` — every visible layer said "configured", and the only way - * to learn otherwise was to query the table and go read `rule-hooks.ts`. - * - * What this file pins, in both directions: - * - * - ONE line per batch, not per row. That boundary is the whole shape of the - * fix — the defect is silence, and a per-row flood would trade it for - * noise, which is the same defect with a different symptom. - * - INFO, never warn/error: the behaviour is correct and self-healing. - * - The maintainer's wording, verbatim, as a contract. - * - The negative faces: a non-system write (which materialises normally), - * an active rule whose criteria the written row does not satisfy (zero - * grants, but legitimately so), an object whose only rule is inactive, and - * an `isSystem` DELETE — deliberately silent, because the remedy the line - * names cannot repair a grant whose record is gone. - */ - -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; -import { SharingService } from './sharing-service.js'; -import { SharingRuleService } from './sharing-rule-service.js'; -import { - bindRuleHooks, - unbindAllRuleHooks, - SHARING_RULE_HOOK_PACKAGE, - SYSTEM_WRITE_SKIP_NOTICE, -} from './rule-hooks.js'; - -interface Row { [k: string]: any } - -const SYS = { isSystem: true, positions: [], permissions: [] } as any; -/** What a seed run / package install / internal importer sends. */ -const SYSTEM_SESSION = { isSystem: true }; -/** What an interactive admin sends — the path that DOES materialise. */ -const ADMIN_SESSION = { isSystem: false, userId: 'admin' }; - -type HookEntry = { event: string; handler: (ctx: any) => any; options: Row }; - -/** - * A fake ObjectQL engine, pinned to the real engine's write dispatch on both - * destructive verbs (#4550 / #5480) so a double looser than the thing it - * replaces cannot turn this suite green on calls production refuses. - */ -function makeEngine() { - const tables: Record = {}; - const hooks: HookEntry[] = []; - const ensure = (n: string) => (tables[n] ??= []); - - function matches(row: Row, f: any): boolean { - if (!f || typeof f !== 'object') return true; - // `$or` / `$and` are conjoined WITH their sibling keys, the way a real - // driver ANDs them — a short-circuiting `return` here would discard every - // sibling equality key in the same object. See #7620. - if (Array.isArray(f.$or) && !f.$or.some((x: any) => matches(row, x))) return false; - if (Array.isArray(f.$and) && !f.$and.every((x: any) => matches(row, x))) return false; - for (const [k, v] of Object.entries(f)) { - if (k === '$or' || k === '$and') continue; - const rv = row[k]; - if (v != null && typeof v === 'object' && '$in' in (v as any)) { - if (!(v as any).$in.includes(rv)) return false; - continue; - } - if (rv !== v) return false; - } - return true; - } - - const engine = { - _tables: tables, - getSchema() { return undefined; }, - async find(o: string, opts?: any) { - const f = opts?.filter ?? opts?.where; - return ensure(o).filter((r) => matches(r, f)).slice(0, opts?.limit ?? 10000); - }, - async insert(o: string, data: any) { const row = { ...data }; ensure(o).push(row); return row; }, - async update(o: string, data: any, options?: any) { - // Pinned to `ObjectQL.update`'s dispatch (#5480): a scalar `data.id` - // outranks `where`/`multi`, and a predicate update without `multi` is - // the shape a real server refuses. - assertEngineUpdateDispatch(data, options); - const t = ensure(o); const i = t.findIndex((r) => r.id === data?.id); - if (i >= 0) t[i] = { ...t[i], ...data }; - return t[i]; - }, - async delete(o: string, opts?: any) { - // Pinned to `ObjectQL.delete`'s dispatch (#4434 / #4550). - assertEngineDeleteDispatch(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 }; - }, - registerHook(event: string, handler: (ctx: any) => any, options: Row = {}) { - hooks.push({ event, handler, options }); - }, - unregisterHooksByPackage(packageId: string) { - let removed = 0; - for (let i = hooks.length - 1; i >= 0; i--) { - if (hooks[i].options.packageId === packageId) { hooks.splice(i, 1); removed++; } - } - return removed; - }, - boundFor(packageId: string) { return hooks.filter((h) => h.options.packageId === packageId); }, - - async fire(event: string, object: string, ctx: any) { - for (const h of [...hooks]) { - if (h.event === event && h.options.object === object) await h.handler(ctx); - } - }, - - /** One row insert, fired the way the engine fires `afterInsert`. */ - async simulateInsert(object: string, row: Row, session: any = ADMIN_SESSION) { - ensure(object).push({ ...row }); - await engine.fire('afterInsert', object, { - object, event: 'afterInsert', input: { data: row }, result: row, session, - }); - }, - - /** `count` rows in one pass — a seed batch, one hook fire per row. */ - async simulateInsertBatch(object: string, rows: Row[], session: any = SYSTEM_SESSION) { - for (const row of rows) await engine.simulateInsert(object, row, session); - }, - - /** A predicate update: no `input.id`, one shared ctx across before/after. */ - async simulateBulkUpdate(object: string, where: any, data: Row, session: any = ADMIN_SESSION) { - const ctx: any = { - object, event: 'beforeUpdate', - input: { id: undefined, data, options: { where, multi: true } }, - session, - }; - await engine.fire('beforeUpdate', object, ctx); - const t = ensure(object); - for (let i = 0; i < t.length; i++) { - if (where != null && !matches(t[i], where)) continue; - t[i] = { ...t[i], ...data }; - } - ctx.event = 'afterUpdate'; - await engine.fire('afterUpdate', object, ctx); - }, - - async simulateBulkDelete(object: string, where: any, session: any = ADMIN_SESSION) { - const ctx: any = { - object, event: 'beforeDelete', - input: { id: undefined, options: { where, multi: true } }, - session, - }; - await engine.fire('beforeDelete', object, ctx); - const t = ensure(object); - for (let i = t.length - 1; i >= 0; i--) if (matches(t[i], where)) t.splice(i, 1); - ctx.event = 'afterDelete'; - await engine.fire('afterDelete', object, ctx); - }, - }; - return engine; -} - -type Engine = ReturnType; - -/** Every `sys_record_share` row a rule materialised. */ -const ruleShares = (engine: Engine) => - (engine._tables.sys_record_share ?? []).filter((r) => r.source === 'rule'); - -/** The #6783 notices this logger saw — by message, not by call count. */ -const notices = (logger: any) => - logger.info.mock.calls.filter((c: any[]) => c[0] === SYSTEM_WRITE_SKIP_NOTICE); - -const rule = (over: Row = {}): Row => ({ - id: 'srule_east', - name: 'east_to_alice', - label: 'East → Alice', - object_name: 'opportunity', - criteria_json: JSON.stringify({ region: 'east' }), - recipient_type: 'user', - recipient_id: 'alice', - access_level: 'edit', - active: true, - ...over, -}); - -describe('#6783 isSystem writes that materialise zero grants say so, once', () => { - let engine: Engine; - let rules: SharingRuleService; - let logger: any; - - /** Bind the hooks against whatever is currently in `sys_sharing_rule`. */ - const bind = async () => { - const ruleRows = await rules.listRules({ activeOnly: true }, SYS); - unbindAllRuleHooks(engine as any); - bindRuleHooks(engine as any, rules, ruleRows, logger); - }; - - beforeEach(async () => { - logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; - engine = makeEngine(); - engine._tables.opportunity = []; - engine._tables.sys_record_share = []; - engine._tables.sys_sharing_rule = [rule()]; - const sharing = new SharingService({ engine: engine as any }); - rules = new SharingRuleService({ engine: engine as any, sharing, logger }); - await bind(); - }); - - // ── the positive face ──────────────────────────────────────────────── - - it('reports the skip, naming the behaviour, the remedy, the object and the rules', async () => { - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, SYSTEM_SESSION); - - expect(notices(logger)).toHaveLength(1); - const [message, meta] = notices(logger)[0]; - // The maintainer's wording is the contract, not a paraphrase of it. - expect(message).toContain( - 'sharing materialisation skipped for isSystem writes; re-evaluate rules or restart to backfill', - ); - expect(meta).toEqual({ object: 'opportunity', rules: ['east_to_alice'] }); - // …and the line is only true because nothing WAS materialised. - expect(ruleShares(engine)).toEqual([]); - }); - - it('emits ONE line for a batch of many rows — the boundary this card is about', async () => { - await engine.simulateInsertBatch('opportunity', [ - { id: 'opp0', region: 'east', owner_id: 'boss' }, - { id: 'opp1', region: 'east', owner_id: 'boss' }, - { id: 'opp2', region: 'east', owner_id: 'boss' }, - { id: 'opp3', region: 'east', owner_id: 'boss' }, - { id: 'opp4', region: 'east', owner_id: 'boss' }, - ]); - - expect(engine._tables.opportunity).toHaveLength(5); - expect(notices(logger)).toHaveLength(1); - expect(ruleShares(engine)).toEqual([]); - }); - - it('counts an isSystem UPDATE as the same skip — insert + update on one object is still one line', async () => { - await engine.simulateInsertBatch('opportunity', [ - { id: 'opp0', region: 'west', owner_id: 'boss' }, - { id: 'opp1', region: 'west', owner_id: 'boss' }, - ]); - await engine.simulateBulkUpdate('opportunity', { region: 'west' }, { region: 'east' }, SYSTEM_SESSION); - - // The update moved both rows INTO the criteria and still granted nothing. - expect(ruleShares(engine)).toEqual([]); - expect(notices(logger)).toHaveLength(1); - }); - - it('is INFO — never warn, never error', async () => { - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east' }, SYSTEM_SESSION); - - expect(notices(logger)).toHaveLength(1); - for (const level of ['warn', 'error'] as const) { - expect( - logger[level].mock.calls.filter((c: any[]) => String(c[0]).includes('materialisation skipped')), - ).toEqual([]); - } - }); - - it('reports each covered object separately — the latch is per object, not global', async () => { - engine._tables.opportunity = []; - engine._tables.contract = []; - engine._tables.sys_sharing_rule.push( - rule({ id: 'srule_contract', name: 'contract_to_alice', object_name: 'contract' }), - ); - await bind(); - - await engine.simulateInsertBatch('opportunity', [{ id: 'opp0', region: 'east' }, { id: 'opp1', region: 'east' }]); - await engine.simulateInsertBatch('contract', [{ id: 'con0', region: 'east' }, { id: 'con1', region: 'east' }]); - - expect(notices(logger).map((c: any[]) => c[1].object).sort()).toEqual(['contract', 'opportunity']); - }); - - it('re-arms on rebind: a rule change gets its own notice instead of inheriting the old silence', async () => { - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east' }, SYSTEM_SESSION); - expect(notices(logger)).toHaveLength(1); - - // What `bindRuleRebindTriggers` does on every `sys_sharing_rule` write. - await bind(); - await engine.simulateInsert('opportunity', { id: 'opp1', region: 'east' }, SYSTEM_SESSION); - - expect(notices(logger)).toHaveLength(2); - }); - - it('never fails the write, even when the logger itself throws', async () => { - // A log sink that is down must not turn an observability line into a - // failed seed row: the notice runs ahead of the hook's own `try`. - const angry = { - info: vi.fn((msg: string) => { - if (msg === SYSTEM_WRITE_SKIP_NOTICE) throw new Error('log sink down'); - }), - warn: vi.fn(), error: vi.fn(), debug: vi.fn(), - }; - const ruleRows = await rules.listRules({ activeOnly: true }, SYS); - unbindAllRuleHooks(engine as any); - bindRuleHooks(engine as any, rules, ruleRows, angry as any); - - await expect( - engine.simulateInsert('opportunity', { id: 'opp0', region: 'east' }, SYSTEM_SESSION), - ).resolves.toBeUndefined(); - - // It tried exactly once, and the latch closed even though the log failed — - // a broken sink must not become one throw per row. - expect(angry.info.mock.calls.filter((c: any[]) => c[0] === SYSTEM_WRITE_SKIP_NOTICE)).toHaveLength(1); - await engine.simulateInsert('opportunity', { id: 'opp1', region: 'east' }, SYSTEM_SESSION); - expect(angry.info.mock.calls.filter((c: any[]) => c[0] === SYSTEM_WRITE_SKIP_NOTICE)).toHaveLength(1); - }); - - // ── the negative faces: silence when nothing is wrong ──────────────── - - it('stays silent for a non-system write — which materialises normally', async () => { - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, ADMIN_SESSION); - - expect(notices(logger)).toEqual([]); - expect(ruleShares(engine).map((r) => r.record_id)).toEqual(['opp0']); - }); - - it('stays silent when an active rule legitimately matches nothing', async () => { - // Rule active, row written, ZERO grants materialised — and correctly so: - // `west` is outside the criteria. Nothing is inert, so nothing is said. - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'west', owner_id: 'boss' }, ADMIN_SESSION); - - expect(ruleShares(engine)).toEqual([]); - expect(notices(logger)).toEqual([]); - }); - - it("stays silent when the object's only rule is inactive — no active rule, no hooks, no line", async () => { - engine._tables.sys_sharing_rule = [rule({ active: false })]; - await bind(); - - expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE)).toEqual([]); - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east' }, SYSTEM_SESSION); - - expect(notices(logger)).toEqual([]); - }); - - it("stays silent on an isSystem DELETE — the line's remedy cannot repair that class", async () => { - // A delete skips REVOCATION, not materialisation, and `evaluateRule` - // iterates records that still exist — so neither re-evaluating nor - // restarting can reach a grant whose record is gone (#4779's orphan). - // `record-share-cascade.ts` and the boot orphan sweep own it instead. - await engine.simulateInsert('opportunity', { id: 'opp0', region: 'east', owner_id: 'boss' }, ADMIN_SESSION); - expect(ruleShares(engine)).toHaveLength(1); - logger.info.mockClear(); - - await engine.simulateBulkDelete('opportunity', { region: 'east' }, SYSTEM_SESSION); - - expect(notices(logger)).toEqual([]); - }); - - it('stays silent on an object no active rule covers', async () => { - engine._tables.invoice = []; - await engine.simulateInsert('invoice', { id: 'inv0', region: 'east' }, SYSTEM_SESSION); - - expect(notices(logger)).toEqual([]); - }); -}); - -describe("#6783 the notice text is the maintainer's ruling, verbatim", () => { - it('carries the ruled wording and the package tag every sharing line uses', () => { - expect(SYSTEM_WRITE_SKIP_NOTICE).toBe( - '[sharing-rule] sharing materialisation skipped for isSystem writes; ' + - 're-evaluate rules or restart to backfill', - ); - }); - - it('names BOTH remedies — re-evaluation and restart', () => { - expect(SYSTEM_WRITE_SKIP_NOTICE).toContain('re-evaluate rules'); - expect(SYSTEM_WRITE_SKIP_NOTICE).toContain('restart to backfill'); - }); -}); diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index c6b206d61e..74b1b52042 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2792,12 +2792,12 @@ "pinned": 1 }, { - "file": "packages/plugins/plugin-sharing/src/system-write-skip-notice.test.ts", + "file": "packages/plugins/plugin-sharing/src/system-write-materialisation.test.ts", "verb": "delete", "pinned": 1 }, { - "file": "packages/plugins/plugin-sharing/src/system-write-skip-notice.test.ts", + "file": "packages/plugins/plugin-sharing/src/system-write-materialisation.test.ts", "verb": "update", "pinned": 1 },