move exception list into rust and use regular comments

Author: david-crespo

nexus/tests/integration_tests/audit_log.rs

@@ -38,20 +38,6 @@ use std::str::FromStr;
 type ControlPlaneTestContext =
     nexus_test_utils::ControlPlaneTestContext<omicron_nexus::Server>;
 
-/// Strip lines starting with `#` from a snapshot file so that the file
-/// can contain human-readable comments explaining why each entry is there.
-fn strip_comments(s: &str) -> String {
-    let mut out: String = s
-        .lines()
-        .filter(|line| !line.starts_with('#'))
-        .collect::<Vec<_>>()
-        .join("\n");
-    if s.ends_with('\n') {
-        out.push('\n');
-    }
-    out
-}
-
 fn to_q(d: DateTime<Utc>) -> String {
     d.to_rfc3339_opts(chrono::SecondsFormat::Micros, true)
 }
@@ -639,15 +625,11 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
     check_manual(
         "POST",
         "/v1/login/fake-silo/local",
-        RequestBuilder::new(
-            client,
-            Method::POST,
-            "/v1/login/fake-silo/local",
-        )
-        .body(Some(&serde_json::json!({
-            "username": "nonexistent",
-            "password": "doesntmatter"
-        }))),
+        RequestBuilder::new(client, Method::POST, "/v1/login/fake-silo/local")
+            .body(Some(&serde_json::json!({
+                "username": "nonexistent",
+                "password": "doesntmatter"
+            }))),
     )
     .await;
 
@@ -688,10 +670,11 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
     check_manual(
         "POST",
         "/device/confirm",
-        RequestBuilder::new(client, Method::POST, "/device/confirm")
-            .body(Some(&device::DeviceAuthVerify {
+        RequestBuilder::new(client, Method::POST, "/device/confirm").body(
+            Some(&device::DeviceAuthVerify {
                 user_code: "fake-code".to_string(),
-            })),
+            }),
+        ),
     )
     .await;
 
@@ -701,49 +684,47 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
         "/device/token",
         RequestBuilder::new(client, Method::POST, "/device/token")
             .body_urlencoded(Some(&device::DeviceAccessTokenRequest {
-                grant_type:
-                    "urn:ietf:params:oauth:grant-type:device_code"
-                        .to_string(),
+                grant_type: "urn:ietf:params:oauth:grant-type:device_code"
+                    .to_string(),
                 device_code: "fake-code".to_string(),
                 client_id: uuid::Uuid::nil(),
             })),
     )
     .await;
 
-    let mut output =
-        String::from("Mutating endpoints without audit logging:\n");
-    for (op_id, (method, path)) in &missing_audit {
-        output.push_str(&format!("{:44} ({:6} {:?})\n", op_id, method, path));
-    }
-
-    output.push_str(
-        "\nMutating endpoints not tested (not in VERIFY_ENDPOINTS):\n",
-    );
-    for (op_id, (method, path)) in &untested_mutating {
-        output.push_str(&format!("{:44} ({:6} {:?})\n", op_id, method, path));
-    }
-
-    // Print a helpful message when there are new uncovered endpoints
-    let expected_path = "tests/output/uncovered-audit-log-endpoints.txt";
-    let expected = strip_comments(
-        &std::fs::read_to_string(expected_path).unwrap_or_default(),
-    );
-    let expected_ops: std::collections::HashSet<&str> = expected
-        .lines()
-        .skip(1) // skip the header line
-        .filter_map(|line| line.split_whitespace().next())
-        .collect();
-    let unexpected_uncovered: Vec<_> = missing_audit
+    // Mutating POST endpoints that are intentionally not audit-logged.
+    // If you're adding a new endpoint here, include a comment explaining why.
+    let allowed_unaudited: BTreeMap<&str, (&str, &str)> = BTreeMap::from([
+        // Intermediate steps of the device OAuth flow. The meaningful
+        // endpoint is device_auth_confirm, which is where authentication
+        // and token creation happen. device_auth_request has no user
+        // identity, and device_access_token is just polling (mostly 400s)
+        // until the token created by confirm is ready.
+        ("device_access_token", ("post", "/device/token")),
+        ("device_auth_request", ("post", "/device/auth")),
+        // Called many times per disk image upload, other related endpoints
+        // cover it. See
+        // https://github.com/oxidecomputer/omicron/pull/10046
+        ("disk_bulk_write_import", ("post", "/v1/disks/{disk}/bulk-write")),
+        // Needs rework to extract actor identity before we can log it.
+        // Low priority since sessions expire anyway.
+        ("logout", ("post", "/v1/logout")),
+        // Read-only queries that happen to use POST for the request body.
+        ("system_timeseries_query", ("post", "/v1/system/timeseries/query")),
+        ("timeseries_query", ("post", "/v1/timeseries/query")),
+    ]);
+
+    let unexpected_unaudited: Vec<_> = missing_audit
         .keys()
-        .filter(|op| !expected_ops.contains(op.as_str()))
+        .filter(|op| !allowed_unaudited.contains_key(op.as_str()))
         .collect();
-    if !unexpected_uncovered.is_empty() {
+    if !unexpected_unaudited.is_empty() {
         eprintln!();
         eprintln!(
             "======================================================================="
         );
         eprintln!("ENDPOINTS MISSING AUDIT LOGGING:");
-        for op in &unexpected_uncovered {
+        for op in &unexpected_unaudited {
             eprintln!("  - {}", op);
         }
         eprintln!();
@@ -758,42 +739,53 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
             "If the endpoint is read-only despite using POST (like the timeseries"
         );
         eprintln!(
-            "query endpoints), add it to uncovered-audit-log-endpoints.txt."
+            "query endpoints), add it to the allowed_unaudited list in this test."
         );
         eprintln!(
             "======================================================================="
         );
         eprintln!();
     }
-
-    // NOTE: We intentionally do NOT use expectorate's assert_contents here
-    // because we don't want EXPECTORATE=overwrite to allow people to
-    // accidentally add uncovered endpoints to the allowlist.
-    similar_asserts::assert_eq!(
-        expected,
-        output,
-        "left: uncovered-audit-log-endpoints.txt, right: actual"
+    assert!(
+        unexpected_unaudited.is_empty(),
+        "Unexpected unaudited endpoints: {:?}",
+        unexpected_unaudited,
     );
 
-    // Check for GET endpoints that unexpectedly have audit logging
-    let mut get_output = String::from("GET endpoints with audit logging:\n");
-    for (op_id, (method, path)) in &unexpected_get_audit {
-        get_output
-            .push_str(&format!("{:44} ({:6} {:?})\n", op_id, method, path));
+    // Check that the allowlist doesn't have stale entries for endpoints
+    // that have since gained audit logging or been removed.
+    let actual_unaudited: BTreeMap<&str, (&str, &str)> = missing_audit
+        .iter()
+        .chain(untested_mutating.iter())
+        .map(|(op, (m, p))| (op.as_str(), (m.as_str(), p.as_str())))
+        .collect();
+    for (op, (method, path)) in &allowed_unaudited {
+        match actual_unaudited.get(op) {
+            None => {
+                panic!(
+                    "Stale allowed_unaudited entry: {} ({} {:?}) — \
+                     endpoint now has audit logging or no longer exists, \
+                     remove it from the list",
+                    op, method, path,
+                );
+            }
+            Some((actual_method, actual_path)) => {
+                assert_eq!(
+                    (actual_method, actual_path),
+                    (method, path),
+                    "allowed_unaudited entry for {} has wrong method/path",
+                    op,
+                );
+            }
+        }
     }
 
-    let get_expected_path = "tests/output/audited-get-endpoints.txt";
-    let get_expected = strip_comments(
-        &std::fs::read_to_string(get_expected_path).unwrap_or_default(),
-    );
-    let get_expected_ops: std::collections::HashSet<&str> = get_expected
-        .lines()
-        .skip(1) // skip the header line
-        .filter_map(|line| line.split_whitespace().next())
-        .collect();
+    // GET endpoints that are intentionally audit-logged (should be rare).
+    let allowed_audited_gets: BTreeMap<&str, (&str, &str)> = BTreeMap::new();
+
     let unexpected_audited: Vec<_> = unexpected_get_audit
         .keys()
-        .filter(|op| !get_expected_ops.contains(op.as_str()))
+        .filter(|op| !allowed_audited_gets.contains_key(op.as_str()))
         .collect();
     if !unexpected_audited.is_empty() {
         eprintln!();
@@ -811,20 +803,16 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
         eprintln!(
             "modify state. If this endpoint was intentionally audited (rare),"
         );
-        eprintln!("add it to audited-get-endpoints.txt.");
+        eprintln!("add it to the allowed_audited_gets list in this test.");
         eprintln!(
             "======================================================================="
         );
         eprintln!();
     }
-
-    // NOTE: We intentionally do NOT use expectorate's assert_contents here
-    // because we don't want EXPECTORATE=overwrite to allow people to
-    // accidentally add audited GET endpoints to the list.
-    similar_asserts::assert_eq!(
-        get_expected,
-        get_output,
-        "left: audited-get-endpoints.txt, right: actual"
+    assert!(
+        unexpected_audited.is_empty(),
+        "Unexpected audited GET endpoints: {:?}",
+        unexpected_audited,
     );
 }