diff --git a/backend/src/handlers/me.rs b/backend/src/handlers/me.rs index 0471bc1..6bd2004 100644 --- a/backend/src/handlers/me.rs +++ b/backend/src/handlers/me.rs @@ -248,15 +248,21 @@ pub async fn delete_account( )); // Audited like the host actions it resembles, with the actor and target being the same person. + // + // The names are passed EXPLICITLY here, unlike every other call site. `audit::record` resolves + // a missing name by looking the id up in `"user"` — and this handler has just hard-deleted that + // row, so the lookup would find nothing and write the NULL that makes the record unreadable. + // This is the row most likely to be read later ("whose photos disappeared?"), and migration 029 + // made these columns non-FK precisely so it would survive the deletion. crate::services::audit::record( &state.pool, auth.event_id, auth.user_id, - None, + Some(&user.display_name), user.role.clone(), "delete_account", Some(auth.user_id), - None, + Some(&user.display_name), Some(serde_json::json!({ "uploads_removed": files.len() })), ) .await; diff --git a/backend/src/services/audit.rs b/backend/src/services/audit.rs index eff277c..a249596 100644 --- a/backend/src/services/audit.rs +++ b/backend/src/services/audit.rs @@ -7,6 +7,21 @@ //! * **Never store a credential.** `reset_pin` is the action most worth recording and the one //! whose payload must never be in `detail` — a table that could hand back a guest's PIN would //! be a worse privacy problem than the gap it closes. +//! +//! **Action slugs actually written**, since migration 029's header lists three (`promote_user`, +//! `demote_user`, `delete_user`) that no call site has ever emitted, and the migration file cannot +//! be corrected without changing its checksum and crash-looping every database that ran it: +//! +//! `ban_user`, `unban_user`, `set_role`, `reset_pin`, `delete_upload`, `delete_comment`, +//! `lock_uploads`, `unlock_uploads`, `release_gallery`, `delete_account`, `patch_config`. +//! Eleven, one per `audit::record` call site — grep for it if this list ages. +//! +//! **There is deliberately no read endpoint.** The table is queried by hand: +//! +//! ```sql +//! SELECT created_at, actor_name, actor_role, action, target_name, detail +//! FROM host_action_audit ORDER BY created_at DESC LIMIT 50; +//! ``` use serde_json::Value; use sqlx::PgPool; @@ -32,6 +47,20 @@ pub async fn record( target_name: Option<&str>, detail: Option, ) { + // Resolve whatever names the caller did not supply. + // + // Migration 029 made `actor_id`/`target_id` deliberately non-FK so "the record survives the + // actor's account being removed, which is exactly when it is most likely to be wanted". Every + // caller passed None for both names, so what survived was a bare uuid resolving to nothing — + // the guarantee the column exists for, minus the only thing that made it readable. + // + // Resolved HERE rather than at eleven call sites so none can be missed. The one caller that + // destroys the row it is recording — `me::delete_account` — must still pass the name in, since + // by the time this runs there is nothing left to look up, and that is precisely the row a host + // will be reading the next morning ("whose photos disappeared?"). + let (actor_name, target_name) = + resolve_names(pool, actor_id, actor_name, target_id, target_name).await; + let result = sqlx::query( "INSERT INTO host_action_audit (event_id, actor_id, actor_name, actor_role, action, target_id, target_name, detail) @@ -39,11 +68,14 @@ pub async fn record( ) .bind(event_id) .bind(actor_id) - .bind(actor_name) - .bind(format!("{actor_role:?}").to_lowercase()) + .bind(actor_name.as_deref()) + // `as_str()`, not `format!("{actor_role:?}")`: the Debug spelling is not a stable wire format, + // so a `#[derive(Debug)]` change or a renamed variant would silently start writing a different + // string into a column nothing validates. `as_str` is the one the rest of the codebase uses. + .bind(actor_role.as_str()) .bind(action) .bind(target_id) - .bind(target_name) + .bind(target_name.as_deref()) .bind(detail) .execute(pool) .await; @@ -60,3 +92,50 @@ pub async fn record( } } } + +/// Fill in any name the caller left as `None`, in ONE query. +/// +/// Best-effort by the same rule as the insert: a failed lookup writes NULL rather than failing the +/// action, and it is one round-trip whether zero, one or both names are missing. +async fn resolve_names( + pool: &PgPool, + actor_id: Uuid, + actor_name: Option<&str>, + target_id: Option, + target_name: Option<&str>, +) -> (Option, Option) { + let need_actor = actor_name.is_none(); + let need_target = target_name.is_none() && target_id.is_some(); + if !need_actor && !need_target { + return ( + actor_name.map(str::to_owned), + target_name.map(str::to_owned), + ); + } + + 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)), + ) +} diff --git a/backend/tests/audit_names.rs b/backend/tests/audit_names.rs new file mode 100644 index 0000000..ac77440 --- /dev/null +++ b/backend/tests/audit_names.rs @@ -0,0 +1,163 @@ +//! 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"); +}