macltok

public

Star0

Fork 0

Watch0

40 commits 3 branches 0 tags 0 releases3.0 MiB

Commit `d8d8a9fe770a`

tarball

atagen <boss@atagen.co>2026-10-22 09:31 UTC

5 files changed+358-32

account: let an oauth login finish after its page went away for the redirect

A web host that navigates to the authorization server loses the process, and with it the PKCE verifier the sdk kept in a private map, so OAuth only worked in a popup. The fork's PendingAuthorization export lets begin_oauth seal the pending login into the store's record, and Accounts::finish_oauth finds it again by the callback's state. Keeping it in the store rather than with the host stops Accounts::list from sweeping the new store mid-redirect.

Modified file: docs/design-decisions.md+20 additions−10 deletions@@ -283,16 +283,24 @@ first still thinks it's alone. with our lock keeping everyone else out, clients
 run with `CrossProcessLockConfig::SingleProcess` and the sdk's own sections skip
 the lease writes and renewal tasks
 
-a pending OAuth login isn't persisted. `begin_oauth` and `finish` have to run in
-the same process, so a web host runs the login in a popup or in a worker that
-outlives the redirect. the sdk keeps what `finish_login` needs, the PKCE
-verifier among it, in `AuthorizationValidationData` inside a `pub(crate)` map
-keyed by `CsrfToken` (matrix-sdk 0.18, `authentication/oauth/mod.rs` around
-262-266 and 1744-1756). nothing public reads it or puts it back, and
-`finish_login` on a fresh client answers `InvalidState`. doing the code grant
-ourselves would copy some 250 lines of the sdk's private oauth behaviour and
-drift from it every upgrade. patching the sdk to export and import that data is
-small, and since we carry a fork now that's the way to do it. not built yet
+a pending OAuth login survives the page going away for its redirect.
+`begin_oauth` saves it as the store's record, sealed like any other since it
+holds the PKCE verifier, through the fork's `OAuth::pending_authorization`.
+`Accounts::finish_oauth` takes the callback url and finds the account by its
+`state`, so the host keeps nothing across the redirect, and
+`Session::finish_oauth` does the same for one store. a host that keeps the
+process alive can still use `PendingOAuthLogin::finish`. while it waits,
+`restore` answers `Restore::OAuthPending`. the record lives in the store
+rather than with the host because `list` sweeps a store with no record, which
+would delete a fresh login's store mid redirect
+
+the pending record keeps whatever the store held before, so a login resuming a
+soft-logged-out device that never comes back puts that device back, and a
+password login can still resume it meanwhile. abort, a failed finish after the
+redirect and an hour's wait all undo the login, a new account's store deleted
+and an old record put back. a failed `PendingOAuthLogin::finish` still holds
+the client, so it puts back an old record and leaves a new one to the hour.
+codes live minutes, so an hour is generous
 
 ## room lists, spaces and profiles
 
@@ -389,6 +397,8 @@ which keep their public sans-io apis
   with `Timeline::toggle_reaction_with_shortcode`
 - `SendQueue::enable_global_upload_progress`, upload progress on the
   client-wide send queue channel
+- `OAuth::pending_authorization` and `restore_pending_authorization`, to
+  finish an oauth login in another process
 
 ## public model
 

Modified file: examples/headless.rs+1−1

@@ -23,7 +23,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {

2323 )

2424 .await?

2525 }

26- None => {

26+ None | Some(Restore::OAuthPending) => {

2727 Session::login_password(

2828 PasswordLogin::new(

2929 required("MATRIX_HOMESERVER")?,

Modified file: src/account.rs+198 additions−11 deletions@@ -11,13 +11,14 @@ use matrix_sdk::{
         AuthSession,
         matrix::MatrixSession,
         oauth::{
-            ClientId, ClientRegistrationData, CsrfToken, OAuthSession, UserSession,
+            ClientId, ClientRegistrationData, CsrfToken, OAuthSession, PendingAuthorization,
+            UserSession,
             registration::{ApplicationType, ClientMetadata, Localized, OAuthGrantType},
         },
     },
     cross_process_lock::CrossProcessLockConfig,
     encryption::{BackupDownloadStrategy, EncryptionSettings},
-    ruma::{OwnedDeviceId, OwnedUserId, serde::Raw},
+    ruma::{MilliSecondsSinceUnixEpoch, OwnedDeviceId, OwnedUserId, serde::Raw},
 };
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
@@ -167,6 +168,7 @@ pub struct PendingOAuthLogin {
     tracking: Option<(Tracking, StoreLock)>,
     authorization_url: String,
     state: Option<CsrfToken>,
+    previous: Option<SessionRecord>,
 }
 
 impl PendingOAuthLogin {
@@ -200,6 +202,9 @@ impl PendingOAuthLogin {
         let oauth = self.client.oauth();
         if let Err(error) = oauth.finish_login(callback_url.into()).await {
             oauth.abort_login(&state).await;
+            if let Some(previous) = self.previous.take() {
+                put_back(self.sessions.as_ref(), previous).await;
+            }
             return Err(account_error("OAuth login", error));
         }
         let (tracking, store_lock) = self
@@ -215,10 +220,36 @@ impl PendingOAuthLogin {
         .await
     }
 
-    pub async fn abort(mut self) {
-        if let Some(state) = self.state.take() {
-            self.client.oauth().abort_login(&state).await;
+    /// puts back what the store held before, or deletes the store if it held nothing
+    pub async fn abort(self) {
+        let Self {
+            client,
+            sessions,
+            tracking,
+            state,
+            previous,
+            ..
+        } = self;
+        let Some(state) = state else {
+            return;
+        };
+        client.oauth().abort_login(&state).await;
+        drop(client);
+        match previous {
+            Some(previous) => put_back(sessions.as_ref(), previous).await,
+            None => {
+                if let Err(error) = sessions.discard().await {
+                    tracing::warn!(%error, "deleting an abandoned OAuth login's store failed");
+                }
+            }
         }
+        drop(tracking);
+    }
+}
+
+async fn put_back(sessions: &dyn SessionStore, previous: SessionRecord) {
+    if let Err(error) = sessions.save(&previous).await {
+        tracing::warn!(%error, "putting back the record an OAuth login replaced failed");
     }
 }
 
@@ -308,6 +339,32 @@ enum AuthenticationRecord {
         meta: SessionMeta,
         client_id: Option<ClientId>,
     },
+    /// an oauth login waiting on its redirect. `authorization` holds the pkce verifier
+    PendingOAuth {
+        client_id: ClientId,
+        state: CsrfToken,
+        authorization: Box<PendingAuthorization>,
+        started_ms: u64,
+        /// what the store held before, put back if the login never finishes
+        previous: Option<Box<SessionRecord>>,
+    },
+}
+
+/// authorization codes live for minutes, so a login this old was abandoned
+const OAUTH_PENDING_FOR_MS: u64 = 60 * 60 * 1000;
+
+/// the authorization server answers in the query, or the fragment when asked to
+pub(crate) fn callback_state(callback: &Url) -> Option<String> {
+    let query = callback.query_pairs();
+    let fragment = url::form_urlencoded::parse(callback.fragment().unwrap_or_default().as_bytes());
+    query
+        .chain(fragment)
+        .find(|(key, _)| key == "state")
+        .map(|(_, state)| state.into_owned())
+}
+
+fn now_ms() -> u64 {
+    MilliSecondsSinceUnixEpoch::now().get().into()
 }
 
 impl SessionRecord {
@@ -316,17 +373,47 @@ impl SessionRecord {
         &self.homeserver
     }
 
-    pub(crate) fn user_id(&self) -> &OwnedUserId {
+    /// a pending oauth login is nobody yet, unless it resumes someone
+    pub(crate) fn user_id(&self) -> Option<&OwnedUserId> {
         match &self.authentication {
-            AuthenticationRecord::Matrix(session) => &session.meta.user_id,
-            AuthenticationRecord::OAuth { user, .. } => &user.meta.user_id,
-            AuthenticationRecord::SoftLoggedOut { meta, .. } => &meta.user_id,
+            AuthenticationRecord::Matrix(session) => Some(&session.meta.user_id),
+            AuthenticationRecord::OAuth { user, .. } => Some(&user.meta.user_id),
+            AuthenticationRecord::SoftLoggedOut { meta, .. } => Some(&meta.user_id),
+            AuthenticationRecord::PendingOAuth { previous, .. } => previous.as_ref()?.user_id(),
+        }
+    }
+
+    /// what a login should resume. a pending oauth login stands in for the record before it
+    pub(crate) fn resumable(self) -> Option<Self> {
+        match self.authentication {
+            AuthenticationRecord::PendingOAuth { previous, .. } => {
+                previous.and_then(|previous| previous.resumable())
+            }
+            _ => Some(self),
         }
     }
 
+    pub(crate) fn pending_oauth_state(&self) -> Option<&str> {
+        match &self.authentication {
+            AuthenticationRecord::PendingOAuth { state, .. } => Some(state.secret()),
+            _ => None,
+        }
+    }
+
+    pub(crate) fn oauth_expired(&self) -> bool {
+        let AuthenticationRecord::PendingOAuth { started_ms, .. } = &self.authentication else {
+            return false;
+        };
+        now_ms().saturating_sub(*started_ms) > OAUTH_PENDING_FOR_MS
+    }
+
     pub(crate) fn soft_logout(&self) -> Option<SoftLogout> {
-        let AuthenticationRecord::SoftLoggedOut { meta, client_id } = &self.authentication else {
-            return None;
+        let (meta, client_id) = match &self.authentication {
+            AuthenticationRecord::SoftLoggedOut { meta, client_id } => (meta, client_id),
+            AuthenticationRecord::PendingOAuth { previous, .. } => {
+                return previous.as_ref()?.soft_logout();
+            }
+            _ => return None,
         };
         Some(SoftLogout {
             homeserver: self.homeserver.clone(),
@@ -351,6 +438,7 @@ impl SessionRecord {
                 }
             }
             soft @ AuthenticationRecord::SoftLoggedOut { .. } => soft,
+            pending @ AuthenticationRecord::PendingOAuth { .. } => pending,
         };
         Self {
             homeserver: self.homeserver,
@@ -362,6 +450,8 @@ impl SessionRecord {
 pub enum Restore {
     Resumed(Session),
     SoftLoggedOut(SoftLogout),
+    /// an oauth login is waiting on its redirect. `Session::finish_oauth` takes the callback
+    OAuthPending,
 }
 
 /// who to log in again as. the store keeps this device's e2ee keys, so a login
@@ -422,6 +512,10 @@ pub enum AccountError {
     UnsupportedAuthentication,
     #[error("the OAuth callback was already submitted")]
     OAuthCallbackAlreadySubmitted,
+    /// nothing is waiting for this callback. it was finished, abandoned, expired, or begun
+    /// somewhere else
+    #[error("no OAuth login is waiting for this callback")]
+    NoPendingOAuth,
     /// what was typed can't name a homeserver, so nothing was sent anywhere
     #[error("that isn't a usable homeserver, {0}")]
     InvalidHomeserver(&'static str),
@@ -586,15 +680,99 @@ impl Session {
             .build()
             .await
             .map_err(|error| account_error("starting OAuth login", error))?;
+        let previous = sessions.load().await?.and_then(SessionRecord::resumable);
+        let pending = SessionRecord {
+            homeserver: client.homeserver().to_string(),
+            authentication: AuthenticationRecord::PendingOAuth {
+                client_id: client
+                    .oauth()
+                    .client_id()
+                    .ok_or(AccountError::MissingSession)?
+                    .clone(),
+                state: authorization.state.clone(),
+                authorization: Box::new(
+                    client
+                        .oauth()
+                        .pending_authorization(&authorization.state)
+                        .await
+                        .ok_or(AccountError::MissingSession)?,
+                ),
+                started_ms: now_ms(),
+                previous: previous.clone().map(Box::new),
+            },
+        };
+        sessions.save(&pending).await?;
         Ok(PendingOAuthLogin {
             client,
             sessions,
             tracking: Some((tracking, store_lock)),
             authorization_url: authorization.url.to_string(),
             state: Some(authorization.state),
+            previous,
         })
     }
 
+    /// finishes a login `begin_oauth` started in another process, from the callback the
+    /// redirect brought back. the pending login is in `sessions`, so nothing else is needed
+    pub async fn finish_oauth(
+        sessions: Arc<dyn SessionStore>,
+        callback_url: &str,
+    ) -> Result<Self, AccountError> {
+        let store_lock = sessions.store().lock().await?;
+        Self::finish_oauth_locked(sessions, store_lock, callback_url).await
+    }
+
+    pub(crate) async fn finish_oauth_locked(
+        sessions: Arc<dyn SessionStore>,
+        store_lock: StoreLock,
+        callback_url: &str,
+    ) -> Result<Self, AccountError> {
+        let callback = Url::parse(callback_url)
+            .map_err(|error| account_error("parsing the OAuth callback URL", error))?;
+        let record = sessions.load().await?.ok_or(AccountError::NoPendingOAuth)?;
+        if record.pending_oauth_state() != callback_state(&callback).as_deref() {
+            return Err(AccountError::NoPendingOAuth);
+        }
+        let AuthenticationRecord::PendingOAuth {
+            client_id,
+            state,
+            authorization,
+            previous,
+            ..
+        } = record.authentication
+        else {
+            return Err(AccountError::NoPendingOAuth);
+        };
+        let client = matrix_client_builder(sessions.store())
+            .homeserver_url(&record.homeserver)
+            .build()
+            .await
+            .map_err(|error| account_error("reopening the Matrix store", error))?;
+        let tracking = Tracking::start(&client, sessions.clone())?;
+        let oauth = client.oauth();
+        oauth.restore_registered_client(client_id);
+        let finished = match oauth
+            .restore_pending_authorization(state, *authorization)
+            .await
+        {
+            Ok(()) => oauth.finish_login(callback.into()).await,
+            Err(error) => Err(error.into()),
+        };
+        if let Err(error) = finished {
+            match previous {
+                Some(previous) => put_back(sessions.as_ref(), *previous).await,
+                None => {
+                    drop((tracking, client));
+                    if let Err(error) = sessions.discard().await {
+                        tracing::warn!(%error, "deleting a failed OAuth login's store failed");
+                    }
+                }
+            }
+            return Err(account_error("OAuth login", error));
+        }
+        Self::saved(client, sessions, tracking, store_lock).await
+    }
+
     /// Restore the record `sessions` holds, or `None` when it holds none.
     /// The caller must also handle rejection and explicit logout.
     pub async fn restore(sessions: Arc<dyn SessionStore>) -> Result<Option<Restore>, AccountError> {
@@ -606,6 +784,15 @@ impl Session {
             AuthenticationRecord::OAuth { client_id, user } => {
                 OAuthSession { client_id, user }.into()
             }
+            AuthenticationRecord::PendingOAuth { .. } if record.oauth_expired() => {
+                let previous = record.resumable();
+                match previous {
+                    Some(previous) => put_back(sessions.as_ref(), previous).await,
+                    None => sessions.discard().await?,
+                }
+                return Box::pin(Self::restore(sessions)).await;
+            }
+            AuthenticationRecord::PendingOAuth { .. } => return Ok(Some(Restore::OAuthPending)),
             AuthenticationRecord::SoftLoggedOut { meta, client_id } => {
                 return Ok(Some(Restore::SoftLoggedOut(SoftLogout {
                     homeserver: record.homeserver,

Modified file: src/accounts.rs+71 additions−8 deletions@@ -6,7 +6,8 @@ use ruma::TransactionId;
 
 use crate::{
     AccountError, EncryptedSessionStore, OAuthLogin, PasswordLogin, PendingOAuthLogin, Restore,
-    Session, SessionRecord, SessionStore, SoftLogout, StoreConfig, account::homeserver_input,
+    Session, SessionRecord, SessionStore, SoftLogout, StoreConfig,
+    account::{callback_state, homeserver_input},
     engine::StoreLock,
 };
 
@@ -94,18 +95,50 @@ impl Accounts {
     }
 
     /// every account with a record. a store left without one, by a login that died halfway or a
-    /// discard that couldn't finish, is deleted on the way past
+    /// discard that couldn't finish, is deleted on the way past, and so is an oauth login whose
+    /// redirect never came back. one that resumed an account puts that account back
     pub async fn list(&self) -> Result<Vec<Account>, AccountError> {
         let mut accounts = Vec::new();
         for account in self.slots().await? {
             match account.sessions.load().await {
                 Ok(None) => self.sweep(&account).await,
+                Ok(Some(record)) if record.oauth_expired() => {
+                    if self.expire(&account).await {
+                        accounts.push(account);
+                    }
+                }
                 Ok(Some(_)) | Err(_) => accounts.push(account),
             }
         }
         Ok(accounts)
     }
 
+    /// finishes an oauth login begun before a redirect, finding its account by the callback's
+    /// state, so the host needs to have kept nothing across the redirect
+    pub async fn finish_oauth(
+        &self,
+        callback_url: &str,
+    ) -> Result<(Account, Session), AccountError> {
+        let state = url::Url::parse(callback_url)
+            .ok()
+            .as_ref()
+            .and_then(callback_state)
+            .ok_or(AccountError::NoPendingOAuth)?;
+        for account in self.slots().await? {
+            let Ok(Some(record)) = account.sessions.load().await else {
+                continue;
+            };
+            if record.pending_oauth_state() != Some(state.as_str()) {
+                continue;
+            }
+            let store_lock = account.sessions.store().lock().await?;
+            let session =
+                Session::finish_oauth_locked(account.sessions(), store_lock, callback_url).await?;
+            return Ok((account, session));
+        }
+        Err(AccountError::NoPendingOAuth)
+    }
+
     /// a soft-logged-out account the login names is resumed as its own device, keeping its keys.
     /// anyone else gets a new account
     pub async fn login_password(
@@ -193,9 +226,10 @@ impl Accounts {
             if !resumes(&record) {
                 continue;
             }
-            return match record.soft_logout() {
-                Some(soft) => Ok(Found::SoftLoggedOut(account, soft)),
-                None => Err(AccountError::AlreadyLoggedIn(record.user_id().clone())),
+            return match (record.soft_logout(), record.user_id()) {
+                (Some(soft), _) => Ok(Found::SoftLoggedOut(account, soft)),
+                (None, Some(user_id)) => Err(AccountError::AlreadyLoggedIn(user_id.clone())),
+                (None, None) => continue,
             };
         }
         Ok(Found::Nobody)
@@ -227,9 +261,36 @@ impl Accounts {
         let Ok(_store_lock) = account.sessions.store().lock().await else {
             return;
         };
-        if !matches!(account.sessions.load().await, Ok(None)) {
-            return;
+        if matches!(account.sessions.load().await, Ok(None)) {
+            self.remove(account).await;
         }
+    }
+
+    /// whether the account is still there. a login running in another tab holds the lock and
+    /// is left alone
+    async fn expire(&self, account: &Account) -> bool {
+        let Ok(_store_lock) = account.sessions.store().lock().await else {
+            return true;
+        };
+        let Ok(Some(record)) = account.sessions.load().await else {
+            return true;
+        };
+        if !record.oauth_expired() {
+            return true;
+        }
+        match record.resumable() {
+            Some(previous) => {
+                let _ = account.sessions.save(&previous).await;
+                true
+            }
+            None => {
+                self.remove(account).await;
+                false
+            }
+        }
+    }
+
+    async fn remove(&self, account: &Account) {
         if account.sessions.discard().await.is_err() {
             return;
         }
@@ -274,7 +335,9 @@ impl Accounts {
 /// is compared by name as well as by url, so a delegated `example.org` still finds the account
 /// discovery took to `matrix.example.org`
 fn record_names(record: &SessionRecord, homeserver: &str, username: &str) -> bool {
-    let user_id = record.user_id();
+    let Some(user_id) = record.user_id() else {
+        return false;
+    };
     let username = username.trim();
     let user = if username.starts_with('@') {
         username.eq_ignore_ascii_case(user_id.as_str())

Modified file: tests/account.rs+68 additions−2 deletions@@ -4,8 +4,8 @@ mod support;
 use std::{path::PathBuf, sync::Arc, time::SystemTime};
 
 use macltok::{
-    AccountError, Accounts, EncryptedSessionStore, EngineConfig, Event, PasswordLogin, Restore,
-    Session, SessionRecord, SessionStore, StoreConfig,
+    AccountError, Accounts, EncryptedSessionStore, EngineConfig, Event, OAuthApplicationType,
+    OAuthLogin, PasswordLogin, Restore, Session, SessionRecord, SessionStore, StoreConfig,
 };
 use matrix_sdk::{ruma::api::client::sync::sync_events::v5, test_utils::mocks::MatrixMockServer};
 
@@ -435,3 +435,69 @@ fn temporary_path() -> PathBuf {
         .as_nanos();
     PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("macltok-account-{nonce}"))
 }
+
+#[tokio::test]
+async fn an_oauth_login_survives_the_page_going_away_for_its_redirect() {
+    let server = MatrixMockServer::new().await;
+    server
+        .mock_versions()
+        .with_simplified_sliding_sync()
+        .ok()
+        .mount()
+        .await;
+    server.mock_who_am_i().ok().mount().await;
+    let oauth = server.oauth();
+    oauth.mock_server_metadata().ok().mount().await;
+    oauth.mock_registration().ok().mount().await;
+    oauth.mock_token().ok().mount().await;
+    let root = temporary_path();
+    let accounts = Accounts::sqlite(&root, "test store passphrase");
+    let redirect_uri = "http://127.0.0.1/callback";
+
+    let (account, pending) = accounts
+        .begin_oauth(
+            OAuthLogin::new(
+                server.uri(),
+                redirect_uri,
+                "http://127.0.0.1/",
+                OAuthApplicationType::Native,
+            )
+            .allow_insecure_loopback(true),
+        )
+        .await
+        .unwrap();
+    let state = url::Url::parse(pending.authorization_url())
+        .unwrap()
+        .query_pairs()
+        .find(|(key, _)| key == "state")
+        .map(|(_, state)| state.into_owned())
+        .unwrap();
+    drop(pending);
+
+    assert!(matches!(
+        account.restore().await.unwrap(),
+        Some(Restore::OAuthPending)
+    ));
+    assert_eq!(
+        accounts.list().await.unwrap().len(),
+        1,
+        "the redirect's store was swept"
+    );
+    assert!(matches!(
+        accounts
+            .finish_oauth(&format!("{redirect_uri}?code=42&state=someone-else"))
+            .await,
+        Err(AccountError::NoPendingOAuth)
+    ));
+
+    let (finished, session) = accounts
+        .finish_oauth(&format!("{redirect_uri}?code=42&state={state}"))
+        .await
+        .unwrap();
+    assert_eq!(finished.id(), account.id());
+    drop(session);
+    assert!(matches!(
+        account.restore().await.unwrap(),
+        Some(Restore::Resumed(_))
+    ));
+}

macltok

Commit d8d8a9fe770a

Commit `d8d8a9fe770a`