Add logging to /login failure (#4688)

Author: sandhose

crates/handlers/src/admin/v1/users/set_password.rs

@@ -145,7 +145,7 @@ mod tests {
     use zeroize::Zeroizing;
 
     use crate::{
-        passwords::PasswordManager,
+        passwords::{PasswordManager, PasswordVerificationResult},
         test_utils::{RequestBuilderExt, ResponseExt, TestState, setup},
     };
 
@@ -185,7 +185,7 @@ mod tests {
         let mut repo = state.repository().await.unwrap();
         let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
         let password = Zeroizing::new(String::from("this is a good enough password"));
-        state
+        let res = state
             .password_manager
             .verify(
                 user_password.version,
@@ -194,6 +194,7 @@ mod tests {
             )
             .await
             .unwrap();
+        assert_eq!(res, PasswordVerificationResult::Success(()));
     }
 
     #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
@@ -244,7 +245,7 @@ mod tests {
         let mut repo = state.repository().await.unwrap();
         let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
         let password = Zeroizing::new("password".to_owned());
-        state
+        let res = state
             .password_manager
             .verify(
                 user_password.version,
@@ -253,6 +254,7 @@ mod tests {
             )
             .await
             .unwrap();
+        assert_eq!(res, PasswordVerificationResult::Success(()));
     }
 
     #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]

crates/handlers/src/compat/login.rs

@@ -32,7 +32,8 @@ use zeroize::Zeroizing;
 use super::{MatrixError, MatrixJsonBody};
 use crate::{
     BoundActivityTracker, Limiter, METER, RequesterFingerprint, impl_from_error_for_route,
-    passwords::PasswordManager, rate_limit::PasswordCheckLimitedError,
+    passwords::{PasswordManager, PasswordVerificationResult},
+    rate_limit::PasswordCheckLimitedError,
 };
 
 static LOGIN_COUNTER: LazyLock<Counter<u64>> = LazyLock::new(|| {
@@ -193,7 +194,7 @@ pub enum RouteError {
     NoPassword,
 
     #[error("password verification failed")]
-    PasswordVerificationFailed(#[source] anyhow::Error),
+    PasswordMismatch,
 
     #[error("request rate limited")]
     RateLimited(#[from] PasswordCheckLimitedError),
@@ -210,6 +211,12 @@ pub enum RouteError {
 
 impl_from_error_for_route!(mas_storage::RepositoryError);
 
+impl From<anyhow::Error> for RouteError {
+    fn from(err: anyhow::Error) -> Self {
+        Self::Internal(err.into())
+    }
+}
+
 impl IntoResponse for RouteError {
     fn into_response(self) -> axum::response::Response {
         let sentry_event_id =
@@ -241,13 +248,11 @@ impl IntoResponse for RouteError {
                 error: "Missing property 'identifier",
                 status: StatusCode::BAD_REQUEST,
             },
-            Self::UserNotFound | Self::NoPassword | Self::PasswordVerificationFailed(_) => {
-                MatrixError {
-                    errcode: "M_FORBIDDEN",
-                    error: "Invalid username/password",
-                    status: StatusCode::FORBIDDEN,
-                }
-            }
+            Self::UserNotFound | Self::NoPassword | Self::PasswordMismatch => MatrixError {
+                errcode: "M_FORBIDDEN",
+                error: "Invalid username/password",
+                status: StatusCode::FORBIDDEN,
+            },
             Self::LoginTookTooLong => MatrixError {
                 errcode: "M_FORBIDDEN",
                 error: "Login token expired",
@@ -576,28 +581,32 @@ async fn user_password_login(
     // Verify the password
     let password = Zeroizing::new(password);
 
-    let new_password_hash = password_manager
+    match password_manager
         .verify_and_upgrade(
             &mut rng,
             user_password.version,
             password,
             user_password.hashed_password.clone(),
         )
-        .await
-        .map_err(RouteError::PasswordVerificationFailed)?;
-
-    if let Some((version, hashed_password)) = new_password_hash {
-        // Save the upgraded password if needed
-        repo.user_password()
-            .add(
-                &mut rng,
-                clock,
-                &user,
-                version,
-                hashed_password,
-                Some(&user_password),
-            )
-            .await?;
+        .await?
+    {
+        PasswordVerificationResult::Success(Some((version, hashed_password))) => {
+            // Save the upgraded password if needed
+            repo.user_password()
+                .add(
+                    &mut rng,
+                    clock,
+                    &user,
+                    version,
+                    hashed_password,
+                    Some(&user_password),
+                )
+                .await?;
+        }
+        PasswordVerificationResult::Success(None) => {}
+        PasswordVerificationResult::Failure => {
+            return Err(RouteError::PasswordMismatch);
+        }
     }
 
     // We're about to create a device, let's explicitly acquire a lock, so that