diff --git a/scripts/seed-roles.ts b/scripts/seed-roles.ts index f459b087..5739aaa6 100644 --- a/scripts/seed-roles.ts +++ b/scripts/seed-roles.ts @@ -7,31 +7,45 @@ * * ── Why these four are `isSystem` ── * - * Each is named by name somewhere in the code: `rolePermitsDevTools()` tests for `'Developer'`, - * `bootstrap.ts` writes `'Super Admin'`, `users.role` defaults to `'Member'`, and `'Admin'` is the one - * `rolePermitsDevTools` deliberately excludes. Renaming or deleting one of those is not a data change, it - * is a code change that has not happened — so the flag exists for the delete and rename paths to refuse on, - * rather than for the failure to turn up later as a missing database role or an account that cannot log in. + * Each is named by name somewhere in the code: this file matches `'Developer'`, `'Admin'` and `'Member'` + * by name on every run, and `bootstrap.ts` writes `'Super Admin'`. Renaming or deleting one of those is not + * a data change, it is a code change that has not happened — so the flag exists for the delete and rename + * paths to refuse on, rather than for the failure to turn up later as a missing database role or an + * account that cannot log in. + * + * ── `description` versus `grantsPostgres`/`grantsDocker` ── + * + * `seedRole` treats these two kinds of column differently, and it matters here specifically because this + * is the one caller that seeds `isSystem: true` rows. `description` is written once, on insert, and never + * touched again — an edit made on one server must not be reverted by the next deploy. `grantsPostgres` and + * `grantsDocker` ARE re-asserted every run for a system role, because they are not the operator's data, + * they are what "Developer" MEANS — the same standing as the name itself. `rolePermitsDevTools()` used to + * be that declaration, hardcoded as `role === 'Developer'`; this table is where it lives now, and this + * SEED array is the only place either flag is set for these four roles. See `seedRole`'s own comment in + * `officer_db/src/roles/queries.ts` for the mechanics. * * ── What this deliberately does NOT do ── * - * No foreign key from `users.role`. That change cannot go through `db:push`: push creates the table empty - * and adds the constraint in the same run, so every existing account violates it — verified against a - * scratch database on 2026-08-23, where push reported the violation, created the table, skipped the - * constraint, and exited 0. The FK is a separate release, once every host holds these rows. - * - * So today `users.role` is still a plain text column and nothing enforces that its value is a real role. - * That is the intended state of this phase: additive, unable to fail, and safe to run on five hosts - * unattended. - * - * Never overwrites. A description edited on one server must not be reverted by the next deploy. + * Never overwrite `description`, as above. And never touch a CUSTOM role's grants — those are set through + * `PATCH /api/roles/:name/grants` (`setRoleGrants`), which itself refuses to touch a system role for the + * mirror-image reason: writing there would look like it worked and then silently revert on the next run of + * this script. */ import { seedRole, waitForDatabase } from '../src/databases/officer_db/src/index'; -const SEED: { name: string; description: string }[] = [ +type SeedRoleEntry = { name: string; description: string; grantsPostgres?: boolean; grantsDocker?: boolean }; + +// Match today's behaviour exactly: only Developer passed `rolePermitsDevTools` before this table existed, +// so Developer is the only role seeded with either flag true. +const SEED: SeedRoleEntry[] = [ { name: 'Super Admin', description: 'Full authority over the platform, and an OS account with sudo' }, { name: 'Admin', description: 'Administers the platform. No operating-system privilege' }, - { name: 'Developer', description: 'A member who is also trusted with developer tooling on the machine' }, + { + name: 'Developer', + description: 'A member who is also trusted with developer tooling on the machine', + grantsPostgres: true, + grantsDocker: true, + }, { name: 'Member', description: 'The default. Reaches only what their role has been granted' }, ]; diff --git a/src/databases/CLAUDE.md b/src/databases/CLAUDE.md index d9c1e30f..55b559ce 100644 --- a/src/databases/CLAUDE.md +++ b/src/databases/CLAUDE.md @@ -1,5 +1,34 @@ # Database Patterns +## Dropping and adding a column in the same push makes drizzle ASK + +`drizzle-kit push` cannot tell a drop-plus-add from a rename, so it stops and asks: + +``` +Is grants_postgres column in roles table created or renamed from another column? +❯ + grants_postgres create column + ~ provisioning_script › grants_postgres rename column +``` + +**Non-interactively that is fatal and silent.** With stdin closed it bails, `bun db:push` still exits 0, and +the `postdb:push` seed then runs against a schema that never changed — which is how you get a confusing +error about a column that does not exist, several steps from the thing that failed. + +Hit on 2026-08-23 replacing `roles.provisioning_script` with `grants_postgres`/`grants_docker`. It is the +same family as the composite-unique truncate prompt below, and it matters more on a deploy: no agent driving +a rollout over SSH has a terminal to answer with. + +**So a release whose schema drops a column carries the SQL in its plan**, guarded and idempotent, and treats +`bun db:push` as the check that it worked rather than the thing that does it: + +```sql +ALTER TABLE roles ADD COLUMN IF NOT EXISTS grants_postgres boolean NOT NULL DEFAULT false; +ALTER TABLE roles ADD COLUMN IF NOT EXISTS grants_docker boolean NOT NULL DEFAULT false; +ALTER TABLE roles DROP COLUMN IF EXISTS provisioning_script; +``` + +Then push, and expect zero DDL. If it still wants to change something, the SQL was wrong. + ## Overview **One PostgreSQL database**, `officer_db`, exposed as the workspace package `officerdb`. Managed with diff --git a/src/databases/officer_db/src/roles/index.ts b/src/databases/officer_db/src/roles/index.ts index e0fc24ba..3c493d82 100644 --- a/src/databases/officer_db/src/roles/index.ts +++ b/src/databases/officer_db/src/roles/index.ts @@ -6,8 +6,10 @@ export { createRole, renameRole, deleteRole, + setRoleGrants, type RoleRow, type CreateRoleResult, type RenameRoleResult, type DeleteRoleResult, + type SetRoleGrantsResult, } from './queries'; diff --git a/src/databases/officer_db/src/roles/queries.test.ts b/src/databases/officer_db/src/roles/queries.test.ts new file mode 100644 index 00000000..785e4f41 --- /dev/null +++ b/src/databases/officer_db/src/roles/queries.test.ts @@ -0,0 +1,131 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { eq, like } from 'drizzle-orm'; +import { db } from '../db'; +import { roles } from './schema'; +import { createRole, getRole, seedRole, setRoleGrants } from './queries'; + +/* + Phase 3 of docs/roles-and-teams.md: `grantsPostgres`/`grantsDocker` replace `rolePermitsDevTools()`'s + hardcoded `role === 'Developer'`. These tests hit the real database, like chat-session-cleanup.test.ts — + the thing being pinned is column plumbing (does the value survive a write and a read back), which a fake + store would pass regardless of whether the SQL is right. + + Every role name here is prefixed `zz-test-role-`, cleaned up in `afterEach` by prefix rather than by an + exact list, so a test that fails before reaching its own cleanup does not leave a row for the next run to + trip over. +*/ + +const PREFIX = 'zz-test-role-'; +const name = (suffix: string) => `${PREFIX}${suffix}`; + +afterEach(async () => { + await db.delete(roles).where(like(roles.name, `${PREFIX}%`)); +}); + +describe('grants round trip through create and read', () => { + test('a role created with both flags true reads back with both true', async () => { + const created = await createRole({ name: name('both'), grantsPostgres: true, grantsDocker: true }); + expect(created.ok).toBe(true); + if (!created.ok) return; + expect(created.role.grantsPostgres).toBe(true); + expect(created.role.grantsDocker).toBe(true); + + const read = await getRole(name('both')); + expect(read?.grantsPostgres).toBe(true); + expect(read?.grantsDocker).toBe(true); + }); + + test('omitted flags default to false, matching the column default', async () => { + const created = await createRole({ name: name('bare') }); + expect(created.ok).toBe(true); + if (!created.ok) return; + expect(created.role.grantsPostgres).toBe(false); + expect(created.role.grantsDocker).toBe(false); + }); + + test('a role with grantsDocker false does not carry it after a Postgres-only grant', async () => { + const created = await createRole({ name: name('pg-only'), grantsPostgres: true }); + expect(created.ok).toBe(true); + if (!created.ok) return; + expect(created.role.grantsPostgres).toBe(true); + expect(created.role.grantsDocker).toBe(false); + }); +}); + +describe('setRoleGrants', () => { + test('changes only the flags supplied, leaving the other as it was', async () => { + const created = await createRole({ name: name('partial'), grantsPostgres: true, grantsDocker: true }); + expect(created.ok).toBe(true); + + const updated = await setRoleGrants(name('partial'), { grantsDocker: false }); + expect(updated).toEqual({ + ok: true, + role: expect.objectContaining({ grantsPostgres: true, grantsDocker: false }), + }); + }); + + test('refuses a system role, so an edit cannot look like it worked and then revert on the next deploy', async () => { + // isSystem is not settable through createRole, so insert the row directly — this is standing in for + // one of the four seeded rows, which this function must never touch. + await db.insert(roles).values({ name: name('system'), isSystem: true, grantsPostgres: false }); + + const result = await setRoleGrants(name('system'), { grantsPostgres: true }); + expect(result).toEqual({ ok: false, reason: 'system' }); + + // And the value is genuinely unchanged, not just the result code. + expect((await getRole(name('system')))?.grantsPostgres).toBe(false); + }); + + test('reports not-found for a role that does not exist', async () => { + expect(await setRoleGrants(name('missing'), { grantsPostgres: true })).toEqual({ + ok: false, + reason: 'not-found', + }); + }); +}); + +describe('seedRole', () => { + test('is idempotent: seeding the same system role twice does not duplicate or error', async () => { + const first = await seedRole({ name: name('seed'), isSystem: true, grantsPostgres: true }); + const second = await seedRole({ name: name('seed'), isSystem: true, grantsPostgres: true }); + expect(first).toBe(true); // created + expect(second).toBe(false); // already present + + const rows = await db.select().from(roles).where(eq(roles.name, name('seed'))); + expect(rows).toHaveLength(1); + }); + + test('corrects a system role whose grants were wrong, without touching a non-system role the same way', async () => { + await seedRole({ name: name('correct-sys'), isSystem: true, grantsPostgres: true, grantsDocker: true }); + // Simulate the migration case: a `db:push` column-add left the row at the boolean default. + await db.update(roles).set({ grantsPostgres: false, grantsDocker: false }).where(eq(roles.name, name('correct-sys'))); + expect(await getRole(name('correct-sys'))).toMatchObject({ grantsPostgres: false, grantsDocker: false }); + + // Re-running the same seed call is the fix — no separate migration script. + await seedRole({ name: name('correct-sys'), isSystem: true, grantsPostgres: true, grantsDocker: true }); + expect(await getRole(name('correct-sys'))).toMatchObject({ grantsPostgres: true, grantsDocker: true }); + }); + + test('never rewrites description once a row exists, only grants', async () => { + await seedRole({ name: name('desc'), description: 'original', isSystem: true, grantsPostgres: false }); + // An operator's edit, which the next deploy's seed call must not revert. + await db.update(roles).set({ description: 'edited by an operator' }).where(eq(roles.name, name('desc'))); + + await seedRole({ name: name('desc'), description: 'original', isSystem: true, grantsPostgres: true }); + + const row = await getRole(name('desc')); + expect(row?.description).toBe('edited by an operator'); + expect(row?.grantsPostgres).toBe(true); + }); + + test('does not touch a non-system role at all, even if seeded once as one', async () => { + // A non-system row's grants belong to whoever created it; seedRole's correction pass only applies + // when `isSystem` is true in the call. + const created = await createRole({ name: name('custom'), grantsPostgres: true }); + expect(created.ok).toBe(true); + + await seedRole({ name: name('custom'), isSystem: false, grantsPostgres: false }); + + expect((await getRole(name('custom')))?.grantsPostgres).toBe(true); + }); +}); diff --git a/src/databases/officer_db/src/roles/queries.ts b/src/databases/officer_db/src/roles/queries.ts index 9aea2f6a..e05893be 100644 --- a/src/databases/officer_db/src/roles/queries.ts +++ b/src/databases/officer_db/src/roles/queries.ts @@ -7,7 +7,8 @@ export type RoleRow = { name: string; description: string | null; isSystem: boolean; - provisioningScript: string | null; + grantsPostgres: boolean; + grantsDocker: boolean; createdAt: Date; }; @@ -24,13 +25,14 @@ export async function listRoles(): Promise<(RoleRow & { userCount: number })[]> name: roles.name, description: roles.description, isSystem: roles.isSystem, - provisioningScript: roles.provisioningScript, + grantsPostgres: roles.grantsPostgres, + grantsDocker: roles.grantsDocker, createdAt: roles.createdAt, userCount: sql`count(${users.id})::int`, }) .from(roles) .leftJoin(users, eq(users.role, roles.name)) - .groupBy(roles.name, roles.description, roles.isSystem, roles.provisioningScript, roles.createdAt) + .groupBy(roles.name, roles.description, roles.isSystem, roles.grantsPostgres, roles.grantsDocker, roles.createdAt) .orderBy(asc(roles.name)); return rows; } @@ -41,22 +43,48 @@ export async function getRole(name: string): Promise { } /** - * Seed a role if it is not there. Idempotent, and it does NOT overwrite an existing row. + * Seed a role if it is not there. Idempotent, but NOT symmetric across columns. * * Called from `postdb:push` on every machine, so it runs again on every schema push for the rest of this - * platform's life. An upsert would mean a description edited on one server is silently reverted by the next - * deploy — so the seed establishes a row and never touches it again. + * platform's life. `description` is never rewritten once a row exists: an upsert there would mean a + * description edited on one server is silently reverted by the next deploy — so that column is set on + * insert and never touched again. + * + * `grantsPostgres`/`grantsDocker` are the opposite, and only for a role seeded with `isSystem: true`: they + * ARE re-asserted on every run, because for a system role they are not an operator's data, they are the + * platform's own declaration of what the role means — the same standing as its name. Two things need that: + * + * - A row can predate these columns. `db:push` adds them with `default false`, so an existing `Developer` + * row gets `grantsPostgres: false, grantsDocker: false` the moment the column lands — an insert-only + * seed would leave it there forever and silently take away dev tooling nobody asked to revoke. + * - It is also what makes the guard in `setRoleGrants` (below) honest. That function refuses to touch a + * system role's grants specifically so an edit cannot LOOK like it worked and then vanish on the next + * deploy; this is the other half of that promise — the seed is the only writer, and it always writes + * the same thing. + * + * A non-system role's grants are never touched here — those belong to whoever created the role, through + * `setRoleGrants`, and this function has no opinion about a row it did not seed. */ export async function seedRole(row: { name: string; description?: string | null; isSystem?: boolean; + grantsPostgres?: boolean; + grantsDocker?: boolean; }): Promise { + const isSystem = row.isSystem ?? false; + const grantsPostgres = row.grantsPostgres ?? false; + const grantsDocker = row.grantsDocker ?? false; + const inserted = await db .insert(roles) - .values({ name: row.name, description: row.description ?? null, isSystem: row.isSystem ?? false }) + .values({ name: row.name, description: row.description ?? null, isSystem, grantsPostgres, grantsDocker }) .onConflictDoNothing({ target: roles.name }) .returning({ name: roles.name }); + + if (inserted.length === 0 && isSystem) { + await db.update(roles).set({ grantsPostgres, grantsDocker }).where(eq(roles.name, row.name)); + } return inserted.length > 0; } @@ -83,14 +111,30 @@ export type CreateRoleResult = { ok: true; role: RoleRow } | { ok: false; reason * 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. + * + * `grantsPostgres`/`grantsDocker` default to false, same as the column defaults — a role created through a + * form should not hand out a Postgres login and a Docker daemon because nobody unchecked a box. There is + * no `isSystem` guard needed on the write itself: a role created here is never system, so nothing later + * pins these two values the way `seedRole` pins Developer's. */ -export async function createRole(input: { name: string; description?: string | null }): Promise { +export async function createRole(input: { + name: string; + description?: string | null; + grantsPostgres?: boolean; + grantsDocker?: boolean; +}): 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 }) + .values({ + name, + description: input.description?.trim() || null, + isSystem: false, + grantsPostgres: input.grantsPostgres ?? false, + grantsDocker: input.grantsDocker ?? false, + }) .onConflictDoNothing({ target: roles.name }) .returning(); const role = inserted[0]; @@ -98,15 +142,52 @@ export async function createRole(input: { name: string; description?: string | n return { ok: true, role }; } +export type SetRoleGrantsResult = { ok: true; role: RoleRow } | { ok: false; reason: 'not-found' | 'system' }; + +/** + * Change a role's provisioning flags after it has been created. + * + * Refuses on `isSystem`, and for a different reason than `renameRole`'s refusal: renaming protects the + * NAME code depends on, this protects a PROMISE. `seedRole` re-asserts a system role's grants on every + * `postdb:push`, so writing here would look like it worked and then silently revert on the next deploy — + * exactly the "says something untrue" trap `manage-users.ts` calls out for the Postgres-follows-role case. + * A custom role has no seed pinning it, so this is the only place its grants are ever set after creation. + * + * Only the two flags supplied are changed; the other keeps its current value, so a caller adjusting just + * `grantsDocker` cannot accidentally reset `grantsPostgres` to false. + */ +export async function setRoleGrants( + name: string, + grants: { grantsPostgres?: boolean; grantsDocker?: boolean }, +): Promise { + const existing = await getRole(name); + if (!existing) return { ok: false, reason: 'not-found' }; + if (existing.isSystem) return { ok: false, reason: 'system' }; + + // Same re-check as renameRole/deleteRole, same reason: the row must not have become system between the + // read above and this write. + const [role] = await db + .update(roles) + .set({ + grantsPostgres: grants.grantsPostgres ?? existing.grantsPostgres, + grantsDocker: grants.grantsDocker ?? existing.grantsDocker, + }) + .where(and(eq(roles.name, name), eq(roles.isSystem, false))) + .returning(); + if (!role) return { ok: false, reason: 'system' }; + return { ok: true, role }; +} + export type RenameRoleResult = | { ok: true; role: RoleRow } | { ok: false; reason: 'not-found' | 'system' | 'invalid-name' | 'duplicate' }; /** - * 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. + * Rename a role. Refuses outright on `isSystem` (D4 in docs/roles-and-teams.md): `scripts/seed-roles.ts` + * matches its four rows by name on every deploy, `bootstrap.ts` writes `'Super Admin'`, and `users.role` + * defaults to `'Member'`. Renaming one of those four is a code change across those call sites that has not + * happened, not a data edit — a rename would leave the seed re-creating the old name from under it rather + * than correcting the row that moved. * * `users.role` and `role_permissions.role` are foreign keys onto this table with `ON UPDATE CASCADE` * (phase 2 part 2), so the `UPDATE` below moves every holder and every grant automatically, in the same diff --git a/src/databases/officer_db/src/roles/schema.ts b/src/databases/officer_db/src/roles/schema.ts index 44be6665..a206df52 100644 --- a/src/databases/officer_db/src/roles/schema.ts +++ b/src/databases/officer_db/src/roles/schema.ts @@ -13,7 +13,7 @@ import { pgTable, text, boolean, timestamp } from 'drizzle-orm/pg-core'; * changes. * * The cost is that a rename is a data migration rather than a label edit. That is honest — a role name - * appears in `rolePermitsDevTools`, in report text and in whatever a plugin has written down. + * appears in `scripts/seed-roles.ts`'s seed table, in report text and in whatever a plugin has written down. * * ── What is NOT here ── * @@ -33,20 +33,30 @@ export const roles = pgTable('roles', { /** * A role the platform's own code names, so it cannot be deleted or renamed. * - * `rolePermitsDevTools()` tests for `'Developer'` by name, `bootstrap.ts` sets `'Super Admin'`, and the - * users table defaults to `'Member'`. Deleting or renaming one of those is not a data change, it is a - * code change that has not happened yet — so the delete and rename paths refuse on this flag rather than - * discovering it as a runtime failure somewhere else. + * `scripts/seed-roles.ts` matches `'Developer'`, `'Admin'` and `'Member'` by name on every deploy, + * `bootstrap.ts` sets `'Super Admin'`, and the users table defaults to `'Member'`. Deleting or renaming + * one of those is not a data change, it is a code change that has not happened yet — so the delete and + * rename paths refuse on this flag rather than discovering it as a runtime failure somewhere else. */ isSystem: boolean('is_system').notNull().default(false), /** - * Reserved for the per-role provisioning that comes next: a script run when an account is given this - * role, so "Developer gets rootless Docker" stops being a special case in `os-user-postgres.ts` and - * becomes a row. Null today, and nothing reads it. + * Per-role provisioning, as a closed vocabulary rather than a script. * - * Declared now rather than later on purpose — it is the reason the owner wanted a table at all, and a - * column that arrives with the feature that uses it is a second migration for no gain. + * D8 in docs/roles-and-teams.md proposed a `provisioningScript` column: a shell script stored on the row + * and run when an account is given the role. That was rejected before it was built — it is arbitrary + * root-level code living in a database row, and D8's own warning explains why that is worse than it + * looks: `usermod -aG docker` is root by a longer route + * (`docker run -v /:/host -it alpine chroot /host` reads `.env`, every member's home and the wallet + * seed). A role editable at runtime must not be a way to grant root by writing a string into a form. + * + * So the row CHOOSES from options the platform code already knows how to carry out safely, the same + * shape as `multiarch` on a plugin's `osDependencies` (`src/servers/plugins/manifest.ts`): a fixed list + * instead of a free string, so the manifest picks rather than supplies. `os-user-postgres.ts` and + * `os-user-docker.ts` still own how each is provisioned; these columns only say whether a given role's + * accounts get it. */ - provisioningScript: text('provisioning_script'), + grantsPostgres: boolean('grants_postgres').notNull().default(false), + /** A rootless Docker daemon. See `grantsPostgres` above and `os-user-docker.ts` for what it costs per member. */ + grantsDocker: boolean('grants_docker').notNull().default(false), createdAt: timestamp('created_at', { withTimezone: true }).notNull().defaultNow(), }); diff --git a/src/servers/api/roles/router.ts b/src/servers/api/roles/router.ts index 9c3a3368..27b7e283 100644 --- a/src/servers/api/roles/router.ts +++ b/src/servers/api/roles/router.ts @@ -1,16 +1,18 @@ import { createRouter } from '../../create-router'; import * as errors from '@@/custom-errors'; import { isSuperAdmin } from '../../super-admin'; -import { listRoles, createRole, renameRole, deleteRole } from 'officerdb'; +import { listRoles, createRole, renameRole, deleteRole, setRoleGrants } from 'officerdb'; /* - /api/roles — role CRUD (phase 2 part 3 of docs/roles-and-teams.md). + /api/roles — role CRUD (phase 2 part 3 of docs/roles-and-teams.md), plus grants (phase 3). - 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". + This is the roles TABLE — a name, a description, the `isSystem` flag, and since phase 3 the two + provisioning flags (`grantsPostgres`, `grantsDocker`). It is not the PERMISSION grants a role holds; + those stay `role_permissions`, administered at `/api/users/permissions` by `permissionAdminRouter`. The + two are separate screens for a reason: this router answers "which roles exist, and what does holding one + provision on the machine", that one answers "what may each reach in the app" — 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 ── @@ -37,13 +39,32 @@ rolesRouter.use(async (ctx, 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`. */ +/** + * POST /api/roles — create a role. Always non-system; see the schema comment on `isSystem`. + * + * `grantsPostgres`/`grantsDocker` default to false when omitted, matching the column defaults — creating + * a role must not hand out a Postgres login and a Docker daemon because a form field was left out of the + * request body. + */ rolesRouter.post('/', async (ctx) => { - const body = ctx.get('body') as { name?: unknown; description?: unknown }; + const body = ctx.get('body') as { + name?: unknown; + description?: unknown; + grantsPostgres?: unknown; + grantsDocker?: unknown; + }; if (typeof body?.name !== 'string') throw errors.BAD_REQUEST('name is required'); const description = typeof body.description === 'string' ? body.description : null; + if (body.grantsPostgres !== undefined && typeof body.grantsPostgres !== 'boolean') { + throw errors.BAD_REQUEST('grantsPostgres must be a boolean'); + } + if (body.grantsDocker !== undefined && typeof body.grantsDocker !== 'boolean') { + throw errors.BAD_REQUEST('grantsDocker must be a boolean'); + } + const grantsPostgres = typeof body.grantsPostgres === 'boolean' ? body.grantsPostgres : undefined; + const grantsDocker = typeof body.grantsDocker === 'boolean' ? body.grantsDocker : undefined; - const result = await createRole({ name: body.name, description }); + const result = await createRole({ name: body.name, description, grantsPostgres, grantsDocker }); if (!result.ok) { throw result.reason === 'invalid-name' ? errors.BAD_REQUEST('a role name cannot be empty') @@ -80,6 +101,41 @@ rolesRouter.patch('/:name', async (ctx) => { return ctx.json({ role: result.role }); }); +/** + * PATCH /api/roles/:name/grants — set `grantsPostgres`/`grantsDocker`. Refuses a system role, for a + * different reason than the rename route above: `scripts/seed-roles.ts` re-asserts a system role's grants + * on every `postdb:push`, so a write here would look like it worked and then silently revert on the next + * deploy. A custom role has no such seed pinning it, so this is where its grants actually get set once it + * exists — `createRole` only takes an initial value. + * + * Either field may be omitted; only the ones present are changed (see `setRoleGrants`). + */ +rolesRouter.patch('/:name/grants', async (ctx) => { + const name = ctx.req.param('name'); + const body = ctx.get('body') as { grantsPostgres?: unknown; grantsDocker?: unknown }; + if (body?.grantsPostgres === undefined && body?.grantsDocker === undefined) { + throw errors.BAD_REQUEST('grantsPostgres and/or grantsDocker is required'); + } + if (body.grantsPostgres !== undefined && typeof body.grantsPostgres !== 'boolean') { + throw errors.BAD_REQUEST('grantsPostgres must be a boolean'); + } + if (body.grantsDocker !== undefined && typeof body.grantsDocker !== 'boolean') { + throw errors.BAD_REQUEST('grantsDocker must be a boolean'); + } + const grantsPostgres = typeof body.grantsPostgres === 'boolean' ? body.grantsPostgres : undefined; + const grantsDocker = typeof body.grantsDocker === 'boolean' ? body.grantsDocker : undefined; + + const result = await setRoleGrants(name, { grantsPostgres, grantsDocker }); + if (!result.ok) { + throw result.reason === 'not-found' + ? errors.NOT_FOUND(`no such role '${name}'`) + : errors.FORBIDDEN( + `'${name}' is a system role — its grants are declared in scripts/seed-roles.ts, not edited here`, + ); + } + 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'); diff --git a/src/servers/api/users/manage-users.ts b/src/servers/api/users/manage-users.ts index a9d7f8de..1afbf4b3 100644 --- a/src/servers/api/users/manage-users.ts +++ b/src/servers/api/users/manage-users.ts @@ -2,7 +2,7 @@ import type { Handler } from 'hono'; import { getUsers, getUserById, updateUser, deleteUser, listRoles, getRole, OWNER_USER_ID } from 'officerdb'; import * as errors from '@@/custom-errors'; import { deprovisionOsAccount } from '@@/os-user-deprovision'; -import { dropPostgresRole, provisionPostgresRole, rolePermitsDevTools } from '@@/os-user-postgres'; +import { dropPostgresRole, provisionPostgresRole } from '@@/os-user-postgres'; import { lookupOsUser } from '@@/os-user'; import { DATA_PATH } from '@@/data-path'; import { broadcastProvisionableUsers } from '@@/plugins/user-provisioning'; @@ -71,9 +71,11 @@ export const updateUserRoleHandler: Handler = async function (ctx) { // The runtime replacement for what USER_ROLES used to guard at the type level (docs/roles-and-teams.md // phase 2 part 4): a role that is not a row in `roles` must never reach `updateUser`, or the FK on // `users.role` throws a raw Postgres error instead of this message. - if (typeof role !== 'string' || !(await getRole(role))) { - throw errors.BAD_REQUEST(`Unknown role '${String(role)}'`); - } + if (typeof role !== 'string') throw errors.BAD_REQUEST(`Unknown role '${String(role)}'`); + // Kept rather than re-fetched below: this is the one read of the DESTINATION role's row this handler + // needs, and `wants` reuses it instead of asking the database the same question twice. + const newRole = await getRole(role); + if (!newRole) throw errors.BAD_REQUEST(`Unknown role '${role}'`); // ck_users_owner_is_super_admin would reject this anyway — the point of catching it here is the // message. A raw CHECK violation surfaces as a 500 with Postgres wording, which tells the person @@ -101,7 +103,7 @@ export const updateUserRoleHandler: Handler = async function (ctx) { // ── The database role follows the platform role ── // - // Without this the gate in `rolePermitsDevTools` is decorative: it would decide what a Developer gets at + // Without this, `grantsPostgres` on the role row is decorative: it would decide what a Developer gets at // creation and then never look again, so demoting one would leave their Postgres role, their databases // and a working password in their ~/.zshenv. A permission that survives its own revocation is worse than // not having gated it, because the UI then says something untrue. @@ -109,8 +111,13 @@ export const updateUserRoleHandler: Handler = async function (ctx) { // REVOKE BEFORE RECORDING, grant after. Dropping first means a failure aborts with the role unchanged, so // the account still says Developer and the whole thing can be retried. Doing it the other way round would // leave a Member holding database access with nothing in the row to indicate it. - const had = rolePermitsDevTools(existing.role); - const wants = rolePermitsDevTools(role); + // + // Docker does NOT follow the role here — only Postgres does. That is a pre-existing gap (docker was + // never wired into role change, only into initial provisioning in provision-os.ts), not something this + // change introduces or was asked to close. + const oldRole = await getRole(existing.role); + const had = oldRole?.grantsPostgres ?? false; + const wants = newRole.grantsPostgres; if (had && !wants && existing.osUser) { const dropped = await dropPostgresRole(existing.osUser); diff --git a/src/servers/api/users/provision-os.ts b/src/servers/api/users/provision-os.ts index feb5b630..46603c09 100644 --- a/src/servers/api/users/provision-os.ts +++ b/src/servers/api/users/provision-os.ts @@ -5,7 +5,7 @@ import { checkOsAccount } from '@@/os-user-verify'; import { provisionSshAccess } from '@@/os-user-ssh'; import { seedShellConfig } from '@@/os-user-shell'; import { provisionClaudeCli } from '@@/os-user-claude'; -import { provisionPostgresRole, rolePermitsDevTools } from '@@/os-user-postgres'; +import { provisionPostgresRole, roleDevGrants } from '@@/os-user-postgres'; import { provisionRootlessDocker } from '@@/os-user-docker'; import { provisionUserDirs } from '@@/data-path'; @@ -42,7 +42,7 @@ export async function provisionOsAccount(params: { userId: number; email: string; username: string; - /** Decides whether they get a Postgres role. See `rolePermitsDevTools`. */ + /** Decides whether they get a Postgres role and/or a Docker daemon. See `roleDevGrants`. */ role: UserRole; /** Inbound SSH key for `authorized_keys`. Already validated by the caller. */ inboundKey?: string | null; @@ -92,10 +92,14 @@ export async function provisionOsAccount(params: { // cannot do it for them and must not try, because the alternative is lending them the owner's credential. const claude = await provisionClaudeCli({ email: params.email, osUser: account.osUser }); - // A Postgres login role of the same name, with CREATEDB — for a Developer, and nobody else. What - // replaced rootless Docker for the "let me run a database to develop against" case, and it inherits - // that feature's role gate along with its purpose: `rolePermitsDevTools` is the one place that rule is - // written down. + // One read of the role row for both gates below, rather than one query per tool — `roleDevGrants` + // returns both flags together for exactly this caller. + const devGrants = await roleDevGrants(params.role); + + // A Postgres login role of the same name, with CREATEDB — gated on `grantsPostgres`. What replaced + // rootless Docker for the "let me run a database to develop against" case, and it inherits that + // feature's role gate along with its purpose: `roleDevGrants` is the one place that rule is written + // down, now as a row on the role rather than a name tested in code. // // Also the step that shuts PUBLIC out of the platform's own database — deliberately inside the function // that creates the role rather than in the setup script, so it cannot be skipped by an install that was @@ -103,7 +107,7 @@ export async function provisionOsAccount(params: { // // Null, not a skipped-but-ok result: "there is no database role because of who they are" and "the // database role worked" are different answers and the caller reports them differently. - const postgres = rolePermitsDevTools(params.role) + const postgres = devGrants.grantsPostgres ? await provisionPostgresRole({ email: params.email, osUser: account.osUser, @@ -112,14 +116,14 @@ export async function provisionOsAccount(params: { }) : null; - // Their own rootless Docker daemon — for a Developer, and nobody else. Last, and the most tolerant of - // failure: a host without the uidmap package or a kernel that will not do rootless still gets a perfectly - // good account, minus containers. + // Their own rootless Docker daemon — gated on `grantsDocker`. Last, and the most tolerant of failure: a + // host without the uidmap package or a kernel that will not do rootless still gets a perfectly good + // account, minus containers. // // Behind the role gate rather than unconditional. The cost is real and it is per member, not per install: // one daemon, one image cache and one subuid range each — see os-user-docker.ts. That is worth paying for // someone whose job is to build things here, and not for an account that just reads its dashboards. - const docker = rolePermitsDevTools(params.role) + const docker = devGrants.grantsDocker ? await provisionRootlessDocker({ osUser: account.osUser, uid: account.uid, diff --git a/src/servers/os-user-postgres.test.ts b/src/servers/os-user-postgres.test.ts new file mode 100644 index 00000000..a8f8d62e --- /dev/null +++ b/src/servers/os-user-postgres.test.ts @@ -0,0 +1,50 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { createRole } from 'officerdb'; +import { db } from 'officerdb/db'; +import { roles } from 'officerdb/schema'; +import { like } from 'drizzle-orm'; +import { roleDevGrants } from './os-user-postgres'; + +/* + `roleDevGrants` is the replacement for the hardcoded `role === 'Developer'` gate — see the comment above + it. What matters here is that it reads the ROW, not the name: a role called something else entirely gets + dev tooling if and only if its `grantsPostgres`/`grantsDocker` columns say so, and a role that does not + exist reads as neither rather than throwing. +*/ + +const PREFIX = 'zz-test-devgrants-'; +const name = (suffix: string) => `${PREFIX}${suffix}`; + +afterEach(async () => { + await db.delete(roles).where(like(roles.name, `${PREFIX}%`)); +}); + +describe('roleDevGrants', () => { + test('reads both flags off a role that is not named Developer', async () => { + const created = await createRole({ name: name('custom-dev'), grantsPostgres: true, grantsDocker: true }); + expect(created.ok).toBe(true); + + expect(await roleDevGrants(name('custom-dev'))).toEqual({ grantsPostgres: true, grantsDocker: true }); + }); + + test('a role with grantsDocker false does not get docker, even with grantsPostgres true', async () => { + const created = await createRole({ name: name('pg-only'), grantsPostgres: true, grantsDocker: false }); + expect(created.ok).toBe(true); + + expect(await roleDevGrants(name('pg-only'))).toEqual({ grantsPostgres: true, grantsDocker: false }); + }); + + test('a role granted neither gets neither', async () => { + const created = await createRole({ name: name('plain') }); + expect(created.ok).toBe(true); + + expect(await roleDevGrants(name('plain'))).toEqual({ grantsPostgres: false, grantsDocker: false }); + }); + + test('a role that does not exist reads as neither, matching "absence denies" rather than throwing', async () => { + await expect(roleDevGrants(name('never-created'))).resolves.toEqual({ + grantsPostgres: false, + grantsDocker: false, + }); + }); +}); diff --git a/src/servers/os-user-postgres.ts b/src/servers/os-user-postgres.ts index e1375b41..78866a23 100644 --- a/src/servers/os-user-postgres.ts +++ b/src/servers/os-user-postgres.ts @@ -1,10 +1,10 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import type { UserRole } from 'officerdb'; import { sql } from 'drizzle-orm'; import postgres from 'postgres'; import { db } from 'officerdb/db'; +import { getRole } from 'officerdb'; import { osUserHome, runAs } from './os-user'; // A Postgres login role for a member, named the same as their Linux account. @@ -66,20 +66,34 @@ import { osUserHome, runAs } from './os-user'; // OWNER TO, CREATE OR REPLACE over the top, and `PGOPTIONS=-c event_triggers=off` (that GUC is superuser // only). A superuser can still use it, which is the recovery path if this ever locks something out. +/** Which dev tools a role's accounts get, read off the role row. See `roleDevGrants` for the reasoning. */ +export type DevToolGrants = { grantsPostgres: boolean; grantsDocker: boolean }; + /** - * Which platform roles get a database role at all. + * Which platform roles get a database role, and which get a rootless Docker daemon — read from the + * `roles` table (`grantsPostgres`, `grantsDocker`) instead of tested by name. * - * `Developer` only. This is the gate rootless Docker was under before it was disabled, inherited by the - * thing that replaced it rather than invented for it — a member who was not trusted to run containers is - * not thereby trusted to run databases. + * Historically this was `role === 'Developer'`, a single hardcoded check that gated both. Docker was the + * original gate — Postgres is what replaced rootless Docker per member (disabled 2026-08-13), and it + * inherited Docker's role gate rather than being given a new one: a member who was not trusted to run + * containers is not thereby trusted to run databases. That history is why both flags still default to the + * same seed value for `Developer` (`scripts/seed-roles.ts`), even though the row now lets them diverge for + * a custom role. * - * `Admin` is deliberately NOT included. Administering the platform is not developing on it, and the two - * are separate roles precisely so they can be held separately. Add it here if that turns out to be wrong; - * do not add it at a call site. + * `Admin` is deliberately seeded with both flags false. Administering the platform is not developing on + * it, and the two are separate roles precisely so they can be held separately — see the seed for where + * that is now decided, since there is no longer a call site to add it at. * - * The owner never reaches this — they run as the service account and already have the cluster. + * The owner never reaches this — they run as the service account and already have the cluster and a + * regular Docker install. + * + * A role that no longer exists (renamed out from under a stale reference, or deleted) reads as neither + * grant, matching "absence denies" everywhere else in this system rather than throwing. */ -export const rolePermitsDevTools = (role: UserRole): boolean => role === 'Developer'; +export async function roleDevGrants(role: string): Promise { + const row = await getRole(role); + return { grantsPostgres: row?.grantsPostgres ?? false, grantsDocker: row?.grantsDocker ?? false }; +} /** Legal Linux/Postgres account name. Identical to `validateUsername`, restated because this one reaches SQL. */ const ROLE_NAME_RE = /^[a-zA-Z0-9._-]{2,32}$/;