refactor(oidc): Use oauth2 for token revocation

Signed-off-by: Kévin Commaille <[email protected]>

crates/matrix-sdk/Cargo.toml

@@ -47,10 +47,8 @@ uniffi = ["dep:uniffi", "matrix-sdk-base/uniffi", "dep:matrix-sdk-ffi-macros"]
 experimental-oidc = [
     "ruma/unstable-msc2967",
     "ruma/unstable-msc4108",
-    "dep:chrono",
     "dep:language-tags",
     "dep:mas-oidc-client",
-    "dep:rand",
     "dep:sha2",
     "dep:tower",
     "dep:oauth2",
@@ -71,7 +69,6 @@ async-trait = { workspace = true }
 axum = { version = "0.8.1", optional = true }
 bytes = "1.9.0"
 bytesize = "1.3.0"
-chrono = { workspace = true, optional = true }
 event-listener = "5.4.0"
 eyeball = { workspace = true }
 eyeball-im = { workspace = true }

crates/matrix-sdk/src/authentication/oidc/cross_process.rs

@@ -539,12 +539,18 @@ mod tests {
         let server = MatrixMockServer::new().await;
 
         let oauth_server = server.oauth();
-        oauth_server.mock_server_metadata().ok().expect(1..).named("server_metadata").mount().await;
-        oauth_server.mock_revocation().ok().expect(1).named("token").mount().await;
+        oauth_server
+            .mock_server_metadata()
+            .ok_https()
+            .expect(1..)
+            .named("server_metadata")
+            .mount()
+            .await;
+        oauth_server.mock_revocation().ok().expect(1).named("revocation").mount().await;
 
         let tmp_dir = tempfile::tempdir()?;
         let client = server.client_builder().sqlite_store(&tmp_dir).unlogged().build().await;
-        let oidc = client.oidc();
+        let oidc = client.oidc().insecure_rewrite_https_to_http();
 
         // Enable cross-process lock.
         oidc.enable_cross_process_refresh_lock("lock".to_owned()).await?;

crates/matrix-sdk/src/authentication/oidc/error.rs

@@ -18,8 +18,12 @@ pub use mas_oidc_client::error::*;
 use matrix_sdk_base::deserialized_responses::PrivOwnedStr;
 use oauth2::ErrorResponseType;
 pub use oauth2::{
-    basic::{BasicErrorResponse, BasicErrorResponseType, BasicRequestTokenError},
-    HttpClientError, RequestTokenError, StandardErrorResponse,
+    basic::{
+        BasicErrorResponse, BasicErrorResponseType, BasicRequestTokenError,
+        BasicRevocationErrorResponse,
+    },
+    ConfigurationError, HttpClientError, RequestTokenError, RevocationErrorResponseType,
+    StandardErrorResponse,
 };
 use ruma::serde::{PartialEqAsRefStr, StringEnum};
 
@@ -81,14 +85,9 @@ pub enum OidcError {
     #[error("failed to refresh token: {0}")]
     RefreshToken(BasicRequestTokenError<HttpClientError<reqwest::Error>>),
 
-    /// The OpenID Connect Provider doesn't support token revocation, aka
-    /// logging out.
-    #[error("no token revocation support")]
-    NoRevocationSupport,
-
-    /// An error occurred generating a random value.
-    #[error(transparent)]
-    Rand(rand::Error),
+    /// An error occurred revoking an OAuth 2.0 access token.
+    #[error("failed to log out: {0}")]
+    Logout(#[from] OauthTokenRevocationError),
 
     /// An error occurred parsing a URL.
     #[error(transparent)]
@@ -224,3 +223,21 @@ pub enum AuthorizationCodeErrorResponseType {
 }
 
 impl ErrorResponseType for AuthorizationCodeErrorResponseType {}
+
+/// All errors that can occur when revoking an OAuth 2.0 token.
+#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum OauthTokenRevocationError {
+    /// Revocation is not supported by the OAuth 2.0 authorization server.
+    #[error("token revocation is not supported")]
+    NotSupported,
+
+    /// The revocation endpoint URL is insecure.
+    #[error(transparent)]
+    Url(ConfigurationError),
+
+    /// An error occurred interacting with the OAuth 2.0 authorization server
+    /// while revoking the token.
+    #[error("failed to revoke token: {0}")]
+    Revoke(RequestTokenError<HttpClientError<reqwest::Error>, BasicRevocationErrorResponse>),
+}

crates/matrix-sdk/src/authentication/oidc/mod.rs

@@ -149,10 +149,9 @@
 use std::{borrow::Cow, collections::HashMap, fmt, future::Future, pin::Pin, sync::Arc};
 
 use as_variant::as_variant;
-use chrono::Utc;
 use error::{
     CrossProcessRefreshLockError, OauthAuthorizationCodeError, OauthDiscoveryError,
-    RedirectUriQueryParseError,
+    OauthTokenRevocationError, RedirectUriQueryParseError,
 };
 use eyeball::SharedObservable;
 use futures_core::Stream;
@@ -162,11 +161,8 @@ use mas_oidc_client::{
         account_management::{build_account_management_url, AccountManagementActionFull},
         discovery::{discover, insecure_discover},
         registration::register_client,
-        revocation::revoke_token,
     },
     types::{
-        client_credentials::ClientCredentials,
-        iana::oauth::OAuthTokenTypeHint,
         oidc::{
             AccountManagementAction, ProviderMetadata, ProviderMetadataVerificationError,
             VerifiedProviderMetadata,
@@ -180,11 +176,10 @@ use matrix_sdk_base::crypto::types::qr_login::QrCodeData;
 use matrix_sdk_base::{once_cell::sync::OnceCell, SessionMeta};
 pub use oauth2::CsrfToken;
 use oauth2::{
-    basic::BasicClient as OauthClient, AsyncHttpClient, HttpRequest, HttpResponse,
-    PkceCodeVerifier, RedirectUrl, RefreshToken, Scope, StandardErrorResponse, TokenResponse,
-    TokenUrl,
+    basic::BasicClient as OauthClient, AccessToken, AsyncHttpClient, HttpRequest, HttpResponse,
+    PkceCodeVerifier, RedirectUrl, RefreshToken, RevocationUrl, Scope, StandardErrorResponse,
+    StandardRevocableToken, TokenResponse, TokenUrl,
 };
-use rand::{rngs::StdRng, SeedableRng};
 use ruma::{
     api::client::discovery::{
         get_authentication_issuer,
@@ -217,9 +212,7 @@ use self::{
     registrations::{ClientId, OidcRegistrations},
 };
 use super::AuthData;
-use crate::{
-    client::SessionChange, http_client::HttpClient, Client, HttpError, RefreshTokenError, Result,
-};
+use crate::{client::SessionChange, Client, HttpError, RefreshTokenError, Result};
 
 pub(crate) struct OidcCtx {
     /// Lock and state when multiple processes may refresh an OIDC session.
@@ -253,13 +246,6 @@ pub(crate) struct OidcAuthData {
     authorization_data: Mutex<HashMap<CsrfToken, AuthorizationValidationData>>,
 }
 
-impl OidcAuthData {
-    /// Get the credentials of client.
-    fn credentials(&self) -> ClientCredentials {
-        ClientCredentials::None { client_id: self.client_id.as_str().to_owned() }
-    }
-}
-
 #[cfg(not(tarpaulin_include))]
 impl fmt::Debug for OidcAuthData {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -272,23 +258,40 @@ impl fmt::Debug for OidcAuthData {
 pub struct Oidc {
     /// The underlying Matrix API client.
     client: Client,
+    /// The HTTP client used for making OAuth 2.0 request.
+    http_client: OauthHttpClient,
 }
 
 impl Oidc {
     pub(crate) fn new(client: Client) -> Self {
-        Self { client }
+        let http_client = OauthHttpClient {
+            inner: client.inner.http_client.inner.clone(),
+            #[cfg(test)]
+            insecure_rewrite_https_to_http: false,
+        };
+        Self { client, http_client }
+    }
+
+    /// Rewrite HTTPS requests to use HTTP instead.
+    ///
+    /// This is a workaround to bypass some checks that require an HTTPS URL,
+    /// but we can only mock HTTP URLs.
+    #[cfg(test)]
+    pub(crate) fn insecure_rewrite_https_to_http(mut self) -> Self {
+        self.http_client.insecure_rewrite_https_to_http = true;
+        self
     }
 
     fn ctx(&self) -> &OidcCtx {
         &self.client.inner.auth_ctx.oidc
     }
 
-    fn http_client(&self) -> &HttpClient {
-        &self.client.inner.http_client
+    fn http_client(&self) -> &OauthHttpClient {
+        &self.http_client
     }
 
     fn http_service(&self) -> HttpService {
-        HttpService::new(self.http_client().clone())
+        HttpService::new(self.client.inner.http_client.clone())
     }
 
     /// Enable a cross-process store lock on the state store, to coordinate
@@ -1582,25 +1585,27 @@ impl Oidc {
 
     /// Log out from the currently authenticated session.
     pub async fn logout(&self) -> Result<(), OidcError> {
-        let provider_metadata = self.provider_metadata().await?;
-        let client_credentials = self.data().ok_or(OidcError::NotAuthenticated)?.credentials();
+        let client_id = self.client_id().ok_or(OidcError::NotAuthenticated)?.clone();
 
-        let revocation_endpoint =
-            provider_metadata.revocation_endpoint.as_ref().ok_or(OidcError::NoRevocationSupport)?;
+        let provider_metadata = self.provider_metadata().await?;
+        let revocation_endpoint = provider_metadata
+            .revocation_endpoint
+            .clone()
+            .ok_or(OauthTokenRevocationError::NotSupported)?;
+        let revocation_url = RevocationUrl::from_url(revocation_endpoint);
 
         let tokens = self.session_tokens().ok_or(OidcError::NotAuthenticated)?;
 
         // Revoke the access token, it should revoke both tokens.
-        revoke_token(
-            &self.http_service(),
-            client_credentials,
-            revocation_endpoint,
-            tokens.access_token,
-            Some(OAuthTokenTypeHint::AccessToken),
-            Utc::now(),
-            &mut rng()?,
-        )
-        .await?;
+        OauthClient::new(client_id)
+            .set_revocation_url(revocation_url)
+            .revoke_token(StandardRevocableToken::AccessToken(AccessToken::new(
+                tokens.access_token,
+            )))
+            .map_err(OauthTokenRevocationError::Url)?
+            .request_async(self.http_client())
+            .await
+            .map_err(OauthTokenRevocationError::Revoke)?;
 
         if let Some(manager) = self.ctx().cross_process_token_refresh_manager.get() {
             manager.on_logout().await?;
@@ -1724,22 +1729,45 @@ pub struct AuthorizationError {
     pub state: CsrfToken,
 }
 
-fn rng() -> Result<StdRng, OidcError> {
-    StdRng::from_rng(rand::thread_rng()).map_err(OidcError::Rand)
-}
-
 fn hash_str(x: &str) -> impl fmt::LowerHex {
     sha2::Sha256::new().chain_update(x).finalize()
 }
 
-impl<'c> AsyncHttpClient<'c> for HttpClient {
+#[derive(Debug, Clone)]
+struct OauthHttpClient {
+    inner: reqwest::Client,
+    /// Rewrite HTTPS requests to use HTTP instead.
+    ///
+    /// This is a workaround to bypass some checks that require an HTTPS URL,
+    /// but we can only mock HTTP URLs.
+    #[cfg(test)]
+    insecure_rewrite_https_to_http: bool,
+}
+
+impl<'c> AsyncHttpClient<'c> for OauthHttpClient {
     type Error = error::HttpClientError<reqwest::Error>;
 
     type Future =
         Pin<Box<dyn Future<Output = Result<HttpResponse, Self::Error>> + Send + Sync + 'c>>;
 
     fn call(&'c self, request: HttpRequest) -> Self::Future {
         Box::pin(async move {
+            #[cfg(test)]
+            let request = if self.insecure_rewrite_https_to_http
+                && request.uri().scheme().is_some_and(|scheme| *scheme == http::uri::Scheme::HTTPS)
+            {
+                let mut request = request;
+
+                let mut uri_parts = request.uri().clone().into_parts();
+                uri_parts.scheme = Some(http::uri::Scheme::HTTP);
+                *request.uri_mut() = http::uri::Uri::from_parts(uri_parts)
+                    .expect("reconstructing URI from parts should work");
+
+                request
+            } else {
+                request
+            };
+
             let response = self.inner.call(request).await?;
 
             Ok(response)

crates/matrix-sdk/src/test_utils/mocks/oauth.rs

@@ -119,6 +119,21 @@ impl<'a> MockEndpoint<'a, ServerMetadataEndpoint> {
         MatrixMock { server: self.server, mock }
     }
 
+    /// Returns a successful metadata response with all the supported endpoints
+    /// using HTTPS URLs.
+    ///
+    /// This should be used with
+    /// `MockClientBuilder::insecure_rewrite_https_to_http()` to bypass checks
+    /// from the oauth2 crate.
+    pub fn ok_https(self) -> MatrixMock<'a> {
+        let issuer = self.server.uri().replace("http://", "https://");
+
+        let metadata = MockServerMetadataBuilder::new(&issuer).build();
+        let mock = self.mock.respond_with(ResponseTemplate::new(200).set_body_json(metadata));
+
+        MatrixMock { server: self.server, mock }
+    }
+
     /// Returns a successful metadata response without the device authorization
     /// endpoint.
     pub fn ok_without_device_authorization(self) -> MatrixMock<'a> {