From 425eca9fea1671d44b98af188281ec2b65fb3c4d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:31:17 +0000 Subject: [PATCH] fix(sharing)!: an edit-level share no longer grants delete (ADR-0111 D3, the verb boundary) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update and delete shared one canEdit gate, and canEdit accepts an edit-level share — so one "edit" grant silently conferred delete (the opposite error from the retired `full` level, which promised delete and gave nothing). A share widens which rows a principal reaches, never which verbs they may use. - ISharingService.canDelete: ownership (widened by write DEPTH) or the modifyAllRecords bypass ONLY; an edit/full share does not confer it. canEdit is unchanged (the update gate). - SharingService.buildWriteFilter takes a verb param: a bulk delete scopes to owned rows alone (no share widening), a bulk update keeps it. - The sharing middleware routes delete through canDelete and logs a specific fail-closed reason on denial (D10). - /security/explain consults canDelete for a delete operation, so the record-level explanation matches enforcement. Breaking: a caller who could delete only through an edit share can no longer delete — delete requires ownership, write depth, or Modify All. No new delete level is introduced. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012P3RFCpjjiSQ7WuamSDW7W --- .changeset/adr-0111-verb-boundary-delete.md | 32 +++++ content/docs/kernel/index.mdx | 2 +- .../runtime-services/sharing-service.mdx | 12 +- content/docs/protocol/objectql/security.mdx | 2 +- ...-management-authority-and-verb-boundary.md | 2 +- .../plugin-security/src/explain-engine.ts | 18 ++- .../plugin-security/src/security-plugin.ts | 5 + .../plugin-sharing/src/sharing-plugin.ts | 53 +++++--- .../src/sharing-service.test.ts | 127 +++++++++++++++++- .../plugin-sharing/src/sharing-service.ts | 81 ++++++++--- .../spec/src/contracts/sharing-service.ts | 26 +++- 11 files changed, 310 insertions(+), 50 deletions(-) create mode 100644 .changeset/adr-0111-verb-boundary-delete.md diff --git a/.changeset/adr-0111-verb-boundary-delete.md b/.changeset/adr-0111-verb-boundary-delete.md new file mode 100644 index 0000000000..25335a37bb --- /dev/null +++ b/.changeset/adr-0111-verb-boundary-delete.md @@ -0,0 +1,32 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-sharing": minor +"@objectstack/plugin-security": minor +--- + +fix(sharing)!: an edit-level share no longer grants delete (ADR-0111 D3, the verb boundary) + +`update` and `delete` shared one `canEdit` gate, and `canEdit` accepts an +`edit`-level share — so one "edit" grant silently conferred delete, the +opposite error from the retired `full` level. A share widens *which rows* a +principal reaches, never *which verbs* they may use (Salesforce Read/Write +cannot delete; Dataverse `Delete` is a distinct privilege; Odoo splits +`write`/`unlink`). + +- `ISharingService.canDelete(object, recordId, context)` — ownership (widened + by write DEPTH) or the `modifyAllRecords` super-user bypass ONLY; an `edit` + or legacy `full` share does not confer it. `canEdit` is unchanged (the + update gate, share included). +- `SharingService.buildWriteFilter` takes a `verb` parameter: a bulk + `delete({multi:true})` scopes to the owner/DEPTH set alone (no share + widening), while a bulk `update` keeps it. +- The sharing middleware routes `delete` through `canDelete` and logs a + specific fail-closed reason on denial (ADR-0111 D10). +- `/security/explain` consults `canDelete` for a `delete` operation, so the + record-level explanation matches enforcement. + +**Breaking**: a caller who could delete a record *only* through an edit-level +share (and holds object-level delete CRUD) can no longer delete it — delete now +requires ownership, write depth, or Modify All Data. No new delete access level +is introduced; a future per-record delete grant would be a capability mask +AND-ed with object CRUD, not a fourth share level. diff --git a/content/docs/kernel/index.mdx b/content/docs/kernel/index.mdx index 66697b0073..e09436934f 100644 --- a/content/docs/kernel/index.mdx +++ b/content/docs/kernel/index.mdx @@ -16,7 +16,7 @@ The kernel is ObjectStack's runtime: it loads your metadata artifact, hosts plug | API | Stability | What it does | | :--- | :--- | :--- | | [`services.data`](/docs/kernel/runtime-services/data-service) | stable | CRUD and queries with the caller's permission context | -| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `canManageShares`, `grant`/`revoke`, `listShares` | +| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `canDelete`, `canManageShares`, `grant`/`revoke`, `listShares` | | [`services.email`](/docs/kernel/runtime-services/email-service) | stable | `send`, `sendTemplate` | | [`services.queue`](/docs/kernel/runtime-services/queue-service) | stable | Background work and queues | | [`services.settings`](/docs/kernel/runtime-services/settings-service) | stable | App/environment settings | diff --git a/content/docs/kernel/runtime-services/sharing-service.mdx b/content/docs/kernel/runtime-services/sharing-service.mdx index 51f9dbe069..04b4d24580 100644 --- a/content/docs/kernel/runtime-services/sharing-service.mdx +++ b/content/docs/kernel/runtime-services/sharing-service.mdx @@ -13,6 +13,7 @@ description: Record-level sharing and editability checks. ```ts services.sharing.buildReadFilter(object: string, context: SharingExecutionContext): Promise services.sharing.canEdit(object: string, recordId: string, context: SharingExecutionContext): Promise +services.sharing.canDelete(object: string, recordId: string, context: SharingExecutionContext): Promise services.sharing.canManageShares(object: string, recordId: string, context: SharingExecutionContext): Promise services.sharing.grant(input: GrantShareInput, context: SharingExecutionContext): Promise services.sharing.revoke(shareId: string, context: SharingExecutionContext, scope?: { object: string; recordId: string }): Promise @@ -27,10 +28,19 @@ the record — its owner, a holder of Modify All Data on the object, or system context. A deployment without `@objectstack/plugin-security` fails closed to owner-only. Pass `{ isSystem: true }` only from platform-internal machinery. +## The verb boundary (ADR-0111 D3) + +A share widens *which rows* a principal reaches, never *which verbs* they may +use. `canEdit` (the **update** gate) accepts an `edit`-level share; `canDelete` +(the **delete** gate) does **not** — delete is ownership (widened by write +DEPTH) or the `modifyAllRecords` super-user bypass only. Delete is not a share +level and never will be; a future per-record delete grant would be a capability +mask AND-ed with object CRUD, not a fourth `access_level`. + ## Returns - `buildReadFilter`: `null` means unrestricted read; otherwise returns an engine filter -- `canEdit` / `canManageShares`: boolean decisions (they return `false` rather than throwing) +- `canEdit` / `canDelete` / `canManageShares`: boolean decisions (they return `false` rather than throwing) - `grant`/`listShares`: normalized `RecordShare` rows ## Typical Errors diff --git a/content/docs/protocol/objectql/security.mdx b/content/docs/protocol/objectql/security.mdx index 1fc090d102..ce66c9ff55 100644 --- a/content/docs/protocol/objectql/security.mdx +++ b/content/docs/protocol/objectql/security.mdx @@ -308,7 +308,7 @@ sharedWith: > **Enforcement status.** Criteria rules with `user` / `position` / `unit_and_subordinates` recipients compile and enforce (the CEL condition lowers to a runtime filter that materializes `sys_record_share` grants, ADR-0058 D3). Owner-type rules and `group`/`guest` recipients are `[experimental — not enforced]`: the seed bootstrap skips them (logged) rather than seeding a permissive match-all (ADR-0049). -> `accessLevel` is one of `read` or `edit`. Sharing widens **which rows** a principal reaches, never **which verbs** they may use — delete and transfer come from ownership, the ADR-0057 DEPTH scopes, or admin scope, and are checked by the object-level CRUD gate before sharing is consulted at all. A third level `full` ("Full Access — transfer/share/delete") was authorable through protocol 16 but never granted any of those verbs: both enforcement sites matched `edit`/`full` alike, so it was equivalent to `edit` while telling admins otherwise, and it was removed (#3865, ADR-0078). Stacks still authoring it are rewritten to `edit` at load by the `sharing-rule-access-level-full-to-edit` conversion. +> `accessLevel` is one of `read` or `edit`. Sharing widens **which rows** a principal reaches, never **which verbs** they may use — an `edit` share opens *update*, not *delete*: delete comes from ownership, the ADR-0057 DEPTH scopes, or the `modifyAllRecords` bypass, enforced by the sharing layer's own `canDelete` gate (distinct from the `canEdit` update gate) on top of the object-level CRUD gate (ADR-0111 D3). A third level `full` ("Full Access — transfer/share/delete") was authorable through protocol 16 but never granted any of those verbs: both enforcement sites matched `edit`/`full` alike, so it was equivalent to `edit` while telling admins otherwise, and it was removed (#3865, ADR-0078). Stacks still authoring it are rewritten to `edit` at load by the `sharing-rule-access-level-full-to-edit` conversion. ### Public Share Links diff --git a/docs/adr/0111-record-share-management-authority-and-verb-boundary.md b/docs/adr/0111-record-share-management-authority-and-verb-boundary.md index df17c2013f..53bd444373 100644 --- a/docs/adr/0111-record-share-management-authority-and-verb-boundary.md +++ b/docs/adr/0111-record-share-management-authority-and-verb-boundary.md @@ -1,6 +1,6 @@ # ADR-0111: Record-share management authority and the verb boundary — sharing needs "who may manage a share" and "which verbs a level grants" -**Status**: Accepted (2026-07-30) — **P0 implemented** (D1/D2/D4/D5/D6/D7/D9: `canManageShares` + `hasWriteBypass` in `plugin-sharing/src/sharing-service.ts` / `plugin-security/src/security-plugin.ts`; verified by the #3902 Mallory reproduction in `plugin-sharing/src/sharing-service.test.ts` and the D6 gate suite in `sharing-rule.test.ts`). **D3 (verb boundary) and D8 (share-link rulings) are not yet implemented** — they land as the separate P1 / follow-up PRs this ADR's rollout section names. +**Status**: Accepted (2026-07-30) — **P0 + P1 implemented**. P0 (D1/D2/D4/D5/D6/D7/D9): `canManageShares` + `hasWriteBypass`, verified by the #3902 Mallory reproduction. P1 (D3, the verb boundary): `canDelete` + verb-split `buildWriteFilter` in `plugin-sharing/src/sharing-service.ts`, routed by the middleware and `/security/explain`, verified by the "edit share cannot delete" suite in `plugin-sharing/src/sharing-service.test.ts`. **D8 (share-link rulings) and the DEPTH management extension (D1 D-future) are not yet implemented** — they land as the follow-up PRs this ADR's rollout section names. **Deciders**: ObjectStack Protocol Architects **Builds on**: [ADR-0049](./0049-no-unenforced-security-properties.md) (enforce-or-remove — a security property that parses but enforces nothing is worse than absent), [ADR-0057](./0057-erp-authorization-core-business-units-and-scope-depth.md) (DEPTH scopes + the `sys_record_share` / `sys_sharing_rule` split), [ADR-0066](./0066-unified-authorization-model.md) (unified capability model; `modifyAllRecords` super-user bit), [ADR-0078](./0078-no-silently-inert-metadata.md) (no silently inert metadata — a persisted share level or recipient type that no gate consults is exactly this), [ADR-0090](./0090-permission-model-v2-concept-convergence.md) (D1 secure-default OWD, D4 retired aliases, D10 delegated identity intersection), [ADR-0091](./0091-grant-lifecycle-and-recertification.md) (time-boxed grants — the lifecycle axis this ADR deliberately does not re-open) **Consumers**: `@objectstack/plugin-sharing` (`sharing-service.ts`, `sharing-rule-service.ts`, `share-link-service.ts`, `sharing-plugin.ts`), `@objectstack/plugin-security` (`ISecurityService` — a write-bypass probe), `@objectstack/rest` (`rest-server.ts` sharing / sharing-rule / share-link routes), `@objectstack/spec` (`contracts/sharing-service.ts`, `security/capabilities.ts`) diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index efbfee7509..585d8c91be 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -202,8 +202,15 @@ export interface ExplainEngineDeps { recordId: string, context: any, ) => Promise>; - /** The sharing service's per-record write gate (`canEdit`) — the by-construction verdict for write operations. */ + /** The sharing service's per-record UPDATE gate (`canEdit`) — the by-construction verdict for update operations. */ canEditRecord?: (object: string, recordId: string, context: any) => Promise; + /** + * [ADR-0111 D3] The sharing service's per-record DELETE gate (`canDelete`) — + * the by-construction verdict for a delete operation. Narrower than + * `canEditRecord`: an edit-level share opens update but not delete, so the + * explanation for a `delete` must consult this rather than the update gate. + */ + canDeleteRecord?: (object: string, recordId: string, context: any) => Promise; } export interface ExplainInput { @@ -609,9 +616,12 @@ async function applyRecordAttribution( ? await deps.sharingReadFilter(object, context).catch(() => null) : undefined; const sharingMatches = sharingFilter === undefined ? undefined : matches(sharingFilter); - // Write ops: the by-construction verdict is the sharing service's own canEdit. - const canEdit = !isRead && deps.canEditRecord && recordExists - ? await deps.canEditRecord(object, recordId, context).catch(() => undefined) + // Write ops: the by-construction verdict is the sharing service's own gate. + // [ADR-0111 D3] delete has its own narrower gate (an edit share does not + // confer delete), so a `delete` explanation consults canDelete, not canEdit. + const writeGate = engineOp === 'delete' ? deps.canDeleteRecord : deps.canEditRecord; + const canEdit = !isRead && writeGate && recordExists + ? await writeGate(object, recordId, context).catch(() => undefined) : undefined; const anyShareAdmits = shareRules.some((r) => r.effect === 'admits'); let sharingOutcome: ExplainRecordAttribution['outcome']; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 9c5510477f..584bd3d3c7 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2212,6 +2212,11 @@ export class SecurityPlugin implements Plugin { ...(sharing && typeof sharing.canEdit === 'function' ? { canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c) } : {}), + // [ADR-0111 D3] The narrower delete gate — an edit share opens update + // but not delete, so a delete explanation must consult this. + ...(sharing && typeof sharing.canDelete === 'function' + ? { canDeleteRecord: (o: string, rid: string, c: any) => sharing.canDelete(o, rid, c) } + : {}), }, { object, operation, context: targetContext, recordId }, ); diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index a1101fefd0..731032833d 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -421,7 +421,7 @@ export class SharingServicePlugin implements Plugin { if (this.options.enforce === false) { ctx.logger.info('SharingServicePlugin: enforcement disabled (enforce=false) — share-link service still registered'); } else { - const mw = buildSharingMiddleware(this.service); + const mw = buildSharingMiddleware(this.service, ctx.logger as any); if (typeof engine.registerMiddleware === 'function') { engine.registerMiddleware(mw, { object: '*' }); ctx.logger.info('SharingServicePlugin: enforcement middleware installed'); @@ -586,9 +586,13 @@ export class SharingServicePlugin implements Plugin { /** * Build the engine middleware that injects read filters and gates * write operations. Exported so it can be unit-tested without booting - * a kernel. + * a kernel. `log` is optional — the [ADR-0111 D10] delete-denial breadcrumb + * is best-effort and absent in unit tests. */ -export function buildSharingMiddleware(service: SharingService): EngineMiddleware { +export function buildSharingMiddleware( + service: SharingService, + log?: { warn?: (msg: string, meta?: any) => void }, +): EngineMiddleware { return async function sharingMiddleware(ctx: OperationContext, next: () => Promise) { const op = ctx.operation; const exec = ctx.context as any; @@ -644,17 +648,23 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar return next(); } - // WRITES — gate on canEdit for update / delete. + // WRITES — gate on the per-VERB check. [ADR-0111 D3] update and delete no + // longer share a gate: `canEdit` accepts an edit-level share, `canDelete` + // does not (a share widens which rows a principal reaches, never which + // verbs). The middleware picks the gate by `op`. if (op === 'update' || op === 'delete') { + const verb: 'update' | 'delete' = op; + const gate = (o: string, id: string, c: any) => + verb === 'delete' ? service.canDelete(o, id, c) : service.canEdit(o, id, c); const data: any = ctx.data; const options: any = ctx.options; const id = inferTargetId(data, options); if (id != null) { - let ok = await service.canEdit(ctx.object, String(id), exec ?? {}); - // [ADR-0090 D10] The delegator must ALSO be able to edit the row — an - // on-behalf-of write may only touch rows the delegator could touch. + let ok = await gate(ctx.object, String(id), exec ?? {}); + // [ADR-0090 D10] The delegator must ALSO be able to perform the write — + // an on-behalf-of write may only touch rows the delegator could touch. if (ok && exec?.onBehalfOf?.userId) { - ok = await service.canEdit(ctx.object, String(id), { + ok = await gate(ctx.object, String(id), { ...exec, userId: exec.onBehalfOf.userId, onBehalfOf: undefined, @@ -662,6 +672,16 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar }); } if (!ok) { + // [ADR-0111 D10] A fail-closed delete denial gets a specific, + // greppable reason so the "edit-share does not grant delete" + // tightening is diagnosable rather than a mystery 403. + if (verb === 'delete') { + log?.warn?.( + `[sharing] delete denied on ${ctx.object} ${id}: an edit-level share does not grant delete; ` + + `delete requires ownership, write depth, or Modify All Data (ADR-0111 D3)`, + { object: ctx.object, recordId: String(id), userId: exec?.userId }, + ); + } const err: any = new Error( `FORBIDDEN: insufficient privileges to ${op} ${ctx.object} ${id}`, ); @@ -672,21 +692,20 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar return next(); } - // Bulk (multi) write — no single id to canEdit-gate (#2982). AND the - // editable-rows filter into the AST so the update/delete only touches - // rows the caller may edit, exactly as the read path scopes finds. The - // engine honours ast.where operation-agnostically (same seam the RLS - // write filter uses). Without this, a `multi:true` write on an - // owner-scoped object would hit every matching row, including peers'. - let writeFilter = await service.buildWriteFilter(ctx.object, exec ?? {}); - // [ADR-0090 D10] Intersect the delegator's editable set for on-behalf-of. + // Bulk (multi) write — no single id to gate (#2982). AND the writable-rows + // filter into the AST so the update/delete only touches rows the caller + // may write, exactly as the read path scopes finds. The verb is threaded + // through so a bulk DELETE scopes to owned rows alone (no share widening), + // while a bulk UPDATE keeps the edit-share widening (ADR-0111 D3). + let writeFilter = await service.buildWriteFilter(ctx.object, exec ?? {}, verb); + // [ADR-0090 D10] Intersect the delegator's writable set for on-behalf-of. if (exec?.onBehalfOf?.userId) { const delFilter = await service.buildWriteFilter(ctx.object, { ...exec, userId: exec.onBehalfOf.userId, onBehalfOf: undefined, __writeScope: exec.__delegatorWriteScope, - }); + }, verb); writeFilter = composeAnd(writeFilter, delFilter); } if (writeFilter) { diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 6cea47c7da..ce4cd87a40 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -298,6 +298,63 @@ describe('SharingService.canEdit', () => { }); }); +describe('[ADR-0111 D3] SharingService.canDelete — the verb boundary', () => { + let engine: ReturnType; + let svc: SharingService; + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + lead: LEAD_SCHEMA, + task: EXPLICIT_PUBLIC_SCHEMA, + note: ORPHAN_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + svc = new SharingService({ engine }); + engine._tables.account = [ + { id: 'a1', name: 'Acme', owner_id: 'alice' }, + { id: 'a2', name: 'Beta', owner_id: 'bob' }, + ]; + }); + + it('system context, public objects, and owner-less objects all allow delete (matches canEdit)', async () => { + expect(await svc.canDelete('account', 'a1', { isSystem: true })).toBe(true); + expect(await svc.canDelete('task', 'anything', { userId: 'carol' })).toBe(true); + engine._tables.note = [{ id: 'n1', body: 'x' }]; + expect(await svc.canDelete('note', 'n1', { userId: 'carol' })).toBe(true); + }); + + it('the record owner may delete', async () => { + expect(await svc.canDelete('account', 'a1', { userId: 'alice' })).toBe(true); + }); + + it('a non-owner without ownership may NOT delete', async () => { + expect(await svc.canDelete('account', 'a1', { userId: 'bob' })).toBe(false); + }); + + it('an EDIT share grants canEdit but NOT canDelete', async () => { + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); + expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); + expect(await svc.canDelete('account', 'a1', { userId: 'bob' })).toBe(false); + }); + + it("a legacy 'full' share likewise grants edit but not delete", async () => { + engine._tables.sys_record_share = [{ + id: 'shr_legacy', object_name: 'account', record_id: 'a1', + recipient_type: 'user', recipient_id: 'bob', access_level: 'full', + }]; + expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); + expect(await svc.canDelete('account', 'a1', { userId: 'bob' })).toBe(false); + }); + + it('Modify All (writeScope=org) may delete a record it does not own', async () => { + expect(await svc.canDelete('account', 'a1', { userId: 'admin', __writeScope: 'org' } as any)).toBe(true); + }); + + it('a principal-less context may not delete a private record', async () => { + expect(await svc.canDelete('account', 'a1', {})).toBe(false); + }); +}); + describe('SharingService.grant / listShares / revoke', () => { let engine: ReturnType; let svc: SharingService; @@ -452,18 +509,76 @@ describe('buildSharingMiddleware (engine integration)', () => { expect(nextCalled).toBe(true); }); - it('allows delete after explicit edit grant', async () => { + // [ADR-0111 D3] The verb boundary: an edit-level share opens UPDATE but NOT + // delete. alice holds an edit share on a2 (owned by bob) — update passes, + // delete is refused (delete needs ownership / write depth / Modify All). + it('an edit share allows update but NOT delete (ADR-0111 D3)', async () => { await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'alice', accessLevel: 'edit' }, { isSystem: true }); const mw = buildSharingMiddleware(svc); + + let updateNext = false; + await mw( + { object: 'account', operation: 'update', data: { id: 'a2', name: 'X' }, context: { userId: 'alice' } } as any, + async () => { updateNext = true; }, + ); + expect(updateNext).toBe(true); + + await expect( + mw( + { object: 'account', operation: 'delete', options: { where: { id: 'a2' } }, context: { userId: 'alice' } } as any, + async () => {}, + ), + ).rejects.toMatchObject({ code: 'FORBIDDEN', status: 403 }); + }); + + it('the record owner may delete their own record', async () => { + const mw = buildSharingMiddleware(svc); + let nextCalled = false; + await mw( + { object: 'account', operation: 'delete', options: { where: { id: 'a1' } }, context: { userId: 'alice' } } as any, + async () => { nextCalled = true; }, + ); + expect(nextCalled).toBe(true); + }); + + it('Modify All (writeScope=org) may delete a record it does not own', async () => { + const mw = buildSharingMiddleware(svc); + let nextCalled = false; + await mw( + { object: 'account', operation: 'delete', options: { where: { id: 'a2' } }, context: { userId: 'admin', __writeScope: 'org' } } as any, + async () => { nextCalled = true; }, + ); + expect(nextCalled).toBe(true); + }); + + it('bulk delete scopes to owned rows ALONE — an edit share does not widen it (ADR-0111 D3)', async () => { + await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); + const mw = buildSharingMiddleware(svc); const ctx: any = { object: 'account', operation: 'delete', - options: { where: { id: 'a2' } }, - context: { userId: 'alice' }, + options: { multi: true, where: {} }, + ast: {}, + context: { userId: 'bob' }, }; - let nextCalled = false; - await mw(ctx, async () => { nextCalled = true; }); - expect(nextCalled).toBe(true); + await mw(ctx, async () => {}); + // bob's edit share on a2 widens a bulk UPDATE but NOT a bulk delete. + expect(ctx.ast.where).toEqual({ owner_id: 'bob' }); + }); + + it('bulk update DOES widen by the edit share (contrast with delete)', async () => { + await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'account', + operation: 'update', + options: { multi: true, where: {} }, + ast: {}, + data: { status: 'x' }, + context: { userId: 'bob' }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ $or: [{ owner_id: 'bob' }, { id: { $in: ['a2'] } }] }); }); it('does not block insert', async () => { diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index ea423b4eb9..191d2c976e 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -214,6 +214,7 @@ export class SharingService implements ISharingService { async buildWriteFilter( object: string, context: SharingExecutionContext, + verb: 'update' | 'delete' = 'update', ): Promise { if (this.shouldBypass(object, context)) return null; @@ -234,6 +235,13 @@ export class SharingService implements ISharingService { ? { [OWNER_FIELD]: ownerIds[0] } : { [OWNER_FIELD]: { $in: ownerIds } }; + // [ADR-0111 D3] The verb boundary applied to BULK writes: a share widens + // which rows a principal may *edit*, never which they may *delete*. So a + // `delete({multi:true})` scopes to the owner/DEPTH set ALONE — the shared + // record ids are NOT OR-ed in, exactly as the single-id `canDelete` gate + // drops the share branch. Update keeps the share widening. + if (verb === 'delete') return ownerMatch; + const grants = await this.engine.find('sys_record_share', { where: { object_name: object, @@ -254,9 +262,33 @@ export class SharingService implements ISharingService { } /** - * Return `true` if the caller may edit `(object, recordId)`. Always - * `true` for system context, public objects, and objects without an - * owner field. + * Does the caller own `(object, recordId)` within their write DEPTH? The + * shared ownership fast-path behind both {@link canEdit} and + * {@link canDelete}. Returns `false` when the record has no owner value. + */ + private async matchesOwnerScope( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + const own = await this.engine.find(object, { + where: { id: recordId }, + fields: ['id', OWNER_FIELD], + limit: 1, + context: SYSTEM_CTX, + }); + const owner = Array.isArray(own) && own[0] ? (own[0] as any)[OWNER_FIELD] : undefined; + if (owner == null) return false; + const writeScope = (context as any).__writeScope as ('own' | 'own_and_reports' | 'unit' | 'unit_and_below' | 'org' | undefined); + if (writeScope === 'org') return true; + const owners = await this.resolveOwnerScopeIds(context, writeScope); + return owners.includes(String(owner)); + } + + /** + * 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. */ async canEdit( object: string, @@ -273,19 +305,7 @@ export class SharingService implements ISharingService { if (!context.userId) return false; // 1) Ownership (write DEPTH widens the owner-set) — fast path. - const own = await this.engine.find(object, { - where: { id: recordId }, - fields: ['id', OWNER_FIELD], - limit: 1, - context: SYSTEM_CTX, - }); - const owner = Array.isArray(own) && own[0] ? (own[0] as any)[OWNER_FIELD] : undefined; - if (owner != null) { - const writeScope = (context as any).__writeScope as ('own' | 'own_and_reports' | 'unit' | 'unit_and_below' | 'org' | undefined); - if (writeScope === 'org') return true; - const owners = await this.resolveOwnerScopeIds(context, writeScope); - if (owners.includes(String(owner))) return true; - } + if (await this.matchesOwnerScope(object, recordId, context)) return true; // 2) Explicit write-level share (`edit`, plus not-yet-normalised `full`). const editGrants = await this.engine.find('sys_record_share', { @@ -303,6 +323,35 @@ export class SharingService implements ISharingService { return Array.isArray(editGrants) && editGrants.length > 0; } + /** + * [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}. + */ + async canDelete( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + if (this.shouldBypass(object, context)) return true; + + const schema = this.engine.getSchema?.(object); + if (!schema) return true; + if (effectiveSharingModel(schema) === 'public') return true; + 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); + } + /** * [ADR-0111 D1] May `context` MANAGE shares (grant / revoke / list) on * `(object, recordId)`? System → yes. Record owner → yes. Super-user write diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 6e938aeaf1..3c117845e1 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -111,9 +111,10 @@ export interface ISharingService { ): Promise; /** - * Return `true` when the principal in `context` may modify the - * record `(object, recordId)`. Owner-only for `private` / `read` - * objects; always true for `public` objects. + * Return `true` when the principal in `context` may UPDATE the record + * `(object, recordId)`. Ownership (widened by write DEPTH) OR a write-level + * ({@link ShareAccessLevel} `edit`) share. Always true for system context, + * `public` objects, and objects with no owner field. */ canEdit( object: string, @@ -121,6 +122,25 @@ export interface ISharingService { context: SharingExecutionContext, ): Promise; + /** + * [ADR-0111 D3] Return `true` when the principal in `context` may DELETE the + * record `(object, recordId)`. + * + * The verb boundary: a share widens *which rows* a principal reaches, never + * *which verbs* they may use — so delete is **ownership (widened by write + * DEPTH) or the `modifyAllRecords` super-user bypass ONLY**, and an `edit` + * share does NOT confer it (Salesforce Read/Write cannot delete; Dataverse + * `Delete` is a distinct privilege; Odoo splits `write`/`unlink`). Always + * true for system context, `public` objects, and objects with no owner + * field, matching {@link canEdit}. A per-record delete grant, if ever added, + * is a capability mask AND-ed with object CRUD — not a share level. + */ + canDelete( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise; + /** * [ADR-0111 D1] May the principal in `context` MANAGE shares (grant / revoke * / list) on `(object, recordId)`? True for system context, the record's