From 2405625f992869e25b4423ae5cbdf3ed7394ba78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Sun, 6 Sep 2026 14:10:42 +0200 Subject: [PATCH 1/2] Put the feedback and tickets admin areas behind the shared identity feedback.auth.ts and tickets.auth.ts each become one binding to requireAppAccess. Everything downstream was already written against requireAdminAuth and res.locals.admin, and both still mean what they meant, so no router or service changed. What changed is the policy: an activated @nachklang.art account is no longer sufficient, an explicit per-app permission is. Three things followed from that and are not obvious from the diff: - APP_ORIGINS gets a production default. It feeds better-auth's trustedOrigins, and this is the first time the tickets and feedback origins matter there - before, the only browser origin that ever reached /admin/auth was the admin app itself. An origin missing from that list fails in a way that is easy to misread: sign-in works, the app works, and only sign-out returns an origin error. - Nothing reads X-Session-* any more; these two files were the last readers, and the calendar module passes its session in query parameters. The headers stay in the CORS allowedHeaders only so a browser still running a pre-cutover bundle gets a clean 401 rather than a preflight failure, and can come out once both frontends are deployed. - 40 admin operations documented a required X-Session-Id/X-Session-Key in swagger. They now declare the AdminSessionCookie scheme the admin module already defined, and each documents a 403 next to its 401. The integration assertions flip as their own comment predicted: one admin cookie opens both /feedback/admin/me and /tickets/admin/me, a user holding only feedback gets 200 and 403 respectively, and a legacy header session gets 401. The unit test that covered the old header authenticator is replaced by one asserting each module is bound to its own app and that neither consults the calendar users service. 163 unit tests and 43 integration tests green. Co-Authored-By: Claude Opus 5 --- src/app.factory.ts | 9 +- src/models/admin/admin.config.ts | 23 +++- src/models/feedback/admin/admin.interface.ts | 13 -- src/models/feedback/admin/admin.router.ts | 13 +- .../feedback/admin/events.admin.router.ts | 62 ++++++---- .../feedback/admin/questions.admin.router.ts | 25 ++-- .../feedback/admin/reports.admin.router.ts | 30 +++-- .../feedback/admin/songs.admin.router.ts | 12 +- src/models/feedback/feedback.auth.ts | 88 ++++---------- src/models/tickets/admin/admin.router.ts | 7 +- .../tickets/admin/events.admin.router.ts | 32 +++-- .../tickets/admin/redemptions.admin.router.ts | 36 ++++-- .../tickets/admin/vouchers.admin.router.ts | 32 +++-- src/models/tickets/tickets.auth.ts | 66 +++------- test/admin/admin.config.test.ts | 34 ++++++ test/admin/auth-binding.ts | 113 ++++++++++++++++++ test/feedback/feedback.auth.test.ts | 97 +++------------ test/integration/admin.auth.test.ts | 32 +++-- test/tickets/tickets.auth.test.ts | 23 ++++ 19 files changed, 439 insertions(+), 308 deletions(-) create mode 100644 test/admin/auth-binding.ts create mode 100644 test/tickets/tickets.auth.test.ts diff --git a/src/app.factory.ts b/src/app.factory.ts index 6b7154d..9261a10 100644 --- a/src/app.factory.ts +++ b/src/app.factory.ts @@ -63,8 +63,13 @@ export const createApp = (): express.Application => { // the dev machine's LAN IP, never "localhost"). Dev-only, same as above. const lanIpRegex = /^http:\/\/(192\.168\.\d{1,3}\.\d{1,3}|10\.\d{1,3}\.\d{1,3}\.\d{1,3}|172\.(1[6-9]|2\d|3[01])\.\d{1,3}\.\d{1,3}):\d+$/; app.use(cors({ - // X-Session-* stay allowed until the calendar module is migrated off the - // legacy header sessions (see docs/calendar-auth-migration.md). + // X-Session-* are no longer read by anything on this side: the step 4 + // cutover took the last two readers (feedback.auth.ts, tickets.auth.ts) + // off them, and the calendar module passes its session in query + // parameters (DEFERRED_SECURITY.md item 1). They stay allowed only so a + // browser still running the pre-cutover tickets or feedback bundle gets + // a clean 401 rather than a CORS preflight failure. Drop them once both + // frontends are deployed - see docs/calendar-auth-migration.md step 5. allowedHeaders: ['Content-Type', 'X-Session-Id', 'X-Session-Key'], // The admin session lives in a cookie, so browsers must be allowed to send // it cross-origin - this is what makes credentials: 'include' work. diff --git a/src/models/admin/admin.config.ts b/src/models/admin/admin.config.ts index 5317ace..75a1a2e 100644 --- a/src/models/admin/admin.config.ts +++ b/src/models/admin/admin.config.ts @@ -73,8 +73,27 @@ const parseList = (value: string | undefined, fallback: string[]): string[] => { return parsed.length > 0 ? parsed : fallback; }; -// The apps whose frontends may talk to /admin/* with credentials. -export const APP_ORIGINS = parseList(process.env.APP_ORIGINS, []).map(origin => origin.replace(/\/$/, '')); +/** + * The apps whose frontends may talk to /admin/* with credentials. + * + * They feed better-auth's `trustedOrigins`, which is what lets the tickets and + * feedback admin areas call /admin/auth/sign-out from their own origin. That + * became load-bearing with the step 4 cutover: before it, the only browser + * origin that ever reached /admin/auth was the admin app itself. + * + * Hence the production default rather than an empty list. An origin missing + * here fails in a way that is easy to misread - sign-in works, the app works, + * and only sign-out returns an origin error - so the two frontends we know + * about are named here and APP_ORIGINS overrides them for a staging host. + * Dev adds the localhost ports separately (see admin.auth.ts). + */ +const DEFAULT_APP_ORIGINS = [ + 'https://tickets.nachklang.art', + 'https://feedback.nachklang.art' +]; + +export const APP_ORIGINS = parseList(process.env.APP_ORIGINS, DEFAULT_APP_ORIGINS) + .map(origin => origin.replace(/\/$/, '')); // Kept in sync by construction rather than by three separate lists: the admin // app itself always counts, and dev adds the local ports. diff --git a/src/models/feedback/admin/admin.interface.ts b/src/models/feedback/admin/admin.interface.ts index 0e3d2c8..3688d10 100644 --- a/src/models/feedback/admin/admin.interface.ts +++ b/src/models/feedback/admin/admin.interface.ts @@ -1,19 +1,6 @@ /** * @swagger * components: - * parameters: - * SessionIdHeader: - * in: header - * name: X-Session-Id - * required: true - * schema: - * type: string - * SessionKeyHeader: - * in: header - * name: X-Session-Key - * required: true - * schema: - * type: string * schemas: * EventAdminSummary: * type: object diff --git a/src/models/feedback/admin/admin.router.ts b/src/models/feedback/admin/admin.router.ts index 5cdf728..1640c66 100644 --- a/src/models/feedback/admin/admin.router.ts +++ b/src/models/feedback/admin/admin.router.ts @@ -26,9 +26,8 @@ adminRouter.use(requireAdminAuth); * summary: Validate the current admin session * description: Used by the Next.js middleware/proxy to gate /admin. Returns the authenticated admin's identity. * tags: [feedback-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * responses: * 200: * description: Success @@ -43,6 +42,8 @@ adminRouter.use(requireAdminAuth); * type: string * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ adminRouter.get('/me', (req: Request, res: Response) => { res.status(200).send({email: res.locals.admin.email, fullName: res.locals.admin.displayName}); @@ -55,9 +56,9 @@ adminRouter.get('/me', (req: Request, res: Response) => { * summary: Delete a single submission * description: Removes the submission and everything under it (its answers, guest book entry, newsletter signup) - for removing an individual abusive or inappropriate entry. Not a bulk moderation tool. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: submissionId * required: true @@ -70,6 +71,8 @@ adminRouter.get('/me', (req: Request, res: Response) => { * description: Unknown submission * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ adminRouter.delete('/submissions/:submissionId', async (req: Request, res: Response) => { try { diff --git a/src/models/feedback/admin/events.admin.router.ts b/src/models/feedback/admin/events.admin.router.ts index 68430a4..517549f 100644 --- a/src/models/feedback/admin/events.admin.router.ts +++ b/src/models/feedback/admin/events.admin.router.ts @@ -18,9 +18,8 @@ export const eventsAdminRouter = express.Router(); * summary: List all events (admin) * description: All events, published or not, past or future, with submission counts. * tags: [feedback-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * responses: * 200: * description: Success @@ -32,13 +31,14 @@ export const eventsAdminRouter = express.Router(); * $ref: '#/components/schemas/EventAdminSummary' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * post: * summary: Create an event * description: Auto-generates the slug from the name and event year; defaults feedback_deadline to event_date + 14 days 23:59:59 unless supplied. * tags: [feedback-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * requestBody: * required: true * content: @@ -68,6 +68,8 @@ export const eventsAdminRouter = express.Router(); * description: Missing required fields * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/', async (req: Request, res: Response) => { try { @@ -101,9 +103,9 @@ eventsAdminRouter.post('/', async (req: Request, res: Response) => { * summary: Get one event (admin) * description: Full event detail including setlist and assigned questions. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -120,12 +122,14 @@ eventsAdminRouter.post('/', async (req: Request, res: Response) => { * description: Unknown event * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * put: * summary: Update an event * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -138,13 +142,15 @@ eventsAdminRouter.post('/', async (req: Request, res: Response) => { * description: Unknown event * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * delete: * summary: Delete an event * description: Refuses with 409 if submissions exist unless ?force=true is passed. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -163,6 +169,8 @@ eventsAdminRouter.post('/', async (req: Request, res: Response) => { * description: Submissions exist and force was not set * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/:eventId', async (req: Request, res: Response) => { try { @@ -214,9 +222,9 @@ eventsAdminRouter.delete('/:eventId', async (req: Request, res: Response) => { * get: * summary: Get an event's setlist * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -227,12 +235,14 @@ eventsAdminRouter.delete('/:eventId', async (req: Request, res: Response) => { * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * post: * summary: Add a song to an event's setlist * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -257,6 +267,8 @@ eventsAdminRouter.delete('/:eventId', async (req: Request, res: Response) => { * description: Missing title * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/:eventId/songs', async (req: Request, res: Response) => { try { @@ -292,9 +304,9 @@ eventsAdminRouter.post('/:eventId/songs', async (req: Request, res: Response) => * summary: Bulk reorder an event's setlist * description: Rewrites song positions as a dense 0..n-1 sequence in one transaction. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -317,6 +329,8 @@ eventsAdminRouter.post('/:eventId/songs', async (req: Request, res: Response) => * description: Reordered * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.put('/:eventId/songs/order', async (req: Request, res: Response) => { try { @@ -334,9 +348,9 @@ eventsAdminRouter.put('/:eventId/songs/order', async (req: Request, res: Respons * get: * summary: Get an event's assigned questions * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -347,13 +361,15 @@ eventsAdminRouter.put('/:eventId/songs/order', async (req: Request, res: Respons * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * put: * summary: Bulk-set an event's assigned questions * description: One transaction - inserts new, updates existing, deletes removed. Keeps the admin UI a simple save-the-whole-list form. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -383,6 +399,8 @@ eventsAdminRouter.put('/:eventId/songs/order', async (req: Request, res: Respons * description: Saved * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/:eventId/questions', async (req: Request, res: Response) => { try { diff --git a/src/models/feedback/admin/questions.admin.router.ts b/src/models/feedback/admin/questions.admin.router.ts index 57c2ca5..ec8c894 100644 --- a/src/models/feedback/admin/questions.admin.router.ts +++ b/src/models/feedback/admin/questions.admin.router.ts @@ -16,9 +16,9 @@ export const questionsAdminRouter = express.Router(); * get: * summary: List the question library * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: query * name: includeArchived * schema: @@ -34,12 +34,13 @@ export const questionsAdminRouter = express.Router(); * $ref: '#/components/schemas/AdminQuestion' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * post: * summary: Create a question * tags: [feedback-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * requestBody: * required: true * content: @@ -61,6 +62,8 @@ export const questionsAdminRouter = express.Router(); * description: Missing or invalid fields * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ questionsAdminRouter.get('/', async (req: Request, res: Response) => { try { @@ -94,9 +97,9 @@ questionsAdminRouter.post('/', async (req: Request, res: Response) => { * summary: Edit a question's label/help text * description: question_type is immutable after creation - the admin UI offers "archive and create new" instead. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: questionId * required: true @@ -123,13 +126,15 @@ questionsAdminRouter.post('/', async (req: Request, res: Response) => { * description: Unknown question * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * delete: * summary: Archive (or hard-delete) a question * description: Archives the question if it has ever been used; hard-deletes it if it has never been assigned to any event. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: questionId * required: true @@ -142,6 +147,8 @@ questionsAdminRouter.post('/', async (req: Request, res: Response) => { * description: Unknown question * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ questionsAdminRouter.put('/:questionId', async (req: Request, res: Response) => { try { diff --git a/src/models/feedback/admin/reports.admin.router.ts b/src/models/feedback/admin/reports.admin.router.ts index 04ece63..2cb88ee 100644 --- a/src/models/feedback/admin/reports.admin.router.ts +++ b/src/models/feedback/admin/reports.admin.router.ts @@ -19,9 +19,9 @@ export const reportsAdminRouter = express.Router(); * summary: Aggregated feedback report for one event * description: Song-pick vote counts, song-rating averages, capped free-text list, guest book count, and newsletter sync counts. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -34,6 +34,8 @@ export const reportsAdminRouter = express.Router(); * description: Unknown event * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ reportsAdminRouter.get('/:eventId/report', async (req: Request, res: Response) => { try { @@ -55,9 +57,9 @@ reportsAdminRouter.get('/:eventId/report', async (req: Request, res: Response) = * summary: Guest Book entries for one event * description: Newest first, paginated. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -81,6 +83,8 @@ reportsAdminRouter.get('/:eventId/report', async (req: Request, res: Response) = * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ reportsAdminRouter.get('/:eventId/guestbook', async (req: Request, res: Response) => { try { @@ -101,9 +105,9 @@ reportsAdminRouter.get('/:eventId/guestbook', async (req: Request, res: Response * summary: Newsletter signups for one event * description: Includes sync_status, so failures can be handled manually. Newest first, paginated. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -127,6 +131,8 @@ reportsAdminRouter.get('/:eventId/guestbook', async (req: Request, res: Response * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ reportsAdminRouter.get('/:eventId/newsletter', async (req: Request, res: Response) => { try { @@ -147,9 +153,9 @@ reportsAdminRouter.get('/:eventId/newsletter', async (req: Request, res: Respons * summary: CSV export of all answers for one event * description: Long format, one row per answer. UTF-8 BOM, `;` separator, RFC 4180 escaping, formula-injection guard. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -162,6 +168,8 @@ reportsAdminRouter.get('/:eventId/newsletter', async (req: Request, res: Respons * text/csv: {} * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ reportsAdminRouter.get('/:eventId/export/responses.csv', async (req: Request, res: Response) => { try { @@ -187,9 +195,9 @@ reportsAdminRouter.get('/:eventId/export/responses.csv', async (req: Request, re * get: * summary: CSV export of Guest Book entries for one event * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -202,6 +210,8 @@ reportsAdminRouter.get('/:eventId/export/responses.csv', async (req: Request, re * text/csv: {} * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ reportsAdminRouter.get('/:eventId/export/guestbook.csv', async (req: Request, res: Response) => { try { diff --git a/src/models/feedback/admin/songs.admin.router.ts b/src/models/feedback/admin/songs.admin.router.ts index d9002fe..c35bb5d 100644 --- a/src/models/feedback/admin/songs.admin.router.ts +++ b/src/models/feedback/admin/songs.admin.router.ts @@ -16,9 +16,9 @@ export const songsAdminRouter = express.Router(); * put: * summary: Edit a song's title/composer * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: songId * required: true @@ -45,13 +45,15 @@ export const songsAdminRouter = express.Router(); * description: Unknown song * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * delete: * summary: Remove a song * description: Past answers keep their song_title_snapshot even after the song is removed. * tags: [feedback-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: songId * required: true @@ -64,6 +66,8 @@ export const songsAdminRouter = express.Router(); * description: Unknown song * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ songsAdminRouter.put('/:songId', async (req: Request, res: Response) => { try { diff --git a/src/models/feedback/feedback.auth.ts b/src/models/feedback/feedback.auth.ts index 9ee26d8..35e34d8 100644 --- a/src/models/feedback/feedback.auth.ts +++ b/src/models/feedback/feedback.auth.ts @@ -1,78 +1,38 @@ -import express from 'express'; -import * as UserService from '../calendar/users/users.service.js'; -import {sendServerError} from './feedback.errors.js'; +import {requireAppAccess} from '../admin/admin.middleware.js'; /** * This file is the ONLY place in the feedback module that knows how admin - * authentication works today. No route handler and no service outside this - * file may import users.service, read session headers, or touch bcrypt. + * authentication works. No route handler and no service outside this file may + * read session headers or resolve a user itself. * - * Today: reuses the existing Calendar users/sessions mechanism. Any - * activated @nachklang.art account may administer feedback — no roles. - * Migrating to Keycloak later means writing a keycloakJwtAuthenticator - * below and changing the one `activeAuthenticator` binding (plus the - * frontend's login route handler) — nothing else in the feedback module - * needs to change. + * Today: the shared admin identity in `src/models/admin/`. A session cookie + * set by /admin/auth on admin.nachklang.art, plus a `feedback` permission on + * the account. Both are re-checked on every request, so disabling a user or + * taking their feedback permission away takes effect immediately. * - * Explicitly forbidden: accepting sessionId/sessionKey from query - * parameters, even "temporarily". That is the exact mistake documented in - * DEFERRED_SECURITY.md item 1 for the Calendar domain, where credentials - * end up in access logs, browser history, proxy logs, and Referer headers. - * Headers only. + * Before 2026-09-06 this was a header session against the calendar users + * table, and any activated @nachklang.art account could administer feedback. + * That is why the swap is a one-line binding: everything downstream only ever + * saw `requireAdminAuth` and `res.locals.admin`, and both still mean what + * they meant. What changed is that access is now granted per user rather than + * implied by having an account. + * + * Explicitly forbidden: accepting session credentials from query parameters, + * even "temporarily". That is the exact mistake documented in + * DEFERRED_SECURITY.md item 1 for the Calendar domain, where credentials end + * up in access logs, browser history, proxy logs, and Referer headers. */ -// The only thing the rest of the feedback module knows about an admin. +// The only thing the rest of the feedback module knows about an admin. The +// shared middleware puts a superset of this on res.locals.admin. export interface AdminIdentity { id: string; email: string; displayName: string; } -// Pluggable strategy: extract + verify credentials from a request. -// Returns the identity, or null if unauthenticated. Throws only on -// infrastructure errors (e.g. the DB being unreachable). -export type AdminAuthenticator = (req: express.Request) => Promise; - -// Current implementation: reads X-Session-Id / X-Session-Key headers, -// delegates to the existing calendar UserService.checkSession(...). -export const sessionHeaderAuthenticator: AdminAuthenticator = async (req) => { - const sessionId = req.header('X-Session-Id'); - const sessionKey = req.header('X-Session-Key'); - if (!sessionId || !sessionKey) { - return null; - } - - const ip = req.ip || ''; - const user = await UserService.checkSession(sessionId, sessionKey, ip); - - // Mirrors the Calendar domain's own convention: a valid session on an - // inactive (not yet activated) account is not sufficient. - if (!user || !user.isActive) { - return null; - } - - return { - id: String(user.userId), - email: user.email, - displayName: user.fullName - }; -}; - -// Swap point: change this one binding to migrate to Keycloak. -export const activeAuthenticator: AdminAuthenticator = sessionHeaderAuthenticator; - // Express middleware used by every admin route. On success: -// res.locals.admin = AdminIdentity, calls next(). On failure: 401. -export const requireAdminAuth: express.RequestHandler = async (req, res, next) => { - try { - const identity = await activeAuthenticator(req); - if (!identity) { - res.status(401).send({status: 'UNAUTHORIZED', message: 'Anmeldung erforderlich.'}); - return; - } - res.locals.admin = identity; - next(); - } catch (e: any) { - sendServerError(res, e); - } -}; +// res.locals.admin = AdminAccess (an AdminIdentity plus permissions), calls +// next(). On failure: 401 when not signed in, 403 when signed in without the +// feedback permission. +export const requireAdminAuth = requireAppAccess('feedback'); diff --git a/src/models/tickets/admin/admin.router.ts b/src/models/tickets/admin/admin.router.ts index dfaf0dc..b7f471c 100644 --- a/src/models/tickets/admin/admin.router.ts +++ b/src/models/tickets/admin/admin.router.ts @@ -16,14 +16,15 @@ adminRouter.use(requireAdminAuth); * get: * summary: Validate the current admin session * tags: [tickets-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * responses: * 200: * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ adminRouter.get('/me', (req: Request, res: Response) => { res.status(200).send({email: res.locals.admin.email, fullName: res.locals.admin.displayName}); diff --git a/src/models/tickets/admin/events.admin.router.ts b/src/models/tickets/admin/events.admin.router.ts index 94642e5..d483a3a 100644 --- a/src/models/tickets/admin/events.admin.router.ts +++ b/src/models/tickets/admin/events.admin.router.ts @@ -11,14 +11,15 @@ export const eventsAdminRouter = express.Router(); * summary: List concerts for the admin event picker * description: Wraps the Calendar module's public-calendar admin listing (includes DRAFT events). * tags: [tickets-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * responses: * 200: * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/', async (req: Request, res: Response) => { try { @@ -35,14 +36,15 @@ eventsAdminRouter.get('/', async (req: Request, res: Response) => { * summary: List public-calendar events not yet added to the ticket shop * description: Source list for the "add a concert" picker - the public calendar holds more than concerts, so events only appear in the ticket shop once explicitly added. * tags: [tickets-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * responses: * 200: * description: Success * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/available', async (req: Request, res: Response) => { try { @@ -58,9 +60,9 @@ eventsAdminRouter.get('/available', async (req: Request, res: Response) => { * get: * summary: Get a concert's voucher/capacity stats * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -75,6 +77,8 @@ eventsAdminRouter.get('/available', async (req: Request, res: Response) => { * $ref: '#/components/schemas/EventStats' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.get('/:eventId/stats', async (req: Request, res: Response) => { try { @@ -91,9 +95,9 @@ eventsAdminRouter.get('/:eventId/stats', async (req: Request, res: Response) => * summary: Set a concert's voucher settings * description: Upserts capacity (null = uncapped), redemption deadline (null = none), and whether to collect a mailing address. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -123,6 +127,8 @@ eventsAdminRouter.get('/:eventId/stats', async (req: Request, res: Response) => * description: Saved * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.put('/:eventId/settings', async (req: Request, res: Response) => { try { @@ -149,9 +155,9 @@ eventsAdminRouter.put('/:eventId/settings', async (req: Request, res: Response) * summary: Remove an event from the ticket shop * description: Deletes its settings row, so it drops out of the picker and reappears in the "add" list. Refused with 409 if vouchers already reference the event - existing vouchers/redemptions stay valid either way. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: eventId * required: true @@ -164,6 +170,8 @@ eventsAdminRouter.put('/:eventId/settings', async (req: Request, res: Response) * description: Vouchers already reference this event * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ eventsAdminRouter.delete('/:eventId/settings', async (req: Request, res: Response) => { try { diff --git a/src/models/tickets/admin/redemptions.admin.router.ts b/src/models/tickets/admin/redemptions.admin.router.ts index 4938536..dd07e29 100644 --- a/src/models/tickets/admin/redemptions.admin.router.ts +++ b/src/models/tickets/admin/redemptions.admin.router.ts @@ -11,9 +11,9 @@ export const redemptionsAdminRouter = express.Router(); * summary: List redemptions (admin) * description: Filterable by event and status (ACTIVE/UNDONE). * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: query * name: eventId * schema: @@ -33,6 +33,8 @@ export const redemptionsAdminRouter = express.Router(); * $ref: '#/components/schemas/RedemptionSummary' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ redemptionsAdminRouter.get('/', async (req: Request, res: Response) => { try { @@ -50,9 +52,9 @@ redemptionsAdminRouter.get('/', async (req: Request, res: Response) => { * get: * summary: Get a single redemption (admin) * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: redemptionId * required: true @@ -65,13 +67,15 @@ redemptionsAdminRouter.get('/', async (req: Request, res: Response) => { * description: Unknown redemption * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled * patch: * summary: Edit a redemption's contact info and/or guest list * description: Only fields present in the body are changed. Growing the guest count is re-checked against the voucher's max guests and the event's remaining capacity. Logs to the audit trail with an optional admin-supplied reason. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: redemptionId * required: true @@ -105,6 +109,8 @@ redemptionsAdminRouter.get('/', async (req: Request, res: Response) => { * description: Not active, exceeds max guests, or exceeds remaining capacity * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ redemptionsAdminRouter.get('/:redemptionId', async (req: Request, res: Response) => { try { @@ -161,9 +167,9 @@ redemptionsAdminRouter.patch('/:redemptionId', async (req: Request, res: Respons * summary: Undo a redemption * description: Reopens the code (back to UNUSED) and marks the redemption UNDONE. Guest data is kept for the audit trail; a later re-redemption creates a new redemption record. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: redemptionId * required: true @@ -186,6 +192,8 @@ redemptionsAdminRouter.patch('/:redemptionId', async (req: Request, res: Respons * description: Redemption is not active * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ redemptionsAdminRouter.post('/:redemptionId/undo', async (req: Request, res: Response) => { try { @@ -211,9 +219,9 @@ redemptionsAdminRouter.post('/:redemptionId/undo', async (req: Request, res: Res * summary: Resend the redemption confirmation email * description: Rebuilds the confirmation email from the stored redemption data and sends it again, then records the outcome on the redemption. Intended for redemptions whose original confirmation email failed. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: redemptionId * required: true @@ -230,6 +238,8 @@ redemptionsAdminRouter.post('/:redemptionId/undo', async (req: Request, res: Res * description: The email relay rejected the send * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ redemptionsAdminRouter.post('/:redemptionId/resend-confirmation', async (req: Request, res: Response) => { try { @@ -259,9 +269,9 @@ redemptionsAdminRouter.post('/:redemptionId/resend-confirmation', async (req: Re * get: * summary: Get a voucher's admin-action audit trail * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: code * required: true @@ -278,6 +288,8 @@ redemptionsAdminRouter.post('/:redemptionId/resend-confirmation', async (req: Re * $ref: '#/components/schemas/AuditLogEntry' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ export const voucherHistoryRouter = express.Router(); voucherHistoryRouter.get('/:code/history', async (req: Request, res: Response) => { diff --git a/src/models/tickets/admin/vouchers.admin.router.ts b/src/models/tickets/admin/vouchers.admin.router.ts index c1f7892..427c305 100644 --- a/src/models/tickets/admin/vouchers.admin.router.ts +++ b/src/models/tickets/admin/vouchers.admin.router.ts @@ -11,9 +11,9 @@ export const vouchersAdminRouter = express.Router(); * summary: List vouchers (admin) * description: Filterable by event and status. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: query * name: eventId * schema: @@ -33,6 +33,8 @@ export const vouchersAdminRouter = express.Router(); * $ref: '#/components/schemas/VoucherCode' * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ vouchersAdminRouter.get('/', async (req: Request, res: Response) => { try { @@ -51,9 +53,8 @@ vouchersAdminRouter.get('/', async (req: Request, res: Response) => { * summary: Batch-generate wildcard codes * description: Generates `quantity` codes sharing the same eligible events and max-guest count, grouped under one batchId. * tags: [tickets-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * requestBody: * required: true * content: @@ -87,6 +88,8 @@ vouchersAdminRouter.get('/', async (req: Request, res: Response) => { * description: Invalid input * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ vouchersAdminRouter.post('/wildcard', async (req: Request, res: Response) => { try { @@ -112,9 +115,8 @@ vouchersAdminRouter.post('/wildcard', async (req: Request, res: Response) => { * summary: Bulk-create personalized codes * description: One code per row (name, email, eligible events, max guests), grouped under one batchId. * tags: [tickets-admin] - * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' + * security: + * - AdminSessionCookie: [] * requestBody: * required: true * content: @@ -147,6 +149,8 @@ vouchersAdminRouter.post('/wildcard', async (req: Request, res: Response) => { * description: Invalid input * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ vouchersAdminRouter.post('/personalized', async (req: Request, res: Response) => { try { @@ -169,9 +173,9 @@ vouchersAdminRouter.post('/personalized', async (req: Request, res: Response) => * get: * summary: Get a single voucher (admin) * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: code * required: true @@ -184,6 +188,8 @@ vouchersAdminRouter.post('/personalized', async (req: Request, res: Response) => * description: Unknown code * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ vouchersAdminRouter.get('/:code', async (req: Request, res: Response) => { try { @@ -205,9 +211,9 @@ vouchersAdminRouter.get('/:code', async (req: Request, res: Response) => { * summary: Void an unredeemed code * description: Only allowed while the code is UNUSED. Logs to the voucher's audit trail. * tags: [tickets-admin] + * security: + * - AdminSessionCookie: [] * parameters: - * - $ref: '#/components/parameters/SessionIdHeader' - * - $ref: '#/components/parameters/SessionKeyHeader' * - in: path * name: code * required: true @@ -230,6 +236,8 @@ vouchersAdminRouter.get('/:code', async (req: Request, res: Response) => { * description: Code is not in UNUSED status * 401: * description: Unauthorized + * 403: + * description: Signed in without the permission for this app, or account disabled */ vouchersAdminRouter.post('/:code/void', async (req: Request, res: Response) => { try { diff --git a/src/models/tickets/tickets.auth.ts b/src/models/tickets/tickets.auth.ts index 4975685..e1ec1a9 100644 --- a/src/models/tickets/tickets.auth.ts +++ b/src/models/tickets/tickets.auth.ts @@ -1,20 +1,23 @@ -import express from 'express'; -import * as UserService from '../calendar/users/users.service.js'; -import {sendServerError} from './tickets.errors.js'; +import {requireAppAccess} from '../admin/admin.middleware.js'; /** * Mirrors the Feedback module's feedback.auth.ts: this is the ONLY place in * the tickets module that knows how admin authentication works. No route - * handler and no service outside this file may import users.service, read - * session headers, or touch bcrypt. + * handler and no service outside this file may read session headers or + * resolve a user itself. * - * Today: reuses the existing Calendar users/sessions mechanism. Any - * activated @nachklang.art account may administer vouchers - no roles, same - * policy as Feedback (see docs/plan-ticket-shop.md). A dedicated - * roles/permissions model is explicitly out of scope for v1. + * Today: the shared admin identity in `src/models/admin/`. A session cookie + * set by /admin/auth on admin.nachklang.art, plus a `tickets` permission on + * the account. Both are re-checked on every request, so disabling a user or + * taking their tickets permission away takes effect immediately. * - * Explicitly forbidden: accepting sessionId/sessionKey from query - * parameters - headers only (see DEFERRED_SECURITY.md item 1). + * Before 2026-09-06 this was a header session against the calendar users + * table, and any activated @nachklang.art account could administer vouchers + * (see docs/plan-ticket-shop.md, which called a roles model out of scope for + * v1). It is in scope now, and lives in the admin module rather than here. + * + * Explicitly forbidden: accepting session credentials from query parameters - + * see DEFERRED_SECURITY.md item 1. */ export interface AdminIdentity { @@ -23,41 +26,6 @@ export interface AdminIdentity { displayName: string; } -export type AdminAuthenticator = (req: express.Request) => Promise; - -export const sessionHeaderAuthenticator: AdminAuthenticator = async (req) => { - const sessionId = req.header('X-Session-Id'); - const sessionKey = req.header('X-Session-Key'); - if (!sessionId || !sessionKey) { - return null; - } - - const ip = req.ip || ''; - const user = await UserService.checkSession(sessionId, sessionKey, ip); - - if (!user || !user.isActive) { - return null; - } - - return { - id: String(user.userId), - email: user.email, - displayName: user.fullName - }; -}; - -export const activeAuthenticator: AdminAuthenticator = sessionHeaderAuthenticator; - -export const requireAdminAuth: express.RequestHandler = async (req, res, next) => { - try { - const identity = await activeAuthenticator(req); - if (!identity) { - res.status(401).send({status: 'UNAUTHORIZED', message: 'Anmeldung erforderlich.'}); - return; - } - res.locals.admin = identity; - next(); - } catch (e: any) { - sendServerError(res, e); - } -}; +// On failure: 401 when not signed in, 403 when signed in without the tickets +// permission. +export const requireAdminAuth = requireAppAccess('tickets'); diff --git a/test/admin/admin.config.test.ts b/test/admin/admin.config.test.ts index c30073f..37ce7fb 100644 --- a/test/admin/admin.config.test.ts +++ b/test/admin/admin.config.test.ts @@ -78,6 +78,40 @@ describe('CLIENT_IP_HEADERS', () => { }); }); +describe('APP_ORIGINS', () => { + beforeEach(() => { + delete process.env.APP_ORIGINS; + }); + + // These reach better-auth's trustedOrigins, and the step 4 cutover made the + // tickets and feedback origins load-bearing: without them their sign-out + // call is rejected while everything else still works. + it('defaults to the two production frontends', async () => { + const config = await loadConfig(); + expect(config.APP_ORIGINS).toEqual([ + 'https://tickets.nachklang.art', + 'https://feedback.nachklang.art' + ]); + }); + + it('is overridden wholesale by the environment, for a staging host', async () => { + process.env.APP_ORIGINS = 'https://tickets.staging.example, https://feedback.staging.example/'; + const config = await loadConfig(); + expect(config.APP_ORIGINS).toEqual([ + 'https://tickets.staging.example', + // Trailing slash stripped: an origin with one never matches. + 'https://feedback.staging.example' + ]); + }); + + it('always includes the admin app itself in ADMIN_ALLOWED_ORIGINS', async () => { + process.env.ADMIN_APP_URL = 'https://admin.nachklang.art'; + const config = await loadConfig(); + expect(config.ADMIN_ALLOWED_ORIGINS).toContain('https://admin.nachklang.art'); + expect(config.ADMIN_ALLOWED_ORIGINS).toContain('https://tickets.nachklang.art'); + }); +}); + describe('isProd', () => { it('is false only for the explicit relaxed environments', async () => { process.env.NODE_ENV = 'development'; diff --git a/test/admin/auth-binding.ts b/test/admin/auth-binding.ts new file mode 100644 index 0000000..21b00b2 --- /dev/null +++ b/test/admin/auth-binding.ts @@ -0,0 +1,113 @@ +import {vi, describe, it, expect, beforeEach, type Mock} from 'vitest'; +import express, {Request, Response} from 'express'; + +/** + * Shared body for the two cutover tests (2026-09-06). feedback.auth.ts and + * tickets.auth.ts used to carry their own header-session authenticator against + * the calendar users table; both are now one binding to the shared admin gate. + * + * What is worth asserting is not how that gate works - admin.middleware.test.ts + * owns that - but that each module is bound to *its own* app, and that neither + * consults the calendar users service any more. The mocks live in the calling + * file because vi.mock is per-module-graph; only the assertions are shared. + */ + +export interface BindingMocks { + /** auth.api.getSession from the mocked admin.auth.js */ + getSession: Mock; + /** loadAccess from the mocked users.admin.service.js */ + loadAccess: Mock; + /** checkSession from the mocked calendar users.service.js */ + checkSession: Mock; +} + +const makeReq = (): Request => ({headers: {cookie: 'nachklang.session_token=abc'}} as unknown as Request); + +const makeRes = (): Response => { + const res: any = {}; + res.status = vi.fn().mockReturnValue(res); + res.send = vi.fn().mockReturnValue(res); + res.locals = {}; + return res as Response; +}; + +const userWith = (...apps: string[]) => ({ + id: 'u1', + email: 'a@nachklang.art', + displayName: 'Anna Admin', + disabled: false, + permissions: apps.map(app => ({app, role: 'access'})), + apps +}); + +export const describeAdminBinding = ( + app: string, + otherApp: string, + middleware: express.RequestHandler, + mocks: () => BindingMocks +): void => { + describe(`${app} requireAdminAuth`, () => { + let m: BindingMocks; + + const run = async () => { + const res = makeRes(); + const next = vi.fn(); + await middleware(makeReq(), res, next); + return {res, next}; + }; + + beforeEach(() => { + m = mocks(); + m.getSession.mockReset(); + m.loadAccess.mockReset(); + m.checkSession.mockReset(); + }); + + it('responds 401 and does not call next() without a session', async () => { + m.getSession.mockResolvedValue(null); + + const {res, next} = await run(); + + expect(res.status).toHaveBeenCalledWith(401); + expect(next).not.toHaveBeenCalled(); + }); + + it(`responds 403 for a signed-in user who only has ${otherApp}`, async () => { + m.getSession.mockResolvedValue({user: {id: 'u1'}}); + m.loadAccess.mockResolvedValue(userWith(otherApp)); + + const {res, next} = await run(); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('responds 403 for a disabled user who still holds the permission', async () => { + m.getSession.mockResolvedValue({user: {id: 'u1'}}); + m.loadAccess.mockResolvedValue({...userWith(app), disabled: true}); + + const {res, next} = await run(); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('sets res.locals.admin and calls next() with the permission', async () => { + m.getSession.mockResolvedValue({user: {id: 'u1'}}); + m.loadAccess.mockResolvedValue(userWith(app)); + + const {res, next} = await run(); + + expect(next).toHaveBeenCalled(); + expect(res.locals.admin).toMatchObject({id: 'u1', email: 'a@nachklang.art', displayName: 'Anna Admin'}); + }); + + it('never falls back to a calendar header session', async () => { + m.getSession.mockResolvedValue(null); + + await run(); + + expect(m.checkSession).not.toHaveBeenCalled(); + }); + }); +}; diff --git a/test/feedback/feedback.auth.test.ts b/test/feedback/feedback.auth.test.ts index a2717c2..9b66930 100644 --- a/test/feedback/feedback.auth.test.ts +++ b/test/feedback/feedback.auth.test.ts @@ -1,88 +1,23 @@ -import {vi, describe, it, expect, beforeEach, type Mock} from 'vitest'; -import {Request, Response} from 'express'; +import {vi, type Mock} from 'vitest'; vi.mock('../../src/models/calendar/users/users.service.js', () => ({ checkSession: vi.fn() })); +vi.mock('../../src/models/admin/admin.auth.js', () => ({ + auth: {api: {getSession: vi.fn()}} +})); +vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({ + loadAccess: vi.fn() +})); import * as UserService from '../../src/models/calendar/users/users.service.js'; -import {requireAdminAuth, sessionHeaderAuthenticator} from '../../src/models/feedback/feedback.auth.js'; +import {auth} from '../../src/models/admin/admin.auth.js'; +import * as UsersService from '../../src/models/admin/users/users.admin.service.js'; +import {requireAdminAuth} from '../../src/models/feedback/feedback.auth.js'; +import {describeAdminBinding} from '../admin/auth-binding.js'; -const mockCheckSession = UserService.checkSession as Mock; - -const makeReq = (headers: Record): Request => { - return { - header: (name: string) => headers[name], - ip: '203.0.113.42' - } as unknown as Request; -}; - -const makeRes = (): Response => { - const res: any = {}; - res.status = vi.fn().mockReturnValue(res); - res.send = vi.fn().mockReturnValue(res); - res.locals = {}; - return res as Response; -}; - -describe('sessionHeaderAuthenticator', () => { - beforeEach(() => mockCheckSession.mockReset()); - - it('returns null when headers are missing', async () => { - const identity = await sessionHeaderAuthenticator(makeReq({})); - expect(identity).toBeNull(); - expect(mockCheckSession).not.toHaveBeenCalled(); - }); - - it('returns null when checkSession finds no user', async () => { - mockCheckSession.mockResolvedValue(null); - const identity = await sessionHeaderAuthenticator(makeReq({'X-Session-Id': '1', 'X-Session-Key': 'k'})); - expect(identity).toBeNull(); - }); - - it('returns null for a valid session on an inactive account', async () => { - mockCheckSession.mockResolvedValue({userId: 1, email: 'a@nachklang.art', fullName: 'A', isActive: false}); - const identity = await sessionHeaderAuthenticator(makeReq({'X-Session-Id': '1', 'X-Session-Key': 'k'})); - expect(identity).toBeNull(); - }); - - it('returns the identity for a valid session on an active account', async () => { - mockCheckSession.mockResolvedValue({userId: 1, email: 'a@nachklang.art', fullName: 'Anna Admin', isActive: true}); - const identity = await sessionHeaderAuthenticator(makeReq({'X-Session-Id': '1', 'X-Session-Key': 'k'})); - expect(identity).toEqual({id: '1', email: 'a@nachklang.art', displayName: 'Anna Admin'}); - }); - - it('passes the session id and key from headers through to checkSession, never from query params', async () => { - mockCheckSession.mockResolvedValue({userId: 1, email: 'a@nachklang.art', fullName: 'A', isActive: true}); - await sessionHeaderAuthenticator(makeReq({'X-Session-Id': '42', 'X-Session-Key': 'sekret'})); - expect(mockCheckSession).toHaveBeenCalledWith('42', 'sekret', '203.0.113.42'); - }); -}); - -describe('requireAdminAuth', () => { - beforeEach(() => mockCheckSession.mockReset()); - - it('responds 401 and does not call next() when unauthenticated', async () => { - mockCheckSession.mockResolvedValue(null); - const req = makeReq({}); - const res = makeRes(); - const next = vi.fn(); - - await requireAdminAuth(req, res, next); - - expect(res.status).toHaveBeenCalledWith(401); - expect(next).not.toHaveBeenCalled(); - }); - - it('sets res.locals.admin and calls next() when authenticated', async () => { - mockCheckSession.mockResolvedValue({userId: 1, email: 'a@nachklang.art', fullName: 'Anna Admin', isActive: true}); - const req = makeReq({'X-Session-Id': '1', 'X-Session-Key': 'k'}); - const res = makeRes(); - const next = vi.fn(); - - await requireAdminAuth(req, res, next); - - expect(next).toHaveBeenCalled(); - expect(res.locals.admin).toEqual({id: '1', email: 'a@nachklang.art', displayName: 'Anna Admin'}); - }); -}); +describeAdminBinding('feedback', 'tickets', requireAdminAuth, () => ({ + getSession: auth.api.getSession as unknown as Mock, + loadAccess: UsersService.loadAccess as Mock, + checkSession: UserService.checkSession as Mock +})); diff --git a/test/integration/admin.auth.test.ts b/test/integration/admin.auth.test.ts index 9a14bc6..468ab23 100644 --- a/test/integration/admin.auth.test.ts +++ b/test/integration/admin.auth.test.ts @@ -221,16 +221,32 @@ describe('requireAppAccess', () => { expect(Array.isArray(res.body)).toBe(true); }); - // Step 2 deliberately does NOT swap the feedback and tickets authenticators: - // they still authenticate against the legacy calendar sessions, so an admin - // cookie means nothing to them yet. This asserts that boundary rather than - // the end state - when step 4 lands, these two expectations become 200/403 - // and this comment goes away. - it('leaves the feedback and tickets admin areas on their legacy authenticator', async () => { + // The step 4 cutover (2026-09-06): the feedback and tickets admin areas now + // sit behind this same gate, so one sign-in reaches every app the user has a + // permission for - and reaches no further. Until step 4 these two returned + // 401 for an admin cookie, because each module still ran its own header + // session against the calendar users table. + it('lets an admin cookie into the feedback and tickets admin areas', async () => { const user = await createAndAcceptInvitation(app, 'o@nachklang.art', 'O', ['feedback', 'tickets']); - expect((await user.agent.get('/feedback/admin/me')).status).toBe(401); - expect((await user.agent.get('/tickets/admin/me')).status).toBe(401); + expect((await user.agent.get('/feedback/admin/me')).status).toBe(200); + expect((await user.agent.get('/tickets/admin/me')).status).toBe(200); + }); + + it('403s each app separately for a user who only holds the other one', async () => { + const user = await createAndAcceptInvitation(app, 'q@nachklang.art', 'Q', ['feedback']); + + expect((await user.agent.get('/feedback/admin/me')).status).toBe(200); + expect((await user.agent.get('/tickets/admin/me')).status).toBe(403); + }); + + it('401s the feedback and tickets admin areas for a legacy header session', async () => { + const res = await request(app) + .get('/feedback/admin/me') + .set('X-Session-Id', '1') + .set('X-Session-Key', 'whatever'); + + expect(res.status).toBe(401); }); }); diff --git a/test/tickets/tickets.auth.test.ts b/test/tickets/tickets.auth.test.ts new file mode 100644 index 0000000..e1f7f0f --- /dev/null +++ b/test/tickets/tickets.auth.test.ts @@ -0,0 +1,23 @@ +import {vi, type Mock} from 'vitest'; + +vi.mock('../../src/models/calendar/users/users.service.js', () => ({ + checkSession: vi.fn() +})); +vi.mock('../../src/models/admin/admin.auth.js', () => ({ + auth: {api: {getSession: vi.fn()}} +})); +vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({ + loadAccess: vi.fn() +})); + +import * as UserService from '../../src/models/calendar/users/users.service.js'; +import {auth} from '../../src/models/admin/admin.auth.js'; +import * as UsersService from '../../src/models/admin/users/users.admin.service.js'; +import {requireAdminAuth} from '../../src/models/tickets/tickets.auth.js'; +import {describeAdminBinding} from '../admin/auth-binding.js'; + +describeAdminBinding('tickets', 'feedback', requireAdminAuth, () => ({ + getSession: auth.api.getSession as unknown as Mock, + loadAccess: UsersService.loadAccess as Mock, + checkSession: UserService.checkSession as Mock +})); -- 2.52.0 From 9811610d55d8be3d8ac88710fca2bae80607fdde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Sun, 6 Sep 2026 14:44:27 +0200 Subject: [PATCH 2/2] Update the deferred-security advice for the cutover DEFERRED_SECURITY.md item 1 still told the calendar to move its session credentials from query parameters into X-Session-Id/X-Session-Key. That was the right advice when two other modules read those headers; both stopped in the cutover, so following it now would build a second mechanism just as the first is being retired. The fix is the shared admin identity, which closes the item outright rather than moving the credential somewhere safer. Also annotates the one assertion in auth-binding.ts that cannot currently fail. It is kept deliberately - it is a tripwire against someone reintroducing a header-session fallback for calendar users who have not been invited yet, which is the shortcut this whole step exists to close - but that was worth saying out loud rather than leaving it to look like an oversight. Co-Authored-By: Claude Opus 5 --- DEFERRED_SECURITY.md | 14 +++++++++++++- test/admin/auth-binding.ts | 5 +++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/DEFERRED_SECURITY.md b/DEFERRED_SECURITY.md index 0321d1a..a933937 100644 --- a/DEFERRED_SECURITY.md +++ b/DEFERRED_SECURITY.md @@ -11,7 +11,19 @@ These items were identified during a security review on 2026-05-02 and conscious `sessionId` and `sessionKey` are currently read from query parameters, which means they appear in server access logs, browser history, proxy logs, and `Referer` headers. -**Fix:** Move to request headers (`X-Session-Id` / `X-Session-Key`) or the request body. Requires a corresponding frontend update. +**Fix (updated 2026-09-06):** Move the calendar onto the shared admin identity - +`requireAppAccess('calendar')` against the better-auth session cookie, per +`docs/calendar-auth-migration.md`. That closes this item outright rather than moving the +credential to a safer place, and it is now the cheaper of the two: the feedback and tickets +modules made the same move on 2026-09-06 for one line each. + +~~Move to request headers (`X-Session-Id` / `X-Session-Key`) or the request body.~~ No longer +the recommendation. Nothing on the server reads those two headers any more - the calendar's +query parameters are the last legacy credential path in the API - so this would build a second +mechanism just as the first is being retired. They survive only in the CORS `allowedHeaders` +list, and only until both frontends are redeployed. + +Either fix requires a corresponding frontend update. > Note: the shared calendar `password` parameter in query params is intentional (iCal clients don't support headers) and is acceptable for the current setup. diff --git a/test/admin/auth-binding.ts b/test/admin/auth-binding.ts index 21b00b2..d50a0c9 100644 --- a/test/admin/auth-binding.ts +++ b/test/admin/auth-binding.ts @@ -102,6 +102,11 @@ export const describeAdminBinding = ( expect(res.locals.admin).toMatchObject({id: 'u1', email: 'a@nachklang.art', displayName: 'Anna Admin'}); }); + // Weaker than it looks and kept deliberately: neither module imports + // checkSession any more, so this cannot fail today. It is a tripwire for + // the change that would matter - someone reintroducing a header-session + // fallback "just for the calendar users who have not been invited yet", + // which is exactly the shortcut the cutover exists to close. it('never falls back to a calendar header session', async () => { m.getSession.mockResolvedValue(null); -- 2.52.0