diff --git a/Cargo.lock b/Cargo.lock index 0ff652d5..f872523f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3270,8 +3270,10 @@ name = "tokenserver-common" version = "0.10.2" dependencies = [ "actix-web", + "backtrace", "serde 1.0.135", "serde_json", + "syncstorage-db-common", "thiserror", ] diff --git a/syncstorage/src/tokenserver/auth/browserid.rs b/syncstorage/src/tokenserver/auth/browserid.rs index ce43c643..1b10cbf2 100644 --- a/syncstorage/src/tokenserver/auth/browserid.rs +++ b/syncstorage/src/tokenserver/auth/browserid.rs @@ -81,7 +81,7 @@ impl VerifyToken for RemoteVerifier { "Unknown error occurred during BrowserID request to FxA: {}", e ), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) } } })?; @@ -124,22 +124,22 @@ impl VerifyToken for RemoteVerifier { reason: Some(reason), } => Err(TokenserverError { context: format!("BrowserID verification error: {}", reason), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Failure { .. } => Err(TokenserverError { context: "Unknown BrowserID verification error".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { issuer, .. } if issuer != self.issuer => Err(TokenserverError { context: "BrowserID issuer mismatch".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { idp_claims: Some(claims), .. } if !claims.token_verified() => Err(TokenserverError { context: "BrowserID assertion not verified".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { email, @@ -238,7 +238,7 @@ impl IdpClaims { Some(Some(_)) | None => Ok(self.keys_changed_at.flatten()), // If the fxa-keysChangedAt claim is null, return an error. Some(None) => Err(TokenserverError { - description: "invalid keysChangedAt", + description: "invalid keysChangedAt".to_owned(), status: "invalid-credentials", location: ErrorLocation::Body, context: "null fxa-keysChangedAt claim in BrowserID assertion".to_owned(), @@ -407,7 +407,7 @@ mod tests { let expected_error = TokenserverError { context: "BrowserID verification error: something broke".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; assert_eq!(expected_error, error); } @@ -423,7 +423,7 @@ mod tests { let expected_error = TokenserverError { context: "Unknown BrowserID verification error".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; assert_eq!(expected_error, error); } @@ -450,7 +450,7 @@ mod tests { let expected_error = TokenserverError { context: "BrowserID issuer mismatch".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; let verifier = RemoteVerifier::try_from(&Settings { fxa_browserid_audience: AUDIENCE.to_owned(), diff --git a/syncstorage/src/tokenserver/auth/mod.rs b/syncstorage/src/tokenserver/auth/mod.rs index ed6e23d4..1a5e4d54 100644 --- a/syncstorage/src/tokenserver/auth/mod.rs +++ b/syncstorage/src/tokenserver/auth/mod.rs @@ -3,7 +3,6 @@ pub mod oauth; use std::fmt; -use actix_web::Error; use async_trait::async_trait; use dyn_clone::{self, DynClone}; use pyo3::{ @@ -13,8 +12,6 @@ use pyo3::{ use serde::{Deserialize, Serialize}; use tokenserver_common::error::TokenserverError; -use crate::error::{ApiError, ApiErrorKind}; - /// Represents the origin of the token used by Sync clients to access their data. #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)] #[serde(rename_all = "lowercase")] @@ -82,7 +79,7 @@ impl Tokenlib { pub fn get_token_and_derived_secret( plaintext: MakeTokenPlaintext, shared_secret: &str, - ) -> Result<(String, String), Error> { + ) -> Result<(String, String), TokenserverError> { Python::with_gil(|py| { // `import tokenlib` let module = PyModule::import(py, "tokenlib").map_err(|e| { @@ -112,7 +109,7 @@ impl Tokenlib { // `return (token, derived_secret)` Ok((token, derived_secret)) }) - .map_err(pyerr_to_actix_error) + .map_err(pyerr_to_tokenserver_error) } } @@ -142,11 +139,13 @@ impl VerifyToken for MockVerifier { async fn verify(&self, _token: String) -> Result { self.valid .then(|| self.verify_output.clone()) - .ok_or_else(|| TokenserverError::invalid_credentials("Unauthorized")) + .ok_or_else(|| TokenserverError::invalid_credentials("Unauthorized".to_owned())) } } -fn pyerr_to_actix_error(e: PyErr) -> Error { - let api_error: ApiError = ApiErrorKind::Internal(e.to_string()).into(); - api_error.into() +fn pyerr_to_tokenserver_error(e: PyErr) -> TokenserverError { + TokenserverError { + context: e.to_string(), + ..TokenserverError::internal_error() + } } diff --git a/syncstorage/src/tokenserver/auth/oauth.rs b/syncstorage/src/tokenserver/auth/oauth.rs index 052d1500..6f709327 100644 --- a/syncstorage/src/tokenserver/auth/oauth.rs +++ b/syncstorage/src/tokenserver/auth/oauth.rs @@ -1,4 +1,3 @@ -use actix_web::Error; use async_trait::async_trait; use futures::TryFutureExt; use pyo3::{ @@ -40,9 +39,9 @@ impl Verifier { } impl TryFrom<&Settings> for Verifier { - type Error = Error; + type Error = TokenserverError; - fn try_from(settings: &Settings) -> Result { + fn try_from(settings: &Settings) -> Result { let inner: Py = Python::with_gil::<_, Result, PyErr>>(|py| { let code = include_str!("verify.py"); let module = PyModule::from_code(py, code, Self::FILENAME, Self::FILENAME)?; @@ -80,7 +79,7 @@ impl TryFrom<&Settings> for Verifier { Ok(object) }) - .map_err(super::pyerr_to_actix_error)?; + .map_err(super::pyerr_to_tokenserver_error)?; Ok(Self { inner, @@ -122,7 +121,7 @@ impl VerifyToken for Verifier { }) .map_err(|e| TokenserverError { context: format!("pyo3 error in OAuth verifier: {}", e), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) })?; match maybe_verify_output_string { @@ -130,13 +129,13 @@ impl VerifyToken for Verifier { serde_json::from_str::(&verify_output_string).map_err(|e| { TokenserverError { context: format!("Invalid OAuth verify output: {}", e), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) } }) } None => Err(TokenserverError { context: "Invalid OAuth token".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), } }; diff --git a/syncstorage/src/tokenserver/extractors.rs b/syncstorage/src/tokenserver/extractors.rs index c77ee70f..c3a9fccd 100644 --- a/syncstorage/src/tokenserver/extractors.rs +++ b/syncstorage/src/tokenserver/extractors.rs @@ -10,7 +10,7 @@ use actix_web::{ dev::Payload, http::StatusCode, web::{Data, Query}, - Error, FromRequest, HttpRequest, + FromRequest, HttpRequest, }; use futures::future::{self, LocalBoxFuture, Ready}; use hex; @@ -25,7 +25,7 @@ use super::{ db::{models::Db, params, pool::DbPool, results}, LogItemsMutator, NodeType, ServerState, TokenserverMetrics, }; -use crate::{error::ApiError, server::metrics, settings::Secrets}; +use crate::{server::metrics, settings::Secrets}; lazy_static! { static ref CLIENT_STATE_REGEX: Regex = Regex::new("^[a-zA-Z0-9._-]{1,32}$").unwrap(); @@ -104,7 +104,7 @@ impl TokenserverRequest { .old_client_states .contains(&self.auth_data.client_state) { - let error_message = "Unacceptable client-state value stale value"; + let error_message = "Unacceptable client-state value stale value".to_owned(); return Err(TokenserverError::invalid_client_state(error_message)); } @@ -114,7 +114,7 @@ impl TokenserverRequest { && opt_cmp!(auth_generation <= user_generation) { let error_message = - "Unacceptable client-state value new value with no generation change"; + "Unacceptable client-state value new value with no generation change".to_owned(); return Err(TokenserverError::invalid_client_state(error_message)); } @@ -124,7 +124,8 @@ impl TokenserverRequest { && opt_cmp!(auth_keys_changed_at <= user_keys_changed_at) { let error_message = - "Unacceptable client-state value new value with no keys_changed_at change"; + "Unacceptable client-state value new value with no keys_changed_at change" + .to_owned(); return Err(TokenserverError::invalid_client_state(error_message)); } @@ -152,7 +153,7 @@ impl TokenserverRequest { impl FromRequest for TokenserverRequest { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -202,16 +203,14 @@ impl FromRequest for TokenserverRequest { db.get_service_id(params::GetServiceId { service: SYNC_SERVICE_NAME.to_owned(), }) - .await - .map_err(ApiError::from)? + .await? .id, ) } else { return Err(TokenserverError::unsupported( - "Unsupported application version", + "Unsupported application version".to_owned(), version.to_owned(), - ) - .into()); + )); } } else { // NOTE: It would probably be better to include the name of the unsupported @@ -220,10 +219,9 @@ impl FromRequest for TokenserverRequest { // new Tokenservers as close as possible, we defer to the error message from // the old Tokenserver. return Err(TokenserverError::unsupported( - "Unsupported application", + "Unsupported application".to_owned(), "application".to_owned(), - ) - .into()); + )); } }; let user = db @@ -235,12 +233,20 @@ impl FromRequest for TokenserverRequest { keys_changed_at: auth_data.keys_changed_at, capacity_release_rate: state.node_capacity_release_rate, }) - .await - .map_err(ApiError::from)?; + .await?; log_items_mutator.insert("first_seen_at".to_owned(), user.first_seen_at.to_string()); let duration = { - let params = Query::::extract(&req).await?; + let params = + Query::::extract(&req) + .await + .map_err(|_| TokenserverError { + description: "invalid query params".to_owned(), + context: "invalid query params".to_owned(), + http_status: StatusCode::BAD_REQUEST, + location: ErrorLocation::Url, + ..Default::default() + })?; // An error in the "duration" query parameter should never cause a request to fail. // Instead, we should simply resort to using the default token duration. @@ -279,7 +285,7 @@ struct QueryParams { impl FromRequest for Box { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -290,12 +296,9 @@ impl FromRequest for Box { .await? .get() .await - .map_err(|e| { - TokenserverError { - context: format!("Couldn't acquire a database connection: {}", e), - ..TokenserverError::internal_error() - } - .into() + .map_err(|e| TokenserverError { + context: format!("Couldn't acquire a database connection: {}", e), + ..TokenserverError::internal_error() }) }) } @@ -303,7 +306,7 @@ impl FromRequest for Box { impl FromRequest for Box { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -328,7 +331,7 @@ pub enum Token { impl FromRequest for Token { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -340,14 +343,14 @@ impl FromRequest for Token { .headers() .get("Authorization") .ok_or_else(|| TokenserverError { - description: "Unauthorized", + description: "Unauthorized".to_owned(), location: ErrorLocation::Body, context: "No Authorization header".to_owned(), ..Default::default() })? .to_str() .map_err(|e| TokenserverError { - description: "Unauthorized", + description: "Unauthorized".to_owned(), location: ErrorLocation::Body, context: format!( "Authorization header contains invalid ASCII characters: {}", @@ -366,22 +369,20 @@ impl FromRequest for Token { } else { // The request must use a Bearer token or BrowserID token Err(TokenserverError { - description: "Unsupported", + description: "Unsupported".to_owned(), location: ErrorLocation::Body, context: "Invalid authorization scheme".to_owned(), ..Default::default() - } - .into()) + }) } } else { // Headers that are not of the format "[AUTH TYPE] [TOKEN]" are invalid Err(TokenserverError { - description: "Unauthorized", + description: "Unauthorized".to_owned(), location: ErrorLocation::Body, context: "Invalid Authorization header format".to_owned(), ..Default::default() - } - .into()) + }) } }) } @@ -400,7 +401,7 @@ pub struct AuthData { impl FromRequest for AuthData { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -467,7 +468,7 @@ struct XClientStateHeader(Option); impl FromRequest for XClientStateHeader { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -485,12 +486,12 @@ impl FromRequest for XClientStateHeader { return Err(TokenserverError { status: "error", location: ErrorLocation::Header, - description: "Invalid client state value", + description: "Invalid client state value".to_owned(), name: "X-Client-State".to_owned(), http_status: StatusCode::BAD_REQUEST, context: "Invalid client state value".to_owned(), - } - .into()); + ..Default::default() + }); } } @@ -509,7 +510,7 @@ struct KeyId { impl FromRequest for KeyId { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = LocalBoxFuture<'static, Result>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -521,15 +522,19 @@ impl FromRequest for KeyId { // The X-KeyID header must be present for requests using OAuth let x_key_id = headers .get("X-KeyID") - .ok_or_else(|| TokenserverError::invalid_key_id("Missing X-KeyID header"))? + .ok_or_else(|| { + TokenserverError::invalid_key_id("Missing X-KeyID header".to_owned()) + })? .to_str() - .map_err(|_| TokenserverError::invalid_key_id("Invalid X-KeyID header"))?; + .map_err(|_| { + TokenserverError::invalid_key_id("Invalid X-KeyID header".to_owned()) + })?; // The X-KeyID header is of the format `[keys_changed_at]-[base64-encoded client state]` (e.g. `00000000000001234-qqo`) let (keys_changed_at_string, encoded_client_state) = x_key_id.split_once('-').ok_or_else(|| TokenserverError { context: "X-KeyID header has invalid format".to_owned(), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) })?; let client_state = { @@ -544,7 +549,7 @@ impl FromRequest for KeyId { "Failed to decode client state base64 in X-KeyID: {}", e ), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) })?; hex::encode(bytes) @@ -562,8 +567,7 @@ impl FromRequest for KeyId { location: ErrorLocation::Body, context: "Client state mismatch in X-Client-State header".to_owned(), ..TokenserverError::default() - } - .into()); + }); } } @@ -575,7 +579,7 @@ impl FromRequest for KeyId { .parse::() .map_err(|e| TokenserverError { context: format!("Non-integral keys_changed_at in X-KeyID: {}", e), - ..TokenserverError::invalid_credentials("Unauthorized") + ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) })?; Ok(KeyId { @@ -588,7 +592,7 @@ impl FromRequest for KeyId { impl FromRequest for TokenserverMetrics { type Config = (); - type Error = Error; + type Error = TokenserverError; type Future = Ready>; fn from_request(req: &HttpRequest, _payload: &mut Payload) -> Self::Future { @@ -606,28 +610,25 @@ impl FromRequest for TokenserverMetrics { } } -fn get_server_state(req: &HttpRequest) -> Result<&Data>, Error> { - req.app_data::>>().ok_or_else(|| { - Error::from(TokenserverError { +fn get_server_state(req: &HttpRequest) -> Result<&Data>, TokenserverError> { + req.app_data::>>() + .ok_or_else(|| TokenserverError { context: "Failed to load the application state".to_owned(), ..TokenserverError::internal_error() }) - }) } -fn get_secret(req: &HttpRequest) -> Result { - let secrets = req.app_data::>>().ok_or_else(|| { - Error::from(TokenserverError { +fn get_secret(req: &HttpRequest) -> Result { + let secrets = req + .app_data::>>() + .ok_or_else(|| TokenserverError { context: "Failed to load the application secrets".to_owned(), ..TokenserverError::internal_error() - }) - })?; + })?; - String::from_utf8(secrets.master_secret.clone()).map_err(|e| { - Error::from(TokenserverError { - context: format!("Failed to read the master secret: {}", e), - ..TokenserverError::internal_error() - }) + String::from_utf8(secrets.master_secret.clone()).map_err(|e| TokenserverError { + context: format!("Failed to read the master secret: {}", e), + ..TokenserverError::internal_error() }) } @@ -766,7 +767,7 @@ mod tests { assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_credentials("Unauthorized"); + let expected_error = TokenserverError::invalid_credentials("Unauthorized".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -811,8 +812,10 @@ mod tests { assert_eq!(response.status(), StatusCode::NOT_FOUND); - let expected_error = - TokenserverError::unsupported("Unsupported application version", "1.0".to_owned()); + let expected_error = TokenserverError::unsupported( + "Unsupported application version".to_owned(), + "1.0".to_owned(), + ); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -831,8 +834,10 @@ mod tests { assert_eq!(response.status(), StatusCode::NOT_FOUND); - let expected_error = - TokenserverError::unsupported("Unsupported application", "application".to_owned()); + let expected_error = TokenserverError::unsupported( + "Unsupported application".to_owned(), + "application".to_owned(), + ); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -851,8 +856,10 @@ mod tests { assert_eq!(response.status(), StatusCode::NOT_FOUND); - let expected_error = - TokenserverError::unsupported("Unsupported application", "application".to_owned()); + let expected_error = TokenserverError::unsupported( + "Unsupported application".to_owned(), + "application".to_owned(), + ); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -902,7 +909,8 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_key_id("Missing X-KeyID header"); + let expected_error = + TokenserverError::invalid_key_id("Missing X-KeyID header".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -915,7 +923,8 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_key_id("Invalid X-KeyID header"); + let expected_error = + TokenserverError::invalid_key_id("Invalid X-KeyID header".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -928,7 +937,7 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_credentials("Unauthorized"); + let expected_error = TokenserverError::invalid_credentials("Unauthorized".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -941,7 +950,7 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_credentials("Unauthorized"); + let expected_error = TokenserverError::invalid_credentials("Unauthorized".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -954,7 +963,8 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_key_id("Invalid X-KeyID header"); + let expected_error = + TokenserverError::invalid_key_id("Invalid X-KeyID header".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -967,7 +977,7 @@ mod tests { let response: HttpResponse = KeyId::extract(&request).await.unwrap_err().into(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); - let expected_error = TokenserverError::invalid_credentials("Unauthorized"); + let expected_error = TokenserverError::invalid_credentials("Unauthorized".to_owned()); let body = extract_body_as_str(ServiceResponse::new(request, response)); assert_eq!(body, serde_json::to_string(&expected_error).unwrap()); } @@ -1182,7 +1192,7 @@ mod tests { }; let error = tokenserver_request.validate().unwrap_err(); - let error_message = "Unacceptable client-state value stale value"; + let error_message = "Unacceptable client-state value stale value".to_owned(); assert_eq!(error, TokenserverError::invalid_client_state(error_message)); } @@ -1219,7 +1229,8 @@ mod tests { }; let error = tokenserver_request.validate().unwrap_err(); - let error_message = "Unacceptable client-state value new value with no generation change"; + let error_message = + "Unacceptable client-state value new value with no generation change".to_owned(); assert_eq!(error, TokenserverError::invalid_client_state(error_message)); } @@ -1257,7 +1268,7 @@ mod tests { let error = tokenserver_request.validate().unwrap_err(); let error_message = - "Unacceptable client-state value new value with no keys_changed_at change"; + "Unacceptable client-state value new value with no keys_changed_at change".to_owned(); assert_eq!(error, TokenserverError::invalid_client_state(error_message)); } diff --git a/syncstorage/src/tokenserver/handlers.rs b/syncstorage/src/tokenserver/handlers.rs index 7a8e0fb8..b896d484 100644 --- a/syncstorage/src/tokenserver/handlers.rs +++ b/syncstorage/src/tokenserver/handlers.rs @@ -18,7 +18,6 @@ use super::{ extractors::TokenserverRequest, NodeType, TokenserverMetrics, }; -use crate::error::ApiError; #[derive(Debug, Serialize)] pub struct TokenserverResult { @@ -36,7 +35,7 @@ pub async fn get_tokenserver_result( req: TokenserverRequest, db: Box, TokenserverMetrics(mut metrics): TokenserverMetrics, -) -> Result { +) -> Result { let updates = update_user(&req, db).await?; let (token, derived_secret) = { @@ -113,7 +112,10 @@ struct UserUpdates { uid: i64, } -async fn update_user(req: &TokenserverRequest, db: Box) -> Result { +async fn update_user( + req: &TokenserverRequest, + db: Box, +) -> Result { // If the keys_changed_at in the request is larger than that stored on the user record, // update to the value in the request. let keys_changed_at = diff --git a/tokenserver-common/Cargo.toml b/tokenserver-common/Cargo.toml index 18ced55c..762bdd26 100644 --- a/tokenserver-common/Cargo.toml +++ b/tokenserver-common/Cargo.toml @@ -5,6 +5,8 @@ edition = "2021" [dependencies] actix-web = "3" +backtrace = "0.3.61" serde = "1.0" serde_json = { version = "1.0", features = ["arbitrary_precision"] } +syncstorage-db-common = { path = "../syncstorage-db-common" } thiserror = "1.0.26" diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 24500676..1598043f 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -1,23 +1,43 @@ -use std::fmt; +use std::{cmp::PartialEq, fmt}; use actix_web::{http::StatusCode, HttpResponse, ResponseError}; +use backtrace::Backtrace; use serde::{ ser::{SerializeMap, Serializer}, Serialize, }; -use thiserror::Error; +use syncstorage_db_common::error::DbError; -#[derive(Clone, Debug, Error, PartialEq)] -#[error("{context}")] +#[derive(Clone, Debug)] pub struct TokenserverError { pub status: &'static str, pub location: ErrorLocation, pub name: String, - pub description: &'static str, + pub description: String, pub http_status: StatusCode, /// For internal use only. Used to report any additional context behind an error to /// distinguish between similar errors in Sentry. pub context: String, + pub backtrace: Backtrace, +} + +// We implement `PartialEq` manually here because `Backtrace` doesn't implement `PartialEq`, so we +// can't derive it +impl PartialEq for TokenserverError { + fn eq(&self, other: &Self) -> bool { + self.status == other.status + && self.location == other.location + && self.name == other.name + && self.description == other.description + && self.http_status == other.http_status + && self.context == other.context + } +} + +impl fmt::Display for TokenserverError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.context) + } } impl Default for TokenserverError { @@ -26,9 +46,10 @@ impl Default for TokenserverError { status: "error", location: ErrorLocation::default(), name: "".to_owned(), - description: "Unauthorized", + description: "Unauthorized".to_owned(), http_status: StatusCode::UNAUTHORIZED, context: "Unauthorized".to_owned(), + backtrace: Backtrace::new(), } } } @@ -52,31 +73,31 @@ impl TokenserverError { } } - pub fn invalid_key_id(description: &'static str) -> Self { + pub fn invalid_key_id(description: String) -> Self { Self { status: "invalid-key-id", + context: description.clone(), description, - context: description.to_owned(), ..Self::default() } } - pub fn invalid_credentials(description: &'static str) -> Self { + pub fn invalid_credentials(description: String) -> Self { Self { status: "invalid-credentials", location: ErrorLocation::Body, + context: description.clone(), description, - context: description.to_owned(), ..Self::default() } } - pub fn invalid_client_state(description: &'static str) -> Self { + pub fn invalid_client_state(description: String) -> Self { Self { status: "invalid-client-state", + context: description.clone(), description, name: "X-Client-State".to_owned(), - context: description.to_owned(), ..Self::default() } } @@ -85,7 +106,7 @@ impl TokenserverError { Self { status: "internal-error", location: ErrorLocation::Internal, - description: "Server error", + description: "Server error".to_owned(), http_status: StatusCode::INTERNAL_SERVER_ERROR, context: "Internal error".to_owned(), ..Self::default() @@ -95,29 +116,30 @@ impl TokenserverError { pub fn resource_unavailable() -> Self { Self { location: ErrorLocation::Body, - description: "Resource is not available", + description: "Resource is not available".to_owned(), http_status: StatusCode::SERVICE_UNAVAILABLE, context: "Resource is not available".to_owned(), ..Default::default() } } - pub fn unsupported(description: &'static str, name: String) -> Self { + pub fn unsupported(description: String, name: String) -> Self { Self { status: "error", location: ErrorLocation::Url, + context: description.clone(), description, name, http_status: StatusCode::NOT_FOUND, - context: description.to_owned(), + ..Self::default() } } - pub fn unauthorized(description: &'static str) -> Self { + pub fn unauthorized(description: String) -> Self { Self { location: ErrorLocation::Body, + context: description.clone(), description, - context: description.to_owned(), ..Self::default() } } @@ -166,7 +188,7 @@ struct ErrorResponse { struct ErrorInstance { location: ErrorLocation, name: String, - description: &'static str, + description: String, } impl From<&TokenserverError> for ErrorResponse { @@ -176,7 +198,7 @@ impl From<&TokenserverError> for ErrorResponse { errors: [ErrorInstance { location: error.location, name: error.name.clone(), - description: error.description, + description: error.description.clone(), }], } } @@ -215,3 +237,28 @@ impl Serialize for TokenserverError { ErrorResponse::from(self).serialize(serializer) } } + +impl From for TokenserverError { + fn from(db_error: DbError) -> Self { + TokenserverError { + description: db_error.to_string(), + context: db_error.to_string(), + backtrace: db_error.backtrace, + http_status: if db_error.status.is_server_error() { + // Use the status code from the DbError if it already suggests an internal error; + // it might be more specific than `StatusCode::SERVICE_UNAVAILABLE` + db_error.status + } else { + StatusCode::SERVICE_UNAVAILABLE + }, + // An unhandled DbError in the Tokenserver code is an internal error + ..TokenserverError::internal_error() + } + } +} + +impl From for HttpResponse { + fn from(inner: TokenserverError) -> Self { + ResponseError::error_response(&inner) + } +} diff --git a/tools/integration_tests/tokenserver/test_interchangeability.py b/tools/integration_tests/tokenserver/test_interchangeability.py deleted file mode 100644 index 10d24e84..00000000 --- a/tools/integration_tests/tokenserver/test_interchangeability.py +++ /dev/null @@ -1,29 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. -import unittest - -from tokenserver.test_support import TestCase - - -class TestInterchangeability(TestCase, unittest.TestCase): - def setUp(self): - super(TestInterchangeability, self).setUp() - - def tearDown(self): - super(TestInterchangeability, self).tearDown() - - def test_generation_change(self): - pass - - def test_keys_changed_at_change(self): - pass - - def test_client_state_change(self): - pass - - def test_replaced_at(self): - pass - - def test_node_reassignment(self): - pass diff --git a/tools/integration_tests/tokenserver/test_node_assignment.py b/tools/integration_tests/tokenserver/test_node_assignment.py index f21eb3f8..c8b41326 100644 --- a/tools/integration_tests/tokenserver/test_node_assignment.py +++ b/tools/integration_tests/tokenserver/test_node_assignment.py @@ -131,4 +131,16 @@ class TestNodeAssignment(TestCase, unittest.TestCase): client_state='aaaa') # All of these nodes are completely full, and no capacity can be # released - self.app.get('/1.0/sync/1.5', headers=headers, status=503) + res = self.app.get('/1.0/sync/1.5', headers=headers, status=503) + # The response has the expected body + expected_error_response = { + 'errors': [ + { + 'description': 'Unexpected error: unable to get a node', + 'location': 'internal', + 'name': '' + } + ], + 'status': 'internal-error' + } + self.assertEqual(res.json, expected_error_response)