Skip to content

Commit 3f6587c

Browse files
committed
fix(plugin-approvals): the read gate reaches the approvals door's visibility in-package, the published ApprovalService type unchanged
The visibility source moves off the class into a module-scoped WeakMap the constructor fills, reached through requestVisibilitySourceOf, which the package entry does not re-export; the by-id row read becomes a module function. ApprovalService's member list equals origin/main's. Claude-Session: https://claude.ai/code/session_013j5gkUCpqQiti4GgPqqmnt Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2a7dfed commit 3f6587c

5 files changed

Lines changed: 103 additions & 70 deletions

File tree

‎.changeset/22559-approval-request-read-gate.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ A read of `sys_approval_request` through the generic data door (`find`, `findOne
1414

1515
Before this, a deployment that granted read on the object served every request of the organization on that door, including the payload snapshots of records the caller cannot open.
1616

17-
- **One rule for both doors.** The gate asks `ApprovalService.visibleRequestIdsFor`, which is the approvals door's own visibility. It is not the activity stream's parent-record gate, which admits every reader of the record: the record-reader tier stays off unless the deployment names the object.
17+
- **One rule for both doors.** The gate asks the approvals service for its own visibility (the same rule `listRequests` and `getRequest` apply), through an in-package handle: the published `ApprovalService` type is unchanged. It is not the activity stream's parent-record gate, which admits every reader of the record: the record-reader tier stays off unless the deployment names the object.
1818
- **Absence, not refusal.** A request the caller may not see answers a by-id read exactly as a missing one does, `404 RECORD_NOT_FOUND`. A list's `total` and a grouped count are narrowed like its rows. A failed visibility answer denies the read and logs a warning.
1919
- **Unchanged.** The approvals door, an administrator's view (Setup → Approvals → Requests), and the approval engine's own reads. A deployment that grants no read on the object still answers `403`.
2020
- The record-reader tier no longer anchors on `sys_approval_request` itself, even when `recordReaderVisibleObjects` names it.

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 81 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ import {
8585
resolveReadableSnapshotFields,
8686
type FieldVisibilitySource,
8787
} from './payload-redaction.js';
88+
import type { RequestVisibilitySource } from './request-read-gate.js';
8889

8990
/**
9091
* Node-era approval runtime (ADR-0019).
@@ -1250,6 +1251,52 @@ export interface ApprovalServiceOptions {
12501251
messageTranslator?: () => ValidationMessageTranslator | undefined;
12511252
}
12521253

1254+
/**
1255+
* The stored row request `requestId` names inside `tenantOrg` (every
1256+
* organization when `null`), read as the system — the one read a by-id serve
1257+
* (`ApprovalService.loadRequest`) and a by-id visibility question (the read
1258+
* gate's source below) both take, so the two cannot disagree about which record
1259+
* an id is anchored on.
1260+
*/
1261+
async function findRequestRow(
1262+
engine: ApprovalEngine,
1263+
requestId: string,
1264+
tenantOrg: string | null,
1265+
): Promise<any | null> {
1266+
const where: any = { id: requestId };
1267+
if (tenantOrg) where.organization_id = tenantOrg;
1268+
const rows = await engine.find('sys_approval_request', {
1269+
where, limit: 1, context: SYSTEM_CTX,
1270+
});
1271+
return Array.isArray(rows) && rows[0] ? rows[0] : null;
1272+
}
1273+
1274+
/**
1275+
* [#22559] Each service's own request visibility, as the generic data door's
1276+
* read gate on `sys_approval_request` asks it (`request-read-gate.ts`).
1277+
*
1278+
* In-package only, and deliberately not a member of {@link ApprovalService}:
1279+
* the published class type stays exactly what it was, and the one consumer —
1280+
* `ApprovalsServicePlugin`, binding the gate — reaches it through
1281+
* {@link requestVisibilitySourceOf}, which the package entry does not
1282+
* re-export. The answer is written by the class itself, in its constructor,
1283+
* so it calls the class's private rule rather than restating it.
1284+
*/
1285+
const REQUEST_VISIBILITY = new WeakMap<ApprovalService, RequestVisibilitySource>();
1286+
1287+
/**
1288+
* [#22559] The visibility source the read gate consumes for `service`: the
1289+
* approvals door's own `visibleRequestIds`, asked for a read that arrives
1290+
* through another door. Every constructed service registers one.
1291+
*/
1292+
export function requestVisibilitySourceOf(service: ApprovalService): RequestVisibilitySource {
1293+
const source = REQUEST_VISIBILITY.get(service);
1294+
if (!source) {
1295+
throw new Error('[approvals] no request visibility source is registered for this ApprovalService instance');
1296+
}
1297+
return source;
1298+
}
1299+
12531300
export class ApprovalService implements IApprovalService {
12541301
private readonly engine: ApprovalEngine;
12551302
private readonly clock: ApprovalClock;
@@ -1292,6 +1339,39 @@ export class ApprovalService implements IApprovalService {
12921339
.map((n) => String(n ?? '').trim())
12931340
.filter(Boolean),
12941341
);
1342+
// [#22559] This door's request visibility, asked by a read that arrives
1343+
// through ANOTHER door: the generic data door's read gate on
1344+
// `sys_approval_request`. It is `visibleRequestIds` itself — the
1345+
// participant set, the override actor's unrestricted view and the
1346+
// default-OFF record-reader tier (#8652) — so the two doors serve one rule
1347+
// and cannot drift apart. ⛔ No second visibility rule lives in the gate.
1348+
//
1349+
// `null` is "every request in scope": the override actor, a system context
1350+
// included (its own reads are this service's, so the gate never narrows
1351+
// them, and answering costs no read). Otherwise the ids this door would
1352+
// serve the caller for a read naming `target`; empty means none. `target`
1353+
// names the record the tier anchors on, exactly as on this door: `object` +
1354+
// `recordId` is a list filtered to one record (`listRequests`); `requestId`
1355+
// is a request loaded by id, which carries its own anchor (`loadRequest`).
1356+
// Absent, the read is untargeted — the participant set alone, as for the
1357+
// inbox.
1358+
REQUEST_VISIBILITY.set(this, {
1359+
visibleRequestIdsFor: async (context, target) => {
1360+
// `organizationId` is not on the envelope — see isOverrideActor().
1361+
const tenantOrg = (context as any)?.organizationId ?? context?.tenantId ?? null;
1362+
// The same first question `visibleRequestIds` asks, asked before the
1363+
// by-id anchor is fetched so the unrestricted answer costs no read.
1364+
if (this.isOverrideActor(context, tenantOrg)) return null;
1365+
let anchor: { object?: string | null; recordId?: string | null } = {
1366+
object: target?.object, recordId: target?.recordId,
1367+
};
1368+
if (!(anchor.object && anchor.recordId) && target?.requestId) {
1369+
const raw = await findRequestRow(this.engine, target.requestId, tenantOrg);
1370+
anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {};
1371+
}
1372+
return this.visibleRequestIds(context, tenantOrg, anchor);
1373+
},
1374+
});
12951375
}
12961376

12971377
/** Attach (or replace) the ADR-0105 D9 posture provider. */
@@ -6528,63 +6608,6 @@ export class ApprovalService implements IApprovalService {
65286608
return [...new Set<string>(list.map(r => String(r.request_id)))];
65296609
}
65306610

6531-
/**
6532-
* [#22559] This door's request visibility, asked by a read that arrives
6533-
* through ANOTHER door: the generic data door's read gate on
6534-
* `sys_approval_request` (`request-read-gate.ts`). It is
6535-
* {@link ApprovalService.visibleRequestIds} itself — the participant set,
6536-
* the override actor's unrestricted view and the default-OFF record-reader
6537-
* tier (#8652) — so the two doors serve one rule and cannot drift apart.
6538-
* ⛔ No second visibility rule lives in the gate.
6539-
*
6540-
* `null` means "every request in scope": the override actor, a system
6541-
* context included (its own reads are this service's, so the gate never
6542-
* narrows them, and answering costs no read). Otherwise the set of request
6543-
* ids this door would serve the caller for a read naming `target`; empty
6544-
* means none.
6545-
*
6546-
* `target` names the record the record-reader tier anchors on, exactly as on
6547-
* this door: `object` + `recordId` is a list filtered to one record
6548-
* ({@link ApprovalService.listRequests}); `requestId` is a request loaded by
6549-
* id, which carries its own anchor ({@link ApprovalService.loadRequest}).
6550-
* Absent, the read is untargeted — the participant set alone, as for the
6551-
* inbox.
6552-
*/
6553-
async visibleRequestIdsFor(
6554-
context: ExecutionContext,
6555-
target?: { object?: string | null; recordId?: string | null; requestId?: string | null },
6556-
): Promise<Set<string> | null> {
6557-
// `organizationId` is not on the envelope — see isOverrideActor().
6558-
const tenantOrg = (context as any)?.organizationId ?? context?.tenantId ?? null;
6559-
// The same first question `visibleRequestIds` asks, asked before the
6560-
// by-id anchor is fetched so the unrestricted answer costs no read.
6561-
if (this.isOverrideActor(context, tenantOrg)) return null;
6562-
let anchor: { object?: string | null; recordId?: string | null } = {
6563-
object: target?.object, recordId: target?.recordId,
6564-
};
6565-
if (!(anchor.object && anchor.recordId) && target?.requestId) {
6566-
const raw = await this.findRequestRow(target.requestId, tenantOrg);
6567-
anchor = raw ? { object: raw.object_name, recordId: raw.record_id } : {};
6568-
}
6569-
return this.visibleRequestIds(context, tenantOrg, anchor);
6570-
}
6571-
6572-
/**
6573-
* The stored row request `requestId` names inside `tenantOrg` (every
6574-
* organization when `null`), read as the system — the one read a by-id serve
6575-
* ({@link ApprovalService.loadRequest}) and a by-id visibility question
6576-
* ({@link ApprovalService.visibleRequestIdsFor}) both take, so the two cannot
6577-
* disagree about which record an id is anchored on.
6578-
*/
6579-
private async findRequestRow(requestId: string, tenantOrg: string | null): Promise<any | null> {
6580-
const where: any = { id: requestId };
6581-
if (tenantOrg) where.organization_id = tenantOrg;
6582-
const rows = await this.engine.find('sys_approval_request', {
6583-
where, limit: 1, context: SYSTEM_CTX,
6584-
});
6585-
return Array.isArray(rows) && rows[0] ? rows[0] : null;
6586-
}
6587-
65886611
/**
65896612
* The request ids this caller is a PARTICIPANT of, or `null` for a caller
65906613
* who may see everything in scope (#3590).
@@ -6969,7 +6992,7 @@ export class ApprovalService implements IApprovalService {
69696992
if (!requestId) return null;
69706993
// `organizationId` is not on the envelope — see isOverrideActor().
69716994
const tenantOrg = (context as any)?.organizationId ?? context?.tenantId;
6972-
const raw = await this.findRequestRow(requestId, tenantOrg ?? null);
6995+
const raw = await findRequestRow(this.engine, requestId, tenantOrg ?? null);
69736996
if (!raw) return null;
69746997
const caller = await this.actingCaller(context);
69756998
// #3590: tenant scoping alone let any authenticated user read any request

‎packages/plugins/plugin-approvals/src/approvals-plugin.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { SysApprovalDelegation } from './sys-approval-delegation.object.js';
1515
import { renderConfirmPage, renderResultPage } from './action-link-pages.js';
1616
import {
1717
ApprovalService,
18+
requestVisibilitySourceOf,
1819
ESCALATION_JOB_NAME,
1920
ESCALATION_SCAN_INTERVAL_MS,
2021
type ApprovalEngine,
@@ -310,7 +311,7 @@ export class ApprovalsServicePlugin implements Plugin {
310311
// the manual API), and a read through the generic door needs the gate
311312
// whichever API wrote the row.
312313
if (typeof (engine as any).registerMiddleware === 'function') {
313-
bindRequestReadGate(engine as any, this.service, ctx.logger);
314+
bindRequestReadGate(engine as any, requestVisibilitySourceOf(this.service), ctx.logger);
314315
} else {
315316
ctx.logger.warn(
316317
'ApprovalsServicePlugin: the ObjectQL engine has no middleware seam — reads of sys_approval_request '

‎packages/plugins/plugin-approvals/src/request-read-gate.integration.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ import { ObjectQL } from '@objectstack/objectql';
5151
import { SqlDriver } from '@objectstack/driver-sql';
5252
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
5353
import type { EngineAggregateOptions, EngineCountOptions, EngineQueryOptions } from '@objectstack/spec/data';
54-
import { ApprovalService } from './approval-service.js';
54+
import { ApprovalService, requestVisibilitySourceOf } from './approval-service.js';
5555
import { ApprovalsServicePlugin, type ApprovalsPluginOptions } from './approvals-plugin.js';
5656
import { SysApprovalRequest } from './sys-approval-request.object.js';
5757
import { SysApprovalAction } from './sys-approval-action.object.js';
@@ -322,7 +322,8 @@ describe('the record-reader tier ON for the object', () => {
322322
describe('fails closed', () => {
323323
it('a visibility answer that fails denies the read, and says so', async () => {
324324
const rig = await boot();
325-
vi.spyOn(rig.svc, 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away'));
325+
// The very source the plugin handed the gate: the service registers one object per instance.
326+
vi.spyOn(requestVisibilitySourceOf(rig.svc), 'visibleRequestIdsFor').mockRejectedValueOnce(new Error('probe went away'));
326327
expect((await listIds(rig, ADMIN)).ids).toEqual([]);
327328
expect(rig.warnings.some((w) => w.includes('request read gate') && w.includes('fail closed'))).toBe(true);
328329
});

‎packages/plugins/plugin-approvals/src/request-read-gate.ts‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,14 @@
1818
*
1919
* ## One rule, two doors
2020
*
21-
* ⛔ Nothing here decides who may see a request. The answer is
22-
* `ApprovalService.visibleRequestIdsFor`, which IS the approvals door's
23-
* `visibleRequestIds`: the participant set (submitter, current approver, past
24-
* actor), the override actor's unrestricted view, and the record-reader tier,
25-
* which stays default OFF. This module only reads which record a query names,
26-
* hands that to the service, and ANDs the answer into the read.
21+
* ⛔ Nothing here decides who may see a request. The answer is the approvals
22+
* service's own: {@link RequestVisibilitySource}, which the plugin reaches
23+
* with `requestVisibilitySourceOf(service)` (`approval-service.ts`, in-package
24+
* only) and which IS the approvals door's private `visibleRequestIds`: the
25+
* participant set (submitter, current approver, past actor), the override
26+
* actor's unrestricted view, and the record-reader tier, which stays default
27+
* OFF. This module only reads which record a query names, hands that to the
28+
* service, and ANDs the answer into the read.
2729
*
2830
* ⛔ This is deliberately NOT `plugin-audit`'s parent-record read gate (the
2931
* activity stream's shape), which keeps a row whenever its parent record is
@@ -76,6 +78,7 @@
7678
* by the tier is served the row read-only on both doors.
7779
*/
7880

81+
import type { ExecutionContext } from '@objectstack/spec/kernel';
7982
import { pinnedEquality } from './payload-predicate-guard.js';
8083
import { APPROVAL_REQUEST_OBJECT, type MiddlewareEngine } from './payload-redaction-middleware.js';
8184

@@ -94,10 +97,15 @@ export interface RequestReadTarget {
9497
requestId?: string;
9598
}
9699

97-
/** The one visibility definition the gate reads: the approvals service's. */
100+
/**
101+
* The one visibility definition the gate reads: the approvals service's,
102+
* written by `ApprovalService`'s constructor and reached through
103+
* `requestVisibilitySourceOf`. `null` is "every request in scope"; a set is the
104+
* ids the approvals door would serve the caller for a read naming `target`.
105+
*/
98106
export interface RequestVisibilitySource {
99107
visibleRequestIdsFor(
100-
context: any,
108+
context: ExecutionContext,
101109
target?: { object?: string | null; recordId?: string | null; requestId?: string | null },
102110
): Promise<Set<string> | null>;
103111
}

0 commit comments

Comments
 (0)