diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index 11d2fdc..61c8aff 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -296,7 +296,8 @@ async fn group_apply_handler( Json(req): Json, ) -> Result, ApplyError> { let group_id = resolve_group_id(svc.groups.as_ref(), &id_or_slug).await?; - require_group_node_writes(&svc, &principal, group_id, &req.bundle, req.prune).await?; + svc.require_group_node_writes(&principal, group_id, &req.bundle, req.prune) + .await?; let claim = OwnershipClaim { project: req.project, takeover: req.takeover, @@ -421,7 +422,7 @@ async fn authz_tree( let group_id = group.id; match apply_prune { Some(prune) => { - require_group_node_writes(svc, principal, group_id, &node.bundle, prune) + svc.require_group_node_writes(principal, group_id, &node.bundle, prune) .await?; } None => { @@ -504,36 +505,6 @@ async fn require_app_node_writes( Ok(()) } -/// Group-node write caps (editor-tier): scripts → `GroupScriptsWrite`, vars → -/// `GroupVarsWrite`, both on prune. -async fn require_group_node_writes( - svc: &ApplyService, - principal: &Principal, - group_id: GroupId, - bundle: &Bundle, - prune: bool, -) -> Result<(), ApplyError> { - if prune || !bundle.scripts.is_empty() { - require( - svc.authz.as_ref(), - principal, - Capability::GroupScriptsWrite(group_id), - ) - .await - .map_err(map_authz)?; - } - if prune || !bundle.vars.is_empty() { - require( - svc.authz.as_ref(), - principal, - Capability::GroupVarsWrite(group_id), - ) - .await - .map_err(map_authz)?; - } - Ok(()) -} - /// Resolve a slug-or-id path param to a `GroupId`, mapping miss → 404. async fn resolve_group_id( groups: &dyn GroupRepository, diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 874320c..f830c48 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -1795,6 +1795,27 @@ impl ApplyService { } } + // §6 content-write authz, in-tx defense-in-depth. `authz_tree` (API + // layer) SKIPS a group node absent at that time (it is to-create), + // deferring to the create-gate in `reconcile_group_structure_tx`. That + // gate only fires when the group is STILL absent at reconcile — so a + // group created by a racer in the window would otherwise have its + // content written with no authz. Re-check content-write caps here for + // every group NOT structurally created/reparented by this apply: a group + // WE created is covered by create-RBAC (`GroupAdmin(parent)` cascades) + // and its uncommitted row is invisible to the pool-based authz cascade + // anyway, so it must not be re-checked; a pre-existing (incl. + // racer-created) group IS committed, so its ancestry resolves and a + // missing cap denies. Runs before the write loops; a denial rolls back. + for p in &prepared { + if let ApplyOwner::Group(gid) = p.owner { + if !structurally_changed.contains(&gid) { + self.require_group_node_writes(claim.principal, gid, &p.node.bundle, prune) + .await?; + } + } + } + // Lock every node's apply key, in sorted order, so concurrent applies // touching any shared node serialize and the ordering can't deadlock. let mut lock_keys: Vec = prepared.iter().map(|p| apply_lock_key(p.owner)).collect(); @@ -2410,6 +2431,39 @@ impl ApplyService { Ok(out) } + /// Group-node write caps (editor-tier): scripts → `GroupScriptsWrite`, vars + /// → `GroupVarsWrite`, both required on prune. The single-group and tree + /// apply authorization (`apply_api::authz_tree`) call this at the API layer; + /// the tree apply ALSO re-checks it in-tx (see `apply_tree`) to close the + /// window where a group absent at authz time appears before reconcile. + pub(crate) async fn require_group_node_writes( + &self, + principal: &Principal, + group_id: GroupId, + bundle: &Bundle, + prune: bool, + ) -> Result<(), ApplyError> { + if prune || !bundle.scripts.is_empty() { + require( + self.authz.as_ref(), + principal, + Capability::GroupScriptsWrite(group_id), + ) + .await + .map_err(map_authz)?; + } + if prune || !bundle.vars.is_empty() { + require( + self.authz.as_ref(), + principal, + Capability::GroupVarsWrite(group_id), + ) + .await + .map_err(map_authz)?; + } + Ok(()) + } + // ------------------------------------------------------------------------ // §7 ownership claim — the ApplyService side (executes the pure verdict). // ------------------------------------------------------------------------ diff --git a/crates/picloud-cli/tests/group_create.rs b/crates/picloud-cli/tests/group_create.rs index bbc4991..36fa7b9 100644 --- a/crates/picloud-cli/tests/group_create.rs +++ b/crates/picloud-cli/tests/group_create.rs @@ -191,3 +191,75 @@ fn creating_a_group_requires_group_admin_on_the_parent() { "a denied create must leave no group behind:\n{ls}" ); } + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn tree_apply_denies_group_content_write_without_the_cap() { + // A tree apply that writes content (here a `[vars]` entry) into a + // PRE-EXISTING group requires the caller hold that group's write cap. This + // pins the invariant the in-tx content-write re-check hardens: a group that + // isn't structurally created by this apply must pass content authz. (The + // race the re-check closes — a group appearing between the API-layer check + // and reconcile — isn't reproducible at the journey level; end to end a + // member without the cap is refused and the var is not written.) + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let base = common::unique_slug("gcw-base"); + let grp = common::unique_slug("gcw-grp"); + let project = common::unique_slug("gcw-proj"); + let _b = GroupGuard::new(&env.url, &env.token, &base); + let _g = GroupGuard::new(&env.url, &env.token, &grp); + + // `base` is the attach point; `grp` pre-exists directly under it. + common::pic_as(&env) + .args(["groups", "create", &base]) + .assert() + .success(); + common::pic_as(&env) + .args(["groups", "create", &grp, "--parent", &base]) + .assert() + .success(); + + // A member with NO membership on `grp` cannot write its vars. + let mem = common::member::member_user(fx, &common::unique_slug("gcw-mem")); + let menv = common::custom_env(&env.url, &mem.token); + common::seed_credentials(&menv, &mem.username); + + // A repo that declares the pre-existing `grp` (attached under `base`) with a + // var — a group content write. + let dir = TempDir::new().unwrap(); + fs::write( + dir.path().join("picloud.toml"), + format!( + "[project]\nslug = \"{project}\"\nparent_group = \"{base}\"\n\n\ + [group]\nslug = \"{grp}\"\nname = \"Grp\"\n\n[vars]\nregion = \"eu\"\n" + ), + ) + .unwrap(); + + let out = common::pic_as(&menv) + .args(["apply", "--dir"]) + .arg(dir.path()) + .output() + .expect("member apply"); + assert!( + !out.status.success(), + "a member without the group's write cap must be refused a content write" + ); + + // The var was NOT written (the tx rolled back / never ran). + let vars = String::from_utf8( + common::pic_as(&env) + .args(["vars", "ls", "--group", &grp]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!( + !vars.contains("region"), + "a denied content write must leave no var behind:\n{vars}" + ); +}