diff --git a/CLAUDE.md b/CLAUDE.md index 4118c3c4..db1b5b50 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -207,7 +207,7 @@ ahead of everything, and it re-verifies the token itself so it covers routes tha - `permissions/registry.ts` — the single enumeration of what the platform can do, in five kinds: `core` (every account, not deniable), `app` (**the grantable surface**), `confined` (grantable, but only to an account that has a Linux user), `execution` and `admin` (owner only, and `execution` is - never grantable at any level). **21 entries as of 2026-08-23** — verified by `bun run docs:check`, which fails if this number drifts again: 5 core, 4 app, 4 confined, 4 execution, 4 admin. It said 27 for nine days. + never grantable at any level). **22 entries as of 2026-08-23** — verified by `bun run docs:check`, which fails if this number drifts again: 5 core, 4 app, 4 confined, 4 execution, 5 admin. It said 27 for nine days. - `permissions/authorize.ts` — resolves "may this account do this". Owner short-circuits first; every other answer is role grants plus core, with `execution`/`admin` stripped even if a row grants them, and `confined` stripped for an account with no `osUser`. diff --git a/src/databases/officer_db/src/roles/index.ts b/src/databases/officer_db/src/roles/index.ts index 23bff5c4..e0fc24ba 100644 --- a/src/databases/officer_db/src/roles/index.ts +++ b/src/databases/officer_db/src/roles/index.ts @@ -1,2 +1,13 @@ export { roles } from './schema'; -export { listRoles, getRole, seedRole, type RoleRow } from './queries'; +export { + listRoles, + getRole, + seedRole, + createRole, + renameRole, + deleteRole, + type RoleRow, + type CreateRoleResult, + type RenameRoleResult, + type DeleteRoleResult, +} from './queries'; diff --git a/src/databases/officer_db/src/roles/queries.ts b/src/databases/officer_db/src/roles/queries.ts index 7cd3eaab..b10b8527 100644 --- a/src/databases/officer_db/src/roles/queries.ts +++ b/src/databases/officer_db/src/roles/queries.ts @@ -1,6 +1,7 @@ -import { asc, eq, sql } from 'drizzle-orm'; +import { and, asc, eq, sql } from 'drizzle-orm'; import { db } from '../db'; import { users } from '../auth/schema'; +import type { UserRole } from '../auth/schema'; import { roles } from './schema'; export type RoleRow = { @@ -59,3 +60,137 @@ export async function seedRole(row: { .returning({ name: roles.name }); return inserted.length > 0; } + +// ── Write half: role CRUD ───────────────────────────────────────────────────────────────────────── +// +// Every function here returns a discriminated result instead of throwing, because the router has to give +// a person a reason they can act on ("Developer is a system role" vs "3 accounts still hold it" are two +// different screens), and a caught exception only carries a string to parse. The one thing each guard +// shares: it re-checks its own invariant inside the write, not just before it, so a race between the +// check and the statement fails closed rather than silently succeeding. + +/** How many accounts hold this role. One definition, so rename and delete cannot disagree on "in use". */ +async function roleHolderCount(name: string): Promise { + const [row] = await db + .select({ count: sql`count(*)::int` }) + .from(users) + .where(eq(users.role, name as UserRole)); + return row?.count ?? 0; +} + +export type CreateRoleResult = { ok: true; role: RoleRow } | { ok: false; reason: 'invalid-name' | 'duplicate' }; + +/** + * Create a role at runtime. `isSystem` is always false here — a role the platform's own code names by + * string (Developer, Super Admin, Member; see the schema comment) is one seeded at startup, never one + * typed into a form, and nothing reachable through this function may claim otherwise. + */ +export async function createRole(input: { name: string; description?: string | null }): Promise { + const name = input.name.trim(); + if (!name) return { ok: false, reason: 'invalid-name' }; + + const inserted = await db + .insert(roles) + .values({ name, description: input.description?.trim() || null, isSystem: false }) + .onConflictDoNothing({ target: roles.name }) + .returning(); + const role = inserted[0]; + if (!role) return { ok: false, reason: 'duplicate' }; + return { ok: true, role }; +} + +export type RenameRoleResult = + | { ok: true; role: RoleRow } + | { ok: false; reason: 'not-found' | 'system' | 'invalid-name' | 'duplicate' | 'in-use'; holderCount?: number }; + +/** + * Rename a role. Refuses outright on `isSystem` (D4 in docs/roles-and-teams.md): `rolePermitsDevTools()` + * tests for `'Developer'` by string, `bootstrap.ts` writes `'Super Admin'`, and `users.role` defaults to + * `'Member'`. Renaming one of those four is a code change across three call sites that has not happened, + * not a data edit — so this refuses rather than quietly breaking them. + * + * `users.role` and `role_permissions.role` are NOT yet foreign keys onto this table (phase 2 part 2 of the + * plan, shipped as its own release), so this UPDATE does not cascade anywhere today — it only relabels the + * `roles` row itself, leaving every holder and every grant still naming the OLD name. Once the FK lands + * with `ON UPDATE CASCADE`, this same statement starts moving them automatically and nothing here changes. + */ +export async function renameRole(from: string, to: string): Promise { + const name = to.trim(); + if (!name) return { ok: false, reason: 'invalid-name' }; + + const existing = await getRole(from); + if (!existing) return { ok: false, reason: 'not-found' }; + if (existing.isSystem) return { ok: false, reason: 'system' }; + if (name === from) return { ok: true, role: existing }; + + /* + Refuse a rename while anybody holds it, for exactly as long as there is no foreign key. + + `users.role` is a plain text column today — phase 2 part 2 of `docs/roles-and-teams.md` ships the + foreign key with ON UPDATE CASCADE as its own release — so renaming this row would leave every holder + pointing at a name that no longer exists. + + It happens to be unreachable right now: `manage-users.ts` validates an incoming role against the + `USER_ROLES` union, so a role created at runtime cannot be assigned and a non-system role has no + holders. That is safety by accident, and it evaporates the moment part 4 makes `UserRole` a plain + string. This guard means those two parts can land in either order without opening a window. + + Delete it when the cascade exists, and not before — the cascade then does this job properly, by moving + the holders rather than refusing. + */ + const held = await roleHolderCount(from); + if (held > 0) return { ok: false, reason: 'in-use', holderCount: held }; + + const clash = await getRole(name); + if (clash) return { ok: false, reason: 'duplicate' }; + + // `isSystem: false` in the WHERE is the re-check: belt and suspenders against a role becoming system + // between the read above and this write. + const [role] = await db + .update(roles) + .set({ name }) + .where(and(eq(roles.name, from), eq(roles.isSystem, false))) + .returning(); + if (!role) return { ok: false, reason: 'system' }; + return { ok: true, role }; +} + +export type DeleteRoleResult = + | { ok: true } + | { ok: false; reason: 'not-found' | 'system' | 'in-use'; holderCount?: number }; + +/** + * Delete a role. Refuses if it is system-named (D4), and refuses if any account currently holds it. + * + * Never reassigns holders to a fallback — silently moving accounts onto a different role's grants is a + * permission change nobody asked for, and "absence denies" is the posture everywhere else in this system. + * The holder count and the delete run in one transaction so a role cannot be handed to someone in the gap + * between counting and deleting. + */ +export async function deleteRole(name: string): Promise { + const existing = await getRole(name); + if (!existing) return { ok: false, reason: 'not-found' }; + if (existing.isSystem) return { ok: false, reason: 'system' }; + + return db.transaction(async (tx) => { + // `name` is not necessarily one of the four literal roles the column's TS type enumerates — this + // function accepts any role that exists as a ROW, which by definition this one is not (yet, if it + // never was). The cast is type-level only: `role` is a plain `text` column, so drizzle emits the same + // `role = $1` whether or not the value belongs to that union. + const [holders] = await tx + .select({ count: sql`count(*)::int` }) + .from(users) + .where(eq(users.role, name as UserRole)); + const count = holders?.count ?? 0; + if (count > 0) return { ok: false, reason: 'in-use', holderCount: count }; + + // Same re-check as renameRole, same reason: the row must not vanish out from under a role that + // became system between the read above and this statement. + const deleted = await tx + .delete(roles) + .where(and(eq(roles.name, name), eq(roles.isSystem, false))) + .returning({ name: roles.name }); + if (!deleted.length) return { ok: false, reason: 'system' }; + return { ok: true }; + }); +} diff --git a/src/servers/api/roles/router.ts b/src/servers/api/roles/router.ts new file mode 100644 index 00000000..39649e87 --- /dev/null +++ b/src/servers/api/roles/router.ts @@ -0,0 +1,109 @@ +import { createRouter } from '../../create-router'; +import * as errors from '@@/custom-errors'; +import { isSuperAdmin } from '../../super-admin'; +import { listRoles, createRole, renameRole, deleteRole } from 'officerdb'; + +/* + /api/roles — role CRUD (phase 2 part 3 of docs/roles-and-teams.md). + + This is the roles TABLE — a name, a description, the `isSystem` flag. It is not the GRANTS a role + holds; that stays `role_permissions`, administered at `/api/users/permissions` by + `permissionAdminRouter`. The two are separate screens for a reason: this router answers "which roles + exist", that one answers "what may each reach" — and deleting a role here has to know the second + question's table too, because the guard below is "does anyone hold it", not "what does it grant". + + ── Owner only, explicitly ── + + `roles` is `kind: 'admin'` in the permission registry, so the global gate already refuses a non-owner + before a request reaches here. This is the same belt-and-braces every other admin router in this + directory adds on top of that (app-store, server-admin): a mistake in the registry must not turn into + the ability to rename or delete a role. + + ── Why delete refuses instead of reassigning ── + + D4 in the design doc: never move a role's holders onto a fallback. That is a permission change nobody + asked for, and "absence denies" — the posture everywhere else in this system — means a role with + accounts still on it stays exactly as reachable as it was until every one of them is moved off by + someone who chose to. +*/ + +export const rolesRouter = createRouter(); + +rolesRouter.use(async (ctx, next) => { + if (!(await isSuperAdmin(ctx.get('user')))) throw errors.FORBIDDEN('Role management is owner-only'); + return next(); +}); + +/** GET /api/roles — every role, with how many accounts hold each. */ +rolesRouter.get('/', async (ctx) => ctx.json({ roles: await listRoles() })); + +/** POST /api/roles — create a role. Always non-system; see the schema comment on `isSystem`. */ +rolesRouter.post('/', async (ctx) => { + const body = ctx.get('body') as { name?: unknown; description?: unknown }; + if (typeof body?.name !== 'string') throw errors.BAD_REQUEST('name is required'); + const description = typeof body.description === 'string' ? body.description : null; + + const result = await createRole({ name: body.name, description }); + if (!result.ok) { + throw result.reason === 'invalid-name' + ? errors.BAD_REQUEST('a role name cannot be empty') + : errors.CONFLICT(`a role named '${body.name.trim()}' already exists`); + } + return ctx.json({ role: result.role }); +}); + +/** + * PATCH /api/roles/:name — rename. Refuses a system role outright: `Developer`, `Super Admin` and + * `Member` are tested by string in code that has not been updated to read this table (see D4), so + * renaming one of those would silently detach the code's own idea of who holds what. + */ +rolesRouter.patch('/:name', async (ctx) => { + const from = ctx.req.param('name'); + const body = ctx.get('body') as { name?: unknown }; + if (typeof body?.name !== 'string') throw errors.BAD_REQUEST('name is required'); + + const result = await renameRole(from, body.name); + if (!result.ok) { + switch (result.reason) { + case 'not-found': + throw errors.NOT_FOUND(`no such role '${from}'`); + case 'system': + throw errors.FORBIDDEN( + `'${from}' is a system role and the platform's own code names it — it cannot be renamed`, + ); + case 'invalid-name': + throw errors.BAD_REQUEST('a role name cannot be empty'); + case 'duplicate': + throw errors.CONFLICT(`a role named '${body.name.trim()}' already exists`); + case 'in-use': + // Refused while held, because there is no ON UPDATE CASCADE yet — see `renameRole`. The count is + // in the message because 'in use' without a number is something nobody can act on. + throw errors.CONFLICT( + `'${from}' is held by ${result.holderCount ?? 'some'} account(s) — renaming it would leave them ` + + 'pointing at a role that no longer exists', + ); + } + } + return ctx.json({ role: result.role }); +}); + +/** DELETE /api/roles/:name — refuses a system role, and refuses one any account still holds. */ +rolesRouter.delete('/:name', async (ctx) => { + const name = ctx.req.param('name'); + const result = await deleteRole(name); + if (!result.ok) { + switch (result.reason) { + case 'not-found': + throw errors.NOT_FOUND(`no such role '${name}'`); + case 'system': + throw errors.FORBIDDEN( + `'${name}' is a system role and the platform's own code names it — it cannot be deleted`, + ); + case 'in-use': + throw errors.CONFLICT( + `${result.holderCount} account(s) still hold '${name}' — move them to another role first`, + ); + } + } + return ctx.json({ ok: true }); +}); diff --git a/src/servers/hono.ts b/src/servers/hono.ts index 4a51ca28..81961803 100644 --- a/src/servers/hono.ts +++ b/src/servers/hono.ts @@ -9,6 +9,7 @@ import { themesRouter } from './api/themes/themes'; import { themeFavoritesRouter } from './api/themes/favorites'; import { waitlistRouter } from './api/waitlist/waitlist'; import { usersRouter } from './api/users/users-router'; +import { rolesRouter } from './api/roles/router'; import { apiKeysRouter } from './api/api-keys/router'; import { skillsRouter } from './api/skills/skills'; import { tasksRouter } from './api/tasks/tasks'; @@ -129,6 +130,7 @@ const PROTECTED_MOUNTS: [prefix: string, router: ReturnType // Protected, unlike `/themes` beside it — see api/themes/favorites.ts for why they are separate mounts. ['/theme-favorites', themeFavoritesRouter], ['/users', usersRouter], + ['/roles', rolesRouter], ['/skills', skillsRouter], ['/tasks', tasksRouter], ['/agents', agentsRouter], diff --git a/src/servers/permissions/registry.ts b/src/servers/permissions/registry.ts index 2822c503..239f43fd 100644 --- a/src/servers/permissions/registry.ts +++ b/src/servers/permissions/registry.ts @@ -461,6 +461,22 @@ const CORE_REGISTRY: Permission[] = [ // `headscale` lived here until 2026-08-15, when it left with the rest of offscale. A plugin declares // its own permissions in its manifest and they are registered at install — see plugins/mount.ts. The // platform no longer knows this permission exists, which is the entire point. + { + key: 'roles', + label: 'Roles', + description: 'Create, rename and delete the roles this server grants', + // Admin, and for the same reason `user-admin` beside it is: a role is not a member's own data, it is + // policy that governs every account holding it, so there is no level of "read" that makes creating or + // deleting one a feature to grant. `registry.test.ts` asserts admin can never be handed out — this + // entry is what makes that assertion mean something for roles specifically. + // + // Separate from `user-admin` rather than folded into it: that permission's `/users` is the account + // list and the grants a role holds (`/users/permissions`); this one is the roles TABLE itself — the + // name, the description, whether it is system-named. Two routers, two questions, so a boot-time check + // that a mounted prefix has exactly one owner stays meaningful for both. + kind: 'admin', + api: ['/roles'], + }, ]; // ── Derived lookups ───────────────────────────────────────────────────────────────────────────────