fix error messages

crates/api_clients/artcraft/artcraft_client/src/utils/login_challenge_client.rs

@@ -1,14 +1,19 @@
+use std::collections::BTreeMap;
 use std::time::Duration;
 
 use log::{info, warn};
 use reqwest::header::SET_COOKIE;
-use reqwest::{Client, Method, RequestBuilder};
+use reqwest::{Client, Method, RequestBuilder, Response};
 use serde::de::DeserializeOwned;
+use serde::Deserialize;
 use url::Url;
 
 use crate::utils::api_host::ApiHost;
 use crate::utils::storyteller_client_builder::storyteller_client_builder;
 
+/// Rejection bodies are small JSON objects; anything larger is not one of ours.
+const MAX_REJECTION_BODY_BYTES: usize = 16 * 1024;
+
 /// Native transport for the session bridge. No request/response bodies, cookies,
 /// or challenge credentials are logged, even at debug level. Never follows redirects.
 #[derive(Clone)]
@@ -21,6 +26,19 @@ pub struct LoginChallengeClient {
 pub struct LoginChallengeClientError {
   pub status: Option<u16>,
   pub message: &'static str,
+  /// The server's explanation for a non-2xx response, when it sent one.
+  pub maybe_rejection: Option<LoginRejection>,
+}
+
+/// The error fields of a rejected login or signup response, such as
+/// `{"error_type":"UsernameTaken","error_fields":{"username":"username is taken"}}`.
+/// Only these fields are parsed, so no credential can end up in this struct.
+#[derive(Debug, Default, Deserialize)]
+pub struct LoginRejection {
+  pub error_type: Option<String>,
+  pub error_message: Option<String>,
+  #[serde(default)]
+  pub error_fields: BTreeMap<String, String>,
 }
 
 impl LoginChallengeClient {
@@ -108,6 +126,7 @@ impl LoginChallengeClient {
       return Err(LoginChallengeClientError {
         status: Some(status.as_u16()),
         message: "Login server rejected the request",
+        maybe_rejection: read_rejection(response).await,
       });
     }
     let session_cookie = response
@@ -137,14 +156,29 @@ impl LoginChallengeClientError {
     Self {
       status: None,
       message,
+      maybe_rejection: None,
     }
   }
 }
 
 impl std::fmt::Display for LoginChallengeClientError {
   fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-    write!(f, "{} (HTTP {:?})", self.message, self.status)
+    match self.status {
+      Some(status) => write!(f, "{} (HTTP {})", self.message, status),
+      None => write!(f, "{}", self.message),
+    }
   }
 }
 
 impl std::error::Error for LoginChallengeClientError {}
+
+async fn read_rejection(response: Response) -> Option<LoginRejection> {
+  if response.content_length().is_some_and(|len| len > MAX_REJECTION_BODY_BYTES as u64) {
+    return None;
+  }
+  let body = response.bytes().await.ok()?;
+  if body.len() > MAX_REJECTION_BODY_BYTES {
+    return None;
+  }
+  serde_json::from_slice(&body).ok()
+}

crates/desktop/artcraft/src/services/storyteller/commands/login_bridge.rs

@@ -19,6 +19,7 @@ use tokio::sync::Mutex;
 use uuid_utils::uuid::generate_random_uuid;
 
 use crate::core::threads::main_window_thread::persist_storyteller_cookies_task::get_credentials_from_cookie_store;
+use crate::services::storyteller::commands::password_auth_error_message::password_auth_error_message;
 use crate::services::storyteller::state::storyteller_credential_manager::StorytellerCredentialManager;
 
 const MAX_PENDING_CHALLENGES: usize = 8;
@@ -74,12 +75,27 @@ impl DesktopLoginError {
       origin, error.status, error.message
     );
     Self {
-      retryable: error
-        .status
-        .map(|s| s == 429 || s >= 500)
-        .unwrap_or(error.message == "Unable to reach the login server"),
+      retryable: is_retryable(&error),
       status: error.status,
-      message: format!("{}: {} (HTTP {:?})", origin, error.message, error.status),
+      message: format!("{}: {}", origin, error),
+    }
+  }
+
+  /// Password login/signup errors are shown to the user verbatim, so they say
+  /// what went wrong ("That username is already taken.") instead of the host.
+  fn from_password_auth(origin: &str, error: LoginChallengeClientError, is_signup: bool) -> Self {
+    warn!(
+      "Password auth failed: origin={} signup={} status={:?} reason={} error_type={:?}",
+      origin,
+      is_signup,
+      error.status,
+      error.message,
+      error.maybe_rejection.as_ref().and_then(|r| r.error_type.as_deref())
+    );
+    Self {
+      retryable: is_retryable(&error),
+      status: error.status,
+      message: password_auth_error_message(&error, is_signup),
     }
   }
 }
@@ -120,12 +136,13 @@ pub(super) async fn password_auth(
 ) -> Result<SessionUserInfo, DesktopLoginError> {
   let client = LoginChallengeClient::new(host)
     .map_err(|e| DesktopLoginError::from_client(&host.to_api_hostname_and_scheme(), e))?;
+  let is_signup = signup.is_some();
   let response = match (login, signup) {
     (Some(request), None) => client.password_login(&request).await,
     (None, Some(request)) => client.password_signup(&request).await,
     _ => return Err(DesktopLoginError::local("Invalid authentication request")),
   }
-  .map_err(|e| DesktopLoginError::from_client(&client.api_origin(), e))?;
+  .map_err(|e| DesktopLoginError::from_password_auth(&client.api_origin(), e, is_signup))?;
   let (response, cookie) = response;
   if !response.success {
     return Err(DesktopLoginError::local(
@@ -335,6 +352,13 @@ fn expired() -> DesktopLoginOutcome {
   }
 }
 
+fn is_retryable(error: &LoginChallengeClientError) -> bool {
+  error
+    .status
+    .map(|s| s == 429 || s >= 500)
+    .unwrap_or(error.message == "Unable to reach the login server")
+}
+
 #[cfg(test)]
 #[path = "storyteller_login_bridge_tests.rs"]
 mod login_bridge_tests;

crates/desktop/artcraft/src/services/storyteller/commands/mod.rs

@@ -4,6 +4,7 @@ pub mod storyteller_get_subscription_command;
 pub mod stripe_checkout;
 pub mod stripe_customer_portal;
 pub(crate) mod login_bridge;
+mod password_auth_error_message;
 pub mod storyteller_cancel_login_challenge_command;
 pub mod storyteller_create_login_challenge_command;
 pub mod storyteller_get_login_session_command;

crates/desktop/artcraft/src/services/storyteller/commands/password_auth_error_message.rs

@@ -0,0 +1,254 @@
+use artcraft_client::utils::login_challenge_client::{LoginChallengeClientError, LoginRejection};
+
+/// Signup validation fields, in the order they appear on the form.
+const FORM_FIELD_ORDER: [&str; 3] = ["username", "email_address", "password"];
+
+/// Turns a rejected `/v1/login` or `/v1/create_account` call into a sentence a
+/// user can act on, e.g. "That username is already taken."
+pub(super) fn password_auth_error_message(error: &LoginChallengeClientError, is_signup: bool) -> String {
+  let Some(status) = error.status else {
+    return match error.message {
+      "Unable to reach the login server" => {
+        "Couldn't reach the ArtCraft servers. Check your internet connection and try again.".to_owned()
+      }
+      _ => "Something went wrong while signing in. Please try again.".to_owned(),
+    };
+  };
+  if status == 429 {
+    return "Too many attempts. Please wait a minute and try again.".to_owned();
+  }
+  if status >= 500 {
+    return "ArtCraft's servers are having trouble right now. Please try again in a few minutes.".to_owned();
+  }
+  if let Some(message) = error.maybe_rejection.as_ref().and_then(rejection_message) {
+    return message;
+  }
+  match (status, is_signup) {
+    (401, false) => "Incorrect username, email, or password.".to_owned(),
+    (_, true) => "We couldn't create your account. Please check your details and try again.".to_owned(),
+    (_, false) => "We couldn't sign you in. Please check your details and try again.".to_owned(),
+  }
+}
+
+fn rejection_message(rejection: &LoginRejection) -> Option<String> {
+  match rejection.error_type.as_deref() {
+    Some("InvalidCredentials") => return Some("Incorrect username, email, or password.".to_owned()),
+    Some("AccountNeedsPassword") => {
+      return Some(
+        "This account doesn't have a password yet. Sign in through the website, or reset your password.".to_owned(),
+      )
+    }
+    _ => {}
+  }
+
+  let ordered_fields = FORM_FIELD_ORDER
+    .iter()
+    .filter_map(|field| rejection.error_fields.get(*field))
+    .chain(
+      rejection
+        .error_fields
+        .iter()
+        .filter(|(field, _)| !FORM_FIELD_ORDER.contains(&field.as_str()))
+        .map(|(_, reason)| reason),
+    );
+  let field_messages: Vec<String> = ordered_fields.map(|reason| field_reason_message(reason)).collect();
+  if !field_messages.is_empty() {
+    return Some(field_messages.join(" "));
+  }
+
+  match rejection.error_type.as_deref() {
+    Some("UsernameTaken") => Some(field_reason_message("username is taken")),
+    Some("EmailTaken") => Some(field_reason_message("email is taken")),
+    Some("UsernameReserved") => Some(field_reason_message("username is reserved")),
+    _ => rejection
+      .error_message
+      .as_deref()
+      .filter(|message| !message.trim().is_empty())
+      .map(sentence),
+  }
+}
+
+/// Server validation reasons are terse lowercase phrases; known ones get a
+/// friendlier rewrite, and anything else is shown as a sentence.
+fn field_reason_message(reason: &str) -> String {
+  let friendly = match reason {
+    "username is taken" => "That username is already taken. Please choose another.",
+    "username is reserved" | "username contains slurs" => "That username isn't available. Please choose another.",
+    "username is too short" => "Usernames must be at least 3 characters.",
+    "username is too long" => "Usernames can be at most 16 characters.",
+    "invalid username characters" => {
+      "Usernames can only contain letters, numbers, underscores (_), and hyphens (-)."
+    }
+    "email is taken" => "An account with that email already exists. Try logging in instead.",
+    "invalid email address" => "Please enter a valid email address.",
+    "password is too short" => "Passwords must be at least 6 characters.",
+    "passwords do not match" => "Passwords do not match.",
+    other => return sentence(other),
+  };
+  friendly.to_owned()
+}
+
+fn sentence(text: &str) -> String {
+  let text = text.trim();
+  let mut chars = text.chars();
+  let mut sentence: String = match chars.next() {
+    Some(first) => first.to_uppercase().chain(chars).collect(),
+    None => return String::new(),
+  };
+  if !sentence.ends_with(['.', '!', '?']) {
+    sentence.push('.');
+  }
+  sentence
+}
+
+#[cfg(test)]
+mod tests {
+  use std::collections::BTreeMap;
+
+  use super::*;
+
+  mod signup_tests {
+    use super::*;
+
+    #[test]
+    fn username_taken() {
+      let error = rejected(400, Some("UsernameTaken"), None, &[("username", "username is taken")]);
+      assert_eq!(signup_message(&error), "That username is already taken. Please choose another.");
+    }
+
+    #[test]
+    fn email_taken() {
+      let error = rejected(400, Some("EmailTaken"), None, &[("email_address", "email is taken")]);
+      assert_eq!(signup_message(&error), "An account with that email already exists. Try logging in instead.");
+    }
+
+    #[test]
+    fn reserved_username_without_fields_uses_error_type() {
+      let error = rejected(400, Some("UsernameReserved"), None, &[]);
+      assert_eq!(signup_message(&error), "That username isn't available. Please choose another.");
+    }
+
+    #[test]
+    fn multiple_bad_fields_follow_form_order() {
+      let error = rejected(
+        400,
+        Some("BadInput"),
+        None,
+        &[("password", "password is too short"), ("email_address", "invalid email address"), ("username", "invalid username characters")],
+      );
+      assert_eq!(
+        signup_message(&error),
+        "Usernames can only contain letters, numbers, underscores (_), and hyphens (-). Please enter a valid email address. Passwords must be at least 6 characters."
+      );
+    }
+
+    #[test]
+    fn unknown_reason_is_shown_as_a_sentence() {
+      let error = rejected(400, Some("BadInput"), None, &[("username", "username cannot start with 'user_'")]);
+      assert_eq!(signup_message(&error), "Username cannot start with 'user_'.");
+    }
+
+    #[test]
+    fn unparseable_400_gets_generic_signup_message() {
+      let error = status_only(400);
+      assert_eq!(signup_message(&error), "We couldn't create your account. Please check your details and try again.");
+    }
+  }
+
+  mod login_tests {
+    use super::*;
+
+    #[test]
+    fn invalid_credentials() {
+      let error = rejected(401, Some("InvalidCredentials"), Some("invalid credentials"), &[]);
+      assert_eq!(login_message(&error), "Incorrect username, email, or password.");
+    }
+
+    #[test]
+    fn account_needs_password() {
+      let error = rejected(401, Some("AccountNeedsPassword"), Some("account was created without a password"), &[]);
+      assert_eq!(
+        login_message(&error),
+        "This account doesn't have a password yet. Sign in through the website, or reset your password."
+      );
+    }
+
+    #[test]
+    fn bare_401_is_treated_as_bad_credentials() {
+      assert_eq!(login_message(&status_only(401)), "Incorrect username, email, or password.");
+    }
+  }
+
+  mod transport_tests {
+    use super::*;
+
+    #[test]
+    fn rate_limited() {
+      assert_eq!(login_message(&status_only(429)), "Too many attempts. Please wait a minute and try again.");
+    }
+
+    #[test]
+    fn server_error() {
+      let error = rejected(500, Some("ServerError"), Some("server error"), &[]);
+      assert_eq!(
+        signup_message(&error),
+        "ArtCraft's servers are having trouble right now. Please try again in a few minutes."
+      );
+    }
+
+    #[test]
+    fn unreachable() {
+      let error = LoginChallengeClientError::invalid("Unable to reach the login server");
+      assert_eq!(
+        login_message(&error),
+        "Couldn't reach the ArtCraft servers. Check your internet connection and try again."
+      );
+    }
+
+    #[test]
+    fn messages_never_expose_raw_http_details() {
+      for status in [400, 401, 403, 404, 429, 500, 502] {
+        for message in [login_message(&status_only(status)), signup_message(&status_only(status))] {
+          assert!(!message.contains("HTTP"), "{message}");
+          assert!(!message.contains("Some("), "{message}");
+          assert!(!message.contains("://"), "{message}");
+        }
+      }
+    }
+  }
+
+  fn login_message(error: &LoginChallengeClientError) -> String {
+    password_auth_error_message(error, false)
+  }
+
+  fn signup_message(error: &LoginChallengeClientError) -> String {
+    password_auth_error_message(error, true)
+  }
+
+  fn status_only(status: u16) -> LoginChallengeClientError {
+    LoginChallengeClientError {
+      status: Some(status),
+      message: "Login server rejected the request",
+      maybe_rejection: None,
+    }
+  }
+
+  fn rejected(
+    status: u16,
+    error_type: Option<&str>,
+    error_message: Option<&str>,
+    error_fields: &[(&str, &str)],
+  ) -> LoginChallengeClientError {
+    LoginChallengeClientError {
+      maybe_rejection: Some(LoginRejection {
+        error_type: error_type.map(str::to_owned),
+        error_message: error_message.map(str::to_owned),
+        error_fields: error_fields
+          .iter()
+          .map(|(field, reason)| (field.to_string(), reason.to_string()))
+          .collect::<BTreeMap<_, _>>(),
+      }),
+      ..status_only(status)
+    }
+  }
+}

crates/desktop/artcraft/src/services/storyteller/commands/storyteller_login_bridge_tests.rs

@@ -263,6 +263,31 @@ async fn password_login_and_signup_verify_persist_and_recheck_via_native_api() {
   }
 }
 
+#[tokio::test]
+async fn password_signup_and_login_rejections_explain_what_went_wrong() {
+  let cases = [
+    (true, 400, json!({"success":false,"error_type":"UsernameTaken","error_fields":{"username":"username is taken"}}), "That username is already taken. Please choose another."),
+    (false, 401, json!({"success":false,"error_type":"InvalidCredentials","error_message":"invalid credentials"}), "Incorrect username, email, or password."),
+  ];
+  for (signup, status, body, expected) in cases {
+    let mut rejected = step(if signup { "/v1/create_account" } else { "/v1/login" }, body);
+    rejected.status = status;
+    let server = server(vec![rejected]);
+    let h = harness();
+    let (login, signup) = if signup {
+      (None, Some(PasswordSignupRequest { username: "google_user".into(), email_address: "user@example.test".into(), password: "fixture_password".into(), password_confirmation: "fixture_password".into(), signup_source: "artcraft".into() }))
+    } else {
+      (Some(PasswordLoginRequest { username_or_email: "google_user".into(), password: "fixture_password".into() }), None)
+    };
+    let error = password_auth(&server.host, &h.manager, &h.jar, login, signup).await.err().unwrap();
+    assert_eq!(error.message, expected);
+    assert_eq!(error.status, Some(status));
+    assert!(!error.retryable);
+    assert!(h.manager.get_credentials().unwrap().is_none());
+    server.thread.join().unwrap();
+  }
+}
+
 #[tokio::test]
 async fn empty_native_cookie_jar_needs_no_session_http_request() {
   let h = harness();