From b3e82a05dbdbfab5ce0b44c8820baefad174dda9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Fri, 21 Mar 2025 15:05:50 +0100 Subject: [PATCH] refactor(oauth): Merge OAuth::login_with_oidc_callback() and OAuth::finish_login() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accept a URL or a query string for simplicity. That way we don't need to expose AuthorizationResponse. Signed-off-by: Kévin Commaille --- bindings/matrix-sdk-ffi/src/client.rs | 8 +- .../src/authentication/oauth/mod.rs | 142 +++++++++--------- .../src/authentication/oauth/tests.rs | 42 ++---- examples/oidc_cli/src/main.rs | 50 ++---- 4 files changed, 95 insertions(+), 147 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/client.rs b/bindings/matrix-sdk-ffi/src/client.rs index 11e229b56..447612417 100644 --- a/bindings/matrix-sdk-ffi/src/client.rs +++ b/bindings/matrix-sdk-ffi/src/client.rs @@ -428,14 +428,10 @@ impl Client { } /// Completes the OIDC login process. - pub async fn login_with_oidc_callback( - &self, - authorization_data: Arc, - callback_url: String, - ) -> Result<(), OidcError> { + pub async fn login_with_oidc_callback(&self, callback_url: String) -> Result<(), OidcError> { let url = Url::parse(&callback_url).or(Err(OidcError::CallbackUrlInvalid))?; - self.inner.oauth().login_with_oidc_callback(&authorization_data, url).await?; + self.inner.oauth().finish_login(url.into()).await?; Ok(()) } diff --git a/crates/matrix-sdk/src/authentication/oauth/mod.rs b/crates/matrix-sdk/src/authentication/oauth/mod.rs index 9520ba9cf..16392b629 100644 --- a/crates/matrix-sdk/src/authentication/oauth/mod.rs +++ b/crates/matrix-sdk/src/authentication/oauth/mod.rs @@ -416,37 +416,6 @@ impl OAuth { LoginWithQrCode::new(&self.client, registration_method, data) } - /// A higher level wrapper around the methods to complete a login after the - /// user has logged in through a webview. This method should be used in - /// tandem with [`OAuth::url_for_oidc`]. - pub async fn login_with_oidc_callback( - &self, - authorization_data: &OAuthAuthorizationData, - callback_url: Url, - ) -> Result<()> { - let response = AuthorizationResponse::parse_uri(&callback_url) - .map_err(OAuthAuthorizationCodeError::from) - .map_err(OAuthError::from)?; - - let code = match response { - AuthorizationResponse::Success(code) => code, - AuthorizationResponse::Error(err) => { - return Err(OAuthError::from(OAuthAuthorizationCodeError::from(err.error)).into()); - } - }; - - // This check will also be done in `finish_authorization`, however it requires - // the client to have called `abort_login` which we can't guarantee so - // lets double check with their supplied authorization data to be safe. - if code.state != authorization_data.state { - return Err(OAuthError::from(OAuthAuthorizationCodeError::InvalidState).into()); - }; - - self.finish_login(code).await?; - - Ok(()) - } - /// Restore or register the OAuth 2.0 client for the server with the given /// metadata, with the given [`OAuthRegistrationStore`]. /// @@ -950,7 +919,8 @@ impl OAuth { /// [`OAuth::restore_registered_client()`]. /// /// [`OAuth::finish_login()`] must be called once the user has been - /// redirected to the `redirect_uri`. + /// redirected to the `redirect_uri`. [`OAuth::abort_login()`] should be + /// called instead if the authorization should be aborted before completion. /// /// # Arguments /// @@ -972,7 +942,7 @@ impl OAuth { /// use anyhow::anyhow; /// use matrix_sdk::{ /// Client, - /// authentication::oauth::{AuthorizationResponse, OAuthRegistrationStore}, + /// authentication::oauth::OAuthRegistrationStore, /// }; /// # use ruma::serde::Raw; /// # use matrix_sdk::authentication::oauth::registration::ClientMetadata; @@ -999,17 +969,7 @@ impl OAuth { /// // Open auth_data.url and wait for response at the redirect URI. /// let redirected_to_uri = open_uri_and_wait_for_redirect(auth_data.url).await; /// - /// let auth_response = AuthorizationResponse::parse_uri(&redirected_to_uri)?; - /// - /// let auth_code = match auth_response { - /// AuthorizationResponse::Success(code) => code, - /// AuthorizationResponse::Error(error) => { - /// oauth.abort_login(&error.state).await; - /// return Err(anyhow!("Authorization failed: {:?}", error)); - /// } - /// }; - /// - /// oauth.finish_login(auth_code).await?; + /// oauth.finish_login(redirected_to_uri.into()).await?; /// /// // The session tokens can be persisted from the /// // `Client::session_tokens()` method. @@ -1039,19 +999,31 @@ impl OAuth { /// /// This method should be called after the URL returned by /// [`OAuthAuthCodeUrlBuilder::build()`] has been presented and the user has - /// been redirected to the redirect URI after a successful authorization. + /// been redirected to the redirect URI after completing the authorization. /// - /// If the authorization has not been successful, [`OAuth::abort_login()`] - /// should be used instead to clean up the local data. + /// If the authorization needs to be cancelled before its completion, + /// [`OAuth::abort_login()`] should be used instead to clean up the local + /// data. /// /// # Arguments /// - /// * `auth_code` - The response received as part of the redirect URI when - /// the authorization was successful. + /// * `url_or_query` - The URI where the user was redirected, or just its + /// query part. /// - /// Returns an error if a request fails, or if the client was already - /// logged in with a different session. - pub async fn finish_login(&self, auth_code: AuthorizationCode) -> Result<()> { + /// Returns an error if the authorization failed, if a request fails, or if + /// the client was already logged in with a different session. + pub async fn finish_login(&self, url_or_query: UrlOrQuery) -> Result<()> { + let response = AuthorizationResponse::parse_url_or_query(&url_or_query) + .map_err(|error| OAuthError::from(OAuthAuthorizationCodeError::from(error)))?; + + let auth_code = match response { + AuthorizationResponse::Success(code) => code, + AuthorizationResponse::Error(error) => { + self.abort_login(&error.state).await; + return Err(OAuthError::from(OAuthAuthorizationCodeError::from(error.error)).into()); + } + }; + let device_id = self.finish_authorization(auth_code).await?; self.load_session(device_id).await } @@ -1166,19 +1138,16 @@ impl OAuth { /// Abort the login process. /// - /// This method should be called after the URL returned by - /// [`OAuthAuthCodeUrlBuilder::build()`] has been presented and the user has - /// been redirected to the redirect URI after a failed authorization, or if - /// the authorization should be aborted before it is completed. + /// This method should be called if an authorization should be aborted + /// before it is completed. /// - /// If the authorization has been successful, [`OAuth::finish_login()`] + /// If the authorization has been completed, [`OAuth::finish_login()`] /// should be used instead. /// /// # Arguments /// - /// * `state` - The state received as part of the redirect URI when the - /// authorization failed, or the one provided in - /// [`OAuthAuthorizationData`] after building the authorization URL. + /// * `state` - The state provided in [`OAuthAuthorizationData`] after + /// building the authorization URL. pub async fn abort_login(&self, state: &CsrfToken) { if let Some(data) = self.data() { data.authorization_data.lock().await.remove(state); @@ -1498,7 +1467,7 @@ struct AuthorizationValidationData { /// The data returned by the server in the redirect URI after a successful /// authorization. #[derive(Debug, Clone)] -pub enum AuthorizationResponse { +enum AuthorizationResponse { /// A successful response. Success(AuthorizationCode), @@ -1507,19 +1476,18 @@ pub enum AuthorizationResponse { } impl AuthorizationResponse { - /// Deserialize an `AuthorizationResponse` from the given URI. + /// Deserialize an `AuthorizationResponse` from a [`UrlOrQuery`]. /// - /// Returns an error if the URL doesn't have the expected format. - pub fn parse_uri(uri: &Url) -> Result { - let Some(query) = uri.query() else { return Err(RedirectUriQueryParseError::MissingQuery) }; - + /// Returns an error if the URL or query doesn't have the expected format. + fn parse_url_or_query(url_or_query: &UrlOrQuery) -> Result { + let query = url_or_query.query().ok_or(RedirectUriQueryParseError::MissingQuery)?; Self::parse_query(query) } /// Deserialize an `AuthorizationResponse` from the query part of a URI. /// /// Returns an error if the query doesn't have the expected format. - pub fn parse_query(query: &str) -> Result { + fn parse_query(query: &str) -> Result { // For some reason deserializing the enum with `serde(untagged)` doesn't work, // so let's try both variants separately. if let Ok(code) = serde_html_form::from_str(query) { @@ -1536,22 +1504,22 @@ impl AuthorizationResponse { /// The data returned by the server in the redirect URI after a successful /// authorization. #[derive(Debug, Clone, Deserialize)] -pub struct AuthorizationCode { +struct AuthorizationCode { /// The code to use to retrieve the access token. - pub code: String, + code: String, /// The unique identifier for this transaction. - pub state: CsrfToken, + state: CsrfToken, } /// The data returned by the server in the redirect URI after an authorization /// error. #[derive(Debug, Clone, Deserialize)] -pub struct AuthorizationError { +struct AuthorizationError { /// The error. #[serde(flatten)] - pub error: StandardErrorResponse, + error: StandardErrorResponse, /// The unique identifier for this transaction. - pub state: CsrfToken, + state: CsrfToken, } fn hash_str(x: &str) -> impl fmt::LowerHex { @@ -1599,3 +1567,31 @@ impl From for ClientRegistrationMethod { Self::Store(value) } } + +/// A full URL or just the query part of a URL. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UrlOrQuery { + /// A full URL. + Url(Url), + + /// The query part of a URL. + Query(String), +} + +impl UrlOrQuery { + /// Get the query part of this [`UrlOrQuery`]. + /// + /// If this is a [`Url`], this extracts the query. + pub fn query(&self) -> Option<&str> { + match self { + Self::Url(url) => url.query(), + Self::Query(query) => Some(query), + } + } +} + +impl From for UrlOrQuery { + fn from(value: Url) -> Self { + Self::Url(value) + } +} diff --git a/crates/matrix-sdk/src/authentication/oauth/tests.rs b/crates/matrix-sdk/src/authentication/oauth/tests.rs index 36cd87742..37c243dc6 100644 --- a/crates/matrix-sdk/src/authentication/oauth/tests.rs +++ b/crates/matrix-sdk/src/authentication/oauth/tests.rs @@ -19,6 +19,7 @@ use wiremock::{ use super::{ registration_store::OAuthRegistrationStore, AuthorizationCode, AuthorizationError, AuthorizationResponse, OAuth, OAuthAuthorizationData, OAuthError, RedirectUriQueryParseError, + UrlOrQuery, }; use crate::{ authentication::oauth::{ @@ -186,7 +187,7 @@ async fn test_high_level_login() -> anyhow::Result<()> { redirect_uri.set_query(Some(&format!("code=42&state={}", authorization_data.state.secret()))); // Then the login should succeed. - oauth.login_with_oidc_callback(&authorization_data, redirect_uri).await?; + oauth.finish_login(redirect_uri.into()).await?; Ok(()) } @@ -220,8 +221,7 @@ async fn test_high_level_login_cancellation() -> anyhow::Result<()> { authorization_data.state.secret() ))); - let error = - oauth.login_with_oidc_callback(&authorization_data, redirect_uri).await.unwrap_err(); + let error = oauth.finish_login(redirect_uri.into()).await.unwrap_err(); // Then a cancellation error should be thrown. assert_matches!( @@ -258,8 +258,7 @@ async fn test_high_level_login_invalid_state() -> anyhow::Result<()> { // When completing login with an old/tampered state. redirect_uri.set_query(Some("code=42&state=imposter_alert")); - let error = - oauth.login_with_oidc_callback(&authorization_data, redirect_uri).await.unwrap_err(); + let error = oauth.finish_login(redirect_uri.into()).await.unwrap_err(); // Then the login should fail by flagging the invalid state. assert_matches!( @@ -333,13 +332,13 @@ async fn test_login_url() -> anyhow::Result<()> { fn test_authorization_response() -> anyhow::Result<()> { let uri = Url::parse("https://example.com")?; assert_matches!( - AuthorizationResponse::parse_uri(&uri), + AuthorizationResponse::parse_url_or_query(&uri.into()), Err(RedirectUriQueryParseError::MissingQuery) ); let uri = Url::parse("https://example.com?code=123&state=456")?; assert_matches!( - AuthorizationResponse::parse_uri(&uri), + AuthorizationResponse::parse_url_or_query(&uri.into()), Ok(AuthorizationResponse::Success(AuthorizationCode { code, state })) => { assert_eq!(code, "123"); assert_eq!(state.secret(), "456"); @@ -348,7 +347,7 @@ fn test_authorization_response() -> anyhow::Result<()> { let uri = Url::parse("https://example.com?error=invalid_scope&state=456")?; assert_matches!( - AuthorizationResponse::parse_uri(&uri), + AuthorizationResponse::parse_url_or_query(&uri.into()), Ok(AuthorizationResponse::Error(AuthorizationError { error, state })) => { assert_eq!(*error.error(), AuthorizationCodeErrorResponseType::InvalidScope); assert_eq!(error.error_description(), None); @@ -370,12 +369,7 @@ async fn test_finish_login() -> anyhow::Result<()> { let oauth = client.oauth(); // If the state is missing, then any attempt to finish authorizing will fail. - let res = oauth - .finish_login(AuthorizationCode { - code: "42".to_owned(), - state: CsrfToken::new("none".to_owned()), - }) - .await; + let res = oauth.finish_login(UrlOrQuery::Query("code=42&state=none".to_owned())).await; assert_matches!( res, @@ -402,12 +396,7 @@ async fn test_finish_login() -> anyhow::Result<()> { } // Finishing the authorization for another state won't work. - let res = oauth - .finish_login(AuthorizationCode { - code: "1337".to_owned(), - state: CsrfToken::new("none".to_owned()), - }) - .await; + let res = oauth.finish_login(UrlOrQuery::Query("code=1337&state=none".to_owned())).await; assert_matches!( res, @@ -433,9 +422,7 @@ async fn test_finish_login() -> anyhow::Result<()> { .mount() .await; - oauth - .finish_login(AuthorizationCode { code: "1337".to_owned(), state: state1.clone() }) - .await?; + oauth.finish_login(UrlOrQuery::Query(format!("code=42&state={}", state1.secret()))).await?; let session_tokens = client.session_tokens().unwrap(); assert_eq!(session_tokens.access_token, "AT1"); @@ -477,9 +464,7 @@ async fn test_finish_login() -> anyhow::Result<()> { .mount() .await; - oauth - .finish_login(AuthorizationCode { code: "1337".to_owned(), state: state2.clone() }) - .await?; + oauth.finish_login(UrlOrQuery::Query(format!("code=42&state={}", state2.secret()))).await?; let session_tokens = client.session_tokens().unwrap(); assert_eq!(session_tokens.access_token, "AT2"); @@ -522,9 +507,8 @@ async fn test_finish_login() -> anyhow::Result<()> { .mount() .await; - let res = oauth - .finish_login(AuthorizationCode { code: "1337".to_owned(), state: state3.clone() }) - .await; + let res = + oauth.finish_login(UrlOrQuery::Query(format!("code=42&state={}", state3.secret()))).await; assert_matches!(res, Err(Error::OAuth(OAuthError::SessionMismatch))); assert!(oauth.data().unwrap().authorization_data.lock().await.get(&state3).is_none()); diff --git a/examples/oidc_cli/src/main.rs b/examples/oidc_cli/src/main.rs index 7f3030a9c..3ad84a36a 100644 --- a/examples/oidc_cli/src/main.rs +++ b/examples/oidc_cli/src/main.rs @@ -19,13 +19,13 @@ use std::{ sync::Arc, }; -use anyhow::{anyhow, bail}; +use anyhow::bail; use futures_util::StreamExt; use matrix_sdk::{ authentication::oauth::{ registration::{ApplicationType, ClientMetadata, Localized, OAuthGrantType}, - AccountManagementActionFull, AuthorizationCode, AuthorizationResponse, ClientId, - ClientRegistrationMethod, CsrfToken, OAuthAuthorizationData, OAuthSession, UserSession, + AccountManagementActionFull, ClientId, ClientRegistrationMethod, OAuthAuthorizationData, + OAuthSession, UrlOrQuery, UserSession, }, config::SyncSettings, encryption::{recovery::RecoveryState, CrossSigningResetAuthType}, @@ -34,7 +34,7 @@ use matrix_sdk::{ events::room::message::{MessageType, OriginalSyncRoomMessageEvent}, serde::Raw, }, - utils::local_server::{LocalServerBuilder, LocalServerRedirectHandle}, + utils::local_server::{LocalServerBuilder, LocalServerRedirectHandle, QueryString}, Client, ClientBuildError, Result, RoomState, }; use matrix_sdk_ui::sync_service::SyncService; @@ -218,27 +218,21 @@ impl OidcCli { // the redirect when the custom URI scheme is opened. let (redirect_uri, server_handle) = LocalServerBuilder::new().spawn().await?; - let OAuthAuthorizationData { url, state } = + let OAuthAuthorizationData { url, .. } = oauth.login(ClientRegistrationMethod::None, redirect_uri, None).build().await?; - let authorization_code = match use_auth_url(&url, &state, server_handle).await { - Ok(code) => code, - Err(err) => { - oauth.abort_login(&state).await; - return Err(err); - } - }; + let query_string = + use_auth_url(&url, server_handle).await.map(|query| query.0).unwrap_or_default(); - match oauth.finish_login(authorization_code).await { + match oauth.finish_login(UrlOrQuery::Query(query_string)).await { Ok(()) => { let user_id = self.client.user_id().expect("Got a user ID"); println!("Logged in as {user_id}"); break; } Err(err) => { - println!("Error: failed to finish login: {err}"); + println!("Error: failed to login: {err}"); println!("Please try again.\n"); - oauth.abort_login(&state).await; continue; } } @@ -731,33 +725,11 @@ fn client_metadata() -> Raw { /// Open the authorization URL and wait for it to be complete. /// /// Returns the code to obtain the access token. -async fn use_auth_url( - url: &Url, - state: &CsrfToken, - server_handle: LocalServerRedirectHandle, -) -> anyhow::Result { +async fn use_auth_url(url: &Url, server_handle: LocalServerRedirectHandle) -> Option { println!("\nPlease authenticate yourself at: {url}\n"); println!("Then proceed to the authorization.\n"); - let response_query = server_handle.await; - - let code = - match AuthorizationResponse::parse_query(response_query.as_deref().unwrap_or_default())? { - AuthorizationResponse::Success(code) => code, - AuthorizationResponse::Error(err) => { - return Err(anyhow!(err.error)); - } - }; - - // Here we only manage one authorization at a time so, if the state string is - // wrong, it is an error. Some clients might want to allow several - // authorizations at once, in which case the state string can be used to - // identify the session that was authorized. - if code.state != *state { - bail!("State strings don't match") - } - - Ok(code) + server_handle.await } /// Handle room messages.