From 4b7f3fc05fd355dbd679eb7d5d63d74fe7027a81 Mon Sep 17 00:00:00 2001 From: jonaro00 <54029719+jonaro00@users.noreply.github.com> Date: Thu, 25 Jul 2024 13:28:49 +0200 Subject: [PATCH] fix(resource-recorder): check project permission by project id (#1839) * nit * feat: allow ulid in project endpoint paths * refa * feat: check by proj id * fix: test --- backends/src/client/gateway.rs | 8 +++++-- backends/src/lib.rs | 23 +++----------------- backends/src/test_utils/gateway.rs | 6 ++++-- cargo-shuttle/src/lib.rs | 4 ++-- gateway/src/auth.rs | 34 +++++++++++++++++++----------- resource-recorder/src/lib.rs | 2 +- 6 files changed, 38 insertions(+), 39 deletions(-) diff --git a/backends/src/client/gateway.rs b/backends/src/client/gateway.rs index 145f58e36..5dca3696c 100644 --- a/backends/src/client/gateway.rs +++ b/backends/src/client/gateway.rs @@ -52,10 +52,14 @@ impl ProjectsDal for ServicesApiClient { } #[instrument(skip_all)] - async fn head_user_project(&self, user_token: &str, project_name: &str) -> Result { + async fn head_user_project( + &self, + user_token: &str, + project_ident: &str, + ) -> Result { self.request_raw( Method::HEAD, - format!("projects/{}", project_name).as_str(), + format!("projects/{}", project_ident).as_str(), None::<()>, Some(header_map_with_bearer(user_token)), ) diff --git a/backends/src/lib.rs b/backends/src/lib.rs index 7dad2b884..363d29470 100644 --- a/backends/src/lib.rs +++ b/backends/src/lib.rs @@ -36,13 +36,7 @@ pub trait ClaimExt { async fn owns_project( &self, projects_dal: &G, - project_name: &str, - ) -> Result; - /// Verify if the claim subject has ownership of a project. - async fn owns_project_id( - &self, - projects_dal: &G, - project_id: &str, + project_ident: &str, ) -> Result; } @@ -84,20 +78,9 @@ impl ClaimExt for Claim { async fn owns_project( &self, projects_dal: &G, - project_name: &str, - ) -> Result { - let token = self.token.as_ref().expect("token to be set"); - projects_dal.head_user_project(token, project_name).await - } - - #[instrument(skip_all)] - async fn owns_project_id( - &self, - projects_dal: &G, - project_id: &str, + project_ident: &str, ) -> Result { let token = self.token.as_ref().expect("token to be set"); - let projects = projects_dal.get_user_project_ids(token).await?; - Ok(projects.iter().any(|id| id == project_id)) + projects_dal.head_user_project(token, project_ident).await } } diff --git a/backends/src/test_utils/gateway.rs b/backends/src/test_utils/gateway.rs index cfc76700c..8bf02548a 100644 --- a/backends/src/test_utils/gateway.rs +++ b/backends/src/test_utils/gateway.rs @@ -68,7 +68,7 @@ pub async fn get_mocked_gateway_server() -> MockServer { let p = projects.clone(); Mock::given(method(http::Method::HEAD)) - .and(path_regex("/projects/[a-z0-9-]+")) + .and(path_regex("/projects/[a-zA-Z0-9-]+")) .respond_with(move |req: &Request| { let Some(bearer) = req.headers.get("AUTHORIZATION") else { return ResponseTemplate::new(401); @@ -77,7 +77,9 @@ pub async fn get_mocked_gateway_server() -> MockServer { let user = bearer.to_str().unwrap().split_whitespace().nth(1).unwrap(); - if p.iter().any(|p| p.owner_id == user && p.name == project) { + if p.iter() + .any(|p| p.owner_id == user && (p.name == project || p.id == project)) + { ResponseTemplate::new(200) } else { ResponseTemplate::new(401) diff --git a/cargo-shuttle/src/lib.rs b/cargo-shuttle/src/lib.rs index 0fa230e5a..0a91bf255 100644 --- a/cargo-shuttle/src/lib.rs +++ b/cargo-shuttle/src/lib.rs @@ -2275,8 +2275,8 @@ impl Shuttle { state => { debug!("deployment logs stream received state: {state} when it expected to receive running state"); println!( - "Deployment entered an unexpected state - Please create a ticket to report this." - ); + "Deployment entered an unexpected state - Please create a ticket to report this." + ); } } diff --git a/gateway/src/auth.rs b/gateway/src/auth.rs index df9ea4a74..9bf00ec82 100644 --- a/gateway/src/auth.rs +++ b/gateway/src/auth.rs @@ -9,6 +9,7 @@ use shuttle_common::claims::Claim; use shuttle_common::models::error::{ApiError, InvalidProjectName, ProjectNotFound}; use thiserror::Error; use tracing::error; +use ulid::Ulid; use crate::api::latest::RouterState; use crate::service; @@ -78,17 +79,27 @@ where .await .map_err(Error::StatusCode)?; - let scope = match Path::::from_request_parts(parts, state).await { - Ok(Path(p)) => p, - Err(_) => Path::<(ProjectName, String)>::from_request_parts(parts, state) - .await - .map(|Path((p, _))| p) - .map_err(|_| InvalidProjectName)?, - }; - let RouterState { service, .. } = RouterState::from_ref(state); - let allowed = claim.is_admin() + // Enables checking HEAD at /projects/{ulid}, used by res-rec to check permission for a proj id + let scope = match Path::::from_request_parts(parts, state).await { + Ok(Path(ulid)) => { + let p = service.find_project_by_id(&ulid.to_string()).await?; + ProjectName::new(&p.name).expect("valid project name") + } + Err(_) => { + // Normal check for project name in path + match Path::::from_request_parts(parts, state).await { + Ok(Path(p)) => p, + Err(_) => Path::<(ProjectName, String)>::from_request_parts(parts, state) + .await + .map(|Path((p, _))| p) + .map_err(|_| InvalidProjectName)?, + } + } + }; + + if claim.is_admin() || claim.is_deployer() || service .permit_client @@ -97,9 +108,8 @@ where &service.find_project_by_name(&scope).await?.id, "develop", // TODO: make this configurable per endpoint? ) - .await?; - - if allowed { + .await? + { Ok(Self { claim, scope }) } else { Err(ProjectNotFound(scope.to_string()).into()) diff --git a/resource-recorder/src/lib.rs b/resource-recorder/src/lib.rs index 1b36894dc..bc3956288 100644 --- a/resource-recorder/src/lib.rs +++ b/resource-recorder/src/lib.rs @@ -117,7 +117,7 @@ where if !claim.is_admin() && !claim.is_deployer() && !claim - .owns_project_id(&self.gateway_client, project_id) + .owns_project(&self.gateway_client, project_id) .await .map_err(|_| Status::internal("could not verify project ownership"))? {