Dima/matrix-rust-sdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(oauth): Merge OAuth::login_with_oidc_callback() and OAuth::finish_login()

Browse Source 
Accept a URL or a query string for simplicity.

That way we don't need to expose AuthorizationResponse.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
This commit is contained in:
Kévin Commaille
2025-03-21 15:05:50 +01:00
committed by Ivan Enderlin
parent a8aa364757
commit b3e82a05db
4 changed files with 95 additions and 147 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bindings/matrix-sdk-ffi/src/client.rs
+2 -6
View File
@@ -428,14 +428,10 @@ impl Client {
}
/// Completes the OIDC login process.
pub async fn login_with_oidc_callback(
&self,
authorization_data: Arc<OAuthAuthorizationData>,
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(())
}
crates/matrix-sdk/src/authentication/oauth/mod.rs
+69 -73
View File
@@ -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<Self, RedirectUriQueryParseError> {
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<Self, RedirectUriQueryParseError> {
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<Self, RedirectUriQueryParseError> {
fn parse_query(query: &str) -> Result<Self, RedirectUriQueryParseError> {
// 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::AuthorizationCodeErrorResponseType>,
error: StandardErrorResponse<error::AuthorizationCodeErrorResponseType>,
/// 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<OAuthRegistrationStore> 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<Url> for UrlOrQuery {
fn from(value: Url) -> Self {
Self::Url(value)
}
}
crates/matrix-sdk/src/authentication/oauth/tests.rs
+13 -29
View File
@@ -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());
examples/oidc_cli/src/main.rs
+11 -39
View File
@@ -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<ClientMetadata> {
/// 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<AuthorizationCode> {
async fn use_auth_url(url: &Url, server_handle: LocalServerRedirectHandle) -> Option<QueryString> {
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.