Skip to content

Commit 5cb62d8

Browse files
os-samclaude
andauthored
fix(security): make clone_permission_set carry all five copied facets (#11755)
* fix(security): make clone_permission_set carry all five copied facets The Clone action POSTs its `params` values to the generic data door, so the params list IS the payload. It named two of the six definition facets a `sys_permission_set` row carries, leaving `system_permissions`, `row_level_security` and `tab_permissions` absent from the body; `permissionSetBodyFromRow()` then read each through `parseMaybeJson(undefined, …)` and filled the empty default. Cloning a set that granted `setup.access`, or one carrying RLS policies, produced a clone with none of them — record created, success toast fired, loss discoverable only by diffing the two records. Urgent as of one commit ago: the save door now refuses an in-place edit of a package-declared set AND its refusal names the clone path, so this action is the platform's own recommended remedy. The accept surface does not move — `permissionSetBodyFromRow()` already read all six columns. What changed is what the action SENDS. `admin_scope` is deliberately NOT copied (maintainer ruling 2026-08-24): an ADR-0090 D12 delegated-admin authority on a new org-owned set is a privilege decision, not a field copy. The dialog description now states the exclusion, so it reads as a decision rather than as the same silent drop. Pinned by `packaged-permission-set-lock.test.ts` pin 6, which READS the action's params list to build the payload instead of restating it, and asserts each facet by identity against a non-empty value. Verified red (3 failed / 13 passed) on the unmodified tree, green (16 passed) after the fix. Fixes #11703 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 * fix(security): annotate richRow's return type so the TEST_DEBT ratchet holds `check:type-check-debt --re-measure` measures plugin-security with the `**/*.test.ts` exclusion dropped, and there the new pin cost +2 raw errors (11 -> 13) on a shrink-only ledger: tsc infers `richRow`'s object-literal type and DROPS the index signature `permissionSetRowFields()` spreads in, so the exclusion control's direct `.admin_scope` read is a TS2339. The package's own `typecheck` excludes tests, so it stayed green throughout — this layer is the only thing that sees it. Fixed at the source, which is the author's remedy; the ledger is untouched. Re-measured 11, matching the recorded entry exactly, with zero errors in this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6d0cccc commit 5cb62d8

7 files changed

Lines changed: 334 additions & 1 deletion

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
Make `clone_permission_set` carry the system permissions, row-level security
6+
and tab permissions it was silently dropping
7+
8+
The Clone action POSTs its `params` values to `/api/v1/data/sys_permission_set`,
9+
so the params list *is* the payload. It named two of the six definition facets a
10+
`sys_permission_set` row carries — `object_permissions` and `field_permissions`
11+
— leaving `system_permissions`, `row_level_security` and `tab_permissions`
12+
absent from the body. `permissionSetBodyFromRow()` then read each one through
13+
`parseMaybeJson(undefined, …)` and filled the empty default, so cloning a set
14+
that grants `setup.access`, or one carrying row-level security policies,
15+
produced a clone with none of them: record created, success toast fired, and the
16+
missing half discoverable only by diffing the two records.
17+
18+
The three now travel, in the same JSON-string shape the two listed columns
19+
already used. Nothing about what the door ACCEPTS changed — `permissionSetBodyFromRow()`
20+
already read all six columns; what changed is what the action SENDS.
21+
22+
This became urgent one commit ago. The save door now refuses an in-place edit of
23+
a package-declared permission set **and its refusal message tells the admin to
24+
clone**, which made this action the platform's own recommended remedy while it
25+
was still dropping three facets — an admin following that instruction lost
26+
grants quietly. The failure direction was fail-closed (fewer grants), which is
27+
why it was quiet.
28+
29+
`admin_scope` is **deliberately not copied** (maintainer ruling 2026-08-24).
30+
Putting an ADR-0090 D12 delegated-admin authority onto a brand-new
31+
organization-owned set on the admin's behalf is a privilege decision, not a
32+
field copy. The Clone dialog now says so in its description, so the omission
33+
reads to the admin as a decision rather than as the same silent drop — grant a
34+
scope deliberately on the new set if it needs one.
35+
36+
Pinned by `packaged-permission-set-lock.test.ts` pin 6, which assembles the
37+
clone payload by READING the action's params list rather than restating it, and
38+
asserts each facet by identity against a non-empty value — the empty default
39+
(`[]` / `{}`) is exactly what a "present" assertion would have accepted. Its
40+
control proves the exclusion is live: the base fixture carries a real
41+
`admin_scope`, and the clone still has none.

packages/plugins/plugin-security/src/objects/sys-permission-set.object.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,26 @@ export const SysPermissionSet = ObjectSchema.create({
9292
visible: "has(record.drift_status) && record.drift_status == 'overlay_shadow'",
9393
},
9494
{
95+
// [#11703] ⭐ THIS PARAMS LIST *IS* THE PAYLOAD. Every definition facet a
96+
// clone should carry has to be named below: the action POSTs its param
97+
// VALUES to the generic data door, so a column that is not a param is
98+
// simply absent from the body — `permissionSetBodyFromRow()` then reads
99+
// it through `parseMaybeJson(undefined, …)` and fills the EMPTY default
100+
// (`[]` / `{}`). An omission here is therefore not a missing input, it is
101+
// a silent grant loss: the clone is created, the success toast fires, and
102+
// the difference is discoverable only by diffing the two records.
103+
//
104+
// That was live until #11703 — three of the six facets
105+
// (`system_permissions`, `row_level_security`, `tab_permissions`) were
106+
// never listed, so cloning a set carrying system permissions or RLS
107+
// produced a clone with none of them. Fail-closed, and therefore quiet.
108+
// It became urgent the moment the save door started REFUSING in-place
109+
// edits of package-declared sets and naming THIS action as the remedy
110+
// (`packaged-permission-set-lock.ts`, maintainer ruling 2026-08-24).
111+
//
112+
// `packaged-permission-set-lock.test.ts` pin 6 READS this list rather
113+
// than restating it, so adding a facet column to the object surfaces
114+
// here rather than silently shipping a clone that drops it.
95115
name: 'clone_permission_set',
96116
label: 'Clone',
97117
icon: 'copy',
@@ -102,6 +122,16 @@ export const SysPermissionSet = ObjectSchema.create({
102122
method: 'POST',
103123
target: '/api/v1/data/sys_permission_set',
104124
bodyExtra: { active: true },
125+
// The explanatory line under the clone dialog's title. Its second
126+
// sentence is the RULED exclusion (2026-08-24): `admin_scope` — an
127+
// ADR-0090 D12 delegated-admin authority — is deliberately NOT copied,
128+
// because putting one on a brand-new set on the admin's behalf is a
129+
// privilege decision, not a field copy. It is stated HERE, where the
130+
// admin is standing when the clone happens, because an UNEXPLAINED
131+
// omission is the same silent drop #11703 reports, merely ruled.
132+
description:
133+
'Copies this set\'s permissions into a new organization-owned set you can edit. '
134+
+ 'Delegated-admin scope is not copied — grant it deliberately on the new set if it needs one.',
105135
successMessage: 'Permission set cloned',
106136
refreshAfter: true,
107137
params: [
@@ -113,6 +143,14 @@ export const SysPermissionSet = ObjectSchema.create({
113143
{ field: 'description', defaultFromRow: true },
114144
{ field: 'object_permissions', defaultFromRow: true },
115145
{ field: 'field_permissions', defaultFromRow: true },
146+
// [#11703] The three facets the clone silently dropped. Same
147+
// JSON-string shape as the two above: `permissionSetRowFields()`
148+
// writes all five with `JSON.stringify`, and the data door parses all
149+
// five back — the accept surface did not move, only what is SENT.
150+
{ field: 'system_permissions', defaultFromRow: true },
151+
{ field: 'row_level_security', defaultFromRow: true },
152+
{ field: 'tab_permissions', defaultFromRow: true },
153+
// ⛔ `admin_scope` is deliberately absent — see `description` above.
116154
],
117155
},
118156
],

packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts

Lines changed: 251 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,11 @@
2929
* 3. the clone path works end to end — org-owned row, no upgrade linkage —
3030
* and the package-declared base is untouched by it;
3131
* 5. fail-closed on ambiguity: a provenance read that cannot ANSWER refuses,
32-
* never accepts.
32+
* never accepts;
33+
* 6. ⭐ what the clone ACTION SENDS (#11703) — pin 3 drives the door with a
34+
* hand-written payload, so it could not see that the ACTION ITSELF listed
35+
* only two of the row's six definition facets. Pin 6 reads the payload out
36+
* of the action definition, so editing that params list is what moves it.
3337
*
3438
* (Pin 4 — the detection reading for overlays that already exist — lives in its
3539
* own suite at the bottom of this file, because it reads rather than writes.)
@@ -62,6 +66,7 @@
6266
import { describe, it, expect } from 'vitest';
6367
import { PermissionSetSchema } from '@objectstack/spec/security';
6468
import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core';
69+
import { SysPermissionSet } from './objects/sys-permission-set.object.js';
6570
import {
6671
createPermissionSetWriteThrough,
6772
permissionSetRowFields,
@@ -499,6 +504,251 @@ describe('pin 3 — the clone path yields an org-owned set with no upgrade linka
499504
});
500505
});
501506

507+
// ─────────────────────────────────────────────────────────────────────────────
508+
// PIN 6 — what the CLONE ACTION SENDS: every copied facet, by identity (#11703)
509+
// ─────────────────────────────────────────────────────────────────────────────
510+
511+
/**
512+
* Pin 3 above drives the data door with a HAND-WRITTEN clone payload. That is
513+
* precisely why it stayed green while this defect was live: it pins what the
514+
* SERVER does with a payload, never what the ACTION DEFINITION chooses to put
515+
* in one. `clone_permission_set` listed two of the row's six definition facets
516+
* on its `params`, so cloning a set carrying system permissions, row-level
517+
* security or tab permissions produced a clone with NONE of them — no error, a
518+
* success toast, and the loss discoverable only by diffing the two records
519+
* (#11703). Fail-closed (fewer grants), and therefore quiet.
520+
*
521+
* It matters more since the ruling one commit above this one: the save door now
522+
* refuses an in-place edit of a package-declared set AND its refusal names the
523+
* clone path, so this action is the platform's own recommended remedy.
524+
*
525+
* So this suite reads the payload OUT OF THE ACTION instead of restating it.
526+
* {@link clonePayload} is the clone dialog in miniature — the admin types the
527+
* two inline params, every `defaultFromRow` param is seeded from the source row
528+
* as objectui's `ActionParamDialog` seeds it, and `bodyExtra` rides along — so
529+
* editing the params list is what moves this suite. That is the whole point:
530+
* the class reopens the next time someone edits that list, unless a test is
531+
* reading the list rather than a copy of it.
532+
*
533+
* ⭐ IDENTITIES, NOT COUNTS. "Five facets came across" holds constant while two
534+
* of them swap, and asserting a facet is merely PRESENT passes on the empty
535+
* default (`[]` / `{}`) that IS the bug. Every facet below is asserted against
536+
* a NAMED, NON-EMPTY expected value.
537+
*/
538+
539+
/** The shipped `clone_permission_set` action — read, never restated. */
540+
const cloneAction = (): any => {
541+
const action = (SysPermissionSet.actions ?? []).find((a: any) => a.name === 'clone_permission_set');
542+
if (!action) throw new Error('clone_permission_set is missing from SysPermissionSet.actions');
543+
return action;
544+
};
545+
546+
/**
547+
* The body the clone dialog POSTs to `/api/v1/data/sys_permission_set`,
548+
* assembled from the ACTION DEFINITION the way the dialog assembles it: a
549+
* `defaultFromRow` param is seeded from the source row under its resolved field
550+
* name and submitted verbatim when the admin does not touch it; an inline param
551+
* carries what the admin typed; `bodyExtra` is merged in.
552+
*/
553+
function clonePayload(row: any, typed: Record<string, any>): Record<string, any> {
554+
const action = cloneAction();
555+
const body: Record<string, any> = { ...(action.bodyExtra ?? {}) };
556+
for (const p of action.params ?? []) {
557+
const key = p.field ?? p.name;
558+
if (p.defaultFromRow) {
559+
if (row[key] !== undefined) body[key] = row[key];
560+
} else if (key in typed) {
561+
body[key] = typed[key];
562+
}
563+
}
564+
return body;
565+
}
566+
567+
/**
568+
* The base an admin clones: EVERY definition facet populated, so no assertion
569+
* below can be satisfied by the empty default the defect produced.
570+
*
571+
* ⚠️ The card's repro sketch named `member_default` as "a set whose
572+
* `system_permissions` is non-empty". Measured on this tree that is not so —
573+
* the platform `member_default` carries a large `rowLevelSecurity` and NO
574+
* system permissions, and `showcase_member_default` carries neither (the D7
575+
* lint hard-blocks system permissions on an everyone-suggested set). The defect
576+
* is real either way; the example was not. A fixture carrying all six facets at
577+
* once is the shape that actually measures all three of the added ones, which a
578+
* single real set would not.
579+
*/
580+
const richSet = (over: Record<string, any> = {}) => ({
581+
name: 'ops_console',
582+
label: 'Ops Console',
583+
description: 'Runs the operations console.',
584+
objects: {
585+
showcase_task: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: false },
586+
showcase_project: { allowRead: true },
587+
},
588+
fields: { 'showcase_project.budget': { readable: true, editable: false } },
589+
// The three facets #11703 dropped, each non-empty and each named below.
590+
systemPermissions: ['setup.access', 'ops.export_data'],
591+
rowLevelSecurity: [
592+
{
593+
name: 'ops_own_rows',
594+
label: 'Own Tasks Only',
595+
description: 'Operators only select tasks assigned to them.',
596+
object: 'showcase_task',
597+
operation: 'select' as const,
598+
using: 'assignee == current_user.email',
599+
positions: ['ops'],
600+
enabled: true,
601+
},
602+
],
603+
tabPermissions: { app_ops: 'default_on' as const, app_admin: 'hidden' as const },
604+
// The RULED exclusion's positive control — see the last test.
605+
adminScope: {
606+
businessUnit: 'field_ops',
607+
includeSubtree: true,
608+
manageAssignments: true,
609+
manageBindings: false,
610+
authorEnvironmentSets: false,
611+
assignablePermissionSets: ['ops_console'],
612+
},
613+
...over,
614+
});
615+
616+
/**
617+
* The row the package door materializes for {@link richSet}.
618+
*
619+
* The return annotation is load-bearing, not decoration: without it tsc infers
620+
* the object-literal type and DROPS the index signature that
621+
* `permissionSetRowFields()` spreads in, so reading `.admin_scope` off the
622+
* result — which the exclusion control below does directly, rather than through
623+
* the `any[]` of `ql.permRows` — is a TS2339. It costs no precision: every
624+
* facet column arrives through that spread already typed `any`.
625+
*/
626+
const richRow = (over: Record<string, any> = {}): Record<string, any> => ({
627+
id: 'ps_rich',
628+
name: 'ops_console',
629+
managed_by: 'package',
630+
package_id: 'com.example.ops',
631+
active: true,
632+
...permissionSetRowFields(richSet()),
633+
...over,
634+
});
635+
636+
describe('pin 6 — the clone action SENDS every facet it copies, and says what it deliberately does not (#11703)', () => {
637+
it('declares a defaultFromRow param for every copied facet — the identities, not a count', () => {
638+
const carried = (cloneAction().params ?? [])
639+
.filter((p: any) => p.defaultFromRow)
640+
.map((p: any) => p.field ?? p.name)
641+
.sort();
642+
643+
// ⭐ A COUNT here (`toHaveLength(6)`) would hold while `system_permissions`
644+
// was swapped for something else. The set is spelled out.
645+
expect(carried, 'the params list is what the dialog sends — these are the facets it carries').toEqual([
646+
'description',
647+
'field_permissions',
648+
'object_permissions',
649+
'row_level_security',
650+
'system_permissions',
651+
'tab_permissions',
652+
]);
653+
// `admin_scope` is the sixth definition column and is RULED out — pinned in
654+
// its own test below so a future reader sees a decision, not an oversight.
655+
expect(carried, 'admin_scope is excluded by ruling, not by accident').not.toContain('admin_scope');
656+
});
657+
658+
it('cloning a fully-populated set carries all five copied facets end to end, by name', async () => {
659+
const ql = makeQl([richSet()]);
660+
const protocol = makeHatchOpenProtocol(ql, { ops_console: richSet() });
661+
registerPermissionSetProjection(protocol, { ql });
662+
ql.permRows.push(richRow());
663+
const mw = makeMiddleware(ql, protocol);
664+
665+
// The payload comes from the ACTION, not from this test.
666+
const payload = clonePayload(ql.permRows[0], {
667+
label: 'Ops Console (local)',
668+
name: 'ops_console_local',
669+
});
670+
await run(mw, {
671+
object: 'sys_permission_set', operation: 'insert', context: userCtx, data: payload,
672+
});
673+
674+
// (a) the DEFINITION that will be enforced — what `saveMetaItem` stored.
675+
const body = protocol.saves.find((s: any) => s.name === 'ops_console_local')?.item;
676+
expect(body, 'the clone was authored into the metadata store').toBeTruthy();
677+
expect(body.name).toBe('ops_console_local');
678+
expect(body.label, 'the admin-typed display name, not the base label').toBe('Ops Console (local)');
679+
expect(body.description).toBe(richSet().description);
680+
expect(body.objects, 'object permissions').toEqual(richSet().objects);
681+
expect(body.fields, 'field permissions').toEqual(richSet().fields);
682+
// ⭐ The three #11703 dropped. `[]` / `{}` here is the defect itself.
683+
expect(body.systemPermissions, 'system permissions — [] here IS the #11703 silent drop').toEqual(
684+
['setup.access', 'ops.export_data'],
685+
);
686+
expect(
687+
(body.rowLevelSecurity ?? []).map((p: any) => p.name),
688+
'row-level security policies, by policy name',
689+
).toEqual(['ops_own_rows']);
690+
expect(body.rowLevelSecurity, 'the RLS policy arrived whole, not as a name-only husk').toEqual(
691+
richSet().rowLevelSecurity,
692+
);
693+
expect(body.tabPermissions, 'tab permissions, per tab').toEqual({
694+
app_ops: 'default_on', app_admin: 'hidden',
695+
});
696+
697+
// (b) the ROW the admin actually diffs the two records through.
698+
const clone = ql.permRows.find((r: any) => r.name === 'ops_console_local');
699+
expect(clone, 'the clone record exists').toBeTruthy();
700+
expect(clone.managed_by, "this org's row").toBe('admin');
701+
expect(clone.package_id ?? null, 'no upgrade linkage').toBeNull();
702+
expect(JSON.parse(clone.system_permissions)).toEqual(['setup.access', 'ops.export_data']);
703+
expect(JSON.parse(clone.row_level_security).map((p: any) => p.name)).toEqual(['ops_own_rows']);
704+
expect(JSON.parse(clone.tab_permissions)).toEqual({ app_ops: 'default_on', app_admin: 'hidden' });
705+
706+
// The base is untouched by all of this (pin 3's invariant, re-checked on
707+
// the richer shape — a payload that now carries five facets is a payload
708+
// with five more chances to write to the wrong row).
709+
const base = ql.permRows.find((r: any) => r.id === 'ps_rich');
710+
expect(JSON.parse(base.system_permissions), 'the package-declared base still declares its own').toEqual(
711+
['setup.access', 'ops.export_data'],
712+
);
713+
expect(base.name).toBe('ops_console');
714+
});
715+
716+
it('CONTROL — admin_scope is deliberately NOT carried (ADR-0090 D12), and the dialog says so', async () => {
717+
// ⭐ POSITIVE CONTROL FIRST. Without it "the clone has no admin_scope" is
718+
// satisfied by a base that never had one, and this test would pin nothing.
719+
const baseRow = richRow();
720+
expect(baseRow.admin_scope, 'the base carries a delegated-admin scope to lose').toBeTruthy();
721+
expect(JSON.parse(baseRow.admin_scope).businessUnit).toBe('field_ops');
722+
723+
// It is not on the wire…
724+
const payload = clonePayload(baseRow, { label: 'Ops Console (local)', name: 'ops_console_local' });
725+
expect(Object.keys(payload), 'the clone dialog never sends admin_scope').not.toContain('admin_scope');
726+
727+
// …and it is not on the clone.
728+
const ql = makeQl([richSet()]);
729+
const protocol = makeHatchOpenProtocol(ql, { ops_console: richSet() });
730+
registerPermissionSetProjection(protocol, { ql });
731+
ql.permRows.push(baseRow);
732+
const mw = makeMiddleware(ql, protocol);
733+
await run(mw, {
734+
object: 'sys_permission_set', operation: 'insert', context: userCtx, data: payload,
735+
});
736+
737+
const body = protocol.saves.find((s: any) => s.name === 'ops_console_local')?.item;
738+
expect(body, 'the clone was authored').toBeTruthy();
739+
expect(body.adminScope, 'a delegated-admin authority is a privilege decision, never a field copy').toBeUndefined();
740+
const clone = ql.permRows.find((r: any) => r.name === 'ops_console_local');
741+
expect(clone.admin_scope ?? null, 'and the column stays empty on the clone').toBeNull();
742+
743+
// ⭐ The exclusion has to READ as a decision to the admin standing in the
744+
// dialog — otherwise it is the same silent drop #11703 reports, merely
745+
// ruled. The dialog's own explanatory line carries it.
746+
const description = String(cloneAction().description ?? '');
747+
expect(description, 'the clone dialog states the exclusion').toMatch(/delegated-admin scope/i);
748+
expect(description, 'and states that it is not copied').toMatch(/not copied/i);
749+
});
750+
});
751+
502752
// ─────────────────────────────────────────────────────────────────────────────
503753
// PIN 5 — fail-closed on ambiguity
504754
// ─────────────────────────────────────────────────────────────────────────────

0 commit comments

Comments
 (0)