From bfb8f2ff9cb54fef77f31657f9ed7c5f87478d13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Thu, 29 Oct 2026 23:24:28 +0100 Subject: [PATCH] Add a read-only role per app for calendar, feedback and tickets A grant can now be `read` instead of `access`: requireAppAccess lets it through for GET/HEAD/OPTIONS and answers 403 to every other method, so new write routes are covered without being marked. `read` and `access` on the same app are rejected together; user management has no read role. /tickets/admin/me and /feedback/admin/me report `readOnly`. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 5 +- docs/admin-roles.md | 73 +++++++++++++ src/models/admin/admin.middleware.ts | 16 ++- src/models/admin/admin.schema.ts | 40 ++++++- src/models/feedback/admin/admin.router.ts | 11 +- src/models/tickets/admin/admin.router.ts | 20 +++- test/admin/admin.middleware.test.ts | 60 ++++++++++- test/admin/admin.schema.test.ts | 41 ++++++++ test/integration/admin.readonly.test.ts | 121 ++++++++++++++++++++++ 9 files changed, 377 insertions(+), 10 deletions(-) create mode 100644 docs/admin-roles.md create mode 100644 test/integration/admin.readonly.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 696493a..adfd463 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,8 +47,9 @@ 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. 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, +`(user_id, app, role)` so one user can hold several roles per app. Two roles exist: `access` +(full access) and `read` (read-only, not for `admin`) - see `docs/admin-roles.md`, including +how `requireAppAccess` enforces `read` by HTTP method; `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 diff --git a/docs/admin-roles.md b/docs/admin-roles.md new file mode 100644 index 0000000..1ba8cf9 --- /dev/null +++ b/docs/admin-roles.md @@ -0,0 +1,73 @@ +# Admin roles: full access and read-only + +Added 2026-10-29. A user's permission is a list of **(app, role)** grants in +`user_app_permissions` (see `src/models/admin/admin.schema.ts`). Each app can now be granted at +one of two levels: + +| Role | UI label (admin app) | Apps | Meaning | +|----------|----------------------|-------------------------------|--------------------------------------------| +| `access` | Vollzugriff | calendar, feedback, tickets, admin | Everything in that app, as before. | +| `read` | Nur lesen | calendar, feedback, tickets | See everything, change nothing. | + +Rules: + +- **`access` and `read` on the same app are mutually exclusive.** `toPermissions` returns `null` + for a list holding both, so `PUT /admin/users/:id/permissions` and `POST /admin/invitations` + answer 400. There is no "higher one wins". +- **`admin` (user management) has no `read` role.** Viewing it means viewing other people's + email addresses, IPs and sessions; that is a different trust level from viewing concerts. +- Existing grants were not migrated: every pre-existing row is `access` and still means full + access. + +## Enforcement + +`requireAppAccess(app)` (no role argument - which is how every app router uses it) does, after +the existing session/disabled/has-any-grant checks: + +- `GET`, `HEAD`, `OPTIONS` - allowed for any grant on the app. +- anything else - allowed only with `access` (`canWrite`). A `read`-only user gets + `403 {status: 'FORBIDDEN', message: 'Du hast für diesen Bereich nur Lesezugriff.'}`. + +This is by method on purpose: a write route added later is covered without being marked. It +relies on one convention that **must stay true**: admin routes read with GET and change with +POST/PUT/PATCH/DELETE. When this was built, every GET handler in tickets, feedback and calendar +was checked to call only read functions (no SQL writes, no mail). A GET with a side effect, or a +POST that only reads, breaks the read-only role in one direction or the other. + +Consequences worth knowing: + +- "Resend confirmation email" (tickets) is a POST and therefore not available read-only - by + decision: it has an effect outside the system. +- CSV exports and reports are GETs and therefore available read-only - also by decision. + Read-only means all data, including personal data. A restricted variant would be a new role. +- `requireAppAccess(app, role)` with an explicit role does **not** apply the method rule: the + caller has already named exactly the grant it needs. + +## Telling the frontend + +- `GET /tickets/admin/me` and `GET /feedback/admin/me` return `readOnly: boolean` alongside + `email` and `fullName`. Frontends hide write controls when it is `true`; the API refuses the + writes regardless. +- `GET /admin/me` returns the full `permissions` list; `isReadOnly(permissions, app)` in + `admin.schema.ts` is the same rule. + +## Calendar + +The API side is done: calendar writes sit behind `requireAppAccess('calendar')` and so refuse a +`read`-only user, and the optional-cookie reads (`/:calendar/json`, `/ical`) check +`access.apps.includes('calendar')`, which a `read` grant satisfies - read-only users see the +restricted calendars. + +The current Angular frontend (`Calendar_Frontend`) was deliberately **not** changed, because it +is being rewritten in Next.js. Until then a read-only calendar user sees the edit buttons and +gets the 403 above when using them. Contract for the rewrite: there is no `/calendar/.../me` +with `readOnly`; read `GET /admin/me` and compute `isReadOnly(permissions, 'calendar')` - or add +a calendar `/me` mirroring tickets/feedback. + +## Adding a finer role later + +`canWrite` checks for `access` specifically, so a new role (e.g. `tickets:redeem`) grants no +writes through the method rule. Watch out: tickets and feedback apply the role-less +`requireAdminAuth` once at the top of their admin router, so that gate would already refuse the +new role's POST before any route-level `requireAppAccess(app, 'redeem')` runs. A finer role +therefore needs `canWrite` (or the top-level gate) taught about it, not just a route-level check. diff --git a/src/models/admin/admin.middleware.ts b/src/models/admin/admin.middleware.ts index 7890e8f..17f1184 100644 --- a/src/models/admin/admin.middleware.ts +++ b/src/models/admin/admin.middleware.ts @@ -2,9 +2,14 @@ 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, AppPermission, AppRole} from './admin.schema.js'; +import {AppName, AppPermission, AppRole, canWrite} from './admin.schema.js'; import {sendServerError} from './admin.errors.js'; +// What a read-only grant may do. Every admin route reads with GET and changes +// with POST/PUT/PATCH/DELETE - keep it that way, or a read-only user either +// loses a view or gains a write. +const READ_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']); + /** * The one authenticator for every admin area in this API. It replaces * feedback.auth.ts and tickets.auth.ts, which each re-implemented the same @@ -131,6 +136,15 @@ export const requireAppAccess = (app: AppName, role?: AppRole): express.RequestH return; } + // The read-only level, enforced by method rather than per route so a + // write route added later is covered without being marked. Only the + // role-less form applies it: a caller asking for a specific role has + // already said exactly what is required. + if (role === undefined && !READ_METHODS.has(req.method) && !canWrite(access.permissions, app)) { + forbidden(res, 'Du hast für diesen Bereich nur Lesezugriff.'); + return; + } + res.locals.admin = access; next(); } catch (e: any) { diff --git a/src/models/admin/admin.schema.ts b/src/models/admin/admin.schema.ts index f43be48..eb7c4c8 100644 --- a/src/models/admin/admin.schema.ts +++ b/src/models/admin/admin.schema.ts @@ -35,13 +35,26 @@ export const isAppName = (value: unknown): value is AppName => { */ export const ACCESS_ROLE = 'access'; +/** + * Read-only access to an app: everything `access` may see, nothing it may + * change. requireAppAccess enforces it by HTTP method - a user holding only + * this role gets GET/HEAD/OPTIONS and a 403 for anything else - so a new write + * route is covered without anyone remembering to mark it. + * + * It and `access` are mutually exclusive on one app (toPermissions rejects a + * list holding both), because "full access and also read-only" has no sensible + * reading. The `admin` app deliberately has no read role: viewing user + * management means viewing other people's addresses, IPs and sessions. + */ +export const READ_ROLE = 'read'; + 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], + calendar: [ACCESS_ROLE, READ_ROLE], + feedback: [ACCESS_ROLE, READ_ROLE], + tickets: [ACCESS_ROLE, READ_ROLE], admin: [ACCESS_ROLE] }; @@ -85,7 +98,7 @@ export const toPermissions = (value: unknown): AppPermission[] | null => { } const seen = new Set(); - return permissions.filter(permission => { + const unique = permissions.filter(permission => { const key = `${permission.app}:${permission.role}`; if (seen.has(key)) { return false; @@ -93,6 +106,11 @@ export const toPermissions = (value: unknown): AppPermission[] | null => { seen.add(key); return true; }); + + // Rejected rather than resolved to either level: whichever one we picked, + // the caller asked for something else. + const contradictory = APP_NAMES.some(app => seen.has(`${app}:${ACCESS_ROLE}`) && seen.has(`${app}:${READ_ROLE}`)); + return contradictory ? null : unique; }; /** The distinct apps a permission list grants any access to. */ @@ -100,6 +118,20 @@ export const appsOf = (permissions: AppPermission[]): AppName[] => { return APP_NAMES.filter(app => permissions.some(permission => permission.app === app)); }; +/** + * Whether these grants allow changing anything in `app`. Only `access` does: + * `read` is the read-only level, and a finer role added later grants exactly + * what its own check says, not writes in general. + */ +export const canWrite = (permissions: AppPermission[], app: AppName): boolean => { + return permissions.some(permission => permission.app === app && permission.role === ACCESS_ROLE); +}; + +/** Holds some grant on `app`, but none that allows changes. */ +export const isReadOnly = (permissions: AppPermission[], app: AppName): boolean => { + return appsOf(permissions).includes(app) && !canWrite(permissions, app); +}; + export interface UserTable { id: string; name: string; diff --git a/src/models/feedback/admin/admin.router.ts b/src/models/feedback/admin/admin.router.ts index 1640c66..3a8013b 100644 --- a/src/models/feedback/admin/admin.router.ts +++ b/src/models/feedback/admin/admin.router.ts @@ -3,6 +3,7 @@ */ import express, {Request, Response} from 'express'; import {requireAdminAuth} from '../feedback.auth.js'; +import {isReadOnly} from '../../admin/admin.schema.js'; import {sendServerError} from '../feedback.errors.js'; import {eventsAdminRouter} from './events.admin.router.js'; import {songsAdminRouter} from './songs.admin.router.js'; @@ -40,13 +41,21 @@ adminRouter.use(requireAdminAuth); * type: string * fullName: * type: string + * readOnly: + * type: boolean + * description: Holds only the read role for feedback - every write answers 403. * 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}); + res.status(200).send({ + email: res.locals.admin.email, + fullName: res.locals.admin.displayName, + // Lets the frontend hide what the API would refuse anyway. + readOnly: isReadOnly(res.locals.admin.permissions, 'feedback') + }); }); /** diff --git a/src/models/tickets/admin/admin.router.ts b/src/models/tickets/admin/admin.router.ts index b7f471c..58e001c 100644 --- a/src/models/tickets/admin/admin.router.ts +++ b/src/models/tickets/admin/admin.router.ts @@ -1,5 +1,6 @@ import express, {Request, Response} from 'express'; import {requireAdminAuth} from '../tickets.auth.js'; +import {isReadOnly} from '../../admin/admin.schema.js'; import {vouchersAdminRouter} from './vouchers.admin.router.js'; import {redemptionsAdminRouter, voucherHistoryRouter} from './redemptions.admin.router.js'; import {eventsAdminRouter} from './events.admin.router.js'; @@ -21,13 +22,30 @@ adminRouter.use(requireAdminAuth); * responses: * 200: * description: Success + * content: + * application/json: + * schema: + * type: object + * properties: + * email: + * type: string + * fullName: + * type: string + * readOnly: + * type: boolean + * description: Holds only the read role for tickets - every write answers 403. * 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}); + res.status(200).send({ + email: res.locals.admin.email, + fullName: res.locals.admin.displayName, + // Lets the frontend hide what the API would refuse anyway. + readOnly: isReadOnly(res.locals.admin.permissions, 'tickets') + }); }); adminRouter.use('/vouchers', vouchersAdminRouter); diff --git a/test/admin/admin.middleware.test.ts b/test/admin/admin.middleware.test.ts index 9433dfc..9171c52 100644 --- a/test/admin/admin.middleware.test.ts +++ b/test/admin/admin.middleware.test.ts @@ -15,7 +15,7 @@ import {requireAppAccess, requireSignedIn, resolveAccess} from '../../src/models const mockGetSession = auth.api.getSession as unknown as Mock; const mockLoadAccess = UsersService.loadAccess as Mock; -const makeReq = (): Request => ({headers: {cookie: 'nachklang.session_token=abc'}} as unknown as Request); +const makeReq = (method = 'GET'): Request => ({method, headers: {cookie: 'nachklang.session_token=abc'}} as unknown as Request); const makeRes = (): Response => { const res: any = {}; @@ -219,4 +219,62 @@ describe('requireAppAccess', () => { expect(next).toHaveBeenCalled(); }); + + describe('read-only grant', () => { + const readOnlyUser = { + ...activeUser, + permissions: [{app: 'tickets', role: 'read'}], + apps: ['tickets'] + }; + + it.each(['GET', 'HEAD', 'OPTIONS'])('lets %s through', async method => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue(readOnlyUser); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('tickets')(makeReq(method), res, next); + + expect(next).toHaveBeenCalled(); + }); + + it.each(['POST', 'PUT', 'PATCH', 'DELETE'])('403s %s', async method => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue(readOnlyUser); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('tickets')(makeReq(method), res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.send).toHaveBeenCalledWith(expect.objectContaining({message: 'Du hast für diesen Bereich nur Lesezugriff.'})); + expect(next).not.toHaveBeenCalled(); + }); + + it('does not let full access to another app write here', async () => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue({ + ...activeUser, + permissions: [{app: 'tickets', role: 'read'}, {app: 'feedback', role: 'access'}], + apps: ['feedback', 'tickets'] + }); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('tickets')(makeReq('POST'), res, next); + + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('lets full access write', async () => { + mockGetSession.mockResolvedValue({user: {id: 'u1'}}); + mockLoadAccess.mockResolvedValue(activeUser); + const res = makeRes(); + const next = vi.fn(); + + await requireAppAccess('feedback')(makeReq('DELETE'), res, next); + + expect(next).toHaveBeenCalled(); + }); + }); }); diff --git a/test/admin/admin.schema.test.ts b/test/admin/admin.schema.test.ts index 9a0550b..6cd73b2 100644 --- a/test/admin/admin.schema.test.ts +++ b/test/admin/admin.schema.test.ts @@ -1,9 +1,12 @@ import {describe, expect, it} from 'vitest'; import { ACCESS_ROLE, + READ_ROLE, appsOf, + canWrite, isAppPermission, isAppRole, + isReadOnly, toPermissions } from '../../src/models/admin/admin.schema.js'; @@ -59,6 +62,44 @@ describe('toPermissions', () => { it('reads an empty list as "no permissions", not as invalid', () => { expect(toPermissions([])).toEqual([]); }); + it('reads the read-only role', () => { + expect(toPermissions([{app: 'tickets', role: READ_ROLE}])).toEqual([ + {app: 'tickets', role: READ_ROLE} + ]); + }); + + it('rejects full and read-only access to the same app together', () => { + expect(toPermissions(['tickets', {app: 'tickets', role: READ_ROLE}])).toBeNull(); + }); + + it('allows full access to one app next to read-only access to another', () => { + expect(toPermissions(['tickets', {app: 'feedback', role: READ_ROLE}])).toEqual([ + {app: 'tickets', role: ACCESS_ROLE}, + {app: 'feedback', role: READ_ROLE} + ]); + }); + + it('rejects read-only access to user management', () => { + expect(toPermissions([{app: 'admin', role: READ_ROLE}])).toBeNull(); + }); +}); + +describe('canWrite / isReadOnly', () => { + it('lets only the access role write', () => { + const permissions = [ + {app: 'tickets' as const, role: ACCESS_ROLE}, + {app: 'feedback' as const, role: READ_ROLE} + ]; + expect(canWrite(permissions, 'tickets')).toBe(true); + expect(canWrite(permissions, 'feedback')).toBe(false); + expect(isReadOnly(permissions, 'tickets')).toBe(false); + expect(isReadOnly(permissions, 'feedback')).toBe(true); + }); + + it('is neither for an app without any grant', () => { + expect(canWrite([], 'calendar')).toBe(false); + expect(isReadOnly([], 'calendar')).toBe(false); + }); }); describe('isAppRole', () => { diff --git a/test/integration/admin.readonly.test.ts b/test/integration/admin.readonly.test.ts new file mode 100644 index 0000000..20bda7b --- /dev/null +++ b/test/integration/admin.readonly.test.ts @@ -0,0 +1,121 @@ +import {describe, it, expect, beforeAll, beforeEach, afterAll} from 'vitest'; +import type {Application} from 'express'; +import {createApp} from '../../src/app.factory.js'; +import {READ_ROLE} from '../../src/models/admin/admin.schema.js'; +import {closeDatabase, createAndAcceptInvitation, resetDatabase} from './helpers.js'; + +/** + * The read-only role end to end: a real session cookie through the real + * middleware into each app's routers. The writes are rejected before any + * handler runs, which is why this suite needs no tickets/feedback/calendar + * database - a 403 here proves the gate held, not that a query failed. + */ + +let app: Application; + +beforeAll(() => { + app = createApp(); +}); + +beforeEach(async () => { + await resetDatabase(); +}); + +afterAll(async () => { + await closeDatabase(); +}); + +const readOnlyEverywhere = async () => { + return createAndAcceptInvitation(app, 'reader@nachklang.art', 'Reader', [ + {app: 'tickets', role: READ_ROLE}, + {app: 'feedback', role: READ_ROLE}, + {app: 'calendar', role: READ_ROLE} + ]); +}; + +describe('read-only access', () => { + it('reports readOnly on each app\'s /me', async () => { + const {agent} = await readOnlyEverywhere(); + + const tickets = await agent.get('/tickets/admin/me'); + const feedback = await agent.get('/feedback/admin/me'); + + expect(tickets.status).toBe(200); + expect(tickets.body.readOnly).toBe(true); + expect(feedback.status).toBe(200); + expect(feedback.body.readOnly).toBe(true); + }); + + it('reports readOnly false for full access', async () => { + const {agent} = await createAndAcceptInvitation(app, 'full@nachklang.art', 'Full', ['tickets']); + + const res = await agent.get('/tickets/admin/me'); + + expect(res.status).toBe(200); + expect(res.body.readOnly).toBe(false); + }); + + it.each([ + ['post', '/tickets/admin/vouchers/wildcard'], + ['post', '/tickets/admin/redemptions/1/resend-confirmation'], + ['patch', '/tickets/admin/redemptions/1'], + ['put', '/tickets/admin/events/1/settings'], + ['delete', '/feedback/admin/submissions/1'], + ['post', '/feedback/admin/events'], + ['post', '/calendar/events'], + ['delete', '/calendar/events/1'] + ] as const)('refuses %s %s', async (method, path) => { + const {agent} = await readOnlyEverywhere(); + + const res = await agent[method](path).send({}); + + expect(res.status).toBe(403); + expect(res.body.message).toBe('Du hast für diesen Bereich nur Lesezugriff.'); + }); + + it('does not open user management', async () => { + const {agent} = await readOnlyEverywhere(); + + expect((await agent.get('/admin/users')).status).toBe(403); + }); +}); + +describe('granting read-only access', () => { + const signedInAdmin = () => createAndAcceptInvitation(app, 'admin@nachklang.art', 'Admin', ['admin']); + + it('stores the read role per app', async () => { + const {agent} = await signedInAdmin(); + const other = await createAndAcceptInvitation(app, 'user@nachklang.art', 'User', ['feedback']); + + const res = await agent + .put(`/admin/users/${other.userId}/permissions`) + .send({permissions: [{app: 'tickets', role: READ_ROLE}, {app: 'feedback', role: 'access'}]}); + + expect(res.status).toBe(200); + expect(res.body.permissions).toEqual(expect.arrayContaining([ + {app: 'tickets', role: READ_ROLE}, + {app: 'feedback', role: 'access'} + ])); + }); + + it('rejects full and read-only access to the same app', async () => { + const {agent} = await signedInAdmin(); + const other = await createAndAcceptInvitation(app, 'user@nachklang.art', 'User', ['feedback']); + + const res = await agent + .put(`/admin/users/${other.userId}/permissions`) + .send({permissions: [{app: 'tickets', role: READ_ROLE}, {app: 'tickets', role: 'access'}]}); + + expect(res.status).toBe(400); + }); + + it('rejects inviting someone with read-only user management', async () => { + const {agent} = await signedInAdmin(); + + const res = await agent + .post('/admin/invitations') + .send({email: 'new@nachklang.art', name: 'New', permissions: [{app: 'admin', role: READ_ROLE}]}); + + expect(res.status).toBe(400); + }); +}); -- 2.54.0