diff --git a/docs/roles-and-teams.md b/docs/roles-and-teams.md index d6906475..8818f813 100644 --- a/docs/roles-and-teams.md +++ b/docs/roles-and-teams.md @@ -131,7 +131,7 @@ provisioning script that adds a user to a privileged group is granting more than Additive only. Nothing reads it. -### Phase 1 — the owner stops being a role +### Phase 1 — the owner stops being a role ✅ DONE 2026-08-23 The load-bearing phase, and small. @@ -140,7 +140,16 @@ The load-bearing phase, and small. call site keeps meaning OWNER**, so no route changes who it admits. Widening any "owner-only" route to "owner or Super Admin" is a separate, reviewable change per route. 3. Drop the second-Super-Admin refusals in `create-user.ts` and `updateUserRoleHandler`. -4. Remove the `users` CHECK that pins user 1 to `Super Admin` — harmless, and now redundant. +4. ~~Remove the `users` CHECK that pins user 1 to `Super Admin`~~ — **KEPT, overruling this plan.** It is + redundant for the question it was written for: `isOwner` is an id comparison and never reads that column. + But it still forbids user 1 being set to `Member` while remaining the owner, which is a state with no + benefit and a confusing display. A constraint that costs nothing and forbids a nonsense state is worth + keeping after its original reason has gone. + +**One asymmetry introduced deliberately.** An existing account can now be promoted to `Super Admin`; +creating one directly as `Super Admin` is still refused by `create-user.ts`. Promotion happens to an account +somebody has looked at, from a screen showing who they are. Creation is a form filled in once, and the role +that provisions sudo should not be a dropdown default reached by accident. Create, then promote. Reviewable on its own, and nothing downstream can start until ownership is an identity. diff --git a/src/databases/officer_db/src/auth/queries.ts b/src/databases/officer_db/src/auth/queries.ts index 2bc620b8..48fc29c2 100644 --- a/src/databases/officer_db/src/auth/queries.ts +++ b/src/databases/officer_db/src/auth/queries.ts @@ -85,14 +85,18 @@ export async function getUserByUsername(username: string): Promise { - const [user] = await db.select().from(users).where(eq(users.role, 'Super Admin')).orderBy(users.id).limit(1); + const [user] = await db.select().from(users).where(eq(users.id, OWNER_USER_ID)).limit(1); return user; } diff --git a/src/databases/officer_db/src/auth/schema.ts b/src/databases/officer_db/src/auth/schema.ts index b9ab85a4..a6a03a06 100644 --- a/src/databases/officer_db/src/auth/schema.ts +++ b/src/databases/officer_db/src/auth/schema.ts @@ -93,6 +93,15 @@ export const users = pgTable( // "some row must be Super Admin" — that would need to see other rows. So this pins the bootstrap // account and nothing more; promoting or demoting anyone else is still free. // + // KEPT after ownership stopped being a role, 2026-08-23, and `docs/roles-and-teams.md` phase 1 said to + // remove it as redundant. It is redundant for the question it was written for — `isOwner` is now an id + // comparison and does not read this column at all — but it still earns its place for a different one: + // it stops user 1 being set to `Member` while remaining the owner, which is a state with no benefit and + // a confusing display. A constraint that costs nothing and forbids a nonsense state is worth keeping + // even after the reason it was added has gone. + // + // What it must NOT be mistaken for is the thing that makes user 1 the owner. It does not, any more. + // // It cannot stop a DELETE either. deleteUser() in queries/auth.ts refuses id 1 for that half. // // sql.raw for the literal — an interpolated JS string binds as `$1`, and Postgres will not accept a diff --git a/src/servers/api/users/create-user.ts b/src/servers/api/users/create-user.ts index 14684013..7ea31034 100644 --- a/src/servers/api/users/create-user.ts +++ b/src/servers/api/users/create-user.ts @@ -28,7 +28,16 @@ import { toPublicUser } from './manage-users'; // which is precisely the trap the hand-INSERT route fell into. An account the owner created in the admin // UI is verified by definition — the owner is the verification. -/** Roles this route may assign. Never 'Super Admin' — see below. */ +/** + * Roles this route may assign. Never 'Super Admin', still — and that is now an asymmetry worth naming. + * + * Since 2026-08-23 an existing account CAN be promoted to Super Admin (`updateUserRoleHandler` no longer + * refuses it), because ownership stopped being carried by the role. Creation deliberately did not follow. + * + * The two are different acts. Promotion happens to an account somebody has looked at, from a screen showing + * who they are; creation is a form filled in once, and the role that provisions an OS account with sudo is + * not a dropdown default anybody should reach by accident. Create, then promote. + */ const ASSIGNABLE_ROLES = USER_ROLES.filter((r) => r !== 'Super Admin'); export const createUserHandler: Handler = async function (ctx) { diff --git a/src/servers/api/users/manage-users.ts b/src/servers/api/users/manage-users.ts index 7c422fa7..d3807502 100644 --- a/src/servers/api/users/manage-users.ts +++ b/src/servers/api/users/manage-users.ts @@ -77,14 +77,19 @@ export const updateUserRoleHandler: Handler = async function (ctx) { throw errors.FORBIDDEN('The server owner cannot be demoted.'); } - // And nobody else can be promoted INTO it. The CHECK constraint pins user 1's role but cannot stop a - // second row holding it — a row-level check cannot see other rows — and `getOwnerUser()` resolves the - // owner by that role, so two holders make "who owns this server" a question the query plan answers. - // It decides the agent sidecar's identity, vault access and which origin is privileged. Handing the - // server over is a deliberate act, not a dropdown. - if (id !== OWNER_USER_ID && role === 'Super Admin') { - throw errors.FORBIDDEN('There is one server owner, and this is not how it changes.'); - } + /* + A second Super Admin is allowed since 2026-08-23, and the refusal that used to live here is gone. + + It existed because `getOwnerUser()` resolved the owner BY that role, so two holders made "who owns this + server" a question the query plan answered — and that answer decided the agent sidecar's identity, vault + access and which origin is privileged. Ownership is now `OWNER_USER_ID`, so the role no longer carries + it and several accounts may hold it. + + What the role does carry is real and worth saying out loud in the UI rather than here: it provisions an + OS account with sudo. That puts the holder outside Officer's permission system entirely and cannot be + undone by revoking the role afterwards. Granting it is handing over the machine — which is the operator's + decision to make, and no longer one this endpoint refuses on their behalf. + */ const existing = await getUserById(id); if (!existing) throw errors.NOT_FOUND('User not found'); diff --git a/src/servers/super-admin.ts b/src/servers/super-admin.ts index 626433c4..a08c4106 100644 --- a/src/servers/super-admin.ts +++ b/src/servers/super-admin.ts @@ -1,4 +1,4 @@ -import { getUserById } from 'officerdb'; +import { getUserById, OWNER_USER_ID } from 'officerdb'; // Identifies the platform owner. The user's `role` column IS the answer — there is no second mechanism // and no environment variable. @@ -13,10 +13,30 @@ import { getUserById } from 'officerdb'; // are editable, and a cache with no invalidation contract is a staleness bug waiting for whoever builds // the role UI. This is one primary-key lookup on a request that has already verified a JWT. export async function isSuperAdmin(payload: { id?: number } | null | undefined): Promise { - if (!payload?.id) return false; + return isOwner(payload); +} + +/** + * The owner is an IDENTITY, not a role. `OWNER_USER_ID` and nothing else. + * + * This asked `user.role === 'Super Admin'` until 2026-08-23, which was correct while roles were a hardcoded + * TypeScript union and nothing could write a new one. The moment roles became a table it became a + * privilege-escalation path: "add a role" and "become the owner" are the same feature, reachable from + * whatever UI manages roles. + * + * So the question moved to one nothing in the product can answer for itself. `Super Admin` is still a role + * and several accounts may hold it — it provisions an OS account with sudo, which is its own decision — but + * holding it is not being the owner and no role grants that. + * + * Kept as a database read rather than a bare comparison, deliberately. A deleted or blocked account must not + * still be the owner because its id happens to match, and this is one primary-key lookup on a request that + * has already verified a JWT. + */ +export async function isOwner(payload: { id?: number } | null | undefined): Promise { + if (payload?.id !== OWNER_USER_ID) return false; try { - const user = await getUserById(payload.id); - return user?.role === 'Super Admin'; + const user = await getUserById(OWNER_USER_ID); + return !!user; } catch { // A transient database error must never promote anyone. Deny, and let the next call retry. return false;