diff --git a/Cargo.lock b/Cargo.lock index 23fa69d2..5ebae267 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4570,6 +4570,7 @@ dependencies = [ "sha2 0.10.9", "thiserror 2.0.18", "tokio", + "tokio-util", "tracing", "url", "uuid 1.23.1", diff --git a/packages/oneclient_app/src/components/microsoft_login.rs b/packages/oneclient_app/src/components/microsoft_login.rs index f27add27..99cc1e45 100644 --- a/packages/oneclient_app/src/components/microsoft_login.rs +++ b/packages/oneclient_app/src/components/microsoft_login.rs @@ -140,10 +140,19 @@ where MutationStateData::Settled { res: Err(err), .. } | MutationStateData::Loading { res: Some(Err(err)), - } => Some(AuthErrorInfo { - message: err.to_string(), - guidance: err.auth_guidance(), - }), + } => { + // A cancellation is the user's own decision arriving back as an + // error. Reporting it would put a red line under a button they + // pressed on purpose, and it would still be there when they start + // the next sign-in. + if err.is_login_cancelled() { + return None; + } + Some(AuthErrorInfo { + message: err.to_string(), + guidance: err.auth_guidance(), + }) + } _ => None, } } diff --git a/packages/oneclient_auth/Cargo.toml b/packages/oneclient_auth/Cargo.toml index 0b3973ae..8d404d70 100644 --- a/packages/oneclient_auth/Cargo.toml +++ b/packages/oneclient_auth/Cargo.toml @@ -21,6 +21,7 @@ polyio.workspace = true reqwest.workspace = true tokio.workspace = true +tokio-util.workspace = true base64.workspace = true chrono.workspace = true diff --git a/packages/oneclient_auth/src/error.rs b/packages/oneclient_auth/src/error.rs index 376f00f1..7639de0e 100644 --- a/packages/oneclient_auth/src/error.rs +++ b/packages/oneclient_auth/src/error.rs @@ -106,6 +106,15 @@ pub enum AuthError { #[error(transparent)] Request(#[from] RequestError), + /// The user backed out of a Microsoft sign-in that was still running. + /// + /// An outcome rather than a failure: it exists so the flow has something to + /// return when it is torn down mid-handshake, and so callers can tell that + /// apart from a sign-in that actually went wrong. Nothing was written by the + /// time this is produced. + #[error("sign-in was cancelled")] + LoginCancelled, + #[error("offline mode requires at least one Microsoft account to be signed in")] OfflineRequiresMicrosoft, diff --git a/packages/oneclient_auth/src/service.rs b/packages/oneclient_auth/src/service.rs index cb2287e3..ca6aacd3 100644 --- a/packages/oneclient_auth/src/service.rs +++ b/packages/oneclient_auth/src/service.rs @@ -4,6 +4,7 @@ use std::sync::{Arc, Mutex as StdMutex}; use oneclient_events::EventBus; use oneclient_net::RequestClient; use tokio::sync::Mutex; +use tokio_util::sync::CancellationToken; use uuid::Uuid; use crate::data::{MicrosoftLoginSession, MinecraftAccount}; @@ -21,6 +22,21 @@ use crate::store::{self, CredentialsStore}; pub const MICROSOFT_LOGIN_PROGRESS: Uuid = Uuid::from_u128(0x4D53_4141_5554_4800_0000_0000_0000_0001); +/// A Microsoft login that has been started but has not finished. +/// +/// The two halves have different owners once the flow is under way, which is +/// the whole reason this is a struct: `finish_microsoft_login` takes the +/// listener and runs with it, while the token stays here so a cancel arriving +/// from the UI has something to flip. +struct PendingLogin { + /// The loopback listener, until the flow claims it. + /// + /// `None` once someone owns it, or once a cancel freed the port before any + /// flow got that far. + browser: Option, + cancel: CancellationToken, +} + /// Owns everything the authentication flows need. /// /// Constructed once by the composition layer and passed down, so nothing here @@ -29,7 +45,7 @@ pub const MICROSOFT_LOGIN_PROGRESS: Uuid = pub struct AuthService { store: Mutex, /// Browser logins waiting on their redirect, keyed by CSRF state token. - pending_logins: Mutex>, + pending_logins: Mutex>, /// Serialises token renewal per account. /// /// Microsoft rotates the refresh token on every use, so two concurrent @@ -74,14 +90,50 @@ impl AuthService { let (browser, pending) = msa::begin_browser_login().await?; let device = msa::begin_device_login(client).await?; - self.pending_logins - .lock() - .await - .insert(browser.state.clone(), pending); + let mut logins = self.pending_logins.lock().await; + // A login cancelled before its flow ever started has nobody left to remove + // it, so the next login sweeps it up. + logins.retain(|_, login| !login.cancel.is_cancelled()); + logins.insert( + browser.state.clone(), + PendingLogin { + browser: Some(pending), + cancel: CancellationToken::new(), + }, + ); Ok(MicrosoftLoginSession { browser, device }) } + /// Takes the loopback listener for `state_token`'s flow, plus the token that + /// cancels it. Only one caller can ever claim a login. + async fn claim_pending_login( + &self, + state_token: &str, + ) -> AuthResult<(PendingBrowserLogin, CancellationToken)> { + let mut logins = self.pending_logins.lock().await; + + // Cancelled between `begin` and here: the listener is already gone, and + // there is nothing to run. + if logins + .get(state_token) + .is_some_and(|login| login.cancel.is_cancelled()) + { + logins.remove(state_token); + return Err(AuthError::LoginCancelled); + } + + let login = logins.get_mut(state_token).ok_or(AuthError::Minecraft( + MinecraftAuthError::BrowserLoginNotFound, + ))?; + let cancel = login.cancel.clone(); + let browser = login.browser.take().ok_or(AuthError::Minecraft( + MinecraftAuthError::BrowserLoginNotFound, + ))?; + + Ok((browser, cancel)) + } + #[tracing::instrument(skip_all)] pub async fn finish_microsoft_login( &self, @@ -89,32 +141,47 @@ impl AuthService { ) -> AuthResult { tracing::info!("finishing Microsoft login"); - let pending = self - .pending_logins - .lock() - .await - .remove(&session.browser.state) - .ok_or(AuthError::Minecraft( - MinecraftAuthError::BrowserLoginNotFound, - ))?; + let (pending, cancel) = self.claim_pending_login(&session.browser.state).await?; let events = self.events.clone(); - let result = msa::finish_dual_login( + let flow = msa::finish_dual_login( self.net.http(), pending, &session.device, |label, current, total| { events.progress(MICROSOFT_LOGIN_PROGRESS, label, current, total); }, - ) - .await - .map_err(AuthError::from); + ); + + // Racing the token is what makes a cancel prompt: losing the race drops + // `flow`, and with it the device-code poll, the loopback listener and + // whichever request was mid-flight. There is nothing left to check + // between polls, and nothing keeps running behind the closed dialog. + let result = tokio::select! { + biased; + () = cancel.cancelled() => Err(AuthError::LoginCancelled), + res = flow => res.map_err(AuthError::from), + }; + + // Whoever runs the flow owns the cleanup; a cancel only flips the token, + // so that an in-flight flow is the one to tear its own state down. + self.pending_logins.lock().await.remove(&session.browser.state); // Drives the card to 100%, which is how a front-end knows the flow is - // over and can clear whatever it was rendering. + // over and can clear whatever it was rendering. A cancelled flow needs + // this just as much as a successful one, or the modal's progress line + // would outlive the sign-in it belongs to. self.events .progress(MICROSOFT_LOGIN_PROGRESS, "Signed in", 1, 1); + // Re-read rather than trusting `result`: the last handshake step can + // complete in the same breath as a cancel, and an account the user has + // already walked away from must not reach the store or the toast. + if cancel.is_cancelled() { + tracing::info!("Microsoft login cancelled by the user"); + return Err(AuthError::LoginCancelled); + } + if let Err(err) = &result { let mut chain = String::new(); let mut source = std::error::Error::source(err); @@ -134,9 +201,22 @@ impl AuthService { .await } + /// Stops an in-flight Microsoft login and leaves nothing of it behind. + /// + /// The entry is deliberately left in the map: `finish_microsoft_login` may + /// not have claimed it yet, and it needs to find the cancelled token rather + /// than a hole it would report as "this sign-in is no longer active". #[tracing::instrument(level = "debug", skip_all)] pub async fn cancel_microsoft_login(&self, state_token: &str) { - self.pending_logins.lock().await.remove(state_token); + let mut logins = self.pending_logins.lock().await; + let Some(login) = logins.get_mut(state_token) else { + return; + }; + + login.cancel.cancel(); + // Frees the port now if no flow ever took the listener; once one has, the + // flow drops it as it unwinds. + login.browser = None; } // --- account management ------------------------------------------------ @@ -281,3 +361,116 @@ impl AuthService { Ok(Some(self.account_for_launch(id).await?)) } } + +#[cfg(test)] +mod tests { + use oneclient_net::NetConfig; + + use super::*; + use crate::data::DeviceCodeLogin; + + fn service(events: EventBus) -> AuthService { + let net = RequestClient::new(NetConfig::default()).expect("net client"); + AuthService::with_store(CredentialsStore::default(), net, events) + } + + /// Registers a login the way `begin_microsoft_login` does, minus the device + /// code request: the browser half is the one that binds a port, and it is + /// the only half that needs no network to set up. + async fn register_login(service: &AuthService) -> MicrosoftLoginSession { + let (browser, pending) = msa::begin_browser_login().await.expect("loopback bind"); + + service.pending_logins.lock().await.insert( + browser.state.clone(), + PendingLogin { + browser: Some(pending), + cancel: CancellationToken::new(), + }, + ); + + MicrosoftLoginSession { + browser, + device: DeviceCodeLogin { + user_code: "ABCD-EFGH".to_string(), + device_code: "device-code".to_string(), + verification_uri: "https://microsoft.com/link".to_string(), + expires_in: 900, + interval: 5, + message: String::new(), + }, + } + } + + #[tokio::test] + async fn a_cancel_that_beats_the_flow_is_still_a_cancel() { + // The flow starts on its own task, so a quick cancel can land before it + // claims the login. Answering that with "this sign-in is no longer + // active" would show the user an error for working the dialog correctly. + let (events, _rx) = EventBus::channel(); + let service = service(events); + let session = register_login(&service).await; + + service.cancel_microsoft_login(session.dedupe_key()).await; + let err = service.finish_microsoft_login(session).await.unwrap_err(); + + assert!(matches!(err, AuthError::LoginCancelled), "{err}"); + assert!(service.pending_logins.lock().await.is_empty()); + assert!(service.store.lock().await.list_accounts().is_empty()); + } + + #[tokio::test] + async fn a_cancel_frees_the_loopback_port_it_was_holding() { + let (events, _rx) = EventBus::channel(); + let service = service(events); + let session = register_login(&service).await; + + service.cancel_microsoft_login(session.dedupe_key()).await; + + let logins = service.pending_logins.lock().await; + let login = logins + .get(session.dedupe_key()) + .expect("the entry is what a flow starting late reads the cancel from"); + assert!(login.cancel.is_cancelled()); + assert!( + login.browser.is_none(), + "the listener should be dropped rather than left bound" + ); + } + + #[tokio::test] + async fn a_cancel_reaches_a_flow_that_has_already_claimed_the_login() { + // Once claimed, the entry is the only channel a cancel has to the flow + // that is running, so it has to survive the claim. + let (events, _rx) = EventBus::channel(); + let service = service(events); + let session = register_login(&service).await; + + let (_listener, cancel) = service + .claim_pending_login(session.dedupe_key()) + .await + .expect("claim"); + service.cancel_microsoft_login(session.dedupe_key()).await; + + assert!(cancel.is_cancelled()); + } + + #[tokio::test] + async fn a_login_can_only_be_claimed_once() { + let (events, _rx) = EventBus::channel(); + let service = service(events); + let session = register_login(&service).await; + + service + .claim_pending_login(session.dedupe_key()) + .await + .expect("claim"); + let claimed_again = service.claim_pending_login(session.dedupe_key()).await; + + assert!(matches!( + claimed_again, + Err(AuthError::Minecraft( + MinecraftAuthError::BrowserLoginNotFound + )) + )); + } +} diff --git a/packages/oneclient_core/src/error.rs b/packages/oneclient_core/src/error.rs index cdc19a26..b6598e59 100644 --- a/packages/oneclient_core/src/error.rs +++ b/packages/oneclient_core/src/error.rs @@ -83,6 +83,19 @@ impl LauncherError { } } + /// Whether this is the user backing out of their own sign-in. + /// + /// Worth telling apart because it is not something to report: by the time it + /// lands the dialog is already gone, and putting a red line under the button + /// would be complaining about the button the user just pressed. + #[must_use] + pub fn is_login_cancelled(&self) -> bool { + matches!( + self, + LauncherError::AuthError(oneclient_auth::AuthError::LoginCancelled) + ) + } + /// Whether this failure reads as "the install is missing pieces", and so is /// worth repairing rather than just reporting. ///