diff --git a/.changeset/derived-capability-managed-by-guard.md b/.changeset/derived-capability-managed-by-guard.md new file mode 100644 index 0000000000..b151260a31 --- /dev/null +++ b/.changeset/derived-capability-managed-by-guard.md @@ -0,0 +1,43 @@ +--- +'@objectstack/plugin-security': patch +--- + +**An admin-authored capability's `label`/`description` survive the boot (#5876).** + +`bootstrapSystemCapabilities` seeds `sys_capability` in two halves: the CURATED +platform capabilities, and the back-compat DERIVED defaults — one row per +capability string a bootstrap permission set grants via `systemPermissions[]` +that nothing declared. Its seed loop refreshed `label`/`description` on whatever +row it found for a name, without looking at `managed_by`, while the comment +directly above it claimed the opposite ("do NOT clobber admin edits"). What +#2909 T3 actually made seed-once is `scope`, and only `scope`. + +For a derived name there is no authored copy to reconcile: `label` is +`humanize(name)` and `description` is `Capability .`, both generated from +the granted string. So an existing row's authored display fields were rewritten +to a humanized placeholder on **every boot**, whoever wrote them — silent data +loss, invisible from the outside. + +Reachable, narrowly, and it needs the admin row to pre-exist the grant: an admin +creates capability `X` in Setup (`managed_by:'admin'` — the only provenance the +ADR-0066 write-guard leaves admin-writable), an app whose bootstrap permission +set grants `X` is installed, and every boot from then on renames it. The reverse +order is not reachable: once the derivation has created the +`managed_by:'platform'` placeholder, the write-guard stops the admin editing it +at all. + +**The derived half now reconciles display fields only on rows it owns** — +`managed_by:'platform'` on a non-curated name, which can only be its own +placeholder from an earlier boot. `admin` rows, `package` rows and rows whose +provenance is missing are left exactly as their author wrote them, and counted +in the new `skippedAuthored` field of the seeding result (reported in the boot +summary, not warned about: nothing is degraded, the capability resolves and the +authored copy is the better one). + +**The curated half is unchanged.** Those definitions are authored by the +platform and a new version legitimately ships new copy, so a curated name still +refreshes the row it finds. `scope` stays seed-once on both halves. + +No migration and no authoring change: a placeholder that was already +overwritten is not restored (the previous text is gone), but it stops being +overwritten again, and an admin's re-edit now sticks. diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts index 1c0cb00b36..bd187205dd 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts @@ -214,14 +214,38 @@ describe('refused declarations vs. the derived placeholder (#4967 Part 1)', () = }); }); - it('REVERSE: dropping that name from the list lets the derivation overwrite the admin row', async () => { - // Why the unowned path checks for an EXISTING row instead of always - // falling through: the derived defaults refresh label/description on any - // row they find. + it('SECOND LAYER (#5876): dropping that name from the list no longer overwrites the admin row', async () => { + // This pin was written by #5875 (as `REVERSE: dropping that name from the + // list lets the derivation overwrite the admin row`) to justify why the + // unowned path checks for an EXISTING row instead of always falling + // through: back then the derived defaults refreshed label/description on + // ANY row they found, so the skip list was the ONLY thing standing between + // an admin-authored row and a humanized placeholder. That fact is fixed — + // #5876 guards the derived reconcile by `managed_by`, so the write site + // now refuses the clobber even when the call site forgets to suppress it. + // + // The suppression list keeps its job (it is what states the boot-order + // contract, and it stops the derivation doing work on names another pass + // owns), but it is no longer load-bearing for THIS shape — which is the + // point of a second layer. const ql = makeQl([]); ql.rows.push({ id: 'cap_admin', name: 'showcase.export_data', label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin' }); await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: [] }); - expect(ql.rows.find((r) => r.name === 'showcase.export_data')?.label).toBe('Showcase Export Data'); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin', + }); + }); + + it('DISCRIMINATION (#5876): with the name dropped, a PLATFORM placeholder is still refreshed', async () => { + // The guard above must not read as "the derivation stopped writing". Same + // fixture, same empty skip list, provenance flipped to the one the derived + // pass owns → the reconcile happens exactly as it always did. + const ql = makeQl([]); + ql.rows.push({ id: 'cap_derived', name: 'showcase.export_data', label: 'Stale Placeholder', description: 'Stale.', managed_by: 'platform' }); + await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: [] }); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + label: 'Showcase Export Data', description: 'Capability showcase.export_data.', managed_by: 'platform', + }); }); it('FOREIGN owner: suppresses, because the other package authored that row', async () => { diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts index d546ccff41..09a8145aca 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts @@ -94,3 +94,117 @@ describe('bootstrapSystemCapabilities (ADR-0066 D1 back-compat seed)', () => { expect(KNOWN_CAPABILITIES.filter((c) => c.scope === 'platform').length).toBeGreaterThanOrEqual(5); }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#5876] The DERIVED half reconciles display fields only on rows it OWNS. +// +// The seed loop used to refresh `label`/`description` on whatever row it found +// for a derived name, whatever its provenance — while the comment above it said +// admin edits were not clobbered. For a derived name `label` is `humanize(name)` +// and `description` is `Capability .`, so an admin-authored row was +// rewritten to a humanized placeholder on EVERY boot (silent data loss; the +// reachable chain is: admin creates the capability in Setup → an app whose +// bootstrap permission set grants it by name is installed → every boot after). +// +// The CURATED half keeps refreshing (the platform ships new copy for its own +// definitions — pinned by 'does NOT clobber an admin-edited scope on re-seed' +// above); only the derived half is guarded, so these pins must DISCRIMINATE +// rather than just prove nothing is written. +// ─────────────────────────────────────────────────────────────────────────── +describe('derived defaults never clobber an authored row (#5876)', () => { + const OPS_SETS = [{ systemPermissions: ['showcase.export_data'] }]; + const AUTHORED = { label: 'Admin Made', description: 'Admin wrote this.' }; + + /** A pre-existing row for a name the derivation would otherwise derive. */ + function seedRow(ql: ReturnType, managed_by: string | undefined) { + ql.rows.push({ + id: 'cap_existing', + name: 'showcase.export_data', + ...AUTHORED, + scope: 'org', + ...(managed_by === undefined ? {} : { managed_by }), + active: true, + }); + } + + it('leaves an ADMIN-authored row untouched', async () => { + const ql = makeQl(); + seedRow(ql, 'admin'); + const out = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + ...AUTHORED, managed_by: 'admin', scope: 'org', + }); + expect(out.skippedAuthored).toBe(1); + // The skip is a SKIP, not a silent failed write: it is not counted as an update. + expect(ql.rows.filter((r) => r.name === 'showcase.export_data')).toHaveLength(1); + }); + + it('leaves a PACKAGE-authored row untouched', async () => { + const ql = makeQl(); + ql.rows.push({ + id: 'cap_pkg', name: 'showcase.export_data', label: 'Export Data', description: 'Bulk export.', + managed_by: 'package', package_id: 'com.acme.reports', active: true, + }); + const out = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + label: 'Export Data', description: 'Bulk export.', managed_by: 'package', package_id: 'com.acme.reports', + }); + expect(out.skippedAuthored).toBe(1); + }); + + it('leaves a row of UNKNOWN provenance untouched (the field defaults to admin)', async () => { + // `sys_capability.managed_by` is required with `defaultValue: 'admin'`, so a + // row that reaches this pass without one is not a platform placeholder — + // "not provably ours" resolves to leave-it-alone, never to overwrite. + const ql = makeQl(); + seedRow(ql, undefined); + const out = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject(AUTHORED); + expect(out.skippedAuthored).toBe(1); + }); + + it('POSITIVE CONTROL: still refreshes its OWN platform placeholder', async () => { + // Same fixture, same grant — only the provenance differs. A guard that also + // switched this case off would be indistinguishable from deleting the + // reconcile, so this is what gives the three pins above their teeth. + const ql = makeQl(); + seedRow(ql, 'platform'); + const out = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + label: 'Showcase Export Data', description: 'Capability showcase.export_data.', managed_by: 'platform', + }); + expect(out.skippedAuthored).toBe(0); + expect(out.updated).toBeGreaterThanOrEqual(1); + // [#2909 T3] `scope` stays seed-once even on a row this pass owns. + expect(ql.rows.find((r) => r.name === 'showcase.export_data')?.scope).toBe('org'); + }); + + it('stays stable across boots: derive, then never re-write the row again', async () => { + const ql = makeQl(); + const boot1 = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(boot1.seeded).toBe(KNOWN_CAPABILITIES.length + 1); + // An admin renames the derived placeholder… which the platform/package write + // guard actually refuses today (see #5876's reachability note), so simulate + // the storage effect only, and re-boot. + const row = ql.rows.find((r) => r.name === 'showcase.export_data')!; + row.label = 'Renamed By Admin'; + row.managed_by = 'admin'; + const boot2 = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(row.label).toBe('Renamed By Admin'); + expect(boot2.seeded).toBe(0); + expect(boot2.skippedAuthored).toBe(1); + }); + + it('the guard is scoped to the DERIVED half — curated names still refresh', async () => { + const ql = makeQl(); + await bootstrapSystemCapabilities(ql, []); + const curated = KNOWN_CAPABILITIES[0]; + const row = ql.rows.find((r) => r.name === curated.name)!; + row.label = 'stale label'; + row.description = 'stale description'; + const out = await bootstrapSystemCapabilities(ql, []); + expect(row.label).toBe(curated.label); + expect(row.description).toBe(curated.description); + expect(out.skippedAuthored).toBe(0); + }); +}); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts index 4574795f91..c27429552f 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts @@ -18,6 +18,18 @@ * seeding). Platform-seeded rows are `managed_by: 'platform'` so they are not * presented as admin-deletable. Runs on `kernel:ready` alongside the other * security bootstraps. + * + * [#5876] The two halves have DIFFERENT authority over an existing row's + * display fields, because they have different claims to authorship: + * - CURATED — the platform authored `label`/`description`, and a new version + * may ship new copy, so the row it finds is refreshed; + * - DERIVED — there is no authored copy at all, only `humanize(name)` and + * `Capability .` generated from a granted string, so it refreshes + * only its OWN placeholder (`managed_by:'platform'` on a non-curated name) + * and never a row an admin or a package authored. + * The seed loop used to refresh both alike while the comment in front of it + * claimed admin edits were preserved — what #2909 T3 actually made seed-once is + * `scope`, and only `scope`. */ import { PLATFORM_CAPABILITIES, type PlatformCapability } from '@objectstack/spec/security'; @@ -78,13 +90,31 @@ interface SeedOptions { materializedCapabilityNames?: Iterable; } +/** Aggregated outcome of a back-compat capability seeding pass. */ +export interface CapabilitySeedResult { + /** Rows inserted (curated definitions + derived placeholders). */ + seeded: number; + /** Rows whose platform display fields were reconciled. */ + updated: number; + /** + * [#5876] Derived names whose existing row is authored elsewhere + * (`managed_by` anything but `'platform'`), so its `label`/`description` were + * left as their author wrote them. Not a degradation — the capability + * resolves and the authored copy is the better one — so it is reported in the + * boot summary rather than warned about (#4632). + */ + skippedAuthored: number; + /** Definitions considered this pass (curated + derived). */ + total: number; +} + export async function bootstrapSystemCapabilities( ql: any, permissionSets: Array<{ systemPermissions?: string[] }> = [], options: SeedOptions = {}, -): Promise<{ seeded: number; updated: number; total: number }> { +): Promise { if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') { - return { seeded: 0, updated: 0, total: 0 }; + return { seeded: 0, updated: 0, skippedAuthored: 0, total: 0 }; } const materialized = new Set(options.materializedCapabilityNames ?? []); @@ -94,26 +124,58 @@ export async function bootstrapSystemCapabilities( // ones that already have a row, which the declared seeder owns. const byName = new Map(); for (const c of KNOWN_CAPABILITIES) byName.set(c.name, c); + // [#5876] Which names came from the DERIVED half. The two halves carry + // different authority over an existing row's display fields (see the + // reconcile guard below), and after this loop `byName` cannot tell them + // apart on its own. + const derivedNames = new Set(); for (const ps of permissionSets) { for (const cap of ps?.systemPermissions ?? []) { if (typeof cap === 'string' && cap && !byName.has(cap) && !materialized.has(cap)) { byName.set(cap, { name: cap, label: humanize(cap), description: `Capability ${cap}.`, scope: 'platform' }); + derivedNames.add(cap); } } } let seeded = 0; let updated = 0; + let skippedAuthored = 0; for (const def of byName.values()) { const existing = await tryFind(ql, 'sys_capability', { name: def.name }, 1); - if (existing[0]?.id) { - // Keep label/description fresh, but do NOT clobber admin edits — only - // platform-owned display fields are reconciled. `scope` is an - // admin-editable classification face (plain select on sys_capability), - // so it is seed-once: written on insert, never refreshed (#2909 T3). - // A curated scope change in a new platform version needs a data - // migration — recorded in the ADR-0094 addendum. - if (await tryUpdate(ql, 'sys_capability', { id: existing[0].id, label: def.label, description: def.description })) { + const row = existing[0]; + if (row?.id) { + // [#5876] Reconcile display fields only where THIS pass owns the copy. + // + // A DERIVED name has no authored copy to ship: `label` is `humanize(name)` + // and `description` is `Capability .`, both generated from the + // string a permission set happened to grant. Refreshing those onto a row + // somebody else authored is not reconciliation, it is overwriting an + // author with a placeholder — every boot, silently. For a non-curated + // name a `managed_by:'platform'` row can only be this same derivation's + // placeholder from an earlier boot, so that is exactly the set of rows + // the derived half may refresh; `admin` (Setup-authored), `package` + // (declared by its owning package) and anything else are left alone. + // + // The CURATED half is unchanged: those definitions are authored by the + // platform and a new version legitimately ships new copy, so a curated + // name still refreshes the row it finds. + // + // NOTE this is the WRITE-side enforcement of the same rule + // `materializedCapabilityNames` states at the CALL site (#4967 Part 1): + // the caller says which names another pass already materialized, and + // this guard holds even when nothing said so — an admin row for a name + // no package ever declared is invisible to that list. + if (derivedNames.has(def.name) && row.managed_by !== 'platform') { + skippedAuthored += 1; + continue; + } + // Keep label/description fresh from the platform's own definition. + // `scope` is an admin-editable classification face (plain select on + // sys_capability), so it is seed-once: written on insert, never + // refreshed (#2909 T3). A curated scope change in a new platform version + // needs a data migration — recorded in the ADR-0094 addendum. + if (await tryUpdate(ql, 'sys_capability', { id: row.id, label: def.label, description: def.description })) { updated += 1; } } else { @@ -129,6 +191,8 @@ export async function bootstrapSystemCapabilities( if (created) seeded += 1; } } - options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', { seeded, updated, total: byName.size }); - return { seeded, updated, total: byName.size }; + options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', { + seeded, updated, skippedAuthored, total: byName.size, + }); + return { seeded, updated, skippedAuthored, total: byName.size }; }