From 69a31051ac017d2c839d7d4f259202eb38701f17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 11 Aug 2026 15:15:09 +0000 Subject: [PATCH] the owner can create accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/users plus an Add-account form in Settings > User management. Until now createUser had one call site — bootstrap, gated on an empty user table — so every non-owner account anywhere had been inserted into Postgres by hand. Created accounts are Active. The column defaults to Unverified and signin refuses anything else with a bare UNAUTHORIZED, which is exactly what made the hand-INSERT route look like a wrong password. Also closes a hole found while reading the write path: a second Super Admin was storable. The CHECK constraint pins user 1's role but cannot see other rows, and getOwnerUser() was LIMIT 1 with no ORDER BY, so two holders would have made "who owns this server" a question the query plan answered — and that answer feeds the agent sidecar's identity, vault access and origin scoping. Both write paths now refuse the role and getOwnerUser() orders by id. USER_DIRS and provisionUserDirs move into data-path.ts so the create handler and scripts/provision-user-dirs.ts cannot disagree about what an account's skeleton is. Co-Authored-By: Claude Opus 5 --- TODO.md | 24 +- scripts/provision-user-dirs.ts | 24 +- .../UserManagement/CreateUserForm.tsx | 214 ++++++++++++++++++ .../Settings/UserManagement/UsersSection.tsx | 18 +- src/databases/officer_db/src/index.ts | 1 + src/databases/officer_db/src/queries/auth.ts | 17 +- src/servers/api/users/create-user.ts | 89 ++++++++ src/servers/api/users/manage-users.ts | 15 +- src/servers/api/users/users-router.ts | 4 + src/servers/data-path.ts | 31 +++ 10 files changed, 408 insertions(+), 29 deletions(-) create mode 100644 src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/CreateUserForm.tsx create mode 100644 src/servers/api/users/create-user.ts diff --git a/TODO.md b/TODO.md index fd3d844c..2036921e 100644 --- a/TODO.md +++ b/TODO.md @@ -14,10 +14,26 @@ the owner's OS user and can never be granted. Indirection there really is accide ## Multi-user -- [ ] **No way to create a second account.** `createUser` has one call site, `auth/bootstrap.ts`, gated - on an empty user table. There is no signup route, no invite flow and no admin create-user - handler, so every member on this instance was inserted into Postgres by hand. This is the - blocker for onboarding anyone who is not already in the database. +- [x] **No way to create a second account.** Fixed 2026-08-11 on `sidecar-app-store`: `POST /api/users` + (`api/users/create-user.ts`, owner-gated) plus an Add-account form in + Settings → User management. Created accounts are `status: 'Active'` — the column defaults to + `'Unverified'` and `signin.ts` refuses anything else with a bare UNAUTHORIZED, which is the trap + the hand-INSERT route fell into. The owner sets the password and reads it out; `passwordChangedAt` + stays null. Directories come from the shared `provisionUserDirs`/`USER_DIRS` in `data-path.ts`, + which `scripts/provision-user-dirs.ts` now imports rather than restating. + +- [ ] **Still no invite flow, and no password reset for a member.** The owner types the password and + tells the person, which means the owner knows it and the member cannot change it back if they + forget theirs — recovery today is delete-and-recreate. An invite (token, expiry, member sets + their own) needs a mail path. This is the next piece, not a nice-to-have. + +- [x] **A second Super Admin was storable, and made the owner nondeterministic.** Fixed 2026-08-11. + `ck_users_owner_is_super_admin` pins user 1's role but a row-level CHECK cannot see other rows, and + `updateUserRoleHandler` happily promoted anyone — while `getOwnerUser()` was + `WHERE role='Super Admin' LIMIT 1` with no ORDER BY. Two holders would have made "who owns this + server" a question the query plan answered, and that answer feeds the agent sidecar's identity, + vault access and origin scoping. Both write paths now refuse the role, the list endpoint offers + `assignableRoles` without it, and `getOwnerUser()` orders by id. - [ ] **`dashboards.id` is a global primary key, and ids are `slugify(name)`.** Two accounts cannot both have a dashboard named "Home". Reachable today: six accounts exist. The recommendation on diff --git a/scripts/provision-user-dirs.ts b/scripts/provision-user-dirs.ts index 403a69f9..0b576fa9 100644 --- a/scripts/provision-user-dirs.ts +++ b/scripts/provision-user-dirs.ts @@ -16,26 +16,10 @@ import { mkdirSync, existsSync } from 'node:fs'; import { join } from 'node:path'; - -const DATA_PATH = process.env.DATA_PATH ?? join(process.cwd(), 'data'); - -// Mirrors the shape the owner's root grew organically. Most of these are also created on demand by -// whichever feature owns them (attachments, dashboards, email_accounts…), so pre-creating them buys -// legibility more than function — the tree shows what a user has without having to use it first. -// -// `home` is the exception, and the reason this exists at all: nothing creates it today. getOwnerHomeDir -// returns process.env.HOME_DIR whenever it is set, which it always is on a real install, so the -// per-user home has never actually been reached. It is where a non-owner's sessions will run. -const USER_DIRS = [ - 'home', - 'attachments', - 'cache', - 'dashboards', - 'email_accounts', - 'general_chat_sessions', - 'logs', - 'sidecar', -] as const; +// The list and DATA_PATH itself come from the platform rather than being restated here. The owner's +// create-account handler provisions the same skeleton, and a script that drifted from it would produce +// accounts that differ by how they were made. Importing data-path.ts pulls in no database and no server. +import { DATA_PATH, USER_DIRS } from '../src/servers/data-path'; const DRY_RUN = process.env.DRY_RUN === '1'; const emails = process.argv.slice(2).filter(Boolean); diff --git a/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/CreateUserForm.tsx b/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/CreateUserForm.tsx new file mode 100644 index 00000000..e0e1b79c --- /dev/null +++ b/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/CreateUserForm.tsx @@ -0,0 +1,214 @@ +import { useState } from 'react'; +import { useQueryClient } from '@tanstack/react-query'; +import { toast } from 'sonner'; +import { Loader2, UserPlus, Dices, Copy, X } from 'lucide-react'; +import { useClient } from 'hooks/useClient'; +import { Button } from '@/components/ui/button'; +import { Input } from '@/components/ui/input'; +import { Label } from '@/components/ui/label'; +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select'; + +// The owner creating an account. Until this existed the only way to add one was an INSERT in Postgres. +// +// The password is visible, not masked, and that is the point: the owner has to be able to read it back +// to the person they are creating it for. Masking a value nobody has yet only makes it easy to typo +// twice. When there is an invite flow this whole field goes away. + +type CreateUserFormProps = { + /** Roles the server will actually accept. Excludes the owner role — see manage-users.ts. */ + roles: string[]; + /** Invalidated on success so the list below refreshes. */ + usersKey: readonly unknown[]; +}; + +// Mirrors validatePassword on the server: length, both cases, a digit and a symbol. Generated rather +// than demanded so the owner is not sitting there inventing one that passes. +function generatePassword(): string { + const lower = 'abcdefghijkmnopqrstuvwxyz'; + const upper = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; + const digits = '23456789'; + const symbols = '!@#$%^&*-_=+'; + const all = lower + upper + digits + symbols; + + const pick = (set: string, count: number) => + Array.from(crypto.getRandomValues(new Uint32Array(count)), (n) => set[n % set.length]!); + + // One of each class first, so the result cannot fail the server's rules by chance, then filled out. + const chars = [...pick(lower, 4), ...pick(upper, 3), ...pick(digits, 3), ...pick(symbols, 2), ...pick(all, 8)]; + + // Shuffled so the classes are not in fixed positions. Fisher-Yates with crypto randomness. + const noise = crypto.getRandomValues(new Uint32Array(chars.length)); + for (let i = chars.length - 1; i > 0; i--) { + const j = noise[i]! % (i + 1); + [chars[i], chars[j]] = [chars[j]!, chars[i]!]; + } + return chars.join(''); +} + +const EMPTY = { email: '', name: '', username: '', password: '', role: 'Member' }; + +export const CreateUserForm = ({ roles, usersKey }: CreateUserFormProps) => { + const client = useClient(); + const queryClient = useQueryClient(); + const [open, setOpen] = useState(false); + const [saving, setSaving] = useState(false); + const [form, setForm] = useState(EMPTY); + + const set = (key: keyof typeof EMPTY) => (value: string) => setForm((prev) => ({ ...prev, [key]: value })); + + const close = () => { + setOpen(false); + setForm(EMPTY); + }; + + const submit = async (ev: React.FormEvent) => { + ev.preventDefault(); + setSaving(true); + try { + await client.post('/users', form); + await queryClient.invalidateQueries({ queryKey: usersKey }); + // The password is named in the toast on purpose. It is the only moment it is recoverable — the + // server stores an argon2 hash and there is no reset flow yet, so an owner who closed the form + // without noting it would have to delete the account and make it again. + toast.success(`${form.email} created`, { + description: `Password: ${form.password}`, + duration: 30_000, + }); + close(); + } catch (ex) { + // The server's message is the useful one here — which field, and why. + toast.error(ex instanceof Error ? ex.message : 'Could not create the account'); + } finally { + setSaving(false); + } + }; + + if (!open) { + return ( + + ); + } + + return ( +
+
+
+

New account

+

+ Created active — they can sign in straight away. Tell them the password; it is not recoverable afterwards. +

+
+ +
+ +
+
+ + set('email')(ev.target.value)} + required + /> +
+ +
+ + set('name')(ev.target.value)} + required + /> +
+ +
+ + set('username')(ev.target.value)} + required + /> +

Letters, numbers, dots, hyphens and underscores.

+
+ +
+ + +

+ What the role may reach is set under Permissions. A role with no grants can sign in and reach nothing but + its own profile. +

+
+ +
+ +
+ set('password')(ev.target.value)} + required + /> + + +
+

+ At least 12 characters, with upper and lower case, a number and a symbol. +

+
+
+ +
+ + +
+
+ ); +}; diff --git a/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/UsersSection.tsx b/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/UsersSection.tsx index 4d6b77b6..04218487 100644 --- a/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/UsersSection.tsx +++ b/src/apps/officer-web/Screens/Dashboard/Settings/UserManagement/UsersSection.tsx @@ -15,6 +15,7 @@ import { AlertDialogHeader, AlertDialogTitle, } from '@/components/ui/alert-dialog'; +import { CreateUserForm } from './CreateUserForm'; type ManagedUser = { id: number; @@ -27,7 +28,14 @@ type ManagedUser = { isOwner: boolean; }; -type UsersResponse = { users: ManagedUser[]; roles: string[]; ownerId: number }; +type UsersResponse = { + users: ManagedUser[]; + /** Every role, for displaying the owner's own value. */ + roles: string[]; + /** Roles the server will accept in a write. Excludes the owner role — both write paths refuse it. */ + assignableRoles: string[]; + ownerId: number; +}; const USERS_KEY = ['MANAGED_USERS']; @@ -86,9 +94,11 @@ export const UsersSection = () => {

Every account on this server. The owner is fixed — the database itself refuses to demote or remove it — so that - row cannot be changed from here. + row cannot be changed from here, and no other account can be promoted into it.

+ +
{data.users.map((user) => { const busy = pendingId === user.id; @@ -114,7 +124,9 @@ export const UsersSection = () => { - {data.roles.map((role) => ( + {/* The owner's row needs its own value present to render at all, and it is disabled + anyway. Every other row offers only what the server will accept. */} + {(user.isOwner ? data.roles : data.assignableRoles).map((role) => ( {role} diff --git a/src/databases/officer_db/src/index.ts b/src/databases/officer_db/src/index.ts index 8c8a9a58..dc5fa039 100644 --- a/src/databases/officer_db/src/index.ts +++ b/src/databases/officer_db/src/index.ts @@ -2,6 +2,7 @@ export { getUsers, getUserById, getUserByEmail, + getUserByUsername, getOwnerUser, getUserCount, createUser, diff --git a/src/databases/officer_db/src/queries/auth.ts b/src/databases/officer_db/src/queries/auth.ts index 79a5dea3..982614f4 100644 --- a/src/databases/officer_db/src/queries/auth.ts +++ b/src/databases/officer_db/src/queries/auth.ts @@ -19,6 +19,14 @@ export async function getUserByEmail(email: string): Promise { + const [user] = await db.select().from(users).where(eq(users.username, username)); + return user; +} + // The owner: the one account with role 'Super Admin'. Sidecars that need "who is the owner" (e.g. the // agent sidecar, which PM2 starts with no email in its env) resolve it here rather than being told by // the main server. @@ -27,8 +35,15 @@ export async function getUserByEmail(email: string): Promise { - const [user] = await db.select().from(users).where(eq(users.role, 'Super Admin')).limit(1); + const [user] = await db.select().from(users).where(eq(users.role, 'Super Admin')).orderBy(users.id).limit(1); return user; } diff --git a/src/servers/api/users/create-user.ts b/src/servers/api/users/create-user.ts new file mode 100644 index 00000000..eb9b4967 --- /dev/null +++ b/src/servers/api/users/create-user.ts @@ -0,0 +1,89 @@ +import type { Handler } from 'hono'; +import { createUser, getUserByEmail, getUserByUsername, USER_ROLES } from 'officerdb'; +import type { UserRole } from 'officerdb'; +import argon2 from 'argon2'; +import * as errors from '@@/custom-errors'; +import { provisionUserDirs } from '@@/data-path'; +import { validatePassword } from '../auth/validate-password'; +import { validateUsername } from '../auth/validate-username'; +import { toPublicUser } from './manage-users'; + +// The owner creating a second account. Until this existed, `createUser` had exactly one call site — +// `auth/bootstrap.ts`, gated on an empty user table — so every non-owner account on any instance had +// been inserted into Postgres by hand. +// +// ── Why the owner sets the password ── +// +// The alternative is an invite: a token emailed to the person, who then sets their own. That is the +// better shape and it needs a mail path, a token table and an expiry policy. This is the honest +// intermediate: the owner types a password and tells the person, the same way they would hand over a +// wifi key. `passwordChangedAt` stays null, so nothing pretends the person chose it. +// +// ── Status is 'Active', deliberately ── +// +// The column defaults to 'Unverified' and `signin.ts` refuses anything that is not 'Active' with a bare +// UNAUTHORIZED. So an account created at the default would be indistinguishable from a wrong password, +// 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. */ +const ASSIGNABLE_ROLES = USER_ROLES.filter((r) => r !== 'Super Admin'); + +export const createUserHandler: Handler = async function (ctx) { + const body = (ctx.get('body') ?? {}) as Record; + + const email = typeof body.email === 'string' ? body.email.trim().toLowerCase() : ''; + const name = typeof body.name === 'string' ? body.name.trim() : ''; + const password = typeof body.password === 'string' ? body.password : ''; + const role = typeof body.role === 'string' ? body.role : 'Member'; + + if (!email || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) throw errors.BAD_REQUEST('Invalid email address'); + if (!name) throw errors.BAD_REQUEST('Name is required'); + + // The same validators bootstrap uses. A member's password protects the same surface a member can + // reach, so there is no argument for a weaker rule here — and two different rules would mean the + // owner could create an account that could not then change its own password to something similar. + const username = validateUsername(typeof body.username === 'string' ? body.username : undefined); + validatePassword(password); + + // Refused rather than filtered, so the owner is told instead of quietly getting a Member. + // + // There is exactly one owner. The database only pins user 1's role — a row-level CHECK cannot say + // "no OTHER row may hold this" — so a second Super Admin is storable, and `getOwnerUser()` would then + // return whichever the query reached first. That answer decides the identity the agent sidecar runs + // as, who reaches the vault and which origin is privileged, so it is not a thing to leave to a query + // plan. If the owner ever needs to hand the server over, that is a deliberate transfer, not a dropdown. + if (!(ASSIGNABLE_ROLES as readonly string[]).includes(role)) { + throw errors.BAD_REQUEST( + role === 'Super Admin' + ? 'There is one server owner and it cannot be created here.' + : `Role must be one of: ${ASSIGNABLE_ROLES.join(', ')}`, + ); + } + + // Checked before the insert purely for the message — both columns are unique, so the database is the + // real guard and this is a race it can lose harmlessly (the insert then throws). + if (await getUserByEmail(email)) throw errors.CONFLICT('An account with that email already exists'); + if (await getUserByUsername(username)) throw errors.CONFLICT('That username is taken'); + + const user = await createUser({ + email, + password: await argon2.hash(password), + name, + username, + status: 'Active', + role: role as UserRole, + }); + + // After the row, and not fatal if it fails. A missing directory is repairable from a shell + // (`bun scripts/provision-user-dirs.ts `); an account that half-exists because mkdir failed + // is not, and the owner would have to go into Postgres to clean it up — the exact thing this route + // is here to stop being necessary. + try { + provisionUserDirs(email); + } catch (ex) { + console.warn(`[users] created ${email} but could not provision its data directories`, ex); + } + + return ctx.json({ user: toPublicUser(user) }, 201); +}; diff --git a/src/servers/api/users/manage-users.ts b/src/servers/api/users/manage-users.ts index 07b57098..9a3f7785 100644 --- a/src/servers/api/users/manage-users.ts +++ b/src/servers/api/users/manage-users.ts @@ -21,7 +21,8 @@ type PublicUser = { isOwner: boolean; }; -const toPublicUser = (u: Awaited>[number]): PublicUser => ({ +/** Shared with create-user.ts, so a created account and a listed one are described the same way. */ +export const toPublicUser = (u: Awaited>[number]): PublicUser => ({ id: u.id, email: u.email, name: u.name, @@ -37,7 +38,10 @@ export const listUsersHandler: Handler = async function (ctx) { const users = await getUsers(); return ctx.json({ users: users.sort((a, b) => a.id - b.id).map(toPublicUser), + // Every role, so the owner's own row can display its value. The UI must not offer 'Super Admin' in a + // picker — both write paths refuse it — which is what `assignableRoles` is for. roles: USER_ROLES, + assignableRoles: USER_ROLES.filter((r) => r !== 'Super Admin'), ownerId: OWNER_USER_ID, }); }; @@ -58,6 +62,15 @@ 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.'); + } + const existing = await getUserById(id); if (!existing) throw errors.NOT_FOUND('User not found'); diff --git a/src/servers/api/users/users-router.ts b/src/servers/api/users/users-router.ts index 440beaa2..326ef152 100644 --- a/src/servers/api/users/users-router.ts +++ b/src/servers/api/users/users-router.ts @@ -5,6 +5,7 @@ import { isSuperAdmin } from '@@/super-admin'; import * as errors from '@@/custom-errors'; import { updateUserHandler } from './update-user'; import { listUsersHandler, updateUserRoleHandler, deleteUserHandler } from './manage-users'; +import { createUserHandler } from './create-user'; import { capabilityAdminRouter } from './capabilities-routes'; export const usersRouter = createRouter(); @@ -24,6 +25,9 @@ const ownerGate: MiddlewareHandler = async (ctx, next) => { }; usersRouter.get('/', ownerGate, listUsersHandler); +// POST, not PUT — and worth noting they sit one line apart. `PUT /` is the selfService exception every +// account may call on itself; `POST /` creates somebody else and is the owner's alone. +usersRouter.post('/', ownerGate, createUserHandler); usersRouter.patch('/:id/role', ownerGate, updateUserRoleHandler); usersRouter.delete('/:id', ownerGate, deleteUserHandler); diff --git a/src/servers/data-path.ts b/src/servers/data-path.ts index 1e605638..e4a0fafb 100644 --- a/src/servers/data-path.ts +++ b/src/servers/data-path.ts @@ -42,6 +42,37 @@ export const getHomeDir = (email: string) => join(DATA_PATH, email, 'home'); // terminals/chats/tasks share config and credentials with the shell they use outside Officer. export const getOwnerHomeDir = (email: string): string => process.env.HOME_DIR ?? getHomeDir(email); +// The directory skeleton a new account gets under DATA_PATH. +// +// Most of these are also created on demand by whichever feature owns them, so pre-creating them buys +// legibility more than function — the tree shows what an account has without it having to be used first. +// `home` is the exception and the reason this exists: nothing else creates it, and it is where a +// non-owner's sessions would run. +// +// Single-sourced here rather than in the script that used to own the list, because there are now two +// callers — `scripts/provision-user-dirs.ts` and the owner's create-account handler — and a skeleton +// that differs depending on how the account was made is a bug nobody would think to look for. +export const USER_DIRS = [ + 'home', + 'attachments', + 'cache', + 'dashboards', + 'email_accounts', + 'general_chat_sessions', + 'logs', + 'sidecar', +] as const; + +/** + * Create an account's root and its skeleton. Idempotent — an existing directory is left exactly as it is. + * + * Keyed on email because that is what the on-disk layout uses everywhere else (`DATA_PATH//…`). + * Renaming an account's email would orphan its directory; that is pre-existing and not this function's + * problem, but it is the reason nothing here derives a path from the id. + */ +export const provisionUserDirs = (email: string): void => { + for (const dir of USER_DIRS) mkdirSync(join(DATA_PATH, email, dir), { recursive: true }); +}; export const getTmpAttachmentsDir = (email: string) => join(DATA_PATH, email, 'attachments', 'tmp');