From 33489585a079fc8e8310f42688b3940547d3e60d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Sun, 6 Sep 2026 11:23:08 +0200 Subject: [PATCH] Model permissions as (app, role) and name passkeys from their AAGUID Two changes to the admin module, both made now because it is not deployed yet and neither is free later. Permissions were "an app", with a `role` column reserved for a future fine-grained model. Reviewing whether that reservation was enough found three problems: - Every row was written with role = 'admin', hardcoded, and the column defaulted to it. On a `tickets` row that reads as "tickets administrator" when it only ever meant "has access", and once real roles existed there would have been no way to tell an old plain grant from a deliberate one. - The role never left the database. /admin/me, the user list, the user detail and both write endpoints all spoke apps: AppName[]. Adding roles would have been a breaking change to /admin/me - and after the cutover that endpoint has two more consumers, turning a local edit into a coordinated deploy of three apps. - The key (user_id, app) allowed one role per app, i.e. a tier rather than a set of capabilities. Choosing later means an ALTER on a live table. So: the key is now (user_id, app, role), the role is `access`, and APP_ROLES in admin.schema.ts is the contract - a role not listed there is rejected with 400 rather than written. permissions: [{app, role}] is on the wire alongside the derived apps: AppName[], which is kept because the three frontends only ever ask "may I show this app?". Both write endpoints accept either shape, and the invitation column (now `permissions`) is parsed leniently: invitations live seven days, so a deploy that changes the shape has in-flight rows in the old one. requireAppAccess(app, role?) takes an optional role; nothing passes one yet. countActiveAdminsForUpdate now counts DISTINCT users rather than rows. With several roles per app, counting rows would make a single admin holding two roles look like two admins and defeat the last-admin guard at exactly the moment it matters. Separately, passkey registration now fills `name` from the authenticator's AAGUID via registration.afterVerification and better-auth's own getAuthenticatorName, yielding "1Password", "iCloud Keychain", "Windows Hello". Without it the column stayed NULL and the account page could only label every passkey "Passkey" - useless when someone has to remove the one on the device they just lost. A client-supplied name still wins; an unknown AAGUID still leaves it blank. 148 unit tests (up from 131, including the new admin.schema.test.ts) and 41 integration tests pass. The integration suite applies sql/admin/001_init.sql, so the new key is exercised rather than trusted. No production migration is needed - the module is not deployed. An existing dev database needs three statements: set role = 'access', drop and re-add the primary key, rename invitations.apps to permissions. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 14 +- docker/init/04-admin-schema.sql | 24 ++- sql/admin/001_init.sql | 16 +- src/models/admin/Admin.router.ts | 5 + src/models/admin/admin.auth.ts | 21 ++- src/models/admin/admin.bootstrap.ts | 3 +- src/models/admin/admin.middleware.ts | 22 ++- src/models/admin/admin.schema.ts | 88 +++++++++- .../admin/invitations/invitations.plugin.ts | 2 +- .../admin/invitations/invitations.router.ts | 29 +++- .../admin/invitations/invitations.service.ts | 45 +++-- src/models/admin/users/users.admin.router.ts | 35 ++-- src/models/admin/users/users.admin.service.ts | 164 +++++++++++++----- test/admin/admin.bootstrap.test.ts | 7 +- test/admin/admin.middleware.test.ts | 50 +++++- test/admin/admin.schema.test.ts | 99 +++++++++++ test/admin/users.admin.router.test.ts | 38 +++- test/integration/admin.auth.test.ts | 17 +- test/integration/admin.users.test.ts | 6 +- test/integration/helpers.ts | 15 +- 20 files changed, 579 insertions(+), 121 deletions(-) create mode 100644 test/admin/admin.schema.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 1828ea1..a6ef8f0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,9 +46,17 @@ other domain keeps the `mariadb` driver), mounted at `/admin/auth/*` for the aut and `/admin` for the JSON routes. Sessions are httpOnly cookies scoped to `.nachklang.art`, so one sign-in covers every app. Accounts are **invite-only** — public sign-up is disabled, and `invitations.plugin.ts` is the only code that creates users. -Permissions are per app in `user_app_permissions`; `requireAppAccess(app)` in -`admin.middleware.ts` is the single authenticator, and it queries the database on every -request (no cookie cache) so disabling a user takes effect at once. `ADMIN_BOOTSTRAP_EMAIL` +A permission is **(app, role)** in `user_app_permissions`, keyed on +`(user_id, app, role)` so one user can hold several roles per app. `access` is the only role +today and means "may use this app at all"; `APP_ROLES` in `admin.schema.ts` is the contract, +and a role not listed there is rejected rather than written. `requireAppAccess(app)` in +`admin.middleware.ts` is the single authenticator - it takes an optional second argument to +narrow to one role, and queries the database on every request (no cookie cache) so disabling +a user takes effect at once. Two things to know before touching this: any count of admins +must count **distinct users**, not permission rows, or a single admin with two roles reads as +two and the last-admin guard stops guarding; and both write endpoints accept +`{permissions: [{app, role}]}` as well as the older `{apps: ['tickets']}`, which means the +same at the `access` role. `ADMIN_BOOTSTRAP_EMAIL` makes sure someone can always get in on a fresh database. *Legacy calendar* — unchanged: users need a `@nachklang.art` email, and after activation diff --git a/docker/init/04-admin-schema.sql b/docker/init/04-admin-schema.sql index a27389a..6b51997 100644 --- a/docker/init/04-admin-schema.sql +++ b/docker/init/04-admin-schema.sql @@ -103,15 +103,21 @@ CREATE TABLE IF NOT EXISTS `rateLimit` ( -- --------------------------------------------------------------------------- -- Which apps a user may administer. `admin` is just another app: holding it is --- what lets someone manage users and invitations. `role` is reserved for --- per-app roles later and is 'admin' for every row today. +-- what lets someone manage users and invitations. A permission is (app, role); +-- `access` is the only role today, and the key admits several per app so finer +-- ones can be added by inserting rows rather than by migrating this table. CREATE TABLE IF NOT EXISTS `user_app_permissions` ( `user_id` VARCHAR(36) NOT NULL, `app` ENUM('calendar','feedback','tickets','admin') NOT NULL, - `role` VARCHAR(32) NOT NULL DEFAULT 'admin', + -- One row per (user, app, role). `access` means "may use this app at all" + -- and is the only role today; the key allows several per app so a finer + -- permission can be added later by inserting rows, not by migrating. + `role` VARCHAR(32) NOT NULL DEFAULT 'access', `granted_by` VARCHAR(36) DEFAULT NULL, `granted_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, - PRIMARY KEY (`user_id`, `app`), + -- (user_id, app) is the leftmost prefix of this key, so the per-request + -- permission lookup needs no separate index. + PRIMARY KEY (`user_id`, `app`, `role`), CONSTRAINT `uap_user_fk` FOREIGN KEY (`user_id`) REFERENCES `user` (`id`) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; @@ -122,7 +128,7 @@ CREATE TABLE IF NOT EXISTS `invitations` ( `email` VARCHAR(255) NOT NULL, `name` VARCHAR(255) NOT NULL, `token_hash` CHAR(64) NOT NULL, - `apps` JSON NOT NULL, + `permissions` JSON NOT NULL, `invited_by` VARCHAR(36) DEFAULT NULL, `created_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, `expires_at` DATETIME NOT NULL, @@ -158,7 +164,7 @@ VALUES ( ); INSERT INTO `user_app_permissions` (`user_id`, `app`, `role`) VALUES - ('dev-user-0000-0000-0000-000000000001', 'calendar', 'admin'), - ('dev-user-0000-0000-0000-000000000001', 'feedback', 'admin'), - ('dev-user-0000-0000-0000-000000000001', 'tickets', 'admin'), - ('dev-user-0000-0000-0000-000000000001', 'admin', 'admin'); + ('dev-user-0000-0000-0000-000000000001', 'calendar', 'access'), + ('dev-user-0000-0000-0000-000000000001', 'feedback', 'access'), + ('dev-user-0000-0000-0000-000000000001', 'tickets', 'access'), + ('dev-user-0000-0000-0000-000000000001', 'admin', 'access'); diff --git a/sql/admin/001_init.sql b/sql/admin/001_init.sql index f3f5b2a..b4bb193 100644 --- a/sql/admin/001_init.sql +++ b/sql/admin/001_init.sql @@ -114,15 +114,21 @@ CREATE TABLE IF NOT EXISTS `rateLimit` ( -- --------------------------------------------------------------------------- -- Which apps a user may administer. `admin` is just another app: holding it is --- what lets someone manage users and invitations. `role` is reserved for --- per-app roles later and is 'admin' for every row today. +-- what lets someone manage users and invitations. A permission is (app, role); +-- `access` is the only role today, and the key admits several per app so finer +-- ones can be added by inserting rows rather than by migrating this table. CREATE TABLE IF NOT EXISTS `user_app_permissions` ( `user_id` VARCHAR(36) NOT NULL, `app` ENUM('calendar','feedback','tickets','admin') NOT NULL, - `role` VARCHAR(32) NOT NULL DEFAULT 'admin', + -- One row per (user, app, role). `access` means "may use this app at all" + -- and is the only role today; the key allows several per app so a finer + -- permission can be added later by inserting rows, not by migrating. + `role` VARCHAR(32) NOT NULL DEFAULT 'access', `granted_by` VARCHAR(36) DEFAULT NULL, `granted_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, - PRIMARY KEY (`user_id`, `app`), + -- (user_id, app) is the leftmost prefix of this key, so the per-request + -- permission lookup needs no separate index. + PRIMARY KEY (`user_id`, `app`, `role`), CONSTRAINT `uap_user_fk` FOREIGN KEY (`user_id`) REFERENCES `user` (`id`) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; @@ -133,7 +139,7 @@ CREATE TABLE IF NOT EXISTS `invitations` ( `email` VARCHAR(255) NOT NULL, `name` VARCHAR(255) NOT NULL, `token_hash` CHAR(64) NOT NULL, - `apps` JSON NOT NULL, + `permissions` JSON NOT NULL, `invited_by` VARCHAR(36) DEFAULT NULL, `created_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, `expires_at` DATETIME NOT NULL, diff --git a/src/models/admin/Admin.router.ts b/src/models/admin/Admin.router.ts index 2e99933..8956df5 100644 --- a/src/models/admin/Admin.router.ts +++ b/src/models/admin/Admin.router.ts @@ -35,6 +35,11 @@ adminRouter.get('/me', requireSignedIn, (req: Request, res: Response) => { id: res.locals.admin.id, email: res.locals.admin.email, fullName: res.locals.admin.displayName, + // `permissions` is the full (app, role) truth; `apps` is the distinct + // apps within it. Both are sent because the three frontends only ever ask + // "may I show this app?", and keeping `apps` means a finer permission can + // land here without a coordinated deploy of all of them. + permissions: res.locals.admin.permissions, apps: res.locals.admin.apps }); }); diff --git a/src/models/admin/admin.auth.ts b/src/models/admin/admin.auth.ts index 8890643..15fdc60 100644 --- a/src/models/admin/admin.auth.ts +++ b/src/models/admin/admin.auth.ts @@ -1,6 +1,6 @@ import {betterAuth} from 'better-auth'; import {APIError} from 'better-auth/api'; -import {passkey} from '@better-auth/passkey'; +import {passkey, getAuthenticatorName} from '@better-auth/passkey'; import {NachklangAdminDB} from './Admin.db.js'; import {invitationsPlugin} from './invitations/invitations.plugin.js'; import {sendPasswordResetMail} from './admin.mail.js'; @@ -116,7 +116,24 @@ export const auth = betterAuth({ passkey({ rpID: PASSKEY_RP_ID, rpName: 'Nachklang', - origin: ADMIN_ALLOWED_ORIGINS + origin: ADMIN_ALLOWED_ORIGINS, + + registration: { + // Without this, every passkey is stored with name = NULL and the + // account page can only label them all "Passkey" - useless at the + // one moment that list matters, when someone has to remove the + // passkey on the device they just lost. + // + // The AAGUID identifies the authenticator *model* (not a device + // and not a person), and better-auth ships the lookup table, so + // this yields "1Password", "iCloud Keychain", "Windows Hello". + // It only fills a blank: a name the client sent always wins, and + // an unknown AAGUID leaves the column NULL as before. + afterVerification: async ({verification}) => { + const name = getAuthenticatorName(verification.registrationInfo?.aaguid); + return name ? {name} : undefined; + } + } }), invitationsPlugin() ], diff --git a/src/models/admin/admin.bootstrap.ts b/src/models/admin/admin.bootstrap.ts index d992349..04f1fc3 100644 --- a/src/models/admin/admin.bootstrap.ts +++ b/src/models/admin/admin.bootstrap.ts @@ -1,6 +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 {ACCESS_ROLE} from './admin.schema.js'; import {ADMIN_APP_URL, ADMIN_BOOTSTRAP_EMAIL, LOG_INVITE_LINKS} from './admin.config.js'; import logger from '../../middleware/logger.js'; @@ -48,7 +49,7 @@ export const bootstrapAdmin = async (): Promise => { const invitation = await InvitationsService.createInvitation( email, 'Nachklang Admin', - ['admin'], + [{app: 'admin', role: ACCESS_ROLE}], null ); diff --git a/src/models/admin/admin.middleware.ts b/src/models/admin/admin.middleware.ts index 2ff7fd7..7890e8f 100644 --- a/src/models/admin/admin.middleware.ts +++ b/src/models/admin/admin.middleware.ts @@ -2,7 +2,7 @@ import express from 'express'; import {fromNodeHeaders} from 'better-auth/node'; import {auth} from './admin.auth.js'; import * as UsersService from './users/users.admin.service.js'; -import {AppName} from './admin.schema.js'; +import {AppName, AppPermission, AppRole} from './admin.schema.js'; import {sendServerError} from './admin.errors.js'; /** @@ -28,6 +28,9 @@ export interface AdminIdentity { export interface AdminAccess extends AdminIdentity { disabled: boolean; + /** Every (app, role) grant. */ + permissions: AppPermission[]; + /** The distinct apps those grants cover. */ apps: AppName[]; } @@ -59,6 +62,7 @@ export const resolveAccess = async (req: express.Request): Promise * what feedback.auth.ts's requireAdminAuth used to be, except that it now * answers 403 for a signed-in user without that app's permission instead of * letting any activated @nachklang.art account in. + * + * The optional second argument narrows it to one role within the app. Nothing + * passes it today - every app has exactly the `access` role - but it is the + * seam a finer permission arrives through. */ -export const requireAppAccess = (app: AppName): express.RequestHandler => { +export const requireAppAccess = (app: AppName, role?: AppRole): express.RequestHandler => { return async (req, res, next) => { try { const access = await resolveAccess(req); @@ -110,7 +118,15 @@ export const requireAppAccess = (app: AppName): express.RequestHandler => { forbidden(res, 'Dieses Konto ist deaktiviert.'); return; } - if (!access.apps.includes(app)) { + + // Without a role this asks "may they open this app at all?", which is + // any grant on it. With one it asks for that specific grant - the hook + // a finer permission plugs into, without touching existing call sites. + const allowed = role === undefined + ? access.apps.includes(app) + : access.permissions.some(permission => permission.app === app && permission.role === role); + + if (!allowed) { forbidden(res, 'Für diesen Bereich fehlt dir die Berechtigung.'); return; } diff --git a/src/models/admin/admin.schema.ts b/src/models/admin/admin.schema.ts index 4ad234a..f43be48 100644 --- a/src/models/admin/admin.schema.ts +++ b/src/models/admin/admin.schema.ts @@ -19,6 +19,87 @@ export const isAppName = (value: unknown): value is AppName => { return typeof value === 'string' && (APP_NAMES as string[]).includes(value); }; +/** + * A permission is (app, role), not just an app. Today every app has exactly one + * role - `access`, "may use this app at all" - so the model looks like a plain + * list of apps and the UI renders one checkbox each. It is written this way + * anyway because the alternative gets expensive fast: `user_app_permissions` + * has primary key (user_id, app, role), so a user can hold several roles for + * the same app, and adding one later is a string in APP_ROLES plus rows - never + * a schema migration and never a change to the shape on the wire. + * + * Note the role is deliberately NOT called `admin`, which is what the column + * defaulted to before: on a `tickets` row that reads as "tickets administrator" + * when it only ever meant "has access", and once real roles exist there would + * be no way to tell the two apart. + */ +export const ACCESS_ROLE = 'access'; + +export type AppRole = string; + +/** Every role that exists, per app, in display order. Extend to add one. */ +export const APP_ROLES: Record = { + calendar: [ACCESS_ROLE], + feedback: [ACCESS_ROLE], + tickets: [ACCESS_ROLE], + admin: [ACCESS_ROLE] +}; + +export interface AppPermission { + app: AppName; + role: AppRole; +} + +export const isAppRole = (app: AppName, role: unknown): role is AppRole => { + return typeof role === 'string' && APP_ROLES[app].includes(role); +}; + +export const isAppPermission = (value: unknown): value is AppPermission => { + if (typeof value !== 'object' || value === null) { + return false; + } + const candidate = value as {app?: unknown; role?: unknown}; + return isAppName(candidate.app) && isAppRole(candidate.app, candidate.role); +}; + +/** + * Normalises whatever a caller sent into a valid, duplicate-free permission + * list. Accepts the richer `{app, role}` form and the plain `AppName` form, + * because `{apps: ['tickets']}` is still what the older callers send and it + * means exactly "tickets at the access role". + */ +export const toPermissions = (value: unknown): AppPermission[] | null => { + if (!Array.isArray(value)) { + return null; + } + + const permissions: AppPermission[] = []; + for (const entry of value) { + if (isAppName(entry)) { + permissions.push({app: entry, role: ACCESS_ROLE}); + } else if (isAppPermission(entry)) { + permissions.push({app: entry.app, role: entry.role}); + } else { + return null; + } + } + + const seen = new Set(); + return permissions.filter(permission => { + const key = `${permission.app}:${permission.role}`; + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +}; + +/** The distinct apps a permission list grants any access to. */ +export const appsOf = (permissions: AppPermission[]): AppName[] => { + return APP_NAMES.filter(app => permissions.some(permission => permission.app === app)); +}; + export interface UserTable { id: string; name: string; @@ -52,7 +133,7 @@ export interface PasskeyTable { export interface UserAppPermissionTable { user_id: string; app: AppName; - role: string; + role: AppRole; granted_by: string | null; granted_at: Generated; } @@ -63,8 +144,9 @@ export interface InvitationTable { email: string; name: string; token_hash: string; - // JSON column holding an AppName[]. - apps: string; + // JSON column holding an AppPermission[]. Older rows may hold a plain + // AppName[]; `parsePermissions` reads both. + permissions: string; invited_by: string | null; created_at: Generated; expires_at: Date; diff --git a/src/models/admin/invitations/invitations.plugin.ts b/src/models/admin/invitations/invitations.plugin.ts index 4904d93..beebab7 100644 --- a/src/models/admin/invitations/invitations.plugin.ts +++ b/src/models/admin/invitations/invitations.plugin.ts @@ -147,7 +147,7 @@ export const invitationsPlugin = () => { return created; }); - await UsersService.setPermissions(user.id, invitation.apps, null); + await UsersService.setPermissions(user.id, invitation.permissions, null); const session = await ctx.context.internalAdapter.createSession(user.id); await setSessionCookie(ctx, {session, user}); diff --git a/src/models/admin/invitations/invitations.router.ts b/src/models/admin/invitations/invitations.router.ts index 8055551..4f8b1d1 100644 --- a/src/models/admin/invitations/invitations.router.ts +++ b/src/models/admin/invitations/invitations.router.ts @@ -1,7 +1,7 @@ import express, {Request, Response} from 'express'; import * as InvitationsService from './invitations.service.js'; import * as UsersService from '../users/users.admin.service.js'; -import {isAppName, AppName} from '../admin.schema.js'; +import {toPermissions} from '../admin.schema.js'; import {sendInvitationMail} from '../admin.mail.js'; import {ADMIN_APP_URL, LOG_INVITE_LINKS} from '../admin.config.js'; import {sendServerError} from '../admin.errors.js'; @@ -64,16 +64,24 @@ invitationsRouter.get('/', async (req: Request, res: Response) => { * application/json: * schema: * type: object - * required: [email, name, apps] + * required: [email, name, permissions] * properties: * email: * type: string * name: * type: string - * apps: + * permissions: * type: array + * description: > + * One entry per (app, role). A plain array of app names is + * accepted too and means the same at the `access` role. * items: - * type: string + * type: object + * properties: + * app: + * type: string + * role: + * type: string * responses: * 201: * description: Invitation created and mailed @@ -86,10 +94,15 @@ invitationsRouter.post('/', async (req: Request, res: Response) => { try { const email = String(req.body?.email || '').trim().toLowerCase(); const name = String(req.body?.name || '').trim(); - const apps: unknown = req.body?.apps; - if (!EMAIL_PATTERN.test(email) || name.length === 0 || !Array.isArray(apps) || !apps.every(isAppName)) { - res.status(400).send({status: 'BAD_REQUEST', message: 'E-Mail, Name und App-Liste sind erforderlich.'}); + // Same two accepted shapes as PUT /admin/users/:id/permissions. + const permissions = toPermissions(req.body?.permissions ?? req.body?.apps); + + if (!EMAIL_PATTERN.test(email) || name.length === 0 || !permissions) { + res.status(400).send({ + status: 'BAD_REQUEST', + message: 'E-Mail, Name und Berechtigungen sind erforderlich.' + }); return; } @@ -107,7 +120,7 @@ invitationsRouter.post('/', async (req: Request, res: Response) => { const invitation = await InvitationsService.createInvitation( email, name, - apps as AppName[], + permissions, res.locals.admin.id ); diff --git a/src/models/admin/invitations/invitations.service.ts b/src/models/admin/invitations/invitations.service.ts index 6d6b2c4..bc6e8d9 100644 --- a/src/models/admin/invitations/invitations.service.ts +++ b/src/models/admin/invitations/invitations.service.ts @@ -1,6 +1,6 @@ import * as crypto from 'crypto'; import {NachklangAdminDB} from '../Admin.db.js'; -import {AppName, APP_NAMES, isAppName} from '../admin.schema.js'; +import {AppPermission, isAppName, isAppPermission, ACCESS_ROLE} from '../admin.schema.js'; const db = NachklangAdminDB.db; @@ -19,7 +19,7 @@ export interface OpenInvitation { id: number; email: string; name: string; - apps: AppName[]; + permissions: AppPermission[]; invitedBy: string | null; createdAt: Date; expiresAt: Date; @@ -29,7 +29,7 @@ export interface AcceptableInvitation { id: number; email: string; name: string; - apps: AppName[]; + permissions: AppPermission[]; } const hashToken = (token: string): string => { @@ -41,11 +41,28 @@ const generateToken = (): string => { return crypto.randomBytes(32).toString('base64url'); }; -const parseApps = (value: unknown): AppName[] => { - // mysql2 hands back a JSON column already parsed; a driver or column-type - // change that turns it into a string must not break the read path. +/** + * Reads the stored permission list. Two shapes are accepted: the current + * `[{app, role}]`, and a bare `['tickets', ...]` from before roles existed, + * which means the same thing at the `access` role. Invitations live for seven + * days, so a deploy that changes the shape has in-flight rows in the old one - + * tolerating both is what stops those invitees from being stranded. + * + * mysql2 hands back a JSON column already parsed; a driver or column-type + * change that turns it into a string must not break the read path either. + */ +const parsePermissions = (value: unknown): AppPermission[] => { const raw = typeof value === 'string' ? JSON.parse(value) : value; - return Array.isArray(raw) ? raw.filter(isAppName) : []; + if (!Array.isArray(raw)) { + return []; + } + + return raw.flatMap((entry): AppPermission[] => { + if (isAppName(entry)) { + return [{app: entry, role: ACCESS_ROLE}]; + } + return isAppPermission(entry) ? [{app: entry.app, role: entry.role}] : []; + }); }; const expiryFromNow = (): Date => { @@ -61,12 +78,12 @@ const expiryFromNow = (): Date => { export const createInvitation = async ( email: string, name: string, - apps: AppName[], + permissions: AppPermission[], invitedBy: string | null ): Promise<{id: number; token: string; expiresAt: Date}> => { const token = generateToken(); const expiresAt = expiryFromNow(); - const validApps = Array.from(new Set(apps)).filter(app => APP_NAMES.includes(app)); + const valid = permissions.filter(isAppPermission); const id = await db.transaction().execute(async trx => { await trx @@ -83,7 +100,7 @@ export const createInvitation = async ( email, name, token_hash: hashToken(token), - apps: JSON.stringify(validApps), + permissions: JSON.stringify(valid), invited_by: invitedBy, created_at: new Date(), expires_at: expiresAt @@ -105,7 +122,7 @@ export const createInvitation = async ( export const findByToken = async (token: string): Promise => { const row = await db .selectFrom('invitations') - .select(['id', 'email', 'name', 'apps']) + .select(['id', 'email', 'name', 'permissions']) .where('token_hash', '=', hashToken(token)) .where('accepted_at', 'is', null) .where('revoked_at', 'is', null) @@ -116,7 +133,7 @@ export const findByToken = async (token: string): Promise => { export const listOpenInvitations = async (): Promise => { const rows = await db .selectFrom('invitations') - .select(['id', 'email', 'name', 'apps', 'invited_by', 'created_at', 'expires_at']) + .select(['id', 'email', 'name', 'permissions', 'invited_by', 'created_at', 'expires_at']) .where('accepted_at', 'is', null) .where('revoked_at', 'is', null) .where('expires_at', '>', new Date()) @@ -160,7 +177,7 @@ export const listOpenInvitations = async (): Promise => { id: row.id, email: row.email, name: row.name, - apps: parseApps(row.apps), + permissions: parsePermissions(row.permissions), invitedBy: row.invited_by, createdAt: row.created_at, expiresAt: row.expires_at diff --git a/src/models/admin/users/users.admin.router.ts b/src/models/admin/users/users.admin.router.ts index 3a1bc35..d88acc0 100644 --- a/src/models/admin/users/users.admin.router.ts +++ b/src/models/admin/users/users.admin.router.ts @@ -1,6 +1,6 @@ import express, {Request, Response} from 'express'; import * as UsersService from './users.admin.service.js'; -import {AppName, isAppName} from '../admin.schema.js'; +import {toPermissions} from '../admin.schema.js'; import {sendServerError} from '../admin.errors.js'; export const usersAdminRouter = express.Router(); @@ -90,26 +90,40 @@ usersAdminRouter.get('/:userId', async (req: Request, res: Response) => { * schema: * type: object * properties: - * apps: + * permissions: * type: array + * description: > + * One entry per (app, role). `access` is the only role today. + * A plain array of app names is also accepted and means the + * same at the `access` role. * items: - * type: string - * enum: [calendar, feedback, tickets, admin] + * type: object + * properties: + * app: + * type: string + * enum: [calendar, feedback, tickets, admin] + * role: + * type: string + * enum: [access] * responses: * 200: * description: Success * 400: - * description: Invalid app name + * description: Invalid app or role * 409: * description: Would lock the last admin out */ usersAdminRouter.put('/:userId/permissions', async (req: Request, res: Response) => { try { const userId = req.params.userId; - const apps: unknown = req.body?.apps; - if (!Array.isArray(apps) || !apps.every(isAppName)) { - res.status(400).send({status: 'BAD_REQUEST', message: 'Ungültige App-Liste.'}); + // `permissions: [{app, role}]` is the real shape; `apps: ['tickets']` is + // accepted as shorthand for the same thing at the `access` role, so a + // caller that predates roles keeps working. + const permissions = toPermissions(req.body?.permissions ?? req.body?.apps); + + if (!permissions) { + res.status(400).send({status: 'BAD_REQUEST', message: 'Ungültige Berechtigungsliste.'}); return; } @@ -119,7 +133,8 @@ 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'); + const keepsAdmin = permissions.some(permission => permission.app === 'admin'); + const losesAdmin = Boolean(target?.apps.includes('admin')) && !keepsAdmin; // Self-lockout is checked here because it needs the caller's identity, // which the service has no business knowing. The last-admin check is @@ -130,7 +145,7 @@ usersAdminRouter.put('/:userId/permissions', async (req: Request, res: Response) return; } - const result = await UsersService.setPermissionsGuarded(userId, apps as AppName[], res.locals.admin.id); + const result = await UsersService.setPermissionsGuarded(userId, permissions, res.locals.admin.id); if (result === 'last-admin') { conflict(res, 'Die letzte Admin-Berechtigung kann nicht entzogen werden.'); return; diff --git a/src/models/admin/users/users.admin.service.ts b/src/models/admin/users/users.admin.service.ts index 50da14b..f2af3ee 100644 --- a/src/models/admin/users/users.admin.service.ts +++ b/src/models/admin/users/users.admin.service.ts @@ -1,6 +1,15 @@ import {Transaction} from 'kysely'; import {NachklangAdminDB} from '../Admin.db.js'; -import {AdminDatabase, AppName, APP_NAMES} from '../admin.schema.js'; +import { + AdminDatabase, + AppName, + AppPermission, + AppRole, + ACCESS_ROLE, + appsOf, + isAppName, + isAppRole +} from '../admin.schema.js'; const db = NachklangAdminDB.db; @@ -18,6 +27,10 @@ export interface UserAccess { email: string; displayName: string; disabled: boolean; + /** Every (app, role) grant this user holds. */ + permissions: AppPermission[]; + /** The distinct apps the above grants any access to. Derived, kept because + * most callers only ever ask "may they open this app at all?". */ apps: AppName[]; } @@ -27,6 +40,7 @@ export interface UserListEntry { id: string; email: string; name: string; + permissions: AppPermission[]; apps: AppName[]; status: UserStatus; createdAt: Date; @@ -61,7 +75,8 @@ export const loadAccess = async (userId: string): Promise => 'user.email as email', 'user.name as name', 'user.disabled as disabled', - 'user_app_permissions.app as app' + 'user_app_permissions.app as app', + 'user_app_permissions.role as role' ]) .execute(); @@ -69,16 +84,32 @@ export const loadAccess = async (userId: string): Promise => return null; } + const permissions = toPermissionRows(rows); + return { id: rows[0].id, email: rows[0].email, displayName: rows[0].name, // MySQL TINYINT(1) comes back as 0/1 through mysql2. disabled: Boolean(rows[0].disabled), - apps: rows.map(row => row.app).filter((app): app is AppName => app !== null) + permissions, + apps: appsOf(permissions) }; }; +/** + * Turns joined permission rows into AppPermission[]. The left join produces one + * row with a null app for a user who holds nothing, and a role written directly + * into the database that no longer appears in APP_ROLES is dropped rather than + * trusted - the table is the store, APP_ROLES is the contract. + */ +const toPermissionRows = (rows: {app: AppName | null; role: string | null}[]): AppPermission[] => { + return rows + .filter((row): row is {app: AppName; role: string} => + isAppName(row.app) && isAppRole(row.app, row.role)) + .map(row => ({app: row.app, role: row.role})); +}; + export const listUsers = async (): Promise => { const users = await db .selectFrom('user') @@ -88,7 +119,7 @@ export const listUsers = async (): Promise => { const permissions = await db .selectFrom('user_app_permissions') - .select(['user_id', 'app']) + .select(['user_id', 'app', 'role']) .execute(); // Last sign-in is derived from the newest session rather than stored: a @@ -107,26 +138,33 @@ export const listUsers = async (): Promise => { .groupBy('userId') .execute(); - const appsByUser = new Map(); + const permissionsByUser = new Map(); for (const row of permissions) { - const apps = appsByUser.get(row.user_id) || []; - apps.push(row.app); - appsByUser.set(row.user_id, apps); + if (!isAppRole(row.app, row.role)) { + continue; + } + const held = permissionsByUser.get(row.user_id) || []; + held.push({app: row.app, role: row.role}); + permissionsByUser.set(row.user_id, held); } const lastSignInByUser = new Map( lastSessions.map(row => [row.userId, row.lastSignInAt as Date | null]) ); - return users.map(user => ({ - id: user.id, - email: user.email, - name: user.name, - apps: appsByUser.get(user.id) || [], - status: user.disabled ? 'deaktiviert' : 'aktiv', - createdAt: user.createdAt, - lastSignInAt: lastSignInByUser.get(user.id) ?? null - })); + return users.map(user => { + const held = permissionsByUser.get(user.id) || []; + return { + id: user.id, + email: user.email, + name: user.name, + permissions: held, + apps: appsOf(held), + status: user.disabled ? ('deaktiviert' as const) : ('aktiv' as const), + createdAt: user.createdAt, + lastSignInAt: lastSignInByUser.get(user.id) ?? null + }; + }); }; export const getUserDetail = async (userId: string): Promise => { @@ -141,7 +179,11 @@ export const getUserDetail = async (userId: string): Promise } const [permissions, sessions, passkeys] = await Promise.all([ - db.selectFrom('user_app_permissions').select('app').where('user_id', '=', userId).execute(), + db + .selectFrom('user_app_permissions') + .select(['app', 'role']) + .where('user_id', '=', userId) + .execute(), db .selectFrom('session') .select(['id', 'createdAt', 'expiresAt', 'ipAddress', 'userAgent']) @@ -156,11 +198,14 @@ export const getUserDetail = async (userId: string): Promise .executeTakeFirst() ]); + const held = toPermissionRows(permissions); + return { id: user.id, email: user.email, name: user.name, - apps: permissions.map(row => row.app), + permissions: held, + apps: appsOf(held), status: user.disabled ? 'deaktiviert' : 'aktiv', createdAt: user.createdAt, lastSignInAt: sessions.length > 0 ? sessions[0].createdAt : null, @@ -174,25 +219,51 @@ export const getUserDetail = async (userId: string): Promise * transaction rather than a diff: the set is at most four rows, and a diff * would only add branches for no measurable gain. */ + +/** The rows a permission list becomes. One row per (app, role). */ +const permissionRows = ( + userId: string, + permissions: AppPermission[], + grantedBy: string | null +) => { + return permissions.map(permission => ({ + user_id: userId, + app: permission.app, + role: permission.role, + granted_by: grantedBy, + granted_at: new Date() + })); +}; + +/** Drops anything not in APP_ROLES and de-duplicates on (app, role). */ +const validPermissions = (permissions: AppPermission[]): AppPermission[] => { + const seen = new Set(); + return permissions.filter(permission => { + if (!isAppName(permission.app) || !isAppRole(permission.app, permission.role)) { + return false; + } + const key = `${permission.app}:${permission.role}`; + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +}; + export const setPermissions = async ( userId: string, - apps: AppName[], + permissions: AppPermission[], grantedBy: string | null ): Promise => { - const unique = Array.from(new Set(apps)).filter(app => APP_NAMES.includes(app)); + const valid = validPermissions(permissions); await db.transaction().execute(async trx => { await trx.deleteFrom('user_app_permissions').where('user_id', '=', userId).execute(); - if (unique.length > 0) { + if (valid.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() - }))) + .values(permissionRows(userId, valid, grantedBy)) .execute(); } }); @@ -217,7 +288,11 @@ const countActiveAdminsForUpdate = async (trx: Transaction): Prom .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')) + // countDistinct, not countAll: with (user_id, app, role) as the key one + // user can hold several roles on `admin`, and counting rows would make a + // single admin with two roles look like two admins - defeating the guard + // at exactly the moment it matters. + .select(({fn}) => fn.count('user_app_permissions.user_id').distinct().as('count')) .forUpdate() .executeTakeFirst(); @@ -230,10 +305,11 @@ const countActiveAdminsForUpdate = async (trx: Transaction): Prom */ export const setPermissionsGuarded = async ( userId: string, - apps: AppName[], + permissions: AppPermission[], grantedBy: string | null ): Promise => { - const unique = Array.from(new Set(apps)).filter(app => APP_NAMES.includes(app)); + const valid = validPermissions(permissions); + const keepsAdmin = valid.some(permission => permission.app === 'admin'); return db.transaction().execute(async trx => { const target = await trx @@ -242,25 +318,20 @@ export const setPermissionsGuarded = async ( .where('user_app_permissions.user_id', '=', userId) .where('user_app_permissions.app', '=', 'admin') .select(['user.disabled as disabled']) + .limit(1) .forUpdate() .executeTakeFirst(); - const losesAdmin = Boolean(target) && !unique.includes('admin'); + const losesAdmin = Boolean(target) && !keepsAdmin; 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) { + if (valid.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() - }))) + .values(permissionRows(userId, valid, grantedBy)) .execute(); } @@ -281,6 +352,7 @@ export const disableUserGuarded = async (userId: string): Promise => { await db .insertInto('user_app_permissions') - .values({user_id: userId, app, role: 'admin', granted_by: grantedBy, granted_at: new Date()}) - .onDuplicateKeyUpdate({role: 'admin'}) + .values({user_id: userId, app, role, granted_by: grantedBy, granted_at: new Date()}) + // The row already existing is the success case - this is "make sure they + // hold it", not "re-grant it" - so nothing is overwritten and granted_by + // keeps naming whoever granted it first. + .onDuplicateKeyUpdate({role}) .execute(); }; diff --git a/test/admin/admin.bootstrap.test.ts b/test/admin/admin.bootstrap.test.ts index 190e7a9..25c24e8 100644 --- a/test/admin/admin.bootstrap.test.ts +++ b/test/admin/admin.bootstrap.test.ts @@ -79,7 +79,12 @@ describe('bootstrapAdmin', () => { await bootstrapAdmin(); - expect(createInvitation).toHaveBeenCalledWith('boss@nachklang.art', 'Nachklang Admin', ['admin'], null); + expect(createInvitation).toHaveBeenCalledWith( + 'boss@nachklang.art', + 'Nachklang Admin', + [{app: 'admin', role: 'access'}], + null + ); expect(mockMail).toHaveBeenCalled(); }); diff --git a/test/admin/admin.middleware.test.ts b/test/admin/admin.middleware.test.ts index 1f542c1..9433dfc 100644 --- a/test/admin/admin.middleware.test.ts +++ b/test/admin/admin.middleware.test.ts @@ -30,6 +30,10 @@ const activeUser = { email: 'a@nachklang.art', displayName: 'A', disabled: false, + permissions: [ + {app: 'feedback', role: 'access'}, + {app: 'admin', role: 'access'} + ], apps: ['feedback', 'admin'] }; @@ -60,6 +64,10 @@ describe('resolveAccess', () => { email: 'a@nachklang.art', displayName: 'A', disabled: false, + permissions: [ + {app: 'feedback', role: 'access'}, + {app: 'admin', role: 'access'} + ], apps: ['feedback', 'admin'] }); // No cookieCache: exactly one lookup per request, never zero. @@ -127,7 +135,11 @@ describe('requireAppAccess', () => { it('403s a signed-in user without that app permission', async () => { mockGetSession.mockResolvedValue({user: {id: 'u1'}}); - mockLoadAccess.mockResolvedValue({...activeUser, apps: ['feedback']}); + mockLoadAccess.mockResolvedValue({ + ...activeUser, + permissions: [{app: 'feedback', role: 'access'}], + apps: ['feedback'] + }); const res = makeRes(); const next = vi.fn(); @@ -171,4 +183,40 @@ describe('requireAppAccess', () => { expect(res.status).toHaveBeenCalledWith(500); expect(next).not.toHaveBeenCalled(); }); + + // The seam a finer per-app permission arrives through. Nothing passes a role + // today, so these two pin the behaviour before there is anything to break. + it('403s when a specific role is required and the user only holds another', async () => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue({ + ...activeUser, + permissions: [{app: 'tickets', role: 'access'}], + apps: ['tickets'] + }); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('tickets', 'refund')(makeReq(), res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('passes when the user holds exactly the required role', async () => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue({ + ...activeUser, + permissions: [ + {app: 'tickets', role: 'access'}, + {app: 'tickets', role: 'refund'} + ], + apps: ['tickets'] + }); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('tickets', 'refund')(makeReq(), res, next); + + expect(next).toHaveBeenCalled(); + }); }); diff --git a/test/admin/admin.schema.test.ts b/test/admin/admin.schema.test.ts new file mode 100644 index 0000000..9a0550b --- /dev/null +++ b/test/admin/admin.schema.test.ts @@ -0,0 +1,99 @@ +import {describe, expect, it} from 'vitest'; +import { + ACCESS_ROLE, + appsOf, + isAppPermission, + isAppRole, + toPermissions +} from '../../src/models/admin/admin.schema.js'; + +/** + * The permission model is (app, role). These tests pin the two properties the + * rest of the module leans on: that the older `['tickets']` shape still means + * "tickets at the access role", and that nothing outside APP_ROLES gets in. + */ + +describe('toPermissions', () => { + it('reads the full (app, role) form', () => { + expect(toPermissions([{app: 'tickets', role: 'access'}])).toEqual([ + {app: 'tickets', role: 'access'} + ]); + }); + + it('reads a plain app list as that app at the access role', () => { + expect(toPermissions(['feedback', 'admin'])).toEqual([ + {app: 'feedback', role: ACCESS_ROLE}, + {app: 'admin', role: ACCESS_ROLE} + ]); + }); + + it('accepts the two forms mixed, which is what a half-migrated caller sends', () => { + expect(toPermissions(['feedback', {app: 'tickets', role: 'access'}])).toEqual([ + {app: 'feedback', role: ACCESS_ROLE}, + {app: 'tickets', role: ACCESS_ROLE} + ]); + }); + + it('drops duplicates of the same (app, role)', () => { + expect(toPermissions(['tickets', {app: 'tickets', role: 'access'}])).toEqual([ + {app: 'tickets', role: ACCESS_ROLE} + ]); + }); + + it('rejects rather than silently dropping an unknown app', () => { + // Silently ignoring it would let "grant calendar + nonsense" look like a + // success while granting less than the caller asked for. + expect(toPermissions(['calendar', 'nonsense'])).toBeNull(); + }); + + it('rejects an unknown role', () => { + expect(toPermissions([{app: 'tickets', role: 'refund'}])).toBeNull(); + }); + + it('rejects anything that is not a list', () => { + expect(toPermissions('admin')).toBeNull(); + expect(toPermissions(null)).toBeNull(); + expect(toPermissions({app: 'admin', role: 'access'})).toBeNull(); + }); + + it('reads an empty list as "no permissions", not as invalid', () => { + expect(toPermissions([])).toEqual([]); + }); +}); + +describe('isAppRole', () => { + it('accepts the access role for every app', () => { + expect(isAppRole('admin', ACCESS_ROLE)).toBe(true); + expect(isAppRole('calendar', ACCESS_ROLE)).toBe(true); + }); + + it('rejects a role that does not exist yet', () => { + expect(isAppRole('tickets', 'refund')).toBe(false); + }); +}); + +describe('isAppPermission', () => { + it('needs both halves to be valid', () => { + expect(isAppPermission({app: 'tickets', role: ACCESS_ROLE})).toBe(true); + expect(isAppPermission({app: 'tickets'})).toBe(false); + expect(isAppPermission({role: ACCESS_ROLE})).toBe(false); + expect(isAppPermission(null)).toBe(false); + }); +}); + +describe('appsOf', () => { + it('collapses several roles on one app to a single entry', () => { + // The point of the derived list: a user with two roles on tickets has + // access to tickets once, not twice. + const apps = appsOf([ + {app: 'tickets', role: ACCESS_ROLE}, + {app: 'tickets', role: 'future-role'}, + {app: 'admin', role: ACCESS_ROLE} + ]); + expect(apps).toEqual(['tickets', 'admin']); + }); + + it('is empty for no permissions', () => { + expect(appsOf([])).toEqual([]); + }); +}); diff --git a/test/admin/users.admin.router.test.ts b/test/admin/users.admin.router.test.ts index df1c95f..1cf6bbe 100644 --- a/test/admin/users.admin.router.test.ts +++ b/test/admin/users.admin.router.test.ts @@ -97,7 +97,37 @@ describe('PUT /admin/users/:id/permissions', () => { const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['tickets']}); expect(res.status).toBe(200); - expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['tickets'], 'me'); + expect(service.setPermissionsGuarded).toHaveBeenCalledWith( + 'other', + [{app: 'tickets', role: 'access'}], + 'me' + ); + }); + + it('accepts the richer {permissions} body', async () => { + service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: []}); + + const res = await request(makeApp('me')) + .put('/admin/users/other/permissions') + .send({permissions: [{app: 'tickets', role: 'access'}]}); + + expect(res.status).toBe(200); + expect(service.setPermissionsGuarded).toHaveBeenCalledWith( + 'other', + [{app: 'tickets', role: 'access'}], + 'me' + ); + }); + + it('rejects a role that does not exist', async () => { + service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: []}); + + const res = await request(makeApp('me')) + .put('/admin/users/other/permissions') + .send({permissions: [{app: 'tickets', role: 'refund'}]}); + + expect(res.status).toBe(400); + expect(service.setPermissionsGuarded).not.toHaveBeenCalled(); }); it('allows granting permissions to someone who has none', async () => { @@ -106,7 +136,11 @@ 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); - expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['feedback', 'tickets'], 'me'); + expect(service.setPermissionsGuarded).toHaveBeenCalledWith( + 'other', + [{app: 'feedback', role: 'access'}, {app: 'tickets', role: 'access'}], + 'me' + ); }); }); diff --git a/test/integration/admin.auth.test.ts b/test/integration/admin.auth.test.ts index 4c3943b..9a14bc6 100644 --- a/test/integration/admin.auth.test.ts +++ b/test/integration/admin.auth.test.ts @@ -9,7 +9,8 @@ import { createAndAcceptInvitation, resetDatabase, sessionCookieFrom, - SESSION_COOKIE + SESSION_COOKIE, + accessTo } from './helpers.js'; /** @@ -66,7 +67,7 @@ describe('invitation acceptance', () => { }); it('sets the session cookie under the configured prefix', async () => { - const invitation = await InvitationsService.createInvitation('b@nachklang.art', 'B', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('b@nachklang.art', 'B', accessTo('feedback'), null); const res = await request(app) .post('/admin/auth/invitations/accept') @@ -89,7 +90,7 @@ describe('invitation acceptance', () => { }); it('previews an invitation without revealing the granted apps', async () => { - const invitation = await InvitationsService.createInvitation('d@nachklang.art', 'D', ['admin'], null); + const invitation = await InvitationsService.createInvitation('d@nachklang.art', 'D', accessTo('admin'), null); const res = await request(app) .post('/admin/auth/invitations/preview') @@ -100,7 +101,7 @@ describe('invitation acceptance', () => { }); it('answers an unknown token exactly like an expired one', async () => { - const invitation = await InvitationsService.createInvitation('e@nachklang.art', 'E', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('e@nachklang.art', 'E', accessTo('feedback'), null); await InvitationsService.revokeInvitation(invitation.id); const unknown = await request(app).post('/admin/auth/invitations/preview').send({token: 'no-such-token'}); @@ -111,7 +112,7 @@ describe('invitation acceptance', () => { }); it('cannot be redeemed twice', async () => { - const invitation = await InvitationsService.createInvitation('f@nachklang.art', 'F', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('f@nachklang.art', 'F', accessTo('feedback'), null); const first = await request(app) .post('/admin/auth/invitations/accept') @@ -125,7 +126,7 @@ describe('invitation acceptance', () => { }); it('rejects an expired invitation', async () => { - const invitation = await InvitationsService.createInvitation('g@nachklang.art', 'G', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('g@nachklang.art', 'G', accessTo('feedback'), null); // Reach past the service to age it: there is deliberately no API for this. const {NachklangAdminDB} = await import('../../src/models/admin/Admin.db.js'); await NachklangAdminDB.db @@ -142,7 +143,7 @@ describe('invitation acceptance', () => { }); it('rejects a password below the minimum length', async () => { - const invitation = await InvitationsService.createInvitation('h@nachklang.art', 'H', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('h@nachklang.art', 'H', accessTo('feedback'), null); const res = await request(app) .post('/admin/auth/invitations/accept') @@ -259,7 +260,7 @@ describe('origin checks', () => { describe('the session cookie is not readable by scripts', () => { it('is HttpOnly and SameSite=Lax', async () => { - const invitation = await InvitationsService.createInvitation('r@nachklang.art', 'R', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('r@nachklang.art', 'R', accessTo('feedback'), null); const res = await request(app) .post('/admin/auth/invitations/accept') .send({token: invitation.token, password: 'devpassword123'}); diff --git a/test/integration/admin.users.test.ts b/test/integration/admin.users.test.ts index ab23430..31f7f3e 100644 --- a/test/integration/admin.users.test.ts +++ b/test/integration/admin.users.test.ts @@ -5,7 +5,7 @@ import {createApp} from '../../src/app.factory.js'; import * as InvitationsService from '../../src/models/admin/invitations/invitations.service.js'; import * as UsersService from '../../src/models/admin/users/users.admin.service.js'; import {bootstrapAdmin} from '../../src/models/admin/admin.bootstrap.js'; -import {closeDatabase, createAndAcceptInvitation, resetDatabase} from './helpers.js'; +import {accessTo, closeDatabase, createAndAcceptInvitation, resetDatabase} from './helpers.js'; let app: Application; @@ -185,7 +185,7 @@ describe('invitations', () => { it('invalidates the previous link on resend', async () => { const {agent} = await signedInAdmin(); - const original = await InvitationsService.createInvitation('new@nachklang.art', 'New', ['feedback'], null); + const original = await InvitationsService.createInvitation('new@nachklang.art', 'New', accessTo('feedback'), null); const resent = await agent.post(`/admin/invitations/${original.id}/resend`); expect(resent.status).toBe(200); @@ -198,7 +198,7 @@ describe('invitations', () => { it('revokes an invitation', async () => { const {agent} = await signedInAdmin(); - const invitation = await InvitationsService.createInvitation('new@nachklang.art', 'New', ['feedback'], null); + const invitation = await InvitationsService.createInvitation('new@nachklang.art', 'New', accessTo('feedback'), null); expect((await agent.delete(`/admin/invitations/${invitation.id}`)).status).toBe(204); expect((await agent.delete(`/admin/invitations/${invitation.id}`)).status).toBe(404); diff --git a/test/integration/helpers.ts b/test/integration/helpers.ts index 1f94aa9..f6dda98 100644 --- a/test/integration/helpers.ts +++ b/test/integration/helpers.ts @@ -3,7 +3,7 @@ import type {Application} from 'express'; import request from 'supertest'; import {NachklangAdminDB} from '../../src/models/admin/Admin.db.js'; import * as InvitationsService from '../../src/models/admin/invitations/invitations.service.js'; -import {AppName} from '../../src/models/admin/admin.schema.js'; +import {ACCESS_ROLE, AppName, AppPermission, toPermissions} from '../../src/models/admin/admin.schema.js'; const db = NachklangAdminDB.db; @@ -44,10 +44,13 @@ export const createAndAcceptInvitation = async ( app: Application, email: string, name: string, - apps: AppName[], + // Takes the shorthand as well as the full form: most tests only care that + // someone can open an app, and `['tickets']` says that with less noise. + grants: (AppName | AppPermission)[], password = 'devpassword123' ) => { - const invitation = await InvitationsService.createInvitation(email, name, apps, null); + const permissions = toPermissions(grants) ?? []; + const invitation = await InvitationsService.createInvitation(email, name, permissions, null); const agent = request.agent(app); const res = await agent @@ -66,3 +69,9 @@ export const cookieHeader = (res: request.Response): string[] => { export const sessionCookieFrom = (res: request.Response): string | undefined => { return cookieHeader(res).find(cookie => cookie.startsWith(SESSION_COOKIE)); }; + +/** `accessTo('feedback')` reads better than the (app, role) literal in tests + * that only care that someone can open an app. */ +export const accessTo = (...apps: AppName[]): AppPermission[] => { + return apps.map(app => ({app, role: ACCESS_ROLE})); +};