feat(server): Move to structured ApiError

clients/python/tests/test_e2e.py

@@ -225,9 +225,7 @@ def test_fails_with_insufficient_auth_perms(server_url: str) -> None:
 
     session = client.session(test_usecase, org=42, project=1337)
 
-    # TODO: When server errors cause appropriate status codes to be returned,
-    # ensure this is 403
-    with pytest.raises(RequestError, check=lambda e: e.status == 500):
+    with pytest.raises(RequestError, check=lambda e: e.status == 403):
         _object_key = session.put(b"test data")
 
 

objectstore-server/src/auth/error.rs

@@ -3,10 +3,6 @@ use thiserror::Error;
 /// Error type for different authorization failure scenarios.
 #[derive(Error, Debug, PartialEq)]
 pub enum AuthError {
-    /// Indicates failure to initialize the auth system (e.g. a configured key is malformed).
-    #[error("failed to initialize auth system: {0}")]
-    InitFailure(String),
-
     /// Indicates that something about the request prevented authorization verification from
     /// happening properly.
     #[error("bad request: {0}")]

objectstore-server/src/auth/key_directory.rs

@@ -1,18 +1,15 @@
 use std::collections::{BTreeMap, HashSet};
 use std::path::Path;
 
+use anyhow::Context;
 use jsonwebtoken::DecodingKey;
 use objectstore_types::Permission;
 
-use crate::auth::AuthError;
 use crate::config::{AuthZ, AuthZVerificationKey};
 
-fn read_key_from_file(filename: &Path) -> Result<DecodingKey, AuthError> {
-    let key_content = std::fs::read_to_string(filename).map_err(|_| {
-        AuthError::InitFailure(format!("key could not be read from '{filename:?}'"))
-    })?;
-    DecodingKey::from_ed_pem(key_content.as_bytes())
-        .map_err(|_| AuthError::InitFailure("key could not be parsed".into()))
+fn read_key_from_file(filename: &Path) -> anyhow::Result<DecodingKey> {
+    let key_content = std::fs::read_to_string(filename).context("reading key")?;
+    DecodingKey::from_ed_pem(key_content.as_bytes()).context("parsing key")
 }
 
 /// Configures the EdDSA public key(s) and permissions used to verify tokens from a single `kid`.
@@ -36,15 +33,16 @@ pub struct PublicKeyConfig {
 }
 
 impl TryFrom<&AuthZVerificationKey> for PublicKeyConfig {
-    type Error = AuthError;
-    fn try_from(key_config: &AuthZVerificationKey) -> Result<Self, Self::Error> {
+    type Error = anyhow::Error;
+
+    fn try_from(key_config: &AuthZVerificationKey) -> Result<Self, anyhow::Error> {
         Ok(Self {
             max_permissions: key_config.max_permissions.clone(),
             key_versions: key_config
                 .key_files
                 .iter()
                 .map(|filename| read_key_from_file(filename))
-                .collect::<Result<Vec<DecodingKey>, AuthError>>()?,
+                .collect::<anyhow::Result<Vec<DecodingKey>>>()?,
         })
     }
 }
@@ -61,15 +59,15 @@ pub struct PublicKeyDirectory {
 }
 
 impl TryFrom<&AuthZ> for PublicKeyDirectory {
-    type Error = AuthError;
+    type Error = anyhow::Error;
 
     fn try_from(auth_config: &AuthZ) -> Result<Self, Self::Error> {
         Ok(Self {
             keys: auth_config
                 .keys
                 .iter()
                 .map(|(kid, key)| Ok((kid.clone(), key.try_into()?)))
-                .collect::<Result<BTreeMap<String, PublicKeyConfig>, AuthError>>()?,
+                .collect::<Result<BTreeMap<String, PublicKeyConfig>, anyhow::Error>>()?,
         })
     }
 }

objectstore-server/src/auth/service.rs

@@ -3,6 +3,7 @@ use objectstore_service::{PayloadStream, StorageService};
 use objectstore_types::{Metadata, Permission};
 
 use crate::auth::AuthContext;
+use crate::endpoints::common::ApiResult;
 
 /// Wrapper around [`StorageService`] that ensures each operation is authorized.
 ///
@@ -40,7 +41,7 @@ impl AuthAwareService {
         Self { service, context }
     }
 
-    fn assert_authorized(&self, perm: Permission, context: &ObjectContext) -> anyhow::Result<()> {
+    fn assert_authorized(&self, perm: Permission, context: &ObjectContext) -> ApiResult<()> {
         if let Some(auth) = &self.context {
             auth.assert_authorized(perm, context)?;
         }
@@ -55,25 +56,23 @@ impl AuthAwareService {
         key: Option<String>,
         metadata: &Metadata,
         stream: PayloadStream,
-    ) -> anyhow::Result<ObjectId> {
+    ) -> ApiResult<ObjectId> {
         self.assert_authorized(Permission::ObjectWrite, &context)?;
-        self.service
+        Ok(self
+            .service
             .insert_object(context, key, metadata, stream)
-            .await
+            .await?)
     }
 
     /// Auth-aware wrapper around [`StorageService::get_object`].
-    pub async fn get_object(
-        &self,
-        id: &ObjectId,
-    ) -> anyhow::Result<Option<(Metadata, PayloadStream)>> {
+    pub async fn get_object(&self, id: &ObjectId) -> ApiResult<Option<(Metadata, PayloadStream)>> {
         self.assert_authorized(Permission::ObjectRead, id.context())?;
-        self.service.get_object(id).await
+        Ok(self.service.get_object(id).await?)
     }
 
     /// Auth-aware wrapper around [`StorageService::delete_object`].
-    pub async fn delete_object(&self, id: &ObjectId) -> anyhow::Result<()> {
+    pub async fn delete_object(&self, id: &ObjectId) -> ApiResult<()> {
         self.assert_authorized(Permission::ObjectDelete, id.context())?;
-        self.service.delete_object(id).await
+        Ok(self.service.delete_object(id).await?)
     }
 }

objectstore-server/src/endpoints/common.rs

@@ -1,42 +1,83 @@
-//!
-//! This is mostly adapted from <https://github.com/tokio-rs/axum/blob/main/examples/anyhow-error-response/src/main.rs>
+//! Common types and utilities for API endpoints.
 
+use std::error::Error;
+
+use axum::Json;
 use axum::http::StatusCode;
 use axum::response::{IntoResponse, Response};
+use objectstore_service::ServiceError;
+use serde::{Deserialize, Serialize};
+use thiserror::Error;
+
+use crate::auth::AuthError;
+
+/// Error type for API operations.
+#[derive(Debug, Error)]
+pub enum ApiError {
+    /// Errors indicating malformed or illegal requests.
+    #[error("client error: {0}")]
+    Client(String),
 
-pub enum AnyhowResponse {
-    Error(anyhow::Error),
-    Response(Response),
+    /// Authorization/authentication errors.
+    #[error("auth error: {0}")]
+    Auth(#[from] AuthError),
+
+    /// Service errors, indicating that something went wrong when receiving or executing a request.
+    #[error("service error: {0}")]
+    Service(#[from] ServiceError),
 }
 
-pub type ApiResult<T> = std::result::Result<T, AnyhowResponse>;
+/// Result type for API operations.
+pub type ApiResult<T> = Result<T, ApiError>;
 
-impl IntoResponse for AnyhowResponse {
-    fn into_response(self) -> Response {
-        match self {
-            AnyhowResponse::Error(error) => {
-                tracing::error!(
-                    error = error.as_ref() as &dyn std::error::Error,
-                    "error handling request"
-                );
-
-                // TODO: Support more nuanced return codes for validation errors etc. See
-                // Relay's ApiErrorResponse and BadStoreRequest as examples.
-                StatusCode::INTERNAL_SERVER_ERROR.into_response()
-            }
-            AnyhowResponse::Response(response) => response,
-        }
-    }
+/// A JSON error response returned by the API.
+#[derive(Serialize, Deserialize, Debug)]
+pub struct ApiErrorResponse {
+    /// The main error message.
+    #[serde(default)]
+    detail: Option<String>,
+    /// Chain of error causes.
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    causes: Vec<String>,
 }
 
-impl From<Response> for AnyhowResponse {
-    fn from(response: Response) -> Self {
-        Self::Response(response)
+impl ApiErrorResponse {
+    /// Creates an error response from an error, extracting the full cause chain.
+    pub fn from_error<E: Error + ?Sized>(error: &E) -> Self {
+        let detail = Some(error.to_string());
+
+        let mut causes = Vec::new();
+        let mut source = error.source();
+        while let Some(s) = source {
+            causes.push(s.to_string());
+            source = s.source();
+        }
+
+        Self { detail, causes }
     }
 }
 
-impl From<anyhow::Error> for AnyhowResponse {
-    fn from(err: anyhow::Error) -> Self {
-        Self::Error(err)
+impl IntoResponse for ApiError {
+    fn into_response(self) -> Response {
+        let status = match &self {
+            ApiError::Client(_) => StatusCode::BAD_REQUEST,
+
+            ApiError::Auth(AuthError::BadRequest(_)) => StatusCode::BAD_REQUEST,
+            ApiError::Auth(AuthError::ValidationFailure(_))
+            | ApiError::Auth(AuthError::VerificationFailure) => StatusCode::UNAUTHORIZED,
+            ApiError::Auth(AuthError::NotPermitted) => StatusCode::FORBIDDEN,
+            ApiError::Auth(AuthError::InternalError(_)) => {
+                tracing::error!(error = &self as &dyn Error, "auth system error");
+                StatusCode::INTERNAL_SERVER_ERROR
+            }
+
+            ApiError::Service(_) => {
+                tracing::error!(error = &self as &dyn Error, "error handling request");
+                StatusCode::INTERNAL_SERVER_ERROR
+            }
+        };
+
+        let body = ApiErrorResponse::from_error(&self);
+        (status, Json(body)).into_response()
     }
 }

objectstore-server/src/endpoints/mod.rs

@@ -6,7 +6,7 @@ use axum::Router;
 
 use crate::state::ServiceState;
 
-mod common;
+pub mod common;
 mod health;
 mod objects;
 

objectstore-server/src/endpoints/objects.rs

@@ -1,4 +1,3 @@
-use anyhow::Context as _;
 use std::io;
 use std::time::SystemTime;
 
@@ -9,6 +8,7 @@ use axum::response::{IntoResponse, Response};
 use axum::routing;
 use axum::{Json, Router};
 use futures_util::{StreamExt, TryStreamExt};
+use objectstore_service::ServiceError;
 use objectstore_service::id::{ObjectContext, ObjectId};
 use objectstore_types::Metadata;
 use serde::Serialize;
@@ -46,8 +46,7 @@ async fn objects_post(
     headers: HeaderMap,
     body: Body,
 ) -> ApiResult<Response> {
-    let mut metadata =
-        Metadata::from_headers(&headers, "").context("extracting metadata from headers")?;
+    let mut metadata = Metadata::from_headers(&headers, "").map_err(ServiceError::from)?;
     metadata.time_created = Some(SystemTime::now());
 
     let stream = body.into_data_stream().map_err(io::Error::other).boxed();
@@ -73,9 +72,7 @@ async fn object_get(
     };
     let stream = MeteredPayloadStream::from(stream, state.rate_limiter.bytes_accumulator()).boxed();
 
-    let headers = metadata
-        .to_headers("", false)
-        .context("extracting metadata from headers")?;
+    let headers = metadata.to_headers("", false).map_err(ServiceError::from)?;
     Ok((headers, Body::from_stream(stream)).into_response())
 }
 
@@ -84,9 +81,7 @@ async fn object_head(service: AuthAwareService, Xt(id): Xt<ObjectId>) -> ApiResu
         return Ok(StatusCode::NOT_FOUND.into_response());
     };
 
-    let headers = metadata
-        .to_headers("", false)
-        .context("extracting metadata from headers")?;
+    let headers = metadata.to_headers("", false).map_err(ServiceError::from)?;
 
     Ok((StatusCode::NO_CONTENT, headers).into_response())
 }
@@ -98,8 +93,7 @@ async fn object_put(
     headers: HeaderMap,
     body: Body,
 ) -> ApiResult<Response> {
-    let mut metadata =
-        Metadata::from_headers(&headers, "").context("extracting metadata from headers")?;
+    let mut metadata = Metadata::from_headers(&headers, "").map_err(ServiceError::from)?;
     metadata.time_created = Some(SystemTime::now());
 
     let ObjectId { context, key } = id;

objectstore-server/src/extractors/service.rs

@@ -1,13 +1,14 @@
 use axum::extract::FromRequestParts;
-use axum::http::{StatusCode, header, request::Parts};
+use axum::http::{header, request::Parts};
 
 use crate::auth::{AuthAwareService, AuthContext};
+use crate::endpoints::common::ApiError;
 use crate::state::ServiceState;
 
 const BEARER_PREFIX: &str = "Bearer ";
 
 impl FromRequestParts<ServiceState> for AuthAwareService {
-    type Rejection = StatusCode;
+    type Rejection = ApiError;
 
     async fn from_request_parts(
         parts: &mut Parts,
@@ -24,11 +25,8 @@ impl FromRequestParts<ServiceState> for AuthAwareService {
             .and_then(|v| v.to_str().ok())
             .and_then(strip_bearer);
 
-        let context =
-            AuthContext::from_encoded_jwt(encoded_token, &state.key_directory).map_err(|err| {
-                tracing::debug!("Authorization rejected: `{:?}`", err);
-                StatusCode::UNAUTHORIZED
-            })?;
+        let context = AuthContext::from_encoded_jwt(encoded_token, &state.key_directory)
+            .inspect_err(|err| tracing::debug!("Authorization rejected: `{:?}`", err))?;
 
         Ok(AuthAwareService::new(state.service.clone(), Some(context)))
     }

objectstore-service/src/lib.rs

@@ -114,7 +114,7 @@ impl StorageService {
         key: Option<String>,
         metadata: &Metadata,
         mut stream: PayloadStream,
-    ) -> anyhow::Result<ObjectId> {
+    ) -> ServiceResult<ObjectId> {
         let start = Instant::now();
 
         let mut first_chunk = BytesMut::new();
@@ -222,7 +222,7 @@ impl StorageService {
     pub async fn get_object(
         &self,
         id: &ObjectId,
-    ) -> anyhow::Result<Option<(Metadata, PayloadStream)>> {
+    ) -> ServiceResult<Option<(Metadata, PayloadStream)>> {
         let start = Instant::now();
 
         let mut backend_choice = "high-volume";
@@ -259,7 +259,7 @@ impl StorageService {
     }
 
     /// Deletes an object stored at the given key, if it exists.
-    pub async fn delete_object(&self, id: &ObjectId) -> anyhow::Result<()> {
+    pub async fn delete_object(&self, id: &ObjectId) -> ServiceResult<()> {
         let start = Instant::now();
 
         if let Some((metadata, _stream)) = self.0.high_volume_backend.get_object(id).await? {