fix(data): serialise token refreshes so overlapping 401s can't force a logout
Test / test (pull_request) Successful in 15m33s
Test / test (pull_request) Successful in 15m33s
Refresh tokens are single-use since the rotation change (PR #136). Two in-flight requests hitting 401 together (replay upload racing a manual sync) both called /api/auth/refresh; the second presented an already- consumed token, got rejected, and surfaced as a spurious 'session expired' re-login prompt. refresh_token() now takes the stale access token that earned the 401 and runs behind a tokio::sync::Mutex; a loser of the lock race that finds the stored token already rotated returns Ok and retries with it instead of spending the new refresh token. Untested caveat: exercising the race in a unit test needs an in-memory auth_tokens double (the real keyring is unavailable on headless Linux runners) — noted for a future test-support addition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -77,6 +77,11 @@ pub struct SolitaireServerClient {
|
|||||||
username: String,
|
username: String,
|
||||||
/// Shared `reqwest` client (keeps connection pools alive across calls).
|
/// Shared `reqwest` client (keeps connection pools alive across calls).
|
||||||
client: reqwest::Client,
|
client: reqwest::Client,
|
||||||
|
/// Serialises token refreshes. The server rotates refresh tokens and each
|
||||||
|
/// is single-use, so two overlapping 401-retries must not both spend one:
|
||||||
|
/// the loser's (already-consumed) token would be rejected and surface as
|
||||||
|
/// a spurious "session expired" to the player. See [`Self::refresh_token`].
|
||||||
|
refresh_lock: tokio::sync::Mutex<()>,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(not(target_arch = "wasm32"))]
|
#[cfg(not(target_arch = "wasm32"))]
|
||||||
@@ -90,6 +95,7 @@ impl SolitaireServerClient {
|
|||||||
base_url: base_url.into().trim_end_matches('/').to_owned(),
|
base_url: base_url.into().trim_end_matches('/').to_owned(),
|
||||||
username: username.into(),
|
username: username.into(),
|
||||||
client: reqwest::Client::new(),
|
client: reqwest::Client::new(),
|
||||||
|
refresh_lock: tokio::sync::Mutex::new(()),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -172,7 +178,27 @@ impl SolitaireServerClient {
|
|||||||
/// The server rotates refresh tokens on each call: the response includes a
|
/// The server rotates refresh tokens on each call: the response includes a
|
||||||
/// new refresh token that replaces the old one. Both tokens are persisted
|
/// new refresh token that replaces the old one. Both tokens are persisted
|
||||||
/// to the OS keychain on success.
|
/// to the OS keychain on success.
|
||||||
async fn refresh_token(&self) -> Result<(), SyncError> {
|
///
|
||||||
|
/// `stale_access` is the access token that just earned the caller a 401.
|
||||||
|
/// Refreshes are serialised behind [`Self::refresh_lock`]: with single-use
|
||||||
|
/// rotated refresh tokens, two overlapping 401-retries (e.g. a replay
|
||||||
|
/// upload racing a manual sync) must not both call `/api/auth/refresh` —
|
||||||
|
/// the second call would present an already-consumed token, get rejected,
|
||||||
|
/// and force a re-login for no user-visible reason. Whoever loses the lock
|
||||||
|
/// race checks whether the stored access token has already moved past
|
||||||
|
/// `stale_access`; if so the refresh already happened and there is nothing
|
||||||
|
/// left to do.
|
||||||
|
async fn refresh_token(&self, stale_access: &str) -> Result<(), SyncError> {
|
||||||
|
let _guard = self.refresh_lock.lock().await;
|
||||||
|
|
||||||
|
if let Ok(current) = load_access_token(&self.username)
|
||||||
|
&& current != stale_access
|
||||||
|
{
|
||||||
|
// Another task refreshed while we waited on the lock; retry with
|
||||||
|
// the token it stored rather than spending the new refresh token.
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
|
||||||
let old_refresh =
|
let old_refresh =
|
||||||
load_refresh_token(&self.username).map_err(|e| SyncError::Auth(e.to_string()))?;
|
load_refresh_token(&self.username).map_err(|e| SyncError::Auth(e.to_string()))?;
|
||||||
|
|
||||||
@@ -231,7 +257,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
// Token expired — refresh and retry once.
|
// Token expired — refresh and retry once.
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -264,7 +290,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
// Token expired — refresh and retry once.
|
// Token expired — refresh and retry once.
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -331,7 +357,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -366,7 +392,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -406,7 +432,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -446,7 +472,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -480,7 +506,7 @@ impl SyncProvider for SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
@@ -542,7 +568,7 @@ impl SolitaireServerClient {
|
|||||||
.map_err(|e| SyncError::Network(e.to_string()))?;
|
.map_err(|e| SyncError::Network(e.to_string()))?;
|
||||||
|
|
||||||
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
if resp.status() == reqwest::StatusCode::UNAUTHORIZED {
|
||||||
self.refresh_token().await?;
|
self.refresh_token(&token).await?;
|
||||||
let new_token = self.access_token()?;
|
let new_token = self.access_token()?;
|
||||||
let resp = self
|
let resp = self
|
||||||
.client
|
.client
|
||||||
|
|||||||
Reference in New Issue
Block a user