Skip to content

Commit ad303ed

Browse files
authored
fix(sharing)!: an edit-level share no longer grants delete (ADR-0111 D3, the verb boundary) (#3975)
Split the write gate by verb: canEdit (update, share-honoured) vs canDelete (ownership / write DEPTH / modifyAllRecords only — no share). buildWriteFilter takes a verb so bulk delete scopes to owned rows alone. Middleware routes delete through canDelete with a fail-closed deny log; /security/explain consults canDelete for delete. Verified by the edit-share-cannot-delete suite.
1 parent 78d8ee8 commit ad303ed

11 files changed

Lines changed: 310 additions & 50 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/plugin-sharing": minor
4+
"@objectstack/plugin-security": minor
5+
---
6+
7+
fix(sharing)!: an edit-level share no longer grants delete (ADR-0111 D3, the verb boundary)
8+
9+
`update` and `delete` shared one `canEdit` gate, and `canEdit` accepts an
10+
`edit`-level share — so one "edit" grant silently conferred delete, the
11+
opposite error from the retired `full` level. A share widens *which rows* a
12+
principal reaches, never *which verbs* they may use (Salesforce Read/Write
13+
cannot delete; Dataverse `Delete` is a distinct privilege; Odoo splits
14+
`write`/`unlink`).
15+
16+
- `ISharingService.canDelete(object, recordId, context)` — ownership (widened
17+
by write DEPTH) or the `modifyAllRecords` super-user bypass ONLY; an `edit`
18+
or legacy `full` share does not confer it. `canEdit` is unchanged (the
19+
update gate, share included).
20+
- `SharingService.buildWriteFilter` takes a `verb` parameter: a bulk
21+
`delete({multi:true})` scopes to the owner/DEPTH set alone (no share
22+
widening), while a bulk `update` keeps it.
23+
- The sharing middleware routes `delete` through `canDelete` and logs a
24+
specific fail-closed reason on denial (ADR-0111 D10).
25+
- `/security/explain` consults `canDelete` for a `delete` operation, so the
26+
record-level explanation matches enforcement.
27+
28+
**Breaking**: a caller who could delete a record *only* through an edit-level
29+
share (and holds object-level delete CRUD) can no longer delete it — delete now
30+
requires ownership, write depth, or Modify All Data. No new delete access level
31+
is introduced; a future per-record delete grant would be a capability mask
32+
AND-ed with object CRUD, not a fourth share level.

content/docs/kernel/index.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ The kernel is ObjectStack's runtime: it loads your metadata artifact, hosts plug
1616
| API | Stability | What it does |
1717
| :--- | :--- | :--- |
1818
| [`services.data`](/docs/kernel/runtime-services/data-service) | stable | CRUD and queries with the caller's permission context |
19-
| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `canManageShares`, `grant`/`revoke`, `listShares` |
19+
| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `canDelete`, `canManageShares`, `grant`/`revoke`, `listShares` |
2020
| [`services.email`](/docs/kernel/runtime-services/email-service) | stable | `send`, `sendTemplate` |
2121
| [`services.queue`](/docs/kernel/runtime-services/queue-service) | stable | Background work and queues |
2222
| [`services.settings`](/docs/kernel/runtime-services/settings-service) | stable | App/environment settings |

content/docs/kernel/runtime-services/sharing-service.mdx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ description: Record-level sharing and editability checks.
1313
```ts
1414
services.sharing.buildReadFilter(object: string, context: SharingExecutionContext): Promise<unknown | null>
1515
services.sharing.canEdit(object: string, recordId: string, context: SharingExecutionContext): Promise<boolean>
16+
services.sharing.canDelete(object: string, recordId: string, context: SharingExecutionContext): Promise<boolean>
1617
services.sharing.canManageShares(object: string, recordId: string, context: SharingExecutionContext): Promise<boolean>
1718
services.sharing.grant(input: GrantShareInput, context: SharingExecutionContext): Promise<RecordShare>
1819
services.sharing.revoke(shareId: string, context: SharingExecutionContext, scope?: { object: string; recordId: string }): Promise<void>
@@ -27,10 +28,19 @@ the record — its owner, a holder of Modify All Data on the object, or system
2728
context. A deployment without `@objectstack/plugin-security` fails closed to
2829
owner-only. Pass `{ isSystem: true }` only from platform-internal machinery.
2930

31+
## The verb boundary (ADR-0111 D3)
32+
33+
A share widens *which rows* a principal reaches, never *which verbs* they may
34+
use. `canEdit` (the **update** gate) accepts an `edit`-level share; `canDelete`
35+
(the **delete** gate) does **not** — delete is ownership (widened by write
36+
DEPTH) or the `modifyAllRecords` super-user bypass only. Delete is not a share
37+
level and never will be; a future per-record delete grant would be a capability
38+
mask AND-ed with object CRUD, not a fourth `access_level`.
39+
3040
## Returns
3141

3242
- `buildReadFilter`: `null` means unrestricted read; otherwise returns an engine filter
33-
- `canEdit` / `canManageShares`: boolean decisions (they return `false` rather than throwing)
43+
- `canEdit` / `canDelete` / `canManageShares`: boolean decisions (they return `false` rather than throwing)
3444
- `grant`/`listShares`: normalized `RecordShare` rows
3545

3646
## Typical Errors

content/docs/protocol/objectql/security.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ sharedWith:
308308

309309
> **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).
310310

311-
> `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.
311+
> `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.
312312

313313
### Public Share Links
314314

docs/adr/0111-record-share-management-authority-and-verb-boundary.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# ADR-0111: Record-share management authority and the verb boundary — sharing needs "who may manage a share" and "which verbs a level grants"
22

3-
**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.
3+
**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.
44
**Deciders**: ObjectStack Protocol Architects
55
**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)
66
**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`)

packages/plugins/plugin-security/src/explain-engine.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,15 @@ export interface ExplainEngineDeps {
202202
recordId: string,
203203
context: any,
204204
) => Promise<Array<{ id?: string; recipient_type?: string; recipient_id?: string; access_level?: 'read' | 'edit' | 'full'; source?: string; source_id?: string }>>;
205-
/** The sharing service's per-record write gate (`canEdit`) — the by-construction verdict for write operations. */
205+
/** The sharing service's per-record UPDATE gate (`canEdit`) — the by-construction verdict for update operations. */
206206
canEditRecord?: (object: string, recordId: string, context: any) => Promise<boolean>;
207+
/**
208+
* [ADR-0111 D3] The sharing service's per-record DELETE gate (`canDelete`) —
209+
* the by-construction verdict for a delete operation. Narrower than
210+
* `canEditRecord`: an edit-level share opens update but not delete, so the
211+
* explanation for a `delete` must consult this rather than the update gate.
212+
*/
213+
canDeleteRecord?: (object: string, recordId: string, context: any) => Promise<boolean>;
207214
}
208215

209216
export interface ExplainInput {
@@ -609,9 +616,12 @@ async function applyRecordAttribution(
609616
? await deps.sharingReadFilter(object, context).catch(() => null)
610617
: undefined;
611618
const sharingMatches = sharingFilter === undefined ? undefined : matches(sharingFilter);
612-
// Write ops: the by-construction verdict is the sharing service's own canEdit.
613-
const canEdit = !isRead && deps.canEditRecord && recordExists
614-
? await deps.canEditRecord(object, recordId, context).catch(() => undefined)
619+
// Write ops: the by-construction verdict is the sharing service's own gate.
620+
// [ADR-0111 D3] delete has its own narrower gate (an edit share does not
621+
// confer delete), so a `delete` explanation consults canDelete, not canEdit.
622+
const writeGate = engineOp === 'delete' ? deps.canDeleteRecord : deps.canEditRecord;
623+
const canEdit = !isRead && writeGate && recordExists
624+
? await writeGate(object, recordId, context).catch(() => undefined)
615625
: undefined;
616626
const anyShareAdmits = shareRules.some((r) => r.effect === 'admits');
617627
let sharingOutcome: ExplainRecordAttribution['outcome'];

packages/plugins/plugin-security/src/security-plugin.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2212,6 +2212,11 @@ export class SecurityPlugin implements Plugin {
22122212
...(sharing && typeof sharing.canEdit === 'function'
22132213
? { canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c) }
22142214
: {}),
2215+
// [ADR-0111 D3] The narrower delete gate — an edit share opens update
2216+
// but not delete, so a delete explanation must consult this.
2217+
...(sharing && typeof sharing.canDelete === 'function'
2218+
? { canDeleteRecord: (o: string, rid: string, c: any) => sharing.canDelete(o, rid, c) }
2219+
: {}),
22152220
},
22162221
{ object, operation, context: targetContext, recordId },
22172222
);

packages/plugins/plugin-sharing/src/sharing-plugin.ts

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ export class SharingServicePlugin implements Plugin {
421421
if (this.options.enforce === false) {
422422
ctx.logger.info('SharingServicePlugin: enforcement disabled (enforce=false) — share-link service still registered');
423423
} else {
424-
const mw = buildSharingMiddleware(this.service);
424+
const mw = buildSharingMiddleware(this.service, ctx.logger as any);
425425
if (typeof engine.registerMiddleware === 'function') {
426426
engine.registerMiddleware(mw, { object: '*' });
427427
ctx.logger.info('SharingServicePlugin: enforcement middleware installed');
@@ -586,9 +586,13 @@ export class SharingServicePlugin implements Plugin {
586586
/**
587587
* Build the engine middleware that injects read filters and gates
588588
* write operations. Exported so it can be unit-tested without booting
589-
* a kernel.
589+
* a kernel. `log` is optional — the [ADR-0111 D10] delete-denial breadcrumb
590+
* is best-effort and absent in unit tests.
590591
*/
591-
export function buildSharingMiddleware(service: SharingService): EngineMiddleware {
592+
export function buildSharingMiddleware(
593+
service: SharingService,
594+
log?: { warn?: (msg: string, meta?: any) => void },
595+
): EngineMiddleware {
592596
return async function sharingMiddleware(ctx: OperationContext, next: () => Promise<void>) {
593597
const op = ctx.operation;
594598
const exec = ctx.context as any;
@@ -644,24 +648,40 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar
644648
return next();
645649
}
646650

647-
// WRITES — gate on canEdit for update / delete.
651+
// WRITES — gate on the per-VERB check. [ADR-0111 D3] update and delete no
652+
// longer share a gate: `canEdit` accepts an edit-level share, `canDelete`
653+
// does not (a share widens which rows a principal reaches, never which
654+
// verbs). The middleware picks the gate by `op`.
648655
if (op === 'update' || op === 'delete') {
656+
const verb: 'update' | 'delete' = op;
657+
const gate = (o: string, id: string, c: any) =>
658+
verb === 'delete' ? service.canDelete(o, id, c) : service.canEdit(o, id, c);
649659
const data: any = ctx.data;
650660
const options: any = ctx.options;
651661
const id = inferTargetId(data, options);
652662
if (id != null) {
653-
let ok = await service.canEdit(ctx.object, String(id), exec ?? {});
654-
// [ADR-0090 D10] The delegator must ALSO be able to edit the row — an
655-
// on-behalf-of write may only touch rows the delegator could touch.
663+
let ok = await gate(ctx.object, String(id), exec ?? {});
664+
// [ADR-0090 D10] The delegator must ALSO be able to perform the write —
665+
// an on-behalf-of write may only touch rows the delegator could touch.
656666
if (ok && exec?.onBehalfOf?.userId) {
657-
ok = await service.canEdit(ctx.object, String(id), {
667+
ok = await gate(ctx.object, String(id), {
658668
...exec,
659669
userId: exec.onBehalfOf.userId,
660670
onBehalfOf: undefined,
661671
__writeScope: exec.__delegatorWriteScope,
662672
});
663673
}
664674
if (!ok) {
675+
// [ADR-0111 D10] A fail-closed delete denial gets a specific,
676+
// greppable reason so the "edit-share does not grant delete"
677+
// tightening is diagnosable rather than a mystery 403.
678+
if (verb === 'delete') {
679+
log?.warn?.(
680+
`[sharing] delete denied on ${ctx.object} ${id}: an edit-level share does not grant delete; ` +
681+
`delete requires ownership, write depth, or Modify All Data (ADR-0111 D3)`,
682+
{ object: ctx.object, recordId: String(id), userId: exec?.userId },
683+
);
684+
}
665685
const err: any = new Error(
666686
`FORBIDDEN: insufficient privileges to ${op} ${ctx.object} ${id}`,
667687
);
@@ -672,21 +692,20 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar
672692
return next();
673693
}
674694

675-
// Bulk (multi) write — no single id to canEdit-gate (#2982). AND the
676-
// editable-rows filter into the AST so the update/delete only touches
677-
// rows the caller may edit, exactly as the read path scopes finds. The
678-
// engine honours ast.where operation-agnostically (same seam the RLS
679-
// write filter uses). Without this, a `multi:true` write on an
680-
// owner-scoped object would hit every matching row, including peers'.
681-
let writeFilter = await service.buildWriteFilter(ctx.object, exec ?? {});
682-
// [ADR-0090 D10] Intersect the delegator's editable set for on-behalf-of.
695+
// Bulk (multi) write — no single id to gate (#2982). AND the writable-rows
696+
// filter into the AST so the update/delete only touches rows the caller
697+
// may write, exactly as the read path scopes finds. The verb is threaded
698+
// through so a bulk DELETE scopes to owned rows alone (no share widening),
699+
// while a bulk UPDATE keeps the edit-share widening (ADR-0111 D3).
700+
let writeFilter = await service.buildWriteFilter(ctx.object, exec ?? {}, verb);
701+
// [ADR-0090 D10] Intersect the delegator's writable set for on-behalf-of.
683702
if (exec?.onBehalfOf?.userId) {
684703
const delFilter = await service.buildWriteFilter(ctx.object, {
685704
...exec,
686705
userId: exec.onBehalfOf.userId,
687706
onBehalfOf: undefined,
688707
__writeScope: exec.__delegatorWriteScope,
689-
});
708+
}, verb);
690709
writeFilter = composeAnd(writeFilter, delFilter);
691710
}
692711
if (writeFilter) {

0 commit comments

Comments
 (0)