From 27c0f6d0f97329c3fd77cfd822f619bf56ac4a5f Mon Sep 17 00:00:00 2001 From: Nathan Thillairajah Date: Thu, 24 Sep 2026 13:44:11 -0400 Subject: [PATCH] Guard app access replacements with an expected revision --- docs/apps-access.md | 24 +++++++++++++ src/bl/apps.rs | 84 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 docs/apps-access.md diff --git a/docs/apps-access.md b/docs/apps-access.md new file mode 100644 index 0000000..a091b7b --- /dev/null +++ b/docs/apps-access.md @@ -0,0 +1,24 @@ +# Guard an access replacement against concurrent edits + +`bl apps access get APP_ID` returns the current policy. On servers that support +revision protection, it also returns `access_revision`. Pass that exact revision +when replacing the policy: + +```sh +bl apps access set APP_ID --visibility restricted --viewer 'EXACT_SUBJECT' --expected-revision 7 +``` + +The revision in this example is illustrative: use the value returned for your +app and environment. Replacement still requires the complete intended viewer +list. To clear a restricted list, use the existing `--clear-viewers` option. + +The CLI checks the server contract before sending a guarded update. If +`access_policy.expected_revision` is absent or false, it fails without sending +the update. If another edit wins, the server returns a conflict; read access +again and review the new policy before retrying. The CLI does not retry the +replacement or fall back to an unguarded request. + +Omitting `--expected-revision` preserves the legacy replacement behavior. +This option is concurrency protection for the existing low-level command, not +recipient discovery or a customer grant/revoke experience. It does not verify +that a supplied subject belongs to an existing account. diff --git a/src/bl/apps.rs b/src/bl/apps.rs index 7fe675e..1d389b5 100644 --- a/src/bl/apps.rs +++ b/src/bl/apps.rs @@ -376,6 +376,11 @@ pub fn command() -> Command { )) .subcommand(control_plane_args( Command::new("set") + .arg(Arg::new("expected-revision") + .long("expected-revision") + .value_name("REVISION") + .value_parser(clap::value_parser!(u64)) + .help("Require the access_revision from access get; fail if the policy changed or the server lacks revision protection")) .about("Replace an app's visibility and explicit viewer list") .long_about( "Replace an app's complete access policy. For restricted visibility, \ @@ -859,6 +864,7 @@ fn run_access_set(config: &SkillsConfig, matches: &ArgMatches) -> Result<()> { ); } let request = AccessRequest { + expected_revision: matches.get_one::("expected-revision").copied(), visibility, viewers, environment: matches.get_one::("environment").map(String::as_str), @@ -913,6 +919,8 @@ struct DeleteAppRequest<'a> { #[derive(Serialize)] struct AccessRequest<'a> { + #[serde(skip_serializing_if = "Option::is_none")] + expected_revision: Option, visibility: &'a str, viewers: Vec<&'a str>, #[serde(skip_serializing_if = "Option::is_none")] @@ -1350,6 +1358,16 @@ impl ControlPlaneClient { app_id: &str, request: &AccessRequest<'_>, ) -> Result { + if request.expected_revision.is_some() { + let contract = self.contract(credential)?; + if contract + .pointer("/access_policy/expected_revision") + .and_then(Value::as_bool) + != Some(true) + { + anyhow::bail!("this Apps Platform server does not advertise access revision protection; no access update was sent"); + } + } let url = self.app_resource_url(app_id, "access", &[])?; let path = url.path().to_string(); self.authorized_json_request(credential, "PUT", &path, |authorization| { @@ -4529,6 +4547,70 @@ mod tests { server_thread.join().expect("join control-plane server"); } + #[test] + fn access_revision_guard_requires_support_and_never_retries_conflicts() { + for supported in [false, true] { + let server = Server::http("127.0.0.1:0").expect("bind server"); + let base_url = format!("http://{}", server.server_addr()); + let server_thread = thread::spawn(move || { + let request = server.recv().expect("contract request"); + assert_eq!(request.method().as_str(), "GET"); + assert_eq!(request.url(), "/v1/agent/contract"); + request + .respond( + Response::from_string( + json!({"access_policy": {"expected_revision": supported}}).to_string(), + ) + .with_header( + Header::from_bytes("Content-Type", "application/json").unwrap(), + ), + ) + .unwrap(); + if supported { + let mut request = server.recv().expect("conditional update"); + assert_eq!(request.method().as_str(), "PUT"); + let mut body = String::new(); + request.as_reader().read_to_string(&mut body).unwrap(); + assert_eq!( + serde_json::from_str::(&body).unwrap(), + json!({"visibility":"restricted", "viewers":["auth0|alice"], "expected_revision":7}) + ); + request + .respond( + Response::from_string(r#"{"error":"access_revision_conflict"}"#) + .with_status_code(409), + ) + .unwrap(); + } + assert!( + server + .recv_timeout(Duration::from_millis(300)) + .unwrap() + .is_none(), + "must not send an unguarded update or retry" + ); + }); + let client = test_control_plane_client(&base_url, Duration::from_secs(2)); + let credential = test_credential("revision_test_credential_123456"); + let error = client + .set_access( + &credential, + "app", + &AccessRequest { + expected_revision: Some(7), + visibility: "restricted", + viewers: vec!["auth0|alice"], + environment: None, + }, + ) + .unwrap_err(); + if !supported { + assert!(error.to_string().contains("no access update was sent")); + } + server_thread.join().unwrap(); + } + } + #[test] fn access_get_and_set_support_each_environment_and_viewer_shape() { let server = Server::http("127.0.0.1:0").expect("bind control-plane server"); @@ -4588,11 +4670,13 @@ mod tests { let credential = test_credential("access_environment_session_credential_123456"); let organization = AccessRequest { + expected_revision: None, visibility: "organization", viewers: vec![], environment: None, }; let restricted = AccessRequest { + expected_revision: None, visibility: "restricted", viewers: vec!["auth0|alice", "auth0|bob"], environment: Some("staging/west?cell=1"),