From 853260db17e66b8bdec2411d90ee6b309da3e88b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 00:53:54 +0000 Subject: [PATCH 1/5] test(dogfood): the pending approver's read of the record its request is about (red first) Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude --- ...nding-approver-record-read.dogfood.test.ts | 249 ++++++++++++++++++ 1 file changed, 249 insertions(+) create mode 100644 packages/qa/dogfood/test/approval-pending-approver-record-read.dogfood.test.ts diff --git a/packages/qa/dogfood/test/approval-pending-approver-record-read.dogfood.test.ts b/packages/qa/dogfood/test/approval-pending-approver-record-read.dogfood.test.ts new file mode 100644 index 00000000000..cdd3ea9c6d7 --- /dev/null +++ b/packages/qa/dogfood/test/approval-pending-approver-record-read.dogfood.test.ts @@ -0,0 +1,249 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #26180 — the approver a step waits on reads the record the request is about, +// through the REST door on a real boot (ruling 6429330267, letter A). +// +// The object is `private`, so the baseline set lets a member read and edit only +// their own rows: neither approver can read the submitter's record by any grant +// the app wrote. The approvals service already resolves the slate; while a step +// is pending, each person on it (and the override holder) may READ the one +// record the request is about — no write, ending when the step resolves. +// +// Real `SecurityPlugin`, real sharing, real flows, real approvals plugin. A +// two-step flow: step one waits on the first approver, step two on the second. +// +// ⭐ the first approver reads the record while step one is pending (by id and +// in the list), and `explain` names the grant: "via approval request N"; +// ⭐ a member on no slate does not; +// ⭐ the approver cannot write it; +// ⭐ the override holder (a tenant administrator with no record bypass) reads +// it while a step is pending; +// ⭐ the step resolves and the read goes with it: approved and advanced (the +// first approver loses it, the second gains it), approved at the last step +// (the second loses it, and so does the override holder), rejected (the +// approver of the rejected step loses it). +// +// The organization wall is pinned at the service (`plugin-approvals` +// `pending-approver-read-grant.integration.test.ts`): this boot is `single`, +// so it has one organization and no wall to cross. + +import { describe, it, expect } from 'vitest'; +import { bootStack } from '@objectstack/verify'; +import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; +import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; +import { defineStack, defineFlow } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; +import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { catalogPermissionSet } from './catalog-permission-set.js'; + +const OBJECT = 'apread_contract'; +const SYS = { context: { isSystem: true } } as const; + +const SUBMITTER = 'apread-submitter@verify.test'; +/** Step one's slate, by email. */ +const FIRST = 'apread-first@verify.test'; +/** Step two's slate, by email. */ +const SECOND = 'apread-second@verify.test'; +/** On no slate; the baseline only. */ +const STRANGER = 'apread-stranger@verify.test'; +/** On no slate; holds the tenant-administrator rung without the record bypass. */ +const OVERRIDER = 'apread-overrider@verify.test'; + +const contract = ObjectSchema.create({ + name: OBJECT, + label: 'Approval Read Contract', + pluralLabel: 'Approval Read Contracts', + sharingModel: 'private', + fields: { name: Field.text({ label: 'Name', required: true }) }, +}); + +const flow = defineFlow({ + name: `${OBJECT}_flow`, + label: 'Approval read flow', + description: 'Fires on insert and opens two approval steps in turn, one approver each.', + type: 'autolaunched', + status: 'active', + nodes: [ + { id: 'start', type: 'start', label: 'On Create', config: { objectName: OBJECT, triggerType: 'record-after-create' } }, + { + id: 'first', + type: 'approval', + label: 'First', + config: { approvers: [{ type: 'user', value: FIRST }], behavior: 'first_response', lockRecord: false }, + }, + { + id: 'second', + type: 'approval', + label: 'Second', + config: { approvers: [{ type: 'user', value: SECOND }], behavior: 'first_response', lockRecord: false }, + }, + { id: 'approved', type: 'end', label: 'Approved' }, + { id: 'rejected', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'first' }, + { id: 'e2', source: 'first', target: 'second', label: 'approve' }, + { id: 'e3', source: 'first', target: 'rejected', label: 'reject' }, + { id: 'e4', source: 'second', target: 'approved', label: 'approve' }, + { id: 'e5', source: 'second', target: 'rejected', label: 'reject' }, + ], +}); + +const stackConfig = defineStack({ + manifest: { + id: 'com.dogfood.approval-pending-approver-record-read', + namespace: 'apread', + version: '0.0.0', + type: 'app', + name: 'Approval Pending Approver Record Read Fixture', + description: 'A two-step approval flow on one private object.', + }, + // ADR-0097: the `record-after-create` start node needs the trigger capability. + requires: ['automation', 'triggers'], + objects: [contract], + flows: [flow], +}); + +/** Everyone's baseline: CRUD on the object, which `private` narrows to their own rows. */ +const baselineSet: PermissionSet = PermissionSetSchema.parse({ + name: 'apread_baseline', + label: 'Approval Read Baseline — own rows of the fixture object', + objects: { [OBJECT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: false } }, +}); + +describe('the approver a pending step waits on reads its record (#26180)', () => { + it( + 'grants read of the one record to the pending slate and the override holder, never write, and ends with the step', + async () => { + const stack = await bootStack(stackConfig as unknown as Parameters[0], { + automation: true, + security: new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, baselineSet], + fallbackPermissionSet: baselineSet.name, + }), + extraPlugins: [new RecordChangeTriggerPlugin(), new ApprovalsServicePlugin()], + }); + try { + const ql: any = await stack.kernel.getServiceAsync('objectql'); + const idOf = async (email: string): Promise => + String((await ql.findOne('sys_user', { where: { email }, context: SYS.context }))?.id ?? ''); + const grant = async (email: string, set: string) => { + expect(await catalogPermissionSet(ql, set), `${set} is served by the catalog`).toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: await idOf(email), permission_set: set }, SYS); + }; + + const submitter = await stack.signUp(SUBMITTER); + const first = await stack.signUp(FIRST); + const second = await stack.signUp(SECOND); + const stranger = await stack.signUp(STRANGER); + const overrider = await stack.signUp(OVERRIDER); + await grant(OVERRIDER, baselineSet.name); + // The tenant-administrator rung, wall-less: administers the + // organization, carries no `viewAllRecords` (ADR-0105 D4) — so it reads + // no one else's private row by its own grants. + await grant(OVERRIDER, 'organization_admin_no_bypass'); + + const create = async (name: string): Promise => { + const res = await stack.apiAs(submitter, 'POST', `/data/${OBJECT}`, { name }); + expect(res.status, await res.clone().text()).toBe(201); + const body = (await res.json()) as { id?: string; record?: { id?: string } }; + return String(body.id ?? body.record?.id ?? ''); + }; + const pendingOn = async (recordId: string): Promise> => + ql.find('sys_approval_request', { where: { object_name: OBJECT, record_id: recordId, status: 'pending' }, context: SYS.context }); + const readStatus = async (token: string, recordId: string): Promise => + (await stack.apiAs(token, 'GET', `/data/${OBJECT}/${encodeURIComponent(recordId)}`)).status; + const listIds = async (token: string): Promise => { + const res = await stack.apiAs(token, 'GET', `/data/${OBJECT}`); + expect(res.status, await res.clone().text()).toBe(200); + const body = (await res.json()) as { records?: Array<{ id: string }>; data?: Array<{ id: string }> }; + return (body.records ?? body.data ?? []).map((r) => String(r.id)).sort(); + }; + const decide = async (token: string, requestId: string, verb: 'approve' | 'reject') => { + const res = await stack.apiAs(token, 'POST', `/approvals/requests/${requestId}/${verb}`, {}); + expect(res.status, `${verb} ${requestId}: ${await res.clone().text()}`).toBe(200); + }; + + const ladderId = await create('ladder contract'); + const rejectId = await create('rejected contract'); + + // The scene, before any read is believed: each record has one pending + // request, on step one, and the submitter's private rows are out of + // reach of every other member by their own grants (the stranger). + const [ladderStepOne] = await pendingOn(ladderId); + expect(ladderStepOne, 'the flow opened step one').toBeTruthy(); + const [rejectStepOne] = await pendingOn(rejectId); + expect(rejectStepOne, 'the flow opened step one').toBeTruthy(); + expect(await readStatus(submitter, ladderId)).toBe(200); + expect(await readStatus(stranger, ladderId)).toBe(404); + expect(await listIds(stranger)).toEqual([]); + + // ⭐ 1 — the pending approver reads the record: by id and in the list. + expect(await readStatus(first, ladderId)).toBe(200); + expect(await listIds(first)).toEqual([ladderId, rejectId].sort()); + // …and the approver of a step not yet open reads nothing. + expect(await readStatus(second, ladderId)).toBe(404); + + // ⭐ 2 — a member on no slate does not. + expect(await readStatus(stranger, ladderId)).toBe(404); + + // ⭐ 3 — read only: the approver's edit is refused, and nothing lands. + const edit = await stack.apiAs(first, 'PATCH', `/data/${OBJECT}/${ladderId}`, { name: 'edited by the approver' }); + expect(edit.status, await edit.clone().text()).toBe(403); + const stored = await ql.findOne(OBJECT, { where: { id: ladderId }, context: SYS.context }); + expect(stored?.name).toBe('ladder contract'); + + // ⭐ 4 — `explain` names the grant: "via approval request N". + const explained = await stack.apiAs(first, 'POST', '/security/explain', { + object: OBJECT, operation: 'read', recordId: ladderId, + }); + const explainText = await explained.text(); + expect(explained.status, explainText).toBe(200); + const decision = JSON.parse(explainText)?.data ?? JSON.parse(explainText); + expect(decision.record).toMatchObject({ recordId: ladderId, visible: true }); + const sharing = (decision.layers ?? []).find((l: any) => l.layer === 'sharing'); + expect(sharing?.record?.outcome).toBe('admitted'); + expect(sharing?.record?.rules).toContainEqual({ + kind: 'record_share', + name: ladderStepOne.id, + grants: 'read', + via: `approval request ${ladderStepOne.id}`, + effect: 'admits', + }); + + // ⭐ 5 — the override holder reads it while a step is pending. + expect(await readStatus(overrider, ladderId)).toBe(200); + + // ⭐ 6a — approved and ADVANCED: step one resolves, step two opens. The + // first approver loses the read at once; the second gains it. + await decide(first, ladderStepOne.id, 'approve'); + const [ladderStepTwo] = await pendingOn(ladderId); + expect(ladderStepTwo, 'step two opened').toBeTruthy(); + expect(ladderStepTwo.id).not.toBe(ladderStepOne.id); + expect(await readStatus(first, ladderId)).toBe(404); + expect(await listIds(first)).toEqual([rejectId]); + expect(await readStatus(second, ladderId)).toBe(200); + expect(await readStatus(overrider, ladderId)).toBe(200); + + // ⭐ 6b — APPROVED at the last step: no pending step is left, so the + // second approver and the override holder lose the read. + await decide(second, ladderStepTwo.id, 'approve'); + expect(await pendingOn(ladderId)).toEqual([]); + expect(await readStatus(second, ladderId)).toBe(404); + expect(await readStatus(overrider, ladderId)).toBe(404); + + // ⭐ 6c — REJECTED: the approver of the rejected step loses the read. + expect(await readStatus(first, rejectId)).toBe(200); + await decide(first, rejectStepOne.id, 'reject'); + expect(await pendingOn(rejectId)).toEqual([]); + expect(await readStatus(first, rejectId)).toBe(404); + expect(await listIds(first)).toEqual([]); + expect(await readStatus(overrider, rejectId)).toBe(404); + } finally { + await stack.stop(); + } + }, + 120_000, + ); +}); From 1639ac9d156c51149cfb7404c0e89ea82922397b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 00:58:29 +0000 Subject: [PATCH 2/5] fix(plugin-approvals,plugin-sharing): the approver a pending step waits on reads the record its request is about While an approval request is pending, each person on its current step's slate and the override holder may read the one record it is about: read only, ending when the step resolves, never across organizations. The approvals service derives the record ids from the slate it already resolves; the sharing read filter ORs them in beside the record shares and fails closed; explain names the grant via the approval request. Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude --- .../26180-pending-approver-record-read.md | 21 ++ .../plugin-approvals/src/approval-service.ts | 30 ++ ...ng-approver-read-grant.integration.test.ts | 275 ++++++++++++++++++ .../src/pending-approver-read-grant.ts | 213 ++++++++++++++ .../src/explain-engine.test.ts | 51 ++++ .../plugin-security/src/explain-engine.ts | 29 +- .../src/security-plugin-explain-access.ts | 7 + .../plugin-sharing/src/sharing-plugin.ts | 8 + .../src/sharing-service.test.ts | 94 ++++++ .../plugin-sharing/src/sharing-service.ts | 77 ++++- 10 files changed, 803 insertions(+), 2 deletions(-) create mode 100644 .changeset/26180-pending-approver-record-read.md create mode 100644 packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts create mode 100644 packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts diff --git a/.changeset/26180-pending-approver-record-read.md b/.changeset/26180-pending-approver-record-read.md new file mode 100644 index 00000000000..42b19dbc69f --- /dev/null +++ b/.changeset/26180-pending-approver-record-read.md @@ -0,0 +1,21 @@ +--- +'@objectstack/plugin-approvals': patch +'@objectstack/plugin-sharing': patch +'@objectstack/plugin-security': patch +--- + +fix(plugin-approvals,plugin-sharing): the approver a pending step waits on can read the record the request is about + +Clause-②: no +Metadata-protocol: none + +An approver was asked to decide on a record they could not open. Unless the app happened to grant them read of that exact row, the request page showed only the snapshot and the record card read "unable to open this record", so a contract was approved without its document being read. + +- **Who reads.** While an approval request is pending, each person on its current step's slate (a slot naming their user id, or the email their own account carries) and the override holder (the administrator who may decide any pending request of their organization) may READ the one record the request is about. A slot left as a `position:` or `team:` literal names no person and grants no read. +- **How far.** Read only: the write gates never read this grant, so the approver still cannot edit or delete the record. Only the record itself, as a reader of it reads it: attachments follow the record read, as they do for any reader. Only while the step is pending: a decision that resolves the step, or advances the request to its next step, ends the grant at the next read. Never across organizations, the platform administrator's override included. An on-behalf-of (agent) principal gets no approval grant. +- **Where it is decided.** `@objectstack/plugin-approvals` derives the records from the slate it already resolves (`ApprovalService.pendingApproverReadGrants`, one indexed read of the pending-approver index per request context, plus one request read per object, memoized per request and retired by any write). `@objectstack/plugin-sharing` ORs those record ids into its read filter beside the caller's record shares, so every read door that composes the sharing filter serves them; it grants nothing when the approvals service is absent or its read fails. +- **Explain.** `@objectstack/plugin-security`'s access explanation names the grant on the sharing layer as a read share "via approval request N". + +The grant is the row-level reach of a read share; the approver still needs read on the object itself. Nothing is declared: no metadata key, no app setting. + +What to do: nothing. An app that wrote sharing rules mirroring its approval routing only so approvers could open the record may drop them. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index fb61622d006..664fedd0944 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -121,6 +121,7 @@ import { } from './payload-redaction.js'; import { APPROVAL_REQUEST_CHILD_OBJECTS, type RequestVisibilitySource } from './request-read-gate.js'; import { nodeConfigLocksRecord, recordHeldByLockingApproval } from './lifecycle-hooks.js'; +import { PendingApproverReadGrantSource, type PendingApproverReadGrants } from './pending-approver-read-grant.js'; // The ADR-0043 pages — the ONE copy of their text, rendered by the plugin's // self-hosted mount and by `handleActionPage` alike. import { renderConfirmPage, renderResultPage } from './action-link-pages.js'; @@ -1597,10 +1598,21 @@ export class ApprovalService implements IApprovalService { * registration, and the audit writer memoizes the same way. */ private readonly recordOrgResolver: RecordOrganizationResolver; + /** + * [#26180] The records a pending approver may read: memoized per execution + * context and engine write epoch ({@link ApprovalService.pendingApproverReadGrants}). + */ + private readonly pendingApproverReads: PendingApproverReadGrantSource; constructor(opts: ApprovalServiceOptions) { this.engine = opts.engine; this.recordOrgResolver = createRecordOrganizationResolver(opts.engine); + this.pendingApproverReads = new PendingApproverReadGrantSource({ + engine: opts.engine, + isOverrideActor: (context, organizationId) => this.isOverrideActor(context, organizationId), + accountEmail: (userId) => this.callerAccountEmail(userId), + logger: opts.logger, + }); this.clock = opts.clock ?? { now: () => new Date() }; this.logger = opts.logger; this.automation = opts.automation; @@ -1663,6 +1675,24 @@ export class ApprovalService implements IApprovalService { }); } + /** + * [#26180 · ruling 6429330267, letter A] The records of `object` the caller + * may READ because a pending approval step waits on them: record id → the + * pending request ids that grant it. + * + * While a request is pending, each resolved person on its current step's + * slate (the caller's user id or account email in a slot) and the override + * holder ({@link ApprovalService.isOverrideActor}) may read the one record + * it is about. Derived from the slate this service resolved, never declared; + * fenced to the caller's active organization; ends when the step resolves. + * plugin-sharing ORs the ids into its read filter and names the requests in + * `explain`. Rejects when a read fails, so the consumer can fail closed. + * The rules and the cost live in `pending-approver-read-grant.ts`. + */ + async pendingApproverReadGrants(object: string, context: ExecutionContext): Promise { + return this.pendingApproverReads.forObject(object, context); + } + /** Attach (or replace) the ADR-0105 D9 posture provider. */ attachTenancyPosture(provider: () => string | undefined): void { this.tenancyPosture = provider; diff --git a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts new file mode 100644 index 00000000000..4b0f3694453 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts @@ -0,0 +1,275 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#26180 · ruling 6429330267, letter A] Which records a pending approval step + * lets a caller READ — `ApprovalService.pendingApproverReadGrants`, the answer + * plugin-sharing ORs into its read filter. + * + * The full door (a real kernel, the security and sharing plugins, a two-step + * flow) is pinned in dogfood's `approval-pending-approver-record-read`. What + * only this level can state is the derivation itself and its bounds: + * + * - who: a PERSON on the pending slate — the caller's user id or account + * email in a slot — and the override holder; a `position:` literal (a + * target that resolved to nobody) names no person and grants no read; + * - while: a decided request grants nothing, at the next read of the same + * request context too (a write retires the memo); + * - where: never across organizations, the platform administrator's override + * included (that boot is `single`, one organization, no wall to cross); + * - cost: a request context pays one account read and one index read for its + * slate, and one request read per object — never a read per record, and a + * caller on no slate stops after the index read; + * - fail: a failed read is rethrown and never memoized. + * + * The rig is the read-gate suites' (`record-reader-opt-in.integration.test.ts`): + * a real ObjectQL engine over better-sqlite3 and the real + * `ApprovalsServicePlugin.start()`. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; +import { SysApprovalAction } from './sys-approval-action.object.js'; +import { SysApprovalApprover } from './sys-approval-approver.object.js'; +import { SysApprovalDelegation } from './sys-approval-delegation.object.js'; +import { DEFAULT_ORGANIZATION_ID, installDefaultOrganization } from './default-organization.testkit.js'; + +const OBJECT = 'contract_sheet'; +const OTHER = 'memo_sheet'; +const PKG = 'approvals-pending-approver-read-test'; +const OTHER_ORG = 'org_other'; + +const SYSTEM: ExecutionContext = { isSystem: true, positions: [], permissions: [] }; +/** A request row as the system reads it. */ +const requestRow = async (rig: Rig, where: Record): Promise => + ((await rig.engine.find('sys_approval_request', { where, context: SYSTEM } satisfies EngineQueryOptions)) as any[])[0]; +/** A member of `org`, acting as themself. */ +const member = (userId: string, org: string = DEFAULT_ORGANIZATION_ID) => + ({ userId, tenantId: org, organizationId: org, positions: [], permissions: [], posture: 'MEMBER' }) as any; + +const SUBMITTER = 'u_submitter'; +/** On the slate by user id. */ +const BY_ID = 'u_by_id'; +/** On the slate by the email their account carries. */ +const BY_EMAIL = 'u_by_email'; +const BY_EMAIL_ADDRESS = 'by.email@verify.test'; +/** Holds position `buyer`; the slate holds only the unresolved `position:buyer` literal. */ +const BUYER = 'u_buyer'; +/** On no slate. */ +const STRANGER = 'u_stranger'; + +function sheet(name: string) { + return { + name, + label: name, + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + }, + }; +} + +/** The account table `callerAccountEmail` reads. */ +const SysUser = { + name: 'sys_user', + label: 'User', + managedBy: 'better-auth', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; + +interface Rig { + engine: ObjectQL; + svc: ApprovalService; + finds: string[]; +} + +let current: ObjectQL | undefined; + +afterEach(async () => { + try { await current?.destroy(); } catch { /* noop */ } + current = undefined; +}); + +async function boot(): Promise { + const engine = new ObjectQL(); + current = engine; + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [ + sheet(OBJECT), sheet(OTHER), SysUser, + SysApprovalRequest, SysApprovalAction, SysApprovalApprover, SysApprovalDelegation, + ]) { + engine.registry.registerObject(def as any, PKG, PKG); + } + await engine.syncSchemas(); + await installDefaultOrganization(engine); + await engine.getDriverForObject('sys_organization')!.create('sys_organization', { id: OTHER_ORG, name: 'Other' }); + await engine.getDriverForObject('sys_user')!.create('sys_user', { id: BY_EMAIL, email: BY_EMAIL_ADDRESS }); + + const services: Record = { objectql: engine }; + const ctx: any = { + getService: (name: string) => { + if (!(name in services)) throw new Error(`[Kernel] Service '${name}' not found`); + return services[name]; + }, + registerService: (name: string, svc: unknown) => { services[name] = svc; }, + hook: () => {}, + logger: { info: () => {}, debug: () => {}, warn: () => {}, error: () => {} }, + }; + await new ApprovalsServicePlugin().start(ctx); + + // Every engine read of the approval tables and the account table, by table — + // the cost the read path pays. Counted at the engine's own `find`. + const finds: string[] = []; + const find = engine.find.bind(engine); + (engine as any).find = (object: string, options?: EngineQueryOptions) => { + if (object.startsWith('sys_approval') || object === 'sys_user') finds.push(object); + return find(object, options); + }; + return { engine, svc: services.approvals as ApprovalService, finds }; +} + +/** Open one pending request on `recordId` of `object`, its slate the given approver targets. */ +async function open( + rig: Rig, + object: string, + recordId: string, + approvers: Array<{ type: 'user' | 'position'; value: string }>, + organizationId: string = DEFAULT_ORGANIZATION_ID, +): Promise { + await rig.engine.insert(object, { id: recordId, title: recordId, organization_id: organizationId }, { context: SYSTEM }); + const opened = await rig.svc.openNodeRequest({ + object, recordId, runId: `run_${recordId}`, nodeId: 'review', flowName: 'contract_review', + config: { approvers, behavior: 'first_response' } as any, + submitterId: SUBMITTER, record: { id: recordId, title: recordId }, organizationId, + }, member(SUBMITTER, organizationId)) as any; + expect(opened?.status, `request on ${recordId} is pending`).toBe('pending'); + return String(opened.id); +} + +/** The grant as plain data: record id → request ids. */ +async function grantsOf(rig: Rig, object: string, context: any): Promise> { + return Object.fromEntries([...(await rig.svc.pendingApproverReadGrants(object, context))].map(([k, v]) => [k, [...v]])); +} + +describe('pendingApproverReadGrants — the records a pending approval step lets a caller read (#26180)', () => { + it('grants a person on the pending slate, by user id or by account email, the one record — and no one else', async () => { + const rig = await boot(); + const byIdRequest = await open(rig, OBJECT, 'c_by_id', [{ type: 'user', value: BY_ID }]); + const byEmailRequest = await open(rig, OBJECT, 'c_by_email', [{ type: 'user', value: BY_EMAIL_ADDRESS }]); + await open(rig, OBJECT, 'c_position', [{ type: 'position', value: 'buyer' }]); + + expect(await grantsOf(rig, OBJECT, member(BY_ID))).toEqual({ c_by_id: [byIdRequest] }); + expect(await grantsOf(rig, OBJECT, member(BY_EMAIL))).toEqual({ c_by_email: [byEmailRequest] }); + // The submitter, a stranger: on no slate. + expect(await grantsOf(rig, OBJECT, member(SUBMITTER))).toEqual({}); + expect(await grantsOf(rig, OBJECT, member(STRANGER))).toEqual({}); + // A `position:` literal names no person: the holder may decide it, but + // the read is the resolved person's alone. + const positionRow = await requestRow(rig, { record_id: 'c_position' }); + expect(String(positionRow.pending_approvers)).toBe('position:buyer'); + expect(await grantsOf(rig, OBJECT, { ...member(BUYER), positions: ['buyer'] })).toEqual({}); + // Another object's records are never granted by this one's requests. + expect(await grantsOf(rig, OTHER, member(BY_ID))).toEqual({}); + // No user, a system context: nothing to grant (system reads are unscoped anyway). + expect(await grantsOf(rig, OBJECT, { positions: [], permissions: [] })).toEqual({}); + expect(await grantsOf(rig, OBJECT, SYSTEM)).toEqual({}); + }); + + it('grants the override holder every pending record of the object in their organization', async () => { + const rig = await boot(); + const a = await open(rig, OBJECT, 'c_a', [{ type: 'user', value: BY_ID }]); + const b = await open(rig, OBJECT, 'c_b', [{ type: 'user', value: BY_EMAIL_ADDRESS }]); + const tenantAdmin = { ...member('u_admin'), posture: 'TENANT_ADMIN', permissions: ['organization_admin_no_bypass'] }; + expect(await grantsOf(rig, OBJECT, tenantAdmin)).toEqual({ c_a: [a], c_b: [b] }); + }); + + it('ends with the step: a decided request grants nothing, on the same request context too', async () => { + const rig = await boot(); + const request = await open(rig, OBJECT, 'c_decided', [{ type: 'user', value: BY_ID }]); + const ctx = member(BY_ID); + expect(await grantsOf(rig, OBJECT, ctx)).toEqual({ c_decided: [request] }); + + await rig.svc.decideNode(request, { decision: 'approve', actorId: BY_ID }, member(BY_ID)); + const row = await requestRow(rig, { id: request }); + expect(row.status).toBe('approved'); + expect(await grantsOf(rig, OBJECT, ctx)).toEqual({}); + expect(await grantsOf(rig, OBJECT, member(BY_ID))).toEqual({}); + + const rejected = await open(rig, OBJECT, 'c_rejected', [{ type: 'user', value: BY_ID }]); + expect(await grantsOf(rig, OBJECT, ctx)).toEqual({ c_rejected: [rejected] }); + await rig.svc.decideNode(rejected, { decision: 'reject', actorId: BY_ID }, member(BY_ID)); + expect(await grantsOf(rig, OBJECT, ctx)).toEqual({}); + }); + + it('never crosses an organization — the platform administrator\'s override included', async () => { + const rig = await boot(); + const there = await open(rig, OBJECT, 'c_there', [{ type: 'user', value: BY_ID }], OTHER_ORG); + const row = await requestRow(rig, { id: there }); + expect(row.organization_id, 'the request lives in the other organization').toBe(OTHER_ORG); + + // The same person, acting in their default organization: nothing. + expect(await grantsOf(rig, OBJECT, member(BY_ID))).toEqual({}); + // …and in the request's own organization: the record. + expect(await grantsOf(rig, OBJECT, member(BY_ID, OTHER_ORG))).toEqual({ c_there: [there] }); + // The platform administrator may DECIDE across the wall; the read stays home. + const platformAdmin = { ...member('u_platform'), posture: 'PLATFORM_ADMIN' }; + expect(await grantsOf(rig, OBJECT, platformAdmin)).toEqual({}); + // An org-less caller reaches only org-less requests. + expect(await grantsOf(rig, OBJECT, { userId: BY_ID, positions: [], permissions: [] })).toEqual({}); + }); + + it('costs one account read and one index read per request context, plus one request read per object — never per record', async () => { + const rig = await boot(); + const r1 = await open(rig, OBJECT, 'c_1', [{ type: 'user', value: BY_ID }]); + const r2 = await open(rig, OBJECT, 'c_2', [{ type: 'user', value: BY_ID }]); + + const ctx = member(BY_ID); + rig.finds.length = 0; + for (let i = 0; i < 5; i++) expect(await grantsOf(rig, OBJECT, ctx)).toEqual({ c_1: [r1], c_2: [r2] }); + expect(await grantsOf(rig, OTHER, ctx)).toEqual({}); + expect(rig.finds).toEqual(['sys_user', 'sys_approval_approver', 'sys_approval_request', 'sys_approval_request']); + + // A caller on no slate stops after the index read, for every object. + const stranger = member(STRANGER); + rig.finds.length = 0; + await grantsOf(rig, OBJECT, stranger); + await grantsOf(rig, OTHER, stranger); + expect(rig.finds).toEqual(['sys_user', 'sys_approval_approver']); + + // Any write through the engine retires the memo: the next read asks again. + await rig.engine.insert(OTHER, { id: 'm_1', title: 'unrelated' }, { context: SYSTEM }); + rig.finds.length = 0; + await grantsOf(rig, OBJECT, ctx); + expect(rig.finds).toEqual(['sys_user', 'sys_approval_approver', 'sys_approval_request']); + }); + + it('rethrows a failed read and never memoizes it', async () => { + const rig = await boot(); + const request = await open(rig, OBJECT, 'c_fault', [{ type: 'user', value: BY_ID }]); + const ctx = member(BY_ID); + const find = (rig.engine as any).find; + let failNext = true; + (rig.engine as any).find = (object: string, options?: EngineQueryOptions) => { + if (object === 'sys_approval_approver' && failNext) { + failNext = false; + return Promise.reject(new Error('store unavailable')); + } + return find(object, options); + }; + await expect(rig.svc.pendingApproverReadGrants(OBJECT, ctx)).rejects.toThrow('store unavailable'); + expect(await grantsOf(rig, OBJECT, ctx)).toEqual({ c_fault: [request] }); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts new file mode 100644 index 00000000000..c2798831fde --- /dev/null +++ b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts @@ -0,0 +1,213 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { ExecutionContext } from '@objectstack/spec/kernel'; + +/** + * [#26180 · ruling 6429330267, letter A] The records a caller may READ because + * an approval step waits on them. + * + * While an approval request is pending, each resolved PERSON on the current + * step's slate, and the override holder, may read the one record the request is + * about. The grant is derived from state this plugin already keeps — the slate + * the open path resolved, mirrored into `sys_approval_approver` — so there is + * no key to declare and nothing for an app to write. plugin-sharing ORs the + * record ids this answers into its read filter beside the caller's record + * shares, so every read door that composes the sharing filter serves them, and + * `explain` names the request. + * + * ## Who + * + * - A PERSON on the slate: a slot naming the caller's user id or the email + * their own account carries — the two addresses `slateNamesAPerson` + * (`approver-address.ts`) calls a person. A `position:

` / `team:` + * literal is an approver target that resolved to nobody when the step opened; + * it names no person, so it grants no one a read. + * - The override holder (`isOverrideActor`, #3424): the administrator who may + * decide any pending request of their organization, so may read what it is + * about. + * + * ## Bounds + * + * - Read only: the answer feeds the READ filter alone. The write gates read + * `sys_record_share` access levels and never this. + * - Only while pending: the index holds a request's rows only while it is + * pending (`syncApproverIndex` clears them when it leaves `pending`, and + * rewrites them on every slate change), and the request row is re-read with + * `status: 'pending'`. A step that resolves stops granting at the next read. + * - Never across organizations (ADR-0105): both reads are fenced to the + * caller's active organization — an org-less caller reaches only org-less + * requests — and the platform admin's override, which crosses the wall to + * DECIDE, does not cross it to READ. + * + * ## Cost + * + * The read path is hot, so the answer is memoized per execution context (the + * object the transport builds once per request, as plugin-security's + * permission-set memo keys on) and per engine write epoch: any write through + * the engine — a decision among them — retires it, and nothing outlives the + * request. A person's first read costs one `sys_user` read for the account + * email and one indexed `sys_approval_approver` read; a caller on no slate + * stops there, for every object of the request. A caller on a slate pays one + * `sys_approval_request` read per object. The override holder pays one + * `sys_approval_request` read per object. Never a read per row. + * + * A failed read is not memoized and is rethrown: the caller (plugin-sharing) + * fails closed and grants nothing. + */ + +/** The `ApprovalEngine` member this module reads. */ +export interface PendingApproverReadEngine { + find(object: string, options?: any): Promise; +} + +/** What the service hands this module: its engine and two of its own answers. */ +export interface PendingApproverReadDeps { + engine: PendingApproverReadEngine; + /** `ApprovalService.isOverrideActor`, asked with the caller's active organization. */ + isOverrideActor(context: ExecutionContext, organizationId: string | null): boolean; + /** The email the caller's own account carries (`ApprovalService.callerAccountEmail`). */ + accountEmail(userId: string): Promise; + logger?: { warn?: (msg: string, meta?: unknown) => void }; +} + +/** record id → the pending request ids that grant it, in the order read. */ +export type PendingApproverReadGrants = ReadonlyMap; + +const SYSTEM_CTX: ExecutionContext = { isSystem: true, positions: [], permissions: [] }; + +/** The window of each probe; a pending queue lives far below it. */ +export const PENDING_APPROVER_READ_CAP = 10_000; + +const NONE: PendingApproverReadGrants = new Map(); + +interface MemoEntry { + epoch: number; + /** The pending request ids naming the caller as a person; `null` for the override holder. */ + slate?: Promise; + byObject: Map>; +} + +/** The caller's active organization, read as the service reads it everywhere else. */ +function activeOrganization(context: ExecutionContext): string | null { + const org = (context as any)?.organizationId ?? context?.tenantId ?? null; + return org == null || String(org) === '' ? null : String(org); +} + +/** The organization fence both probes carry: the caller's organization, or org-less rows only. */ +function organizationFence(org: string | null): Record { + return { organization_id: org ?? { $null: true } }; +} + +/** The engine's write epoch when it exposes one; without it nothing is reused. */ +function writeEpochOf(engine: unknown): number | undefined { + const current = (engine as { writeEpoch?: { current?: unknown } } | null)?.writeEpoch?.current; + return typeof current === 'number' ? current : undefined; +} + +export class PendingApproverReadGrantSource { + private readonly memo = new WeakMap(); + + constructor(private readonly deps: PendingApproverReadDeps) {} + + /** The record ids of `object` the caller may read through a pending approval step. */ + async forObject(object: string, context: ExecutionContext): Promise { + if (!object || !context || context.isSystem) return NONE; + const userId = context.userId != null ? String(context.userId) : ''; + if (!userId) return NONE; + const entry = this.entryFor(context); + if (!entry) return this.compute(object, context, undefined); + const cached = entry.byObject.get(object); + if (cached) return cached; + const pending = this.compute(object, context, entry); + entry.byObject.set(object, pending); + // A failed read is never reused: the next read asks again. + pending.catch(() => { + if (entry.byObject.get(object) === pending) entry.byObject.delete(object); + }); + return pending; + } + + /** The memo entry for this context at the current epoch, or `undefined` when nothing may be reused. */ + private entryFor(context: ExecutionContext): MemoEntry | undefined { + const epoch = writeEpochOf(this.deps.engine); + if (epoch === undefined || typeof context !== 'object') return undefined; + const held = this.memo.get(context); + if (held && held.epoch === epoch) return held; + const fresh: MemoEntry = { epoch, byObject: new Map() }; + this.memo.set(context, fresh); + return fresh; + } + + private async compute( + object: string, + context: ExecutionContext, + entry: MemoEntry | undefined, + ): Promise { + const org = activeOrganization(context); + let slate: readonly string[] | null; + if (entry) { + if (!entry.slate) { + entry.slate = this.slateRequestIds(context, org); + const held = entry.slate; + held.catch(() => { + if (entry.slate === held) entry.slate = undefined; + }); + } + slate = await entry.slate; + } else { + slate = await this.slateRequestIds(context, org); + } + if (slate !== null && slate.length === 0) return NONE; + + const where: Record = { object_name: object, status: 'pending', ...organizationFence(org) }; + if (slate !== null) where.id = slate.length === 1 ? slate[0] : { $in: [...slate] }; + const rows = await this.deps.engine.find('sys_approval_request', { + where, + fields: ['id', 'record_id'], + limit: PENDING_APPROVER_READ_CAP, + context: SYSTEM_CTX, + }); + const list: any[] = Array.isArray(rows) ? rows : []; + this.warnOnWindow(list.length, 'sys_approval_request'); + const grants = new Map(); + for (const row of list) { + if (row?.record_id == null || row?.id == null) continue; + const recordId = String(row.record_id); + const requests = grants.get(recordId) ?? []; + requests.push(String(row.id)); + grants.set(recordId, requests); + } + return grants; + } + + /** + * The pending requests whose slate names the caller as a person, or `null` + * for the override holder, whose reach is every pending request of their + * organization. One indexed read, the same one "My Pending" makes, on the + * person addresses only. + */ + private async slateRequestIds(context: ExecutionContext, org: string | null): Promise { + if (this.deps.isOverrideActor(context, org)) return null; + const userId = String(context.userId); + const email = await this.deps.accountEmail(userId); + const persons = email ? [userId, email] : [userId]; + const rows = await this.deps.engine.find('sys_approval_approver', { + where: { approver: persons.length === 1 ? persons[0] : { $in: persons }, ...organizationFence(org) }, + fields: ['request_id'], + limit: PENDING_APPROVER_READ_CAP, + context: SYSTEM_CTX, + }); + const list: any[] = Array.isArray(rows) ? rows : []; + this.warnOnWindow(list.length, 'sys_approval_approver'); + return [...new Set(list.map((r) => String(r?.request_id ?? '')).filter(Boolean))]; + } + + private warnOnWindow(count: number, table: string): void { + if (count < PENDING_APPROVER_READ_CAP) return; + this.deps.logger?.warn?.( + `[approvals] the pending approver's record-read probe on '${table}' hit its window — some records a pending ` + + 'approver may read are not granted on this read', + { cap: PENDING_APPROVER_READ_CAP, table }, + ); + } +} diff --git a/packages/plugins/plugin-security/src/explain-engine.test.ts b/packages/plugins/plugin-security/src/explain-engine.test.ts index 4f7affade17..651054099bd 100644 --- a/packages/plugins/plugin-security/src/explain-engine.test.ts +++ b/packages/plugins/plugin-security/src/explain-engine.test.ts @@ -539,6 +539,57 @@ describe('explainAccess — record-grained (C2 / ADR-0095)', () => { expect(d.record).toMatchObject({ visible: true, decidedBy: 'sharing' }); }); + // [#26180] A pending approval step the principal is on grants READ of the + // one record: the sharing service ORs its record id in, and explain names + // the request it came through — on a read, and as no grant on a write. + it('[#26180] names the pending approval request a read comes through: "via approval request N"', async () => { + const asked: unknown[] = []; + const deps = (canEdit?: boolean) => ({ + ...recDeps({ + layered: { layer0: null, layer1: null }, + record: { id: 'r1', organization_id: 'org1', owner_id: 'u_other' }, + shares: [], + sharingFilter: { $or: [{ owner_id: 'u1' }, { id: { $in: ['r1'] } }] }, + canEdit, + }), + listApprovalReadGrants: async (object: string, recordId: string, context: unknown) => { + asked.push([object, recordId, context]); + return ['apr_7']; + }, + }); + const read = await explainAccess(deps(), { object: 'leave_request', operation: 'read', context: REC_CTX, recordId: 'r1' }); + const sharing = read.layers.find((l) => l.layer === 'sharing')!; + expect(sharing.record!.outcome).toBe('admitted'); + expect(sharing.record!.rules).toEqual([ + { kind: 'record_share', name: 'apr_7', grants: 'read', via: 'approval request apr_7', effect: 'admits' }, + ]); + expect(sharing.record!.detail).toContain('via approval request apr_7'); + expect(read.record).toMatchObject({ visible: true, decidedBy: 'sharing' }); + // Asked with the EXPLAINED principal's context, for this record. + expect(asked).toEqual([['leave_request', 'r1', REC_CTX]]); + + // A write: the same grant is evaluated, grants no write, and the gate decides. + const update = await explainAccess(deps(false), { object: 'leave_request', operation: 'update', context: REC_CTX, recordId: 'r1' }); + const writeSharing = update.layers.find((l) => l.layer === 'sharing')!; + expect(writeSharing.record!.rules).toEqual([ + { kind: 'record_share', name: 'apr_7', grants: 'read', via: 'approval request apr_7', effect: 'neutral' }, + ]); + expect(writeSharing.record!.outcome).toBe('excluded'); + expect(update.record).toMatchObject({ visible: false }); + }); + + it('[#26180] a failing approval lookup attributes nothing and fails nothing', async () => { + const d = await explainAccess( + { + ...recDeps({ layered: { layer0: null, layer1: null }, record: { id: 'r1', organization_id: 'org1', owner_id: 'u_other' }, shares: [], sharingFilter: { owner_id: 'u1' } }), + listApprovalReadGrants: async () => { throw new Error('approvals unavailable'); }, + }, + { object: 'leave_request', operation: 'read', context: REC_CTX, recordId: 'r1' }, + ); + expect(d.layers.find((l) => l.layer === 'sharing')!.record!.rules).toEqual([]); + expect(d.record).toMatchObject({ visible: false, decidedBy: 'sharing' }); + }); + it('private object, non-owner, no admitting share — not visible, decidedBy sharing', async () => { const d = await explainAccess( recDeps({ layered: { layer0: null, layer1: null }, record: { id: 'r1', organization_id: 'org1', owner_id: 'u_other' }, shares: [], sharingFilter: { owner_id: 'u1' } }), diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index f984c61aa23..9c43af7f8fb 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -419,6 +419,14 @@ export interface ExplainEngineDeps { recordId: string, context: any, ) => Promise>; + /** + * [#26180] The pending approval requests that grant the principal READ of + * the record while their step is pending (`SharingService.listApprovalReadGrants`, + * the same answer the sharing read filter ORs in). Each is attributed on the + * sharing layer as a read share "via approval request N". Optional: a deps + * bag wired without it attributes no approval, as before. + */ + listApprovalReadGrants?: (object: string, recordId: string, context: any) => Promise; /** The sharing service's per-record UPDATE gate (`canEdit`) — the by-construction verdict for update operations. */ canEditRecord?: (object: string, recordId: string, context: any) => Promise; /** @@ -1848,6 +1856,22 @@ async function applyRecordAttribution( effect: recipMatchesUser ? 'admits' : 'neutral', }; }); + // [#26180] A pending approval step the principal is on grants READ of the + // one record it is about, while it is pending: named as the read share it is + // ("via approval request N"). It admits a read and nothing else: on a write + // it is evaluated and neutral, as the write gates never read it. + const approvalRequests = deps.listApprovalReadGrants && recordExists + ? await deps.listApprovalReadGrants(object, recordId, context).catch(() => [] as string[]) + : []; + for (const requestId of approvalRequests ?? []) { + shareRules.push({ + kind: 'record_share', + name: String(requestId), + grants: 'read', + via: `approval request ${requestId}`, + effect: isRead ? 'admits' : 'neutral', + }); + } // A rejected read filter is not a `null` one: the matcher reads `null` as // "no filter" and admits the row, while the find it explains fails. const sharingFilterOrFault = deps.sharingReadFilter && recordExists @@ -2036,7 +2060,10 @@ async function applyRecordAttribution( ? (ownerIsMe ? 'Caller owns the record — visible without a share.' : anyShareAdmits - ? `${shareRules.length} share(s) attached; access is granted for this record.` + ? (isRead && approvalRequests.length > 0 + ? `Read is granted via approval request ${approvalRequests.join(', ')}: the principal is on ` + + "its pending step's slate or holds the override, and the grant ends when the step resolves." + : `${shareRules.length} share(s) attached; access is granted for this record.`) : sharingFilter === null ? `${shareRules.length} share(s) attached; none grants the caller access, but the sharing ` + 'service imposes no owner-match on this object (rowFilter null), so the record is admitted without one.' diff --git a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts index 5d6bf8e0660..98a0eda3b3c 100644 --- a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts +++ b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts @@ -417,6 +417,13 @@ export async function explainAccessForCaller( // manager — the exact behaviour this binding had before the gate. ? { listRecordShares: (o: string, rid: string) => sharing.listShares(o, rid, { isSystem: true }) } : {}), + // [#26180] The pending approval requests that grant the EXPLAINED + // principal read of the record — the same answer the sharing read filter + // ORs in, asked with the explained context, so the attribution names the + // request the filter admitted the row through. + ...(sharing && typeof sharing.listApprovalReadGrants === 'function' + ? { listApprovalReadGrants: (o: string, rid: string, c: any) => sharing.listApprovalReadGrants(o, rid, c) } + : {}), ...(sharing && typeof sharing.canEdit === 'function' ? { canEditRecord: delegatedWriteGate((o, rid, c) => sharing.canEdit(o, rid, c)) } : {}), diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index e19ba4a77b1..ca82a5b30fe 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -53,6 +53,7 @@ import { SysBusinessUnit, SysBusinessUnitMember } from '@objectstack/platform-ob import { SharingService, type SharingEngine, + type SharingApprovalReadProbe, type SharingSecurityProbe, type SharingTenancyProbe, } from './sharing-service.js'; @@ -710,6 +711,13 @@ export class SharingServicePlugin implements Plugin { try { return ctx.getService('tenancy'); } catch { return null; } }, + // [#26180] Late-bound `approvals` service: the records a pending + // approver may read, ORed into the read filter. Optional plugin, so + // the slice is declared structurally; absent → no approval grant. + approvals: () => { + try { return ctx.getService('approvals'); } + catch { return null; } + }, }); ctx.registerService('sharing', this.service); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index 9b44a430272..5e3e0290038 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -229,6 +229,100 @@ describe('SharingService.buildReadFilter', () => { }); }); +// [#26180 · ruling 6429330267, letter A] The records a pending approval step +// lets the caller READ, asked of the late-bound `approvals` service and ORed in +// beside the record shares. The derivation (who, while, where, cost) is the +// approvals service's own suite; the door, on a real boot, is dogfood's +// `approval-pending-approver-record-read`. What this level states is the +// composition: the ids join the shares on the READ filter only, and every +// failure grants nothing. +describe('[#26180] SharingService — the pending approver\'s read', () => { + let engine: ReturnType; + const grants = (entries: Array<[string, string[]]>) => new Map(entries); + const serviceWith = (approvals: SharingServiceOptions['approvals']) => new SharingService({ engine, approvals }); + const asked: Array<{ object: string; userId?: string }> = []; + const pendingOn = (entries: Array<[string, string[]]>) => () => ({ + pendingApproverReadGrants: async (object: string, context: any) => { + asked.push({ object, userId: context?.userId }); + return grants(entries); + }, + }); + + beforeEach(() => { + engine = makeFakeEngine({ + account: ACCOUNT_SCHEMA, + lead: LEAD_SCHEMA, + sys_record_share: { name: 'sys_record_share' }, + }); + asked.length = 0; + }); + + it('ORs the pending-approval record ids in beside the owner-match', async () => { + const svc = serviceWith(pendingOn([['a9', ['apr_1']]])); + expect(await svc.buildReadFilter('account', { userId: 'alice' })).toEqual({ + $or: [{ owner_id: 'alice' }, { id: { $in: ['a9'] } }], + }); + expect(asked).toEqual([{ object: 'account', userId: 'alice' }]); + }); + + it('joins the record shares, once per id', async () => { + await new SharingService({ engine }).grant({ object: 'account', recordId: 'a1', recipientId: 'alice' }, { isSystem: true }); + const svc = serviceWith(pendingOn([['a1', ['apr_1']], ['a9', ['apr_2']]])); + const f: any = await svc.buildReadFilter('account', { userId: 'alice' }); + expect(f.$or[0]).toEqual({ owner_id: 'alice' }); + expect([...f.$or[1].id.$in].sort()).toEqual(['a1', 'a9']); + }); + + it('never reaches a write gate: an approver on the slate who owns nothing edits and deletes nothing', async () => { + const svc = serviceWith(pendingOn([['a9', ['apr_1']]])); + await engine.insert('account', { id: 'a9', owner_id: 'bob' }); + expect(await svc.canEdit('account', 'a9', { userId: 'alice' })).toBe(false); + expect(await svc.canDelete('account', 'a9', { userId: 'alice' })).toBe(false); + expect(await svc.buildWriteFilter('account', { userId: 'alice' })).toEqual({ owner_id: 'alice' }); + }); + + it('is not asked where the read filter imposes nothing, or denies all', async () => { + const svc = serviceWith(pendingOn([['a9', ['apr_1']]])); + expect(await svc.buildReadFilter('lead', { userId: 'alice' })).toBeNull(); + expect(await svc.buildReadFilter('account', {})).toEqual({ id: '__deny_all__' }); + expect(asked).toEqual([]); + }); + + it('grants nothing to an on-behalf-of principal: the grant is the person\'s own (ADR-0090 D10)', async () => { + const svc = serviceWith(pendingOn([['a9', ['apr_1']]])); + const delegated = { userId: 'agent_1', onBehalfOf: { userId: 'alice' } } as any; + expect(await svc.buildReadFilter('account', delegated)).toEqual({ owner_id: 'agent_1' }); + expect(await svc.listApprovalReadGrants('account', 'a9', delegated)).toEqual([]); + expect(asked).toEqual([]); + }); + + it('fails closed: no service, no member, a throwing lookup or a rejected read all grant nothing', async () => { + const ownerOnly = { owner_id: 'alice' }; + const warn = vi.fn(); + const variants: Array = [ + undefined, + () => null, + () => { throw new Error('[Kernel] Service \'approvals\' not found'); }, + () => ({}), + () => ({ pendingApproverReadGrants: async () => { throw new Error('store unavailable'); } }), + ]; + for (const approvals of variants) { + const svc = new SharingService({ engine, approvals, logger: { warn } }); + expect(await svc.buildReadFilter('account', { userId: 'alice' })).toEqual(ownerOnly); + expect(await svc.listApprovalReadGrants('account', 'a9', { userId: 'alice' })).toEqual([]); + } + // The rejected read is the one failure that is said, once per read. + expect(warn).toHaveBeenCalledTimes(2); + expect(String(warn.mock.calls[0][0])).toContain('granting none (fail-closed)'); + }); + + it('names, for explain, the requests that grant the one record', async () => { + const svc = serviceWith(pendingOn([['a9', ['apr_1', 'apr_2']], ['a8', ['apr_3']]])); + expect(await svc.listApprovalReadGrants('account', 'a9', { userId: 'alice' })).toEqual(['apr_1', 'apr_2']); + expect(await svc.listApprovalReadGrants('account', 'a7', { userId: 'alice' })).toEqual([]); + }); +}); + // [ADR-0138 D3 item 6, as the maintainer's ruling C amends it — comment // 6148183570 on #23266] The guest reaches rows through share links only, never // through the object's sharing model: its read filter is deny-all on every diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index c1f616f503c..6c98a6a502f 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -335,6 +335,9 @@ function hasOwnerField(schema: any): boolean { * precisely what `filter(Boolean)` used to drop — so the non-null path is * unchanged and only the nullish rows are newly excluded. */ +/** [#26180] No pending approval grants a read: the fail-closed answer. */ +const NO_APPROVAL_GRANTS: ReadonlyMap = new Map(); + function grantedRecordIds(grants: unknown): string[] { if (!Array.isArray(grants)) return []; return grants @@ -460,6 +463,21 @@ export interface SharingTenancyProbe { readonly isolationActive?: boolean; } +/** + * [#26180 · ruling 6429330267, letter A] The one member of the `approvals` + * service the read filter asks: the records of an object the caller may READ + * because a pending approval step waits on them (record id → the pending + * request ids that grant it). Structural, as {@link SharingSecurityProbe} is: + * this plugin does not depend on `@objectstack/plugin-approvals`, it declares + * the slice it reads, and a stack without approvals grants nothing. + */ +export interface SharingApprovalReadProbe { + pendingApproverReadGrants?( + object: string, + context: ExecutionContext, + ): Promise>; +} + export interface SharingServiceOptions { engine: SharingEngine; /** Object names that bypass sharing — typically platform internals. */ @@ -491,6 +509,13 @@ export interface SharingServiceOptions { * refuses: an unresolvable posture must not be read as "no wall, carry on". */ tenancy?: () => SharingTenancyProbe | null | undefined; + /** + * [#26180] Late-bound lookup for the `approvals` service, asked by + * {@link SharingService.buildReadFilter} for the records a pending approver + * may read. Absent / throwing / returning null → no approval grant: the + * filter is the owner-match and the record shares, as without approvals. + */ + approvals?: () => SharingApprovalReadProbe | null | undefined; /** [#5103] Optional logger for the record-delete cascade / orphan sweep. */ logger?: { info?: (msg: any, ...rest: any[]) => void; @@ -540,6 +565,7 @@ export class SharingService implements ISharingService { private readonly hierarchyResolver?: () => IHierarchyScopeResolver | null | undefined; private readonly securityService?: () => SharingSecurityProbe | null | undefined; private readonly tenancy?: () => SharingTenancyProbe | null | undefined; + private readonly approvals?: () => SharingApprovalReadProbe | null | undefined; private readonly logger?: SharingServiceOptions['logger']; /** @@ -567,6 +593,7 @@ export class SharingService implements ISharingService { this.hierarchyResolver = options.hierarchyResolver; this.securityService = options.securityService; this.tenancy = options.tenancy; + this.approvals = options.approvals; this.logger = options.logger; this.bypassObjects = new Set([ 'sys_record_share', @@ -658,7 +685,13 @@ export class SharingService implements ISharingService { context: SYSTEM_CTX, }); - const grantedIds: string[] = grantedRecordIds(grants); + // [#26180] …and the records a pending approval step waits on the caller + // for: READ only (no write gate reads this), only while the step is + // pending, never across organizations — the approvals service's rules. + const approvalIds = [...(await this.pendingApproverReadGrants(object, context)).keys()]; + const grantedIds: string[] = approvalIds.length === 0 + ? grantedRecordIds(grants) + : [...new Set([...grantedRecordIds(grants), ...approvalIds])]; if (grantedIds.length === 0) { return ownerMatch; @@ -672,6 +705,48 @@ export class SharingService implements ISharingService { }; } + /** + * [#26180] The pending approval requests that grant the caller READ of + * `recordId` — what `explain` names ("via approval request N") beside the + * record's shares. The same answer {@link buildReadFilter} ORs in, so the two + * cannot disagree; empty where the filter would grant nothing. + */ + async listApprovalReadGrants(object: string, recordId: string, context: ExecutionContext): Promise { + return [...((await this.pendingApproverReadGrants(object, context)).get(String(recordId)) ?? [])]; + } + + /** + * [#26180] The `approvals` service's answer for this read, failing CLOSED: + * no service, no member, a delegated (on-behalf-of) principal, or a read that + * throws all grant nothing, and the read goes on with the owner-match and the + * record shares alone. The grant is the person's own, as they act themselves: + * an agent acting on their behalf is not on the slate, and its read is never + * widened by the delegator's grants (ADR-0090 D10). + */ + private async pendingApproverReadGrants( + object: string, + context: ExecutionContext, + ): Promise> { + if (!context?.userId || context.onBehalfOf?.userId) return NO_APPROVAL_GRANTS; + let probe: SharingApprovalReadProbe | null | undefined; + try { + probe = this.approvals?.(); + } catch { + return NO_APPROVAL_GRANTS; + } + if (!probe || typeof probe.pendingApproverReadGrants !== 'function') return NO_APPROVAL_GRANTS; + try { + return (await probe.pendingApproverReadGrants(object, context)) ?? NO_APPROVAL_GRANTS; + } catch (e) { + this.logger?.warn?.( + `[sharing] the pending approver's read of '${object}' could not be resolved — granting none ` + + `(fail-closed): an approver on a pending step reads only what their own grants reach until it resolves`, + { object, userId: context.userId, error: e instanceof Error ? e.message : String(e) }, + ); + return NO_APPROVAL_GRANTS; + } + } + /** * Build a `FilterCondition` restricting a **bulk** (multi-row) write to the * records the caller may edit — the write analogue of {@link buildReadFilter}. From bd22137695169563d39d2dce5c75fde060b955d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:04:31 +0000 Subject: [PATCH 3/5] test(plugin-approvals): stamp the unrelated write's organization in the memo-retirement pin Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude --- .../src/pending-approver-read-grant.integration.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts index 4b0f3694453..d7e66073699 100644 --- a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.integration.test.ts @@ -250,7 +250,7 @@ describe('pendingApproverReadGrants — the records a pending approval step lets expect(rig.finds).toEqual(['sys_user', 'sys_approval_approver']); // Any write through the engine retires the memo: the next read asks again. - await rig.engine.insert(OTHER, { id: 'm_1', title: 'unrelated' }, { context: SYSTEM }); + await rig.engine.insert(OTHER, { id: 'm_1', title: 'unrelated', organization_id: DEFAULT_ORGANIZATION_ID }, { context: SYSTEM }); rig.finds.length = 0; await grantsOf(rig, OBJECT, ctx); expect(rig.finds).toEqual(['sys_user', 'sys_approval_approver', 'sys_approval_request']); From 1def5baf5875f5c6ca0e51b3587bef3c71ecfd77 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:15:23 +0000 Subject: [PATCH 4/5] fix(plugin-approvals): take the pending approver's person addresses from approver-address.ts, and classify the site Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude --- .../plugin-approvals/src/approver-address-readers.test.ts | 5 +++++ .../plugin-approvals/src/pending-approver-read-grant.ts | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts b/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts index 1d1b3d69a73..a5fe95cf16b 100644 --- a/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts +++ b/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts @@ -147,6 +147,11 @@ const SLOT_SITES: Readonly> = { why: 'a presence test, not a comparison: selects the action rows that hold a slot to label (#23889 boot ' + 'backfill); the slot is compared with nothing, and no caller identity is involved', }, + 'pending-approver-read-grant.ts · slateRequestIds · approver: persons.length === 1 ? persons[0] : { $in: persons }': { + role: 'reader', + why: 'the pending approver\'s record read (#26180): `persons` is actingAddresses(caller) with no positions — the ' + + 'user id and account email slateNamesAPerson calls a person; a position literal names no person, so grants no read', + }, "sys-approval-request.object.ts · (module) · field: 'pending_approvers'": { role: 'declarative', why: 'the `my_pending` list view\'s substring filter on {current_user_id}; served only by the generic data ' diff --git a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts index c2798831fde..a1318cefdd2 100644 --- a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts +++ b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts @@ -1,6 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { actingAddresses } from './approver-address.js'; /** * [#26180 · ruling 6429330267, letter A] The records a caller may READ because @@ -189,8 +190,9 @@ export class PendingApproverReadGrantSource { private async slateRequestIds(context: ExecutionContext, org: string | null): Promise { if (this.deps.isOverrideActor(context, org)) return null; const userId = String(context.userId); - const email = await this.deps.accountEmail(userId); - const persons = email ? [userId, email] : [userId]; + // The caller's acting addresses with no positions: the user id and the + // account email, the two `slateNamesAPerson` calls a person. + const persons = actingAddresses({ userId, email: await this.deps.accountEmail(userId), positions: [] }); const rows = await this.deps.engine.find('sys_approval_approver', { where: { approver: persons.length === 1 ? persons[0] : { $in: persons }, ...organizationFence(org) }, fields: ['request_id'], From e8fb0d3bb12fbb202af1ba9789227e8585cb055c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:21:46 +0000 Subject: [PATCH 5/5] refactor(plugin-approvals): hold only answers that were read in the pending approver's memo A rejected read leaves nothing behind, with no cleanup step; the system context needs no branch of its own (the sharing filter bypasses it). Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude --- .../src/pending-approver-read-grant.ts | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts index a1318cefdd2..a38db4a7a18 100644 --- a/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts +++ b/packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts @@ -52,7 +52,7 @@ import { actingAddresses } from './approver-address.js'; * `sys_approval_request` read per object. The override holder pays one * `sys_approval_request` read per object. Never a read per row. * - * A failed read is not memoized and is rethrown: the caller (plugin-sharing) + * A failed read is never held and is rethrown: the caller (plugin-sharing) * fails closed and grants nothing. */ @@ -81,11 +81,12 @@ export const PENDING_APPROVER_READ_CAP = 10_000; const NONE: PendingApproverReadGrants = new Map(); +/** One request context's answers at one write epoch. Only an answer that was read is held. */ interface MemoEntry { epoch: number; /** The pending request ids naming the caller as a person; `null` for the override holder. */ - slate?: Promise; - byObject: Map>; + slate?: readonly string[] | null; + byObject: Map; } /** The caller's active organization, read as the service reads it everywhere else. */ @@ -112,20 +113,17 @@ export class PendingApproverReadGrantSource { /** The record ids of `object` the caller may read through a pending approval step. */ async forObject(object: string, context: ExecutionContext): Promise { - if (!object || !context || context.isSystem) return NONE; + // A system context never reaches here: the sharing filter bypasses it. + if (!object || !context) return NONE; const userId = context.userId != null ? String(context.userId) : ''; if (!userId) return NONE; const entry = this.entryFor(context); - if (!entry) return this.compute(object, context, undefined); - const cached = entry.byObject.get(object); + const cached = entry?.byObject.get(object); if (cached) return cached; - const pending = this.compute(object, context, entry); - entry.byObject.set(object, pending); - // A failed read is never reused: the next read asks again. - pending.catch(() => { - if (entry.byObject.get(object) === pending) entry.byObject.delete(object); - }); - return pending; + // A failed read rejects before anything is held: the next read asks again. + const grants = await this.compute(object, context, entry); + entry?.byObject.set(object, grants); + return grants; } /** The memo entry for this context at the current epoch, or `undefined` when nothing may be reused. */ @@ -145,18 +143,10 @@ export class PendingApproverReadGrantSource { entry: MemoEntry | undefined, ): Promise { const org = activeOrganization(context); - let slate: readonly string[] | null; - if (entry) { - if (!entry.slate) { - entry.slate = this.slateRequestIds(context, org); - const held = entry.slate; - held.catch(() => { - if (entry.slate === held) entry.slate = undefined; - }); - } - slate = await entry.slate; - } else { + let slate = entry?.slate; + if (slate === undefined) { slate = await this.slateRequestIds(context, org); + if (entry) entry.slate = slate; } if (slate !== null && slate.length === 0) return NONE;