Skip to content

Commit 38088e5

Browse files
authored
Add logging to /login failure (#4688)
2 parents 5c4b3c8 + 35a8d6c commit 38088e5

File tree

4 files changed

+260
-95
lines changed

4 files changed

+260
-95
lines changed

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ mod tests {
145145
use zeroize::Zeroizing;
146146

147147
use crate::{
148-
passwords::PasswordManager,
148+
passwords::{PasswordManager, PasswordVerificationResult},
149149
test_utils::{RequestBuilderExt, ResponseExt, TestState, setup},
150150
};
151151

@@ -185,7 +185,7 @@ mod tests {
185185
let mut repo = state.repository().await.unwrap();
186186
let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
187187
let password = Zeroizing::new(String::from("this is a good enough password"));
188-
state
188+
let res = state
189189
.password_manager
190190
.verify(
191191
user_password.version,
@@ -194,6 +194,7 @@ mod tests {
194194
)
195195
.await
196196
.unwrap();
197+
assert_eq!(res, PasswordVerificationResult::Success(()));
197198
}
198199

199200
#[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]
@@ -244,7 +245,7 @@ mod tests {
244245
let mut repo = state.repository().await.unwrap();
245246
let user_password = repo.user_password().active(&user).await.unwrap().unwrap();
246247
let password = Zeroizing::new("password".to_owned());
247-
state
248+
let res = state
248249
.password_manager
249250
.verify(
250251
user_password.version,
@@ -253,6 +254,7 @@ mod tests {
253254
)
254255
.await
255256
.unwrap();
257+
assert_eq!(res, PasswordVerificationResult::Success(()));
256258
}
257259

258260
#[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")]

crates/handlers/src/compat/login.rs

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ use zeroize::Zeroizing;
3232
use super::{MatrixError, MatrixJsonBody};
3333
use crate::{
3434
BoundActivityTracker, Limiter, METER, RequesterFingerprint, impl_from_error_for_route,
35-
passwords::PasswordManager, rate_limit::PasswordCheckLimitedError,
35+
passwords::{PasswordManager, PasswordVerificationResult},
36+
rate_limit::PasswordCheckLimitedError,
3637
};
3738

3839
static LOGIN_COUNTER: LazyLock<Counter<u64>> = LazyLock::new(|| {
@@ -193,7 +194,7 @@ pub enum RouteError {
193194
NoPassword,
194195

195196
#[error("password verification failed")]
196-
PasswordVerificationFailed(#[source] anyhow::Error),
197+
PasswordMismatch,
197198

198199
#[error("request rate limited")]
199200
RateLimited(#[from] PasswordCheckLimitedError),
@@ -210,6 +211,12 @@ pub enum RouteError {
210211

211212
impl_from_error_for_route!(mas_storage::RepositoryError);
212213

214+
impl From<anyhow::Error> for RouteError {
215+
fn from(err: anyhow::Error) -> Self {
216+
Self::Internal(err.into())
217+
}
218+
}
219+
213220
impl IntoResponse for RouteError {
214221
fn into_response(self) -> axum::response::Response {
215222
let sentry_event_id =
@@ -241,13 +248,11 @@ impl IntoResponse for RouteError {
241248
error: "Missing property 'identifier",
242249
status: StatusCode::BAD_REQUEST,
243250
},
244-
Self::UserNotFound | Self::NoPassword | Self::PasswordVerificationFailed(_) => {
245-
MatrixError {
246-
errcode: "M_FORBIDDEN",
247-
error: "Invalid username/password",
248-
status: StatusCode::FORBIDDEN,
249-
}
250-
}
251+
Self::UserNotFound | Self::NoPassword | Self::PasswordMismatch => MatrixError {
252+
errcode: "M_FORBIDDEN",
253+
error: "Invalid username/password",
254+
status: StatusCode::FORBIDDEN,
255+
},
251256
Self::LoginTookTooLong => MatrixError {
252257
errcode: "M_FORBIDDEN",
253258
error: "Login token expired",
@@ -576,28 +581,32 @@ async fn user_password_login(
576581
// Verify the password
577582
let password = Zeroizing::new(password);
578583

579-
let new_password_hash = password_manager
584+
match password_manager
580585
.verify_and_upgrade(
581586
&mut rng,
582587
user_password.version,
583588
password,
584589
user_password.hashed_password.clone(),
585590
)
586-
.await
587-
.map_err(RouteError::PasswordVerificationFailed)?;
588-
589-
if let Some((version, hashed_password)) = new_password_hash {
590-
// Save the upgraded password if needed
591-
repo.user_password()
592-
.add(
593-
&mut rng,
594-
clock,
595-
&user,
596-
version,
597-
hashed_password,
598-
Some(&user_password),
599-
)
600-
.await?;
591+
.await?
592+
{
593+
PasswordVerificationResult::Success(Some((version, hashed_password))) => {
594+
// Save the upgraded password if needed
595+
repo.user_password()
596+
.add(
597+
&mut rng,
598+
clock,
599+
&user,
600+
version,
601+
hashed_password,
602+
Some(&user_password),
603+
)
604+
.await?;
605+
}
606+
PasswordVerificationResult::Success(None) => {}
607+
PasswordVerificationResult::Failure => {
608+
return Err(RouteError::PasswordMismatch);
609+
}
601610
}
602611

603612
// We're about to create a device, let's explicitly acquire a lock, so that

0 commit comments

Comments
 (0)