diff --git a/.env.example b/.env.example index b858e3a..ba0d234 100644 --- a/.env.example +++ b/.env.example @@ -1,5 +1,11 @@ # Values containing #, ", \ or surrounding spaces must be single-quoted # (dotenv 16 treats an unquoted # as a comment): DB_PASSWORD='abc#def' +# REQUIRED. The admin module treats anything other than "development" or "test" +# as production: strict secrets, cross-subdomain cookies, no relaxed CORS. +# Leaving it unset is therefore safe-by-default but will refuse to boot without +# the admin secrets below. Set it to development for local work. +NODE_ENV=development + PORT=3000 DB_HOST= @@ -26,8 +32,10 @@ TICKETS_RATE_LIMIT_MAX=10 TICKETS_RATE_LIMIT_WINDOW_MIN=10 ADMIN_DB= -# 32+ random bytes, e.g. `openssl rand -base64 48`. Rotating it signs everyone -# out and invalidates outstanding password-reset links. +# 32+ random bytes, e.g. `openssl rand -base64 48`. Mandatory outside +# development/test - there is deliberately no fallback, since a hardcoded one +# would be a published signing key. Rotating it signs everyone out and +# invalidates outstanding password-reset links. BETTER_AUTH_SECRET= API_BASE_URL=http://localhost:3000 ADMIN_APP_URL=http://localhost:3002 @@ -39,6 +47,19 @@ PASSKEY_RP_ID=localhost # user already exists). Idempotent, safe to leave set. ADMIN_BOOTSTRAP_EMAIL= +# The header the reverse proxy puts the real client IP in, and the proxy hops to +# trust. Get these right or better-auth cannot resolve a client IP and every +# request shares ONE rate-limit bucket (/sign-in/* allows 3 per 10 seconds, so +# one noisy client locks everyone out). Check with: +# SELECT `key` FROM rateLimit; -- a "no-trusted-ip" row means it is happening. +CLIENT_IP_HEADERS=x-real-ip +TRUSTED_PROXY_IPS= + +# Writes invitation links to the log. That link is a live account-creation +# credential, so this is refused outside development. Needed locally, where the +# mail relay is off and only the token's hash is stored. +ADMIN_LOG_INVITE_LINKS=true + MEMBER_CREDENTIAL=123 CHOIR_CREDENTIAL=123 MANAGEMENT_CREDENTIAL=123 diff --git a/CLAUDE.md b/CLAUDE.md index 5c9b4f3..1828ea1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,8 +83,16 @@ backslash escapes inside double quotes are expanded. Wrap any value containing ` surrounding spaces in single quotes (`DB_PASSWORD='abc#def'`), which are taken literally. A truncated password shows up as MariaDB "Access denied ... (using password: YES)". +**`NODE_ENV` is load-bearing for the admin module.** Only the explicit values +`development` and `test` relax anything; everything else, *including unset*, is treated as +production (strict secrets, cross-subdomain cookies, no localhost CORS). That direction is +deliberate: a Plesk vhost does not set `NODE_ENV`, and the inverse arrangement would +silently degrade the signing key, the cookie domain and the CORS list at once. Local work +needs `NODE_ENV=development`. + Copy `.env.example` (or create `.env`) with: ``` +NODE_ENV= PORT= DB_HOST= DB_USER= @@ -97,6 +105,9 @@ ADMIN_APP_URL= APP_ORIGINS= PASSKEY_RP_ID= ADMIN_BOOTSTRAP_EMAIL= +CLIENT_IP_HEADERS= +TRUSTED_PROXY_IPS= +ADMIN_LOG_INVITE_LINKS= FEEDBACK_DB= FEEDBACK_IP_SALT= FEEDBACK_RATE_LIMIT_MAX= diff --git a/package-lock.json b/package-lock.json index 083d70a..bd3e9b8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "version": "0.1.0", "license": "ISC", "dependencies": { + "@better-auth/core": "^1.7.2", "@better-auth/passkey": "^1.7.2", "app-root-path": "^3.0.0", "axios": "^1.20.0", diff --git a/package.json b/package.json index 8644567..03e19b9 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "author": "", "license": "ISC", "dependencies": { + "@better-auth/core": "^1.7.2", "@better-auth/passkey": "^1.7.2", "app-root-path": "^3.0.0", "axios": "^1.20.0", diff --git a/src/models/admin/admin.auth.ts b/src/models/admin/admin.auth.ts index 7ab6309..8890643 100644 --- a/src/models/admin/admin.auth.ts +++ b/src/models/admin/admin.auth.ts @@ -10,7 +10,9 @@ import { ADMIN_ALLOWED_ORIGINS, API_BASE_URL, BETTER_AUTH_SECRET, + CLIENT_IP_HEADERS, PASSKEY_RP_ID, + TRUSTED_PROXY_IPS, isProd } from './admin.config.js'; @@ -94,10 +96,12 @@ export const auth = betterAuth({ : {enabled: false}, ipAddress: { // better-auth reads the request itself and does not know about - // Express's `trust proxy`, so the header has to be named here. - // Verify against what Plesk's nginx actually sets before relying on - // the rate limiter (see the plan's pre-deploy checklist). - ipAddressHeaders: ['x-real-ip', 'x-forwarded-for'] + // Express's `trust proxy`, so both the header and the trusted hops + // have to be named here. Getting this wrong does not fail loudly - + // it collapses every client into one rate-limit bucket. See the + // commentary on CLIENT_IP_HEADERS in admin.config.ts. + ipAddressHeaders: CLIENT_IP_HEADERS, + ...(TRUSTED_PROXY_IPS.length > 0 ? {trustedProxies: TRUSTED_PROXY_IPS} : {}) } }, diff --git a/src/models/admin/admin.bootstrap.ts b/src/models/admin/admin.bootstrap.ts index 717755f..d992349 100644 --- a/src/models/admin/admin.bootstrap.ts +++ b/src/models/admin/admin.bootstrap.ts @@ -1,7 +1,7 @@ import * as UsersService from './users/users.admin.service.js'; import * as InvitationsService from './invitations/invitations.service.js'; import {sendInvitationMail} from './admin.mail.js'; -import {ADMIN_APP_URL, ADMIN_BOOTSTRAP_EMAIL, isProd} from './admin.config.js'; +import {ADMIN_APP_URL, ADMIN_BOOTSTRAP_EMAIL, LOG_INVITE_LINKS} from './admin.config.js'; import logger from '../../middleware/logger.js'; /** @@ -55,10 +55,10 @@ export const bootstrapAdmin = async (): Promise => { const mailed = await sendInvitationMail(email, 'Nachklang Admin', invitation.token, invitation.expiresAt); logger.info('Admin bootstrap: invitation created', {email, mailed}); - // Outside production the Salesforce mail relay is usually off, so the - // link is logged instead - that is how a local setup gets its first - // admin. Never in production: the log would then hold a live credential. - if (!isProd) { + // With the mail relay off, the logged link is how a local setup gets its + // first admin. Explicit opt-in (see LOG_INVITE_LINKS): the link is a + // live credential, so this must never depend on NODE_ENV alone. + if (LOG_INVITE_LINKS) { logger.info(`Admin bootstrap: ${ADMIN_APP_URL}/accept-invite?token=${invitation.token}`); } } catch (e: any) { diff --git a/src/models/admin/admin.config.ts b/src/models/admin/admin.config.ts index d5bb027..4ea4c80 100644 --- a/src/models/admin/admin.config.ts +++ b/src/models/admin/admin.config.ts @@ -1,3 +1,4 @@ +import * as crypto from 'crypto'; import * as dotenv from 'dotenv'; import logger from '../../middleware/logger.js'; @@ -8,13 +9,28 @@ dotenv.config(); * (better-auth trustedOrigins, passkey origins) and app.ts (CORS) need the * same origin list, and a second parser would drift from this one. * - * In production every value is mandatory: a missing BETTER_AUTH_SECRET or a - * wrong ADMIN_APP_URL is the kind of misconfiguration that fails as "login - * silently does nothing" hours later, so it fails at boot instead. In dev the - * localhost defaults below let a fresh checkout run without an .env. + * Read this before changing the environment handling below: several security + * properties depend on it, and they are deliberately arranged to fail *safe*. + * + * `NODE_ENV` is opt-in to relaxed behaviour, not opt-in to strict behaviour. + * Only the explicit values 'development' and 'test' relax anything; anything + * else - including NODE_ENV being unset, which is exactly what a fresh Plesk + * vhost gives you - is treated as production. The inverse arrangement is a + * trap: it degrades the cookie domain, the CORS origin list and the signing + * key all at once, and every one of those failures is silent. + * + * The signing key is never allowed to be a known constant. In dev, an unset + * BETTER_AUTH_SECRET becomes a random per-process value: sessions do not + * survive a restart, which is mildly annoying and much better than a default + * secret that can be copied out of this file and used against production. */ -export const isProd = process.env.NODE_ENV === 'production'; +const nodeEnv = process.env.NODE_ENV; + +// Explicitly relaxed environments. Everything else, unset included, is strict. +const isRelaxedEnv = nodeEnv === 'development' || nodeEnv === 'test'; + +export const isProd = !isRelaxedEnv; const required = (name: string, devDefault: string): string => { const value = process.env[name]; @@ -22,8 +38,11 @@ const required = (name: string, devDefault: string): string => { return value; } if (isProd) { - logger.error(`Admin module: ${name} is not set`); - throw new Error(`${name} must be set in production`); + logger.error( + `Admin module: ${name} is not set (NODE_ENV=${nodeEnv ?? 'unset'}, so strict mode applies; ` + + 'set NODE_ENV=development for local work)' + ); + throw new Error(`${name} must be set unless NODE_ENV is development or test`); } return devDefault; }; @@ -33,9 +52,11 @@ export const ADMIN_APP_URL = required('ADMIN_APP_URL', 'http://localhost:3002'); // 32+ random bytes; better-auth signs cookies and reset tokens with it. // Rotating it invalidates every session, which is why it is not derived. +// There is no hardcoded fallback on purpose: a constant committed here would +// be a published signing key the moment someone deploys without setting it. export const BETTER_AUTH_SECRET = required( 'BETTER_AUTH_SECRET', - 'dev-only-insecure-secret-do-not-use-in-production' + crypto.randomBytes(48).toString('base64') ); // Passkeys are bound to this: a credential registered for "nachklang.art" @@ -43,15 +64,17 @@ export const BETTER_AUTH_SECRET = required( // works in dev. Changing it invalidates every registered passkey. export const PASSKEY_RP_ID = process.env.PASSKEY_RP_ID || (isProd ? 'nachklang.art' : 'localhost'); -// The apps whose frontends may talk to /admin/* with credentials. -const parseOrigins = (value: string | undefined): string[] => { - return (value || '') +const parseList = (value: string | undefined, fallback: string[]): string[] => { + const parsed = (value || '') .split(',') - .map(origin => origin.trim().replace(/\/$/, '')) - .filter(origin => origin.length > 0); + .map(entry => entry.trim()) + .filter(entry => entry.length > 0); + + return parsed.length > 0 ? parsed : fallback; }; -export const APP_ORIGINS = parseOrigins(process.env.APP_ORIGINS); +// The apps whose frontends may talk to /admin/* with credentials. +export const APP_ORIGINS = parseList(process.env.APP_ORIGINS, []).map(origin => origin.replace(/\/$/, '')); // Kept in sync by construction rather than by three separate lists: the admin // app itself always counts, and dev adds the local ports. @@ -60,4 +83,51 @@ export const ADMIN_ALLOWED_ORIGINS = Array.from(new Set([ ...APP_ORIGINS ])); +/** + * The header the reverse proxy puts the real client IP in, and the proxy hops + * to trust when reading it. + * + * This matters more than it looks. better-auth does not know about Express's + * `trust proxy`; it reads the request itself. If it cannot resolve a client IP + * it falls back to a single shared bucket ("no-trusted-ip") for the whole + * process - and /sign-in/* carries a default of 3 requests per 10 seconds, so + * one noisy client would lock every user out of every app. + * + * Without TRUSTED_PROXY_IPS, better-auth rejects a multi-value + * x-forwarded-for outright (it cannot tell which hop is the client), which is + * exactly the case that produces that shared bucket. Set it to the address or + * CIDR of Plesk's nginx. Conversely, listing a header the proxy does not + * overwrite lets a client set its own IP and mint itself an unlimited + * brute-force budget - so the default is the single header nginx sets, not a + * permissive list. + */ +export const CLIENT_IP_HEADERS = parseList(process.env.CLIENT_IP_HEADERS, ['x-real-ip']); + +export const TRUSTED_PROXY_IPS = parseList(process.env.TRUSTED_PROXY_IPS, []); + +if (isProd && TRUSTED_PROXY_IPS.length === 0) { + logger.warn( + 'Admin module: TRUSTED_PROXY_IPS is not set. If the proxy sends a multi-value ' + + `${CLIENT_IP_HEADERS.join('/')}, better-auth cannot resolve a client IP and every ` + + 'request shares one rate-limit bucket. Verify with: SELECT `key` FROM rateLimit - ' + + 'a "no-trusted-ip" row means this is happening.' + ); +} + export const ADMIN_BOOTSTRAP_EMAIL = process.env.ADMIN_BOOTSTRAP_EMAIL || ''; + +/** + * Whether to write invitation links to the log. An invitation link is a live + * account-creation credential, so this is an explicit opt-in rather than + * something inferred from NODE_ENV: local work needs it (the mail relay is + * usually off, and only the token's hash is stored, so there is otherwise no + * way to walk the accept flow), and production must never have it. + * + * Refused outright in strict mode, so setting it in a production .env by + * accident fails at boot instead of quietly filling the log with credentials. + */ +export const LOG_INVITE_LINKS = process.env.ADMIN_LOG_INVITE_LINKS === 'true' && !isProd; + +if (process.env.ADMIN_LOG_INVITE_LINKS === 'true' && isProd) { + logger.error('Admin module: ADMIN_LOG_INVITE_LINKS is set outside development - refusing to log invitation tokens'); +} diff --git a/src/models/admin/admin.schema.ts b/src/models/admin/admin.schema.ts index d26763d..4ad234a 100644 --- a/src/models/admin/admin.schema.ts +++ b/src/models/admin/admin.schema.ts @@ -72,10 +72,26 @@ export interface InvitationTable { revoked_at: Date | null; } +export interface VerificationTable { + id: string; + identifier: string; + value: string; + expiresAt: Date; +} + +export interface RateLimitTable { + id: string; + key: string; + count: number; + lastRequest: number; +} + export interface AdminDatabase { user: UserTable; session: SessionTable; passkey: PasskeyTable; + verification: VerificationTable; + rateLimit: RateLimitTable; user_app_permissions: UserAppPermissionTable; invitations: InvitationTable; } diff --git a/src/models/admin/invitations/invitations.plugin.ts b/src/models/admin/invitations/invitations.plugin.ts index 37ec281..4904d93 100644 --- a/src/models/admin/invitations/invitations.plugin.ts +++ b/src/models/admin/invitations/invitations.plugin.ts @@ -2,6 +2,7 @@ import * as z from 'zod'; import {APIError, createAuthEndpoint} from 'better-auth/api'; import {setSessionCookie} from 'better-auth/cookies'; import {createLocalAccountIssuer} from 'better-auth/db'; +import {runWithTransaction} from '@better-auth/core/context'; import type {BetterAuthPlugin} from 'better-auth'; import * as InvitationsService from './invitations.service.js'; import * as UsersService from '../users/users.admin.service.js'; @@ -105,29 +106,45 @@ export const invitationsPlugin = () => { throw invalidToken(); } + // The user and its credential account go in one better-auth + // transaction, the way better-auth's own sign-up route does + // it: a half-created account with no password is not + // recoverable through any route this API exposes. + // + // It cannot cover everything, though. `user_app_permissions` + // and `invitations` are written through this module's own + // Kysely pool, which is a different connection, so no single + // transaction spans both. What follows is therefore + // compensated by hand rather than rolled back. + let user: Awaited> | null = null; try { - const user = await ctx.context.internalAdapter.createUser( - { - email: invitation.email, - name: invitation.name, - // Accepting a link sent to that mailbox *is* the - // proof of address ownership, so there is no - // separate verification mail (plan decision 15). - emailVerified: true, - disabled: false - }, - {method: 'email-password'} - ); + user = await runWithTransaction(ctx.context.adapter, async () => { + const created = await ctx.context.internalAdapter.createUser( + { + email: invitation.email, + name: invitation.name, + // Accepting a link sent to that mailbox *is* + // the proof of address ownership, so there is + // no separate verification mail (plan + // decision 15). + emailVerified: true, + disabled: false + }, + {method: 'email-password'} + ); - // Same call better-auth's own sign-up route makes, down to - // the synthetic issuer - a credential account written any - // other way would not be found on sign-in. - await ctx.context.internalAdapter.linkAccount({ - userId: user.id, - providerId: 'credential', - issuer: createLocalAccountIssuer('credential'), - accountId: user.id, - password: await ctx.context.password.hash(ctx.body.password) + // Same call better-auth's own sign-up route makes, + // down to the synthetic issuer - a credential account + // written any other way is not found on sign-in. + await ctx.context.internalAdapter.linkAccount({ + userId: created.id, + providerId: 'credential', + issuer: createLocalAccountIssuer('credential'), + accountId: created.id, + password: await ctx.context.password.hash(ctx.body.password) + }); + + return created; }); await UsersService.setPermissions(user.id, invitation.apps, null); @@ -139,12 +156,34 @@ export const invitationsPlugin = () => { user: {id: user.id, email: user.email, name: user.name} }); } catch (e: any) { - // The invitation is already marked accepted at this - // point. Leaving it that way is deliberate: a token that - // has been through a half-completed account creation - // should not stay usable. The admin can send a new - // invitation, and this log says why one is needed. - logger.error('Admin: invitation accepted but account creation failed', { + // Undo what committed, so the invitee can use their link + // again instead of being stranded with a burnt token, an + // account they cannot sign into, and an admin who cannot + // re-invite them (the create route 409s on an existing + // user, and there is no delete route by design). + // + // Deleting the user is safe here: it was created moments + // ago in this request, and acceptance already established + // that no account for this address existed before. + try { + if (user) { + await ctx.context.internalAdapter.deleteUser(user.id); + } + await InvitationsService.unmarkAccepted(invitation.id); + } catch (compensationError: any) { + // Now the state really is inconsistent, and only a + // human can sort it out. Say so loudly and precisely. + logger.error('Admin: invitation acceptance failed AND its rollback failed', { + invitationId: invitation.id, + email: invitation.email, + userId: user?.id, + detail: e?.message, + compensationDetail: compensationError?.message + }); + throw e; + } + + logger.error('Admin: invitation acceptance failed and was rolled back', { invitationId: invitation.id, detail: e?.message }); diff --git a/src/models/admin/invitations/invitations.router.ts b/src/models/admin/invitations/invitations.router.ts index cc5433f..8055551 100644 --- a/src/models/admin/invitations/invitations.router.ts +++ b/src/models/admin/invitations/invitations.router.ts @@ -3,10 +3,9 @@ import * as InvitationsService from './invitations.service.js'; import * as UsersService from '../users/users.admin.service.js'; import {isAppName, AppName} from '../admin.schema.js'; import {sendInvitationMail} from '../admin.mail.js'; -import {ADMIN_APP_URL} from '../admin.config.js'; +import {ADMIN_APP_URL, LOG_INVITE_LINKS} from '../admin.config.js'; import {sendServerError} from '../admin.errors.js'; import logger from '../../../middleware/logger.js'; -import {isProd} from '../admin.config.js'; export const invitationsRouter = express.Router(); @@ -21,14 +20,16 @@ export const invitationsRouter = express.Router(); const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; /** - * Outside production the Salesforce relay is normally off, so the invitation - * mail never arrives and only the token's hash is stored - there would be no - * way to walk through the accept flow locally. Logging the link closes that, - * and mirrors what the bootstrap already does. Never in production: the log - * would then hold a live credential. + * With the mail relay off (the normal local setup) the invitation mail never + * arrives, and only the token's hash is stored, so there would be no way to + * walk through the accept flow. Logging the link closes that. + * + * Gated on an explicit opt-in rather than on NODE_ENV: the link is a live + * account-creation credential, and "not production" is too weak a condition to + * hang that on. See LOG_INVITE_LINKS in admin.config.ts. */ const logInviteLinkInDev = (token: string): void => { - if (!isProd) { + if (LOG_INVITE_LINKS) { logger.info(`Admin: invitation link ${ADMIN_APP_URL}/accept-invite?token=${encodeURIComponent(token)}`); } }; diff --git a/src/models/admin/invitations/invitations.service.ts b/src/models/admin/invitations/invitations.service.ts index 50e6157..6d6b2c4 100644 --- a/src/models/admin/invitations/invitations.service.ts +++ b/src/models/admin/invitations/invitations.service.ts @@ -133,6 +133,19 @@ export const markAccepted = async (invitationId: number): Promise => { return Number(result.numUpdatedRows) > 0; }; +/** + * Reverses markAccepted. Used only to compensate a failed acceptance: the user + * could not be created, so the link must become usable again rather than + * stranding the invitee with a burnt token and no account. + */ +export const unmarkAccepted = async (invitationId: number): Promise => { + await db + .updateTable('invitations') + .set({accepted_at: null}) + .where('id', '=', invitationId) + .execute(); +}; + export const listOpenInvitations = async (): Promise => { const rows = await db .selectFrom('invitations') diff --git a/src/models/admin/users/users.admin.router.ts b/src/models/admin/users/users.admin.router.ts index b22251c..3a1bc35 100644 --- a/src/models/admin/users/users.admin.router.ts +++ b/src/models/admin/users/users.admin.router.ts @@ -121,18 +121,21 @@ usersAdminRouter.put('/:userId/permissions', async (req: Request, res: Response) const target = await UsersService.loadAccess(userId); const losesAdmin = Boolean(target?.apps.includes('admin')) && !(apps as AppName[]).includes('admin'); + // Self-lockout is checked here because it needs the caller's identity, + // which the service has no business knowing. The last-admin check is + // NOT done here: it has to be inside the write transaction to survive + // two admins acting at the same time (see setPermissionsGuarded). if (losesAdmin && userId === res.locals.admin.id) { conflict(res, 'Du kannst dir die Admin-Berechtigung nicht selbst entziehen.'); return; } - // Only an enabled admin counts; see countActiveAdmins. - if (losesAdmin && !target?.disabled && (await UsersService.countActiveAdmins()) <= 1) { + const result = await UsersService.setPermissionsGuarded(userId, apps as AppName[], res.locals.admin.id); + if (result === 'last-admin') { conflict(res, 'Die letzte Admin-Berechtigung kann nicht entzogen werden.'); return; } - await UsersService.setPermissions(userId, apps as AppName[], res.locals.admin.id); res.status(200).send(await UsersService.getUserDetail(userId)); } catch (e: any) { sendServerError(res, e); @@ -166,12 +169,12 @@ usersAdminRouter.post('/:userId/disable', async (req: Request, res: Response) => return; } - if (target.apps.includes('admin') && !target.disabled && (await UsersService.countActiveAdmins()) <= 1) { + const result = await UsersService.disableUserGuarded(userId); + if (result === 'last-admin') { conflict(res, 'Der letzte aktive Admin kann nicht deaktiviert werden.'); return; } - await UsersService.disableUser(userId); res.status(200).send(await UsersService.getUserDetail(userId)); } catch (e: any) { sendServerError(res, e); diff --git a/src/models/admin/users/users.admin.service.ts b/src/models/admin/users/users.admin.service.ts index d23de61..50da14b 100644 --- a/src/models/admin/users/users.admin.service.ts +++ b/src/models/admin/users/users.admin.service.ts @@ -1,5 +1,6 @@ +import {Transaction} from 'kysely'; import {NachklangAdminDB} from '../Admin.db.js'; -import {AppName, APP_NAMES} from '../admin.schema.js'; +import {AdminDatabase, AppName, APP_NAMES} from '../admin.schema.js'; const db = NachklangAdminDB.db; @@ -93,10 +94,15 @@ export const listUsers = async (): Promise => { // Last sign-in is derived from the newest session rather than stored: a // session row is created on every sign-in and we never update its // createdAt, so max(createdAt) is exactly that, with no extra column to - // keep in sync. Sessions are pruned on expiry, so this goes back to null - // for someone who has not signed in for over 30 days. + // keep in sync. + // + // The expiry filter must match getUserDetail's. better-auth only deletes an + // expired session when someone actually presents it, so expired rows linger + // - without this, the list would report a last sign-in for someone the + // detail view shows as never having signed in. const lastSessions = await db .selectFrom('session') + .where('expiresAt', '>', new Date()) .select(({fn}) => ['userId', fn.max('createdAt').as('lastSignInAt')]) .groupBy('userId') .execute(); @@ -192,6 +198,103 @@ export const setPermissions = async ( }); }; +/** + * Why the guards live down here rather than in the router: they are + * check-then-act, and the check has to happen inside the same transaction as + * the write, over locked rows. Two admins each removing the other's `admin` + * permission at the same moment would otherwise both read a count of 2, both + * pass, and both commit - leaving nobody who can administer anything, with + * ADMIN_BOOTSTRAP_EMAIL at the next restart as the only way back in. + * + * `SELECT ... FOR UPDATE` makes the second transaction wait and re-read the + * count the first one just changed. + */ +export type LastAdminGuardResult = 'ok' | 'last-admin'; + +const countActiveAdminsForUpdate = async (trx: Transaction): Promise => { + const row = await trx + .selectFrom('user_app_permissions') + .innerJoin('user', 'user.id', 'user_app_permissions.user_id') + .where('user_app_permissions.app', '=', 'admin') + .where('user.disabled', '=', false) + .select(({fn}) => fn.countAll().as('count')) + .forUpdate() + .executeTakeFirst(); + + return Number(row?.count ?? 0); +}; + +/** + * Replaces a user's permissions, refusing to remove the last active admin. + * Returns 'last-admin' instead of throwing so the router can answer 409. + */ +export const setPermissionsGuarded = async ( + userId: string, + apps: AppName[], + grantedBy: string | null +): Promise => { + const unique = Array.from(new Set(apps)).filter(app => APP_NAMES.includes(app)); + + return db.transaction().execute(async trx => { + const target = await trx + .selectFrom('user_app_permissions') + .innerJoin('user', 'user.id', 'user_app_permissions.user_id') + .where('user_app_permissions.user_id', '=', userId) + .where('user_app_permissions.app', '=', 'admin') + .select(['user.disabled as disabled']) + .forUpdate() + .executeTakeFirst(); + + const losesAdmin = Boolean(target) && !unique.includes('admin'); + if (losesAdmin && !target?.disabled && (await countActiveAdminsForUpdate(trx)) <= 1) { + return 'last-admin'; + } + + await trx.deleteFrom('user_app_permissions').where('user_id', '=', userId).execute(); + if (unique.length > 0) { + await trx + .insertInto('user_app_permissions') + .values(unique.map(app => ({ + user_id: userId, + app, + role: 'admin', + granted_by: grantedBy, + granted_at: new Date() + }))) + .execute(); + } + + return 'ok'; + }); +}; + +/** + * Disables a user and revokes every session, refusing to disable the last + * active admin. Same locking rationale as setPermissionsGuarded. + */ +export const disableUserGuarded = async (userId: string): Promise => { + return db.transaction().execute(async trx => { + const isAdmin = await trx + .selectFrom('user_app_permissions') + .innerJoin('user', 'user.id', 'user_app_permissions.user_id') + .where('user_app_permissions.user_id', '=', userId) + .where('user_app_permissions.app', '=', 'admin') + .where('user.disabled', '=', false) + .select('user_app_permissions.user_id') + .forUpdate() + .executeTakeFirst(); + + if (isAdmin && (await countActiveAdminsForUpdate(trx)) <= 1) { + return 'last-admin'; + } + + await trx.updateTable('user').set({disabled: true}).where('id', '=', userId).execute(); + await trx.deleteFrom('session').where('userId', '=', userId).execute(); + + return 'ok'; + }); +}; + export const grantPermission = async ( userId: string, app: AppName, diff --git a/test/admin/users.admin.router.test.ts b/test/admin/users.admin.router.test.ts index bb6b4e3..df1c95f 100644 --- a/test/admin/users.admin.router.test.ts +++ b/test/admin/users.admin.router.test.ts @@ -7,7 +7,9 @@ vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({ getUserDetail: vi.fn(), loadAccess: vi.fn(), setPermissions: vi.fn(), + setPermissionsGuarded: vi.fn(), disableUser: vi.fn(), + disableUserGuarded: vi.fn(), enableUser: vi.fn(), revokeSession: vi.fn(), countActiveAdmins: vi.fn(), @@ -40,6 +42,8 @@ beforeEach(() => { } service.getUserDetail.mockResolvedValue({id: 'other', apps: []}); service.userExists.mockResolvedValue(true); + service.setPermissionsGuarded.mockResolvedValue('ok'); + service.disableUserGuarded.mockResolvedValue('ok'); }); describe('PUT /admin/users/:id/permissions', () => { @@ -47,7 +51,7 @@ describe('PUT /admin/users/:id/permissions', () => { const res = await request(makeApp()).put('/admin/users/other/permissions').send({apps: ['calendar', 'nope']}); expect(res.status).toBe(400); - expect(service.setPermissions).not.toHaveBeenCalled(); + expect(service.setPermissionsGuarded).not.toHaveBeenCalled(); }); it('rejects a non-array body', async () => { @@ -62,7 +66,7 @@ describe('PUT /admin/users/:id/permissions', () => { const res = await request(makeApp()).put('/admin/users/ghost/permissions').send({apps: []}); expect(res.status).toBe(404); - expect(service.setPermissions).not.toHaveBeenCalled(); + expect(service.setPermissionsGuarded).not.toHaveBeenCalled(); }); it('refuses to remove the caller\'s own admin permission', async () => { @@ -72,30 +76,28 @@ describe('PUT /admin/users/:id/permissions', () => { const res = await request(makeApp('me')).put('/admin/users/me/permissions').send({apps: ['feedback']}); expect(res.status).toBe(409); - expect(service.setPermissions).not.toHaveBeenCalled(); + expect(service.setPermissionsGuarded).not.toHaveBeenCalled(); }); - // Defence in depth: with the caller themselves being an active admin this - // count cannot actually reach 1 in production, but the guard is what makes - // that safe to rely on rather than to reason about. - it('refuses to remove the last remaining active admin', async () => { + // The last-admin decision is made inside the write transaction (so two + // admins acting at once cannot both pass a check-then-act); the router's + // job is only to turn that verdict into a 409. + it('answers 409 when the service reports the last admin would be removed', async () => { service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']}); - service.countActiveAdmins.mockResolvedValue(1); + service.setPermissionsGuarded.mockResolvedValue('last-admin'); const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: []}); expect(res.status).toBe(409); - expect(service.setPermissions).not.toHaveBeenCalled(); }); it('allows removing an admin while another active admin remains', async () => { service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']}); - service.countActiveAdmins.mockResolvedValue(2); const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['tickets']}); expect(res.status).toBe(200); - expect(service.setPermissions).toHaveBeenCalledWith('other', ['tickets'], 'me'); + expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['tickets'], 'me'); }); it('allows granting permissions to someone who has none', async () => { @@ -104,8 +106,7 @@ describe('PUT /admin/users/:id/permissions', () => { const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['feedback', 'tickets']}); expect(res.status).toBe(200); - // Nothing is being taken away, so the last-admin count is not consulted. - expect(service.countActiveAdmins).not.toHaveBeenCalled(); + expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['feedback', 'tickets'], 'me'); }); }); @@ -114,17 +115,16 @@ describe('POST /admin/users/:id/disable', () => { const res = await request(makeApp('me')).post('/admin/users/me/disable'); expect(res.status).toBe(409); - expect(service.disableUser).not.toHaveBeenCalled(); + expect(service.disableUserGuarded).not.toHaveBeenCalled(); }); - it('refuses to disable the last active admin', async () => { + it('answers 409 when the service reports the last active admin would be disabled', async () => { service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']}); - service.countActiveAdmins.mockResolvedValue(1); + service.disableUserGuarded.mockResolvedValue('last-admin'); const res = await request(makeApp('me')).post('/admin/users/other/disable'); expect(res.status).toBe(409); - expect(service.disableUser).not.toHaveBeenCalled(); }); it('disables a non-admin user', async () => { @@ -133,7 +133,7 @@ describe('POST /admin/users/:id/disable', () => { const res = await request(makeApp('me')).post('/admin/users/other/disable'); expect(res.status).toBe(200); - expect(service.disableUser).toHaveBeenCalledWith('other'); + expect(service.disableUserGuarded).toHaveBeenCalledWith('other'); }); it('404s for an unknown user', async () => { diff --git a/test/integration/helpers.ts b/test/integration/helpers.ts index 63770e3..1f94aa9 100644 --- a/test/integration/helpers.ts +++ b/test/integration/helpers.ts @@ -11,13 +11,23 @@ const db = NachklangAdminDB.db; // prefix is only added over https. export const SESSION_COOKIE = 'nachklang.session_token'; -/** Wipes every table between test files. Child tables first - the FKs to - * `user` are ON DELETE CASCADE, but rateLimit and invitations are not. */ +/** + * Wipes every table between test files. Child tables first - the FKs to `user` + * are ON DELETE CASCADE, but the rest are not. + * + * `rateLimit` matters more than it looks: the limiter is enabled during the + * suite, and better-auth caps /sign-in/* at 3 requests per 10 seconds. All + * tests resolve to the same client IP, so they share one bucket - without this + * reset the suite would start failing with 429s that look like auth bugs as + * soon as a third sign-in assertion is added. + */ export const resetDatabase = async (): Promise => { await db.deleteFrom('session').execute(); await db.deleteFrom('user_app_permissions').execute(); await db.deleteFrom('passkey').execute(); await db.deleteFrom('invitations').execute(); + await db.deleteFrom('verification').execute(); + await db.deleteFrom('rateLimit').execute(); await db.deleteFrom('user').execute(); };