Skip to content

Commit c1dcacd

Browse files
authored
fix(sharing)!: give the share-management surface the authorization layer it never had (ADR-0111 P0, #3902) (#3965)
canManageShares (owner / Modify All via fail-closed hasWriteBypass / system) enforced in the service; revoke symmetric + scope-validated + manual-only; listShares management-gated; sys_record_share reads self-scoped; /sharing/rules gated on the new manage_sharing capability; no inert grants (recipientType=user, posture-validated, source-keyed upsert). Verified by the #3902 Mallory reproduction.
1 parent ffb003c commit c1dcacd

17 files changed

Lines changed: 905 additions & 41 deletions
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/plugin-sharing": minor
4+
"@objectstack/plugin-security": minor
5+
"@objectstack/rest": minor
6+
---
7+
8+
fix(sharing)!: the share-management surface gains the authorization layer it never had (ADR-0111 P0, #3902)
9+
10+
Record sharing shipped as a data layer with no authorization of its own: every
11+
`/data/:object/:id/shares` and `/sharing/rules` route authenticated the caller
12+
and then ran the service under `SYSTEM_CTX` — any signed-in user could revoke
13+
anyone's share, enumerate who-can-see-what, write self-grants, and define /
14+
evaluate org-wide sharing rules. ADR-0111's P0 rulings land here:
15+
16+
- **D1/D2**`ISharingService.canManageShares(object, recordId, context)`:
17+
system, the record's owner, or a holder of Modify All Data (probed via the
18+
new fail-closed `ISecurityService.hasWriteBypass`). Enforced in the SERVICE,
19+
so every caller is covered; without plugin-security it fails closed to
20+
owner-only.
21+
- **D4**`revoke` is symmetric with grant, validates the share belongs to the
22+
URL's record (`NOT_FOUND` on mismatch), and refuses non-`manual` rows
23+
(`CONFLICT` — a rule-materialised grant would be resurrected by the next
24+
reconcile).
25+
- **D5**`listShares` is management-gated (invisible record → `NOT_FOUND`,
26+
visible-but-not-manager → `PERMISSION_DENIED`), and the open
27+
`/data/sys_record_share` read surface is self-scoped: non-admin callers see
28+
only rows naming them as recipient or grantor.
29+
- **D6** — the whole `/sharing/rules` surface (list/create/get/delete/evaluate)
30+
requires the new **`manage_sharing`** capability (D9; seeded into
31+
`admin_full_access`, `manage_platform_settings` honoured as the legacy
32+
equivalent), enforced in `SharingRuleService`.
33+
- **D7** — no inert grants: `recipientType` is narrowed to `user` (the only
34+
type any gate enforces), grants on objects the sharing gates never consult
35+
(public model, no `owner_id`, bypass, `controlled_by_parent`) fail with
36+
`SHARING_NOT_ENABLED` (422), and the manual upsert keys on
37+
`(object, record, recipient, source)` so manual and rule rows coexist.
38+
39+
**Breaking** for callers that relied on the missing gate: unauthorized share
40+
management now fails with 403/404/409/422 instead of silently succeeding, and
41+
`ISharingService.revoke` gained an optional `scope` parameter. The verb
42+
boundary (edit ≠ delete, ADR-0111 D3) is NOT in this change — it lands as the
43+
separate P1.

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`, `grant`/`revoke`, `listShares` |
19+
| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `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: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,34 @@ 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.canManageShares(object: string, recordId: string, context: SharingExecutionContext): Promise<boolean>
1617
services.sharing.grant(input: GrantShareInput, context: SharingExecutionContext): Promise<RecordShare>
17-
services.sharing.revoke(shareId: string, context: SharingExecutionContext): Promise<void>
18+
services.sharing.revoke(shareId: string, context: SharingExecutionContext, scope?: { object: string; recordId: string }): Promise<void>
1819
services.sharing.listShares(object: string, recordId: string, context: SharingExecutionContext): Promise<RecordShare[]>
1920
```
2021

22+
## Management authority (ADR-0111)
23+
24+
`grant` / `revoke` / `listShares` are **management operations**, enforced in the
25+
service for every non-system caller: the caller must hold `canManageShares` on
26+
the record — its owner, a holder of Modify All Data on the object, or system
27+
context. A deployment without `@objectstack/plugin-security` fails closed to
28+
owner-only. Pass `{ isSystem: true }` only from platform-internal machinery.
29+
2130
## Returns
2231

2332
- `buildReadFilter`: `null` means unrestricted read; otherwise returns an engine filter
24-
- `canEdit`: boolean decision
33+
- `canEdit` / `canManageShares`: boolean decisions (they return `false` rather than throwing)
2534
- `grant`/`listShares`: normalized `RecordShare` rows
2635

2736
## Typical Errors
2837

2938
- `FORBIDDEN` (403) — a write denied by the `canEdit` gate. Thrown by the sharing engine middleware; `canEdit` itself returns `false` rather than throwing.
30-
- `VALIDATION_FAILED``grant`/`revoke` called without a required field (`object`, `recordId`, `recipientId`, or `shareId`). `revoke` is otherwise a no-op when the share id is not found.
39+
- `VALIDATION_FAILED` (400) — `grant`/`revoke` called without a required field (`object`, `recordId`, `recipientId`, or `shareId`), or `grant` with a non-`user` `recipientType` (only `user` rows are enforced by the gates; group/position recipients are delivered via sharing rules).
40+
- `PERMISSION_DENIED` (403) — the caller does not hold `canManageShares` on the record (ADR-0111 D1).
41+
- `NOT_FOUND` (404) — the record is missing **or not visible to the caller** (indistinguishable by design), or a `revoke` share id does not exist / does not belong to the `scope` record.
42+
- `CONFLICT` (409) — `revoke` on a rule-materialised share (`source != 'manual'`); the next rule reconciliation would silently re-grant it. Deactivate or edit the sharing rule instead.
43+
- `SHARING_NOT_ENABLED` (422) — `grant` on an object the sharing gates never consult (public sharing model, no `owner_id` field, a bypass object, or `controlled_by_parent`).
3144

3245
## Example
3346

content/docs/permissions/sharing-rules.mdx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,18 @@ compiler. A condition the compiler cannot lower is **skipped and logged —
149149
never seeded as a permissive match-all** (ADR-0049): a bad condition
150150
under-shares rather than over-shares.
151151

152+
### Rule administration requires `manage_sharing` (ADR-0111 D6)
153+
154+
A sharing rule is an **org-wide grant generator**, so the whole programmatic
155+
surface`GET`/`POST` `{basePath}/sharing/rules`, `GET`/`DELETE`
156+
`{basePath}/sharing/rules/:idOrName`, and `POST …/:idOrName/evaluate`requires
157+
the **`manage_sharing`** capability (seeded into `admin_full_access`;
158+
`manage_platform_settings` is honoured as the legacy equivalent). The gate is
159+
enforced in the service itself, not just at the route, so every caller is
160+
covered; an unauthorized call fails with `403 PERMISSION_DENIED`. Boot
161+
seeding, lifecycle hooks, and backfills run as system context and are
162+
unaffected.
163+
152164
### There is no "share every record" rule
153165

154166
The predicate is **mandatory on every authoring path**, whether you declare

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**: Proposed (2026-07-29)
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.
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/objects/default-permission-sets.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ const baseDefaultPermissionSets: PermissionSet[] = [
121121
'manage_users',
122122
'manage_metadata',
123123
'manage_platform_settings',
124+
// [ADR-0111 D9] Sharing administration — gates the sharing-rule surface
125+
// and (in the DEPTH extension) non-owner share management.
126+
'manage_sharing',
124127
'setup.access',
125128
'setup.write',
126129
'studio.access',

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,28 @@ export class SecurityPlugin implements Plugin {
660660
// reaches the middleware as a plain `find` and `allowExport` would never
661661
// be consulted — the REST export route asks HERE before it streams.
662662
canExport: (object: string, context?: any) => this.canExport(object, context),
663+
// [ADR-0111 D2] Super-user WRITE bypass probe — the management-authority
664+
// primitive behind `ISharingService.canManageShares`. Explicit
665+
// `modifyAllRecords` only (NOT the effective write scope, whose
666+
// unmatched-object case fails open to 'org'); fails CLOSED on
667+
// resolution errors, principal-less contexts, and on-behalf-of
668+
// contexts (no D10 delegator intersection on this path).
669+
hasWriteBypass: async (object: string, context?: any): Promise<boolean> => {
670+
if (context?.isSystem) return true;
671+
if (!context?.userId) return false;
672+
if (context?.onBehalfOf?.userId) return false;
673+
try {
674+
const meta = await this.getObjectSecurityMeta(object);
675+
const sets = await this.resolvePermissionSetsForContext(context);
676+
return this.permissionEvaluator.hasSuperuserWriteBypass(object, sets, { isPrivate: meta.isPrivate });
677+
} catch (e) {
678+
this.logger.warn?.(
679+
`[security] hasWriteBypass failed for object '${object}' (user ${context?.userId ?? 'unknown'}) — denying (fail-closed)`,
680+
e instanceof Error ? e : new Error(String(e)),
681+
);
682+
return false;
683+
}
684+
},
663685
// [ADR-0046 §6.7] Effective permission-set NAMES for a caller — the
664686
// primitive the REST read layer needs to evaluate a permission-set-
665687
// gated book/doc audience ({ permissionSet: '…' }). Same resolution
@@ -2179,7 +2201,13 @@ export class SecurityPlugin implements Plugin {
21792201
? { sharingReadFilter: (o: string, c: any) => sharing.buildReadFilter(o, c) }
21802202
: {}),
21812203
...(sharing && typeof sharing.listShares === 'function'
2182-
? { listRecordShares: (o: string, rid: string, c: any) => sharing.listShares(o, rid, c) }
2204+
// [ADR-0111 D5] listShares is now management-gated in the sharing
2205+
// service, but explain's own caller authorization already ran
2206+
// (explaining ANOTHER user requires `manage_users` — D12). Read the
2207+
// stored rows under system context so the record story keeps its
2208+
// share attribution when the EXPLAINED principal isn't a share
2209+
// manager — the exact behaviour this binding had before the gate.
2210+
? { listRecordShares: (o: string, rid: string) => sharing.listShares(o, rid, { isSystem: true }) }
21832211
: {}),
21842212
...(sharing && typeof sharing.canEdit === 'function'
21852213
? { canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c) }

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,13 @@ export class SharingServicePlugin implements Plugin {
390390
try { return ctx.getService<any>('hierarchy-scope-resolver'); }
391391
catch { return null; }
392392
},
393+
// [ADR-0111 D1/D2] Late-bound security probe for canManageShares'
394+
// Modify-All path. Absent (no plugin-security) → owner-only, fail
395+
// closed — a degraded security stack never widens sharing authority.
396+
securityService: () => {
397+
try { return ctx.getService<any>('security'); }
398+
catch { return null; }
399+
},
393400
});
394401
ctx.registerService('sharing', this.service);
395402

@@ -588,6 +595,29 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar
588595

589596
// READS — AND the visibility filter into the AST.
590597
if (op === 'find' || op === 'findOne' || op === 'count' || op === 'aggregate') {
598+
// [ADR-0111 D5] `sys_record_share` sits on the sharing BYPASS list (the
599+
// enforcement queries must not recurse through their own gate), which
600+
// used to leave its read surface wide open — any authenticated caller
601+
// could enumerate every share row via `/data/sys_record_share` ("who can
602+
// see what", plus the share ids the revoke gate protects). Non-system
603+
// callers without sharing-admin capability are scoped to rows that NAME
604+
// them (as recipient or grantor); principal-less callers see nothing.
605+
// The Setup admin views hold `manage_sharing` (seeded into
606+
// `admin_full_access`; `manage_platform_settings` honoured as the legacy
607+
// gate those pages used) and keep the tenant-wide list.
608+
if (ctx.object === 'sys_record_share' && !exec?.isSystem) {
609+
const caps: string[] = Array.isArray(exec?.systemPermissions) ? exec.systemPermissions : [];
610+
if (!caps.includes('manage_sharing') && !caps.includes('manage_platform_settings')) {
611+
const selfScope = exec?.userId
612+
? { $or: [{ recipient_id: exec.userId }, { granted_by: exec.userId }] }
613+
: { id: '__deny_all__' };
614+
const ast: any = ctx.ast ?? {};
615+
ast.where = composeAnd(ast.where, selfScope);
616+
ast.filter = composeAnd(ast.filter, selfScope);
617+
ctx.ast = ast;
618+
}
619+
return next();
620+
}
591621
let filter = await service.buildReadFilter(ctx.object, exec ?? {});
592622
// [ADR-0090 D10] Agent/service intersection on the OWD/sharing axis. When
593623
// the principal acts on behalf of a user, the owner-match and record

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,29 @@ export class SharingRuleService implements ISharingRuleService {
7474
this.logger = opts.logger;
7575
}
7676

77+
/**
78+
* [ADR-0111 D6] The sharing-rule surface is tenant-wide sharing
79+
* ADMINISTRATION — a rule is an org-wide grant generator, and `evaluate`
80+
* triggers materialisation, so every verb (list/get included) requires the
81+
* `manage_sharing` capability. Enforced HERE, not at the route, so every
82+
* caller is covered (#3902's widened finding: any signed-in user could
83+
* define a broad-criteria rule naming themself and evaluate it into
84+
* org-wide `sys_record_share` grants). `manage_platform_settings` is
85+
* honoured as the legacy gate the Setup sharing pages used before
86+
* `manage_sharing` existed. System contexts (boot seeding, hooks, backfills,
87+
* the REST-independent plugin machinery) bypass.
88+
*/
89+
private assertCanManageRules(context: SharingExecutionContext): void {
90+
if (context?.isSystem) return;
91+
const caps = Array.isArray(context?.systemPermissions) ? context.systemPermissions : [];
92+
if (caps.includes('manage_sharing') || caps.includes('manage_platform_settings')) return;
93+
throw new Error(
94+
'PERMISSION_DENIED: sharing-rule administration requires the manage_sharing capability (ADR-0111 D6)',
95+
);
96+
}
97+
7798
async defineRule(input: DefineSharingRuleInput, context: SharingExecutionContext): Promise<SharingRuleRow> {
99+
this.assertCanManageRules(context);
78100
if (!input.name) throw new Error('VALIDATION_FAILED: name is required');
79101
if (!input.label) throw new Error('VALIDATION_FAILED: label is required');
80102
if (!input.object) throw new Error('VALIDATION_FAILED: object is required');
@@ -176,6 +198,7 @@ export class SharingRuleService implements ISharingRuleService {
176198
filter: { object?: string; activeOnly?: boolean },
177199
context: SharingExecutionContext,
178200
): Promise<SharingRuleRow[]> {
201+
this.assertCanManageRules(context); // [ADR-0111 D6]
179202
const where: any = {};
180203
if (filter.object) where.object_name = filter.object;
181204
if (filter.activeOnly) where.active = true;
@@ -191,6 +214,7 @@ export class SharingRuleService implements ISharingRuleService {
191214
}
192215

193216
async getRule(idOrName: string, context: SharingExecutionContext): Promise<SharingRuleRow | null> {
217+
this.assertCanManageRules(context); // [ADR-0111 D6]
194218
if (!idOrName) return null;
195219
const orgId = (context as any)?.organizationId ?? (context as any)?.tenantId;
196220
const byId = await this.engine.find('sys_sharing_rule', {
@@ -209,6 +233,7 @@ export class SharingRuleService implements ISharingRuleService {
209233
}
210234

211235
async deleteRule(idOrName: string, context: SharingExecutionContext): Promise<void> {
236+
this.assertCanManageRules(context); // [ADR-0111 D6]
212237
const row = await this.getRule(idOrName, context);
213238
if (!row) return;
214239
// Drop materialised grants first so we don't orphan them.
@@ -223,6 +248,7 @@ export class SharingRuleService implements ISharingRuleService {
223248
}
224249

225250
async evaluateRule(idOrName: string, context: SharingExecutionContext): Promise<SharingRuleEvaluationResult> {
251+
this.assertCanManageRules(context); // [ADR-0111 D6]
226252
const rule = await this.getRule(idOrName, context);
227253
if (!rule) throw new Error('RULE_NOT_FOUND');
228254
if (!rule.active) {

0 commit comments

Comments
 (0)