diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index bc4f21b01c5..cbd5ede2be6 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -331,6 +331,11 @@ pub struct Project { } impl Project { + // TODO-cleanup see note on Organization::id above. + pub fn id(&self) -> Uuid { + self.project_id + } + /// Returns an authz resource representing any child of this Project (e.g., /// an Instance or Disk) /// diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index b289a1d0d91..479713a7aa8 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -325,7 +325,7 @@ impl DataStore { /// /// This function does no authz checks because it is not possible to know /// just by looking up an Organization's id what privileges are required. - pub async fn organization_lookup_id( + pub async fn organization_lookup_path( &self, name: &Name, ) -> LookupResult { @@ -340,8 +340,6 @@ impl DataStore { ) -> LookupResult<(authz::Organization, Organization)> { let (authz_org, db_org) = self.organization_lookup_noauthz(name).await?; - // TODO-security See the note in authz::authorize(). This needs to - // return a 404, not a 403. opctx.authorize(authz::Action::Read, &authz_org).await?; Ok((authz_org, db_org)) } @@ -475,7 +473,7 @@ impl DataStore { ) -> UpdateResult { use db::schema::organization::dsl; - let authz_org = self.organization_lookup_id(name).await?; + let authz_org = self.organization_lookup_path(name).await?; opctx.authorize(authz::Action::Modify, &authz_org).await?; diesel::update(dsl::organization) @@ -528,17 +526,22 @@ impl DataStore { }) } - /// Lookup a project by name. - pub async fn project_fetch( + /// Fetches a Project from the database and returns both the database row + /// and an authz::Project for doing authz checks + /// + /// See [`DataStore::organization_lookup_noauthz()`] for intended use cases + /// and caveats. + // TODO-security See the note on organization_lookup_noauthz(). + async fn project_lookup_noauthz( &self, - organization_id: &Uuid, - name: &Name, - ) -> LookupResult { + authz_org: &authz::Organization, + project_name: &Name, + ) -> LookupResult<(authz::Project, Project)> { use db::schema::project::dsl; dsl::project .filter(dsl::time_deleted.is_null()) - .filter(dsl::organization_id.eq(*organization_id)) - .filter(dsl::name.eq(name.clone())) + .filter(dsl::organization_id.eq(authz_org.id())) + .filter(dsl::name.eq(project_name.clone())) .select(Project::as_select()) .first_async(self.pool()) .await @@ -546,11 +549,49 @@ impl DataStore { public_error_from_diesel_pool( e, ResourceType::Project, - LookupType::ByName(name.as_str().to_owned()), + LookupType::ByName(project_name.as_str().to_owned()), + ) + }) + .map(|p| { + ( + authz_org + .project(p.id(), LookupType::from(&project_name.0)), + p, ) }) } + /// Look up the id for a Project based on its name + /// + /// Returns an [`authz::Project`] (which makes the id available). + /// + /// This function does no authz checks because it is not possible to know + /// just by looking up an Project's id what privileges are required. + pub async fn project_lookup_path( + &self, + organization_name: &Name, + project_name: &Name, + ) -> LookupResult { + let authz_org = + self.organization_lookup_path(organization_name).await?; + self.project_lookup_noauthz(&authz_org, project_name) + .await + .map(|(p, _)| p) + } + + /// Lookup a project by name. + pub async fn project_fetch( + &self, + opctx: &OpContext, + authz_org: &authz::Organization, + name: &Name, + ) -> LookupResult<(authz::Project, Project)> { + let (authz_org, db_org) = + self.project_lookup_noauthz(authz_org, name).await?; + opctx.authorize(authz::Action::Read, &authz_org).await?; + Ok((authz_org, db_org)) + } + /// Delete a project /* * TODO-correctness This needs to check whether there are any resources that @@ -582,29 +623,6 @@ impl DataStore { Ok(()) } - /// Look up the id for a project based on its name - pub async fn project_lookup_id_by_name( - &self, - organization_id: &Uuid, - name: &Name, - ) -> Result { - use db::schema::project::dsl; - dsl::project - .filter(dsl::time_deleted.is_null()) - .filter(dsl::organization_id.eq(*organization_id)) - .filter(dsl::name.eq(name.clone())) - .select(dsl::id) - .get_result_async::(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ResourceType::Project, - LookupType::ByName(name.as_str().to_owned()), - ) - }) - } - pub async fn projects_list_by_id( &self, opctx: &OpContext, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index fa9c7c3096b..b7bd371fe54 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -477,8 +477,10 @@ async fn organization_projects_get_project( let organization_name = &path.organization_name; let project_name = &path.project_name; let handler = async { - let project = - nexus.project_fetch(&organization_name, &project_name).await?; + let opctx = OpContext::for_external_api(&rqctx).await?; + let project = nexus + .project_fetch(&opctx, &organization_name, &project_name) + .await?; Ok(HttpResponseOk(project.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index f762a55a9e0..440495a6547 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -541,8 +541,10 @@ impl Nexus { organization_name: &Name, new_project: ¶ms::ProjectCreate, ) -> CreateResult { - let org = - self.db_datastore.organization_lookup_id(organization_name).await?; + let org = self + .db_datastore + .organization_lookup_path(organization_name) + .await?; // Create a project. let db_project = db::model::Project::new(org.id(), new_project.clone()); @@ -577,15 +579,19 @@ impl Nexus { pub async fn project_fetch( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, ) -> LookupResult { - let organization_id = self + let authz_org = self + .db_datastore + .organization_lookup_path(organization_name) + .await?; + Ok(self .db_datastore - .organization_lookup_id(organization_name) + .project_fetch(opctx, &authz_org, project_name) .await? - .id(); - self.db_datastore.project_fetch(&organization_id, project_name).await + .1) } pub async fn projects_list_by_name( @@ -594,8 +600,10 @@ impl Nexus { organization_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let authz_org = - self.db_datastore.organization_lookup_id(organization_name).await?; + let authz_org = self + .db_datastore + .organization_lookup_path(organization_name) + .await?; self.db_datastore .projects_list_by_name(opctx, &authz_org, pagparams) .await @@ -607,8 +615,10 @@ impl Nexus { organization_name: &Name, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - let authz_org = - self.db_datastore.organization_lookup_id(organization_name).await?; + let authz_org = self + .db_datastore + .organization_lookup_path(organization_name) + .await?; self.db_datastore .projects_list_by_id(opctx, &authz_org, pagparams) .await @@ -621,7 +631,7 @@ impl Nexus { ) -> DeleteResult { let organization_id = self .db_datastore - .organization_lookup_id(organization_name) + .organization_lookup_path(organization_name) .await? .id(); self.db_datastore.project_delete(&organization_id, project_name).await @@ -635,7 +645,7 @@ impl Nexus { ) -> UpdateResult { let organization_id = self .db_datastore - .organization_lookup_id(organization_name) + .organization_lookup_path(organization_name) .await? .id(); self.db_datastore @@ -657,15 +667,11 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; self.db_datastore.project_list_disks(&project_id, pagparams).await } @@ -675,8 +681,10 @@ impl Nexus { project_name: &Name, params: ¶ms::DiskCreate, ) -> CreateResult { - let project = - self.project_fetch(organization_name, project_name).await?; + let authz_project = self + .db_datastore + .project_lookup_path(organization_name, project_name) + .await?; /* * Until we implement snapshots, do not allow disks to be created with a @@ -692,7 +700,7 @@ impl Nexus { let disk_id = Uuid::new_v4(); let disk = db::model::Disk::new( disk_id, - project.id(), + authz_project.id(), params.clone(), db::model::DiskRuntimeState::new(), ); @@ -729,33 +737,22 @@ impl Nexus { project_name: &Name, disk_name: &Name, ) -> LookupResult<(db::model::Disk, authz::ProjectChild)> { - let organization_id = self - .db_datastore - .organization_lookup_id(organization_name) - .await? - .id(); - let project_id = self + let authz_project = self .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) + .project_lookup_path(organization_name, project_name) .await?; let disk = self .db_datastore - .disk_fetch_by_name(&project_id, disk_name) + .disk_fetch_by_name(&authz_project.id(), disk_name) .await?; let disk_id = disk.id(); Ok(( disk, - authz::FLEET - .organization( - organization_id, - LookupType::from(&organization_name.0), - ) - .project(project_id, LookupType::from(&project_name.0)) - .child_generic( - ResourceType::Disk, - disk_id, - LookupType::from(&disk_name.0), - ), + authz_project.child_generic( + ResourceType::Disk, + disk_id, + LookupType::from(&disk_name.0), + ), )) } @@ -834,15 +831,11 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; self.db_datastore.project_list_instances(&project_id, pagparams).await } @@ -852,18 +845,13 @@ impl Nexus { project_name: &Name, params: ¶ms::InstanceCreate, ) -> CreateResult { - let organization_id = self + let authz_project = self .db_datastore - .organization_lookup_id(organization_name) - .await? - .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) + .project_lookup_path(organization_name, project_name) .await?; let saga_params = Arc::new(sagas::ParamsInstanceCreate { - project_id, + project_id: authz_project.id(), create_params: params.clone(), }); @@ -941,15 +929,11 @@ impl Nexus { * instances? Presumably we need to clean them up at some point, but * not right away so that callers can see that they've been destroyed. */ - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; let instance = self .db_datastore .instance_fetch_by_name(&project_id, instance_name) @@ -963,15 +947,11 @@ impl Nexus { project_name: &Name, instance_name: &Name, ) -> LookupResult { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; self.db_datastore .instance_fetch_by_name(&project_id, instance_name) .await @@ -1527,15 +1507,11 @@ impl Nexus { project_name: &Name, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; let vpcs = self.db_datastore.project_list_vpcs(&project_id, pagparams).await?; Ok(vpcs) @@ -1547,22 +1523,20 @@ impl Nexus { project_name: &Name, params: ¶ms::VpcCreate, ) -> CreateResult { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; let vpc_id = Uuid::new_v4(); let system_router_id = Uuid::new_v4(); let default_route_id = Uuid::new_v4(); let default_subnet_id = Uuid::new_v4(); - // TODO: Ultimately when the VPC is created a system router w/ an appropriate setup should also be created. - // Given that the underlying systems aren't wired up yet this is a naive implementation to populate the database - // with a starting router. Eventually this code should be replaced with a saga that'll handle creating the VPC and + // TODO: Ultimately when the VPC is created a system router w/ an + // appropriate setup should also be created. Given that the underlying + // systems aren't wired up yet this is a naive implementation to + // populate the database with a starting router. Eventually this code + // should be replaced with a saga that'll handle creating the VPC and // its underlying system let router = db::model::VpcRouter::new( system_router_id, @@ -1571,10 +1545,12 @@ impl Nexus { params::VpcRouterCreate { identity: IdentityMetadataCreateParams { name: "system".parse().unwrap(), - description: "Routes are automatically added to this router as vpc subnets are created".into() - } - } - ); + description: "Routes are automatically added to this \ + router as vpc subnets are created" + .into(), + }, + }, + ); let _ = self.db_datastore.vpc_create_router(router).await?; let route = db::model::RouterRoute::new( default_route_id, @@ -1594,9 +1570,11 @@ impl Nexus { }, ); - // TODO: This is both fake and utter nonsense. It should be eventually replaced with the proper behavior for creating - // the default route which may not even happen here. Creating the vpc, its system router, and that routers default route - // should all be apart of the same transaction. + // TODO: This is both fake and utter nonsense. It should be eventually + // replaced with the proper behavior for creating the default route + // which may not even happen here. Creating the vpc, its system router, + // and that routers default route should all be apart of the same + // transaction. self.db_datastore.router_create_route(route).await?; let vpc = db::model::Vpc::new( vpc_id, @@ -1619,11 +1597,13 @@ impl Nexus { ), }, ipv4_block: Some(Ipv4Net( - // TODO: This value should be replaced with the correct ipv4 range for a default subnet + // TODO: This value should be replaced with the correct ipv4 + // range for a default subnet "10.1.9.32/16".parse::().unwrap(), )), ipv6_block: Some(Ipv6Net( - // TODO: This value should be replaced w/ the first `/64` ipv6 from the address block + // TODO: This value should be replaced w/ the first `/64` + // ipv6 from the address block "2001:db8::0/64".parse::().unwrap(), )), }, @@ -1652,15 +1632,11 @@ impl Nexus { project_name: &Name, vpc_name: &Name, ) -> LookupResult { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; Ok(self.db_datastore.vpc_fetch_by_name(&project_id, vpc_name).await?) } @@ -1671,15 +1647,11 @@ impl Nexus { vpc_name: &Name, params: ¶ms::VpcUpdate, ) -> UpdateResult<()> { - let organization_id = self + let project_id = self .db_datastore - .organization_lookup_id(organization_name) + .project_lookup_path(organization_name, project_name) .await? .id(); - let project_id = self - .db_datastore - .project_lookup_id_by_name(&organization_id, project_name) - .await?; let vpc = self.db_datastore.vpc_fetch_by_name(&project_id, vpc_name).await?; Ok(self diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index d20f9f17a2d..9cf091b31f4 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -148,3 +148,16 @@ pub async fn create_router( ) .await } + +pub async fn project_get( + client: &ClientTestContext, + project_url: &str, +) -> Project { + NexusRequest::object_get(client, project_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to get project") + .parsed_body() + .expect("failed to parse Project") +} diff --git a/nexus/tests/integration_tests/basic.rs b/nexus/tests/integration_tests/basic.rs index 8db1e6889df..b8b7de0995b 100644 --- a/nexus/tests/integration_tests/basic.rs +++ b/nexus/tests/integration_tests/basic.rs @@ -9,13 +9,13 @@ * TODO-coverage add test for racks, sleds */ -use dropshot::test_util::object_get; 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; use http::StatusCode; +use nexus_test_utils::resource_helpers::project_get; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Name; @@ -241,6 +241,30 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) { project_ids }; + /* + * Unauthenticated and unauthorized users cannot fetch the Project. + */ + let simproject1_url = "/organizations/test-org/projects/simproject1"; + NexusRequest::expect_failure( + client, + http::StatusCode::NOT_FOUND, + http::Method::GET, + simproject1_url, + ) + .execute() + .await + .expect("failed to make request"); + NexusRequest::expect_failure( + client, + http::StatusCode::NOT_FOUND, + http::Method::GET, + simproject1_url, + ) + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("failed to make request"); + /* * Error case: GET /organizations/test-org/projects/simproject1/nonexistent * (a path that does not exist beneath a resource that does exist) @@ -716,10 +740,6 @@ async fn projects_list( .all_items } -async fn project_get(client: &ClientTestContext, project_url: &str) -> Project { - object_get::(client, project_url).await -} - async fn sleds_list(client: &ClientTestContext, sleds_url: &str) -> Vec { objects_list_page::(client, sleds_url).await.items } diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 921240fa12e..8a2392e5786 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -4,10 +4,9 @@ use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; +use nexus_test_utils::resource_helpers::project_get; use omicron_nexus::external_api::views::Project; -use dropshot::test_util::object_get; - use nexus_test_utils::resource_helpers::{create_organization, create_project}; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; @@ -28,11 +27,11 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { create_project(&client, &org_name, &p2_name).await; let p1_url = format!("/organizations/{}/projects/{}", org_name, p1_name); - let project: Project = object_get(&client, &p1_url).await; + let project: Project = project_get(&client, &p1_url).await; assert_eq!(project.identity.name, p1_name); let p2_url = format!("/organizations/{}/projects/{}", org_name, p2_name); - let project: Project = object_get(&client, &p2_url).await; + let project: Project = project_get(&client, &p2_url).await; assert_eq!(project.identity.name, p2_name); /* diff --git a/tools/oxapi_demo b/tools/oxapi_demo index 94283bfad60..bc24146a8fc 100755 --- a/tools/oxapi_demo +++ b/tools/oxapi_demo @@ -236,7 +236,7 @@ function cmd_project_delete function cmd_project_get { [[ $# != 2 ]] && usage "expected ORGANIZATION_NAME PROJECT_NAME" - do_curl "/organizations/$1/projects/$2" + do_curl_authn "/organizations/$1/projects/$2" } function cmd_project_list_instances