From d3e64295cf804fc3d31d82c0562231e9935c4e5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Fri, 7 Mar 2025 23:46:52 +0100 Subject: [PATCH] refactor(oidc): Add redirect URI as an argument of url_for_oidc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Being able to always use the first redirect URI in the client metadata seems to be very specific to the FFI bindings. For example clients that need to bind a port on localhost need to provide a custom redirect URI each time. So we ask for the redirect URI, and keep the current behavior only for the bindings. Signed-off-by: Kévin Commaille --- bindings/matrix-sdk-ffi/src/authentication.rs | 10 +++-- bindings/matrix-sdk-ffi/src/client.rs | 8 +++- .../src/authentication/oidc/error.rs | 4 -- .../matrix-sdk/src/authentication/oidc/mod.rs | 15 +++---- .../src/authentication/oidc/tests.rs | 45 +++++++++---------- crates/matrix-sdk/src/test_utils/client.rs | 8 +++- 6 files changed, 46 insertions(+), 44 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/authentication.rs b/bindings/matrix-sdk-ffi/src/authentication.rs index 028b16b1f..cccddecc3 100644 --- a/bindings/matrix-sdk-ffi/src/authentication.rs +++ b/bindings/matrix-sdk-ffi/src/authentication.rs @@ -137,12 +137,17 @@ pub struct OidcConfiguration { pub dynamic_registrations_file: String, } +impl OidcConfiguration { + pub(crate) fn redirect_uri(&self) -> Result { + Url::parse(&self.redirect_uri).map_err(|_| OidcError::CallbackUrlInvalid) + } +} + impl TryInto for &OidcConfiguration { type Error = OidcError; fn try_into(self) -> Result { - let redirect_uri = - Url::parse(&self.redirect_uri).map_err(|_| OidcError::CallbackUrlInvalid)?; + let redirect_uri = self.redirect_uri()?; let client_name = self.client_name.as_ref().map(|n| Localized::new(n.to_owned(), [])); let client_uri = self.client_uri.localized_url()?; let logo_uri = self.logo_uri.localized_url()?; @@ -198,7 +203,6 @@ impl From for OidcError { fn from(e: SdkOidcError) -> OidcError { match e { SdkOidcError::Discovery(error) if error.is_not_supported() => OidcError::NotSupported, - SdkOidcError::MissingRedirectUri => OidcError::MetadataInvalid, SdkOidcError::AuthorizationCode(OauthAuthorizationCodeError::RedirectUri(_)) | SdkOidcError::AuthorizationCode(OauthAuthorizationCodeError::InvalidState) => { OidcError::CallbackUrlInvalid diff --git a/bindings/matrix-sdk-ffi/src/client.rs b/bindings/matrix-sdk-ffi/src/client.rs index 1debbee80..47b92fa1a 100644 --- a/bindings/matrix-sdk-ffi/src/client.rs +++ b/bindings/matrix-sdk-ffi/src/client.rs @@ -413,6 +413,8 @@ impl Client { prompt: Option, ) -> Result, OidcError> { let oidc_metadata: VerifiedClientMetadata = oidc_configuration.try_into()?; + let redirect_uri = oidc_configuration.redirect_uri()?; + let registrations_file = Path::new(&oidc_configuration.dynamic_registrations_file); let static_registrations = oidc_configuration .static_registrations @@ -428,7 +430,11 @@ impl Client { let registrations = OidcRegistrations::new(registrations_file, oidc_metadata, static_registrations)?; - let data = self.inner.oidc().url_for_oidc(registrations, prompt.map(Into::into)).await?; + let data = self + .inner + .oidc() + .url_for_oidc(registrations, redirect_uri, prompt.map(Into::into)) + .await?; Ok(Arc::new(data)) } diff --git a/crates/matrix-sdk/src/authentication/oidc/error.rs b/crates/matrix-sdk/src/authentication/oidc/error.rs index 054eeb5a5..fb97b1f6b 100644 --- a/crates/matrix-sdk/src/authentication/oidc/error.rs +++ b/crates/matrix-sdk/src/authentication/oidc/error.rs @@ -64,10 +64,6 @@ pub enum OidcError { #[error("client not registered")] NotRegistered, - /// The supplied redirect URIs are missing or empty. - #[error("missing or empty redirect URIs")] - MissingRedirectUri, - /// The device ID was not returned by the homeserver after login. #[error("missing device ID in response")] MissingDeviceId, diff --git a/crates/matrix-sdk/src/authentication/oidc/mod.rs b/crates/matrix-sdk/src/authentication/oidc/mod.rs index 988aecdd5..c76fd34ad 100644 --- a/crates/matrix-sdk/src/authentication/oidc/mod.rs +++ b/crates/matrix-sdk/src/authentication/oidc/mod.rs @@ -445,6 +445,10 @@ impl Oidc { /// loaded from, if the client is already registered, or stored into, if /// the client is not registered yet. /// + /// * `redirect_uri` - The URI where the end user will be redirected after + /// authorizing the login. It must be one of the redirect URIs in the + /// client metadata used for registration. + /// /// * `prompt` - The desired user experience in the web UI. `None` means /// that the user wishes to login into an existing account, and /// `Some(Prompt::Create)` means that the user wishes to register a new @@ -452,21 +456,14 @@ impl Oidc { pub async fn url_for_oidc( &self, registrations: OidcRegistrations, + redirect_uri: Url, prompt: Option, ) -> Result { let metadata = self.provider_metadata().await?; - let redirect_uris = registrations - .verified_metadata - .redirect_uris - .clone() - .ok_or(OidcError::MissingRedirectUri)?; - - let redirect_url = redirect_uris.first().ok_or(OidcError::MissingRedirectUri)?; - self.configure(metadata.issuer().to_owned(), registrations).await?; - let mut data_builder = self.login(redirect_url.clone(), None)?; + let mut data_builder = self.login(redirect_uri, None)?; if let Some(prompt) = prompt { data_builder = data_builder.prompt(vec![prompt]); diff --git a/crates/matrix-sdk/src/authentication/oidc/tests.rs b/crates/matrix-sdk/src/authentication/oidc/tests.rs index 071f003fa..3337fc218 100644 --- a/crates/matrix-sdk/src/authentication/oidc/tests.rs +++ b/crates/matrix-sdk/src/authentication/oidc/tests.rs @@ -3,10 +3,7 @@ use std::collections::HashMap; use anyhow::Context as _; use assert_matches::assert_matches; use assert_matches2::assert_let; -use mas_oidc_client::{ - requests::account_management::AccountManagementActionFull, - types::registration::VerifiedClientMetadata, -}; +use mas_oidc_client::requests::account_management::AccountManagementActionFull; use matrix_sdk_test::async_test; use oauth2::{CsrfToken, PkceCodeChallenge, RedirectUrl}; use ruma::{ @@ -34,7 +31,7 @@ use crate::{ test_utils::{ client::{ mock_prev_session_tokens_with_refresh, mock_session_tokens_with_refresh, - oauth::{mock_client_metadata, mock_session}, + oauth::{mock_client_metadata, mock_redirect_uri, mock_session}, MockClientBuilder, }, mocks::{oauth::MockServerMetadataBuilder, MatrixMockServer}, @@ -44,8 +41,7 @@ use crate::{ const REDIRECT_URI_STRING: &str = "http://127.0.0.1:6778/oidc/callback"; -async fn mock_environment( -) -> anyhow::Result<(Oidc, MatrixMockServer, VerifiedClientMetadata, OidcRegistrations)> { +async fn mock_environment() -> anyhow::Result<(Oidc, MatrixMockServer, Url, OidcRegistrations)> { let server = MatrixMockServer::new().await; server.mock_who_am_i().ok().named("whoami").mount().await; @@ -59,10 +55,9 @@ async fn mock_environment( let registrations_path = tempdir().unwrap().path().join("oidc").join("registrations.json"); let registrations = - OidcRegistrations::new(®istrations_path, client_metadata.clone(), HashMap::new()) - .unwrap(); + OidcRegistrations::new(®istrations_path, client_metadata, HashMap::new()).unwrap(); - Ok((client.oidc(), server, client_metadata, registrations)) + Ok((client.oidc(), server, mock_redirect_uri(), registrations)) } /// Check the URL in the given authorization data. @@ -157,12 +152,13 @@ async fn check_authorization_url( #[async_test] async fn test_high_level_login() -> anyhow::Result<()> { // Given a fresh environment. - let (oidc, _server, metadata, registrations) = mock_environment().await.unwrap(); + let (oidc, _server, mut redirect_uri, registrations) = mock_environment().await.unwrap(); assert!(oidc.issuer().is_none()); assert!(oidc.client_id().is_none()); // When getting the OIDC login URL. - let authorization_data = oidc.url_for_oidc(registrations, Some(Prompt::Create)).await.unwrap(); + let authorization_data = + oidc.url_for_oidc(registrations, redirect_uri.clone(), Some(Prompt::Create)).await.unwrap(); // Then the client should be configured correctly. assert_let!(Some(issuer) = oidc.issuer()); @@ -171,11 +167,10 @@ async fn test_high_level_login() -> anyhow::Result<()> { check_authorization_url(&authorization_data, &oidc, issuer, None, Some("create"), None).await; // When completing the login with a valid callback. - let mut callback_uri = metadata.redirect_uris.clone().unwrap().first().unwrap().clone(); - callback_uri.set_query(Some(&format!("code=42&state={}", authorization_data.state.secret()))); + redirect_uri.set_query(Some(&format!("code=42&state={}", authorization_data.state.secret()))); // Then the login should succeed. - oidc.login_with_oidc_callback(&authorization_data, callback_uri).await?; + oidc.login_with_oidc_callback(&authorization_data, redirect_uri).await?; Ok(()) } @@ -183,8 +178,9 @@ async fn test_high_level_login() -> anyhow::Result<()> { #[async_test] async fn test_high_level_login_cancellation() -> anyhow::Result<()> { // Given a client ready to complete login. - let (oidc, _server, metadata, registrations) = mock_environment().await.unwrap(); - let authorization_data = oidc.url_for_oidc(registrations, None).await.unwrap(); + let (oidc, _server, mut redirect_uri, registrations) = mock_environment().await.unwrap(); + let authorization_data = + oidc.url_for_oidc(registrations, redirect_uri.clone(), None).await.unwrap(); assert_let!(Some(issuer) = oidc.issuer()); assert!(oidc.client_id().is_some()); @@ -192,13 +188,12 @@ async fn test_high_level_login_cancellation() -> anyhow::Result<()> { check_authorization_url(&authorization_data, &oidc, issuer, None, None, None).await; // When completing login with a cancellation callback. - let mut callback_uri = metadata.redirect_uris.clone().unwrap().first().unwrap().clone(); - callback_uri.set_query(Some(&format!( + redirect_uri.set_query(Some(&format!( "error=access_denied&state={}", authorization_data.state.secret() ))); - let error = oidc.login_with_oidc_callback(&authorization_data, callback_uri).await.unwrap_err(); + let error = oidc.login_with_oidc_callback(&authorization_data, redirect_uri).await.unwrap_err(); // Then a cancellation error should be thrown. assert_matches!( @@ -212,8 +207,9 @@ async fn test_high_level_login_cancellation() -> anyhow::Result<()> { #[async_test] async fn test_high_level_login_invalid_state() -> anyhow::Result<()> { // Given a client ready to complete login. - let (oidc, _server, metadata, registrations) = mock_environment().await.unwrap(); - let authorization_data = oidc.url_for_oidc(registrations, None).await.unwrap(); + let (oidc, _server, mut redirect_uri, registrations) = mock_environment().await.unwrap(); + let authorization_data = + oidc.url_for_oidc(registrations, redirect_uri.clone(), None).await.unwrap(); assert_let!(Some(issuer) = oidc.issuer()); assert!(oidc.client_id().is_some()); @@ -221,10 +217,9 @@ async fn test_high_level_login_invalid_state() -> anyhow::Result<()> { check_authorization_url(&authorization_data, &oidc, issuer, None, None, None).await; // When completing login with an old/tampered state. - let mut callback_uri = metadata.redirect_uris.clone().unwrap().first().unwrap().clone(); - callback_uri.set_query(Some("code=42&state=imposter_alert")); + redirect_uri.set_query(Some("code=42&state=imposter_alert")); - let error = oidc.login_with_oidc_callback(&authorization_data, callback_uri).await.unwrap_err(); + let error = oidc.login_with_oidc_callback(&authorization_data, redirect_uri).await.unwrap_err(); // Then the login should fail by flagging the invalid state. assert_matches!( diff --git a/crates/matrix-sdk/src/test_utils/client.rs b/crates/matrix-sdk/src/test_utils/client.rs index 995b46060..c14f73fea 100644 --- a/crates/matrix-sdk/src/test_utils/client.rs +++ b/crates/matrix-sdk/src/test_utils/client.rs @@ -191,16 +191,20 @@ pub mod oauth { ClientId::new("test_client_id".to_owned()) } + /// A redirect URI, for unit or integration tests. + pub fn mock_redirect_uri() -> Url { + Url::parse("http://127.0.0.1/").expect("redirect URI should be valid") + } + /// `VerifiedClientMetadata` that should be valid in most cases, for unit or /// integration tests. pub fn mock_client_metadata() -> VerifiedClientMetadata { - let redirect_uri = Url::parse("http://127.0.0.1/").expect("redirect URI should be valid"); let client_uri = Url::parse("https://github.com/matrix-org/matrix-rust-sdk") .expect("client URI should be valid"); ClientMetadata { application_type: Some(ApplicationType::Native), - redirect_uris: Some(vec![redirect_uri]), + redirect_uris: Some(vec![mock_redirect_uri()]), grant_types: Some(vec![ GrantType::AuthorizationCode, GrantType::RefreshToken,