From 21162c3df588bdd205affbcb4853bf982a446e7f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 25 Sep 2024 13:46:33 -0400 Subject: [PATCH] update sso details if changed, refactor --- .../queries/google_sign_in_accounts/mod.rs | 3 +- .../update_google_sign_in_account.rs | 61 +++++++++++++++++ .../users/google_sso/google_sso_handler.rs | 51 +++++++------- .../handle_creating_user_account.rs | 4 +- .../google_sso/handle_existing_sso_link.rs | 67 +++++++++++++++++++ .../handle_linking_existing_account.rs | 5 +- .../google_sso/handle_new_sso_account.rs | 43 ++++++------ .../endpoints/users/google_sso/mod.rs | 5 +- 8 files changed, 185 insertions(+), 54 deletions(-) create mode 100644 crates/lib/mysql_queries/src/queries/google_sign_in_accounts/update_google_sign_in_account.rs create mode 100644 crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_link.rs diff --git a/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/mod.rs b/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/mod.rs index 1dc42a973d..51ebdb3301 100644 --- a/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/mod.rs +++ b/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/mod.rs @@ -1,2 +1,3 @@ pub mod get_google_sign_in_account_by_subject; -pub mod insert_google_sign_in_account; \ No newline at end of file +pub mod insert_google_sign_in_account; +pub mod update_google_sign_in_account; \ No newline at end of file diff --git a/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/update_google_sign_in_account.rs b/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/update_google_sign_in_account.rs new file mode 100644 index 0000000000..e683ee431c --- /dev/null +++ b/crates/lib/mysql_queries/src/queries/google_sign_in_accounts/update_google_sign_in_account.rs @@ -0,0 +1,61 @@ +use sqlx; +use sqlx::Executor; + +use crate::utils::transactor::Transactor; +use errors::AnyhowResult; + +pub struct UpdateGoogleSignInArgs<'e, 't> { + pub subject: &'e str, + + pub email_address: &'e str, + + pub is_email_verified: bool, + + pub maybe_locale: Option<&'e str>, + + pub maybe_name: Option<&'e str>, + pub maybe_given_name: Option<&'e str>, + pub maybe_family_name: Option<&'e str>, + + pub creator_ip_address: &'e str, + + pub transactor: Transactor<'e, 't>, +} + +pub async fn update_google_sign_in_account<'e, 't>( + args: UpdateGoogleSignInArgs<'e, 't> +) -> AnyhowResult<()> +{ + let query = sqlx::query!( + r#" +UPDATE google_sign_in_accounts +SET + email_address = ?, + is_email_verified = ?, + + maybe_locale = ?, + + maybe_name = ?, + maybe_given_name = ?, + maybe_family_name = ?, + + ip_address_update = ?, + + version = version + 1 +WHERE + subject = ? + "#, + args.email_address, + args.is_email_verified, + args.maybe_locale, + args.maybe_name, + args.maybe_given_name, + args.maybe_family_name, + args.creator_ip_address, + args.subject, + ); + + let _r = args.transactor.execute(query).await?; + + Ok(()) +} 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 339548acfa..f602d130f4 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 @@ -10,7 +10,7 @@ use std::fmt::Formatter; use crate::http_server::endpoints::beta_keys::redeem_beta_key_handler::RedeemBetaKeyError; use crate::http_server::endpoints::users::create_account_handler::{CreateAccountErrorResponse, CreateAccountSuccessResponse}; use crate::http_server::endpoints::users::google_sso::check_claims::check_claims; -use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::handle_new_sso_account; +use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::{handle_new_sso_account, NewSsoArgs}; use crate::http_server::endpoints::users::session_info_handler::SessionInfoError; use crate::http_server::session::http::http_user_session_manager::HttpUserSessionManager; use crate::http_server::validations::is_reserved_username::is_reserved_username; @@ -48,7 +48,7 @@ use tokens::tokens::user_sessions::UserSessionToken; use tokens::tokens::users::UserToken; use user_input_common::check_for_slurs::contains_slurs; use utoipa::ToSchema; - +use crate::http_server::endpoints::users::google_sso::handle_existing_sso_link::{handle_existing_sso_link, ExistingLinkArgs}; /* ALGORITHM --> [SSO RECORD LOOKUP] --> I. SSO Record Exists @@ -93,12 +93,12 @@ use utoipa::ToSchema; - Other email providers may behave weirdly with their own canonicalization schemes. TODO: - - Handle existing account linkage - - Should we store canonical emails? - - User source enum - Store all the claims info - Verify all paths (including legacy sign up paths) - Update claims info on re-login (if diff exists) + + Decisions: + - Should we store canonical emails? --> No, you can create more than one account per gmail. */ #[derive(ToSchema, Deserialize)] @@ -211,13 +211,20 @@ pub async fn google_sso_handler( claims.email(), claims.email_verified()); - let subject = claims.subject() + let claims_subject = claims.subject() .map(|s| s.to_string()) .ok_or_else(|| { warn!("no subject in google claims"); GoogleCreateAccountErrorResponse::bad_request() })?; + let claims_email_address = claims.email() + .map(|email| email.to_string()) + .ok_or_else(|| { + warn!("no email address in google claims"); + GoogleCreateAccountErrorResponse::bad_request() + })?; + let mut mysql_connection = mysql_pool.acquire() .await .map_err(|e| { @@ -225,7 +232,7 @@ pub async fn google_sso_handler( GoogleCreateAccountErrorResponse::server_error() })?; - let maybe_sso_account = get_google_sign_in_account(&subject, &mut *mysql_connection) + 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); @@ -238,29 +245,25 @@ pub async fn google_sso_handler( match maybe_sso_account { Some(sso_account) => { - let user_token = match sso_account.maybe_user_token { - Some(token) => token.clone(), - None => { - // NB: If accounts get into this state (e.g. if we support de-linking), we'll need to - // consider how to migrate accounts and handle all the various account states. - // For now, we'll just deny this possibility. - warn!("no user token for existing google sign in account!"); - return Err(GoogleCreateAccountErrorResponse::server_error()); - }, - }; - - // TODO: Handle updating SSO details (eg new profile picture). + let user_token = handle_existing_sso_link(ExistingLinkArgs { + http_request: &http_request, + sso_account: &sso_account, + claims, + claims_email_address: &claims_email_address, + mysql_connection: &mut mysql_connection, + }).await?; maybe_user_token = Some(user_token); maybe_user_display_name = sso_account.maybe_user_display_name.clone(); }, None => { - let result = handle_new_sso_account( - &http_request, - &subject, - &mut mysql_connection, + let result = handle_new_sso_account(NewSsoArgs { + http_request: &http_request, claims, - ).await?; + claims_subject: &claims_subject, + claims_email_address: &claims_email_address, + mysql_connection: &mut mysql_connection, + }).await?; maybe_user_token = Some(result.user_token); maybe_user_display_name = Some(result.user_display_name); diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_creating_user_account.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_creating_user_account.rs index d646985b0f..0adc34ea36 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_creating_user_account.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_creating_user_account.rs @@ -21,7 +21,7 @@ pub struct CreateArgs<'a> { pub http_request: &'a HttpRequest, pub claims: Claims, pub claims_subject: &'a str, - pub claims_email: &'a str, + pub claims_email_address: &'a str, pub user_email_address: &'a str, pub mysql_connection: &'a mut PoolConnection, } @@ -98,7 +98,7 @@ pub async fn handle_creating_user_account( let _token = insert_google_sign_in_account(InsertGoogleSignInArgs { subject: args.claims_subject, maybe_user_token: Some(&user_token), - email_address: &args.claims_email, // NB: The one from the Google claims, not our canonicalized one. + email_address: &args.claims_email_address, // NB: The one from the Google claims, not our canonicalized one. is_email_verified: args.claims.email_verified(), maybe_locale: args.claims.locale(), maybe_name: args.claims.name(), diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_link.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_link.rs new file mode 100644 index 0000000000..0fe798c11a --- /dev/null +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_existing_sso_link.rs @@ -0,0 +1,67 @@ +use crate::http_server::endpoints::users::google_sso::google_sso_handler::GoogleCreateAccountErrorResponse; +use crate::http_server::endpoints::users::google_sso::handle_new_sso_account::NewSsoAccountInfo; +use actix_web::HttpRequest; +use google_sign_in::claims::claims::Claims; +use http_server_common::request::get_request_ip::get_request_ip; +use log::warn; +use mysql_queries::queries::google_sign_in_accounts::get_google_sign_in_account_by_subject::GoogleSignInAccount; +use mysql_queries::queries::google_sign_in_accounts::update_google_sign_in_account::{update_google_sign_in_account, UpdateGoogleSignInArgs}; +use mysql_queries::utils::transactor::Transactor; +use sqlx::pool::PoolConnection; +use sqlx::MySql; +use tokens::tokens::users::UserToken; + +pub struct ExistingLinkArgs<'a> { + pub http_request: &'a HttpRequest, + pub sso_account: &'a GoogleSignInAccount, + pub claims: Claims, + pub claims_email_address: &'a str, + pub mysql_connection: &'a mut PoolConnection, +} + +pub async fn handle_existing_sso_link( + args: ExistingLinkArgs<'_> +) + -> Result +{ + let user_token = match &args.sso_account.maybe_user_token { + Some(token) => token.clone(), + None => { + // NB: If accounts get into this state (e.g. if we support de-linking), we'll need to + // consider how to migrate accounts and handle all the various account states. + // For now, we'll just deny this possibility. + warn!("no user token for existing google sign in account!"); + return Err(GoogleCreateAccountErrorResponse::server_error()); + }, + }; + + if should_update_sso_claims(args.sso_account, &args.claims) { + let ip_address = get_request_ip(args.http_request); + + update_google_sign_in_account(UpdateGoogleSignInArgs { + subject: &args.sso_account.subject, + email_address: args.claims_email_address, + is_email_verified: args.claims.email_verified(), + maybe_locale: args.claims.locale(), + maybe_name: args.claims.name(), + maybe_given_name: args.claims.given_name(), + maybe_family_name: args.claims.family_name(), + creator_ip_address: &ip_address, + transactor: Transactor::for_connection(args.mysql_connection), + }).await.map_err(|err| { + warn!("error updating google sign in account: {:?}", err); + GoogleCreateAccountErrorResponse::server_error() + })?; + } + + Ok(user_token) +} + +fn should_update_sso_claims(sso_account: &GoogleSignInAccount, claims: &Claims) -> bool { + sso_account.email_address.as_deref() != claims.email() + || sso_account.is_email_verified != claims.email_verified() + || sso_account.maybe_locale.as_deref() != claims.locale() + || sso_account.maybe_name.as_deref() != claims.name() + || sso_account.maybe_given_name.as_deref() != claims.given_name() + || sso_account.maybe_family_name.as_deref() != claims.family_name() +} diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_linking_existing_account.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_linking_existing_account.rs index b4450634c0..34657180b7 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_linking_existing_account.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/handle_linking_existing_account.rs @@ -13,6 +13,7 @@ pub struct LinkArgs<'a> { pub http_request: &'a HttpRequest, pub claims: Claims, pub claims_subject: &'a str, + pub claims_email_address: &'a str, pub user_account: UserRecordForLogin, pub mysql_connection: &'a mut PoolConnection, } @@ -21,8 +22,6 @@ pub async fn handle_linking_existing_account( ) -> Result { - // TODO: Double check email address in the claims before linking! - let mut transaction = args.mysql_connection.begin() .await .map_err(|e| { @@ -35,7 +34,7 @@ pub async fn handle_linking_existing_account( let _token = insert_google_sign_in_account(InsertGoogleSignInArgs { subject: args.claims_subject, maybe_user_token: Some(&args.user_account.token), - email_address: "todo@todo.com", // TODO // NB: The one from the Google claims, not our canonicalized one. + email_address: args.claims_email_address, is_email_verified: args.claims.email_verified(), maybe_locale: args.claims.locale(), maybe_name: args.claims.name(), 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 f2d62f3019..a676c6a0ba 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 @@ -21,6 +21,14 @@ use sqlx::{Acquire, MySql}; use tokens::tokens::user_sessions::UserSessionToken; use tokens::tokens::users::UserToken; +pub struct NewSsoArgs<'a> { + pub http_request: &'a HttpRequest, + pub claims: Claims, + pub claims_subject: &'a str, + pub claims_email_address: &'a str, + pub mysql_connection: &'a mut PoolConnection, +} + pub struct NewSsoAccountInfo { pub user_token: UserToken, pub user_display_name: String, @@ -28,28 +36,18 @@ pub struct NewSsoAccountInfo { } pub async fn handle_new_sso_account( - http_request: &HttpRequest, - claims_subject: &str, - mysql_connection: &mut PoolConnection, - claims: Claims, + args: NewSsoArgs<'_> ) -> Result { - let claims_email_address = claims.email() - .map(|email| email.to_string()) - .ok_or_else(|| { - warn!("no email address in google claims"); - GoogleCreateAccountErrorResponse::bad_request() - })?; - // 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. // A better normalization function in the future may handle dots and plus // signs in Gmail addresses, for instance. - let user_email_address = canonicalize_email_for_users_table(&claims_email_address); + let user_email_address = canonicalize_email_for_users_table(&args.claims_email_address); let maybe_user_account = - lookup_user_for_login_by_email_with_transactor(&user_email_address, Transactor::for_connection(mysql_connection)) + lookup_user_for_login_by_email_with_transactor(&user_email_address, Transactor::for_connection(args.mysql_connection)) .await .map_err(|err| { warn!("error looking up user by email: {:?}", err); @@ -59,21 +57,22 @@ pub async fn handle_new_sso_account( match maybe_user_account { Some(user_account) => { handle_linking_existing_account(LinkArgs { - http_request, - claims, - claims_subject, + http_request: args.http_request, + claims: args.claims, + claims_subject: args.claims_subject, + claims_email_address: &args.claims_email_address, user_account, - mysql_connection, + mysql_connection: args.mysql_connection, }).await }, None => { handle_creating_user_account(CreateArgs { - http_request, - claims, - claims_subject, - claims_email: &claims_email_address, + http_request: args.http_request, + claims: args.claims, + claims_subject: args.claims_subject, + claims_email_address: &args.claims_email_address, user_email_address: &user_email_address, - mysql_connection, + mysql_connection: args.mysql_connection, }).await }, } diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/mod.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/mod.rs index 1c4faae2b9..bc54be9205 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/mod.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/users/google_sso/mod.rs @@ -1,5 +1,6 @@ pub mod check_claims; pub mod google_sso_handler; +pub mod handle_creating_user_account; +pub mod handle_existing_sso_link; pub mod handle_linking_existing_account; -pub mod handle_new_sso_account; -mod handle_creating_user_account; \ No newline at end of file +pub mod handle_new_sso_account; \ No newline at end of file