Skip to content

Commit 363d7a7

Browse files
[authz] remove expectorate for authz coverage test (#9761)
Depends on #9760 Fixes #9748 --------- Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
1 parent e051ce7 commit 363d7a7

2 files changed

Lines changed: 47 additions & 27 deletions

File tree

nexus/tests/integration_tests/audit_log.rs

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,6 @@ async fn test_audit_log_create_delete_ops(ctx: &ControlPlaneTestContext) {
401401
#[nexus_test]
402402
async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
403403
use super::endpoints::{AllowedMethod, VERIFY_ENDPOINTS};
404-
use expectorate::assert_contents;
405404
use nexus_test_utils::http_testing::{AuthnMode, NexusRequest};
406405
use openapiv3::OpenAPI;
407406
use std::collections::BTreeMap;
@@ -564,16 +563,22 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
564563
"If the endpoint is read-only despite using POST (like the timeseries"
565564
);
566565
eprintln!(
567-
"query endpoints), rerun the test with EXPECTORATE=overwrite to update"
566+
"query endpoints), add it to uncovered-audit-log-endpoints.txt."
568567
);
569-
eprintln!("the list of uncovered endpoints.");
570568
eprintln!(
571569
"======================================================================="
572570
);
573571
eprintln!();
574572
}
575573

576-
assert_contents(expected_path, &output);
574+
// NOTE: We intentionally do NOT use expectorate's assert_contents here
575+
// because we don't want EXPECTORATE=overwrite to allow people to
576+
// accidentally add uncovered endpoints to the allowlist.
577+
similar_asserts::assert_eq!(
578+
expected,
579+
output,
580+
"left: uncovered-audit-log-endpoints.txt, right: actual"
581+
);
577582

578583
// Check for GET endpoints that unexpectedly have audit logging
579584
let mut get_output = String::from("GET endpoints with audit logging:\n");
@@ -610,16 +615,21 @@ async fn test_audit_log_coverage(ctx: &ControlPlaneTestContext) {
610615
eprintln!(
611616
"modify state. If this endpoint was intentionally audited (rare),"
612617
);
613-
eprintln!(
614-
"rerun the test with EXPECTORATE=overwrite to update the list."
615-
);
618+
eprintln!("add it to audited-get-endpoints.txt.");
616619
eprintln!(
617620
"======================================================================="
618621
);
619622
eprintln!();
620623
}
621624

622-
assert_contents(get_expected_path, &get_output);
625+
// NOTE: We intentionally do NOT use expectorate's assert_contents here
626+
// because we don't want EXPECTORATE=overwrite to allow people to
627+
// accidentally add audited GET endpoints to the list.
628+
similar_asserts::assert_eq!(
629+
get_expected,
630+
get_output,
631+
"left: audited-get-endpoints.txt, right: actual"
632+
);
623633
}
624634

625635
fn verify_entry(

nexus/tests/integration_tests/unauthorized_coverage.rs

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -126,25 +126,35 @@ fn test_unauthorized_coverage() {
126126
));
127127
}
128128

129-
// If you're here because this assertion failed, check that if you've added
130-
// any API operations to Nexus, you've also added a corresponding test in
131-
// "unauthorized.rs" so that it will automatically be checked for its
132-
// behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS.
133-
// Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`].
134-
// If you _added_ a test that covered an endpoint from the allowlist --
135-
// hooray! Just delete the corresponding line from this file. (Why is this
136-
// not `expectorate::assert_contents`? Because we only expect this file to
137-
// ever shrink, which is easy enough to fix by hand, and we don't want to
138-
// make it easy to accidentally add things to the allowlist.)
139-
// let expected_uncovered_endpoints =
140-
// std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt")
141-
// .expect("failed to load file of allowed uncovered endpoints");
142-
143-
// TODO: Update this to remove overwrite capabilities
144-
// See https://github.com/oxidecomputer/expectorate/pull/12
145-
assert_contents(
146-
"tests/output/uncovered-authz-endpoints.txt",
147-
uncovered_endpoints.as_str(),
129+
// If you're here because this assertion failed, you've added an API
130+
// operation to Nexus without adding a corresponding test in
131+
// "unauthorized.rs" to check its behavior for unauthenticated and
132+
// unauthorized users. DO NOT SKIP THIS. Even if you're just adding a stub,
133+
// see [`Nexus::unimplemented_todo()`].
134+
//
135+
// To fix this:
136+
// 1. Add a VerifyEndpoint entry in endpoints.rs for your new endpoint
137+
// 2. Run the test_unauthorized test to verify it works
138+
//
139+
// The allowed uncovered endpoints file should only ever SHRINK (when you
140+
// add coverage for an endpoint). It should never grow. If you've added
141+
// coverage for an endpoint, you can remove it from the allowlist file.
142+
//
143+
// NOTE: We intentionally do NOT use expectorate's assert_contents here
144+
// because we don't want EXPECTORATE=overwrite to allow people to
145+
// accidentally add uncovered endpoints to the allowlist.
146+
let expected_uncovered_endpoints =
147+
std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt")
148+
.expect("failed to read uncovered-authz-endpoints.txt");
149+
assert!(
150+
uncovered_endpoints == expected_uncovered_endpoints,
151+
"Uncovered endpoints list doesn't match expected.\n\n\
152+
If you ADDED a new endpoint, add authz coverage in endpoints.rs.\n\n\
153+
If you ADDED coverage for an existing endpoint, remove it from \
154+
tests/output/uncovered-authz-endpoints.txt.\n\n\
155+
Expected:\n{}\n\nActual:\n{}",
156+
expected_uncovered_endpoints,
157+
uncovered_endpoints
148158
);
149159
}
150160

0 commit comments

Comments
 (0)