diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 527327450a..3ec0c44ec2 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -521,11 +521,13 @@ pub enum ResourceType { Project, Dataset, Disk, + Image, Instance, NetworkInterface, Rack, Sled, SagaDbg, + Snapshot, Volume, Vpc, VpcFirewallRule, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 680590535c..8ba7f3db2d 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1201,7 +1201,7 @@ async fn images_get( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let images = nexus - .list_images( + .images_list( &opctx, &data_page_params_for(&rqctx, &query)? .map_name(|n| Name::ref_cast(n)), @@ -1233,7 +1233,7 @@ async fn images_post( let new_image_params = &new_image.into_inner(); let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let image = nexus.create_image(&opctx, &new_image_params).await?; + let image = nexus.image_create(&opctx, &new_image_params).await?; Ok(HttpResponseCreated(image.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -1289,7 +1289,7 @@ async fn images_delete_image( let image_name = &path.image_name; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - nexus.delete_image(&opctx, &image_name).await?; + nexus.image_delete(&opctx, &image_name).await?; Ok(HttpResponseDeleted()) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 07f3ed0669..befbf7102d 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -21,7 +21,7 @@ pub mod db; // Public for documentation examples mod defaults; pub mod external_api; // Public for testing pub mod internal_api; // Public for testing -mod nexus; +pub mod nexus; // Public for documentation examples mod populate; mod saga_interface; mod sagas; diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index b96c0072d6..8d4741f821 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -273,6 +273,193 @@ impl Nexus { } } + /// Used as the body of a "stub" endpoint -- one that's currently + /// unimplemented but that we eventually intend to implement + /// + /// Even though an endpoint is unimplemented, it's useful if it implements + /// the correct authn/authz behaviors behaviors for unauthenticated and + /// authenticated, unauthorized requests. This allows us to maintain basic + /// authn/authz test coverage for stub endpoints, which in turn helps us + /// ensure that all endpoints are covered. + /// + /// In order to implement the correct authn/authz behavior, we need to know + /// a little about the endpoint. This is given by the `visibility` + /// argument. See the examples below. + /// + /// # Examples + /// + /// ## A top-level API endpoint (always visible) + /// + /// For example, "/my-new-kind-of-resource". The assumption is that the + /// _existence_ of this endpoint is not a secret. Use: + /// + /// ``` + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// + /// async fn my_things_list( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// ) -> Result<(), Error> + /// { + /// Err(nexus.unimplemented_todo(opctx, Unimpl::Public).await) + /// } + /// ``` + /// + /// ## An authz-protected resource under the top level + /// + /// For example, "/my-new-kind-of-resource/demo" (where "demo" is the name + /// of a specific resource of type "my-new-kind-of-resource"). Use: + /// + /// ``` + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// use omicron_common::api::external::LookupType; + /// use omicron_common::api::external::ResourceType; + /// + /// async fn my_thing_fetch( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// the_name: &Name, + /// ) -> Result<(), Error> + /// { + /// // You will want to have defined your own ResourceType variant for + /// // this resource, even though it's still a stub. + /// let resource_type: ResourceType = todo!(); + /// let lookup_type = LookupType::ByName(the_name.to_string()); + /// let not_found_error = lookup_type.into_not_found(resource_type); + /// let unimp = Unimpl::ProtectedLookup(not_found_error); + /// Err(nexus.unimplemented_todo(opctx, unimp).await) + /// } + /// ``` + /// + /// This does the bare minimum to produce an appropriate 404 "Not Found" + /// error for authenticated, unauthorized users. + /// + /// ## An authz-protected API endpoint under some other (non-stub) resource + /// + /// ### ... when the endpoint never returns 404 (e.g., "list", "create") + /// + /// For example, "/organizations/my-org/my-new-kind-of-resource". In this + /// case, your function should do whatever lookup of the non-stub resource + /// that the function will eventually do, and then treat it like the first + /// example. + /// + /// Here's an example stub for the "list" endpoint for a new resource + /// underneath Organizations: + /// + /// ``` + /// use omicron_nexus::authz; + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::lookup::LookupPath; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// + /// async fn organization_list_my_thing( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// organization_name: &Name, + /// ) -> Result<(), Error> + /// { + /// let (.., _authz_org) = LookupPath::new(opctx, datastore) + /// .organization_name(organization_name) + /// .lookup_for(authz::Action::ListChildren) + /// .await?; + /// Err(nexus.unimplemented_todo(opctx, Unimpl::Public).await) + /// } + /// ``` + /// + /// ### ... when the endpoint can return 404 (e.g., "get", "delete") + /// + /// You can treat this exactly like the second example above. Here's an + /// example stub for the "get" endpoint for that same resource: + /// + /// ``` + /// use omicron_nexus::authz; + /// use omicron_nexus::context::OpContext; + /// use omicron_nexus::db::lookup::LookupPath; + /// use omicron_nexus::db::model::Name; + /// use omicron_nexus::db::DataStore; + /// use omicron_nexus::nexus::Nexus; + /// use omicron_nexus::nexus::Unimpl; + /// use omicron_common::api::external::Error; + /// use omicron_common::api::external::LookupType; + /// use omicron_common::api::external::ResourceType; + /// + /// async fn my_thing_fetch( + /// nexus: &Nexus, + /// datastore: &DataStore, + /// opctx: &OpContext, + /// organization_name: &Name, + /// the_name: &Name, + /// ) -> Result<(), Error> + /// { + /// // You will want to have defined your own ResourceType variant for + /// // this resource, even though it's still a stub. + /// let resource_type: ResourceType = todo!(); + /// let lookup_type = LookupType::ByName(the_name.to_string()); + /// let not_found_error = lookup_type.into_not_found(resource_type); + /// let unimp = Unimpl::ProtectedLookup(not_found_error); + /// Err(nexus.unimplemented_todo(opctx, unimp).await) + /// } + /// ``` + pub async fn unimplemented_todo( + &self, + opctx: &OpContext, + visibility: Unimpl, + ) -> Error { + // Deny access to non-super-users. This is really just for the benefit + // of the authz coverage tests. By requiring (and testing) correct + // authz behavior for stubs, we ensure that that behavior is preserved + // when the stub's implementation is fleshed out. + match opctx.authorize(authz::Action::Modify, &authz::FLEET).await { + Err(error @ Error::Forbidden) => { + // Emulate the behavior of `Authz::authorize()`: if this is a + // non-public resource, then the user should get a 404, not a + // 403, when authorization fails. + if let Unimpl::ProtectedLookup(lookup_error) = visibility { + lookup_error + } else { + error + } + } + Err(error) => error, + Ok(_) => { + // In the event that a superuser actually gets this far, produce + // a server error. + // + // It's tempting to use other status codes here: + // + // "501 Not Implemented" is specifically when we don't recognize + // the HTTP method and cannot implement it on _any_ resource. + // + // "405 Method Not Allowed" is specifically when an HTTP method + // isn't supported. That doesn't feel quite right either -- + // this is usually interpreted to mean "not part of the API", + // which it obviously _is_, since the client found it in the API + // spec. + // + // Neither of these is true: this HTTP method on this HTTP + // resource is part of the API, and it will be supported by the + // server, but it doesn't work yet. + Error::internal_error("endpoint is not implemented") + } + } + } + // TODO-robustness we should have a limit on how many sled agents there can // be (for graceful degradation at large scale). pub async fn upsert_sled( @@ -844,116 +1031,158 @@ impl Nexus { Ok(()) } - pub async fn list_images( + pub async fn images_list( &self, - _opctx: &OpContext, + opctx: &OpContext, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } - pub async fn create_image( + pub async fn image_create( self: &Arc, - _opctx: &OpContext, + opctx: &OpContext, _params: ¶ms::ImageCreate, ) -> CreateResult { - unimplemented!(); + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn image_fetch( &self, - _opctx: &OpContext, - _image_name: &Name, + opctx: &OpContext, + image_name: &Name, ) -> LookupResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } - pub async fn delete_image( + pub async fn image_delete( self: &Arc, - _opctx: &OpContext, - _image_name: &Name, + opctx: &OpContext, + image_name: &Name, ) -> DeleteResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let error = lookup_type.into_not_found(ResourceType::Image); + Err(self + .unimplemented_todo(opctx, Unimpl::ProtectedLookup(error)) + .await) } pub async fn project_list_images( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_create_image( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _params: ¶ms::ImageCreate, ) -> CreateResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::CreateChild) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_image_fetch( &self, - _opctx: &OpContext, + opctx: &OpContext, _organization_name: &Name, _project_name: &Name, - _image_name: &Name, + image_name: &Name, ) -> LookupResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let not_found_error = lookup_type.into_not_found(ResourceType::Image); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_delete_image( self: &Arc, - _opctx: &OpContext, + opctx: &OpContext, _organization_name: &Name, _project_name: &Name, - _image_name: &Name, + image_name: &Name, ) -> DeleteResult { - unimplemented!(); + let lookup_type = LookupType::ByName(image_name.to_string()); + let not_found_error = lookup_type.into_not_found(ResourceType::Image); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_create_snapshot( self: &Arc, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _params: ¶ms::SnapshotCreate, ) -> CreateResult { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn project_list_snapshots( &self, - _opctx: &OpContext, - _organization_name: &Name, - _project_name: &Name, + opctx: &OpContext, + organization_name: &Name, + project_name: &Name, _pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - unimplemented!(); + let _ = LookupPath::new(opctx, &self.db_datastore) + .organization_name(organization_name) + .project_name(project_name) + .lookup_for(authz::Action::ListChildren) + .await?; + Err(self.unimplemented_todo(opctx, Unimpl::Public).await) } pub async fn snapshot_fetch( &self, - _opctx: &OpContext, + opctx: &OpContext, _organization_name: &Name, _project_name: &Name, - _snapshot_name: &Name, + snapshot_name: &Name, ) -> LookupResult { - unimplemented!(); + let lookup_type = LookupType::ByName(snapshot_name.to_string()); + let not_found_error = + lookup_type.into_not_found(ResourceType::Snapshot); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } pub async fn project_delete_snapshot( self: &Arc, - _opctx: &OpContext, + opctx: &OpContext, _organization_name: &Name, _project_name: &Name, - _snapshot_name: &Name, + snapshot_name: &Name, ) -> DeleteResult { - unimplemented!(); + let lookup_type = LookupType::ByName(snapshot_name.to_string()); + let not_found_error = + lookup_type.into_not_found(ResourceType::Snapshot); + let unimp = Unimpl::ProtectedLookup(not_found_error); + Err(self.unimplemented_todo(opctx, unimp).await) } // Instances @@ -3337,6 +3566,21 @@ impl Nexus { } } +/// For unimplemented endpoints, indicates whether the resource identified +/// by this endpoint will always be publicly visible or not +/// +/// For example, the resource "/images" is well-known (it's part of the +/// API). Being unauthorized to list images will result in a "403 +/// Forbidden". It's `UnimplResourceVisibility::Public'. +/// +/// By contrast, the resource "/images/some-image" is not publicly-known. +/// If you're not authorized to view it, you'll get a "404 Not Found". It's +/// `Unimpl::ProtectedLookup(LookupType::ByName("some-image"))`. +pub enum Unimpl { + Public, + ProtectedLookup(Error), +} + fn generate_session_token() -> String { // TODO: "If getrandom is unable to provide secure entropy this method will panic." // Should we explicitly handle that? diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 28314c98d0..b46f098dd1 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -53,8 +53,12 @@ lazy_static! { format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); pub static ref DEMO_PROJECT_URL_DISKS: String = format!("{}/disks", *DEMO_PROJECT_URL); + pub static ref DEMO_PROJECT_URL_IMAGES: String = + format!("{}/images", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_URL_INSTANCES: String = format!("{}/instances", *DEMO_PROJECT_URL); + pub static ref DEMO_PROJECT_URL_SNAPSHOTS: String = + format!("{}/snapshots", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_URL_VPCS: String = format!("{}/vpcs", *DEMO_PROJECT_URL); pub static ref DEMO_PROJECT_CREATE: params::ProjectCreate = @@ -194,6 +198,34 @@ lazy_static! { subnet_name: DEMO_VPC_SUBNET_NAME.clone(), ip: None, }; + + // Images + pub static ref DEMO_IMAGE_NAME: Name = "demo-image".parse().unwrap(); + pub static ref DEMO_IMAGE_URL: String = + format!("/images/{}", *DEMO_IMAGE_NAME); + pub static ref DEMO_PROJECT_IMAGE_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_IMAGES, *DEMO_IMAGE_NAME); + pub static ref DEMO_IMAGE_CREATE: params::ImageCreate = + params::ImageCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_IMAGE_NAME.clone(), + description: String::from(""), + }, + source: params::ImageSource::Url(String::from("dummy")) + }; + + // Snapshots + pub static ref DEMO_SNAPSHOT_NAME: Name = "demo-snapshot".parse().unwrap(); + pub static ref DEMO_SNAPSHOT_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_SNAPSHOTS, *DEMO_SNAPSHOT_NAME); + pub static ref DEMO_SNAPSHOT_CREATE: params::SnapshotCreate = + params::SnapshotCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_SNAPSHOT_NAME.clone(), + description: String::from(""), + }, + disk: DEMO_DISK_NAME.clone(), + }; } /// Describes an API endpoint to be verified by the "unauthorized" test @@ -256,7 +288,21 @@ pub enum AllowedMethod { /// be uncommon. In most cases, resources are identified either by names /// that we define here or uuids that we control in the test suite (e.g., /// the rack and sled uuids). + /// + /// This is not necessary for methods other than `GET`. We only need this + /// to configure the test's expectation for *privileged* requests. For the + /// other HTTP methods, we only make unprivileged requests, and they should + /// always fail in the correct way. GetNonexistent, + /// HTTP "GET" method that is not yet implemented + /// + /// This should be a transient state, used only for stub APIs. + /// + /// This is not necessary for methods other than `GET`. We only need this + /// to configure the test's expectation for *privileged* requests. For the + /// other HTTP methods, we only make unprivileged requests, and they should + /// always fail in the correct way. + GetUnimplemented, /// HTTP "POST" method, with sample input (which should be valid input for /// this endpoint) Post(serde_json::Value), @@ -272,6 +318,7 @@ impl AllowedMethod { AllowedMethod::Delete => &Method::DELETE, AllowedMethod::Get => &Method::GET, AllowedMethod::GetNonexistent => &Method::GET, + AllowedMethod::GetUnimplemented => &Method::GET, AllowedMethod::Post(_) => &Method::POST, AllowedMethod::Put(_) => &Method::PUT, } @@ -285,7 +332,8 @@ impl AllowedMethod { match self { AllowedMethod::Delete | AllowedMethod::Get - | AllowedMethod::GetNonexistent => None, + | AllowedMethod::GetNonexistent + | AllowedMethod::GetUnimplemented => None, AllowedMethod::Post(body) => Some(&body), AllowedMethod::Put(body) => Some(&body), } @@ -564,6 +612,49 @@ lazy_static! { ], }, + /* Project images */ + + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_IMAGES, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IMAGE_CREATE).unwrap() + ), + ], + }, + + VerifyEndpoint { + url: &*DEMO_PROJECT_IMAGE_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ], + }, + + /* Snapshots */ + + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_SNAPSHOTS, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(DEMO_SNAPSHOT_CREATE.clone()).unwrap(), + ) + ] + }, + VerifyEndpoint { + url: &*DEMO_SNAPSHOT_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ] + }, + /* Instances */ VerifyEndpoint { url: &*DEMO_PROJECT_URL_INSTANCES, @@ -719,5 +810,26 @@ lazy_static! { serde_json::Value::Null )], }, + + /* Images */ + + VerifyEndpoint { + url: "/images", + visibility: Visibility::Public, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_IMAGE_CREATE).unwrap() + ), + ], + }, + VerifyEndpoint { + url: &*DEMO_IMAGE_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::GetUnimplemented, + AllowedMethod::Delete, + ], + }, ]; } diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 3fde028fea..29bce430a3 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -77,13 +77,11 @@ SUMMARY OF REQUESTS MADE KEY, USING HEADER AND EXAMPLE ROW: - +----------------------------> privileged GET (expects 200) - | (digit = last digit of 200-level - | response) + +----------------------------> privileged GET (expects 200 or 500) + | (digit = last digit of status code) | | +-> privileged GET (expects same as above) - | | (digit = last digit of 200-level - | | response) + | | (digit = last digit of status code) | | ('-' => skipped (N/A)) ^ ^ HEADER: G GET PUT POST DEL TRCE G URL @@ -242,26 +240,47 @@ async fn verify_endpoint( // response. Otherwise, the test might later succeed by coincidence. We // might find a 404 because of something that actually doesn't exist rather // than something that's just hidden from unauthorized users. - let get_allowed = endpoint - .allowed_methods - .iter() - .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) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(), - ) - } else { - warn!(log, "test: skipping privileged GET (method not allowed)"); - record_operation(WhichTest::PrivilegedGet(None)); - None + let get_allowed = endpoint.allowed_methods.iter().find(|allowed| { + matches!(allowed, AllowedMethod::Get | AllowedMethod::GetUnimplemented) + }); + let resource_before = match get_allowed { + Some(AllowedMethod::Get) => { + info!(log, "test: privileged GET"); + record_operation(WhichTest::PrivilegedGet(Some( + &http::StatusCode::OK, + ))); + Some( + NexusRequest::object_get(client, endpoint.url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(), + ) + } + Some(AllowedMethod::GetUnimplemented) => { + info!(log, "test: privileged GET (unimplemented)"); + let expected_status = http::StatusCode::INTERNAL_SERVER_ERROR; + record_operation(WhichTest::PrivilegedGet(Some(&expected_status))); + NexusRequest::expect_failure( + client, + expected_status, + http::Method::GET, + endpoint.url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + None + } + Some(_) => unimplemented!(), + None => { + warn!(log, "test: skipping privileged GET (method not allowed)"); + record_operation(WhichTest::PrivilegedGet(None)); + None + } }; print!(" "); diff --git a/nexus/tests/integration_tests/unauthorized_coverage.rs b/nexus/tests/integration_tests/unauthorized_coverage.rs index b76727d744..e92a5af98b 100644 --- a/nexus/tests/integration_tests/unauthorized_coverage.rs +++ b/nexus/tests/integration_tests/unauthorized_coverage.rs @@ -126,11 +126,17 @@ fn test_unauthorized_coverage() { // If you're here because this assertion failed, check that if you've added // any API operations to Nexus, you've also added a corresponding test in // "unauthorized.rs" so that it will automatically be checked for its - // behavior for unauthenticated and unauthorized users. - assert_contents( - "tests/output/uncovered-authz-endpoints.txt", - &uncovered_endpoints, - ); + // behavior for unauthenticated and unauthorized users. DO NOT SKIP THIS. + // Even if you're just adding a stub, see [`Nexus::unimplemented_todo()`]. + // If you _added_ a test that covered an endpoint from the allowlist -- + // hooray! Just delete the corresponding line from this file. (Why is this + // not `expectorage::assert_contents`? Because we only expect this file to + // ever shrink, which is easy enough to fix by hand, and we don't want to + // make it easy to accidentally add things to the allowlist.) + let expected_uncovered_endpoints = + std::fs::read_to_string("tests/output/uncovered-authz-endpoints.txt") + .expect("failed to load file of allowed uncovered endpoints"); + assert_eq!(expected_uncovered_endpoints, uncovered_endpoints); } #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index ecb7a4d63d..37134686de 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,20 +1,8 @@ API endpoints with no coverage in authz tests: -images_delete_image (delete "/images/{image_name}") -project_images_delete_image (delete "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") -project_snapshots_delete_snapshot (delete "/organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name}") silos_delete_silo (delete "/silos/{silo_name}") -images_get (get "/images") -images_get_image (get "/images/{image_name}") -project_images_get (get "/organizations/{organization_name}/projects/{project_name}/images") -project_images_get_image (get "/organizations/{organization_name}/projects/{project_name}/images/{image_name}") -project_snapshots_get (get "/organizations/{organization_name}/projects/{project_name}/snapshots") -project_snapshots_get_snapshot (get "/organizations/{organization_name}/projects/{project_name}/snapshots/{snapshot_name}") session_me (get "/session/me") silos_get (get "/silos") silos_get_silo (get "/silos/{silo_name}") -images_post (post "/images") spoof_login (post "/login") logout (post "/logout") -project_images_post (post "/organizations/{organization_name}/projects/{project_name}/images") -project_snapshots_post (post "/organizations/{organization_name}/projects/{project_name}/snapshots") silos_post (post "/silos")