diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 586f9c184bb7..07e304be032d 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -29,7 +29,7 @@ use codex_mcp::oauth_login_support; use codex_mcp::resolve_oauth_scopes; use codex_mcp::should_retry_without_scopes; use codex_protocol::protocol::McpAuthStatus; -use codex_rmcp_client::delete_oauth_tokens; +use codex_rmcp_client::delete_oauth_tokens_locked; use codex_rmcp_client::perform_oauth_login; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::format_env_display; @@ -523,12 +523,14 @@ async fn run_logout(config_overrides: &CliConfigOverrides, logout_args: LogoutAr _ => bail!("OAuth logout is only supported for streamable_http transports."), }; - match delete_oauth_tokens( + match delete_oauth_tokens_locked( &name, &url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), - ) { + ) + .await + { Ok(true) => println!("Removed OAuth credentials for '{name}'."), Ok(false) => println!("No OAuth credentials stored for '{name}'."), Err(err) => return Err(anyhow!("failed to delete OAuth credentials: {err}")), diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 5aae4493ce34..df787945081d 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -23,6 +23,7 @@ pub use in_process_transport::InProcessTransportFactory; pub use oauth::StoredOAuthTokens; pub use oauth::WrappedOAuthTokenResponse; pub use oauth::delete_oauth_tokens; +pub use oauth::delete_oauth_tokens_locked; pub use oauth::save_oauth_tokens; pub use perform_oauth_login::OAuthProviderError; pub use perform_oauth_login::OauthLoginHandle; diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 9759981f59d4..17dfd5ed8b18 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -58,6 +58,7 @@ use codex_keyring_store::KeyringStore; use codex_utils_home_dir::find_codex_home; pub(crate) use self::persistor::OAuthPersistor; +use self::refresh_lock::RefreshCredentialLock; pub(crate) use self::resolved_store::ResolvedOAuthCredentialStore; pub(crate) use self::resolved_store::ResolvedOAuthTokens; pub(crate) use self::resolved_store::load_oauth_tokens_from_store; @@ -231,6 +232,43 @@ pub fn save_oauth_tokens( ) } +pub(crate) async fn save_oauth_tokens_locked( + server_name: &str, + tokens: &StoredOAuthTokens, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, +) -> Result<()> { + let keyring_store = DefaultKeyringStore; + save_oauth_tokens_locked_with_keyring_store( + &keyring_store, + server_name, + tokens, + store_mode, + keyring_backend_kind, + ) + .await +} + +async fn save_oauth_tokens_locked_with_keyring_store( + keyring_store: &K, + server_name: &str, + tokens: &StoredOAuthTokens, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, +) -> Result<()> { + // Login persistence shares the refresh transaction lock so a completed login always becomes + // authoritative: it either lands before refresh's reread or waits and overwrites the refresh + // result afterward. + let _lock = RefreshCredentialLock::acquire_for_server(server_name, &tokens.url).await?; + save_oauth_tokens_with_keyring_store( + keyring_store, + server_name, + tokens, + store_mode, + keyring_backend_kind, + ) +} + fn save_oauth_tokens_with_keyring_store( keyring_store: &K, server_name: &str, @@ -367,6 +405,42 @@ pub fn delete_oauth_tokens( ) } +pub async fn delete_oauth_tokens_locked( + server_name: &str, + url: &str, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, +) -> Result { + let keyring_store = DefaultKeyringStore; + delete_oauth_tokens_locked_with_keyring_store( + &keyring_store, + store_mode, + keyring_backend_kind, + server_name, + url, + ) + .await +} + +async fn delete_oauth_tokens_locked_with_keyring_store( + keyring_store: &K, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + server_name: &str, + url: &str, +) -> Result { + // Logout shares the refresh transaction lock so refresh cannot resurrect credentials after a + // completed delete: it either observes the deletion or finishes before logout removes it. + let _lock = RefreshCredentialLock::acquire_for_server(server_name, url).await?; + delete_oauth_tokens_from_keyring_and_file( + keyring_store, + store_mode, + keyring_backend_kind, + server_name, + url, + ) +} + fn delete_oauth_tokens_from_keyring_and_file( keyring_store: &K, store_mode: OAuthCredentialsStoreMode, diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index 7a9d242bc465..cd2b961b8103 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -28,8 +28,8 @@ use urlencoding::decode; use crate::StoredOAuthTokens; use crate::WrappedOAuthTokenResponse; use crate::oauth::compute_expires_at_millis; +use crate::oauth::save_oauth_tokens_locked; use crate::oauth_http_client::OAuthHttpClientAdapter; -use crate::save_oauth_tokens; use crate::utils::build_default_headers; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::OAuthCredentialsStoreMode; @@ -623,12 +623,13 @@ impl OauthLoginFlow { token_response: WrappedOAuthTokenResponse(credentials), expires_at, }; - save_oauth_tokens( + save_oauth_tokens_locked( &self.server_name, &stored, self.store_mode, self.keyring_backend_kind, - )?; + ) + .await?; Ok(()) }