Review comments

Author: erikjohnston

crates/data-model/src/users.rs

@@ -47,14 +47,6 @@ impl User {
     }
 }
 
-impl Display for User {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        // Format the user as their username, this is useful for logging and
-        // debugging.
-        self.username.fmt(f)
-    }
-}
-
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 pub struct Password {
     pub id: Ulid,

crates/handlers/src/views/login.rs

@@ -190,7 +190,7 @@ pub(crate) async fn post(
     // First, lookup the user
     let Some(user) = get_user_by_email_or_by_username(site_config, &mut repo, username).await?
     else {
-        tracing::warn!("User not found: {username}");
+        tracing::warn!(username, "User not found");
         let form_state = form_state.with_error_on_form(FormError::InvalidCredentials);
         PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         return render(
@@ -209,7 +209,7 @@ pub(crate) async fn post(
 
     // Check the rate limit
     if let Err(e) = limiter.check_password(requester, &user) {
-        tracing::warn!(error = &e as &dyn std::error::Error);
+        tracing::warn!(error = &e as &dyn std::error::Error, "ratelimit exceeded");
         let form_state = form_state.with_error_on_form(FormError::RateLimitExceeded);
         PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         return render(
@@ -230,7 +230,7 @@ pub(crate) async fn post(
     let Some(user_password) = repo.user_password().active(&user).await? else {
         // There is no password for this user, but we don't want to disclose that. Show
         // a generic 'invalid credentials' error instead
-        tracing::warn!("No password for user: {user}");
+        tracing::warn!(username, "No password for user");
         let form_state = form_state.with_error_on_form(FormError::InvalidCredentials);
         PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         return render(
@@ -274,7 +274,7 @@ pub(crate) async fn post(
         }
         Ok(PasswordVerificationResult::Success(None)) => user_password,
         Ok(PasswordVerificationResult::Failure) => {
-            tracing::warn!("Failed to verify/upgrade password for user: {user}");
+            tracing::warn!(username, "Failed to verify/upgrade password for user");
             let form_state = form_state.with_error_on_form(FormError::InvalidCredentials);
             PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "mismatch")]);
             return render(
@@ -296,7 +296,7 @@ pub(crate) async fn post(
     // Now that we have checked the user password, we now want to show an error if
     // the user is locked or deactivated
     if user.deactivated_at.is_some() {
-        tracing::warn!("User is deactivated: {user}");
+        tracing::warn!(username, "User is deactivated");
         PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng);
         let ctx = AccountInactiveContext::new(user)
@@ -307,7 +307,7 @@ pub(crate) async fn post(
     }
 
     if user.locked_at.is_some() {
-        tracing::warn!("User is locked: {user}");
+        tracing::warn!(username, "User is locked");
         PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]);
         let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng);
         let ctx = AccountInactiveContext::new(user)