feat: Import images correctly from OIDC claims

changelog.d/+40240015.feature.md

@@ -0,0 +1 @@
+Profile banners are now treated as images when imported from OIDC claims, like avatars already were.

changelog.d/+93c14424.bugfix.md

@@ -0,0 +1 @@
+For deployments using OIDC with `profile_key_import_mode` set to `on_login`, users' avatars will no longer be clobbered if they didn't change their avatar since their last login.

conduwuit-example.toml

@@ -2378,10 +2378,15 @@
 # Per-room overrides to the user's display name or avatar will be
 # preserved by the import process.
 #
-# SECURITY NOTE: If the `avatar_url` field is set, Continuwuity will
-# perform a HTTP GET to the URL in the mapped claim and use the returned
-# file as the user's profile picture. Make sure your users are not able
-# to set the value of the mapped claim to an arbitrary URL.
+# SECURITY NOTE: For the following profile fields, Continuwuity will treat
+# the value of the mapped claim as an HTTP URL and upload the file located
+# at that URL to its internal media repository:
+#
+# - `avatar_url`
+# - `chat.commet.profile_banner`
+#
+# If you map these fields to claims, make sure your users cannot set those
+# claims to arbitrary URLs.
 #
 #profile_key_map = { displayname = "name" }
 

src/core/config/mod.rs

@@ -2864,10 +2864,15 @@ pub struct OidcConfig {
 	/// Per-room overrides to the user's display name or avatar will be
 	/// preserved by the import process.
 	///
-	/// SECURITY NOTE: If the `avatar_url` field is set, Continuwuity will
-	/// perform a HTTP GET to the URL in the mapped claim and use the returned
-	/// file as the user's profile picture. Make sure your users are not able
-	/// to set the value of the mapped claim to an arbitrary URL.
+	/// SECURITY NOTE: For the following profile fields, Continuwuity will treat
+	/// the value of the mapped claim as an HTTP URL and upload the file located
+	/// at that URL to its internal media repository:
+	///
+	/// - `avatar_url`
+	/// - `chat.commet.profile_banner`
+	///
+	/// If you map these fields to claims, make sure your users cannot set those
+	/// claims to arbitrary URLs.
 	///
 	/// default: { displayname = "name" }
 	#[serde(default = "default_profile_key_map")]

src/database/maps.rs

@@ -130,6 +130,10 @@ pub(super) fn open_list(db: &Arc<Engine>, maps: &[Descriptor]) -> Result<Maps> {
 	},
 	Descriptor {
 		name: "openidsubject_currentpictureurl",
+		..descriptor::DROPPED
+	},
+	Descriptor {
+		name: "openidsubjectprofilefield_url",
 		..descriptor::RANDOM_SMALL
 	},
 	Descriptor {

src/service/oidc/mod.rs

@@ -47,7 +47,7 @@ pub struct Service {
 
 struct Data {
 	openidsubject_localpart: Arc<Map>,
-	openidsubject_currentpictureurl: Arc<Map>,
+	openidsubjectprofilefield_url: Arc<Map>,
 }
 struct Services {
 	config: Dep<config::Service>,
@@ -144,7 +144,7 @@ fn build(args: crate::Args<'_>) -> Result<Arc<Self>> {
 			runtime: args.server.runtime().clone(),
             db: Data {
                 openidsubject_localpart: args.db["openidsubject_localpart"].clone(),
-				openidsubject_currentpictureurl: args.db["openidsubject_currentpictureurl"].clone(),
+				openidsubjectprofilefield_url: args.db["openidsubjectprofilefield_url"].clone(),
             },
             client: args.server.config.oauth.oidc.as_ref().map(|config| -> Result<OidcClient> {
 				Ok(OidcClient {
@@ -214,6 +214,10 @@ fn name(&self) -> &str { crate::service::make_name(std::module_path!()) }
 }
 
 impl Service {
+	// Profile keys that will be treated as image URLs and imported
+	// into the media repository. Keep this synced with the list in the
+	// doc comment for `profile_key_map` in the config.
+	const IMAGE_PROFILE_KEYS: &[&str] = &["avatar_url", "chat.commet.profile_banner"];
 	const SERVER_MISCONFIGURED: &str =
 		"Identity server is misconfigured. Contact your homeserver's administrator.";
 
@@ -433,41 +437,66 @@ pub async fn complete_session(
 			let user_id = user_id.clone();
 			let subject = claims.subject().to_string();
 			let profile_key_map = config.profile_key_map.clone();
-			let openidsubject_currentpictureurl = self.db.openidsubject_currentpictureurl.clone();
+			let openidsubjectprofilefield_url = self.db.openidsubjectprofilefield_url.clone();
 			let users = self.services.users.clone();
 			let media = self.services.media.clone();
 
 			let import_task = self.runtime.spawn(async move {
+				// TODO: once we get support for full profile replacement, use
+				// that logic instead
 				for (field, claim) in &profile_key_map {
 					let Some(value) = all_claims.get(claim).cloned() else {
 						warn!(?field, ?claim, "IDP provided no value for this mapped claim");
 						continue;
 					};
 
-					let value = if let Some(picture_url) = value.as_str()
-						&& field == ProfileFieldName::AvatarUrl.as_str()
-						&& openidsubject_currentpictureurl
-							.get(&subject)
-							.await
-							.deserialized::<String>()
-							.ok()
-							.is_none_or(|current_picture| current_picture != picture_url)
-					{
-						match media.download_media(picture_url).await {
-							| Ok((mxc, size)) => {
-								openidsubject_currentpictureurl.insert(&subject, picture_url);
-								info!(?picture_url, ?mxc, ?size, "Downloaded profile picture");
+					let value = if Self::IMAGE_PROFILE_KEYS.contains(&field.as_str()) {
+						if let Some(url) = value.as_str() {
+							// Check the last value the IDP gave us for this
+							// profile field. We have to track this in a
+							// separate keyspace because, for fields in
+							// `IMAGE_PROFILE_KEYS`, what gets saved in
+							// the user's profile data is a MXC URI that has no
+							// relationship to the URL the IDP gave us.
+							if openidsubjectprofilefield_url
+								.qry(&(&subject, field))
+								.await
+								.deserialized::<String>()
+								.ok()
+								.is_none_or(|current_url| current_url != url)
+							{
+								match media.download_media(url).await {
+									| Ok((mxc, _)) => {
+										openidsubjectprofilefield_url.put((&subject, field), url);
+										info!(?url, ?field, ?mxc, "Downloaded profile image");
 
-								ProfileFieldValue::AvatarUrl(mxc)
-							},
-							| Err(err) => {
-								warn!(
-									?claim,
-									?picture_url,
-									"Failed to download profile picture: {err}"
-								);
+										ProfileFieldValue::new(
+											field,
+											Value::String(mxc.to_string()),
+										)
+										.expect("MXC should be valid for this field")
+									},
+									| Err(err) => {
+										warn!(
+											?field,
+											?url,
+											"Failed to download profile image: {err}"
+										);
+										continue;
+									},
+								}
+							} else {
+								// This claim hasn't changed from its previous
+								// value, don't update it.
 								continue;
-							},
+							}
+						} else {
+							warn!(
+								?claim,
+								?value,
+								"Claim value for profile image was not a string"
+							);
+							continue;
 						}
 					} else {
 						match ProfileFieldValue::new(field, value.clone()) {
@@ -499,8 +528,10 @@ pub async fn complete_session(
 				info!("Profile import complete");
 			});
 
-			// Only wait for import to complete if this is a new account,
-			// so they see the correct profile information in the account panel
+			// Only wait for the import to complete if this is a new account,
+			// so they see the correct profile information in the account panel.
+			// Otherwise let it run in the background to avoid blocking the
+			// sign-in process.
 			if new_account_registered {
 				let _ = import_task.await;
 			}