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})); +};