From 0ded1dd99bb8077da2aec70ca6073e369bca156b Mon Sep 17 00:00:00 2001 From: Dave Kempe Date: Wed, 30 Sep 2026 17:18:44 +1000 Subject: [PATCH] test: move the map-group CLI tests to an integration test They call assert_cmd's cargo_bin, which needs CARGO_BIN_EXE_rustguac. Cargo exports that only to integration tests under tests/, never to a #[cfg(test)] module inside src/. As unit tests they passed locally purely because a stale target/debug/rustguac was lying around for assert_cmd's fallback to find; on a clean checkout, and so in CI, all five failed with "`CARGO_BIN_EXE_rustguac` is unset". They have been broken since they were written and nothing noticed, because the suite had never run in CI. Caught by the cargo test step added in 83a9110, on its first run. Database assertions now go through rusqlite directly rather than the crate's own db helpers, since rustguac is a binary crate with no lib target for an integration test to import. That is the better boundary regardless: it checks the state the CLI actually leaves behind. rusqlite joins dev-dependencies, which adds nothing to the tree as it is already a runtime dependency. --- Cargo.toml | 3 + src/main.rs | 138 ---------------------------------- tests/cli_map_group.rs | 165 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 138 deletions(-) create mode 100644 tests/cli_map_group.rs diff --git a/Cargo.toml b/Cargo.toml index f36a8f3..78080c9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,3 +96,6 @@ pulldown-cmark = "0.13" [dev-dependencies] assert_cmd = "2.2.2" +# Integration tests read the SQLite the CLI wrote. Already a runtime +# dependency, so this adds nothing new to the tree. +rusqlite = { version = "0.40", features = ["bundled"] } diff --git a/src/main.rs b/src/main.rs index e07c99e..ca5832f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1544,9 +1544,6 @@ fn rewrite_branding(html: &str, site_title: &str, logo_url: Option<&str>) -> Str #[cfg(test)] mod tests { - // Aliased: a bare `Command` here would shadow the clap `Command` enum that - // `use super::*` brings into scope. - use assert_cmd::Command as CargoBin; use super::*; @@ -1630,139 +1627,4 @@ mod tests { "no requests were throttled — rate-limit not applied" ); } - /// Scratch directory for a CLI test, removed on drop so a panicking test - /// leaves nothing behind. Named per test because cargo runs tests as - /// threads in one process, so a pid-only name would collide. - struct ScratchDir(std::path::PathBuf); - - impl ScratchDir { - fn new(name: &str) -> Self { - let path = - std::env::temp_dir().join(format!("rustguac-cli-{}-{}", name, std::process::id())); - let _ = std::fs::remove_dir_all(&path); - std::fs::create_dir_all(&path).expect("create scratch dir"); - std::fs::write(path.join("config.toml"), "db_path = \"./rustguac.db\"\n") - .expect("write config"); - ScratchDir(path) - } - - fn path(&self) -> &std::path::Path { - &self.0 - } - - fn db(&self) -> Db { - db::init_db(&self.0.join("rustguac.db")).expect("open database") - } - - /// Runs the CLI against this scratch dir, returning the assertion so - /// callers can require success or failure. - fn run(&self, args: &[&str]) -> assert_cmd::assert::Assert { - let mut cmd = CargoBin::cargo_bin("rustguac").unwrap(); - cmd.current_dir(self.path()) - .args(["--config", "config.toml"]) - .args(args) - .assert() - } - } - - impl Drop for ScratchDir { - fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.0); - } - } - - #[test] - fn cli_map_group() { - let dir = ScratchDir::new("map-group"); - dir.run(&["map-group", "--group", "superhumans", "--role", "admin"]) - .success(); - - let mappings = db::list_group_mappings(&dir.db()).expect("list group mappings"); - assert_eq!(mappings.len(), 1); - assert_eq!(mappings[0].oidc_group, "superhumans"); - assert_eq!(mappings[0].role, "admin"); - } - - #[test] - fn cli_map_group_rejects_unknown_role() { - let dir = ScratchDir::new("map-group-bad-role"); - dir.run(&["map-group", "--group", "superhumans", "--role", "root"]) - .failure(); - - assert!( - db::list_group_mappings(&dir.db()) - .expect("list group mappings") - .is_empty(), - "an invalid role must not create a mapping" - ); - } - - /// A duplicate must fail with guidance rather than a raw UNIQUE constraint - /// error, and must leave the existing role untouched. - #[test] - fn cli_map_group_duplicate_needs_force() { - let dir = ScratchDir::new("map-group-dup"); - dir.run(&["map-group", "--group", "staff", "--role", "viewer"]) - .success(); - - let assert = dir - .run(&["map-group", "--group", "staff", "--role", "admin"]) - .failure(); - let stderr = String::from_utf8_lossy(&assert.get_output().stderr).to_string(); - assert!( - stderr.contains("--force"), - "duplicate should point at --force, got: {stderr}" - ); - assert!( - !stderr.contains("UNIQUE"), - "duplicate should not surface a raw SQL error, got: {stderr}" - ); - - let mappings = db::list_group_mappings(&dir.db()).expect("list group mappings"); - assert_eq!(mappings.len(), 1); - assert_eq!(mappings[0].role, "viewer", "role must be unchanged"); - } - - #[test] - fn cli_map_group_force_remaps() { - let dir = ScratchDir::new("map-group-force"); - dir.run(&["map-group", "--group", "staff", "--role", "viewer"]) - .success(); - dir.run(&[ - "map-group", - "--group", - "staff", - "--role", - "admin", - "--force", - ]) - .success(); - - let mappings = db::list_group_mappings(&dir.db()).expect("list group mappings"); - assert_eq!(mappings.len(), 1, "force must re-map, not add a second row"); - assert_eq!(mappings[0].role, "admin"); - } - - /// Mapping a group to a role is a privilege grant, so it must leave an - /// audit trail even when done from a shell instead of the API. - #[test] - fn cli_map_group_is_audited() { - let dir = ScratchDir::new("map-group-audit"); - dir.run(&["map-group", "--group", "staff", "--role", "admin"]) - .success(); - - let database = dir.db(); - let conn = database.lock().unwrap(); - let (action, target, details): (String, String, String) = conn - .query_row( - "SELECT action, target, details FROM cli_audit_log ORDER BY id DESC LIMIT 1", - [], - |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), - ) - .expect("an audit row should exist"); - - assert_eq!(action, "map_group"); - assert_eq!(target, "staff"); - assert_eq!(details, "role=admin"); - } } diff --git a/tests/cli_map_group.rs b/tests/cli_map_group.rs new file mode 100644 index 0000000..1aa3776 --- /dev/null +++ b/tests/cli_map_group.rs @@ -0,0 +1,165 @@ +//! CLI behaviour for `rustguac map-group`. +//! +//! These live here rather than in a `#[cfg(test)]` module inside `src/` +//! because they invoke the built binary. Cargo only exports +//! `CARGO_BIN_EXE_rustguac`, which `assert_cmd::cargo_bin` needs, to +//! integration tests. As unit tests they appeared to pass locally only +//! because a stale `target/debug/rustguac` happened to be present for +//! assert_cmd's fallback to find; on a clean checkout they failed. +//! +//! Database assertions go through rusqlite directly rather than the crate's +//! own helpers, since rustguac is a binary crate with no lib target for an +//! integration test to import. That is arguably the better boundary anyway: +//! it checks the state the CLI actually leaves behind. + +use assert_cmd::Command; +use rusqlite::Connection; + +/// Scratch directory for one test, removed on drop so a panicking test leaves +/// nothing behind. Named per test because cargo runs tests as threads within +/// one process, so a pid-only name would collide. +struct ScratchDir(std::path::PathBuf); + +impl ScratchDir { + fn new(name: &str) -> Self { + let path = + std::env::temp_dir().join(format!("rustguac-cli-{}-{}", name, std::process::id())); + let _ = std::fs::remove_dir_all(&path); + std::fs::create_dir_all(&path).expect("create scratch dir"); + std::fs::write(path.join("config.toml"), "db_path = \"./rustguac.db\"\n") + .expect("write config"); + ScratchDir(path) + } + + fn path(&self) -> &std::path::Path { + &self.0 + } + + fn conn(&self) -> Connection { + Connection::open(self.0.join("rustguac.db")).expect("open database") + } + + /// Every group mapping as (group, role), ordered by id. + fn mappings(&self) -> Vec<(String, String)> { + let conn = self.conn(); + let mut stmt = conn + .prepare("SELECT oidc_group, role FROM group_role_mappings ORDER BY id") + .expect("prepare"); + let rows = stmt + .query_map([], |r| Ok((r.get(0)?, r.get(1)?))) + .expect("query"); + rows.map(|r| r.expect("row")).collect() + } + + /// Runs the CLI against this scratch dir, returning the assertion so + /// callers can require success or failure. + fn run(&self, args: &[&str]) -> assert_cmd::assert::Assert { + let mut cmd = Command::cargo_bin("rustguac").unwrap(); + cmd.current_dir(self.path()) + .args(["--config", "config.toml"]) + .args(args) + .assert() + } +} + +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +#[test] +fn cli_map_group() { + let dir = ScratchDir::new("map-group"); + dir.run(&["map-group", "--group", "superhumans", "--role", "admin"]) + .success(); + + assert_eq!( + dir.mappings(), + vec![("superhumans".to_string(), "admin".to_string())] + ); +} + +#[test] +fn cli_map_group_rejects_unknown_role() { + let dir = ScratchDir::new("map-group-bad-role"); + dir.run(&["map-group", "--group", "superhumans", "--role", "root"]) + .failure(); + + assert!( + dir.mappings().is_empty(), + "an invalid role must not create a mapping" + ); +} + +/// A duplicate must fail with guidance rather than a raw UNIQUE constraint +/// error, and must leave the existing role untouched. +#[test] +fn cli_map_group_duplicate_needs_force() { + let dir = ScratchDir::new("map-group-dup"); + dir.run(&["map-group", "--group", "staff", "--role", "viewer"]) + .success(); + + let assert = dir + .run(&["map-group", "--group", "staff", "--role", "admin"]) + .failure(); + let stderr = String::from_utf8_lossy(&assert.get_output().stderr).to_string(); + assert!( + stderr.contains("--force"), + "duplicate should point at --force, got: {stderr}" + ); + assert!( + !stderr.contains("UNIQUE"), + "duplicate should not surface a raw SQL error, got: {stderr}" + ); + + assert_eq!( + dir.mappings(), + vec![("staff".to_string(), "viewer".to_string())], + "role must be unchanged" + ); +} + +#[test] +fn cli_map_group_force_remaps() { + let dir = ScratchDir::new("map-group-force"); + dir.run(&["map-group", "--group", "staff", "--role", "viewer"]) + .success(); + dir.run(&[ + "map-group", + "--group", + "staff", + "--role", + "admin", + "--force", + ]) + .success(); + + assert_eq!( + dir.mappings(), + vec![("staff".to_string(), "admin".to_string())], + "force must re-map, not add a second row" + ); +} + +/// Mapping a group to a role is a privilege grant, so it must leave an audit +/// trail even when done from a shell instead of the API. +#[test] +fn cli_map_group_is_audited() { + let dir = ScratchDir::new("map-group-audit"); + dir.run(&["map-group", "--group", "staff", "--role", "admin"]) + .success(); + + let conn = dir.conn(); + let (action, target, details): (String, String, String) = conn + .query_row( + "SELECT action, target, details FROM cli_audit_log ORDER BY id DESC LIMIT 1", + [], + |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + ) + .expect("an audit row should exist"); + + assert_eq!(action, "map_group"); + assert_eq!(target, "staff"); + assert_eq!(details, "role=admin"); +}