diff --git a/crates/api_clients/artcraft/artcraft_client/src/utils/login_challenge_client.rs b/crates/api_clients/artcraft/artcraft_client/src/utils/login_challenge_client.rs index 4d1539494d..1c2bf91af7 100644 --- a/crates/api_clients/artcraft/artcraft_client/src/utils/login_challenge_client.rs +++ b/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, pub message: &'static str, + /// The server's explanation for a non-2xx response, when it sent one. + pub maybe_rejection: Option, +} + +/// 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, + pub error_message: Option, + #[serde(default)] + pub error_fields: BTreeMap, } 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 { + 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() +} diff --git a/crates/desktop/artcraft/src/services/storyteller/commands/login_bridge.rs b/crates/desktop/artcraft/src/services/storyteller/commands/login_bridge.rs index 863860c0c2..286acbd31d 100644 --- a/crates/desktop/artcraft/src/services/storyteller/commands/login_bridge.rs +++ b/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 { 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; diff --git a/crates/desktop/artcraft/src/services/storyteller/commands/mod.rs b/crates/desktop/artcraft/src/services/storyteller/commands/mod.rs index b526c8f6f0..ca86129fd1 100644 --- a/crates/desktop/artcraft/src/services/storyteller/commands/mod.rs +++ b/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; diff --git a/crates/desktop/artcraft/src/services/storyteller/commands/password_auth_error_message.rs b/crates/desktop/artcraft/src/services/storyteller/commands/password_auth_error_message.rs new file mode 100644 index 0000000000..cb92c6749b --- /dev/null +++ b/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 { + 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 = 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::>(), + }), + ..status_only(status) + } + } +} diff --git a/crates/desktop/artcraft/src/services/storyteller/commands/storyteller_login_bridge_tests.rs b/crates/desktop/artcraft/src/services/storyteller/commands/storyteller_login_bridge_tests.rs index 2233f72997..1dfd39723c 100644 --- a/crates/desktop/artcraft/src/services/storyteller/commands/storyteller_login_bridge_tests.rs +++ b/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();