From fdf3a5a336c98fc74697bd777719caa317b4045d Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Fri, 31 Mar 2023 14:06:10 +0100 Subject: [PATCH 1/2] Improve error handling for S3 invalid username/password/bucket --- src/error.rs | 106 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 94 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index a16765d..6fce9c7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -141,6 +141,14 @@ impl ErrorResponse { Self::new(StatusCode::BAD_REQUEST, error) } + /// Return a 401 unauthorised ErrorResponse + fn unauthorised(error: &E) -> Self + where + E: std::error::Error + Send + Sync, + { + Self::new(StatusCode::UNAUTHORIZED, error) + } + /// Return a 404 not found ErrorResponse fn not_found(error: &E) -> Self where @@ -194,9 +202,22 @@ impl From for ErrorResponse { GetObjectErrorKind::InvalidObjectState(_) | GetObjectErrorKind::NoSuchKey(_) => Self::bad_request(&error), - // FIXME: Quite a lot of error cases end up here - invalid username, - // password, etc. - GetObjectErrorKind::Unhandled(_) => Self::internal_server_error(&error), + // Quite a lot of error cases end up as unhandled. Attempt to determine + // the error from the code. + GetObjectErrorKind::Unhandled(_) => { + match get_obj_error.code() { + // Bad request + Some("NoSuchBucket") => Self::bad_request(&error), + + // Unauthorised + Some("InvalidAccessKeyId") | Some("SignatureDoesNotMatch") => { + Self::unauthorised(&error) + } + + // Internal server error + _ => Self::internal_server_error(&error), + } + } // The enum is marked as non-exhaustive _ => Self::internal_server_error(&error), @@ -301,22 +322,83 @@ mod tests { test_active_storage_error(error, StatusCode::BAD_REQUEST, message, caused_by).await; } + // Helper function for S3 GetObjectError errors + async fn test_s3_get_object_error( + sdk_error: SdkError, + status: StatusCode, + caused_by: Option>, + ) { + let error = ActiveStorageError::S3GetObject(sdk_error); + let message = "error retrieving object from S3 storage"; + test_active_storage_error(error, status, message, caused_by).await; + } + + fn get_smithy_response() -> SmithyResponse { + let sdk_body = "body"; + let http_response = HttpResponse::new(sdk_body.into()); + SmithyResponse::new(http_response) + } + #[tokio::test] async fn s3_get_object_error() { // Jump through hoops to create an SdkError. let smithy_error = SmithyError::builder().message("fake smithy error").build(); let no_such_key_error = GetObjectErrorKind::NoSuchKey(NoSuchKey::builder().build()); let get_object_error = GetObjectError::new(no_such_key_error, smithy_error); - let sdk_body = "body"; - let http_response = HttpResponse::new(sdk_body.into()); - let smithy_response = SmithyResponse::new(http_response); - let error = ActiveStorageError::S3GetObject(SdkError::service_error( - get_object_error, - smithy_response, - )); - let message = "error retrieving object from S3 storage"; + let sdk_error = SdkError::service_error(get_object_error, get_smithy_response()); let caused_by = Some(vec!["service error", "NoSuchKey"]); - test_active_storage_error(error, StatusCode::BAD_REQUEST, message, caused_by).await; + test_s3_get_object_error(sdk_error, StatusCode::BAD_REQUEST, caused_by).await; + } + + #[tokio::test] + async fn s3_get_object_invalid_access_key_error() { + // Jump through hoops to create an SdkError. + let smithy_error = SmithyError::builder() + .message("fake smithy error") + .code("InvalidAccessKeyId") + .build(); + let get_object_error = GetObjectError::generic(smithy_error); + let sdk_error = SdkError::service_error(get_object_error, get_smithy_response()); + let caused_by = Some(vec![ + "service error", + "unhandled error", + "Error { code: \"InvalidAccessKeyId\", message: \"fake smithy error\" }", + ]); + test_s3_get_object_error(sdk_error, StatusCode::UNAUTHORIZED, caused_by).await; + } + + #[tokio::test] + async fn s3_get_object_no_such_bucket() { + // Jump through hoops to create an SdkError. + let smithy_error = SmithyError::builder() + .message("fake smithy error") + .code("NoSuchBucket") + .build(); + let get_object_error = GetObjectError::generic(smithy_error); + let sdk_error = SdkError::service_error(get_object_error, get_smithy_response()); + let caused_by = Some(vec![ + "service error", + "unhandled error", + "Error { code: \"NoSuchBucket\", message: \"fake smithy error\" }", + ]); + test_s3_get_object_error(sdk_error, StatusCode::BAD_REQUEST, caused_by).await; + } + + #[tokio::test] + async fn s3_get_object_sig_does_not_match_error() { + // Jump through hoops to create an SdkError. + let smithy_error = SmithyError::builder() + .message("fake smithy error") + .code("SignatureDoesNotMatch") + .build(); + let get_object_error = GetObjectError::generic(smithy_error); + let sdk_error = SdkError::service_error(get_object_error, get_smithy_response()); + let caused_by = Some(vec![ + "service error", + "unhandled error", + "Error { code: \"SignatureDoesNotMatch\", message: \"fake smithy error\" }", + ]); + test_s3_get_object_error(sdk_error, StatusCode::UNAUTHORIZED, caused_by).await; } #[tokio::test] From c4c295e666c864af190bb666242cfa6d07ec941e Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Fri, 31 Mar 2023 14:09:08 +0100 Subject: [PATCH 2/2] tracing: Log error messages on server error (5XX) responses --- src/error.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index 6fce9c7..a32c13d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -11,7 +11,9 @@ use axum::{ }; use ndarray::ShapeError; use serde::{Deserialize, Serialize}; +use std::error::Error; use thiserror::Error; +use tracing::{event, Level}; /// Active Storage server error type /// @@ -169,7 +171,7 @@ impl ErrorResponse { impl From for ErrorResponse { /// Convert from an `ActiveStorageError` into an `ErrorResponse`. fn from(error: ActiveStorageError) -> Self { - match &error { + let response = match &error { // Bad request ActiveStorageError::EmptyArray { operation: _ } | ActiveStorageError::RequestDataJsonRejection(_) @@ -228,7 +230,19 @@ impl From for ErrorResponse { _ => Self::internal_server_error(&error), } } + }; + + // Log server errors. + if response.status.is_server_error() { + event!(Level::ERROR, "{}", error.to_string()); + let mut current = error.source(); + while let Some(source) = current { + event!(Level::ERROR, "Caused by: {}", source.to_string()); + current = source.source(); + } } + + response } }