diff --git a/packages/oneclient_app/src/bridge/runtime.rs b/packages/oneclient_app/src/bridge/runtime.rs index 7a8b622c..7f3f70b4 100644 --- a/packages/oneclient_app/src/bridge/runtime.rs +++ b/packages/oneclient_app/src/bridge/runtime.rs @@ -540,14 +540,11 @@ impl CoreBridgeRuntime { let state = LauncherState::get()?; tokio::spawn(async move { let notifier = state.services.notifier.clone(); - let account = match oneclient_core::auth::get_default_account().await { - Ok(Some(account)) => Some(account), - Ok(None) => oneclient_core::auth::list_accounts() - .await - .ok() - .and_then(|accounts| accounts.into_iter().next()), + // Renews a lapsed token before launching + let account = match oneclient_core::auth::default_account_for_launch().await { + Ok(account) => account, Err(err) => { - notifier.game_failed(cluster_id, err.to_string()); + notifier.game_failed(cluster_id, format!("{err:#}")); return; } }; diff --git a/packages/oneclient_app/src/hooks/queries/auth.rs b/packages/oneclient_app/src/hooks/queries/auth.rs index 9392940f..d777507c 100644 --- a/packages/oneclient_app/src/hooks/queries/auth.rs +++ b/packages/oneclient_app/src/hooks/queries/auth.rs @@ -295,7 +295,7 @@ pub struct RefreshAccountKeys { } impl MutationCapability for RefreshAccountMutation { - type Ok = Option; + type Ok = MinecraftAccount; type Err = LauncherError; type Keys = RefreshAccountKeys; diff --git a/packages/oneclient_core/src/auth/data.rs b/packages/oneclient_core/src/auth/data.rs index e0eecabb..63beb128 100644 --- a/packages/oneclient_core/src/auth/data.rs +++ b/packages/oneclient_core/src/auth/data.rs @@ -32,6 +32,10 @@ impl MinecraftAccount { pub fn is_offline(&self) -> bool { self.kind == AccountKind::Offline } + + pub fn is_expired(&self) -> bool { + self.expires <= Utc::now() + chrono::TimeDelta::seconds(60) + } } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] diff --git a/packages/oneclient_core/src/auth/mod.rs b/packages/oneclient_core/src/auth/mod.rs index 2be14f2c..f7e50ee7 100644 --- a/packages/oneclient_core/src/auth/mod.rs +++ b/packages/oneclient_core/src/auth/mod.rs @@ -12,6 +12,10 @@ pub use data::{ }; pub use msa::PendingBrowserLogin; +use std::collections::HashMap; +use std::sync::{Arc, LazyLock, Mutex as StdMutex}; + +use tokio::sync::Mutex; use uuid::Uuid; use crate::LauncherResult; @@ -112,7 +116,7 @@ pub async fn get_account(id: Uuid) -> LauncherResult> { pub async fn get_default_account() -> LauncherResult> { let state = LauncherState::get()?; let mut auth = state.auth.lock().await; - auth.default_account(&state.services).await + auth.default_account().await } #[tracing::instrument(level = "debug", skip_all, fields(?id))] @@ -130,24 +134,100 @@ pub async fn remove_account(id: Uuid) -> LauncherResult<()> { Ok(()) } +/// Serialises token renewal per account. +/// +/// Microsoft rotates the refresh token on every use, so two concurrent renewals +/// of one account would race: the loser spends an already-consumed refresh token +/// and the account gets signed out. Clusters can launch in parallel, so this is +/// reachable — the guard makes the second caller wait and reuse the first +/// caller's result instead of starting its own chain. +static REFRESH_GUARDS: LazyLock>>>> = + LazyLock::new(|| StdMutex::new(HashMap::new())); + +fn refresh_guard(id: Uuid) -> Arc> { + Arc::clone( + REFRESH_GUARDS + .lock() + .expect("refresh guard registry poisoned") + .entry(id) + .or_default(), + ) +} + +/// Clones an account out of the store, returning an owned value so no caller +/// keeps the credentials-store lock alive past this call. +async fn account_snapshot( + state: &Arc, + id: Uuid, +) -> LauncherResult { + let auth = state.auth.lock().await; + auth.get_account(id) + .cloned() + .ok_or_else(|| AuthError::AccountNotFound(id).into()) +} + +/// Renews `id`'s access token if it has lapsed, returning a usable account. +/// +/// The credentials-store lock is deliberately not held across the Microsoft +/// handshake, only around the reads and the final write. Holding it across +/// those six sequential round trips is what used to stall every other account +/// read behind a multi-second network call. +#[tracing::instrument(level = "debug", skip(state), fields(%id))] +async fn renew_token( + state: &Arc, + id: Uuid, + force: bool, +) -> LauncherResult { + let existing = account_snapshot(state, id).await?; + if !existing.is_microsoft() || (!force && !existing.is_expired()) { + return Ok(existing); + } + + let guard = refresh_guard(id); + let _serialised = guard.lock().await; + + let existing = account_snapshot(state, id).await?; + if !existing.is_microsoft() || (!force && !existing.is_expired()) { + return Ok(existing); + } + + tracing::info!(username = %existing.username, "renewing Microsoft access token"); + match msa::refresh_microsoft_account(state.services.requester.http(), &existing).await { + Ok(refreshed) => { + state + .auth + .lock() + .await + .commit_refreshed_account(refreshed.clone()) + .await?; + Ok(refreshed) + } + Err(err) => { + let err = crate::LauncherError::from(AuthError::from(err)); + if store::is_transient_auth_error(&err) { + tracing::warn!("keeping existing token after transient renewal failure: {err}"); + Ok(existing) + } else { + Err(err) + } + } + } +} + #[tracing::instrument(level = "debug", skip_all, fields(%id))] -pub async fn refresh_account(id: Uuid) -> LauncherResult> { +pub async fn refresh_account(id: Uuid) -> LauncherResult { let state = LauncherState::get()?; - let mut auth = state.auth.lock().await; - auth.refresh_microsoft_account(id, &state.services).await + renew_token(&state, id, true).await } #[tracing::instrument(level = "debug", skip_all)] pub async fn refresh_all_accounts() -> LauncherResult> { let state = LauncherState::get()?; - let mut auth = state.auth.lock().await; - let ids: Vec = auth.users.keys().copied().collect(); + let ids: Vec = state.auth.lock().await.users.keys().copied().collect(); let mut refreshed = Vec::with_capacity(ids.len()); for id in ids { - if let Some(account) = auth.refresh_microsoft_account(id, &state.services).await? { - refreshed.push(account); - } + refreshed.push(renew_token(&state, id, true).await?); } Ok(refreshed) @@ -156,8 +236,22 @@ pub async fn refresh_all_accounts() -> LauncherResult> { #[tracing::instrument(level = "debug", skip_all, fields(%id))] pub async fn account_for_launch(id: Uuid) -> LauncherResult { let state = LauncherState::get()?; - let mut auth = state.auth.lock().await; - auth.account_for_launch(id, &state.services).await + let account = renew_token(&state, id, false).await?; + + if account.is_offline() && !state.auth.lock().await.has_microsoft_account() { + return Err(AuthError::OfflineRequiresMicrosoft.into()); + } + + Ok(account) +} + +#[tracing::instrument(level = "debug", skip_all)] +pub async fn default_account_for_launch() -> LauncherResult> { + let state = LauncherState::get()?; + let Some(id) = state.auth.lock().await.resolve_default_id().await? else { + return Ok(None); + }; + Ok(Some(account_for_launch(id).await?)) } pub async fn has_microsoft_account() -> LauncherResult { diff --git a/packages/oneclient_core/src/auth/store.rs b/packages/oneclient_core/src/auth/store.rs index 4169cec0..91aa885c 100644 --- a/packages/oneclient_core/src/auth/store.rs +++ b/packages/oneclient_core/src/auth/store.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; -use chrono::Utc; use uuid::Uuid; use super::error::AuthError; @@ -114,30 +113,15 @@ impl CredentialsStore { Ok(account) } - #[tracing::instrument(level = "debug", skip(self, services), fields(%id))] - pub async fn refresh_microsoft_account( + #[tracing::instrument(level = "debug", skip_all, fields(%account.id))] + pub async fn commit_refreshed_account( &mut self, - id: Uuid, - services: &LauncherServices, - ) -> LauncherResult> { - let Some(existing) = self.users.get(&id).cloned() else { - return Ok(None); - }; - - if existing.kind != AccountKind::Microsoft { - return Ok(Some(existing)); - } - - let client = services.requester.http(); - let account = super::msa::refresh_microsoft_account(client, &existing) - .await - .map_err(AuthError::from)?; - - self.users.insert(id, account.clone()); + account: MinecraftAccount, + ) -> LauncherResult<()> { + self.users.insert(account.id, account); self.save().await?; - tracing::debug!("refreshed Microsoft account"); - - Ok(Some(account)) + tracing::debug!("stored refreshed Microsoft account"); + Ok(()) } #[tracing::instrument(level = "debug", skip(self), fields(%id))] @@ -166,10 +150,7 @@ impl CredentialsStore { } #[tracing::instrument(level = "debug", skip_all)] - pub async fn default_account( - &mut self, - services: &LauncherServices, - ) -> LauncherResult> { + pub async fn resolve_default_id(&mut self) -> LauncherResult> { let id = self .default_user .or_else(|| self.users.keys().copied().next()); @@ -178,69 +159,38 @@ impl CredentialsStore { return Ok(None); }; - let Some(account) = self.users.get(&id).cloned() else { + if !self.users.contains_key(&id) { return Ok(None); - }; + } if self.default_user != Some(id) { self.default_user = Some(id); self.save().await?; } - if account.kind == AccountKind::Offline { - if !self.has_microsoft_account() { - return Err(AuthError::OfflineRequiresMicrosoft.into()); - } - return Ok(Some(account)); - } - - if account.expires >= Utc::now() { - return Ok(Some(account)); - } - - let old = account.clone(); - match self.refresh_microsoft_account(id, services).await { - Ok(Some(updated)) => Ok(Some(updated)), - Ok(None) => Ok(None), - Err(err) => { - if is_transient_auth_error(&err) { - Ok(Some(old)) - } else { - Err(err) - } - } - } + Ok(Some(id)) } - #[tracing::instrument(level = "debug", skip(self, services), fields(%id))] - pub async fn account_for_launch( - &mut self, - id: Uuid, - services: &LauncherServices, - ) -> LauncherResult { - if !self.users.contains_key(&id) { - return Err(AuthError::AccountNotFound(id).into()); - } + /// The current account as stored. Never touches the network: callers that + /// need a usable token go through [`crate::auth::account_for_launch`]. + #[tracing::instrument(level = "debug", skip_all)] + pub async fn default_account(&mut self) -> LauncherResult> { + let Some(id) = self.resolve_default_id().await? else { + return Ok(None); + }; + let Some(account) = self.users.get(&id).cloned() else { + return Ok(None); + }; - let previous_default = self.default_user; - self.default_user = Some(id); - - let account = self - .default_account(services) - .await? - .ok_or(AuthError::AccountNotFound(id))?; - - self.default_user = previous_default; - - if account.kind == AccountKind::Offline && !self.has_microsoft_account() { + if account.is_offline() && !self.has_microsoft_account() { return Err(AuthError::OfflineRequiresMicrosoft.into()); } - Ok(account) + Ok(Some(account)) } } -fn is_transient_auth_error(err: &crate::LauncherError) -> bool { +pub(super) fn is_transient_auth_error(err: &crate::LauncherError) -> bool { match err { crate::LauncherError::AuthError(super::error::AuthError::Minecraft( super::error::MinecraftAuthError::RequestError { source, .. }, diff --git a/packages/oneclient_core/src/minecraft/profile.rs b/packages/oneclient_core/src/minecraft/profile.rs index 4e9149c2..777567bf 100644 --- a/packages/oneclient_core/src/minecraft/profile.rs +++ b/packages/oneclient_core/src/minecraft/profile.rs @@ -1,4 +1,5 @@ use base64::Engine; +use chrono::{DateTime, TimeDelta, Utc}; use reqwest::Method; use serde::{Deserialize, Serialize}; @@ -243,31 +244,75 @@ pub async fn fetch_logged_in_profile( Ok(profile) } +/// How long a cached profile is served without revalidating. +/// +/// The alternative is a Mojang round trip before the first avatar can paint on +/// every cold start. Skins change rarely, so this trades a bounded amount of +/// staleness for startup that doesn't wait on the network. +const PROFILE_CACHE_TTL_HOURS: i64 = 6; + +#[derive(Debug, Serialize, Deserialize)] +struct CachedProfile { + fetched_at: DateTime, + view: PlayerProfileView, +} + +impl CachedProfile { + fn is_fresh(&self) -> bool { + Utc::now() - self.fetched_at < TimeDelta::hours(PROFILE_CACHE_TTL_HOURS) + } +} + +async fn fetch_profile_view( + client: &RequestClient, + uuid: &str, + access_token: Option<&str>, +) -> LauncherResult { + if let Some(token) = access_token.filter(|t| !t.is_empty()) { + let full = fetch_logged_in_profile(client, token).await?; + return Ok(PlayerProfileView::from(full)); + } + let summary = fetch_player_profile(client, uuid).await?; + Ok(PlayerProfileView::from(summary)) +} + +/// Resolves a player's public profile preferring on-disk cache #[tracing::instrument(level = "debug", skip(client, access_token))] pub async fn fetch_player_profile_view( client: &RequestClient, uuid: &str, access_token: Option<&str>, ) -> LauncherResult { - let fetched = async { - if let Some(token) = access_token.filter(|t| !t.is_empty()) { - let full = fetch_logged_in_profile(client, token).await?; - return Ok(PlayerProfileView::from(full)); - } - let summary = fetch_player_profile(client, uuid).await?; - Ok(PlayerProfileView::from(summary)) - } - .await; + let authenticated = access_token.is_some_and(|t| !t.is_empty()); - match fetched { + if !authenticated && let Some(cached) = read_cached_profile(uuid).await { + if cached.is_fresh() { + return Ok(cached.view); + } + + return match fetch_profile_view(client, uuid, access_token).await { + Ok(view) => { + write_cached_profile(uuid, &view).await; + Ok(view) + } + Err(err) => { + tracing::warn!("serving stale cached profile for {uuid}: {err}"); + Ok(cached.view) + } + }; + } + + match fetch_profile_view(client, uuid, access_token).await { Ok(view) => { - write_cached_profile(uuid, &view).await; + if !authenticated { + write_cached_profile(uuid, &view).await; + } Ok(view) } Err(err) => match read_cached_profile(uuid).await { Some(cached) => { tracing::warn!("using cached player profile for {uuid}: {err}"); - Ok(cached) + Ok(cached.view) } None => Err(err), }, @@ -283,7 +328,11 @@ async fn write_cached_profile(uuid: &str, view: &PlayerProfileView) { let Some(path) = profile_cache_path(uuid) else { return; }; - let Ok(bytes) = serde_json::to_vec(view) else { + let entry = CachedProfile { + fetched_at: Utc::now(), + view: view.clone(), + }; + let Ok(bytes) = serde_json::to_vec(&entry) else { return; }; if let Some(parent) = path.parent() { @@ -294,8 +343,89 @@ async fn write_cached_profile(uuid: &str, view: &PlayerProfileView) { } } -async fn read_cached_profile(uuid: &str) -> Option { +async fn read_cached_profile(uuid: &str) -> Option { let path = profile_cache_path(uuid)?; let bytes = polyio::read(&path).await.ok()?; - serde_json::from_slice(&bytes).ok() + + if let Ok(entry) = serde_json::from_slice::(&bytes) { + return Some(entry); + } + + let view = serde_json::from_slice::(&bytes).ok()?; + Some(CachedProfile { + fetched_at: DateTime::UNIX_EPOCH, + view, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_launcher_dir() { + let dir = std::env::temp_dir().join(format!("oneclient-profile-test-{}", uuid::Uuid::new_v4())); + crate::paths::set_launcher_dir(dir); + } + + fn sample_view(username: &str) -> PlayerProfileView { + PlayerProfileView { + uuid: "069a79f4-44e9-4726-a5be-fca90e38aaf5".into(), + username: username.into(), + is_slim: true, + skin_url: Some("https://textures.minecraft.net/texture/abc".into()), + cape_url: None, + skins: Vec::new(), + capes: Vec::new(), + } + } + + #[tokio::test] + async fn fresh_cache_entry_is_served_without_network() { + temp_launcher_dir(); + let uuid = uuid::Uuid::new_v4().to_string(); + let view = sample_view("Cached"); + write_cached_profile(&uuid, &view).await; + + let client = RequestClient::new().unwrap(); + let resolved = fetch_player_profile_view(&client, &uuid, None) + .await + .expect("fresh cache entry should resolve offline"); + + assert_eq!(resolved, view); + } + + #[tokio::test] + async fn entry_past_ttl_is_not_fresh() { + let stale = CachedProfile { + fetched_at: Utc::now() - TimeDelta::hours(PROFILE_CACHE_TTL_HOURS + 1), + view: sample_view("Stale"), + }; + assert!(!stale.is_fresh()); + + let recent = CachedProfile { + fetched_at: Utc::now() - TimeDelta::hours(PROFILE_CACHE_TTL_HOURS - 1), + view: sample_view("Recent"), + }; + assert!(recent.is_fresh()); + } + + #[tokio::test] + async fn legacy_cache_entry_loads_and_revalidates() { + temp_launcher_dir(); + let uuid = uuid::Uuid::new_v4().to_string(); + let view = sample_view("Legacy"); + + let path = profile_cache_path(&uuid).unwrap(); + polyio::create_dir_all(path.parent().unwrap()).await.unwrap(); + polyio::write(&path, &serde_json::to_vec(&view).unwrap()) + .await + .unwrap(); + + let cached = read_cached_profile(&uuid) + .await + .expect("legacy entry should still parse"); + + assert_eq!(cached.view, view); + assert!(!cached.is_fresh(), "legacy entry should be due for revalidation"); + } } diff --git a/packages/oneclient_core/tests/auth_offline.rs b/packages/oneclient_core/tests/auth_offline.rs index d1b5812e..844e4ee0 100644 --- a/packages/oneclient_core/tests/auth_offline.rs +++ b/packages/oneclient_core/tests/auth_offline.rs @@ -1,8 +1,16 @@ +use chrono::{TimeDelta, Utc}; use oneclient_core::auth::{ offline_account, offline_uuid, AccountKind, CredentialsStore, }; use uuid::Uuid; +// fake launcher dir because yes +fn isolate_launcher_dir() { + oneclient_core::paths::set_launcher_dir( + std::env::temp_dir().join(format!("oneclient-auth-test-{}", Uuid::new_v4())), + ); +} + fn fake_microsoft_account(username: &str) -> oneclient_core::auth::MinecraftAccount { let mut account = offline_account(username.to_string()); account.kind = AccountKind::Microsoft; @@ -14,7 +22,7 @@ fn fake_microsoft_account(username: &str) -> oneclient_core::auth::MinecraftAcco #[test] fn offline_uuid_matches_vanilla_algorithm() { let id = offline_uuid("Notch"); - + assert_eq!( id, Uuid::parse_str("b50ad385-829d-3141-a216-7e7d7539ba7f").unwrap() @@ -24,6 +32,8 @@ fn offline_uuid_matches_vanilla_algorithm() { #[tokio::test] async fn offline_account_blocked_without_microsoft() { + isolate_launcher_dir(); + let mut store = CredentialsStore::default(); let err = store.add_offline_account("Steve".into()).unwrap_err(); @@ -32,6 +42,8 @@ async fn offline_account_blocked_without_microsoft() { #[tokio::test] async fn offline_account_allowed_when_microsoft_exists() { + isolate_launcher_dir(); + let mut store = CredentialsStore::default(); let msa = fake_microsoft_account("MsaUser"); store.users.insert(msa.id, msa); @@ -43,6 +55,8 @@ async fn offline_account_allowed_when_microsoft_exists() { #[tokio::test] async fn default_offline_fails_when_last_microsoft_removed() { + isolate_launcher_dir(); + let mut store = CredentialsStore::default(); let msa = fake_microsoft_account("MsaUser"); let msa_id = msa.id; @@ -52,7 +66,45 @@ async fn default_offline_fails_when_last_microsoft_removed() { store.default_user = Some(offline.id); store.users.remove(&msa_id); - let services = oneclient_core::dev::ephemeral_services().await.unwrap(); - let err = store.default_account(&services).await.unwrap_err(); + let err = store.default_account().await.unwrap_err(); assert!(err.to_string().contains("Microsoft")); } + +#[test] +fn token_expiring_imminently_counts_as_expired() { + let mut account = fake_microsoft_account("MsaUser"); + + account.expires = Utc::now() + TimeDelta::hours(1); + assert!(!account.is_expired(), "a token with an hour left is usable"); + + account.expires = Utc::now() + TimeDelta::seconds(5); + assert!( + account.is_expired(), + "a token about to lapse must be renewed before launching" + ); + + account.expires = Utc::now() - TimeDelta::hours(1); + assert!(account.is_expired()); +} + +#[tokio::test] +async fn default_account_returns_expired_token_without_refreshing() { + isolate_launcher_dir(); + + let mut store = CredentialsStore::default(); + let mut msa = fake_microsoft_account("MsaUser"); + msa.expires = Utc::now() - TimeDelta::hours(5); + let msa_id = msa.id; + store.users.insert(msa_id, msa); + + store.default_user = Some(msa_id); + + let account = store + .default_account() + .await + .expect("an expired token must not fail the read path") + .expect("the only account should be the default"); + + assert_eq!(account.id, msa_id); + assert!(account.is_expired()); +}