diff --git a/clients/python/tests/test_e2e.py b/clients/python/tests/test_e2e.py index 2f78e391..9c66631b 100644 --- a/clients/python/tests/test_e2e.py +++ b/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") diff --git a/objectstore-server/src/auth/error.rs b/objectstore-server/src/auth/error.rs index 5410ce1b..01cccae7 100644 --- a/objectstore-server/src/auth/error.rs +++ b/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}")] diff --git a/objectstore-server/src/auth/key_directory.rs b/objectstore-server/src/auth/key_directory.rs index 2d0f0d12..053a4a84 100644 --- a/objectstore-server/src/auth/key_directory.rs +++ b/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 { - 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 { + 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 { + type Error = anyhow::Error; + + fn try_from(key_config: &AuthZVerificationKey) -> Result { Ok(Self { max_permissions: key_config.max_permissions.clone(), key_versions: key_config .key_files .iter() .map(|filename| read_key_from_file(filename)) - .collect::, AuthError>>()?, + .collect::>>()?, }) } } @@ -61,7 +59,7 @@ pub struct PublicKeyDirectory { } impl TryFrom<&AuthZ> for PublicKeyDirectory { - type Error = AuthError; + type Error = anyhow::Error; fn try_from(auth_config: &AuthZ) -> Result { Ok(Self { @@ -69,7 +67,7 @@ impl TryFrom<&AuthZ> for PublicKeyDirectory { .keys .iter() .map(|(kid, key)| Ok((kid.clone(), key.try_into()?))) - .collect::, AuthError>>()?, + .collect::, anyhow::Error>>()?, }) } } diff --git a/objectstore-server/src/auth/service.rs b/objectstore-server/src/auth/service.rs index 8429ced1..8c9ce1cc 100644 --- a/objectstore-server/src/auth/service.rs +++ b/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, metadata: &Metadata, stream: PayloadStream, - ) -> anyhow::Result { + ) -> ApiResult { 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> { + pub async fn get_object(&self, id: &ObjectId) -> ApiResult> { 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?) } } diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 8da1c300..2306ca4d 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -1,42 +1,83 @@ -//! -//! This is mostly adapted from +//! 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 = std::result::Result; +/// Result type for API operations. +pub type ApiResult = Result; -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, + /// Chain of error causes. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, } -impl From 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(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 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() } } diff --git a/objectstore-server/src/endpoints/mod.rs b/objectstore-server/src/endpoints/mod.rs index 6907977e..31398c3f 100644 --- a/objectstore-server/src/endpoints/mod.rs +++ b/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; diff --git a/objectstore-server/src/endpoints/objects.rs b/objectstore-server/src/endpoints/objects.rs index 783551f2..c1751f4b 100644 --- a/objectstore-server/src/endpoints/objects.rs +++ b/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 { - 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) -> 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 { - 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; diff --git a/objectstore-server/src/extractors/service.rs b/objectstore-server/src/extractors/service.rs index beb4608c..235bc4eb 100644 --- a/objectstore-server/src/extractors/service.rs +++ b/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 for AuthAwareService { - type Rejection = StatusCode; + type Rejection = ApiError; async fn from_request_parts( parts: &mut Parts, @@ -24,11 +25,8 @@ impl FromRequestParts 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))) } diff --git a/objectstore-service/src/lib.rs b/objectstore-service/src/lib.rs index 9bb1d8f8..0fd1a021 100644 --- a/objectstore-service/src/lib.rs +++ b/objectstore-service/src/lib.rs @@ -114,7 +114,7 @@ impl StorageService { key: Option, metadata: &Metadata, mut stream: PayloadStream, - ) -> anyhow::Result { + ) -> ServiceResult { 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> { + ) -> ServiceResult> { 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? {