From f5b99a359dbd17e80b13d6ca9604f2e5202e08f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 05:28:03 +0000 Subject: [PATCH] fix(plugin-security,plugin-sharing): write path consults the VAMA bypass through one shared predicate (#4647) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `security/explain` and the data write path answered one (principal, record, operation) triple differently: a Modify All Data holder against an OWNERLESS row of a `private` object got `allowed: true` with a `vama_bypass` layer claiming "ownership and sharing checks are skipped", and a 403 from `PATCH /data/…`. Filling `owner_id` in made the same PATCH succeed, so the write path was running the record-level ownership check the layer said was skipped; `sys_attachment`'s `canEdit(parent)` gate agreed with the 403. Per the maintainer ruling (option A), the write path was the wrong side: - `PermissionEvaluator.superuserBypassSets` is now THE bypass predicate, returning the granting set names. `hasSuperuserReadBypass` / `hasSuperuserWriteBypass` delegate to it, so `ISecurityService.hasWriteBypass` and explain's `vama_bypass` layer fold through one function. - `SharingService.canEdit` / `canDelete` consult that bypass via the existing late-bound probe AFTER ownership and shares fail (the `__writeScope === 'org'` proxy they leaned on is only reached past a NULL-owner early return). `canManageShares` shares the same helper. The attachment parent gate and the comment gate converge for free — they call `canEdit`. - The widening is exactly Modify-scoped: explain's layer is operation-aware (modify bit on writes, view bit on reads) and names the missing bit when a View All Data holder is refused a write. The probe still fails closed with no plugin-security, a throwing probe, or an on-behalf-of context. - Record-grained explain reflects the bypass: `decidedBy: 'vama_bypass'`, a per-record attribution on the layer, and a sharing-layer detail that credits the bypass instead of reporting "no share grants write" beside `allowed: true`. Tests: a cross-path convergence suite wires the real SecurityPlugin service, the real SharingService and the real security+sharing middleware chain over one engine and asserts explain == the write for update, delete and the attachment gate, plus the non-VAMA and view-only negative controls. Fixes #4647 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t --- .changeset/vama-write-path-bypass.md | 58 +++ packages/plugins/plugin-security/package.json | 1 + .../src/explain-engine.test.ts | 62 +++ .../plugin-security/src/explain-engine.ts | 131 +++++-- .../src/permission-evaluator.ts | 89 ++++- .../src/vama-write-path-convergence.test.ts | 366 ++++++++++++++++++ .../src/sharing-service.test.ts | 85 ++++ .../plugin-sharing/src/sharing-service.ts | 85 +++- pnpm-lock.yaml | 3 + 9 files changed, 825 insertions(+), 55 deletions(-) create mode 100644 .changeset/vama-write-path-bypass.md create mode 100644 packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts diff --git a/.changeset/vama-write-path-bypass.md b/.changeset/vama-write-path-bypass.md new file mode 100644 index 0000000000..c8d0adc64e --- /dev/null +++ b/.changeset/vama-write-path-bypass.md @@ -0,0 +1,58 @@ +--- +"@objectstack/plugin-security": patch +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-security,plugin-sharing): the write path consults the View/Modify All Data bypass — one predicate for `security/explain` and `/data` (#4647) + +A **Modify All Data** holder, a `sharingModel: 'private'` object, and a record +whose `owner_id` is NULL got two opposite answers for one +(principal, record, operation) triple: + +``` +POST /api/v1/security/explain { object, operation: 'update', recordId } + → allowed: true, layers[vama_bypass]: "View/Modify All Data bypass held + via [admin_full_access] — ownership and sharing checks are skipped" +PATCH /api/v1/data/crm_contract/ + → 403 FORBIDDEN +``` + +Filling `owner_id` in made the same PATCH succeed, so the write path really was +running the record-level ownership check the bypass layer said had been skipped. +`sys_attachment`'s `canEdit(parent)` gate agreed with the 403, not with explain. +Ownerless rows are not exotic: a system-context seed writes them by design (the +seed loader disables `owner_id` injection). + +**The write path was the side that was wrong.** Modify All Data means an admin +edits any record regardless of ownership (the Salesforce reference frame this +platform's `modifyAllRecords` already follows, #1883), so: + +- `SharingService.canEdit` / `canDelete` now consult the super-user write bypass + **after** ownership and shares have failed, through the existing late-bound + `ISecurityService.hasWriteBypass` probe. The `sys_attachment` + `canEdit(parent)` gate and the sharing-rule management gate reach the same + answer because they call the same function. +- The bypass they consult and the one `security/explain` reports are now **one + predicate** — `PermissionEvaluator.superuserBypassSets` — rather than two + independent readings of the permission sets. A cross-path test pins the triple + through both `explain` and the real write middleware chain and asserts they + agree, for update, delete and the attachment gate. + +**The widening is exactly Modify-scoped.** `viewAllRecords` ("View All Data") is +a read power and never grants write: explain's `vama_bypass` layer is now +operation-aware, asking for the modify bit on a write and the view bit on a read, +and a view-only holder is refused on both paths. The probe still fails **closed** +— no `@objectstack/plugin-security`, a throwing probe, a principal-less or +on-behalf-of context all degrade to owner-only. + +**Explain payload self-consistency.** For a record-grained request the top-level +`allowed` and the `record` verdict no longer contradict each other on this +triple: the row is `visible: true` with `decidedBy: 'vama_bypass'`, the +`vama_bypass` layer carries its own per-record attribution, and the `sharing` +layer credits the bypass instead of reporting "no ownership and no edit/full +share grants write" next to `allowed: true`. Where the bypass is not what +admitted the row (owner, or an admitting share) the previous `decidedBy` is +unchanged. Note that for a principal with **no** bypass, an object-level +`allowed: true` beside `record.visible: false` remains correct and intended — +`allowed` answers the object question, `record` answers the row question, and it +is the `record` verdict that the write path mirrors. diff --git a/packages/plugins/plugin-security/package.json b/packages/plugins/plugin-security/package.json index fb78ee43ca..400c1317b1 100644 --- a/packages/plugins/plugin-security/package.json +++ b/packages/plugins/plugin-security/package.json @@ -24,6 +24,7 @@ "@objectstack/spec": "workspace:*" }, "devDependencies": { + "@objectstack/plugin-sharing": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/plugins/plugin-security/src/explain-engine.test.ts b/packages/plugins/plugin-security/src/explain-engine.test.ts index c28701db15..5ed2a2f399 100644 --- a/packages/plugins/plugin-security/src/explain-engine.test.ts +++ b/packages/plugins/plugin-security/src/explain-engine.test.ts @@ -293,6 +293,68 @@ describe('explainAccess — record-grained (C2 / ADR-0095)', () => { expect(d.record).toMatchObject({ recordId: 'r1', visible: true }); }); + // ── [#4647] the bypass, at row granularity ────────────────────────────── + // The report: `allowed: true` + a `vama_bypass` layer saying "ownership and + // sharing checks are skipped", sitting next to `record: { visible: false, + // decidedBy: 'sharing' }` — one payload, two opposite answers. Under the + // ruling the bypass is real, so the ROW story has to show it (and the write + // gate, which now consults the same predicate, agrees). + const VIEW_ONLY = PermissionSetSchema.parse({ + name: 'compliance_auditor', + objects: { '*': { allowRead: true, allowEdit: true, viewAllRecords: true } }, + }); + const OWNERLESS_ROW = { id: 'r1', organization_id: 'org1', owner_id: null }; + const ADMIN_CTX = { userId: 'a1', tenantId: 'org1', positions: ['platform_admin', 'everyone'], permissions: [] }; + + it('[#4647] Modify All Data admits an OWNERLESS private record — record verdict agrees with the top level', async () => { + const d = await explainAccess( + recDeps({ + sets: [ADMIN], layered: { layer0: null, layer1: null }, + record: OWNERLESS_ROW, shares: [], sharingFilter: { owner_id: 'a1' }, + canEdit: true, // the fixed gate: ownership fails, the bypass admits + }), + { object: 'leave_request', operation: 'update', context: ADMIN_CTX, recordId: 'r1' }, + ); + expect(d.allowed).toBe(true); + expect(d.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' }); + const vama = d.layers.find((l) => l.layer === 'vama_bypass')!; + expect(vama.verdict).toBe('widens'); + expect(vama.record!.outcome).toBe('admitted'); + // The sharing layer credits the bypass rather than claiming a share it never saw. + expect(d.layers.find((l) => l.layer === 'sharing')!.record!.detail).toContain('Modify All Data bypass'); + }); + + it('[#4647] View All Data alone does NOT bypass a WRITE — and the layer names the missing bit', async () => { + const d = await explainAccess( + recDeps({ + sets: [VIEW_ONLY], layered: { layer0: null, layer1: null }, + record: OWNERLESS_ROW, shares: [], sharingFilter: { owner_id: 'a1' }, + canEdit: false, // the gate refuses: no modify bit + }), + { object: 'leave_request', operation: 'update', context: ADMIN_CTX, recordId: 'r1' }, + ); + expect(d.record!.visible).toBe(false); + const vama = d.layers.find((l) => l.layer === 'vama_bypass')!; + expect(vama.verdict).toBe('not_applicable'); + expect(vama.detail).toContain('View All Data'); + expect(vama.detail).toContain('Modify All Data'); + expect(vama.contributors).toEqual([]); + }); + + it('[#4647] the same View All Data set DOES bypass a READ of that record', async () => { + const d = await explainAccess( + recDeps({ + sets: [VIEW_ONLY], layered: { layer0: null, layer1: null }, + record: OWNERLESS_ROW, shares: [], sharingFilter: { owner_id: 'a1' }, + }), + { object: 'leave_request', operation: 'read', context: ADMIN_CTX, recordId: 'r1' }, + ); + const vama = d.layers.find((l) => l.layer === 'vama_bypass')!; + expect(vama.verdict).toBe('widens'); + expect(vama.contributors.map((c) => c.name)).toEqual(['compliance_auditor']); + expect(d.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' }); + }); + it('degrades gracefully with no record-grained deps — object-level layers plus a best-effort verdict', async () => { // Only the base object-level deps: recordId is given but fetchRecord / // computeLayeredRlsFilter etc. are absent (e.g. no plugin-sharing). diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 585d8c91be..7ab7135794 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -32,6 +32,7 @@ import type { ExplainRecordAttribution, } from '@objectstack/spec/security'; import type { PermissionEvaluator } from './permission-evaluator.js'; +import { superuserBypassBitForOperation } from './permission-evaluator.js'; const SYSTEM_CTX = { isSystem: true } as const; @@ -480,6 +481,16 @@ interface RecordAttributionContext { owd: { model: string; effect: 'private' | 'read' | 'public' }; capsDeny: boolean; crudAllowed: boolean; + /** + * [#4647] Whether the object-level pass found the View/Modify All Data bypass + * EFFECTIVE for this operation (already D10-intersected, already + * operation-scoped to the right bit). The row story consumes the verdict the + * `vama_bypass` layer published — it never re-derives it — so the layer, the + * record attribution and the write gate stay one answer. + */ + vamaEffective: boolean; + /** [#4647] The sets that carry the bypass, for the row-level detail text. */ + vamaSets: string[]; } /** @@ -498,7 +509,7 @@ interface RecordAttributionContext { async function applyRecordAttribution( ra: RecordAttributionContext, ): Promise<{ record: NonNullable; posture: AuthzPosture }> { - const { deps, object, recordId, engineOp, context, sets, layers, owd, capsDeny, crudAllowed } = ra; + const { deps, object, recordId, engineOp, context, sets, layers, owd, capsDeny, crudAllowed, vamaEffective, vamaSets } = ra; const isRead = engineOp === 'find'; const posture = derivePosture(context); @@ -651,14 +662,44 @@ async function applyRecordAttribution( ? 'Baseline is not private — sharing adds nothing beyond it for this record.' : canEdit !== undefined ? (canEdit - ? 'The sharing service grants write on this record (ownership or an edit/full share).' + // [#4647] Name the REAL reason the gate admitted the row. The + // write gate consults the Modify All Data bypass after + // ownership and shares, so "the sharing service grants write" + // would be a false attribution for a bypass holder — precisely + // the mis-reporting this issue was filed on, inverted. + ? (!ownerIsMe && !anyShareAdmits && vamaEffective + ? `The write gate admits this record via the Modify All Data bypass [${vamaSets.join(', ')}], ` + + 'not ownership or a share (see the vama_bypass layer).' + : 'The sharing service grants write on this record (ownership or an edit/full share).') : 'No ownership and no edit/full share grants write on this record.') : sharingOutcome === 'admitted' ? (ownerIsMe ? 'Caller owns the record — visible without a share.' : `${shareRules.length} share(s) attached; access is granted for this record.`) - : `${shareRules.length} share(s) attached; none grants the caller access to this record.`, + : `${shareRules.length} share(s) attached; none grants the caller access to this record.` + + (vamaEffective + ? ` Superseded by the View/Modify All Data bypass [${vamaSets.join(', ')}] — see the vama_bypass layer.` + : ''), }; } + // ── vama_bypass: what the bypass did to THIS row ───────────────────────── + // [#4647] The layer that claims "ownership and sharing checks are skipped" + // now says so at row granularity too, and says it from the same verdict the + // write gate consults. + const vamaLayer = layers.find((l) => l.layer === 'vama_bypass'); + if (vamaLayer) { + vamaLayer.record = !recordExists + ? { outcome: 'not_evaluated', rules: [], detail: 'Record not found; the bypass was not evaluated.' } + : vamaEffective + ? { + outcome: 'admitted', + rowFilter: null, + rules: [], + detail: `View/Modify All Data via [${vamaSets.join(', ')}] admits this record regardless of ownership — ` + + 'the same bypass the write path consults (#4647).', + } + : { outcome: 'not_evaluated', rules: [], detail: 'No View/Modify All Data bypass applies to this record.' }; + } + // ── rls: the business (Layer 1) predicate for this record ──────────────── const rlsLayer = layers.find((l) => l.layer === 'rls'); if (rlsLayer) { @@ -694,11 +735,26 @@ async function applyRecordAttribution( // ── decision.record: bottom line + decisive layer ──────────────────────── const tenantExcluded = tenantRecord.outcome === 'excluded'; const rlsExcluded = rlsLayer?.record?.outcome === 'excluded'; + // [#4647] The bypass admits any row of this object for this principal — the + // row-level meaning of the `vama_bypass` layer's own claim. On the WRITE + // branch the gate (`canEdit`/`canDelete`) is still the authority when it is + // available, because the gate is what the request will actually hit and it + // now consults this same bypass; the disjunction below only carries the + // bypass where no gate answered (a deployment without plugin-sharing) and on + // the read branch, whose row filter the bypass short-circuits identically. + const vamaAdmitsRow = vamaEffective && recordExists; const businessRowAdmits = isRead - ? owd.effect !== 'private' || ownerIsMe || sharingOutcome === 'admitted' + ? owd.effect !== 'private' || ownerIsMe || sharingOutcome === 'admitted' || vamaAdmitsRow : canEdit !== undefined ? canEdit - : owd.effect === 'public' || ownerIsMe || sharingOutcome === 'admitted'; + : owd.effect === 'public' || ownerIsMe || sharingOutcome === 'admitted' || vamaAdmitsRow; + + // [#4647] Was the bypass DECISIVE? Only where the baseline and the concrete + // shares would otherwise have excluded the row — an owner or a shared-to + // principal is admitted with or without it, and reporting `vama_bypass` there + // would over-credit the grant. + const baselineAdmitsRow = isRead ? owd.effect !== 'private' : owd.effect === 'public'; + const bypassWasDecisive = vamaAdmitsRow && !baselineAdmitsRow && !ownerIsMe && !anyShareAdmits; let visible: boolean; let decidedBy: NonNullable['decidedBy']; @@ -710,15 +766,17 @@ async function applyRecordAttribution( else if (!businessRowAdmits) { visible = false; decidedBy = owd.effect === 'private' ? 'sharing' : 'owd_baseline'; } else { visible = true; - decidedBy = (owd.effect === 'private' && !ownerIsMe && sharingOutcome === 'admitted') - ? 'sharing' - : layer1 != null - ? 'rls' - : owd.effect === 'private' && ownerIsMe - ? 'owd_baseline' - : layer0 != null - ? 'tenant_isolation' - : 'object_crud'; + decidedBy = bypassWasDecisive + ? 'vama_bypass' + : (owd.effect === 'private' && !ownerIsMe && sharingOutcome === 'admitted') + ? 'sharing' + : layer1 != null + ? 'rls' + : owd.effect === 'private' && ownerIsMe + ? 'owd_baseline' + : layer0 != null + ? 'tenant_isolation' + : 'object_crud'; } return { @@ -955,14 +1013,21 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput }); // ── 8. vama_bypass ───────────────────────────────────────────────────── + // [#4647] Resolved through `PermissionEvaluator.superuserBypassSets` — the + // ONE bypass predicate. `ISecurityService.hasWriteBypass` folds through the + // same function, and that is what plugin-sharing's `canEdit`/`canDelete` + // (hence the `sys_attachment` `canEdit(parent)` gate) consult on the write + // path. explain and enforcement ask the same question of the same code, so + // they can no longer answer it differently for one (principal, record, + // operation) triple. + // + // The bit is OPERATION-scoped: a write asks for `modifyAllRecords` exactly as + // the write gate does, because "View All Data" is a read power and must not + // widen a write. Reading either bit here (the pre-#4647 behaviour) would have + // re-created the same contradiction one bit down. + const vamaBit = superuserBypassBitForOperation(dataOp); const vamaOf = (list: PermissionSet[]): string[] => - list - .filter((s: any) => { - const objects = s?.objects ?? {}; - const entry = objects[object] ?? objects['*']; - return entry && (entry.viewAllRecords === true || entry.modifyAllRecords === true); - }) - .map((s: any) => String(s.name ?? '?')); + deps.evaluator.superuserBypassSets(object, list, { isPrivate: secMeta.isPrivate, bit: vamaBit }); const agentVama = vamaOf(sets); const delegatorVama = delegatorSets ? vamaOf(delegatorSets) : null; // [ADR-0090 D10] The bypass only survives the intersection when BOTH sides @@ -971,16 +1036,28 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput // belt-and-braces at evaluation time). const vamaEffective = agentVama.length > 0 && (delegatorVama === null || delegatorVama.length > 0); const vamaSets = agentVama; + // [#4647] A write question whose answer is "no bypass" still owes the admin + // WHICH bit is missing: holding View All Data and being refused an update is + // exactly the case this layer is read for. + const viewOnlySets = vamaBit === 'modify' && agentVama.length === 0 + ? deps.evaluator.superuserBypassSets(object, sets, { isPrivate: secMeta.isPrivate, bit: 'view' }) + : []; layers.push({ layer: 'vama_bypass', verdict: vamaEffective ? 'widens' : 'not_applicable', detail: vamaEffective ? `View/Modify All Data bypass held via [${vamaSets.join(', ')}]` + (delegatorVama ? ` AND by the delegator [${delegatorVama.join(', ')}]` : '') + - ` — ownership and sharing checks are skipped.` + ` — ownership and sharing checks are skipped` + + (vamaBit === 'modify' + ? ` (Modify All Data: the write path consults this SAME bypass, #4647).` + : `.`) : agentVama.length > 0 && delegatorVama !== null && delegatorVama.length === 0 ? `Agent holds View/Modify All Data via [${agentVama.join(', ')}] but the DELEGATOR does not — D10 intersection strips the bypass.` - : 'No View/Modify All Data bypass.', + : viewOnlySets.length > 0 + ? `View All Data held via [${viewOnlySets.join(', ')}] does NOT bypass ownership for ${operation} — ` + + `a write bypass requires Modify All Data (modifyAllRecords), so ownership and sharing still decide (#4647).` + : 'No View/Modify All Data bypass.', contributors: vamaEffective ? vamaSets.map((n) => ({ kind: 'permission_set' as const, name: n, via: viaOf(n) })) : [], }); @@ -1028,6 +1105,7 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput if (input.recordId) { const out = await applyRecordAttribution({ deps, object, recordId: input.recordId, engineOp: dataOp, context, sets, layers, owd, capsDeny, crudAllowed, + vamaEffective, vamaSets, }); recordVerdict = out.record; posture = out.posture; @@ -1050,6 +1128,13 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput // rows through the same filter, so omitting it would hide the very // narrowing that explains a short export. ...(operation === 'read' || operation === 'export' ? { readFilter: readFilter ?? null } : {}), + // [#4647] `allowed` answers the OBJECT question and `record` the ROW one; + // what they may never do is contradict each other about the same row. The + // pre-#4647 payload could carry `allowed: true` beside + // `record: { visible: false, decidedBy: 'sharing' }` for a Modify All Data + // holder — the row verdict denying what the bypass layer above it said was + // skipped. They agree now because the row verdict comes from the write gate + // that consults the same bypass `allowed`'s RLS composition short-circuits. ...(recordVerdict ? { record: recordVerdict } : {}), }; return decision; diff --git a/packages/plugins/plugin-security/src/permission-evaluator.ts b/packages/plugins/plugin-security/src/permission-evaluator.ts index 0773d9e563..49f809b817 100644 --- a/packages/plugins/plugin-security/src/permission-evaluator.ts +++ b/packages/plugins/plugin-security/src/permission-evaluator.ts @@ -55,6 +55,33 @@ const MODIFY_ALL_WRITE_KEYS = new Set([ /** CRUD operation class an object-level `requiredPermissions` map keys on. */ export type CrudBucket = 'read' | 'create' | 'update' | 'delete'; +/** + * [#4647] Which super-user ("View/Modify All Data") bit answers a bypass + * question: + * + * - `view` → the READ bypass: `viewAllRecords` OR `modifyAllRecords` + * (Modify All Data implies View All Data). + * - `modify` → the WRITE bypass: `modifyAllRecords` ONLY. "View All Data" is + * a read power and must never widen a write — the whole point of + * shipping the two bits separately. + */ +export type SuperuserBypassBit = 'view' | 'modify'; + +/** + * [#4647] The bypass bit that governs an ObjectQL operation, DERIVED from + * {@link crudBucketForOperation} so a future operation added to the CRUD map is + * classified automatically instead of being silently treated as a read. + * + * `export` is the one op with no CRUD bit of its own; it is a bulk READ + * (`export ⊆ list`, #3544), so it asks for the view bit. Everything the CRUD + * map does not classify as a read asks for the stronger `modify` bit — the + * fail-closed direction for an unknown operation. + */ +export function superuserBypassBitForOperation(operation: string): SuperuserBypassBit { + if (operation === 'export') return 'view'; + return crudBucketForOperation(operation) === 'read' ? 'view' : 'modify'; +} + /** * [ADR-0066 ⑤] Map a raw ObjectQL operation to the CRUD class a per-operation * `requiredPermissions` map is keyed on, DERIVED from `OPERATION_TO_PERMISSION` @@ -242,6 +269,48 @@ export class PermissionEvaluator { return out; } + /** + * [ADR-0066 D2 / ① — #4647] **THE** "View/Modify All Data bypass held?" + * predicate. Returns the NAMES of the resolved sets that carry the requested + * bit for `objectName` (empty array = not held), honouring the private + * posture (see {@link resolveObjectPermission}). + * + * This is deliberately the ONE function every consumer folds through, because + * the bypass used to be decided in two places that disagreed (#4647): the + * explain engine's `vama_bypass` layer answered "bypass held — ownership and + * sharing are skipped" from its own inline read of `objects[name] ?? ['*']`, + * while the write path never consulted the bypass at all — so a Modify All + * Data holder was told `allowed: true` by `security/explain` and handed a 403 + * by `PATCH /data/…` for the same (principal, record, operation) triple. + * Both sides now resolve the bypass HERE: + * + * - explain → `explain-engine.ts` §8 `vama_bypass` + * - writes → {@link hasSuperuserWriteBypass} → `ISecurityService.hasWriteBypass` + * → plugin-sharing `SharingService.canEdit` / `canDelete` + * (and through `canEdit`, the `sys_attachment` parent gate) + * + * Returning the set names rather than a boolean is what keeps the two halves + * honest: the layer's `contributors` attribution and the enforcement decision + * are the same list, so a report that names a granting set cannot coexist + * with a gate that found none. + */ + superuserBypassSets( + objectName: string, + permissionSets: PermissionSet[], + opts: { isPrivate?: boolean; bit: SuperuserBypassBit }, + ): string[] { + const out: string[] = []; + for (const ps of permissionSets) { + const op = resolveObjectPermission(ps, objectName, opts.isPrivate ?? false); + if (!op) continue; + const held = opts.bit === 'modify' + ? Boolean(op.modifyAllRecords) + : Boolean(op.viewAllRecords || op.modifyAllRecords); + if (held) out.push(String((ps as { name?: unknown }).name ?? '?')); + } + return out; + } + /** * [ADR-0066 D2 / ①] Does any resolved set grant the super-user READ bypass * (`viewAllRecords`/`modifyAllRecords`, the "View All Data" power) for the @@ -254,24 +323,22 @@ export class PermissionEvaluator { permissionSets: PermissionSet[], opts: { isPrivate?: boolean } = {}, ): boolean { - for (const ps of permissionSets) { - const op = resolveObjectPermission(ps, objectName, opts.isPrivate ?? false); - if (op && (op.viewAllRecords || op.modifyAllRecords)) return true; - } - return false; + return this.superuserBypassSets(objectName, permissionSets, { ...opts, bit: 'view' }).length > 0; } - /** [ADR-0066 D2 / ①] Super-user WRITE bypass (`modifyAllRecords`) for the object. */ + /** + * [ADR-0066 D2 / ①] Super-user WRITE bypass (`modifyAllRecords`) for the + * object — "Modify All Data": an admin may edit any record regardless of + * ownership (#1883's Salesforce reference frame, re-affirmed for the write + * path in #4647). Same predicate the explain engine reports, so the two can + * never answer differently. + */ hasSuperuserWriteBypass( objectName: string, permissionSets: PermissionSet[], opts: { isPrivate?: boolean } = {}, ): boolean { - for (const ps of permissionSets) { - const op = resolveObjectPermission(ps, objectName, opts.isPrivate ?? false); - if (op && op.modifyAllRecords) return true; - } - return false; + return this.superuserBypassSets(objectName, permissionSets, { ...opts, bit: 'modify' }).length > 0; } /** diff --git a/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts b/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts new file mode 100644 index 0000000000..40a7003df6 --- /dev/null +++ b/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts @@ -0,0 +1,366 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#4647] `security/explain` and the DATA WRITE PATH must answer one +// (principal, record, operation) triple the same way. +// +// The reported contradiction: a **View/Modify All Data holder**, a +// `sharingModel: 'private'` object, and a record whose platform ownership +// column is NULL (system-context seeds routinely produce those — the seed +// loader disables `owner_id` injection). `POST /security/explain` answered +// `allowed: true` with a `vama_bypass` layer stating "ownership and sharing +// checks are skipped", while `PATCH /data/…` answered `403 FORBIDDEN`; filling +// `owner_id` in made the same PATCH succeed, proving the write path was running +// a record-level ownership check the bypass layer claimed had been skipped. +// `sys_attachment`'s `canEdit(parent)` gate agreed with PATCH, not with explain. +// +// Maintainer ruling (2026-08-04, option A): Modify All Data means what it says +// — an admin edits any record regardless of ownership (#1883's Salesforce +// reference frame) — so the WRITE PATH was the side missing the bypass, and +// both sides must resolve it through ONE predicate. +// +// This test wires the REAL objects on both sides — SecurityPlugin's registered +// `security` service (whose `hasWriteBypass` is what plugin-sharing probes), +// the real `SharingService`, the real security + sharing middleware chain the +// engine runs for a by-id write, and the real `explain` entry point — over one +// in-memory engine, and asserts the two paths agree. A regression on either +// side breaks it, which is the only property worth pinning here: not "explain +// says X", but "explain and the write it explains say the same thing". +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { SharingService, buildSharingMiddleware } from '@objectstack/plugin-sharing'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin } from './security-plugin.js'; +import { PermissionEvaluator, superuserBypassBitForOperation } from './permission-evaluator.js'; + +// ── the repro's metadata ─────────────────────────────────────────────────── + +/** The reported object: user-owned, `private` OWD, `private` access posture. */ +const CONTRACT_SCHEMA = { + name: 'crm_contract', + sharingModel: 'private', + access: { default: 'private' }, + fields: { + id: { name: 'id' }, + name: { name: 'name' }, + signed_by: { name: 'signed_by' }, + owner_id: { name: 'owner_id' }, + organization_id: { name: 'organization_id' }, + }, +}; + +/** Platform admin: Modify All Data (and View All Data, which Modify implies). */ +const ADMIN_FULL_ACCESS: PermissionSet = PermissionSetSchema.parse({ + name: 'admin_full_access', + objects: { + '*': { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + viewAllRecords: true, modifyAllRecords: true, + }, + }, +}); + +/** + * The bit-separation control: View All Data WITHOUT Modify All Data, holding + * the ordinary edit/delete CRUD bits so the OBJECT-level gate passes and the + * ROW-level decision is the only thing under test. A read-all auditor must not + * inherit a write bypass — the widening this issue lands has to be exactly + * Modify-scoped. + */ +const COMPLIANCE_AUDITOR: PermissionSet = PermissionSetSchema.parse({ + name: 'compliance_auditor', + objects: { + '*': { allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true }, + }, +}); + +/** The ordinary member — no bypass of any kind, explicit per-object CRUD. */ +const MEMBER_DEFAULT: PermissionSet = PermissionSetSchema.parse({ + name: 'member_default', + objects: { crm_contract: { allowRead: true, allowEdit: true, allowDelete: true } }, +}); + +const PERMISSION_SETS = [ADMIN_FULL_ACCESS, COMPLIANCE_AUDITOR, MEMBER_DEFAULT]; + +/** The record at the heart of the report: NULL `owner_id` (no owner at all). */ +const OWNERLESS = { id: 'rec_ownerless', name: 'Ownerless', owner_id: null, organization_id: 'org1' }; +/** Control: an owned record, owned by somebody ELSE. */ +const OWNED_BY_OTHER = { id: 'rec_owned', name: 'Owned', owner_id: 'u_other', organization_id: 'org1' }; + +// ── in-memory engine (the shape both plugins consume) ────────────────────── + +function makeEngine() { + const tables: Record = { + crm_contract: [{ ...OWNERLESS }, { ...OWNED_BY_OTHER }], + sys_record_share: [], + }; + const matches = (row: any, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + if (Array.isArray(filter.$or)) return filter.$or.some((f: any) => matches(row, f)); + if (Array.isArray(filter.$and)) return filter.$and.every((f: any) => matches(row, f)); + for (const [k, v] of Object.entries(filter)) { + if (k === '$or' || k === '$and') continue; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + if (row[k] !== v) return false; + } + return true; + }; + const middlewares: any[] = []; + return { + _tables: tables, + _middlewares: middlewares, + registerMiddleware: (mw: any) => middlewares.push(mw), + getSchema: (name: string) => (name === 'crm_contract' ? CONTRACT_SCHEMA : undefined), + async find(object: string, options: any = {}) { + const rows = (tables[object] ??= []); + return rows.filter((r) => matches(r, options.filter ?? options.where)).slice(0, options.limit ?? 1000); + }, + async findOne(object: string, options: any = {}) { + const rows = await this.find(object, { ...options, limit: 1 }); + return rows[0] ?? null; + }, + async insert(object: string, data: any) { + (tables[object] ??= []).push({ ...data }); + return data; + }, + }; +} + +// ── the stack: real SecurityPlugin + real SharingService, one engine ─────── + +interface Stack { + security: any; + sharing: SharingService; + /** Run a by-id write through the REAL middleware chain (security → sharing). */ + write: ( + operation: 'update' | 'delete', + recordId: string, + context: any, + ) => Promise<{ ok: true } | { ok: false; code?: string; message: string }>; +} + +async function makeStack(): Promise { + const engine = makeEngine(); + const metadata = { + get: async (_type: string, name: string) => (name === 'crm_contract' ? CONTRACT_SCHEMA : null), + list: async () => PERMISSION_SETS, + }; + // Late-bound on BOTH sides, exactly as the kernel wires them: the sharing + // service probes `security`, and the explain engine resolves `sharing`. + let security: any; + let sharing: SharingService; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata, + get sharing() { return sharing; }, + }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, impl: any) => { + if (name === 'security') security = impl; + }, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx); + await plugin.start(ctx); + if (!security) throw new Error('SecurityPlugin did not register the security service'); + + sharing = new SharingService({ engine: engine as any, securityService: () => security }); + const sharingMw = buildSharingMiddleware(sharing, ctx.logger); + const securityMw = engine._middlewares[0]; + + return { + security, + sharing, + async write(operation, recordId, context) { + const opCtx: any = { + object: 'crm_contract', + operation, + context: { ...context }, + ...(operation === 'update' + ? { data: { id: recordId, signed_by: 'x' } } + : { options: { id: recordId } }), + }; + let reached = false; + try { + await securityMw(opCtx, async () => { + await sharingMw(opCtx, async () => { reached = true; }); + }); + } catch (e: any) { + return { ok: false, code: e?.code, message: String(e?.message ?? e) }; + } + return reached ? { ok: true } : { ok: false, message: 'middleware swallowed the write' }; + }, + }; +} + +/** The execution context shape `resolveAuthzContext` hands the middleware. */ +const ctxFor = (userId: string, ...permissions: string[]) => ({ + userId, tenantId: 'org1', positions: [], permissions, +}); +const ADMIN_CTX = ctxFor('u_admin', 'admin_full_access'); +const AUDITOR_CTX = ctxFor('u_auditor', 'compliance_auditor'); +const MEMBER_CTX = ctxFor('u_member', 'member_default'); + +const explainOf = async (stack: Stack, context: any, operation: string, recordId = OWNERLESS.id) => + stack.security.explain({ object: 'crm_contract', operation, recordId }, context); + +const layerOf = (decision: any, layer: string) => + (decision.layers ?? []).find((l: any) => l.layer === layer); + +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#4647] VAMA holder + private OWD + ownerless record — explain vs. the write path', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('UPDATE: explain and the data write path both ADMIT, and explain credits the bypass', async () => { + const decision = await explainOf(stack, ADMIN_CTX, 'update'); + const write = await stack.write('update', OWNERLESS.id, ADMIN_CTX); + + // The two answers that used to be opposite. + expect(decision.allowed, 'top-level verdict').toBe(true); + expect(decision.record, 'record verdict agrees with the top level').toMatchObject({ + recordId: OWNERLESS.id, + visible: true, + decidedBy: 'vama_bypass', + }); + expect(write, 'PATCH-equivalent through the real middleware chain').toEqual({ ok: true }); + + // …and the layer that claimed the bypass now shows it at row granularity. + const vama = layerOf(decision, 'vama_bypass'); + expect(vama.verdict).toBe('widens'); + expect(vama.contributors.map((c: any) => c.name)).toContain('admin_full_access'); + expect(vama.record.outcome).toBe('admitted'); + // No longer "no ownership and no share grants write" next to allowed:true. + expect(layerOf(decision, 'sharing').record.outcome).toBe('admitted'); + expect(layerOf(decision, 'sharing').record.detail).toContain('Modify All Data bypass'); + }); + + it('DELETE: same triple, same convergence (canDelete has no share branch to hide behind)', async () => { + const decision = await explainOf(stack, ADMIN_CTX, 'delete'); + const write = await stack.write('delete', OWNERLESS.id, ADMIN_CTX); + expect(decision.allowed).toBe(true); + expect(decision.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' }); + expect(write).toEqual({ ok: true }); + }); + + it("the sys_attachment canEdit(parent) gate converges too — it calls the SAME gate", async () => { + // The exact call `attachment-access-hooks.ts` makes before allowing an + // attach: `sharing.canEdit(parent_object, parent_id, callerContext(ctx))`. + const attachmentGate = await stack.sharing.canEdit('crm_contract', OWNERLESS.id, { + userId: ADMIN_CTX.userId, + tenantId: ADMIN_CTX.tenantId, + positions: ADMIN_CTX.positions, + permissions: ADMIN_CTX.permissions, + } as any); + const decision = await explainOf(stack, ADMIN_CTX, 'update'); + expect(attachmentGate, 'ATTACHMENT_PARENT_ACCESS no longer fires for a Modify All holder').toBe(true); + expect(attachmentGate).toBe(decision.record.visible); + }); + + it('an OWNED record was already consistent and stays so (no behaviour drift)', async () => { + const decision = await explainOf(stack, ADMIN_CTX, 'update', OWNED_BY_OTHER.id); + const write = await stack.write('update', OWNED_BY_OTHER.id, ADMIN_CTX); + expect(decision.record!.visible).toBe(true); + expect(write).toEqual({ ok: true }); + }); + + // ── negative control 1: no bypass at all ──────────────────────────────── + it('a NON-VAMA member stays excluded on BOTH paths', async () => { + const decision = await explainOf(stack, MEMBER_CTX, 'update'); + const write = await stack.write('update', OWNERLESS.id, MEMBER_CTX); + const attachmentGate = await stack.sharing.canEdit('crm_contract', OWNERLESS.id, MEMBER_CTX as any); + + expect(decision.record).toMatchObject({ visible: false, decidedBy: 'sharing' }); + expect(write.ok).toBe(false); + expect((write as any).code).toBe('FORBIDDEN'); + expect(attachmentGate).toBe(false); + expect(layerOf(decision, 'vama_bypass').verdict).toBe('not_applicable'); + // The record verdict — not the object-level `allowed` — is what the write + // path answers, and they agree. + expect(decision.record!.visible).toBe(write.ok); + }); + + // ── negative control 2: the widening is exactly Modify-scoped ─────────── + it('View All Data WITHOUT Modify All Data does NOT grant write, on either path', async () => { + const decision = await explainOf(stack, AUDITOR_CTX, 'update'); + const write = await stack.write('update', OWNERLESS.id, AUDITOR_CTX); + const attachmentGate = await stack.sharing.canEdit('crm_contract', OWNERLESS.id, AUDITOR_CTX as any); + + expect(decision.record!.visible).toBe(false); + expect(write.ok).toBe(false); + expect(attachmentGate).toBe(false); + // …and the layer explains WHICH bit is missing rather than going silent. + const vama = layerOf(decision, 'vama_bypass'); + expect(vama.verdict).toBe('not_applicable'); + expect(vama.detail).toContain('View All Data'); + expect(vama.detail).toContain('Modify All Data'); + expect(vama.contributors).toEqual([]); + }); + + it('the same auditor DOES bypass on READ — the two bits are separate powers', async () => { + const decision = await explainOf(stack, AUDITOR_CTX, 'read'); + const vama = layerOf(decision, 'vama_bypass'); + expect(vama.verdict).toBe('widens'); + expect(vama.contributors.map((c: any) => c.name)).toContain('compliance_auditor'); + expect(decision.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' }); + }); + + it('a deployment WITHOUT the security probe degrades to owner-only (fail closed)', async () => { + // ADR-0111 D2: the probe is structural on purpose — no plugin-security, no + // bypass. The gate must not fall open just because it cannot ask. + const engine = makeEngine(); + const bare = new SharingService({ engine: engine as any }); + expect(await bare.canEdit('crm_contract', OWNERLESS.id, ADMIN_CTX as any)).toBe(false); + expect(await bare.canDelete('crm_contract', OWNERLESS.id, ADMIN_CTX as any)).toBe(false); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#4647] PermissionEvaluator.superuserBypassSets — the one predicate', () => { + const evaluator = new PermissionEvaluator(); + + it('the modify bit is required for a WRITE bypass; the view bit is not enough', () => { + const opts = { isPrivate: true }; + expect(evaluator.superuserBypassSets('crm_contract', [COMPLIANCE_AUDITOR], { ...opts, bit: 'view' })) + .toEqual(['compliance_auditor']); + expect(evaluator.superuserBypassSets('crm_contract', [COMPLIANCE_AUDITOR], { ...opts, bit: 'modify' })) + .toEqual([]); + expect(evaluator.superuserBypassSets('crm_contract', [ADMIN_FULL_ACCESS], { ...opts, bit: 'modify' })) + .toEqual(['admin_full_access']); + // Modify implies View. + expect(evaluator.superuserBypassSets('crm_contract', [ADMIN_FULL_ACCESS], { ...opts, bit: 'view' })) + .toEqual(['admin_full_access']); + expect(evaluator.superuserBypassSets('crm_contract', [MEMBER_DEFAULT], { ...opts, bit: 'view' })).toEqual([]); + }); + + it('the boolean helpers are the same predicate, so explain and enforcement cannot drift', () => { + const sets = [COMPLIANCE_AUDITOR]; + expect(evaluator.hasSuperuserReadBypass('crm_contract', sets, { isPrivate: true })).toBe(true); + expect(evaluator.hasSuperuserWriteBypass('crm_contract', sets, { isPrivate: true })).toBe(false); + expect(evaluator.hasSuperuserWriteBypass('crm_contract', [ADMIN_FULL_ACCESS], { isPrivate: true })).toBe(true); + }); + + it('maps every operation onto the bit that governs it (export reads, writes modify)', () => { + expect(superuserBypassBitForOperation('find')).toBe('view'); + expect(superuserBypassBitForOperation('findOne')).toBe('view'); + expect(superuserBypassBitForOperation('count')).toBe('view'); + expect(superuserBypassBitForOperation('export')).toBe('view'); + expect(superuserBypassBitForOperation('update')).toBe('modify'); + expect(superuserBypassBitForOperation('delete')).toBe('modify'); + expect(superuserBypassBitForOperation('insert')).toBe('modify'); + expect(superuserBypassBitForOperation('transfer')).toBe('modify'); + expect(superuserBypassBitForOperation('purge')).toBe('modify'); + // An unmapped operation asks for the STRONGER bit (fail closed). + expect(superuserBypassBitForOperation('some_future_op')).toBe('modify'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 80a649597b..7e4be30c0d 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -302,6 +302,91 @@ describe('SharingService.canEdit', () => { }); }); +// ───────────────────────────────────────────────────────────────────── +// [#4647] Modify All Data on the WRITE gates. +// +// The reported contradiction: `security/explain` answered `allowed: true` for a +// Modify-All holder against an OWNERLESS row of a `private` object ("ownership +// and sharing checks are skipped"), while the write path returned 403 — because +// these gates never asked about the bypass, and the `__writeScope === 'org'` +// proxy they leaned on is checked only AFTER `matchesOwnerScope` has already +// refused a NULL `owner_id`. Ruling (2026-08-04): Modify All Data means an +// admin edits any record regardless of ownership, so the gate asks — through +// the SAME `hasWriteBypass` predicate explain reports. +// ───────────────────────────────────────────────────────────────────── + +describe('[#4647] SharingService write gates consult Modify All Data', () => { + const OWNERLESS_ID = 'a_orphan'; + let engine: ReturnType; + /** Counts probe calls so "asked LAST, never on the fast path" is a fact, not a hope. */ + let probeCalls: number; + + const withBypass = (held: boolean) => { + probeCalls = 0; + return new SharingService({ + engine, + securityService: () => ({ + hasWriteBypass: async () => { probeCalls++; return held; }, + }), + }); + }; + + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + probeCalls = 0; + engine._tables.account = [ + // The seed-shaped row: the platform ownership column is NULL. + { id: OWNERLESS_ID, name: 'Seeded', owner_id: null }, + { id: 'a1', name: 'Acme', owner_id: 'alice' }, + ]; + }); + + it('canEdit admits a Modify All holder on an OWNERLESS private record', async () => { + expect(await withBypass(true).canEdit('account', OWNERLESS_ID, { userId: 'admin' })).toBe(true); + }); + + it('canDelete admits a Modify All holder on an OWNERLESS private record', async () => { + expect(await withBypass(true).canDelete('account', OWNERLESS_ID, { userId: 'admin' })).toBe(true); + }); + + it("a principal WITHOUT the modify bit stays excluded (View All Data is not a write bypass)", async () => { + const svc = withBypass(false); // what hasWriteBypass answers for a view-only set + expect(await svc.canEdit('account', OWNERLESS_ID, { userId: 'auditor' })).toBe(false); + expect(await svc.canDelete('account', OWNERLESS_ID, { userId: 'auditor' })).toBe(false); + }); + + it('a throwing probe fails CLOSED — a broken security service never widens a write', async () => { + const svc = new SharingService({ + engine, + securityService: () => ({ hasWriteBypass: async () => { throw new Error('boom'); } }), + }); + expect(await svc.canEdit('account', OWNERLESS_ID, { userId: 'admin' })).toBe(false); + expect(await svc.canDelete('account', OWNERLESS_ID, { userId: 'admin' })).toBe(false); + }); + + it('no security service at all (no plugin-security) degrades to owner-only', async () => { + const svc = new SharingService({ engine }); + expect(await svc.canEdit('account', OWNERLESS_ID, { userId: 'admin' })).toBe(false); + expect(await svc.canDelete('account', OWNERLESS_ID, { userId: 'admin' })).toBe(false); + }); + + it('the probe is asked LAST — ownership and an edit share never pay for it', async () => { + const svc = withBypass(true); + expect(await svc.canEdit('account', 'a1', { userId: 'alice' })).toBe(true); // owner + expect(probeCalls, 'owner fast-path does not probe').toBe(0); + + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); + expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); // share + expect(probeCalls, 'share branch does not probe either').toBe(0); + + expect(await svc.canEdit('account', OWNERLESS_ID, { userId: 'admin' })).toBe(true); + expect(probeCalls, 'only the otherwise-denied path probes').toBe(1); + }); +}); + describe('[ADR-0111 D3] SharingService.canDelete — the verb boundary', () => { let engine: ReturnType; let svc: SharingService; diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 5daffcc5e9..6ff9b29e9d 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -296,10 +296,47 @@ export class SharingService implements ISharingService { return owners.includes(String(owner)); } + /** + * [#4647] Does the caller hold **Modify All Data** (`modifyAllRecords`) on + * `object`? Probed through the late-bound security service, which answers + * from `PermissionEvaluator.superuserBypassSets` — the SAME predicate the + * explain engine's `vama_bypass` layer reports. That shared function is the + * whole point: before #4647 explain answered "bypass held, ownership and + * sharing are skipped" while this side never asked at all, so a Modify All + * Data holder was told `allowed: true` by `security/explain` and handed a + * 403 by `PATCH /data/…` on the very same record. + * + * Consulted only AFTER ownership and shares have failed, so the ordinary + * write costs no extra resolution. + * + * **Fails CLOSED** (ADR-0111 D2): no security service (a deployment without + * `@objectstack/plugin-security`), a throwing probe, a principal-less or + * on-behalf-of context → `false`, i.e. owner-only as before. + */ + private async hasModifyAllBypass( + object: string, + context: SharingExecutionContext, + ): Promise { + const probe = this.securityService?.(); + if (!probe || typeof probe.hasWriteBypass !== 'function') return false; + try { + return (await probe.hasWriteBypass(object, context)) === true; + } catch { + return false; + } + } + /** * Return `true` if the caller may UPDATE `(object, recordId)`: ownership - * (widened by write DEPTH) OR an explicit write-level share. Always `true` - * for system context, public objects, and objects without an owner field. + * (widened by write DEPTH), an explicit write-level share, or — [#4647] — + * the `modifyAllRecords` super-user bypass. Always `true` for system context, + * public objects, and objects without an owner field. + * + * The bypass branch is what makes "Modify All Data" mean what it says + * (an admin edits any record regardless of ownership — #1883's Salesforce + * reference frame) on rows the DEPTH fast-path cannot reach: an OWNERLESS + * row (`owner_id` NULL, which system-context seeds routinely produce) matches + * no owner set at any depth, so ownership alone refused it. */ async canEdit( object: string, @@ -331,19 +368,27 @@ export class SharingService implements ISharingService { limit: 1, context: SYSTEM_CTX, }); - return Array.isArray(editGrants) && editGrants.length > 0; + if (Array.isArray(editGrants) && editGrants.length > 0) return true; + + // 3) [#4647] Modify All Data — the explicit bypass, asked LAST and answered + // by the same predicate `security/explain` reports. + return this.hasModifyAllBypass(object, context); } /** * [ADR-0111 D3] Return `true` if the caller may DELETE `(object, recordId)`. * * Deliberately NARROWER than {@link canEdit}: ownership (widened by write - * DEPTH) or the `modifyAllRecords` super-user bypass — which reaches this - * gate as `__writeScope === 'org'`, set by plugin-security's evaluator — and - * NOTHING ELSE. An `edit` (or legacy `full`) share opens update but not - * delete: sharing widens rows, never verbs. Always `true` for system - * context, public objects, and objects without an owner field, matching - * {@link canEdit}. + * DEPTH) or the `modifyAllRecords` super-user bypass — and NOTHING ELSE. An + * `edit` (or legacy `full`) share opens update but not delete: sharing widens + * rows, never verbs. Always `true` for system context, public objects, and + * objects without an owner field, matching {@link canEdit}. + * + * [#4647] The bypass is now asked EXPLICITLY (`hasWriteBypass`) instead of + * only riding in as `__writeScope === 'org'`. The scope proxy was silently + * partial: `matchesOwnerScope` refuses an OWNERLESS row before it ever looks + * at the scope, so a Modify All Data holder could not delete a row with a + * NULL `owner_id` — while `security/explain` said the bypass applied. */ async canDelete( object: string, @@ -358,9 +403,12 @@ export class SharingService implements ISharingService { if (!hasOwnerField(schema)) return true; if (!context.userId) return false; - // Ownership / write DEPTH / Modify All (as `__writeScope === 'org'`) only — - // no share branch. This is the whole difference from canEdit. - return this.matchesOwnerScope(object, recordId, context); + // Ownership / write DEPTH only — no share branch. This is the whole + // difference from canEdit. + if (await this.matchesOwnerScope(object, recordId, context)) return true; + + // [#4647] Modify All Data — the same explicit bypass canEdit consults. + return this.hasModifyAllBypass(object, context); } /** @@ -400,17 +448,12 @@ export class SharingService implements ISharingService { return false; } - const probe = this.securityService?.(); - // Modify All Data — the EXPLICIT bypass only (ADR-0111 D1/D2; never the // effective write scope, whose unmatched-object case fails open to 'org'). - try { - if (probe && typeof probe.hasWriteBypass === 'function') { - if ((await probe.hasWriteBypass(object, context)) === true) return true; - } - } catch { - /* fall through */ - } + // [#4647] Shared with `canEdit`/`canDelete` so the three gates cannot drift. + if (await this.hasModifyAllBypass(object, context)) return true; + + const probe = this.securityService?.(); // [ADR-0111 D1 DEPTH] Hierarchy-manager authority: a caller whose effective // WRITE scope on this object is a HIERARCHY scope may manage shares on a diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d69e1aaec7..9f527c649a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1581,6 +1581,9 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/plugin-sharing': + specifier: workspace:* + version: link:../plugin-sharing '@types/node': specifier: ^26.1.2 version: 26.1.2