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([]);