mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Centralize plugin auth capability filtering (#27902)
## Summary This is the first step in making plugin auth routing consistent. The rule should not live as one-off checks in every place that loads or displays plugin capabilities. This PR introduces a small resolver for the auth-level policy: given a plugin's declared apps, MCP servers, current auth mode, and active state, return the capabilities that are actually usable in that context. ## Why Product rule: - SiWC auth can use app connectors, so app declarations stay available. - API-key/direct auth cannot use app connectors, so app declarations are removed. - When an active plugin has both an app and an MCP server with the same name, the app route wins for Codex-backed auth and the conflicting MCP server is hidden. Putting that rule in `capabilities.rs` gives the rest of the stack one place to ask instead of duplicating auth checks in loader, manager, marketplace, and details code. ## Validation - `cargo fmt` - `cargo test -p codex-core-plugins`
This commit is contained in:
committed by
GitHub
Unverified
parent
8aac63f477
commit
7e0dce91df
@@ -0,0 +1,32 @@
|
||||
use codex_app_server_protocol::AuthMode;
|
||||
use codex_plugin::AppDeclaration;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
||||
pub fn apps_route_available(auth_mode: Option<AuthMode>) -> bool {
|
||||
auth_mode.is_some_and(AuthMode::uses_codex_backend)
|
||||
}
|
||||
|
||||
pub(crate) fn apply_app_mcp_routing_policy<M>(
|
||||
apps: &mut Vec<AppDeclaration>,
|
||||
mcp_servers: &mut HashMap<String, M>,
|
||||
auth_mode: Option<AuthMode>,
|
||||
plugin_active: bool,
|
||||
) {
|
||||
if !apps_route_available(auth_mode) {
|
||||
apps.clear();
|
||||
return;
|
||||
}
|
||||
|
||||
if plugin_active && !apps.is_empty() {
|
||||
let app_declaration_names = apps
|
||||
.iter()
|
||||
.map(|app| app.name.as_str())
|
||||
.collect::<HashSet<_>>();
|
||||
mcp_servers.retain(|name, _| !app_declaration_names.contains(name.as_str()));
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "app_mcp_routing_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,99 @@
|
||||
use super::*;
|
||||
use codex_plugin::AppConnectorId;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
|
||||
fn app(name: &str) -> AppDeclaration {
|
||||
AppDeclaration {
|
||||
name: name.to_string(),
|
||||
connector_id: AppConnectorId(format!("connector_{name}")),
|
||||
category: None,
|
||||
}
|
||||
}
|
||||
|
||||
fn mcp_servers(mcp_servers: impl IntoIterator<Item = (&'static str, i32)>) -> HashMap<String, i32> {
|
||||
mcp_servers
|
||||
.into_iter()
|
||||
.map(|(name, value)| (name.to_string(), value))
|
||||
.collect::<HashMap<_, _>>()
|
||||
}
|
||||
|
||||
fn sorted_app_names(apps: &[AppDeclaration]) -> Vec<String> {
|
||||
let mut names = apps.iter().map(|app| app.name.clone()).collect::<Vec<_>>();
|
||||
names.sort();
|
||||
names
|
||||
}
|
||||
|
||||
fn sorted_mcp_server_names(mcp_servers: &HashMap<String, i32>) -> Vec<String> {
|
||||
let mut names = mcp_servers.keys().cloned().collect::<Vec<_>>();
|
||||
names.sort();
|
||||
names
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apps_route_available_tracks_auth_mode() {
|
||||
assert!(apps_route_available(Some(AuthMode::Chatgpt)));
|
||||
assert!(apps_route_available(Some(AuthMode::AgentIdentity)));
|
||||
assert!(!apps_route_available(Some(AuthMode::ApiKey)));
|
||||
assert!(!apps_route_available(/*auth_mode*/ None));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_mcp_routing_clears_apps_when_apps_route_is_unavailable() {
|
||||
let mut apps = vec![app("linear")];
|
||||
let mut mcp_servers = mcp_servers([("linear", 1), ("docs", 2)]);
|
||||
|
||||
apply_app_mcp_routing_policy(
|
||||
&mut apps,
|
||||
&mut mcp_servers,
|
||||
Some(AuthMode::ApiKey),
|
||||
/*plugin_active*/ true,
|
||||
);
|
||||
|
||||
assert!(apps.is_empty());
|
||||
assert_eq!(
|
||||
sorted_mcp_server_names(&mcp_servers),
|
||||
vec!["docs".to_string(), "linear".to_string()]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_mcp_routing_preserves_apps_and_removes_conflicting_mcp_with_apps_route() {
|
||||
let mut apps = vec![app("linear"), app("notion")];
|
||||
let mut mcp_servers = mcp_servers([("linear", 1), ("docs", 2), ("notion", 3)]);
|
||||
|
||||
apply_app_mcp_routing_policy(
|
||||
&mut apps,
|
||||
&mut mcp_servers,
|
||||
Some(AuthMode::Chatgpt),
|
||||
/*plugin_active*/ true,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
sorted_app_names(&apps),
|
||||
vec!["linear".to_string(), "notion".to_string()]
|
||||
);
|
||||
assert_eq!(
|
||||
sorted_mcp_server_names(&mcp_servers),
|
||||
vec!["docs".to_string()]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn app_mcp_routing_preserves_mcp_conflicts_when_plugin_is_inactive() {
|
||||
let mut apps = vec![app("linear")];
|
||||
let mut mcp_servers = mcp_servers([("linear", 1), ("docs", 2)]);
|
||||
|
||||
apply_app_mcp_routing_policy(
|
||||
&mut apps,
|
||||
&mut mcp_servers,
|
||||
Some(AuthMode::Chatgpt),
|
||||
/*plugin_active*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(sorted_app_names(&apps), vec!["linear".to_string()]);
|
||||
assert_eq!(
|
||||
sorted_mcp_server_names(&mcp_servers),
|
||||
vec!["docs".to_string(), "linear".to_string()]
|
||||
);
|
||||
}
|
||||
@@ -1,3 +1,4 @@
|
||||
mod app_mcp_routing;
|
||||
mod discoverable;
|
||||
pub mod installed_marketplaces;
|
||||
pub mod loader;
|
||||
@@ -24,6 +25,7 @@ pub const OPENAI_BUNDLED_MARKETPLACE_NAME: &str = "openai-bundled";
|
||||
pub type LoadedPlugin = codex_plugin::LoadedPlugin<codex_config::McpServerConfig>;
|
||||
pub type PluginLoadOutcome = codex_plugin::PluginLoadOutcome<codex_config::McpServerConfig>;
|
||||
|
||||
pub use app_mcp_routing::apps_route_available;
|
||||
pub use discoverable::ToolSuggestDiscoverablePlugin;
|
||||
pub use discoverable::ToolSuggestPluginDiscoveryInput;
|
||||
pub use loader::PluginHookLoadOutcome;
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
use crate::OPENAI_CURATED_MARKETPLACE_NAME;
|
||||
use crate::app_mcp_routing::apply_app_mcp_routing_policy;
|
||||
use crate::app_mcp_routing::apps_route_available;
|
||||
use crate::manifest::PluginManifestHooks;
|
||||
use crate::manifest::PluginManifestPaths;
|
||||
use crate::manifest::load_plugin_manifest;
|
||||
@@ -1085,20 +1087,17 @@ pub async fn load_plugin_mcp_servers(
|
||||
auth_mode: Option<AuthMode>,
|
||||
) -> HashMap<String, McpServerConfig> {
|
||||
let mut mcp_servers = load_declared_plugin_mcp_servers(plugin_root).await;
|
||||
if !auth_mode.is_some_and(AuthMode::uses_codex_backend) || mcp_servers.is_empty() {
|
||||
if !apps_route_available(auth_mode) || mcp_servers.is_empty() {
|
||||
return mcp_servers;
|
||||
}
|
||||
|
||||
let app_declarations = load_plugin_apps(plugin_root).await;
|
||||
if app_declarations.is_empty() {
|
||||
return mcp_servers;
|
||||
}
|
||||
|
||||
let app_declaration_names = app_declarations
|
||||
.iter()
|
||||
.map(|app| app.name.as_str())
|
||||
.collect::<HashSet<_>>();
|
||||
mcp_servers.retain(|name, _| !app_declaration_names.contains(name.as_str()));
|
||||
let mut app_declarations = load_plugin_apps(plugin_root).await;
|
||||
apply_app_mcp_routing_policy(
|
||||
&mut app_declarations,
|
||||
&mut mcp_servers,
|
||||
auth_mode,
|
||||
/*plugin_active*/ true,
|
||||
);
|
||||
mcp_servers
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::PluginLoadOutcome;
|
||||
use crate::OPENAI_CURATED_MARKETPLACE_NAME;
|
||||
use crate::app_mcp_routing::apply_app_mcp_routing_policy;
|
||||
use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack;
|
||||
use crate::loader::PluginHookLoadOutcome;
|
||||
use crate::loader::configured_curated_plugin_ids_from_codex_home;
|
||||
@@ -210,23 +211,15 @@ fn project_plugin_load_outcome_for_auth(
|
||||
outcome: PluginLoadOutcome,
|
||||
auth_mode: Option<AuthMode>,
|
||||
) -> PluginLoadOutcome {
|
||||
let apps_route_available = auth_mode.is_some_and(AuthMode::uses_codex_backend);
|
||||
let mut plugins = outcome.plugins().to_vec();
|
||||
for plugin in &mut plugins {
|
||||
if apps_route_available {
|
||||
if plugin.is_active() && !plugin.apps.is_empty() {
|
||||
let app_declaration_names = plugin
|
||||
.apps
|
||||
.iter()
|
||||
.map(|app| app.name.as_str())
|
||||
.collect::<HashSet<_>>();
|
||||
plugin
|
||||
.mcp_servers
|
||||
.retain(|name, _| !app_declaration_names.contains(name.as_str()));
|
||||
}
|
||||
} else {
|
||||
plugin.apps.clear();
|
||||
}
|
||||
let plugin_active = plugin.is_active();
|
||||
apply_app_mcp_routing_policy(
|
||||
&mut plugin.apps,
|
||||
&mut plugin.mcp_servers,
|
||||
auth_mode,
|
||||
plugin_active,
|
||||
);
|
||||
}
|
||||
PluginLoadOutcome::from_plugins(plugins)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user