diff --git a/backend/src/services/audit.rs b/backend/src/services/audit.rs index a249596..a0adb08 100644 --- a/backend/src/services/audit.rs +++ b/backend/src/services/audit.rs @@ -139,3 +139,176 @@ async fn resolve_names( .or_else(|| target_id.and_then(lookup)), ) } + +/// These live HERE, not in `tests/`, and that is the entire point. +/// +/// `backend/` is a binary crate, so an integration test cannot import `record`. The house rule in +/// `tests/common/mod.rs` — copy the production SQL character-for-character — works for pinning +/// behaviour that already existed, but applied to a NEW fix whose only coverage is the copy it +/// proves nothing: the fix and its test become two independent implementations, and deleting the +/// fix leaves the test green. The previous `tests/audit_names.rs` did exactly that, down to +/// asserting `actor_role == "host"` against its own hardcoded `.bind("host")` — an assertion that +/// could not fail for any change to the code it named. +/// +/// A `#[cfg(test)]` module inside the binary can call the real function, so these do. +#[cfg(test)] +mod tests { + use super::*; + + async fn seed_event(pool: &PgPool, slug: &str) -> Uuid { + sqlx::query_scalar("INSERT INTO event (slug, name) VALUES ($1, $2) RETURNING id") + .bind(slug) + .bind("Hochzeit") + .fetch_one(pool) + .await + .expect("seed event") + } + + async fn seed_user(pool: &PgPool, event_id: Uuid, name: &str) -> Uuid { + sqlx::query_scalar( + "INSERT INTO \"user\" (event_id, display_name, recovery_pin_hash) + VALUES ($1, $2, 'x') RETURNING id", + ) + .bind(event_id) + .bind(name) + .fetch_one(pool) + .await + .expect("seed user") + } + + async fn audit_row(pool: &PgPool, action: &str) -> Option<(Option, Option)> { + sqlx::query_as( + "SELECT actor_name, target_name FROM host_action_audit + WHERE action = $1 ORDER BY created_at DESC LIMIT 1", + ) + .bind(action) + .fetch_optional(pool) + .await + .expect("audit lookup") + } + + /// The ordinary case: the caller supplies no names and `record` resolves both from the ids. + /// This is what nine of the eleven call sites do. Revert `resolve_names` and both names go NULL. + #[sqlx::test] + async fn a_recorded_action_carries_both_names_without_the_caller_supplying_them(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let host = seed_user(&pool, event_id, "Gastgeberin Greta").await; + let guest = seed_user(&pool, event_id, "Gesperrter Gustav").await; + + record( + &pool, + event_id, + host, + None, + UserRole::Host, + "ban_user", + Some(guest), + None, + None, + ) + .await; + + let (actor_name, target_name) = audit_row(&pool, "ban_user").await.expect("a row"); + assert_eq!(actor_name.as_deref(), Some("Gastgeberin Greta")); + assert_eq!(target_name.as_deref(), Some("Gesperrter Gustav")); + } + + /// `as_str()`, not the `Debug` spelling. Asserted against `UserRole::as_str` itself rather than + /// a literal, so it tracks a rename instead of pretending to: what must hold is that the column + /// carries the SAME string the rest of the codebase uses, whatever that string is. Swap line 75 + /// back to `format!("{actor_role:?}")` and this goes red on the `Host`/`host` casing. + #[sqlx::test] + async fn the_role_column_carries_the_canonical_spelling(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let host = seed_user(&pool, event_id, "Gastgeberin Greta").await; + + record( + &pool, + event_id, + host, + None, + UserRole::Host, + "release_gallery", + None, + None, + None, + ) + .await; + + let role: String = sqlx::query_scalar( + "SELECT actor_role FROM host_action_audit WHERE action = 'release_gallery'", + ) + .fetch_one(&pool) + .await + .expect("role"); + assert_eq!(role, UserRole::Host.as_str()); + assert_ne!( + role, + format!("{:?}", UserRole::Host), + "the Debug spelling is not a wire format" + ); + } + + /// The case the columns exist for. `delete_account` hard-deletes the user row, so a name + /// resolved AFTER the fact would be NULL — the caller has to pass it in. + #[sqlx::test] + async fn a_name_supplied_by_the_caller_survives_the_row_being_deleted(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let leaver = seed_user(&pool, event_id, "Abschied Anke").await; + + // Exactly the order `me::delete_account` runs in: the row goes first, the audit row second. + sqlx::query("DELETE FROM \"user\" WHERE id = $1") + .bind(leaver) + .execute(&pool) + .await + .expect("delete user"); + + record( + &pool, + event_id, + leaver, + Some("Abschied Anke"), + UserRole::Guest, + "delete_account", + Some(leaver), + Some("Abschied Anke"), + None, + ) + .await; + + let (actor_name, target_name) = audit_row(&pool, "delete_account").await.expect("a row"); + assert_eq!( + actor_name.as_deref(), + Some("Abschied Anke"), + "the audit row must name the deleted account — resolving it later is impossible" + ); + assert_eq!(target_name.as_deref(), Some("Abschied Anke")); + } + + /// And the failure mode that made this worth testing: with nothing supplied and nothing to look + /// up, the write must still succeed (an audit row must never fail an action) and carry NULLs. + #[sqlx::test] + async fn an_unresolvable_name_writes_the_row_anyway(pool: PgPool) { + let event_id = seed_event(&pool, "wedding").await; + let ghost = Uuid::new_v4(); + + record( + &pool, + event_id, + ghost, + None, + UserRole::Host, + "reset_pin", + Some(ghost), + None, + None, + ) + .await; + + let (actor_name, target_name) = audit_row(&pool, "reset_pin") + .await + .expect("the row must be written even when no name can be resolved"); + assert_eq!(actor_name, None); + assert_eq!(target_name, None); + } +} diff --git a/backend/tests/audit_names.rs b/backend/tests/audit_names.rs deleted file mode 100644 index ac77440..0000000 --- a/backend/tests/audit_names.rs +++ /dev/null @@ -1,163 +0,0 @@ -//! The host action audit must stay READABLE, which means the names must actually be there. -//! -//! Migration 029 made `actor_id`/`target_id` non-FK on the stated grounds that "the record must -//! survive the actor's account being removed, which is exactly when it is most likely to be -//! wanted". Every call site passed `None` for both name columns, so what survived a deletion was a -//! bare uuid resolving to nothing — the guarantee, minus the only thing that made it useful. -//! -//! A NULL name fails silently: the row is written, the action succeeds, and nobody finds out until -//! a host is trying to answer "what happened to my photo?" the next morning. So it is asserted. - -mod common; - -use common::*; -use sqlx::PgPool; -use uuid::Uuid; - -async fn audit_row( - pool: &PgPool, - action: &str, -) -> Option<(Option, Option, String)> { - sqlx::query_as( - "SELECT actor_name, target_name, actor_role FROM host_action_audit - WHERE action = $1 ORDER BY created_at DESC LIMIT 1", - ) - .bind(action) - .fetch_optional(pool) - .await - .expect("audit lookup") -} - -/// The ordinary case: the caller supplies no names and `record` resolves both from the ids. -/// This is what nine of the eleven call sites do. -#[sqlx::test] -async fn a_recorded_action_carries_both_names_without_the_caller_supplying_them(pool: PgPool) { - let event_id = seed_event(&pool, "wedding").await; - let host = seed_user(&pool, event_id, "Gastgeberin Greta").await; - let guest = seed_user(&pool, event_id, "Gesperrter Gustav").await; - - eventsnap_backend_audit_record(&pool, event_id, host, None, "ban_user", Some(guest), None) - .await; - - let (actor_name, target_name, actor_role) = audit_row(&pool, "ban_user") - .await - .expect("a row was written"); - assert_eq!(actor_name.as_deref(), Some("Gastgeberin Greta")); - assert_eq!(target_name.as_deref(), Some("Gesperrter Gustav")); - // `as_str()`, not the Debug spelling — a renamed variant must not silently change the column. - assert_eq!(actor_role, "host"); -} - -/// The case the columns exist for. `delete_account` hard-deletes the user row, so a name resolved -/// AFTER the fact would be NULL — the caller has to pass it in, and this is what proves it does. -#[sqlx::test] -async fn a_name_supplied_by_the_caller_survives_the_row_being_deleted(pool: PgPool) { - let event_id = seed_event(&pool, "wedding").await; - let leaver = seed_user(&pool, event_id, "Abschied Anke").await; - - // Exactly the order `me::delete_account` runs in: the row goes first, the audit row second. - sqlx::query("DELETE FROM \"user\" WHERE id = $1") - .bind(leaver) - .execute(&pool) - .await - .expect("delete user"); - - eventsnap_backend_audit_record( - &pool, - event_id, - leaver, - Some("Abschied Anke"), - "delete_account", - Some(leaver), - Some("Abschied Anke"), - ) - .await; - - let (actor_name, target_name, _) = audit_row(&pool, "delete_account") - .await - .expect("a row was written"); - assert_eq!( - actor_name.as_deref(), - Some("Abschied Anke"), - "the audit row must name the deleted account — resolving it later is impossible" - ); - assert_eq!(target_name.as_deref(), Some("Abschied Anke")); -} - -/// And the failure mode that made this worth testing: with nothing supplied and nothing to look up, -/// the write must still succeed (an audit row must never fail an action) and simply carry NULLs. -#[sqlx::test] -async fn an_unresolvable_name_writes_the_row_anyway(pool: PgPool) { - let event_id = seed_event(&pool, "wedding").await; - let ghost = Uuid::new_v4(); - - eventsnap_backend_audit_record(&pool, event_id, ghost, None, "reset_pin", Some(ghost), None) - .await; - - let (actor_name, target_name, _) = audit_row(&pool, "reset_pin") - .await - .expect("the row must be written even when no name can be resolved"); - assert_eq!(actor_name, None); - assert_eq!(target_name, None); -} - -/// SRC: `services/audit.rs::record` — the resolution + insert, verbatim, since the crate is a -/// binary and the function is not importable from an integration test. -async fn eventsnap_backend_audit_record( - pool: &PgPool, - event_id: Uuid, - actor_id: Uuid, - actor_name: Option<&str>, - action: &str, - target_id: Option, - target_name: Option<&str>, -) { - let need_actor = actor_name.is_none(); - let need_target = target_name.is_none() && target_id.is_some(); - let (actor_name, target_name) = if !need_actor && !need_target { - ( - actor_name.map(str::to_owned), - target_name.map(str::to_owned), - ) - } else { - let mut wanted: Vec = Vec::with_capacity(2); - if need_actor { - wanted.push(actor_id); - } - if let Some(t) = target_id - && need_target - { - wanted.push(t); - } - let rows: Vec<(Uuid, String)> = - sqlx::query_as("SELECT id, display_name FROM \"user\" WHERE id = ANY($1)") - .bind(&wanted) - .fetch_all(pool) - .await - .unwrap_or_default(); - let lookup = |id: Uuid| rows.iter().find(|(i, _)| *i == id).map(|(_, n)| n.clone()); - ( - actor_name.map(str::to_owned).or_else(|| lookup(actor_id)), - target_name - .map(str::to_owned) - .or_else(|| target_id.and_then(lookup)), - ) - }; - - sqlx::query( - "INSERT INTO host_action_audit - (event_id, actor_id, actor_name, actor_role, action, target_id, target_name, detail) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8)", - ) - .bind(event_id) - .bind(actor_id) - .bind(actor_name.as_deref()) - .bind("host") - .bind(action) - .bind(target_id) - .bind(target_name.as_deref()) - .bind(Option::::None) - .execute(pool) - .await - .expect("audit insert must not fail"); -} diff --git a/backend/tests/upload_concurrency.rs b/backend/tests/upload_concurrency.rs index 3c55f78..e35e107 100644 --- a/backend/tests/upload_concurrency.rs +++ b/backend/tests/upload_concurrency.rs @@ -15,7 +15,9 @@ use common::*; use sqlx::PgPool; use uuid::Uuid; -/// SRC: `handlers/upload.rs:313-322` — the guarded quota increment, verbatim. +/// SRC: `handlers/upload.rs::create_upload` — the guarded quota increment, verbatim. +/// (Named, not line-numbered: the previous pointer drifted by ~130 lines and landed in unrelated +/// code, which is how a hand-copied fixture silently stops matching its original.) /// Returns `rows_affected()`; the handler aborts the whole upload tx when this is 0. async fn quota_inc(exec: impl sqlx::PgExecutor<'_>, user_id: Uuid, size: i64, limit: i64) -> u64 { sqlx::query( @@ -146,7 +148,8 @@ async fn quota_guard_is_atomic_under_concurrent_transactions(pool: PgPool) { // 6. The `FOR SHARE` upload lock vs. the release // ───────────────────────────────────────────────────────────────────────────── -/// SRC: `handlers/upload.rs:297-303` — the in-transaction re-check under a row lock, verbatim. +/// SRC: `handlers/upload.rs::create_upload` — the in-transaction `FOR SHARE` re-check, verbatim. +/// (Named, not line-numbered — see the note on `quota_inc`.) async fn lock_and_read_event( tx: &mut sqlx::PgConnection, event_id: Uuid, @@ -254,12 +257,19 @@ async fn for_share_upload_lock_serializes_against_release(pool: PgPool) { } /// The other side of the same lock: once the release has COMMITTED, the next upload's `FOR SHARE` -/// re-read sees `export_released_at` set and the handler rejects it with `UploadsLocked`. +/// re-read sees `export_released_at` set and the handler bails out. /// /// PREVENTS: the same lost photo, on the losing side of the race — a photo committing AFTER the /// export snapshot would be in the live feed but missing from the keepsake. Rejecting is the correct -/// outcome, and it is reversible: `UploadsLocked` (not Forbidden) tells the client to keep the blob -/// and resume when the host reopens. +/// outcome, and it is reversible: the client keeps the blob and resumes when the host reopens. +/// +/// SCOPE, because the name overstates it: this asserts only what the LOCKED READ observes. It does +/// not go through the handler, so it says nothing about which error the handler picks. That +/// distinction is load-bearing — `create_upload` answers a released gallery with `GalleryReleased` +/// and a plain lock with `UploadsLocked`, in that order, and the two drive different client +/// behaviour (a `reopen` park vs. a retry). The ordering is covered end-to-end by +/// `e2e/specs/10-flow-review/upload-lock-code.spec.ts` and `02-upload/retry-after-release.spec.ts`; +/// this test's doc used to claim `UploadsLocked` outright and was simply wrong after that split. #[sqlx::test] async fn upload_after_release_commits_sees_the_lock_and_is_rejected(pool: PgPool) { let event_id = seed_event(&pool, "wedding").await; diff --git a/e2e/specs/02-upload/retry-after-release.spec.ts b/e2e/specs/02-upload/retry-after-release.spec.ts index a4d49ae..0c6eed6 100644 --- a/e2e/specs/02-upload/retry-after-release.spec.ts +++ b/e2e/specs/02-upload/retry-after-release.spec.ts @@ -75,11 +75,21 @@ test.describe('Upload — a retry after release replays instead of refusing', () ); // 4. And no second row was created — the whole point of the key. + // + // Counted by UPLOADER, not by `id`. Filtering on `u.id === original.id` looks like a duplicate + // check and is not one: a duplicate row gets a fresh uuid, so it could never match, and the + // filter yields exactly 1 whether the gallery holds one copy or five. This guest uploaded once + // successfully ('a.jpg'); 'b.jpg' was refused at step 2 and the retry must have replayed rather + // than stored, so their total must be exactly one. const feed = await fetch(`${BASE}/api/v1/feed?limit=100`, { headers: { Authorization: `Bearer ${g.jwt}` }, }); const items: any[] = (await feed.json()).uploads ?? []; - const mine = items.filter((u) => u.id === original.id); - expect(mine.length, 'the photo must appear exactly once').toBe(1); + const mine = items.filter((u) => u.user_id === g.userId); + expect( + mine.length, + `the retry must not have stored a second copy; got ${mine.map((u) => u.id).join(', ')}` + ).toBe(1); + expect(mine[0].id, 'and the one that exists is the original').toBe(original.id); }); }); diff --git a/frontend/export-viewer/vite.standalone.config.js b/frontend/export-viewer/vite.standalone.config.js index ffd0c48..b8d3d80 100644 --- a/frontend/export-viewer/vite.standalone.config.js +++ b/frontend/export-viewer/vite.standalone.config.js @@ -115,8 +115,17 @@ export default defineConfig({ // viewer at all. Before the guard existed the build could not fail, so neither could this. // // The only output is a single `index.html`, overwritten on every successful build, so - // there is nothing to accumulate — and a failed build now leaves the last good artifact in - // place instead of deleting it. + // there is nothing to accumulate. + // + // This protects ONE of the two failure paths, not both. `inlineThemeFonts` errors from + // `generateBundle`, before anything is written, so the previous artifact survives intact. + // The external-asset assertion errors from `writeBundle` — which runs AFTER Vite has + // written `index.html` — so that failure does overwrite the good viewer with the broken + // one. It cannot ship: the build exits non-zero, and `the_keepsake_viewer_is_compiled_into_ + // this_binary` plus the `!html.contains("url(/")` assertion both fail the Rust test suite + // before any image is built. But if a `writeBundle` failure is what you are looking at, + // restore the artifact with `git checkout backend/static/export-viewer/` rather than + // assuming the working copy is still the last good one. emptyOutDir: false, target: 'es2020' }