diff --git a/.changeset/curated-capability-seed-owns-its-row.md b/.changeset/curated-capability-seed-owns-its-row.md new file mode 100644 index 0000000000..e0eb4f9bbe --- /dev/null +++ b/.changeset/curated-capability-seed-owns-its-row.md @@ -0,0 +1,65 @@ +--- +'@objectstack/plugin-security': minor +--- + +**The curated capability seeder reconciles the row the platform owns — never an organization's (#8470).** + +`bootstrapSystemCapabilities` upserted each curated capability with +`find('sys_capability', { where: { name }, limit: 1 })` under a system context, +i.e. across organizations. Since #8461 made `sys_capability.name` unique per +ORGANIZATION rather than installation-wide (ADR-0120 D1, closing the +cross-tenant existence oracle #8323 reports), that lookup can have **two** +candidates: the platform's own row in the NULL-organization bucket, and one an +admin authored inside their organization — a supported action (ADR-0066 D1: the +platform DEFINES, admins EXTEND in Setup). + +Two harms followed, the second worse: + +1. an organization's authored `label`/`description` were overwritten with the + platform's copy at every boot; and +2. when the organization's row was the one selected **before** the platform's + row existed, the seeder took the update branch and the curated definition was + **never inserted, in any bucket, installation-wide**. + +**Ordering was not the missing property.** #4363's pagination tie-breaker +already appends `ORDER BY id` to any paged read of a driver-managed table, and +`limit: 1` counts as paged on both the SQL and MongoDB drivers — so the lookup +was already deterministic, deterministic on `id`, which says nothing about who +owns the row. Worse, being stable made it permanent: an installation that picked +the wrong row picked it again on every subsequent boot instead of self-healing. + +**The curated lookup is now scoped to `managed_by: 'platform'` AND +`organization_id: null`** — the two facts that jointly define the platform's own +row, and which together make the result set a provable singleton (the post-#8461 +unique key is `(COALESCE(organization_id, …), name)`, so the platform bucket +admits at most one row per name). The DERIVED half is unchanged: its own #5876 +guard already refuses to touch a row it does not own. + +The organization's row keeps its authored copy, and the platform's curated row +is seeded regardless of what any organization has authored. + +**New `blockedCurated` field on the seeding result** (a widened return type — +breaking for anyone constructing `CapabilitySeedResult` by hand, hence `minor` +per the launch-window convention). It counts, and warns about, the one collision +the seeder now declines to resolve by overwriting: a curated name already held +in the platform bucket by a row the scoped lookup did not match. Previously that +case was "resolved" by clobbering the other author; now it is refused, and +refusing silently would be its own defect, since `tryInsert` swallows the +engine's unique-constraint refusal. The warning states the provenance it +actually **read** off the blocking row rather than asserting who authored it — +the seeder observes "no platform-owned row matched, and the insert was refused", +which is not the same fact as "this row belongs to someone else". + +Reachable and ordinary, not hypothetical: `organization_id` auto-stamping lives +in the enterprise `@objectstack/organizations` runtime, which is also what +activates every walled posture — so on a deployment without it (`single` +posture, no stamper) every Setup-authored capability row lands in the +NULL-organization bucket. + +No authorization behaviour changes. Grants (`systemPermissions`) and +requirements (`requiredPermissions`) resolve capabilities **by name**, and no +runtime code path reads a `sys_capability` row to decide access — so a +never-seeded curated row was a registry/Setup-listing defect, not a privilege +one. No migration: an already-overwritten organization row is not restored, but +it stops being overwritten, and a missing curated row is seeded on the next +boot. 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 6102e60e5a..e863ec64d0 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts @@ -22,7 +22,16 @@ function makeQl(declared: any[] = []) { async find(object: string, q: any) { if (object !== 'sys_capability') return []; const where = q?.where ?? {}; - return rows.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)); + // [#8470] A `null` comparand is IS NULL, not `=== null`: `driver-sql` + // compiles `{ field: null }` to `IS NULL`, `driver-memory`'s matcher uses + // `value == condition`, and MongoDB matches null-or-missing — none of them + // is strict equality against an ABSENT key. `bootstrapSystemCapabilities` + // (called by several cases below) scopes its curated lookup with + // `organization_id: null`, which strict `===` would make unsatisfiable + // here while it works in production. + return rows.filter((r) => + Object.entries(where).every(([k, v]) => (v === null ? r[k] == null : r[k] === v)), + ); }, async insert(object: string, data: any) { if (object !== 'sys_capability') return null; 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 09a8145aca..e57f0014c6 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts @@ -1,20 +1,64 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { bootstrapSystemCapabilities, KNOWN_CAPABILITIES } from './bootstrap-system-capabilities.js'; -/** Minimal in-memory ql for sys_capability seeding. */ +/** + * Minimal in-memory ql for sys_capability seeding. + * + * [#8470] Three behaviours are modelled on purpose, and NOT because a test + * needed them to pass: each was measured against `SqlDriver` on better-sqlite3 + * (better-sqlite3 fixture mirroring `sys-capability.object.ts`, including its + * `{ fields: ['name'], unique: 'organization' }` index) before being written + * here. The point of the exercise was #8470's own warning — that the ORIGINAL + * fake returned rows in INSERTION order, which is a property of the double and + * of no driver in the system, so a pin written against it could be green for a + * reason the product does not have. + * + * 1. **`limit` orders by `id` ascending.** #4363 appends a pagination + * tie-breaker (`ORDER BY id`) to any paged read of a driver-managed table, + * and `limit: 1` counts as paged on `SqlDriver` and `MongoDBDriver` alike — + * only `findOne` opts out (`singleRowLookup`). Measured: with a platform row + * and an org row sharing a name, `find({ where: { name }, limit: 1 })` + * returned the row with the smaller `id` under BOTH insertion orders. So the + * seeder's lookup is not a coin flip; it is a stable choice made on a key + * unrelated to ownership. Insertion order is what the double used to model, + * and it models nothing. + * 2. **A `null` comparand matches a null OR absent value.** `driver-sql` + * compiles `{ field: null }` to `IS NULL`; `driver-memory`'s matcher uses + * `value == condition`; MongoDB matches null-or-missing. Strict `===`, which + * this double used, matches NONE of them and would have made + * `organization_id: null` unsatisfiable here while working in production. + * 3. **`insert` enforces the declared unique key**, `(COALESCE(organization_id, + * '__global__'), name)` — measured rejecting a second platform-bucket row + * for a name, and admitting an organization's row for that same name. + * `tryInsert` swallows the engine's refusal, which is why the seeder counts + * it (`blockedCurated`) rather than trusting silence. + */ function makeQl() { const rows: any[] = []; + /** The NULL-safe organization key part, ADR-0120 D3. */ + const bucketOf = (r: any): string => + r?.organization_id == null ? '__global__' : String(r.organization_id); return { rows, async find(object: string, q: any) { if (object !== 'sys_capability') return []; const where = q?.where ?? {}; - return rows.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)); + const matched = rows.filter((r) => + // (2) `null` is IS NULL, not `=== null`. + Object.entries(where).every(([k, v]) => (v === null ? r[k] == null : r[k] === v)), + ); + if (q?.limit === undefined) return matched; + // (1) The #4363 tie-breaker. BINARY collation, as SQLite compares ids. + return [...matched] + .sort((a, b) => (String(a.id) < String(b.id) ? -1 : String(a.id) > String(b.id) ? 1 : 0)) + .slice(0, q.limit); }, async insert(object: string, data: any) { if (object !== 'sys_capability') return null; + // (3) One holder per (organization bucket, name). + if (rows.some((r) => r.name === data.name && bucketOf(r) === bucketOf(data))) return null; rows.push({ ...data }); return { id: data.id }; }, @@ -195,6 +239,24 @@ describe('derived defaults never clobber an authored row (#5876)', () => { expect(boot2.skippedAuthored).toBe(1); }); + // [#8470] POSITIVE CONTROL for the untouched half. The curated fix must not + // make this guard unreachable: a DERIVED name on an org-authored row still + // skips. If the curated scoping ever leaked into the derived branch, the + // derived lookup would stop finding the authored row and would DERIVE a + // placeholder over it instead of skipping — `skippedAuthored` would fall to 0. + it('[#8470] the derived guard still fires for an ORG-authored row (untouched half)', async () => { + const ql = makeQl(); + ql.rows.push({ + id: 'aaa_org_derived', organization_id: 'org_jia', name: 'showcase.export_data', + ...AUTHORED, scope: 'org', managed_by: 'admin', active: true, + }); + const out = await bootstrapSystemCapabilities(ql, OPS_SETS); + expect(out.skippedAuthored).toBe(1); + expect(ql.rows.find((r) => r.id === 'aaa_org_derived')).toMatchObject({ + ...AUTHORED, managed_by: 'admin', + }); + }); + it('the guard is scoped to the DERIVED half — curated names still refresh', async () => { const ql = makeQl(); await bootstrapSystemCapabilities(ql, []); @@ -208,3 +270,251 @@ describe('derived defaults never clobber an authored row (#5876)', () => { expect(out.skippedAuthored).toBe(0); }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#8470] The CURATED half reconciles THE ROW THE PLATFORM OWNS. +// +// Since #8461 a capability `name` is unique per ORGANIZATION (ADR-0120 D1), so +// an admin may author `manage_users` inside their organization while the +// platform holds its own row in the NULL-organization bucket. A lookup on +// `{ name }` alone then has two candidates and cannot say which it means. +// +// ## Why "just add an ORDER BY" is not the fix, and how these pins are forced +// +// The lookup was ALREADY ordered. #4363 appends `ORDER BY id` to any paged read +// of a driver-managed table and `limit: 1` is paged, so on `SqlDriver` and +// `MongoDBDriver` the seeder's choice is stable — stable on `id`, which says +// nothing about who owns the row, and stable FOREVER on a given installation, +// so a boot that reconciles the wrong row never self-heals. The double models +// that rule (see `makeQl`), which is what makes the adverse case FORCIBLE +// rather than accidental: the org row is given the id `aaa_org_authored`, which +// sorts before every seeder-minted `cap_…` id, so the old code selects it. +// `zzz_org_authored` is the same fixture with the ordering benign. +// +// That pair is the anti-vacuity argument. The old behaviour DIFFERS between the +// two ids; the fixed behaviour is identical, because it never depended on the +// order in the first place. +// ─────────────────────────────────────────────────────────────────────────── +describe('[#8470] the curated half owns its row, not whichever row shares the name', () => { + const CURATED = KNOWN_CAPABILITIES.find((c) => c.name === 'manage_users')!; + const ORG_AUTHORED = { + name: 'manage_users', + label: 'ORG CUSTOM LABEL', + description: 'Authored by the organization admin in Setup.', + scope: 'org' as const, + managed_by: 'admin', + active: true, + }; + + /** An organization's row for a CURATED name — a supported ADR-0066 D1 action. */ + function orgRow(id: string, organization_id = 'org_jia') { + return { id, organization_id, ...ORG_AUTHORED }; + } + + const platformRowFor = (ql: ReturnType, name: string) => + ql.rows.find((r) => r.name === name && r.managed_by === 'platform' && r.organization_id == null); + + // ── The headline harm: `platformRowExists: false` ── + // + // Card row 3. Ordering is irrelevant here — with no platform row there is only + // ONE candidate, so the old lookup selected the organization's row whatever + // the tie-breaker did. Two distinct harms, asserted separately so a partial + // regression cannot hide behind the other. + it('seeds the platform row even when an organization already holds the name', async () => { + const ql = makeQl(); + ql.rows.push(orgRow('aaa_org_authored')); + const out = await bootstrapSystemCapabilities(ql, []); + + // Harm 2 — the worse one: the curated definition exists in NO bucket. + const platform = platformRowFor(ql, 'manage_users'); + expect(platform).toBeDefined(); + expect(platform).toMatchObject({ + label: CURATED.label, description: CURATED.description, scope: CURATED.scope, + managed_by: 'platform', active: true, + }); + // All 8 curated names land, not just the 7 with no collision. + expect(out.seeded).toBe(KNOWN_CAPABILITIES.length); + expect(out.blockedCurated).toBe(0); + }); + + it("leaves the organization's authored copy exactly as its author wrote it", async () => { + const ql = makeQl(); + ql.rows.push(orgRow('aaa_org_authored')); + await bootstrapSystemCapabilities(ql, []); + + // Harm 1: the org row must not be reconciled to the platform's copy. + expect(ql.rows.find((r) => r.id === 'aaa_org_authored')).toEqual({ + id: 'aaa_org_authored', organization_id: 'org_jia', ...ORG_AUTHORED, + }); + }); + + // ── The forced coin flip: card row 2, which the filer flagged as NOT safe ── + it.each([ + ['adverse — the org row sorts FIRST', 'aaa_org_authored'], + ['benign — the org row sorts LAST', 'zzz_org_authored'], + ])('reconciles the platform row and only the platform row (%s)', async (_label, orgId) => { + const ql = makeQl(); + // Boot 1 seeds the platform's row; the admin then authors theirs. + await bootstrapSystemCapabilities(ql, []); + ql.rows.push(orgRow(orgId)); + // A new platform version ships new copy — simulate the stale row it finds. + const platform = platformRowFor(ql, 'manage_users')!; + platform.label = 'stale label'; + platform.description = 'stale description'; + + const out = await bootstrapSystemCapabilities(ql, []); + + // The platform's own row IS refreshed (the curated reconcile is not simply + // switched off — the failure mode that would make every pin here vacuous). + expect(platform).toMatchObject({ + label: CURATED.label, description: CURATED.description, managed_by: 'platform', + }); + // …and the organization's row is untouched under BOTH id orders. Without + // the ownership scoping these two rows disagree: the adverse id makes the + // seeder write the platform copy onto the org row and leave `stale label` + // on its own. + expect(ql.rows.find((r) => r.id === orgId)).toEqual({ + id: orgId, organization_id: 'org_jia', ...ORG_AUTHORED, + }); + expect(out.seeded).toBe(0); + expect(out.blockedCurated).toBe(0); + }); + + // ── The PLATFORM-BUCKET provenance matrix ───────────────────────────────── + // + // Every case above is about a name with too MANY candidates. This block is + // the other axis: one row, in the platform's own bucket, varying only by + // `managed_by`. It exists because the scoped predicate is a CONJUNCTION, and + // a conjunction can also match ZERO rows where it should match one. + // + // `managed_by: 'platform'` is the row the seeder owns and must still find — + // the NEGATIVE case, and the one that would catch the predicate excluding the + // platform's own row on an installation that predates something. + // + // Reachability of the other three, measured rather than assumed: + // - `admin` — REACHABLE and ordinary. `organization_id` auto-stamping lives + // in the enterprise `@objectstack/organizations` runtime, which is also + // what ACTIVATES every walled posture. A deployment without it is `single` + // posture with no stamper, so EVERY Setup-authored capability row lands in + // the NULL-organization bucket. This is the default community shape, not + // an edge case. + // - `package` — REACHABLE via name promotion: `bootstrapDeclaredCapabilities` + // refuses a name in `PLATFORM_CAPABILITY_NAMES`, but a package that + // declared a name BEFORE the platform curated it (`setup.write` and + // `manage_sharing` were both added to the curated set after the fact) left + // a `managed_by:'package'` row in that bucket. + // - no value at all — NOT reachable through the engine: `managed_by` is + // `required: true` with `defaultValue: 'admin'`, and `applyFieldDefaults` + // resolves defaults on insert BEFORE the beforeInsert hooks, so an insert + // omitting it stores `'admin'`, never null. Kept as a pin anyway because + // the diagnostic must not assert an ownership verdict it cannot observe — + // which is the whole reason the message names the value it READ. + it.each([ + ['platform — the row the seeder OWNS', 'platform', 'matched'], + ['admin — Setup-authored, single-posture deployment', 'admin', 'blocked'], + ['package — declared before the name was curated', 'package', 'blocked'], + ['(absent) — not engine-reachable; the message must still be truthful', undefined, 'blocked'], + ])('platform-bucket row, managed_by=%s', async (_label, managedBy, expected) => { + const ql = makeQl(); + const PRE = { + id: 'aaa_pre_existing', + name: 'manage_users', + label: 'PRE-EXISTING LABEL', + description: 'written by whoever owns this row', + scope: 'platform' as const, + active: true, + ...(managedBy === undefined ? {} : { managed_by: managedBy }), + }; + ql.rows.push({ ...PRE }); + const warn = vi.fn(); + const out = await bootstrapSystemCapabilities(ql, [], { logger: { warn } }); + const row = ql.rows.find((r) => r.id === 'aaa_pre_existing')!; + + if (expected === 'matched') { + // The platform's own row is found by the conjunction and reconciled. + expect(out.blockedCurated).toBe(0); + expect(warn).not.toHaveBeenCalled(); + expect(row).toMatchObject({ label: CURATED.label, description: CURATED.description }); + expect(out.updated).toBeGreaterThanOrEqual(1); + } else { + expect(out.blockedCurated).toBe(1); + // Declining is the point — the blocking row is untouched… + expect(row).toEqual(PRE); + // …and reported. The message names the provenance it READ rather than + // asserting who authored the row: on a row carrying some other + // `managed_by`, "a row this pass does not own" would be a false sentence + // printed on every boot. + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('manage_users'); + expect(warn.mock.calls[0][0]).toContain( + managedBy === undefined ? 'a row carrying no managed_by value' : `managed_by='${managedBy}'`, + ); + expect(warn.mock.calls[0][0]).not.toContain('does not own'); + expect(warn.mock.calls[0][1]).toEqual({ + name: 'manage_users', + blockingRowId: 'aaa_pre_existing', + blockingManagedBy: managedBy ?? null, + }); + } + // Either way the other 7 curated names are unaffected. + expect(out.seeded).toBe(KNOWN_CAPABILITIES.length - 1); + }); + + // The THIRD branch of the diagnostic. The insert was refused, so something + // stopped it — but if the follow-up read then finds nothing, that is NOT an + // ordinary collision (a racing writer, or a refusal that was never the unique + // key) and must not be described as an unstamped row. Reachable here as the + // general shape of a rejected write: `tryInsert` returns null on ANY engine + // refusal, not only a duplicate key. + it('says so when the insert was refused but no blocking row is visible', async () => { + // Built by OVERRIDING the file's one double rather than declaring a second: + // `check:engine-double-contract` counts unguarded engine doubles per file + // against a shrink-only baseline, and a fresh literal declaring the engine + // verbs would raise it. Overriding states the same fixture — this is that + // double, with the write refused and nothing stored. + const ql = Object.assign(makeQl(), { + async find() { return []; }, + async insert() { return null; }, + }); + const warn = vi.fn(); + const out = await bootstrapSystemCapabilities(ql, [], { logger: { warn } }); + + expect(out.seeded).toBe(0); + expect(out.blockedCurated).toBe(KNOWN_CAPABILITIES.length); + expect(warn).toHaveBeenCalledTimes(KNOWN_CAPABILITIES.length); + expect(warn.mock.calls[0][0]).toContain('NO blocking row is visible'); + expect(warn.mock.calls[0][0]).toContain('worth investigating'); + // …and it must NOT borrow either of the other two branches' sentences. + expect(warn.mock.calls[0][0]).not.toContain('carrying no managed_by value'); + expect(warn.mock.calls[0][0]).not.toContain('left exactly as it is'); + expect(warn.mock.calls[0][1]).toEqual({ + name: 'manage_users', blockingRowId: undefined, blockingManagedBy: null, + }); + }); + + // A clean install must not pay for any of this: no collision, no warn, no + // counter. (A `blockedCurated` that fired on the happy path would make the + // pin above green for the wrong reason.) + it('a clean install seeds every curated name with nothing blocked', async () => { + const ql = makeQl(); + const warn = vi.fn(); + const out = await bootstrapSystemCapabilities(ql, [], { logger: { warn } }); + expect(out.seeded).toBe(KNOWN_CAPABILITIES.length); + expect(out.blockedCurated).toBe(0); + expect(warn).not.toHaveBeenCalled(); + }); + + // Two organizations may each hold the name (ADR-0066 D1 "admins EXTEND"), and + // neither displaces the platform's row. This is also the pin that would catch + // a fix written as "take the row with no organization_id" alone: it would + // still be a singleton here, but see the sibling case above for why + // `managed_by` is needed as well. + it('is unaffected by the NUMBER of organizations holding the name', async () => { + const ql = makeQl(); + ql.rows.push(orgRow('aaa_org_jia', 'org_jia'), orgRow('aab_org_yi', 'org_yi')); + const out = await bootstrapSystemCapabilities(ql, []); + expect(platformRowFor(ql, 'manage_users')).toBeDefined(); + expect(out.seeded).toBe(KNOWN_CAPABILITIES.length); + expect(ql.rows.filter((r) => r.name === 'manage_users')).toHaveLength(3); + }); +}); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts index c27429552f..9da35b2eb9 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts @@ -30,6 +30,51 @@ * The seed loop used to refresh both alike while the comment in front of it * claimed admin edits were preserved — what #2909 T3 actually made seed-once is * `scope`, and only `scope`. + * + * [#8470] The CURATED half looks up THE ROW THE PLATFORM OWNS, not the first row + * that happens to share the name. + * + * `tryFind` runs under `SYSTEM_CTX`, which carries no `tenantId`: the security + * middleware short-circuits on `isSystem` before Layer 0 is composed, and the + * engine's `buildDriverOptions` sets a driver tenant scope only when + * `execCtx.tenantId !== undefined`. The lookup therefore reads ACROSS + * organizations by construction — which is correct for a seeder, and is exactly + * why the predicate has to say which row it means. + * + * Since #8461 made `sys_capability.name` unique per ORGANIZATION rather than per + * installation (ADR-0120 D1, the cross-tenant existence oracle #8323 reports), an + * admin may author `manage_users` inside their organization while the platform + * holds its own row in the NULL-organization bucket. A lookup on `{ name }` alone + * then has two candidates and picks between them on grounds unrelated to + * ownership, with two harms: an organization's authored copy is overwritten with + * the platform's at every boot, and — when the org row is the one selected before + * the platform's row exists — the curated row is NEVER INSERTED, in any bucket, + * installation-wide. + * + * Note what the remedy is NOT. "Give the lookup an ORDER BY" was already true and + * did not help: #4363's pagination tie-breaker appends `ORDER BY id ASC` to any + * paged read of a driver-managed table, and `limit: 1` counts as paged on both + * `SqlDriver` and `MongoDBDriver` (only `findOne` opts out via + * `singleRowLookup`). So this lookup was already DETERMINISTIC on the shipped + * drivers — deterministic on `id`, a key with no relationship whatsoever to who + * owns the row, and stable per installation, so a boot that reconciles the wrong + * row keeps reconciling it forever rather than self-healing. Determinism was + * never the missing property; OWNERSHIP was. + * + * The predicate is therefore `managed_by: 'platform'` AND `organization_id: null` + * — the two facts that jointly define "the platform's own row". They also make + * the result set provably a singleton, which is what retires the ordering + * question rather than answering it: the post-#8461 unique key is + * `(COALESCE(organization_id, …), name)`, so the NULL-organization bucket admits + * at most ONE row per name, and `limit: 1` over a set of size ≤ 1 cannot be + * arbitrary. `managed_by` alone would not carry that guarantee (a platform-marked + * row sitting inside an organization — from seed data or a legacy import — would + * restore the two-candidate state), and `organization_id` alone would not + * distinguish the platform's row from an admin's in a single-organization + * deployment, where both live in the same bucket. + * + * The DERIVED half's lookup is deliberately UNCHANGED (its own guard, #5876, + * already refuses to touch a row it does not own). */ import { PLATFORM_CAPABILITIES, type PlatformCapability } from '@objectstack/spec/security'; @@ -104,6 +149,21 @@ export interface CapabilitySeedResult { * boot summary rather than warned about (#4632). */ skippedAuthored: number; + /** + * [#8470] Curated definitions whose platform row is ABSENT and could not be + * written, because a row this pass does not own already holds the name in the + * NULL-organization bucket (a Setup-authored row on a single-organization + * deployment, or a package row that claimed a curated name). + * + * This counter exists because the alternative is silence in both directions. + * Before the ownership-scoped lookup the seeder resolved that collision by + * OVERWRITING the other author's row; now it correctly declines to — but + * `tryInsert` swallows the engine's unique-constraint refusal, so declining + * would otherwise look exactly like a clean boot while a curated capability is + * missing from the registry installation-wide. Counted and warned, never + * silent. + */ + blockedCurated: number; /** Definitions considered this pass (curated + derived). */ total: number; } @@ -114,7 +174,7 @@ export async function bootstrapSystemCapabilities( options: SeedOptions = {}, ): Promise { if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') { - return { seeded: 0, updated: 0, skippedAuthored: 0, total: 0 }; + return { seeded: 0, updated: 0, skippedAuthored: 0, blockedCurated: 0, total: 0 }; } const materialized = new Set(options.materializedCapabilityNames ?? []); @@ -141,8 +201,17 @@ export async function bootstrapSystemCapabilities( let seeded = 0; let updated = 0; let skippedAuthored = 0; + let blockedCurated = 0; for (const def of byName.values()) { - const existing = await tryFind(ql, 'sys_capability', { name: def.name }, 1); + const isDerived = derivedNames.has(def.name); + // [#8470] CURATED: address the platform's OWN row. DERIVED: unchanged — its + // own `managed_by` guard below is what keeps it off rows it does not own + // (#5876), and narrowing its lookup here would change a half this card + // deliberately leaves alone. + const lookup = isDerived + ? { name: def.name } + : { name: def.name, managed_by: 'platform', organization_id: null }; + const existing = await tryFind(ql, 'sys_capability', lookup, 1); const row = existing[0]; if (row?.id) { // [#5876] Reconcile display fields only where THIS pass owns the copy. @@ -166,7 +235,7 @@ export async function bootstrapSystemCapabilities( // the caller says which names another pass already materialized, and // this guard holds even when nothing said so — an admin row for a name // no package ever declared is invisible to that list. - if (derivedNames.has(def.name) && row.managed_by !== 'platform') { + if (isDerived && row.managed_by !== 'platform') { skippedAuthored += 1; continue; } @@ -189,10 +258,57 @@ export async function bootstrapSystemCapabilities( active: true, }); if (created) seeded += 1; + else if (!isDerived) { + // [#8470] The curated row is absent AND could not be written — the + // NULL-organization bucket already holds the name under a row the + // scoped lookup did not match. Report it: a curated capability missing + // from the registry is the harm this card is about, and an unreported + // one is indistinguishable from a clean boot (`tryInsert` swallows the + // engine's unique-constraint refusal). + // + // The diagnostic states what was OBSERVED, and it costs one extra read + // — on this branch only — to be able to. The seeder knows two things: + // no row matched `managed_by:'platform' AND organization_id IS NULL`, + // and the insert was refused. It does NOT know who authored the row + // that blocked it, so it must not say "a row this pass does not own": + // that is an ownership verdict, and on a hypothetical platform row + // carrying some other `managed_by` the sentence would be false, printed + // every boot. Read the blocking row's provenance and name it instead. + // + // Note this REPORTS the distinction; it deliberately does not ACT on + // it. Adopting a differently-stamped row into the platform's identity + // would reverse the #5876 ruling that "not provably ours" resolves to + // leave-it-alone, and backfilling a stamp is a data migration. Both are + // maintainer calls, and neither is made here. + blockedCurated += 1; + // THREE outcomes, not two. "No row came back" and "a row came back + // carrying no managed_by" are different observations, and collapsing + // them would repeat this branch's own defect one level down: the insert + // WAS refused, so something blocked it, and a follow-up read that finds + // nothing is a genuinely interesting state (a racing writer, or a + // refusal that was never the unique key) — not an ordinary unstamped + // row. Say which one was seen. + const blocking = (await tryFind(ql, 'sys_capability', { name: def.name, organization_id: null }, 1))[0]; + const observation = blocking === undefined + ? 'NO blocking row is visible there at all — so the refusal was not the unique key, or the ' + + 'row has gone since. That is not an ordinary collision and is worth investigating' + : blocking.managed_by == null + ? 'a row carrying no managed_by value already holds the name, and was left exactly as it is' + : `a row with managed_by='${String(blocking.managed_by)}' already holds the name, and was ` + + 'left exactly as it is'; + options.logger?.warn?.( + `[security] curated capability "${def.name}" has no platform row and could not be seeded. ` + + 'In the platform (NULL-organization) bucket, where the declared unique key admits one row ' + + `per name: ${observation}. The platform definition is therefore missing from sys_capability ` + + 'installation-wide. Grants and requiredPermissions referencing the name are unaffected — ' + + 'they resolve by name, not by row.', + { name: def.name, blockingRowId: blocking?.id, blockingManagedBy: blocking?.managed_by ?? null }, + ); + } } } options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', { - seeded, updated, skippedAuthored, total: byName.size, + seeded, updated, skippedAuthored, blockedCurated, total: byName.size, }); - return { seeded, updated, skippedAuthored, total: byName.size }; + return { seeded, updated, skippedAuthored, blockedCurated, total: byName.size }; } diff --git a/packages/plugins/plugin-security/src/objects/sys-capability.object.ts b/packages/plugins/plugin-security/src/objects/sys-capability.object.ts index 8f7e80ffb8..84fed9311a 100644 --- a/packages/plugins/plugin-security/src/objects/sys-capability.object.ts +++ b/packages/plugins/plugin-security/src/objects/sys-capability.object.ts @@ -215,8 +215,18 @@ export const SysCapability = ObjectSchema.create({ // // Platform-seeded rows carry no organization, and the organization key part // is NULL-safe (`COALESCE(organization_id, '__global__')`, ADR-0120 D3), so - // they remain unique among themselves and `bootstrapSystemCapabilities`' - // upsert-by-name is unaffected. + // they remain unique among themselves. + // + // [#8470] This comment used to end "…and `bootstrapSystemCapabilities`' + // upsert-by-name is unaffected". That was WRONG, and the correction belongs + // next to the index that unmasked it. Widening the key to per-organization + // made the two-row state (platform row + an org's row, same `name`) + // REACHABLE, and the seeder's `find({ name }, limit: 1)` had no way to say + // which of the two it meant. The index is not the defect and must not be + // narrowed back — that would reinstate #8323's cross-tenant existence + // oracle. The seeder now scopes its curated lookup to `managed_by: + // 'platform'` + `organization_id: null`, i.e. exactly the bucket this key + // part keeps a singleton. { fields: ['name'], unique: 'organization' }, { fields: ['scope'] }, { fields: ['active'] },