diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 093a77371..d7a009ffd 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -233,6 +233,14 @@ jobs: - name: 'Type check: va-apple-music-url-remediation (BS#2009)' run: npm run typecheck --workspace=jobs/va-apple-music-url-remediation + # Same BS#2009 gap as above: this job carries the Track 2d + # precondition gate that stands between a rerun and an unrecoverable + # legal-name loss (DJ real-name PII safeguards plan) — the one job in + # jobs/** where shipping uncompiled-but-green is the worst possible + # place for that gap. + - name: 'Type check: auth-user-name-backfill' + run: npm run typecheck --workspace=jobs/auth-user-name-backfill + - name: Lint run: npm run lint diff --git a/CLAUDE.md b/CLAUDE.md index a431246e0..357c5686b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,6 +72,7 @@ npm workspaces: | `@wxyc/flowsheet-april-gap-import` | `jobs/flowsheet-april-gap-import/` | One-shot dry-run-by-default import (BS#2119): backfill the closed BS#351 residue — 403 `FLOWSHEET_ENTRY_PROD` rows tubafrenzy holds that Backend never received because the pre-fix ETL silently dropped every track entry with `START_TIME=0`. Default scope is the unambiguous 2026-04-16 → 2026-04-20 window (399 rows / 15 shows, `GAP_IMPORT_WINDOW_START`/`END` widen it); the 4 post-Phase-3 August rows are deliberately excluded pending a per-row provenance check (see #1543). Insert-only, `ON CONFLICT (legacy_entry_id) DO NOTHING`, never `DO UPDATE`. Reuses `jobs/flowsheet-etl`'s pure mappers (extracted to `transform.ts`/`show-id-map.ts`/`fetch-legacy.ts`'s `fetchLegacyEntriesInWindow` by this same issue, since importing `job.ts` itself self-invokes the ETL) — never `flowsheet-etl`/`legacy-linkage-resolve` directly. Batched inserts with cooperative live-DJ pause + inter-batch gap. **Four refusals before any write** (all also fire in dry-run): Backend-side id-count floor, cohort-size ceiling, an upstream `GAP_IMPORT_MIN_CANDIDATE_COUNT` floor (default 1 — zero candidates is a bad window, not an empty one; candidates come from tubafrenzy and don't depend on Backend state), and a `GAP_IMPORT_MAX_NULL_KEY_ROWS` orphan guard (default 0) counting **non-marker** rows the target shows already hold with `legacy_entry_id IS NULL` — both the cohort diff and the `ON CONFLICT` target key on that column and a unique index doesn't constrain NULLs, so a dj-site row whose back-stamp was skipped (the `legacy-mirror-reconcile` Sweep 2 orphan class) is invisible to both and would be inserted twice. `dj_name` comes from the canonical `resolveShowDjName` (`@wxyc/database` `dj-name.ts`, extracted from `flowsheet.service.ts` by the BS#2119 review), never a re-derived `COALESCE` — the donor's copy predates `dj_name_override` (BS#1321) and omits the literal-"Anonymous" filter (BS#1286). | | `@wxyc/va-apple-music-url-remediation` | `jobs/va-apple-music-url-remediation/` | One-shot V/A Apple-URL remediation (BS#2000) for the deep-links BS persisted from LML's pre-LML#1139 V/A-blind Apple **track** matcher (the `Various Artists - ` prefix scores ~85 between any two V/A credits, and the LML#782 album-dropped fallback re-admitted winners on a vacuous artist + a generic standard's title). **Two arms, because the two tables have different recovery paths.** `flowsheet` gets a real LML re-verify — one lookup per DISTINCT `(artist, album, track)` (BS#1192: Apple URLs are track-aware), fanned to every row with that triple — because it has no status column and the worker never revisits `enriched_match`, so a null there is terminal. `album_metadata` gets **zero LML calls**: it is invalidated to `apple_music_url=NULL` + `apple_music_status='unresolved'` + `streaming_reask_attempts=0` so the BS#1915 hourly `streaming-reask.ts` sweep re-adjudicates it through the guarded matcher (it is album-keyed while the URL is a _track_ deep-link, so there is no honest triple to re-query with). **Scope correction to the issue body, which said not to touch `album_metadata`:** `flowsheet.service.ts` serves `coalesce(album_metadata.apple_music_url, flowsheet.apple_music_url)` and `enrich.ts` writes BOTH from the same track probe, so for a linked row the DJ-visible value is the album_metadata one. **Phase order is load-bearing — flowsheet FIRST**, since nulling `album_metadata` unmasks flowsheet's value via that coalesce (album phase is skipped entirely if flowsheet fails). Three-way verdict (`url` / `none` / `indeterminate`): a `none` needs THREE consecutive null passes and `indeterminate` (shed, BS#1293 skip, empty `results`, transport error) writes nothing — LML#904 measured ~56% of Apple probes nulling on LML's own self-throttle, so a single null is not evidence (same BS#1915 doctrine, bought procedurally since `flowsheet` lacks the status column). In-band rescue-rate detector aborts the run when observed throttle-nulls exceed `VA_REMEDIATION_MAX_RESCUE_RATE`; an indeterminate triple is skipped-and-accounted, never halted-in-place (the BS#1011 wedge). Coarse SQL net over `fold_artist_name` (the fold MUST be in SQL — `lower('Vàrious Artists')` matches nothing) + the `isVariousArtistsCredit` arbiter in `va-artist.ts`, which shares `foldArtistName` with the net and deliberately disagrees with all three existing repo V/A predicates (two substring, one that calls `Various Artists-Rock-Y` not-various). Compare-and-set UPDATE (this job overwrites a NON-null value while two other writers touch the column); flowsheet UPDATE omits `updated_at` (trigger 0084); ANALYZE after. Dry-run by default and makes ZERO LML calls. Hard-gated on LML#1139's guard **and** its cache purge being deployed, plus a confirmed `LML_APPLE_MUSIC_RATE_PER_MIN` roll-up. Cooperative live-DJ pause (BS#2009): both phases probe once per page, before the page loads (never once per row); a throwing probe is fail-open (logged, captured, treated as no activity) rather than aborting the run and losing its `last_id` resume cursors — ported from the `streaming-url-remediation` / `flowsheet-ghost-row-sweep` donors' `waitForQuietPeriod` + `safeProbe`. | | `@wxyc/metadata-no-match-digest` | `jobs/metadata-no-match-digest/` | Daily cron digest: reads the `cronjob_runs` watermark, queries `flowsheet` for rows that flipped to `metadata_status='enriched_no_match'` since the last run (filtered/sorted on `updated_at`, never `metadata_attempt_at` — the live CDC enrichment worker deliberately leaves that NULL on no-match rows), and emails one HTML+text digest to `DIGEST_RECIPIENT_EMAIL` (default `jake@wxyc.org`) via a self-contained SES sender (`email.ts`, deliberately not `@wxyc/authentication` — see the job's README): catalog/rotation-linked misses listed in full (`format.ts` Section A, feeds the `rotation.discogs_release_id` backfill), freeform misses aggregated by artist with a top-25 + "…and N more" cap (Section B), each line with a synthesized Discogs search URL. Zero misses sends no email but still advances the watermark; a send failure leaves the watermark untouched so the next run retries the same window. First run bounds the window to the last 24h. Read-only against `flowsheet`; no schema migration. All times rendered Pacific (`PT` label); the fixed-UTC cron schedule (`07 15 * * *`) means the rendered wall-clock hour drifts across the DST boundary (documented in the job's README). DB-only — no `@wxyc/lml-client` dependency, exempt from the LML cron-spacing policy (`docs/ops-cron-scheduling.md`). | +| `@wxyc/auth-user-name-backfill` | `jobs/auth-user-name-backfill/` | One-shot backfill (DJ real-name PII safeguards plan, Track 2d): rewrites `auth_user.name` to `resolveDjDisplayName(dj_name) ?? username` for the ~139 rows that predate the `databaseHooks.user.create.before`/`update.before` choke point (`shared/authentication/src/derive-user-display-name.ts`), skipping anonymous users and the `'Auto DJ'` service account. Dry-run by default (`--execute` to write). Opens with a machine-enforced preserve-first precondition gate (`decide.ts`'s `violatesPreserveFirstPrecondition`) that aborts non-zero — in both dry-run and execute mode — if any row still holds its ONLY copy of a legal name in `name` (real_name blank, name not `'Anonymous'`/`'Auto DJ'`/username/the on-air handle): **must NOT run with `--execute` before plan step 2a** (the reviewed manual SQL that copies `name -> real_name` for exactly those rows) has run against the target database — the gate makes that ordering machine-enforced rather than operator-trusted. | ### API Server (`apps/backend`) diff --git a/Dockerfile.auth-user-name-backfill b/Dockerfile.auth-user-name-backfill new file mode 100644 index 000000000..bbf15e359 --- /dev/null +++ b/Dockerfile.auth-user-name-backfill @@ -0,0 +1,37 @@ +#Build stage +FROM node:24-alpine AS builder + +WORKDIR /auth-user-name-backfill-builder + +COPY ./package.json ./package-lock.json ./ +COPY ./tsconfig.base.json ./ +COPY ./shared/database ./shared/database +COPY ./jobs/auth-user-name-backfill ./jobs/auth-user-name-backfill + +RUN npm ci && npm run build --workspace=@wxyc/database --workspace=@wxyc/auth-user-name-backfill + +#Production stage +FROM node:24-alpine AS prod + +WORKDIR /auth-user-name-backfill + +COPY ./package* ./ +COPY ./jobs/auth-user-name-backfill/package* ./jobs/auth-user-name-backfill/ +COPY ./shared/database/package* ./shared/database/ + +RUN npm install --omit=dev + +COPY --from=builder ./auth-user-name-backfill-builder/jobs/auth-user-name-backfill/dist ./jobs/auth-user-name-backfill/dist +COPY --from=builder ./auth-user-name-backfill-builder/shared/database/dist ./shared/database/dist + +# Tiny single-table workload (~139 candidate rows) — no bulk-update tuning +# needed here, unlike the batched flowsheet/shows backfills. +ENV DB_APPLICATION_NAME=wxyc-auth-user-name-backfill + +# ENTRYPOINT + empty CMD so docker-level args (`--execute`) pass through to +# the job rather than replacing the launcher — same reasoning as +# Dockerfile.legacy-dj-name-remediation. With `CMD ["npm", "start"]`, +# `docker run --execute` would replace the whole command instead of +# extending it. +ENTRYPOINT ["node", "/auth-user-name-backfill/jobs/auth-user-name-backfill/dist/job.js"] +CMD [] diff --git a/apps/auth/app.ts b/apps/auth/app.ts index bd9cf1b0d..f95d264c7 100644 --- a/apps/auth/app.ts +++ b/apps/auth/app.ts @@ -255,10 +255,14 @@ app.post('/auth/admin/provision-user', async (req, res) => { return res.status(403).json({ error: 'Forbidden: admin role required' }); } - // Validate required fields + // Validate required fields. `name` is deliberately NOT in this list + // (DJ real-name PII safeguards plan, Track 2c) — provisionUser() derives + // the stored name itself from djName/username, never from a + // client-supplied value. A still-supplied `name` is accepted-and-ignored + // for the deploy overlap window while dj-site still sends it. const body = (req.body ?? {}) as Record; - const { email, username, name, organizationSlug, role, realName, djName } = body; - const missing = ['email', 'username', 'name', 'organizationSlug', 'role'].filter( + const { email, username, organizationSlug, role, realName, djName } = body; + const missing = ['email', 'username', 'organizationSlug', 'role'].filter( (field) => !body[field] || typeof body[field] !== 'string' ); if (missing.length > 0) { @@ -273,7 +277,6 @@ app.post('/auth/admin/provision-user', async (req, res) => { const result = await provisionUser({ email: email as string, username: username as string, - name: name as string, organizationSlug: organizationSlug as string, role: role as string, realName: realName as string | undefined, diff --git a/apps/auth/create-auto-dj-user.ts b/apps/auth/create-auto-dj-user.ts index dc3db84e2..e2399d52d 100644 --- a/apps/auth/create-auto-dj-user.ts +++ b/apps/auth/create-auto-dj-user.ts @@ -77,8 +77,15 @@ export const createAutoDjUser = async (): Promise => { await provisionUser({ email, username: 'autodj', - name: 'Auto DJ', // required (notNull); internal, never surfaced publicly - djName: 'Auto DJ', // the public HANDLE — what appears on-air + // `name` is required (notNull) but is NOT the invariant carrier here — + // `djName` below is. deriveUserNameOnCreate (databaseHooks.user.create.before, + // shared/authentication/src/derive-user-display-name.ts) derives the stored + // `name` from `djName`, falling back to `username` only when `djName` is + // absent/blank/'Anonymous'. Deleting `djName` while trusting a comment that + // called `name` load-bearing would surface this account as its username + // ('autodj') instead of the intended 'Auto DJ' handle. + name: 'Auto DJ', + djName: 'Auto DJ', // the public HANDLE — what appears on-air, and what derives `name` above organizationSlug, role: 'dj', password, diff --git a/apps/auth/provision-user.ts b/apps/auth/provision-user.ts index d1b6d0060..8cabc4f1a 100644 --- a/apps/auth/provision-user.ts +++ b/apps/auth/provision-user.ts @@ -13,7 +13,7 @@ import { validateUsername, WXYCRoles, } from '@wxyc/authentication'; -import { db, user } from '@wxyc/database'; +import { db, deriveUserPublicName, user } from '@wxyc/database'; import { eq } from 'drizzle-orm'; /** Error with an HTTP status code for the provision-user endpoint. */ @@ -32,7 +32,14 @@ export interface ProvisionUserInput { username: string; /** Internal-only: createDefaultUser supplies an explicit bootstrap password. */ password?: string; - name: string; + /** + * Accepted-and-ignored: `provisionUser` derives the stored `name` itself + * (DJ real-name PII safeguards plan, Track 2c) from `djName`/`username`, + * never from a client-supplied value. Optional here only so an + * already-deployed caller that still sends it doesn't fail validation + * during the overlap window; dj-site drops the field in a follow-up PR. + */ + name?: string; organizationSlug: string; role: string; realName?: string; @@ -66,8 +73,21 @@ const errorMessage = (e: unknown): string => (e instanceof Error ? e.message : S * sync, since bypassing better-auth's endpoint handler skips plugin hooks. */ export async function provisionUser(input: ProvisionUserInput): Promise { - const { email, username, name, organizationSlug, role, realName, djName } = input; + const { email, username, organizationSlug, role, realName, djName } = input; const password = input.password ?? generateProvisionBootstrapPassword(); + // Track 2c: derive `name` here rather than trust `input.name` — belt and + // suspenders with the databaseHooks.user.create.before hook + // (auth.definition.ts), because whether the hook merge runs before + // better-auth's adapter enforces the core schema's `required: true` on + // `name` is unverified against the lockfile-resolved better-auth version. + // Supplying the value here removes that ordering dependency entirely. + // `username` is a required ProvisionUserInput field (validated below), so + // this is never empty in practice; `input.name` is not read at all. The + // policy itself (handle, else username) is `deriveUserPublicName` — + // see shared/database/src/dj-name.ts — with `?? username` kept as this + // site's own terminal fallback for the same reason the create hook and the + // backfill job each keep theirs. + const derivedName = deriveUserPublicName(djName ?? null, username) ?? username; // 1. Validate role // `Object.hasOwn`, not `role in WXYCRoles`: `in` walks the prototype chain, @@ -112,7 +132,13 @@ export async function provisionUser(input: ProvisionUserInput): Promise + * real_name` for exactly these rows) has not run against this database. + * Rewriting `name` here before that copy exists would lose the legal name + * outright: "the one unrecoverable failure in this plan." The gate makes + * run order irrelevant — it aborts regardless of whether 2a already ran. + * + * SQL's `IS DISTINCT FROM` is null-safe inequality; plain `!==` on two + * `string | null` values reproduces it exactly in JS (`null !== null` is + * `false`, matching `NULL IS DISTINCT FROM NULL` = false; `null !== 'x'` is + * `true`, matching the SQL). + * + * HANDLE EXEMPTION (BS#2297 review finding 2): a user provisioned after + * this PR's `databaseHooks.user.create.before` hook deploys can legitimately + * end up with `name = `, `real_name` blank, and `name` distinct from + * `username` (no username chosen yet, or a username that differs from the + * handle) — that shape used to false-positive this gate forever, and the + * gate's remediation message ("run 2a first") would have had an operator + * copy a HANDLE into the real_name PII column. A row whose trimmed `name` + * equals its resolved handle holds no legal name in `name` at all — there is + * nothing to preserve — so it's exempted alongside the existing + * `name === username` exemption. This is the same "handle-is-real-name" + * exemption the stored-data scrub carries: a DJ whose real name coincides + * with their handle is exempted too, and correctly so — there's no + * information loss in skipping it (see decide.test.ts's exemption-matrix + * comment for the full reasoning). + */ +export function violatesPreserveFirstPrecondition( + row: Pick +): boolean { + const realNameBlank = row.realName === null || row.realName.trim() === ''; + const handle = resolveDjDisplayName(row.djName ?? null); + const nameIsHandle = handle !== null && row.name.trim() === handle; + return ( + realNameBlank && + !row.isAnonymous && + row.name !== 'Anonymous' && + row.name !== 'Auto DJ' && + row.name !== row.username && + !nameIsHandle + ); +} + +/** + * Decide the backfilled `name` for one auth_user row. + * + * `name := deriveUserPublicName(dj_name, username)` — the `auth_user.name` + * policy from `dj-name.ts`, computed in TypeScript via the canonical helper + * shared with the create hook and `provisionUser`, never re-derived in SQL. + * (The gate predicate above stays on `resolveDjDisplayName` directly — it + * needs only the handle link, not the username fallback.) + * + * Returns `undefined` (leave the row unchanged) when: + * - the user is anonymous (per-device throwaways, not station members — + * `databaseHooks.user.create.after` never adds them to the roster + * either); + * - `name` is already the literal `'Auto DJ'` service-account marker + * (`create-auto-dj-user.ts`) — checked explicitly rather than relying on + * the derived value happening to equal it; + * - neither a usable handle nor a `username` exists — nothing to backfill + * to, so the row is left exactly as the live read path already + * tolerates it; + * - the derived value already equals the stored `name` — a no-op write is + * avoided rather than executed. + */ +export function decideAuthUserNameBackfill( + row: Pick +): string | undefined { + if (row.isAnonymous) return undefined; + if (row.name === 'Auto DJ') return undefined; + const derived = deriveUserPublicName(row.djName ?? null, row.username ?? null) ?? undefined; + if (derived === undefined) return undefined; + if (derived === row.name) return undefined; + return derived; +} diff --git a/jobs/auth-user-name-backfill/job.ts b/jobs/auth-user-name-backfill/job.ts new file mode 100644 index 000000000..3eafda1e4 --- /dev/null +++ b/jobs/auth-user-name-backfill/job.ts @@ -0,0 +1,187 @@ +/** + * One-shot backfill: rewrite `auth_user.name` to the value it should always + * have held — the on-air handle, else `username` — so the column stops + * being a hidden second copy of the legal name. + * + * DJ real-name PII safeguards plan, Track 2d. The runtime counterpart to + * Track 2b's `databaseHooks.user` before-hooks (`shared/authentication/src/ + * derive-user-display-name.ts`): those hooks stop the leak from growing on + * every future write; this job repairs the ~139 rows that predate them. + * + * DRY-RUN IS THE DEFAULT, matching `jobs/flowsheet-ghost-row-sweep`'s + * convention (not `jobs/legacy-dj-name-remediation`'s inverted one): the + * container reports what it would change with zero writes; pass `--execute` + * to write. + * + * docker run --rm --env-file .env # dry-run + * docker run --rm --env-file .env --execute # writes + * + * Opens with a MACHINE-ENFORCED PRECONDITION GATE (see decide.ts's + * `violatesPreserveFirstPrecondition`) that aborts non-zero — in both + * dry-run and execute mode — if any row still holds its only copy of a + * legal name in `name`. That means Track 2a's preserve-first copy + * (`name -> real_name`, reviewed manual SQL) has not run against this + * database; the gate makes run order irrelevant instead of trusting an + * operator to sequence 2a before 2d correctly. + * + * The non-anonymous rows are read in a single SELECT (see `fetchAllUsers`'s + * docblock for why anonymous rows are filtered out at the query) and the + * decision is computed IN-PROCESS via the canonical `resolveDjDisplayName` + * helper, then written back only for rows that differ. `auth_user`'s + * non-anonymous slice is small (~139 candidate rows out of the whole + * roster) — the batching / id-cursor machinery `flowsheet-dj-name-backfill` + * needs for a many-million-row table would be unneeded complexity here. + */ + +import { sql } from 'drizzle-orm'; +import { db, closeDatabaseConnection } from '@wxyc/database'; +import { decideAuthUserNameBackfill, violatesPreserveFirstPrecondition, type AuthUserBackfillRow } from './decide.js'; + +const JOB_NAME = 'auth-user-name-backfill'; + +/** + * `--execute` opts into writing; the default (and `--dry-run`, spelled out + * explicitly) reports without touching the database. Throws on contradictory + * flags rather than silently picking one, mirroring + * flowsheet-ghost-row-sweep's `resolveDryRun`. + */ +export const resolveDryRun = (argv: string[] = process.argv): boolean => { + const execute = argv.includes('--execute'); + const dryRun = argv.includes('--dry-run'); + if (execute && dryRun) { + throw new Error('Contradictory flags: pass either --execute or --dry-run (the default), not both.'); + } + return !execute; +}; + +type RawUserRow = { + id: string; + name: string; + username: string | null; + dj_name: string | null; + real_name: string | null; + is_anonymous: boolean | null; +}; + +/** + * The one read this job performs. No LIMIT — see the module doc for why + * that's fine here (a single small table, ~139 candidate rows). + * + * WHERE excludes anonymous per-device rows (FINDING 7, BS#2297 review). + * `decideAuthUserNameBackfill` and `runPreconditionGate` already skip + * `is_anonymous` rows unconditionally, so this is behavior-identical — it + * just stops pulling them (and everyone's `real_name`) into process memory + * to be immediately discarded. Anonymous rows plausibly dominate + * `auth_user`; there's no reason to load legal names for rows this job + * never writes to. + */ +export const fetchAllUsers = async (): Promise => { + const rows = (await db.execute(sql` + SELECT "id", "name", "username", "dj_name", "real_name", "is_anonymous" + FROM "auth_user" + WHERE "is_anonymous" IS DISTINCT FROM true + `)) as unknown as RawUserRow[]; + return rows.map((r) => ({ + id: r.id, + name: r.name, + username: r.username, + djName: r.dj_name, + realName: r.real_name, + isAnonymous: r.is_anonymous ?? false, + })); +}; + +/** + * Abort non-zero (throw) if any row still relies on `auth_user.name` as its + * only copy of a legal name. Runs against the SAME rows the backfill loop + * below computes decisions from — one SELECT does double duty as both the + * gate's input and the backfill's input. Fires identically in dry-run and + * execute mode (see the module doc) — this function isn't told which mode + * it's running in. + * + * The failure message carries the COMPLETE violating-id list, not a sample + * (BS#2297 review finding 5): an operator remediating this needs every id to + * act on, not "and N more" left to re-derive by re-running the + * (still-PII-holding) query themselves. Ids ONLY — never the row's + * name/username/dj_name values, which would put legal names in every log + * sink this error reaches — quoted and comma-joined so the list pastes + * directly into `WHERE id IN (...)`. + */ +export const runPreconditionGate = (rows: AuthUserBackfillRow[]): void => { + const violations = rows.filter(violatesPreserveFirstPrecondition); + if (violations.length === 0) return; + + const idList = violations.map((r) => `'${r.id}'`).join(', '); + throw new Error( + `[${JOB_NAME}] Refusing to run: ${violations.length} row(s) hold their ONLY copy of a legal name in ` + + `auth_user.name (real_name is blank, name is not 'Anonymous'/'Auto DJ'/username, and name is not the ` + + `on-air handle). This means Track 2a of the DJ real-name PII safeguards plan has not run against this ` + + `database: copy name -> real_name for these ids, then re-run this job. Violating id(s) (WHERE id IN (...)): ` + + idList + ); +}; + +/** Write path for a single decided row. Raw SQL, not the query builder — this job bypasses better-auth's hooks entirely (a direct data repair, not a user-facing write). */ +export const applyUpdate = async (id: string, name: string): Promise => { + await db.execute(sql` + UPDATE "auth_user" + SET "name" = ${name}, "updated_at" = now() + WHERE "id" = ${id} + `); +}; + +export interface BackfillSummary { + scanned: number; + updated: number; + skipped: number; + dryRun: boolean; +} + +export const runBackfill = async (opts: { dryRun: boolean }): Promise => { + console.log(`[${JOB_NAME}] Starting. dry_run=${opts.dryRun}`); + + const rows = await fetchAllUsers(); + console.log(`[${JOB_NAME}] Loaded ${rows.length} auth_user row(s).`); + + runPreconditionGate(rows); + console.log( + `[${JOB_NAME}] Precondition gate passed: no row still relies on auth_user.name as its only legal-name copy.` + ); + + let updated = 0; + let skipped = 0; + for (const row of rows) { + const decision = decideAuthUserNameBackfill(row); + if (decision === undefined) { + skipped += 1; + continue; + } + console.log( + `[${JOB_NAME}] ${opts.dryRun ? '[dry-run] would update' : 'updating'} id=${row.id} name: ${JSON.stringify(row.name)} -> ${JSON.stringify(decision)}` + ); + if (!opts.dryRun) { + await applyUpdate(row.id, decision); + } + updated += 1; + } + + const summary: BackfillSummary = { scanned: rows.length, updated, skipped, dryRun: opts.dryRun }; + console.log( + `[${JOB_NAME}] Done. scanned=${summary.scanned} updated=${summary.updated} skipped=${summary.skipped} dry_run=${summary.dryRun}` + ); + return summary; +}; + +const main = async () => { + try { + const dryRun = resolveDryRun(); + await runBackfill({ dryRun }); + } finally { + await closeDatabaseConnection(); + } +}; + +main().catch((error) => { + console.error(`[${JOB_NAME}] Failed:`, error); + process.exitCode = 1; +}); diff --git a/jobs/auth-user-name-backfill/package.json b/jobs/auth-user-name-backfill/package.json new file mode 100644 index 000000000..e6c9a671b --- /dev/null +++ b/jobs/auth-user-name-backfill/package.json @@ -0,0 +1,26 @@ +{ + "name": "@wxyc/auth-user-name-backfill", + "job-type": "one-shot", + "version": "1.0.0", + "description": "One-shot backfill: rewrite auth_user.name to the on-air handle (or username) it should always have held, so the column stops being a hidden second copy of the legal name (DJ real-name PII safeguards plan, Track 2d). Opens with a machine-enforced precondition gate that refuses to run until Track 2a's preserve-first copy (name -> real_name) has run. Dry-run by default; --execute to write. ~139 candidate rows, single SELECT-all.", + "type": "module", + "main": "./dist/job.js", + "scripts": { + "start": "node dist/job.js", + "build": "tsup --minify", + "clean": "rm -rf dist", + "docker:build": "docker build -t wxyc_auth_user_name_backfill:ci -f ../../Dockerfile.auth-user-name-backfill ../../", + "dev": "tsup --watch", + "typecheck": "tsc --noEmit" + }, + "license": "PolyForm-Noncommercial-1.0.0", + "dependencies": { + "@wxyc/database": "^1.0.0" + }, + "peerDependencies": { + "drizzle-orm": "^0.45.0" + }, + "devDependencies": { + "typescript": "^6.0.3" + } +} diff --git a/jobs/auth-user-name-backfill/tsconfig.json b/jobs/auth-user-name-backfill/tsconfig.json new file mode 100644 index 000000000..d5ee8b781 --- /dev/null +++ b/jobs/auth-user-name-backfill/tsconfig.json @@ -0,0 +1,12 @@ +{ + "extends": ["../../tsconfig.base.json"], + "references": [{ "path": "../../shared/database" }], + "include": ["."], + "compilerOptions": { + "module": "esnext", + "moduleResolution": "bundler", + "paths": { + "@/*": ["./*"] + } + } +} diff --git a/jobs/auth-user-name-backfill/tsup.config.ts b/jobs/auth-user-name-backfill/tsup.config.ts new file mode 100644 index 000000000..d471e5600 --- /dev/null +++ b/jobs/auth-user-name-backfill/tsup.config.ts @@ -0,0 +1,21 @@ +import { defineConfig } from 'tsup'; +import { resolve, dirname } from 'path'; +import { fileURLToPath } from 'url'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +export default defineConfig((options) => ({ + entry: ['job.ts'], + format: ['esm'], + outDir: 'dist', + clean: true, + onSuccess: options.watch ? 'node ./dist/job.js' : undefined, + minify: !options.watch, + + esbuildOptions(options) { + options.alias = { + '@': resolve(__dirname), + }; + }, +})); diff --git a/package-lock.json b/package-lock.json index 4c120e767..010f2af1b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -561,6 +561,20 @@ "drizzle-orm": "^0.45.0" } }, + "jobs/auth-user-name-backfill": { + "name": "@wxyc/auth-user-name-backfill", + "version": "1.0.0", + "license": "PolyForm-Noncommercial-1.0.0", + "dependencies": { + "@wxyc/database": "^1.0.0" + }, + "devDependencies": { + "typescript": "^6.0.3" + }, + "peerDependencies": { + "drizzle-orm": "^0.45.0" + } + }, "jobs/broken-fk-recovery": { "name": "@wxyc/broken-fk-recovery", "version": "1.0.0", @@ -6548,6 +6562,10 @@ "resolved": "apps/auth", "link": true }, + "node_modules/@wxyc/auth-user-name-backfill": { + "resolved": "jobs/auth-user-name-backfill", + "link": true + }, "node_modules/@wxyc/authentication": { "resolved": "shared/authentication", "link": true diff --git a/shared/authentication/src/auth.definition.ts b/shared/authentication/src/auth.definition.ts index b1b539094..ca2ec8e89 100644 --- a/shared/authentication/src/auth.definition.ts +++ b/shared/authentication/src/auth.definition.ts @@ -38,6 +38,7 @@ import { capSessionUpdateAgainstDeviceFlow, DEVICE_SESSION_TTL_MS, } from './device-authorization'; +import { deriveUserNameOnCreate, deriveOrRejectUserNameOnUpdate } from './derive-user-display-name'; import { sendEmail, sendOTPEmail, sendResetPasswordEmail, sendVerificationEmailMessage } from './email'; import { buildTrustedClients } from './oidc-trusted-clients'; import { buildLoginPage } from './oidc-login-page'; @@ -521,6 +522,14 @@ export const auth = betterAuth({ databaseHooks: { user: { create: { + // DJ real-name PII safeguards plan, Track 2b: the one choke point + // for auth_user.name — see derive-user-display-name.ts for the full + // constraint writeup (the {data} merge contract, why update.before + // can't fetch the row it's updating). + before: async (data) => + deriveUserNameOnCreate( + data as { name: string; username?: string | null; djName?: string | null } & Record + ), after: async (createdUser) => { const u = createdUser as { id: string; isAnonymous?: boolean | null }; // Anonymous-plugin users are per-device throwaways, not station members. @@ -567,6 +576,15 @@ export const auth = betterAuth({ } }, }, + update: { + // Same Track 2b choke point as create.before above — payload-only, + // see derive-user-display-name.ts for why (update.before never + // receives the row it's updating). This hook can also return + // `false` and abort the entire write — see the rejection policy on + // `deriveOrRejectUserNameOnUpdate` in derive-user-display-name.ts. + before: async (data) => + deriveOrRejectUserNameOnUpdate(data as { djName?: string | null } & Record), + }, }, session: { update: { @@ -616,8 +634,36 @@ export const auth = betterAuth({ user: { additionalFields: { - realName: { type: 'string', required: false }, - djName: { type: 'string', required: false }, + // input: false (BS#2297 review finding 3): without it, better-auth's + // public POST /update-user let any signed-in session rewrite + // realName/djName directly — bypassing every enumerated write flow + // (provisioning, onboarding, dj-site's admin roster path) and the + // databaseHooks.user choke point this whole plan builds around. + // input:false is enforced only by the public route handlers + // (api/routes/update-user.mjs's parseUserInput call, and sign-up's); + // verified it does NOT break the two legitimate writers, both of + // which bypass that filtering entirely: + // - dj-site's roster editing (authClient.admin.updateUser) hits the + // admin plugin's adminUpdateUser route, which passes ctx.body.data + // straight to internalAdapter.updateUser with no input-filtering + // call anywhere in that route. + // - provisioning (provision-user.ts, internalAdapter.createUser) and + // onboarding (complete-onboarding.ts, internalAdapter.updateUser) + // both call internalAdapter methods directly, which hand their + // payload straight to createWithHooks/updateWithHooks — no + // input-filtering call there either. + // + // This lock and the databaseHooks.user veto (derive-user-display-name.ts's + // deriveOrRejectUserNameOnUpdate) are complementary, not redundant: this + // input:false block cannot reach the core `name` field at all (better-auth + // destructures `name` out of `body` before parseUserInput ever runs, so + // there is no additionalFields gate for it to sit behind), and the hook + // veto cannot be bypassed by a non-public writer the way an input-level + // lock could be (it runs inside createWithHooks/updateWithHooks itself, + // not in route-level filtering). Do not remove either mechanism believing + // the other already covers it. + realName: { type: 'string', required: false, input: false }, + djName: { type: 'string', required: false, input: false }, appSkin: { type: 'string', required: true, defaultValue: 'modern-light' }, isAnonymous: { type: 'boolean', required: false, defaultValue: false }, hasCompletedOnboarding: { type: 'boolean', required: false, defaultValue: false }, diff --git a/shared/authentication/src/derive-user-display-name.ts b/shared/authentication/src/derive-user-display-name.ts new file mode 100644 index 000000000..df7f8c925 --- /dev/null +++ b/shared/authentication/src/derive-user-display-name.ts @@ -0,0 +1,149 @@ +import { deriveUserPublicName, resolveDjDisplayName } from '@wxyc/database'; + +/** + * The single choke point for `auth_user.name`: pure `databaseHooks.user` + * before-hook helpers that derive `name` from the on-air handle + * (`auth_user.dj_name`) or `username`, never from a legal name. + * + * DJ real-name PII safeguards plan, Track 2b. Wired into + * `databaseHooks.user.create.before` / `update.before` in `auth.definition.ts`. + * + * Two verified constraints from better-auth's `db/with-hooks.mjs` (v1.6.26, + * the lockfile-resolved version) shape both functions here: + * + * 1. **The `{ data }` merge contract is load-bearing, not style.** Both hooks + * below depend on it identically — the exact quoted contract, and why + * `false` differs from `undefined`, is documented once, on + * `deriveOrRejectUserNameOnUpdate` below (it's the function whose + * rejection behavior actually depends on the distinction). + * + * 2. **`update.before` never receives the user id.** `internalAdapter + * .updateUser(userId, data)` builds `where: [{ field: 'id', value: userId + * }]` itself and calls `updateWithHooks(data, where, 'user', ...)` — the + * hook only ever sees `toRun(data, context)`, never `where`. There is no + * row fetch available inside an `update.before` hook, by construction. + * `complete-onboarding.ts`'s `internalAdapter.updateUser` call (outside + * any better-auth endpoint) goes through the identical path, so a + * request-context fallback isn't available there either. + * + * Constraint 2 is why `deriveOrRejectUserNameOnUpdate` is payload-only: it + * can only ever answer "does THIS update's own `djName` resolve to a usable + * handle?", never "what is this user's handle right now?". Two update shapes + * are therefore deliberately left untouched rather than guessed at: + * + * - **Handle-clear** (`djName` present but blank/'Anonymous'): the prior + * `name` — itself already a handle or a username post-backfill — stays. + * - **Username-only rename** (no `djName` key in the payload at all): a + * username-only payload can't reveal whether the user currently has a + * handle, so deriving from `username` here risks clobbering a live + * handle with the new username. `name` stays at its prior value. (This + * is also why the update hook calls `resolveDjDisplayName` directly + * instead of `deriveUserPublicName` from `@wxyc/database`'s + * `dj-name.ts` — that helper's username link is create/backfill-only; + * see its docblock.) + * + * Both are cosmetic staleness, not a PII regression: after the 2d backfill, + * whatever `name` was already holding is structurally non-PII (an earlier + * handle or an earlier username), never a legal name. The sentinel spec + * (Track 3b) is what actually polices the PII half of this invariant going + * forward. + * + * The full rejection policy for a `name`-carrying update payload — what + * happens, and why — is documented once, on `deriveOrRejectUserNameOnUpdate` + * below. + */ + +/** + * `databaseHooks.user.create.before`. + * + * The full create payload is present (unlike `update.before`), so the chain + * is the `auth_user.name` policy itself: `deriveUserPublicName` (on-air + * handle, else `username`) from `@wxyc/database`'s `dj-name.ts`, falling back + * to whatever `name` the caller supplied when neither resolves. The final + * fallback is deliberate — it's how the literal `'Anonymous'` (better-auth's + * anonymous plugin) and `'Auto DJ'` (`create-auto-dj-user.ts`) survive this + * hook unclobbered: neither has a resolvable handle or a `username`, so + * `derived` lands back on the supplied `name` and the no-op branch below + * fires. + */ +export function deriveUserNameOnCreate( + data: { name: string; username?: string | null; djName?: string | null } & Record +): { data: { name: string } } | undefined { + // `?? null` coercion is pinned here, same as Track 0's http-mirror.ts call: + // deriveUserPublicName is typed `(djName: string | null, username: string | + // null)` and both are optional on this payload shape under the package's + // `strict: true`. + const derived = deriveUserPublicName(data.djName ?? null, data.username ?? null) ?? data.name; + if (derived === data.name) return undefined; + return { data: { name: derived } }; +} + +/** + * `databaseHooks.user.update.before`. + * + * Payload-only by construction — see constraint 2 in the module docblock + * above. Derives a new `name` only when THIS update's own payload carries a + * `djName` key that resolves to a usable handle. Any other shape (`djName` + * absent, blank, or the literal `'Anonymous'`) is left untouched: see the + * handle-clear / username-only-rename note above for why that's the correct + * call, not a missed case. + * + * REJECTION POLICY (BS#2297 review finding 1) — canonical statement; nowhere + * else in this codebase restates it in full, only points here. Better-auth's + * core `POST /update-user` accepts a client-supplied `name` from any + * signed-in session (`api/routes/update-user.mjs`: `const { name, image, + * ...rest } = body`) and writes it verbatim once it reaches the adapter. + * Before this policy, a name-only payload fell through to the "no djName + * key" branch and returned `undefined` — a no-op from this hook's point of + * view — so better-auth wrote the client-supplied `name` straight to the + * database, re-creating exactly the hidden-legal-name-copy state this plan + * exists to close (`auth_user.name` silently holding a legal name again). + * + * So: any payload that carries a `name` key is rejected with `false` unless + * it ALSO carries a `djName` that resolves to a usable handle — in which + * case the returned `{ data: { name: handle } }` overrides the + * client-supplied `name` via the merge contract below. This is deliberately + * broader than "just don't derive from it": a payload carrying `name` + * alongside a blank/'Anonymous' djName is also rejected, closing the trivial + * bypass of attaching an unusable djName to a bare-name payload to slip past + * the rejection. + * + * THE MERGE CONTRACT this rejection depends on (constraint 1, module + * docblock above): `createWithHooks`/`updateWithHooks` call `toRun(data, + * context)` and only merge a returned value that satisfies `typeof result + * === 'object' && 'data' in result` — `actualData = { ...actualData, + * ...result.data }` (hook result spread AFTER actualData, which is why the + * override above wins regardless of what the payload's own `name` was). A + * mutated `data` argument, a bare object, or any shape other than `{ data: + * {...} }` is silently discarded: the hook appears to run but the write + * proceeds with the original payload. `false` tells `updateWithHooks` to + * abort the write entirely (`with-hooks.mjs`: `if (result === false) return + * null`) — the row is never touched, not even for other fields in the same + * payload — so `undefined` is the only correct "no-op" return, never `{}` + * and never a mutated argument. + * + * Direct writes to `name` are prohibited categorically, not case-by-case: + * after this program's PRs, no legitimate writer sends bare `name` on + * update (dj-site's roster editing sends `realName`/`djName` only; + * onboarding sends `realName`/`djName`; provisioning is a `create`, not an + * `update`, and is covered by `deriveUserNameOnCreate` instead). A + * `name`-carrying update payload reaching this hook is therefore always + * either a stale/misbehaving caller or an attempted direct write — reject + * it rather than silently accept or silently drop just the `name` field + * (better-auth's hook contract has no "drop one field" return shape; the + * only choices are override the whole payload's fate via `{ data }` or + * abort via `false`). + * + * This hook's veto is one of two independent PII-write guards, complementary + * rather than redundant with the other — `auth.definition.ts`'s + * `user.additionalFields` `input: false` lock on `realName`/`djName`. See + * the full two-writers writeup at that site for why neither can be removed + * believing the other covers it. + */ +export function deriveOrRejectUserNameOnUpdate( + data: { name?: unknown; djName?: string | null } & Record +): { data: { name: string } } | false | undefined { + const handle = 'djName' in data ? resolveDjDisplayName(data.djName ?? null) : null; + if (handle !== null) return { data: { name: handle } }; + return 'name' in data ? false : undefined; +} diff --git a/shared/database/src/dj-name.ts b/shared/database/src/dj-name.ts index 11ade03e5..b5fc2c969 100644 --- a/shared/database/src/dj-name.ts +++ b/shared/database/src/dj-name.ts @@ -105,3 +105,34 @@ export const resolveShowDjName = (input: { if (legacy && legacy.trim().length > 0) return legacy.trim(); return null; }; + +/** + * The `auth_user.name` policy — on-air handle, else `username` — in one + * place. Three call sites derived this chain independently (`apps/auth/ + * provision-user.ts`, the `databaseHooks.user.create.before` hook, and the + * `auth-user-name-backfill` job's rewrite-target computation); consolidated + * here so the policy has exactly one definition, same reasoning as + * `resolveDjDisplayName` and `resolveShowDjName` above. + * + * MICRO-BEHAVIOR CHANGE: the username link trims and blanks a whitespace-only + * `username`, converging on the mirror's documented contract (PR #2292). + * Every current writer validates `username` against `/^[a-zA-Z0-9_.]+$/` + * before it reaches any of these call sites, so a whitespace-only username is + * unreachable through normal writes — this only changes behavior for a + * manually-edited legacy row that already holds one. + * + * CREATE/BACKFILL-ONLY: this helper is for the create and backfill paths + * only. `databaseHooks.user.update.before` (`deriveOrRejectUserNameOnUpdate` + * in `derive-user-display-name.ts`) deliberately calls `resolveDjDisplayName` + * directly and does NOT fall back to `username` — an update payload carrying + * only `username` cannot reveal whether the user currently has a live + * handle, so deriving from it there risks clobbering one. Do not "fix" the + * update hook to use this helper; see that file's docblock for the full + * reasoning. + */ +export const deriveUserPublicName = (djName: string | null, username: string | null): string | null => { + const handle = resolveDjDisplayName(djName); + if (handle !== null) return handle; + const trimmed = username?.trim() ?? ''; + return trimmed.length > 0 ? trimmed : null; +}; diff --git a/tests/integration/dj-real-name-sentinel.spec.js b/tests/integration/dj-real-name-sentinel.spec.js new file mode 100644 index 000000000..fba5e91f2 --- /dev/null +++ b/tests/integration/dj-real-name-sentinel.spec.js @@ -0,0 +1,242 @@ +/** + * DJ real-name PII sentinel wire spec (DJ real-name PII safeguards plan, + * Track 3b) — the mechanism that would have caught `a0cd1979`. + * + * Seeds an `auth_user` row shaped exactly like the pre-Track-2 conflation + * this whole plan exists to unscrew: `real_name` and `name` both hold the + * legal-name sentinel. Track 2's databaseHooks (2b), the provision route + * (2c), and the backfill job (2d) all prevent a row reaching that shape + * going forward — this spec doesn't exercise any of those; it proves the + * PUBLIC READ surfaces never surface the legal name regardless of how a row + * got into that shape, which is the actual PII-leak failure mode (BS#1286/ + * BS#1393/BS#2281 were all downstream of a *read* path, not a write path). + * + * Whole-body string match against `JSON.stringify(res.body)` — catches + * fields that don't exist yet, unlike a keys-only shape assertion. Every + * `it` also carries a positive control (the seeded HANDLE, a value that is + * NOT the legal name) so a passing "no sentinel" assertion can't be + * accidentally vacuous — it only means something if the row was actually in + * scope for the query. + * + * Follows flowsheet-range.spec.js's getTestDb/supertest/cleanup pattern. + */ + +const postgres = require('postgres'); +const request = require('supertest')(`${process.env.TEST_HOST}:${process.env.PORT}`); + +const SCHEMA = process.env.WXYC_SCHEMA_NAME || 'wxyc_schema'; + +// The literal from the plan — never write this constant anywhere outside +// this spec's seed/assert pair. +const SENTINEL_REAL_NAME = 'SENTINEL-REAL-NAME-93aF'; +// A resolvable, structurally non-PII handle — distinct from (and not a +// substring of) SENTINEL_REAL_NAME, so a passing "sentinel absent" check +// can't be a false negative from accidental overlap. +const HANDLE = 'SENTINEL-HANDLE-93aF'; +const MARKER_ARTIST = 'DJ Real-Name Sentinel Probe Artist'; +const MARKER_TRACK = 'DJ Real-Name Sentinel Probe Track'; + +const USER_ID = 'sentinel-pii-probe-user-000001'; +const USER_EMAIL = 'sentinel-pii-probe-93af@test.wxyc.org'; +const USER_USERNAME = 'sentinel_pii_probe_93af'; + +// Fixed past window for /flowsheet/range and /flowsheet/search (time- +// independent, but kept inside the window for tidiness) — 1995, clear of +// flowsheet-range.spec.js's 1998 window and outside anything the shared +// dev/CI schema seeds, so the range query's result set is exactly this +// spec's fixture. +const WINDOW_START = Date.UTC(1995, 0, 15, 0, 0, 0); +const WINDOW_END = Date.UTC(1995, 0, 16, 0, 0, 0); +const at = (offsetMs) => new Date(WINDOW_START + offsetMs).toISOString(); + +function makeSql() { + return postgres({ + host: process.env.DB_HOST || 'localhost', + port: parseInt(process.env.DB_PORT || process.env.CI_DB_PORT || '5433', 10), + database: process.env.DB_NAME || 'wxyc_db', + user: process.env.DB_USERNAME || 'test-user', + password: process.env.DB_PASSWORD || 'test-pw', + onnotice: () => {}, + max: 2, + }); +} + +describe('DJ real-name PII sentinel (DJ real-name PII safeguards plan, Track 3b)', () => { + let sql; + let showId; + const entryIds = {}; + + beforeAll(async () => { + sql = makeSql(); + + // Defensive pre-delete (a prior crashed run could otherwise leave these + // rows behind and produce a confusing false pass/fail) — same pattern as + // internal-flowsheet-webhook.spec.js's seedShow. + await sql.unsafe( + `DELETE FROM "${SCHEMA}".flowsheet WHERE show_id IN (SELECT id FROM "${SCHEMA}".shows WHERE primary_dj_id = $1)`, + [USER_ID] + ); + await sql.unsafe(`DELETE FROM "${SCHEMA}".shows WHERE primary_dj_id = $1`, [USER_ID]); + await sql.unsafe(`DELETE FROM auth_user WHERE id = $1`, [USER_ID]); + + // Seed the auth_user row shaped exactly like the pre-Track-2 conflation: + // `name` duplicates `real_name`. `dj_name` is a normal, resolvable + // handle — the write path this row simulates (dj-site provisioning, + // pre-2c) always had a real dj_name available; the conflation was never + // about a missing handle, only about `name` secretly also holding the + // legal name. + await sql` + INSERT INTO auth_user (id, name, email, real_name, dj_name, username, is_anonymous) + VALUES (${USER_ID}, ${SENTINEL_REAL_NAME}, ${USER_EMAIL}, ${SENTINEL_REAL_NAME}, ${HANDLE}, ${USER_USERNAME}, false) + `; + + const showRows = await sql` + INSERT INTO ${sql(SCHEMA)}.shows (primary_dj_id, start_time, end_time) + VALUES (${USER_ID}, ${at(60 * 60 * 1000)}::timestamptz, NULL) + RETURNING id + `; + showId = showRows[0].id; + + const insertEntry = async (key, addTimeIso, entryType, extra = {}) => { + const rows = await sql` + INSERT INTO ${sql(SCHEMA)}.flowsheet + (show_id, add_time, entry_type, dj_name, artist_name, album_title, track_title, message, play_order) + VALUES ( + ${showId}, + ${addTimeIso}::timestamptz, + ${entryType}, + ${HANDLE}, + ${extra.artist_name ?? null}, + ${extra.album_title ?? null}, + ${extra.track_title ?? null}, + ${extra.message ?? null}, + ${extra.play_order ?? 1} + ) + RETURNING id`; + entryIds[key] = rows[0].id; + }; + + // In-window rows — exercised by GET /flowsheet/range and + // GET /flowsheet/search, neither of which depends on freshness. + await insertEntry('marker', at(60 * 60 * 1000), 'show_start', { message: `${MARKER_TRACK} start` }); + await insertEntry('track', at(2 * 60 * 60 * 1000), 'track', { + artist_name: MARKER_ARTIST, + album_title: 'Sentinel Probe Album', + track_title: MARKER_TRACK, + }); + + // Far-future row — GET /flowsheet and GET /flowsheet/latest order by + // (add_time DESC, id DESC) with no content filter available, so this is + // the only way to make inclusion deterministic without depending on + // what else lands in the table during the test run. + const farFuture = new Date(Date.now() + 100 * 365 * 24 * 60 * 60 * 1000).toISOString(); + await insertEntry('latest', farFuture, 'track', { + artist_name: MARKER_ARTIST, + album_title: 'Sentinel Probe Album', + track_title: MARKER_TRACK, + }); + }); + + afterAll(async () => { + if (!sql) return; + const ids = Object.values(entryIds); + if (ids.length) { + await sql.unsafe(`DELETE FROM "${SCHEMA}".flowsheet WHERE id = ANY($1::int[])`, [ids]); + } + if (showId) { + await sql.unsafe(`DELETE FROM "${SCHEMA}".shows WHERE id = $1`, [showId]); + } + await sql.unsafe(`DELETE FROM auth_user WHERE id = $1`, [USER_ID]); + await sql.end({ timeout: 5 }); + }); + + const assertNoSentinel = (res) => { + expect(res.status).toBe(200); + expect(JSON.stringify(res.body)).not.toContain(SENTINEL_REAL_NAME); + }; + + // Collapses the five interchangeable "never leaks" bodies below (BS#2297 + // review, fix 6): each endpoint keeps its own named `it` — what varies + // enough per endpoint to matter in failure output — but the boilerplate of + // firing a GET and running assertNoSentinel on it is written once. + // Positive controls stay bespoke; their assertions differ too much per + // endpoint to share. + const expectNoSentinelLeak = async (pathAndQuery) => { + const res = await request.get(pathAndQuery); + assertNoSentinel(res); + }; + + describe('GET /flowsheet (default, most-recent page)', () => { + it('never leaks the sentinel real name', () => expectNoSentinelLeak('/flowsheet')); + + it('positive control: included our probe row', async () => { + // Track-type V2 entries don't carry a dj_name field at all + // (transformToV2: "Track entries do not include dj_name in the v2 + // payload" — flowsheet.dj_name on track rows exists solely for the + // search hot path). artist_name is this endpoint's proof of + // inclusion instead. + const res = await request.get('/flowsheet'); + const mine = res.body.entries.find((e) => e.id === entryIds.latest); + expect(mine).toBeDefined(); + expect(mine.artist_name).toBe(MARKER_ARTIST); + }); + }); + + describe('GET /flowsheet/latest', () => { + it('never leaks the sentinel real name', () => expectNoSentinelLeak('/flowsheet/latest')); + + it('positive control: returned our probe row', async () => { + // Same track-type-has-no-dj_name-on-the-wire note as GET /flowsheet above. + const res = await request.get('/flowsheet/latest'); + expect(res.body.id).toBe(entryIds.latest); + expect(res.body.artist_name).toBe(MARKER_ARTIST); + }); + }); + + describe('GET /flowsheet/range', () => { + const fetchWindow = () => request.get(`/flowsheet/range?start=${WINDOW_START}&end=${WINDOW_END}`); + + it('never leaks the sentinel real name', () => + expectNoSentinelLeak(`/flowsheet/range?start=${WINDOW_START}&end=${WINDOW_END}`)); + + it('positive control: resolves the show dj_name through the live auth_user JOIN to the handle, not the real name', async () => { + const res = await fetchWindow(); + const show = res.body.shows.find((s) => s.id === showId); + expect(show).toBeDefined(); + expect(show.dj_name).toBe(HANDLE); + // The projection is a fixed field set with no id/real-name-shaped key + // — same shape flowsheet-range.spec.js pins for the general case. + expect(Object.keys(show).sort()).toEqual([ + 'dj_name', + 'end_time', + 'id', + 'show_name', + 'specialty_id', + 'start_time', + ]); + }); + + it('positive control: the marker entry also projects the handle', async () => { + const res = await fetchWindow(); + const marker = res.body.entries.find((e) => e.id === entryIds.marker); + expect(marker).toBeDefined(); + expect(marker.dj_name).toBe(HANDLE); + }); + }); + + describe('GET /flowsheet/search', () => { + it('never leaks the sentinel real name on an unfiltered query', () => expectNoSentinelLeak('/flowsheet/search')); + + it('never leaks the sentinel real name on a dj: operator query matching the handle', () => + expectNoSentinelLeak(`/flowsheet/search?q=${encodeURIComponent(`dj:${HANDLE}`)}`)); + + it('positive control: the dj: operator query found our probe row(s), projecting the handle rather than the real name', async () => { + const res = await request.get(`/flowsheet/search?q=${encodeURIComponent(`dj:${HANDLE}`)}`); + const mine = res.body.results.filter((r) => r.id === entryIds.track || r.id === entryIds.latest); + expect(mine.length).toBeGreaterThan(0); + for (const row of mine) { + expect(row.dj_name).toBe(HANDLE); + } + }); + }); +}); diff --git a/tests/mocks/database.mock.ts b/tests/mocks/database.mock.ts index 30deabfd6..8711e764c 100644 --- a/tests/mocks/database.mock.ts +++ b/tests/mocks/database.mock.ts @@ -747,7 +747,12 @@ export { intArrayLiteral } from '../../shared/database/src/int-array-literal.js' // would let a consumer's test pass while the real chain disagrees, which is // precisely the drift the extraction exists to prevent. The job tests assert // their resolver AGAINST `resolveShowDjName`, so it must be the real one. -export { resolveDjDisplayName, showDjNameOverride, resolveShowDjName } from '../../shared/database/src/dj-name.js'; +export { + resolveDjDisplayName, + showDjNameOverride, + resolveShowDjName, + deriveUserPublicName, +} from '../../shared/database/src/dj-name.js'; // Stubs of shared/database/src/last-logged-show-entry.ts (BS#2118 sites // 5/7/8). NOT re-exported from source, unlike the pure dj-name chain above: diff --git a/tests/unit/auth/provision-user.test.ts b/tests/unit/auth/provision-user.test.ts index c00776488..9daeeeabc 100644 --- a/tests/unit/auth/provision-user.test.ts +++ b/tests/unit/auth/provision-user.test.ts @@ -30,10 +30,23 @@ const mockDbUpdate = jest.fn().mockReturnValue({ }), }); -jest.mock('@wxyc/database', () => ({ - db: { update: (...args: unknown[]) => mockDbUpdate(...args) }, - user: { id: 'id' }, -})); +jest.mock('@wxyc/database', () => { + // deriveUserPublicName is the REAL implementation, sourced via + // jest.requireActual from tests/mocks/database.mock.ts (same pattern as + // album-plays-refresh.service.test.ts) rather than a second, hand-wired + // jest.requireActual pointed straight at shared/database/src/dj-name.ts — + // the mock file already re-exports the real PII-safe dj-name chain, so + // this is the one place that decides which real helpers pass through a + // mocked @wxyc/database. provisionUser's Track 2c derivation is asserted + // against the actual chain, the same reason grantsAdminFlag below is + // wired in real rather than restated. + const actual: typeof import('../../mocks/database.mock') = jest.requireActual('../../mocks/database.mock'); + return { + db: { update: (...args: unknown[]) => mockDbUpdate(...args) }, + user: { id: 'id' }, + deriveUserPublicName: actual.deriveUserPublicName, + }; +}); jest.mock('drizzle-orm', () => ({ eq: jest.fn((a: unknown, b: unknown) => ({ field: a, value: b })), @@ -189,7 +202,10 @@ describe('provisionUser()', () => { hasCompletedOnboarding: false, appSkin: 'modern-light', username: validInput.username, - name: validInput.name, + // Track 2c: name is DERIVED (resolveDjDisplayName(djName) ?? + // username), never the caller-supplied `name` — validInput.name + // ('New DJ') must NOT appear here. + name: validInput.djName, }) ); }); @@ -231,6 +247,50 @@ describe('provisionUser()', () => { }); }); + describe('name derivation (Track 2c)', () => { + // DJ real-name PII safeguards plan, Track 2c: provisionUser stops + // trusting a caller-supplied `name` and derives it itself + // (resolveDjDisplayName(djName) ?? username) — belt and suspenders with + // the databaseHooks.user.create.before hook. + + it('should succeed with no name in the input at all', async () => { + const { name: _name, ...withoutName } = validInput; + await expect(provisionUser(withoutName)).resolves.toBeDefined(); + }); + + it('should NOT pass a caller-supplied name to createUser', async () => { + await provisionUser({ ...validInput, name: 'SENTINEL-CLIENT-SUPPLIED-NAME' }); + + const call = mockCreateUser.mock.calls[0]?.[0] as Record; + expect(call.name).not.toBe('SENTINEL-CLIENT-SUPPLIED-NAME'); + }); + + it('should derive name from djName when djName is a usable handle', async () => { + await provisionUser({ ...validInput, djName: 'DJ Jazzy Jane' }); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ name: 'DJ Jazzy Jane' })); + }); + + it('should fall back to username when djName is absent', async () => { + const { djName: _djName, ...withoutDjName } = validInput; + await provisionUser(withoutDjName); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ name: validInput.username })); + }); + + it('should fall back to username when djName is the literal Anonymous', async () => { + await provisionUser({ ...validInput, djName: 'Anonymous' }); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ name: validInput.username })); + }); + + it('should preserve Auto DJ (create-auto-dj-user.ts-shaped input: name and djName both "Auto DJ")', async () => { + await provisionUser({ ...validInput, name: 'Auto DJ', djName: 'Auto DJ' }); + + expect(mockCreateUser).toHaveBeenCalledWith(expect.objectContaining({ name: 'Auto DJ' })); + }); + }); + describe('upsert against auto-created member row', () => { // The databaseHooks.user.create.after hook in auth.definition.ts // auto-creates a member row with role='member' for every non-anonymous diff --git a/tests/unit/authentication/derive-user-display-name.test.ts b/tests/unit/authentication/derive-user-display-name.test.ts new file mode 100644 index 000000000..6e6b158f4 --- /dev/null +++ b/tests/unit/authentication/derive-user-display-name.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from '@jest/globals'; + +// @wxyc/database resolves to tests/mocks/database.mock.ts (jest.unit.config.ts +// moduleNameMapper), which re-exports the REAL resolveDjDisplayName from +// shared/database/src/dj-name.ts — these tests exercise the actual chain, not +// a stub of it. + +import { + deriveUserNameOnCreate, + deriveOrRejectUserNameOnUpdate, +} from '../../../shared/authentication/src/derive-user-display-name'; + +describe('deriveUserNameOnCreate', () => { + it('derives the on-air handle when djName is usable', () => { + const result = deriveUserNameOnCreate({ + name: 'realname-in-name-field', + username: 'jane_dj', + djName: 'DJ Jazzy Jane', + }); + expect(result).toEqual({ data: { name: 'DJ Jazzy Jane' } }); + }); + + it('falls back to username when djName is absent', () => { + const result = deriveUserNameOnCreate({ name: 'realname-in-name-field', username: 'jane_dj' }); + expect(result).toEqual({ data: { name: 'jane_dj' } }); + }); + + it('falls back to username when djName is the literal Anonymous', () => { + const result = deriveUserNameOnCreate({ name: 'realname-in-name-field', username: 'jane_dj', djName: 'Anonymous' }); + expect(result).toEqual({ data: { name: 'jane_dj' } }); + }); + + it('preserves the literal Anonymous name for the anonymous plugin (no username, no djName)', () => { + const result = deriveUserNameOnCreate({ name: 'Anonymous' }); + expect(result).toBeUndefined(); + }); + + it('preserves the literal Auto DJ name for the auto-DJ service account', () => { + // create-auto-dj-user.ts supplies djName: 'Auto DJ' too — resolveDjDisplayName + // treats 'Auto DJ' as a usable (non-'Anonymous') handle, so derived equals + // the supplied name either way and the no-op branch fires. + const result = deriveUserNameOnCreate({ name: 'Auto DJ', username: 'autodj', djName: 'Auto DJ' }); + expect(result).toBeUndefined(); + }); + + it('is a no-op when the derived value already equals the supplied name', () => { + const result = deriveUserNameOnCreate({ name: 'jane_dj', username: 'jane_dj' }); + expect(result).toBeUndefined(); + }); + + it('trims and treats a blank djName as unusable, falling through to username', () => { + const result = deriveUserNameOnCreate({ name: 'realname-in-name-field', username: 'jane_dj', djName: ' ' }); + expect(result).toEqual({ data: { name: 'jane_dj' } }); + }); +}); + +describe('deriveOrRejectUserNameOnUpdate', () => { + it('derives the handle when the update sets a usable djName', () => { + const result = deriveOrRejectUserNameOnUpdate({ djName: 'DJ Jazzy Jane' }); + expect(result).toEqual({ data: { name: 'DJ Jazzy Jane' } }); + }); + + it('leaves name untouched when the update clears the handle (blank djName)', () => { + const result = deriveOrRejectUserNameOnUpdate({ djName: '' }); + expect(result).toBeUndefined(); + }); + + it('leaves name untouched on a username-only payload (no djName key at all)', () => { + const result = deriveOrRejectUserNameOnUpdate({ username: 'new_username' }); + expect(result).toBeUndefined(); + }); + + it('leaves name untouched when djName is set to the literal Anonymous', () => { + const result = deriveOrRejectUserNameOnUpdate({ djName: 'Anonymous' }); + expect(result).toBeUndefined(); + }); + + it('derives from djName on an onboarding-shaped payload carrying both realName and djName', () => { + // complete-onboarding.ts's markOnboardingComplete builds exactly this + // shape: { realName, djName, ... } via internalAdapter.updateUser, which + // still flows through updateWithHooks. + const result = deriveOrRejectUserNameOnUpdate({ realName: 'Jane Doe', djName: 'DJ Jazzy Jane' }); + expect(result).toEqual({ data: { name: 'DJ Jazzy Jane' } }); + }); + + it('leaves name untouched when djName is explicitly null', () => { + const result = deriveOrRejectUserNameOnUpdate({ djName: null }); + expect(result).toBeUndefined(); + }); + + // FINDING 1 (2297 review) rejection policy — full writeup lives once, on + // deriveOrRejectUserNameOnUpdate's docblock in derive-user-display-name.ts. + it('rejects a name-only payload outright (djName key absent)', () => { + const result = deriveOrRejectUserNameOnUpdate({ name: 'Some Real Name' }); + expect(result).toBe(false); + }); + + it('derives the handle and overrides a client-supplied name when both are present', () => { + // The override is better-auth's merge order, not this function — see the + // "MERGE CONTRACT" section of the docblock referenced above. + const result = deriveOrRejectUserNameOnUpdate({ name: 'Some Real Name', djName: 'DJ Jazzy Jane' }); + expect(result).toEqual({ data: { name: 'DJ Jazzy Jane' } }); + }); + + // Closes the trivial bypass of the name-only rejection above: without + // this, a caller could smuggle a bare `name` write through by attaching + // an unusable `djName` (blank, or the literal 'Anonymous') to the same + // payload — djName would be "present" but resolve to no handle, and the + // client-supplied name would slip through unrejected. A name-carrying + // payload is only ever allowed to leave `name` alone when it does NOT + // also try to set an unusable djName in the same breath; here it does + // both, so it's rejected exactly like the name-only case. + it('rejects a name payload accompanied by an unusable (blank) djName', () => { + const result = deriveOrRejectUserNameOnUpdate({ name: 'Some Real Name', djName: '' }); + expect(result).toBe(false); + }); + + it('rejects a name payload accompanied by djName set to the literal Anonymous', () => { + const result = deriveOrRejectUserNameOnUpdate({ name: 'Some Real Name', djName: 'Anonymous' }); + expect(result).toBe(false); + }); +}); diff --git a/tests/unit/authentication/pii-additional-fields-input.test.ts b/tests/unit/authentication/pii-additional-fields-input.test.ts new file mode 100644 index 000000000..20f773f25 --- /dev/null +++ b/tests/unit/authentication/pii-additional-fields-input.test.ts @@ -0,0 +1,29 @@ +import * as fs from 'fs'; +import * as path from 'path'; + +// FINDING 3 (BS#2297 review): auth.definition.ts's user.additionalFields +// defaulted realName/djName to input:true, so better-auth's public +// POST /update-user let any signed-in session rewrite them directly. Full +// writeup — the two-writers verification, and why this lock is complementary +// to (not redundant with) the databaseHooks.user veto — lives once, at the +// additionalFields.realName/djName site in auth.definition.ts. +describe('auth.definition.ts user.additionalFields PII input locks', () => { + const authDefPath = path.resolve(__dirname, '../../../shared/authentication/src/auth.definition.ts'); + let source: string; + + beforeAll(() => { + // eslint-disable-next-line security/detect-non-literal-fs-filename + source = fs.readFileSync(authDefPath, 'utf-8'); + }); + + it.each(['realName', 'djName'])( + 'locks %s to input: false so the public /update-user route cannot write it directly', + (field) => { + const match = source.match(new RegExp(`${field}:\\s*\\{([^}]*)\\}`)); + if (match === null) { + throw new Error(`additionalFields.${field} block not found in auth.definition.ts`); + } + expect(match[1]).toMatch(/input:\s*false/); + } + ); +}); diff --git a/tests/unit/database/dj-name.test.ts b/tests/unit/database/dj-name.test.ts new file mode 100644 index 000000000..d773e5fd3 --- /dev/null +++ b/tests/unit/database/dj-name.test.ts @@ -0,0 +1,51 @@ +/** + * Unit tests for `deriveUserPublicName` (shared/database/src/dj-name.ts). + * + * The `auth_user.name` policy — on-air handle, else username — consolidated + * from three independent call sites (apps/auth/provision-user.ts, the + * databaseHooks.user.create.before hook, the auth-user-name-backfill job's + * rewrite-target computation). See the function's docblock for why the + * update hook does NOT use this helper (its username link is + * create/backfill-only). + */ + +import { deriveUserPublicName } from '../../../shared/database/src/dj-name'; + +describe('deriveUserPublicName', () => { + it('the handle wins when djName is a usable handle', () => { + expect(deriveUserPublicName('DJ Jazzy Jane', 'jane_dj')).toBe('DJ Jazzy Jane'); + }); + + it('falls back to username when djName is absent', () => { + expect(deriveUserPublicName(null, 'jane_dj')).toBe('jane_dj'); + }); + + it('falls back to username when djName is the literal Anonymous', () => { + expect(deriveUserPublicName('Anonymous', 'jane_dj')).toBe('jane_dj'); + }); + + it('falls back to username when djName is blank', () => { + expect(deriveUserPublicName(' ', 'jane_dj')).toBe('jane_dj'); + }); + + // The documented micro-behavior change (BS#2297 review finding 1): a + // whitespace-only username is trimmed and treated as blank, converging on + // the mirror's documented contract (PR #2292). Unreachable via any current + // writer — usernames validate against /^[a-zA-Z0-9_.]+$/ before reaching + // this helper — so this only matters for a manually-edited legacy row. + it('returns null when both djName and username are whitespace-only', () => { + expect(deriveUserPublicName(' ', ' ')).toBeNull(); + }); + + it('returns null when djName is absent and username is whitespace-only', () => { + expect(deriveUserPublicName(null, ' ')).toBeNull(); + }); + + it('returns null when neither djName nor username is usable', () => { + expect(deriveUserPublicName(null, null)).toBeNull(); + }); + + it('trims a valid username', () => { + expect(deriveUserPublicName(null, ' jane_dj ')).toBe('jane_dj'); + }); +}); diff --git a/tests/unit/jobs/auth-user-name-backfill/decide.test.ts b/tests/unit/jobs/auth-user-name-backfill/decide.test.ts new file mode 100644 index 000000000..7e0eaa180 --- /dev/null +++ b/tests/unit/jobs/auth-user-name-backfill/decide.test.ts @@ -0,0 +1,134 @@ +/** + * Unit tests for the auth_user.name backfill's pure decision functions + * (DJ real-name PII safeguards plan, Track 2d). + * + * @wxyc/database resolves to tests/mocks/database.mock.ts (jest.unit.config.ts + * moduleNameMapper), which re-exports the REAL resolveDjDisplayName / + * deriveUserPublicName from shared/database/src/dj-name.ts — both decision + * functions here are exercised against the actual PII-safe chain, not a + * stub of it. + */ + +import { describe, it, expect } from '@jest/globals'; +import { + decideAuthUserNameBackfill, + violatesPreserveFirstPrecondition, +} from '../../../../jobs/auth-user-name-backfill/decide'; + +// Mirrors job.test.ts's rawRow(overrides) pattern: a default row shaped like +// the common case for both describe blocks below (case 1 of the gate suite, +// case 2 of the decide suite), so each test shows only the fields its case +// actually varies. `realName` is unused by decideAuthUserNameBackfill but +// harmless to carry — same "one factory, ignore what a caller doesn't need" +// convention as job.test.ts's rawRow. +const row = ( + overrides: Partial> = {} +) => ({ + realName: null, + isAnonymous: false, + name: 'Jane Doe', + username: 'jane_dj', + djName: null, + ...overrides, +}); + +describe('violatesPreserveFirstPrecondition (2a preserve-first gate predicate)', () => { + it('flags a row whose only legal-name copy is in name (real_name blank, name is neither Anonymous/Auto DJ/username)', () => { + expect(violatesPreserveFirstPrecondition(row())).toBe(true); + }); + + it('flags a row whose real_name is whitespace-only', () => { + expect(violatesPreserveFirstPrecondition(row({ realName: ' ' }))).toBe(true); + }); + + it('does not flag a row that already has a real_name', () => { + expect(violatesPreserveFirstPrecondition(row({ realName: 'Jane Doe' }))).toBe(false); + }); + + it('does not flag an anonymous user', () => { + expect(violatesPreserveFirstPrecondition(row({ isAnonymous: true, name: 'Anonymous', username: null }))).toBe( + false + ); + }); + + it('does not flag the literal Anonymous name', () => { + expect(violatesPreserveFirstPrecondition(row({ name: 'Anonymous', username: null }))).toBe(false); + }); + + it('does not flag the literal Auto DJ service-account name', () => { + expect(violatesPreserveFirstPrecondition(row({ name: 'Auto DJ', username: 'autodj', djName: 'Auto DJ' }))).toBe( + false + ); + }); + + it('does not flag a row where name already equals username (handle-less user with no real name to lose)', () => { + expect(violatesPreserveFirstPrecondition(row({ name: 'jane_dj' }))).toBe(false); + }); + + it('null-safely treats name/username as distinct when username is null (IS DISTINCT FROM semantics)', () => { + expect(violatesPreserveFirstPrecondition(row({ username: null }))).toBe(true); + }); + + // FINDING 2 (2297 review): a user provisioned AFTER this PR deploys can + // legitimately have name=handle, real_name blank, and name distinct from + // username (e.g. no username chosen yet, or username differs from the + // on-air handle) — that shape used to false-positive the gate forever, + // and the gate's own remediation message ("run 2a first") would have had + // an operator copy a HANDLE into the real_name PII column. Exempt any row + // whose trimmed `name` equals the resolved handle: there is no legal name + // to preserve, because `name` never held anything but the handle. + it('does not flag a post-deploy-provisioned row: name is the on-air handle, real_name blank, name distinct from username', () => { + expect( + violatesPreserveFirstPrecondition(row({ name: 'DJ Jazzy Jane', username: 'jjane', djName: 'DJ Jazzy Jane' })) + ).toBe(false); + }); + + it('still flags a genuine legacy row: name is a legal-looking value distinct from both the handle and username', () => { + expect(violatesPreserveFirstPrecondition(row({ username: 'jjane', djName: 'DJ Jazzy Jane' }))).toBe(true); + }); + + // Handle-is-real-name edge: a DJ whose real legal name coincidentally + // matches their on-air handle (e.g. handle "Jane Doe"). This row is + // exempted by the same rule as the post-deploy case above — trim(name) + // equals the resolved handle — even though `name` here also happens to + // equal what would be the real name. That's fine: 2a's own audit SQL + // carries the identical exemption ("handle-is-real-name", same as the + // stored-data scrub's), and there is genuinely no information lost by + // skipping this row — real_name still gets backfilled from `name` at 2a + // if it's ever populated by some other means, and the backfill job (2d) + // never overwrites a row's `name` away from a value that already equals + // its resolved handle (decideAuthUserNameBackfill is a no-op there too). + it('does not flag the handle-is-real-name coincidence: name equals both the handle and what could be a legal name', () => { + expect(violatesPreserveFirstPrecondition(row({ username: 'jjane', djName: 'Jane Doe' }))).toBe(false); + }); +}); + +describe('decideAuthUserNameBackfill', () => { + it('derives the handle when djName is a usable handle', () => { + expect(decideAuthUserNameBackfill(row({ djName: 'DJ Jazzy Jane' }))).toBe('DJ Jazzy Jane'); + }); + + it('falls back to username when djName is absent', () => { + expect(decideAuthUserNameBackfill(row())).toBe('jane_dj'); + }); + + it('falls back to username when djName is the literal Anonymous', () => { + expect(decideAuthUserNameBackfill(row({ djName: 'Anonymous' }))).toBe('jane_dj'); + }); + + it('leaves the row unchanged when the user is anonymous', () => { + expect(decideAuthUserNameBackfill(row({ isAnonymous: true, name: 'Anonymous', username: null }))).toBeUndefined(); + }); + + it('leaves the row unchanged when name is the literal Auto DJ', () => { + expect(decideAuthUserNameBackfill(row({ name: 'Auto DJ', username: 'autodj', djName: 'Auto DJ' }))).toBeUndefined(); + }); + + it('leaves the row unchanged when neither a usable handle nor a username exists', () => { + expect(decideAuthUserNameBackfill(row({ username: null }))).toBeUndefined(); + }); + + it('is a no-op when the derived value already equals the stored name', () => { + expect(decideAuthUserNameBackfill(row({ name: 'jane_dj' }))).toBeUndefined(); + }); +}); diff --git a/tests/unit/jobs/auth-user-name-backfill/job.test.ts b/tests/unit/jobs/auth-user-name-backfill/job.test.ts new file mode 100644 index 000000000..9c1be12c8 --- /dev/null +++ b/tests/unit/jobs/auth-user-name-backfill/job.test.ts @@ -0,0 +1,226 @@ +/** + * Unit tests for the auth_user.name backfill job orchestration (DJ real-name + * PII safeguards plan, Track 2d). The pure decisions (per-row rewrite, gate + * predicate) are tested directly in decide.test.ts; this file drives + * fetchAllUsers / runPreconditionGate / applyUpdate / runBackfill against a + * mocked db.execute. + */ + +import { db } from '@wxyc/database'; +import { + fetchAllUsers, + runPreconditionGate, + applyUpdate, + runBackfill, +} from '../../../../jobs/auth-user-name-backfill/job'; +import { renderSql } from '../../../utils/render-sql'; + +const rawRow = (overrides: Partial> = {}) => ({ + id: 'user-1', + name: 'Jane Doe', + username: 'jane_dj', + dj_name: 'DJ Jazzy Jane', + real_name: 'Jane Doe', + is_anonymous: false, + ...overrides, +}); + +describe('auth-user-name-backfill: fetchAllUsers', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + // FINDING 7 (2297 review): the un-filtered SELECT pulled every anonymous + // per-device row — and everyone's real_name — into memory only for + // decideAuthUserNameBackfill to discard the anonymous ones immediately. + // Anonymous rows plausibly dominate auth_user; there's no reason to pull + // legal names into process memory for rows the job throws away unread. + // The WHERE clause is behavior-identical: decideAuthUserNameBackfill (and + // the precondition gate) already skip is_anonymous rows unconditionally, + // so excluding them earlier changes nothing about which rows get written. + it('issues a SELECT against auth_user filtered to non-anonymous rows, with no LIMIT', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce([]); + + await fetchAllUsers(); + + const call = (db.execute as jest.Mock).mock.calls[0]; + const sqlText = renderSql(call?.[0]); + expect(sqlText).toMatch(/SELECT[\s\S]*FROM[\s\S]*auth_user/i); + expect(sqlText).toMatch(/WHERE[\s\S]*is_anonymous[\s\S]*DISTINCT FROM[\s\S]*true/i); + expect(sqlText).not.toMatch(/LIMIT/i); + }); + + it('maps snake_case columns onto the camelCase row shape', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce([rawRow()]); + + const rows = await fetchAllUsers(); + + expect(rows).toEqual([ + { + id: 'user-1', + name: 'Jane Doe', + username: 'jane_dj', + djName: 'DJ Jazzy Jane', + realName: 'Jane Doe', + isAnonymous: false, + }, + ]); + }); + + it('defaults a null is_anonymous to false', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce([rawRow({ is_anonymous: null })]); + + const rows = await fetchAllUsers(); + + expect(rows[0].isAnonymous).toBe(false); + }); +}); + +// Minimal violating row: real_name blank, name distinct from both username +// and the (absent) handle — see decide.test.ts's violatesPreserveFirstPrecondition +// suite for the full exemption matrix; this file only needs a +// known-violating shape to drive runPreconditionGate's id-list output. +const violatingRow = (id: string) => ({ + id, + name: 'Jane Doe', + username: 'jane_dj', + djName: null, + realName: null, + isAnonymous: false, +}); + +describe('auth-user-name-backfill: runPreconditionGate', () => { + it('does not throw when no row violates the 2a preserve-first predicate', () => { + expect(() => + runPreconditionGate([ + { id: 'u1', name: 'jane_dj', username: 'jane_dj', djName: null, realName: null, isAnonymous: false }, + { id: 'u2', name: 'Jane Doe', username: 'jane_dj', djName: null, realName: 'Jane Doe', isAnonymous: false }, + ]) + ).not.toThrow(); + }); + + it('throws naming Track 2a when a row still holds its only legal-name copy in name', () => { + expect(() => + runPreconditionGate([ + { + id: 'sentinel-user', + name: 'Jane Doe', + username: 'jane_dj', + djName: null, + realName: null, + isAnonymous: false, + }, + ]) + ).toThrow(/Track 2a/); + }); + + it('includes the violating row id(s) in the error message', () => { + expect(() => + runPreconditionGate([ + { + id: 'sentinel-user-001', + name: 'Jane Doe', + username: 'jane_dj', + djName: null, + realName: null, + isAnonymous: false, + }, + ]) + ).toThrow(/sentinel-user-001/); + }); + + // BS#2297 review finding 5: the message used to sample the first 10 + // violating ids ("...and N more"), leaving an operator to re-derive the + // rest by re-running the (still-PII-holding) query themselves. It must now + // carry every id, unconditionally. + it('includes every violating id, not a truncated sample, for more than 10 violations', () => { + const ids = Array.from({ length: 12 }, (_, i) => `sentinel-user-${String(i).padStart(3, '0')}`); + + expect(() => runPreconditionGate(ids.map(violatingRow))).toThrow(new RegExp(ids.map((id) => `'${id}'`).join(', '))); + }); + + it('quotes and comma-joins the violating ids for a direct WHERE id IN (...) paste', () => { + expect(() => runPreconditionGate([violatingRow('sentinel-user-a'), violatingRow('sentinel-user-b')])).toThrow( + /'sentinel-user-a', 'sentinel-user-b'/ + ); + }); + + it('tells the operator to copy name -> real_name for these ids, then re-run', () => { + expect(() => runPreconditionGate([violatingRow('sentinel-user-001')])).toThrow( + /copy name -> real_name for these ids, then re-run/ + ); + }); +}); + +describe('auth-user-name-backfill: applyUpdate', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('writes the derived name for the given id', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce({ count: 1 }); + + await applyUpdate('user-1', 'DJ Jazzy Jane'); + + const call = (db.execute as jest.Mock).mock.calls[0]; + const sqlText = renderSql(call?.[0]); + expect(sqlText).toMatch(/UPDATE[\s\S]*auth_user[\s\S]*SET[\s\S]*name/i); + expect(sqlText).toMatch(/WHERE[\s\S]*id/i); + const serialized = JSON.stringify(call?.[0]); + expect(serialized).toContain('user-1'); + expect(serialized).toContain('DJ Jazzy Jane'); + }); +}); + +describe('auth-user-name-backfill: runBackfill', () => { + beforeEach(() => { + jest.clearAllMocks(); + jest.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('aborts before writing anything when the precondition gate fails, even in dry-run', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce([ + rawRow({ id: 'sentinel-user', real_name: null, name: 'Jane Doe', username: 'jane_dj' }), + ]); + + await expect(runBackfill({ dryRun: true })).rejects.toThrow(/Track 2a/); + // Only the SELECT ran — no UPDATE was issued. + expect((db.execute as jest.Mock).mock.calls.length).toBe(1); + }); + + it('writes updated rows and skips unchanged ones in execute mode', async () => { + (db.execute as jest.Mock) + .mockResolvedValueOnce([ + rawRow({ id: 'u1', name: 'Jane Doe', username: 'jane_dj', dj_name: 'DJ Jazzy Jane', real_name: 'Jane Doe' }), + rawRow({ id: 'u2', name: 'bob_dj', username: 'bob_dj', dj_name: null, real_name: 'Bob Smith' }), + rawRow({ id: 'u3', name: 'Anonymous', username: null, dj_name: null, real_name: null, is_anonymous: true }), + ]) + .mockResolvedValue({ count: 1 }); + + const summary = await runBackfill({ dryRun: false }); + + expect(summary).toEqual({ scanned: 3, updated: 1, skipped: 2, dryRun: false }); + // SELECT + one UPDATE (u1 only — u2 already matches its derived value, u3 is anonymous). + expect((db.execute as jest.Mock).mock.calls.length).toBe(2); + const updateCall = (db.execute as jest.Mock).mock.calls[1]; + const serialized = JSON.stringify(updateCall?.[0]); + expect(serialized).toContain('u1'); + expect(serialized).toContain('DJ Jazzy Jane'); + }); + + it('computes the same summary in dry-run mode but issues no UPDATE', async () => { + (db.execute as jest.Mock).mockResolvedValueOnce([ + rawRow({ id: 'u1', name: 'Jane Doe', username: 'jane_dj', dj_name: 'DJ Jazzy Jane', real_name: 'Jane Doe' }), + ]); + + const summary = await runBackfill({ dryRun: true }); + + expect(summary).toEqual({ scanned: 1, updated: 1, skipped: 0, dryRun: true }); + // Only the SELECT — dry-run issues no write. + expect((db.execute as jest.Mock).mock.calls.length).toBe(1); + }); +});