From 20fd57a6aaa06db2af6a25f70faffca295e35fe5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 10:38:48 +0000 Subject: [PATCH] fix(plugin-security): report only materialized capability names, so a refused declaration falls back to the derived placeholder (#4967 Part 1/3) bootstrapDeclaredCapabilities filled its returned name list BEFORE the upsert decided anything, so all three refusal paths reported a name they never wrote a row for. The caller uses that list to tell bootstrapSystemCapabilities to skip deriving the back-compat placeholder, so a declaration refused for want of an owning package suppressed the placeholder too and the capability then existed in no sys_capability row at all -- writing the declaration was strictly worse than omitting it. The list (renamed materializedNames) now reports only names this pass CONFIRMED have a row: written here (seeded/updated/claimed), or an existing row that must not be clobbered (admin-authored, another package's, or a curated platform name the curated pass owns). The unowned path reports its name only when a row already exists, and otherwise falls through to the derivation. Adds the skippedUnowned counter that path never had, so every named declaration lands in exactly one counter and the list reconciles with them. Part 3: the unowned-refusal diagnostic stays a warn (#4632 -- functional degradation, not durability) and now names the permission set(s) that GRANT the capability plus the actual consequence, threaded in as an argument from the bootstrap permission sets rather than any new global state. Part 2 of #4967 (stack.capabilities -> registry _packageId) is out of scope here and tracked as #5870. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../refused-capability-declaration-hole.md | 48 ++++ .../bootstrap-declared-capabilities.test.ts | 225 +++++++++++++++++- .../src/bootstrap-declared-capabilities.ts | 160 +++++++++++-- .../src/bootstrap-system-capabilities.test.ts | 4 +- .../src/bootstrap-system-capabilities.ts | 25 +- .../plugin-security/src/security-plugin.ts | 22 +- 6 files changed, 444 insertions(+), 40 deletions(-) create mode 100644 .changeset/refused-capability-declaration-hole.md diff --git a/.changeset/refused-capability-declaration-hole.md b/.changeset/refused-capability-declaration-hole.md new file mode 100644 index 0000000000..792a392f91 --- /dev/null +++ b/.changeset/refused-capability-declaration-hole.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): 被拒收的 capability 声明不再连派生占位一起压掉 (#4967 Part 1/3) + +`SecurityPlugin` 分两遍种 `sys_capability`:第一遍落包声明的 capability +(`managed_by:'package'` + `package_id`),第二遍种平台 curated 集合 + 从 +permission set 的 `systemPermissions[]` **派生**的 back-compat 占位,并**跳过** +第一遍报上来的名字,以免占位把已写好的声明覆盖掉。 + +问题在于第一遍报的是「读到的每个名字」,而不是「真正落了行的名字」: +`bootstrapDeclaredCapabilities` 在 upsert 作出任何决定**之前**就把 +`cap.name` 推进了返回列表。而 upsert 有三条**拒收**路径,一行都不写。其中 +「声明没有归属包」这一条既没写行、又占住了名字,于是派生占位也被跳过—— +capability **在任何一行里都不存在**。净效果是:**写下这条声明,比不写还糟** +(不写至少还有派生占位)。这正是 showcase 的 +`showcase.export_data` 只留下一条 `warn` 的成因。 + +修法是把「上报」与「读到」拆开:一个名字进入上报列表(现更名为 +`materializedNames`)的条件,是本遍**确认它有行**——本遍写成了 +(seeded / updated / claimed),或找到一行不能被覆盖的既有行(admin 自建、他包 +所有、curated 平台名)。三条拒收路径按「派生是否会覆盖既有 authored 行」分别 +处置,理由写在代码里: + +- **curated 平台名**:仍然上报。curated 那一遍无条件种这些名字,行必然存在; + 且派生路径本来就够不到 curated 名(它已在 curated 表里)。 +- **他包所有 / admin 自建**:仍然上报。行存在且 label/description 是**作者写 + 的**,派生会把它们刷成 humanize 出来的占位——压掉派生正是这份列表的用途。 +- **没有归属包**:仅当已存在一行时才上报。没有行时回落到派生占位,和「从未 + 写过这条声明」时一样。 + +同时补上这条路径此前缺失的计数器 `skippedUnowned`,于是每条具名声明恰好落在 +一个计数器里,列表与计数器可以对账。 + +**行为变化(升级须知)**:一条被拒收(无归属包)且被某个 permission set 授权 +的 capability,此前在 `sys_capability` 里**没有任何行**,现在会出现一行 +`managed_by:'platform'` 的派生占位——即它在 Setup 的能力列表里可见、可解析、 +带 humanize 出来的 label。注意这不改变**运行时判定**:权限求值一直是按 +`systemPermissions[]` 里的字符串取并集的,从不查 `sys_capability`;恢复的是 +注册表一侧的 declared = enforced(能力有定义记录、可见、可管理、有 provenance), +不是把一个原本不生效的授权变成生效。若某个部署依赖「那条能力在能力列表里查不 +到」,升级后它会出现。 + +诊断消息同时按 #4632 改进(级别仍为 `warn` —— 功能性降级,非持久性失败): +拒收时点名**授权它的 permission set**,并写明真实后果,例如 +`[security] declared capability "showcase.export_data" has no owning package (granted by showcase_ops): falls back to the back-compat derived placeholder …`。 +无人授权、或已有行的情形各有对应措辞。 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 10dd57bff8..1c0cb00b36 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts @@ -41,7 +41,7 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () = ]); const out = await bootstrapDeclaredCapabilities(ql, null); expect(out.seeded).toBe(1); - expect(out.declaredNames).toEqual(['export_data']); + expect(out.materializedNames).toEqual(['export_data']); const row = ql.rows.find((r) => r.name === 'export_data'); expect(row).toMatchObject({ name: 'export_data', @@ -88,6 +88,7 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () = const ql = makeQl([{ name: 'orphan_cap', label: 'Orphan' }]); const out = await bootstrapDeclaredCapabilities(ql, null); expect(out.seeded).toBe(0); + expect(out.skippedUnowned).toBe(1); expect(ql.rows.find((r) => r.name === 'orphan_cap')).toBeUndefined(); }); @@ -123,11 +124,11 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () = }); it('declared name suppresses the implicit derived placeholder (no clobber)', async () => { - // Full boot order: declared first, then system with declaredNames. + // Full boot order: declared first, then system with materializedNames. const ql = makeQl([{ name: 'export_data', label: 'Export Data', scope: 'org', _packageId: 'com.acme.reports' }]); const cap = await bootstrapDeclaredCapabilities(ql, null); await bootstrapSystemCapabilities(ql, [{ systemPermissions: ['export_data'] }], { - declaredCapabilityNames: cap.declaredNames, + materializedCapabilityNames: cap.materializedNames, }); const row = ql.rows.find((r) => r.name === 'export_data'); // The package row is untouched — no humanized placeholder overwrote it. @@ -138,6 +139,222 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () = it('returns an empty outcome when nothing is declared', async () => { const ql = makeQl([]); const out = await bootstrapDeclaredCapabilities(ql, null); - expect(out).toMatchObject({ seeded: 0, updated: 0, claimed: 0, declaredNames: [] }); + expect(out).toMatchObject({ seeded: 0, updated: 0, claimed: 0, skippedUnowned: 0, materializedNames: [] }); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// [#4967 Part 1] A REFUSED declaration must not suppress the back-compat +// derivation. `materializedNames` reports the names this pass CONFIRMED have a +// row — the three refusal paths land on different sides of that line, for +// different reasons, so each gets its own pin (and, where the direction is not +// obvious, the reverse case that shows what the other answer would cost). +// ─────────────────────────────────────────────────────────────────────────── +describe('refused declarations vs. the derived placeholder (#4967 Part 1)', () => { + const OPS_SETS = [{ name: 'showcase_ops', systemPermissions: ['setup.access', 'showcase.export_data'] }]; + + it('NO OWNING PACKAGE + no row: falls through, so the derivation materializes it', async () => { + // The showcase repro: `showcase.export_data` declared without a resolvable + // owner, granted by `showcase_ops`. + const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]); + const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS }); + + // The declaration is refused — no package row, and (the fix) the name is + // NOT reported as materialized. + expect(out.seeded).toBe(0); + expect(out.skippedUnowned).toBe(1); + expect(out.materializedNames).toEqual([]); + + // Second pass — the capability now exists, as the back-compat placeholder + // it would have had if the declaration had never been written. + await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames }); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + name: 'showcase.export_data', managed_by: 'platform', active: true, + }); + // …and it RESOLVES by name, which is what the granting permission set needs + // from the registry (Setup listing, provenance, ADR-0066 ⑨ lint sources). + expect(await ql.find('sys_capability', { where: { name: 'showcase.export_data' } })).toHaveLength(1); + }); + + it('REVERSE: the pre-#4967 list (every declared name) leaves the capability in no row at all', async () => { + // Same fixture, same second pass — only the skip list is the old one, which + // reported a name the first pass refused to write. Nothing derives it and + // nothing declared it: the hole. + const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]); + await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS }); + await bootstrapSystemCapabilities(ql, OPS_SETS, { + materializedCapabilityNames: ['showcase.export_data'], // ← the old `declaredNames` + }); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toBeUndefined(); + }); + + it('is stable across boots: the refusal re-derives nothing and duplicates nothing', async () => { + const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]); + for (let boot = 0; boot < 2; boot += 1) { + const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS }); + await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames }); + } + expect(ql.rows.filter((r) => r.name === 'showcase.export_data')).toHaveLength(1); + // Boot 2 finds the placeholder, so the refusal now reports the name — the + // row exists and must not be re-derived over. + const out2 = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS }); + expect(out2.skippedUnowned).toBe(1); + expect(out2.materializedNames).toEqual(['showcase.export_data']); + }); + + it('NO OWNING PACKAGE + an admin row: still suppresses, so the placeholder cannot clobber it', async () => { + const ql = makeQl([{ name: 'showcase.export_data', label: 'Declared Label' }]); + ql.rows.push({ id: 'cap_admin', name: 'showcase.export_data', label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin' }); + const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS }); + expect(out.skippedUnowned).toBe(1); + expect(out.materializedNames).toEqual(['showcase.export_data']); + await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames }); + expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({ + label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin', + }); + }); + + 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. + 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'); + }); + + it('FOREIGN owner: suppresses, because the other package authored that row', async () => { + const sets = [{ name: 'ops', systemPermissions: ['shared_cap'] }]; + const ql = makeQl([{ name: 'shared_cap', label: 'Mine', _packageId: 'com.b' }]); + ql.rows.push({ id: 'cap_x', name: 'shared_cap', label: 'Owner Label', description: 'Owner wrote this.', managed_by: 'package', package_id: 'com.a' }); + const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: sets }); + expect(out.skippedForeign).toBe(1); + expect(out.materializedNames).toEqual(['shared_cap']); + await bootstrapSystemCapabilities(ql, sets, { materializedCapabilityNames: out.materializedNames }); + expect(ql.rows.find((r) => r.name === 'shared_cap')).toMatchObject({ + label: 'Owner Label', description: 'Owner wrote this.', package_id: 'com.a', + }); + }); + + it('CURATED platform name: suppresses, and the curated pass seeds the row regardless', async () => { + // A no-op for the skip list (the derived path never reaches a curated name + // — it is already in the curated map), but a truthful answer: the row + // exists after the second pass either way. + const sets = [{ name: 'ops', systemPermissions: ['manage_users'] }]; + const ql = makeQl([{ name: 'manage_users', label: 'Evil', _packageId: 'com.acme.evil' }]); + const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: sets }); + expect(out.skippedPlatform).toBe(1); + expect(out.materializedNames).toEqual(['manage_users']); + await bootstrapSystemCapabilities(ql, sets, { materializedCapabilityNames: out.materializedNames }); + expect(ql.rows.find((r) => r.name === 'manage_users')).toMatchObject({ + label: 'Manage Users', managed_by: 'platform', // curated definition, not the package's + }); + }); + + it('materializedNames reconciles with the outcome counters', async () => { + const ql = makeQl([ + { name: 'a.new', _packageId: 'com.a' }, // → seeded + { name: 'a.own', label: 'Fresh', _packageId: 'com.a' }, // → updated + { name: 'a.derived', _packageId: 'com.a' }, // → claimed + { name: 'a.admin', _packageId: 'com.a' }, // → skippedAdmin + { name: 'a.foreign', _packageId: 'com.a' }, // → skippedForeign + { name: 'manage_users', _packageId: 'com.a' }, // → skippedPlatform + { name: 'a.orphan' }, // → skippedUnowned, NO row + ]); + ql.rows.push({ id: 'c1', name: 'a.own', managed_by: 'package', package_id: 'com.a' }); + ql.rows.push({ id: 'c2', name: 'a.derived', managed_by: 'platform' }); + ql.rows.push({ id: 'c3', name: 'a.admin', managed_by: 'admin' }); + ql.rows.push({ id: 'c4', name: 'a.foreign', managed_by: 'package', package_id: 'com.z' }); + + const out = await bootstrapDeclaredCapabilities(ql, null); + + expect(out).toMatchObject({ + seeded: 1, updated: 1, claimed: 1, + skippedAdmin: 1, skippedForeign: 1, skippedPlatform: 1, skippedUnowned: 1, + }); + // Every named declaration lands in exactly one counter… + const counted = out.seeded + out.updated + out.claimed + + out.skippedAdmin + out.skippedForeign + out.skippedPlatform + out.skippedUnowned; + expect(counted).toBe(7); + // …and `materializedNames` is that set minus the refusal that found no row. + expect(out.materializedNames).toEqual(['a.new', 'a.own', 'a.derived', 'a.admin', 'a.foreign', 'manage_users']); + expect(out.materializedNames).toHaveLength(counted - out.skippedUnowned); + expect(out.materializedNames).not.toContain('a.orphan'); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// [#4967 Part 3] The refusal diagnostic names the GRANTOR permission set(s) +// and the actual consequence. Level stays `warn` per #4632 (functional +// degradation, not a durability failure). +// ─────────────────────────────────────────────────────────────────────────── +describe('unowned-declaration diagnostic (#4967 Part 3)', () => { + function spyLogger() { + const warns: Array<{ msg: string; meta?: Record }> = []; + const errors: string[] = []; + return { + warns, + errors, + logger: { + warn: (msg: string, meta?: Record) => { warns.push({ msg, meta }); }, + error: (msg: string) => { errors.push(msg); }, + }, + }; + } + + it('names every permission set that grants the capability, and stays a warn', async () => { + const { warns, errors, logger } = spyLogger(); + const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]); + await bootstrapDeclaredCapabilities(ql, null, { + logger, + permissionSets: [ + { name: 'showcase_ops', systemPermissions: ['setup.access', 'showcase.export_data'] }, + { name: 'showcase_admin', systemPermissions: ['showcase.export_data'] }, + { name: 'unrelated', systemPermissions: ['setup.access'] }, + ], + }); + const w = warns.find((x) => x.msg.includes('showcase.export_data')); + expect(w).toBeDefined(); + expect(w!.msg).toContain('has no owning package'); + expect(w!.msg).toContain('showcase_ops'); + expect(w!.msg).toContain('showcase_admin'); + expect(w!.msg).not.toContain('unrelated'); + // The consequence, not just the name: it still exists, without provenance. + expect(w!.msg).toContain('derived placeholder'); + expect(w!.meta?.grantedBy).toEqual(['showcase_ops', 'showcase_admin']); + // [#4632] functional degradation → warn, never error. + expect(errors).toEqual([]); + }); + + it('says so plainly when NOTHING grants the capability (it exists nowhere)', async () => { + const { warns, logger } = spyLogger(); + const ql = makeQl([{ name: 'never_granted' }]); + await bootstrapDeclaredCapabilities(ql, null, { logger, permissionSets: [{ name: 'ops', systemPermissions: ['setup.access'] }] }); + const w = warns.find((x) => x.msg.includes('never_granted')); + expect(w!.msg).toContain('granted by no bootstrap permission set'); + expect(w!.msg).toContain('materialized nowhere'); + expect(w!.meta?.grantedBy).toEqual([]); + }); + + it('reports the existing row when one already resolves the name', async () => { + const { warns, logger } = spyLogger(); + const ql = makeQl([{ name: 'showcase.export_data' }]); + ql.rows.push({ id: 'cap_p', name: 'showcase.export_data', managed_by: 'platform' }); + await bootstrapDeclaredCapabilities(ql, null, { + logger, + permissionSets: [{ name: 'showcase_ops', systemPermissions: ['showcase.export_data'] }], + }); + const w = warns.find((x) => x.msg.includes('showcase.export_data')); + expect(w!.msg).toContain('left as-is'); + expect(w!.meta?.grantedBy).toEqual(['showcase_ops']); + }); + + it('falls back to a placeholder label for an unnamed permission set', async () => { + const { warns, logger } = spyLogger(); + const ql = makeQl([{ name: 'orphan_cap' }]); + await bootstrapDeclaredCapabilities(ql, null, { logger, permissionSets: [{ systemPermissions: ['orphan_cap'] }] }); + const w = warns.find((x) => x.msg.includes('orphan_cap')); + expect(w!.meta?.grantedBy).toEqual(['(unnamed permission set)']); }); }); diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts index b76e1afd9f..c8fd73fe32 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.ts @@ -34,9 +34,21 @@ * - admin-authored rows (`managed_by:'admin'`) are NEVER clobbered. * * Runs on `kernel:ready` in `@objectstack/plugin-security` alongside the other - * declared-metadata seeders. The set of declared names is returned so the - * caller can tell `bootstrapSystemCapabilities` to SKIP re-deriving (and thus - * clobbering) an explicitly-declared capability. + * declared-metadata seeders. {@link CapabilitySeedOutcome.materializedNames} is + * returned so the caller can tell `bootstrapSystemCapabilities` to SKIP + * re-deriving (and thus clobbering) a capability that already has a row. + * + * [#4967 Part 1] That list reports names this pass CONFIRMED have a row — NOT + * every name it read. The two are different facts, and conflating them turned a + * REFUSED declaration into a hole: the refusal writes no row, the reported name + * suppressed the back-compat derivation too, and the capability then existed in + * no row at all, leaving every `systemPermissions` grant naming it inert. Adding + * an explicit declaration was therefore strictly WORSE than omitting one. The + * suppression rule is now uniform and stated per refusal path in + * {@link upsertPackageCapability}: a name suppresses the derivation IFF + * `sys_capability` holds a row for it after this pass — protecting an authored + * row from a humanized placeholder is the whole purpose of the list, and a + * refusal with no row is not an authored row there is anything to protect. */ import { @@ -49,8 +61,21 @@ import { import { readDeclared } from './bootstrap-declared-permissions.js'; import { PLATFORM_CAPABILITY_NAMES } from '@objectstack/spec/security'; +/** The only shape this seeder reads off a permission set: who grants what. */ +type GrantingPermissionSet = { name?: string; systemPermissions?: readonly string[] }; + interface SeedOptions { logger?: ProjectionLogger; + /** + * [#4967 Part 3] The bootstrap permission sets that GRANT capabilities via + * `systemPermissions[]` — the SAME array `bootstrapSystemCapabilities` derives + * its placeholders from, so "granted by X" and "will be derived" are read off + * one source. Used only to name the grantor(s) in the refusal diagnostics: a + * seeder-side warn that names the capability but not the permission set(s) + * that grant it does not tell the reader the consequence. Threaded as an + * argument on every call — never module-level state. + */ + permissionSets?: readonly GrantingPermissionSet[]; } /** Aggregated outcome of a declared-capability seeding pass. */ @@ -61,8 +86,30 @@ export interface CapabilitySeedOutcome { skippedAdmin: number; skippedForeign: number; skippedPlatform: number; - /** Names of every capability EXPLICITLY declared (regardless of seed outcome). */ - declaredNames: string[]; + /** + * [#4967 Part 1] Declarations refused for want of an owning package + * (`_packageId`/`packageId` both absent) — previously the one refusal path + * with no counter at all, which is why `declaredNames` and the counters could + * not be reconciled. + */ + skippedUnowned: number; + /** + * [#4967 Part 1] Names this pass CONFIRMED are materialized in + * `sys_capability`, i.e. the names `bootstrapSystemCapabilities` must NOT + * re-derive a placeholder for. A name lands here when this pass wrote its row + * (seeded / updated / claimed) or found a row it must not clobber + * (admin-authored, another package's, or a curated platform name the curated + * pass owns) — and NOT when a refusal left the name with no row anywhere. + * + * Accounting: every named declaration falls into exactly one counter, so + * `seeded + updated + claimed + skippedAdmin + skippedForeign + + * skippedPlatform + skippedUnowned` is the number of named declarations read, + * and `materializedNames` is that same set minus the refusals that found no + * row. (Pre-existing caveat, unchanged: a write the engine rejects increments + * no counter — and a rejected INSERT deliberately keeps its name out of this + * list, so the derivation gets its own attempt.) + */ + materializedNames: string[]; } function humanize(name: string): string { @@ -78,38 +125,101 @@ function capabilityRowFields(cap: any): { label: string; description: string; sc }; } +/** + * [#4967 Part 3] Index `capability name → granting permission set name(s)` over + * the bootstrap permission sets, so a refusal can name the grantor(s) — and, + * because this is the same array the back-compat derivation reads, can state + * the ACTUAL consequence rather than a generic "not materialized". + */ +function indexGrantors(sets: readonly GrantingPermissionSet[] = []): Map { + const byCapability = new Map(); + for (const ps of sets) { + const setName = typeof ps?.name === 'string' && ps.name ? ps.name : '(unnamed permission set)'; + for (const cap of ps?.systemPermissions ?? []) { + if (typeof cap !== 'string' || !cap) continue; + const grantors = byCapability.get(cap); + if (!grantors) byCapability.set(cap, [setName]); + else if (!grantors.includes(setName)) grantors.push(setName); + } + } + return byCapability; +} + +/** + * [#4967 Part 3] The unowned-declaration diagnostic. Stays at `warn` per the + * #4632 rule — this is FUNCTIONAL degradation (a declaration that does not + * carry its provenance), not a durability failure. What changes is the payload: + * it names the permission set(s) that GRANT the capability, and the real + * consequence of the refusal, which differs by whether a row already exists and + * whether anything grants the name at all. + */ +function unownedRefusalMessage(name: string, grantors: readonly string[], hasRow: boolean): string { + const granted = grantors.length > 0 + ? `granted by ${grantors.join(', ')}` + : 'granted by no bootstrap permission set'; + const consequence = hasRow + ? 'an existing sys_capability row already resolves it and is left as-is — the declaration adds no package provenance' + : grantors.length > 0 + ? 'falls back to the back-compat derived placeholder — the grant resolves, but with no package provenance (ADR-0086 D3: uninstall undefined)' + : 'nothing derives it either — the capability is materialized nowhere'; + return `[security] declared capability "${name}" has no owning package (${granted}): ${consequence}`; +} + /** * Upsert ONE declared capability into `sys_capability` under the owning * `packageId`, applying the ADR-0066 D1 provenance rules (own-row re-seed, * derived-platform-row claim, curated/foreign/admin refuse-or-skip). + * + * Returns whether the name is MATERIALIZED — i.e. whether `sys_capability` + * holds a row for it after this call, and so whether the caller must suppress + * the back-compat derived placeholder for it (#4967 Part 1). The three refusal + * paths differ, and the difference is the whole bug: + * + * - CURATED platform name → `true`. The curated pass of + * `bootstrapSystemCapabilities` seeds every curated name unconditionally, so + * the row exists regardless of this refusal; the derived path cannot reach a + * curated name in the first place (it is already in the curated map), which + * makes the answer a no-op here — but a truthful one. + * - FOREIGN owner (and admin-authored rows) → `true`. A row exists and its + * label/description are AUTHORED; re-deriving would overwrite them with a + * humanized placeholder, which is exactly what the suppression list is for. + * - NO OWNING PACKAGE → `true` only if a row already exists. With no row this + * is the #4967 hole: suppressing the derivation left the capability existing + * nowhere, so it falls through and the placeholder is derived as it would + * have been had the declaration never been written. */ async function upsertPackageCapability( ql: any, cap: any, packageId: string | null | undefined, out: CapabilitySeedOutcome, + grantors: readonly string[], logger?: ProjectionLogger, -): Promise { - if (!cap?.name) return; +): Promise { + if (!cap?.name) return false; // Curated platform capabilities are platform-owned — a package must never // claim one (that would let it silently redefine `manage_users`, `setup.access`, …). if (PLATFORM_CAPABILITY_NAMES.has(cap.name)) { out.skippedPlatform += 1; logger?.warn?.('[security] capability name is a curated platform capability — not materialized as package', { name: cap.name }); - return; + return true; } + const fields = capabilityRowFields(cap); + const existing = (await tryFind(ql, 'sys_capability', { name: cap.name }, 1))[0]; + // A `managed_by:'package'` row without a `package_id` makes uninstall // undefined (the ambiguity ADR-0086 D3 removes) — skip an unowned declaration. if (!packageId) { - logger?.warn?.('[security] capability has no owning package — not materialized', { name: cap.name }); - return; + out.skippedUnowned += 1; + logger?.warn?.(unownedRefusalMessage(cap.name, grantors, Boolean(existing?.id)), { + name: cap.name, + grantedBy: [...grantors], + }); + return Boolean(existing?.id); } - const fields = capabilityRowFields(cap); - const existing = (await tryFind(ql, 'sys_capability', { name: cap.name }, 1))[0]; - if (!existing?.id) { const created = await tryInsert(ql, 'sys_capability', { id: genId('cap'), @@ -120,7 +230,9 @@ async function upsertPackageCapability( active: true, }); if (created) out.seeded += 1; - return; + // A rejected insert leaves no row — fall through so the derivation gets its + // own attempt rather than the name landing in a hole. + return Boolean(created); } if (existing.managed_by === 'package') { @@ -133,7 +245,8 @@ async function upsertPackageCapability( name: cap.name, declaredBy: packageId, ownedBy: existing.package_id, }); } - return; + // Either way a package-authored row exists and must not be re-derived over. + return true; } if (existing.managed_by === 'platform') { @@ -143,11 +256,12 @@ async function upsertPackageCapability( if (await tryUpdate(ql, 'sys_capability', { id: existing.id, ...fields, managed_by: 'package', package_id: packageId })) { out.claimed += 1; } - return; + return true; } // `admin` (or any other) — environment/admin-authored. Never clobbered. out.skippedAdmin += 1; + return true; } export async function bootstrapDeclaredCapabilities( @@ -156,7 +270,9 @@ export async function bootstrapDeclaredCapabilities( options: SeedOptions = {}, ): Promise { const out: CapabilitySeedOutcome = { - seeded: 0, updated: 0, claimed: 0, skippedAdmin: 0, skippedForeign: 0, skippedPlatform: 0, declaredNames: [], + seeded: 0, updated: 0, claimed: 0, + skippedAdmin: 0, skippedForeign: 0, skippedPlatform: 0, skippedUnowned: 0, + materializedNames: [], }; if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') return out; @@ -169,13 +285,19 @@ export async function bootstrapDeclaredCapabilities( } if (!Array.isArray(caps) || caps.length === 0) return out; + const grantorsByCapability = indexGrantors(options.permissionSets); + for (const cap of caps) { if (!cap?.name) continue; - out.declaredNames.push(cap.name); // Registry provenance first (ADR-0010 `_packageId`), author-declared // spec `packageId` (ADR-0086 D3) as fallback. const packageId: string | undefined = cap._packageId ?? cap.packageId ?? undefined; - await upsertPackageCapability(ql, cap, packageId, out, options.logger); + const grantors = grantorsByCapability.get(cap.name) ?? []; + const materialized = await upsertPackageCapability(ql, cap, packageId, out, grantors, options.logger); + // [#4967 Part 1] Report the name ONLY once this pass knows a row exists for + // it. Reporting it before the upsert decided anything is what let a refused + // declaration suppress the derivation it needed. + if (materialized) out.materializedNames.push(cap.name); } options.logger?.info?.('[security] declared capabilities seeded into sys_capability (ADR-0066 D1)', { 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 3d701a7445..d546ccff41 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts @@ -54,10 +54,10 @@ describe('bootstrapSystemCapabilities (ADR-0066 D1 back-compat seed)', () => { expect(ql.rows.find((x) => x.name === 'manage_org_users')?.scope).toBe('org'); }); - it('does NOT derive a placeholder for an explicitly-declared capability', async () => { + it('does NOT derive a placeholder for an already-materialized capability', async () => { const ql = makeQl(); await bootstrapSystemCapabilities(ql, [{ systemPermissions: ['export_data', 'approve_invoice'] }], { - declaredCapabilityNames: ['export_data'], + materializedCapabilityNames: ['export_data'], }); // `export_data` is owned by the declared seeder — no platform placeholder. expect(ql.rows.find((x: any) => x.name === 'export_data')).toBeUndefined(); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts index 56c375aac6..4574795f91 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts @@ -62,13 +62,20 @@ function humanize(name: string): string { interface SeedOptions { logger?: { info?: (m: string, meta?: Record) => void; warn?: (m: string, meta?: Record) => void }; /** - * [ADR-0066 D1] Capability names that a package has EXPLICITLY declared via - * `defineCapability` (materialized by `bootstrapDeclaredCapabilities`). The - * implicit derived-defaults path SKIPS these so it never overwrites an - * authored capability's label/description/scope (or its package provenance) - * with a humanized placeholder. Curated platform capabilities are unaffected. + * [ADR-0066 D1] Capability names that `bootstrapDeclaredCapabilities` has + * confirmed ALREADY HAVE a `sys_capability` row + * ({@link CapabilitySeedOutcome.materializedNames}). The implicit + * derived-defaults path SKIPS these so it never overwrites an authored + * capability's label/description (or its package provenance) with a humanized + * placeholder. Curated platform capabilities are unaffected. + * + * [#4967 Part 1] "Materialized", NOT "declared": a declaration the seeder + * REFUSED (no owning package) writes no row, so suppressing the derivation + * for it left the capability existing in no row at all and every + * `systemPermissions` grant naming it inert. Such a name is deliberately + * absent from this list and derives its placeholder as it always did. */ - declaredCapabilityNames?: Iterable; + materializedCapabilityNames?: Iterable; } export async function bootstrapSystemCapabilities( @@ -80,16 +87,16 @@ export async function bootstrapSystemCapabilities( return { seeded: 0, updated: 0, total: 0 }; } - const declared = new Set(options.declaredCapabilityNames ?? []); + const materialized = new Set(options.materializedCapabilityNames ?? []); // Build the full definition set: curated first, then any extra capability // string referenced by the seeded permission sets (derived defaults) — EXCEPT - // ones a package explicitly declared, which the declared seeder owns. + // 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); for (const ps of permissionSets) { for (const cap of ps?.systemPermissions ?? []) { - if (typeof cap === 'string' && cap && !byName.has(cap) && !declared.has(cap)) { + if (typeof cap === 'string' && cap && !byName.has(cap) && !materialized.has(cap)) { byName.set(cap, { name: cap, label: humanize(cap), description: `Capability ${cap}.`, scope: 'platform' }); } } diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 48be2e6327..b945dd0039 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1997,19 +1997,29 @@ export class SecurityPlugin implements Plugin { // `stack.capabilities`) land with `managed_by:'package'` + package_id // provenance — the formal replacement for the implicit derive-from- // systemPermissions back-door. THEN the platform curated set + the - // back-compat derived defaults, SKIPPING any name a package already - // declared (so the placeholder never clobbers the authored capability). - let declaredCapabilityNames: string[] = []; + // back-compat derived defaults, SKIPPING any name that already HAS a row + // (so the placeholder never clobbers the authored capability). + // [#4967 Part 1] The skip list is the names the first pass confirmed are + // materialized — not every name it read. A declaration the first pass + // REFUSES (no owning package) writes no row, so skipping its derivation + // too left the capability existing nowhere and every grant naming it + // inert; it now falls through to the placeholder. The permission sets are + // passed to the first pass as well, so a refusal can name the grantor(s) + // it affects (#4967 Part 3). + let materializedCapabilityNames: string[] = []; try { - const capOutcome = await bootstrapDeclaredCapabilities(ql, this.metadata, { logger: ctx.logger }); - declaredCapabilityNames = capOutcome.declaredNames; + const capOutcome = await bootstrapDeclaredCapabilities(ql, this.metadata, { + logger: ctx.logger, + permissionSets: this.bootstrapPermissionSets, + }); + materializedCapabilityNames = capOutcome.materializedNames; } catch (e) { ctx.logger.warn('[security] declared-capability seeding failed', { error: (e as Error).message }); } try { await bootstrapSystemCapabilities(ql, this.bootstrapPermissionSets, { logger: ctx.logger, - declaredCapabilityNames, + materializedCapabilityNames, }); } catch (e) { ctx.logger.warn('[security] capability seeding failed', { error: (e as Error).message });