From 2773ff3e16c2c8bce97287dce71f98723351be4f Mon Sep 17 00:00:00 2001 From: Ginger Date: Thu, 24 Sep 2026 09:48:06 -0400 Subject: [PATCH] feat: Import images correctly from OIDC claims --- changelog.d/+40240015.feature.md | 1 + changelog.d/+93c14424.bugfix.md | 1 + conduwuit-example.toml | 13 +++-- src/core/config/mod.rs | 13 +++-- src/database/maps.rs | 4 ++ src/service/oidc/mod.rs | 85 ++++++++++++++++++++++---------- 6 files changed, 82 insertions(+), 35 deletions(-) create mode 100644 changelog.d/+40240015.feature.md create mode 100644 changelog.d/+93c14424.bugfix.md diff --git a/changelog.d/+40240015.feature.md b/changelog.d/+40240015.feature.md new file mode 100644 index 000000000..1183b69d2 --- /dev/null +++ b/changelog.d/+40240015.feature.md @@ -0,0 +1 @@ +Profile banners are now treated as images when imported from OIDC claims, like avatars already were. diff --git a/changelog.d/+93c14424.bugfix.md b/changelog.d/+93c14424.bugfix.md new file mode 100644 index 000000000..35a6b83aa --- /dev/null +++ b/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. diff --git a/conduwuit-example.toml b/conduwuit-example.toml index ae37686ed..d0bf4e51f 100644 --- a/conduwuit-example.toml +++ b/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" } diff --git a/src/core/config/mod.rs b/src/core/config/mod.rs index 02497681c..e59077d78 100644 --- a/src/core/config/mod.rs +++ b/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")] diff --git a/src/database/maps.rs b/src/database/maps.rs index 69e441372..013fc9270 100644 --- a/src/database/maps.rs +++ b/src/database/maps.rs @@ -130,6 +130,10 @@ pub(super) fn open_list(db: &Arc, maps: &[Descriptor]) -> Result { }, Descriptor { name: "openidsubject_currentpictureurl", + ..descriptor::DROPPED + }, + Descriptor { + name: "openidsubjectprofilefield_url", ..descriptor::RANDOM_SMALL }, Descriptor { diff --git a/src/service/oidc/mod.rs b/src/service/oidc/mod.rs index a7bed9847..d0369cf5e 100644 --- a/src/service/oidc/mod.rs +++ b/src/service/oidc/mod.rs @@ -47,7 +47,7 @@ pub struct Service { struct Data { openidsubject_localpart: Arc, - openidsubject_currentpictureurl: Arc, + openidsubjectprofilefield_url: Arc, } struct Services { config: Dep, @@ -144,7 +144,7 @@ fn build(args: crate::Args<'_>) -> Result> { 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 { 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::() - .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::() + .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; }