Skip to content

Commit ffb31fc

Browse files
fix(plugin-security)!: granted_by is provenance — the delegated-admin gate stamps the writer on every non-system insert, and the column is readonly (#22243)
Fixes #22201 Clause-②: no (narrowing) `granted_by` on `sys_user_permission_set` and `sys_user_position` records who wrote a grant. This PR enforces that, following the seat's ruling A in the claim: - Both columns are now `readonly: true`. - `DelegatedAdminGate.assert` first runs the authority decision (`authorize`, the old body). It then stamps the caller's `userId` into `granted_by` on every non-system insert it admits into either table, whatever the payload carried. Tenant-level admins, scoped delegates and self-delegations are stamped alike. A refused write throws before the stamp. - This one stamp (`stampWriter`) replaces the three old conditional stamps. Those ran on the delegate and self-delegation paths only, and only when the value was null. - System-context writers are unchanged. `isSystem` short-circuits the security middleware before the gate. The change does not widen who may grant or what a grant gives. It touches nothing in `packages/objectql` or `packages/spec`, and it writes no data. ## Measured: stored `granted_by` per writer case All cases ran on a real `ObjectQL` engine over `SqlDriver` (better-sqlite3), with the real `SecurityPlugin` middleware, and so the real gate, in front. OTHER is another existing `sys_user` that the caller names as the granter. - **Before** = `origin/main` 3b49318 source, read with the rig at 295745e. - **Readonly only** = option B: `readonly: true`, gate unchanged. - **After** = this branch. | writer case | table | before | readonly only | after | |---|---|---|---|---| | tenant-level admin, names OTHER | both | OTHER | OTHER | writer | | tenant-level admin, names none | both | null | null | writer | | scoped delegate, names OTHER | both | OTHER | OTHER | writer | | scoped delegate, names none | both | writer | writer | writer | | system-context insert, names OTHER | both | OTHER | OTHER | OTHER | | invitation placement `apply` (system) | `sys_user_position` | issuer | issuer | issuer | | organization-admin reconcile, triggered by a system `sys_member` write attributed to a human | `sys_user_permission_set` | that human | that human | that human | | non-system UPDATE renaming the granter (admin or delegate) | both | lands: OTHER | stripped: unchanged | stripped: unchanged | **Mechanism finding (premise correction).** Readonly alone does not change what an insert stores. On insert, the engine's create-side static readonly strip skips every `sys_` object (`staticReadonlyInsertSubject`). The card's premise was that "an explicit caller value … is stripped by `stripReadonlyFields`, so the row lands NULL". That holds on update only. On insert, the caller's value lands as sent. The gate's in-place middleware stamp is therefore what lands, and #14088's hook-write record is not involved. The update strip does judge `sys_` objects, and that is what closes the update door. ## Measured: audit bucket per case (`ObjectQL.inspectDanglingReferences`) | row | readonly removed (ablation B1/B2) | after | |---|---|---| | `granted_by` names an id with no `sys_user` row | `dangling` | `provenance`; no `[integrity]` warning logged | | control: `user_id` names an id with no `sys_user` row | `dangling` | `dangling`; exactly one `[integrity]` warning | Both tables are covered. ## Family, measured one by one - **`sys_user_position.granted_by`: same writer class, same change.** Its non-system writes go through the gate, for both tenant-level admins and delegates (including self-delegation). Its system writer, invitation placement, sets the issuer. - **`sys_record_share.granted_by` (plugin-sharing): a different class, left unchanged.** - The object is `managedBy: 'engine-owned'`, so the ADR-0103 engine-owned guard refuses a user-context generic write. - Its one writer is the sharing service's grant. That writer inserts under a system context and stamps `granted_by` from the acting context's `userId`, which is null for a rule reconcile. - `GrantShareInput` carries no granter, so no caller can name one. "granted_by = writer" already holds there. ## Tests - **`granted-by-writer-provenance.test.ts` (new, 20 cases).** Each runs on the real engine with the real `SecurityPlugin`. - Non-system inserts store the writer: 2 tables × 2 callers × named or absent. - System writers keep their value: the plain system insert, invitation placement and the organization-admin reconcile. - A non-system update cannot rewrite the granter: 2 tables × 2 callers. - The audit files a deleted granter under `provenance` with no warning, beside the business-lookup control that lands in `dangling` and warns: per table. - **`delegated-admin-gate.test.ts` (+16 cases).** These pin the gate's own half: - named or absent granter, per principal and table; - batch inserts; - a refused insert is not stamped; - an update is not stamped; - a self-delegation that names another granter; - `sys_member` and `sys_position_permission_set` never gain the key. - **`write-preview-field-gate-parity.test.ts` (fixture).** The suite's `sys_user_position` stand-in now declares `granted_by`, as the shipped object does. The gate now stamps a tenant-level admin's insert, and the stand-in's missing column was refused as an unknown field before the rule that suite pins was reached. - **The platform-admin promotion.** `bootstrap-platform-admin.ts` is read only here (PR #22214 is in flight), so it is not called. Its path, a plain `{ isSystem: true }` insert, is the system-insert pin. ## Ablations Each mutation went to disk through `scripts/ablation-replace.mjs`, with the anchor hitting exactly once and the blob changing. Each restore was proven by the blob hash equalling the HEAD blob and `git diff HEAD` being empty. The subject resolves through relative source imports, with `@objectstack/objectql` aliased to source, so no `dist/` was in the path. | ablation | mutation | red / green | restore | |---|---|---|---| | A | `stampWriter` keeps a supplied value (`granted_by == null` guard) | 11 red (every "names another user" pin, real-engine and unit) / 92 green | blob 8fb5c41 == HEAD | | B1 | `sys_user_permission_set.granted_by` `readonly: false` | 3 red (2 update pins, provenance pin: the row lands in `dangling`) / 17 green | blob 2bed0a5 == HEAD | | B2 | `sys_user_position.granted_by` `readonly: false` | 3 red (same three, for that table) / 17 green | blob 9df5977 == HEAD | | C | `stampWriter` call removed | 22 red (11 named, 8 absent, 3 legacy stamp pins) / 81 green | blob 8fb5c41 == HEAD | The first B1 attempt was a no-op. The replacement was a substring of the anchor, so the tool refused (`replace x1 -> x1`), and the command never ran. It was re-run with `readonly: false`, and the table above shows that run. ## Validation All readings below were taken at `b4ed57b0ad`, after merging `origin/main` (0e9371f). - **plugin-security tests.** `vitest run --maxWorkers=2` (whole package): 177 files, 3766 passed, 45 skipped, exit 0. - **plugin-security typecheck.** `pnpm --filter @objectstack/plugin-security typecheck`: exit 0. That covers the build program, the scripts program and `check:test-typecheck`, which reports 0 errors. `tsc --listFiles` over `tsconfig.test.json` lists all three touched test files. - **Build.** `turbo run build --filter=!@objectstack/docs`: 72 of 72 tasks succeeded. - **Gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, with no paths, derives 65 commands. All 65 ran, plus `pnpm --filter @objectstack/spec check:generated`, and every one exited 0. - `--ran` reconciliation, with an exit code recorded per command: 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. - `check-adr-0087-registration`: green, disposition `not-required (no-migration-prescription)`. - `check-changeset-no-major`: green, "This diff introduces no `major` bump". The level axis needs a PR payload, so it is left to CI. - `check:engine-double-contract`: green. - `check:i18n`: green, 9 packages in sync. - `check:dual-build-cjs-loads`: green, 106 entry points across 66 packages. - **ESLint, narrowed to the changed files.** - The 6 changed `.ts` files ran under the repo's own `eslint.config.mjs` with `--no-inline-config`. The JSON report lists 6 files, with 0 errors and 0 warnings, so none was ignored. - That config enables no type-aware linting (no `parserOptions.project`, no typed rules; see its own comment), so this diff cannot change the verdict on any file it does not touch. - The repo-wide `pnpm lint` is left to CI. ## Acceptance notes - **The supplied value is replaced, not refused.** A non-system insert that names a granter gets the writer, which shows in the returned row. On insert there is no `droppedFields` entry or strip warning for it, because the create-side strip does not run on `sys_` objects. - **One description is now incomplete.** `sys_user_position.granted_by` still says "(stamped by the delegated-admin gate for delegate writes)". That is still true, but it no longer covers every case. Rewording it would re-key the translated bundles in four locales, so it is left alone. The docs pages under `content/docs/permissions/` describe the delegate stamp, which also still holds. - **No stored row is rewritten.** The production row's sentinel is out of scope (triage ruling); clearing it is cloud's maintainer's write. - **Dogfood is declared to CI and was not run locally.** The dogfood suites that read `granted_by` expect the writer or a truthy value (`delegation-of-duty`, `showcase-permission-zoo`, `membership-actor-attribution`), which this change keeps. --- _Generated by [Claude Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 31d24a5 commit ffb31fc

7 files changed

Lines changed: 481 additions & 17 deletions
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
'@objectstack/plugin-security': minor
3+
---
4+
5+
fix(plugin-security)!: `granted_by` on the grant tables is provenance: the platform stamps the writer, and a non-system caller can no longer name the granter
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) An accept-set narrowing at the delegated-admin write gate over an existing column: `sys_user_permission_set.granted_by` and `sys_user_position.granted_by` gain `readonly: true` and keep their name, type, target and storage, so no authorable key, spelling, export or stored shape moves and there is nothing for `objectstack migrate meta` to rewrite. A caller that sent a granter has nothing to change in what it sends; the value it named is replaced by its own user id. Stored rows keep their bytes. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers this column and this diff adds none (not registered or already-registered); and the change narrows what a runtime write door stores, not a runtime TypeScript interface or a type surface alone (not runtime-interface-only or type-surface-only). -->
10+
11+
**BREAKING** accept-set narrowing, shipped as `minor` under the repo's launch-window convention for breaking changes.
12+
13+
`granted_by` records who wrote a grant. The platform already treated it that way: the delegated-admin gate stamped the writer, and the organization-admin reconcile writes the attributed human or null (ADR-0118 D1, an id or null and never a sentinel). But the column was a plain lookup, so a non-system caller could store any existing user as the granter, and the integrity audit filed a granter that no longer resolves as a broken business reference, which raised its `[integrity] stored references that resolve to nothing` warning on every run.
14+
15+
**What moves for consumers.**
16+
17+
- **Non-system inserts store the writer.** The delegated-admin gate now stamps the caller's user id into `granted_by` on every `sys_user_permission_set` and `sys_user_position` insert it admits, whatever the payload carried. A tenant-level admin's insert and a delegate's insert are stamped alike. Before, a supplied value was stored as sent, and a tenant-level admin's insert that supplied none stored `null`.
18+
- **Non-system updates cannot change it.** Both columns are now `readonly: true`, so the engine's update strip drops a caller's `granted_by` and the row keeps its granter. Before, the update landed. A caller that passes `strictReadonlyWrites` is refused instead, as for every read-only column.
19+
- **The integrity audit files it as provenance.** A `granted_by` that names a user who no longer exists is reported under the audit's `provenance` bucket, which does not raise the warning. A business reference that resolves to nothing is reported under `dangling` and warns, as before.
20+
21+
**Unchanged.**
22+
23+
- System-context writers keep the value they write: invitation placement stamps the issuer, the organization-admin reconcile stamps the attributed human or `null`, and the platform-admin promotion writes `null`. A server-side writer that must record someone other than its caller writes under a system context, as these do.
24+
- Who may write a grant, and what a grant gives, are unchanged. The gate's authority decision runs first; a refused write is not stamped.
25+
- Stored rows are not rewritten, and no stored row is refused on read. A row whose `granted_by` already names someone else, or holds a value that names no user, keeps it.
26+
- `sys_record_share.granted_by` (plugin-sharing) is unchanged: its only writer is the sharing service, which already stamps the acting user under a system context.

‎packages/plugins/plugin-security/src/delegated-admin-gate.test.ts‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ const insertAssignment = (ctx: any, row: any) => h.gate.assert({
112112
});
113113

114114
describe('DelegatedAdminGate — tenant admins and outsiders', () => {
115-
it('tenant-level admin (superuser wildcard) passes untouched', async () => {
115+
it('tenant-level admin (superuser wildcard) passes — the ordinary CRUD/RLS checks decide', async () => {
116116
await expect(insertAssignment(h.ctxOf('tenant_admin'), {
117117
user_id: 'u_west_1', position: 'mixed_pos', business_unit_id: 'bu_west',
118118
})).resolves.toBeUndefined();
@@ -979,3 +979,72 @@ describe('DelegatedAdminGate — a position is picked by organization, not by ar
979979
})).rejects.toThrow(/position 'closer' distributes permission set 'finance_user', which is not in the scope's allowlist/);
980980
});
981981
});
982+
983+
// ── granted_by is the writer on every admitted insert ─────────────────────
984+
//
985+
// `granted_by` is provenance (`readonly` on both grant tables): the gate stamps
986+
// the writer on every non-system insert it admits, whatever the payload
987+
// carried — a caller-supplied granter is overwritten, not kept, and a
988+
// tenant-level admin's grant is stamped too. What is stored on the real engine
989+
// is pinned in `granted-by-writer-provenance.test.ts`; this block pins the
990+
// gate's own half, path by path.
991+
describe('DelegatedAdminGate — granted_by is the writer on every admitted insert', () => {
992+
const grantRow = (extra: Record<string, unknown> = {}): any => ({ user_id: 'u_east_1', permission_set_id: 'ps_sales', ...extra });
993+
const assignmentRow = (extra: Record<string, unknown> = {}): any => ({ user_id: 'u_east_1', position: 'sales_rep', business_unit_id: 'bu_es', ...extra });
994+
995+
for (const [object, row] of [
996+
['sys_user_position', assignmentRow],
997+
['sys_user_permission_set', grantRow],
998+
] as const) {
999+
for (const principal of ['tenant_admin', 'delegate'] as const) {
1000+
it(`${object}: ${principal} naming another user as granter is stamped with the writer`, async () => {
1001+
const data = row({ granted_by: 'u_west_1' });
1002+
await expect(h.gate.assert({ object, operation: 'insert', data, context: h.ctxOf(principal) })).resolves.toBeUndefined();
1003+
expect(data.granted_by).toBe(`u_${principal}`);
1004+
});
1005+
1006+
it(`${object}: ${principal} naming no granter is stamped with the writer`, async () => {
1007+
const data = row();
1008+
await expect(h.gate.assert({ object, operation: 'insert', data, context: h.ctxOf(principal) })).resolves.toBeUndefined();
1009+
expect(data.granted_by).toBe(`u_${principal}`);
1010+
});
1011+
}
1012+
1013+
it(`${object}: every row of a batch insert is stamped`, async () => {
1014+
const data = [row({ granted_by: 'u_west_1' }), row()];
1015+
await expect(h.gate.assert({ object, operation: 'insert', data, context: h.ctxOf('tenant_admin') })).resolves.toBeUndefined();
1016+
expect(data.map((r: any) => r.granted_by)).toEqual(['u_tenant_admin', 'u_tenant_admin']);
1017+
});
1018+
1019+
it(`${object}: a refused insert is not stamped`, async () => {
1020+
const data = row({ granted_by: 'u_west_1' });
1021+
await expect(h.gate.assert({ object, operation: 'insert', data, context: h.ctxOf('crud_only') })).rejects.toThrow(/delegated adminScope/);
1022+
expect(data.granted_by).toBe('u_west_1');
1023+
});
1024+
1025+
it(`${object}: an update is not stamped — the column is readonly, so the engine's update strip owns it`, async () => {
1026+
const data = { id: 'a_prev', granted_by: 'u_west_1' };
1027+
await expect(h.gate.assert({ object, operation: 'update', data, context: h.ctxOf('tenant_admin') })).resolves.toBeUndefined();
1028+
expect(data.granted_by).toBe('u_west_1');
1029+
});
1030+
}
1031+
1032+
it('a self-delegation naming another user as granter is stamped with the delegator', async () => {
1033+
const d = makeDelegationHarness();
1034+
const row: any = {
1035+
user_id: 'u_deleg', position: 'approver', delegated_from: 'u_boss', valid_until: iso(T0 + 10 * DAY),
1036+
reason: 'vacation stand-in', granted_by: 'u_relay',
1037+
};
1038+
await expect(d.delegate('u_boss', row)).resolves.toBeUndefined();
1039+
expect(row.granted_by).toBe('u_boss');
1040+
});
1041+
1042+
it('a governed object with no granted_by column is never given one', async () => {
1043+
const member: any = { user_id: 'u_x', organization_id: 'org_1', role: 'admin' };
1044+
await expect(h.gate.assert({ object: 'sys_member', operation: 'insert', data: member, context: h.ctxOf('tenant_admin') })).resolves.toBeUndefined();
1045+
const binding: any = { position_id: 'pos_sales', permission_set_id: 'ps_sales' };
1046+
await expect(h.gate.assert({ object: 'sys_position_permission_set', operation: 'insert', data: binding, context: h.ctxOf('tenant_admin') })).resolves.toBeUndefined();
1047+
expect('granted_by' in member).toBe(false);
1048+
expect('granted_by' in binding).toBe(false);
1049+
});
1050+
});

‎packages/plugins/plugin-security/src/delegated-admin-gate.ts‎

Lines changed: 48 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,40 @@ const GOVERNED_OBJECTS = new Set([
148148
const GOVERNED_OPERATIONS = new Set(['insert', 'update', 'delete', 'transfer', 'restore', 'purge']);
149149
const ANCHOR_POSITIONS = new Set(['everyone', 'guest']);
150150

151+
/**
152+
* The governed objects whose rows record their writer in `granted_by` — a
153+
* `readonly` provenance column on both (ADR-0090 D12's dual audit: `granted_by`
154+
* = writer, `delegated_from` = authority source, `reason` = why).
155+
*
156+
* The writer is the value, so a caller does not get to name one: every
157+
* non-system insert that this gate admits is stamped with its own `userId`,
158+
* whatever the payload carried — tenant-level admins, delegates and
159+
* self-delegations alike. The stamp is assigned in place on the payload rows
160+
* BEFORE the engine's insert pipeline snapshots them; the engine's create-side
161+
* `readonly` strip does not judge `sys_` objects, so the stamp is what lands.
162+
* A non-system UPDATE gets no stamp: the column is `readonly`, so the engine's
163+
* update strip drops a caller's value and the row keeps its writer.
164+
*
165+
* System writers never reach this gate (`isSystem` short-circuits the security
166+
* middleware first) and keep the id or null they wrote: invitation placement
167+
* writes its issuer, the organization-admin reconcile the attributed human or
168+
* null, the platform-admin promotion null (ADR-0118 D1 — never a sentinel).
169+
*/
170+
const GRANTED_BY_OBJECTS = new Set(['sys_user_position', 'sys_user_permission_set']);
171+
172+
/**
173+
* Stamp the writer into `granted_by` on every row of an admitted non-system
174+
* insert into a {@link GRANTED_BY_OBJECTS} table. Unconditional on purpose:
175+
* keeping a supplied value would let a caller name any user as the granter.
176+
*/
177+
function stampWriter(opCtx: any, userId: unknown): void {
178+
if (opCtx?.operation !== 'insert' || !GRANTED_BY_OBJECTS.has(opCtx?.object)) return;
179+
if (userId == null || userId === '') return;
180+
for (const row of rowsOf(opCtx)) {
181+
if (row && typeof row === 'object') row.granted_by = userId;
182+
}
183+
}
184+
151185
export interface DelegatedAdminGateDeps {
152186
/** ObjectQL engine handle (system-context reads for pre-images/lookups). */
153187
ql: any;
@@ -242,6 +276,14 @@ export class DelegatedAdminGate {
242276
if (!GOVERNED_OBJECTS.has(opCtx?.object)) return;
243277
if (!GOVERNED_OPERATIONS.has(opCtx?.operation)) return;
244278

279+
// Authority first: a refused write throws here and is never stamped.
280+
await this.authorize(opCtx);
281+
// Then provenance, on every admitted path — see {@link GRANTED_BY_OBJECTS}.
282+
stampWriter(opCtx, opCtx.context?.userId);
283+
}
284+
285+
/** The authority decision for a governed write: returns when admitted, throws when refused. */
286+
private async authorize(opCtx: any): Promise<void> {
245287
const ctx = opCtx.context ?? {};
246288
// Every business-unit anchor this call resolves is resolved in the
247289
// caller's own organization — see {@link callerOrganizationId}.
@@ -346,9 +388,9 @@ export class DelegatedAdminGate {
346388

347389
switch (opCtx.object) {
348390
case 'sys_user_position':
349-
return this.assertAssignmentWrite(opCtx, ctx, held, organizationId);
391+
return this.assertAssignmentWrite(opCtx, held, organizationId);
350392
case 'sys_user_permission_set':
351-
return this.assertDirectGrantWrite(opCtx, ctx, held);
393+
return this.assertDirectGrantWrite(opCtx, held);
352394
case 'sys_position_permission_set':
353395
return this.assertBindingWrite(opCtx, held);
354396
case 'sys_permission_set':
@@ -536,8 +578,9 @@ export class DelegatedAdminGate {
536578
* 7. [cloud#830 follow-up] if the delegation carries a `business_unit_id`
537579
* anchor, that anchor falls inside the delegator's OWN effective anchor
538580
* for the position — a delegation may NARROW visibility, never widen it.
539-
* The writer is stamped into `granted_by` (dual audit: `granted_by` = writer,
540-
* `delegated_from` = authority source).
581+
* The writer is stamped into `granted_by` by {@link DelegatedAdminGate.assert}
582+
* once this returns (dual audit: `granted_by` = writer, `delegated_from` =
583+
* authority source).
541584
*/
542585
private async assertSelfDelegation(opCtx: any, ctx: any, organizationId?: string): Promise<void> {
543586
const now = this.now();
@@ -640,9 +683,6 @@ export class DelegatedAdminGate {
640683
deny(`position '${positionName}' distributes the admin set '${b.name}' — administration cannot be self-delegated (D12 containment)`, { position: positionName, permissionSet: b.name });
641684
}
642685
}
643-
644-
// Dual audit: stamp the writer (never overwrite an explicit value).
645-
if (row.granted_by == null) row.granted_by = ctx.userId;
646686
}
647687
}
648688

@@ -737,7 +777,6 @@ export class DelegatedAdminGate {
737777

738778
private async assertAssignmentWrite(
739779
opCtx: any,
740-
ctx: any,
741780
held: HeldScope[],
742781
organizationId?: string,
743782
): Promise<void> {
@@ -777,16 +816,12 @@ export class DelegatedAdminGate {
777816
{ operation: opCtx.operation, object: opCtx.object, position: positionName },
778817
);
779818
}
780-
// Audit stamp: who granted this (insert only; never overwrite an explicit value).
781-
if (opCtx.operation === 'insert' && t.next && t.next.granted_by == null && ctx.userId) {
782-
t.next.granted_by = ctx.userId;
783-
}
784819
}
785820
}
786821

787822
// ── sys_user_permission_set: direct user ↔ set grants ───────────────
788823

789-
private async assertDirectGrantWrite(opCtx: any, ctx: any, held: HeldScope[]): Promise<void> {
824+
private async assertDirectGrantWrite(opCtx: any, held: HeldScope[]): Promise<void> {
790825
const targets = await this.materializeTargets(opCtx, 'sys_user_permission_set');
791826
for (const t of targets) {
792827
const row = t.next ?? t.prev ?? {};
@@ -817,9 +852,6 @@ export class DelegatedAdminGate {
817852
{ operation: opCtx.operation, object: opCtx.object, permissionSet: setName },
818853
);
819854
}
820-
if (opCtx.operation === 'insert' && t.next && t.next.granted_by == null && ctx.userId) {
821-
t.next.granted_by = ctx.userId;
822-
}
823855
}
824856
}
825857

0 commit comments

Comments
 (0)