bug: fix metrics and BrowserID error context (#1294)

mozilla-services · Apr 29, 2022 · a086a11 · a086a11
1 parent 8ff7a40
commit a086a11
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 9 deletions.
diff --git a/src/tokenserver/auth/browserid.rs b/src/tokenserver/auth/browserid.rs
@@ -122,6 +122,12 @@ impl VerifyToken for RemoteVerifier {
                     ..Default::default()
                 })
             }
+            VerifyResponse::Failure {
+                reason: Some(reason),
+            } => Err(TokenserverError {
+                context: format!("BrowserID verification error: {}", reason),
+                ..TokenserverError::invalid_credentials("Unauthorized")
+            }),
             VerifyResponse::Failure { .. } => Err(TokenserverError {
                 context: "Unknown BrowserID verification error".to_owned(),
                 ..TokenserverError::invalid_credentials("Unauthorized")
@@ -391,7 +397,7 @@ mod tests {
             assert_eq!(expected_error, error);
         }
 
-        // {"status": "error"} in body with random reason
+        // {"status": "failure"} in body with random reason
         {
             let mock = mockito::mock("POST", "/v2")
                 .with_header("content-type", "application/json")
@@ -401,6 +407,22 @@ mod tests {
             let error = verifier.verify(assertion.to_owned()).await.unwrap_err();
             mock.assert();
 
+            let expected_error = TokenserverError {
+                context: "BrowserID verification error: something broke".to_owned(),
+                ..TokenserverError::invalid_credentials("Unauthorized")
+            };
+            assert_eq!(expected_error, error);
+        }
+        // {"status": "failure"} in body with no reason
+        {
+            let mock = mockito::mock("POST", "/v2")
+                .with_header("content-type", "application/json")
+                .with_body("{\"status\": \"failure\"}")
+                .create();
+
+            let error = verifier.verify(assertion.to_owned()).await.unwrap_err();
+            mock.assert();
+
             let expected_error = TokenserverError {
                 context: "Unknown BrowserID verification error".to_owned(),
                 ..TokenserverError::invalid_credentials("Unauthorized")

diff --git a/src/tokenserver/db/models.rs b/src/tokenserver/db/models.rs
@@ -215,7 +215,7 @@ impl TokenserverDb {
         const DEFAULT_CAPACITY_RELEASE_RATE: f32 = 0.1;
 
         let mut metrics = self.metrics.clone();
-        metrics.start_timer("tokenserver.storage.get_best_node", None);
+        metrics.start_timer("storage.get_best_node", None);
 
         // We may have to retry the query if we need to release more capacity. This loop allows
         // a maximum of five retries before bailing out.
@@ -407,7 +407,7 @@ impl TokenserverDb {
     /// Creates a new user and assigns them to a node.
     fn allocate_user_sync(&self, params: params::AllocateUser) -> DbResult<results::AllocateUser> {
         let mut metrics = self.metrics.clone();
-        metrics.start_timer("tokenserver.storage.allocate_user", None);
+        metrics.start_timer("storage.allocate_user", None);
 
         // Get the least-loaded node
         let node = self.get_best_node_sync(params::GetBestNode {

diff --git a/src/tokenserver/db/pool.rs b/src/tokenserver/db/pool.rs
@@ -98,7 +98,7 @@ impl From<actix_web::error::BlockingError<DbError>> for DbError {
 impl DbPool for TokenserverPool {
     async fn get(&self) -> Result<Box<dyn Db>, DbError> {
         let mut metrics = self.metrics.clone();
-        metrics.start_timer("tokenserver.storage.get_pool", None);
+        metrics.start_timer("storage.get_pool", None);
 
         let pool = self.clone();
         let conn = block(move || pool.inner.get().map_err(DbError::from)).await?;

diff --git a/src/tokenserver/extractors.rs b/src/tokenserver/extractors.rs
@@ -411,7 +411,7 @@ impl FromRequest for AuthData {
             let token = Token::extract(&req).await?;
 
             let TokenserverMetrics(mut metrics) = TokenserverMetrics::extract(&req).await?;
-            metrics.start_timer("tokenserver.token_verification", None);
+            metrics.start_timer("token_verification", None);
 
             match token {
                 Token::BrowserIdAssertion(assertion) => {

diff --git a/src/tokenserver/handlers.rs b/src/tokenserver/handlers.rs
@@ -16,9 +16,8 @@ use super::{
     },
     error::TokenserverError,
     extractors::TokenserverRequest,
-    NodeType,
+    NodeType, TokenserverMetrics,
 };
-use crate::server::metrics::Metrics;
 
 #[derive(Debug, Serialize)]
 pub struct TokenserverResult {
@@ -35,14 +34,14 @@ pub struct TokenserverResult {
 pub async fn get_tokenserver_result(
     req: TokenserverRequest,
     db: Box<dyn Db>,
-    mut metrics: Metrics,
+    TokenserverMetrics(mut metrics): TokenserverMetrics,
 ) -> Result<HttpResponse, Error> {
     let updates = update_user(&req, db).await?;
 
     let (token, derived_secret) = {
         let token_plaintext = get_token_plaintext(&req, &updates)?;
 
-        metrics.start_timer("tokenserver.token_creation", None);
+        metrics.start_timer("token_creation", None);
         // Get the token and secret
         Tokenlib::get_token_and_derived_secret(token_plaintext, &req.shared_secret)?
     };