Stop refreshing a dead login token (CE-20)

src/auth/client.rs

@@ -1,5 +1,7 @@
 use crate::api::client::ApiContext;
-use crate::auth::types::{DeviceAuthResponse, OAuthError, StoredCredentials, TokenResponse};
+use crate::auth::types::{
+    DeviceAuthResponse, OAuthError, StoredCredentials, TokenRequestError, TokenResponse,
+};
 use crate::config;
 use crate::http;
 use std::thread;
@@ -55,40 +57,38 @@ impl OAuthClient {
         })
     }
 
-    /// Common token exchange logic - POST to /worker/oauth/token with given body
-    fn exchange_token(&self, body: serde_json::Value) -> Result<StoredCredentials, String> {
+    /// Common token exchange logic - POST to /worker/oauth/token with given body.
+    /// A refusal comes back parsed so callers can tell a dead grant from an outage.
+    fn exchange_token(
+        &self,
+        body: serde_json::Value,
+    ) -> Result<StoredCredentials, TokenRequestError> {
         let url = format!("{}/worker/oauth/token", self.base_url);
 
         let (_agent, request) = ApiContext::http_post(&url, Some(30));
         let request = request.set("Content-Type", "application/json");
-        let response = http::send_with_body(request, &body.to_string())
-            .map_err(|e| format!("Failed to connect to server: {}", e))?;
+        let response = http::send_with_body(request, &body.to_string()).map_err(|e| {
+            TokenRequestError::Transport(format!("Failed to connect to server: {}", e))
+        })?;
 
-        let response_body = response
-            .as_str()
-            .map_err(|e| format!("Invalid response encoding: {}", e))?;
+        let response_body = response.as_str().map_err(|e| {
+            TokenRequestError::Transport(format!("Invalid response encoding: {}", e))
+        })?;
 
         if response.status_code != 200 {
-            let error: OAuthError = serde_json::from_str(response_body).unwrap_or(OAuthError {
-                error: "unknown_error".to_string(),
-                error_description: None,
-            });
-
-            let msg = error
-                .error_description
-                .unwrap_or_else(|| error.error.clone());
-            return Err(msg);
+            return Err(parse_token_error(response.status_code, response_body));
         }
 
         let token_response: TokenResponse = serde_json::from_str(response_body)
-            .map_err(|e| format!("Invalid token response: {}", e))?;
+            .map_err(|e| TokenRequestError::Transport(format!("Invalid token response: {}", e)))?;
 
         let now = chrono::Utc::now().timestamp();
         Ok(StoredCredentials {
             access_token: token_response.access_token,
             refresh_token: token_response.refresh_token,
             access_token_expires_at: now + token_response.expires_in as i64,
             refresh_token_expires_at: now + token_response.refresh_expires_in as i64,
+            refresh_failed: None,
         })
     }
 
@@ -176,6 +176,7 @@ impl OAuthClient {
                     refresh_token: token_response.refresh_token,
                     access_token_expires_at: now + token_response.expires_in as i64,
                     refresh_token_expires_at: now + token_response.refresh_expires_in as i64,
+                    refresh_failed: None,
                 });
             }
 
@@ -225,15 +226,17 @@ impl OAuthClient {
     }
 
     /// Refresh the access token using a refresh token
-    pub fn refresh_access_token(&self, refresh_token: &str) -> Result<StoredCredentials, String> {
+    pub fn refresh_access_token(
+        &self,
+        refresh_token: &str,
+    ) -> Result<StoredCredentials, TokenRequestError> {
         let body = serde_json::json!({
             "grant_type": "refresh_token",
             "refresh_token": refresh_token,
             "client_id": "autter-cli"
         });
 
         self.exchange_token(body)
-            .map_err(|e| format!("Token refresh failed: {}", e))
     }
 
     /// Exchange a Personal Access Token (PAT) for credentials.
@@ -310,6 +313,23 @@ impl OAuthClient {
     }
 }
 
+/// Turn a non-200 token-endpoint reply into a typed error. Anything that is
+/// not an OAuth error body (a proxy page, an empty 5xx) counts as transport.
+fn parse_token_error(status: u16, body: &str) -> TokenRequestError {
+    match serde_json::from_str::<OAuthError>(body) {
+        Ok(error) => TokenRequestError::Refused { status, error },
+        Err(_) => {
+            let detail = body.trim();
+            let detail = if detail.is_empty() {
+                "no details".to_string()
+            } else {
+                detail.chars().take(200).collect()
+            };
+            TokenRequestError::Transport(format!("HTTP {status}: {detail}"))
+        }
+    }
+}
+
 impl Default for OAuthClient {
     fn default() -> Self {
         Self::new()
@@ -458,6 +478,37 @@ mod tests {
         assert!(error.error_description.is_none());
     }
 
+    #[test]
+    fn test_parse_token_error_keeps_the_oauth_fields() {
+        let error = parse_token_error(
+            400,
+            r#"{"error":"invalid_grant","error_description":"Refresh token is not valid. Run `autter login` to sign in again.","error_code":"refresh_token_invalid","login_required":true}"#,
+        );
+        let TokenRequestError::Refused { status, error } = &error else {
+            panic!("expected a refusal, got {error:?}");
+        };
+        assert_eq!(*status, 400);
+        assert_eq!(error.error, "invalid_grant");
+        assert_eq!(error.error_code.as_deref(), Some("refresh_token_invalid"));
+        assert_eq!(error.login_required, Some(true));
+    }
+
+    #[test]
+    fn test_parse_token_error_treats_non_oauth_pages_as_transport() {
+        let error = parse_token_error(502, "<html>Bad Gateway</html>");
+        assert_eq!(
+            error,
+            TokenRequestError::Transport("HTTP 502: <html>Bad Gateway</html>".to_string())
+        );
+        assert!(!error.requires_login());
+
+        let error = parse_token_error(503, "  ");
+        assert_eq!(
+            error,
+            TokenRequestError::Transport("HTTP 503: no details".to_string())
+        );
+    }
+
     // ============= Device Auth Response Parsing Tests =============
 
     #[test]
@@ -512,6 +563,7 @@ mod tests {
             refresh_token: "test".to_string(),
             access_token_expires_at: now + expires_in as i64,
             refresh_token_expires_at: now + refresh_expires_in as i64,
+            refresh_failed: None,
         };
 
         // Access token should expire in about 1 hour

src/auth/credentials.rs

@@ -1,7 +1,7 @@
 #[cfg(all(not(test), feature = "keyring"))]
 use crate::auth::credential_backend::KeyringBackend;
 use crate::auth::credential_backend::{CredentialBackend, FileBackend};
-use crate::auth::types::StoredCredentials;
+use crate::auth::types::{RefreshFailure, StoredCredentials};
 #[cfg(not(test))]
 use crate::config::Config;
 use std::path::PathBuf;
@@ -141,6 +141,7 @@ impl CredentialStore {
                 refresh_token: creds.refresh_token,
                 access_token_expires_at: 0,
                 refresh_token_expires_at: creds.refresh_token_expires_at,
+                refresh_failed: creds.refresh_failed,
             };
             self.store(&migrated)?;
             eprintln!("Migrated stored credentials to API-only authentication.");
@@ -150,6 +151,50 @@ impl CredentialStore {
         Ok(Some(creds))
     }
 
+    /// Store refreshed credentials only if the store still holds
+    /// `sent_refresh_token`, the token the refresh used. Returns `Ok(false)`
+    /// without writing when a sibling refresh, `autter login` or `autter logout`
+    /// changed the store first: overwriting would put back an older session.
+    ///
+    /// Callers hold the refresh locks, so this mainly guards a process whose
+    /// wait for `credentials.lock` timed out.
+    pub fn store_if_current(
+        &self,
+        sent_refresh_token: &str,
+        creds: &StoredCredentials,
+    ) -> Result<bool, String> {
+        match self.load()? {
+            Some(current) if current.refresh_token == sent_refresh_token => {
+                self.store(creds)?;
+                Ok(true)
+            }
+            _ => Ok(false),
+        }
+    }
+
+    /// Persist that the server refused `refresh_token` for good, if it is
+    /// still the stored one. An earlier refusal of the same token is kept, so
+    /// its timestamp (which keys the one-time notice) stays stable. Returns the
+    /// refusal now on record, or `None` when the store moved on to other
+    /// credentials (which must not be marked).
+    pub fn mark_refresh_failed(
+        &self,
+        refresh_token: &str,
+        failure: RefreshFailure,
+    ) -> Result<Option<RefreshFailure>, String> {
+        match self.load()? {
+            Some(mut current) if current.refresh_token == refresh_token => {
+                if let Some(existing) = current.refresh_failed {
+                    return Ok(Some(existing));
+                }
+                current.refresh_failed = Some(failure.clone());
+                self.store(&current)?;
+                Ok(Some(failure))
+            }
+            _ => Ok(None),
+        }
+    }
+
     /// Clear stored credentials
     pub fn clear(&self) -> Result<(), String> {
         self.backend.clear()
@@ -212,6 +257,7 @@ mod tests {
             refresh_token: "test_refresh_token_67890".to_string(),
             access_token_expires_at: chrono::Utc::now().timestamp() + 3600,
             refresh_token_expires_at: chrono::Utc::now().timestamp() + 86400 * 90,
+            refresh_failed: None,
         }
     }
 
@@ -258,6 +304,7 @@ mod tests {
             refresh_token: "opaque-refresh-token".to_string(),
             access_token_expires_at: chrono::Utc::now().timestamp() + 3600,
             refresh_token_expires_at: chrono::Utc::now().timestamp() + 86400,
+            refresh_failed: None,
         };
         store.store(&legacy).unwrap();
 
@@ -281,6 +328,7 @@ mod tests {
                 refresh_token: String::new(),
                 access_token_expires_at: 1,
                 refresh_token_expires_at: 1,
+                refresh_failed: None,
             })
             .unwrap();
 
@@ -303,6 +351,116 @@ mod tests {
         assert_eq!(loaded.access_token, "new_access_token");
     }
 
+    // ============= Compare-and-set writes (CE-20) =============
+
+    fn credentials_with_refresh_token(refresh_token: &str) -> StoredCredentials {
+        StoredCredentials {
+            refresh_token: refresh_token.to_string(),
+            ..make_test_credentials()
+        }
+    }
+
+    fn refusal(at: i64) -> RefreshFailure {
+        RefreshFailure {
+            at,
+            error: "invalid_grant".to_string(),
+            error_code: Some("refresh_token_invalid".to_string()),
+            description: Some("Refresh token is not valid.".to_string()),
+        }
+    }
+
+    #[test]
+    fn test_store_if_current_writes_over_the_token_that_was_sent() {
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        store
+            .store(&credentials_with_refresh_token("rt-1"))
+            .unwrap();
+
+        let rotated = credentials_with_refresh_token("rt-2");
+        assert!(store.store_if_current("rt-1", &rotated).unwrap());
+        assert_eq!(store.load().unwrap().unwrap().refresh_token, "rt-2");
+    }
+
+    #[test]
+    fn test_store_if_current_keeps_a_newer_sibling_rotation() {
+        // Three processes refreshed rt-1 at once. The first wrote rt-2; the
+        // later ones must not put back their own (now older) rotation.
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        store
+            .store(&credentials_with_refresh_token("rt-2"))
+            .unwrap();
+
+        let late = credentials_with_refresh_token("rt-2b");
+        assert!(!store.store_if_current("rt-1", &late).unwrap());
+        assert_eq!(store.load().unwrap().unwrap().refresh_token, "rt-2");
+    }
+
+    #[test]
+    fn test_store_if_current_does_not_resurrect_a_logout() {
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        assert!(
+            !store
+                .store_if_current("rt-1", &credentials_with_refresh_token("rt-2"))
+                .unwrap()
+        );
+        assert!(store.load().unwrap().is_none());
+    }
+
+    #[test]
+    fn test_mark_refresh_failed_marks_only_the_refused_token() {
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        store
+            .store(&credentials_with_refresh_token("rt-new"))
+            .unwrap();
+
+        // The store moved on (login or sibling rotation): nothing is marked.
+        assert_eq!(
+            store.mark_refresh_failed("rt-old", refusal(1)).unwrap(),
+            None
+        );
+        assert!(!store.load().unwrap().unwrap().is_login_required());
+
+        assert_eq!(
+            store.mark_refresh_failed("rt-new", refusal(1)).unwrap(),
+            Some(refusal(1))
+        );
+        let stored = store.load().unwrap().unwrap();
+        assert_eq!(stored.refresh_failed, Some(refusal(1)));
+        assert_eq!(stored.refresh_token, "rt-new");
+    }
+
+    #[test]
+    fn test_mark_refresh_failed_keeps_the_first_refusal() {
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        store
+            .store(&credentials_with_refresh_token("rt-1"))
+            .unwrap();
+        store.mark_refresh_failed("rt-1", refusal(10)).unwrap();
+
+        assert_eq!(
+            store.mark_refresh_failed("rt-1", refusal(20)).unwrap(),
+            Some(refusal(10))
+        );
+        assert_eq!(
+            store.load().unwrap().unwrap().refresh_failed,
+            Some(refusal(10))
+        );
+    }
+
+    #[test]
+    fn test_login_store_clears_the_refusal() {
+        let store = CredentialStore::with_backend(Box::new(MockBackend::new()));
+        store
+            .store(&credentials_with_refresh_token("rt-1"))
+            .unwrap();
+        store.mark_refresh_failed("rt-1", refusal(1)).unwrap();
+
+        store
+            .store(&credentials_with_refresh_token("rt-login"))
+            .unwrap();
+        assert!(!store.load().unwrap().unwrap().is_login_required());
+    }
+
     #[test]
     fn test_load_nonexistent_with_mock() {
         let store = CredentialStore::with_backend(Box::new(MockBackend::new()));