diff --git a/docker/init/01-calendar-schema-dev.sql b/docker/init/01-calendar-schema-dev.sql index e70493b..da03336 100644 --- a/docker/init/01-calendar-schema-dev.sql +++ b/docker/init/01-calendar-schema-dev.sql @@ -1,5 +1,8 @@ -- Local dev only. Real schema, provided directly by the repo owner -- (calendars, events, event_versions, sessions, users) - not a guess. +-- Columns added by this repo's own migrations under sql/calendar/ are folded +-- in here rather than appended, so a fresh dev container matches production +-- after every migration has been applied. Keep the two in step. USE nachklang_calendar; CREATE TABLE `calendars` ( @@ -39,9 +42,14 @@ CREATE TABLE `events` ( `uuid` text NOT NULL, `created_date` datetime DEFAULT current_timestamp(), `created_by_id` int(11) NOT NULL, + -- Bridge to the admin module's user ids; see sql/calendar/001_add_admin_user_bridge.sql. + `created_by_user_id` varchar(36) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL, + -- Archived creator name; see sql/calendar/002_snapshot_legacy_creator_names.sql. + `created_by_name` varchar(255) DEFAULT NULL, PRIMARY KEY (`event_id`), KEY `events_calendars_calendar_id_fk` (`calendar_id`), KEY `events_users_user_id_fk` (`created_by_id`), + KEY `events_created_by_user_idx` (`created_by_user_id`), CONSTRAINT `events_calendars_calendar_id_fk` FOREIGN KEY (`calendar_id`) REFERENCES `calendars` (`calendar_id`), CONSTRAINT `events_users_user_id_fk` FOREIGN KEY (`created_by_id`) REFERENCES `users` (`user_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; @@ -58,11 +66,16 @@ CREATE TABLE `event_versions` ( `location` text DEFAULT NULL, `url` text DEFAULT NULL, `version_created_by_id` int(11) DEFAULT NULL, + -- Bridge to the admin module's user ids; see sql/calendar/001_add_admin_user_bridge.sql. + `version_created_by_user_id` varchar(36) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL, + -- Archived editor name; see sql/calendar/002_snapshot_legacy_creator_names.sql. + `version_created_by_name` varchar(255) DEFAULT NULL, `status` text DEFAULT NULL, `version_created_at` datetime DEFAULT current_timestamp(), PRIMARY KEY (`event_version_id`), KEY `event_versions_events_event_id_fk` (`event_id`), KEY `event_versions_users_user_id_fk` (`version_created_by_id`), + KEY `event_versions_created_by_user_idx` (`version_created_by_user_id`), CONSTRAINT `event_versions_events_event_id_fk` FOREIGN KEY (`event_id`) REFERENCES `events` (`event_id`) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT `event_versions_users_user_id_fk` FOREIGN KEY (`version_created_by_id`) REFERENCES `users` (`user_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; @@ -78,12 +91,16 @@ INSERT INTO calendars (calendar_id, name, includes_calendars) VALUES INSERT INTO users (email, password_hash, full_name, is_active) VALUES ('dev@nachklang.art', '$2b$10$vmj7POS/68SGE.eI7pGjMegrw0vNNZ2HVSUTra5NRsl8iOLwiMgZK', 'Dev Admin', 1); -INSERT INTO events (calendar_id, uuid, created_by_id) VALUES - (1, UUID(), 1), - (1, UUID(), 1), - (1, UUID(), 1); +-- Two rows are left on the legacy path and one carries an admin user id, so +-- dev exercises both branches of the step 3 dual-read rather than only the +-- happy one. It is deliberately a PUBLIC event, so the anonymous listing the +-- website uses covers both. The id is the dev admin from 04-admin-schema.sql. +INSERT INTO events (calendar_id, uuid, created_by_id, created_by_user_id, created_by_name) VALUES + (1, UUID(), 1, NULL, 'Dev Admin'), + (1, UUID(), 1, 'dev-user-0000-0000-0000-000000000001', NULL), + (1, UUID(), 1, NULL, 'Dev Admin'); -INSERT INTO event_versions (event_id, name, description, start_datetime, end_datetime, whole_day, location, url, status, version_created_by_id) VALUES - (1, 'Frühlingskonzert 2026', 'Erstes Konzert der Reihe', '2026-04-18 19:00:00', '2026-04-18 21:00:00', 0, 'Musikhochschule, Karlsruhe', 'https://www.nachklang.art/events/fruehlingskonzert-2026', 'PUBLIC', 1), - (2, 'Sommerkonzert 2026', 'Zweites Konzert der Reihe', '2026-07-11 19:00:00', '2026-07-11 21:00:00', 0, 'Christuskirche, Karlsruhe', 'https://www.nachklang.art/events/sommerkonzert-2026', 'PUBLIC', 1), - (3, 'Adventskonzert 2026', 'Drittes Konzert der Reihe', '2026-12-05 19:00:00', '2026-12-05 21:00:00', 0, 'Stadtkirche, Karlsruhe', 'https://www.nachklang.art/events/adventskonzert-2026', 'DRAFT', 1); +INSERT INTO event_versions (event_id, name, description, start_datetime, end_datetime, whole_day, location, url, status, version_created_by_id, version_created_by_user_id, version_created_by_name) VALUES + (1, 'Frühlingskonzert 2026', 'Erstes Konzert der Reihe', '2026-04-18 19:00:00', '2026-04-18 21:00:00', 0, 'Musikhochschule, Karlsruhe', 'https://www.nachklang.art/events/fruehlingskonzert-2026', 'PUBLIC', 1, NULL, 'Dev Admin'), + (2, 'Sommerkonzert 2026', 'Zweites Konzert der Reihe', '2026-07-11 19:00:00', '2026-07-11 21:00:00', 0, 'Christuskirche, Karlsruhe', 'https://www.nachklang.art/events/sommerkonzert-2026', 'PUBLIC', 1, 'dev-user-0000-0000-0000-000000000001', NULL), + (3, 'Adventskonzert 2026', 'Drittes Konzert der Reihe', '2026-12-05 19:00:00', '2026-12-05 21:00:00', 0, 'Stadtkirche, Karlsruhe', 'https://www.nachklang.art/events/adventskonzert-2026', 'DRAFT', 1, NULL, 'Dev Admin'); diff --git a/docs/calendar-auth-migration.md b/docs/calendar-auth-migration.md index ac3792f..63dc449 100644 --- a/docs/calendar-auth-migration.md +++ b/docs/calendar-auth-migration.md @@ -1,8 +1,12 @@ # Migrating the Calendar domain onto the admin identity module -Status: **not started.** 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. +Status: **steps 1 and 3 done** (2026-09-06), step 2 dropped by decision, part of step 5 +brought forward, step 4 next. + +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 +are now live, so the calendar is the last module still on the legacy query-parameter +sessions. ## Why the calendar was left out @@ -38,37 +42,114 @@ permissions can be granted before anything else moves. Each step is meant to leave production working on its own. -1. **Add a bridging column.** `ALTER TABLE events ADD COLUMN created_by_user_id - VARCHAR(36) NULL`, indexed. Nothing reads it yet. -2. **Map the accounts.** For every legacy `users` row that should survive, invite the - person through the admin UI. On acceptance, backfill `events.created_by_user_id` from - `events.created_by_id` via an email-to-new-id mapping. Everyone not re-invited keeps - working on the legacy path until step 4. -3. **Dual-read.** Change `events.service.ts` to prefer `created_by_user_id` and fall back - to `created_by_id`. Writes fill both. This is the only step that is temporary code, and - it should carry a removal note pointing at step 5. +1. **Add a bridging column.** ~~`ALTER TABLE events ADD COLUMN created_by_user_id + VARCHAR(36) NULL`, indexed. Nothing reads it yet.~~ **Done 2026-09-06**, as + `sql/calendar/001_add_admin_user_bridge.sql` - the first migration this repo owns for the + calendar schema, mirrored into `docker/init/01-calendar-schema-dev.sql`. It covers both + `events.created_by_user_id` and `event_versions.version_created_by_user_id`, and carries + no foreign key (see "What the code actually looks like" below). The dev seed leaves two + events on the legacy path and gives one an admin id, so step 3's dual-read has both cases + to exercise. Verified by applying the pre-migration schema and then the migration to a + throwaway MariaDB 11 container, and diffing `SHOW CREATE TABLE` against a fresh dev + schema: identical. Applied to the running dev database on the same day; a dev container + created before then needs it applied, or recreating. +2. ~~**Map the accounts.**~~ **Dropped 2026-09-06.** There is no backfill: since the + creator is only ever a display name (see below), old events keep resolving through the + legacy join until step 5 and then simply lose the name. Re-inviting the people who + actually still need calendar access remains an operational task, but it is no longer a + migration step and nothing is blocked on it. +3. **Dual-read.** ~~Change `events.service.ts` to prefer `created_by_user_id` and fall back + to `created_by_id`. Writes fill both.~~ **Done 2026-09-06.** `events.service.ts` now reads + both columns and prefers the admin one, resolving the name through a single + `findDisplayNames` lookup against the admin database per result set (added to + `users.admin.service.ts` for this). Four copies of the same SELECT and four copies of the + row mapper were collapsed into one of each first - the dual read would otherwise have had + to be written four times. + + A name now has three possible sources, tried weakest first: the legacy join, then the + `created_by_name` snapshot from migration 002, then the live admin lookup - which wins + because it is the only one that follows an account being renamed. An admin id that no + longer resolves falls back rather than blanking, and a failure to reach the admin database + is caught and logged rather than propagated, so an anonymous read of the public calendar + never depends on the admin database being up. Covered by + `test/calendar/events.service.test.ts`. + + **Writes are not dual-written**, contrary to the original plan: before the cutover the + request only ever carries a legacy session, so there is no admin id available to write. + Writes start filling `created_by_user_id` (and stop filling `created_by_id`) in step 4. 4. **Switch the routes.** Replace the query-parameter session checks in `events.router.ts` and `users.router.ts` with `requireAppAccess('calendar')`, and change the Angular frontend to `withCredentials: true` against the same origin list. Deploy the API first; the calendar frontend is broken between the two deploys, so pick a quiet time. This closes `DEFERRED_SECURITY.md` item 1. + + Two things this step has to carry that the original sequence put in step 5: + + - **`sql/calendar/003_*.sql` must make `events.created_by_id` nullable** (`MODIFY + created_by_id INT NULL`). It is `NOT NULL` today, so the first event created after the + cutover would otherwise fail to insert - there is no legacy int id to write any more. + `event_versions.version_created_by_id` is already nullable. The foreign key can stay + until step 5; it permits NULL. The same migration should re-run 002's idempotent + name backfill, to catch anything created between the two deploys. + - **The public calendar must stay anonymous.** `hasAccess('public')` returns true before + any credential check, and nachklang.art reads `/calendar/events/public/json` and + `/public/json/next` with no session at all. Pinned by + `test/calendar/credentials.service.test.ts` so this cannot regress quietly. 5. **Drop the legacy path.** Remove `users.service.ts`'s session handling, the `sessions` - table, `created_by_id`, and the dual-read from step 3. Legacy `/calendar/users/*` stays + 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 + safe). The names were archived ahead of time by + `sql/calendar/002_snapshot_legacy_creator_names.sql`, so nothing is lost here. Legacy `/calendar/users/*` stays only if something still calls it - otherwise delete it too. `X-Session-Id` / `X-Session-Key` can then come out of the CORS `allowedHeaders` list in `src/app.factory.ts`. +## What the code actually looks like (surveyed 2026-09-06) + +Four things found while doing step 1 that change how the later steps should be built: + +- **`created_by_id` is display-only.** Nothing authorises on it. `events.router.ts` gates + PUT, POST, DELETE and `/move` on `user?.isActive` alone - there is no "only the creator may + edit" rule anywhere - and the column is read back solely to render `created_by_name` and + `last_modified_by_name`. That de-risks steps 2, 3 and 5 considerably: an event whose + creator never gets re-invited loses a name in the UI, it does not become uneditable or + invisible. It also means the step 2 backfill is best-effort, not a precondition. +- **The two schemas are separate databases.** `nachklang_calendar` and `nachklang_admin` + have their own connection pools (`Calendar.db.ts` vs the admin module's Kysely instance). + So the bridging columns get no foreign key, and - the part the original sequence missed - + **the `LEFT OUTER JOIN users` that produces the creator's name cannot simply be repointed**. + It would have to become a cross-schema join, which hardcodes the admin database name into + calendar SQL and ties the two schemas together exactly as an FK would. Recommendation for + step 3: drop the join for the new path and resolve names in the service layer instead - + collect the distinct ids from the result set and do one lookup against the admin users + service. One extra query per listing, no coupling, and it keeps working if the admin + database ever moves. +- **`events.created_by_id` is `NOT NULL`.** Step 5 cannot simply stop writing it; that step + has to drop the column (and its FK to `users`) in the same migration that stops the writes, + or make it nullable first. +- **Every calendar read already hits the session table.** `/:calendar/json` calls + `UserService.checkSession` before falling back to `credentials.service.ts`, so the shared + credentials are the *fallback*, not the primary path. Step 4 replaces the first half of + that with `requireAppAccess('calendar')` and has to decide what happens to the second half + - which is the first open question below. + ## Open questions to settle before starting -- **The shared calendar credentials.** Do `MEMBER_CREDENTIAL` and friends stay as a - separate mechanism (they serve people with no account at all, and iCal clients that - cannot send headers), or do read-only accounts replace them? This is a product decision, - not a technical one, and it decides how much of `credentials.service.ts` survives. -- **The iCal export.** `GET /calendar/events/{calendar}/ical` takes a password in the query - string on purpose, because iCal clients cannot send headers. Cookie sessions do not help - here; this endpoint likely keeps its own scheme. -- **Which legacy accounts to keep.** Step 2 is the moment to not re-invite people who no - longer need access. -- **`event_versions.version_created_by_id`.** The same INT reference again, joined in - `events.service.ts` for the "last modified by" name. It has to move with `events`, and it - is the reason step 1's bridging column needs a sibling on `event_versions`. +**Settled 2026-09-06:** + +- **The shared calendar credentials keep working, but only for iCal.** The web app goes + cookie-only at step 4; `MEMBER_CREDENTIAL` and friends survive on + `GET /calendar/events/{calendar}/ical`, which is the one case where the client genuinely + cannot send a cookie. Everything else in `credentials.service.ts` goes with step 5. + `public` stays anonymous everywhere - see the note under step 4. +- **The iCal export keeps its own scheme.** Same reasoning; it is the reason the shared + credentials survive at all rather than an exception to their removal. +- **No account backfill.** See step 2 above. +- **Pre-cutover authorship is archived, not discarded.** `events.created_by_name` and + `event_versions.version_created_by_name`, backfilled once by migration 002 and never + written again. This was originally listed as a step 5 question; it was brought forward so + the data is safe well before the table that holds it is dropped. + +**Nothing is open.** The last one - ~~`event_versions.version_created_by_id`~~, the same INT +reference on the version rows - was handled in passing: step 1 gave it a sibling bridging +column, step 2 a sibling snapshot, and step 3 reads it exactly like `events`. diff --git a/sql/calendar/001_add_admin_user_bridge.sql b/sql/calendar/001_add_admin_user_bridge.sql new file mode 100644 index 0000000..765d889 --- /dev/null +++ b/sql/calendar/001_add_admin_user_bridge.sql @@ -0,0 +1,38 @@ +-- Nachklang e.V. Calendar module — step 1 of docs/calendar-auth-migration.md. +-- Adds the bridging columns that let an event record who created it as an +-- *admin* user id (VARCHAR(36)) alongside the legacy calendar users.user_id +-- (INT). Apply manually against the CALENDAR_DB database: +-- mysql -h -u -p < 001_add_admin_user_bridge.sql +-- +-- Numbered 001 because this is the first migration this repo owns for the +-- calendar schema: the tables themselves predate it and were provided by the +-- repo owner (mirrored for dev in docker/init/01-calendar-schema-dev.sql). +-- +-- Nothing reads these columns yet — step 3 introduces the dual-read. Adding +-- them first means the backfill in step 2 has somewhere to write, and this +-- migration can be applied to production on its own without any code change. +-- +-- No foreign key, on purpose. The admin `user` table lives in a *different* +-- database (nachklang_admin) behind a different connection pool, and a +-- cross-schema FK would tie the two schemas' lifecycles together: you could no +-- longer dump, restore or move one without the other. The reference is +-- enforced in application code, which is also where the legacy/new fallback +-- lives. +-- +-- The collation is pinned to the admin database's (utf8mb4_unicode_ci) rather +-- than inherited from the calendar tables' utf8mb4_general_ci. These columns +-- hold ids that only ever compare against nachklang_admin.user.id, and a +-- mismatched collation makes any such comparison fail at runtime with +-- "Illegal mix of collations" instead of at review time. + +ALTER TABLE `events` + ADD COLUMN `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`); + +ALTER TABLE `event_versions` + ADD COLUMN `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`); diff --git a/sql/calendar/002_snapshot_legacy_creator_names.sql b/sql/calendar/002_snapshot_legacy_creator_names.sql new file mode 100644 index 0000000..be08e5b --- /dev/null +++ b/sql/calendar/002_snapshot_legacy_creator_names.sql @@ -0,0 +1,42 @@ +-- Nachklang e.V. Calendar module — step 5 preparation, brought forward. +-- Apply manually against the CALENDAR_DB database, after 001: +-- mysql -h -u -p < 002_snapshot_legacy_creator_names.sql +-- +-- Snapshots the creator's and last editor's *name* onto the event itself. +-- +-- Why: the creator is only ever rendered as a name (nothing authorises on it), +-- and today that name comes from joining the calendar's own `users` table. +-- Step 5 drops that table, which would silently erase the authorship of every +-- event created before the cutover. There is no account backfill to save them +-- either - that was dropped deliberately, see docs/calendar-auth-migration.md. +-- One text column per reference keeps the history at no ongoing cost. +-- +-- These columns are an archive, not a source of truth. Nothing writes them +-- after this backfill: events created from the cutover onwards carry an admin +-- user id, whose name is resolved live so that renaming an account updates +-- 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. +-- +-- 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`; + +ALTER TABLE `event_versions` + ADD COLUMN `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 + SET e.created_by_name = u.full_name + WHERE e.created_by_name IS NULL; + +UPDATE `event_versions` v + JOIN `users` u ON u.user_id = v.version_created_by_id + SET v.version_created_by_name = u.full_name + WHERE v.version_created_by_name IS NULL; diff --git a/src/models/admin/users/users.admin.service.ts b/src/models/admin/users/users.admin.service.ts index f2af3ee..a3ff2d3 100644 --- a/src/models/admin/users/users.admin.service.ts +++ b/src/models/admin/users/users.admin.service.ts @@ -437,3 +437,30 @@ export const findUserByEmail = async (email: string): Promise<{id: string; email return row ?? null; }; + +/** + * Display names for a set of user ids, as an id -> name map. Ids that no + * longer exist are simply absent from the map rather than mapping to a + * placeholder, so callers can distinguish "deleted account" from "never had + * one" and choose their own fallback. + * + * This exists for the calendar migration (docs/calendar-auth-migration.md + * step 3): the calendar lives in a different database, so it cannot join + * against `user` to render "created by". One lookup per result set keeps that + * cheap without coupling the two schemas. + */ +export const findDisplayNames = async (ids: readonly string[]): Promise> => { + const distinct = Array.from(new Set(ids.filter(id => id))); + if (distinct.length === 0) { + // Kysely renders `in ()` for an empty list, which MariaDB rejects. + return new Map(); + } + + const rows = await db + .selectFrom('user') + .select(['id', 'name']) + .where('id', 'in', distinct) + .execute(); + + return new Map(rows.map(row => [row.id, row.name])); +}; diff --git a/src/models/calendar/events/event.interface.ts b/src/models/calendar/events/event.interface.ts index cafb87f..0caf877 100644 --- a/src/models/calendar/events/event.interface.ts +++ b/src/models/calendar/events/event.interface.ts @@ -67,16 +67,35 @@ * example: "John Doe" * createdById: * type: integer - * description: The ID of the user who created the event + * deprecated: true + * description: > + * The legacy calendar user id of the creator. Being replaced by + * createdByUserId; see docs/calendar-auth-migration.md. Null on + * events created after the cutover. + * nullable: true * example: 456 + * createdByUserId: + * type: string + * nullable: true + * description: The admin-module user id of the creator, once it has one + * example: "8f1c0f2e-0f1a-4b9e-9a7c-2d5f1b3c4d5e" * lastModifiedBy: * type: string * description: The name of the user who last modified the event * example: "John Doe" * lastModifiedById: * type: integer - * description: The ID of the user who last modified the event + * deprecated: true + * nullable: true + * description: > + * The legacy calendar user id of the last editor. Being replaced + * by lastModifiedByUserId. * example: 456 + * lastModifiedByUserId: + * type: string + * nullable: true + * description: The admin-module user id of the last editor, once it has one + * example: "8f1c0f2e-0f1a-4b9e-9a7c-2d5f1b3c4d5e" * url: * type: string * description: A URL with more information about the event @@ -102,10 +121,15 @@ export interface Event { createdDate: Date; lastModifiedDate?: Date; location: string; + /** Display name of the creator, from whichever id below resolved. */ createdBy?: string; - createdById: number; + createdById?: number | null; + /** Set once the event's creator exists in the admin module. Preferred over + * createdById when both are present; see docs/calendar-auth-migration.md. */ + createdByUserId?: string | null; lastModifiedBy?: string; - lastModifiedById?: number; + lastModifiedById?: number | null; + lastModifiedByUserId?: string | null; url: string; wholeDay: boolean; repeatFrequency: string; diff --git a/src/models/calendar/events/events.service.ts b/src/models/calendar/events/events.service.ts index 0d261ab..8cffb46 100644 --- a/src/models/calendar/events/events.service.ts +++ b/src/models/calendar/events/events.service.ts @@ -2,28 +2,52 @@ import * as dotenv from 'dotenv'; import {Guid} from 'guid-typescript'; import {Event} from './event.interface.js'; import {NachklangCalendarDB} from '../Calendar.db.js'; +import * as AdminUsersService from '../../admin/users/users.admin.service.js'; +import logger from '../../../middleware/logger.js'; dotenv.config(); /** - * Returns all events for the given calendar - * @param calendarId The calendar Id + * Step 3 of docs/calendar-auth-migration.md: the dual read. + * + * An event records its creator twice - `created_by_id`, the legacy INT into + * the calendar database's own `users` table, and `created_by_user_id`, the + * admin module's VARCHAR(36) id. Old rows have only the first, rows written + * after the step 4 cutover will have only the second, and the two live in + * different databases, so this file has to read both and prefer the new one. + * + * The one thing the creator is used for is a display name. Nothing authorises + * on it - there is no "only the creator may edit" rule anywhere - which is why + * a name that cannot be resolved degrades to blank instead of to an error. + * + * That name has three possible sources, and they are tried weakest first: + * + * 1. LEGACY - joining the calendar's own `users` table on `created_by_id`. + * 2. `created_by_name`, the snapshot migration 002 took of exactly that join, + * so the authorship of pre-cutover events survives step 5 dropping the + * table. An archive: nothing writes it after the backfill. + * 3. The admin module's `user.name`, looked up live for rows that carry an + * admin id. It wins because it is the only one that follows a rename. + * + * Removal note: everything marked LEGACY below comes out in step 5, together + * with the `users`/`sessions` tables and the `created_by_id` columns. The + * snapshot stays - it is the reason step 5 can drop them. */ -export const getAllEvents = async (calendarId: number): Promise => { - let conn = await NachklangCalendarDB.getConnection(); - let eventRows: Event[] = []; - try { - const calendarQuery = 'SELECT calendar_id, includes_calendars FROM calendars WHERE calendar_id = ?'; - const calendarRes = await conn.query(calendarQuery, calendarId); - let calendarsToFetch: number[] = [calendarId]; - for(let row of calendarRes) { - let includes: number[] = JSON.parse(row.includes_calendars); - calendarsToFetch = [...calendarsToFetch, ...includes]; - } - const eventsQuery = ` - SELECT e.calendar_id, e.uuid, e.created_date, e.created_by_id, u.full_name as created_by_name, u2.full_name as last_modified_by_name, v.* FROM events e +/** + * The one SELECT the four read paths share. It was copied out four times + * before, which is precisely why the dual read had to be added in four + * places; callers append their own WHERE and ORDER BY. + * + * `v.*` carries `version_created_by_user_id` and `version_created_by_name` + * along with the rest of the version row, so only the `events` columns need + * naming. The two joined names are aliased `legacy_*` because the unprefixed + * names are now real columns. + */ +const EVENT_SELECT = ` + SELECT e.calendar_id, e.uuid, e.created_date, e.created_by_id, e.created_by_user_id, e.created_by_name, + u.full_name as legacy_created_by_name, u2.full_name as legacy_last_modified_by_name, v.* FROM events e INNER JOIN ( SELECT event_id, MAX(event_version_id) AS latest_version FROM event_versions @@ -33,34 +57,124 @@ export const getAllEvents = async (calendarId: number): Promise => { INNER JOIN event_versions v ON v.event_id = latest_versions.event_id AND v.event_version_id = latest_versions.latest_version LEFT OUTER JOIN users u ON u.user_id = e.created_by_id - LEFT OUTER JOIN users u2 ON u2.user_id = v.version_created_by_id - WHERE e.calendar_id IN (?) AND v.status = 'PUBLIC' - ORDER BY e.event_id`; - const eventsRes = await conn.query(eventsQuery, [calendarsToFetch]); + LEFT OUTER JOIN users u2 ON u2.user_id = v.version_created_by_id`; - for (let row of eventsRes) { - eventRows.push({ - eventId: row.event_id, - calendarId: row.calendar_id, - uuid: row.uuid, - name: row.name, - description: row.description, - startDateTime: row.start_datetime, - endDateTime: row.end_datetime, - createdDate: row.created_date, - lastModifiedDate: row.version_created_at, - location: row.location, - createdBy: row.created_by_name, - createdById: row.created_by_id, - lastModifiedBy: row.last_modified_by_name, - lastModifiedById: row.version_created_by_id, - url: row.url, - wholeDay: row.whole_day, - repeatFrequency: row.repeat_frequency - }); +/** + * Maps a result row to an Event. `status` is included only where it always + * was: the admin views and the by-id lookup return it, the two public listings + * do not. + */ +const toEvent = (row: any, includeStatus: boolean): Event => { + const event: Event = { + eventId: row.event_id, + calendarId: row.calendar_id, + uuid: row.uuid, + name: row.name, + description: row.description, + startDateTime: row.start_datetime, + endDateTime: row.end_datetime, + createdDate: row.created_date, + lastModifiedDate: row.version_created_at, + location: row.location, + // Name resolution, weakest first: the LEGACY join against the calendar + // users table, then the snapshot taken in migration 002, then - in + // resolveAdminNames below - the live admin name, which wins because it + // is the only one that follows an account being renamed. + createdBy: row.created_by_name ?? row.legacy_created_by_name, + createdById: row.created_by_id, + createdByUserId: row.created_by_user_id ?? null, + lastModifiedBy: row.version_created_by_name ?? row.legacy_last_modified_by_name, + lastModifiedById: row.version_created_by_id, + lastModifiedByUserId: row.version_created_by_user_id ?? null, + url: row.url, + wholeDay: row.whole_day, + repeatFrequency: row.repeat_frequency + }; + + if (includeStatus) { + event.status = row.status; + } + + return event; +}; + +/** + * Fills in creator/editor names for rows that carry an admin user id, by way + * of a single lookup against the admin database. The calendar cannot join + * against `user` - it is a different schema behind a different pool - and + * making it one would tie the two schemas together as tightly as a foreign key + * would. + * + * A failure here is swallowed on purpose. These endpoints include the public + * calendar the website reads anonymously, and a name is decoration: if the + * admin database is unreachable, an event should still render with whatever + * the legacy join produced rather than 500 the whole listing. The alternative + * would widen the public calendar's blast radius to include the admin + * database, which it has never depended on before. + */ +const resolveAdminNames = async (events: Event[]): Promise => { + const ids = events + .flatMap(event => [event.createdByUserId, event.lastModifiedByUserId]) + .filter((id): id is string => Boolean(id)); + + if (ids.length === 0) { + return; + } + + let names: Map; + try { + names = await AdminUsersService.findDisplayNames(ids); + } catch (e: any) { + logger.warn('Calendar: could not resolve creator names from the admin database: ' + e.message); + return; + } + + for (const event of events) { + const createdBy = event.createdByUserId ? names.get(event.createdByUserId) : undefined; + if (createdBy) { + event.createdBy = createdBy; } - return eventRows; + const lastModifiedBy = event.lastModifiedByUserId ? names.get(event.lastModifiedByUserId) : undefined; + if (lastModifiedBy) { + event.lastModifiedBy = lastModifiedBy; + } + } +}; + +/** + * The calendars a listing has to cover: the requested one plus whatever it + * declares in `includes_calendars`. + */ +const calendarsToFetch = async (conn: any, calendarId: number): Promise => { + const calendarQuery = 'SELECT calendar_id, includes_calendars FROM calendars WHERE calendar_id = ?'; + const calendarRes = await conn.query(calendarQuery, calendarId); + let calendars: number[] = [calendarId]; + for (let row of calendarRes) { + let includes: number[] = JSON.parse(row.includes_calendars); + calendars = [...calendars, ...includes]; + } + return calendars; +}; + +/** + * Returns all events for the given calendar + * @param calendarId The calendar Id + */ +export const getAllEvents = async (calendarId: number): Promise => { + let conn = await NachklangCalendarDB.getConnection(); + try { + const calendars = await calendarsToFetch(conn, calendarId); + + const eventsQuery = `${EVENT_SELECT} + WHERE e.calendar_id IN (?) AND v.status = 'PUBLIC' + ORDER BY e.event_id`; + const eventsRes = await conn.query(eventsQuery, [calendars]); + + const events = eventsRes.map((row: any) => toEvent(row, false)); + await resolveAdminNames(events); + + return events; } catch (err) { throw err; } finally { @@ -76,48 +190,16 @@ export const getAllEvents = async (calendarId: number): Promise => { */ export const getAllEventsAdmin = async (calendarId: number): Promise => { let conn = await NachklangCalendarDB.getConnection(); - let eventRows: Event[] = []; try { - const eventsQuery = ` - SELECT e.calendar_id, e.uuid, e.created_date, e.created_by_id, u.full_name as created_by_name, u2.full_name as last_modified_by_name, v.* FROM events e - INNER JOIN ( - SELECT event_id, MAX(event_version_id) AS latest_version - FROM event_versions - GROUP BY event_id - ) latest_versions - ON e.event_id = latest_versions.event_id - INNER JOIN event_versions v - ON v.event_id = latest_versions.event_id AND v.event_version_id = latest_versions.latest_version - LEFT OUTER JOIN users u ON u.user_id = e.created_by_id - LEFT OUTER JOIN users u2 ON u2.user_id = v.version_created_by_id + const eventsQuery = `${EVENT_SELECT} WHERE e.calendar_id = ? ORDER BY e.event_id`; const eventsRes = await conn.query(eventsQuery, calendarId); - for (let row of eventsRes) { - eventRows.push({ - eventId: row.event_id, - calendarId: row.calendar_id, - uuid: row.uuid, - name: row.name, - description: row.description, - startDateTime: row.start_datetime, - endDateTime: row.end_datetime, - createdDate: row.created_date, - lastModifiedDate: row.version_created_at, - location: row.location, - createdBy: row.created_by_name, - createdById: row.created_by_id, - lastModifiedBy: row.last_modified_by_name, - lastModifiedById: row.version_created_by_id, - url: row.url, - wholeDay: row.whole_day, - repeatFrequency: row.repeat_frequency, - status: row.status - }); - } + const events = eventsRes.map((row: any) => toEvent(row, true)); + await resolveAdminNames(events); - return eventRows; + return events; } catch (err) { throw err; } finally { @@ -136,18 +218,7 @@ export const getAllEventsAdmin = async (calendarId: number): Promise => export const getEventById = async (eventId: number): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { - const eventsQuery = ` - SELECT e.calendar_id, e.uuid, e.created_date, e.created_by_id, u.full_name as created_by_name, u2.full_name as last_modified_by_name, v.* FROM events e - INNER JOIN ( - SELECT event_id, MAX(event_version_id) AS latest_version - FROM event_versions - GROUP BY event_id - ) latest_versions - ON e.event_id = latest_versions.event_id - INNER JOIN event_versions v - ON v.event_id = latest_versions.event_id AND v.event_version_id = latest_versions.latest_version - LEFT OUTER JOIN users u ON u.user_id = e.created_by_id - LEFT OUTER JOIN users u2 ON u2.user_id = v.version_created_by_id + const eventsQuery = `${EVENT_SELECT} WHERE e.event_id = ?`; const eventsRes = await conn.query(eventsQuery, eventId); @@ -155,27 +226,10 @@ export const getEventById = async (eventId: number): Promise => { return null; } - const row = eventsRes[0]; - return { - eventId: row.event_id, - calendarId: row.calendar_id, - uuid: row.uuid, - name: row.name, - description: row.description, - startDateTime: row.start_datetime, - endDateTime: row.end_datetime, - createdDate: row.created_date, - lastModifiedDate: row.version_created_at, - location: row.location, - createdBy: row.created_by_name, - createdById: row.created_by_id, - lastModifiedBy: row.last_modified_by_name, - lastModifiedById: row.version_created_by_id, - url: row.url, - wholeDay: row.whole_day, - repeatFrequency: row.repeat_frequency, - status: row.status - } as Event; + const event = toEvent(eventsRes[0], true); + await resolveAdminNames([event]); + + return event; } catch (err) { throw err; } finally { @@ -283,56 +337,23 @@ export const moveEvent = async (event: Event): Promise => { export const getNextUpcomingEvent = async (calendarId: number): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { - const calendarQuery = 'SELECT calendar_id, includes_calendars FROM calendars WHERE calendar_id = ?'; - const calendarRes = await conn.query(calendarQuery, calendarId); - let calendarsToFetch: number[] = [calendarId]; - for(let row of calendarRes) { - let includes: number[] = JSON.parse(row.includes_calendars); - calendarsToFetch = [...calendarsToFetch, ...includes]; - } + const calendars = await calendarsToFetch(conn, calendarId); const now = new Date(); - const eventsQuery = ` - SELECT e.calendar_id, e.uuid, e.created_date, e.created_by_id, u.full_name as created_by_name, u2.full_name as last_modified_by_name, v.* FROM events e - INNER JOIN ( - SELECT event_id, MAX(event_version_id) AS latest_version - FROM event_versions - GROUP BY event_id - ) latest_versions - ON e.event_id = latest_versions.event_id - INNER JOIN event_versions v - ON v.event_id = latest_versions.event_id AND v.event_version_id = latest_versions.latest_version - LEFT OUTER JOIN users u ON u.user_id = e.created_by_id - LEFT OUTER JOIN users u2 ON u2.user_id = v.version_created_by_id + const eventsQuery = `${EVENT_SELECT} WHERE e.calendar_id IN (?) AND v.status = 'PUBLIC' AND v.start_datetime > ? ORDER BY v.start_datetime ASC LIMIT 1`; - const eventsRes = await conn.query(eventsQuery, [calendarsToFetch, now]); + const eventsRes = await conn.query(eventsQuery, [calendars, now]); if (eventsRes.length === 0) { return null; } - const row = eventsRes[0]; - return { - eventId: row.event_id, - calendarId: row.calendar_id, - uuid: row.uuid, - name: row.name, - description: row.description, - startDateTime: row.start_datetime, - endDateTime: row.end_datetime, - createdDate: row.created_date, - lastModifiedDate: row.version_created_at, - location: row.location, - createdBy: row.created_by_name, - createdById: row.created_by_id, - lastModifiedBy: row.last_modified_by_name, - lastModifiedById: row.version_created_by_id, - url: row.url, - wholeDay: row.whole_day, - repeatFrequency: row.repeat_frequency - } as Event; + const event = toEvent(eventsRes[0], false); + await resolveAdminNames([event]); + + return event; } catch (err) { throw err; } finally { diff --git a/test/calendar/credentials.service.test.ts b/test/calendar/credentials.service.test.ts new file mode 100644 index 0000000..09d94f9 --- /dev/null +++ b/test/calendar/credentials.service.test.ts @@ -0,0 +1,53 @@ +import {describe, expect, it, vi, beforeEach} from 'vitest'; + +vi.mock('../../src/models/calendar/users/users.service.js', () => ({ + checkSession: vi.fn() +})); + +import * as UserService from '../../src/models/calendar/users/users.service.js'; +import * as CredentialService from '../../src/models/calendar/events/credentials.service.js'; + +/** + * The public calendar is read anonymously by nachklang.art to show the next + * upcoming event. That is a load-bearing property, not an accident: the + * calendar auth migration (docs/calendar-auth-migration.md) keeps the shared + * credentials only for the iCal export and moves everything else onto session + * cookies, and the failure mode of getting that wrong is the public website + * silently losing its events feed. + * + * So this pins both halves: public needs nothing, and the restricted calendars + * still need something. + */ +describe('hasAccess', () => { + beforeEach(() => { + vi.resetAllMocks(); + process.env.MEMBER_CREDENTIAL = 'member-secret'; + process.env.CHOIR_CREDENTIAL = 'choir-secret'; + process.env.MANAGEMENT_CREDENTIAL = 'management-secret'; + }); + + it('lets anyone read the public calendar with no session and no password', async () => { + await expect(CredentialService.hasAccess('public', '', '', '', '127.0.0.1')).resolves.toBe(true); + + // It must not even reach the session check - an anonymous read of the + // public calendar should not depend on the users table being available. + expect(UserService.checkSession).not.toHaveBeenCalled(); + }); + + it.each([ + ['members', 'member-secret'], + ['choir', 'choir-secret'], + ['management', 'management-secret'], + ['birthdays', 'choir-secret'] + ])('refuses %s without a credential and allows it with one', async (calendar, secret) => { + (UserService.checkSession as any).mockResolvedValue(null); + + await expect(CredentialService.hasAccess(calendar, '', '', '', '127.0.0.1')).resolves.toBe(false); + await expect(CredentialService.hasAccess(calendar, '', '', 'wrong', '127.0.0.1')).resolves.toBe(false); + await expect(CredentialService.hasAccess(calendar, '', '', secret, '127.0.0.1')).resolves.toBe(true); + }); + + it('refuses an unknown calendar outright', async () => { + await expect(CredentialService.hasAccess('nope', '', '', 'member-secret', '127.0.0.1')).resolves.toBe(false); + }); +}); diff --git a/test/calendar/events.service.test.ts b/test/calendar/events.service.test.ts new file mode 100644 index 0000000..19489e1 --- /dev/null +++ b/test/calendar/events.service.test.ts @@ -0,0 +1,191 @@ +import {describe, expect, it, vi, beforeEach} from 'vitest'; + +const connection = { + query: vi.fn(), + execute: vi.fn(), + beginTransaction: vi.fn(), + commit: vi.fn(), + rollback: vi.fn(), + end: vi.fn() +}; + +vi.mock('../../src/models/calendar/Calendar.db.js', () => ({ + NachklangCalendarDB: {getConnection: vi.fn(async () => connection)} +})); + +vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({ + findDisplayNames: vi.fn() +})); + +import {NachklangCalendarDB} from '../../src/models/calendar/Calendar.db.js'; +import * as AdminUsersService from '../../src/models/admin/users/users.admin.service.js'; +import * as EventService from '../../src/models/calendar/events/events.service.js'; + +/** + * Step 3 of docs/calendar-auth-migration.md. The property under test is that + * an event's creator resolves from whichever of its three possible sources is + * strongest - the live admin name, then the snapshot from migration 002, then + * the legacy join - and that a failure to reach the admin database costs a name + * rather than the whole response: the public calendar is read anonymously by + * the website and has never depended on the admin database being up. + */ + +// One row of the shape the shared SELECT produces. +const row = (over: Record = {}) => ({ + event_id: 1, + calendar_id: 1, + uuid: 'uuid-1', + name: 'Konzert', + description: '', + start_datetime: new Date('2026-04-18T19:00:00Z'), + end_datetime: new Date('2026-04-18T21:00:00Z'), + created_date: new Date('2026-01-01T00:00:00Z'), + version_created_at: new Date('2026-01-02T00:00:00Z'), + location: '', + created_by_id: 7, + created_by_user_id: null, + created_by_name: null, + legacy_created_by_name: 'Legacy Person', + version_created_by_id: 7, + version_created_by_user_id: null, + version_created_by_name: null, + legacy_last_modified_by_name: 'Legacy Person', + url: '', + whole_day: 0, + repeat_frequency: '', + status: 'PUBLIC', + ...over +}); + +/** getAllEvents runs the calendars lookup first, then the events query. */ +const givenEvents = (...rows: unknown[]) => { + connection.query.mockReset(); + connection.query + .mockResolvedValueOnce([{calendar_id: 1, includes_calendars: '[]'}]) + .mockResolvedValueOnce(rows); +}; + +beforeEach(() => { + vi.clearAllMocks(); + connection.end.mockResolvedValue(undefined); + (NachklangCalendarDB.getConnection as any).mockResolvedValue(connection); +}); + +describe('creator names', () => { + it('uses the legacy join when the row has no admin id', async () => { + givenEvents(row()); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Legacy Person'); + expect(events[0].createdById).toBe(7); + expect(events[0].createdByUserId).toBeNull(); + // Nothing to resolve, so the admin database is not touched at all. + expect(AdminUsersService.findDisplayNames).not.toHaveBeenCalled(); + }); + + it('prefers the admin name when the row carries an admin id', async () => { + givenEvents(row({ + created_by_user_id: 'admin-1', + version_created_by_user_id: 'admin-2' + })); + (AdminUsersService.findDisplayNames as any).mockResolvedValue( + new Map([['admin-1', 'Neue Person'], ['admin-2', 'Andere Person']]) + ); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Neue Person'); + expect(events[0].lastModifiedBy).toBe('Andere Person'); + // The legacy id is still reported during the transition. + expect(events[0].createdById).toBe(7); + expect(events[0].createdByUserId).toBe('admin-1'); + }); + + it('prefers the snapshot over the legacy join', async () => { + givenEvents(row({ + created_by_name: 'Archived Person', + version_created_by_name: 'Archived Person' + })); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Archived Person'); + expect(events[0].lastModifiedBy).toBe('Archived Person'); + }); + + it('prefers the live admin name over the snapshot', async () => { + // A renamed account has to win over an archive that was correct when it + // was taken - otherwise renaming someone would leave stale names behind. + givenEvents(row({created_by_user_id: 'admin-1', created_by_name: 'Archived Person'})); + (AdminUsersService.findDisplayNames as any).mockResolvedValue(new Map([['admin-1', 'Neue Person']])); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Neue Person'); + }); + + it('keeps the snapshot when step 5 has removed the legacy join', async () => { + // What a post-step-5 row looks like: no legacy id, no join, snapshot only. + givenEvents(row({ + created_by_id: null, + legacy_created_by_name: undefined, + legacy_last_modified_by_name: undefined, + created_by_name: 'Archived Person', + version_created_by_name: 'Archived Person' + })); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Archived Person'); + expect(events[0].lastModifiedBy).toBe('Archived Person'); + }); + + it('falls back to the legacy name when the admin account is gone', async () => { + givenEvents(row({created_by_user_id: 'deleted'})); + (AdminUsersService.findDisplayNames as any).mockResolvedValue(new Map()); + + const events = await EventService.getAllEvents(1); + + expect(events[0].createdBy).toBe('Legacy Person'); + }); + + it('resolves a mixed result set in a single lookup', async () => { + givenEvents( + row({event_id: 1}), + row({event_id: 2, created_by_user_id: 'admin-1', version_created_by_user_id: 'admin-1'}), + row({event_id: 3, created_by_user_id: 'admin-1', version_created_by_user_id: 'admin-1'}) + ); + (AdminUsersService.findDisplayNames as any).mockResolvedValue(new Map([['admin-1', 'Neue Person']])); + + const events = await EventService.getAllEvents(1); + + expect(events.map(e => e.createdBy)).toEqual(['Legacy Person', 'Neue Person', 'Neue Person']); + expect(AdminUsersService.findDisplayNames).toHaveBeenCalledTimes(1); + }); + + it('still returns the events when the admin database is unreachable', async () => { + givenEvents(row({created_by_user_id: 'admin-1'})); + (AdminUsersService.findDisplayNames as any).mockRejectedValue(new Error('ECONNREFUSED')); + + const events = await EventService.getAllEvents(1); + + expect(events).toHaveLength(1); + expect(events[0].name).toBe('Konzert'); + // Degrades to the legacy name rather than failing the request. + expect(events[0].createdBy).toBe('Legacy Person'); + }); +}); + +describe('status', () => { + it('is omitted from the public listing and present in the admin one', async () => { + givenEvents(row()); + const publicEvents = await EventService.getAllEvents(1); + expect(publicEvents[0].status).toBeUndefined(); + + connection.query.mockReset(); + connection.query.mockResolvedValueOnce([row()]); + const adminEvents = await EventService.getAllEventsAdmin(1); + expect(adminEvents[0].status).toBe('PUBLIC'); + }); +});