Skip to content

Commit c189059

Browse files
committed
fix(plugin-approvals): an expression approver's failed cross-organization read filter refuses the node, named as that read
The ADR-0105 D9 read filter on an expression approver's team and position values ran outside the refusing try, so its failure failed the node with the raw error. It now refuses with ApproverDirectoryUnreadableError naming the filter's read, as the static resolver's filter does; one helper words that read for both. Pinned on both paths with the filter made to reject. The ownership-queue warning for a walk that could not vouch for completion logs the reason only, without the walk's "so the expansion refuses instead", since the queue still answers nobody. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Utp6CXqXpuVf14Mqo1pznh
1 parent 98d62fd commit c189059

4 files changed

Lines changed: 184 additions & 13 deletions

File tree

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

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1180,6 +1180,17 @@ function holderReadRefusal(scope: string, err: unknown): Error {
11801180
*/
11811181
const DIRECTORY_REFUSING_APPROVER_TYPES: ReadonlySet<string> = new Set(['queue', 'team', 'position']);
11821182

1183+
/**
1184+
* [#29604] The read a failed ADR-0105 D9 cross-organization read filter names:
1185+
* that filter's, not the directory's. One wording for the static resolver and
1186+
* the `expression` one, so a refusal from either says the same thing.
1187+
*/
1188+
function crossOrgFilterReadFailure(directoryOrg: unknown, organizationId: unknown, err: unknown): string {
1189+
return `the cross-organization read filter (who among the people of organization '${directoryOrg}' can read `
1190+
+ `this request of organization '${organizationId}') could not be read: `
1191+
+ `${err instanceof Error ? err.message : String(err)}.`;
1192+
}
1193+
11831194
/**
11841195
* [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The
11851196
* approval node refuses to open because a directory read behind one of its
@@ -2692,11 +2703,7 @@ export class ApprovalService implements IApprovalService {
26922703
directoryOrgId: directoryOrg,
26932704
});
26942705
} catch (err) {
2695-
throw Object.assign(new Error(
2696-
`the cross-organization read filter (who among the people of organization '${directoryOrg}' can read `
2697-
+ `this request of organization '${organizationId}') could not be read: `
2698-
+ `${err instanceof Error ? err.message : String(err)}.`,
2699-
), { cause: err });
2706+
throw Object.assign(new Error(crossOrgFilterReadFailure(directoryOrg, organizationId, err)), { cause: err });
27002707
}
27012708
};
27022709

@@ -3046,9 +3053,23 @@ export class ApprovalService implements IApprovalService {
30463053
users = [];
30473054
}
30483055
if (crossOrg && users.length) {
3049-
users = await filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, {
3050-
approverType: resolveAs, value: key, directoryOrgId: directoryOrg,
3051-
});
3056+
try {
3057+
users = await filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, {
3058+
approverType: resolveAs, value: key, directoryOrgId: directoryOrg,
3059+
});
3060+
} catch (err) {
3061+
// [#29604] The read filter is a read behind a `team` or `position`
3062+
// value as much as the expansion is: it refuses the node the same
3063+
// way, naming the filter's read as the static resolver's `bounded`
3064+
// does. Every other case fails as it always did.
3065+
if (onDirectoryFailure === 'refuse' && DIRECTORY_REFUSING_APPROVER_TYPES.has(resolveAs)) {
3066+
throw new ApproverDirectoryUnreadableError({
3067+
type: resolveAs, value: key, organizationId: directoryOrg,
3068+
failedRead: crossOrgFilterReadFailure(directoryOrg, organizationId, err), expression: source, cause: err,
3069+
});
3070+
}
3071+
throw err;
3072+
}
30523073
}
30533074
if (!users.length) {
30543075
slots.push({ id: `${resolveAs}:${key}`, subGroup: key });
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The
5+
* ADR-0105 D9 cross-organization read filter is a read behind a `position` or
6+
* `team` approver as much as the directory expansion is. When it fails, the
7+
* node refuses to open with `ApproverDirectoryUnreadableError`, naming THAT
8+
* read — on the static resolver (its `bounded` filter) and on an `expression`
9+
* approver's `resolveAs` values alike.
10+
*
11+
* `filterApproversWhoCanRead` answers its own failed `sys_member` read by
12+
* keeping the slate, so the only way to make it fail is to make it reject: the
13+
* module is wrapped here with a pass-through spy, and each case says when it
14+
* rejects. Same scenario as `approver-cross-org.integration.test.ts`: a Plant A
15+
* request routed to the group CFO.
16+
*/
17+
18+
import { describe, it, expect, beforeEach, vi } from 'vitest';
19+
import { ApprovalService, ApproverDirectoryUnreadableError } from './approval-service.js';
20+
import { filterApproversWhoCanRead } from './approver-org-scope.js';
21+
22+
vi.mock('./approver-org-scope.js', async (importOriginal) => {
23+
const actual = await importOriginal<typeof import('./approver-org-scope.js')>();
24+
return { ...actual, filterApproversWhoCanRead: vi.fn(actual.filterApproversWhoCanRead) };
25+
});
26+
27+
interface Row { [k: string]: any }
28+
29+
function makeEngine(seed: Record<string, Row[]>) {
30+
const tables: Record<string, Row[]> = { ...seed };
31+
const ensure = (n: string) => (tables[n] ??= []);
32+
const matches = (row: Row, filter: any): boolean => {
33+
if (!filter || typeof filter !== 'object') return true;
34+
for (const [k, v] of Object.entries(filter)) {
35+
if (k === '$or') {
36+
if (!(v as any[]).some((sub) => matches(row, sub))) return false;
37+
continue;
38+
}
39+
if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);
40+
const rv = row[k];
41+
if (v != null && typeof v === 'object' && '$in' in (v as any)) {
42+
if (!(v as any).$in.includes(rv)) return false;
43+
continue;
44+
}
45+
if (v != null && typeof v === 'object' && '$ne' in (v as any)) {
46+
if (rv === (v as any).$ne) return false;
47+
continue;
48+
}
49+
if (rv !== v) return false;
50+
}
51+
return true;
52+
};
53+
return {
54+
_tables: tables,
55+
async find(object: string, options?: any) {
56+
return ensure(object).filter((r) => matches(r, options?.filter ?? options?.where));
57+
},
58+
async insert(object: string, data: Row) { ensure(object).push({ ...data }); return { ...data }; },
59+
async update() { return {}; },
60+
async delete() { return {}; },
61+
async count(object: string) { return ensure(object).length; },
62+
registerHook() { /* no-op */ },
63+
unregisterHooksByPackage() { /* no-op */ },
64+
};
65+
}
66+
67+
const SEED = () => ({
68+
sys_user: [{ id: 'u_cfo' }],
69+
sys_organization: [
70+
{ id: 'o_group', slug: 'acme-group', parent_organization_id: null },
71+
{ id: 'o_plant', slug: 'acme-plant-a', parent_organization_id: 'o_group' },
72+
],
73+
sys_user_position: [{ id: 'up1', user_id: 'u_cfo', position: 'cfo', organization_id: 'o_group' }],
74+
sys_member: [{ id: 'm1', user_id: 'u_cfo', organization_id: 'o_plant', role: 'member' }],
75+
});
76+
77+
const CTX = { userId: 'u_submitter', organizationId: 'o_plant', positions: [], permissions: [] } as any;
78+
79+
const node = (approver: Record<string, unknown>) => ({
80+
object: 'purchase_order',
81+
recordId: 'po1',
82+
runId: 'run_1',
83+
nodeId: 'group_signoff',
84+
flowName: 'po_approval',
85+
config: { approvers: [approver], behavior: 'unanimous' as const, lockRecord: false, onEmptyApprovers: 'admin_rescue' as const },
86+
record: { id: 'po1', signoff_position: 'cfo' },
87+
});
88+
89+
const STATIC_CFO = { type: 'position', value: 'cfo', organization: '$root' };
90+
const EXPRESSION_CFO = { type: 'expression', value: 'trigger.signoff_position', resolveAs: 'position', organization: '$root' };
91+
92+
const FILTER_READ = /^the cross-organization read filter \(who among the people of organization 'o_group' can read this request of organization 'o_plant'\) could not be read: membership read filter down\.$/;
93+
94+
describe('ADR-0142 D6 — a failed cross-organization read filter refuses the node, named as that read (#29604)', () => {
95+
let engine: ReturnType<typeof makeEngine>;
96+
let svc: ApprovalService;
97+
let n = 0;
98+
99+
beforeEach(() => {
100+
vi.mocked(filterApproversWhoCanRead).mockClear();
101+
engine = makeEngine(SEED());
102+
n = 0;
103+
svc = new ApprovalService({
104+
engine: engine as any,
105+
clock: { now: () => new Date(1767000000000 + (n++) * 1000) },
106+
tenancyPosture: () => 'group',
107+
});
108+
});
109+
110+
const refusalOf = (p: Promise<unknown>) => p.then(() => null, (e: unknown) => e);
111+
const filterDown = () => vi.mocked(filterApproversWhoCanRead).mockRejectedValueOnce(new Error('membership read filter down'));
112+
113+
for (const [label, approver, type, value] of [
114+
['a static position approver', STATIC_CFO, 'position', 'cfo'],
115+
['an expression approver resolving as position', EXPRESSION_CFO, 'position', 'cfo'],
116+
] as const) {
117+
it(`control — ${label} reaches the group CFO through the filter`, async () => {
118+
const req = await svc.openNodeRequest(node(approver), CTX) as any;
119+
expect(req.pending_approvers).toEqual(['u_cfo']);
120+
expect(filterApproversWhoCanRead).toHaveBeenCalledTimes(1);
121+
});
122+
123+
it(`${label}: the filter's failure refuses the node and names the filter's read`, async () => {
124+
filterDown();
125+
const refusal = await refusalOf(svc.openNodeRequest(node(approver), CTX));
126+
127+
expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
128+
expect(refusal).toMatchObject({ approverType: type, approverValue: value, retryable: true });
129+
expect((refusal as ApproverDirectoryUnreadableError).failedRead).toMatch(FILTER_READ);
130+
expect(engine._tables['sys_approval_request'] ?? []).toEqual([]);
131+
});
132+
}
133+
});

‎packages/plugins/plugin-sharing/src/ownership-queue-service.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,15 +119,17 @@ function isRecord(v: unknown): v is Record<string, unknown> {
119119

120120
/**
121121
* The reason behind a failed authored read, for a warning that answers nobody.
122-
* A walk's refusal words its own consequence ("so the expansion refuses"),
123-
* which is not this service's answer yet, so the driver's reason it carries as
124-
* `cause` is logged instead; a walk that could not vouch for completion has no
125-
* cause and logs its own sentence.
122+
* A walk's refusal ends on its own consequence ("…, so the expansion refuses
123+
* instead."), which is not this service's answer until #29604 part 2, so only
124+
* the reason is logged: the driver's error the refusal carries as `cause`, or,
125+
* for a walk that could not vouch for completion (no cause), the refusal's
126+
* sentence with that closing clause cut.
126127
*/
127128
function readFailureReason(err: unknown): string {
128129
const cause = (err as { cause?: unknown } | null)?.cause;
129130
if (cause instanceof Error) return cause.message;
130-
return err instanceof Error ? err.message : String(err);
131+
const message = err instanceof Error ? err.message : String(err);
132+
return message.replace(/ [^.]*\brefuses instead\.$/, '');
131133
}
132134

133135
/**

‎packages/plugins/plugin-sharing/src/ownership-queue.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,21 @@ describe('[ADR-0142 D2] OwnershipQueueService — the queue reads page to comple
370370
expect(warn.mock.calls[0][0]).toMatch(/sys_queue_member read failed/);
371371
expect(warn.mock.calls[0][1]).toEqual({ error: 'sys_queue_member page 2 unavailable' });
372372
});
373+
374+
it('a read that cannot vouch for completion logs its reason, not the walk\'s "refuses instead" — the queue answers nobody', async () => {
375+
// A full page whose last row carries no id: the walk cannot resume after it.
376+
const tables = bigQueue(10_000);
377+
delete tables.sys_queue_member.at(-1)!.id;
378+
const warn = vi.fn();
379+
const svc = service(makeEngine({ tables }), { warn });
380+
381+
expect(await svc.expandMembers('big', 'org_a')).toEqual([]);
382+
expect(warn).toHaveBeenCalledTimes(1);
383+
expect(warn.mock.calls[0][1].error).toBe(
384+
"[sharing] the member rows of queue 'big' in organization 'org_a' could not be read: "
385+
+ 'the read by id could not continue past a page (a row without an id, or a page that repeated the cursor).',
386+
);
387+
});
373388
});
374389

375390
describe('[ADR-0131 D4] OwnershipQueueService.hasAnyQueue — the session payload\'s one boolean', () => {

0 commit comments

Comments
 (0)