From 3edfc13ff88c3b5361bc58302dccedce1e35dead Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 11:21:11 +0000 Subject: [PATCH 1/3] fix(plugin-approvals): a payload_display lookup target is titled only for a target record the reader may read enrichRows asks the platform's caller-scoped row-access answer (resolveReadableParentIds, @objectstack/core) for the lookup targets of a read, once per target object, and titles only the readable ones. An unreadable target gets no display value and its stored id stands, as a deleted one does. The subject's title and snapshot are unchanged. Claude-Session: https://claude.ai/code/session_017WLmd5in9RXu7AjyHR8fKQ Co-authored-by: Claude --- .../23637-lookup-target-title-row-access.md | 12 + .../plugin-approvals/src/approval-service.ts | 39 ++- .../implicit-reference-target-lookup.test.ts | 16 +- ...arget-title-row-access.integration.test.ts | 292 ++++++++++++++++++ 4 files changed, 348 insertions(+), 11 deletions(-) create mode 100644 .changeset/23637-lookup-target-title-row-access.md create mode 100644 packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts diff --git a/.changeset/23637-lookup-target-title-row-access.md b/.changeset/23637-lookup-target-title-row-access.md new file mode 100644 index 00000000000..b0b5e46d9c0 --- /dev/null +++ b/.changeset/23637-lookup-target-title-row-access.md @@ -0,0 +1,12 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-approvals): the inbox's referenced-record titles (`payload_display`) are served only for a referenced record the caller may read + +Clause-②: no + +- **Referenced-record titles follow the reader's access to the referenced RECORD.** `listRequests` and `getRequest` now title a lookup in the payload snapshot only when the reading caller may read the record it points at, asked through the platform's one caller-scoped row-access answer (`resolveReadableParentIds`, the same one the comment, activity, attachment and mail gates ask), once per referenced object per read. So the referenced object's object-level read, its sharing and its row-level rules decide, as they already do for the data API's `$expand`. A referenced record the reader may not read gets no display value and the snapshot's stored id stands, as it already does for a deleted referenced record. A reader who may read the record gets the same title as before, still picked from the fields they may be served. +- **Nothing about the record under approval changes.** Its title (`record_title`), its label and its field-level-security-redacted snapshot are served to a routed approver exactly as before, whatever their access to that record. + +A patch: a narrowing of served values only. Nothing is granted, and no schema, route, error code, translation key or export moves. With no security plugin composed, every caller-scoped read answers every record, so every title is served as before. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 0a8b5fc6121..92dcd37fe00 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -64,7 +64,12 @@ import type { EnableLike, ObjectCapabilitiesParsed } from '@objectstack/spec/dat // mechanism for a second producer, and commit aa5994e17 landed this service's key // (`approval_recall_not_submitter`) into it ahead of this consumer half. import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system'; -import { isGrantActive, omitInternalFieldsFromWriteResponse, resolveUserAuthzGrants } from '@objectstack/core'; +import { + isGrantActive, + omitInternalFieldsFromWriteResponse, + resolveReadableParentIds, + resolveUserAuthzGrants, +} from '@objectstack/core'; import { filterApproversWhoCanRead, resolveApproverDirectoryOrg, @@ -6692,6 +6697,8 @@ export class ApprovalService implements IApprovalService { * the same target. It takes no caller: the read runs under a system context. * [#23618] What is SERVED from that read is the reader's: the title is picked * from the target as served to the reader ({@link titlesAsServed}). + * [#23637] And only for a target record the reader may read, asked as the + * reader after this gate ({@link ApprovalService.enrichRows}). * * A refused target is not read, so its key gets no display value and the * snapshot's stored id stands, the answer a deleted target already gets. A @@ -6786,8 +6793,10 @@ export class ApprovalService implements IApprovalService { * and the lookup targets' `payload_display` ({@link ApprovalService.enrichRows}). * The records are read under the system context, so the read narrows nothing * (#7497 ruling C: record access stays with object permissions, sharing and - * row-level rules); only what is served from it is narrowed. Throws what the - * read throws; each caller keeps its own best-effort catch. + * row-level rules); only what is served from it is narrowed. Which `ids` are + * read is the caller's: the subject's are every request's record, the + * targets' only those the reader may read ([#23637], {@link enrichRows}). + * Throws what the read throws; each caller keeps its own best-effort catch. */ private async titlesAsServed( object: string, @@ -6859,6 +6868,15 @@ export class ApprovalService implements IApprovalService { * target gets. The targets' answers are asked through * {@link servedFields} into `served`, once per object per read, so a target * that is also a subject object reuses the subject's answer. + * [#23637] And only a target RECORD the reader may read is titled at all: + * the readable ids come from `resolveReadableParentIds` (`@objectstack/core`), + * the one caller-scoped "which of these records may this caller read" answer + * the parent-derived read gates ask, so the target object's object-level + * read, sharing and row-level rules decide, as they do for the data door's + * `$expand`. One ask per target object per read. An unreadable target gets + * no display value and its stored id stands, as a deleted one does. The + * SUBJECT is not asked: its title and snapshot are served whatever the + * reader's access to the subject record (#7497 ruling C, #10749). */ private async enrichRows( rows: ApprovalRequestRow[], @@ -6901,11 +6919,22 @@ export class ApprovalService implements IApprovalService { } } const refTitles = new Map(); - const targets = Array.from(refIds.keys()).filter((object) => this.servesPayloadDisplayTarget(object)); + const exposed = Array.from(refIds.keys()).filter((object) => this.servesPayloadDisplayTarget(object)); + // [#23637] Only the target RECORDS the reader may read are titled: the + // platform's one caller-scoped row-access answer, one ask per target object + // per read. An object the reader may not read, a row its sharing or + // row-level rules hide, and a deleted record all come back absent, so that + // key gets no display value and the snapshot's stored id stands. + const readableIds = await resolveReadableParentIds( + this.engine, + context, + new Map(exposed.map((object) => [object, refIds.get(object)!])), + ); + const targets = exposed.filter((object) => (readableIds.get(object)?.size ?? 0) > 0); await this.servedFields(targets, context, served); for (const object of targets) { try { - await this.titlesAsServed(object, refIds.get(object)!, served.get(object), refTitles); + await this.titlesAsServed(object, readableIds.get(object)!, served.get(object), refTitles); } catch { /* referenced object unreadable — leave unresolved */ } } diff --git a/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts b/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts index 70b1573cf7b..2c17b049a0c 100644 --- a/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts +++ b/packages/plugins/plugin-approvals/src/implicit-reference-target-lookup.test.ts @@ -27,8 +27,8 @@ import { ApprovalService } from './approval-service.js'; type FieldDefs = Record; type Row = Record; -/** What the enrichment read, in the order it read it. */ -interface FindCall { object: string; ids: string[] } +/** What the enrichment read, in the order it read it, and whether as the system. */ +interface FindCall { object: string; ids: string[]; system: boolean } /** The two engine members the enrichment path reads. */ function makeEngine( @@ -38,9 +38,9 @@ function makeEngine( const finds: FindCall[] = []; const engine = { getSchema: (object: string) => schemas[object], - async find(object: string, options?: { where?: { id?: { $in?: string[] } } }) { + async find(object: string, options?: { where?: { id?: { $in?: string[] } }; context?: { isSystem?: boolean } }) { const wanted = options?.where?.id?.$in; - finds.push({ object, ids: wanted ?? [] }); + finds.push({ object, ids: wanted ?? [], system: options?.context?.isSystem === true }); const rows = tables[object] ?? []; return wanted ? rows.filter(r => wanted.includes(String(r.id))) : rows; }, @@ -155,9 +155,13 @@ describe('inbox display enrichment — the previously silent path', () => { // The read the silent path never issued. `assignee` is this object's ONLY // reference field, so before the repair the enrichment asked `sys_user` // nothing at all — this is the half a happy-value assertion cannot see. + // [#23637] Two reads of it: the reader's row-access ask for the target + // record, then the title read of the record the reader may read. const userReads = finds.filter(f => f.object === 'sys_user'); - expect(userReads).toHaveLength(1); - expect(userReads[0]?.ids).toEqual(['u7']); + expect(userReads).toEqual([ + { object: 'sys_user', ids: ['u7'], system: false }, + { object: 'sys_user', ids: ['u7'], system: true }, + ]); // …and the value the reviewer actually sees, instead of the raw id. expect(rows[0]?.payload_display).toEqual({ assignee: 'Grace Hopper' }); diff --git a/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts b/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts new file mode 100644 index 00000000000..8e63be66e36 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts @@ -0,0 +1,292 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23637] A `payload_display` title — the title of the record a snapshot's + * lookup points at — is served to a reader only for a target RECORD that reader + * may read. + * + * ## The contract + * + * The target's title is read under the system context, so the read itself + * narrows nothing: before this card it was narrowed by the target's declared + * API exposure (#22738) and by the reader's field-level security on the target + * object (#23618), but never by whether the reader may read the target record + * at all. The two other doors that title a lookup target judge exactly that: + * the data door's `$expand` reads the target as the caller, and the dataset + * door's labels are row-scope filtered. #7497 ruling C keeps record access with + * object permissions, sharing and the application's row-level rules, and gives + * an approver the inbox entry, the request detail with the FLS-redacted + * snapshot, and the actions — the snapshot holds the target's id, not its + * title. So the title is served only for a target record the platform's one + * caller-scoped row-access answer (`resolveReadableParentIds`, + * `@objectstack/core`) says the reader may read; an unreadable target gets no + * display value and the stored id stands, the answer a deleted target already + * gets. Nothing about the SUBJECT moves: its title and snapshot are served as + * before, whatever the reader's access to the subject record. + * + * ## What this pins + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory). Record readability is the one + * stand-in, in place of `plugin-security`: one middleware per business object + * refuses a caller with no object read and scopes one with read to the rows it + * may read — the two answers a caller-scoped read gets from the security + * middleware (a refusal, or a narrowed result). No field-visibility source is + * attached, so every field passes through (the FLS half is + * `lookup-target-title-reader-fls.integration.test.ts`). + * + * (a) a reader denied the target ROW (a row rule on a readable object, no + * object read at all, a self-lookup to a hidden row) gets no title for + * that key, on the single read and on the list, and the stored id stands + * — the shape a deleted target gives; + * (a-row) per record, not per object: in one list, the target row the reader + * may read keeps its title while a hidden row of the same object does not; + * (b) the control: a reader who may read every target row gets every title; + * (c) one row-access ask per target object per read, naming every id of that + * object the read titles, on the single read and on a multi-row list; + * (d) the subject is untouched: a reader who may read no subject row is + * served the subject's title, label and snapshot exactly as the control. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; + +/** May read only `A_MINE` of the accounts, no vendor at all, and no deal row. */ +const RESTRICTED = 'u_row_restricted'; +/** May read every row (the control). */ +const READER = 'u_row_reader'; +const SUBMITTER = 'u_row_sub'; + +/** Target titles the restricted reader must never be served. */ +const ACCOUNT_TITLE = 'Globex hidden account'; +const VENDOR_TITLE = 'Initech vendor, no object read'; +const PARENT_TITLE = 'Parent deal, hidden row'; +/** The account row the restricted reader may read. */ +const MINE_TITLE = 'Umbrella account, readable'; + +const account = { + name: 'crm_account', + label: 'Account', + nameField: 'name', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; +const vendor = { + name: 'crm_vendor', + label: 'Vendor', + nameField: 'name', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; +const deal = { + name: 'crm_deal', + label: 'Deal', + nameField: 'name', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + amount: { name: 'amount', label: 'Amount', type: 'number' as const }, + account: { name: 'account', label: 'Account', type: 'lookup' as const, reference: 'crm_account' }, + vendor: { name: 'vendor', label: 'Vendor', type: 'lookup' as const, reference: 'crm_vendor' }, + parent: { name: 'parent', label: 'Parent deal', type: 'lookup' as const, reference: 'crm_deal' }, + }, +}; +const sysUser = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; + +const READ_OPS = new Set(['find', 'findOne', 'count', 'aggregate']); +/** object → user → the rows that user may read; a user absent here has no object read. */ +const READABLE: Record | 'all'>> = { + crm_account: { [RESTRICTED]: new Set(['A_MINE']), [READER]: 'all' }, + crm_vendor: { [READER]: 'all' }, + crm_deal: { [RESTRICTED]: new Set(), [READER]: 'all' }, +}; + +/** A caller-scoped read of a business object, as the stand-in saw it. */ +interface CallerRead { object: string; userId: string; ids: string[] } + +/** Record readability, the stand-in (see the header), on the three business objects. */ +function guardReads(engine: ObjectQL, reads: CallerRead[]): void { + for (const object of Object.keys(READABLE)) { + engine.registerMiddleware(async (op: any, next: () => Promise) => { + const ctx = op.context; + if (READ_OPS.has(op.operation) && ctx && !ctx.isSystem && op.ast) { + const userId = String(ctx.userId ?? ''); + const inList = op.ast.where?.id?.$in; + reads.push({ object, userId, ids: Array.isArray(inList) ? [...inList].map(String).sort() : [] }); + const grant = READABLE[object][userId]; + if (!grant) { + throw Object.assign(new Error(`[Security] Access denied: no read on '${object}'`), { + code: 'PERMISSION_DENIED', status: 403, + }); + } + if (grant !== 'all') { + const scope = { id: { $in: [...grant] } }; + op.ast.where = op.ast.where ? { $and: [op.ast.where, scope] } : scope; + } + } + return next(); + }, { object }); + } +} + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +describe('[#23637] a lookup target\'s title is served only for a target record its reader may read', () => { + let engine: ObjectQL; + let svc: ApprovalService; + const reads: CallerRead[] = []; + let n = 0; + + /** One pending request on a fresh deal whose snapshot points at the given targets. */ + const openDeal = async (targets: { account: string; vendor: string; parent: string }) => { + n += 1; + const recordId = `D${n}`; + const record = { id: recordId, name: `Deal ${n}`, amount: 100 * n, ...targets }; + await engine.insert('crm_deal', record, { context: SYSTEM } as any); + const row: any = await svc.openNodeRequest({ + object: 'crm_deal', recordId, runId: `run_${n}`, nodeId: 'review', flowName: 'deal_review', + config: { + approvers: [RESTRICTED, READER].map((value) => ({ type: 'user', value })), + behavior: 'first_response', + } as any, + submitterId: SUBMITTER, record, + }, asUser(SUBMITTER)); + return { requestId: String(row.id), recordId, record }; + }; + const hidden = { account: 'A1', vendor: 'V1', parent: 'D_PARENT' }; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, account, vendor, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + guardReads(engine, reads); + for (const id of [RESTRICTED, READER, SUBMITTER]) { + await engine.insert('sys_user', { id, name: id, email: `${id}@example.com` }, { context: SYSTEM } as any); + } + await engine.insert('crm_account', { id: 'A1', name: ACCOUNT_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_account', { id: 'A_MINE', name: MINE_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_vendor', { id: 'V1', name: VENDOR_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_deal', { id: 'D_PARENT', name: PARENT_TITLE, amount: 1 }, { context: SYSTEM } as any); + svc = new ApprovalService({ + engine: engine as any, + logger: { warn: () => {}, info: () => {}, error: () => {}, debug: () => {} } as any, + }); + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('(a) a reader denied the target row gets no title for it — single read and list; the stored id stands', async () => { + const { requestId, recordId } = await openDeal(hidden); + // The shape a deleted target gives, read off the same reader. + const gone = await openDeal({ account: 'A_GONE', vendor: 'V_GONE', parent: 'D_GONE' }); + + const one: any = await svc.getRequest(requestId, asUser(RESTRICTED)); + const [listed]: any[] = await svc.listRequests({ recordId }, asUser(RESTRICTED)); + const deleted: any = await svc.getRequest(gone.requestId, asUser(RESTRICTED)); + for (const [door, row] of [['getRequest', one], ['listRequests', listed]] as const) { + expect(row, door).toBeTruthy(); + // A row rule on a readable object, no object read, a self-lookup to a hidden row. + expect(row.payload_display ?? {}, door).not.toHaveProperty('account'); + expect(row.payload_display ?? {}, door).not.toHaveProperty('vendor'); + expect(row.payload_display ?? {}, door).not.toHaveProperty('parent'); + expect(row.payload_display, door).toEqual(deleted.payload_display); + // The stored ids stand. + expect(row.payload, door).toMatchObject(hidden); + for (const title of [ACCOUNT_TITLE, VENDOR_TITLE, PARENT_TITLE]) { + expect(JSON.stringify(row), door).not.toContain(title); + } + } + }); + + it('(a-row) per record, not per object: the target row the reader may read keeps its title', async () => { + const mine = await openDeal({ account: 'A_MINE', vendor: 'V1', parent: 'D_PARENT' }); + const theirs = await openDeal(hidden); + const rows: any[] = await svc.listRequests({ object: 'crm_deal', approverId: RESTRICTED }, asUser(RESTRICTED)); + const byRecord = new Map(rows.map((r) => [r.record_id, r])); + expect(byRecord.get(mine.recordId)?.payload_display).toEqual({ account: MINE_TITLE }); + expect(byRecord.get(theirs.recordId)?.payload_display ?? {}).not.toHaveProperty('account'); + expect(JSON.stringify(rows)).not.toContain(ACCOUNT_TITLE); + }); + + it('(b) control: a reader who may read every target row gets every title — single read and list', async () => { + const { requestId, recordId } = await openDeal(hidden); + const one: any = await svc.getRequest(requestId, asUser(READER)); + const [listed]: any[] = await svc.listRequests({ recordId }, asUser(READER)); + for (const [door, row] of [['getRequest', one], ['listRequests', listed]] as const) { + expect(row.payload_display, door).toEqual({ account: ACCOUNT_TITLE, vendor: VENDOR_TITLE, parent: PARENT_TITLE }); + } + }); + + it('(c) one row-access ask per target object per read, naming every id the read titles', async () => { + const first = await openDeal(hidden); + await openDeal({ account: 'A_MINE', vendor: 'V1', parent: 'D_PARENT' }); + + reads.length = 0; + await svc.getRequest(first.requestId, asUser(RESTRICTED)); + expect(reads).toEqual([ + { object: 'crm_account', userId: RESTRICTED, ids: ['A1'] }, + { object: 'crm_vendor', userId: RESTRICTED, ids: ['V1'] }, + { object: 'crm_deal', userId: RESTRICTED, ids: ['D_PARENT'] }, + ]); + + reads.length = 0; + const rows = await svc.listRequests({ object: 'crm_deal', approverId: RESTRICTED }, asUser(RESTRICTED)); + expect(rows.length).toBeGreaterThan(1); + expect(reads.map((r) => r.object)).toEqual(['crm_account', 'crm_vendor', 'crm_deal']); + expect(reads.find((r) => r.object === 'crm_account')?.ids).toEqual(expect.arrayContaining(['A1', 'A_MINE'])); + }); + + it('(d) the subject is untouched: a reader who may read no subject row is served it as the control is', async () => { + const { requestId, recordId, record } = await openDeal(hidden); + for (const door of ['getRequest', 'listRequests'] as const) { + const read = async (userId: string): Promise => door === 'getRequest' + ? svc.getRequest(requestId, asUser(userId)) + : (await svc.listRequests({ recordId }, asUser(userId)))[0]; + const restricted = await read(RESTRICTED); + const control = await read(READER); + expect(restricted.record_title, door).toBe(record.name); + expect(restricted.object_label, door).toBe('Deal'); + expect(restricted.payload, door).toEqual(control.payload); + expect(restricted.payload, door).toMatchObject(record); + expect(restricted.record_title, door).toBe(control.record_title); + } + }); +}); From 9e88df4551ccd4b3a1c6249f2f534840ac0f7cad Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 11:28:30 +0000 Subject: [PATCH 2/3] fix(plugin-approvals): a lookup target object the reader may not read is skipped by the object-level read admission before any caller-scoped read enrichRows asks the security service's canReadObject for each exposed target object, with the reader's envelope minus the operation-private keys, before the caller-scoped row-access read, so an object the reader may not read at all is not read as the reader and the security middleware logs no denial for it. Fails closed on a throw or a non-true answer; with no service, or one without the member, every target is left to the caller-scoped read. Wired by ApprovalsServicePlugin through the registered security service, resolved on every read. Claude-Session: https://claude.ai/code/session_017WLmd5in9RXu7AjyHR8fKQ Co-authored-by: Claude --- .../plugin-approvals/src/approval-service.ts | 97 +++++++- .../plugin-approvals/src/approvals-plugin.ts | 14 ++ ...arget-title-row-access.integration.test.ts | 211 +++++++++++++++--- 3 files changed, 283 insertions(+), 39 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 92dcd37fe00..0e12be2ed24 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -47,6 +47,8 @@ import type { ApprovalStatus, ApprovalCancelReason, ResumeFailureReport, + ISecurityService, + SecurityContext, } from '@objectstack/spec/contracts'; // [#7135] The full `resolveAuthzContext` envelope — what `IApprovalService` // declares for every one of these context parameters since commit aa4b90d9a (the @@ -69,6 +71,7 @@ import { omitInternalFieldsFromWriteResponse, resolveReadableParentIds, resolveUserAuthzGrants, + withoutOperationPrivateKeys, } from '@objectstack/core'; import { filterApproversWhoCanRead, @@ -128,6 +131,22 @@ export interface ApprovalEngine { getSchema?(objectName: string): unknown; } +/** + * [#23637] The single member of the security service's contract a lookup + * target's title consults before the target is read as the reader: the + * OBJECT-level read admission (`ISecurityService.canReadObject`, the engine + * middleware's own object-level read gate, arm for arm). Optional on the + * contract, so its absence is handled by construction. + */ +export type ObjectReadAdmission = Pick; + +/** + * [#23637] Resolves the security service on every read: the security plugin + * may register after the approvals plugin, and a resolver captured once would + * pin "no service" for the life of the process. + */ +export type ObjectReadAdmissionSource = () => ObjectReadAdmission | null | undefined; + export interface ApprovalClock { now(): Date } /** @@ -1444,6 +1463,8 @@ export class ApprovalService implements IApprovalService { private publicBaseUrl: string; private tenancyPosture?: () => string | undefined; private fieldVisibility?: FieldVisibilitySource; + /** [#23637] Lazily-resolved object-level read admission ({@link attachObjectReadAdmission}). */ + private objectReadAdmission?: ObjectReadAdmissionSource; /** [#11993] Lazily-resolved deployment i18n lookup for refusal copy. */ private messageTranslator?: () => ValidationMessageTranslator | undefined; /** @@ -1538,6 +1559,17 @@ export class ApprovalService implements IApprovalService { this.fieldVisibility = source; } + /** + * [#23637] Attach (or replace) the resolver of the security service whose + * object-level read admission (`canReadObject`) is asked before a lookup + * target is read as the reader ({@link ApprovalService.admittedTargetObjects}). + * Resolved on every read, as {@link attachFieldVisibility}'s source is: the + * security plugin may register after this one. + */ + attachObjectReadAdmission(source: ObjectReadAdmissionSource | undefined): void { + this.objectReadAdmission = source; + } + /** * [#10749 / #23594] The subject fields the READING caller may be served, per * subject object of `rows` — {@link ApprovalService.servedFields} for the @@ -6718,6 +6750,54 @@ export class ApprovalService implements IApprovalService { return canServeApiOperation(enable, 'get'); } + /** + * [#23637] The lookup-target objects of `objects` the reader may read at all, + * by the security service's object-level read admission, asked once per + * object before any of them is read as the reader + * ({@link ApprovalService.enrichRows}). + * + * A refused object is not read as the reader: its keys get no display value, + * exactly as its refused caller-scoped read would answer, and it logs no + * denial for a read the reader never asked for. ⛔ It is asked with the + * reader's own envelope (minus the operation-private keys, as + * `resolveReadableParentIds` sends it): no second derivation of authority, + * and no muting of the middleware's log. + * + * It only ever NARROWS what is read: an admitted object is still read as the + * reader, and that read still decides sharing, row-level rules and every gate + * after the object-level arms. Fails CLOSED: an admission that throws (said + * at `warn`, a fault rather than a denial), or answers anything but `true`, + * refuses that object. With no security service composed, or one without the + * optional `canReadObject`, every object is left to the caller-scoped read, + * which still decides, so the absence can never widen the answer. + */ + private async admittedTargetObjects(objects: readonly string[], context: ExecutionContext): Promise { + let admission: ObjectReadAdmission | null | undefined; + try { + admission = this.objectReadAdmission?.(); + } catch { + admission = undefined; + } + const canReadObject = admission?.canReadObject; + if (typeof canReadObject !== 'function') return [...objects]; + const envelope = withoutOperationPrivateKeys((context ?? {}) as Record); + const admitted: string[] = []; + for (const object of objects) { + try { + // `=== true`: an answer that is not a plain yes refuses (fail closed). + if ((await canReadObject.call(admission, object, { ...envelope } as SecurityContext)) === true) { + admitted.push(object); + } + } catch (err: any) { + this.logger?.warn?.( + `[approvals] ApprovalService.enrichRows: the object-level read admission for "${object}" failed, ` + + `so its titles are not resolved and the stored ids stand (fail-closed): ${err?.message ?? err}`, + ); + } + } + return admitted; + } + /** * Field key → display label for an object's schema. Lets the inbox summary * show a human field name ("考核状态") instead of a title-cased machine key @@ -6873,7 +6953,10 @@ export class ApprovalService implements IApprovalService { * the one caller-scoped "which of these records may this caller read" answer * the parent-derived read gates ask, so the target object's object-level * read, sharing and row-level rules decide, as they do for the data door's - * `$expand`. One ask per target object per read. An unreadable target gets + * `$expand`. One ask per target object per read, of the objects the + * security service's object-level read admission admits + * ({@link ApprovalService.admittedTargetObjects}), so an object the reader may + * not read at all is not read as the reader. An unreadable target gets * no display value and its stored id stands, as a deleted one does. The * SUBJECT is not asked: its title and snapshot are served whatever the * reader's access to the subject record (#7497 ruling C, #10749). @@ -6922,15 +7005,17 @@ export class ApprovalService implements IApprovalService { const exposed = Array.from(refIds.keys()).filter((object) => this.servesPayloadDisplayTarget(object)); // [#23637] Only the target RECORDS the reader may read are titled: the // platform's one caller-scoped row-access answer, one ask per target object - // per read. An object the reader may not read, a row its sharing or - // row-level rules hide, and a deleted record all come back absent, so that - // key gets no display value and the snapshot's stored id stands. + // per read, of the objects the reader may read at all. An object the reader + // may not read, a row its sharing or row-level rules hide, and a deleted + // record all come back absent, so that key gets no display value and the + // snapshot's stored id stands. + const admitted = await this.admittedTargetObjects(exposed, context); const readableIds = await resolveReadableParentIds( this.engine, context, - new Map(exposed.map((object) => [object, refIds.get(object)!])), + new Map(admitted.map((object) => [object, refIds.get(object)!])), ); - const targets = exposed.filter((object) => (readableIds.get(object)?.size ?? 0) > 0); + const targets = admitted.filter((object) => (readableIds.get(object)?.size ?? 0) > 0); await this.servedFields(targets, context, served); for (const object of targets) { try { diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 18760fa904a..ffdaebd5269 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -21,6 +21,7 @@ import { ESCALATION_SCAN_INTERVAL_MS, type ApprovalEngine, type ApprovalMessagingSurface, + type ObjectReadAdmission, } from './approval-service.js'; import { bindApprovalLockHook, @@ -292,6 +293,19 @@ export class ApprovalsServicePlugin implements Plugin { : Promise.resolve(undefined); }, }); + // [#23637] The same service's object-level read admission (`canReadObject`), + // asked before a lookup target is read as the reader, so a target object + // the reader may not read at all is skipped without a logged denial + // (`ApprovalService.admittedTargetObjects`). Resolved on every read, as + // above; an absent service, or one without the member, leaves every + // target to the caller-scoped read. + this.service.attachObjectReadAdmission(() => { + try { + return ctx.getService('security'); + } catch { + return undefined; + } + }); // Record lock: block edits to a record while it has a pending request. // Record-delete cancel (#13568): void a record's pending requests when the diff --git a/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts b/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts index 8e63be66e36..ed64b4ffe79 100644 --- a/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/lookup-target-title-row-access.integration.test.ts @@ -45,13 +45,25 @@ * (c) one row-access ask per target object per read, naming every id of that * object the read titles, on the single read and on a multi-row list; * (d) the subject is untouched: a reader who may read no subject row is - * served the subject's title, label and snapshot exactly as the control. + * served the subject's title, label and snapshot exactly as the control; + * (e) with the security service's object-level read admission attached + * (`canReadObject`, standing in from the same table), an object the reader + * may not read at all is not read as the reader — so the security + * middleware has no denial to log — and the answer is unchanged; an + * admitted object is still narrowed by row; the admission is asked once + * per target object per read, without the operation-private keys; + * (e-closed) an admission that throws, or answers anything but `true`, + * refuses that object (the throw said once at `warn`); a service without + * the member leaves every target to the caller-scoped read; + * (f) composed: the real `ApprovalsServicePlugin.start()` hands its service + * the registered `security` service's admission, so the wiring a + * deployment runs is the one (e) measures. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { ObjectQL } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; -import { ApprovalService } from './approval-service.js'; +import { ApprovalService, type ObjectReadAdmission } from './approval-service.js'; import { ApprovalsServicePlugin } from './approvals-plugin.js'; const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; @@ -119,6 +131,10 @@ const READABLE: Record | 'all'>> = { crm_deal: { [RESTRICTED]: new Set(), [READER]: 'all' }, }; +/** The object-level half of the same table: a user absent for an object may not read it. */ +const mayReadObject = (object: string, context: any): boolean => + !!context?.isSystem || READABLE[object]?.[String(context?.userId ?? '')] !== undefined; + /** A caller-scoped read of a business object, as the stand-in saw it. */ interface CallerRead { object: string; userId: string; ids: string[] } @@ -161,51 +177,69 @@ async function packageObjects(): Promise { return manifest.objects; } +const hidden = { account: 'A1', vendor: 'V1', parent: 'D_PARENT' }; + +/** A real engine over sqlite with the three business objects, seeded, behind the stand-in. */ +async function bootEngine(reads: CallerRead[]): Promise { + const engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, account, vendor, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + guardReads(engine, reads); + for (const id of [RESTRICTED, READER, SUBMITTER]) { + await engine.insert('sys_user', { id, name: id, email: `${id}@example.com` }, { context: SYSTEM } as any); + } + await engine.insert('crm_account', { id: 'A1', name: ACCOUNT_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_account', { id: 'A_MINE', name: MINE_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_vendor', { id: 'V1', name: VENDOR_TITLE }, { context: SYSTEM } as any); + await engine.insert('crm_deal', { id: 'D_PARENT', name: PARENT_TITLE, amount: 1 }, { context: SYSTEM } as any); + return engine; +} + +/** One pending request on a new deal `recordId` whose snapshot points at `targets`. */ +async function openDealOn( + engine: ObjectQL, + svc: ApprovalService, + recordId: string, + n: number, + targets: { account: string; vendor: string; parent: string }, +) { + const record = { id: recordId, name: `Deal ${n}`, amount: 100 * n, ...targets }; + await engine.insert('crm_deal', record, { context: SYSTEM } as any); + const row: any = await svc.openNodeRequest({ + object: 'crm_deal', recordId, runId: `run_${recordId}`, nodeId: 'review', flowName: 'deal_review', + config: { + approvers: [RESTRICTED, READER].map((value) => ({ type: 'user', value })), + behavior: 'first_response', + } as any, + submitterId: SUBMITTER, record, + }, asUser(SUBMITTER)); + return { requestId: String(row.id), recordId, record }; +} + describe('[#23637] a lookup target\'s title is served only for a target record its reader may read', () => { let engine: ObjectQL; let svc: ApprovalService; const reads: CallerRead[] = []; + const warns: string[] = []; let n = 0; /** One pending request on a fresh deal whose snapshot points at the given targets. */ const openDeal = async (targets: { account: string; vendor: string; parent: string }) => { n += 1; - const recordId = `D${n}`; - const record = { id: recordId, name: `Deal ${n}`, amount: 100 * n, ...targets }; - await engine.insert('crm_deal', record, { context: SYSTEM } as any); - const row: any = await svc.openNodeRequest({ - object: 'crm_deal', recordId, runId: `run_${n}`, nodeId: 'review', flowName: 'deal_review', - config: { - approvers: [RESTRICTED, READER].map((value) => ({ type: 'user', value })), - behavior: 'first_response', - } as any, - submitterId: SUBMITTER, record, - }, asUser(SUBMITTER)); - return { requestId: String(row.id), recordId, record }; + return openDealOn(engine, svc, `D${n}`, n, targets); }; - const hidden = { account: 'A1', vendor: 'V1', parent: 'D_PARENT' }; beforeAll(async () => { - engine = new ObjectQL(); - engine.registerDriver(new SqlDriver({ - client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, - }), true); - await engine.init(); - for (const def of [sysUser, account, vendor, deal, ...(await packageObjects())]) { - engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); - } - await engine.syncSchemas(); - guardReads(engine, reads); - for (const id of [RESTRICTED, READER, SUBMITTER]) { - await engine.insert('sys_user', { id, name: id, email: `${id}@example.com` }, { context: SYSTEM } as any); - } - await engine.insert('crm_account', { id: 'A1', name: ACCOUNT_TITLE }, { context: SYSTEM } as any); - await engine.insert('crm_account', { id: 'A_MINE', name: MINE_TITLE }, { context: SYSTEM } as any); - await engine.insert('crm_vendor', { id: 'V1', name: VENDOR_TITLE }, { context: SYSTEM } as any); - await engine.insert('crm_deal', { id: 'D_PARENT', name: PARENT_TITLE, amount: 1 }, { context: SYSTEM } as any); + engine = await bootEngine(reads); svc = new ApprovalService({ engine: engine as any, - logger: { warn: () => {}, info: () => {}, error: () => {}, debug: () => {} } as any, + logger: { warn: (msg: string) => { warns.push(String(msg)); }, info: () => {}, error: () => {}, debug: () => {} } as any, }); }); @@ -289,4 +323,115 @@ describe('[#23637] a lookup target\'s title is served only for a target record i expect(restricted.record_title, door).toBe(control.record_title); } }); + + it('(e) an object the reader may not read at all is not read as the reader; the answer is unchanged', async () => { + const asked: Array<{ object: string; userId: string; keys: string[] }> = []; + const admission: ObjectReadAdmission = { + async canReadObject(object: string, context?: any) { + asked.push({ object, userId: String(context?.userId ?? ''), keys: Object.keys(context ?? {}).sort() }); + return mayReadObject(object, context); + }, + }; + svc.attachObjectReadAdmission(() => admission); + try { + const mine = await openDeal({ account: 'A_MINE', vendor: 'V1', parent: 'D_PARENT' }); + const theirs = await openDeal(hidden); + + reads.length = 0; + const one: any = await svc.getRequest(theirs.requestId, { ...asUser(RESTRICTED), __readScope: 'org' }); + expect(reads.map((r) => r.object)).toEqual(['crm_account', 'crm_deal']); + expect(asked).toEqual(['crm_account', 'crm_vendor', 'crm_deal'].map((object) => ({ + object, userId: RESTRICTED, keys: ['permissions', 'positions', 'userId'], + }))); + expect(one.payload_display).toBeUndefined(); + expect(one.payload).toMatchObject(hidden); + + reads.length = 0; + asked.length = 0; + const rows: any[] = await svc.listRequests({ object: 'crm_deal', approverId: RESTRICTED }, asUser(RESTRICTED)); + expect(reads.map((r) => r.object)).toEqual(['crm_account', 'crm_deal']); + expect(asked.map((a) => a.object)).toEqual(['crm_account', 'crm_vendor', 'crm_deal']); + const byRecord = new Map(rows.map((r) => [r.record_id, r])); + // Admitted is not unrestricted: the row scope still decides. + expect(byRecord.get(mine.recordId)?.payload_display).toEqual({ account: MINE_TITLE }); + expect(byRecord.get(theirs.recordId)?.payload_display).toBeUndefined(); + for (const title of [ACCOUNT_TITLE, VENDOR_TITLE, PARENT_TITLE]) expect(JSON.stringify(rows)).not.toContain(title); + + // The control is admitted everywhere and keeps every title. + const control: any = await svc.getRequest(theirs.requestId, asUser(READER)); + expect(control.payload_display).toEqual({ account: ACCOUNT_TITLE, vendor: VENDOR_TITLE, parent: PARENT_TITLE }); + } finally { + svc.attachObjectReadAdmission(undefined); + } + }); + + it('(e-closed) an admission that throws or does not answer true refuses; one without the member reads as before', async () => { + const { requestId } = await openDeal(hidden); + svc.attachObjectReadAdmission(() => ({ + async canReadObject(object: string) { + if (object === 'crm_deal') throw new Error('permission resolution failed'); + return (object === 'crm_vendor' ? 'yes' : true) as unknown as boolean; + }, + })); + try { + warns.length = 0; + reads.length = 0; + const row: any = await svc.getRequest(requestId, asUser(READER)); + expect(row.payload_display).toEqual({ account: ACCOUNT_TITLE }); + expect(reads.map((r) => r.object)).toEqual(['crm_account']); + expect(warns.filter((w) => w.includes('object-level read admission for "crm_deal"'))).toHaveLength(1); + + svc.attachObjectReadAdmission(() => ({}) as ObjectReadAdmission); + reads.length = 0; + const before: any = await svc.getRequest(requestId, asUser(READER)); + expect(before.payload_display).toEqual({ account: ACCOUNT_TITLE, vendor: VENDOR_TITLE, parent: PARENT_TITLE }); + expect(reads.map((r) => r.object)).toEqual(['crm_account', 'crm_vendor', 'crm_deal']); + } finally { + svc.attachObjectReadAdmission(undefined); + } + }); +}); + +describe('[#23637] (f) composed: the plugin hands its service the registered security service\'s admission', () => { + let engine: ObjectQL | undefined; + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('a target object the security service refuses is not read as the reader', async () => { + const reads: CallerRead[] = []; + engine = await bootEngine(reads); + const asked: string[] = []; + const services: Record = { + objectql: engine, + // No field-level answer (pass-through), and the object-level admission. + security: { + getReadableFields: async () => undefined, + getQueryableFields: async () => undefined, + async canReadObject(object: string, context?: any) { + asked.push(object); + return mayReadObject(object, context); + }, + }, + }; + 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); + const svc = services.approvals as ApprovalService; + const { requestId } = await openDealOn(engine, svc, 'D_COMPOSED', 1, hidden); + + reads.length = 0; + const row: any = await svc.getRequest(requestId, asUser(RESTRICTED)); + expect(asked).toEqual(['crm_account', 'crm_vendor', 'crm_deal']); + expect(reads.map((r) => r.object)).toEqual(['crm_account', 'crm_deal']); + expect(row.payload_display).toBeUndefined(); + expect(row.payload).toMatchObject(hidden); + }); }); From fd3b6e8ecf519fcc647aa6466b2f41e2ef4a14bc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 11:34:39 +0000 Subject: [PATCH 3/3] docs(changeset): name the object-level read admission in the 23637 changeset Claude-Session: https://claude.ai/code/session_017WLmd5in9RXu7AjyHR8fKQ Co-authored-by: Claude --- .changeset/23637-lookup-target-title-row-access.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/23637-lookup-target-title-row-access.md b/.changeset/23637-lookup-target-title-row-access.md index b0b5e46d9c0..ab96d6d54a6 100644 --- a/.changeset/23637-lookup-target-title-row-access.md +++ b/.changeset/23637-lookup-target-title-row-access.md @@ -6,7 +6,7 @@ fix(plugin-approvals): the inbox's referenced-record titles (`payload_display`) Clause-②: no -- **Referenced-record titles follow the reader's access to the referenced RECORD.** `listRequests` and `getRequest` now title a lookup in the payload snapshot only when the reading caller may read the record it points at, asked through the platform's one caller-scoped row-access answer (`resolveReadableParentIds`, the same one the comment, activity, attachment and mail gates ask), once per referenced object per read. So the referenced object's object-level read, its sharing and its row-level rules decide, as they already do for the data API's `$expand`. A referenced record the reader may not read gets no display value and the snapshot's stored id stands, as it already does for a deleted referenced record. A reader who may read the record gets the same title as before, still picked from the fields they may be served. +- **Referenced-record titles follow the reader's access to the referenced RECORD.** `listRequests` and `getRequest` now title a lookup in the payload snapshot only when the reading caller may read the record it points at, asked through the platform's one caller-scoped row-access answer (`resolveReadableParentIds`, the same one the comment, activity, attachment and mail gates ask), once per referenced object per read. So the referenced object's object-level read, its sharing and its row-level rules decide, as they already do for the data API's `$expand`. A referenced record the reader may not read gets no display value and the snapshot's stored id stands, as it already does for a deleted referenced record. A reader who may read the record gets the same title as before, still picked from the fields they may be served. A referenced object the reader may not read at all is first skipped by the security service's object-level read admission (`canReadObject`), so an inbox read does not add an access-denied warning to the server log for it. - **Nothing about the record under approval changes.** Its title (`record_title`), its label and its field-level-security-redacted snapshot are served to a routed approver exactly as before, whatever their access to that record. A patch: a narrowing of served values only. Nothing is granted, and no schema, route, error code, translation key or export moves. With no security plugin composed, every caller-scoped read answers every record, so every title is served as before.