fix(auth): actually cancel an in-flight Microsoft sign-in (#696)

Cargo.lock

@@ -4570,6 +4570,7 @@ dependencies = [
  "sha2 0.10.9",
  "thiserror 2.0.18",
  "tokio",
+ "tokio-util",
  "tracing",
  "url",
  "uuid 1.23.1",

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 {
+        } => {
+            // 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,
     }
 }

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

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,
 

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<PendingBrowserLogin>,
+	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<CredentialsStore>,
 	/// Browser logins waiting on their redirect, keyed by CSRF state token.
-	pending_logins: Mutex<HashMap<String, PendingBrowserLogin>>,
+	pending_logins: Mutex<HashMap<String, PendingLogin>>,
 	/// 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<MinecraftAccount> {
 		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
+			))
+		));
+	}
+}

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.
     ///