From 03a008fb5ccba0b6b876cd93b59ad2c4afe683ea Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Mon, 25 May 2026 20:21:12 -0400 Subject: [PATCH] use CommonWebError for alert visibility during google SSO (#1570) --- .../web/storyteller_web/src/docs/api_doc.rs | 2 - .../users/google_sso/check_claims.rs | 13 ++- .../users/google_sso/google_sso_handler.rs | 94 ++++--------------- .../google_sso/handle_existing_sso_account.rs | 8 +- .../google_sso/handle_new_sso_account.rs | 6 +- ...andle_new_sso_account_for_existing_user.rs | 10 +- .../handle_new_sso_account_for_new_user.rs | 23 +++-- 7 files changed, 52 insertions(+), 104 deletions(-) diff --git a/crates/service/web/storyteller_web/src/docs/api_doc.rs b/crates/service/web/storyteller_web/src/docs/api_doc.rs index eaa3370da3..275ced2f5a 100644 --- a/crates/service/web/storyteller_web/src/docs/api_doc.rs +++ b/crates/service/web/storyteller_web/src/docs/api_doc.rs @@ -897,8 +897,6 @@ use crate::http_server::endpoints::media_files::list::list_batch_generated_redux GetWeightError, GetWeightPathInfo, GetWeightResponse, - GoogleCreateAccountErrorResponse, - GoogleCreateAccountErrorType, GoogleCreateAccountRequest, GoogleCreateAccountSuccessResponse, InferTtsError, diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/check_claims.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/check_claims.rs index e37391540f..44e8cf30f4 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/check_claims.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/check_claims.rs @@ -1,4 +1,5 @@ -use crate::http_server::endpoints::users::google_sso::google_sso_handler::{GoogleCreateAccountErrorResponse, GoogleCreateAccountRequest}; +use crate::http_server::common_responses::common_web_error::CommonWebError; +use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountRequest; use crate::state::certs::google_sign_in_cert::GoogleSignInCert; use google_sign_in::claims::claims::Claims; use google_sign_in::decode_and_verify_token_claims::decode_and_verify_token_claims; @@ -9,12 +10,12 @@ use std::collections::HashSet; pub async fn check_claims( request: &GoogleCreateAccountRequest, google_sign_in_cert: &GoogleSignInCert, -) -> Result { +) -> Result { let keys = google_sign_in_cert.fetch_key_map(false) .await .map_err(|e| { warn!("error downloading google certs: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(e) })?; let verification_options = Some(build_options()); @@ -28,7 +29,7 @@ pub async fn check_claims( .await .map_err(|e| { warn!("error refreshing google certs: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(e) })?; let verification_options = Some(build_options()); @@ -36,7 +37,9 @@ pub async fn check_claims( let claims = decode_and_verify_token_claims(&keys, &request.google_credential, verification_options) .map_err(|e| { warn!("error decoding google token claims: {:?}", e); - GoogleCreateAccountErrorResponse::bad_request() + // The detailed error is logged above; the user-facing message is intentionally + // generic so we don't leak JWT internals. + CommonWebError::BadInputWithSimpleMessage("invalid google credential".to_string()) })?; claims diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/google_sso_handler.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/google_sso_handler.rs index 49e8114d50..63d5536da3 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/google_sso_handler.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/google_sso_handler.rs @@ -3,24 +3,19 @@ //#![forbid(unused_mut)] //#![forbid(unused_variables)] -use std::fmt; -use std::fmt::Formatter; - -use crate::util::lookup::resolve_referral_info::resolve_referral_info; -use mysql_queries::queries::user_referrals::insert_user_referral::{insert_user_referral, InsertUserReferralArgs}; +use crate::http_server::common_responses::common_web_error::CommonWebError; use crate::http_server::endpoints::users::google_sso::check_claims::check_claims; use crate::http_server::endpoints::users::google_sso::handle_existing_sso_account::{handle_existing_sso_account, ExistingAccountArgs}; use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::{handle_new_sso_account, NewSsoArgs}; use crate::state::certs::google_sign_in_cert::GoogleSignInCert; +use crate::util::lookup::resolve_referral_info::resolve_referral_info; use actix_artcraft::sessions::user_sessions::http_user_session_manager::HttpUserSessionManager; -use actix_web::error::ResponseError; -use actix_web::http::StatusCode; use actix_web::web::{Data, Json}; use actix_web::{HttpRequest, HttpResponse}; use http_server_common::request::get_request_ip::get_request_ip; -use http_server_common::response::serialize_as_json_error::serialize_as_json_error; use log::{info, warn}; use mysql_queries::queries::google_sign_in_accounts::get_google_sign_in_account_by_subject::get_google_sign_in_account; +use mysql_queries::queries::user_referrals::insert_user_referral::{insert_user_referral, InsertUserReferralArgs}; use mysql_queries::queries::users::user_sessions::create_user_session_with_transactor::create_user_session_with_transactor; use mysql_queries::utils::transactor::Transactor; use sqlx::{Acquire, MySqlPool}; @@ -118,62 +113,6 @@ pub struct GoogleCreateAccountSuccessResponse { pub maybe_user_display_name: Option, } -#[derive(ToSchema, Serialize, Debug)] -pub struct GoogleCreateAccountErrorResponse { - pub success: bool, - pub error_type: GoogleCreateAccountErrorType, -} - -#[derive(ToSchema, Copy, Clone, Debug, Serialize)] -pub enum GoogleCreateAccountErrorType { - BadRequest, // Other request malformed errors, eg. bad Origin header - BadInput, - EmailTaken, - ServerError, - UsernameReserved, - UsernameTaken, -} - -impl GoogleCreateAccountErrorResponse { - pub fn server_error() -> Self { - Self { - success: false, - error_type: GoogleCreateAccountErrorType::ServerError, - } - } - - pub fn bad_request() -> Self { - Self { - success: false, - error_type: GoogleCreateAccountErrorType::BadRequest, - } - } -} - -// NB: Not using DeriveMore since Clion doesn't understand it. -impl fmt::Display for GoogleCreateAccountErrorResponse { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self.error_type) - } -} - -impl ResponseError for GoogleCreateAccountErrorResponse { - fn status_code(&self) -> StatusCode { - match self.error_type { - GoogleCreateAccountErrorType::BadRequest => StatusCode::BAD_REQUEST, - GoogleCreateAccountErrorType::BadInput => StatusCode::BAD_REQUEST, - GoogleCreateAccountErrorType::EmailTaken => StatusCode::BAD_REQUEST, - GoogleCreateAccountErrorType::ServerError => StatusCode::INTERNAL_SERVER_ERROR, - GoogleCreateAccountErrorType::UsernameReserved => StatusCode::BAD_REQUEST, - GoogleCreateAccountErrorType::UsernameTaken => StatusCode::BAD_REQUEST, - } - } - - fn error_response(&self) -> HttpResponse { - serialize_as_json_error(self) - } -} - /// Sign in or sign up with "Sign in with Google" credentials. /// This supports both sign up for new users and sign in for existing users. #[utoipa::path( @@ -182,9 +121,9 @@ impl ResponseError for GoogleCreateAccountErrorResponse { path = "/v1/accounts/google_sso", responses( (status = 200, description = "Success", body = GoogleCreateAccountSuccessResponse), - (status = 400, description = "Bad input", body = GoogleCreateAccountErrorResponse), - (status = 401, description = "Not authorized", body = GoogleCreateAccountErrorResponse), - (status = 500, description = "Server error", body = GoogleCreateAccountErrorResponse), + (status = 400, description = "Bad input", body = CommonWebError), + (status = 401, description = "Not authorized", body = CommonWebError), + (status = 500, description = "Server error", body = CommonWebError), ), params( ("request" = GoogleCreateAccountRequest, description = "Payload for Request"), @@ -196,7 +135,7 @@ pub async fn google_sso_handler( mysql_pool: Data, session_cookie_manager: Data, google_sign_in_cert: Data, -) -> Result +) -> Result { let claims = check_claims(&request, &google_sign_in_cert).await?; @@ -208,28 +147,28 @@ pub async fn google_sso_handler( .map(|s| s.to_string()) .ok_or_else(|| { warn!("no subject in google claims"); - GoogleCreateAccountErrorResponse::bad_request() + CommonWebError::BadInputWithSimpleMessage("no subject in google claims".to_string()) })?; let claims_email_address = claims.email() .map(|email| email.to_string()) .ok_or_else(|| { warn!("no email address in google claims"); - GoogleCreateAccountErrorResponse::bad_request() + CommonWebError::BadInputWithSimpleMessage("no email address in google claims".to_string()) })?; let mut mysql_connection = mysql_pool.acquire() .await .map_err(|e| { warn!("Could not acquire DB pool: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_error(e) })?; let maybe_sso_account = get_google_sign_in_account(&claims_subject, &mut *mysql_connection) .await .map_err(|err| { warn!("error getting google sign in account: {:?}", err); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(err) })?; let user_token; @@ -312,7 +251,7 @@ pub async fn google_sso_handler( .await .map_err(|e| { warn!("error creating user session: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(e) })?; info!("new user session created"); @@ -339,16 +278,16 @@ pub fn construct_http_response( user_token: &UserToken, maybe_user_display_name: Option, username_not_yet_customized: bool, -) -> Result { +) -> Result { let session_cookie = match session_cookie_manager.create_cookie(&session_token, &user_token) { Ok(cookie) => cookie, - Err(_) => return Err(GoogleCreateAccountErrorResponse::server_error()), + Err(err) => return Err(CommonWebError::from_error(err)), }; let signed_session = match session_cookie_manager.encode_session_payload(&session_token, &user_token) { Ok(payload) => payload, - Err(_) => return Err(GoogleCreateAccountErrorResponse::server_error()), + Err(err) => return Err(CommonWebError::from_error(err)), }; let response = GoogleCreateAccountSuccessResponse { @@ -358,8 +297,7 @@ pub fn construct_http_response( maybe_user_display_name, }; - let body = serde_json::to_string(&response) - .map_err(|_e| GoogleCreateAccountErrorResponse::server_error())?; + let body = serde_json::to_string(&response)?; Ok(HttpResponse::Ok() .cookie(session_cookie) diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_account.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_account.rs index 493071546d..be0b2e4e9d 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_account.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_account.rs @@ -1,4 +1,4 @@ -use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountErrorResponse; +use crate::http_server::common_responses::common_web_error::CommonWebError; use actix_web::HttpRequest; use google_sign_in::claims::claims::Claims; use http_server_common::request::get_request_ip::get_request_ip; @@ -21,7 +21,7 @@ pub struct ExistingAccountArgs<'a> { pub async fn handle_existing_sso_account( args: ExistingAccountArgs<'_> ) - -> Result + -> Result { let user_token = match &args.sso_account.maybe_user_token { Some(token) => token.clone(), @@ -30,7 +30,9 @@ pub async fn handle_existing_sso_account( // consider how to migrate accounts and handle all the various account states. // For now, we'll just deny this possibility. It should not happen. warn!("no user token for existing google sign in account!"); - return Err(GoogleCreateAccountErrorResponse::server_error()); + return Err(CommonWebError::server_error_with_message( + "no user token for existing google sign in account", + )); }, }; diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account.rs index 07d0fe32fc..96a85e8959 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account.rs @@ -1,4 +1,4 @@ -use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountErrorResponse; +use crate::http_server::common_responses::common_web_error::CommonWebError; use crate::http_server::endpoints::users::google_sso::handle_new_sso_account_for_existing_user::{handle_new_sso_account_for_existing_user, LinkArgs}; use crate::http_server::endpoints::users::google_sso::handle_new_sso_account_for_new_user::{handle_new_sso_account_for_new_user, CreateArgs}; use crate::util::canonicalize_email_for_users_table::canonicalize_email_for_users_table; @@ -33,7 +33,7 @@ pub struct NewSsoAccountInfo { pub async fn handle_new_sso_account( args: NewSsoArgs<'_> ) - -> Result + -> Result { // NB: We use this routine to "normalize" email addresses in the user table. // It won't necessarily match the email address in the Google claims. @@ -46,7 +46,7 @@ pub async fn handle_new_sso_account( .await .map_err(|err| { warn!("error looking up user by email: {:?}", err); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(err) })?; match maybe_user_account { diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_existing_user.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_existing_user.rs index 93c078751e..31f2b9210c 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_existing_user.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_existing_user.rs @@ -1,4 +1,4 @@ -use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountErrorResponse; +use crate::http_server::common_responses::common_web_error::CommonWebError; use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::NewSsoAccountInfo; pub use actix_web::HttpRequest; use google_sign_in::claims::claims::Claims; @@ -20,13 +20,13 @@ pub struct LinkArgs<'a> { pub async fn handle_new_sso_account_for_existing_user( args: LinkArgs<'_> ) - -> Result + -> Result { let mut transaction = args.mysql_connection.begin() .await .map_err(|e| { warn!("Could not begin transaction: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_error(e) })?; let ip_address = get_request_ip(&args.http_request); @@ -44,14 +44,14 @@ pub async fn handle_new_sso_account_for_existing_user( transaction: &mut transaction, }).await.map_err(|err| { warn!("error inserting google sign in account: {:?}", err); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(err) })?; transaction.commit() .await .map_err(|e| { warn!("Could not commit transaction: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_error(e) })?; Ok(NewSsoAccountInfo { diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_new_user.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_new_user.rs index 3cd5b99a6f..bce507df70 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_new_user.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_new_sso_account_for_new_user.rs @@ -1,4 +1,4 @@ -use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountErrorResponse; +use crate::http_server::common_responses::common_web_error::CommonWebError; use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::NewSsoAccountInfo; use crate::http_server::session::lookup::user_session_feature_flags::UserSessionFeatureFlags; use actix_artcraft::requests::get_request_signup_source_enum::get_request_signup_source_enum; @@ -31,13 +31,13 @@ pub struct CreateArgs<'a> { pub async fn handle_new_sso_account_for_new_user( args: CreateArgs<'_> ) - -> Result + -> Result { let mut transaction = args.mysql_connection.begin() .await .map_err(|e| { warn!("Could not begin transaction: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_error(e) })?; // Enroll users in studio temporarily. @@ -87,19 +87,26 @@ pub async fn handle_new_sso_account_for_new_user( }, Err(err) => { warn!("error creating account from google sso: {:?}", err); - return Err(GoogleCreateAccountErrorResponse::server_error()); + // CreateAccountError doesn't impl std::error::Error, so fold it into anyhow. + return Err(CommonWebError::from_anyhow_error( + anyhow::anyhow!("error creating account from google sso: {:?}", err), + )); }, } } let user_token = maybe_user_token.ok_or_else(|| { error!("no username without collision after several tries (token)"); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::server_error_with_message( + "no username without collision after several tries (token)", + ) })?; let user_display_name = maybe_user_display_name.ok_or_else(|| { error!("no username without collision after several tries (display name)"); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::server_error_with_message( + "no username without collision after several tries (display name)", + ) })?; let _token = insert_google_sign_in_account(InsertGoogleSignInArgs { @@ -115,14 +122,14 @@ pub async fn handle_new_sso_account_for_new_user( transaction: &mut transaction, }).await.map_err(|err| { warn!("error inserting google sign in account: {:?}", err); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_anyhow_error(err) })?; transaction.commit() .await .map_err(|e| { warn!("Could not commit transaction: {:?}", e); - GoogleCreateAccountErrorResponse::server_error() + CommonWebError::from_error(e) })?; Ok(NewSsoAccountInfo {