From ab659b6f8dea1418d01cb5c228a103a4089bf732 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Mar 2022 14:41:04 -0700 Subject: [PATCH 1/5] unauthorized test could print summary --- Cargo.lock | 1 + nexus/Cargo.toml | 1 + nexus/tests/integration_tests/unauthorized.rs | 84 +++++++++++++++++++ .../unauthorized_coverage.rs | 2 - 4 files changed, 86 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7bd3525933b..e1cd660f636 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2441,6 +2441,7 @@ dependencies = [ "structopt", "subprocess", "tempfile", + "term", "thiserror", "tokio", "tokio-postgres", diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index 1dd7edf3831..ed70e6225d8 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -123,6 +123,7 @@ omicron-test-utils = { path = "../test-utils" } openapiv3 = "1.0" regex = "1.5.5" subprocess = "0.2.8" +term = "0.7" [dev-dependencies.openapi-lint] git = "https://github.com/oxidecomputer/openapi-lint" diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 0c0aacdad9c..a7fe05c5c7f 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -67,11 +67,40 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { // Verify the hardcoded endpoints. info!(log, "verifying endpoints"); + print!("{}", VERIFY_HEADER); for endpoint in &*VERIFY_ENDPOINTS { verify_endpoint(&log, client, endpoint).await; } } +const VERIFY_HEADER: &str = r#" +SUMMARY OF REQUESTS MADE + + +----------------------------- privileged GET (expects 200) + | (digit = last digit of 200-level response) + | + | +----+----+----+----+----- HTTP methods tested (see below) + | | | | | | (digit = last digit of 400-level response) + | | | | | | + | | | | | | +-- privileged GET (expects same as above) + | | | | | | | (digit = last digit of 200-level response) + | _| _| _| _| _| | ('-' => skipped (N/A)) + v / \ / \ / \ / \ / \ v +HEADER: G GET PUT POST DEL TRCE G URL +EXAMPLE: 0 3111 5555 3111 5555 5555 0 /organizations + ROW |||| + |||| TEST CASES FOR EACH HTTP METHOD: + +|||----------------------- authenticated, unauthorized request + +||----------------------- unauthenticated request + +|----------------------- bad authentication: no such user + +----------------------- bad authentication: invalid syntax + + In this case, an unauthenthicated request to "GET /organizations" returned + 401. All requests to "PUT /organizations" returned 405. + +G GET PUT POST DEL TRCE G URL +"#; + // // SETUP PHASE // @@ -216,6 +245,7 @@ async fn verify_endpoint( .any(|allowed| matches!(allowed, AllowedMethod::Get)); let resource_before: Option = if get_allowed { info!(log, "test: privileged GET"); + record_operation(WhichTest::PrivilegedGet(Some(&http::StatusCode::OK))); Some( NexusRequest::object_get(client, endpoint.url) .authn_as(AuthnMode::PrivilegedUser) @@ -227,9 +257,12 @@ async fn verify_endpoint( ) } else { warn!(log, "test: skipping privileged GET (method not allowed)"); + record_operation(WhichTest::PrivilegedGet(None)); None }; + print!(" "); + // For each of the HTTP methods we use in the API as well as TRACE, we'll // make several requests to this URL and verify the results. let methods = @@ -258,6 +291,7 @@ async fn verify_endpoint( .await .unwrap(); verify_response(&response); + record_operation(WhichTest::Unprivileged(&expected_status)); // Next, make an unauthenticated request. info!(log, "test: unauthenticated"; "method" => ?method); @@ -273,6 +307,7 @@ async fn verify_endpoint( .await .unwrap(); verify_response(&response); + record_operation(WhichTest::Unauthenticated(&expected_status)); // Now try a few requests with bogus credentials. We should get the // same error as if we were unauthenticated. This is sort of duplicated @@ -304,6 +339,7 @@ async fn verify_endpoint( .await .unwrap(); verify_response(&response); + record_operation(WhichTest::UnknownUser(&expected_status)); // Now try a syntactically invalid authn header. info!(log, "test: bogus creds: bad cred syntax"; "method" => ?method); @@ -320,6 +356,9 @@ async fn verify_endpoint( .await .unwrap(); verify_response(&response); + record_operation(WhichTest::InvalidHeader(&expected_status)); + + print!(" "); } // If we fetched the resource earlier, fetch it again and check the state. @@ -348,7 +387,14 @@ async fn verify_endpoint( resource_before, resource_after, "resource changed after making a bunch of failed requests" ); + record_operation(WhichTest::PrivilegedGetCheck(Some( + &http::StatusCode::OK, + ))); + } else { + record_operation(WhichTest::PrivilegedGetCheck(None)); } + + println!(" {}", endpoint.url); } /// Verifies the body of an HTTP response for status codes 401, 403, 404, or 405 @@ -379,3 +425,41 @@ fn verify_response(response: &TestResponse) { _ => unimplemented!(), } } + +/// Describes the tests run by [`verify_endpoint()`]. +enum WhichTest<'a> { + PrivilegedGet(Option<&'a http::StatusCode>), + Unprivileged(&'a http::StatusCode), + Unauthenticated(&'a http::StatusCode), + UnknownUser(&'a http::StatusCode), + InvalidHeader(&'a http::StatusCode), + PrivilegedGetCheck(Option<&'a http::StatusCode>), +} + +/// Prints one cell of the giant summary table describing the successful result +/// of one HTTP request. +fn record_operation(whichtest: WhichTest<'_>) { + // Extract the status code for the test. + let status_code = match whichtest { + WhichTest::PrivilegedGet(s) | WhichTest::PrivilegedGetCheck(s) => s, + WhichTest::Unprivileged(s) => Some(s), + WhichTest::Unauthenticated(s) => Some(s), + WhichTest::UnknownUser(s) => Some(s), + WhichTest::InvalidHeader(s) => Some(s), + }; + + // We'll print out the third digit of the HTTP status code. + let c = match status_code { + Some(s) => s.as_str().chars().nth(2).unwrap(), + None => '-', + }; + + // We only get here for successful results, so they're all green. You might + // think the color is pointless, but it does help the reader make sense of + // the mess of numbers that shows up in the table for the different response + // codes. + let mut t = term::stdout().unwrap(); + t.fg(term::color::GREEN).unwrap(); + write!(t, "{}", c).unwrap(); + t.reset().unwrap(); +} diff --git a/nexus/tests/integration_tests/unauthorized_coverage.rs b/nexus/tests/integration_tests/unauthorized_coverage.rs index 6cd74eeabd4..b76727d7447 100644 --- a/nexus/tests/integration_tests/unauthorized_coverage.rs +++ b/nexus/tests/integration_tests/unauthorized_coverage.rs @@ -73,8 +73,6 @@ fn test_unauthorized_coverage() { }) .collect(); - println!("spec operations: {:?}", spec_operations); - // Go through each of the authz test cases and match each one against an // OpenAPI operation. let mut unexpected_endpoints = String::from( From 8d88d69451824054d60b21784c3bb1ea07f8eb8e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Mar 2022 16:59:38 -0700 Subject: [PATCH 2/5] Ben's suggestion --- nexus/tests/integration_tests/unauthorized.rs | 31 +++++++++++-------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index a7fe05c5c7f..3c6044d27e0 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -76,24 +76,29 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { const VERIFY_HEADER: &str = r#" SUMMARY OF REQUESTS MADE - +----------------------------- privileged GET (expects 200) - | (digit = last digit of 200-level response) +KEY: + + +----------------------------> privileged GET (expects 200) + | (digit = last digit of 200-level + | response) | - | +----+----+----+----+----- HTTP methods tested (see below) - | | | | | | (digit = last digit of 400-level response) + | +----+----+----+----+----> HTTP methods tested (see below) + | | | | | | (digit = last digit of 400-level + | | | | | | response) | | | | | | - | | | | | | +-- privileged GET (expects same as above) - | | | | | | | (digit = last digit of 200-level response) + | | | | | | +-> privileged GET (expects same as above) + | | | | | | | (digit = last digit of 200-level + | | | | | | | response) | _| _| _| _| _| | ('-' => skipped (N/A)) - v / \ / \ / \ / \ / \ v + ^ / \ / \ / \ / \ / \ ^ HEADER: G GET PUT POST DEL TRCE G URL EXAMPLE: 0 3111 5555 3111 5555 5555 0 /organizations - ROW |||| - |||| TEST CASES FOR EACH HTTP METHOD: - +|||----------------------- authenticated, unauthorized request - +||----------------------- unauthenticated request - +|----------------------- bad authentication: no such user - +----------------------- bad authentication: invalid syntax + ROW ^^^^ + |||| TEST CASES FOR EACH HTTP METHOD: + +|||----------------------< authenticated, unauthorized request + +||----------------------< unauthenticated request + +|----------------------< bad authentication: no such user + +----------------------< bad authentication: invalid syntax In this case, an unauthenthicated request to "GET /organizations" returned 401. All requests to "PUT /organizations" returned 405. From b4a831f68f8bcde05a05febe446aa361767ef775 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Mar 2022 17:21:28 -0700 Subject: [PATCH 3/5] try something else --- nexus/tests/integration_tests/unauthorized.rs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 3c6044d27e0..32dd5bb7d2f 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -76,21 +76,17 @@ async fn test_unauthorized(cptestctx: &ControlPlaneTestContext) { const VERIFY_HEADER: &str = r#" SUMMARY OF REQUESTS MADE -KEY: +KEY, USING HEADER AND EXAMPLE ROW: +----------------------------> privileged GET (expects 200) | (digit = last digit of 200-level | response) | - | +----+----+----+----+----> HTTP methods tested (see below) - | | | | | | (digit = last digit of 400-level - | | | | | | response) - | | | | | | - | | | | | | +-> privileged GET (expects same as above) - | | | | | | | (digit = last digit of 200-level - | | | | | | | response) - | _| _| _| _| _| | ('-' => skipped (N/A)) - ^ / \ / \ / \ / \ / \ ^ + | +-> privileged GET (expects same as above) + | | (digit = last digit of 200-level + | | response) + | | ('-' => skipped (N/A)) + ^ ^ HEADER: G GET PUT POST DEL TRCE G URL EXAMPLE: 0 3111 5555 3111 5555 5555 0 /organizations ROW ^^^^ @@ -100,6 +96,12 @@ EXAMPLE: 0 3111 5555 3111 5555 5555 0 /organizations +|----------------------< bad authentication: no such user +----------------------< bad authentication: invalid syntax + \__/ \__/ \__/ \__/ \__/ + GET PUT etc. The test cases are repeated for each HTTP method. + + The number in each cell is the last digit of the 400-level response + that was expected for this test case. + In this case, an unauthenthicated request to "GET /organizations" returned 401. All requests to "PUT /organizations" returned 405. From 7af2309020445f8b147e4420ede2c5c84ebf7d07 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Mar 2022 18:32:40 -0700 Subject: [PATCH 4/5] fix CI --- nexus/tests/integration_tests/unauthorized.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 32dd5bb7d2f..4eae1c5e680 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -465,8 +465,12 @@ fn record_operation(whichtest: WhichTest<'_>) { // think the color is pointless, but it does help the reader make sense of // the mess of numbers that shows up in the table for the different response // codes. - let mut t = term::stdout().unwrap(); - t.fg(term::color::GREEN).unwrap(); + let t = term::stdout(); + if let Some(term) = t { + t.fg(term::color::GREEN).unwrap(); + } write!(t, "{}", c).unwrap(); - t.reset().unwrap(); + if let Some(term) = t { + t.reset().unwrap(); + } } From 9316477130fa74c3b2747337e5e17f268bb6ffe1 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Mar 2022 18:39:46 -0700 Subject: [PATCH 5/5] fix CI (cont) --- nexus/tests/integration_tests/unauthorized.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 4eae1c5e680..c879569455c 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -466,11 +466,12 @@ fn record_operation(whichtest: WhichTest<'_>) { // the mess of numbers that shows up in the table for the different response // codes. let t = term::stdout(); - if let Some(term) = t { - t.fg(term::color::GREEN).unwrap(); - } - write!(t, "{}", c).unwrap(); - if let Some(term) = t { - t.reset().unwrap(); + if let Some(mut term) = t { + term.fg(term::color::GREEN).unwrap(); + write!(term, "{}", c).unwrap(); + term.reset().unwrap(); + term.flush().unwrap(); + } else { + print!("{}", c); } }