From 684742a49635e5e8c124dfc8cf07d8760d3a6596 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 15:24:14 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(sharing)!:=20remove=20the=20`full`=20ac?= =?UTF-8?q?cess=20level=20=E2=80=94=20it=20promised=20delete/transfer/shar?= =?UTF-8?q?e=20and=20granted=20`edit`=20(#3865)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sys_sharing_rule.access_level` / `sys_record_share.access_level` offered three levels, the third documented as **Full Access (Transfer, Share, Delete)**. No code path granted transfer, re-share, or delete because of it: both enforcement sites matched `access_level in ('edit','full')`, so `full` was byte-equivalent to `edit`. An admin picking "Full Access" in Setup was told they had granted delete rights and had not — declared-but-unenforced metadata (ADR-0078, ADR-0049), the same defect that retired the `queue` recipient before it. Measured on showcase, a `full` recipient got read/update allowed and delete DENIED with `decidedBy=object_crud` — the object-level CRUD gate rejected the delete *before* sharing was consulted at all. That is the model working, not an oversight to patch around. Record sharing widens WHICH ROWS a principal reaches, never WHICH VERBS they may use: Salesforce sharing rules stop at Read-Only / Read-Write (its Full Access is owner / hierarchy / Modify All only, never grantable by a rule), and Dataverse ANDs every shared access right against the security role's own privilege. Delete and transfer belong to ownership, the ADR-0057 DEPTH scopes, and admin scope. Authoring rejects, enforcement tolerates, data normalises (the ADR-0090 D4 idiom): - `SharingLevel` (spec/security) and `ShareAccessLevel` (spec/contracts) narrow to `read | edit`; the `Field.select` on both objects offers the same two, so the Setup dropdown no longer shows the misleading option. - `SharingService.grant()` and `SharingRuleService.defineRule()` gain the access-level validation they never had. Previously ANY value was persisted verbatim, so a typo'd level became a grant no gate would ever match — the same inert-metadata bug one layer down. `full` normalises to `edit`; anything unrecognised is a `VALIDATION_FAILED` the REST layer maps to 400. - The read/write gates keep matching `edit`/`full` on purpose. Narrowing them would silently REVOKE every not-yet-migrated grant; authoring narrowness and enforcement tolerance are different jobs. - A boot backfill normalises stored `full` rows on both tables (writing with `isSystem` so the provenance hook does not mark a package-seeded rule `customized`), and the `sharing-rule-access-level-full-to-edit` conversion rewrites declarative stacks at load. Lossless by construction: the two levels were already equivalent, so the rewrite cannot change an access decision — unlike the OWD `sharingModel: 'full'` alias retired in ADR-0090 D4, which changed posture and had to be delegated to the author. The explain surfaces keep accepting `'full'`: they REPORT stored rows, and a legacy row must stay explainable rather than crash the panel. Verified on a running CRM backend: live metadata offers only read/edit; a grant of `full` returns 201 persisted as `edit`; a bogus level returns 400; and legacy `full` rows seeded directly into SQLite came back `edit` after reboot with `managed_by`/`customized` untouched. Reviving a real per-record delete grant is a separate design — a capability mask ANDed with object CRUD, plus the share-administration model that would have to authorise re-sharing — not a fourth enum member. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017346r3TMNqpbTLkT5d49uQ --- .../sharing-access-level-full-removed.md | 54 ++++++++ content/docs/permissions/sharing-rules.mdx | 2 +- content/docs/protocol/objectql/security.mdx | 4 +- docs/notes/crm-development-standards.mdx | 2 +- .../plugin-security/src/explain-engine.ts | 10 +- .../plugin-sharing/src/access-level.test.ts | 100 ++++++++++++++ .../plugin-sharing/src/access-level.ts | 126 ++++++++++++++++++ .../plugin-sharing/src/boot-backfill.test.ts | 82 +++++++++++- packages/plugins/plugin-sharing/src/index.ts | 8 ++ .../src/objects/sys-record-share.object.ts | 14 +- .../src/objects/sys-sharing-rule.object.ts | 12 +- .../plugin-sharing/src/sharing-plugin.ts | 109 ++++++++++++++- .../src/sharing-rule-service.ts | 11 +- .../src/sharing-service.test.ts | 44 +++++- .../plugin-sharing/src/sharing-service.ts | 20 ++- .../src/translations/en.objects.generated.ts | 9 +- .../translations/es-ES.objects.generated.ts | 9 +- .../translations/ja-JP.objects.generated.ts | 9 +- .../translations/zh-CN.objects.generated.ts | 9 +- .../spec/src/contracts/sharing-service.ts | 14 +- packages/spec/src/conversions/registry.ts | 62 +++++++++ packages/spec/src/migrations/registry.ts | 10 +- packages/spec/src/security/explain.zod.ts | 13 +- packages/spec/src/security/sharing.test.ts | 15 ++- packages/spec/src/security/sharing.zod.ts | 24 +++- 25 files changed, 720 insertions(+), 52 deletions(-) create mode 100644 .changeset/sharing-access-level-full-removed.md create mode 100644 packages/plugins/plugin-sharing/src/access-level.test.ts create mode 100644 packages/plugins/plugin-sharing/src/access-level.ts diff --git a/.changeset/sharing-access-level-full-removed.md b/.changeset/sharing-access-level-full-removed.md new file mode 100644 index 0000000000..87eae343bf --- /dev/null +++ b/.changeset/sharing-access-level-full-removed.md @@ -0,0 +1,54 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-sharing": minor +"@objectstack/plugin-security": patch +--- + +fix(sharing): remove the `full` access level — it promised delete/transfer/share and granted `edit` (#3865) + +`sys_sharing_rule.access_level` / `sys_record_share.access_level` offered three +levels, the third documented as **Full Access (Transfer, Share, Delete)**. No +code path granted transfer, re-share, or delete because of it: both enforcement +sites matched `access_level in ('edit','full')`, so `full` was byte-equivalent +to `edit`. An admin picking "Full Access" in Setup was told they had granted +delete rights and had not — declared-but-unenforced metadata (ADR-0078, +ADR-0049), the same defect that retired the `queue` recipient before it. + +Measured on showcase, a `full` recipient got `read: allowed`, `update: allowed`, +`delete: DENIED` — and the denial came from `decidedBy=object_crud`, i.e. the +object-level CRUD gate rejected the delete *before* sharing was consulted at +all. That is not an oversight to patch around; it is the model working. Record +sharing widens **which rows** a principal reaches, never **which verbs** they +may use — the same split Salesforce enforces (its sharing rules stop at +Read-Only / Read-Write; Full Access is owner / hierarchy / Modify All only, +never grantable by a rule) and Dataverse enforces by AND-ing every shared access +right against the security role's own privilege. Delete and transfer belong to +ownership, the ADR-0057 DEPTH scopes, and admin scope. + +**What changed** + +- `SharingLevel` (spec/security) and `ShareAccessLevel` (spec/contracts) are now + `read | edit`. The `Field.select` on both objects offers the same two, so the + Setup dropdown no longer shows the misleading option. +- `SharingService.grant()` and `SharingRuleService.defineRule()` gained the + access-level validation they never had: `full` normalises to `edit`, and an + unrecognised level is a `VALIDATION_FAILED` (HTTP 400) instead of being + persisted verbatim as a grant no gate would ever match. +- Enforcement stays deliberately wider than authoring — the read/write gates + still match `edit`/`full` — so a row written before this release keeps + working. Narrowing them would silently *revoke* access. +- A boot backfill normalises stored `full` rows on both tables, and the + `sharing-rule-access-level-full-to-edit` conversion rewrites declarative + stacks at load, so nothing needs consumer action. + +**Migration.** None. `full` and `edit` were already behaviourally identical, so +rewriting one to the other cannot change an access decision — unlike the OWD +`sharingModel: 'full'` alias retired in ADR-0090 D4, which changed posture and +had to be delegated to the author. A stack that still authors `accessLevel: +'full'` converts at load with a deprecation notice; stored rows normalise at +next boot. Code that pinned the `ShareAccessLevel` type to `'full'` no longer +compiles — use `'edit'`. + +Reviving a real per-record delete grant is a separate design (a capability mask +AND-ed with object CRUD, plus the share-administration model that would have to +authorise re-sharing), not a fourth enum member. diff --git a/content/docs/permissions/sharing-rules.mdx b/content/docs/permissions/sharing-rules.mdx index 7bfae11705..d586c33ee9 100644 --- a/content/docs/permissions/sharing-rules.mdx +++ b/content/docs/permissions/sharing-rules.mdx @@ -125,7 +125,7 @@ export const AccountTeamSharingRule = defineSharingRule({ // Who to share with (a single recipient — see the recipient types below) sharedWith: { type: 'position', value: 'sales_manager' }, - // Access level granted: read | edit | full + // Access level granted: read | edit accessLevel: 'edit', }); ``` diff --git a/content/docs/protocol/objectql/security.mdx b/content/docs/protocol/objectql/security.mdx index 5883a7e912..c63b15b1b4 100644 --- a/content/docs/protocol/objectql/security.mdx +++ b/content/docs/protocol/objectql/security.mdx @@ -280,7 +280,7 @@ Share records whose fields match a CEL predicate (`CriteriaSharingRuleSchema` in name: share_enterprise_accounts type: criteria object: account -accessLevel: read # read | edit | full +accessLevel: read # read | edit condition: 'record.account_type == "Enterprise"' sharedWith: type: position # user | group | position | unit_and_subordinates | guest @@ -308,7 +308,7 @@ sharedWith: > **Enforcement status.** Criteria rules with `user` / `position` / `unit_and_subordinates` recipients compile and enforce (the CEL condition lowers to a runtime filter that materializes `sys_record_share` grants, ADR-0058 D3). Owner-type rules and `group`/`guest` recipients are `[experimental — not enforced]`: the seed bootstrap skips them (logged) rather than seeding a permissive match-all (ADR-0049). -> `accessLevel` is one of `read`, `edit`, or `full`. `full` additionally grants transfer/share/delete. +> `accessLevel` is one of `read` or `edit`. Sharing widens **which rows** a principal reaches, never **which verbs** they may use — delete and transfer come from ownership, the ADR-0057 DEPTH scopes, or admin scope, and are checked by the object-level CRUD gate before sharing is consulted at all. A third level `full` ("Full Access — transfer/share/delete") was declared until protocol 16 but never granted any of those verbs: both enforcement sites matched `edit`/`full` alike, so it was equivalent to `edit` while telling admins otherwise, and it was removed (#3865, ADR-0078). Stacks still authoring it are rewritten to `edit` at load by the `sharing-rule-access-level-full-to-edit` conversion. ### Public Share Links diff --git a/docs/notes/crm-development-standards.mdx b/docs/notes/crm-development-standards.mdx index 57857b7701..54810f9e7a 100644 --- a/docs/notes/crm-development-standards.mdx +++ b/docs/notes/crm-development-standards.mdx @@ -294,7 +294,7 @@ export const MySharingRule = defineSharingRule({ value: 'role_name', }, - accessLevel: 'edit', // 'read' | 'edit' | 'full' + accessLevel: 'edit', // 'read' | 'edit' }); ``` diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index df37e2d805..efbfee7509 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -188,7 +188,15 @@ export interface ExplainEngineDeps { fetchRecord?: (object: string, recordId: string, engineOperation: string) => Promise | null>; /** The sharing service's own read-filter contribution for the object (owner-match OR granted-ids), same as enforcement AND-s in. */ sharingReadFilter?: (object: string, context: any) => Promise; - /** The concrete `sys_record_share` rows attached to the record (for `rules[]` attribution). */ + /** + * The concrete `sys_record_share` rows attached to the record (for `rules[]` + * attribution). + * + * `access_level` stays wider than the authorable `read`/`edit`: explain + * REPORTS stored rows, and a `full` row written before #3865 retired that + * level (normalised to `edit` by the sharing plugin's boot backfill) must + * still be explainable rather than crash the panel. + */ listRecordShares?: ( object: string, recordId: string, diff --git a/packages/plugins/plugin-sharing/src/access-level.test.ts b/packages/plugins/plugin-sharing/src/access-level.test.ts new file mode 100644 index 0000000000..2acfedaa8e --- /dev/null +++ b/packages/plugins/plugin-sharing/src/access-level.test.ts @@ -0,0 +1,100 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#3865] The access-level vocabulary after `full` was retired. + * + * `full` was declared "Full Access (Transfer, Share, Delete)" but every + * enforcement site matched `access_level in ('edit','full')`, so it granted + * exactly what `edit` grants while telling admins otherwise — the + * declared-but-unenforced trap ADR-0078 bans. These tests pin the three-layer + * posture that replaced it: authoring rejects, enforcement tolerates, data + * normalises. + */ + +import { describe, it, expect } from 'vitest'; +import { + ACCESS_LEVELS, + WRITE_ACCESS_LEVELS, + isKnownAccessLevel, + normalizeAccessLevel, + normalizeStoredAccessLevel, +} from './access-level.js'; + +describe('access-level vocabulary', () => { + it('offers exactly read and edit for authoring', () => { + expect([...ACCESS_LEVELS]).toEqual(['read', 'edit']); + }); + + it('keeps the write gate wider than the authorable set', () => { + // The whole point of the tolerance layer: a row persisted before the boot + // backfill ran must keep working. If this ever narrows to ['edit'], every + // un-migrated `full` grant silently loses write access. + expect([...WRITE_ACCESS_LEVELS]).toEqual(['edit', 'full']); + }); +}); + +describe('normalizeAccessLevel (authoring path)', () => { + it('passes the authorable levels through unchanged', () => { + expect(normalizeAccessLevel('read')).toBe('read'); + expect(normalizeAccessLevel('edit')).toBe('edit'); + }); + + it("normalises the retired 'full' to 'edit'", () => { + // Lossless precisely because `full` was inert — the two already behaved + // identically, so no principal gains or loses anything. + expect(normalizeAccessLevel('full')).toBe('edit'); + }); + + it('throws VALIDATION_FAILED on an unrecognised level', () => { + // Coercing an unknown level would recreate the bug in a new spot: a grant + // that looks issued and that no gate matches. + expect(() => normalizeAccessLevel('admin')).toThrow(/VALIDATION_FAILED/); + expect(() => normalizeAccessLevel('owner')).toThrow(/VALIDATION_FAILED/); + expect(() => normalizeAccessLevel(7)).toThrow(/VALIDATION_FAILED/); + }); + + it('applies the fallback only for a missing value', () => { + expect(normalizeAccessLevel(undefined, 'read')).toBe('read'); + expect(normalizeAccessLevel(null, 'edit')).toBe('edit'); + expect(normalizeAccessLevel('', 'read')).toBe('read'); + // A PRESENT but invalid value must never fall back — that would swallow + // the author's mistake. + expect(() => normalizeAccessLevel('nope', 'read')).toThrow(/VALIDATION_FAILED/); + }); + + it('requires a level when no fallback is supplied', () => { + expect(() => normalizeAccessLevel(undefined)).toThrow(/VALIDATION_FAILED/); + }); +}); + +describe('normalizeStoredAccessLevel (read path)', () => { + it('normalises known levels without throwing', () => { + expect(normalizeStoredAccessLevel('read')).toBe('read'); + expect(normalizeStoredAccessLevel('edit')).toBe('edit'); + expect(normalizeStoredAccessLevel('full')).toBe('edit'); + }); + + it('fails CLOSED to read on unrecognised or missing stored data', () => { + // Never throws: projecting an existing row must not take down the rule + // evaluator or an admin list view over one bad byte. Degrades to the + // NARROWEST level rather than trusting it. + expect(normalizeStoredAccessLevel('admin')).toBe('read'); + expect(normalizeStoredAccessLevel(undefined)).toBe('read'); + expect(normalizeStoredAccessLevel(null)).toBe('read'); + expect(normalizeStoredAccessLevel({})).toBe('read'); + }); +}); + +describe('isKnownAccessLevel', () => { + it('accepts authorable and retired-but-stored levels', () => { + expect(isKnownAccessLevel('read')).toBe(true); + expect(isKnownAccessLevel('edit')).toBe(true); + expect(isKnownAccessLevel('full')).toBe(true); + }); + + it('rejects anything else', () => { + expect(isKnownAccessLevel('admin')).toBe(false); + expect(isKnownAccessLevel(undefined)).toBe(false); + expect(isKnownAccessLevel(1)).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/access-level.ts b/packages/plugins/plugin-sharing/src/access-level.ts new file mode 100644 index 0000000000..e110f89b34 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/access-level.ts @@ -0,0 +1,126 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Share access-level vocabulary — the one place `read` / `edit` (and the + * retired `full`) are defined for this plugin. + * + * ## Why `full` is gone (#3865) + * + * `full` was declared as "Full Access (Transfer, Share, Delete)" but **no code + * path ever granted transfer, re-share, or delete because of it**: the read + * gate (`buildReadFilter`), the bulk-write gate (`buildWriteFilter`) and the + * per-record gate (`canEdit`) all matched `access_level in ('edit','full')`, so + * it was byte-equivalent to `edit`. An admin picking "Full Access" in Setup was + * told they had granted delete rights and had not — declared-but-unenforced + * metadata, the exact authoring trap ADR-0078 / ADR-0049 ban, and the reason + * `recipient_type: 'queue'` was removed before it. + * + * It is not coming back as an enum member. Record sharing widens **which rows** + * a principal reaches, never **which verbs** they may use — the same split + * Salesforce enforces (its sharing rules stop at Read-Only / Read-Write; Full + * Access is owner / hierarchy / Modify All only, never grantable by a rule) and + * Dataverse enforces by AND-ing every shared access right against the security + * role's own privilege. Delete and transfer belong to ownership, the ADR-0057 + * DEPTH scopes, and admin scope. A future per-record delete grant would be a + * capability mask AND-ed with object CRUD, not a fourth level. + * + * ## The three-layer posture (ADR-0090 D4 idiom) + * + * 1. **Authoring rejects** — `SharingLevel` / the `Field.select` enums no longer + * offer `full`; {@link normalizeAccessLevel} turns an explicit `full` from an + * older client into `edit` and rejects anything unrecognised outright. + * 2. **Runtime tolerates** — the enforcement gates keep matching + * {@link WRITE_ACCESS_LEVELS}, so a row persisted before the backfill ran is + * still honoured (it means `edit`; refusing it would silently REVOKE access). + * 3. **Data normalises** — a boot backfill rewrites stored `full` rows to + * `edit`. Behaviour-preserving by construction, because the two were already + * equivalent. + */ + +import type { ShareAccessLevel } from '@objectstack/spec/contracts'; + +/** The authorable levels, in widening order. */ +export const ACCESS_LEVELS: readonly ShareAccessLevel[] = ['read', 'edit']; + +/** + * Retired spellings that map losslessly onto an authorable level. + * + * `full` → `edit` is lossless *because* `full` was inert: the two already + * behaved identically, so rewriting one to the other cannot change any access + * decision. (Contrast the OWD `sharingModel: 'full'` alias retired in ADR-0090 + * D4, which had no equivalent target and had to be delegated to the author.) + */ +const RETIRED_ACCESS_LEVELS: Readonly> = { + full: 'edit', +}; + +/** + * Levels that open the write gate, INCLUDING the retired `full`. + * + * Deliberately wider than {@link ACCESS_LEVELS}: enforcement reads persisted + * rows, which may predate normalisation. Dropping `full` here would silently + * revoke access from every not-yet-migrated grant — a fail-open→fail-closed + * flip nobody asked for. Authoring narrowness and enforcement tolerance are + * different jobs. + */ +export const WRITE_ACCESS_LEVELS: readonly string[] = ['edit', 'full']; + +/** `true` when `value` is a stored level this plugin still honours. */ +export function isKnownAccessLevel(value: unknown): boolean { + return typeof value === 'string' + && (ACCESS_LEVELS.includes(value as ShareAccessLevel) || value in RETIRED_ACCESS_LEVELS); +} + +/** + * Normalise an inbound access level to the authorable vocabulary. + * + * - `read` / `edit` pass through. + * - `full` normalises to `edit` — quietly, because it is what `full` already + * meant; failing an old client's request would be a regression with no + * security benefit. + * - anything else (including `null`/`undefined` when no `fallback` is given) + * throws `VALIDATION_FAILED`, which the REST layer maps to a 400. Silently + * coercing an unrecognised level would be the same silent-inertness bug in a + * new spot: a caller asking for `admin` must be told it does not exist, not + * handed a `read` grant they did not request. + * + * @param value the inbound level (typically from an HTTP body or a rule row) + * @param fallback level to use when `value` is null/undefined; omit to require one + */ +export function normalizeAccessLevel( + value: unknown, + fallback?: ShareAccessLevel, +): ShareAccessLevel { + if (value == null || value === '') { + if (fallback !== undefined) return fallback; + throw new Error('VALIDATION_FAILED: accessLevel is required'); + } + if (typeof value === 'string') { + if (ACCESS_LEVELS.includes(value as ShareAccessLevel)) return value as ShareAccessLevel; + const retired = RETIRED_ACCESS_LEVELS[value]; + if (retired) return retired; + } + throw new Error( + `VALIDATION_FAILED: accessLevel must be one of ${ACCESS_LEVELS.join(' | ')} ` + + `(received ${JSON.stringify(value)})`, + ); +} + +/** + * Normalise a level read back OUT of storage. Never throws. + * + * The write-path twin ({@link normalizeAccessLevel}) rejects an unrecognised + * level so the caller learns immediately. A read path cannot do that: the row + * already exists, and throwing while projecting it would take down the rule + * evaluator or the admin list view over one bad byte. So this one fails + * **closed** — an unrecognised stored level degrades to `read`, the narrowest + * level, rather than being trusted or crashing (the same fail-closed posture + * `effectiveSharingModel` takes for an unknown `sharingModel`). + */ +export function normalizeStoredAccessLevel(value: unknown): ShareAccessLevel { + try { + return normalizeAccessLevel(value, 'read'); + } catch { + return 'read'; + } +} diff --git a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts index e6e8397c30..96000170f5 100644 --- a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts +++ b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts @@ -14,7 +14,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; -import { backfillRuleGrants } from './sharing-plugin.js'; +import { backfillRuleGrants, backfillRetiredAccessLevels } from './sharing-plugin.js'; interface Row { [k: string]: any } @@ -126,3 +126,83 @@ describe('backfillRuleGrants (#2926 ③ — seed rows materialize at boot)', () expect((engine._tables.sys_record_share ?? []).some((s) => s.record_id === 'inq_new')).toBe(true); }); }); + +describe("backfillRetiredAccessLevels (#3865 — stored 'full' normalises to 'edit')", () => { + let engine: ReturnType; + + beforeEach(() => { + engine = makeEngine(); + }); + + it("rewrites 'full' to 'edit' on both sharing rules and record shares", async () => { + engine._tables.sys_sharing_rule = [ + { id: 'rule_full', name: 'legacy', access_level: 'full', managed_by: 'package' }, + { id: 'rule_read', name: 'keep', access_level: 'read' }, + ]; + engine._tables.sys_record_share = [ + { id: 'shr_full', object_name: 'account', record_id: 'a1', access_level: 'full' }, + { id: 'shr_edit', object_name: 'account', record_id: 'a2', access_level: 'edit' }, + ]; + + const counts = await backfillRetiredAccessLevels(engine as any); + + expect(counts).toEqual({ rules: 1, shares: 1 }); + expect(engine._tables.sys_sharing_rule.map((r) => r.access_level).sort()).toEqual(['edit', 'read']); + expect(engine._tables.sys_record_share.map((r) => r.access_level)).toEqual(['edit', 'edit']); + }); + + it('leaves the provenance columns alone (a package rule must not become customized)', async () => { + // The provenance stamp hook skips isSystem writes, and this backfill uses + // one — if a seeded rule came out `customized: true`, the seeder would stop + // updating it forever (#2909 T1). + engine._tables.sys_sharing_rule = [ + { id: 'rule_full', name: 'legacy', access_level: 'full', managed_by: 'package', customized: false }, + ]; + await backfillRetiredAccessLevels(engine as any); + expect(engine._tables.sys_sharing_rule[0]).toMatchObject({ + access_level: 'edit', + managed_by: 'package', + customized: false, + }); + }); + + it('is idempotent — a second boot finds nothing to do', async () => { + engine._tables.sys_record_share = [ + { id: 'shr_full', object_name: 'account', record_id: 'a1', access_level: 'full' }, + ]; + expect(await backfillRetiredAccessLevels(engine as any)).toEqual({ rules: 0, shares: 1 }); + expect(await backfillRetiredAccessLevels(engine as any)).toEqual({ rules: 0, shares: 0 }); + }); + + it('is a no-op on a clean database', async () => { + expect(await backfillRetiredAccessLevels(engine as any)).toEqual({ rules: 0, shares: 0 }); + }); + + it('never blocks boot when the tables are missing or the driver refuses', async () => { + const broken = { + ...engine, + async find() { throw new Error('no such table: sys_sharing_rule'); }, + }; + const warn = vi.fn(); + await expect(backfillRetiredAccessLevels(broken as any, { warn })).resolves.toEqual({ rules: 0, shares: 0 }); + expect(warn).toHaveBeenCalled(); + }); + + it('stops instead of spinning when updates silently fail to stick', async () => { + // Guards the re-query loop: it selects on the very column it mutates, so an + // update that does not persist would otherwise re-select the same batch + // forever and hang boot. + engine._tables.sys_record_share = [ + { id: 'shr_full', object_name: 'account', record_id: 'a1', access_level: 'full' }, + ]; + const stuck = { + ...engine, + find: engine.find, + async update() { throw new Error('read-only replica'); }, + }; + const warn = vi.fn(); + await expect(backfillRetiredAccessLevels(stuck as any, { warn })).resolves.toEqual({ rules: 0, shares: 0 }); + expect(warn).toHaveBeenCalled(); + expect(engine._tables.sys_record_share[0].access_level).toBe('full'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/index.ts b/packages/plugins/plugin-sharing/src/index.ts index 25132b2046..602936b0d0 100644 --- a/packages/plugins/plugin-sharing/src/index.ts +++ b/packages/plugins/plugin-sharing/src/index.ts @@ -40,8 +40,16 @@ export { SharingServicePlugin, buildSharingMiddleware, backfillRuleGrants, + backfillRetiredAccessLevels, type SharingPluginOptions, } from './sharing-plugin.js'; +export { + ACCESS_LEVELS, + WRITE_ACCESS_LEVELS, + isKnownAccessLevel, + normalizeAccessLevel, + normalizeStoredAccessLevel, +} from './access-level.js'; export type { ISharingService, ISharingRuleService, diff --git a/packages/plugins/plugin-sharing/src/objects/sys-record-share.object.ts b/packages/plugins/plugin-sharing/src/objects/sys-record-share.object.ts index 53b290a745..f4ab5ae78c 100644 --- a/packages/plugins/plugin-sharing/src/objects/sys-record-share.object.ts +++ b/packages/plugins/plugin-sharing/src/objects/sys-record-share.object.ts @@ -16,7 +16,9 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; * into every `find` against that object. * - For objects with `sharingModel: 'private' | 'read'`, the same * middleware enforces edit/delete by checking ownership OR a share - * row with `access_level in ('edit','full')`. + * row with `access_level in ('edit','full')`. `full` is no longer + * authorable (#3865 — it never granted more than `edit`); the gates + * keep matching it so not-yet-normalised rows stay honoured. * * Conventions: * - `object_name` is the short object name (e.g. `account`, `lead`). @@ -154,12 +156,18 @@ export const SysRecordShare = ObjectSchema.create({ }), access_level: Field.select( - ['read', 'edit', 'full'], + // `full` ("Full Access — transfer/share/delete") was removed: no code + // path granted any of those verbs because of it — the read and write + // gates matched `access_level in ('edit','full')`, making it identical to + // `edit` while claiming more (ADR-0078 declared-but-unenforced; #3865). + // Rows persisted with `full` are normalised to `edit` (grant-time + boot + // backfill) and stay honoured by the gates until they are. + ['read', 'edit'], { label: 'Access Level', required: true, defaultValue: 'read', - description: 'What the recipient can do — read | edit | full (transfer/share/delete)', + description: 'What the recipient can do — read, or read and edit', group: 'Recipient', }, ), diff --git a/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts b/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts index 099b1dfb28..fbb76c654a 100644 --- a/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts +++ b/packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts @@ -179,11 +179,21 @@ export const SysSharingRule = ObjectSchema.create({ }), access_level: Field.select( - ['read', 'edit', 'full'], + // `full` was removed for the same reason as `queue` above: it was + // declared-but-unenforced. Labelled "Full Access (Transfer, Share, + // Delete)", it was matched only as `access_level in ('edit','full')` by + // the two enforcement sites, so it granted exactly what `edit` grants — + // offering it told an admin they had granted delete rights they had not + // (ADR-0078; #3865). Record sharing widens WHICH ROWS a principal + // reaches, never WHICH VERBS: delete/transfer come from ownership, the + // ADR-0057 DEPTH scopes, and admin scope. Stored `full` rows are + // normalised to `edit` on write and by the boot backfill. + ['read', 'edit'], { label: 'Access Level', required: true, defaultValue: 'read', + description: 'What the recipients may do with the matching records — read them, or read and edit them.', group: 'Recipient', }, ), diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 073d55491a..623fcce77c 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -75,6 +75,100 @@ export async function backfillRuleGrants( return reconciled; } +/** + * [#3865] Boot backfill: normalise stored `access_level: 'full'` rows to + * `'edit'` on `sys_sharing_rule` and `sys_record_share`. + * + * `full` was declared "Full Access (Transfer, Share, Delete)" but no code path + * granted any of those verbs because of it — both enforcement gates matched + * `access_level in ('edit','full')`, so it was byte-equivalent to `edit` + * (ADR-0078 declared-but-unenforced). Having retired it from the authoring + * surface, this closes the loop on rows already persisted. + * + * **Behaviour-preserving by construction**: the two levels were already + * equivalent, so no principal gains or loses access. Contrast the OWD + * `sharingModel: 'full'` retirement (ADR-0090 D4), which changed posture and + * therefore had to be delegated to the author rather than auto-migrated. + * + * Writes with `isSystem` so the provenance stamp hook treats this as the + * package door, not an admin edit — a package-seeded rule must NOT come out of + * this marked `customized`, or the seeder would stop updating it forever + * (#2909 T1). + * + * Best-effort and idempotent: a missing table or a failed row is logged and + * skipped, never fatal to boot, and a second run finds nothing to do. + */ +export async function backfillRetiredAccessLevels( + engine: SharingEngine, + logger?: { info?: (msg: string, meta?: any) => void; warn?: (msg: string, meta?: any) => void }, +): Promise<{ rules: number; shares: number }> { + const BATCH = 500; + const SYS = { isSystem: true, positions: [], permissions: [] } as any; + const counts = { rules: 0, shares: 0 }; + + const normalizeObject = async (object: 'sys_sharing_rule' | 'sys_record_share'): Promise => { + let migrated = 0; + // Re-query per batch rather than paginating: each pass mutates the very + // predicate it selects on, so offsets would skip rows. Bounded by a + // no-progress break so a silently-failing update can't spin forever. + for (;;) { + const rows = await engine.find(object, { + where: { access_level: 'full' }, + fields: ['id'], + limit: BATCH, + context: SYS, + }); + const batch = Array.isArray(rows) ? rows : []; + if (batch.length === 0) break; + + let updatedThisPass = 0; + for (const row of batch) { + const id = (row as any)?.id; + if (!id) continue; + try { + await engine.update(object, { id, access_level: 'edit' }, { context: SYS }); + updatedThisPass += 1; + } catch (err: any) { + logger?.warn?.('SharingServicePlugin: access-level backfill failed for row', { + object, + id, + error: err?.message, + }); + } + } + migrated += updatedThisPass; + if (updatedThisPass === 0) { + logger?.warn?.('SharingServicePlugin: access-level backfill made no progress — stopping', { + object, + remaining: batch.length, + }); + break; + } + } + return migrated; + }; + + for (const object of ['sys_sharing_rule', 'sys_record_share'] as const) { + try { + const migrated = await normalizeObject(object); + if (object === 'sys_sharing_rule') counts.rules = migrated; + else counts.shares = migrated; + } catch (err: any) { + // Table absent (plugin loaded without its objects) or driver refusing the + // filter — never fatal, the gates still honour `full` meanwhile. + logger?.warn?.('SharingServicePlugin: access-level backfill skipped', { + object, + error: err?.message, + }); + } + } + + if (counts.rules > 0 || counts.shares > 0) { + logger?.info?.("SharingServicePlugin: normalised retired access_level 'full' → 'edit'", counts); + } + return counts; +} + /** * SharingServicePlugin — registers `sys_record_share`, the `sharing` * service, and the engine middleware that enforces @@ -84,7 +178,7 @@ export async function backfillRuleGrants( * * - `sharingModel: 'private'` → reads filtered to `(owner_id == me) OR * (record explicitly shared with me)`. Writes require ownership or - * an `edit`/`full` share. + * an `edit` share. * - `sharingModel: 'public_read'` → reads unrestricted; writes gated as * above (typical "everyone can see, only owner can edit"). * - any other value (or no value) → no enforcement. This keeps @@ -115,6 +209,8 @@ export class SharingServicePlugin implements Plugin { private service?: SharingService; private ruleService?: SharingRuleService; private linkService?: ShareLinkService; + /** Resolved once in `kernel:ready`; reused by the `kernel:bootstrapped` backfills. */ + private engine?: SharingEngine; constructor(options: SharingPluginOptions = {}) { this.options = options; @@ -283,6 +379,7 @@ export class SharingServicePlugin implements Plugin { ctx.logger.warn('SharingServicePlugin: no ObjectQL engine — service NOT registered'); return; } + this.engine = engine as SharingEngine; this.service = new SharingService({ engine: engine as SharingEngine, @@ -453,6 +550,16 @@ export class SharingServicePlugin implements Plugin { // has settled — so the reconcile sees the seeded rows. Idempotent: a runtime // write that already materialized a grant is reconciled to the same state. ctx.hook('kernel:bootstrapped', async () => { + // [#3865] Normalise retired `access_level: 'full'` rows FIRST, so the + // rule reconcile below materialises grants from already-canonical rules + // (and any `full` share rows it re-grants land as `edit` in one pass + // instead of being rewritten on the next boot). + try { + if (this.engine) await backfillRetiredAccessLevels(this.engine, ctx.logger as any); + } catch (err: any) { + ctx.logger.warn('SharingServicePlugin: access-level backfill (kernel:bootstrapped) failed', { error: err?.message }); + } + if (!this.ruleService) return; try { const rules = await this.ruleService.listRules({ activeOnly: true }, { isSystem: true } as any); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index c93b03df64..3eac072315 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -11,6 +11,7 @@ import type { } from '@objectstack/spec/contracts'; import type { SharingEngine } from './sharing-service.js'; import type { SharingService } from './sharing-service.js'; +import { normalizeAccessLevel, normalizeStoredAccessLevel } from './access-level.js'; import { TeamGraphService } from './team-graph.js'; import { PositionGraphService } from './position-graph.js'; import { BusinessUnitGraphService } from './business-unit-graph.js'; @@ -50,7 +51,11 @@ function rowFromRule(row: any): SharingRuleRow { criteria: parseCriteria(row.criteria_json), recipient_type: row.recipient_type as SharingRuleRecipientType, recipient_id: row.recipient_id, - access_level: row.access_level as ShareAccessLevel, + // Projected through the normaliser, not cast: a rule row stored before + // `full` was retired (#3865) must report the level it actually enforces. + // This also makes reconciliation self-healing — a `full` rule now differs + // from its `full` share rows, so the next pass re-grants them as `edit`. + access_level: normalizeStoredAccessLevel(row.access_level), active: row.active !== false, managed_by: row.managed_by ?? null, customized: row.customized === true, @@ -93,7 +98,9 @@ export class SharingRuleService implements ISharingRuleService { const orgId = (context as any)?.organizationId ?? (context as any)?.tenantId ?? null; const now = new Date().toISOString(); - const accessLevel: ShareAccessLevel = input.accessLevel ?? 'read'; + // Authoring path — `full` normalises to `edit`, anything unrecognised is a + // loud VALIDATION_FAILED alongside the required-field checks above (#3865). + const accessLevel: ShareAccessLevel = normalizeAccessLevel(input.accessLevel, 'read'); const active = input.active !== false; const criteriaJson = input.criteria == null ? null diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 1fb0050a34..cb7caa117c 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -269,6 +269,19 @@ describe('SharingService.canEdit', () => { expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); }); + // [#3865] Enforcement stays deliberately WIDER than authoring: `full` is no + // longer grantable, but a row persisted before the boot backfill ran still + // means `edit`. Dropping it from the gate would silently REVOKE access. + it("still honours a stored 'full' share written before it was retired", async () => { + engine._tables.sys_record_share = [ + { + id: 'shr_legacy', object_name: 'account', record_id: 'a1', + recipient_type: 'user', recipient_id: 'bob', access_level: 'full', + }, + ]; + expect(await svc.canEdit('account', 'a1', { userId: 'bob' })).toBe(true); + }); + it('enforces canEdit for sharingModel=read', async () => { expect(await svc.canEdit('lead', 'l1', { userId: 'alice' })).toBe(true); expect(await svc.canEdit('lead', 'l1', { userId: 'bob' })).toBe(false); @@ -305,12 +318,39 @@ describe('SharingService.grant / listShares / revoke', () => { it('upserts on second call with same (object, record, recipient)', async () => { const a = await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob' }, { userId: 'admin' }); - const b = await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'full' }, { userId: 'admin' }); + const b = await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { userId: 'admin' }); expect(engine._tables.sys_record_share.length).toBe(1); expect(b.id).toBe(a.id); - expect(b.access_level).toBe('full'); + expect(b.access_level).toBe('edit'); + }); + + // [#3865] `full` claimed "Full Access (Transfer, Share, Delete)" while both + // gates matched `edit`/`full` alike — it granted exactly `edit`. Retired from + // the authoring surface; an older client still sending it is normalised + // rather than rejected (that would revoke access it already had). + it("normalises the retired 'full' level to 'edit' instead of persisting it", async () => { + const r = await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'full' as any }, + { userId: 'admin' }, + ); + expect(r.access_level).toBe('edit'); + expect(engine._tables.sys_record_share[0].access_level).toBe('edit'); }); + it('rejects an unrecognised access level instead of silently persisting it', async () => { + // A level no gate matches is a grant that looks issued and enforces + // nothing — the same declared-but-inert trap `full` was (ADR-0078). + await expect( + svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'admin' as any }, + { userId: 'admin' }, + ), + ).rejects.toThrow(/VALIDATION_FAILED/); + // Rejected before the upsert even queries — the table is never touched. + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + }); + + it('listShares returns all grants on a record', async () => { await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob' }, { userId: 'admin' }); await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'carol', accessLevel: 'edit' }, { userId: 'admin' }); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 8b9f733740..6643b4d06f 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -8,6 +8,7 @@ import type { SharingExecutionContext, ShareAccessLevel, } from '@objectstack/spec/contracts'; +import { WRITE_ACCESS_LEVELS, normalizeAccessLevel } from './access-level.js'; /** * Shape of the data engine the service actually needs. Kept narrow so @@ -188,9 +189,9 @@ export class SharingService implements ISharingService { * restriction applies (system/bypass, public objects, no owner field). * * Editable-set = owner-match (widened by write DEPTH) OR records shared to - * the caller at `edit`/`full`. Unlike reads, this applies to BOTH `private` - * and `read` (public_read) models — public_read is read-open but write-owned; - * only a fully `public` object is write-open. + * the caller at a {@link WRITE_ACCESS_LEVELS} level. Unlike reads, this + * applies to BOTH `private` and `read` (public_read) models — public_read is + * read-open but write-owned; only a fully `public` object is write-open. */ async buildWriteFilter( object: string, @@ -220,7 +221,7 @@ export class SharingService implements ISharingService { object_name: object, recipient_type: 'user', recipient_id: context.userId, - access_level: { $in: ['edit', 'full'] }, + access_level: { $in: [...WRITE_ACCESS_LEVELS] }, }, fields: ['record_id'], limit: 5000, @@ -268,14 +269,14 @@ export class SharingService implements ISharingService { if (owners.includes(String(owner))) return true; } - // 2) Explicit edit / full share. + // 2) Explicit write-level share (`edit`, plus not-yet-normalised `full`). const editGrants = await this.engine.find('sys_record_share', { where: { object_name: object, record_id: recordId, recipient_type: 'user', recipient_id: context.userId, - access_level: { $in: ['edit', 'full'] }, + access_level: { $in: [...WRITE_ACCESS_LEVELS] }, }, fields: ['id'], limit: 1, @@ -297,7 +298,12 @@ export class SharingService implements ISharingService { if (!input.recipientId) throw new Error('VALIDATION_FAILED: recipientId is required'); const recipientType = input.recipientType ?? 'user'; - const accessLevel: ShareAccessLevel = input.accessLevel ?? 'read'; + // 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` + // normalises to `edit` (what it always meant); everything unrecognised is a + // loud VALIDATION_FAILED the REST layer maps to 400. + const accessLevel: ShareAccessLevel = normalizeAccessLevel(input.accessLevel, 'read'); const source = input.source ?? 'manual'; // Upsert: if a row with same (object, record, recipient) exists, diff --git a/packages/plugins/plugin-sharing/src/translations/en.objects.generated.ts b/packages/plugins/plugin-sharing/src/translations/en.objects.generated.ts index cfd76042a0..9120f5ba38 100644 --- a/packages/plugins/plugin-sharing/src/translations/en.objects.generated.ts +++ b/packages/plugins/plugin-sharing/src/translations/en.objects.generated.ts @@ -42,11 +42,10 @@ export const enObjects: NonNullable = { }, access_level: { label: "Access Level", - help: "What the recipient can do — read | edit | full (transfer/share/delete)", + help: "What the recipient can do — read, or read and edit", options: { read: "read", - edit: "edit", - full: "full" + edit: "edit" } }, source: { @@ -146,10 +145,10 @@ export const enObjects: NonNullable = { }, access_level: { label: "Access Level", + help: "What the recipients may do with the matching records — read them, or read and edit them.", options: { read: "read", - edit: "edit", - full: "full" + edit: "edit" } }, active: { diff --git a/packages/plugins/plugin-sharing/src/translations/es-ES.objects.generated.ts b/packages/plugins/plugin-sharing/src/translations/es-ES.objects.generated.ts index 17dd86dee6..5ac01f3783 100644 --- a/packages/plugins/plugin-sharing/src/translations/es-ES.objects.generated.ts +++ b/packages/plugins/plugin-sharing/src/translations/es-ES.objects.generated.ts @@ -42,11 +42,10 @@ export const esESObjects: NonNullable = { }, access_level: { label: "Nivel de acceso", - help: "Lo que puede hacer el destinatario: read | edit | full (transfer/share/delete).", + help: "Lo que puede hacer el destinatario: leer, o leer y editar.", options: { read: "Leer", - edit: "Editar", - full: "Total" + edit: "Editar" } }, source: { @@ -146,10 +145,10 @@ export const esESObjects: NonNullable = { }, access_level: { label: "Nivel de acceso", + help: "Lo que los destinatarios pueden hacer con los registros coincidentes: leerlos, o leerlos y editarlos.", options: { read: "Leer", - edit: "Editar", - full: "Total" + edit: "Editar" } }, active: { diff --git a/packages/plugins/plugin-sharing/src/translations/ja-JP.objects.generated.ts b/packages/plugins/plugin-sharing/src/translations/ja-JP.objects.generated.ts index 1d4cb41e9c..e9a38f4864 100644 --- a/packages/plugins/plugin-sharing/src/translations/ja-JP.objects.generated.ts +++ b/packages/plugins/plugin-sharing/src/translations/ja-JP.objects.generated.ts @@ -42,11 +42,10 @@ export const jaJPObjects: NonNullable = { }, access_level: { label: "アクセスレベル", - help: "受信者に許可される操作 — read | edit | full（転送/共有/削除）", + help: "受信者に許可される操作 — 閲覧、または閲覧と編集", options: { read: "閲覧", - edit: "編集", - full: "フルアクセス" + edit: "編集" } }, source: { @@ -146,10 +145,10 @@ export const jaJPObjects: NonNullable = { }, access_level: { label: "アクセスレベル", + help: "対象レコードに対して受信者に許可される操作 — 閲覧、または閲覧と編集。", options: { read: "閲覧", - edit: "編集", - full: "フルアクセス" + edit: "編集" } }, active: { diff --git a/packages/plugins/plugin-sharing/src/translations/zh-CN.objects.generated.ts b/packages/plugins/plugin-sharing/src/translations/zh-CN.objects.generated.ts index ba201682c4..5721f54b94 100644 --- a/packages/plugins/plugin-sharing/src/translations/zh-CN.objects.generated.ts +++ b/packages/plugins/plugin-sharing/src/translations/zh-CN.objects.generated.ts @@ -42,11 +42,10 @@ export const zhCNObjects: NonNullable = { }, access_level: { label: "访问级别", - help: "接收方可以执行的操作——read | edit | full（转移 / 共享 / 删除）", + help: "接收方可以执行的操作——读取，或读取并编辑", options: { read: "读取", - edit: "编辑", - full: "完全访问" + edit: "编辑" } }, source: { @@ -146,10 +145,10 @@ export const zhCNObjects: NonNullable = { }, access_level: { label: "访问级别", + help: "接收方可以对匹配到的记录执行的操作——读取，或读取并编辑。", options: { read: "读取", - edit: "编辑", - full: "完全访问" + edit: "编辑" } }, active: { diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 97f07f087e..d687928f87 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -31,8 +31,18 @@ export type ShareRecipientType = | 'unit_and_subordinates' | 'guest'; -/** Access level on a single record. */ -export type ShareAccessLevel = 'read' | 'edit' | 'full'; +/** + * Access level on a single record — the authorable subset, mirroring + * `SharingLevel` in spec/security. + * + * `full` ("Full Access (Transfer, Share, Delete)") was REMOVED: no code path + * ever granted transfer / re-share / delete because of it, so it was + * byte-equivalent to `edit` while telling admins otherwise (ADR-0078; #3865). + * Stored `'full'` rows are normalised to `'edit'` by the sharing plugin + * (grant-time + a boot backfill) and remain honoured by the read/write gates + * meanwhile, so widening this type back is never needed for compatibility. + */ +export type ShareAccessLevel = 'read' | 'edit'; /** Why a share row exists (used by the rule evaluator to reconcile). */ export type ShareSource = 'manual' | 'rule' | 'team' | 'inherited'; diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index a48b07403a..6923b472c0 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -607,6 +607,67 @@ const pageComponentVisibilityToVisibleWhen: MetadataConversion = { }, }; +/** + * Sharing-rule `accessLevel: 'full'` → `'edit'` (protocol 16, #3865). + * + * `full` was documented as "Full Access (Transfer, Share, Delete)" but no code + * path ever granted transfer, re-share, or delete because of it: both + * enforcement sites matched `access_level in ('edit','full')`, so it behaved as + * `edit` while telling admins it granted more (ADR-0078 declared-but-unenforced; + * ADR-0049). It was removed from `SharingLevel`, which makes this rewrite + * strictly **lossless** — unlike the OWD `sharingModel: 'full'` alias, which had + * no equivalent target and was delegated to a step-13 semantic TODO. Here the + * old and new shapes are already behaviourally identical, so the loader can + * convert with zero consumer action. + * + * **Live window**: the protocol-16 loader accepts the deprecated value so a + * stack still authoring `'full'` keeps loading (the zod enum now rejects it at + * parse, and this entry runs at `normalizeStackInput` *before* that). The + * runtime counterpart for already-persisted rows lives in `plugin-sharing` + * (grant-time normalisation + a boot backfill over `sys_sharing_rule` / + * `sys_record_share`). + */ +const sharingRuleAccessLevelFullToEdit: MetadataConversion = { + id: 'sharing-rule-access-level-full-to-edit', + toMajor: 16, + surface: 'sharingRule.accessLevel', + summary: "sharing-rule accessLevel 'full' → 'edit' (#3865 — `full` never granted more than `edit`)", + apply(stack, emit) { + return mapCollection(stack, 'sharingRules', (rule, path) => { + if (rule.accessLevel !== 'full') return rule; + emit({ from: 'full', to: 'edit', path: `${path}.accessLevel` }); + return { ...rule, accessLevel: 'edit' }; + }); + }, + fixture: { + before: { + sharingRules: [ + { + name: 'share_open_deals', + type: 'criteria', + object: 'crm_deal', + accessLevel: 'full', + condition: 'record.status == "open"', + sharedWith: { type: 'business_unit', value: 'bu_sales' }, + }, + ], + }, + after: { + sharingRules: [ + { + name: 'share_open_deals', + type: 'criteria', + object: 'crm_deal', + accessLevel: 'edit', + condition: 'record.status == "open"', + sharedWith: { type: 'business_unit', value: 'bu_sales' }, + }, + ], + }, + expectedNotices: 1, + }, +}; + /** * All conversions, keyed by the protocol major that introduced the canonical * shape. Newest majors last; ordering within a major is application order. @@ -616,6 +677,7 @@ export const CONVERSIONS_BY_MAJOR: Readonly z.object({ /** Identifier of the concrete rule/share/policy (sharing-rule name, policy id, share row id). */ name: z.string().describe('Stable identifier of the concrete rule, share, or policy that was evaluated.'), /** - * For sharing sources: the access level this rule grants on the record - * (mirrors `SharingLevel` — read/edit/full). + * For sharing sources: the access level this rule grants on the record. + * + * Deliberately WIDER than the authorable `SharingLevel` (`read`/`edit`): this + * is a REPORTING surface over rows that already exist, and a not-yet-migrated + * `'full'` row (removed as authorable in #3865, normalised to `'edit'` by the + * sharing plugin's boot backfill) must still be explainable. Reporting a + * stored value must never throw — an explain panel that crashes on legacy + * data is worse than one that shows the legacy label. Do NOT copy this + * widening onto an authoring surface. */ grants: z.enum(['read', 'edit', 'full']).optional() - .describe('Access level a sharing source grants on the record (mirrors SharingLevel).'), + .describe('Access level a sharing source grants on the record (authorable: read/edit; `full` appears only for legacy rows pending normalisation).'), /** How the rule reached the principal (e.g. `group:sales_team`, `position:approver`, `owner`, `criteria: status == open`). */ via: z.string().optional() .describe('How the rule reached the principal — recipient group/position, ownership, or the matching criteria.'), diff --git a/packages/spec/src/security/sharing.test.ts b/packages/spec/src/security/sharing.test.ts index 5be0a59593..0ca947c888 100644 --- a/packages/spec/src/security/sharing.test.ts +++ b/packages/spec/src/security/sharing.test.ts @@ -43,7 +43,7 @@ describe('ShareRecipientType', () => { describe('SharingLevel', () => { it('should accept valid sharing levels', () => { - const validLevels = ['read', 'edit', 'full']; + const validLevels = ['read', 'edit']; validLevels.forEach(level => { expect(() => SharingLevel.parse(level)).not.toThrow(); @@ -54,6 +54,17 @@ describe('SharingLevel', () => { expect(() => SharingLevel.parse('write')).toThrow(); expect(() => SharingLevel.parse('delete')).toThrow(); }); + + it("rejects the retired 'full' level", () => { + // [#3865] `full` was declared "Full Access (Transfer, Share, Delete)" but + // no code path granted any of those verbs — both gates matched + // `edit`/`full` alike, so it was byte-equivalent to `edit` while telling + // admins otherwise. Declared-but-unenforced (ADR-0078 / ADR-0049), same + // reason `queue` and `guest` are rejected above. Stacks still authoring it + // are rewritten at load by the `sharing-rule-access-level-full-to-edit` + // conversion, so this rejection is reachable only via a direct parse. + expect(() => SharingLevel.parse('full')).toThrow(); + }); }); describe('OWDModel', () => { @@ -229,7 +240,7 @@ describe('SharingRuleSchema', () => { }); it('should accept different access levels', () => { - const levels: Array = ['read', 'edit', 'full']; + const levels: Array = ['read', 'edit']; levels.forEach(level => { const rule = SharingRuleSchema.parse({ diff --git a/packages/spec/src/security/sharing.zod.ts b/packages/spec/src/security/sharing.zod.ts index d4bed38d60..275e408b6f 100644 --- a/packages/spec/src/security/sharing.zod.ts +++ b/packages/spec/src/security/sharing.zod.ts @@ -35,11 +35,33 @@ export const SharingRuleType = z.enum([ /** * Sharing Level * What access is granted? + * + * Both members map onto an enforced runtime behaviour: `read` widens the read + * filter (`buildReadFilter`), `edit` additionally opens the write gate + * (`buildWriteFilter` / `canEdit`). + * + * Removed (never enforced): `full`, documented as "Full Access (Transfer, + * Share, Delete)". NO code path granted transfer, re-share, or delete because + * of it — both enforcement sites matched `access_level in ('edit','full')`, + * making it byte-equivalent to `edit`. An admin picking it in Setup was told + * they had granted delete rights and had not; a level that validates and then + * silently does nothing is an authoring trap (ADR-0078, ADR-0049) — the same + * reason `ShareRecipientType` below dropped `queue` / `guest`. + * + * This is also where the industry model lands: record sharing widens WHICH + * ROWS a principal reaches, never WHICH VERBS they may use. Salesforce sharing + * rules stop at Read-Only / Read-Write (its Full Access is owner / hierarchy / + * Modify All only, never grantable by a rule); Dataverse AND-s any shared + * access right against the security role's own privilege. Delete and transfer + * belong to ownership, the hierarchy DEPTH scopes (ADR-0057), and admin scope + * — not to a sharing level. Re-share additionally presupposes a + * share-administration model that does not exist yet. Reviving `full` means + * designing a capability mask AND-ed with object CRUD, not re-adding an enum + * member (#3865). */ export const SharingLevel = z.enum([ 'read', // Read Only 'edit', // Read / Write - 'full' // Full Access (Transfer, Share, Delete) ]); /** From 338aef804b1ceda7a0285439787ed05ff5b152f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 15:43:55 +0000 Subject: [PATCH 2/2] docs(spec): regenerate the security reference tables for the narrowed sharing level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `content/docs/references/` is generated from the zod schemas and is checked in CI (`@objectstack/spec check:docs`); narrowing `SharingLevel` left it stale. - `security/sharing.mdx` — `accessLevel` is now `read | edit` and the enum member list drops `full`. - `security/explain.mdx` — `grants` deliberately still lists `full`; only its description changes, since explain reports stored rows and a legacy row must stay explainable until the boot backfill normalises it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017346r3TMNqpbTLkT5d49uQ --- content/docs/references/security/explain.mdx | 2 +- content/docs/references/security/sharing.mdx | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/content/docs/references/security/explain.mdx b/content/docs/references/security/explain.mdx index 9f492fd7ec..eba197c611 100644 --- a/content/docs/references/security/explain.mdx +++ b/content/docs/references/security/explain.mdx @@ -153,7 +153,7 @@ ADR-0095 D2 posture rung — PLATFORM_ADMIN crosses the tenant wall where object | :--- | :--- | :--- | :--- | | **kind** | `Enum<'tenant_filter' \| 'owd_baseline' \| 'ownership' \| 'record_share' \| 'sharing_rule' \| 'team' \| 'territory' \| 'rls_policy'>` | ✅ | The row-visibility source kind evaluated for this record at this layer. | | **name** | `string` | ✅ | Stable identifier of the concrete rule, share, or policy that was evaluated. | -| **grants** | `Enum<'read' \| 'edit' \| 'full'>` | optional | Access level a sharing source grants on the record (mirrors SharingLevel). | +| **grants** | `Enum<'read' \| 'edit' \| 'full'>` | optional | Access level a sharing source grants on the record (authorable: read/edit; `full` appears only for legacy rows pending normalisation). | | **via** | `string` | optional | How the rule reached the principal — recipient group/position, ownership, or the matching criteria. | | **predicate** | `any` | optional | The row predicate this rule contributed, when it is filter-shaped (null = unrestricted). | | **effect** | `Enum<'admits' \| 'excludes' \| 'neutral'>` | ✅ | The rule's effect on THIS record: admits, excludes, or neutral. | diff --git a/content/docs/references/security/sharing.mdx b/content/docs/references/security/sharing.mdx index b8bd38e65d..a03e75d3c5 100644 --- a/content/docs/references/security/sharing.mdx +++ b/content/docs/references/security/sharing.mdx @@ -36,7 +36,7 @@ const result = CriteriaSharingRule.parse(data); | **description** | `string` | optional | Administrative notes | | **object** | `string` | ✅ | Target Object Name | | **active** | `boolean` | optional | | -| **accessLevel** | `Enum<'read' \| 'edit' \| 'full'>` | optional | | +| **accessLevel** | `Enum<'read' \| 'edit'>` | optional | | | **sharedWith** | `{ type: Enum<'user' \| 'team' \| 'position' \| 'unit_and_subordinates' \| 'business_unit'>; value: string }` | ✅ | The recipient of the shared access | | **type** | `'criteria'` | ✅ | | | **condition** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | ✅ | Predicate (CEL). e.g. P`record.department == "Sales"` | @@ -75,7 +75,6 @@ const result = CriteriaSharingRule.parse(data); * `read` * `edit` -* `full` --- @@ -91,7 +90,7 @@ const result = CriteriaSharingRule.parse(data); | **description** | `string` | optional | Administrative notes | | **object** | `string` | ✅ | Target Object Name | | **active** | `boolean` | optional | | -| **accessLevel** | `Enum<'read' \| 'edit' \| 'full'>` | optional | | +| **accessLevel** | `Enum<'read' \| 'edit'>` | optional | | | **sharedWith** | `{ type: Enum<'user' \| 'team' \| 'position' \| 'unit_and_subordinates' \| 'business_unit'>; value: string }` | ✅ | The recipient of the shared access | | **type** | `'criteria'` | ✅ | | | **condition** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | ✅ | Predicate (CEL). e.g. P`record.department == "Sales"` |