feat: add __error__ endpoint to Tokenserver (#1375)

Closes #1364

Cargo.lock

@@ -3273,6 +3273,7 @@ dependencies = [
  "backtrace",
  "serde 1.0.135",
  "serde_json",
+ "syncstorage-common",
  "syncstorage-db-common",
  "thiserror",
 ]

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<String>;
+}

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)
     }
 

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<String> {
-        self.kind.metric_label()
-    }
 }
 
 impl Error for ApiError {
@@ -274,3 +261,22 @@ impl From<DbError> 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<String> {
+        self.kind.metric_label()
+    }
+}

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)

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);

syncstorage/src/tokenserver/handlers.rs

@@ -220,3 +220,13 @@ pub async fn heartbeat(db: Box<dyn Db>) -> Result<HttpResponse, Error> {
         }
     }
 }
+
+/// Generates an error to test the Sentry integration
+pub async fn test_error() -> Result<HttpResponse, TokenserverError> {
+    error!("Test Error");
+    Err(TokenserverError {
+        context: "Test error for Sentry".to_owned(),
+        description: "Test error for Sentry".to_owned(),
+        ..TokenserverError::internal_error()
+    })
+}

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::<ApiError>() {
-                        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::<TokenserverError>() {
+                        process_error(tokenserver_error, metrics.as_ref(), &tags);
                     }
                 }
             }
@@ -156,17 +150,37 @@ where
     }
 }
 
-/// Custom `sentry::event_from_error` for `ApiError`
+fn process_error<E>(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<E>(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::<E>() {
             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<E>(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
 }
 

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"

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<TokenserverError> 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<String> {
+        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
+        }
+    }
+}