From 75231c8feb996c7aa8746aeb88c9c3d428245e25 Mon Sep 17 00:00:00 2001 From: Ethan Donowitz <8703826+ethowitz@users.noreply.github.com> Date: Thu, 18 Aug 2022 10:02:49 -0400 Subject: [PATCH] feat: add `__error__` endpoint to Tokenserver (#1375) Closes #1364 --- Cargo.lock | 1 + syncstorage-common/src/lib.rs | 6 ++ syncstorage-db-common/src/error.rs | 2 +- syncstorage/src/error.rs | 34 ++++++----- syncstorage/src/server/mod.rs | 3 + syncstorage/src/tokenserver/auth/browserid.rs | 23 +++++++- syncstorage/src/tokenserver/handlers.rs | 10 ++++ syncstorage/src/web/middleware/sentry.rs | 59 ++++++++++++------- tokenserver-common/Cargo.toml | 1 + tokenserver-common/src/error.rs | 34 ++++++++++- 10 files changed, 134 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3234c5ff..0c14e1ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3273,6 +3273,7 @@ dependencies = [ "backtrace", "serde 1.0.135", "serde_json", + "syncstorage-common", "syncstorage-db-common", "thiserror", ] diff --git a/syncstorage-common/src/lib.rs b/syncstorage-common/src/lib.rs index 3ca04e85..2b508038 100644 --- a/syncstorage-common/src/lib.rs +++ b/syncstorage-common/src/lib.rs @@ -19,3 +19,9 @@ macro_rules! impl_fmt_display { } }; } + +pub trait ReportableError { + fn error_backtrace(&self) -> String; + fn is_sentry_event(&self) -> bool; + fn metric_label(&self) -> Option; +} diff --git a/syncstorage-db-common/src/error.rs b/syncstorage-db-common/src/error.rs index f1fe90fc..d7526c40 100644 --- a/syncstorage-db-common/src/error.rs +++ b/syncstorage-db-common/src/error.rs @@ -65,7 +65,7 @@ impl DbError { DbErrorKind::Internal(msg.to_owned()).into() } - pub fn is_reportable(&self) -> bool { + pub fn is_sentry_event(&self) -> bool { !matches!(&self.kind, DbErrorKind::Conflict) } diff --git a/syncstorage/src/error.rs b/syncstorage/src/error.rs index 11c402e5..37d4754c 100644 --- a/syncstorage/src/error.rs +++ b/syncstorage/src/error.rs @@ -21,7 +21,7 @@ use serde::{ Serialize, }; -use syncstorage_common::{from_error, impl_fmt_display}; +use syncstorage_common::{from_error, impl_fmt_display, ReportableError}; use syncstorage_db_common::error::DbError; use thiserror::Error; @@ -96,15 +96,6 @@ impl ApiErrorKind { } impl ApiError { - pub fn is_reportable(&self) -> bool { - // Should we report this error to sentry? - self.status.is_server_error() - && match &self.kind { - ApiErrorKind::Db(dbe) => dbe.is_reportable(), - _ => self.kind.metric_label().is_none(), - } - } - fn weave_error_code(&self) -> WeaveError { match &self.kind { ApiErrorKind::Validation(ver) => ver.weave_error_code(), @@ -143,10 +134,6 @@ impl ApiError { pub fn is_bso_not_found(&self) -> bool { matches!(&self.kind, ApiErrorKind::Db(dbe) if dbe.is_bso_not_found()) } - - pub fn metric_label(&self) -> Option { - self.kind.metric_label() - } } impl Error for ApiError { @@ -274,3 +261,22 @@ impl From for ApiError { from_error!(HawkError, ApiError, ApiErrorKind::Hawk); from_error!(ValidationError, ApiError, ApiErrorKind::Validation); + +impl ReportableError for ApiError { + fn error_backtrace(&self) -> String { + format!("{:#?}", self.backtrace) + } + + fn is_sentry_event(&self) -> bool { + // Should we report this error to sentry? + self.status.is_server_error() + && match &self.kind { + ApiErrorKind::Db(dbe) => dbe.is_sentry_event(), + _ => self.kind.metric_label().is_none(), + } + } + + fn metric_label(&self) -> Option { + self.kind.metric_label() + } +} diff --git a/syncstorage/src/server/mod.rs b/syncstorage/src/server/mod.rs index 7a0f0efc..4e80d3a5 100644 --- a/syncstorage/src/server/mod.rs +++ b/syncstorage/src/server/mod.rs @@ -215,6 +215,9 @@ macro_rules! build_app_without_syncstorage { .body(include_str!("../../version.json")) })), ) + .service( + web::resource("/__error__").route(web::get().to(tokenserver::handlers::test_error)), + ) .service(web::resource("/").route(web::get().to(|_: HttpRequest| { HttpResponse::Found() .header(LOCATION, SYNC_DOCS_URL) diff --git a/syncstorage/src/tokenserver/auth/browserid.rs b/syncstorage/src/tokenserver/auth/browserid.rs index 1b10cbf2..a6fbefa7 100644 --- a/syncstorage/src/tokenserver/auth/browserid.rs +++ b/syncstorage/src/tokenserver/auth/browserid.rs @@ -1,7 +1,7 @@ use async_trait::async_trait; use reqwest::{Client as ReqwestClient, StatusCode}; use serde::{de::Deserializer, Deserialize, Serialize}; -use tokenserver_common::error::{ErrorLocation, TokenserverError}; +use tokenserver_common::error::{ErrorLocation, TokenType, TokenserverError}; use super::VerifyToken; use crate::tokenserver::settings::Settings; @@ -72,6 +72,7 @@ impl VerifyToken for RemoteVerifier { "Request error occurred during BrowserID request to FxA: {}", e ), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() } } else { @@ -81,6 +82,7 @@ impl VerifyToken for RemoteVerifier { "Unknown error occurred during BrowserID request to FxA: {}", e ), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) } } @@ -92,6 +94,7 @@ impl VerifyToken for RemoteVerifier { "FxA returned a status code other than 200 ({})", response.status().as_u16() ), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }); } @@ -106,6 +109,7 @@ impl VerifyToken for RemoteVerifier { "Invalid BrowserID verification response received from FxA: {}", e ), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() })?; @@ -117,6 +121,7 @@ impl VerifyToken for RemoteVerifier { status: "invalid-timestamp", location: ErrorLocation::Body, context: "Expired BrowserID assertion".to_owned(), + token_type: TokenType::BrowserId, ..Default::default() }) } @@ -124,14 +129,17 @@ impl VerifyToken for RemoteVerifier { reason: Some(reason), } => Err(TokenserverError { context: format!("BrowserID verification error: {}", reason), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Failure { .. } => Err(TokenserverError { context: "Unknown BrowserID verification error".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { issuer, .. } if issuer != self.issuer => Err(TokenserverError { context: "BrowserID issuer mismatch".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { @@ -139,6 +147,7 @@ impl VerifyToken for RemoteVerifier { .. } if !claims.token_verified() => Err(TokenserverError { context: "BrowserID assertion not verified".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }), VerifyResponse::Okay { @@ -227,6 +236,7 @@ impl IdpClaims { // If the fxa-generation claim is null, return an error. Some(None) => Err(TokenserverError { context: "null fxa-generation claim in BrowserID assertion".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_generation() }), } @@ -242,6 +252,7 @@ impl IdpClaims { status: "invalid-credentials", location: ErrorLocation::Body, context: "null fxa-keysChangedAt claim in BrowserID assertion".to_owned(), + token_type: TokenType::BrowserId, ..Default::default() }), } @@ -339,6 +350,7 @@ mod tests { let expected_error = TokenserverError { context: "FxA returned a status code other than 200 (500)".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -356,6 +368,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: expected value at line 1 column 1".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -373,6 +386,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: unknown variant `error`, expected `okay` or `failure` at line 1 column 18".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -390,6 +404,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: unknown variant `potato`, expected `okay` or `failure` at line 1 column 19".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -407,6 +422,7 @@ mod tests { let expected_error = TokenserverError { context: "BrowserID verification error: something broke".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; assert_eq!(expected_error, error); @@ -423,6 +439,7 @@ mod tests { let expected_error = TokenserverError { context: "Unknown BrowserID verification error".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; assert_eq!(expected_error, error); @@ -450,6 +467,7 @@ mod tests { let expected_error = TokenserverError { context: "BrowserID issuer mismatch".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::invalid_credentials("Unauthorized".to_owned()) }; let verifier = RemoteVerifier::try_from(&Settings { @@ -523,6 +541,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: invalid type: integer `42`, expected a string".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -544,6 +563,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: invalid type: null, expected a string".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); @@ -564,6 +584,7 @@ mod tests { let expected_error = TokenserverError { context: "Invalid BrowserID verification response received from FxA: error decoding response body: missing field `issuer`".to_owned(), + token_type: TokenType::BrowserId, ..TokenserverError::resource_unavailable() }; assert_eq!(expected_error, error); diff --git a/syncstorage/src/tokenserver/handlers.rs b/syncstorage/src/tokenserver/handlers.rs index 2fafe20c..8c709e75 100644 --- a/syncstorage/src/tokenserver/handlers.rs +++ b/syncstorage/src/tokenserver/handlers.rs @@ -220,3 +220,13 @@ pub async fn heartbeat(db: Box) -> Result { } } } + +/// Generates an error to test the Sentry integration +pub async fn test_error() -> Result { + error!("Test Error"); + Err(TokenserverError { + context: "Test error for Sentry".to_owned(), + description: "Test error for Sentry".to_owned(), + ..TokenserverError::internal_error() + }) +} diff --git a/syncstorage/src/web/middleware/sentry.rs b/syncstorage/src/web/middleware/sentry.rs index e7149d3b..d5cacf7d 100644 --- a/syncstorage/src/web/middleware/sentry.rs +++ b/syncstorage/src/web/middleware/sentry.rs @@ -1,5 +1,5 @@ use std::error::Error as StdError; -use std::task::Context; +use std::task::{Context, Poll}; use std::{cell::RefCell, rc::Rc}; use actix_web::{ @@ -9,12 +9,13 @@ use actix_web::{ }; use futures::future::{self, LocalBoxFuture}; use sentry::protocol::Event; -use std::task::Poll; +use sentry_backtrace::parse_stacktrace; +use syncstorage_common::ReportableError; +use tokenserver_common::error::TokenserverError; use crate::error::ApiError; use crate::server::{metrics::Metrics, ServerState}; use crate::web::tags::Tags; -use sentry_backtrace::parse_stacktrace; pub struct SentryWrapper; @@ -138,16 +139,9 @@ where } Some(e) => { if let Some(apie) = e.as_error::() { - if let Some(metrics) = metrics { - if let Some(label) = apie.metric_label() { - metrics.incr(&label); - } - } - if !apie.is_reportable() { - trace!("Sentry: Not reporting error: {:?}", apie); - return Ok(sresp); - } - report(&tags, event_from_error(apie)); + process_error(apie, metrics.as_ref(), &tags); + } else if let Some(tokenserver_error) = e.as_error::() { + process_error(tokenserver_error, metrics.as_ref(), &tags); } } } @@ -156,17 +150,37 @@ where } } -/// Custom `sentry::event_from_error` for `ApiError` +fn process_error(err: &E, metrics: Option<&Metrics>, tags: &Tags) +where + E: ReportableError + StdError + 'static, +{ + if let Some(metrics) = metrics { + if let Some(label) = err.metric_label() { + metrics.incr(&label); + } + } + + if err.is_sentry_event() { + report(tags, event_from_error(err)); + } else { + trace!("Sentry: Not reporting error: {:?}", err); + } +} + +/// Custom `sentry::event_from_error` for `ReportableError` /// /// `sentry::event_from_error` can't access `std::Error` backtraces as its /// `backtrace()` method is currently Rust nightly only. This function works -/// against `HandlerError` instead to access its backtrace. -pub fn event_from_error(err: &ApiError) -> Event<'static> { +/// against `ReportableError` instead to access its backtrace. +pub fn event_from_error(err: &E) -> Event<'static> +where + E: ReportableError + StdError + 'static, +{ let mut exceptions = vec![exception_from_error_with_backtrace(err)]; let mut source = err.source(); while let Some(err) = source { - let exception = if let Some(err) = err.downcast_ref() { + let exception = if let Some(err) = err.downcast_ref::() { exception_from_error_with_backtrace(err) } else { exception_from_error(err) @@ -183,14 +197,15 @@ pub fn event_from_error(err: &ApiError) -> Event<'static> { } } -/// Custom `exception_from_error` support function for `ApiError` +/// Custom `exception_from_error` support function for `ReportableError` /// /// Based moreso on sentry_failure's `exception_from_single_fail`. -fn exception_from_error_with_backtrace(err: &ApiError) -> sentry::protocol::Exception { +fn exception_from_error_with_backtrace(err: &E) -> sentry::protocol::Exception +where + E: ReportableError + StdError, +{ let mut exception = exception_from_error(err); - // format the stack trace with alternate debug to get addresses - let bt = format!("{:#?}", err.backtrace); - exception.stacktrace = parse_stacktrace(&bt); + exception.stacktrace = parse_stacktrace(&err.error_backtrace()); exception } diff --git a/tokenserver-common/Cargo.toml b/tokenserver-common/Cargo.toml index c7b2f447..09cc2ea1 100644 --- a/tokenserver-common/Cargo.toml +++ b/tokenserver-common/Cargo.toml @@ -8,5 +8,6 @@ actix-web = "3" backtrace = "0.3.61" serde = "1.0" serde_json = { version = "1.0", features = ["arbitrary_precision"] } +syncstorage-common = { path = "../syncstorage-common" } 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 1598043f..0d52eac8 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -1,4 +1,4 @@ -use std::{cmp::PartialEq, fmt}; +use std::{cmp::PartialEq, error::Error, fmt}; use actix_web::{http::StatusCode, HttpResponse, ResponseError}; use backtrace::Backtrace; @@ -6,6 +6,7 @@ use serde::{ ser::{SerializeMap, Serializer}, Serialize, }; +use syncstorage_common::ReportableError; use syncstorage_db_common::error::DbError; #[derive(Clone, Debug)] @@ -19,8 +20,17 @@ pub struct TokenserverError { /// distinguish between similar errors in Sentry. pub context: String, pub backtrace: Backtrace, + pub token_type: TokenType, } +#[derive(Clone, Debug)] +pub enum TokenType { + BrowserId, + Oauth, +} + +impl Error for TokenserverError {} + // We implement `PartialEq` manually here because `Backtrace` doesn't implement `PartialEq`, so we // can't derive it impl PartialEq for TokenserverError { @@ -50,6 +60,7 @@ impl Default for TokenserverError { http_status: StatusCode::UNAUTHORIZED, context: "Unauthorized".to_owned(), backtrace: Backtrace::new(), + token_type: TokenType::Oauth, } } } @@ -262,3 +273,24 @@ impl From for HttpResponse { ResponseError::error_response(&inner) } } + +impl ReportableError for TokenserverError { + fn error_backtrace(&self) -> String { + format!("{:#?}", self.backtrace) + } + + fn is_sentry_event(&self) -> bool { + self.http_status.is_server_error() && self.metric_label().is_none() + } + + fn metric_label(&self) -> Option { + if self.http_status.is_client_error() { + match self.token_type { + TokenType::BrowserId => Some("request.error.browser_id".to_owned()), + TokenType::Oauth => Some("request.error.oauth".to_owned()), + } + } else { + None + } + } +}