From 0c7d59cb1d313d1f9e6058540826c5d24c55b599 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:04:24 +0000 Subject: [PATCH] fix(sharing)!: give the share-management surface the authorization layer it never had (ADR-0111 P0, #3902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every /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. This lands ADR-0111's P0 rulings: - D1/D2: ISharingService.canManageShares (owner / Modify All via the new fail-closed ISecurityService.hasWriteBypass probe / system), enforced in the service so every caller is covered; owner-only without plugin-security. - D4: revoke is symmetric with grant, validates the share belongs to the URL's record, and refuses non-manual rows (the reconcile would resurrect them). - D5: listShares is management-gated; /data/sys_record_share reads are self-scoped to rows naming the caller unless they hold sharing-admin capability. - D6: the /sharing/rules surface requires the new manage_sharing capability (D9; seeded into admin_full_access, manage_platform_settings honoured as legacy), enforced in SharingRuleService. - D7: no inert grants — recipientType narrowed to 'user', grants on objects the gates never consult refuse with SHARING_NOT_ENABLED, and the manual upsert keys on (object, record, recipient, source) so manual and rule rows coexist. The explain engine reads share rows under system context now that listShares is gated (its own caller authorization already ran). Verified by the #3902 Mallory reproduction (grant/revoke/enumerate) and the D6 gate suite. The verb boundary (edit ≠ delete, D3) is P1, not here. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012P3RFCpjjiSQ7WuamSDW7W --- .../adr-0111-sharing-authorization-face.md | 43 +++ content/docs/kernel/index.mdx | 2 +- .../runtime-services/sharing-service.mdx | 19 +- content/docs/permissions/sharing-rules.mdx | 12 + ...-management-authority-and-verb-boundary.md | 2 +- .../src/objects/default-permission-sets.ts | 3 + .../plugin-security/src/security-plugin.ts | 30 +- .../plugin-sharing/src/sharing-plugin.ts | 30 ++ .../src/sharing-rule-service.ts | 26 ++ .../plugin-sharing/src/sharing-rule.test.ts | 59 ++++ .../src/sharing-service.test.ts | 304 +++++++++++++++++- .../plugin-sharing/src/sharing-service.ts | 247 +++++++++++++- packages/rest/src/rest-server.ts | 65 ++-- packages/rest/src/rest.test.ts | 8 +- .../spec/src/contracts/security-service.ts | 19 ++ .../spec/src/contracts/sharing-service.ts | 69 +++- packages/spec/src/security/capabilities.ts | 8 + 17 files changed, 905 insertions(+), 41 deletions(-) create mode 100644 .changeset/adr-0111-sharing-authorization-face.md 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). */