diff --git a/changelog.d/1942.bugfix.md b/changelog.d/1942.bugfix.md new file mode 100644 index 000000000..6e35d33a6 --- /dev/null +++ b/changelog.d/1942.bugfix.md @@ -0,0 +1 @@ +Fixed existing accounts failing to link when logging in with OIDC if `prompt_for_localpart` was `false`. diff --git a/src/service/oidc/mod.rs b/src/service/oidc/mod.rs index 3dc786c68..653b0b3c4 100644 --- a/src/service/oidc/mod.rs +++ b/src/service/oidc/mod.rs @@ -115,6 +115,21 @@ pub enum SessionCompletionStatus { Complete(OwnedUserId), } +pub enum ClaimedLocalUser { + /// The claim refers to an existing user. + Existing(OwnedUserId), + /// The claim refers to a new user ID which should be registered. + New(OwnedUserId), +} + +impl ClaimedLocalUser { + fn into_user_id(self) -> OwnedUserId { + match self { + | Self::Existing(user_id) | Self::New(user_id) => user_id, + } + } +} + #[async_trait] impl crate::Service for Service { fn build(args: crate::Args<'_>) -> Result> { @@ -303,7 +318,7 @@ impl Service { .expect("claims should be an object") .to_owned(); - debug!(?all_claims); + debug!(?all_claims, "Got claims from the identity provider"); let subject = claims.subject().as_str(); @@ -326,14 +341,13 @@ impl Service { .get(&config.preferred_username_claim) .and_then(|claim| claim.as_str()) { - self.services - .users - .determine_registration_user_id(Some(preferred_username.to_owned()), None, None) + self.identify_claimed_local_user(preferred_username) .await + .map(ClaimedLocalUser::into_user_id) .map_err(|err| { error!("Preferred username claim is not a valid localpart: {err}"); - "Your preferred username is not a valid Matrix user ID localpart. Contact \ - your homeserver's administrator." + "Your preferred username could not be converted to a valid Matrix user ID. \ + Contact your homeserver's administrator." })? } else { error!("Preferred username claim was not present or was not a string"); @@ -482,4 +496,22 @@ impl Service { } pub fn unlink_user(&self, subject: &str) { self.db.openidsubject_localpart.remove(subject); } + + /// Determine what user ID a localpart claim refers to. + pub async fn identify_claimed_local_user(&self, claim: &str) -> Result { + if let Ok(user_id) = + UserId::parse(format!("@{}:{}", claim, self.services.globals.server_name())) + && self.services.users.status(&user_id).await.is_active() + { + Ok(ClaimedLocalUser::Existing(user_id)) + } else { + let user_id = self + .services + .users + .determine_registration_user_id(Some(claim.to_owned()), None, None) + .await?; + + Ok(ClaimedLocalUser::New(user_id)) + } + } } diff --git a/src/web/pages/oidc/complete.rs b/src/web/pages/oidc/complete.rs index bcffd13c8..32a0d342d 100644 --- a/src/web/pages/oidc/complete.rs +++ b/src/web/pages/oidc/complete.rs @@ -6,9 +6,12 @@ use axum::{ response::Redirect, routing::on, }; -use conduwuit_service::{oauth::grant::AuthorizationCodeResponse, oidc::SessionCompletionStatus}; +use conduwuit_service::{ + oauth::grant::AuthorizationCodeResponse, + oidc::{ClaimedLocalUser, SessionCompletionStatus}, +}; use futures::FutureExt; -use ruma::{OwnedServerName, UserId}; +use ruma::OwnedServerName; use serde::{Deserialize, de::IgnoredAny}; use tower_sessions::Session; @@ -99,65 +102,60 @@ async fn route_complete( state: OidcSessionState::Authorized { claims: Box::new(claims.clone()) }, }) .await - .expect("Should be able to serialize oidc session"); + .expect("should be able to serialize oidc session"); services.oidc.complete_session(&claims, None).await }, | OidcSessionState::Authorized { claims } => { let supplied_user_id = if let Some(form) = form { - if let Ok(user_id) = UserId::parse(format!( - "@{}:{}", - form.username, - services.globals.server_name() - )) && services.users.status(&user_id).await.is_active() + match services + .oidc + .identify_claimed_local_user(&form.username) + .await { - let user_card = UserCard::for_local_user(&services, user_id.clone()).await; + | Ok(ClaimedLocalUser::Existing(user_id)) => { + let user_card = + UserCard::for_local_user(&services, user_id.clone()).await; - if let Some(password) = form.password { - if services - .users - .check_password(&user_id, &password) - .await - .is_ok() - { - Some(user_id) + if let Some(password) = form.password { + if services + .users + .check_password(&user_id, &password) + .await + .is_ok() + { + Some(user_id) + } else { + return response!(OidcComplete::new( + context, + OidcCompleteBody::PasswordPrompt { + username: form.username, + user_card, + password_error: true + } + )); + } } else { return response!(OidcComplete::new( context, OidcCompleteBody::PasswordPrompt { username: form.username, user_card, - password_error: true + password_error: false, } )); } - } else { + }, + | Ok(ClaimedLocalUser::New(user_id)) => Some(user_id), + | Err(err) => { return response!(OidcComplete::new( context, - OidcCompleteBody::PasswordPrompt { - username: form.username, - user_card, - password_error: false, + OidcCompleteBody::UsernamePrompt { + server_name: services.globals.server_name().to_owned(), + username_error: Some(err.message()), } )); - } - } else { - match services - .users - .determine_registration_user_id(Some(form.username), None, None) - .await - { - | Ok(user_id) => Some(user_id), - | Err(err) => { - return response!(OidcComplete::new( - context, - OidcCompleteBody::UsernamePrompt { - server_name: services.globals.server_name().to_owned(), - username_error: Some(err.message()), - } - )); - }, - } + }, } } else { None