`backend/tests/` follows a house rule of copying production SQL character-for-
character rather than calling `src/`, because the crate is a binary and nothing
in it is importable from an integration test. For pinning behaviour that already
existed that is a defensible trade. 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.
`audit_names.rs` did exactly that. It never called `audit::record` — it
reimplemented `resolve_names` and the INSERT inside the test file, down to a
hardcoded `.bind("host")`, and then asserted `actor_role == "host"` against its
own literal. That assertion could not fail for any change to the code it named,
and grep confirmed there was no other coverage of the audit-name work anywhere.
Moved into `#[cfg(test)]` inside `services/audit.rs`, where the real function IS
callable. CI already runs `cargo test --all-features` with a live DATABASE_URL,
so `#[sqlx::test]` works there; verified all four run and pass. The role
assertion now compares against `UserRole::as_str()` itself rather than a literal,
so it tracks a rename instead of pretending to, plus an explicit `assert_ne!`
against the Debug spelling.
Also:
- `retry-after-release.spec.ts` filtered the feed on `u.id === original.id` to
prove "no second row was created". A duplicate gets a fresh uuid and could
never match, so the filter yielded exactly 1 whether the gallery held one copy
or five. Counts by uploader now, with the original's identity asserted
separately. (The rest of that spec is sound — its 403 control and replay-id
check both fail if the header fast-path is reverted.)
- `upload_after_release_commits_sees_the_lock_and_is_rejected` claimed the
handler answers `UploadsLocked`. It answers `GalleryReleased` since the check
order was inverted on this branch, and the test asserts no variant at all.
Documented what it actually covers (the locked READ) and where the ordering IS
covered (two e2e specs).
- Two `// SRC:` pointers had drifted ~130 lines into unrelated code, which is how
a hand-copied fixture silently stops matching its original. Now named, not
numbered.
- `emptyOutDir: false` claimed a failed viewer build "leaves the last good
artifact in place". True for the `generateBundle` error, false for the newer
`writeBundle` assertion, which fires after Vite has already written the file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
315 lines
12 KiB
Rust
315 lines
12 KiB
Rust
//! Append-only record of privileged actions. See migration 029 for why it exists.
|
|
//!
|
|
//! Design constraints, both learned from the rest of this codebase:
|
|
//!
|
|
//! * **Never fail the action.** An audit write that can turn a successful ban into a 500 makes
|
|
//! moderation less reliable than no audit at all. Every failure here is logged and swallowed.
|
|
//! * **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;
|
|
use uuid::Uuid;
|
|
|
|
use crate::models::user::UserRole;
|
|
|
|
/// Record one privileged action.
|
|
///
|
|
/// Takes `&PgPool` rather than a transaction on purpose: the audit row is not part of the action's
|
|
/// atomicity. If the action commits and the audit write fails we want the action to stand (and a
|
|
/// loud log line); if the action rolls back, an orphan audit row saying "someone tried" is more
|
|
/// useful than silence.
|
|
#[allow(clippy::too_many_arguments)]
|
|
pub async fn record(
|
|
pool: &PgPool,
|
|
event_id: Uuid,
|
|
actor_id: Uuid,
|
|
actor_name: Option<&str>,
|
|
actor_role: UserRole,
|
|
action: &str,
|
|
target_id: Option<Uuid>,
|
|
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)
|
|
VALUES ($1, $2, $3, $4, $5, $6, $7, $8)",
|
|
)
|
|
.bind(event_id)
|
|
.bind(actor_id)
|
|
.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.as_deref())
|
|
.bind(detail)
|
|
.execute(pool)
|
|
.await;
|
|
|
|
match result {
|
|
Ok(_) => {}
|
|
Err(e) => {
|
|
// `error`, not `warn`: losing an audit row is the kind of thing that should show up in
|
|
// whatever is watching the logs, even though it must not fail the request.
|
|
tracing::error!(
|
|
error = ?e, action, %actor_id, ?target_id,
|
|
"failed to write host action audit row"
|
|
);
|
|
}
|
|
}
|
|
}
|
|
|
|
/// 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)),
|
|
)
|
|
}
|
|
|
|
/// 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<String>, Option<String>)> {
|
|
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);
|
|
}
|
|
}
|