diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 58896c688bc..bae4e4bfcae 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -1197,16 +1197,18 @@ impl DataStore { pub async fn project_list_disks( &self, - project_id: &Uuid, + opctx: &OpContext, + authz_project: &authz::Project, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - use db::schema::disk::dsl; + opctx.authorize(authz::Action::ListChildren, authz_project).await?; + use db::schema::disk::dsl; paginated(dsl::disk, dsl::name, &pagparams) .filter(dsl::time_deleted.is_null()) - .filter(dsl::project_id.eq(*project_id)) + .filter(dsl::project_id.eq(authz_project.id())) .select(Disk::as_select()) - .load_async::(self.pool()) + .load_async::(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 46c5915cfea..54ac30f3733 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -577,8 +577,10 @@ async fn project_disks_get( let organization_name = &path.organization_name; let project_name = &path.project_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disks = nexus .project_list_disks( + &opctx, organization_name, project_name, &data_page_params_for(&rqctx, &query)? @@ -616,8 +618,10 @@ async fn project_disks_post( let project_name = &path.project_name; let new_disk_params = &new_disk.into_inner(); let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let disk = nexus .project_create_disk( + &opctx, &organization_name, &project_name, &new_disk_params, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 0de77a45af8..bbb739d06f6 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -665,20 +665,23 @@ impl Nexus { pub async fn project_list_disks( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let project_id = self + let authz_project = self .db_datastore .project_lookup_path(organization_name, project_name) - .await? - .id(); - self.db_datastore.project_list_disks(&project_id, pagparams).await + .await?; + self.db_datastore + .project_list_disks(opctx, &authz_project, pagparams) + .await } pub async fn project_create_disk( self: &Arc, + opctx: &OpContext, organization_name: &Name, project_name: &Name, params: ¶ms::DiskCreate, @@ -688,6 +691,11 @@ impl Nexus { .project_lookup_path(organization_name, project_name) .await?; + // TODO-security This may need to be revisited once we implement authz + // checks for saga actions. Even then, though, it still will be correct + // (if possibly redundant) to check this here. + opctx.authorize(authz::Action::CreateChild, &authz_project).await?; + /* * Until we implement snapshots, do not allow disks to be created with a * snapshot id. diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 6bcd66ebfed..8652f7c86cf 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -514,15 +514,17 @@ impl<'a> NexusRequest<'a> { testctx: &'a ClientTestContext, collection_url: &str, initial_params: &str, - limit: usize, + limit: Option, ) -> Result, anyhow::Error> where T: Clone + serde::de::DeserializeOwned, { - let url_base = format!("{}?limit={}&", collection_url, limit); let mut npages = 0; let mut all_items = Vec::new(); let mut next_token: Option = None; + const DEFAULT_PAGE_SIZE: usize = 10; + let limit = limit.unwrap_or(DEFAULT_PAGE_SIZE); + let url_base = format!("{}?limit={}&", collection_url, limit); loop { let url = if let Some(next_token) = &next_token { diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 9cf091b31f4..8f1ba7df714 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -9,6 +9,8 @@ use dropshot::test_util::ClientTestContext; use dropshot::HttpErrorResponseBody; use dropshot::Method; use http::StatusCode; +use omicron_common::api::external::ByteCount; +use omicron_common::api::external::Disk; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::VpcRouter; use omicron_nexus::external_api::params; @@ -30,33 +32,50 @@ where .unwrap() } -pub async fn create_organization( +pub async fn object_create( client: &ClientTestContext, - organization_name: &str, -) -> Organization { - let input = params::OrganizationCreate { - identity: IdentityMetadataCreateParams { - name: organization_name.parse().unwrap(), - description: "an org".to_string(), - }, - }; - NexusRequest::objects_post(client, "/organizations", &input) + path: &str, + input: &InputType, +) -> OutputType +where + InputType: serde::Serialize, + OutputType: serde::de::DeserializeOwned, +{ + NexusRequest::objects_post(client, path, input) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("failed to make request") + .expect("failed to make \"create\" request") .parsed_body() .unwrap() } +pub async fn create_organization( + client: &ClientTestContext, + organization_name: &str, +) -> Organization { + object_create( + client, + "/organizations", + ¶ms::OrganizationCreate { + identity: IdentityMetadataCreateParams { + name: organization_name.parse().unwrap(), + description: "an org".to_string(), + }, + }, + ) + .await +} + pub async fn create_project( client: &ClientTestContext, organization_name: &str, project_name: &str, ) -> Project { - NexusRequest::objects_post( + let url = format!("/organizations/{}/projects", &organization_name); + object_create( client, - &format!("/organizations/{}/projects", &organization_name), + &url, ¶ms::ProjectCreate { identity: IdentityMetadataCreateParams { name: project_name.parse().unwrap(), @@ -64,12 +83,32 @@ pub async fn create_project( }, }, ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() .await - .expect("failed to make request") - .parsed_body() - .unwrap() +} + +pub async fn create_disk( + client: &ClientTestContext, + organization_name: &str, + project_name: &str, + disk_name: &str, +) -> Disk { + let url = format!( + "/organizations/{}/projects/{}/disks", + organization_name, project_name + ); + object_create( + client, + &url, + ¶ms::DiskCreate { + identity: IdentityMetadataCreateParams { + name: disk_name.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + snapshot_id: None, + size: ByteCount::from_gibibytes_u32(1), + }, + ) + .await } pub async fn create_vpc( diff --git a/nexus/tests/integration_tests/basic.rs b/nexus/tests/integration_tests/basic.rs index 52ffb4edf68..6047a667889 100644 --- a/nexus/tests/integration_tests/basic.rs +++ b/nexus/tests/integration_tests/basic.rs @@ -549,7 +549,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -572,7 +572,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=name-ascending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -595,7 +595,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=name-descending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -617,7 +617,7 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) { &client, projects_url, "sort_by=id-ascending", - projects_subset, + Some(projects_subset), ) .await .expect("failed to list projects") @@ -671,7 +671,7 @@ async fn projects_list( client: &ClientTestContext, projects_url: &str, ) -> Vec { - NexusRequest::iter_collection_authn(client, projects_url, "", 10) + NexusRequest::iter_collection_authn(client, projects_url, "", None) .await .expect("failed to list projects") .all_items diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 05935ac1953..d7af2dd525d 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -5,6 +5,7 @@ //! Tests basic disk support in the API use crucible_agent_client::types::State as RegionState; +use dropshot::HttpErrorResponseBody; use http::method::Method; use http::StatusCode; use omicron_common::api::external::ByteCount; @@ -30,6 +31,7 @@ use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::identity_eq; +use nexus_test_utils::resource_helpers::create_disk; use nexus_test_utils::resource_helpers::create_organization; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils::ControlPlaneTestContext; @@ -159,15 +161,7 @@ async fn test_disk_create_attach_detach_delete( // Create a disk. let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let disk: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let disk = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; assert_eq!(disk.identity.name, DISK_NAME); assert_eq!(disk.identity.description, "sells rainsticks"); assert_eq!(disk.project_id, test.project_id); @@ -324,19 +318,22 @@ async fn test_disk_create_disk_that_already_exists_fails( snapshot_id: None, size: ByteCount::from_gibibytes_u32(1), }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); let disk = disk_get(&client, &disk_url).await; // Attempt to create a second disk with a conflicting name. - let error = client - .make_request_error_body( - Method::POST, - &disks_url, - new_disk, - StatusCode::BAD_REQUEST, - ) - .await; + let error: HttpErrorResponseBody = NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); assert_eq!( error.message, format!("already exists: disk \"{}\"", DISK_NAME) @@ -357,15 +354,7 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { // Create a disk. let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let disk: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let disk = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; // Create an instance to attach the disk. let url_instances = get_instances_url(); @@ -676,76 +665,12 @@ async fn test_disk_move_between_instances(cptestctx: &ControlPlaneTestContext) { ); } -#[nexus_test] -async fn test_disk_deletion_requires_authentication( - cptestctx: &ControlPlaneTestContext, -) { - let client = &cptestctx.external_client; - DiskTest::new(&cptestctx).await; - let disks_url = get_disks_url(); - - // Create a disk. - let disk_url = format!("{}/{}", disks_url, DISK_NAME); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; - - const BAD_DISK_NAME: &str = "wonderful-knife"; - let bad_disk_url = format!("{}/{}", disks_url, BAD_DISK_NAME); - - // If we are not authenticated, we should not be able to delete the disk. - // - // We should see the same error regardless of the existence of the disk. - let urls = [&disk_url, &bad_disk_url]; - for url in &urls { - NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::DELETE, - &url, - ) - .execute() - .await - .expect("expected request to fail"); - } - - // If we are unprivileged, we should not be able to delete the disk. - // - // We should see the same error regardless of the existence of the disk. - for url in &urls { - NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::DELETE, - &url, - ) - .authn_as(AuthnMode::UnprivilegedUser) - .execute() - .await - .expect("expected request to fail"); - } - - // Privileged users can delete disks. - NexusRequest::object_delete(client, &disk_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to delete disk"); -} - #[nexus_test] async fn test_disk_creation_region_requested_then_started( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; let test = DiskTest::new(&cptestctx).await; - let disks_url = get_disks_url(); // Before we create a disk, set the response from the Crucible Agent: // no matter what regions get requested, they'll always *start* as @@ -768,15 +693,7 @@ async fn test_disk_creation_region_requested_then_started( // The disk is created successfully, even when this "requested" -> "started" // transition occurs. - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - size: ByteCount::from_gibibytes_u32(1), - }; - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } // Tests that region allocation failure causes disk allocation to fail. @@ -822,14 +739,15 @@ async fn test_disk_region_creation_failure( // // TODO: Maybe consider making this a more informative error? // How should we propagate this to the client? - client - .make_request_error_body( - Method::POST, - &disks_url, - new_disk.clone(), - StatusCode::INTERNAL_SERVER_ERROR, - ) - .await; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::INTERNAL_SERVER_ERROR)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); // After the failed allocation, the disk should not exist. let disks = disks_list(&client, &disks_url).await; @@ -852,7 +770,7 @@ async fn test_disk_region_creation_failure( test.sled_agent.get_crucible_dataset(test.zpool_id, *id).await; crucible.set_create_callback(Box::new(|_| RegionState::Created)).await; } - let _: Disk = objects_post(&client, &disks_url, new_disk.clone()).await; + let _ = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { @@ -860,7 +778,10 @@ async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { } async fn disks_list(client: &ClientTestContext, list_url: &str) -> Vec { - objects_list_page::(client, list_url).await.items + NexusRequest::iter_collection_authn(client, list_url, "", None) + .await + .expect("failed to list disks") + .all_items } fn disks_eq(disk1: &Disk, disk2: &Disk) { diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 3a463ffbb3c..60ffe559622 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -39,7 +39,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { &client, &projects_url, "", - 10, + None, ) .await .expect("failed to list projects") @@ -62,7 +62,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { &client, &format!("/organizations/{}/projects", org2_name), "", - 10, + None, ) .await .expect("failed to list projects") diff --git a/nexus/tests/integration_tests/roles_builtin.rs b/nexus/tests/integration_tests/roles_builtin.rs index 084f447eb7e..9faf8b1bdab 100644 --- a/nexus/tests/integration_tests/roles_builtin.rs +++ b/nexus/tests/integration_tests/roles_builtin.rs @@ -46,7 +46,7 @@ async fn test_roles_builtin(cptestctx: &ControlPlaneTestContext) { // This endpoint uses a custom pagination scheme that is easy to get wrong. // Let's test that all markers do work. let roles_paginated = - NexusRequest::iter_collection_authn(&testctx, "/roles", "", 1) + NexusRequest::iter_collection_authn(&testctx, "/roles", "", Some(1)) .await .expect("failed to iterate all roles"); assert_eq!(roles, roles_paginated.all_items); diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index bba9645a836..7abbdbea957 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -16,6 +16,7 @@ use nexus_test_utils::http_testing::RequestBuilder; use nexus_test_utils::http_testing::TestResponse; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Name; @@ -122,6 +123,8 @@ lazy_static! { static ref DEMO_PROJECT_NAME: Name = "demo-project".parse().unwrap(); static ref DEMO_PROJECT_URL: String = format!("{}/{}", *DEMO_ORG_PROJECTS_URL, *DEMO_PROJECT_NAME); + static ref DEMO_PROJECT_URL_DISKS: String = + format!("{}/disks", *DEMO_PROJECT_URL); static ref DEMO_PROJECT_CREATE: params::ProjectCreate = params::ProjectCreate { identity: IdentityMetadataCreateParams { @@ -129,6 +132,20 @@ lazy_static! { description: "".parse().unwrap(), }, }; + + // Disk used for testing + static ref DEMO_DISK_NAME: Name = "demo-disk".parse().unwrap(); + static ref DEMO_DISK_URL: String = + format!("{}/{}", *DEMO_PROJECT_URL_DISKS, *DEMO_DISK_NAME); + static ref DEMO_DISK_CREATE: params::DiskCreate = + params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DEMO_PROJECT_NAME.clone(), + description: "".parse().unwrap(), + }, + snapshot_id: None, + size: ByteCount::from_gibibytes_u32(16), + }; } // @@ -287,6 +304,20 @@ lazy_static! { ], }, + VerifyEndpoint { + url: &*DEMO_PROJECT_URL_DISKS, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_DISK_CREATE).unwrap() + ), + ], + }, + + // TODO-coverage The single disk endpoint belongs here, but we've only + // implemented authz for DELETE, not GET yet. + VerifyEndpoint { url: "/roles", visibility: Visibility::Public, diff --git a/tools/oxapi_demo b/tools/oxapi_demo index fdd1395ff5a..1851f0683c2 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -249,7 +249,7 @@ function cmd_project_list_instances function cmd_project_list_disks { [[ $# != 2 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME" - do_curl "/organizations/$1/projects/$2/disks" + do_curl_authn "/organizations/$1/projects/$2/disks" } function cmd_project_list_vpcs @@ -328,7 +328,7 @@ function cmd_disk_create_demo { [[ $# != 3 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME DISK_NAME" mkjson name="$3" description="a disk called $3" size=1024 | - do_curl "/organizations/$1/projects/$2/disks" -X POST -T - + do_curl_authn "/organizations/$1/projects/$2/disks" -X POST -T - } function cmd_disk_get