openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-06 20:20:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Add managed per-app approval requirements

Browse Source
This commit is contained in:
Alex Zamoshchin
2026-06-02 12:51:35 -04:00
parent 859dbe2761
commit a18da7cca1
5 changed files with 221 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/cloud-config/src/lib.rs
+6 -2
View File
@@ -1452,6 +1452,7 @@ deny_read = [{deny_read:?}]
r#"
[apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa]
enabled = false
allowed_approvals_reviewers = ["user"]
"#,
));
@@ -1463,7 +1464,10 @@ enabled = false
"connector_5f3c8c41a1e54ad7a76272c89e2554fa".to_string(),
codex_config::AppRequirementToml {
enabled: Some(false),
tools: None,
allowed_approvals_reviewers: Some(vec![
codex_protocol::config_types::ApprovalsReviewer::User,
]),
..Default::default()
},
)]),
}),
@@ -1488,7 +1492,6 @@ approval_mode = "approve"
apps: BTreeMap::from([(
"connector_5f3c8c41a1e54ad7a76272c89e2554fa".to_string(),
codex_config::AppRequirementToml {
enabled: None,
tools: Some(codex_config::AppToolsRequirementsToml {
tools: BTreeMap::from([(
"calendar/list_events".to_string(),
@@ -1497,6 +1500,7 @@ approval_mode = "approve"
},
)]),
}),
..Default::default()
},
)]),
}),
codex-rs/config/src/config_requirements.rs
+51 -5
View File
@@ -764,12 +764,14 @@ impl AppToolsRequirementsToml {
#[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)]
pub struct AppRequirementToml {
pub enabled: Option<bool>,
pub allowed_approvals_reviewers: Option<Vec<ApprovalsReviewer>>,
pub tools: Option<AppToolsRequirementsToml>,
}
impl AppRequirementToml {
pub fn is_empty(&self) -> bool {
self.enabled.is_none()
&& self.allowed_approvals_reviewers.is_none()
&& self
.tools
.as_ref()
@@ -791,7 +793,8 @@ impl AppsRequirementsToml {
/// Merge app requirements from a lower-precedence source into an existing higher-precedence set.
/// This lets managed sources (for example Cloud/MDM) enforce setting disablement across layers,
/// while exact tool approval settings keep the higher-precedence value when present.
/// while app reviewer constraints and exact tool approval settings keep the higher-precedence
/// value when present.
pub(crate) fn merge_app_requirements_descending(
base: &mut AppsRequirementsToml,
incoming: AppsRequirementsToml,
@@ -807,6 +810,10 @@ pub(crate) fn merge_app_requirements_descending(
higher_precedence.or(lower_precedence)
};
if base_requirement.allowed_approvals_reviewers.is_none() {
base_requirement.allowed_approvals_reviewers =
incoming_requirement.allowed_approvals_reviewers;
}
let Some(incoming_tools) = incoming_requirement.tools else {
continue;
};
@@ -2048,6 +2055,7 @@ allowed_approvals_reviewers = ["user"]
let toml_str = r#"
[apps.connector_123123]
enabled = false
allowed_approvals_reviewers = ["user"]
"#;
let requirements: ConfigRequirementsToml = from_str(toml_str)?;
@@ -2058,7 +2066,8 @@ allowed_approvals_reviewers = ["user"]
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]),
..Default::default()
},
)]),
})
@@ -2080,7 +2089,6 @@ allowed_approvals_reviewers = ["user"]
apps: BTreeMap::from([(
"connector_123123".to_string(),
AppRequirementToml {
enabled: None,
tools: Some(AppToolsRequirementsToml {
tools: BTreeMap::from([(
"calendar/list_events".to_string(),
@@ -2089,6 +2097,7 @@ allowed_approvals_reviewers = ["user"]
},
)]),
}),
..Default::default()
},
)]),
})
@@ -2105,7 +2114,7 @@ allowed_approvals_reviewers = ["user"]
(*app_id).to_string(),
AppRequirementToml {
enabled: *enabled,
tools: None,
..Default::default()
},
)
})
@@ -2122,7 +2131,6 @@ allowed_approvals_reviewers = ["user"]
apps: BTreeMap::from([(
app_id.to_string(),
AppRequirementToml {
enabled: None,
tools: Some(AppToolsRequirementsToml {
tools: BTreeMap::from([(
tool_name.to_string(),
@@ -2131,6 +2139,7 @@ allowed_approvals_reviewers = ["user"]
},
)]),
}),
..Default::default()
},
)]),
}
@@ -2250,6 +2259,43 @@ allowed_approvals_reviewers = ["user"]
);
}
#[test]
fn merge_app_requirements_descending_preserves_higher_app_reviewer_constraint() {
let mut merged = AppsRequirementsToml {
apps: BTreeMap::from([(
"connector_123123".to_string(),
AppRequirementToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]),
..Default::default()
},
)]),
};
let lower = AppsRequirementsToml {
apps: BTreeMap::from([(
"connector_123123".to_string(),
AppRequirementToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..Default::default()
},
)]),
};
merge_app_requirements_descending(&mut merged, lower);
assert_eq!(
merged,
AppsRequirementsToml {
apps: BTreeMap::from([(
"connector_123123".to_string(),
AppRequirementToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]),
..Default::default()
},
)]),
}
);
}
#[test]
fn merge_unset_fields_merges_apps_across_sources_with_enabled_evaluation() {
let higher_source = RequirementSource::CloudRequirements;
codex-rs/config/src/requirements_layers/stack_tests.rs
+3
View File
@@ -226,6 +226,7 @@ shared = false
[apps.connector_1]
enabled = false
allowed_approvals_reviewers = ["auto_review", "user"]
[apps.connector_1.tools.search]
approval_mode = "prompt"
@@ -244,6 +245,7 @@ shared = true
[apps.connector_1]
enabled = true
allowed_approvals_reviewers = ["user"]
[apps.connector_1.tools.search]
approval_mode = "approve"
@@ -264,6 +266,7 @@ shared = true
[apps.connector_1]
enabled = true
allowed_approvals_reviewers = ["user"]
[apps.connector_1.tools.list]
approval_mode = "prompt"
codex-rs/core/src/connectors.rs
+51 -27
View File
@@ -24,6 +24,7 @@ use crate::config::Config;
use crate::mcp::McpManager;
use crate::plugins::list_tool_suggest_discoverable_plugins;
use crate::session::INITIAL_SUBMIT_ID;
use codex_config::AppRequirementToml;
use codex_config::AppsRequirementsToml;
use codex_config::types::AppToolApproval;
use codex_config::types::ApprovalsReviewer;
@@ -577,28 +578,53 @@ pub(crate) fn mcp_approvals_reviewer(
server_name: &str,
connector_id: Option<&str>,
) -> ApprovalsReviewer {
let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME {
read_user_apps_config(config).and_then(|apps_config| {
connector_id
.and_then(|connector_id| apps_config.apps.get(connector_id))
.and_then(|app| app.approvals_reviewer)
})
} else {
None
};
if let Some(reviewer) = app_reviewer
&& config
.config_layer_stack
.requirements()
.approvals_reviewer
.can_set(&reviewer)
.is_ok()
{
return reviewer;
if server_name != CODEX_APPS_MCP_SERVER_NAME {
return config.approvals_reviewer;
}
config.approvals_reviewer
let app_reviewer = read_user_apps_config(config).and_then(|apps_config| {
connector_id
.and_then(|connector_id| apps_config.apps.get(connector_id))
.and_then(|app| app.approvals_reviewer)
});
let app_requirement = config
.config_layer_stack
.requirements_toml()
.apps
.as_ref()
.and_then(|apps| connector_id.and_then(|connector_id| apps.apps.get(connector_id)));
app_approvals_reviewer(config, app_reviewer, app_requirement)
}
fn app_approvals_reviewer(
config: &Config,
configured: Option<ApprovalsReviewer>,
requirement: Option<&AppRequirementToml>,
) -> ApprovalsReviewer {
let resolved_app_allowed =
requirement.and_then(|requirement| requirement.allowed_approvals_reviewers.as_deref());
if let Some(resolved_app_allowed) = resolved_app_allowed {
return configured
.into_iter()
.chain([config.approvals_reviewer])
.find(|reviewer| resolved_app_allowed.contains(reviewer))
.or_else(|| resolved_app_allowed.first().copied())
// An empty managed allow-list must not route through Guardian.
.unwrap_or(ApprovalsReviewer::User);
}
configured
.filter(|reviewer| {
config
.config_layer_stack
.requirements()
.approvals_reviewer
.can_set(reviewer)
.is_ok()
})
.unwrap_or(config.approvals_reviewer)
}
fn read_apps_config(config: &Config) -> Option<AppsConfigToml> {
@@ -648,14 +674,12 @@ fn managed_app_tool_approval(
tool_name: &str,
) -> Option<AppToolApproval> {
let connector_id = connector_id?;
requirements_apps_config?
.apps
.get(connector_id)?
let requirement = requirements_apps_config?.apps.get(connector_id)?;
requirement
.tools
.as_ref()?
.tools
.get(tool_name)?
.approval_mode
.as_ref()
.and_then(|tools| tools.tools.get(tool_name))
.and_then(|tool| tool.approval_mode)
}
fn app_is_enabled(apps_config: &AppsConfigToml, connector_id: Option<&str>) -> bool {
codex-rs/core/src/connectors_tests.rs
+110 -8
View File
@@ -474,6 +474,108 @@ approvals_reviewer = "user"
);
}
#[tokio::test]
async fn app_approvals_reviewer_uses_first_resolved_app_requirement_when_constraints_conflict() {
let codex_home = tempdir().expect("tempdir should succeed");
std::fs::write(
codex_home.path().join(CONFIG_TOML_FILE),
r#"
approvals_reviewer = "user"
"#,
)
.expect("write config");
let requirements = ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]),
apps: Some(AppsRequirementsToml {
apps: BTreeMap::from([(
"calendar".to_string(),
AppRequirementToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..Default::default()
},
)]),
}),
..Default::default()
};
let config = ConfigBuilder::default()
.codex_home(codex_home.path().to_path_buf())
.cloud_requirements(CloudRequirementsLoader::new(async move {
Ok(Some(requirements))
}))
.build()
.await
.expect("config should build");
assert_eq!(
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
ApprovalsReviewer::AutoReview
);
}
#[tokio::test]
async fn app_approvals_reviewer_respects_app_reviewer_requirements() {
for (global, allowed, expected_global, expected_app) in [
(
"auto_review",
vec![ApprovalsReviewer::User],
ApprovalsReviewer::AutoReview,
ApprovalsReviewer::User,
),
(
"user",
vec![ApprovalsReviewer::AutoReview],
ApprovalsReviewer::User,
ApprovalsReviewer::AutoReview,
),
(
"auto_review",
Vec::new(),
ApprovalsReviewer::AutoReview,
ApprovalsReviewer::User,
),
] {
let codex_home = tempdir().expect("tempdir should succeed");
std::fs::write(
codex_home.path().join(CONFIG_TOML_FILE),
format!(
r#"
approvals_reviewer = "{global}"
"#
),
)
.expect("write config");
let requirements = ConfigRequirementsToml {
apps: Some(AppsRequirementsToml {
apps: BTreeMap::from([(
"calendar".to_string(),
AppRequirementToml {
allowed_approvals_reviewers: Some(allowed),
..Default::default()
},
)]),
}),
..Default::default()
};
let config = ConfigBuilder::default()
.codex_home(codex_home.path().to_path_buf())
.cloud_requirements(CloudRequirementsLoader::new(async move {
Ok(Some(requirements))
}))
.build()
.await
.expect("config should build");
assert_eq!(
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
expected_app
);
assert_eq!(
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("drive")),
expected_global
);
}
}
#[test]
fn requirements_disabled_connector_overrides_enabled_connector() {
let mut effective_apps = AppsConfigToml {
@@ -491,7 +593,7 @@ fn requirements_disabled_connector_overrides_enabled_connector() {
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
};
@@ -524,7 +626,7 @@ fn requirements_enabled_does_not_override_disabled_connector() {
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(true),
tools: None,
..Default::default()
},
)]),
};
@@ -558,7 +660,7 @@ enabled = true
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
}),
@@ -602,7 +704,7 @@ async fn cloud_requirements_disable_connector_applies_without_user_apps_table()
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
}),
@@ -653,7 +755,7 @@ async fn local_requirements_disable_connector_overrides_user_apps_config() {
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
}),
@@ -705,7 +807,7 @@ async fn local_requirements_disable_connector_applies_without_user_apps_table()
"connector_123123".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
}),
@@ -747,7 +849,7 @@ async fn with_app_enabled_state_preserves_unrelated_disabled_connector() {
"connector_drive".to_string(),
AppRequirementToml {
enabled: Some(false),
tools: None,
..Default::default()
},
)]),
}),
@@ -829,7 +931,6 @@ fn app_tool_requirements(
apps: BTreeMap::from([(
app_id.to_string(),
AppRequirementToml {
enabled: None,
tools: Some(AppToolsRequirementsToml {
tools: BTreeMap::from([(
tool_name.to_string(),
@@ -838,6 +939,7 @@ fn app_tool_requirements(
},
)]),
}),
..Default::default()
},
)]),
}