mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Report OAuth authentication failures in MCP status snapshots (#44359)
## Why Status-only discovery could retain an `OAuth` status based on stored credentials even after the connection attempt failed authentication. This discovery path has no event channel to report the failure. ## What changed Update the snapshot to report `NotLoggedIn` for servers previously marked `OAuth` whose connection attempts failed authentication. Preserve other authentication statuses and OAuth statuses for failures unrelated to authentication. ## Testing Add a regression test covering OAuth authentication failure, successful OAuth startup, an unrelated provider error, and a bearer-token authentication failure. GitOrigin-RevId: 9df68fb1f818e40acab0c71bb4fda118f03a7611
This commit is contained in:
committed by
copyberry
parent
e2a9ee05f4
commit
d996b4f02a
@@ -125,7 +125,11 @@ impl McpConnectionSet {
|
||||
}
|
||||
}
|
||||
|
||||
fn insert_test_client(&mut self, name: impl Into<String>, client: AsyncManagedClient) {
|
||||
pub(crate) fn insert_test_client(
|
||||
&mut self,
|
||||
name: impl Into<String>,
|
||||
client: AsyncManagedClient,
|
||||
) {
|
||||
let name = name.into();
|
||||
self.servers.insert(
|
||||
name,
|
||||
@@ -540,7 +544,7 @@ async fn prepared_call_timeout_includes_trusted_access_lookup() {
|
||||
assert!(format!("{error:#}").contains("timed out awaiting tools/call after 1s"));
|
||||
}
|
||||
|
||||
async fn create_ready_async_managed_client(tools: Vec<ToolInfo>) -> AsyncManagedClient {
|
||||
pub(crate) async fn create_ready_async_managed_client(tools: Vec<ToolInfo>) -> AsyncManagedClient {
|
||||
AsyncManagedClient {
|
||||
client: futures::future::ready::<Result<ManagedClient, StartupOutcomeError>>(Ok(
|
||||
create_test_managed_client(tools).await,
|
||||
|
||||
@@ -802,13 +802,22 @@ async fn collect_mcp_server_status_snapshot_from_manager(
|
||||
.insert(tool_name, tool);
|
||||
}
|
||||
|
||||
// Status-only discovery has no event channel. Report OAuth failures from the completed
|
||||
// connection attempt instead of retaining the credential-presence status read beforehand.
|
||||
let mut auth_statuses = auth_statuses_from_entries(&auth_status_entries);
|
||||
for server_name in mcp_connection_manager.authentication_failed_servers().await {
|
||||
if auth_statuses.get(&server_name) == Some(&McpAuthStatus::OAuth) {
|
||||
auth_statuses.insert(server_name, McpAuthStatus::NotLoggedIn);
|
||||
}
|
||||
}
|
||||
|
||||
McpServerStatusSnapshot {
|
||||
server_infos,
|
||||
tools_by_server,
|
||||
tools_errors,
|
||||
resources: convert_mcp_resources(resources),
|
||||
resource_templates: convert_mcp_resource_templates(resource_templates),
|
||||
auth_statuses: auth_statuses_from_entries(&auth_status_entries),
|
||||
auth_statuses,
|
||||
server_names,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
use super::*;
|
||||
use crate::McpPluginAttribution;
|
||||
use crate::McpServerRegistration;
|
||||
use crate::connection_manager::tests::create_ready_async_managed_client;
|
||||
use crate::mcp::auth::McpAuthStatusEntry;
|
||||
use crate::rmcp_client::StartupOutcomeError;
|
||||
use codex_config::Constrained;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::AuthKeyringBackendKind;
|
||||
@@ -12,12 +15,73 @@ use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_rmcp_client::McpAuthState;
|
||||
use futures::FutureExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
#[tokio::test]
|
||||
async fn status_snapshot_only_downgrades_oauth_authentication_failures() {
|
||||
let auth_failure = StartupOutcomeError::Failed {
|
||||
error: "OAuth refresh token was rejected".to_string(),
|
||||
is_authentication_required: true,
|
||||
};
|
||||
let mut manager = McpConnectionSet::empty(/*prefix_mcp_tool_names*/ true);
|
||||
let mut auth_status_entries = HashMap::new();
|
||||
for (name, auth_state, startup_error) in [
|
||||
(
|
||||
"oauth-failed",
|
||||
McpAuthState::OAuth,
|
||||
Some(auth_failure.clone()),
|
||||
),
|
||||
("oauth-ready", McpAuthState::OAuth, None),
|
||||
(
|
||||
"oauth-provider-error",
|
||||
McpAuthState::OAuth,
|
||||
Some(StartupOutcomeError::Failed {
|
||||
error: "provider temporarily unavailable".to_string(),
|
||||
is_authentication_required: false,
|
||||
}),
|
||||
),
|
||||
("bearer", McpAuthState::BearerToken, Some(auth_failure)),
|
||||
] {
|
||||
let mut client = create_ready_async_managed_client(Vec::new()).await;
|
||||
if let Some(error) = startup_error {
|
||||
client.client = futures::future::ready(Err(error)).boxed().shared();
|
||||
}
|
||||
manager.insert_test_client(name, client);
|
||||
auth_status_entries.insert(
|
||||
name.to_string(),
|
||||
McpAuthStatusEntry {
|
||||
config: None,
|
||||
auth_state,
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
let server_names = auth_status_entries.keys().cloned().collect();
|
||||
let snapshot = collect_mcp_server_status_snapshot_from_manager(
|
||||
&manager,
|
||||
auth_status_entries,
|
||||
server_names,
|
||||
McpSnapshotDetail::ToolsAndAuthOnly,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
snapshot.auth_statuses,
|
||||
HashMap::from([
|
||||
("oauth-failed".to_string(), McpAuthStatus::NotLoggedIn),
|
||||
("oauth-ready".to_string(), McpAuthStatus::OAuth),
|
||||
("oauth-provider-error".to_string(), McpAuthStatus::OAuth),
|
||||
("bearer".to_string(), McpAuthStatus::BearerToken),
|
||||
]),
|
||||
);
|
||||
}
|
||||
|
||||
pub(crate) fn test_mcp_config(codex_home: PathBuf) -> McpConfig {
|
||||
McpConfig {
|
||||
chatgpt_base_url: "https://chatgpt.com".to_string(),
|
||||
|
||||
Reference in New Issue
Block a user