From 96b11524d53b3616a28f33e2b057655be1639f6f Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 15:04:53 +0200 Subject: [PATCH] Error refactoring --- src/api/admin/api_server.rs | 2 +- src/api/admin/bucket.rs | 4 +- src/api/admin/error.rs | 13 ++-- src/api/admin/mod.rs | 7 +- src/api/common_error.rs | 17 ++--- src/api/error.rs | 127 ++++++++------------------------- src/api/helpers.rs | 8 +-- src/api/k2v/batch.rs | 4 +- src/api/k2v/range.rs | 2 +- src/api/k2v/router.rs | 4 +- src/api/s3/api_server.rs | 2 +- src/api/s3/bucket.rs | 4 +- src/api/s3/copy.rs | 14 ++-- src/api/s3/get.rs | 8 +-- src/api/s3/list.rs | 2 +- src/api/s3/post_object.rs | 34 ++++----- src/api/s3/put.rs | 12 ++-- src/api/s3/router.rs | 2 +- src/api/s3/website.rs | 14 ++-- src/api/signature/mod.rs | 2 +- src/api/signature/payload.rs | 14 ++-- src/api/signature/streaming.rs | 6 +- src/web/error.rs | 33 +++------ src/web/web_server.rs | 6 +- 24 files changed, 135 insertions(+), 206 deletions(-) diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index b344a51b..b2effa62 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -17,9 +17,9 @@ use garage_util::error::Error as GarageError; use crate::generic_server::*; -use crate::admin::error::*; use crate::admin::bucket::*; use crate::admin::cluster::*; +use crate::admin::error::*; use crate::admin::key::*; use crate::admin::router::{Authorization, Endpoint}; diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index 1ecb66ab..db1fda0f 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -16,8 +16,8 @@ use garage_model::garage::Garage; use garage_model::permission::*; use garage_model::s3::object_table::ObjectFilter; -use crate::admin::key::ApiBucketKeyPerm; use crate::admin::error::*; +use crate::admin::key::ApiBucketKeyPerm; use crate::admin::parse_json_body; pub async fn handle_list_buckets(garage: &Arc) -> Result, Error> { @@ -98,7 +98,7 @@ pub async fn handle_get_bucket_info( .ok_or_bad_request("Bucket not found")?, _ => { return Err(Error::bad_request( - "Either id or globalAlias must be provided (but not both)" + "Either id or globalAlias must be provided (but not both)", )); } }; diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs index 3e488d8d..1f49fed5 100644 --- a/src/api/admin/error.rs +++ b/src/api/admin/error.rs @@ -3,10 +3,10 @@ use hyper::header::HeaderValue; use hyper::{Body, HeaderMap, StatusCode}; use garage_model::helper::error::Error as HelperError; -use garage_util::error::Error as GarageError; use crate::generic_server::ApiError; -pub use crate::common_error::*; +use crate::common_error::CommonError; +pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; /// Errors of this crate #[derive(Debug, Error)] @@ -47,7 +47,9 @@ pub enum Error { } impl From for Error -where CommonError: From { +where + CommonError: From, +{ fn from(err: T) -> Self { Error::CommonError(CommonError::from(err)) } @@ -83,7 +85,10 @@ impl ApiError for Error { } fn http_body(&self, garage_region: &str, path: &str) -> Body { - Body::from(format!("ERROR: {}\n\ngarage region: {}\npath: {}", self, garage_region, path)) + Body::from(format!( + "ERROR: {}\n\ngarage region: {}\npath: {}", + self, garage_region, path + )) } } diff --git a/src/api/admin/mod.rs b/src/api/admin/mod.rs index 68839039..73700e6e 100644 --- a/src/api/admin/mod.rs +++ b/src/api/admin/mod.rs @@ -1,14 +1,13 @@ pub mod api_server; -mod router; mod error; +mod router; mod bucket; mod cluster; mod key; - -use serde::{Deserialize}; -use hyper::{Request, Body}; +use hyper::{Body, Request}; +use serde::Deserialize; use error::*; diff --git a/src/api/common_error.rs b/src/api/common_error.rs index 8be85f97..eca27e6b 100644 --- a/src/api/common_error.rs +++ b/src/api/common_error.rs @@ -1,12 +1,8 @@ use err_derive::Error; -use hyper::header::HeaderValue; -use hyper::{Body, HeaderMap, StatusCode}; +use hyper::StatusCode; -use garage_model::helper::error::Error as HelperError; use garage_util::error::Error as GarageError; -use crate::generic_server::ApiError; - /// Errors of this crate #[derive(Debug, Error)] pub enum CommonError { @@ -36,8 +32,9 @@ impl CommonError { | GarageError::RemoteError(_) | GarageError::Quorum(_, _, _, _), ) => StatusCode::SERVICE_UNAVAILABLE, - CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => - StatusCode::INTERNAL_SERVER_ERROR, + CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => { + StatusCode::INTERNAL_SERVER_ERROR + } CommonError::BadRequest(_) => StatusCode::BAD_REQUEST, } } @@ -57,7 +54,11 @@ where fn ok_or_bad_request>(self, reason: M) -> Result { match self { Ok(x) => Ok(x), - Err(e) => Err(CommonError::BadRequest(format!("{}: {}", reason.as_ref(), e))), + Err(e) => Err(CommonError::BadRequest(format!( + "{}: {}", + reason.as_ref(), + e + ))), } } } diff --git a/src/api/error.rs b/src/api/error.rs index 90bfccef..3cb97019 100644 --- a/src/api/error.rs +++ b/src/api/error.rs @@ -7,24 +7,17 @@ use hyper::{Body, HeaderMap, StatusCode}; use garage_model::helper::error::Error as HelperError; use garage_util::error::Error as GarageError; +use crate::common_error::CommonError; +pub use crate::common_error::{OkOrBadRequest, OkOrInternalError}; use crate::generic_server::ApiError; use crate::s3::xml as s3_xml; /// Errors of this crate #[derive(Debug, Error)] pub enum Error { - // Category: internal error - /// Error related to deeper parts of Garage - #[error(display = "Internal error: {}", _0)] - InternalError(#[error(source)] GarageError), - - /// Error related to Hyper - #[error(display = "Internal error (Hyper error): {}", _0)] - Hyper(#[error(source)] hyper::Error), - - /// Error related to HTTP - #[error(display = "Internal error (HTTP error): {}", _0)] - Http(#[error(source)] http::Error), + #[error(display = "{}", _0)] + /// Error from common error + CommonError(CommonError), // Category: cannot process /// No proper api key was used, or the signature was invalid @@ -101,10 +94,6 @@ pub enum Error { #[error(display = "Invalid HTTP range: {:?}", _0)] InvalidRange(#[error(from)] (http_range::HttpRangeParseError, u64)), - /// The client sent an invalid request - #[error(display = "Bad request: {}", _0)] - BadRequest(String), - /// The client asked for an invalid return format (invalid Accept header) #[error(display = "Not acceptable: {}", _0)] NotAcceptable(String), @@ -114,6 +103,15 @@ pub enum Error { NotImplemented(String), } +impl From for Error +where + CommonError: From, +{ + fn from(err: T) -> Self { + Error::CommonError(CommonError::from(err)) + } +} + impl From for Error { fn from(err: roxmltree::Error) -> Self { Self::InvalidXml(format!("{}", err)) @@ -129,16 +127,16 @@ impl From for Error { impl From for Error { fn from(err: HelperError) -> Self { match err { - HelperError::Internal(i) => Self::InternalError(i), - HelperError::BadRequest(b) => Self::BadRequest(b), - e => Self::BadRequest(format!("{}", e)), + HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), + HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), + e => Self::CommonError(CommonError::BadRequest(format!("{}", e))), } } } impl From for Error { fn from(err: multer::Error) -> Self { - Self::BadRequest(err.to_string()) + Self::bad_request(err) } } @@ -157,18 +155,26 @@ impl Error { Error::Forbidden(_) => "AccessDenied", Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed", Error::NotImplemented(_) => "NotImplemented", - Error::InternalError( + Error::CommonError(CommonError::InternalError( GarageError::Timeout | GarageError::RemoteError(_) | GarageError::Quorum(_, _, _, _), - ) => "ServiceUnavailable", - Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => "InternalError", + )) => "ServiceUnavailable", + Error::CommonError( + CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_), + ) => "InternalError", _ => "InvalidRequest", } } + pub fn internal_error(msg: M) -> Self { + Self::CommonError(CommonError::InternalError(GarageError::Message( + msg.to_string(), + ))) + } + pub fn bad_request(msg: M) -> Self { - Self::BadRequest(msg.to_string()) + Self::CommonError(CommonError::BadRequest(msg.to_string())) } } @@ -176,19 +182,12 @@ impl ApiError for Error { /// Get the HTTP status code that best represents the meaning of the error for the client fn http_status_code(&self) -> StatusCode { match self { + Error::CommonError(c) => c.http_status_code(), Error::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND, Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED, Error::Forbidden(_) => StatusCode::FORBIDDEN, Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, - Error::InternalError( - GarageError::Timeout - | GarageError::RemoteError(_) - | GarageError::Quorum(_, _, _, _), - ) => StatusCode::SERVICE_UNAVAILABLE, - Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => { - StatusCode::INTERNAL_SERVER_ERROR - } Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE, Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, _ => StatusCode::BAD_REQUEST, @@ -230,67 +229,3 @@ impl ApiError for Error { })) } } - -/// Trait to map error to the Bad Request error code -pub trait OkOrBadRequest { - type S; - fn ok_or_bad_request>(self, reason: M) -> Result; -} - -impl OkOrBadRequest for Result -where - E: std::fmt::Display, -{ - type S = T; - fn ok_or_bad_request>(self, reason: M) -> Result { - match self { - Ok(x) => Ok(x), - Err(e) => Err(Error::BadRequest(format!("{}: {}", reason.as_ref(), e))), - } - } -} - -impl OkOrBadRequest for Option { - type S = T; - fn ok_or_bad_request>(self, reason: M) -> Result { - match self { - Some(x) => Ok(x), - None => Err(Error::BadRequest(reason.as_ref().to_string())), - } - } -} - -/// Trait to map an error to an Internal Error code -pub trait OkOrInternalError { - type S; - fn ok_or_internal_error>(self, reason: M) -> Result; -} - -impl OkOrInternalError for Result -where - E: std::fmt::Display, -{ - type S = T; - fn ok_or_internal_error>(self, reason: M) -> Result { - match self { - Ok(x) => Ok(x), - Err(e) => Err(Error::InternalError(GarageError::Message(format!( - "{}: {}", - reason.as_ref(), - e - )))), - } - } -} - -impl OkOrInternalError for Option { - type S = T; - fn ok_or_internal_error>(self, reason: M) -> Result { - match self { - Some(x) => Ok(x), - None => Err(Error::InternalError(GarageError::Message( - reason.as_ref().to_string(), - ))), - } - } -} diff --git a/src/api/helpers.rs b/src/api/helpers.rs index 5e249dae..e94e8b00 100644 --- a/src/api/helpers.rs +++ b/src/api/helpers.rs @@ -52,7 +52,7 @@ pub fn authority_to_host(authority: &str) -> Result { let mut iter = authority.chars().enumerate(); let (_, first_char) = iter .next() - .ok_or_else(|| Error::BadRequest("Authority is empty".to_string()))?; + .ok_or_else(|| Error::bad_request("Authority is empty".to_string()))?; let split = match first_char { '[' => { @@ -60,7 +60,7 @@ pub fn authority_to_host(authority: &str) -> Result { match iter.next() { Some((_, ']')) => iter.next(), _ => { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Authority {} has an illegal format", authority ))) @@ -73,7 +73,7 @@ pub fn authority_to_host(authority: &str) -> Result { let authority = match split { Some((i, ':')) => Ok(&authority[..i]), None => Ok(authority), - Some((_, _)) => Err(Error::BadRequest(format!( + Some((_, _)) => Err(Error::bad_request(format!( "Authority {} has an illegal format", authority ))), @@ -134,7 +134,7 @@ pub fn parse_bucket_key<'a>( None => (path, None), }; if bucket.is_empty() { - return Err(Error::BadRequest("No bucket specified".to_string())); + return Err(Error::bad_request("No bucket specified")); } Ok((bucket, key)) } diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs index a97bd7f2..26d3cef0 100644 --- a/src/api/k2v/batch.rs +++ b/src/api/k2v/batch.rs @@ -88,7 +88,7 @@ async fn handle_read_batch_query( let (items, more, next_start) = if query.single_item { if query.prefix.is_some() || query.end.is_some() || query.limit.is_some() || query.reverse { - return Err(Error::BadRequest("Batch query parameters 'prefix', 'end', 'limit' and 'reverse' must not be set when singleItem is true.".into())); + return Err(Error::bad_request("Batch query parameters 'prefix', 'end', 'limit' and 'reverse' must not be set when singleItem is true.")); } let sk = query .start @@ -183,7 +183,7 @@ async fn handle_delete_batch_query( let deleted_items = if query.single_item { if query.prefix.is_some() || query.end.is_some() { - return Err(Error::BadRequest("Batch query parameters 'prefix' and 'end' must not be set when singleItem is true.".into())); + return Err(Error::bad_request("Batch query parameters 'prefix' and 'end' must not be set when singleItem is true.")); } let sk = query .start diff --git a/src/api/k2v/range.rs b/src/api/k2v/range.rs index cd019723..8d4cefbc 100644 --- a/src/api/k2v/range.rs +++ b/src/api/k2v/range.rs @@ -31,7 +31,7 @@ where (None, Some(s)) => (Some(s.clone()), false), (Some(p), Some(s)) => { if !s.starts_with(p) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Start key '{}' does not start with prefix '{}'", s, p ))); diff --git a/src/api/k2v/router.rs b/src/api/k2v/router.rs index f948ffce..611b6629 100644 --- a/src/api/k2v/router.rs +++ b/src/api/k2v/router.rs @@ -62,7 +62,7 @@ impl Endpoint { .unwrap_or((path.to_owned(), "")); if bucket.is_empty() { - return Err(Error::BadRequest("Missing bucket name".to_owned())); + return Err(Error::bad_request("Missing bucket name".to_owned())); } if *req.method() == Method::OPTIONS { @@ -83,7 +83,7 @@ impl Endpoint { Method::PUT => Self::from_put(partition_key, &mut query)?, Method::DELETE => Self::from_delete(partition_key, &mut query)?, _ if req.method() == method_search => Self::from_search(partition_key, &mut query)?, - _ => return Err(Error::BadRequest("Unknown method".to_owned())), + _ => return Err(Error::bad_request("Unknown method".to_owned())), }; if let Some(message) = query.nonempty_message() { diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 2bf4a1bc..af9f03e7 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -310,7 +310,7 @@ impl ApiHandler for S3ApiServer { ) .await } else { - Err(Error::BadRequest(format!( + Err(Error::bad_request(format!( "Invalid endpoint: list-type={}", list_type ))) diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs index 93048a8c..6ecda2cd 100644 --- a/src/api/s3/bucket.rs +++ b/src/api/s3/bucket.rs @@ -130,7 +130,7 @@ pub async fn handle_create_bucket( if let Some(location_constraint) = cmd { if location_constraint != garage.config.s3_api.s3_region { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Cannot satisfy location constraint `{}`: buckets can only be created in region `{}`", location_constraint, garage.config.s3_api.s3_region @@ -163,7 +163,7 @@ pub async fn handle_create_bucket( } else { // Create the bucket! if !is_valid_bucket_name(&bucket_name) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "{}: {}", bucket_name, INVALID_BUCKET_NAME_MESSAGE ))); diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs index 4e94d887..825b8fc0 100644 --- a/src/api/s3/copy.rs +++ b/src/api/s3/copy.rs @@ -201,8 +201,8 @@ pub async fn handle_upload_part_copy( let mut ranges = http_range::HttpRange::parse(range_str, source_version_meta.size) .map_err(|e| (e, source_version_meta.size))?; if ranges.len() != 1 { - return Err(Error::BadRequest( - "Invalid x-amz-copy-source-range header: exactly 1 range must be given".into(), + return Err(Error::bad_request( + "Invalid x-amz-copy-source-range header: exactly 1 range must be given", )); } else { ranges.pop().unwrap() @@ -230,8 +230,8 @@ pub async fn handle_upload_part_copy( // This is only for small files, we don't bother handling this. // (in AWS UploadPartCopy works for parts at least 5MB which // is never the case of an inline object) - return Err(Error::BadRequest( - "Source object is too small (minimum part size is 5Mb)".into(), + return Err(Error::bad_request( + "Source object is too small (minimum part size is 5Mb)", )); } ObjectVersionData::FirstBlock(_meta, _first_block_hash) => (), @@ -250,7 +250,7 @@ pub async fn handle_upload_part_copy( // Check this part number hasn't yet been uploaded if let Some(dv) = dest_version { if dv.has_part_number(part_number) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Part number {} has already been uploaded", part_number ))); @@ -536,8 +536,8 @@ impl CopyPreconditionHeaders { (None, None, None, Some(ims)) => v_date > *ims, (None, None, None, None) => true, _ => { - return Err(Error::BadRequest( - "Invalid combination of x-amz-copy-source-if-xxxxx headers".into(), + return Err(Error::bad_request( + "Invalid combination of x-amz-copy-source-if-xxxxx headers", )) } }; diff --git a/src/api/s3/get.rs b/src/api/s3/get.rs index 3edf22a6..794bd4e9 100644 --- a/src/api/s3/get.rs +++ b/src/api/s3/get.rs @@ -210,8 +210,8 @@ pub async fn handle_get( match (part_number, parse_range_header(req, last_v_meta.size)?) { (Some(_), Some(_)) => { - return Err(Error::BadRequest( - "Cannot specify both partNumber and Range header".into(), + return Err(Error::bad_request( + "Cannot specify both partNumber and Range header", )); } (Some(pn), None) => { @@ -302,9 +302,9 @@ async fn handle_get_range( let body: Body = Body::from(bytes[begin as usize..end as usize].to_vec()); Ok(resp_builder.body(body)?) } else { - None.ok_or_internal_error( + Err(Error::internal_error( "Requested range not present in inline bytes when it should have been", - ) + )) } } ObjectVersionData::FirstBlock(_meta, _first_block_hash) => { diff --git a/src/api/s3/list.rs b/src/api/s3/list.rs index e2848c57..d97aafe5 100644 --- a/src/api/s3/list.rs +++ b/src/api/s3/list.rs @@ -588,7 +588,7 @@ impl ListObjectsQuery { "]" => Ok(RangeBegin::AfterKey { key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?, }), - _ => Err(Error::BadRequest("Invalid continuation token".to_string())), + _ => Err(Error::bad_request("Invalid continuation token".to_string())), }, // StartAfter has defined semantics in the spec: diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs index 86fa7880..91648a19 100644 --- a/src/api/s3/post_object.rs +++ b/src/api/s3/post_object.rs @@ -48,7 +48,7 @@ pub async fn handle_post_object( let field = if let Some(field) = multipart.next_field().await? { field } else { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Request did not contain a file".to_owned(), )); }; @@ -66,14 +66,14 @@ pub async fn handle_post_object( "tag" => (/* tag need to be reencoded, but we don't support them yet anyway */), "acl" => { if params.insert("x-amz-acl", content).is_some() { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Field 'acl' provided more than one time".to_string(), )); } } _ => { if params.insert(&name, content).is_some() { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Field '{}' provided more than one time", name ))); @@ -145,7 +145,7 @@ pub async fn handle_post_object( .ok_or_bad_request("Invalid expiration date")? .into(); if Utc::now() - expiration > Duration::zero() { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Expiration date is in the paste".to_string(), )); } @@ -159,7 +159,7 @@ pub async fn handle_post_object( "policy" | "x-amz-signature" => (), // this is always accepted, as it's required to validate other fields "content-type" => { let conds = conditions.params.remove("content-type").ok_or_else(|| { - Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key)) + Error::bad_request(format!("Key '{}' is not allowed in policy", param_key)) })?; for cond in conds { let ok = match cond { @@ -169,7 +169,7 @@ pub async fn handle_post_object( } }; if !ok { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Key '{}' has value not allowed in policy", param_key ))); @@ -178,7 +178,7 @@ pub async fn handle_post_object( } "key" => { let conds = conditions.params.remove("key").ok_or_else(|| { - Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key)) + Error::bad_request(format!("Key '{}' is not allowed in policy", param_key)) })?; for cond in conds { let ok = match cond { @@ -186,7 +186,7 @@ pub async fn handle_post_object( Operation::StartsWith(s) => key.starts_with(&s), }; if !ok { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Key '{}' has value not allowed in policy", param_key ))); @@ -201,7 +201,7 @@ pub async fn handle_post_object( continue; } let conds = conditions.params.remove(¶m_key).ok_or_else(|| { - Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key)) + Error::bad_request(format!("Key '{}' is not allowed in policy", param_key)) })?; for cond in conds { let ok = match cond { @@ -209,7 +209,7 @@ pub async fn handle_post_object( Operation::StartsWith(s) => value.to_str()?.starts_with(s.as_str()), }; if !ok { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Key '{}' has value not allowed in policy", param_key ))); @@ -220,7 +220,7 @@ pub async fn handle_post_object( } if let Some((param_key, _)) = conditions.params.iter().next() { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Key '{}' is required in policy, but no value was provided", param_key ))); @@ -326,7 +326,7 @@ impl Policy { match condition { PolicyCondition::Equal(map) => { if map.len() != 1 { - return Err(Error::BadRequest("Invalid policy item".to_owned())); + return Err(Error::bad_request("Invalid policy item".to_owned())); } let (mut k, v) = map.into_iter().next().expect("size was verified"); k.make_ascii_lowercase(); @@ -334,7 +334,7 @@ impl Policy { } PolicyCondition::OtherOp([cond, mut key, value]) => { if key.remove(0) != '$' { - return Err(Error::BadRequest("Invalid policy item".to_owned())); + return Err(Error::bad_request("Invalid policy item".to_owned())); } key.make_ascii_lowercase(); match cond.as_str() { @@ -347,7 +347,7 @@ impl Policy { .or_default() .push(Operation::StartsWith(value)); } - _ => return Err(Error::BadRequest("Invalid policy item".to_owned())), + _ => return Err(Error::bad_request("Invalid policy item".to_owned())), } } PolicyCondition::SizeRange(key, min, max) => { @@ -355,7 +355,7 @@ impl Policy { length.0 = length.0.max(min); length.1 = length.1.min(max); } else { - return Err(Error::BadRequest("Invalid policy item".to_owned())); + return Err(Error::bad_request("Invalid policy item".to_owned())); } } } @@ -420,14 +420,14 @@ where self.read += bytes.len() as u64; // optimization to fail early when we know before the end it's too long if self.length.end() < &self.read { - return Poll::Ready(Some(Err(Error::BadRequest( + return Poll::Ready(Some(Err(Error::bad_request( "File size does not match policy".to_owned(), )))); } } Poll::Ready(None) => { if !self.length.contains(&self.read) { - return Poll::Ready(Some(Err(Error::BadRequest( + return Poll::Ready(Some(Err(Error::bad_request( "File size does not match policy".to_owned(), )))); } diff --git a/src/api/s3/put.rs b/src/api/s3/put.rs index 89aa8d84..d50e32b0 100644 --- a/src/api/s3/put.rs +++ b/src/api/s3/put.rs @@ -183,7 +183,7 @@ fn ensure_checksum_matches( ) -> Result<(), Error> { if let Some(expected_sha256) = content_sha256 { if expected_sha256 != data_sha256sum { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Unable to validate x-amz-content-sha256".to_string(), )); } else { @@ -192,7 +192,7 @@ fn ensure_checksum_matches( } if let Some(expected_md5) = content_md5 { if expected_md5.trim_matches('"') != base64::encode(data_md5sum) { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Unable to validate content-md5".to_string(), )); } else { @@ -428,7 +428,7 @@ pub async fn handle_put_part( // Check part hasn't already been uploaded if let Some(v) = version { if v.has_part_number(part_number) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Part number {} has already been uploaded", part_number ))); @@ -513,7 +513,7 @@ pub async fn handle_complete_multipart_upload( let version = version.ok_or(Error::NoSuchKey)?; if version.blocks.is_empty() { - return Err(Error::BadRequest("No data was uploaded".to_string())); + return Err(Error::bad_request("No data was uploaded".to_string())); } let headers = match object_version.state { @@ -574,8 +574,8 @@ pub async fn handle_complete_multipart_upload( .map(|x| x.part_number) .eq(block_parts.into_iter()); if !same_parts { - return Err(Error::BadRequest( - "Part numbers in block list and part list do not match. This can happen if a part was partially uploaded. Please abort the multipart upload and try again.".into(), + return Err(Error::bad_request( + "Part numbers in block list and part list do not match. This can happen if a part was partially uploaded. Please abort the multipart upload and try again." )); } diff --git a/src/api/s3/router.rs b/src/api/s3/router.rs index 446ceb54..e920e162 100644 --- a/src/api/s3/router.rs +++ b/src/api/s3/router.rs @@ -342,7 +342,7 @@ impl Endpoint { Method::POST => Self::from_post(key, &mut query)?, Method::PUT => Self::from_put(key, &mut query, req.headers())?, Method::DELETE => Self::from_delete(key, &mut query)?, - _ => return Err(Error::BadRequest("Unknown method".to_owned())), + _ => return Err(Error::bad_request("Unknown method".to_owned())), }; if let Some(message) = query.nonempty_message() { diff --git a/src/api/s3/website.rs b/src/api/s3/website.rs index 561130dc..4fc7b7bb 100644 --- a/src/api/s3/website.rs +++ b/src/api/s3/website.rs @@ -176,7 +176,7 @@ impl WebsiteConfiguration { || self.index_document.is_some() || self.routing_rules.is_some()) { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Bad XML: can't have RedirectAllRequestsTo and other fields".to_owned(), )); } @@ -222,7 +222,7 @@ impl WebsiteConfiguration { impl Key { pub fn validate(&self) -> Result<(), Error> { if self.key.0.is_empty() { - Err(Error::BadRequest( + Err(Error::bad_request( "Bad XML: error document specified but empty".to_owned(), )) } else { @@ -234,7 +234,7 @@ impl Key { impl Suffix { pub fn validate(&self) -> Result<(), Error> { if self.suffix.0.is_empty() | self.suffix.0.contains('/') { - Err(Error::BadRequest( + Err(Error::bad_request( "Bad XML: index document is empty or contains /".to_owned(), )) } else { @@ -247,7 +247,7 @@ impl Target { pub fn validate(&self) -> Result<(), Error> { if let Some(ref protocol) = self.protocol { if protocol.0 != "http" && protocol.0 != "https" { - return Err(Error::BadRequest("Bad XML: invalid protocol".to_owned())); + return Err(Error::bad_request("Bad XML: invalid protocol".to_owned())); } } Ok(()) @@ -269,19 +269,19 @@ impl Redirect { pub fn validate(&self, has_prefix: bool) -> Result<(), Error> { if self.replace_prefix.is_some() { if self.replace_full.is_some() { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Bad XML: both ReplaceKeyPrefixWith and ReplaceKeyWith are set".to_owned(), )); } if !has_prefix { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Bad XML: ReplaceKeyPrefixWith is set, but KeyPrefixEquals isn't".to_owned(), )); } } if let Some(ref protocol) = self.protocol { if protocol.0 != "http" && protocol.0 != "https" { - return Err(Error::BadRequest("Bad XML: invalid protocol".to_owned())); + return Err(Error::bad_request("Bad XML: invalid protocol".to_owned())); } } // TODO there are probably more invalide cases, but which ones? diff --git a/src/api/signature/mod.rs b/src/api/signature/mod.rs index 5646f4fa..e3554080 100644 --- a/src/api/signature/mod.rs +++ b/src/api/signature/mod.rs @@ -16,7 +16,7 @@ type HmacSha256 = Hmac; pub fn verify_signed_content(expected_sha256: Hash, body: &[u8]) -> Result<(), Error> { if expected_sha256 != sha256sum(body) { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Request content hash does not match signed hash".to_string(), )); } diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 9137dd2d..52c4d401 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -105,7 +105,7 @@ fn parse_authorization( let (auth_kind, rest) = authorization.split_at(first_space); if auth_kind != "AWS4-HMAC-SHA256" { - return Err(Error::BadRequest("Unsupported authorization method".into())); + return Err(Error::bad_request("Unsupported authorization method")); } let mut auth_params = HashMap::new(); @@ -129,10 +129,11 @@ fn parse_authorization( let date = headers .get("x-amz-date") .ok_or_bad_request("Missing X-Amz-Date field") + .map_err(Error::from) .and_then(|d| parse_date(d))?; if Utc::now() - date > Duration::hours(24) { - return Err(Error::BadRequest("Date is too old".to_string())); + return Err(Error::bad_request("Date is too old".to_string())); } let auth = Authorization { @@ -156,7 +157,7 @@ fn parse_query_authorization( headers: &HashMap, ) -> Result { if algorithm != "AWS4-HMAC-SHA256" { - return Err(Error::BadRequest( + return Err(Error::bad_request( "Unsupported authorization method".to_string(), )); } @@ -179,10 +180,10 @@ fn parse_query_authorization( .get("x-amz-expires") .ok_or_bad_request("X-Amz-Expires not found in query parameters")? .parse() - .map_err(|_| Error::BadRequest("X-Amz-Expires is not a number".to_string()))?; + .map_err(|_| Error::bad_request("X-Amz-Expires is not a number".to_string()))?; if duration > 7 * 24 * 3600 { - return Err(Error::BadRequest( + return Err(Error::bad_request( "X-Amz-Exprires may not exceed a week".to_string(), )); } @@ -190,10 +191,11 @@ fn parse_query_authorization( let date = headers .get("x-amz-date") .ok_or_bad_request("Missing X-Amz-Date field") + .map_err(Error::from) .and_then(|d| parse_date(d))?; if Utc::now() - date > Duration::seconds(duration) { - return Err(Error::BadRequest("Date is too old".to_string())); + return Err(Error::bad_request("Date is too old".to_string())); } Ok(Authorization { diff --git a/src/api/signature/streaming.rs b/src/api/signature/streaming.rs index ded9d993..6c326c54 100644 --- a/src/api/signature/streaming.rs +++ b/src/api/signature/streaming.rs @@ -87,7 +87,7 @@ fn compute_streaming_payload_signature( let mut hmac = signing_hmac.clone(); hmac.update(string_to_sign.as_bytes()); - Hash::try_from(&hmac.finalize().into_bytes()).ok_or_internal_error("Invalid signature") + Ok(Hash::try_from(&hmac.finalize().into_bytes()).ok_or_internal_error("Invalid signature")?) } mod payload { @@ -163,10 +163,10 @@ impl From for Error { match err { SignedPayloadStreamError::Stream(e) => e, SignedPayloadStreamError::InvalidSignature => { - Error::BadRequest("Invalid payload signature".into()) + Error::bad_request("Invalid payload signature") } SignedPayloadStreamError::Message(e) => { - Error::BadRequest(format!("Chunk format error: {}", e)) + Error::bad_request(format!("Chunk format error: {}", e)) } } } diff --git a/src/web/error.rs b/src/web/error.rs index e9ed5314..478731b5 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -3,50 +3,39 @@ use hyper::header::HeaderValue; use hyper::{HeaderMap, StatusCode}; use garage_api::generic_server::ApiError; -use garage_util::error::Error as GarageError; /// Errors of this crate #[derive(Debug, Error)] pub enum Error { /// An error received from the API crate #[error(display = "API error: {}", _0)] - ApiError(#[error(source)] garage_api::Error), - - // Category: internal error - /// Error internal to garage - #[error(display = "Internal error: {}", _0)] - InternalError(#[error(source)] GarageError), + ApiError(garage_api::Error), /// The file does not exist #[error(display = "Not found")] NotFound, - /// The request contained an invalid UTF-8 sequence in its path or in other parameters - #[error(display = "Invalid UTF-8: {}", _0)] - InvalidUtf8(#[error(source)] std::str::Utf8Error), - - /// The client send a header with invalid value - #[error(display = "Invalid header value: {}", _0)] - InvalidHeader(#[error(source)] hyper::header::ToStrError), - /// The client sent a request without host, or with unsupported method #[error(display = "Bad request: {}", _0)] BadRequest(String), } +impl From for Error +where + garage_api::Error: From, +{ + fn from(err: T) -> Self { + Error::ApiError(garage_api::Error::from(err)) + } +} + impl Error { /// Transform errors into http status code pub fn http_status_code(&self) -> StatusCode { match self { Error::NotFound => StatusCode::NOT_FOUND, Error::ApiError(e) => e.http_status_code(), - Error::InternalError( - GarageError::Timeout - | GarageError::RemoteError(_) - | GarageError::Quorum(_, _, _, _), - ) => StatusCode::SERVICE_UNAVAILABLE, - Error::InternalError(_) => StatusCode::INTERNAL_SERVER_ERROR, - _ => StatusCode::BAD_REQUEST, + Error::BadRequest(_) => StatusCode::BAD_REQUEST, } } diff --git a/src/web/web_server.rs b/src/web/web_server.rs index 867adc51..e83bc4cb 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -207,7 +207,7 @@ async fn serve_file(garage: Arc, req: &Request) -> Result handle_options_for_bucket(req, &bucket), Method::HEAD => handle_head(garage.clone(), req, bucket_id, &key, None).await, Method::GET => handle_get(garage.clone(), req, bucket_id, &key, None).await, - _ => Err(ApiError::BadRequest("HTTP method not supported".into())), + _ => Err(ApiError::bad_request("HTTP method not supported")), } .map_err(Error::from); @@ -290,9 +290,7 @@ fn path_to_key<'a>(path: &'a str, index: &str) -> Result, Error> { let path_utf8 = percent_encoding::percent_decode_str(path).decode_utf8()?; if !path_utf8.starts_with('/') { - return Err(Error::BadRequest( - "Path must start with a / (slash)".to_string(), - )); + return Err(Error::BadRequest("Path must start with a / (slash)".into())); } match path_utf8.chars().last() {