From d960ac8e24587a7c8ef711f3d6aee880c1b90858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Sun, 6 Sep 2026 22:41:49 +0200 Subject: [PATCH] Fold the pre-deploy review findings into the calendar cutover A fresh-context review before deploying found two things that would have broken production, both in the runbook rather than the code. The deploy order named only migration 003. Production has none of the three - 001 and 002 were only ever applied to the dev database - and the new API reads the columns they add on every request, so following it literally would have 500'd every calendar call including the anonymous feed the public website uses. Step 4 now carries a numbered checklist with a verification query. APP_ORIGINS replaces the code's default list rather than adding to it, so naming calendar.nachklang.art in DEFAULT_APP_ORIGINS is not enough if that variable is set on the vhost - and its failure mode is the quiet one the config already warns about, where everything works except sign-out. Added to the same checklist. Also from the review: The two operands of the read guard on /json/next and /ical were swapped so the password check short-circuits first. They are side-effect free, so the order was free - but the old one put an admin-database query in front of the public feed for any caller holding a .nachklang.art cookie, which is a dependency that feed has never had. Two tests now assert the admin database is not consulted at all. /:calendar/json/next had no route-level test, despite being the endpoint the public website actually calls and the property named as load-bearing. Covered now, along with the rest of its credential matrix. Migrations 001 and 002 gained IF NOT EXISTS. They are applied by hand with no tracking table, so a partial re-run should be a no-op rather than an error that aborts the rest of the paste. Verified by applying all three twice to a throwaway container and diffing against the dev schema. Swagger: two descriptions still claimed authentication was required where the public calendar needs none, the calendar enum omitted `birthdays`, and a `createdBy` request-body field was documented and read but never persisted - misleading in a way that suggests a client can set authorship. Removed. The CORS comment describing the calendar's query-parameter sessions is no longer true and was rewritten. Co-Authored-By: Claude Opus 5 --- docs/calendar-auth-migration.md | 49 +++++++++++++++--- sql/calendar/001_add_admin_user_bridge.sql | 8 +-- .../002_snapshot_legacy_creator_names.sql | 10 ++-- src/app.factory.ts | 15 +++--- src/models/calendar/events/events.router.ts | 50 ++++++++++++------- test/calendar/events.router.test.ts | 37 ++++++++++++++ 6 files changed, 129 insertions(+), 40 deletions(-) diff --git a/docs/calendar-auth-migration.md b/docs/calendar-auth-migration.md index c9a229c..6da4cdd 100644 --- a/docs/calendar-auth-migration.md +++ b/docs/calendar-auth-migration.md @@ -1,7 +1,12 @@ # Migrating the Calendar domain onto the admin identity module -Status: **steps 1-4 done** (2026-09-06), step 2 dropped by decision, part of step 5 brought -forward. Only step 5, the removal of the legacy path, is left. +Status: **steps 1-4 implemented 2026-09-06, not yet merged or deployed.** Step 2 dropped by +decision, part of step 5 brought forward. Only step 5, the removal of the legacy path, is +left to write. + +> Read the deploy checklist under step 4 before applying anything. "Done" below means the +> code exists on a branch, **not** that production has it - and in particular production has +> none of the three migrations. Written 2026-09-05 alongside the admin module (step 2 of `docs/plan-admin-auth.md` in the nachklang-admin repo), which deliberately left the calendar alone. Steps 1-4 of that plan @@ -116,11 +121,41 @@ Each step is meant to leave production working on its own. `test/calendar/credentials.service.test.ts`, the routes themselves in `test/calendar/events.router.test.ts` - so this cannot regress quietly. - **Deploy order:** migration 003, then the API, then the calendar frontend. The frontend is - broken between the last two (its old bundle sends query credentials the new API ignores), - so pick a quiet moment. Production also needs `calendar.nachklang.art` in the admin app's - `NEXT_PUBLIC_ALLOWED_REDIRECT_ORIGINS`, which is a **build-time** value: a rebuild, not a - restart. + ### Deploy checklist + + Production has **none** of the three migrations: 001 and 002 were only ever applied to the + dev database. The API build below selects `created_by_user_id` and `created_by_name` on + every read, so deploying it against a database missing them fails every calendar request + including the anonymous public feed the website uses. In order: + + 1. **Apply `sql/calendar/001`, `002`, `003`, in that order**, against `CALENDAR_DB`. All + three are re-runnable, so applying one that is already applied is a no-op. Verify + before continuing: + `SHOW COLUMNS FROM events LIKE '%by_user%'; SHOW COLUMNS FROM events LIKE '%by_name%';` + - four rows across the two tables, and `created_by_id` nullable. + 2. **Check `APP_ORIGINS` on the API vhost.** `calendar.nachklang.art` is in the code's + default list, but the environment variable *replaces* that list rather than adding to + it - so if it is set at all (the tickets/feedback cutover may have set it), append + `https://calendar.nachklang.art` or the calendar's sign-out will 403 while everything + else works. That is the failure mode the comment in `admin.config.ts` warns about. + 3. **Deploy the API.** + 4. **Deploy the calendar frontend immediately after.** Do not leave a gap - see below. + 5. **Re-run 002's two `UPDATE` statements.** Between step 1 and step 3 the old API was + still writing `created_by_id` with no snapshot; those few rows would otherwise lose + their author at step 5. + 6. **Rebuild the admin app** if `NEXT_PUBLIC_ALLOWED_REDIRECT_ORIGINS` does not already + contain `https://calendar.nachklang.art`. It is a **build-time** value, so a restart + does nothing. + + **The window between steps 3 and 4 does not look broken, which is the danger.** The old + Angular bundle starts by calling `POST /calendar/users/checkSessionValid`, and those + legacy routes are untouched - so it still succeeds and the page renders as signed in. What + the user then sees is an empty event table and saves that silently do nothing. It looks + like the calendar lost its data, not like a deploy in progress. Keep the gap to minutes, + or take the frontend offline for it. + + **One-way door:** any iCal subscription whose URL carries `?sessionId=&sessionKey=` rather + than `?password=` stops working permanently. The shared-password URLs are unaffected. 5. **Drop the legacy path.** Remove `users.service.ts`'s session handling, the `sessions` table, `created_by_id`, and the legacy half of the step 3 read (the `users` join and its `legacy_*` aliases - the snapshot fallback stays, it is what makes dropping the table diff --git a/sql/calendar/001_add_admin_user_bridge.sql b/sql/calendar/001_add_admin_user_bridge.sql index 765d889..35e0bad 100644 --- a/sql/calendar/001_add_admin_user_bridge.sql +++ b/sql/calendar/001_add_admin_user_bridge.sql @@ -26,13 +26,13 @@ -- "Illegal mix of collations" instead of at review time. ALTER TABLE `events` - ADD COLUMN `created_by_user_id` VARCHAR(36) + ADD COLUMN IF NOT EXISTS `created_by_user_id` VARCHAR(36) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL DEFAULT NULL AFTER `created_by_id`, - ADD KEY `events_created_by_user_idx` (`created_by_user_id`); + ADD KEY IF NOT EXISTS `events_created_by_user_idx` (`created_by_user_id`); ALTER TABLE `event_versions` - ADD COLUMN `version_created_by_user_id` VARCHAR(36) + ADD COLUMN IF NOT EXISTS `version_created_by_user_id` VARCHAR(36) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL DEFAULT NULL AFTER `version_created_by_id`, - ADD KEY `event_versions_created_by_user_idx` (`version_created_by_user_id`); + ADD KEY IF NOT EXISTS `event_versions_created_by_user_idx` (`version_created_by_user_id`); diff --git a/sql/calendar/002_snapshot_legacy_creator_names.sql b/sql/calendar/002_snapshot_legacy_creator_names.sql index be08e5b..b11ae6a 100644 --- a/sql/calendar/002_snapshot_legacy_creator_names.sql +++ b/sql/calendar/002_snapshot_legacy_creator_names.sql @@ -17,19 +17,19 @@ -- everywhere. The read path prefers the live admin name, falls back to this -- snapshot, and falls back again to the join until step 5 removes it. -- --- The backfill is written to be idempotent (`WHERE ... IS NULL`) so it can be --- re-run. Step 4's migration does exactly that, to catch anything created --- between this migration and the cutover. +-- The whole file is re-runnable: IF NOT EXISTS on the columns, and the backfill +-- only touches rows with no snapshot yet. Step 4's migration re-runs the +-- backfill, to catch anything created between this migration and the cutover. -- -- No charset clause: unlike 001's id columns these hold display text that is -- only ever compared against other calendar data, so they inherit the tables' -- utf8mb4_general_ci like the columns they are copied from. ALTER TABLE `events` - ADD COLUMN `created_by_name` VARCHAR(255) NULL DEFAULT NULL AFTER `created_by_user_id`; + ADD COLUMN IF NOT EXISTS `created_by_name` VARCHAR(255) NULL DEFAULT NULL AFTER `created_by_user_id`; ALTER TABLE `event_versions` - ADD COLUMN `version_created_by_name` VARCHAR(255) NULL DEFAULT NULL AFTER `version_created_by_user_id`; + ADD COLUMN IF NOT EXISTS `version_created_by_name` VARCHAR(255) NULL DEFAULT NULL AFTER `version_created_by_user_id`; UPDATE `events` e JOIN `users` u ON u.user_id = e.created_by_id diff --git a/src/app.factory.ts b/src/app.factory.ts index 9261a10..3328f90 100644 --- a/src/app.factory.ts +++ b/src/app.factory.ts @@ -63,13 +63,14 @@ 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-* 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. + // X-Session-* are no longer read by anything on this side, and no longer + // sent by anything either: the tickets and feedback cutover took the last + // two readers off them, and the calendar cutover removed the last legacy + // credential path in the API (its session used to travel in query + // parameters - DEFERRED_SECURITY.md item 1, now closed). They stay allowed + // only so a browser still running a pre-cutover tickets or feedback bundle + // gets a clean 401 rather than a CORS preflight failure. Drop them once + // those have aged out - 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/calendar/events/events.router.ts b/src/models/calendar/events/events.router.ts index af8f15f..3d6c25c 100644 --- a/src/models/calendar/events/events.router.ts +++ b/src/models/calendar/events/events.router.ts @@ -89,7 +89,7 @@ const signedInEditor = async (req: Request): Promise => { * required: true * schema: * type: string - * enum: [public, members, choir, management] + * enum: [public, members, choir, management, birthdays] * description: The name of the calendar to get events from * - in: query * name: password @@ -182,7 +182,10 @@ eventsRouter.get('/:calendar/json', async (req: Request, res: Response) => { * /calendar/events/{calendar}/json/next: * get: * summary: Get the next upcoming event from a calendar - * description: Returns the next upcoming event from the specified calendar. Authentication required. + * description: > + * The next upcoming event. The public calendar is open to everyone; the + * others need either a signed-in account with the calendar permission or the + * calendar's shared password. * tags: * - calendar * parameters: @@ -191,7 +194,7 @@ eventsRouter.get('/:calendar/json', async (req: Request, res: Response) => { * required: true * schema: * type: string - * enum: [public, members, choir, management] + * enum: [public, members, choir, management, birthdays] * description: The name of the calendar to get the next event from * - in: query * name: password @@ -270,10 +273,19 @@ eventsRouter.get('/:calendar/json/next', async (req: Request, res: Response) => let calendarId: number = calendarNames.get(calendarName)!.id; - // Signed in, or holding the calendar's shared password. The password path + // Holding the calendar's shared password, or signed in. The password path // is what keeps iCal subscriptions working - a calendar client cannot // send a cookie. - if (!await signedInEditor(req) && ! await CredentialService.hasAccess(calendarName, password)) { + // + // The password is checked FIRST so that `public`, which needs no + // credential at all, short-circuits before signedInEditor runs. Otherwise + // every request from a browser that happens to hold a .nachklang.art + // cookie - which is any signed-in user on any of the four apps - would put + // an admin-database query in front of the anonymous public feed, with no + // timeout. Both operands are side-effect free, so the order is free to + // choose; this order is the one that keeps the public calendar + // independent of the admin database. + if (! await CredentialService.hasAccess(calendarName, password) && !await signedInEditor(req)) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } @@ -304,7 +316,10 @@ eventsRouter.get('/:calendar/json/next', async (req: Request, res: Response) => * /calendar/events/{calendar}/ical: * get: * summary: Get all events from a specific calendar in iCal format - * description: Returns all events from the specified calendar in iCal format for calendar applications. Authentication required. + * description: > + * The calendar in iCal format. The public calendar is open to everyone; the + * others take the calendar's shared password in the query string, which is + * why that mechanism survives - an iCal client cannot send a cookie. * tags: * - calendar * parameters: @@ -313,7 +328,7 @@ eventsRouter.get('/:calendar/json/next', async (req: Request, res: Response) => * required: true * schema: * type: string - * enum: [public, members, choir, management] + * enum: [public, members, choir, management, birthdays] * description: The name of the calendar to get events from * - in: query * name: password @@ -383,10 +398,19 @@ eventsRouter.get('/:calendar/ical', async (req: Request, res: Response) => { let calendarId: number = calendarNames.get(calendarName)!.id; - // Signed in, or holding the calendar's shared password. The password path + // Holding the calendar's shared password, or signed in. The password path // is what keeps iCal subscriptions working - a calendar client cannot // send a cookie. - if (!await signedInEditor(req) && ! await CredentialService.hasAccess(calendarName, password)) { + // + // The password is checked FIRST so that `public`, which needs no + // credential at all, short-circuits before signedInEditor runs. Otherwise + // every request from a browser that happens to hold a .nachklang.art + // cookie - which is any signed-in user on any of the four apps - would put + // an admin-database query in front of the anonymous public feed, with no + // timeout. Both operands are side-effect free, so the order is free to + // choose; this order is the one that keeps the public calendar + // independent of the admin database. + if (! await CredentialService.hasAccess(calendarName, password) && !await signedInEditor(req)) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } @@ -630,9 +654,6 @@ eventsRouter.post('/', requireCalendarAccess, async (req: Request, res: Response * location: * type: string * example: "Musikhochschule, Karlsruhe" - * createdBy: - * type: string - * example: "John Doe" * url: * type: string * example: "https://www.nachklang.art/events/concert" @@ -732,7 +753,6 @@ eventsRouter.put('/:eventId', requireCalendarAccess, async (req: Request, res: R endDateTime: new Date(req.body.endDateTime), createdDate: new Date(), location: req.body.location ?? '', - createdBy: req.body.createdBy ?? '', // LEGACY createdById is deliberately not set: there is no calendar // user id any more, and migration 003 made the column nullable. createdByUserId: admin.id, @@ -809,9 +829,6 @@ eventsRouter.put('/:eventId', requireCalendarAccess, async (req: Request, res: R * location: * type: string * example: "Musikhochschule, Karlsruhe" - * createdBy: - * type: string - * example: "John Doe" * url: * type: string * example: "https://www.nachklang.art/events/concert" @@ -908,7 +925,6 @@ eventsRouter.put('/move/:eventId', requireCalendarAccess, async (req: Request, r endDateTime: new Date(req.body.endDateTime), createdDate: new Date(), location: req.body.location ?? '', - createdBy: req.body.createdBy ?? '', // LEGACY createdById is deliberately not set: there is no calendar // user id any more, and migration 003 made the column nullable. createdByUserId: admin.id, diff --git a/test/calendar/events.router.test.ts b/test/calendar/events.router.test.ts index 6b5ce8e..79c3ef5 100644 --- a/test/calendar/events.router.test.ts +++ b/test/calendar/events.router.test.ts @@ -70,6 +70,7 @@ beforeEach(() => { process.env.MEMBER_CREDENTIAL = 'member-secret'; (EventService.getAllEvents as any).mockResolvedValue([]); (EventService.getAllEventsAdmin as any).mockResolvedValue([]); + (EventService.getNextUpcomingEvent as any).mockResolvedValue({eventId: 1, name: 'Konzert'}); (EventService.createEvent as any).mockResolvedValue(1); (EventService.updateEvent as any).mockResolvedValue(1); (EventService.moveEvent as any).mockResolvedValue(true); @@ -127,6 +128,42 @@ describe('reading', () => { await request(app).get('/calendar/events/public/json').expect(200); }); + // The endpoint www.nachklang.art actually calls for its next-event teaser. + // Tested separately from /json because it takes a different code path - it + // has no admin view and no editor branch - so covering /json proves nothing + // about it, and its failure is invisible until someone notices the website + // has gone quiet. + it('serves the next upcoming event anonymously on the public calendar', async () => { + await request(app).get('/calendar/events/public/json/next').expect(200); + + // And without asking the admin database who the caller is: the public + // feed must not acquire a dependency it has never had. + expect(auth.api.getSession).not.toHaveBeenCalled(); + }); + + it('refuses the next upcoming event on a restricted calendar without a credential', async () => { + await request(app).get('/calendar/events/members/json/next').expect(403); + }); + + it('serves the next upcoming event to a shared password', async () => { + await request(app) + .get('/calendar/events/members/json/next') + .query({password: 'member-secret'}) + .expect(200); + }); + + it('serves the next upcoming event to a signed-in editor', async () => { + signedInAs(['calendar']); + + await request(app).get('/calendar/events/members/json/next').expect(200); + }); + + it('does not consult the admin database for the anonymous public iCal export', async () => { + await request(app).get('/calendar/events/public/ical').expect(200); + + expect(auth.api.getSession).not.toHaveBeenCalled(); + }); + it('keeps the shared password working on the iCal export', async () => { (EventService.getAllEvents as any).mockResolvedValue([]);