From a086a118445233c31ddd136feac74c207d707dd3 Mon Sep 17 00:00:00 2001 From: Ethan Donowitz <8703826+ethowitz@users.noreply.github.com> Date: Fri, 29 Apr 2022 10:14:17 -0400 Subject: [PATCH] bug: fix metrics and BrowserID error context (#1294) --- src/tokenserver/auth/browserid.rs | 24 +++++++++++++++++++++++- src/tokenserver/db/models.rs | 4 ++-- src/tokenserver/db/pool.rs | 2 +- src/tokenserver/extractors.rs | 2 +- src/tokenserver/handlers.rs | 7 +++---- 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/tokenserver/auth/browserid.rs b/src/tokenserver/auth/browserid.rs index 421657430e..f235270144 100644 --- 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 index a8bb10d651..ff8163d538 100644 --- 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 { 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 index e2088af295..a4cf97db19 100644 --- a/src/tokenserver/db/pool.rs +++ b/src/tokenserver/db/pool.rs @@ -98,7 +98,7 @@ impl From> for DbError { impl DbPool for TokenserverPool { async fn get(&self) -> Result, 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 index 0ae6b9a979..095e1b7867 100644 --- 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 index 9ebcce60ab..61beeeaf9b 100644 --- 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, - mut metrics: Metrics, + TokenserverMetrics(mut metrics): TokenserverMetrics, ) -> Result { 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)? };