From 60c9392485f3c404fe60cbc960535ba73f77d164 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 04:43:50 +0000 Subject: [PATCH] =?UTF-8?q?feat(sharing):=20ISharingService=20=E7=9A=84?= =?UTF-8?q?=E6=AF=8F=E8=A1=8C=E5=86=99=E5=88=A4=E5=AE=9A=E8=A1=A5=E4=B8=89?= =?UTF-8?q?=E6=80=81(=E6=94=BE=E8=A1=8C/=E4=B8=8D=E8=A1=A8=E6=80=81/?= =?UTF-8?q?=E6=8B=92=E7=BB=9D)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5492 维护者裁决 B 案的 step 1:契约与默认实现同 PR,防「声明了没人实现」窗口。 plugin-security 前像门的 provenance 分层合成是 step 2,本次一行未动。 - packages/spec/src/contracts/sharing-service.ts - SharingWriteVerdict = 'allow' | 'abstain' | 'deny'(普通 TS 类型,非 zod 派生) - ISharingService.checkEdit() / checkDelete():三态主形态,动作边界照 ADR-0111 D3 继承(edit 共享 → update 放行、delete 仍拒),两者 abstain 集合相同 - canEdit() / canDelete() 保留并被定义为投影 `verdict !== 'deny'`,真值表零漂移 - packages/plugins/plugin-sharing/src/sharing-service.ts - 三态实现;shouldBypass 的两个理由拆开:isSystem → allow,bypass 名单 → abstain - writeGateFailClosed():查询失败一律 deny(永不 abstain)并 logger.error 记名 - 测试:spec 侧编译期 + 散文 pin;plugin 侧三态逐分支、E2 无 owner_id 形状、 fail-closed 两条、投影不漂移的 9 分支真值表(含反空转) - api-surface/contracts.json 随新导出重生成 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .changeset/sharing-write-verdict-tristate.md | 45 ++++ .../src/sharing-service.test.ts | 239 ++++++++++++++++++ .../plugin-sharing/src/sharing-service.ts | 221 ++++++++++++---- packages/spec/api-surface/contracts.json | 1 + .../src/contracts/sharing-service.test.ts | 122 ++++++++- .../spec/src/contracts/sharing-service.ts | 122 ++++++++- 6 files changed, 688 insertions(+), 62 deletions(-) create mode 100644 .changeset/sharing-write-verdict-tristate.md diff --git a/.changeset/sharing-write-verdict-tristate.md b/.changeset/sharing-write-verdict-tristate.md new file mode 100644 index 0000000000..d420cdab3c --- /dev/null +++ b/.changeset/sharing-write-verdict-tristate.md @@ -0,0 +1,45 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-sharing": minor +--- + +feat(sharing): `ISharingService` 的每行写判定补三态 —— 放行 / 不表态 / 拒绝(#6428) + +#5492 的维护者裁决(2026-08-07,B 案)分两步兑现两种已声明的写扩权,本次是 **step 1: +契约与默认实现**。plugin-security 前像门的 provenance 分层合成是 step 2,本次一行未动。 + +**为什么二态不够(实测,不是推演)。** `canEdit()` 用同一个 `true` 表达了两件事 —— +「我有依据放行」与「本服务对这一行根本不设门」。对只**追加**一道门的调用方(sharing +中间件、`sys_attachment` 父记录门、ADR-0055 master 判定)这没问题:`true` = 「我不拦 +你」。对让这个答案去**顶替另一个权威的地板**的调用方就是 fail-open —— #5492 的 E2 实验 +把前像写门委托给 `canEdit()` 后,在**没有 `owner_id` 列**的对象上,普通成员跨 creator +的 UPDATE 变成 `ok: true`(main 上是 403),因为平台的 `created_by` 所有权地板正是这类 +对象唯一的行级写门,而一个「不表态」的 `true` 把它盖掉了。 + +**新增契约面**(`@objectstack/spec/contracts`): + +- `SharingWriteVerdict = 'allow' | 'abstain' | 'deny'` —— 闭合联合,普通 TS 类型 + (非 zod 派生,不进 ADR-0122 的 pin 计数)。 +- `ISharingService.checkEdit()` / `checkDelete()` —— 三态主形态,动作边界照 ADR-0111 D3 + 继承:`edit` 级共享让 `checkEdit` 答 `allow`、同一行 `checkDelete` 仍答 `deny`;两者 + 的 `abstain` 集合完全相同(两道门对「哪些对象由共享设门」意见一致,只在动词上分歧)。 + +**兼容:`canEdit()` / `canDelete()` 原样保留,语义零漂移。** 它们被定义为三态的 +**投影** `verdict !== 'deny'` —— 从前对 public / 无 owner 字段 / bypass 对象返回的那个 +`true`,现在落在 `abstain` 上,投影回来仍是 `true`。真值表逐分支被测试钉住(9 个分支 +× 两个动词),因为 `resolveSharingCanEdit`(plugin-security)与 `sys_attachment` 父记录 +门读的正是这一列,翻掉任何一格都是本 PR 未触及的包里的静默权限变更。 + +**fail-closed 落点:查询失败是 `deny`,永远不是 `abstain`。** 两者对合成方是相反的指令 +(`abstain` 把这一行交给另一个权威,`deny` 就地终结),把失败读成「没有意见」正是造出上述 +fail-open 的那个混淆。默认实现把所有权查询与共享查询整段包在 fail-closed 分支里,并 +`logger.error` 记名,不静默吞。 + +**行为变化(一处,方向收紧)**:引擎查询抛错时,`canEdit`/`canDelete` 从**向外抛**改为 +返回 `false`。两个既有调用点本来就在自己那侧 catch 成 `false`(`resolveSharingCanEdit` +的 #5386 fail-closed、attachment hook 的降级读),所以对它们是同一结果;其余调用点由 +「异常中止写入」变成「403 拒绝写入」,严格不更宽松。 + +**解锁**:#5492 step 2 的前像门可以按 provenance 分层合成 —— `abstain` 回落平台所有权 +地板、`allow` 按声明顶替地板、`deny` 维持拒绝 —— 而不必在 security 侧重算一份 +owner/depth/share/bypass(那会是同一契约的第二份实现)。#5491 与 #5492 同批落地。 diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 02afb8dd49..098bfd46c5 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -1452,3 +1452,242 @@ describe('[#5859] resolveOwnerScopeIds fills the AUTHORITATIVE organization', () expect(seen).toHaveLength(0); }); }); + +// ───────────────────────────────────────────────────────────────────── +// [#6428] Tri-state write verdicts (#5492 ruling B, step 1). +// +// `canEdit` answered ONE `true` for two different facts — "I permit this +// write" and "record sharing does not enforce on this row at all". #5492's E2 +// experiment delegated a pre-image write gate to it and measured the cost: on +// an object with NO `owner_id` column, an ordinary member's cross-creator +// UPDATE came back `ok: true` where `main` answers 403, because the platform's +// `created_by` ownership floor is that object's only row-level write gate and +// an abstaining `true` overrode it. +// +// `checkEdit` / `checkDelete` separate the two; `canEdit` / `canDelete` stay +// exactly as they were (`verdict !== 'deny'`), which the parity block below +// pins branch by branch so a two-state caller cannot drift under this change. +// ───────────────────────────────────────────────────────────────────── + +describe('[#6428] SharingService.checkEdit / checkDelete — allow / abstain / deny', () => { + let engine: ReturnType; + let svc: SharingService; + + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, // private + owner_id → sharing enforces + whiteboard: CANON_PUBLIC_RW_SCHEMA, // public_read_write → sharing does not + note: ORPHAN_SCHEMA, // private, NO owner_id → the E2 shape + sys_record_share: { name: 'sys_record_share' }, + }); + svc = new SharingService({ engine }); + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'alice' }]; + engine._tables.whiteboard = [{ id: 'w1', name: 'Board', owner_id: 'alice' }]; + engine._tables.note = [{ id: 'n1', body: 'an object with no owner column' }]; + }); + + it('allow — the record owner, on both verbs', async () => { + expect(await svc.checkEdit('account', 'a1', { userId: 'alice' })).toBe('allow'); + expect(await svc.checkDelete('account', 'a1', { userId: 'alice' })).toBe('allow'); + }); + + it('deny — a stranger on a sharing-enforced object', async () => { + expect(await svc.checkEdit('account', 'a1', { userId: 'bob' })).toBe('deny'); + expect(await svc.checkDelete('account', 'a1', { userId: 'bob' })).toBe('deny'); + }); + + it('deny — a principal-less context (no userId is not "no opinion")', async () => { + expect(await svc.checkEdit('account', 'a1', {})).toBe('deny'); + expect(await svc.checkDelete('account', 'a1', {})).toBe('deny'); + }); + + it('deny — a read-level share', async () => { + await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'read' }, + { isSystem: true }, + ); + expect(await svc.checkEdit('account', 'a1', { userId: 'bob' })).toBe('deny'); + }); + + it('[ADR-0111 D3] an edit share is allow on update and DENY on delete — the verb boundary survives the tri-state', async () => { + await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, + { isSystem: true }, + ); + expect(await svc.checkEdit('account', 'a1', { userId: 'bob' })).toBe('allow'); + // NOT `abstain`: the delete gate has a real opinion here and it is "no". + // An abstain would hand the row to a caller's fallback and let an edit + // share leak the delete verb through the back door. + expect(await svc.checkDelete('account', 'a1', { userId: 'bob' })).toBe('deny'); + }); + + it('[#5492 E2] abstain — an object with NO owner_id column, where the boolean said `true`', async () => { + // THE pin this card exists for. The verdict is "I do not enforce here", + // so #5492 step 2 can keep the platform `created_by` floor in force… + expect(await svc.checkEdit('note', 'n1', { userId: 'bob' })).toBe('abstain'); + expect(await svc.checkDelete('note', 'n1', { userId: 'bob' })).toBe('abstain'); + // …while the two-state projection every current caller reads is unchanged. + expect(await svc.canEdit('note', 'n1', { userId: 'bob' })).toBe(true); + expect(await svc.canDelete('note', 'n1', { userId: 'bob' })).toBe(true); + }); + + it('abstain — a public object, and an object this engine has no schema for', async () => { + expect(await svc.checkEdit('whiteboard', 'w1', { userId: 'bob' })).toBe('abstain'); + expect(await svc.checkDelete('whiteboard', 'w1', { userId: 'bob' })).toBe('abstain'); + expect(await svc.checkEdit('ghost', 'g1', { userId: 'bob' })).toBe('abstain'); + }); + + it('the two bypass reasons split: a system context ALLOWS, a bypass-listed object ABSTAINS', async () => { + // Merging these was the ambiguity in miniature. A platform-internal writer + // is positively permitted; `sys_user` is merely not sharing-enforced, and + // saying `allow` there would invite a composing caller to skip the gate + // that actually guards those tables. + expect(await svc.checkEdit('account', 'a1', { isSystem: true })).toBe('allow'); + expect(await svc.checkDelete('account', 'a1', { isSystem: true })).toBe('allow'); + expect(await svc.checkEdit('sys_user', 'u1', { userId: 'bob' })).toBe('abstain'); + expect(await svc.checkDelete('sys_user', 'u1', { userId: 'bob' })).toBe('abstain'); + }); + + it('[#4647] allow — Modify All Data, still asked LAST', async () => { + let probeCalls = 0; + const withBypass = new SharingService({ + engine, + securityService: () => ({ + hasWriteBypass: async () => { probeCalls++; return true; }, + }), + }); + expect(await withBypass.checkEdit('account', 'a1', { userId: 'bob' })).toBe('allow'); + expect(await withBypass.checkDelete('account', 'a1', { userId: 'bob' })).toBe('allow'); + expect(probeCalls).toBe(2); + // The owner never pays for the probe: ownership answers first. + probeCalls = 0; + expect(await withBypass.checkEdit('account', 'a1', { userId: 'alice' })).toBe('allow'); + expect(probeCalls).toBe(0); + }); +}); + +describe('[#6428] fail-closed: an unresolvable verdict is DENY, never abstain', () => { + let engine: ReturnType; + let logged: any[]; + let svc: SharingService; + + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + logged = []; + svc = new SharingService({ + engine, + logger: { error: (...args: any[]) => { logged.push(args); } }, + }); + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'alice' }]; + }); + + it('a throwing ownership lookup denies — and says so in the log', async () => { + // Non-vacuity: this caller is ALLOWED while the engine works. + expect(await svc.checkEdit('account', 'a1', { userId: 'alice' })).toBe('allow'); + + engine.find = async () => { throw new Error('engine down'); }; + + // `abstain` here would be the fail-open: it tells a composing caller + // "nobody objects", on a row this service is supposed to be guarding. + expect(await svc.checkEdit('account', 'a1', { userId: 'alice' })).toBe('deny'); + expect(await svc.checkDelete('account', 'a1', { userId: 'alice' })).toBe('deny'); + // The boolean projection converges with it: a denial, not a thrown 500. + expect(await svc.canEdit('account', 'a1', { userId: 'alice' })).toBe(false); + expect(await svc.canDelete('account', 'a1', { userId: 'alice' })).toBe(false); + + expect(logged.length).toBeGreaterThan(0); + expect(String(logged[0][0])).toContain('fail-closed'); + expect(String(logged[0][0])).toContain('#6428'); + }); + + it('a throwing SHARE lookup denies too — the whole evaluation is covered, not just the first query', async () => { + await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, + { isSystem: true }, + ); + // Non-vacuity: the share is what makes this caller `allow`… + expect(await svc.checkEdit('account', 'a1', { userId: 'bob' })).toBe('allow'); + + const realFind = engine.find.bind(engine); + engine.find = async (object: string, options?: any) => { + if (object === 'sys_record_share') throw new Error('share table unavailable'); + return realFind(object, options); + }; + + // …so losing exactly that lookup must refuse, never fall back to "I have + // no opinion about a row I was gating a second ago". + expect(await svc.checkEdit('account', 'a1', { userId: 'bob' })).toBe('deny'); + }); +}); + +describe('[#6428] the boolean projection does not drift (compatibility clause)', () => { + let engine: ReturnType; + let svc: SharingService; + + beforeEach(async () => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + whiteboard: CANON_PUBLIC_RW_SCHEMA, + note: ORPHAN_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + svc = new SharingService({ engine }); + engine._tables.account = [{ id: 'a1', name: 'Acme', owner_id: 'alice' }]; + engine._tables.whiteboard = [{ id: 'w1', name: 'Board', owner_id: 'alice' }]; + engine._tables.note = [{ id: 'n1', body: 'no owner column' }]; + await svc.grant( + { object: 'account', recordId: 'a1', recipientId: 'carol', accessLevel: 'edit' }, + { isSystem: true }, + ); + }); + + // Every branch of both gates, with the boolean answer `main` gives today. + // `resolveSharingCanEdit` (plugin-security :3484) and the `sys_attachment` + // parent gate read exactly this column, so a single flipped cell here is a + // silent enforcement change in packages this PR does not touch. + const CASES: Array<{ + what: string; + object: string; + id: string; + ctx: any; + edit: boolean; + del: boolean; + }> = [ + { what: 'owner', object: 'account', id: 'a1', ctx: { userId: 'alice' }, edit: true, del: true }, + { what: 'stranger', object: 'account', id: 'a1', ctx: { userId: 'bob' }, edit: false, del: false }, + { what: 'edit-share holder', object: 'account', id: 'a1', ctx: { userId: 'carol' }, edit: true, del: false }, + { what: 'no principal', object: 'account', id: 'a1', ctx: {}, edit: false, del: false }, + { what: 'system context', object: 'account', id: 'a1', ctx: { isSystem: true }, edit: true, del: true }, + { what: 'public object', object: 'whiteboard', id: 'w1', ctx: { userId: 'bob' }, edit: true, del: true }, + { what: 'no owner column', object: 'note', id: 'n1', ctx: { userId: 'bob' }, edit: true, del: true }, + { what: 'unknown object', object: 'ghost', id: 'g1', ctx: { userId: 'bob' }, edit: true, del: true }, + { what: 'bypass-listed object', object: 'sys_user', id: 'u1', ctx: { userId: 'bob' }, edit: true, del: true }, + ]; + + it('canEdit / canDelete are `verdict !== deny` on every branch, and unchanged from the two-state era', async () => { + for (const c of CASES) { + const editVerdict = await svc.checkEdit(c.object, c.id, c.ctx); + const deleteVerdict = await svc.checkDelete(c.object, c.id, c.ctx); + expect(await svc.canEdit(c.object, c.id, c.ctx), `canEdit — ${c.what}`).toBe(c.edit); + expect(await svc.canDelete(c.object, c.id, c.ctx), `canDelete — ${c.what}`).toBe(c.del); + expect(editVerdict !== 'deny', `projection parity, canEdit — ${c.what}`).toBe(c.edit); + expect(deleteVerdict !== 'deny', `projection parity, canDelete — ${c.what}`).toBe(c.del); + } + }); + + it('the abstain set is exactly where the boolean `true` carried no permission', async () => { + // Anti-vacuity for the table above: three of its `true` cells are abstains, + // not allows — which is the whole content of this change. If a later edit + // made these `allow`, the parity test would stay green and the fail-open + // would be back. + for (const [object, id] of [['whiteboard', 'w1'], ['note', 'n1'], ['ghost', 'g1'], ['sys_user', 'u1']]) { + expect(await svc.checkEdit(object, id, { userId: 'bob' }), `${object} must abstain`).toBe('abstain'); + } + // …and the ones that are real permissions stay `allow`. + expect(await svc.checkEdit('account', 'a1', { userId: 'alice' })).toBe('allow'); + expect(await svc.checkEdit('account', 'a1', { isSystem: true })).toBe('allow'); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 91dfa15802..4d3a77e304 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -7,6 +7,7 @@ import type { GrantShareInput, SharingExecutionContext, ShareAccessLevel, + SharingWriteVerdict, } from '@objectstack/spec/contracts'; import { normalizeTenancyPosture, @@ -415,53 +416,185 @@ export class SharingService implements ISharingService { } } + /** + * [#6428] The two reasons {@link shouldBypass} answers `true` are NOT the + * same verdict, and merging them is what the tri-state exists to undo. + * + * - `context.isSystem` → **`allow`**. A platform-internal writer (audit, + * migrations, this plugin's own reconciliation) is positively permitted; + * the contract calls it a complete bypass, and a composing caller must not + * re-gate it behind somebody else's floor. + * - a bypass-LISTED object → **`abstain`**. Record sharing simply does not + * enforce on `sys_user` / `sys_record_share` / … — it is not a statement + * that the write is permitted, and whatever else guards those tables + * (RLS, the platform ownership floor) still decides. + * + * `null` = "no bypass applies, keep evaluating". + */ + private bypassVerdict( + object: string, + context: SharingExecutionContext, + ): SharingWriteVerdict | null { + if (context?.isSystem) return 'allow'; + if (this.bypassObjects.has(object)) return 'abstain'; + return null; + } + + /** + * [#6428] The one place an unresolvable write gate becomes a verdict. + * + * **`deny`, never `abstain`** — the two are opposite instructions to a + * composing caller (`abstain` hands the row to another authority, `deny` + * ends it), and reading a failed lookup as "no opinion" is exactly the + * confusion that produced #5492's measured fail-open. Logged rather than + * swallowed: a silently-denied write is indistinguishable from a legitimate + * refusal, which is how a broken engine looks like a permissions problem. + */ + private writeGateFailClosed( + verb: 'update' | 'delete', + object: string, + recordId: string, + context: SharingExecutionContext, + err: unknown, + ): SharingWriteVerdict { + this.logger?.error?.( + `[sharing] the ${verb} gate could not resolve a verdict for '${object}' record ` + + `'${recordId}' (user ${context?.userId ?? 'unknown'}) — DENYING (fail-closed, #6428): ` + + 'a failed lookup is a refusal, never an abstention', + err instanceof Error ? err : new Error(String(err)), + ); + return 'deny'; + } + + /** + * [#6428] Tri-state UPDATE verdict — the single place the update gate is + * decided, and the primary form of {@link canEdit}. + * + * `allow` when a POSITIVE basis exists: ownership (widened by write DEPTH), + * an explicit write-level share, or — [#4647] — the `modifyAllRecords` + * super-user bypass. That bypass branch is what makes "Modify All Data" mean + * what it says (an admin edits any record regardless of ownership — #1883's + * Salesforce reference frame) on rows the DEPTH fast-path cannot reach: an + * OWNERLESS row (`owner_id` NULL, which system-context seeds routinely + * produce) matches no owner set at any depth, so ownership alone refused it. + * + * `abstain` when record sharing does not enforce on the row at all — a + * public object, an object with no `owner_id` field, a bypass-listed + * internal, an object with no resolvable schema. Historically these returned + * the same `true` as a real grant, and #5492's E2 experiment measured what + * that costs a caller which lets the answer OVERRIDE the platform's + * `created_by` ownership floor: an ordinary member's cross-creator UPDATE + * succeeded on owner-less objects, where the floor alone had been refusing it. + * + * `deny` for everything else, including a lookup that throws. + */ + async checkEdit( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + const bypass = this.bypassVerdict(object, context); + if (bypass) return bypass; + + try { + const schema = this.engine.getSchema?.(object); + if (!schema) return 'abstain'; + const model = effectiveSharingModel(schema); + if (model === 'public') return 'abstain'; + if (!hasOwnerField(schema)) return 'abstain'; + if (!context.userId) return 'deny'; + + // 1) Ownership (write DEPTH widens the owner-set) — fast path. + if (await this.matchesOwnerScope(object, recordId, context)) return 'allow'; + + // 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: [...WRITE_ACCESS_LEVELS] }, + }, + fields: ['id'], + limit: 1, + context: SYSTEM_CTX, + }); + if (Array.isArray(editGrants) && editGrants.length > 0) return 'allow'; + + // 3) [#4647] Modify All Data — the explicit bypass, asked LAST and + // answered by the same predicate `security/explain` reports. + return (await this.hasModifyAllBypass(object, context)) ? 'allow' : 'deny'; + } catch (err) { + return this.writeGateFailClosed('update', object, recordId, context, err); + } + } + /** * Return `true` if the caller may UPDATE `(object, recordId)`: ownership * (widened by write DEPTH), an explicit write-level share, or — [#4647] — * the `modifyAllRecords` super-user bypass. Always `true` for system context, * public objects, and objects without an owner field. * - * The bypass branch is what makes "Modify All Data" mean what it says - * (an admin edits any record regardless of ownership — #1883's Salesforce - * reference frame) on rows the DEPTH fast-path cannot reach: an OWNERLESS - * row (`owner_id` NULL, which system-context seeds routinely produce) matches - * no owner set at any depth, so ownership alone refused it. + * [#6428] The two-state PROJECTION of {@link checkEdit}: `true` for every + * verdict that is not `deny`. The truth table is byte-for-byte the historical + * one — `abstain` is where the old `true` for public / owner-less / bypassed + * objects went — so every existing caller (the sharing middleware, the + * `sys_attachment` parent gate, the ADR-0055 master check) keeps its exact + * semantics. A caller that would let this answer OVERRIDE another authority + * must read {@link checkEdit} instead, because only there is "I permit this" + * distinguishable from "I do not enforce here". */ async canEdit( object: string, recordId: string, context: SharingExecutionContext, ): Promise { - if (this.shouldBypass(object, context)) return true; - - const schema = this.engine.getSchema?.(object); - if (!schema) return true; - const model = effectiveSharingModel(schema); - if (model === 'public') return true; - if (!hasOwnerField(schema)) return true; - if (!context.userId) return false; - - // 1) Ownership (write DEPTH widens the owner-set) — fast path. - if (await this.matchesOwnerScope(object, recordId, context)) return true; + return (await this.checkEdit(object, recordId, context)) !== 'deny'; + } - // 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: [...WRITE_ACCESS_LEVELS] }, - }, - fields: ['id'], - limit: 1, - context: SYSTEM_CTX, - }); - if (Array.isArray(editGrants) && editGrants.length > 0) return true; + /** + * [#6428 / ADR-0111 D3] Tri-state DELETE verdict — the primary form of + * {@link canDelete}. + * + * Deliberately NARROWER than {@link checkEdit}: `allow` is ownership (widened + * by write DEPTH) or the `modifyAllRecords` super-user bypass — and NOTHING + * ELSE. An `edit` (or legacy `full`) share opens update but not delete + * (sharing widens rows, never verbs), so a share holder gets `deny` here + * while `checkEdit` answers `allow` on the same row. The `abstain` set is + * IDENTICAL to `checkEdit`'s: both gates agree about which objects sharing + * enforces on, and differ only about the verb. + * + * [#4647] The bypass is asked EXPLICITLY (`hasWriteBypass`) instead of only + * riding in as `__writeScope === 'org'`. The scope proxy was silently + * partial: `matchesOwnerScope` refuses an OWNERLESS row before it ever looks + * at the scope, so a Modify All Data holder could not delete a row with a + * NULL `owner_id` — while `security/explain` said the bypass applied. + */ + async checkDelete( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise { + const bypass = this.bypassVerdict(object, context); + if (bypass) return bypass; - // 3) [#4647] Modify All Data — the explicit bypass, asked LAST and answered - // by the same predicate `security/explain` reports. - return this.hasModifyAllBypass(object, context); + try { + const schema = this.engine.getSchema?.(object); + if (!schema) return 'abstain'; + if (effectiveSharingModel(schema) === 'public') return 'abstain'; + if (!hasOwnerField(schema)) return 'abstain'; + if (!context.userId) return 'deny'; + + // Ownership / write DEPTH only — no share branch. This is the whole + // difference from checkEdit. + if (await this.matchesOwnerScope(object, recordId, context)) return 'allow'; + + // [#4647] Modify All Data — the same explicit bypass checkEdit consults. + return (await this.hasModifyAllBypass(object, context)) ? 'allow' : 'deny'; + } catch (err) { + return this.writeGateFailClosed('delete', object, recordId, context, err); + } } /** @@ -473,31 +606,15 @@ export class SharingService implements ISharingService { * rows, never verbs. Always `true` for system context, public objects, and * objects without an owner field, matching {@link canEdit}. * - * [#4647] The bypass is now asked EXPLICITLY (`hasWriteBypass`) instead of - * only riding in as `__writeScope === 'org'`. The scope proxy was silently - * partial: `matchesOwnerScope` refuses an OWNERLESS row before it ever looks - * at the scope, so a Modify All Data holder could not delete a row with a - * NULL `owner_id` — while `security/explain` said the bypass applied. + * [#6428] The two-state PROJECTION of {@link checkDelete}, on the same rule + * as {@link canEdit}: `true` for everything that is not a `deny`. */ async canDelete( object: string, recordId: string, context: SharingExecutionContext, ): Promise { - if (this.shouldBypass(object, context)) return true; - - const schema = this.engine.getSchema?.(object); - if (!schema) return true; - if (effectiveSharingModel(schema) === 'public') return true; - if (!hasOwnerField(schema)) return true; - if (!context.userId) return false; - - // Ownership / write DEPTH only — no share branch. This is the whole - // difference from canEdit. - if (await this.matchesOwnerScope(object, recordId, context)) return true; - - // [#4647] Modify All Data — the same explicit bypass canEdit consults. - return this.hasModifyAllBypass(object, context); + return (await this.checkDelete(object, recordId, context)) !== 'deny'; } /** diff --git a/packages/spec/api-surface/contracts.json b/packages/spec/api-surface/contracts.json index 03b5347eca..cb29295c52 100644 --- a/packages/spec/api-surface/contracts.json +++ b/packages/spec/api-surface/contracts.json @@ -263,6 +263,7 @@ "SharingRuleEvaluationResult (interface)", "SharingRuleRecipientType (type)", "SharingRuleRow (interface)", + "SharingWriteVerdict (type)", "SmsDeliveryStatus (type)", "SmsTransportSendResult (interface)", "StartupOptions (type)", diff --git a/packages/spec/src/contracts/sharing-service.test.ts b/packages/spec/src/contracts/sharing-service.test.ts index ce1c8a65b3..00ccf77381 100644 --- a/packages/spec/src/contracts/sharing-service.test.ts +++ b/packages/spec/src/contracts/sharing-service.test.ts @@ -3,8 +3,10 @@ import { describe, it, expect } from 'vitest'; import type { HierarchyScopeContext, + ISharingService, RecordShareRecipientType, SharingRuleRecipientType, + SharingWriteVerdict, } from './sharing-service'; import { ShareRecipientType } from '../security/sharing.zod'; @@ -111,12 +113,17 @@ describe('[#5125] ISharingService write-gate bypass documentation parity', () => 'canDelete', 'canEdit', 'canManageShares', + // [#6428] The tri-state primaries. Enumerated here so a gate can never be + // added to this interface without deciding whether it documents the + // bypass — the loop below is what that decision is written into. + 'checkDelete', + 'checkEdit', 'grant', 'listShares', 'revoke', ]); - for (const gate of ['canEdit', 'canDelete', 'canManageShares'] as const) { + for (const gate of ['canEdit', 'canDelete', 'canManageShares', 'checkEdit', 'checkDelete'] as const) { expect(docOf.get(gate), `${gate} must document the modifyAllRecords bypass`) .toContain('modifyAllRecords'); } @@ -361,3 +368,116 @@ describe('[#5817] ISharingService module header — one gate per write verb', () expect(header).toMatch(/\bdelete\b/); }); }); + +/** + * [#6428] The write gates are TRI-STATE, and the two-state projection stays. + * + * `canEdit()` answered one `true` for two different facts — "I permit this + * write" and "I do not enforce on this row at all" — which is safe only for a + * caller that ADDS this gate to whatever else guards the row. #5492's E2 + * experiment delegated a pre-image write gate to it and measured the cost: on + * objects with **no `owner_id` column**, an ordinary member's cross-creator + * UPDATE came back `ok: true` where `main` answers 403, because the platform's + * `created_by` ownership floor was that object's only row-level write gate and + * an abstaining `true` overrode it. + * + * Two kinds of pin, because the change has two halves: compile-time ones that + * tsc evaluates (`tsconfig.test.json` compiles this file — #5286), and prose + * ones, because the compatibility rule ("`canEdit` is `verdict !== 'deny'`") + * and the fail-closed rule ("a failed lookup is `deny`, never `abstain`") are + * obligations on IMPLEMENTERS that no type can carry. + */ +describe('[#6428] ISharingService tri-state write verdict', () => { + it('SharingWriteVerdict names exactly allow / abstain / deny (compile-time)', () => { + const everyVerdict: SharingWriteVerdict[] = ['allow', 'abstain', 'deny']; + // A fourth state would leave a composing caller with a case it cannot map + // onto an authorization decision — the union is deliberately closed. + // @ts-expect-error `unknown` is not a verdict this contract defines + const notAVerdict: SharingWriteVerdict = 'unknown'; + expect(everyVerdict).toHaveLength(3); + expect(notAVerdict).toBe('unknown'); + }); + + it('checkEdit/checkDelete answer the verdict; canEdit/canDelete stay boolean (compile-time)', () => { + // THE shape pin. An implementation must offer BOTH forms — the tri-state + // primary and the boolean projection existing callers already read — so + // the migration can never be "the interface promises a verdict nobody + // implements", which is the declared-not-delivered window this card's + // cross-domain exception exists to avoid. + const gates: Pick< + ISharingService, + 'checkEdit' | 'checkDelete' | 'canEdit' | 'canDelete' + > = { + checkEdit: async () => 'allow', + checkDelete: async () => 'abstain', + canEdit: async () => true, + canDelete: async () => false, + }; + + // The projection is ONE-WAY: a boolean cannot stand in for a verdict, or + // "I abstain" would be spellable as `true` again. + // @ts-expect-error boolean is not assignable to Promise + const collapsed: Pick = { checkEdit: async () => true }; + + expect(typeof gates.checkEdit).toBe('function'); + expect(typeof collapsed.checkEdit).toBe('function'); + }); + + it('the contract writes down the fail-closed rule and the projection rule', async () => { + const ts = (await import('typescript')).default; + const { readFileSync } = await import('node:fs'); + const { dirname, resolve } = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const file = resolve(dirname(fileURLToPath(import.meta.url)), 'sharing-service.ts'); + const source = ts.createSourceFile( + file, + readFileSync(file, 'utf8'), + ts.ScriptTarget.Latest, + /* setParentNodes */ true, + ); + + const verdictDoc = source.statements + .filter((s): s is import('typescript').TypeAliasDeclaration => ts.isTypeAliasDeclaration(s)) + .find((s) => s.name.text === 'SharingWriteVerdict') + ?.getFullText(source); + expect(verdictDoc, 'SharingWriteVerdict must still be declared in this file').toBeDefined(); + + // Anti-vacuity: this really is the documented type, not an empty match. + expect(verdictDoc).toContain('abstain'); + + // THE pin the card names: a failed lookup must NOT be dressed up as "no + // opinion", because `abstain` hands the row to another authority while + // `deny` ends it. Deleting this sentence turns the contract silent on the + // exact confusion that produced the fail-open. + expect(verdictDoc).toContain('never `abstain`'); + // …and `abstain` must be stated as NOT a permission, so a consumer cannot + // read "no opinion" as "allowed". + expect(verdictDoc).toMatch(/\*\*Not a permission\.\*\*/); + + const iface = source.statements.find( + (s): s is import('typescript').InterfaceDeclaration => + ts.isInterfaceDeclaration(s) && s.name.text === 'ISharingService', + ); + const docOf = new Map(); + for (const member of iface!.members) { + if (!ts.isMethodSignature(member) || !member.name || !ts.isIdentifier(member.name)) continue; + docOf.set(member.name.text, member.getFullText(source)); + } + + // The compatibility clause, in writing: the boolean form is defined AS the + // projection, so an implementer cannot "improve" it into denying an + // abstain and silently tighten every existing caller. + for (const projection of ['canEdit', 'canDelete'] as const) { + expect(docOf.get(projection), `${projection} must declare itself a projection`) + .toContain('PROJECTION'); + expect(docOf.get(projection), `${projection} must say which verdict maps to false`) + .toMatch(/not (a )?`deny`/); + } + + // Anti-vacuity: the search DISCRIMINATES. `buildReadFilter` contributes a + // filter, not a write verdict, and naming the write vocabulary there would + // itself be drift. + expect(docOf.get('buildReadFilter')).not.toContain('abstain'); + }); +}); diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index 8fd427fba9..9be3ae38f1 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -19,8 +19,20 @@ * for `delete` and is deliberately NARROWER (ADR-0111 D3) — a * share widens which ROWS a principal reaches, never which VERBS * they may use, so an `edit` share does NOT confer delete: - * ownership or the bypass only. Each returns `true` when the - * caller may perform that operation, `false` otherwise. + * ownership or the bypass only. + * + * [#6428] Each gate has TWO forms, and the difference matters to + * anyone COMPOSING this answer with another authority's. + * `checkEdit()` / `checkDelete()` are the primary form and answer a + * tri-state {@link SharingWriteVerdict} — `allow` (a positive basis + * to permit), `abstain` (this service does not enforce on this row + * at all), `deny` (refused). `canEdit()` / `canDelete()` are the + * two-state PROJECTION every pre-existing caller already reads: + * `true` for anything that is not a `deny`, so their truth table is + * byte-for-byte what it has always been. Read the tri-state whenever + * an `abstain` must NOT be mistaken for permission — see + * {@link SharingWriteVerdict} for the measured fail-open that + * collapsing the two into one `true` produced (#5492 E2). * * Manual share CRUD is exposed via `grant()`, `revoke()`, and * `listShares()`. The REST layer wires these to @@ -117,12 +129,49 @@ export interface SharingExecutionContext { isSystem?: boolean; } +/** + * [#6428] The verdict a per-record WRITE gate returns. THREE states, because + * two cannot say what this service actually knows: + * + * - **`allow`** — a POSITIVE basis to permit the write exists: ownership + * (widened by write DEPTH), an `edit`-level `sys_record_share` row (update + * only — ADR-0111 D3), the `modifyAllRecords` super-user bypass, or a system + * context. + * - **`abstain`** — this service has NO OPINION about this row, because record + * sharing does not enforce on it at all: a `public` sharing model, an object + * with no owner field, a bypass-listed platform internal, an object this + * engine has no schema for. **Not a permission.** Whatever other authority + * guards the row still decides, and a caller that has no other authority to + * consult may treat it as "nothing here stops you" — which is exactly what + * {@link ISharingService.canEdit}'s `true` has always meant. + * - **`deny`** — this service REFUSES: the object IS sharing-enforced and the + * principal has no ownership, no write-level share and no bypass — or the + * answer could not be resolved at all. + * + * **Why the third state exists (measured, not theoretical).** Collapsing + * `allow` and `abstain` into one `true` is safe for a caller that only ADDS a + * gate (the sharing middleware reads `true` as "I do not stop you") and unsafe + * for a caller that lets this verdict OVERRIDE another authority's floor. + * #5492's experiment E2 delegated a pre-image write gate to `canEdit()` and an + * ordinary member's cross-creator UPDATE became `ok: true` on objects with **no + * `owner_id` column** — where `main` answers 403 — because the platform's + * `created_by` ownership floor is the only row-level write gate such objects + * have, and an abstaining `true` overrode it. + * + * **A resolution failure is `deny`, never `abstain`.** The two are opposite + * instructions to a composing caller: `abstain` hands the decision on, `deny` + * ends it. Reading a failed lookup as "no opinion" is precisely the confusion + * that produced that fail-open, so an implementation that cannot answer must + * refuse rather than step aside. + */ +export type SharingWriteVerdict = 'allow' | 'abstain' | 'deny'; + /** * Public contract. * * Implementations should treat `context.isSystem === true` as a - * complete bypass (no filter, every `canEdit` returns `true`) so that - * platform-internal writers (audit, migrations, the sharing plugin + * complete bypass (no filter, every write gate answers `allow` / `true`) so + * that platform-internal writers (audit, migrations, the sharing plugin * itself) cannot deadlock on their own enforcement. */ export interface ISharingService { @@ -137,11 +186,19 @@ export interface ISharingService { ): Promise; /** - * Return `true` when the principal in `context` may UPDATE the record - * `(object, recordId)`. Ownership (widened by write DEPTH), a write-level - * ({@link ShareAccessLevel} `edit`) share, OR — [#4647] — the - * `modifyAllRecords` super-user bypass. Always true for system context, - * `public` objects, and objects with no owner field. + * [#6428] The UPDATE gate, in its primary tri-state form: may the principal + * in `context` UPDATE the record `(object, recordId)`, and does this service + * have an opinion at all? + * + * - `allow` — ownership (widened by write DEPTH), a write-level + * ({@link ShareAccessLevel} `edit`) share, [#4647] the `modifyAllRecords` + * super-user bypass, or a system context. + * - `abstain` — record sharing does not enforce on this row: a `public` + * object, an object with **no owner field**, a bypass-listed platform + * internal, or an object with no resolvable schema. The answer belongs to + * whatever else guards the row (see {@link SharingWriteVerdict}). + * - `deny` — a sharing-enforced object where the principal has none of the + * above, a principal-less context, or an unresolvable answer. * * The bypass is the same `ISecurityService.hasWriteBypass` predicate * {@link canDelete} and {@link canManageShares} consult, so the three write @@ -150,6 +207,31 @@ export interface ISharingService { * write costs no extra resolution, and it **fails CLOSED** (ADR-0111 D2): * no security service, a throwing probe, or a principal-less / * on-behalf-of context leaves the answer at owner-plus-share only. + * + * **Fail-closed, restated because the tri-state is where it gets confused:** + * a lookup that throws is a `deny`, never an `abstain`. + */ + checkEdit( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise; + + /** + * Return `true` when the principal in `context` may UPDATE the record + * `(object, recordId)`. Ownership (widened by write DEPTH), a write-level + * ({@link ShareAccessLevel} `edit`) share, OR — [#4647] — the + * `modifyAllRecords` super-user bypass. Always true for system context, + * `public` objects, and objects with no owner field. + * + * [#6428] The two-state PROJECTION of {@link checkEdit}: `true` for every + * verdict that is not `deny`, i.e. `allow` and `abstain` alike. That + * collapse is the historical semantics, kept byte-for-byte so existing + * callers do not drift — it is correct for a caller that only ADDS this gate + * to whatever else already guards the row (the sharing middleware, the + * `sys_attachment` parent gate, the ADR-0055 master check), and WRONG for a + * caller that would let this answer override another authority's floor. The + * latter must read {@link checkEdit}. */ canEdit( object: string, @@ -157,6 +239,25 @@ export interface ISharingService { context: SharingExecutionContext, ): Promise; + /** + * [#6428 / ADR-0111 D3] The DELETE gate, in its primary tri-state form. + * + * The verb boundary is inherited, not restated: a share widens *which rows* + * a principal reaches, never *which verbs* they may use, so an `edit` share + * that makes {@link checkEdit} answer `allow` leaves this one at `deny` + * (Salesforce Read/Write cannot delete; Dataverse `Delete` is a distinct + * privilege; Odoo splits `write`/`unlink`). `allow` is ownership (widened by + * write DEPTH), the `modifyAllRecords` super-user bypass, or system context + * ONLY; `abstain` covers exactly the rows {@link checkEdit} abstains on, so + * the two gates agree about which objects sharing enforces on and disagree + * only about the verb. + */ + checkDelete( + object: string, + recordId: string, + context: SharingExecutionContext, + ): Promise; + /** * [ADR-0111 D3] Return `true` when the principal in `context` may DELETE the * record `(object, recordId)`. @@ -169,6 +270,9 @@ export interface ISharingService { * true for system context, `public` objects, and objects with no owner * field, matching {@link canEdit}. A per-record delete grant, if ever added, * is a capability mask AND-ed with object CRUD — not a share level. + * + * [#6428] The two-state PROJECTION of {@link checkDelete}, on the same rule + * as {@link canEdit}: `true` for everything that is not a `deny`. */ canDelete( object: string,