From 6cd3217c2ed3aaa4b2e14bca90ebf7d8cb5bdac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Fri, 7 Mar 2025 23:25:32 +0100 Subject: [PATCH] refactor(oidc): Don't take the client metadata as an argument of url_for_oidc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OidcRegistrations already hold the metadata. We can just clone it lazily when we need it. Signed-off-by: Kévin Commaille --- bindings/matrix-sdk-ffi/src/client.rs | 13 +++---------- crates/matrix-sdk/src/authentication/oidc/mod.rs | 16 +++++++--------- .../src/authentication/oidc/registrations.rs | 2 +- .../matrix-sdk/src/authentication/oidc/tests.rs | 9 +++------ 4 files changed, 14 insertions(+), 26 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/client.rs b/bindings/matrix-sdk-ffi/src/client.rs index d3a27801b..1debbee80 100644 --- a/bindings/matrix-sdk-ffi/src/client.rs +++ b/bindings/matrix-sdk-ffi/src/client.rs @@ -425,17 +425,10 @@ impl Client { Some((issuer, ClientId::new(client_id.clone()))) }) .collect::>(); - let registrations = OidcRegistrations::new( - registrations_file, - oidc_metadata.clone(), - static_registrations, - )?; + let registrations = + OidcRegistrations::new(registrations_file, oidc_metadata, static_registrations)?; - let data = self - .inner - .oidc() - .url_for_oidc(oidc_metadata, registrations, prompt.map(Into::into)) - .await?; + let data = self.inner.oidc().url_for_oidc(registrations, prompt.map(Into::into)).await?; Ok(Arc::new(data)) } diff --git a/crates/matrix-sdk/src/authentication/oidc/mod.rs b/crates/matrix-sdk/src/authentication/oidc/mod.rs index 2a9d468e2..988aecdd5 100644 --- a/crates/matrix-sdk/src/authentication/oidc/mod.rs +++ b/crates/matrix-sdk/src/authentication/oidc/mod.rs @@ -441,9 +441,6 @@ impl Oidc { /// /// # Arguments /// - /// * `client_metadata` - The [`VerifiedClientMetadata`] to register, if - /// needed. - /// /// * `registrations` - The storage where the registered client ID will be /// loaded from, if the client is already registered, or stored into, if /// the client is not registered yet. @@ -454,18 +451,20 @@ impl Oidc { /// account. pub async fn url_for_oidc( &self, - client_metadata: VerifiedClientMetadata, registrations: OidcRegistrations, prompt: Option, ) -> Result { let metadata = self.provider_metadata().await?; - let redirect_uris = - client_metadata.redirect_uris.clone().ok_or(OidcError::MissingRedirectUri)?; + 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(), client_metadata, registrations).await?; + self.configure(metadata.issuer().to_owned(), registrations).await?; let mut data_builder = self.login(redirect_url.clone(), None)?; @@ -515,7 +514,6 @@ impl Oidc { async fn configure( &self, issuer: String, - client_metadata: VerifiedClientMetadata, registrations: OidcRegistrations, ) -> std::result::Result<(), OidcError> { if self.client_id().is_some() { @@ -529,7 +527,7 @@ impl Oidc { } tracing::info!("Registering this client for OIDC."); - self.register_client(client_metadata, None).await?; + self.register_client(registrations.verified_metadata.clone(), None).await?; tracing::info!("Persisting OIDC registration data."); self.store_client_registration(®istrations) diff --git a/crates/matrix-sdk/src/authentication/oidc/registrations.rs b/crates/matrix-sdk/src/authentication/oidc/registrations.rs index e7e34c3bb..e16cadd44 100644 --- a/crates/matrix-sdk/src/authentication/oidc/registrations.rs +++ b/crates/matrix-sdk/src/authentication/oidc/registrations.rs @@ -53,7 +53,7 @@ pub struct OidcRegistrations { file_path: PathBuf, /// The hash for the metadata used to register the client. /// This is used to check if the client needs to be re-registered. - verified_metadata: VerifiedClientMetadata, + pub(super) verified_metadata: VerifiedClientMetadata, /// Pre-configured registrations for use with issuers that don't support /// dynamic client registration. static_registrations: HashMap, diff --git a/crates/matrix-sdk/src/authentication/oidc/tests.rs b/crates/matrix-sdk/src/authentication/oidc/tests.rs index 9d0de9f2b..071f003fa 100644 --- a/crates/matrix-sdk/src/authentication/oidc/tests.rs +++ b/crates/matrix-sdk/src/authentication/oidc/tests.rs @@ -162,8 +162,7 @@ async fn test_high_level_login() -> anyhow::Result<()> { assert!(oidc.client_id().is_none()); // When getting the OIDC login URL. - let authorization_data = - oidc.url_for_oidc(metadata.clone(), registrations, Some(Prompt::Create)).await.unwrap(); + let authorization_data = oidc.url_for_oidc(registrations, Some(Prompt::Create)).await.unwrap(); // Then the client should be configured correctly. assert_let!(Some(issuer) = oidc.issuer()); @@ -185,8 +184,7 @@ async fn test_high_level_login() -> anyhow::Result<()> { 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(metadata.clone(), registrations, None).await.unwrap(); + let authorization_data = oidc.url_for_oidc(registrations, None).await.unwrap(); assert_let!(Some(issuer) = oidc.issuer()); assert!(oidc.client_id().is_some()); @@ -215,8 +213,7 @@ async fn test_high_level_login_cancellation() -> anyhow::Result<()> { 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(metadata.clone(), registrations, None).await.unwrap(); + let authorization_data = oidc.url_for_oidc(registrations, None).await.unwrap(); assert_let!(Some(issuer) = oidc.issuer()); assert!(oidc.client_id().is_some());