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 });