diff --git a/.changeset/17516-permission-set-collision-diagnostic.md b/.changeset/17516-permission-set-collision-diagnostic.md new file mode 100644 index 0000000000..debf72fa9c --- /dev/null +++ b/.changeset/17516-permission-set-collision-diagnostic.md @@ -0,0 +1,29 @@ +--- +"@objectstack/plugin-security": minor +--- + +A **permission-set name collision now reaches the author**. When a package declares a permission set whose name a *different* package already owns, `bootstrapDeclaredPermissions` refuses to write into that row — correct under ADR-0086 D4, and unchanged — but the refusal is no longer invisible (#17516). + +Measured on the pre-change tree, with a collision seeded and **no logger passed**: + +``` +skippedForeign = 1 (the entire declared set was dropped) +author-visible console lines = 0 (log, info, warn, error, debug — all five) +diagnostic records on outcome = undefined +``` + +The branch reported through `logger?.warn?.(…)` — optionally chained **twice** — so a caller that passed no logger produced no output at all, and a package's whole declared permission set vanished with one internal counter incremented. The comment there said *"refuse loudly"*; nothing about it was loud. Same case after the change: + +``` +skippedForeign = 1 (unchanged — the skip is not what was wrong) +author-visible console lines = 1 warn: [security] [permission_set_name_collision] … +diagnostic records on outcome = 1 { name, declaredBy, ownedBy, message, fix } +``` + +- **It prints with no sink injected.** `reportPermissionSetNameCollisions` falls back to `console.warn`, per the #10556 ruling that silent-by-declaration is rejected — an injected host sink still replaces it rather than printing beside it. The call keeps the receiver (a property-access call, never a detached `logger.warn ?? console.warn`), so a class-based host sink does not throw. +- **The refusal is also readable without a log.** `PermissionSeedOutcome` gains an optional `collisions` array carrying one diagnostic per dropped set — absent, never `[]`, when the pass hit none. A counter with no record is what made the drop undiagnosable. +- **One derivation, so two doors cannot drift.** `permissionSetNameIsForeign`, `permissionSetNameCollisionDiagnostic` and `formatPermissionSetNameCollisionDiagnostic` are exported from the package entry so a compile-time door consumes them rather than re-deriving the predicate or re-spelling the wording — the shape #14553 established for `navigationContributions`. ⚠️ Only the **runtime** door ships here; the compile-time door (`os build` / `os validate`) lives in another package and is not part of this change. +- **A stable, greppable token**, `permission_set_name_collision`, is stamped as `event` on every report. It is a snake_case data value, not an ADR-0112 error code: it is never routed to `error.code` and never reaches a wire refusal, the same discrimination the sibling `position_name_fold_grant` token already makes in this package. +- **The branch comment's premise is corrected.** It claimed package-namespaced object api names make set-name collisions a packaging bug rather than a merge case. **ADR-0130 D1 falsifies that** — N packages may co-own one namespace — so a collision is a legal configuration that gets *more* common, not an error that should never happen. The diagnostic's `fix` text names both legal resolutions. + +⛔ **No wire byte moves and no skip changes.** The foreign row is still never written; `skippedForeign` still counts it; the ADR-0086 P2 publish materializer still returns its existing `permission set name is owned by another package` failure text. A non-colliding pass stays completely silent on all five console channels, asserted over a pass that really does seed and re-seed. diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts index 2cfa72f197..97bd99c6f9 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts @@ -105,7 +105,16 @@ describe('bootstrapDeclaredPermissions (ADR-0086 D5)', () => { }); expect(r.skippedForeign).toBe(1); expect(ql.rows[0].package_id).toBe('com.example.crm'); - expect(warns.some((w) => String(w.m).includes('owned by another package'))).toBe(true); + // [#17516] Re-anchored from the old prose ('owned by another package') to + // the stable token the report now stamps. The substance this pin asserts is + // unchanged — the refusal is reported — but the token is what an operator + // greps and what the sibling doors key on, so prose drift can no longer + // quietly unpin it. The read-back half is asserted beside it: a counter + // with no record is what made this drop invisible. + expect(warns.some((w) => String(w.m).includes('permission_set_name_collision'))).toBe(true); + expect(r.collisions).toEqual([ + expect.objectContaining({ name: 'crm_sales_rep', declaredBy: 'com.example.other', ownedBy: 'com.example.crm' }), + ]); }); it('skips a declared set with no resolvable owning package (warned, not seeded)', async () => { diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts index b83f836070..c11b5d82fb 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts @@ -68,6 +68,12 @@ import { reportSeedWriteRefusals, type SeedWriteRefusals, } from './per-organization-catalog.js'; +import { + permissionSetNameCollisionDiagnostic, + permissionSetNameIsForeign, + reportPermissionSetNameCollisions, + type PermissionSetNameCollisionDiagnostic, +} from './permission-set-name-collision.js'; export type { PermissionSeedOutcome } from './permission-set-projection.js'; @@ -206,6 +212,14 @@ export async function upsertPackagePermissionSet( * materialized nothing). */ refusals?: SeedWriteRefusals; + /** + * [#17516] Collects set-name collisions so the pass reports them ONCE + * instead of a line per dropped set. Passed by the boot catalog loop; the + * ADR-0086 P2 publish materializer passes nothing and the refusal is + * reported at the branch instead — ⛔ never dropped, which is the whole + * point of this card. + */ + collisions?: PermissionSetNameCollisionDiagnostic[]; }, ): Promise { const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 }; @@ -260,7 +274,7 @@ export async function upsertPackagePermissionSet( } if (existing.managed_by === 'package') { - if (existing.package_id === packageId) { + if (!permissionSetNameIsForeign(existing.package_id, packageId)) { // Our own row — re-seed so the record always reflects the shipped/published // declaration (idempotent; covers version bumps without bookkeeping). // @@ -283,13 +297,40 @@ export async function upsertPackagePermissionSet( out.updated += 1; } } else { - // Package-namespaced object api names make set-name collisions a - // packaging bug, not a merge case — refuse loudly (ADR-0086 D4: - // a package never writes into a foreign record). + // [#17516] The SKIP is unchanged and correct — ADR-0086 D4: a package + // never writes into a foreign record. What changed is that it is no + // longer invisible. + // + // ⚠️ The premise this branch used to state — "Package-namespaced object + // api names make set-name collisions a packaging bug, not a merge case" + // — is FALSIFIED by ADR-0130 D1, which lets N packages co-own one + // namespace (the ADR records it under "What was NOT decided"). A + // collision is therefore a legal configuration that gets MORE common as + // co-ownership lands, not a packaging error that should never happen. So + // the author reading it is the normal case, not the pathological one. + // + // ⛔ And the old line did not refuse loudly, whatever it claimed: + // `logger?.warn?.(…)` is optionally chained TWICE, so a caller passing no + // logger produced NO OUTPUT AT ALL and an entire declared permission set + // disappeared with one counter moved. The report now goes through + // `reportPermissionSetNameCollisions`, which prints with no sink injected + // (#10556: silent-by-declaration is rejected), and the diagnostic RECORD + // travels back on the outcome so a caller that reads no log at all — a + // boot report, a test — can still ask what happened. out.skippedForeign += 1; - logger?.warn?.('[security] permission set name owned by another package — skipped', { - name: ps.name, declaredBy: packageId, ownedBy: existing.package_id, + const diagnostic = permissionSetNameCollisionDiagnostic({ + name: String(ps.name), + declaredBy: packageId, + ownedBy: typeof existing.package_id === 'string' ? existing.package_id : null, + ...(organizationId ? { organizationId } : {}), }); + out.collisions = [diagnostic]; + // The boot loop collects and reports ONCE per pass. The ADR-0086 P2 + // publish materializer upserts a single set and passes no collector, so + // it reports here — it has no pass to summarise, and inheriting the old + // silence is the one outcome this card forbids. + if (opts?.collisions) opts.collisions.push(diagnostic); + else reportPermissionSetNameCollisions(logger, [diagnostic], organizationId); } return out; } @@ -344,13 +385,15 @@ export async function bootstrapDeclaredPermissions( // One log per pass, not per refused row: a legacy platform-wide unique index // refuses EVERY declared permission set, and a line each would bury the remedy. const refusals = createSeedWriteRefusals(); + // [#17516] Declared sets dropped because another package owns the name. + const collisions: PermissionSetNameCollisionDiagnostic[] = []; for (const ps of sets) { if (!ps?.name) continue; // Registry provenance first (ADR-0010 `_packageId`), author-declared // spec `packageId` (ADR-0086 D3) as fallback. const packageId: string | undefined = ps._packageId ?? ps.packageId ?? undefined; - const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName, organizationId, residue, refusals }); + const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName, organizationId, residue, refusals, collisions }); out.seeded += r.seeded; out.updated += r.updated; out.unchanged += r.unchanged; @@ -370,6 +413,11 @@ export async function bootstrapDeclaredPermissions( } // Before the counts, so an operator reads WHY the count is zero beside it. reportSeedWriteRefusals(options.logger, refusals, organizationId); + // [#17516] Said once per pass, and said even when no logger was injected — + // the whole defect was that this refusal reached nobody. The records go back + // on the outcome too, for a caller that reads no log at all. + reportPermissionSetNameCollisions(options.logger, collisions, organizationId); + if (collisions.length > 0) out.collisions = collisions; if (out.unreadable > 0) { // Said once, with the count: these sets were neither seeded nor reconciled // because the record could not be READ. Silence here would read exactly diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts index a3c42c4132..74802f50d3 100644 --- a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts @@ -431,7 +431,14 @@ describe('#10946 — a name declared twice in one batch keeps its loud refusal', expect(r.skippedForeign).toBe(1); expect(ql.rows).toHaveLength(1); expect(ql.rows[0].package_id).toBe('com.example.a'); - expect(warns.some((w) => w.includes('owned by another package'))).toBe(true); + // [#17516] Re-anchored from the old prose to the stable token the report + // stamps — the assertion's substance (the refusal is REPORTED, not merely + // counted) is unchanged, and the record is asserted beside it so "loud" + // means reaching a reader rather than moving a counter. + expect(warns.some((w) => w.includes('permission_set_name_collision'))).toBe(true); + expect(r.collisions).toEqual([ + expect.objectContaining({ name: 'shared_name', declaredBy: 'com.example.b', ownedBy: 'com.example.a' }), + ]); }); }); diff --git a/packages/plugins/plugin-security/src/index.ts b/packages/plugins/plugin-security/src/index.ts index a80726af87..228fe0e20e 100644 --- a/packages/plugins/plugin-security/src/index.ts +++ b/packages/plugins/plugin-security/src/index.ts @@ -55,6 +55,23 @@ export type { InvitationPlacementService, } from './invitation-placement.js'; export { bootstrapDeclaredPermissions } from './bootstrap-declared-permissions.js'; +// [#17516] The set-name collision diagnostic. EXPORTED because its whole +// purpose is to be the ONE derivation every door shares: the runtime door below +// raises it today, and the compile-time door (`os build` / `os validate`, which +// lives in another package) must consume these rather than re-deriving either +// the predicate or the wording — that drift is what this card is about, one +// layer up. +export { + PERMISSION_SET_NAME_COLLISION, + formatPermissionSetNameCollisionDiagnostic, + permissionSetNameCollisionDiagnostic, + permissionSetNameIsForeign, + reportPermissionSetNameCollisions, +} from './permission-set-name-collision.js'; +export type { + CollisionReportSink, + PermissionSetNameCollisionDiagnostic, +} from './permission-set-name-collision.js'; // [ADR-0094] sys_permission_set pure-projection machinery. export { permissionSetRowFields, diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts new file mode 100644 index 0000000000..7e73aa0b7e --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts @@ -0,0 +1,333 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17516] The set-name collision refusal must REACH THE AUTHOR. + * + * ## What these pins are about, and what they deliberately do not touch + * + * ⛔ Not the skip. Refusing to write into a `sys_permission_set` row another + * package owns is correct under ADR-0086 D4 and is asserted UNCHANGED in every + * case below (`skippedForeign` still counts, the foreign row is never mutated). + * The defect was that the refusal was invisible: `logger?.warn?.(…)` at that + * branch is optionally chained TWICE, so a caller passing no logger produced no + * output at all and an entire declared permission set disappeared with one + * internal counter moved. + * + * ## The measurement these replace + * + * On the pre-fix tree the first case below measured ZERO console lines across + * every channel, and `undefined` for the outcome's diagnostic records, while + * `skippedForeign` was 1. That is the card's reading, reproduced. + * + * ## The discriminating half + * + * A pin that only asserts "something was printed on a collision" passes just as + * well against a seeder that prints on EVERY seeded set, which would be a + * different defect (#12015's: a diagnostic that fires always is as unreadable as + * one that never fires). So the no-collision control asserts SILENCE on all + * five console channels, over a pass that really does seed and really does + * re-seed its own row. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; + +import { + bootstrapDeclaredPermissions, + upsertPackagePermissionSet, +} from './bootstrap-declared-permissions.js'; +import { + PERMISSION_SET_NAME_COLLISION, + formatPermissionSetNameCollisionDiagnostic, + permissionSetNameCollisionDiagnostic, + permissionSetNameIsForeign, + reportPermissionSetNameCollisions, +} from './permission-set-name-collision.js'; + +/** Minimal in-memory ql + registry for sys_permission_set seeding. */ +function makeQl(declared: any[] = []) { + const rows: any[] = []; + return { + rows, + registry: { listItems: (type: string) => (type === 'permission' ? declared : []) }, + async find(object: string, q: any) { + if (object !== 'sys_permission_set') return []; + const where = q?.where ?? {}; + const hits = rows.filter((r) => Object.entries(where).every(([k, v]) => { + // ⛔ REFUSE what this double does not implement. A `$or` / `$and` read + // as a FIELD NAME is the silently-wrong shape: `r.$or` is `undefined`, + // no row matches, and a suite asserts on an empty result set with + // nothing erroring (`check:where-matcher`, shape (b)). The sibling + // double in `objects/reserved-identity-names.test.ts` refuses the same way. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported combinator ${k}`); + if (v && typeof v === 'object' && !Array.isArray(v)) { + const inList = (v as any).$in; + if (Array.isArray(inList)) return inList.includes(r[k]); + throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`); + } + return r[k] === v; + })); + // Hold the caller's BOUND, after the filter and by PRESENCE, so `limit: 0` + // returns nothing rather than everything. `defaultLookup` really does read + // `{ where: { name }, limit: organizationId ? 5 : 1 }` (#10103), and a + // limit-blind double cannot tell that read from an unbounded one + // (`check:objectql-double-limit`). + return typeof q?.limit === 'number' ? hits.slice(0, q.limit) : hits; + }, + async insert(object: string, data: any) { + if (object !== 'sys_permission_set') return null; + rows.push({ ...data }); + return { id: data.id }; + }, + // `assertEngineUpdateDispatch` rather than a hand-rolled id check: a fake + // looser than `ObjectQL.update` is how a dead route once shipped with its + // suite green (`check:engine-double-contract`). + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + if (object !== 'sys_permission_set') return; + if (dispatch.kind !== 'by-id') throw new Error('fake driver: only by-id update is modelled'); + const r = rows.find((x) => x.id === dispatch.id); + if (r) Object.assign(r, data); + }, + }; +} + +const declaredSet = (over: Record = {}) => ({ + name: 'crm_sales_rep', + label: 'Sales Rep', + objects: { crm_lead: { allowRead: true } }, + _packageId: 'com.example.b', + ...over, +}); + +/** + * Capture EVERY console channel. "Author-visible output" is not channel-specific + * — the pre-fix failure was that NONE of them carried anything — so a pin that + * watched only `warn` could be satisfied by a change that moved the silence. + */ +function captureConsole() { + const seen: string[] = []; + const spies = (['log', 'info', 'warn', 'error', 'debug'] as const).map((m) => + vi.spyOn(console, m).mockImplementation((...args: unknown[]) => { + seen.push(`${m}: ${args.map((a) => (typeof a === 'string' ? a : JSON.stringify(a))).join(' ')}`); + }), + ); + return { seen, restore: () => spies.forEach((s) => s.mockRestore()) }; +} + +describe('[#17516] set-name collision reaches the author', () => { + it('prints, WITH NO LOGGER INJECTED, and returns the diagnostic record', async () => { + const ql = makeQl([declaredSet()]); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + // No logger — the exact call shape under which the old branch was mute. + out = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + cap.restore(); + } + + // ⛔ The refusal itself is UNCHANGED. + expect(out.skippedForeign).toBe(1); + expect(out.seeded).toBe(0); + expect(out.updated).toBe(0); + expect(ql.rows).toHaveLength(1); + expect(ql.rows[0].package_id).toBe('com.example.a'); + expect(ql.rows[0].object_permissions).toBe('{}'); + + // The half that was missing: the author is told, through the console, + // because no sink was injected (#10556 — silent-by-declaration rejected). + expect(cap.seen).toHaveLength(1); + expect(cap.seen[0]).toContain(PERMISSION_SET_NAME_COLLISION); + expect(cap.seen[0]).toMatch(/^warn:/); + expect(cap.seen[0]).toContain('NOT materialized'); + + // And the RECORD, for a caller that reads no log at all. + expect(out.collisions).toHaveLength(1); + const d = out.collisions![0]!; + expect(d.event).toBe(PERMISSION_SET_NAME_COLLISION); + expect(d.severity).toBe('warning'); + expect(d.name).toBe('crm_sales_rep'); + expect(d.declaredBy).toBe('com.example.b'); + expect(d.ownedBy).toBe('com.example.a'); + // The remedy names both legal resolutions, including the ADR-0130 D1 one + // the old comment's premise denied could exist. + expect(d.fix).toContain('ADR-0130 D1'); + }); + + it('DISCRIMINATING CONTROL: a pass with no collision says nothing at all', async () => { + // Two real writes happen here — a first seed and an own-row re-seed — so + // the silence is over a pass that is doing work, not an empty one. + const ql = makeQl([declaredSet({ _packageId: 'com.example.a' })]); + + const first = captureConsole(); + let r1: Awaited>; + try { + r1 = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + first.restore(); + } + expect(r1.seeded).toBe(1); + expect(r1.skippedForeign).toBe(0); + expect(r1.collisions).toBeUndefined(); + expect(first.seen).toEqual([]); + + // Re-seed the SAME package's own row after a declaration change. + (ql as any).registry = { + listItems: () => [declaredSet({ _packageId: 'com.example.a', objects: { crm_lead: { allowRead: true, allowCreate: true } } })], + }; + const second = captureConsole(); + let r2: Awaited>; + try { + r2 = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + second.restore(); + } + expect(r2.updated).toBe(1); + expect(r2.skippedForeign).toBe(0); + expect(r2.collisions).toBeUndefined(); + expect(second.seen).toEqual([]); + }); + + it('reports through an INJECTED sink instead of the console when one is given', async () => { + const ql = makeQl([declaredSet()]); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const warned: Array<{ m: string; meta?: Record }> = []; + const cap = captureConsole(); + try { + await bootstrapDeclaredPermissions(ql, undefined, { + logger: { warn: (m, meta) => { warned.push({ m, meta }); } }, + }); + } finally { + cap.restore(); + } + + const collision = warned.filter((w) => w.m.includes(PERMISSION_SET_NAME_COLLISION)); + expect(collision).toHaveLength(1); + expect(collision[0]!.meta?.collisions).toEqual([ + expect.objectContaining({ name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a' }), + ]); + // The host sink REPLACES the console default — not both. + expect(cap.seen).toEqual([]); + }); + + it('the ADR-0086 P2 single-set path reports too — it has no pass to summarise', async () => { + const ql = makeQl(); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + // No logger, no collector — the publish materializer's exact call shape. + out = await upsertPackagePermissionSet(ql, declaredSet(), 'com.example.b'); + } finally { + cap.restore(); + } + + expect(out.skippedForeign).toBe(1); + expect(out.collisions).toHaveLength(1); + expect(cap.seen).toHaveLength(1); + expect(cap.seen[0]).toContain(PERMISSION_SET_NAME_COLLISION); + }); + + it('a package-managed row with NO package_id is foreign, not adoptable', async () => { + const ql = makeQl([declaredSet()]); + // ADR-0086 D3's exact ambiguity: managed_by 'package', owner unprovable. + ql.rows.push({ + id: 'ps_unowned', + name: 'crm_sales_rep', + managed_by: 'package', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + out = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + cap.restore(); + } + + expect(out.skippedForeign).toBe(1); + expect(out.updated).toBe(0); + expect(ql.rows[0].object_permissions).toBe('{}'); + expect(out.collisions![0]!.ownedBy).toBeNull(); + expect(cap.seen).toHaveLength(1); + }); +}); + +describe('[#17516] the shared derivation both doors must use', () => { + it('permissionSetNameIsForeign: same owner is ours, anything else is foreign', () => { + expect(permissionSetNameIsForeign('com.example.a', 'com.example.a')).toBe(false); + expect(permissionSetNameIsForeign('com.example.a', 'com.example.b')).toBe(true); + // A nullish owner is FOREIGN — never silently adopted on a name match. + expect(permissionSetNameIsForeign(undefined, 'com.example.b')).toBe(true); + expect(permissionSetNameIsForeign(null, 'com.example.b')).toBe(true); + // Both unowned is not a collision between two different packages. + expect(permissionSetNameIsForeign(null, undefined)).toBe(false); + }); + + /** + * The literal is asserted rather than only imported — the precedent the + * sibling `position_name_fold_grant` pin sets. Importing it on both sides + * would let a rename pass green while every operator's grep went dead. + */ + it('stamps a stable, greppable token', () => { + expect(PERMISSION_SET_NAME_COLLISION).toBe('permission_set_name_collision'); + const d = permissionSetNameCollisionDiagnostic({ + name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a', + }); + expect(formatPermissionSetNameCollisionDiagnostic(d)) + .toContain('[security] [permission_set_name_collision]'); + }); + + it('reportPermissionSetNameCollisions says nothing when there is nothing to say', () => { + const cap = captureConsole(); + try { + reportPermissionSetNameCollisions(undefined, []); + } finally { + cap.restore(); + } + expect(cap.seen).toEqual([]); + }); + + /** + * The measured-and-rejected spelling next door: `(logger.warn ?? console.warn)(…)` + * detaches the receiver, and a class-based host sink reaching for `this` + * throws. Every double in this package is a plain closure and would survive + * it, so the pin uses a real class. + */ + it('keeps the receiver when the host sink is a class instance', () => { + class HostSink { + lines: string[] = []; + warn(m: string): void { this.lines.push(m); } + } + const sink = new HostSink(); + const d = permissionSetNameCollisionDiagnostic({ + name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a', + }); + expect(() => reportPermissionSetNameCollisions(sink, [d])).not.toThrow(); + expect(sink.lines).toHaveLength(1); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.ts new file mode 100644 index 0000000000..5aa5ff77e8 --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.ts @@ -0,0 +1,224 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17516] The set-name collision diagnostic — ONE derivation, ONE wording, for + * every door that has to tell an author their declared permission set was not + * materialized. + * + * ## What was wrong + * + * `bootstrapDeclaredPermissions` refuses to write into a `sys_permission_set` + * row another package owns (ADR-0086 D4: a package never writes into a foreign + * record). That refusal is CORRECT and is unchanged here. What was wrong is + * that it was INVISIBLE: the whole declared set vanished with an internal + * counter incremented and one `logger?.warn?.(…)` line — optionally chained + * TWICE, so a caller that passed no logger produced no output at all. The + * comment at that branch said "refuse loudly"; nothing about it was loud, and + * nothing was written back anywhere an author could read it. + * + * That is the #10556 doctrine's own failure shape, one surface over: a report + * channel that is silent by declaration. The maintainer ruling there + * (2026-08-24) rejected silent-by-declaration and made `SecurityPlugin`'s own + * sink console-backed by default — loud until a host injects one. + * {@link reportPermissionSetNameCollisions} carries the same guarantee for this + * refusal, which is why it takes a possibly-absent sink and still prints. + * + * ## Why a module, and not two call sites + * + * The precedent this card names is #14553's `navigationContributions` fix: a + * diagnostic raised at BOTH doors — runtime and compile (`os build` / + * `os validate`) — behind ONE shared predicate so the two cannot drift. A + * compile-time door that called a collision fine while the runtime dropped the + * set would be indistinguishable, to the author, from the silence being fixed + * here. + * + * ⚠️ Only the RUNTIME door is built here. The compile-time door lives in + * `packages/cli` (`domain:cli`), a different lane, and this change deliberately + * does not reach into it. What this module exists to guarantee is that when + * that door is built it consumes {@link permissionSetNameIsForeign} and + * {@link permissionSetNameCollisionDiagnostic} rather than re-deriving either — + * which is why both, and the wording, are exported from the package entry + * rather than kept module-private. + * + * ## Why `event` and not `code` + * + * {@link PERMISSION_SET_NAME_COLLISION} is a snake_case DATA VALUE, not an + * ADR-0112 error code: it is never routed to `error.code`, never reaches a + * wire refusal, and travels as the structured half of a warning about an + * artifact. That is the discrimination the sibling `position_name_fold_grant` + * and `platform_owner_wall_bypass` tokens in `security-plugin.ts` already make + * in this package, and it is why the stamp below is spelled `event:` rather + * than `code:` — ⛔ a `code:` position here would be claiming a vocabulary this + * value is not in. + */ + +/** + * The minimal sink this module reports through. + * + * Structurally satisfied by both `ProjectionLogger` and the catalog's + * `SeedLogger` — declared here rather than imported so this module stays a leaf + * and `permission-set-projection.ts` can name the diagnostic type without a + * cycle. `warn` is non-optional for the #9754 reason its two siblings give: a + * fallback channel that may itself be absent is not a fallback. + */ +export interface CollisionReportSink { + warn: (message: string, meta?: Record) => void; +} + +/** + * The stable token stamped on every set-name-collision report, so an operator + * can grep ONE string for every silently-dropped permission set. + * + * Exported — unlike the sibling `position_name_fold_grant`, which is + * deliberately package-private because nothing outside ever consumes it. This + * one has a second consumer by construction: the compile-time door this card's + * precedent requires. A door that re-spells the literal is the drift the shared + * derivation exists to prevent. + */ +export const PERMISSION_SET_NAME_COLLISION = 'permission_set_name_collision'; + +/** One declared permission set that was NOT materialized because another package owns its name. */ +export interface PermissionSetNameCollisionDiagnostic { + /** Always {@link PERMISSION_SET_NAME_COLLISION}. */ + readonly event: typeof PERMISSION_SET_NAME_COLLISION; + /** + * Never `error`: the platform refuses the write and carries on, and the + * failure direction is CLOSED — the set is not installed, so nothing is + * over-granted. ⚠️ Were a path ever measured in which this drop ALLOWS an + * access that should have been refused, that is a different severity and a + * different card. + */ + readonly severity: 'warning'; + /** The declared set's name — the `sys_permission_set.name` that collided. */ + readonly name: string; + /** The package whose declaration was dropped. */ + readonly declaredBy: string; + /** + * The package that owns the standing row. `null` when the row is + * package-managed but carries no `package_id` — an unowned row is still not + * ours to write, and saying so is more useful than printing `undefined`. + */ + readonly ownedBy: string | null; + /** The organization whose catalog pass hit this, when the pass was scoped. */ + readonly organizationId?: string; + readonly message: string; + readonly fix: string; +} + +/** + * THE shared predicate: is the standing row's owner a DIFFERENT package from + * the one declaring this set? + * + * Both doors ask exactly this question and must get exactly this answer. + * + * ⚠️ Deliberately asks only about OWNERS, because that is the only datum the + * two doors certainly share: the runtime door reads `package_id` off a + * `sys_permission_set` row, while a compile-time door composing one artifact + * reads it off whichever declaration claimed the name first. Folding the row's + * `managed_by` check in here would make the predicate unanswerable from a + * declaration — the caller decides that a row is package-managed at all + * (env-authored rows are a different branch entirely and are never clobbered), + * and then asks this. + * + * ⚠️ A nullish owner is FOREIGN, not "ours". A package-managed row with no + * `package_id` is the exact ambiguity ADR-0086 D3 exists to remove, and + * adopting it on a name match would be a package writing into a record it + * cannot prove it owns. This preserves the behaviour the branch already had — + * ⛔ this card changes what the author is TOLD, never what is skipped. + */ +export function permissionSetNameIsForeign( + ownerPackageId: string | null | undefined, + declaringPackageId: string | null | undefined, +): boolean { + return (ownerPackageId ?? null) !== (declaringPackageId ?? null); +} + +/** + * Word ONE collision. Callers decide reachability (see the seeder's + * package-managed branch); this only words the finding, so both doors print + * the same sentence. + */ +export function permissionSetNameCollisionDiagnostic(input: { + name: string; + declaredBy: string; + ownedBy?: string | null; + organizationId?: string; +}): PermissionSetNameCollisionDiagnostic { + const ownedBy = input.ownedBy ?? null; + const owner = ownedBy ?? '(a package-managed row with no package_id)'; + return { + event: PERMISSION_SET_NAME_COLLISION, + severity: 'warning', + name: input.name, + declaredBy: input.declaredBy, + ownedBy, + ...(input.organizationId === undefined ? {} : { organizationId: input.organizationId }), + message: + `Package "${input.declaredBy}" declares permission set "${input.name}", but that set name is ` + + `already owned by ${owner}. The ENTIRE declared set was NOT materialized — a package never ` + + `writes into a foreign record (ADR-0086 D4) — so none of its object, field, tab or system ` + + `permissions are in effect, and nothing else reports this.`, + fix: + `Rename the set in package "${input.declaredBy}" to a name it owns (prefix it with the ` + + `package's own module prefix), or — if the two packages genuinely co-own this namespace ` + + `(ADR-0130 D1) — have exactly one of them declare the set and let the other depend on it.`, + }; +} + +/** One line carrying the whole finding — the text every door prints. */ +export function formatPermissionSetNameCollisionDiagnostic( + d: PermissionSetNameCollisionDiagnostic, +): string { + return `[security] [${d.event}] ${d.message} Fix: ${d.fix}`; +} + +/** + * Report every collision one catalog pass found — the half that makes the + * refusal reach the author. + * + * ⛔ NOT `logger?.warn?.(…)`. That is the defect this module exists to remove: + * with no sink injected it evaluates to nothing at all, and the set disappears + * with only a counter moved. A caller that passes no sink gets `console.warn`, + * for the reason #10556's ruling gives — loud until a host injects one. + * + * ⛔ NOT `(logger?.warn ?? console.warn)(…)` either: that evaluates to a bare + * function and calls it with `this === undefined`, and a class-based host sink + * (`@objectstack/core`'s `ObjectLogger`) reaches for `this` and throws. The + * property-access call form below keeps the receiver — the same measured + * conclusion `logSeedDurabilityFailure` records next door. + * + * ONE line per pass, not one per dropped set: a package that collides on its + * whole declaration would otherwise bury its own remedy. Every diagnostic + * travels in the structured meta beside it, and the caller gets the records + * themselves on the pass outcome, so nothing is summarised away. + * + * `warn`, not `error`: this is a FUNCTIONAL degradation in the AGENTS.md sense + * — the deployment is visibly smaller than it was authored to be, and the next + * principal who needs the grant is refused. Nothing claimed to be persisted + * silently failed to land; the write was deliberately never attempted. + */ +export function reportPermissionSetNameCollisions( + logger: CollisionReportSink | undefined, + collisions: readonly PermissionSetNameCollisionDiagnostic[], + organizationId?: string, +): void { + if (collisions.length === 0) return; + const n = collisions.length; + const message = + `[security] [${PERMISSION_SET_NAME_COLLISION}] ${n} declared permission ` + + `set${n === 1 ? ' was' : 's were'} NOT materialized — another package already owns ` + + `${n === 1 ? 'that set name' : 'those set names'} (ADR-0086 D4). ` + + `The declaring package's permissions are NOT in effect.`; + const meta = { + event: PERMISSION_SET_NAME_COLLISION, + ...(organizationId ? { organization: organizationId } : {}), + collisions: collisions.map((d) => ({ + name: d.name, + declaredBy: d.declaredBy, + ownedBy: d.ownedBy, + fix: d.fix, + })), + }; + if (logger) logger.warn(message, meta); + else console.warn(message, meta); +} diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index f8701b7b42..341778fad5 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -93,6 +93,7 @@ import { PermissionSetSchema } from '@objectstack/spec/security'; import { seedCtx, type SeedWriteRefusals } from './per-organization-catalog.js'; import { buildExistingByName, type ExistingByNameIndex } from './seed-name-lookup.js'; +import type { PermissionSetNameCollisionDiagnostic } from './permission-set-name-collision.js'; import { ENV_PROJECTION_MARKER, assertPermissionSetNotPackageDeclared, @@ -219,6 +220,18 @@ export interface PermissionSeedOutcome { skippedForeign: number; /** Records retired because their definition was deleted from metadata. */ deleted?: number; + /** + * [#17516] The diagnostic for each set counted in {@link skippedForeign} — + * the declaration was dropped WHOLE because another package owns that set + * name (ADR-0086 D4). + * + * ⚠️ This is the read-back half of that refusal, not a duplicate of the log + * line. The counter alone is what made the drop invisible: a caller holding + * it knows a number and nothing about which sets, which packages, or what to + * do about it. Absent — never `[]` — when the pass hit no collision, so a + * consumer can tell "none" apart from "this pass does not report them". + */ + collisions?: readonly PermissionSetNameCollisionDiagnostic[]; } /** diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 68b8a906d9..e0654a41d4 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2886,6 +2886,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-security/src/permission-set-name-collision.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts", "verb": "delete",