refactor(oidc): Add redirect URI as an argument of url_for_oidc

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 <zecakeh@tedomum.fr>

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, OidcError> {
+        Url::parse(&self.redirect_uri).map_err(|_| OidcError::CallbackUrlInvalid)
+    }
+}
+
 impl TryInto<VerifiedClientMetadata> for &OidcConfiguration {
     type Error = OidcError;
 
     fn try_into(self) -> Result<VerifiedClientMetadata, Self::Error> {
-        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<SdkOidcError> 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

bindings/matrix-sdk-ffi/src/client.rs

@@ -413,6 +413,8 @@ impl Client {
         prompt: Option<OidcPrompt>,
     ) -> Result<Arc<OidcAuthorizationData>, 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))
     }

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,

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<Prompt>,
     ) -> Result<OidcAuthorizationData, OidcError> {
         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]);

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(&registrations_path, client_metadata.clone(), HashMap::new())
-            .unwrap();
+        OidcRegistrations::new(&registrations_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!(

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,