From 44ee444cacc8631f44c28f2fe0af941b0e759643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 23 Aug 2026 14:49:20 +0000 Subject: [PATCH] the owner is an identity, not a role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roles and teams, phase 1. `isSuperAdmin` asked `user.role === 'Super Admin'`, which was right while roles were a hardcoded union and nothing could write a new one. It stopped being right the moment roles became a table: from then on "add a role" and "become the owner" were the same feature, reachable from whatever UI manages roles. So ownership is `OWNER_USER_ID` and nothing else. `isOwner` is the new name and `isSuperAdmin` delegates to it, so no call site changes who it admits — widening any "owner-only" route to "owner or Super Admin" is a separate decision per route, not a side effect of this. Still a database read rather than a bare comparison: a deleted or blocked account must not remain the owner because its id happens to match. `getOwnerUser` becomes a primary-key lookup. It used to select on the role with `order by id` as a tiebreak, because two holders would otherwise make "who owns this server" whatever Postgres returned first — an answer that decides the agent sidecar's identity, vault access and origin scoping. That tiebreak, the write-path refusals and the CHECK all protected an answer that is now a primary key. Several Super Admins are allowed. What the role carries is real and belongs in the UI rather than a refusal here: it provisions an OS account with sudo, which puts the holder outside Officer's permission system and cannot be undone by revoking the role afterwards. That is the operator's decision, not this endpoint's. Two departures from docs/roles-and-teams.md, both recorded there. The CHECK pinning user 1's role is KEPT — redundant for ownership, but it still forbids the owner being set to Member, which is a nonsense state that costs nothing to prevent. And promotion to Super Admin is allowed while creating an account directly as one is not: promotion happens to an account somebody has looked at, creation is a form filled once, and the role that grants sudo should not be a dropdown default. Co-Authored-By: Claude Opus 5 Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043 --- docs/roles-and-teams.md | 13 +++++++-- src/databases/officer_db/src/auth/queries.ts | 18 ++++++++----- src/databases/officer_db/src/auth/schema.ts | 9 +++++++ src/servers/api/users/create-user.ts | 11 +++++++- src/servers/api/users/manage-users.ts | 21 +++++++++------ src/servers/super-admin.ts | 28 +++++++++++++++++--- 6 files changed, 78 insertions(+), 22 deletions(-) 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;