diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 41a2cb152e..38c6b43d14 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -806,26 +806,23 @@ impl DataStore { */ pub async fn project_delete( &self, - organization_id: &Uuid, - name: &Name, + opctx: &OpContext, + authz_project: &authz::Project, ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_project).await?; + use db::schema::project::dsl; let now = Utc::now(); diesel::update(dsl::project) .filter(dsl::time_deleted.is_null()) - .filter(dsl::organization_id.eq(*organization_id)) - .filter(dsl::name.eq(name.clone())) + .filter(dsl::id.eq(authz_project.id())) .set(dsl::time_deleted.eq(now)) .returning(Project::as_returning()) - .get_result_async(self.pool()) + .get_result_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { - public_error_from_diesel_pool_lookup( - e, - ResourceType::Project, - LookupType::ByName(name.as_str().to_owned()), - ) + public_error_from_diesel_pool_authz(e, authz_project) })?; Ok(()) } @@ -868,30 +865,24 @@ impl DataStore { .map_err(public_error_from_diesel_pool_shouldnt_fail) } - /// Updates a project by name (clobbering update -- no etag) + /// Updates a project (clobbering update -- no etag) pub async fn project_update( &self, - organization_id: &Uuid, - name: &Name, + opctx: &OpContext, + authz_project: &authz::Project, updates: ProjectUpdate, ) -> UpdateResult { - use db::schema::project::dsl; + opctx.authorize(authz::Action::Modify, authz_project).await?; + use db::schema::project::dsl; diesel::update(dsl::project) .filter(dsl::time_deleted.is_null()) - .filter(dsl::organization_id.eq(*organization_id)) - .filter(dsl::name.eq(name.clone())) + .filter(dsl::id.eq(authz_project.id())) .set(updates) .returning(Project::as_returning()) - .get_result_async(self.pool()) + .get_result_async(self.pool_authorized(opctx).await?) .await - .map_err(|e| { - public_error_from_diesel_pool_lookup( - e, - ResourceType::Project, - LookupType::ByName(name.as_str().to_owned()), - ) - }) + .map_err(|e| public_error_from_diesel_pool_authz(e, authz_project)) } /* diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 40b9594d0f..46c5915cfe 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -506,7 +506,8 @@ async fn organization_projects_delete_project( let organization_name = ¶ms.organization_name; let project_name = ¶ms.project_name; let handler = async { - nexus.project_delete(&organization_name, &project_name).await?; + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus.project_delete(&opctx, &organization_name, &project_name).await?; Ok(HttpResponseDeleted()) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -538,8 +539,10 @@ async fn organization_projects_put_project( let organization_name = &path.organization_name; let project_name = &path.project_name; let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let newproject = nexus .project_update( + &opctx, &organization_name, &project_name, &updated_project.into_inner(), diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 59cd7d1b60..ff35230165 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -632,34 +632,30 @@ impl Nexus { pub async fn project_delete( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, ) -> DeleteResult { - let organization_id = self + let authz_project = self .db_datastore - .organization_lookup_path(organization_name) - .await? - .id(); - self.db_datastore.project_delete(&organization_id, project_name).await + .project_lookup_path(organization_name, project_name) + .await?; + self.db_datastore.project_delete(opctx, &authz_project).await } pub async fn project_update( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, new_params: ¶ms::ProjectUpdate, ) -> UpdateResult { - let organization_id = self + let authz_project = self .db_datastore - .organization_lookup_path(organization_name) - .await? - .id(); + .project_lookup_path(organization_name, project_name) + .await?; self.db_datastore - .project_update( - &organization_id, - project_name, - new_params.clone().into(), - ) + .project_update(opctx, &authz_project, new_params.clone().into()) .await } diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 9a78153916..6bcd66ebfe 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -479,6 +479,22 @@ impl<'a> NexusRequest<'a> { ) } + /// Returns a new `NexusRequest` suitable for `PUT $uri` + pub fn object_put( + testctx: &'a ClientTestContext, + uri: &str, + body: Option<&B>, + ) -> Self + where + B: serde::Serialize, + { + NexusRequest::new( + RequestBuilder::new(testctx, http::Method::PUT, uri) + .body(body) + .expect_status(Some(http::StatusCode::OK)), + ) + } + /// Convenience constructor for failure cases pub fn expect_failure( testctx: &'a ClientTestContext, diff --git a/nexus/tests/integration_tests/basic.rs b/nexus/tests/integration_tests/basic.rs index b8b7de0995..55648c53f5 100644 --- a/nexus/tests/integration_tests/basic.rs +++ b/nexus/tests/integration_tests/basic.rs @@ -10,7 +10,6 @@ */ use dropshot::test_util::objects_list_page; -use dropshot::test_util::read_json; use dropshot::test_util::ClientTestContext; use dropshot::HttpErrorResponseBody; use http::method::Method; @@ -312,46 +311,49 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { * Delete "simproject2". We'll make sure that's reflected in the other * requests. */ - client - .make_request_no_body( - Method::DELETE, - "/organizations/test-org/projects/simproject2", - StatusCode::NO_CONTENT, - ) - .await - .expect("expected success"); + NexusRequest::object_delete( + client, + "/organizations/test-org/projects/simproject2", + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("expected request to fail"); /* * Having deleted "simproject2", verify "GET", "PUT", and "DELETE" on * "/organizations/test-org/projects/simproject2". */ - client - .make_request_error( - Method::GET, - "/organizations/test-org/projects/simproject2", + for method in [Method::GET, Method::DELETE] { + NexusRequest::expect_failure( + client, StatusCode::NOT_FOUND, - ) - .await; - client - .make_request_error( - Method::DELETE, + method, "/organizations/test-org/projects/simproject2", - StatusCode::NOT_FOUND, ) - .await; - client - .make_request_error_body( + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request"); + } + NexusRequest::new( + RequestBuilder::new( + client, Method::PUT, "/organizations/test-org/projects/simproject2", - params::ProjectUpdate { - identity: IdentityMetadataUpdateParams { - name: None, - description: None, - }, - }, - StatusCode::NOT_FOUND, ) - .await; + .body(Some(¶ms::ProjectUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + })) + .expect_status(Some(StatusCode::NOT_FOUND)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request"); /* * Similarly, verify "GET /organizations/test-org/projects" @@ -383,25 +385,61 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { ); /* - * Update "simproject3". We'll make sure that's reflected in the other - * requests. + * Unprivileged users should not be able to update a Project. */ let project_update = params::ProjectUpdate { identity: IdentityMetadataUpdateParams { name: None, - description: Some("Li'l lightnin'".to_string()), + description: None, }, }; - let mut response = client - .make_request( + NexusRequest::new( + RequestBuilder::new( + client, Method::PUT, "/organizations/test-org/projects/simproject3", - Some(project_update), - StatusCode::OK, ) - .await - .expect("expected success"); - let project: Project = read_json(&mut response).await; + .body(Some(&project_update)) + .expect_status(Some(StatusCode::NOT_FOUND)), + ) + .execute() + .await + .expect("failed to make request"); + NexusRequest::new( + RequestBuilder::new( + client, + Method::PUT, + "/organizations/test-org/projects/simproject3", + ) + .body(Some(&project_update)) + .expect_status(Some(StatusCode::NOT_FOUND)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("failed to make request"); + + /* + * Update "simproject3". We'll make sure that's reflected in the other + * requests. + */ + let project_update = params::ProjectUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: Some("Li'l lightnin'".to_string()), + }, + }; + let project = NexusRequest::object_put( + client, + "/organizations/test-org/projects/simproject3", + Some(&project_update), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("expected success") + .parsed_body::() + .expect("failed to parse Project from PUT response"); assert_eq!(project.identity.id, new_project_ids[2]); assert_eq!(project.identity.name, "simproject3"); assert_eq!(project.identity.description, "Li'l lightnin'"); @@ -425,27 +463,31 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { description: Some("little lightning".to_string()), }, }; - let mut response = client - .make_request( - Method::PUT, - "/organizations/test-org/projects/simproject3", - Some(project_update), - StatusCode::OK, - ) - .await - .expect("failed to make request to server"); - let project: Project = read_json(&mut response).await; + let project = NexusRequest::object_put( + client, + "/organizations/test-org/projects/simproject3", + Some(&project_update), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("expected success") + .parsed_body::() + .expect("failed to parse Project from PUT response"); assert_eq!(project.identity.id, new_project_ids[2]); assert_eq!(project.identity.name, "lil-lightnin"); assert_eq!(project.identity.description, "little lightning"); - client - .make_request_error( - Method::GET, - "/organizations/test-org/projects/simproject3", - StatusCode::NOT_FOUND, - ) - .await; + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + "/organizations/test-org/projects/simproject3", + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("expected success"); /* * Try to create a project with a name that conflicts with an existing one. diff --git a/nexus/tests/integration_tests/organizations.rs b/nexus/tests/integration_tests/organizations.rs index 3cb26103aa..aab6f3187b 100644 --- a/nexus/tests/integration_tests/organizations.rs +++ b/nexus/tests/integration_tests/organizations.rs @@ -5,8 +5,6 @@ use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; use omicron_nexus::external_api::views::Organization; -use dropshot::test_util::object_delete; - use http::method::Method; use http::StatusCode; use nexus_test_utils::resource_helpers::{ @@ -168,7 +166,11 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { .expect("failed to make request"); // Delete the project, then delete the organization - object_delete(&client, &project_url).await; + NexusRequest::object_delete(&client, &project_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to make request"); NexusRequest::object_delete(&client, &o2_url) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 8a2392e578..fc71598039 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -4,7 +4,10 @@ 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::resource_helpers::project_get; +use omicron_common::api::external::IdentityMetadataUpdateParams; +use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::Project; use nexus_test_utils::resource_helpers::{create_organization, create_project}; @@ -94,4 +97,54 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { .all_items; assert_eq!(projects.len(), 1); assert_eq!(projects[0].identity.name, p1_name); + + // Unauthenticated and unauthorized users should not be able to delete or + // modify a Project. + NexusRequest::expect_failure( + &client, + http::StatusCode::NOT_FOUND, + http::Method::DELETE, + &p1_url, + ) + .execute() + .await + .expect("failed to make request"); + NexusRequest::expect_failure( + &client, + http::StatusCode::NOT_FOUND, + http::Method::DELETE, + &p1_url, + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("failed to make request"); + + NexusRequest::new( + RequestBuilder::new(&client, http::Method::PUT, &p1_url) + .body(Some(¶ms::ProjectUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + })) + .expect_status(Some(http::StatusCode::NOT_FOUND)), + ) + .execute() + .await + .expect("failed to make request"); + NexusRequest::new( + RequestBuilder::new(&client, http::Method::PUT, &p1_url) + .body(Some(¶ms::ProjectUpdate { + identity: IdentityMetadataUpdateParams { + name: None, + description: None, + }, + })) + .expect_status(Some(http::StatusCode::NOT_FOUND)), + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("failed to make request"); }