More error refactoring

author: Alex Auvolat <alex@adnab.me> 2022-05-13 19:18:51 +0200
committer: Alex Auvolat <alex@adnab.me> 2022-05-13 19:18:51 +0200
commit: ea325d78d36d19f59a0849ace1f4567e2b095bd7 (patch)
tree: bfc05dd1f8df3d1fae84a1af433f4ae56dbc2c31
parent: ec16d166f940f59098ae5cc0c0b3d8298f1bcc78 (diff)
download: garage-ea325d78d36d19f59a0849ace1f4567e2b095bd7.tar.gz
garage-ea325d78d36d19f59a0849ace1f4567e2b095bd7.zip
22 files changed, 209 insertions, 259 deletions
diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs
index b2effa62..098a54aa 100644
--- a/src/api/admin/api_server.rs
+++ b/src/api/admin/api_server.rs
@@ -114,13 +114,9 @@ impl ApiHandler for AdminApiServer {
 
 		if let Some(h) = expected_auth_header {
 			match req.headers().get("Authorization") {
-				None => Err(Error::Forbidden(
-					"Authorization token must be provided".into(),
-				)),
+				None => Err(Error::forbidden("Authorization token must be provided")),
 				Some(v) if v.to_str().map(|hv| hv == h).unwrap_or(false) => Ok(()),
-				_ => Err(Error::Forbidden(
-					"Invalid authorization token provided".into(),
-				)),
+				_ => Err(Error::forbidden("Invalid authorization token provided")),
 			}?;
 		}
 
diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs
index b226c015..c5518e4e 100644
--- a/src/api/admin/bucket.rs
+++ b/src/api/admin/bucket.rs
@@ -18,6 +18,7 @@ use garage_model::s3::object_table::ObjectFilter;
 
 use crate::admin::error::*;
 use crate::admin::key::ApiBucketKeyPerm;
+use crate::common_error::CommonError;
 use crate::helpers::parse_json_body;
 
 pub async fn handle_list_buckets(garage: &Arc<Garage>) -> Result<Response<Body>, Error> {
@@ -233,7 +234,7 @@ pub async fn handle_create_bucket(
 
 		if let Some(alias) = garage.bucket_alias_table.get(&EmptyKey, ga).await? {
 			if alias.state.get().is_some() {
-				return Err(Error::BucketAlreadyExists);
+				return Err(CommonError::BucketAlreadyExists.into());
 			}
 		}
 	}
@@ -333,7 +334,7 @@ pub async fn handle_delete_bucket(
 		)
 		.await?;
 	if !objects.is_empty() {
-		return Err(Error::BucketNotEmpty);
+		return Err(CommonError::BucketNotEmpty.into());
 	}
 
 	// --- done checking, now commit ---
diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs
index 1d68dc69..bb35c16b 100644
--- a/src/api/admin/error.rs
+++ b/src/api/admin/error.rs
@@ -4,9 +4,9 @@ use hyper::{Body, HeaderMap, StatusCode};
 
 use garage_model::helper::error::Error as HelperError;
 
-use crate::generic_server::ApiError;
 use crate::common_error::CommonError;
-pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
+pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
+use crate::generic_server::ApiError;
 
 /// Errors of this crate
 #[derive(Debug, Error)]
@@ -16,30 +16,9 @@ pub enum Error {
 	CommonError(CommonError),
 
 	// Category: cannot process
-	/// No proper api key was used, or the signature was invalid
-	#[error(display = "Forbidden: {}", _0)]
-	Forbidden(String),
-
 	/// The API access key does not exist
 	#[error(display = "Access key not found")]
 	NoSuchAccessKey,
-
-	/// The bucket requested don't exists
-	#[error(display = "Bucket not found")]
-	NoSuchBucket,
-
-	/// Tried to create a bucket that already exist
-	#[error(display = "Bucket already exists")]
-	BucketAlreadyExists,
-
-	/// Tried to delete a non-empty bucket
-	#[error(display = "Tried to delete a non-empty bucket")]
-	BucketNotEmpty,
-
-	// Category: bad request
-	/// Bucket name is not valid according to AWS S3 specs
-	#[error(display = "Invalid bucket name")]
-	InvalidBucketName,
 }
 
 impl<T> From<T> for Error
@@ -51,14 +30,16 @@ where
 	}
 }
 
+impl CommonErrorDerivative for Error {}
+
 impl From<HelperError> for Error {
 	fn from(err: HelperError) -> Self {
 		match err {
 			HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)),
 			HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)),
-			HelperError::InvalidBucketName(_) => Self::InvalidBucketName,
+			HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName),
+			HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket),
 			HelperError::NoSuchAccessKey(_) => Self::NoSuchAccessKey,
-			HelperError::NoSuchBucket(_) => Self::NoSuchBucket,
 		}
 	}
 }
@@ -68,10 +49,7 @@ impl ApiError for Error {
 	fn http_status_code(&self) -> StatusCode {
 		match self {
 			Error::CommonError(c) => c.http_status_code(),
-			Error::NoSuchAccessKey | Error::NoSuchBucket => StatusCode::NOT_FOUND,
-			Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT,
-			Error::Forbidden(_) => StatusCode::FORBIDDEN,
-			Error::InvalidBucketName => StatusCode::BAD_REQUEST,
+			Error::NoSuchAccessKey => StatusCode::NOT_FOUND,
 		}
 	}
 
@@ -80,15 +58,10 @@ impl ApiError for Error {
 	}
 
 	fn http_body(&self, garage_region: &str, path: &str) -> Body {
+		// TODO nice json error
 		Body::from(format!(
 			"ERROR: {}\n\ngarage region: {}\npath: {}",
 			self, garage_region, path
 		))
 	}
 }
-
-impl Error {
-	pub fn bad_request<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::BadRequest(msg.to_string()))
-	}
-}
diff --git a/src/api/common_error.rs b/src/api/common_error.rs
index 48106e03..b6dbf059 100644
--- a/src/api/common_error.rs
+++ b/src/api/common_error.rs
@@ -6,7 +6,7 @@ use garage_util::error::Error as GarageError;
 /// Errors of this crate
 #[derive(Debug, Error)]
 pub enum CommonError {
-	// Category: internal error
+	// ---- INTERNAL ERRORS ----
 	/// Error related to deeper parts of Garage
 	#[error(display = "Internal error: {}", _0)]
 	InternalError(#[error(source)] GarageError),
@@ -19,9 +19,34 @@ pub enum CommonError {
 	#[error(display = "Internal error (HTTP error): {}", _0)]
 	Http(#[error(source)] http::Error),
 
-	/// The client sent an invalid request
+	// ---- GENERIC CLIENT ERRORS ----
+	/// Proper authentication was not provided
+	#[error(display = "Forbidden: {}", _0)]
+	Forbidden(String),
+
+	/// Generic bad request response with custom message
 	#[error(display = "Bad request: {}", _0)]
 	BadRequest(String),
+
+	// ---- SPECIFIC ERROR CONDITIONS ----
+	// These have to be error codes referenced in the S3 spec here:
+	// https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList
+	/// The bucket requested don't exists
+	#[error(display = "Bucket not found")]
+	NoSuchBucket,
+
+	/// Tried to create a bucket that already exist
+	#[error(display = "Bucket already exists")]
+	BucketAlreadyExists,
+
+	/// Tried to delete a non-empty bucket
+	#[error(display = "Tried to delete a non-empty bucket")]
+	BucketNotEmpty,
+
+	// Category: bad request
+	/// Bucket name is not valid according to AWS S3 specs
+	#[error(display = "Invalid bucket name")]
+	InvalidBucketName,
 }
 
 impl CommonError {
@@ -36,15 +61,53 @@ impl CommonError {
 				StatusCode::INTERNAL_SERVER_ERROR
 			}
 			CommonError::BadRequest(_) => StatusCode::BAD_REQUEST,
+			CommonError::Forbidden(_) => StatusCode::FORBIDDEN,
+			CommonError::NoSuchBucket => StatusCode::NOT_FOUND,
+			CommonError::BucketNotEmpty | CommonError::BucketAlreadyExists => StatusCode::CONFLICT,
+			CommonError::InvalidBucketName => StatusCode::BAD_REQUEST,
 		}
 	}
 
+	pub fn aws_code(&self) -> &'static str {
+		match self {
+			CommonError::Forbidden(_) => "AccessDenied",
+			CommonError::InternalError(
+				GarageError::Timeout
+				| GarageError::RemoteError(_)
+				| GarageError::Quorum(_, _, _, _),
+			) => "ServiceUnavailable",
+			CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => {
+				"InternalError"
+			}
+			CommonError::BadRequest(_) => "InvalidRequest",
+			CommonError::NoSuchBucket => "NoSuchBucket",
+			CommonError::BucketAlreadyExists => "BucketAlreadyExists",
+			CommonError::BucketNotEmpty => "BucketNotEmpty",
+			CommonError::InvalidBucketName => "InvalidBucketName",
+		}
+	}
 
 	pub fn bad_request<M: ToString>(msg: M) -> Self {
 		CommonError::BadRequest(msg.to_string())
 	}
 }
 
+pub trait CommonErrorDerivative: From<CommonError> {
+	fn internal_error<M: ToString>(msg: M) -> Self {
+		Self::from(CommonError::InternalError(GarageError::Message(
+			msg.to_string(),
+		)))
+	}
+
+	fn bad_request<M: ToString>(msg: M) -> Self {
+		Self::from(CommonError::BadRequest(msg.to_string()))
+	}
+
+	fn forbidden<M: ToString>(msg: M) -> Self {
+		Self::from(CommonError::Forbidden(msg.to_string()))
+	}
+}
+
 /// Trait to map error to the Bad Request error code
 pub trait OkOrBadRequest {
 	type S;
diff --git a/src/api/helpers.rs b/src/api/helpers.rs
index 599be3f7..aa350e3c 100644
--- a/src/api/helpers.rs
+++ b/src/api/helpers.rs
@@ -2,7 +2,7 @@ use hyper::{Body, Request};
 use idna::domain_to_unicode;
 use serde::Deserialize;
 
-use crate::common_error::{*, CommonError as Error};
+use crate::common_error::{CommonError as Error, *};
 
 /// What kind of authorization is required to perform a given action
 #[derive(Debug, Clone, PartialEq, Eq)]
diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs
index b70fcdff..eb0fbdd7 100644
--- a/src/api/k2v/api_server.rs
+++ b/src/api/k2v/api_server.rs
@@ -7,13 +7,12 @@ use hyper::{Body, Method, Request, Response};
 
 use opentelemetry::{trace::SpanRef, KeyValue};
 
-use garage_table::util::*;
 use garage_util::error::Error as GarageError;
 
 use garage_model::garage::Garage;
 
-use crate::k2v::error::*;
 use crate::generic_server::*;
+use crate::k2v::error::*;
 
 use crate::signature::payload::check_payload_signature;
 use crate::signature::streaming::*;
@@ -84,14 +83,14 @@ impl ApiHandler for K2VApiServer {
 
 		// The OPTIONS method is procesed early, before we even check for an API key
 		if let Endpoint::Options = endpoint {
-			return Ok(handle_options_s3api(garage, &req, Some(bucket_name)).await
+			return Ok(handle_options_s3api(garage, &req, Some(bucket_name))
+				.await
 				.ok_or_bad_request("Error handling OPTIONS")?);
 		}
 
 		let (api_key, mut content_sha256) = check_payload_signature(&garage, "k2v", &req).await?;
-		let api_key = api_key.ok_or_else(|| {
-			Error::Forbidden("Garage does not support anonymous access yet".to_string())
-		})?;
+		let api_key = api_key
+			.ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?;
 
 		let req = parse_streaming_body(
 			&api_key,
@@ -101,13 +100,14 @@ impl ApiHandler for K2VApiServer {
 			"k2v",
 		)?;
 
-		let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?;
+		let bucket_id = garage
+			.bucket_helper()
+			.resolve_bucket(&bucket_name, &api_key)
+			.await?;
 		let bucket = garage
-			.bucket_table
-			.get(&EmptyKey, &bucket_id)
-			.await?
-			.filter(|b| !b.state.is_deleted())
-			.ok_or(Error::NoSuchBucket)?;
+			.bucket_helper()
+			.get_existing_bucket(bucket_id)
+			.await?;
 
 		let allowed = match endpoint.authorization_type() {
 			Authorization::Read => api_key.allow_read(&bucket_id),
@@ -117,9 +117,7 @@ impl ApiHandler for K2VApiServer {
 		};
 
 		if !allowed {
-			return Err(Error::Forbidden(
-				"Operation is not allowed for this key.".to_string(),
-			));
+			return Err(Error::forbidden("Operation is not allowed for this key."));
 		}
 
 		// Look up what CORS rule might apply to response.
diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs
index 8eae471c..db9901cf 100644
--- a/src/api/k2v/batch.rs
+++ b/src/api/k2v/batch.rs
@@ -12,8 +12,8 @@ use garage_model::garage::Garage;
 use garage_model::k2v::causality::*;
 use garage_model::k2v::item_table::*;
 
-use crate::k2v::error::*;
 use crate::helpers::*;
+use crate::k2v::error::*;
 use crate::k2v::range::read_range;
 
 pub async fn handle_insert_batch(
diff --git a/src/api/k2v/error.rs b/src/api/k2v/error.rs
index 6b9e81e6..4d8c1154 100644
--- a/src/api/k2v/error.rs
+++ b/src/api/k2v/error.rs
@@ -5,7 +5,7 @@ use hyper::{Body, HeaderMap, StatusCode};
 use garage_model::helper::error::Error as HelperError;
 
 use crate::common_error::CommonError;
-pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
+pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
 use crate::generic_server::ApiError;
 use crate::signature::error::Error as SignatureError;
 
@@ -17,10 +17,6 @@ pub enum Error {
 	CommonError(CommonError),
 
 	// Category: cannot process
-	/// No proper api key was used, or the signature was invalid
-	#[error(display = "Forbidden: {}", _0)]
-	Forbidden(String),
-
 	/// Authorization Header Malformed
 	#[error(display = "Authorization header malformed, expected scope: {}", _0)]
 	AuthorizationHeaderMalformed(String),
@@ -29,10 +25,6 @@ pub enum Error {
 	#[error(display = "Key not found")]
 	NoSuchKey,
 
-	/// The bucket requested don't exists
-	#[error(display = "Bucket not found")]
-	NoSuchBucket,
-
 	/// Some base64 encoded data was badly encoded
 	#[error(display = "Invalid base64: {}", _0)]
 	InvalidBase64(#[error(source)] base64::DecodeError),
@@ -59,11 +51,15 @@ where
 	}
 }
 
+impl CommonErrorDerivative for Error {}
+
 impl From<HelperError> for Error {
 	fn from(err: HelperError) -> Self {
 		match err {
 			HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)),
 			HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)),
+			HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName),
+			HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket),
 			e => Self::CommonError(CommonError::BadRequest(format!("{}", e))),
 		}
 	}
@@ -73,35 +69,26 @@ impl From<SignatureError> for Error {
 	fn from(err: SignatureError) -> Self {
 		match err {
 			SignatureError::CommonError(c) => Self::CommonError(c),
-			SignatureError::AuthorizationHeaderMalformed(c) => Self::AuthorizationHeaderMalformed(c),
-			SignatureError::Forbidden(f) => Self::Forbidden(f),
+			SignatureError::AuthorizationHeaderMalformed(c) => {
+				Self::AuthorizationHeaderMalformed(c)
+			}
 			SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i),
 			SignatureError::InvalidHeader(h) => Self::InvalidHeader(h),
 		}
 	}
 }
 
-impl Error {
-	//pub fn internal_error<M: ToString>(msg: M) -> Self {
-	//	Self::CommonError(CommonError::InternalError(GarageError::Message(
-	//		msg.to_string(),
-	//	)))
-	//}
-
-	pub fn bad_request<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::BadRequest(msg.to_string()))
-	}
-}
-
 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 => StatusCode::NOT_FOUND,
-			Error::Forbidden(_) => StatusCode::FORBIDDEN,
+			Error::NoSuchKey => StatusCode::NOT_FOUND,
 			Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE,
-			_ => StatusCode::BAD_REQUEST,
+			Error::AuthorizationHeaderMalformed(_)
+			| Error::InvalidBase64(_)
+			| Error::InvalidHeader(_)
+			| Error::InvalidUtf8Str(_) => StatusCode::BAD_REQUEST,
 		}
 	}
 
@@ -110,6 +97,7 @@ impl ApiError for Error {
 	}
 
 	fn http_body(&self, garage_region: &str, path: &str) -> Body {
+		// TODO nice json error
 		Body::from(format!(
 			"ERROR: {}\n\ngarage region: {}\npath: {}",
 			self, garage_region, path
diff --git a/src/api/k2v/range.rs b/src/api/k2v/range.rs
index 6aa5c90c..1f7dc4cd 100644
--- a/src/api/k2v/range.rs
+++ b/src/api/k2v/range.rs
@@ -7,8 +7,8 @@ use std::sync::Arc;
 use garage_table::replication::TableShardedReplication;
 use garage_table::*;
 
-use crate::k2v::error::*;
 use crate::helpers::key_after_prefix;
+use crate::k2v::error::*;
 
 /// Read range in a Garage table.
 /// Returns (entries, more?, nextStart)
diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs
index 4df9ee6d..87d0f288 100644
--- a/src/api/s3/api_server.rs
+++ b/src/api/s3/api_server.rs
@@ -8,14 +8,13 @@ use hyper::{Body, Method, Request, Response};
 
 use opentelemetry::{trace::SpanRef, KeyValue};
 
-use garage_table::util::*;
 use garage_util::error::Error as GarageError;
 
 use garage_model::garage::Garage;
 use garage_model::key_table::Key;
 
-use crate::s3::error::*;
 use crate::generic_server::*;
+use crate::s3::error::*;
 
 use crate::signature::payload::check_payload_signature;
 use crate::signature::streaming::*;
@@ -119,14 +118,14 @@ impl ApiHandler for S3ApiServer {
 			return handle_post_object(garage, req, bucket_name.unwrap()).await;
 		}
 		if let Endpoint::Options = endpoint {
-			return handle_options_s3api(garage, &req, bucket_name).await
+			return handle_options_s3api(garage, &req, bucket_name)
+				.await
 				.map_err(Error::from);
 		}
 
 		let (api_key, mut content_sha256) = check_payload_signature(&garage, "s3", &req).await?;
-		let api_key = api_key.ok_or_else(|| {
-			Error::Forbidden("Garage does not support anonymous access yet".to_string())
-		})?;
+		let api_key = api_key
+			.ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?;
 
 		let req = parse_streaming_body(
 			&api_key,
@@ -150,13 +149,14 @@ impl ApiHandler for S3ApiServer {
 			return handle_create_bucket(&garage, req, content_sha256, api_key, bucket_name).await;
 		}
 
-		let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?;
+		let bucket_id = garage
+			.bucket_helper()
+			.resolve_bucket(&bucket_name, &api_key)
+			.await?;
 		let bucket = garage
-			.bucket_table
-			.get(&EmptyKey, &bucket_id)
-			.await?
-			.filter(|b| !b.state.is_deleted())
-			.ok_or(Error::NoSuchBucket)?;
+			.bucket_helper()
+			.get_existing_bucket(bucket_id)
+			.await?;
 
 		let allowed = match endpoint.authorization_type() {
 			Authorization::Read => api_key.allow_read(&bucket_id),
@@ -166,9 +166,7 @@ impl ApiHandler for S3ApiServer {
 		};
 
 		if !allowed {
-			return Err(Error::Forbidden(
-				"Operation is not allowed for this key.".to_string(),
-			));
+			return Err(Error::forbidden("Operation is not allowed for this key."));
 		}
 
 		// Look up what CORS rule might apply to response.
diff --git a/src/api/s3/bucket.rs b/src/api/s3/bucket.rs
index d4a6b0cb..1304cc07 100644
--- a/src/api/s3/bucket.rs
+++ b/src/api/s3/bucket.rs
@@ -14,6 +14,7 @@ use garage_util::crdt::*;
 use garage_util::data::*;
 use garage_util::time::*;
 
+use crate::common_error::CommonError;
 use crate::s3::error::*;
 use crate::s3::xml as s3_xml;
 use crate::signature::verify_signed_content;
@@ -158,7 +159,7 @@ pub async fn handle_create_bucket(
 		// otherwise return a forbidden error.
 		let kp = api_key.bucket_permissions(&bucket_id);
 		if !(kp.allow_write || kp.allow_owner) {
-			return Err(Error::BucketAlreadyExists);
+			return Err(CommonError::BucketAlreadyExists.into());
 		}
 	} else {
 		// Create the bucket!
@@ -239,7 +240,7 @@ pub async fn handle_delete_bucket(
 			)
 			.await?;
 		if !objects.is_empty() {
-			return Err(Error::BucketNotEmpty);
+			return Err(CommonError::BucketNotEmpty.into());
 		}
 
 		// --- done checking, now commit ---
diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs
index 2468678e..0fc16993 100644
--- a/src/api/s3/copy.rs
+++ b/src/api/s3/copy.rs
@@ -18,8 +18,8 @@ use garage_model::s3::block_ref_table::*;
 use garage_model::s3::object_table::*;
 use garage_model::s3::version_table::*;
 
+use crate::helpers::parse_bucket_key;
 use crate::s3::error::*;
-use crate::helpers::{parse_bucket_key};
 use crate::s3::put::{decode_upload_id, get_headers};
 use crate::s3::xml::{self as s3_xml, xmlns_tag};
 
@@ -413,10 +413,13 @@ async fn get_copy_source(
 	let copy_source = percent_encoding::percent_decode_str(copy_source).decode_utf8()?;
 
 	let (source_bucket, source_key) = parse_bucket_key(&copy_source, None)?;
-	let source_bucket_id = garage.bucket_helper().resolve_bucket(&source_bucket.to_string(), api_key).await?;
+	let source_bucket_id = garage
+		.bucket_helper()
+		.resolve_bucket(&source_bucket.to_string(), api_key)
+		.await?;
 
 	if !api_key.allow_read(&source_bucket_id) {
-		return Err(Error::Forbidden(format!(
+		return Err(Error::forbidden(format!(
 			"Reading from bucket {} not allowed for this key",
 			source_bucket
 		)));
diff --git a/src/api/s3/cors.rs b/src/api/s3/cors.rs
index 1ad4f2f8..c7273464 100644
--- a/src/api/s3/cors.rs
+++ b/src/api/s3/cors.rs
@@ -15,7 +15,6 @@ use crate::signature::verify_signed_content;
 
 use garage_model::bucket_table::{Bucket, CorsRule as GarageCorsRule};
 use garage_model::garage::Garage;
-use garage_table::*;
 use garage_util::data::*;
 
 pub async fn handle_get_cors(bucket: &Bucket) -> Result<Response<Body>, Error> {
@@ -48,14 +47,11 @@ pub async fn handle_delete_cors(
 	bucket_id: Uuid,
 ) -> Result<Response<Body>, Error> {
 	let mut bucket = garage
-		.bucket_table
-		.get(&EmptyKey, &bucket_id)
-		.await?
-		.ok_or(Error::NoSuchBucket)?;
+		.bucket_helper()
+		.get_existing_bucket(bucket_id)
+		.await?;
 
-	let param = bucket
-		.params_mut()
-		.ok_or_internal_error("Bucket should not be deleted at this point")?;
+	let param = bucket.params_mut().unwrap();
 
 	param.cors_config.update(None);
 	garage.bucket_table.insert(&bucket).await?;
@@ -78,14 +74,11 @@ pub async fn handle_put_cors(
 	}
 
 	let mut bucket = garage
-		.bucket_table
-		.get(&EmptyKey, &bucket_id)
-		.await?
-		.ok_or(Error::NoSuchBucket)?;
+		.bucket_helper()
+		.get_existing_bucket(bucket_id)
+		.await?;
 
-	let param = bucket
-		.params_mut()
-		.ok_or_internal_error("Bucket should not be deleted at this point")?;
+	let param = bucket.params_mut().unwrap();
 
 	let conf: CorsConfiguration = from_reader(&body as &[u8])?;
 	conf.validate()?;
@@ -119,12 +112,7 @@ pub async fn handle_options_s3api(
 		let helper = garage.bucket_helper();
 		let bucket_id = helper.resolve_global_bucket_name(&bn).await?;
 		if let Some(id) = bucket_id {
-			let bucket = garage
-				.bucket_table
-				.get(&EmptyKey, &id)
-				.await?
-				.filter(|b| !b.state.is_deleted())
-				.ok_or(Error::NoSuchBucket)?;
+			let bucket = garage.bucket_helper().get_existing_bucket(id).await?;
 			handle_options_for_bucket(req, &bucket)
 		} else {
 			// If there is a bucket name in the request, but that name
@@ -185,7 +173,7 @@ pub fn handle_options_for_bucket(
 		}
 	}
 
-	Err(Error::Forbidden("This CORS request is not allowed.".into()))
+	Err(Error::forbidden("This CORS request is not allowed."))
 }
 
 pub fn find_matching_cors_rule<'a>(
diff --git a/src/api/s3/error.rs b/src/api/s3/error.rs
index a0c4703c..4edff3a1 100644
--- a/src/api/s3/error.rs
+++ b/src/api/s3/error.rs
@@ -5,10 +5,9 @@ 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::common_error::CommonError;
-pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
+pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
 use crate::generic_server::ApiError;
 use crate::s3::xml as s3_xml;
 use crate::signature::error::Error as SignatureError;
@@ -21,10 +20,6 @@ pub enum Error {
 	CommonError(CommonError),
 
 	// Category: cannot process
-	/// No proper api key was used, or the signature was invalid
-	#[error(display = "Forbidden: {}", _0)]
-	Forbidden(String),
-
 	/// Authorization Header Malformed
 	#[error(display = "Authorization header malformed, expected scope: {}", _0)]
 	AuthorizationHeaderMalformed(String),
@@ -33,22 +28,10 @@ pub enum Error {
 	#[error(display = "Key not found")]
 	NoSuchKey,
 
-	/// The bucket requested don't exists
-	#[error(display = "Bucket not found")]
-	NoSuchBucket,
-
 	/// The multipart upload requested don't exists
 	#[error(display = "Upload not found")]
 	NoSuchUpload,
 
-	/// Tried to create a bucket that already exist
-	#[error(display = "Bucket already exists")]
-	BucketAlreadyExists,
-
-	/// Tried to delete a non-empty bucket
-	#[error(display = "Tried to delete a non-empty bucket")]
-	BucketNotEmpty,
-
 	/// Precondition failed (e.g. x-amz-copy-source-if-match)
 	#[error(display = "At least one of the preconditions you specified did not hold")]
 	PreconditionFailed,
@@ -75,14 +58,6 @@ pub enum Error {
 	#[error(display = "Invalid UTF-8: {}", _0)]
 	InvalidUtf8String(#[error(source)] std::string::FromUtf8Error),
 
-	/// Some base64 encoded data was badly encoded
-	#[error(display = "Invalid base64: {}", _0)]
-	InvalidBase64(#[error(source)] base64::DecodeError),
-
-	/// Bucket name is not valid according to AWS S3 specs
-	#[error(display = "Invalid bucket name")]
-	InvalidBucketName,
-
 	/// The client sent invalid XML data
 	#[error(display = "Invalid XML: {}", _0)]
 	InvalidXml(String),
@@ -95,10 +70,6 @@ pub enum Error {
 	#[error(display = "Invalid HTTP range: {:?}", _0)]
 	InvalidRange(#[error(from)] (http_range::HttpRangeParseError, u64)),
 
-	/// The client asked for an invalid return format (invalid Accept header)
-	#[error(display = "Not acceptable: {}", _0)]
-	NotAcceptable(String),
-
 	/// The client sent a request for an action not supported by garage
 	#[error(display = "Unimplemented action: {}", _0)]
 	NotImplemented(String),
@@ -113,6 +84,20 @@ where
 	}
 }
 
+impl CommonErrorDerivative for Error {}
+
+impl From<HelperError> for Error {
+	fn from(err: HelperError) -> Self {
+		match err {
+			HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)),
+			HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)),
+			HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName),
+			HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket),
+			e => Self::bad_request(format!("{}", e)),
+		}
+	}
+}
+
 impl From<roxmltree::Error> for Error {
 	fn from(err: roxmltree::Error) -> Self {
 		Self::InvalidXml(format!("{}", err))
@@ -125,22 +110,13 @@ impl From<quick_xml::de::DeError> for Error {
 	}
 }
 
-impl From<HelperError> for Error {
-	fn from(err: HelperError) -> Self {
-		match err {
-			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<SignatureError> for Error {
 	fn from(err: SignatureError) -> Self {
 		match err {
 			SignatureError::CommonError(c) => Self::CommonError(c),
-			SignatureError::AuthorizationHeaderMalformed(c) => Self::AuthorizationHeaderMalformed(c),
-			SignatureError::Forbidden(f) => Self::Forbidden(f),
+			SignatureError::AuthorizationHeaderMalformed(c) => {
+				Self::AuthorizationHeaderMalformed(c)
+			}
 			SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i),
 			SignatureError::InvalidHeader(h) => Self::InvalidHeader(h),
 		}
@@ -156,39 +132,22 @@ impl From<multer::Error> for Error {
 impl Error {
 	pub fn aws_code(&self) -> &'static str {
 		match self {
+			Error::CommonError(c) => c.aws_code(),
 			Error::NoSuchKey => "NoSuchKey",
-			Error::NoSuchBucket => "NoSuchBucket",
 			Error::NoSuchUpload => "NoSuchUpload",
-			Error::BucketAlreadyExists => "BucketAlreadyExists",
-			Error::BucketNotEmpty => "BucketNotEmpty",
 			Error::PreconditionFailed => "PreconditionFailed",
 			Error::InvalidPart => "InvalidPart",
 			Error::InvalidPartOrder => "InvalidPartOrder",
 			Error::EntityTooSmall => "EntityTooSmall",
-			Error::Forbidden(_) => "AccessDenied",
 			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
 			Error::NotImplemented(_) => "NotImplemented",
-			Error::CommonError(CommonError::InternalError(
-				GarageError::Timeout
-				| GarageError::RemoteError(_)
-				| GarageError::Quorum(_, _, _, _),
-			)) => "ServiceUnavailable",
-			Error::CommonError(
-				CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_),
-			) => "InternalError",
-			_ => "InvalidRequest",
+			Error::InvalidXml(_) => "MalformedXML",
+			Error::InvalidRange(_) => "InvalidRange",
+			Error::InvalidUtf8Str(_) | Error::InvalidUtf8String(_) | Error::InvalidHeader(_) => {
+				"InvalidRequest"
+			}
 		}
 	}
-
-	pub fn internal_error<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::InternalError(GarageError::Message(
-			msg.to_string(),
-		)))
-	}
-
-	pub fn bad_request<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::BadRequest(msg.to_string()))
-	}
 }
 
 impl ApiError for Error {
@@ -196,14 +155,18 @@ impl ApiError for Error {
 	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::NoSuchKey | Error::NoSuchUpload => StatusCode::NOT_FOUND,
 			Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED,
-			Error::Forbidden(_) => StatusCode::FORBIDDEN,
-			Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE,
 			Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE,
 			Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED,
-			_ => StatusCode::BAD_REQUEST,
+			Error::AuthorizationHeaderMalformed(_)
+			| Error::InvalidPart
+			| Error::InvalidPartOrder
+			| Error::EntityTooSmall
+			| Error::InvalidXml(_)
+			| Error::InvalidUtf8Str(_)
+			| Error::InvalidUtf8String(_)
+			| Error::InvalidHeader(_) => StatusCode::BAD_REQUEST,
 		}
 	}
 
diff --git a/src/api/s3/list.rs b/src/api/s3/list.rs
index b4ba5bcd..12f6149d 100644
--- a/src/api/s3/list.rs
+++ b/src/api/s3/list.rs
@@ -16,8 +16,8 @@ use garage_model::s3::version_table::Version;
 use garage_table::{EmptyKey, EnumerationOrder};
 
 use crate::encoding::*;
-use crate::s3::error::*;
 use crate::helpers::key_after_prefix;
+use crate::s3::error::*;
 use crate::s3::put as s3_put;
 use crate::s3::xml as s3_xml;
 
@@ -582,11 +582,17 @@ impl ListObjectsQuery {
 				// representing the key to start with.
 				(Some(token), _) => match &token[..1] {
 					"[" => Ok(RangeBegin::IncludingKey {
-						key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?,
+						key: String::from_utf8(
+							base64::decode(token[1..].as_bytes())
+								.ok_or_bad_request("Invalid continuation token")?,
+						)?,
 						fallback_key: None,
 					}),
 					"]" => Ok(RangeBegin::AfterKey {
-						key: String::from_utf8(base64::decode(token[1..].as_bytes())?)?,
+						key: String::from_utf8(
+							base64::decode(token[1..].as_bytes())
+								.ok_or_bad_request("Invalid continuation token")?,
+						)?,
 					}),
 					_ => Err(Error::bad_request("Invalid continuation token".to_string())),
 				},
diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs
index c4b63452..302ebe01 100644
--- a/src/api/s3/post_object.rs
+++ b/src/api/s3/post_object.rs
@@ -89,9 +89,7 @@ pub async fn handle_post_object(
 		.to_str()?;
 	let credential = params
 		.get("x-amz-credential")
-		.ok_or_else(|| {
-			Error::Forbidden("Garage does not support anonymous access yet".to_string())
-		})?
+		.ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?
 		.to_str()?;
 	let policy = params
 		.get("policy")
@@ -128,15 +126,16 @@ pub async fn handle_post_object(
 	)
 	.await?;
 
-	let bucket_id = garage.bucket_helper().resolve_bucket(&bucket, &api_key).await?;
+	let bucket_id = garage
+		.bucket_helper()
+		.resolve_bucket(&bucket, &api_key)
+		.await?;
 
 	if !api_key.allow_write(&bucket_id) {
-		return Err(Error::Forbidden(
-			"Operation is not allowed for this key.".to_string(),
-		));
+		return Err(Error::forbidden("Operation is not allowed for this key."));
 	}
 
-	let decoded_policy = base64::decode(&policy)?;
+	let decoded_policy = base64::decode(&policy).ok_or_bad_request("Invalid policy")?;
 	let decoded_policy: Policy =
 		serde_json::from_slice(&decoded_policy).ok_or_bad_request("Invalid policy")?;
 
diff --git a/src/api/s3/router.rs b/src/api/s3/router.rs
index b12c63a7..0e769558 100644
--- a/src/api/s3/router.rs
+++ b/src/api/s3/router.rs
@@ -3,9 +3,9 @@ use std::borrow::Cow;
 use hyper::header::HeaderValue;
 use hyper::{HeaderMap, Method, Request};
 
-use crate::s3::error::{Error, OkOrBadRequest};
 use crate::helpers::Authorization;
 use crate::router_macros::{generateQueryParameters, router_match};
+use crate::s3::error::*;
 
 router_match! {@func
 
diff --git a/src/api/s3/website.rs b/src/api/s3/website.rs
index b2582c4b..133c8327 100644
--- a/src/api/s3/website.rs
+++ b/src/api/s3/website.rs
@@ -10,7 +10,6 @@ use crate::signature::verify_signed_content;
 
 use garage_model::bucket_table::*;
 use garage_model::garage::Garage;
-use garage_table::*;
 use garage_util::data::*;
 
 pub async fn handle_get_website(bucket: &Bucket) -> Result<Response<Body>, Error> {
@@ -47,14 +46,11 @@ pub async fn handle_delete_website(
 	bucket_id: Uuid,
 ) -> Result<Response<Body>, Error> {
 	let mut bucket = garage
-		.bucket_table
-		.get(&EmptyKey, &bucket_id)
-		.await?
-		.ok_or(Error::NoSuchBucket)?;
+		.bucket_helper()
+		.get_existing_bucket(bucket_id)
+		.await?;
 
-	let param = bucket
-		.params_mut()
-		.ok_or_internal_error("Bucket should not be deleted at this point")?;
+	let param = bucket.params_mut().unwrap();
 
 	param.website_config.update(None);
 	garage.bucket_table.insert(&bucket).await?;
@@ -77,14 +73,11 @@ pub async fn handle_put_website(
 	}
 
 	let mut bucket = garage
-		.bucket_table
-		.get(&EmptyKey, &bucket_id)
-		.await?
-		.ok_or(Error::NoSuchBucket)?;
+		.bucket_helper()
+		.get_existing_bucket(bucket_id)
+		.await?;
 
-	let param = bucket
-		.params_mut()
-		.ok_or_internal_error("Bucket should not be deleted at this point")?;
+	let param = bucket.params_mut().unwrap();
 
 	let conf: WebsiteConfiguration = from_reader(&body as &[u8])?;
 	conf.validate()?;
diff --git a/src/api/signature/error.rs b/src/api/signature/error.rs
index 69f3c6c5..3ef5cdcd 100644
--- a/src/api/signature/error.rs
+++ b/src/api/signature/error.rs
@@ -1,9 +1,7 @@
 use err_derive::Error;
 
-use garage_util::error::Error as GarageError;
-
 use crate::common_error::CommonError;
-pub use crate::common_error::{OkOrBadRequest, OkOrInternalError};
+pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
 
 /// Errors of this crate
 #[derive(Debug, Error)]
@@ -16,10 +14,6 @@ pub enum Error {
 	#[error(display = "Authorization header malformed, expected scope: {}", _0)]
 	AuthorizationHeaderMalformed(String),
 
-	/// No proper api key was used, or the signature was invalid
-	#[error(display = "Forbidden: {}", _0)]
-	Forbidden(String),
-
 	// Category: bad request
 	/// The request contained an invalid UTF-8 sequence in its path or in other parameters
 	#[error(display = "Invalid UTF-8: {}", _0)]
@@ -39,16 +33,4 @@ where
 	}
 }
 
-
-impl Error {
-	pub fn internal_error<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::InternalError(GarageError::Message(
-			msg.to_string(),
-		)))
-	}
-
-	pub fn bad_request<M: ToString>(msg: M) -> Self {
-		Self::CommonError(CommonError::BadRequest(msg.to_string()))
-	}
-}
-
+impl CommonErrorDerivative for Error {}
diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs
index 155a6f94..4c7934e5 100644
--- a/src/api/signature/payload.rs
+++ b/src/api/signature/payload.rs
@@ -303,7 +303,7 @@ pub async fn verify_v4(
 		.get(&EmptyKey, &key_id)
 		.await?
 		.filter(|k| !k.state.is_deleted())
-		.ok_or_else(|| Error::Forbidden(format!("No such key: {}", &key_id)))?;
+		.ok_or_else(|| Error::forbidden(format!("No such key: {}", &key_id)))?;
 	let key_p = key.params().unwrap();
 
 	let mut hmac = signing_hmac(
@@ -316,7 +316,7 @@ pub async fn verify_v4(
 	hmac.update(payload);
 	let our_signature = hex::encode(hmac.finalize().into_bytes());
 	if signature != our_signature {
-		return Err(Error::Forbidden("Invalid signature".to_string()));
+		return Err(Error::forbidden("Invalid signature".to_string()));
 	}
 
 	Ok(key)
diff --git a/src/model/helper/bucket.rs b/src/model/helper/bucket.rs
index 2f1c6ae9..734cb40e 100644
--- a/src/model/helper/bucket.rs
+++ b/src/model/helper/bucket.rs
@@ -6,10 +6,10 @@ use garage_util::time::*;
 
 use crate::bucket_alias_table::*;
 use crate::bucket_table::*;
-use crate::key_table::*;
 use crate::garage::Garage;
 use crate::helper::error::*;
 use crate::helper::key::KeyHelper;
+use crate::key_table::*;
 use crate::permission::BucketKeyPerm;
 
 pub struct BucketHelper<'a>(pub(crate) &'a Garage);
@@ -51,11 +51,7 @@ impl<'a> BucketHelper<'a> {
 	}
 
 	#[allow(clippy::ptr_arg)]
-	pub async fn resolve_bucket(
-		&self,
-		bucket_name: &String,
-		api_key: &Key,
-	) -> Result<Uuid, Error> {
+	pub async fn resolve_bucket(&self, bucket_name: &String, api_key: &Key) -> Result<Uuid, Error> {
 		let api_key_params = api_key
 			.state
 			.as_option()
@@ -64,8 +60,8 @@ impl<'a> BucketHelper<'a> {
 		if let Some(Some(bucket_id)) = api_key_params.local_aliases.get(bucket_name) {
 			Ok(*bucket_id)
 		} else {
-			Ok(self.
-				resolve_global_bucket_name(bucket_name)
+			Ok(self
+				.resolve_global_bucket_name(bucket_name)
 				.await?
 				.ok_or_else(|| Error::NoSuchBucket(bucket_name.to_string()))?)
 		}
diff --git a/src/web/web_server.rs b/src/web/web_server.rs
index dad98dfc..c30d8957 100644
--- a/src/web/web_server.rs
+++ b/src/web/web_server.rs
@@ -18,9 +18,11 @@ use opentelemetry::{
 
 use crate::error::*;
 
-use garage_api::s3::error::{Error as ApiError, OkOrBadRequest, OkOrInternalError};
 use garage_api::helpers::{authority_to_host, host_to_bucket};
 use garage_api::s3::cors::{add_cors_headers, find_matching_cors_rule, handle_options_for_bucket};
+use garage_api::s3::error::{
+	CommonErrorDerivative, Error as ApiError, OkOrBadRequest, OkOrInternalError,
+};
 use garage_api::s3::get::{handle_get, handle_head};
 
 use garage_model::garage::Garage;
author	Alex Auvolat <alex@adnab.me>	2022-05-13 19:18:51 +0200
committer	Alex Auvolat <alex@adnab.me>	2022-05-13 19:18:51 +0200
commit	ea325d78d36d19f59a0849ace1f4567e2b095bd7 (patch)
tree	bfc05dd1f8df3d1fae84a1af433f4ae56dbc2c31
parent	ec16d166f940f59098ae5cc0c0b3d8298f1bcc78 (diff)
download	garage-ea325d78d36d19f59a0849ace1f4567e2b095bd7.tar.gz garage-ea325d78d36d19f59a0849ace1f4567e2b095bd7.zip