diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index f80cd6eb47..b4b6c6cce5 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -74,6 +74,7 @@ use self::store_lock::OAuthStoreLockFailure; use codex_config::McpServerOAuthConfig; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; +use rmcp::transport::auth::AuthError; use rmcp::transport::auth::AuthorizationManager; use tokio::sync::Mutex; @@ -343,6 +344,21 @@ fn normalized_oauth_credentials(tokens: Option<&StoredOAuthTokens>) -> Option, +) -> Option { + normalized_oauth_credentials(tokens).map(|mut tokens| { + // Compare the representation File can persist, not vendor-only response fields. + let response = &mut tokens.token_response.0; + response.set_extra_fields(VendorExtraTokenFields::default()); + response.set_token_type(BasicTokenType::Bearer); + if response.scopes().is_some_and(Vec::is_empty) { + response.set_scopes(None); + } + tokens + }) +} + fn oauth_tokens_are_usable(tokens: &StoredOAuthTokens) -> bool { if tokens.client_id.trim().is_empty() { return false; @@ -588,7 +604,7 @@ fn save_oauth_tokens_with_keyring_and_cleanup_file( } }; - let file_removed = delete_oauth_tokens_from_file(&key)?; + let file_removed = delete_oauth_tokens_from_file(server_name, url, &key)?; Ok(keyring_removed || file_removed) }); observation.record_delete_attempt(Store::Multiple, &result); @@ -760,6 +776,16 @@ struct OAuthPersistorInner { last_credentials: Mutex>, } +impl OAuthPersistorInner { + fn refreshed_server_name(&self, previous: Option<&StoredOAuthTokens>) -> String { + // File refreshes update the matched row in place, including its original server name. + match (self.credential_store.backend, previous) { + (resolved_store::Backend::File, Some(previous)) => previous.server_name.clone(), + _ => self.server_name.clone(), + } + } +} + impl OAuthPersistor { pub(crate) fn new( server_name: String, @@ -792,11 +818,77 @@ impl OAuthPersistor { reason = "AuthorizationManager async access must be serialized through its mutex" )] pub(crate) async fn persist_if_needed(&self) -> Result<()> { - let (client_id, maybe_credentials) = { - let manager = self.inner.authorization_manager.clone(); - let guard = manager.lock().await; - guard.get_credentials().await - }?; + { + let manager = self.inner.authorization_manager.lock().await; + if !self.credentials_changed(&manager).await? { + return Ok(()); + } + } + + let credential_guard = self.acquire_persistence_guard().await?; + let manager = self.inner.authorization_manager.lock().await; + self.persist_with_manager(&manager, &credential_guard).await + } + + pub(super) async fn acquire_persistence_guard( + &self, + ) -> std::result::Result { + let guard = + RefreshCredentialLock::acquire_for_server(&self.inner.server_name, &self.inner.url) + .await + .map_err(|_| { + AuthError::CredentialStoreError( + "failed to lock MCP OAuth credentials".to_string(), + ) + })?; + let inner = Arc::clone(&self.inner); + let current = tokio::task::spawn_blocking(move || { + inner + .credential_store + .load(&DefaultKeyringStore, &inner.server_name, &inner.url) + }) + .await + .map_err(|_| { + AuthError::CredentialStoreError( + "MCP OAuth credential validation task failed".to_string(), + ) + })? + .map_err(|_| { + AuthError::CredentialStoreError("failed to validate MCP OAuth credentials".to_string()) + })?; + let expected = self.stored_credentials().await; + let expected = match self.inner.credential_store.backend { + resolved_store::Backend::File => normalized_file_oauth_credentials(expected.as_ref()), + resolved_store::Backend::Keyring(_) => expected, + }; + if normalized_oauth_credentials(current.as_ref()) != expected { + warn!( + server_name = %self.inner.server_name, + "stored MCP OAuth credentials changed; authorization must be rebuilt" + ); + return Err(AuthError::AuthorizationRequired); + } + Ok(guard) + } + + async fn credentials_changed(&self, manager: &AuthorizationManager) -> Result { + let (client_id, maybe_credentials) = manager.get_credentials().await?; + let last_credentials = self.inner.last_credentials.lock().await; + Ok(match maybe_credentials { + Some(credentials) => last_credentials.as_ref().is_none_or(|previous| { + previous.client_id != client_id + || previous.token_response != WrappedOAuthTokenResponse(credentials) + }), + None => last_credentials.is_some(), + }) + } + + pub(super) async fn persist_with_manager( + &self, + manager: &AuthorizationManager, + _credential_guard: &RefreshCredentialLock, + ) -> Result<()> { + let (client_id, maybe_credentials) = manager.get_credentials().await?; match maybe_credentials { Some(credentials) => { @@ -814,7 +906,7 @@ impl OAuthPersistor { compute_expires_at_millis(&credentials) }; let stored = StoredOAuthTokens { - server_name: self.inner.server_name.clone(), + server_name: self.inner.refreshed_server_name(last_credentials.as_ref()), url: self.inner.url.clone(), issuer: last_credentials .as_ref() @@ -827,6 +919,7 @@ impl OAuthPersistor { self.inner.credential_store.save_with_refresh_telemetry( &DefaultKeyringStore, &self.inner.server_name, + last_credentials.as_ref(), &stored, )?; *last_credentials = Some(stored); @@ -878,6 +971,16 @@ struct FallbackTokenEntry { executor_owned: bool, } +impl FallbackTokenEntry { + fn matches_legacy_key(&self, server_name: &str, url: &str) -> bool { + !server_name.starts_with("executor:") + && !server_name.starts_with("local:") + && !self.executor_owned + && self.server_name == server_name + && self.server_url == url + } +} + fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result> { let _store_lock = OAuthStoreLock::acquire_for_read(OAuthStore::File)?; load_oauth_tokens_from_file_with_lock_held(server_name, url) @@ -897,20 +1000,14 @@ fn load_oauth_tokens_from_file_with_lock_held( let local_server_name = server_name.strip_prefix("local:").unwrap_or(server_name); for (stored_key, entry) in &store { - let matches_credential = if server_name.starts_with("executor:") { - stored_key == &key - && entry.executor_owned - && entry.server_name == server_name - && entry.server_url == url - } else if entry.executor_owned { - false + let matches_credential = if stored_key != &key { + entry.matches_legacy_key(server_name, url) + } else if server_name.starts_with("executor:") { + entry.executor_owned && entry.server_name == server_name && entry.server_url == url } else { - entry.server_url == url - // Escaped names may also match another server's stored, escaped name. - // Only accept a legacy unescaped entry under this identity's own key. - && (!server_name.starts_with("local:") || stored_key == &key) - && (entry.server_name == local_server_name - || (stored_key == &key && entry.server_name == server_name)) + !entry.executor_owned + && entry.server_url == url + && (entry.server_name == local_server_name || entry.server_name == server_name) }; if !matches_credential { continue; @@ -963,34 +1060,36 @@ fn save_oauth_tokens_to_file_with_lock_held(tokens: &StoredOAuthTokens) -> Resul anyhow::bail!("executor OAuth credential key conflicts with a host-owned credential"); } - let token_response = &tokens.token_response.0; - let expires_at = tokens - .expires_at - .or_else(|| compute_expires_at_millis(token_response)); - let refresh_token = token_response - .refresh_token() - .map(|token| token.secret().to_string()); - let scopes = token_response - .scopes() - .map(|s| s.iter().map(|s| s.to_string()).collect()) - .unwrap_or_default(); - let entry = FallbackTokenEntry { - server_name: tokens.server_name.clone(), - server_url: tokens.url.clone(), - issuer: tokens.issuer.clone(), - client_id: tokens.client_id.clone(), - access_token: token_response.access_token().secret().to_string(), - expires_at, - refresh_token, - scopes, - executor_owned, - }; - - store.insert(key, entry); + remove_file_credential(&mut store, &tokens.server_name, &tokens.url, &key); + store.insert(key, FallbackTokenEntry::from(tokens)); write_fallback_file(&store) } -fn delete_oauth_tokens_from_file(key: &str) -> Result { +impl From<&StoredOAuthTokens> for FallbackTokenEntry { + fn from(tokens: &StoredOAuthTokens) -> Self { + let token_response = &tokens.token_response.0; + Self { + server_name: tokens.server_name.clone(), + server_url: tokens.url.clone(), + issuer: tokens.issuer.clone(), + client_id: tokens.client_id.clone(), + access_token: token_response.access_token().secret().to_string(), + expires_at: tokens + .expires_at + .or_else(|| compute_expires_at_millis(token_response)), + refresh_token: token_response + .refresh_token() + .map(|token| token.secret().to_string()), + scopes: token_response + .scopes() + .map(|scopes| scopes.iter().map(|scope| scope.to_string()).collect()) + .unwrap_or_default(), + executor_owned: tokens.server_name.starts_with("executor:"), + } + } +} + +fn delete_oauth_tokens_from_file(server_name: &str, url: &str, key: &str) -> Result { let _store_lock = OAuthStoreLock::acquire_for_write(OAuthStore::File)?; let mut store = match read_fallback_file_unlocked()? { Some(store) => store, @@ -1004,8 +1103,7 @@ fn delete_oauth_tokens_from_file(key: &str) -> Result { anyhow::bail!("executor OAuth credential key conflicts with a host-owned credential"); } - let removed = store.remove(key).is_some(); - + let removed = remove_file_credential(&mut store, server_name, url, key); if removed { write_fallback_file(&store)?; } @@ -1013,6 +1111,21 @@ fn delete_oauth_tokens_from_file(key: &str) -> Result { Ok(removed) } +fn remove_file_credential( + store: &mut FallbackFile, + server_name: &str, + url: &str, + key: &str, +) -> bool { + let original_len = store.len(); + // Enterprise keys are profile-scoped. A legacy ordinary row may share the name and URL. + let enterprise_scoped = server_name.starts_with("ema-idp:"); + store.retain(|stored_key, entry| { + stored_key != key && (enterprise_scoped || !entry.matches_legacy_key(server_name, url)) + }); + store.len() != original_len +} + pub(crate) fn compute_expires_at_millis(response: &OAuthTokenResponse) -> Option { let expires_in = response.expires_in()?; let now = SystemTime::now() @@ -1638,6 +1751,18 @@ mod tests { let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; store.save(KEYRING_SERVICE, &key, &serialized)?; super::save_oauth_tokens_to_file(&tokens)?; + let mut entries = read_fallback_file_unlocked()?.expect("stored credentials"); + entries.insert( + "older-build-key".to_string(), + FallbackTokenEntry::from(&tokens), + ); + let other = StoredOAuthTokens { + server_name: "other".into(), + ..tokens.clone() + }; + let other_key = compute_store_key(&other.server_name, &other.url)?; + entries.insert(other_key.clone(), FallbackTokenEntry::from(&other)); + write_fallback_file(&entries)?; let removed = super::delete_oauth_tokens_from_keyring_and_file( &store, @@ -1648,7 +1773,31 @@ mod tests { )?; assert!(removed); assert!(!store.contains(&key)); - assert!(!super::fallback_file_path()?.exists()); + let remaining = read_fallback_file_unlocked()?.expect("other credential"); + assert_eq!(remaining.into_keys().collect::>(), vec![other_key]); + Ok(()) + } + + #[test] + fn enterprise_file_cleanup_preserves_legacy_local_credentials() -> Result<()> { + let _env = TempCodexHome::new(); + let mut tokens = sample_tokens(); + tokens.server_name = "ema-idp:demo".to_string(); + let key = compute_store_key(&tokens.server_name, &tokens.url)?; + let local_key = compute_store_key("local:ema-idp:demo", &tokens.url)?; + let entry = FallbackTokenEntry::from(&tokens); + write_fallback_file(&FallbackFile::from([ + (key.clone(), entry.clone()), + (local_key.clone(), entry), + ]))?; + + assert!(delete_oauth_tokens_from_file( + &tokens.server_name, + &tokens.url, + &key + )?); + let remaining = read_fallback_file_unlocked()?.expect("legacy local credential"); + assert_eq!(remaining.into_keys().collect::>(), vec![local_key]); Ok(()) } diff --git a/codex-rs/rmcp-client/src/oauth/credential_store.rs b/codex-rs/rmcp-client/src/oauth/credential_store.rs index d7e6bc5e90..b2d02e56e5 100644 --- a/codex-rs/rmcp-client/src/oauth/credential_store.rs +++ b/codex-rs/rmcp-client/src/oauth/credential_store.rs @@ -253,11 +253,13 @@ impl CredentialStore for OAuthCredentialStore .upgrade() .context("OAuth credential mutation requires an active refresh guard") .map_err(credential_store_error)?; + let expected = self.inner.last_credentials.lock().await.clone(); let tokens = tokio::task::spawn_blocking(move || -> Result { let _guard = guard; inner.store.save_with_refresh_telemetry( &inner.keyring, &inner.server_name, + expected.as_ref(), &tokens, )?; Ok(tokens) diff --git a/codex-rs/rmcp-client/src/oauth/refresh_transaction.rs b/codex-rs/rmcp-client/src/oauth/refresh_transaction.rs index 641f930c92..d5c8221e01 100644 --- a/codex-rs/rmcp-client/src/oauth/refresh_transaction.rs +++ b/codex-rs/rmcp-client/src/oauth/refresh_transaction.rs @@ -249,6 +249,7 @@ impl OAuthPersistor { let result = self.inner.credential_store.save_with_refresh_telemetry( keyring_store, &self.inner.server_name, + Some(&latest), &refreshed, ); if let Err(error) = result { @@ -390,7 +391,7 @@ fn refreshed_tokens( token_response.set_scopes(previous.token_response.0.scopes().cloned()); } StoredOAuthTokens { - server_name: inner.server_name.clone(), + server_name: inner.refreshed_server_name(Some(previous)), url: inner.url.clone(), issuer: previous.issuer.clone(), client_id: previous.client_id.clone(), diff --git a/codex-rs/rmcp-client/src/oauth/resolved_store.rs b/codex-rs/rmcp-client/src/oauth/resolved_store.rs index f141c9cd74..b30bf5e31f 100644 --- a/codex-rs/rmcp-client/src/oauth/resolved_store.rs +++ b/codex-rs/rmcp-client/src/oauth/resolved_store.rs @@ -10,7 +10,9 @@ use codex_keyring_store::KeyringStore; use codex_otel::auth_storage::AuthStorageOriginator; use codex_otel::auth_storage::Operation; use codex_otel::auth_storage::Store; +use rmcp::transport::auth::AuthError; +use super::FallbackTokenEntry; use super::OAuthKeyringLoadError; use super::OAuthStore; use super::OAuthStoreLock; @@ -24,6 +26,7 @@ use super::load_oauth_tokens_from_file; use super::load_oauth_tokens_from_file_with_lock_held; use super::load_oauth_tokens_from_keyring; use super::load_oauth_tokens_from_secrets_keyring_with_lock_held; +use super::normalized_file_oauth_credentials; use super::normalized_oauth_credentials; use super::read_fallback_file_unlocked; use super::save_oauth_tokens_to_direct_keyring; @@ -180,15 +183,51 @@ impl ResolvedOAuthCredentialStore { result } - /// Records the refresh-persistence milestone in addition to the underlying pinned save. + /// Persists a refresh to the pinned authority and records the refresh-persistence milestone. + /// + /// File replaces the matched row only if it still holds the expected credentials. Login + /// writes continue to use the ordinary save path. pub(crate) fn save_with_refresh_telemetry( self, keyring_store: &K, server_name: &str, + expected: Option<&StoredOAuthTokens>, tokens: &StoredOAuthTokens, ) -> Result<()> { let mut observation = telemetry::resolved(self, Operation::RefreshPersist); - let result = self.save(keyring_store, server_name, tokens); + let result = match self.backend { + Backend::File => { + let mut save = telemetry::resolved(self, Operation::Save); + let result = (|| { + let _store_lock = OAuthStoreLock::acquire_for_write(OAuthStore::File)?; + let (key, current) = + load_oauth_tokens_from_file_with_lock_held(server_name, &tokens.url)? + .ok_or(AuthError::AuthorizationRequired)?; + if normalized_oauth_credentials(Some(¤t)) + != normalized_file_oauth_credentials(expected) + { + return Err(AuthError::AuthorizationRequired.into()); + } + + let mut store = + read_fallback_file_unlocked()?.context("OAuth file disappeared")?; + // Keep the same matched row and stored identity, including legacy keys. + let entry = store + .get_mut(&key) + .context("OAuth file entry disappeared")?; + *entry = FallbackTokenEntry { + server_name: current.server_name, + server_url: current.url, + executor_owned: entry.executor_owned, + ..FallbackTokenEntry::from(tokens) + }; + write_fallback_file(&store) + })(); + save.record_save_attempt(Store::File, &result); + result + } + Backend::Keyring(_) => self.save(keyring_store, server_name, tokens), + }; observation.record_save_attempt(telemetry::store(self), &result); if matches!(self.backend, Backend::Keyring(_)) && let Err(error) = &result @@ -307,7 +346,7 @@ impl ResolvedOAuthCredentialStore { let mut observation = telemetry::resolved(self, Operation::Delete); let result = match self.backend { Backend::File => compute_store_key(server_name, url) - .and_then(|key| delete_oauth_tokens_from_file(&key)), + .and_then(|key| delete_oauth_tokens_from_file(server_name, url, &key)), Backend::Keyring(AuthKeyringBackendKind::Direct) => { delete_oauth_tokens_from_direct_keyring(keyring_store, server_name, url) } diff --git a/codex-rs/rmcp-client/src/oauth/tests/credential_store_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/credential_store_tests.rs index 02dc54a270..f9e2ba2907 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/credential_store_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/credential_store_tests.rs @@ -59,8 +59,8 @@ async fn mutations_require_and_retain_the_transaction_guard() -> Result<()> { load_oauth_tokens_from_file(&initial.server_name, &initial.url)?.is_none(), !clear, ); - // Guard acquisition now rereads storage. Restore the deleted credential first, - // then delete it again under the guard so the queued save must recreate it. + // Guard acquisition requires a durable credential. Delete it under the guard in the + // save case: a queued save must retain the guard without recreating the row. save_oauth_tokens_to_file(&initial)?; let guard = CredentialStore::acquire_refresh_guard(&store) .await? @@ -87,7 +87,7 @@ async fn mutations_require_and_retain_the_transaction_guard() -> Result<()> { drop(aggregate_lock); let _guard = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), contender).await??; let durable = load_oauth_tokens_from_file(&initial.server_name, &initial.url)?; - assert_eq!(durable.is_none(), clear); + assert!(durable.is_none()); } Ok(()) } diff --git a/codex-rs/rmcp-client/src/oauth/tests/persistor_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/persistor_tests.rs index a8b2eb499e..1b0e7e60e4 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/persistor_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/persistor_tests.rs @@ -44,6 +44,7 @@ use super::assert_tokens_match_without_expiry; use super::sample_tokens; use crate::http_client_adapter::StreamableHttpClientAdapter; use crate::http_client_adapter::StreamableHttpRedirectMode; +use crate::oauth::FallbackTokenEntry; use crate::oauth::OAuthCredentialStore; use crate::oauth::OAuthPersistor; use crate::oauth::OAuthRuntime; @@ -55,10 +56,12 @@ use crate::oauth::compute_store_key; use crate::oauth::delete_oauth_tokens; use crate::oauth::load_oauth_tokens_from_file; use crate::oauth::normalized_oauth_credentials; +use crate::oauth::read_fallback_file_unlocked; use crate::oauth::refresh_lock::RefreshCredentialLock; use crate::oauth::save_oauth_tokens; use crate::oauth::save_oauth_tokens_to_file; use crate::oauth::stored_oauth_credentials; +use crate::oauth::write_fallback_file; use crate::oauth_http_client::OAuthHttpClientAdapter; use crate::oauth_http_client::PROACTIVE_REFRESH_TIMEOUT; use crate::startup_error::is_authentication_required_error; @@ -1035,3 +1038,140 @@ fn expired_tokens(url: &str) -> StoredOAuthTokens { .set_expires_in(Some(&Duration::ZERO)); tokens } + +#[expect( + clippy::await_holding_invalid_type, + reason = "exercise the existing manager-locked refresh before the persistence hook" +)] +#[tokio::test(flavor = "current_thread")] +async fn legacy_persistence_preserves_new_login_and_logout() -> Result<()> { + for logout in [false, true] { + let (_env, server, initial) = test_context().await?; + write_fallback_file(&[("!older-build".into(), FallbackTokenEntry::from(&initial))].into())?; + let persistor = persistor_for(&initial).await?; + // Simulate credentials changed by the existing AuthClient before its persistence hook. + let refresh = Mock::given(method("POST")).and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "refreshed", "refresh_token": "rotated", "token_type": "Bearer", "expires_in": 3600 + }))).expect(1).mount_as_scoped(&server).await; + persistor + .inner + .authorization_manager + .lock() + .await + .refresh_token() + .await?; + drop(refresh); + let replacement = if logout { + None + } else { + let mut next = initial.clone(); + next.client_id = "new-login".to_string(); + Some(next) + }; + match &replacement { + Some(next) => { + save_oauth_tokens( + &next.server_name, + next, + OAuthCredentialsStoreMode::File, + AuthKeyringBackendKind::Direct, + ) + .await? + } + None => { + delete_oauth_tokens( + &initial.server_name, + &initial.url, + OAuthCredentialsStoreMode::File, + AuthKeyringBackendKind::Direct, + ) + .await?; + } + } + assert!(matches!( + persistor + .persist_if_needed() + .await + .unwrap_err() + .downcast_ref::(), + Some(AuthError::AuthorizationRequired) + )); + assert_eq!( + load_oauth_tokens_from_file(&initial.server_name, &initial.url)?, + replacement + ); + } + Ok(()) +} + +#[tokio::test(flavor = "current_thread")] +async fn legacy_file_refreshes_preserve_the_matched_credential() -> Result<()> { + assert_legacy_file_refreshes("server", "server").await?; + assert_legacy_file_refreshes("local:ema-idp:demo", "ema-idp:demo").await +} + +#[expect( + clippy::await_holding_invalid_type, + reason = "exercise the existing manager-locked refresh before the persistence hook" +)] +async fn assert_legacy_file_refreshes(requested_name: &str, stored_name: &str) -> Result<()> { + let (_env, server, mut initial) = test_context().await?; + initial.server_name = stored_name.to_string(); + let legacy_key = if requested_name == stored_name { + "!legacy-oauth-key".to_string() + } else { + // An escaped local name can read its old unescaped row only under its own key. + compute_store_key(requested_name, &initial.url)? + }; + write_fallback_file(&[(legacy_key.clone(), FallbackTokenEntry::from(&initial))].into())?; + let persistor = OAuthPersistor::new( + requested_name.to_string(), + initial.url.clone(), + Arc::new(TokioMutex::new(authorization_manager_for(&initial).await?)), + ResolvedOAuthCredentialStore::file(), + Some(initial.clone()), + /*oauth_config*/ None, + ); + for generation in 1..=2 { + let access = format!("access-{generation}"); + let refresh = format!("refresh-{generation}"); + let mock = Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": access, + "refresh_token": refresh, + "token_type": "Bearer", + "expires_in": 3600, + "id_token": "vendor-id-token", + }))) + .expect(1) + .mount_as_scoped(&server) + .await; + if generation == 1 { + persistor.refresh_if_needed().await?; + } else { + persistor + .inner + .authorization_manager + .lock() + .await + .refresh_token() + .await?; + persistor.persist_if_needed().await?; + } + let entries = read_fallback_file_unlocked()?.expect("File credentials"); + assert_eq!(entries.len(), 1); + let saved = entries.get(&legacy_key).expect("original File entry"); + assert_eq!(saved.server_name, stored_name); + assert_eq!(saved.access_token, access); + assert_eq!(saved.refresh_token.as_ref(), Some(&refresh)); + let cached = persistor + .stored_credentials() + .await + .expect("cached credentials"); + assert_eq!(cached.server_name, stored_name); + drop(mock); + } + Ok(()) +} diff --git a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs index 8213c5ce42..0a3513bfaa 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/store_lock_tests.rs @@ -584,11 +584,13 @@ fn file_store_load_and_delete_observe_aggregate_lock() -> Result<()> { assert_tokens_match_without_expiry(&loaded, &tokens); let key = crate::oauth::compute_store_key(&tokens.server_name, &tokens.url)?; + let server_name = tokens.server_name.clone(); + let url = tokens.url.clone(); let removed = complete_after_store_lock_contention( env.path(), OAuthStore::File, || Ok(()), - move || crate::oauth::delete_oauth_tokens_from_file(&key), + move || crate::oauth::delete_oauth_tokens_from_file(&server_name, &url, &key), )?; assert!(removed); assert!(load_oauth_tokens_from_file(&tokens.server_name, &tokens.url)?.is_none()); diff --git a/codex-rs/rmcp-client/src/oauth/tests/telemetry_tests.rs b/codex-rs/rmcp-client/src/oauth/tests/telemetry_tests.rs index a3af6f2a47..972495a213 100644 --- a/codex-rs/rmcp-client/src/oauth/tests/telemetry_tests.rs +++ b/codex-rs/rmcp-client/src/oauth/tests/telemetry_tests.rs @@ -420,10 +420,10 @@ async fn legacy_refresh_persistence_records_each_write_once() -> Result<()> { }; assert_eq!(result.is_err(), fail_save); let mut expected = Vec::new(); - for operation in if proactive { - vec!["load", "save", "refresh_persist"] + for operation in if !proactive && fail_save { + vec!["load"] } else { - vec!["save", "refresh_persist"] + vec!["load", "save", "refresh_persist"] } { expected.push(expected_metric( /*count*/ 1, @@ -435,7 +435,7 @@ async fn legacy_refresh_persistence_records_each_write_once() -> Result<()> { ("secure_outcome", "not_attempted"), ( "outcome", - if fail_save && operation != "load" { + if fail_save && (!proactive || operation != "load") { "error" } else { "success"