fix: Correctly handle preferred_username claim

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`.

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<Arc<Self>> {
@@ -303,7 +318,7 @@ pub async fn complete_session(
 			.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 @@ pub async fn complete_session(
 			.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 @@ pub fn link_user(&self, user_id: &UserId, subject: &str) {
 	}
 
 	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<ClaimedLocalUser> {
+		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))
+		}
+	}
 }

src/web/pages/oidc/complete.rs

@@ -6,9 +6,12 @@
 	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