diff --git a/.changeset/adr-0111-sharing-authorization-face.md b/.changeset/adr-0111-sharing-authorization-face.md new file mode 100644 index 0000000000..79792bbe98 --- /dev/null +++ b/.changeset/adr-0111-sharing-authorization-face.md @@ -0,0 +1,43 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-sharing": minor +"@objectstack/plugin-security": minor +"@objectstack/rest": minor +--- + +fix(sharing)!: the share-management surface gains the authorization layer it never had (ADR-0111 P0, #3902) + +Record sharing shipped as a data layer with no authorization of its own: every +`/data/:object/:id/shares` and `/sharing/rules` route authenticated the caller +and then ran the service under `SYSTEM_CTX` — any signed-in user could revoke +anyone's share, enumerate who-can-see-what, write self-grants, and define / +evaluate org-wide sharing rules. ADR-0111's P0 rulings land here: + +- **D1/D2** — `ISharingService.canManageShares(object, recordId, context)`: + system, the record's owner, or a holder of Modify All Data (probed via the + new fail-closed `ISecurityService.hasWriteBypass`). Enforced in the SERVICE, + so every caller is covered; without plugin-security it fails closed to + owner-only. +- **D4** — `revoke` is symmetric with grant, validates the share belongs to the + URL's record (`NOT_FOUND` on mismatch), and refuses non-`manual` rows + (`CONFLICT` — a rule-materialised grant would be resurrected by the next + reconcile). +- **D5** — `listShares` is management-gated (invisible record → `NOT_FOUND`, + visible-but-not-manager → `PERMISSION_DENIED`), and the open + `/data/sys_record_share` read surface is self-scoped: non-admin callers see + only rows naming them as recipient or grantor. +- **D6** — the whole `/sharing/rules` surface (list/create/get/delete/evaluate) + requires the new **`manage_sharing`** capability (D9; seeded into + `admin_full_access`, `manage_platform_settings` honoured as the legacy + equivalent), enforced in `SharingRuleService`. +- **D7** — no inert grants: `recipientType` is narrowed to `user` (the only + type any gate enforces), grants on objects the sharing gates never consult + (public model, no `owner_id`, bypass, `controlled_by_parent`) fail with + `SHARING_NOT_ENABLED` (422), and the manual upsert keys on + `(object, record, recipient, source)` so manual and rule rows coexist. + +**Breaking** for callers that relied on the missing gate: unauthorized share +management now fails with 403/404/409/422 instead of silently succeeding, and +`ISharingService.revoke` gained an optional `scope` parameter. The verb +boundary (edit ≠ delete, ADR-0111 D3) is NOT in this change — it lands as the +separate P1. diff --git a/content/docs/kernel/index.mdx b/content/docs/kernel/index.mdx index cfcd09835d..66697b0073 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`, `grant`/`revoke`, `listShares` | +| [`services.sharing`](/docs/kernel/runtime-services/sharing-service) | stable | `buildReadFilter`, `canEdit`, `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 8760007182..51f9dbe069 100644 --- a/content/docs/kernel/runtime-services/sharing-service.mdx +++ b/content/docs/kernel/runtime-services/sharing-service.mdx @@ -13,21 +13,34 @@ 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.canManageShares(object: string, recordId: string, context: SharingExecutionContext): Promise services.sharing.grant(input: GrantShareInput, context: SharingExecutionContext): Promise -services.sharing.revoke(shareId: string, context: SharingExecutionContext): Promise +services.sharing.revoke(shareId: string, context: SharingExecutionContext, scope?: { object: string; recordId: string }): Promise services.sharing.listShares(object: string, recordId: string, context: SharingExecutionContext): Promise ``` +## Management authority (ADR-0111) + +`grant` / `revoke` / `listShares` are **management operations**, enforced in the +service for every non-system caller: the caller must hold `canManageShares` on +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. + ## Returns - `buildReadFilter`: `null` means unrestricted read; otherwise returns an engine filter -- `canEdit`: boolean decision +- `canEdit` / `canManageShares`: boolean decisions (they return `false` rather than throwing) - `grant`/`listShares`: normalized `RecordShare` rows ## Typical Errors - `FORBIDDEN` (403) — a write denied by the `canEdit` gate. Thrown by the sharing engine middleware; `canEdit` itself returns `false` rather than throwing. -- `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. +- `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). +- `PERMISSION_DENIED` (403) — the caller does not hold `canManageShares` on the record (ADR-0111 D1). +- `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. +- `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. +- `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`). ## Example diff --git a/content/docs/permissions/sharing-rules.mdx b/content/docs/permissions/sharing-rules.mdx index 643e04b1a9..b0eb488cf9 100644 --- a/content/docs/permissions/sharing-rules.mdx +++ b/content/docs/permissions/sharing-rules.mdx @@ -149,6 +149,18 @@ compiler. A condition the compiler cannot lower is **skipped and logged — never seeded as a permissive match-all** (ADR-0049): a bad condition under-shares rather than over-shares. +### Rule administration requires `manage_sharing` (ADR-0111 D6) + +A sharing rule is an **org-wide grant generator**, so the whole programmatic +surface — `GET`/`POST` `{basePath}/sharing/rules`, `GET`/`DELETE` +`{basePath}/sharing/rules/:idOrName`, and `POST …/:idOrName/evaluate` — requires +the **`manage_sharing`** capability (seeded into `admin_full_access`; +`manage_platform_settings` is honoured as the legacy equivalent). The gate is +enforced in the service itself, not just at the route, so every caller is +covered; an unauthorized call fails with `403 PERMISSION_DENIED`. Boot +seeding, lifecycle hooks, and backfills run as system context and are +unaffected. + ### There is no "share every record" rule The predicate is **mandatory on every authoring path**, whether you declare 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 5af1a18f88..df17c2013f 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**: Proposed (2026-07-29) +**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. **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/objects/default-permission-sets.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts index d04b7a8daa..fd795d6753 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts @@ -121,6 +121,9 @@ const baseDefaultPermissionSets: PermissionSet[] = [ 'manage_users', 'manage_metadata', 'manage_platform_settings', + // [ADR-0111 D9] Sharing administration — gates the sharing-rule surface + // and (in the DEPTH extension) non-owner share management. + 'manage_sharing', 'setup.access', 'setup.write', 'studio.access', diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index f63eeef8c9..9c5510477f 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -660,6 +660,28 @@ export class SecurityPlugin implements Plugin { // reaches the middleware as a plain `find` and `allowExport` would never // be consulted — the REST export route asks HERE before it streams. canExport: (object: string, context?: any) => this.canExport(object, context), + // [ADR-0111 D2] Super-user WRITE bypass probe — the management-authority + // primitive behind `ISharingService.canManageShares`. Explicit + // `modifyAllRecords` only (NOT the effective write scope, whose + // unmatched-object case fails open to 'org'); fails CLOSED on + // resolution errors, principal-less contexts, and on-behalf-of + // contexts (no D10 delegator intersection on this path). + hasWriteBypass: async (object: string, context?: any): Promise => { + if (context?.isSystem) return true; + if (!context?.userId) return false; + if (context?.onBehalfOf?.userId) return false; + try { + const meta = await this.getObjectSecurityMeta(object); + const sets = await this.resolvePermissionSetsForContext(context); + return this.permissionEvaluator.hasSuperuserWriteBypass(object, sets, { isPrivate: meta.isPrivate }); + } catch (e) { + this.logger.warn?.( + `[security] hasWriteBypass failed for object '${object}' (user ${context?.userId ?? 'unknown'}) — denying (fail-closed)`, + e instanceof Error ? e : new Error(String(e)), + ); + return false; + } + }, // [ADR-0046 §6.7] Effective permission-set NAMES for a caller — the // primitive the REST read layer needs to evaluate a permission-set- // gated book/doc audience ({ permissionSet: '…' }). Same resolution @@ -2179,7 +2201,13 @@ export class SecurityPlugin implements Plugin { ? { sharingReadFilter: (o: string, c: any) => sharing.buildReadFilter(o, c) } : {}), ...(sharing && typeof sharing.listShares === 'function' - ? { listRecordShares: (o: string, rid: string, c: any) => sharing.listShares(o, rid, c) } + // [ADR-0111 D5] listShares is now management-gated in the sharing + // service, but explain's own caller authorization already ran + // (explaining ANOTHER user requires `manage_users` — D12). Read the + // stored rows under system context so the record story keeps its + // share attribution when the EXPLAINED principal isn't a share + // manager — the exact behaviour this binding had before the gate. + ? { listRecordShares: (o: string, rid: string) => sharing.listShares(o, rid, { isSystem: true }) } : {}), ...(sharing && typeof sharing.canEdit === 'function' ? { canEditRecord: (o: string, rid: string, c: any) => sharing.canEdit(o, rid, c) } diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index b7a2ec342d..a1101fefd0 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -390,6 +390,13 @@ export class SharingServicePlugin implements Plugin { try { return ctx.getService('hierarchy-scope-resolver'); } catch { return null; } }, + // [ADR-0111 D1/D2] Late-bound security probe for canManageShares' + // Modify-All path. Absent (no plugin-security) → owner-only, fail + // closed — a degraded security stack never widens sharing authority. + securityService: () => { + try { return ctx.getService('security'); } + catch { return null; } + }, }); ctx.registerService('sharing', this.service); @@ -588,6 +595,29 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar // READS — AND the visibility filter into the AST. if (op === 'find' || op === 'findOne' || op === 'count' || op === 'aggregate') { + // [ADR-0111 D5] `sys_record_share` sits on the sharing BYPASS list (the + // enforcement queries must not recurse through their own gate), which + // used to leave its read surface wide open — any authenticated caller + // could enumerate every share row via `/data/sys_record_share` ("who can + // see what", plus the share ids the revoke gate protects). Non-system + // callers without sharing-admin capability are scoped to rows that NAME + // them (as recipient or grantor); principal-less callers see nothing. + // The Setup admin views hold `manage_sharing` (seeded into + // `admin_full_access`; `manage_platform_settings` honoured as the legacy + // gate those pages used) and keep the tenant-wide list. + if (ctx.object === 'sys_record_share' && !exec?.isSystem) { + const caps: string[] = Array.isArray(exec?.systemPermissions) ? exec.systemPermissions : []; + if (!caps.includes('manage_sharing') && !caps.includes('manage_platform_settings')) { + const selfScope = exec?.userId + ? { $or: [{ recipient_id: exec.userId }, { granted_by: exec.userId }] } + : { id: '__deny_all__' }; + const ast: any = ctx.ast ?? {}; + ast.where = composeAnd(ast.where, selfScope); + ast.filter = composeAnd(ast.filter, selfScope); + ctx.ast = ast; + } + return next(); + } let filter = await service.buildReadFilter(ctx.object, exec ?? {}); // [ADR-0090 D10] Agent/service intersection on the OWD/sharing axis. When // the principal acts on behalf of a user, the owner-match and record diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 5eaed5a767..f3632edbc3 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -74,7 +74,29 @@ export class SharingRuleService implements ISharingRuleService { this.logger = opts.logger; } + /** + * [ADR-0111 D6] The sharing-rule surface is tenant-wide sharing + * ADMINISTRATION — a rule is an org-wide grant generator, and `evaluate` + * triggers materialisation, so every verb (list/get included) requires the + * `manage_sharing` capability. Enforced HERE, not at the route, so every + * caller is covered (#3902's widened finding: any signed-in user could + * define a broad-criteria rule naming themself and evaluate it into + * org-wide `sys_record_share` grants). `manage_platform_settings` is + * honoured as the legacy gate the Setup sharing pages used before + * `manage_sharing` existed. System contexts (boot seeding, hooks, backfills, + * the REST-independent plugin machinery) bypass. + */ + private assertCanManageRules(context: SharingExecutionContext): void { + if (context?.isSystem) return; + const caps = Array.isArray(context?.systemPermissions) ? context.systemPermissions : []; + if (caps.includes('manage_sharing') || caps.includes('manage_platform_settings')) return; + throw new Error( + 'PERMISSION_DENIED: sharing-rule administration requires the manage_sharing capability (ADR-0111 D6)', + ); + } + async defineRule(input: DefineSharingRuleInput, context: SharingExecutionContext): Promise { + this.assertCanManageRules(context); if (!input.name) throw new Error('VALIDATION_FAILED: name is required'); if (!input.label) throw new Error('VALIDATION_FAILED: label is required'); if (!input.object) throw new Error('VALIDATION_FAILED: object is required'); @@ -176,6 +198,7 @@ export class SharingRuleService implements ISharingRuleService { filter: { object?: string; activeOnly?: boolean }, context: SharingExecutionContext, ): Promise { + this.assertCanManageRules(context); // [ADR-0111 D6] const where: any = {}; if (filter.object) where.object_name = filter.object; if (filter.activeOnly) where.active = true; @@ -191,6 +214,7 @@ export class SharingRuleService implements ISharingRuleService { } async getRule(idOrName: string, context: SharingExecutionContext): Promise { + this.assertCanManageRules(context); // [ADR-0111 D6] if (!idOrName) return null; const orgId = (context as any)?.organizationId ?? (context as any)?.tenantId; const byId = await this.engine.find('sys_sharing_rule', { @@ -209,6 +233,7 @@ export class SharingRuleService implements ISharingRuleService { } async deleteRule(idOrName: string, context: SharingExecutionContext): Promise { + this.assertCanManageRules(context); // [ADR-0111 D6] const row = await this.getRule(idOrName, context); if (!row) return; // Drop materialised grants first so we don't orphan them. @@ -223,6 +248,7 @@ export class SharingRuleService implements ISharingRuleService { } async evaluateRule(idOrName: string, context: SharingExecutionContext): Promise { + this.assertCanManageRules(context); // [ADR-0111 D6] const rule = await this.getRule(idOrName, context); if (!rule) throw new Error('RULE_NOT_FOUND'); if (!rule.active) { diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 1aa5c61c57..bc428fb478 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -633,3 +633,62 @@ describe('#3896 — missing criteria never becomes "share every record" (ADR-004 }); }); }); + +// ───────────────────────────────────────────────────────────────────── +// [ADR-0111 D6] Sharing-rule administration requires `manage_sharing`. +// The #3902 widened finding: every /sharing/rules verb ran under +// SYSTEM_CTX with no caller check, so any signed-in user could define a +// broad-criteria rule naming themself and evaluate it into org-wide +// grants. The gate lives in the SERVICE so every caller is covered. +// ───────────────────────────────────────────────────────────────────── + +describe('[ADR-0111 D6] sharing-rule management gate', () => { + let engine: ReturnType; + let rules: SharingRuleService; + const MALLORY = { userId: 'mallory', systemPermissions: [] } as any; + const RULE_ADMIN = { userId: 'admin', systemPermissions: ['manage_sharing'] } as any; + const LEGACY_ADMIN = { userId: 'admin', systemPermissions: ['manage_platform_settings'] } as any; + + const input = { + name: 'self_grant', + label: 'Self grant', + object: 'opportunity', + criteria: { amount: { $gt: 0 } }, + recipientType: 'user' as const, + recipientId: 'mallory', + accessLevel: 'edit' as const, + }; + + beforeEach(() => { + engine = makeEngine(); + engine._tables.opportunity = [{ id: 'opp1', amount: 100, owner_id: 'someone' }]; + rules = new SharingRuleService({ engine: engine as any, sharing: new SharingService({ engine: engine as any }) }); + }); + + it('an ordinary user can neither define, list, get, delete, nor evaluate rules', async () => { + await expect(rules.defineRule(input, MALLORY)).rejects.toThrow(/PERMISSION_DENIED/); + await expect(rules.listRules({}, MALLORY)).rejects.toThrow(/PERMISSION_DENIED/); + await expect(rules.getRule('anything', MALLORY)).rejects.toThrow(/PERMISSION_DENIED/); + await expect(rules.deleteRule('anything', MALLORY)).rejects.toThrow(/PERMISSION_DENIED/); + await expect(rules.evaluateRule('anything', MALLORY)).rejects.toThrow(/PERMISSION_DENIED/); + expect(engine._tables.sys_sharing_rule ?? []).toHaveLength(0); + }); + + it('manage_sharing authorizes the full surface', async () => { + const r = await rules.defineRule(input, RULE_ADMIN); + expect(r.id).toBeTruthy(); + expect((await rules.listRules({}, RULE_ADMIN)).length).toBe(1); + await rules.deleteRule(r.id, RULE_ADMIN); + expect((await rules.listRules({}, RULE_ADMIN)).length).toBe(0); + }); + + it('the legacy manage_platform_settings gate is honoured', async () => { + const r = await rules.defineRule(input, LEGACY_ADMIN); + expect(r.id).toBeTruthy(); + }); + + it('system contexts (boot seeding, hooks, backfills) bypass the gate', async () => { + const r = await rules.defineRule(input, { isSystem: true } as any); + expect(r.id).toBeTruthy(); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index cb7caa117c..6cea47c7da 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -209,8 +209,9 @@ describe('SharingService.buildReadFilter', () => { }); it('returns owner OR shared-record filter when grants exist', async () => { - await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'alice' }, { userId: 'admin' }); - await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'alice', accessLevel: 'edit' }, { userId: 'admin' }); + // Setup grants run as system — grant authorization has its own suite below. + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'alice' }, { isSystem: true }); + await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'alice', accessLevel: 'edit' }, { isSystem: true }); const f: any = await svc.buildReadFilter('account', { userId: 'alice' }); expect(f.$or).toBeDefined(); expect(f.$or[0]).toEqual({ owner_id: 'alice' }); @@ -260,12 +261,12 @@ describe('SharingService.canEdit', () => { }); it('returns false for read-only share', async () => { - await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'read' }, { userId: 'admin' }); + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'read' }, { isSystem: true }); expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(false); }); it('returns true for edit share', async () => { - await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { userId: 'admin' }); + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); }); @@ -303,6 +304,10 @@ describe('SharingService.grant / listShares / revoke', () => { beforeEach(() => { engine = makeFakeEngine({ account: ACCOUNT_SCHEMA, sys_record_share: {} }); svc = new SharingService({ engine }); + // [ADR-0111 D1] admin OWNS a1 — the grants below exercise the legitimate + // owner path through the management gate (unauthorized paths have their + // own suite below). + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'admin' }]; }); it('creates a new grant on first call', async () => { @@ -448,7 +453,7 @@ describe('buildSharingMiddleware (engine integration)', () => { }); it('allows delete after explicit edit grant', async () => { - await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'alice', accessLevel: 'edit' }, { userId: 'admin' }); + await svc.grant({ object: 'account', recordId: 'a2', recipientId: 'alice', accessLevel: 'edit' }, { isSystem: true }); const mw = buildSharingMiddleware(svc); const ctx: any = { object: 'account', @@ -518,7 +523,7 @@ describe('buildSharingMiddleware (engine integration)', () => { }); it('bulk update: edit-shared records widen the editable set', async () => { - await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { userId: 'admin' }); + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { isSystem: true }); const mw = buildSharingMiddleware(svc); const ctx: any = { object: 'account', @@ -578,3 +583,290 @@ describe('buildSharingMiddleware (engine integration)', () => { expect(ctx.ast.where).toBeUndefined(); }); }); + +// ───────────────────────────────────────────────────────────────────── +// [ADR-0111] Share-management authority — the #3902 reproduction. +// mallory: an ordinary signed-in user with no grants of any kind. +// alice: owner of account a1. admin: holds Modify All via the probe. +// ───────────────────────────────────────────────────────────────────── + +describe('[ADR-0111 D1] SharingService.canManageShares', () => { + let engine: ReturnType; + beforeEach(() => { + engine = makeFakeEngine({ account: ACCOUNT_SCHEMA, sys_record_share: {} }); + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'alice' }]; + }); + + it('system context manages everything', async () => { + const svc = new SharingService({ engine }); + expect(await svc.canManageShares('account', 'a1', { isSystem: true })).toBe(true); + }); + + it('the record owner manages their record', async () => { + const svc = new SharingService({ engine }); + expect(await svc.canManageShares('account', 'a1', { userId: 'alice' })).toBe(true); + }); + + it('a non-owner without Modify All does NOT manage (fail closed, no security plugin)', async () => { + const svc = new SharingService({ engine }); + expect(await svc.canManageShares('account', 'a1', { userId: 'mallory' })).toBe(false); + }); + + it('Modify All Data (security probe) manages a record it does not own', async () => { + const svc = new SharingService({ + engine, + securityService: () => ({ hasWriteBypass: async () => true }), + }); + expect(await svc.canManageShares('account', 'a1', { userId: 'admin' })).toBe(true); + }); + + it('a throwing probe fails CLOSED to deny', async () => { + const svc = new SharingService({ + engine, + securityService: () => ({ hasWriteBypass: async () => { throw new Error('boom'); } }), + }); + expect(await svc.canManageShares('account', 'a1', { userId: 'admin' })).toBe(false); + }); + + it('a missing record and a principal-less context both deny', async () => { + const svc = new SharingService({ engine }); + expect(await svc.canManageShares('account', 'nope', { userId: 'alice' })).toBe(false); + expect(await svc.canManageShares('account', 'a1', {})).toBe(false); + }); +}); + +describe('[ADR-0111 D1/D4/D5] the #3902 Mallory reproduction', () => { + let engine: ReturnType; + let svc: SharingService; + beforeEach(() => { + engine = makeFakeEngine({ account: ACCOUNT_SCHEMA, sys_record_share: {} }); + svc = new SharingService({ engine }); + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'alice' }]; + }); + + it('① Mallory cannot revoke a share alice granted (was: 204, silent revoke)', async () => { + const share = await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'colleague', accessLevel: 'edit' }, + { userId: 'alice' }, + ); + await expect( + svc.revoke(share.id, { userId: 'mallory' }, { object: 'account', recordId: 'a1' }), + ).rejects.toThrow(/PERMISSION_DENIED/); + expect(engine._tables.sys_record_share.length).toBe(1); // still there + // alice (owner) still can — symmetric with grant. + await svc.revoke(share.id, { userId: 'alice' }, { object: 'account', recordId: 'a1' }); + expect(engine._tables.sys_record_share.length).toBe(0); + }); + + it('② Mallory cannot enumerate the shares on alice\'s record (was: 200, full list)', async () => { + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'colleague' }, { userId: 'alice' }); + await expect( + svc.listShares('account', 'a1', { userId: 'mallory' }), + ).rejects.toThrow(/PERMISSION_DENIED/); + // The owner keeps the list. + expect((await svc.listShares('account', 'a1', { userId: 'alice' })).length).toBe(1); + }); + + it('③ Mallory cannot grant herself access (was: 201, row persisted with granted_by=mallory)', async () => { + await expect( + svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'mallory', accessLevel: 'edit' }, + { userId: 'mallory' }, + ), + ).rejects.toThrow(/PERMISSION_DENIED/); + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('missing/invisible record reads as NOT_FOUND, not as a permission verdict', async () => { + await expect( + svc.listShares('account', 'ghost', { userId: 'mallory' }), + ).rejects.toThrow(/NOT_FOUND/); + }); +}); + +describe('[ADR-0111 D4] revoke ownership + source validation', () => { + let engine: ReturnType; + let svc: SharingService; + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + lead: LEAD_SCHEMA, + sys_record_share: {}, + }); + svc = new SharingService({ engine }); + engine._tables.account = [{ id: 'a1', owner_id: 'alice' }]; + engine._tables.lead = [{ id: 'l1', owner_id: 'alice' }]; + }); + + it('a share id cannot be revoked through an unrelated record path (scope mismatch → NOT_FOUND)', async () => { + const share = await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob' }, + { userId: 'alice' }, + ); + await expect( + svc.revoke(share.id, { userId: 'alice' }, { object: 'lead', recordId: 'l1' }), + ).rejects.toThrow(/NOT_FOUND/); + expect(engine._tables.sys_record_share.length).toBe(1); + }); + + it('a rule-materialised share refuses manual revoke (CONFLICT — the reconcile would resurrect it)', async () => { + engine._tables.sys_record_share = [{ + id: 'shr_rule', object_name: 'account', record_id: 'a1', + recipient_type: 'user', recipient_id: 'bob', access_level: 'read', + source: 'rule', source_id: 'srule_1', + }]; + await expect( + svc.revoke('shr_rule', { userId: 'alice' }, { object: 'account', recordId: 'a1' }), + ).rejects.toThrow(/CONFLICT/); + // The system path (rule reconciliation) still deletes by id. + await svc.revoke('shr_rule', { isSystem: true }); + expect(engine._tables.sys_record_share.length).toBe(0); + }); + + it('a missing share id is NOT_FOUND for a user, a no-op for system', async () => { + await expect( + svc.revoke('shr_ghost', { userId: 'alice' }, { object: 'account', recordId: 'a1' }), + ).rejects.toThrow(/NOT_FOUND/); + await svc.revoke('shr_ghost', { isSystem: true }); // no throw + }); +}); + +describe('[ADR-0111 D7] no inert grants', () => { + let engine: ReturnType; + let svc: SharingService; + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + whiteboard: CANON_PUBLIC_RW_SCHEMA, + note: ORPHAN_SCHEMA, + detail_item: { + name: 'detail_item', + sharingModel: 'controlled_by_parent', + fields: { id: {}, owner_id: {} }, + }, + sys_record_share: {}, + }); + svc = new SharingService({ engine }); + engine._tables.account = [{ id: 'a1', owner_id: 'alice' }]; + engine._tables.whiteboard = [{ id: 'w1', owner_id: 'alice' }]; + engine._tables.note = [{ id: 'n1' }]; + engine._tables.detail_item = [{ id: 'd1', owner_id: 'alice' }]; + }); + + it('refuses non-user recipient types instead of persisting rows no gate reads', async () => { + for (const recipientType of ['group', 'position', 'unit_and_subordinates', 'guest'] as const) { + await expect( + svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'g1', recipientType: recipientType as any }, + { userId: 'alice' }, + ), + ).rejects.toThrow(/VALIDATION_FAILED/); + } + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('refuses a grant on a public object (no gate would ever consult it)', async () => { + await expect( + svc.grant({ object: 'whiteboard', recordId: 'w1', recipientId: 'bob' }, { userId: 'alice' }), + ).rejects.toThrow(/SHARING_NOT_ENABLED/); + }); + + it('refuses a grant on an owner-less object', async () => { + await expect( + svc.grant({ object: 'note', recordId: 'n1', recipientId: 'bob' }, { userId: 'alice' }), + ).rejects.toThrow(/SHARING_NOT_ENABLED/); + }); + + it('refuses a grant on a controlled_by_parent detail (share the master instead)', async () => { + await expect( + svc.grant({ object: 'detail_item', recordId: 'd1', recipientId: 'bob' }, { userId: 'alice' }), + ).rejects.toThrow(/SHARING_NOT_ENABLED.*master/); + }); + + it('refuses a grant on a bypass object', async () => { + await expect( + svc.grant({ object: 'sys_user', recordId: 'u1', recipientId: 'bob' }, { userId: 'alice' }), + ).rejects.toThrow(/SHARING_NOT_ENABLED/); + }); + + it('a manual grant coexists with a rule-materialised row instead of clobbering it', async () => { + engine._tables.sys_record_share = [{ + id: 'shr_rule', object_name: 'account', record_id: 'a1', + recipient_type: 'user', recipient_id: 'bob', access_level: 'read', + source: 'rule', source_id: 'srule_1', + }]; + const manual = await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, + { userId: 'alice' }, + ); + expect(manual.id).not.toBe('shr_rule'); + expect(engine._tables.sys_record_share.length).toBe(2); + const rule = engine._tables.sys_record_share.find(r => r.id === 'shr_rule'); + expect(rule?.source).toBe('rule'); // untouched + }); +}); + +describe('[ADR-0111 D5] sys_record_share read self-scope (middleware)', () => { + let engine: ReturnType; + let svc: SharingService; + beforeEach(() => { + engine = makeFakeEngine({ account: ACCOUNT_SCHEMA, sys_record_share: {} }); + svc = new SharingService({ engine }); + }); + + const findCtx = (context: any): any => ({ + object: 'sys_record_share', + operation: 'find', + ast: {}, + context, + }); + + it('a plain user is scoped to rows that NAME them (recipient or grantor)', async () => { + const mw = buildSharingMiddleware(svc); + const ctx = findCtx({ userId: 'mallory', systemPermissions: [] }); + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ + $or: [{ recipient_id: 'mallory' }, { granted_by: 'mallory' }], + }); + }); + + it('manage_sharing (and the legacy manage_platform_settings) keeps the tenant-wide list', async () => { + const mw = buildSharingMiddleware(svc); + for (const cap of ['manage_sharing', 'manage_platform_settings']) { + const ctx = findCtx({ userId: 'admin', systemPermissions: [cap] }); + await mw(ctx, async () => {}); + expect(ctx.ast.where).toBeUndefined(); + } + }); + + it('a principal-less caller sees nothing (deny-all)', async () => { + const mw = buildSharingMiddleware(svc); + const ctx = findCtx({}); + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ id: '__deny_all__' }); + }); + + it('system context is untouched', async () => { + const mw = buildSharingMiddleware(svc); + const ctx = findCtx({ isSystem: true }); + await mw(ctx, async () => {}); + expect(ctx.ast.where).toBeUndefined(); + }); + + it('composes with a caller-provided filter instead of replacing it', async () => { + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'sys_record_share', + operation: 'find', + ast: { where: { object_name: 'account' } }, + context: { userId: 'bob', systemPermissions: [] }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ + $and: [ + { object_name: 'account' }, + { $or: [{ recipient_id: 'bob' }, { granted_by: 'bob' }] }, + ], + }); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 6643b4d06f..ea423b4eb9 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -82,6 +82,15 @@ function hasOwnerField(schema: any): boolean { return Boolean(schema?.fields && OWNER_FIELD in schema.fields); } +/** + * [ADR-0111 D2] The narrow slice of `ISecurityService` the management gate + * needs — kept structural so unit tests can pass a stub and so a deployment + * without `@objectstack/plugin-security` degrades to owner-only (fail closed). + */ +export interface SharingSecurityProbe { + hasWriteBypass?(object: string, context: unknown): Promise; +} + export interface SharingServiceOptions { engine: SharingEngine; /** Object names that bypass sharing — typically platform internals. */ @@ -91,6 +100,13 @@ export interface SharingServiceOptions { * (`hierarchy-scope-resolver` service). Returns null in the open edition. */ hierarchyResolver?: () => IHierarchyScopeResolver | null | undefined; + /** + * [ADR-0111 D1/D2] Late-bound lookup for the `security` service, probed for + * the super-user write bypass (`modifyAllRecords`) in + * {@link SharingService.canManageShares}. Absent / throwing / returning + * null → management authority fails CLOSED to owner-only. + */ + securityService?: () => SharingSecurityProbe | null | undefined; } /** @@ -105,10 +121,12 @@ export class SharingService implements ISharingService { private readonly engine: SharingEngine; private readonly bypassObjects: Set; private readonly hierarchyResolver?: () => IHierarchyScopeResolver | null | undefined; + private readonly securityService?: () => SharingSecurityProbe | null | undefined; constructor(options: SharingServiceOptions) { this.engine = options.engine; this.hierarchyResolver = options.hierarchyResolver; + this.securityService = options.securityService; this.bypassObjects = new Set([ 'sys_record_share', 'sys_user', @@ -285,9 +303,142 @@ export class SharingService implements ISharingService { return Array.isArray(editGrants) && editGrants.length > 0; } + /** + * [ADR-0111 D1] May `context` MANAGE shares (grant / revoke / list) on + * `(object, recordId)`? System → yes. Record owner → yes. Super-user write + * bypass (`modifyAllRecords`, probed via the late-bound security service) → + * yes. Everything else — a missing record, a principal-less context, a + * probe failure, a deployment without plugin-security — fails CLOSED to + * `false`. The DEPTH extension (hierarchy managers may share subordinates' + * records) is a named ADR-0111 direction, deliberately not implemented here. + */ + async canManageShares( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + if (context?.isSystem) return true; + if (!object || !recordId || !context?.userId) return false; + + // Ownership — read under system context so field-level masking cannot + // hide the owner column from the decision itself. + try { + const rows = await this.engine.find(object, { + where: { id: recordId }, + fields: ['id', OWNER_FIELD], + limit: 1, + context: SYSTEM_CTX, + }); + const row: any = Array.isArray(rows) ? rows[0] : undefined; + if (!row) return false; + const owner = row[OWNER_FIELD]; + if (owner != null && String(owner) === String(context.userId)) return true; + } catch { + return false; + } + + // 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 { + const probe = this.securityService?.(); + if (probe && typeof probe.hasWriteBypass === 'function') { + return (await probe.hasWriteBypass(object, context)) === true; + } + } catch { + /* fall through to deny */ + } + return false; + } + + /** + * [ADR-0111 D5] Is `(object, recordId)` VISIBLE to the caller? Reads under + * the CALLER's own context so the security RLS and sharing read filters + * decide, exactly as a plain find would. Any failure → not visible (fail + * closed); callers surface that as NOT_FOUND so a missing record and an + * invisible one are indistinguishable. + */ + private async isRecordVisible( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + try { + const rows = await this.engine.find(object, { + where: { id: recordId }, + fields: ['id'], + limit: 1, + context, + }); + return Array.isArray(rows) && rows.length > 0; + } catch { + return false; + } + } + + /** + * [ADR-0111 D1/D5] The shared pre-flight for every management verb: + * invisible/missing record → NOT_FOUND (404); visible but unmanageable → + * PERMISSION_DENIED (403). System context skips both. + */ + private async assertCanManageShares( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + if (context?.isSystem) return; + if (!(await this.isRecordVisible(object, recordId, context))) { + throw new Error(`NOT_FOUND: ${object}/${recordId} not found`); + } + if (!(await this.canManageShares(object, recordId, context))) { + throw new Error( + `PERMISSION_DENIED: managing shares on ${object}/${recordId} requires record ownership or Modify All Data (ADR-0111 D1)`, + ); + } + } + + /** + * [ADR-0111 D7] The object must be one the sharing gates actually consult — + * otherwise the grant would persist a row no read/write decision ever reads + * (the ADR-0078 silently-inert trap, inverted: "share" succeeds and nothing + * is shared). Bypass objects, `controlled_by_parent` (a detail record's + * access follows its master, ADR-0055), public models, and owner-less + * objects all refuse with SHARING_NOT_ENABLED (REST: 422). An engine + * without schema access skips the check — it cannot know, and this guard is + * an inertness guard, not the authority gate. + */ + private assertSharingEnforced(object: string): void { + if (this.bypassObjects.has(object)) { + throw new Error( + `SHARING_NOT_ENABLED: '${object}' bypasses record sharing; a share row on it would never be consulted`, + ); + } + if (typeof this.engine.getSchema !== 'function') return; + const schema = this.engine.getSchema(object); + if (!schema) throw new Error(`NOT_FOUND: unknown object '${object}'`); + const declared = schema?.sharingModel ?? schema?.security?.sharingModel; + if (declared === 'controlled_by_parent') { + throw new Error( + `SHARING_NOT_ENABLED: '${object}' is controlled by its parent (master-detail); share the master record instead`, + ); + } + if (effectiveSharingModel(schema) === 'public' || !hasOwnerField(schema)) { + throw new Error( + `SHARING_NOT_ENABLED: '${object}' is not under record-sharing enforcement ` + + `(public sharing model or no '${OWNER_FIELD}' field); a share row on it would never be consulted`, + ); + } + } + /** * Upsert a share row. Returning the existing row when an identical * grant already exists keeps the REST endpoint idempotent. + * + * [ADR-0111 D1/D7] Non-system callers must hold {@link canManageShares} on + * the record; the recipient must be a `user` (the only type any gate + * enforces); and the object must be in an enforcing sharing posture. The + * upsert matches on `(object, record, recipient, source)` so a manual grant + * never clobbers a rule-materialised row (and vice versa) — when both exist, + * the gates' `$in` queries make the widest level win (grants are additive). */ async grant( input: GrantShareInput, @@ -298,6 +449,16 @@ export class SharingService implements ISharingService { if (!input.recipientId) throw new Error('VALIDATION_FAILED: recipientId is required'); const recipientType = input.recipientType ?? 'user'; + // [ADR-0111 D7] Only `user` recipients are consulted by the read/write + // gates; persisting any other type would be a silently inert grant + // (ADR-0078). Group / business-unit principals are delivered via sharing + // rules, whose evaluator expands them into per-user rows. + if (recipientType !== 'user') { + throw new Error( + `VALIDATION_FAILED: recipientType must be 'user' (received ${JSON.stringify(recipientType)}) — ` + + `group/position recipients are delivered via sharing rules (ADR-0111 D7)`, + ); + } // Validate BEFORE any write. Previously anything at all was persisted // verbatim, so a typo'd level became a grant that no gate ever matched — a // share row that looks granted and enforces nothing (#3865). `full` @@ -306,14 +467,27 @@ export class SharingService implements ISharingService { const accessLevel: ShareAccessLevel = normalizeAccessLevel(input.accessLevel, 'read'); const source = input.source ?? 'manual'; - // Upsert: if a row with same (object, record, recipient) exists, - // update its access level / reason; otherwise insert a new one. + // [ADR-0111 D1/D7] Authorization + posture, service-side so every caller + // is covered (#3902 ③ — the REST route used to hand any signed-in user + // straight to this SYSTEM_CTX write path). System callers bypass: the + // rule evaluator materialises through here under its own validation. + if (!context?.isSystem) { + this.assertSharingEnforced(input.object); + await this.assertCanManageShares(input.object, input.recordId, context); + } + + // Upsert: if a row with same (object, record, recipient, source) exists, + // update its access level / reason; otherwise insert a new one. `source` + // is part of the key (ADR-0111 D7): a manual grant must not clobber a + // rule-materialised row — the rule's next reconcile would fight it (flip + // it back or purge it), leaving access flapping between the two answers. const existing = await this.engine.find('sys_record_share', { where: { object_name: input.object, record_id: input.recordId, recipient_type: recipientType, recipient_id: input.recipientId, + source, }, limit: 1, context: SYSTEM_CTX, @@ -352,21 +526,82 @@ export class SharingService implements ISharingService { return row as RecordShare; } - /** Delete a share row by id. No-op when not found. */ - async revoke(shareId: string, _context: SharingExecutionContext): Promise { + /** + * Delete a share row by id. + * + * [ADR-0111 D4] `revoke(shareId)` used to delete unconditionally — any + * signed-in user holding (or enumerating) a share id could silently strip + * any user's access to any record (#3902 ①). Non-system callers now must: + * hold {@link canManageShares} on the share's record (revoke is SYMMETRIC + * with grant — no granter exception); pass a `scope` match when the caller + * supplies one (the REST route forwards its URL's object/record, so a share + * id cannot be revoked through an unrelated path); and target a `manual` + * row — rule-materialised grants would be silently re-granted on the next + * reconcile, so "revoked" would be a lie (deactivate the rule instead). + * System callers keep the historical delete-by-id no-op-when-missing + * behaviour (the rule evaluator's reconciliation path). + */ + async revoke( + shareId: string, + context: SharingExecutionContext, + scope?: { object: string; recordId: string }, + ): Promise { if (!shareId) throw new Error('VALIDATION_FAILED: shareId is required'); + if (context?.isSystem) { + await this.engine.delete('sys_record_share', { + where: { id: shareId }, + context: SYSTEM_CTX, + }); + return; + } + + const rows = await this.engine.find('sys_record_share', { + where: { id: shareId }, + limit: 1, + context: SYSTEM_CTX, + }); + const row: any = Array.isArray(rows) ? rows[0] : undefined; + // Missing row and scope-mismatched row are the same 404 — neither + // confirms the share's existence to a caller who cannot manage it. + if (!row) throw new Error(`NOT_FOUND: share ${shareId} not found`); + if ( + scope + && (String(row.object_name) !== String(scope.object) + || String(row.record_id) !== String(scope.recordId)) + ) { + throw new Error(`NOT_FOUND: share ${shareId} not found on ${scope.object}/${scope.recordId}`); + } + await this.assertCanManageShares(String(row.object_name), String(row.record_id), context); + if (row.source != null && row.source !== 'manual') { + throw new Error( + `CONFLICT: share ${shareId} is materialised by source '${row.source}' and would be re-granted ` + + `on the next reconciliation — deactivate or edit its sharing rule instead (ADR-0111 D4)`, + ); + } await this.engine.delete('sys_record_share', { where: { id: shareId }, context: SYSTEM_CTX, }); } - /** List share rows for `(object, recordId)`. */ + /** + * List share rows for `(object, recordId)`. + * + * [ADR-0111 D5] Management-gated for non-system callers: enumerating who + * can see a record is both an information disclosure and the exact recon + * that hands an attacker the share ids the revoke gate protects (#3902 ②). + * Salesforce's Sharing Detail page is likewise owner/hierarchy/admin-only. + * "What is shared with me / by me" is served by the self-scoped + * `sys_record_share` read surface instead. + */ async listShares( object: string, recordId: string, - _context: SharingExecutionContext, + context: SharingExecutionContext, ): Promise { + if (!context?.isSystem) { + await this.assertCanManageShares(object, recordId, context); + } const rows = await this.engine.find('sys_record_share', { where: { object_name: object, record_id: recordId }, orderBy: [{ field: 'created_at', order: 'desc' }], diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 70d86a5959..c74a613553 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -5667,8 +5667,32 @@ export class RestServer { code: 'NOT_IMPLEMENTED', message: 'Sharing service is not configured on this deployment', }); + // [ADR-0111] The service enforces authorization (D1/D4/D5/D7) and + // signals the verdict via message prefixes, the plugin's established + // error idiom — this maps them onto HTTP. Returns true when handled. + const respondSharingError = (res: any, error: any): boolean => { + const msg = String(error?.message ?? error ?? ''); + const map: Array<[string, number]> = [ + ['VALIDATION_FAILED', 400], + ['PERMISSION_DENIED', 403], + ['NOT_FOUND', 404], + ['CONFLICT', 409], + ['SHARING_NOT_ENABLED', 422], + ]; + for (const [code, status] of map) { + if (msg.startsWith(code)) { + res.status(status).json({ + code, + error: msg.replace(new RegExp(`^${code}:\\s*`), ''), + }); + return true; + } + } + return false; + }; - // GET — list shares on a record. + // GET — list shares on a record. [ADR-0111 D5] Management-gated in the + // service: invisible record → 404, visible-but-not-manager → 403. this.routeManager.register({ method: 'GET', path: `${dataPath}/:object/:id/shares`, @@ -5682,6 +5706,7 @@ export class RestServer { const rows = await svc.listShares(req.params.object, req.params.id, context ?? {}); res.json({ data: rows }); } catch (error: any) { + if (respondSharingError(res, error)) return; logError('[REST] List shares error:', error); res.status(500).json({ code: 'SHARES_LIST_FAILED', error: String(error?.message ?? error).slice(0, 500) }); } @@ -5689,7 +5714,8 @@ export class RestServer { metadata: { summary: 'List per-record sharing grants', tags: ['sharing'] }, }); - // POST — grant access. + // POST — grant access. [ADR-0111 D1/D7] Authorization + posture live + // in the service; this route only maps verdicts (403/404/422/400). this.routeManager.register({ method: 'POST', path: `${dataPath}/:object/:id/shares`, @@ -5711,21 +5737,10 @@ export class RestServer { sourceId: body.sourceId ?? body.source_id, reason: body.reason, }; - try { - const row = await svc.grant(input, context ?? {}); - res.status(201).json(row); - } catch (err: any) { - const msg = String(err?.message ?? err ?? ''); - if (msg.startsWith('VALIDATION_FAILED')) { - res.status(400).json({ - code: 'VALIDATION_FAILED', - error: msg.replace(/^VALIDATION_FAILED:\s*/, ''), - }); - return; - } - throw err; - } + const row = await svc.grant(input, context ?? {}); + res.status(201).json(row); } catch (error: any) { + if (respondSharingError(res, error)) return; logError('[REST] Grant share error:', error); res.status(500).json({ code: 'SHARE_GRANT_FAILED', error: String(error?.message ?? error).slice(0, 500) }); } @@ -5733,7 +5748,10 @@ export class RestServer { metadata: { summary: 'Grant a per-record share to a principal', tags: ['sharing'] }, }); - // DELETE — revoke a share by id. + // DELETE — revoke a share by id. [ADR-0111 D4] The URL's + // (object, id) is forwarded as the revoke scope so a share id can only + // be revoked through the record it belongs to; the service enforces + // management authority and the manual-source rule (409). this.routeManager.register({ method: 'DELETE', path: `${dataPath}/:object/:id/shares/:shareId`, @@ -5744,9 +5762,14 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; const svc = await resolveService(environmentId); if (!svc) return respond501(res); - await svc.revoke(req.params.shareId, context ?? {}); + await svc.revoke( + req.params.shareId, + context ?? {}, + { object: req.params.object, recordId: req.params.id }, + ); res.status(204).end(); } catch (error: any) { + if (respondSharingError(res, error)) return; logError('[REST] Revoke share error:', error); res.status(500).json({ code: 'SHARE_REVOKE_FAILED', error: String(error?.message ?? error).slice(0, 500) }); } @@ -5790,6 +5813,12 @@ export class RestServer { if (msg.startsWith('VALIDATION_FAILED')) { return res.status(400).json({ code: 'VALIDATION_FAILED', error: msg.replace(/^VALIDATION_FAILED:\s*/, '') }); } + // [ADR-0111 D6] The service gates every verb on `manage_sharing` + // (enforced there so non-REST callers are covered too) — map its + // verdict rather than burying it in a 500. + if (msg.startsWith('PERMISSION_DENIED')) { + return res.status(403).json({ code: 'PERMISSION_DENIED', error: msg.replace(/^PERMISSION_DENIED:\s*/, '') }); + } if (msg.startsWith('RULE_NOT_FOUND')) { return res.status(404).json({ code: 'RULE_NOT_FOUND', error: msg.replace(/^RULE_NOT_FOUND:?\s*/, '') }); } diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 1b2b2cbe03..e5aa82fdf4 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -1436,7 +1436,13 @@ describe('RestServer', () => { const { revoke: route } = getShareRoutes(rest); const res = { json: vi.fn(), status: vi.fn().mockReturnThis(), end: vi.fn() }; await route!.handler({ params: { object: 'account', id: 'a1', shareId: 'shr_X' } } as any, res as any); - expect(revoke).toHaveBeenCalledWith('shr_X', expect.anything()); + expect(revoke).toHaveBeenCalledWith( + 'shr_X', + expect.anything(), + // [ADR-0111 D4] The URL's record is forwarded as the revoke scope + // so a share id cannot be revoked through an unrelated path. + { object: 'account', recordId: 'a1' }, + ); expect(res.status).toHaveBeenCalledWith(204); }); }); diff --git a/packages/spec/src/contracts/security-service.ts b/packages/spec/src/contracts/security-service.ts index 72f30050ae..c38ad9032a 100644 --- a/packages/spec/src/contracts/security-service.ts +++ b/packages/spec/src/contracts/security-service.ts @@ -217,6 +217,25 @@ export interface ISecurityService { */ canExport(object: string, context?: SecurityContext): Promise; + /** + * [ADR-0111 D2] Whether `context` holds the super-user WRITE bypass + * (`modifyAllRecords`, "Modify All Data") for `object` — the EXPLICIT bit + * only, resolved from the caller's permission sets exactly as the CRUD + * middleware resolves them. + * + * The probe behind the sharing layer's management-authority gate + * (`ISharingService.canManageShares`): a non-owner may manage shares on a + * record only with this bypass. It deliberately does NOT reuse the effective + * write SCOPE — `getEffectiveScope` returns `'org'` for the + * "no permission set mentions this object" case (a compatibility fail-open on + * the read path), which as a management gate would be a fresh hole. + * + * **Fails CLOSED.** A resolution failure, a principal-less context, or an + * on-behalf-of context (the D10 delegator intersection is not computed on + * this path) returns `false`. A system context returns `true`. + */ + hasWriteBypass(object: string, context?: SecurityContext): Promise; + /** * Explain WHY access is granted or denied — the decision plus the layers that * produced it (permission sets, object permissions, RLS, sharing, field mask). diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index d687928f87..6e938aeaf1 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -81,6 +81,13 @@ export interface SharingExecutionContext { tenantId?: string; positions?: string[]; permissions?: string[]; + /** + * [ADR-0111] Capability names the caller holds (`manage_sharing`, …) — + * resolved by `resolveAuthzContext` alongside `permissions` (which carries + * permission-set NAMES, not capabilities). Consulted by the sharing-rule + * management gate; absent → the caller holds no capabilities (fail closed). + */ + systemPermissions?: string[]; isSystem?: boolean; } @@ -114,13 +121,67 @@ export interface ISharingService { context: SharingExecutionContext, ): Promise; - /** Create or upsert a manual share row. */ + /** + * [ADR-0111 D1] May the principal in `context` MANAGE shares (grant / revoke + * / list) on `(object, recordId)`? True for system context, the record's + * owner, and holders of the super-user write bypass (`modifyAllRecords`, + * probed via the late-bound security service). **Fails closed**: no security + * service → owner-only; unknown record / principal-less context → `false`. + * + * This is the single gate every manual share-management operation consults — + * enforcement lives in the SERVICE, so every caller (REST or otherwise) is + * covered. The DEPTH extension (hierarchy managers) is a named ADR-0111 + * direction, not implemented here. + */ + canManageShares( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise; + + /** + * Create or upsert a manual share row. + * + * [ADR-0111 D1/D7] For non-system callers: requires {@link canManageShares} + * (`PERMISSION_DENIED`), an existing record VISIBLE to the caller + * (`NOT_FOUND` — missing and invisible are indistinguishable), a `user` + * recipient (`VALIDATION_FAILED` — other recipient types are not enforced by + * any gate and are refused rather than persisted inert, ADR-0078), and an + * object in an enforcing sharing posture (`SHARING_NOT_ENABLED` — a share on + * a public / bypass / owner-less / `controlled_by_parent` object enforces + * nothing). Upserts match on `(object, record, recipient, source)` so a + * manual grant never clobbers a rule-materialised row (D7). + */ grant(input: GrantShareInput, context: SharingExecutionContext): Promise; - /** Remove a share row by id. No-op when not found. */ - revoke(shareId: string, context: SharingExecutionContext): Promise; + /** + * Remove a share row by id. + * + * [ADR-0111 D4] For non-system callers: the row must exist and — when + * `scope` is provided (REST passes the URL's object/record) — belong to that + * record (`NOT_FOUND` otherwise), the caller must hold + * {@link canManageShares} on the record (`PERMISSION_DENIED` — revoke is + * symmetric with grant, no granter exception), and only `source: 'manual'` + * rows are revocable (`CONFLICT` — rule-derived grants are reconciled back + * by the evaluator; deactivate the rule instead). System context keeps the + * historical delete-by-id no-op-when-missing behaviour (the rule evaluator's + * reconciliation path). + */ + revoke( + shareId: string, + context: SharingExecutionContext, + scope?: { object: string; recordId: string }, + ): Promise; - /** List all share rows attached to `(object, recordId)`. */ + /** + * List all share rows attached to `(object, recordId)`. + * + * [ADR-0111 D5] For non-system callers this is MANAGEMENT-gated: an + * invisible/missing record throws `NOT_FOUND`, a visible record without + * {@link canManageShares} throws `PERMISSION_DENIED`. "What is shared with + * me / by me" is served by the self-scoped `sys_record_share` read surface, + * not by this method. + */ listShares( object: string, recordId: string, diff --git a/packages/spec/src/security/capabilities.ts b/packages/spec/src/security/capabilities.ts index 8a81eedb5d..8ce78878a9 100644 --- a/packages/spec/src/security/capabilities.ts +++ b/packages/spec/src/security/capabilities.ts @@ -47,6 +47,14 @@ export const PLATFORM_CAPABILITIES: readonly PlatformCapability[] = [ // granted — which was harmless only while settings writes went ungated. { name: 'setup.write', label: 'Write Settings', description: 'Save changes to tenant/Setup settings pages.', scope: 'org' }, { name: 'studio.access', label: 'Studio Access', description: 'Enter the Studio metadata-design surfaces.', scope: 'platform' }, + // [ADR-0111 D9] Sharing administration right-sized below full platform admin: + // gates the sharing-rule surface (define / delete / evaluate — an org-wide + // grant generator) and, in the DEPTH extension, the non-owner path to + // per-record share management. Salesforce ships the analogous standalone + // "Manage Sharing" permission. Seeded into `admin_full_access` so existing + // admin flows are unchanged; `manage_platform_settings` is honoured as a + // legacy equivalent by the enforcement seams that predate this capability. + { name: 'manage_sharing', label: 'Manage Sharing', description: 'Administer record sharing: author and evaluate sharing rules, and manage per-record shares beyond one’s own records.', scope: 'org' }, ]; /** Set of built-in capability names, for fast membership checks (lint, gating). */