From 377df21e42806d7e9d8535467cdc411217ca8715 Mon Sep 17 00:00:00 2001 From: Jordan Doyle Date: Sat, 18 Sep 2021 15:37:39 +0100 Subject: [PATCH] Clean up Crate fetching/permissions checking code in API layer --- chartered-db/src/users.rs | 2 +- chartered-web/src/main.rs | 1 + chartered-web/src/endpoints/mod.rs | 12 ------------ chartered-web/src/models/crates.rs | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ chartered-web/src/models/mod.rs | 1 + chartered-web/src/endpoints/cargo_api/download.rs | 15 ++++++--------- chartered-web/src/endpoints/cargo_api/owners.rs | 17 +++++------------ chartered-web/src/endpoints/cargo_api/publish.rs | 27 ++++++++++++--------------- chartered-web/src/endpoints/cargo_api/yank.rs | 43 ++++++++++++++++++++----------------------- chartered-web/src/endpoints/web_api/ssh_key.rs | 2 +- chartered-web/src/endpoints/web_api/crates/info.rs | 14 +++++--------- chartered-web/src/endpoints/web_api/crates/members.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------ 12 files changed, 155 insertions(+), 115 deletions(-) diff --git a/chartered-db/src/users.rs b/chartered-db/src/users.rs index 0c3434f..2fc1cef 100644 --- a/chartered-db/src/users.rs +++ a/chartered-db/src/users.rs @@ -154,7 +154,7 @@ conn: ConnectionPool, ssh_key_id: uuid::Uuid, ) -> Result { - use crate::schema::user_ssh_keys::dsl::{uuid, user_id, user_ssh_keys}; + use crate::schema::user_ssh_keys::dsl::{user_id, user_ssh_keys, uuid}; tokio::task::spawn_blocking(move || { let conn = conn.get()?; diff --git a/chartered-web/src/main.rs b/chartered-web/src/main.rs index 1041c6d..0db7056 100644 --- a/chartered-web/src/main.rs +++ a/chartered-web/src/main.rs @@ -1,8 +1,9 @@ #![deny(clippy::pedantic)] #![allow(clippy::module_name_repetitions)] mod endpoints; mod middleware; +mod models; use axum::{ handler::{delete, get, patch, post, put}, diff --git a/chartered-web/src/endpoints/mod.rs b/chartered-web/src/endpoints/mod.rs index 08a6267..3625724 100644 --- a/chartered-web/src/endpoints/mod.rs +++ a/chartered-web/src/endpoints/mod.rs @@ -1,15 +1,3 @@ -macro_rules! ensure_has_crate_perm { - ($db:expr, $user:expr, $crate_expr:expr, $($permission:path | -> $error:expr$(,)?),*) => {{ - let perms = $user.get_crate_permissions($db.clone(), $crate_expr.id).await?; - - $( - if !perms.contains($permission) { - return Err($error); - } - )* - }}; -} - #[derive(serde::Serialize)] pub struct ErrorResponse { error: Option, diff --git a/chartered-web/src/models/crates.rs b/chartered-web/src/models/crates.rs new file mode 100644 index 0000000..60f8b61 100644 --- /dev/null +++ a/chartered-web/src/models/crates.rs @@ -1,0 +1,60 @@ +use axum::http::StatusCode; +use chartered_db::{ + crates::Crate, + users::{User, UserCratePermissionValue as Permission}, + ConnectionPool, +}; +use std::sync::Arc; +use thiserror::Error; + +#[derive(Error, Debug)] +pub enum CrateFetchError { + NotFound, + MissingPermission(chartered_db::users::UserCratePermissionValue), + Database(#[from] chartered_db::Error), +} + +impl CrateFetchError { + pub fn status_code(&self) -> StatusCode { + match self { + Self::NotFound | Self::MissingPermission(Permission::VISIBLE) => StatusCode::NOT_FOUND, + Self::MissingPermission(_) => StatusCode::FORBIDDEN, + Self::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, + } + } +} + +impl std::fmt::Display for CrateFetchError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Self::NotFound | Self::MissingPermission(Permission::VISIBLE) => { + write!(f, "The requested crate does not exist") + } + Self::MissingPermission(v) => { + write!(f, "You don't have the {:?} permission for this crate", v) + } + Self::Database(_) => write!(f, "An error occurred while fetching the crate."), + } + } +} + +pub async fn get_crate_with_permissions( + db: ConnectionPool, + user: Arc, + crate_name: String, + required_permissions: &[chartered_db::users::UserCratePermissionValue], +) -> Result, CrateFetchError> { + let crate_ = Crate::find_by_name(db.clone(), crate_name) + .await? + .ok_or(CrateFetchError::NotFound) + .map(std::sync::Arc::new)?; + let has_permissions = user.get_crate_permissions(db, crate_.id).await?; + + for required_permission in required_permissions { + if !has_permissions.contains(*required_permission) { + return Err(CrateFetchError::MissingPermission(*required_permission)); + } + } + + Ok(crate_) +} diff --git a/chartered-web/src/models/mod.rs b/chartered-web/src/models/mod.rs new file mode 100644 index 0000000..30e78d1 100644 --- /dev/null +++ a/chartered-web/src/models/mod.rs @@ -1,0 +1,1 @@ +pub mod crates; diff --git a/chartered-web/src/endpoints/cargo_api/download.rs b/chartered-web/src/endpoints/cargo_api/download.rs index 70dc1a2..ccae396 100644 --- a/chartered-web/src/endpoints/cargo_api/download.rs +++ a/chartered-web/src/endpoints/cargo_api/download.rs @@ -1,6 +1,6 @@ +use crate::models::crates::get_crate_with_permissions; use axum::extract; use chartered_db::{ - crates::Crate, users::{User, UserCratePermissionValue as Permission}, ConnectionPool, }; @@ -14,10 +14,10 @@ Database(#[from] chartered_db::Error), #[error("Failed to fetch crate file")] File(#[from] std::io::Error), - #[error("The requested crate does not exist")] - NoCrate, #[error("The requested version does not exist for the crate")] NoVersion, + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), } impl Error { @@ -26,7 +26,8 @@ match self { Self::Database(_) | Self::File(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate | Self::NoVersion => StatusCode::NOT_FOUND, + Self::NoVersion => StatusCode::NOT_FOUND, + Self::CrateFetch(e) => e.status_code(), } } } @@ -38,11 +39,7 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate); + let crate_ = get_crate_with_permissions(db.clone(), user, name, &[Permission::VISIBLE]).await?; let version = crate_.version(db, version).await?.ok_or(Error::NoVersion)?; diff --git a/chartered-web/src/endpoints/cargo_api/owners.rs b/chartered-web/src/endpoints/cargo_api/owners.rs index f194d70..5fe7e32 100644 --- a/chartered-web/src/endpoints/cargo_api/owners.rs +++ a/chartered-web/src/endpoints/cargo_api/owners.rs @@ -1,6 +1,6 @@ +use crate::models::crates::get_crate_with_permissions; use axum::{extract, Json}; use chartered_db::{ - crates::Crate, users::{User, UserCratePermissionValue as Permission}, ConnectionPool, }; @@ -12,8 +12,8 @@ pub enum Error { #[error("Failed to query database")] Database(#[from] chartered_db::Error), - #[error("The requested crate does not exist")] - NoCrate, + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), } impl Error { @@ -22,7 +22,7 @@ match self { Self::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate => StatusCode::NOT_FOUND, + Self::CrateFetch(e) => e.status_code(), } } } @@ -47,14 +47,7 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!( - db, user, crate_, - Permission::VISIBLE | -> Error::NoCrate - ); + let crate_ = get_crate_with_permissions(db.clone(), user, name, &[Permission::VISIBLE]).await?; let users = crate_ .owners(db) diff --git a/chartered-web/src/endpoints/cargo_api/publish.rs b/chartered-web/src/endpoints/cargo_api/publish.rs index 845ebf8..52fdb3e 100644 --- a/chartered-web/src/endpoints/cargo_api/publish.rs +++ a/chartered-web/src/endpoints/cargo_api/publish.rs @@ -1,7 +1,7 @@ +use crate::models::crates::get_crate_with_permissions; use axum::extract; use bytes::Bytes; use chartered_db::{ - crates::Crate, users::{User, UserCratePermissionValue as Permission}, ConnectionPool, }; @@ -15,10 +15,8 @@ pub enum Error { #[error("Failed to query database")] Database(#[from] chartered_db::Error), - #[error("The requested crate does not exist")] - NoCrate, - #[error("You don't have permission to publish versions for this crate")] - NoPermission, + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), #[error("Invalid JSON from client: {0}")] JsonParse(#[from] serde_json::Error), #[error("Invalid body")] @@ -31,8 +29,7 @@ match self { Self::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate => StatusCode::NOT_FOUND, - Self::NoPermission => StatusCode::FORBIDDEN, + Self::CrateFetch(e) => e.status_code(), Self::JsonParse(_) | Self::MetadataParse => StatusCode::BAD_REQUEST, } } @@ -61,15 +58,13 @@ parse(body.as_ref()).map_err(|_| Error::MetadataParse)?; let metadata: Metadata = serde_json::from_slice(metadata_bytes)?; - let crate_ = Crate::find_by_name(db.clone(), metadata.inner.name.to_string()) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!( - db, user, crate_, - Permission::VISIBLE | -> Error::NoCrate, - Permission::PUBLISH_VERSION | -> Error::NoPermission, - ); + let crate_ = get_crate_with_permissions( + db.clone(), + user, + metadata.inner.name.to_string(), + &[Permission::VISIBLE, Permission::PUBLISH_VERSION], + ) + .await?; let file_ref = chartered_fs::Local.write(crate_bytes).await.unwrap(); diff --git a/chartered-web/src/endpoints/cargo_api/yank.rs b/chartered-web/src/endpoints/cargo_api/yank.rs index c3589cb..7346aa9 100644 --- a/chartered-web/src/endpoints/cargo_api/yank.rs +++ a/chartered-web/src/endpoints/cargo_api/yank.rs @@ -1,6 +1,6 @@ +use crate::models::crates::get_crate_with_permissions; use axum::{extract, Json}; use chartered_db::{ - crates::Crate, users::{User, UserCratePermissionValue as Permission}, ConnectionPool, }; @@ -12,10 +12,8 @@ pub enum Error { #[error("Failed to query database")] Database(#[from] chartered_db::Error), - #[error("The requested crate does not exist")] - NoCrate, - #[error("You don't have {0:?} permission for this crate")] - NoPermission(Permission), + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), } impl Error { @@ -24,8 +22,7 @@ match self { Self::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate => StatusCode::NOT_FOUND, - Self::NoPermission(_) => StatusCode::FORBIDDEN, + Self::CrateFetch(e) => e.status_code(), } } } @@ -42,15 +39,13 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!( - db, user, crate_, - Permission::VISIBLE | -> Error::NoCrate, - Permission::YANK_VERSION | -> Error::NoPermission(Permission::YANK_VERSION), - ); + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::YANK_VERSION], + ) + .await?; crate_.yank_version(db, version, true).await?; @@ -62,15 +57,13 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!( - db, user, crate_, - Permission::VISIBLE | -> Error::NoCrate, - Permission::YANK_VERSION | -> Error::NoPermission(Permission::YANK_VERSION), - ); + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::YANK_VERSION], + ) + .await?; crate_.yank_version(db, version, false).await?; diff --git a/chartered-web/src/endpoints/web_api/ssh_key.rs b/chartered-web/src/endpoints/web_api/ssh_key.rs index 2231390..923ca05 100644 --- a/chartered-web/src/endpoints/web_api/ssh_key.rs +++ a/chartered-web/src/endpoints/web_api/ssh_key.rs @@ -1,12 +1,12 @@ use chartered_db::{users::User, ConnectionPool}; use axum::{extract, Json}; +use chartered_db::uuid::Uuid; use chrono::NaiveDateTime; use log::warn; use serde::{Deserialize, Serialize}; use std::sync::Arc; use thiserror::Error; -use chartered_db::uuid::Uuid; use crate::endpoints::ErrorResponse; diff --git a/chartered-web/src/endpoints/web_api/crates/info.rs b/chartered-web/src/endpoints/web_api/crates/info.rs index 0359c85..e4c82f7 100644 --- a/chartered-web/src/endpoints/web_api/crates/info.rs +++ a/chartered-web/src/endpoints/web_api/crates/info.rs @@ -1,6 +1,6 @@ +use crate::models::crates::get_crate_with_permissions; use axum::{extract, Json}; use chartered_db::{ - crates::Crate, users::{User, UserCratePermissionValue as Permission}, ConnectionPool, }; @@ -15,8 +15,8 @@ Database(#[from] chartered_db::Error), #[error("Failed to fetch crate file")] File(#[from] std::io::Error), - #[error("The requested crate does not exist")] - NoCrate, + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), } impl Error { @@ -25,7 +25,7 @@ match self { Self::Database(_) | Self::File(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate => StatusCode::NOT_FOUND, + Self::CrateFetch(e) => e.status_code(), } } } @@ -37,11 +37,7 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate); + let crate_ = get_crate_with_permissions(db.clone(), user, name, &[Permission::VISIBLE]).await?; let versions = crate_.clone().versions(db).await?; diff --git a/chartered-web/src/endpoints/web_api/crates/members.rs b/chartered-web/src/endpoints/web_api/crates/members.rs index 29bc45a..a2fbc06 100644 --- a/chartered-web/src/endpoints/web_api/crates/members.rs +++ a/chartered-web/src/endpoints/web_api/crates/members.rs @@ -1,5 +1,10 @@ +use crate::models::crates::get_crate_with_permissions; use axum::{extract, Json}; -use chartered_db::{ConnectionPool, crates::Crate, users::{User, UserCratePermissionValue as Permission}, uuid::Uuid}; +use chartered_db::{ + users::{User, UserCratePermissionValue as Permission}, + uuid::Uuid, + ConnectionPool, +}; use serde::{Deserialize, Serialize}; use std::sync::Arc; use thiserror::Error; @@ -24,11 +29,13 @@ extract::Extension(db): extract::Extension, extract::Extension(user): extract::Extension>, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) - .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission); + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::MANAGE_USERS], + ) + .await?; let members = crate_ .members(db) @@ -59,13 +66,17 @@ extract::Extension(user): extract::Extension>, extract::Json(req): extract::Json, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::MANAGE_USERS], + ) + .await?; + + let action_user = User::find_by_uuid(db.clone(), req.user_uuid) .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission); - - let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?; + .ok_or(Error::InvalidUserId)?; let affected_rows = crate_ .update_permissions(db, action_user.id, req.permissions) @@ -83,14 +94,18 @@ extract::Extension(user): extract::Extension>, extract::Json(req): extract::Json, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::MANAGE_USERS], + ) + .await?; + + let action_user = User::find_by_uuid(db.clone(), req.user_uuid) .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission); + .ok_or(Error::InvalidUserId)?; - let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?; - crate_ .insert_permissions(db, action_user.id, req.permissions) .await?; @@ -109,13 +124,17 @@ extract::Extension(user): extract::Extension>, extract::Json(req): extract::Json, ) -> Result, Error> { - let crate_ = Crate::find_by_name(db.clone(), name) + let crate_ = get_crate_with_permissions( + db.clone(), + user, + name, + &[Permission::VISIBLE, Permission::MANAGE_USERS], + ) + .await?; + + let action_user = User::find_by_uuid(db.clone(), req.user_uuid) .await? - .ok_or(Error::NoCrate) - .map(std::sync::Arc::new)?; - ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission); - - let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?; + .ok_or(Error::InvalidUserId)?; crate_.delete_member(db, action_user.id).await?; @@ -126,10 +145,8 @@ pub enum Error { #[error("Failed to query database")] Database(#[from] chartered_db::Error), - #[error("The requested crate does not exist")] - NoCrate, - #[error("You don't have permission to manage users for this crate")] - NoPermission, + #[error("{0}")] + CrateFetch(#[from] crate::models::crates::CrateFetchError), #[error("Permissions update conflict, user was removed as a member of the crate")] UpdateConflictRemoved, #[error("An invalid user id was given")] @@ -142,8 +159,7 @@ match self { Self::Database(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::NoCrate => StatusCode::NOT_FOUND, - Self::NoPermission => StatusCode::FORBIDDEN, + Self::CrateFetch(e) => e.status_code(), Self::UpdateConflictRemoved => StatusCode::CONFLICT, Self::InvalidUserId => StatusCode::BAD_REQUEST, } -- rgit 0.1.3