From dd65fcaf8cafad5aa017ec2d363544ba5f350d15 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 09:53:19 +0000 Subject: [PATCH 1/2] fix(plugin-sharing): per-grant catch in both reconcile loops so a refused grant no longer aborts the pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After #14484 `sys_record_share` is tenant-scoped in the #13491 ledger, so an organization-less system insert on it is refused loudly with `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` on a walled install. A platform-global sharing rule materialising a grant onto an organization-less record resolves no organization and meets that refusal; with no per-grant catch it propagated and that rule's reconcile pass aborted mid-loop, taking the pass's stale-row revocations with it. Those revocations are the security-relevant half: a stale over-grant persisted across every later pass, which met the same record and died in the same place. Both loops now attempt each grant individually. A refusal is logged with the rule, object, record, recipient and the engine's code, counted in the pass result, and the pass continues. The catch is narrow — only `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` is absorbed. `record-share-organization-stamp.test.ts` deliberately pins the abort on the other error a pass can meet here (the scoped update half answering `RECORD_NOT_FOUND`), a shape the 2026-09-02 contract review left standing; a catch-all would retire that decision as a side effect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- ...econcile-per-grant-organization-refusal.md | 58 +++ packages/plugins/plugin-sharing/src/index.ts | 4 + .../reconcile-refused-grant-continues.test.ts | 447 ++++++++++++++++++ .../src/sharing-rule-service.ts | 219 ++++++--- 4 files changed, 663 insertions(+), 65 deletions(-) create mode 100644 .changeset/sharing-reconcile-per-grant-organization-refusal.md create mode 100644 packages/plugins/plugin-sharing/src/reconcile-refused-grant-continues.test.ts diff --git a/.changeset/sharing-reconcile-per-grant-organization-refusal.md b/.changeset/sharing-reconcile-per-grant-organization-refusal.md new file mode 100644 index 0000000000..f01b04c0b2 --- /dev/null +++ b/.changeset/sharing-reconcile-per-grant-organization-refusal.md @@ -0,0 +1,58 @@ +--- +"@objectstack/plugin-sharing": minor +--- + +fix(plugin-sharing): one refused grant no longer aborts a sharing rule's reconcile pass — its stale-row revocations still run (#14754) + +After #14484 `sys_record_share` is `tenant-scoped` in the #13491 ledger, so on a +walled install an organization-less system insert on it is refused loudly with +`ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` (#8844). `SharingService.grant` +resolves the organization on every path that can; a platform-global sharing +rule (`organization_id = null`, its sweep unscoped) materialising a grant onto +an organization-LESS record resolves none, and meets that refusal. + +`SharingRuleService.reconcile` / `reconcileForRecord` had no per-grant catch, so +the refusal propagated and **that rule's pass aborted mid-loop**. Two things +were lost, and they are not equally serious: + +- the remaining grants — recoverable, the next pass writes them; +- **the stale-row revocations of that pass** — not recoverable by waiting, + because every subsequent pass meets the same organization-less record and + dies in the same place. A stale over-grant of that rule therefore persisted + indefinitely, and the record kept aborting the pass until it was repaired by + hand. That is the security-relevant half. + +Measured while pinning this, and it sharpens the point: the engine returns +organization-less rows **last** in a rule's criteria sweep (the driver's +NULL-org compatibility arm is appended to the scoped arm). So a refused grant +is nearly always one of the final attempts of a pass, and what an abort +destroyed was hardly ever "the remaining grants" — it was almost entirely the +revoke loop that runs after the whole upsert loop. + +Both loops now attempt each grant individually. A refusal is logged with the +rule, object, record, recipient and the engine's own code, counted, and the +pass **continues** — the remaining grants and, above all, the stale-row +revocations still run. + +**The catch is deliberately narrow.** Only +`ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` is absorbed; every other error +rethrows unchanged. A catch-all would swallow real defects and report a pass +that "completed" having written nothing. It would also silently retire a +reviewed decision: `record-share-organization-stamp.test.ts` deliberately pins +the abort on the OTHER error a reconcile pass can meet here — the scoped update +half answering `RECORD_NOT_FOUND` for a row stamped with a different +organization — which the 2026-09-02 contract review left standing on "loud +beats a wrong count". Those three pins are unchanged and still green. + +**Why `minor` rather than `patch`.** The repair is a bug fix, but it reports +through a new key. `reconcile` / `reconcileForRecord` / `evaluateRule` / +`evaluateAllForRecord` now return `SharingRuleReconcilePassResult` — the spec's +`SharingRuleEvaluationResult` plus `grantsRefused: number` — and that type is +newly exported from the package index. Purely additive: the contract in +`@objectstack/spec` is untouched, its six declared fields are unchanged, and a +consumer typed against `ISharingRuleService` keeps compiling as it did. Same +shape as `fix(runtime): tell an action handler when its caller-scope record load +was refused` (#14143), which shipped `minor` for the same reason. + +`grantsRefused > 0` does **not** mean the pass failed. It means the pass met a +record it cannot grant on and carried on — which is the whole point. diff --git a/packages/plugins/plugin-sharing/src/index.ts b/packages/plugins/plugin-sharing/src/index.ts index abc2b3aa9b..7fce572acc 100644 --- a/packages/plugins/plugin-sharing/src/index.ts +++ b/packages/plugins/plugin-sharing/src/index.ts @@ -23,6 +23,10 @@ export { export { SharingRuleService, type SharingRuleServiceOptions, + // [#14754] The spec's `SharingRuleEvaluationResult` plus the pass's + // `grantsRefused` count. Additive: the six declared fields are unchanged, and + // a consumer typed against the spec contract keeps compiling untouched. + type SharingRuleReconcilePassResult, } from './sharing-rule-service.js'; export { ShareLinkService, diff --git a/packages/plugins/plugin-sharing/src/reconcile-refused-grant-continues.test.ts b/packages/plugins/plugin-sharing/src/reconcile-refused-grant-continues.test.ts new file mode 100644 index 0000000000..36a7dbf501 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/reconcile-refused-grant-continues.test.ts @@ -0,0 +1,447 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14754] One refused grant no longer takes a rule's whole reconcile pass with + * it — and, above all, no longer takes that pass's STALE-ROW REVOCATIONS. + * + * ## The shape, and which half of it is the security half + * + * After #14484 `sys_record_share` is `tenant-scoped` in the #13491 ledger, so + * on a walled install an organization-less system insert on it is refused + * loudly with `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` (#8844). + * `SharingService.grant` resolves the organization on every path that can; a + * platform-global rule (`organization_id = null`, its sweep unscoped) + * materialising onto an organization-LESS record resolves none, and meets the + * refusal. + * + * Before this card the refusal propagated out of the reconcile loop and that + * rule's pass ABORTED mid-loop. Two things were lost, and they are not equally + * serious: + * + * - the remaining grants — recoverable, the next pass writes them; + * - **the stale-row revocations of that pass** — NOT recoverable by waiting, + * because every subsequent pass meets the same organization-less record and + * dies at the same place. A stale over-grant of that rule therefore persists + * indefinitely. That is the security half, and it is pinned SEPARATELY + * below: a catch that swallowed the refusal and then skipped the revocation + * anyway would satisfy the "pass continued" half while leaving the defect + * exactly where it was. + * + * ## Why a real driver and a real engine + * + * The refusal under test is the ENGINE's, raised inside + * `Engine.resolveSystemInsertOrganization` from the ledger classification and + * the deployment posture. A fake engine would have to imitate the very thing + * whose behaviour decides the case. So these cases run a real `SqlDriver` on + * better-sqlite3 `:memory:` behind a real `ObjectQL` on an `isolated` posture, + * the way `record-share-organization-stamp.test.ts` does for the #14484 stamp. + * + * ## Why the fixture's organization-less record is in the MIDDLE + * + * "The pass continued" is only observable if a grant was still ATTEMPTED after + * the refused one. The sweep returns the matched records in insertion order, so + * the fixture inserts `rec_orgless` second of three matching records — and the + * attempt order is asserted rather than assumed, because a fixture that + * silently reordered would turn this pin into a tautology. + * + * ## The catch is NARROW, and one case here holds that line + * + * Only `ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED` is absorbed. The last case + * feeds the loop a DIFFERENT engine error and pins that it still propagates — + * the same direction `record-share-organization-stamp.test.ts` pins for the + * scoped-update `RECORD_NOT_FOUND` shape the 2026-09-02 review left standing. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; + +import { SharingService } from './sharing-service.js'; +import { SharingRuleService } from './sharing-rule-service.js'; + +const OBJECT = 'os14754_deal'; +const ORG_A = 'org_a'; +const ORG_B = 'org_b'; + +const SYSTEM: ExecutionContext = { isSystem: true, positions: [], permissions: [] }; +const WON = { stage: 'won' }; + +const DEAL_FIELDS: Record> = { + id: { type: 'text', name: 'id', label: 'Id', primary: true }, + stage: { type: 'text', name: 'stage', label: 'Stage' }, + owner_id: { type: 'text', name: 'owner_id', label: 'Owner' }, + organization_id: { type: 'text', name: 'organization_id', label: 'Org' }, +}; + +const SHARE_FIELDS: Record> = { + id: { type: 'text', name: 'id', label: 'Id', primary: true }, + organization_id: { type: 'text', name: 'organization_id', label: 'Org' }, + object_name: { type: 'text', name: 'object_name', label: 'Object' }, + record_id: { type: 'text', name: 'record_id', label: 'Record' }, + recipient_type: { type: 'text', name: 'recipient_type', label: 'Recipient type' }, + recipient_id: { type: 'text', name: 'recipient_id', label: 'Recipient' }, + access_level: { type: 'text', name: 'access_level', label: 'Access' }, + source: { type: 'text', name: 'source', label: 'Source' }, + source_id: { type: 'text', name: 'source_id', label: 'Source id' }, + reason: { type: 'text', name: 'reason', label: 'Reason' }, + granted_by: { type: 'text', name: 'granted_by', label: 'Grantor' }, + created_at: { type: 'text', name: 'created_at', label: 'Created' }, + updated_at: { type: 'text', name: 'updated_at', label: 'Updated' }, +}; + +const RULE_FIELDS: Record> = { + id: { type: 'text', name: 'id', label: 'Id', primary: true }, + organization_id: { type: 'text', name: 'organization_id', label: 'Org' }, + name: { type: 'text', name: 'name', label: 'Name' }, + label: { type: 'text', name: 'label', label: 'Label' }, + description: { type: 'text', name: 'description', label: 'Description' }, + object_name: { type: 'text', name: 'object_name', label: 'Object' }, + criteria_json: { type: 'text', name: 'criteria_json', label: 'Criteria' }, + recipient_type: { type: 'text', name: 'recipient_type', label: 'Recipient type' }, + recipient_id: { type: 'text', name: 'recipient_id', label: 'Recipient' }, + access_level: { type: 'text', name: 'access_level', label: 'Access' }, + active: { type: 'boolean', name: 'active', label: 'Active' }, + managed_by: { type: 'text', name: 'managed_by', label: 'Managed by' }, + customized: { type: 'boolean', name: 'customized', label: 'Customized' }, + created_at: { type: 'text', name: 'created_at', label: 'Created' }, + updated_at: { type: 'text', name: 'updated_at', label: 'Updated' }, +}; + +interface ShareRow { + id: string; + record_id: string; + recipient_id: string; + organization_id: string | null; + source_id: string | null; +} + +const open: SqlDriver[] = []; + +/** + * Five records. MEASURED sweep order (not insertion order — see below): + * + * rec_first ORG_A won grant lands + * rec_last ORG_B won grant lands + * rec_orgless (null) won grant REFUSED on a walled posture + * rec_orgless_2 (null) won grant REFUSED — attempted ONLY if the pass continued + * rec_stale ORG_A lost not matched; carries the stale row that must be REVOKED + * + * ⚠️ Measured on this fixture: the engine returns the organization-LESS rows + * LAST, after every organization-carrying row, whatever the insertion order + * (raw driver order is `rec_first, rec_orgless, rec_last, …`; the engine's + * filtered read answers `rec_first, rec_last, rec_orgless, …`). That is the + * driver's NULL-org compatibility arm being appended to the scoped arm, the + * same "and — through the driver's compatibility arm — deal_p1, in that order" + * `record-share-organization-stamp.test.ts` records. + * + * The consequence matters for this card and is why the fixture holds TWO + * organization-less records: on the real shape a refused grant is usually one + * of the LAST attempts of the pass, so what an abort destroyed was hardly ever + * "the remaining grants" — it was almost entirely the STALE-ROW REVOCATIONS + * that run after the whole upsert loop. The security half is not merely the + * more serious half, it is very nearly the ONLY half. A second refusal is + * therefore the order-independent witness that the loop survived the first. + */ +async function boot(posture: 'single' | 'isolated' | 'group' = 'isolated') { + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + open.push(driver); + + const ql = new ObjectQL(); + ql.registerDriver(driver as never, true); + await ql.init(); + ql.setTenancyPostureProvider(() => posture); + ql.registerObject({ name: OBJECT, label: 'Deal', sharingModel: 'private', fields: DEAL_FIELDS } as never); + ql.registerObject({ name: 'sys_record_share', label: 'Record Share', isSystem: true, fields: SHARE_FIELDS } as never); + ql.registerObject({ name: 'sys_sharing_rule', label: 'Sharing Rule', isSystem: true, fields: RULE_FIELDS } as never); + await driver.initObjects([ + { name: OBJECT, fields: DEAL_FIELDS } as never, + { name: 'sys_record_share', fields: SHARE_FIELDS } as never, + { name: 'sys_sharing_rule', fields: RULE_FIELDS } as never, + ]); + + await driver.create(OBJECT, { id: 'rec_first', stage: 'won', owner_id: 'u_a', organization_id: ORG_A } as never); + await driver.create(OBJECT, { id: 'rec_orgless', stage: 'won', owner_id: 'u_p' } as never); + await driver.create(OBJECT, { id: 'rec_last', stage: 'won', owner_id: 'u_b', organization_id: ORG_B } as never); + await driver.create(OBJECT, { id: 'rec_orgless_2', stage: 'won', owner_id: 'u_p' } as never); + await driver.create(OBJECT, { id: 'rec_stale', stage: 'lost', owner_id: 'u_a', organization_id: ORG_A } as never); + + const warn = vi.fn(); + const sharing = new SharingService({ engine: ql as never }); + const rules = new SharingRuleService({ engine: ql as never, sharing, logger: { warn } }); + + return { + driver, + ql, + sharing, + rules, + warn, + /** Every `sys_record_share` row, read straight off the driver. */ + shares: async (): Promise => { + const rows = (await driver.find('sys_record_share', {} as never)) as any[]; + return rows + .map((r) => ({ + id: String(r.id), + record_id: String(r.record_id), + recipient_id: String(r.recipient_id), + organization_id: r.organization_id ?? null, + source_id: r.source_id ?? null, + })) + .sort((a, b) => a.record_id.localeCompare(b.record_id) || a.recipient_id.localeCompare(b.recipient_id)); + }, + /** A pre-existing rule grant the next pass should find stale and revoke. */ + seedStaleRow: (id: string, recordId: string, ruleId: string, recipientId = 'u_plat', organizationId: string | null = ORG_A) => + driver.create('sys_record_share', { + id, + object_name: OBJECT, + record_id: recordId, + recipient_type: 'user', + recipient_id: recipientId, + access_level: 'read', + source: 'rule', + source_id: ruleId, + ...(organizationId ? { organization_id: organizationId } : {}), + created_at: '2026-01-01T00:00:00Z', + } as never), + /** Records the recordId of every grant ATTEMPT, in order, and calls through. */ + traceGrants: () => { + const attempted: string[] = []; + const original = sharing.grant.bind(sharing); + vi.spyOn(sharing, 'grant').mockImplementation((async (input: any, ctx: any) => { + attempted.push(String(input.recordId)); + return original(input, ctx); + }) as never); + return attempted; + }, + }; +} + +afterEach(async () => { + vi.restoreAllMocks(); + while (open.length) await open.pop()?.disconnect?.(); +}); + +const platformGlobalRule = (rules: SharingRuleService, name: string, accessLevel: 'read' | 'edit' = 'read') => + rules.defineRule( + { + name, + label: 'Platform won', + object: OBJECT, + criteria: WON, + recipientType: 'user', + recipientId: 'u_plat', + accessLevel, + } as never, + SYSTEM, + ); + +describe('[#14754] reconcile: a refused grant is counted and the pass CONTINUES', () => { + it('CONTROL: on an `isolated` posture the organization-less record really is refused — the precondition, measured', async () => { + const { ql, shares } = await boot('isolated'); + await expect( + ql.insert( + 'sys_record_share', + { + id: 'shr_bare', + object_name: OBJECT, + record_id: 'rec_orgless', + recipient_type: 'user', + recipient_id: 'u_plat', + access_level: 'read', + source: 'rule', + }, + { context: SYSTEM } as never, + ), + ).rejects.toMatchObject({ code: 'ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED', status: 500 }); + expect(await shares()).toEqual([]); + }); + + it('HALF 1 — the pass does not abort: the refusal is counted, and the grant AFTER it still lands', async () => { + const { rules, shares } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_continues'); + expect(rule.organization_id).toBeNull(); + + const result = await rules.evaluateRule(rule.id, SYSTEM); + + // It RESOLVED. Before this card it REJECTED with the engine's refusal. + expect(result.grantsRefused).toBe(2); + expect(result.grantsCreated).toBe(2); + expect(result.matchedRecords).toBe(4); + + // Both organization-carrying records got their grant; neither refused one has any. + const rows = await shares(); + expect(rows.map((r) => [r.record_id, r.organization_id])).toEqual([ + ['rec_first', ORG_A], + ['rec_last', ORG_B], + ]); + }); + + it('HALF 1, continuation witness — a grant is still ATTEMPTED after the first refusal', async () => { + const booted = await boot('isolated'); + const rule = await platformGlobalRule(booted.rules, 'os14754_order'); + const attempted = booted.traceGrants(); + + const result = await booted.rules.evaluateRule(rule.id, SYSTEM); + + // The measured order — organization-carrying rows first, the NULL-org + // compatibility arm appended. Asserted rather than assumed, so a change in + // the engine's read shape shows up here instead of quietly turning the + // relation below into a tautology. + expect(attempted).toEqual(['rec_first', 'rec_last', 'rec_orgless', 'rec_orgless_2']); + + // The load-bearing relation: the FIRST refusal is not the last attempt of + // the pass, and the attempt after it was made. A pass that aborted at the + // first refusal could not have attempted `rec_orgless_2` at all, and could + // not have counted two refusals. + const firstRefusalAt = attempted.indexOf('rec_orgless'); + expect(firstRefusalAt).toBeLessThan(attempted.length - 1); + expect(result.grantsRefused).toBe(2); + }); + + it('HALF 2 (the security half) — the STALE ROW of that same pass is REVOKED', async () => { + const { rules, shares, seedStaleRow } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_revokes'); + + // `rec_stale` is `lost`, so the rule does not match it: a completed pass + // revokes its leftover grant. It is the witness that the pass reached its + // revoke loop at all. + await seedStaleRow('shr_stale', 'rec_stale', rule.id); + expect((await shares()).map((r) => r.id)).toEqual(['shr_stale']); + + const result = await rules.evaluateRule(rule.id, SYSTEM); + + expect(result.grantsRevoked).toBe(1); + expect(result.grantsRefused).toBe(2); + // The row is GONE from the table — not merely counted. + const rows = await shares(); + expect(rows.map((r) => r.id)).not.toContain('shr_stale'); + expect(rows.map((r) => r.record_id)).toEqual(['rec_first', 'rec_last']); + }); + + it('the refusal is LOGGED with the rule, object, record and the engine code', async () => { + const { rules, warn } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_logs'); + + await rules.evaluateRule(rule.id, SYSTEM); + + const refusals = warn.mock.calls.filter((c) => String(c[0]).includes('refused by the engine organization rule')); + // One line per refused GRANT, naming the record — the operator's only route + // from "this rule reports refusals" to "these are the records to repair". + expect(refusals).toHaveLength(2); + expect(refusals.map((c) => (c[1] as any).record)).toEqual(['rec_orgless', 'rec_orgless_2']); + expect(refusals[0]![1]).toMatchObject({ + rule: 'os14754_logs', + object: OBJECT, + record: 'rec_orgless', + recipient: 'u_plat', + code: 'ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED', + }); + }); + + it('MEASURED BOUNDARY — the refusal gates the INSERT half only: an organization-less row that ALREADY exists is UPDATED, not refused', async () => { + const { driver, rules, shares } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_update_half', 'read'); + + // A pre-existing rule grant on the organization-less record, carrying no + // organization (which is how it got there before #14484). The pass wants + // to raise it to `edit`. + await driver.create('sys_record_share', { + id: 'shr_orgless_old', object_name: OBJECT, record_id: 'rec_orgless', recipient_type: 'user', + recipient_id: 'u_plat', access_level: 'read', source: 'rule', source_id: rule.id, + created_at: '2026-01-01T00:00:00Z', + } as never); + + const raised = await rules.defineRule( + { name: 'os14754_update_half', label: 'Platform won', object: OBJECT, criteria: WON, recipientType: 'user', recipientId: 'u_plat', accessLevel: 'edit' } as never, + SYSTEM, + ); + expect(raised.id).toBe(rule.id); + + const result = await rules.evaluateRule(rule.id, SYSTEM); + + // The engine's rule refuses an INSERT ("Insert on '' was + // REFUSED"); the update half is not gated by it, and `grant` sends an + // organization-less update UNSCOPED (`tenantId: undefined`), which reaches + // the NULL row. So `rec_orgless` is UPDATED here while `rec_orgless_2` — + // which has no row yet — is refused. Measured, and recorded because it + // bounds this card: a refusal cannot reach the `cur` (update) branch of + // either loop today. + const rows = await shares(); + const updatedRow = rows.find((r) => r.id === 'shr_orgless_old'); + expect(updatedRow).toBeDefined(); + expect(result.grantsUpdated).toBe(1); + expect(result.grantsRefused).toBe(1); + expect(result.grantsRevoked).toBe(0); + }); + + it('an unrelated engine error is NOT absorbed — the catch is narrow, and stays narrow', async () => { + const { ql, rules } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_narrow'); + + // A different failure on the same seam: the grant's own insert blows up + // with something that is not the organization refusal. + const originalInsert = ql.insert.bind(ql); + vi.spyOn(ql, 'insert').mockImplementation((async (object: string, doc: any, options?: any) => { + if (object === 'sys_record_share') { + const err: any = new Error('simulated driver outage'); + err.code = 'ERR_DRIVER_UNAVAILABLE'; + throw err; + } + return originalInsert(object, doc, options); + }) as never); + + await expect(rules.evaluateRule(rule.id, SYSTEM)).rejects.toMatchObject({ code: 'ERR_DRIVER_UNAVAILABLE' }); + }); +}); + +describe('[#14754] reconcileForRecord: the same two halves on the per-record hook path', () => { + it('HALF 2 — the refused grant does not stop that record\'s OWN stale revocation', async () => { + const { rules, shares, seedStaleRow } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_hook_revokes'); + + // A recipient the rule no longer expands to, on the organization-less + // record itself. The pass's grant for `u_plat` is refused; `u_gone`'s row + // is stale and must still go. + await seedStaleRow('shr_hook_stale', 'rec_orgless', rule.id, 'u_gone', null); + expect((await shares()).map((r) => r.id)).toEqual(['shr_hook_stale']); + + const [result] = await rules.evaluateAllForRecord(OBJECT, 'rec_orgless', SYSTEM); + + expect(result).toMatchObject({ grantsRefused: 1, grantsRevoked: 1, grantsCreated: 0 }); + expect(await shares()).toEqual([]); + }); + + it('HALF 1 — one rule refused no longer aborts the whole per-record sweep: the NEXT rule still reconciles', async () => { + const { rules, shares, seedStaleRow } = await boot('isolated'); + const refusing = await platformGlobalRule(rules, 'os14754_hook_a'); + const following = await platformGlobalRule(rules, 'os14754_hook_b', 'edit'); + + // The second rule's own stale row on the same record — reachable only if + // the first rule's refusal did not abort `evaluateAllForRecord`. + await seedStaleRow('shr_hook_b_stale', 'rec_orgless', following.id, 'u_gone', null); + + const results = await rules.evaluateAllForRecord(OBJECT, 'rec_orgless', SYSTEM); + + expect(results).toHaveLength(2); + expect(results.map((r) => r.ruleId)).toEqual([refusing.id, following.id]); + expect(results.every((r) => r.grantsRefused === 1)).toBe(true); + expect(results[1]!.grantsRevoked).toBe(1); + expect(await shares()).toEqual([]); + }); + + it('an organization-CARRYING record on the same posture is untouched by any of this', async () => { + const { rules, shares } = await boot('isolated'); + const rule = await platformGlobalRule(rules, 'os14754_hook_control'); + + const [result] = await rules.evaluateAllForRecord(OBJECT, 'rec_first', SYSTEM); + + expect(result).toMatchObject({ grantsRefused: 0, grantsCreated: 1 }); + expect((await shares()).map((r) => [r.record_id, r.organization_id, r.source_id])).toEqual([ + ['rec_first', ORG_A, rule.id], + ]); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 42a94b415a..dc843b134f 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -21,6 +21,14 @@ import { BUILTIN_IDENTITY_PLATFORM_ADMIN } from '@objectstack/spec/identity'; // — one honouring the 1/0 and 'false' storage shapes, one not — is how the // enforcement hole this closes gets re-opened one seam over. import { isRowActive } from '@objectstack/core'; +// [#14754] The engine's organization refusal for a system write on a +// tenant-scoped object (#8844). Imported as a TYPE only: the runtime check is a +// `code` compare, which is the convention that class itself documents ("a +// caller that catches it identifies it by `code`") so the check survives the +// package boundary where two copies of that module can exist. Typing the +// literal FROM the class is what keeps the two spellings from drifting — a +// typo here would not be a failing test, it would be a catch that never fires. +import type { SystemWriteOrganizationRequiredError } from '@objectstack/objectql'; import type { SharingEngine } from './sharing-service.js'; import type { SharingService } from './sharing-service.js'; import { normalizeAccessLevel, normalizeStoredAccessLevel } from './access-level.js'; @@ -70,6 +78,35 @@ function rowFromRule(row: any): SharingRuleRow { }; } +/** + * [#14754] The one engine refusal a reconcile pass absorbs per grant. + * + * Spelled once, and typed from the engine's own declaration so it cannot drift + * from the code the engine actually throws. + */ +const ENGINE_ORGANIZATION_REFUSAL_CODE: SystemWriteOrganizationRequiredError['code'] = + 'ERR_SYSTEM_WRITE_ORGANIZATION_REQUIRED'; + +/** + * [#14754] What one reconcile pass did, plus the grants the engine's + * organization rule REFUSED in it. + * + * Extends the spec's {@link SharingRuleEvaluationResult} rather than changing + * it: the contract lives in `@objectstack/spec` and is another lane's to move, + * and every declared consumer keeps compiling against the six fields it always + * had. The seventh is additive and rides along for the callers that want it — + * the boot backfill's aggregate and this package's own pins. + * + * ⛔ `grantsRefused > 0` is NOT "the pass failed". It is the pass reporting that + * it met a record it cannot grant on (an organization-less record under a + * walled posture) and CONTINUED — which is the whole point: the remaining + * grants and, above all, the stale-row revocations still ran. + */ +export interface SharingRuleReconcilePassResult extends SharingRuleEvaluationResult { + /** Grants the engine's organization rule refused; each one is also logged. */ + grantsRefused: number; +} + export interface SharingRuleServiceOptions { engine: SharingEngine; sharing: SharingService; @@ -655,14 +692,15 @@ export class SharingRuleService implements ISharingRuleService { } as any); } - async evaluateRule(idOrName: string, context: ExecutionContext): Promise { + async evaluateRule(idOrName: string, context: ExecutionContext): Promise { this.assertCanManageRules(context); // [ADR-0111 D6] const rule = await this.getRule(idOrName, context); if (!rule) throw new Error('RULE_NOT_FOUND'); if (!rule.active) { // Inactive — purge any leftover grants and report revoke count. const revoked = await this.purgeRuleGrants(rule.id); - return { ruleId: rule.id, matchedRecords: 0, expandedUsers: 0, grantsCreated: 0, grantsUpdated: 0, grantsRevoked: revoked }; + // [#14754] An inactive rule attempts no grant, so it can refuse none. + return { ruleId: rule.id, matchedRecords: 0, expandedUsers: 0, grantsCreated: 0, grantsUpdated: 0, grantsRevoked: revoked, grantsRefused: 0 }; } const matches = await this.findMatchingRecords(rule); const users = await this.expandRecipient(rule); @@ -749,10 +787,10 @@ export class SharingRuleService implements ISharingRuleService { object: string, recordId: string, context: ExecutionContext, - ): Promise { + ): Promise { const rules = await this.listRules({ object }, context); if (rules.length === 0) return []; - const results: SharingRuleEvaluationResult[] = []; + const results: SharingRuleReconcilePassResult[] = []; // [#8710] ONE pass, so N rules naming the same position pay ONE catalogue // read — and the memo dies with this call, so the next pass re-reads and a // deactivation is honoured immediately. @@ -1236,11 +1274,86 @@ export class SharingRuleService implements ISharingRuleService { } } + /** + * [#14754] Materialise ONE rule grant, absorbing the engine's organization + * refusal so the pass survives it. + * + * ## The defect this closes, and which half of it is the security half + * + * `sys_record_share` is tenant-scoped in the #13491 ledger (#14484), so on a + * walled install an organization-less system insert on it is refused loudly + * with {@link ENGINE_ORGANIZATION_REFUSAL_CODE}. `SharingService.grant` + * resolves the organization on every path that can; a platform-global rule + * (`organization_id = null`, its sweep unscoped) materialising onto an + * organization-less record resolves none, and meets the refusal. + * + * Without this catch the refusal propagated out of the reconcile loop and + * that rule's pass ABORTED mid-loop. The grants already written stayed; the + * remaining grants — and **the stale-row revocations of that pass** — did + * not happen. The revocation half is the security-relevant one: a stale + * over-grant of that rule persisted across every pass that met the same + * record, and that record kept aborting the pass until it was repaired. + * + * ## Why the catch is narrow, and must stay narrow + * + * ONLY {@link ENGINE_ORGANIZATION_REFUSAL_CODE} is absorbed; everything else + * rethrows unchanged. Two reasons, and the second is measured: + * + * - A catch-all would swallow real defects — a driver outage, a criteria + * bug, a permission failure — and report a pass that "completed" having + * written nothing, which is the silence #6783 spent a card removing. + * - `record-share-organization-stamp.test.ts` DELIBERATELY pins the abort on + * the OTHER error a reconcile pass can meet here: the scoped update half + * answering `RECORD_NOT_FOUND` (404) for a row stamped with a different + * organization. That shape was reviewed on 2026-09-02 and left standing + * ("loud beats a wrong count"). Widening this catch to any error would + * retire that decision as a side effect, and its three pins are what + * notices. + * + * Returns `true` when the grant landed, `false` when it was refused — + * never throws for the refusal, and the caller counts what it is told. + */ + private async grantOrAbsorbOrganizationRefusal( + rule: SharingRuleRow, + recordId: string, + recipientId: string, + ): Promise { + try { + await this.sharing.grant( + { + object: rule.object_name, + recordId, + recipientType: 'user', + recipientId, + accessLevel: rule.access_level, + source: 'rule', + sourceId: rule.id, + reason: `rule:${rule.name}`, + } as any, + this.criteriaContext(rule), + ); + return true; + } catch (err: any) { + if (err?.code !== ENGINE_ORGANIZATION_REFUSAL_CODE) throw err; + this.logger?.warn?.( + '[sharing-rule] grant refused by the engine organization rule — counted, pass continues', + { + rule: rule.name ?? rule.id, + object: rule.object_name, + record: recordId, + recipient: recipientId, + code: ENGINE_ORGANIZATION_REFUSAL_CODE, + }, + ); + return false; + } + } + private async reconcile( rule: SharingRuleRow, matchedIds: string[], users: string[], - ): Promise { + ): Promise { const existing = await this.engine.find('sys_record_share', { where: { source: 'rule', source_id: rule.id }, fields: ['id', 'record_id', 'recipient_id', 'access_level'], @@ -1257,43 +1370,30 @@ export class SharingRuleService implements ISharingRuleService { let created = 0; let updated = 0; let revoked = 0; + let refused = 0; - // Upsert desired. + // Upsert desired. [#14754] Per grant, not per pass: a refusal is counted + // and the loop goes on, so the revoke loop below is REACHED. for (const [k, want] of desired.entries()) { const cur = existingMap.get(k); if (cur) { + // [#14754] Taken out of the stale set BEFORE the grant is attempted. + // The row is DESIRED; a refused level update must leave it standing. + // Deleting it only on success would drop it through to the revoke loop + // and retract a grant the rule still wants — this card's defect + // inverted, and strictly worse than the abort it replaces. + existingMap.delete(k); if (cur.access_level !== rule.access_level) { - await this.sharing.grant( - { - object: rule.object_name, - recordId: want.record_id, - recipientType: 'user', - recipientId: want.recipient_id, - accessLevel: rule.access_level, - source: 'rule', - sourceId: rule.id, - reason: `rule:${rule.name}`, - } as any, - this.criteriaContext(rule), - ); - updated += 1; + if (await this.grantOrAbsorbOrganizationRefusal(rule, want.record_id, want.recipient_id)) { + updated += 1; + } else { + refused += 1; + } } - existingMap.delete(k); - } else { - await this.sharing.grant( - { - object: rule.object_name, - recordId: want.record_id, - recipientType: 'user', - recipientId: want.recipient_id, - accessLevel: rule.access_level, - source: 'rule', - sourceId: rule.id, - reason: `rule:${rule.name}`, - } as any, - this.criteriaContext(rule), - ); + } else if (await this.grantOrAbsorbOrganizationRefusal(rule, want.record_id, want.recipient_id)) { created += 1; + } else { + refused += 1; } } // Revoke stale. @@ -1309,6 +1409,7 @@ export class SharingRuleService implements ISharingRuleService { grantsCreated: created, grantsUpdated: updated, grantsRevoked: revoked, + grantsRefused: refused, }; } @@ -1317,7 +1418,7 @@ export class SharingRuleService implements ISharingRuleService { recordId: string, match: boolean, users: string[], - ): Promise { + ): Promise { const existing = await this.engine.find('sys_record_share', { where: { source: 'rule', source_id: rule.id, record_id: recordId }, fields: ['id', 'record_id', 'recipient_id', 'access_level'], @@ -1330,43 +1431,30 @@ export class SharingRuleService implements ISharingRuleService { let created = 0; let updated = 0; let revoked = 0; + let refused = 0; if (match) { + // [#14754] Per grant, exactly as in {@link reconcile}: one recipient the + // engine's organization rule refuses no longer takes the rest of this + // record's pass — nor its revoke loop — with it. for (const userId of users) { const cur = existingMap.get(userId); if (cur) { + // [#14754] Out of the stale set BEFORE the attempt — see the twin + // comment in {@link reconcile}: a refused level update must leave the + // desired row standing, never fall through to the revoke loop. + existingMap.delete(userId); if (cur.access_level !== rule.access_level) { - await this.sharing.grant( - { - object: rule.object_name, - recordId, - recipientType: 'user', - recipientId: userId, - accessLevel: rule.access_level, - source: 'rule', - sourceId: rule.id, - reason: `rule:${rule.name}`, - } as any, - this.criteriaContext(rule), - ); - updated += 1; + if (await this.grantOrAbsorbOrganizationRefusal(rule, recordId, userId)) { + updated += 1; + } else { + refused += 1; + } } - existingMap.delete(userId); - } else { - await this.sharing.grant( - { - object: rule.object_name, - recordId, - recipientType: 'user', - recipientId: userId, - accessLevel: rule.access_level, - source: 'rule', - sourceId: rule.id, - reason: `rule:${rule.name}`, - } as any, - this.criteriaContext(rule), - ); + } else if (await this.grantOrAbsorbOrganizationRefusal(rule, recordId, userId)) { created += 1; + } else { + refused += 1; } } } @@ -1384,6 +1472,7 @@ export class SharingRuleService implements ISharingRuleService { grantsCreated: created, grantsUpdated: updated, grantsRevoked: revoked, + grantsRefused: refused, }; } From 7919e803667f4a66ada8e204d83545f3ee976a39 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 10:28:46 +0000 Subject: [PATCH 2/2] docs(permissions): re-anchor the system-context census to the two shifted elevation reads `check:check-system-context-census` failed with pure line rot: the per-grant catch added 37 lines above `sharing-rule-service.ts`'s two `context?.isSystem` elevation reads, so the census page's anchors pointed at :157/:382 while the reads now sit at :194/:419. Re-anchored with the gate's own `--fix`; no prose and no behaviour changed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- content/docs/permissions/system-context.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 6990eb7383..120559d401 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -137,7 +137,7 @@ The largest single consumer — **20 of the 109 sites**. | 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` | | 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` | +| 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:194`, `:419` | ### 4. Approvals, reports, attachments, comments, knowledge