fix(audit): give the audit trail the names that make it readable

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". All eleven call sites then 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.

`record` now resolves whatever the caller omitted, in one query, so no call
site can forget. `me::delete_account` passes its names explicitly because it
has already hard-deleted the row by then — that is the one record a host is
most likely to be reading the next morning ("whose photos disappeared?").

Also: `actor_role` is written with `as_str()` rather than
`format!("{actor_role:?}")`. The Debug spelling is not a stable wire format,
so a derive change or a renamed variant would have silently started writing a
different string into a column nothing validates.

Migration 029's header lists three action slugs (`promote_user`,
`demote_user`, `delete_user`) that no call site has ever emitted, and it
cannot be corrected — editing an applied migration changes its checksum and
crash-loops every database that ran it. The real list, verified against the
call sites, is documented in this module instead, along with the fact that
there is no read endpoint and the query to use by hand.

A NULL name fails silently, so it is now asserted: names resolved from ids,
names surviving the row's deletion, and a row still written when neither can
be resolved (an audit write must never fail the action it records).

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;

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<Value>,
 ) {
+    // 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(actor_name.as_deref())
-    .bind(format!("{actor_role:?}").to_lowercase())
+    // `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<Uuid>,
+    target_name: Option<&str>,
+) -> (Option<String>, Option<String>) {
+    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<Uuid> = 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)),
+    )
+}

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<String>, Option<String>, 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<Uuid>,
+    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<Uuid> = 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::<serde_json::Value>::None)
+    .execute(pool)
+    .await
+    .expect("audit insert must not fail");
+}