diff --git a/AGENTS.md b/AGENTS.md index 0dbb090..105ad54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -121,7 +121,14 @@ them. Revisit only if the stated reason stops holding. Permissions screen's create form instead and needed a second guard for the assign form, which could also mint one — deleting the screen removed both. - `ADMIN_PERMISSIONS` **defaults to empty**: every permission is owned by the plugin that gates on - it, and a host-invented default would gate nothing. This makes the seed a function of what + it, and a host-invented default would gate nothing. **An unusable value there is dropped with a + warning, never fatal** — fail-loud belongs at the manifest boundary, where a developer authored + the mistake; `bootstrap` gates `web`, so refusing operator env takes the whole stack down. This + is not hypothetical: `admin` was this setting's own default until 2026-08-05, so a boot-breaking + value is the *expected* leftover on upgrade, and a revision of this branch shipped exactly that + bug past a green CI. `e2e-tests/compose.auth.yml` now seeds `ADMIN_PERMISSIONS: admin,users:read` + so the container proves it survives one; verified by negative control (re-adding the throw fails + that suite at stack-up). This makes the seed a function of what `bootstrap` discovers, and a plugin dropped in after first boot therefore needs `docker compose up -d` (which re-runs the one-shot), not `restart web`. The base file gives `bootstrap` and `web` the same baked `plugins/`; only `compose.override.yml`'s dev-only `.:/app` diff --git a/compose.override.yml b/compose.override.yml index 4842bcb..52e3e94 100644 --- a/compose.override.yml +++ b/compose.override.yml @@ -22,14 +22,17 @@ services: # Mount your own menu/branding override into the empty config/ dir (defaults apply otherwise): # - ./config:/app/config:ro # your config/menu.ts — see examples/config/menu.ts for a template - # The seed grants what the installed plugins declare, so bootstrap must discover the same plugins - # as web. Only dev needs saying: the base file gives both services the image's baked plugins/, and - # it is the `.:/app` above — dev-only — that makes web diverge onto the host tree. Mirror it here - # rather than in the base file, where it would instead desynchronise them (and collide with the - # e2e stacks, which mount individual plugins *inside* this path). + # Mirror web's source mount so bootstrap discovers the same plugins *and* runs the same code. Only + # dev needs saying: the base file gives both services the image's baked copy, and it is the + # `.:/app` above — dev-only — that makes web diverge onto the host tree. Without the mirror, + # bootstrap silently runs whatever `src/` was baked at image-build time, so an edit to + # bootstrap.ts appears to do nothing until someone remembers `--build`. + # It belongs here and not in the base file, where it would desynchronise prod and collide with the + # e2e stacks, which bind individual plugins *inside* /app/plugins. bootstrap: volumes: - - ./plugins:/app/plugins:ro + - .:/app + - /app/node_modules # Mock backend ready for the reference plugin (examples/plugins/scheduling): plugins/ ships empty, so # the plugin is opt-in — `cp -r examples/plugins/scheduling plugins/scheduling`, restart, and this diff --git a/e2e-tests/compose.auth.yml b/e2e-tests/compose.auth.yml index 7e3a716..764f64e 100644 --- a/e2e-tests/compose.auth.yml +++ b/e2e-tests/compose.auth.yml @@ -33,9 +33,14 @@ services: # This stack mounts no plugins, so nothing declares a permission for the bootstrap to seed — and # the suite asserts that Keto's grants reach the JWT claim. Name one explicitly so there is # something to project. + # + # `admin` rides along on purpose: it was this setting's default until 2026-08-05 and is not a legal + # `:` name, so it is exactly the leftover an upgrading deployment carries. An + # earlier revision made that fatal, and bootstrap gates `web` — so if the boot ever refuses operator + # env again, `web` never turns healthy and this suite fails instead of CI going green over it. bootstrap: environment: - ADMIN_PERMISSIONS: users:read + ADMIN_PERMISSIONS: admin,users:read # Shorten the session→JWT TTL and expose a network-resolvable base_url (ory/kratos/e2e.yml), # merged after the base config. diff --git a/src/auth/bootstrap.test.ts b/src/auth/bootstrap.test.ts index 6ebe5e9..f9cd395 100644 --- a/src/auth/bootstrap.test.ts +++ b/src/auth/bootstrap.test.ts @@ -33,19 +33,31 @@ test("permissionTuple grants a permission to user: in the Permission namespa test("seedPermissions unions ADMIN_PERMISSIONS (empty by default) with the discovered plugins' declared permissions", () => { // Clean clone: no ADMIN_PERMISSIONS, the scheduling plugin declares its two names → the demo admin // holds exactly what the installed plugins gate on, derived from discovery, not hardcoded here. - assert.deepEqual(seedPermissions(undefined, ["scheduling:read", "scheduling:write"]), ["scheduling:read", "scheduling:write"]); + const names = (env: string | undefined, declared: string[]): string[] => seedPermissions(env, declared).permissions; + assert.deepEqual(names(undefined, ["scheduling:read", "scheduling:write"]), ["scheduling:read", "scheduling:write"]); // No plugins → nothing to grant. A host-invented base would be a permission that gates nothing. - assert.deepEqual(seedPermissions(undefined, []), []); - assert.deepEqual(seedPermissions("ops:read, ops:write ", ["inventory:read"]), ["ops:read", "ops:write", "inventory:read"]); // env trimmed + extended - assert.deepEqual(seedPermissions("scheduling:read", ["scheduling:read"]), ["scheduling:read"]); // dedup, no double grant - assert.deepEqual(seedPermissions(",, ", [" scheduling:read ", ""]), ["scheduling:read"]); // blanks dropped, names trimmed (both sides) + assert.deepEqual(names(undefined, []), []); + assert.deepEqual(names("ops:read, ops:write ", ["inventory:read"]), ["ops:read", "ops:write", "inventory:read"]); // env trimmed + extended + assert.deepEqual(names("scheduling:read", ["scheduling:read"]), ["scheduling:read"]); // dedup, no double grant + assert.deepEqual(names(",, ", [" scheduling:read ", ""]), ["scheduling:read"]); // blanks dropped, names trimmed (both sides) }); -test("seedPermissions refuses an ADMIN_PERMISSIONS name that isn't :", () => { - // The operator's env is the one remaining hand-typed path; a manifest's names were checked at - // discovery. `admin` would otherwise write a tuple that gates nothing, with no error anywhere. - assert.throws(() => seedPermissions("admin", []), /ADMIN_PERMISSIONS.*:.*admin/s); - assert.throws(() => seedPermissions("users:read,Bad Name", []), /Bad Name/); +// The regression this pins: an earlier revision *threw* here, so `ADMIN_PERMISSIONS=admin` — this +// setting's own default until 2026-08-05 — exited bootstrap 1, and bootstrap gates `web`, so a +// leftover variable bricked the whole stack on upgrade. Bootstrap must never refuse to start over +// operator env: drop what it can't use, report it, seed the rest. +test("seedPermissions drops an ADMIN_PERMISSIONS name that isn't :, and never throws", () => { + const legacy = seedPermissions("admin", ["users:read"]); + assert.deepEqual(legacy, { ignored: ["admin"], permissions: ["users:read"] }); + + const mixed = seedPermissions("admin, ops:read ,Bad Name", ["users:read"]); + assert.deepEqual(mixed, { ignored: ["admin", "Bad Name"], permissions: ["ops:read", "users:read"] }); + + // Whatever an operator puts there, the boot survives it — that is the property, not the parsing. + for (const value of ["admin", "Bad Name", ":", "::", "a".repeat(200), ",,,", "ADMIN", "1"]) { + assert.doesNotThrow(() => seedPermissions(value, ["users:read"]), value); + assert.deepEqual(seedPermissions(value, ["users:read"]).permissions.includes("users:read"), true, value); + } }); test("seedAdmin on a fresh stack creates the identity and grants every permission (one tuple each)", async () => { diff --git a/src/auth/bootstrap.ts b/src/auth/bootstrap.ts index ee7c8fb..95e850f 100644 --- a/src/auth/bootstrap.ts +++ b/src/auth/bootstrap.ts @@ -36,14 +36,18 @@ export function permissionTuple(userId: string, permission: string) { // The base is empty because permissions are `:` and every one of them is owned by // the plugin that gates on it — a host-invented default would gate nothing. // ADMIN_PERMISSIONS is the one place an operator names a permission by hand, so it is held to the -// same `:` rule discovery applies to a manifest — fail loud rather than write a -// tuple that gates nothing. A declared name has already passed that check at discovery. -export function seedPermissions(adminPermissionsEnv: string | undefined, declaredNames: string[]): string[] { +// same `:` rule discovery applies to a manifest — but *dropped with a warning*, +// never fatal. Fail-loud belongs at the manifest boundary, where a developer authored the mistake +// and can fix it; this is operator env, bootstrap gates `web`, and the whole stack must not refuse +// to start over a stale variable. `admin` was this setting's own default before 2026-08-05, so a +// value that bricks the boot is the *expected* leftover on any upgrade. The name it would have +// written gates nothing anyway. Declared names already passed the check at discovery. +export function seedPermissions(adminPermissionsEnv: string | undefined, declaredNames: string[]): { ignored: string[]; permissions: string[] } { const clean = (xs: string[]): string[] => xs.map((r) => r.trim()).filter(Boolean); const configured = clean((adminPermissionsEnv ?? "").split(",")); - const bad = configured.filter((name) => !isValidPermissionName(name)); - if (bad.length > 0) throw new Error(`bootstrap: ADMIN_PERMISSIONS must be : names, e.g. "things:read"; got ${bad.join(", ")}`); - return [...new Set([...configured, ...clean(declaredNames)])]; + const ignored = configured.filter((name) => !isValidPermissionName(name)); + const valid = configured.filter((name) => isValidPermissionName(name)); + return { ignored, permissions: [...new Set([...valid, ...clean(declaredNames)])] }; } // --- JWKS safety net ----------------------------------------------------------------- @@ -155,7 +159,10 @@ async function main() { // Seed every discovered plugin's declared permission names (plus any ADMIN_PERMISSIONS), so the // shipped example — and any dropped-in plugin — works for the demo admin without a host edit. const declared = declaredPermissions(await discoverPlugins()).map((decl) => decl.name); - const permissions = seedPermissions(env["ADMIN_PERMISSIONS"], declared); + const { ignored, permissions } = seedPermissions(env["ADMIN_PERMISSIONS"], declared); + if (ignored.length > 0) { + log.warn("ignoring ADMIN_PERMISSIONS entries that are not :", { ignored: ignored.join(", ") }); + } const email = env["ADMIN_EMAIL"] ?? "admin@plainpages.local"; const password = env["ADMIN_PASSWORD"] ?? "admin"; const result = await seedAdmin({