Skip to content

Commit 5cb6e6e

Browse files
fix(plugin-approvals): the delegation form's Delegator picker offers only the signed-in user (#24834)
Fixes #24764 Clause-②: no ## What changed `sys_approval_delegation.delegator_id` now declares a picker filter: ```ts lookupFilters: [{ field: 'id', operator: 'eq', value: '{current_user_id}' }], ``` The delegation write guard (`bindDelegationWriteGuard` in `packages/plugins/plugin-approvals/src/lifecycle-hooks.ts`) accepts one delegator on a non-system write: the acting user. Its docstring records the decision: "a delegation names its own author as delegator; acting for someone else is done through the approval-level override". The field was a free `sys_user` lookup, so Approvals → Delegations → New offered every user, and the server refused all but one of them with 403. The picker now offers the signed-in user and nobody else. - **Landing.** `packages/plugins/plugin-approvals/src/sys-approval-delegation.object.ts`, where the card expected it. - **Unchanged.** The guard: no refusal is widened and there is no admin exemption. The field stays `required`. What a write may store does not change, because nothing on the write path reads `lookupFilters`. - **Not touched.** No spec edit, no objectui edit, no consumer-side mapping. `approval-service.ts` is not touched. ## Why this vocabulary (measured) - `lookupFilters` is the spec's structured picker filter (`packages/spec/src/data/field.zod.ts`). In the pinned console (objectui `12dc7b47c3`, `.objectui-sha`), `LookupField` lowers each entry through `lookupFiltersToRecord` (`eq` becomes `{ id: value }`). It sends the result as the `$filter` of its candidate query, and the popover, the record-picker dialog, the people picker and the recents rail all use that query. - The engine resolves `{current_user_id}` in a read's `where` against the caller (`ObjectQL.resolveWhereTokens`, the one gate every server read passes). `@objectstack/lint`'s filter-token rule already covers `lookupFilters` (`validate-filter-tokens.test.ts`). - M4 (order on insert) measured: `applyFieldDefaults` runs, then the `beforeInsert` hooks (where the guard stamps an omitted delegator), then record validation (`required`). Nothing here needed `required` changed. ## Route not taken: a `current_user` default (raised as a decision in the report) Option (ii) from the dispatch, `defaultValue: 'current_user'`, is honoured by the pinned console: create forms seed the acting user on a `sys_user` lookup (`plugin-form/src/schemaDefaults.ts`). It would open the form on the one value the guard accepts. I measured it on the real engine at this branch, as an insert that omits `delegator_id`. It changes exactly one cell of the write accept set: | caller | base | with the default | |---|---|---| | member `u_admin` | stored `u_admin` (the guard stamps it) | stored `u_admin` | | anonymous, non-system | refused, `FORBIDDEN` 403 | refused, `FORBIDDEN` 403 | | system, no user id | refused, `VALIDATION_FAILED` (`required`) | refused, `VALIDATION_FAILED` | | system, user id `u_svc` | refused, `VALIDATION_FAILED` (`required`) | **stored `u_svc`** | No HTTP door produces a system context, and no code in this repo writes delegation rows. Even so, it is an accept-set change, and the claim's `Clause-②: no` does not cover one. The picker filter alone makes the form offer only what the guard accepts, so this PR does not add the default and leaves it for a decision. The first commit on this branch carried the default, and the second removed it after the measurement. The net diff has no default. ## Tests New file: `packages/plugins/plugin-approvals/src/sys-approval-delegation-delegator-picker.test.ts`. It runs a real `ObjectQL` over `SqlDriver` (better-sqlite3, in-memory, real DDL) with the guard bound, and asserts: 1. the declaration, read off the object and off the registered body; 2. the picker's candidate query returns the caller and nobody else, for each of three callers (the declared filter, lowered the way the console lowers it); 3. the guard accepts every candidate the picker offers; 4. the guard is not widened: a create naming another user is refused with `FORBIDDEN` / 403, and so is an update relabelling the delegator, and nothing is stored; 5. `required` stays. Ablations, each through `scripts/ablation-replace.mjs`: the anchor hit once, the blob changed on disk, the test ran, and the file was restored to the HEAD blob with `git diff HEAD` empty. The test imports the object and the guard from `src/` by relative path, so no `dist/` is involved. - **A1, drop the filter.** 6 failed, 3 passed. The picker offers `[u_admin, u_priya, u_tomas]` instead of `[caller]`, both declaration pins fail (`expected undefined to deeply equal`), and the guard refuses an offered candidate. - **A2, put a foreign delegator back into the guard** (`if (false && has && supplied !== userId) deny();`). 2 failed, 7 passed: both refusal pins, with "promise resolved instead of rejecting". - **A3, `required: false` on the delegator.** 2 failed, 7 passed: the declaration pin, and "`required` stays" (the promise resolved instead of rejecting). ### Readings at head `529f78b82` (after the last commit; `main` merged in at `307ce4c4b`) - `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2`: 87 files, 1156 tests passed, exit 0. - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0. `check:test-typecheck: OK`, and the debt ledger is unchanged (8 files, 324 errors, 27 signatures). - Local gate subset: the 68 commands `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derives for this diff all exit 0, and `--ran` reconciles 68 derived, 68 run, 0 NOT-MEASURED. The same 68-command list derives at `origin/main` `c3a8e07b6` with these three files applied. - Repo-wide runs (`pnpm lint`, `test:repo` and the full farm) are left to this PR's CI. ## Out of scope (the seat files these) - **The console says nothing when the 403 comes back** (objectui's half of the card). At the pin, `ModalForm`'s submit `catch` hands the error to `schema.onError` and rethrows. It shows no toast and no field error of its own, so the dialog just stays open. objectui#13161 does not cover it: that card is about the action-param dialog, which closes on a server error and shows a toast. ## Acceptance notes - The edit form's Delegator picker narrows to the acting user too. That matches the guard on update, which refuses a delegator other than the caller. - `lookupFilters` is presentation only. The guard remains the enforcement, and this PR claims nothing beyond the picker. --- _Generated by [Claude Code](https://claude.ai/code/session_01UmqzD97e14iws6hmVhgZ5y)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5fcc794 commit 5cb6e6e

3 files changed

Lines changed: 182 additions & 0 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): the out-of-office delegation form's Delegator picker offers only the signed-in user, the one delegator the server accepts
6+
7+
Clause-②: no
8+
9+
No accept set moves. The field gains a picker filter, which nothing on the write path reads, and the delegation write guard and `required` are unchanged.
10+
11+
- **Before.** `sys_approval_delegation.delegator_id` was a free `sys_user` lookup, so Approvals → Delegations → New offered every user. The delegation write guard accepts one delegator on a non-system write, the acting user, so picking anyone else was refused with 403 `FORBIDDEN`.
12+
- **Now.** The field declares `lookupFilters: [{ field: 'id', operator: 'eq', value: '{current_user_id}' }]`. The engine resolves the placeholder against the caller on every read, so the picker's candidate query offers the signed-in user and nobody else.
13+
- **Unchanged.** What a write may store. A create or an update that names another delegator is still refused, and there is no admin exemption. An admin covering for an absent approver reassigns, recalls or rejects that approver's pending approvals through the approval override instead.
Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The Delegator picker of `sys_approval_delegation` offers exactly what the
5+
* delegation write guard admits.
6+
*
7+
* `bindDelegationWriteGuard` (lifecycle-hooks.ts) admits one delegator on a
8+
* non-system write — the acting user — and refuses every other with 403: a
9+
* delegation names its own author as delegator, and acting for an absent
10+
* approver is the approval-level override (the guard's docstring holds the
11+
* ruling). The field was a free `sys_user` lookup, so the console's create
12+
* dialog offered every user and the server refused all but one of them. It
13+
* now declares `lookupFilters` on `id` = `{current_user_id}`: the picker's
14+
* candidate query, whose placeholder the engine resolves against the caller
15+
* on every read.
16+
*
17+
* Through a real `ObjectQL` over `SqlDriver` (better-sqlite3, in-memory, real
18+
* DDL) with the guard bound, this file asserts:
19+
*
20+
* 1. the declaration, read off the object and off the registered body;
21+
* 2. the picker's candidate query returns the caller and nobody else, for
22+
* each caller;
23+
* 3. every candidate it offers is admitted by the guard;
24+
* 4. the guard is not widened: a create or an update naming anyone else is
25+
* still refused, `FORBIDDEN` / 403, and nothing is stored;
26+
* 5. `required` stays: a write with no acting user and no delegator is
27+
* refused.
28+
*/
29+
30+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
31+
import { ObjectQL } from '@objectstack/objectql';
32+
import { SqlDriver } from '@objectstack/driver-sql';
33+
import type { EngineCountOptions, EngineQueryOptions } from '@objectstack/spec/data';
34+
import { SysApprovalDelegation } from './sys-approval-delegation.object.js';
35+
import { bindDelegationWriteGuard } from './lifecycle-hooks.js';
36+
37+
const OBJECT = 'sys_approval_delegation';
38+
const SYSTEM = { context: { isSystem: true } } as any;
39+
const asUser = (userId: string) => ({ context: { userId, positions: [], permissions: [] } }) as any;
40+
41+
/** The install's people: an admin covering for Priya, and a colleague. */
42+
const USERS = [
43+
{ id: 'u_admin', name: 'Dev Admin' },
44+
{ id: 'u_priya', name: 'Priya Raman' },
45+
{ id: 'u_tomas', name: 'Tomas Okafor' },
46+
];
47+
48+
/** Just enough of `sys_user` for the picker's candidate query. */
49+
const sysUser = {
50+
name: 'sys_user',
51+
label: 'User',
52+
fields: {
53+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
54+
name: { name: 'name', label: 'Name', type: 'text' as const },
55+
},
56+
};
57+
58+
type LookupFilter = { field: string; operator: string; value: unknown };
59+
60+
/** The one candidate the guard admits: the acting user, by id. */
61+
const ACTING_USER_ONLY: LookupFilter[] = [{ field: 'id', operator: 'eq', value: '{current_user_id}' }];
62+
63+
const delegatorField = (): Record<string, any> =>
64+
(SysApprovalDelegation.fields as Record<string, any>).delegator_id;
65+
66+
/**
67+
* The picker's candidate query, from the declared `lookupFilters`. The
68+
* console lowers each `eq` entry to `{ [field]: value }` and sends it as the
69+
* `$filter` of its `sys_user` query (objectui `lookupFiltersToRecord`); the
70+
* declaration test holds that every entry here is `eq`.
71+
*/
72+
function pickerQuery(): EngineQueryOptions {
73+
const filters = (delegatorField().lookupFilters ?? []) as LookupFilter[];
74+
const where: Record<string, unknown> = {};
75+
for (const f of filters) where[f.field] = f.value;
76+
return { where };
77+
}
78+
79+
/** The delegations that name Priya as delegator. */
80+
const PRIYA_AS_DELEGATOR: EngineCountOptions = { where: { delegator_id: 'u_priya' } };
81+
82+
const byId = (id: string): EngineQueryOptions => ({ where: { id } });
83+
84+
describe('sys_approval_delegation — the Delegator picker offers only what the write guard admits', () => {
85+
let engine: ObjectQL;
86+
87+
beforeAll(async () => {
88+
engine = new ObjectQL();
89+
const driver = new SqlDriver({
90+
client: 'better-sqlite3',
91+
connection: { filename: ':memory:' },
92+
useNullAsDefault: true,
93+
});
94+
engine.registerDriver(driver, true);
95+
await engine.init();
96+
engine.registry.registerObject(sysUser as any, 'com.objectstack.test.delegator-picker');
97+
engine.registry.registerObject(SysApprovalDelegation as any, 'com.objectstack.test.delegator-picker');
98+
await engine.syncSchemas();
99+
bindDelegationWriteGuard(engine as any);
100+
for (const u of USERS) await engine.insert('sys_user', u, SYSTEM);
101+
});
102+
103+
afterAll(async () => {
104+
try { await engine?.destroy(); } catch { /* noop */ }
105+
});
106+
107+
it('declares the acting user as the picker\'s only candidate, and stays a required sys_user lookup', () => {
108+
const field = delegatorField();
109+
expect(field.type).toBe('lookup');
110+
expect(field.reference).toBe('sys_user');
111+
expect(field.required).toBe(true);
112+
expect(field.lookupFilters).toEqual(ACTING_USER_ONLY);
113+
});
114+
115+
it('the registered body keeps the declaration', () => {
116+
const registered = (engine.registry.getObject(OBJECT) as any).fields.delegator_id;
117+
expect(registered.lookupFilters).toEqual(ACTING_USER_ONLY);
118+
});
119+
120+
it.each(USERS.map((u) => [u.id]))('the picker offers %s themselves and nobody else', async (caller) => {
121+
const candidates = await engine.find('sys_user', pickerQuery(), asUser(caller));
122+
expect(candidates.map((r: any) => r.id)).toEqual([caller]);
123+
});
124+
125+
it('every candidate the picker offers is admitted by the guard', async () => {
126+
const candidates = await engine.find('sys_user', pickerQuery(), asUser('u_admin'));
127+
expect(candidates.length).toBeGreaterThan(0);
128+
for (const candidate of candidates) {
129+
const created = await engine.insert(
130+
OBJECT, { delegator_id: (candidate as any).id, delegate_id: 'u_tomas' }, asUser('u_admin'),
131+
);
132+
expect(created.delegator_id).toBe('u_admin');
133+
}
134+
});
135+
136+
it('the guard is not widened: a create naming anyone else is refused FORBIDDEN / 403 and stores nothing', async () => {
137+
const before = await engine.count(OBJECT, PRIYA_AS_DELEGATOR, SYSTEM);
138+
await expect(
139+
engine.insert(OBJECT, { delegator_id: 'u_priya', delegate_id: 'u_tomas' }, asUser('u_admin')),
140+
).rejects.toMatchObject({ code: 'FORBIDDEN', statusCode: 403 });
141+
expect(await engine.count(OBJECT, PRIYA_AS_DELEGATOR, SYSTEM)).toBe(before);
142+
});
143+
144+
it('the guard is not widened: an update relabelling the delegator is refused FORBIDDEN / 403', async () => {
145+
const own = await engine.insert(OBJECT, { delegator_id: 'u_admin', delegate_id: 'u_tomas' }, asUser('u_admin'));
146+
await expect(
147+
engine.update(OBJECT, { id: own.id, delegator_id: 'u_priya' }, asUser('u_admin')),
148+
).rejects.toMatchObject({ code: 'FORBIDDEN', statusCode: 403 });
149+
const row = await engine.findOne(OBJECT, byId(own.id), SYSTEM);
150+
expect(row?.delegator_id).toBe('u_admin');
151+
});
152+
153+
it('`required` stays: a system write with no acting user and no delegator is refused', async () => {
154+
await expect(engine.insert(OBJECT, { delegate_id: 'u_tomas' }, SYSTEM)).rejects.toMatchObject({
155+
code: 'VALIDATION_FAILED',
156+
fields: expect.arrayContaining([expect.objectContaining({ field: 'delegator_id', code: 'required' })]),
157+
});
158+
});
159+
});

‎packages/plugins/plugin-approvals/src/sys-approval-delegation.object.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,19 @@ export const SysApprovalDelegation = ObjectSchema.create({
8686
group: 'Delegation',
8787
}),
8888

89+
// A delegation names its own author as delegator: on every non-system
90+
// write `bindDelegationWriteGuard` (lifecycle-hooks.ts) admits exactly one
91+
// value here, the acting user, and refuses any other with 403 (its
92+
// docstring holds why there is no admin exemption). So the picker offers
93+
// that one value, instead of every user with all but one refused:
94+
// `lookupFilters` is the picker's candidate query, and the engine resolves
95+
// `{current_user_id}` against the caller on every read. Presentation only
96+
// — the guard stays the enforcement, and what a write may store is
97+
// unchanged. Pinned in `sys-approval-delegation-delegator-picker.test.ts`.
8998
delegator_id: Field.lookup('sys_user', {
9099
label: 'Delegator',
91100
required: true,
101+
lookupFilters: [{ field: 'id', operator: 'eq', value: '{current_user_id}' }],
92102
group: 'Delegation',
93103
description: 'The user going out of office; their individually-routed approver slots are rerouted while active.',
94104
}),

0 commit comments

Comments
 (0)