diff --git a/crates/manager-core/src/apply_api.rs b/crates/manager-core/src/apply_api.rs index bd81c9c..77a7530 100644 --- a/crates/manager-core/src/apply_api.rs +++ b/crates/manager-core/src/apply_api.rs @@ -395,16 +395,20 @@ async fn enforce_env_approval( .map_err(map_authz)?; } NodeKind::Group => { - if let Some(g) = svc - .groups - .get_by_slug(&node.slug) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))? - { - require(svc.authz.as_ref(), principal, Capability::GroupAdmin(g.id)) - .await - .map_err(map_authz)?; - } + // Resolve UUID-or-slug and FAIL CLOSED on miss — mirror + // `authz_tree`/`prepare_tree`. A bare `get_by_slug` skips this + // admin gate when the node addresses the group by its UUID (which + // `resolve_group`/`resolve_group_id` still resolve for the apply), + // letting a group editor apply to a confirm-required env without + // the admin authority M5 requires. + let group_id = resolve_group_id(svc.groups.as_ref(), &node.slug).await?; + require( + svc.authz.as_ref(), + principal, + Capability::GroupAdmin(group_id), + ) + .await + .map_err(map_authz)?; } } } diff --git a/crates/manager-core/src/apply_service.rs b/crates/manager-core/src/apply_service.rs index 3c5af40..a218d31 100644 --- a/crates/manager-core/src/apply_service.rs +++ b/crates/manager-core/src/apply_service.rs @@ -1658,12 +1658,26 @@ impl ApplyService { crate::group_repo::set_group_owner_tx(&mut tx, *gid, pid) .await .map_err(map_group_err)?; + // Audit (§7.4): seizing another project's group is at + // least as sensitive as the M5 gated-env override — record + // the actor + the ousted owner. + tracing::info!( + user_id = ?actor, + group = %slug, + prev_owner_key = %okey, + "apply: group ownership TAKEN OVER (audit)" + ); report.groups_taken_over += 1; } None => { crate::group_repo::set_group_owner_tx(&mut tx, *gid, pid) .await .map_err(map_group_err)?; + tracing::info!( + user_id = ?actor, + group = %slug, + "apply: group ownership CLAIMED (audit)" + ); report.groups_claimed += 1; } } @@ -2005,57 +2019,63 @@ impl ApplyService { token_parts.push(format!("proj|{}", envs.join(","))); } - // Ownership (§7, M3): resolve this repo's project, then classify each - // declared group node as ours / unclaimed / owned-by-another. Folding - // the owner key into the token means a claim or takeover by a different - // repo between plan and apply trips StateMoved. (Every group node on the - // tree-apply path pre-exists — `resolve_group` errors otherwise — so - // there is no to-create node to skip, unlike a create-capable engine.) - let our_project_id = match project_key { - Some(k) => crate::group_repo::get_project_id_by_key(&self.pool, k) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))?, - None => None, - }; + // Ownership (§7, M3): classify each declared group node as ours / + // unclaimed / owned-by-another, and fold the owner key into the token so + // a claim or takeover by a different repo between plan and apply trips + // StateMoved. Gated on `project_key.is_some()` — a legacy caller with NO + // project key skips ALL ownership (no conflicts, no prune, no owner token + // fold), matching the "optional for legacy callers" contract. Note the + // discriminator is a *key present*, NOT a *persisted project*: a + // first-ever apply has a key but no `projects` row yet (our_project_id == + // None), and MUST still see an already-owned group as a conflict. (Every + // group node pre-exists — `resolve_group` errors otherwise.) let mut existing_group_nodes: Vec<(GroupId, String)> = Vec::new(); - let mut declared_group_slugs: HashSet = HashSet::new(); let mut conflicts: Vec = Vec::new(); - for n in &bundle.nodes { - if n.kind != NodeKind::Group { - continue; - } - let lc = n.slug.to_lowercase(); - declared_group_slugs.insert(lc.clone()); - let gid = group_id_by_slug[&lc]; - existing_group_nodes.push((gid, n.slug.clone())); - let owner = crate::group_repo::get_group_owner(&self.pool, gid) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))?; - match &owner { - Some((_, key)) => token_parts.push(format!("own|{lc}|{key}")), - None => token_parts.push(format!("own|{lc}|none")), - } - if let Some((oid, okey)) = owner { - if Some(oid) != our_project_id { - conflicts.push(OwnershipConflict { - slug: n.slug.clone(), - owner_key: okey, - }); + let mut prune_candidates: Vec<(GroupId, String)> = Vec::new(); + if project_key.is_some() { + let our_project_id = match project_key { + Some(k) => crate::group_repo::get_project_id_by_key(&self.pool, k) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?, + None => None, + }; + let mut declared_group_slugs: HashSet = HashSet::new(); + for n in &bundle.nodes { + if n.kind != NodeKind::Group { + continue; + } + let lc = n.slug.to_lowercase(); + declared_group_slugs.insert(lc.clone()); + let gid = group_id_by_slug[&lc]; + existing_group_nodes.push((gid, n.slug.clone())); + let owner = crate::group_repo::get_group_owner(&self.pool, gid) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))?; + match &owner { + Some((_, key)) => token_parts.push(format!("own|{lc}|{key}")), + None => token_parts.push(format!("own|{lc}|none")), + } + if let Some((oid, okey)) = owner { + if Some(oid) != our_project_id { + conflicts.push(OwnershipConflict { + slug: n.slug.clone(), + owner_key: okey, + }); + } } } - } - // Structural-prune candidates: groups THIS project owns that the - // manifest omits. Computed read-only here (so `plan` can list them); - // `apply --prune` re-derives the set in-tx and deletes leaf-first. - let mut prune_candidates: Vec<(GroupId, String)> = Vec::new(); - if let Some(pid) = our_project_id { - for (gid, slug) in crate::group_repo::list_owned_groups(&self.pool, pid) - .await - .map_err(|e| ApplyError::Backend(e.to_string()))? - { - if !declared_group_slugs.contains(&slug.to_lowercase()) { - prune_candidates.push((gid, slug)); + // Structural-prune candidates: groups THIS project owns that the + // manifest omits. Computed read-only here (so `plan` can list them); + // `apply --prune` re-derives the set in-tx and deletes leaf-first. + if let Some(pid) = our_project_id { + for (gid, slug) in crate::group_repo::list_owned_groups(&self.pool, pid) + .await + .map_err(|e| ApplyError::Backend(e.to_string()))? + { + if !declared_group_slugs.contains(&slug.to_lowercase()) { + prune_candidates.push((gid, slug)); + } } } } diff --git a/crates/manager-core/src/group_repo.rs b/crates/manager-core/src/group_repo.rs index f438991..99fb2ac 100644 --- a/crates/manager-core/src/group_repo.rs +++ b/crates/manager-core/src/group_repo.rs @@ -469,21 +469,29 @@ pub(crate) async fn list_owned_groups_tx( } /// Does this group hold data a structural prune would SILENTLY cascade away -/// (§7 M3 safety guard)? The `apps`/child-group FKs are RESTRICT (delete aborts -/// loudly), but the §11.6 group-owned surfaces are `ON DELETE CASCADE`: shared -/// collections (`group_collections`, covering kv/docs/files/topic/queue) and -/// group secrets (owner-polymorphic `secrets.group_id`). Rather than let -/// `apply --prune` vaporize a tenant's shared data + secrets because a manifest -/// node was dropped, the prune skips such a group with a warning — the operator -/// must delete it explicitly. Returns true if the group has any shared-collection -/// marker or any secret. +/// (§7 M3 safety guard)? The `apps`/child-group/group-script FKs are RESTRICT +/// (delete aborts loudly), but the §11.6 group-owned data surfaces are +/// `ON DELETE CASCADE`. Rather than let `apply --prune` vaporize a tenant's +/// shared data + secrets because a manifest node was dropped, the prune skips +/// such a group with a warning — the operator must delete it explicitly. +/// +/// We check the actual DATA rows, not just the collection MARKER: un-declaring a +/// `collections = [...]` entry deletes only the `group_collections` marker while +/// the `group_kv_entries`/`group_docs`/`group_files`/`group_queue_messages` rows +/// survive (they're reaped only on group delete). Checking markers alone would +/// miss that orphaned-but-live data and cascade it away. Returns true if the +/// group has any shared-collection marker, any secret, or any shared data row. pub(crate) async fn group_has_protected_data_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, group_id: GroupId, ) -> Result { let row: (bool,) = sqlx::query_as( - "SELECT EXISTS (SELECT 1 FROM group_collections WHERE group_id = $1) \ - OR EXISTS (SELECT 1 FROM secrets WHERE group_id = $1)", + "SELECT EXISTS (SELECT 1 FROM group_collections WHERE group_id = $1) \ + OR EXISTS (SELECT 1 FROM secrets WHERE group_id = $1) \ + OR EXISTS (SELECT 1 FROM group_kv_entries WHERE group_id = $1) \ + OR EXISTS (SELECT 1 FROM group_docs WHERE group_id = $1) \ + OR EXISTS (SELECT 1 FROM group_files WHERE group_id = $1) \ + OR EXISTS (SELECT 1 FROM group_queue_messages WHERE group_id = $1)", ) .bind(group_id.into_inner()) .fetch_one(&mut **tx) diff --git a/crates/picloud-cli/src/cmds/plan.rs b/crates/picloud-cli/src/cmds/plan.rs index 131b9f1..e98f962 100644 --- a/crates/picloud-cli/src/cmds/plan.rs +++ b/crates/picloud-cli/src/cmds/plan.rs @@ -50,8 +50,13 @@ pub async fn run_tree(dir: &Path, env: Option<&str>, mode: OutputMode) -> Result let creds = config::resolve()?; let client = Client::from_creds(&creds)?; let (bundle, _count, _envs) = crate::discover::build_tree(dir, env)?; - // Mint/read this repo's stable project key (§7, M3) so the plan can surface - // ownership conflicts + structural-prune candidates for us. + // Mint/read this repo's project key (§7, M3) so the plan can surface + // ownership conflicts + prune candidates. `plan` DOES mint it (a rare + // write for an otherwise read-only command) rather than read-only-peek, + // because the bound-plan token folds the ownership state gated on a key + // being present: a keyless plan and a keyed apply would fold DIFFERENT + // token parts and trip a spurious `StateMoved`. Minting here keeps plan and + // apply keyed identically. The key is local + gitignored + idempotent. let project_key = crate::linkstate::ensure_project_key(dir)?; let plan = client.plan_tree(&bundle, Some(&project_key)).await?; if !plan.state_token.is_empty() { diff --git a/crates/picloud-cli/tests/approval.rs b/crates/picloud-cli/tests/approval.rs index 6850bff..880027d 100644 --- a/crates/picloud-cli/tests/approval.rs +++ b/crates/picloud-cli/tests/approval.rs @@ -165,3 +165,76 @@ fn approving_a_gated_apply_requires_admin() { "approval denial should be an authz error:\n{err}" ); } + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn gated_group_node_admin_gate_is_not_bypassable_by_uuid_slug() { + // Regression (§4.2): `enforce_env_approval` must resolve a group node by + // UUID-or-slug and FAIL CLOSED — a bare `get_by_slug` skipped the GroupAdmin + // gate when the node addressed the group by its UUID (which the apply still + // resolves), letting a group EDITOR apply to a confirm-required env without + // admin. We drive the raw `/tree/apply` wire directly (the CLI only ever + // sends slugs) with the group node's UUID as its slug. + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let group = common::unique_slug("appr3-g"); + let _g = GroupGuard::new(&env.url, &env.token, &group); + common::pic_as(&env) + .args(["groups", "create", &group]) + .assert() + .success(); + + // Resolve the group's UUID. + let http = reqwest::blocking::Client::new(); + let detail: serde_json::Value = http + .get(format!("{}/api/v1/admin/groups/{}", env.url, group)) + .bearer_auth(&env.token) + .send() + .expect("get group") + .json() + .expect("group json"); + let group_uuid = detail["id"].as_str().expect("group id").to_string(); + + // A group `editor` — write caps, but NOT GroupAdmin. + let m = member::member_user(fx, &common::unique_username("appr3")); + common::pic_as(&env) + .args([ + "groups", "members", "add", &group, &m.id, "--role", "editor", + ]) + .assert() + .success(); + + // A gated bundle whose single group node is addressed by UUID. A `vars` + // entry makes the node non-empty (and exercises the editor's write cap). + let body = serde_json::json!({ + "bundle": { + "nodes": [{ + "kind": "group", + "slug": group_uuid, + "bundle": { "vars": { "region": "eu" } } + }], + "project": { "environments": [{ "name": "production", "confirm": true }] } + }, + "prune": false, + "env": "production", + "approved_envs": ["production"], + "allow_takeover": false, + }); + + let resp = http + .post(format!("{}/api/v1/admin/tree/apply", env.url)) + .bearer_auth(&m.token) + .json(&body) + .send() + .expect("tree apply"); + assert_eq!( + resp.status().as_u16(), + 403, + "a group editor must be 403'd approving a gated apply even when the node \ + is addressed by UUID (got {}: {})", + resp.status(), + resp.text().unwrap_or_default() + ); +} diff --git a/crates/picloud-cli/tests/ownership.rs b/crates/picloud-cli/tests/ownership.rs index 177220e..f4be0a4 100644 --- a/crates/picloud-cli/tests/ownership.rs +++ b/crates/picloud-cli/tests/ownership.rs @@ -286,3 +286,47 @@ fn prune_refuses_to_cascade_a_groups_shared_data() { "a group owning shared collections must NOT be pruned" ); } + +#[ignore = "needs DATABASE_URL pointing at a running Postgres"] +#[test] +fn legacy_apply_without_project_key_ignores_ownership() { + // Regression (§7): a tree apply with NO project key (legacy / direct-API + // caller) must skip ALL ownership — no conflicts, no claims. A bug gated the + // conflict classification on `our_project_id` (None for a keyless caller), + // so ANY already-owned declared group was spuriously rejected. Drive the raw + // wire without `project_key` (the CLI always sends one). + let Some(fx) = common::fixture_or_skip() else { + return; + }; + let env = common::admin_env(fx); + let slug = common::unique_slug("own-legacy"); + let _g = GroupGuard::new(&env.url, &env.token, &slug); + create_group(&env, &slug, None); + + // Repo A claims the group (a normal CLI apply — sends a project key). + let repo_a = group_dir(&slug, ""); + assert!( + apply(&env, repo_a.path(), &[]).status.success(), + "claim apply should succeed" + ); + + // A keyless raw apply of the same (now-owned) group must NOT 409 — ownership + // is simply not evaluated without a project key. + let http = reqwest::blocking::Client::new(); + let body = serde_json::json!({ + "bundle": { "nodes": [{ "kind": "group", "slug": slug, "bundle": {} }] }, + "prune": false, + }); + let resp = http + .post(format!("{}/api/v1/admin/tree/apply", env.url)) + .bearer_auth(&env.token) + .json(&body) + .send() + .expect("tree apply"); + assert!( + resp.status().is_success(), + "a keyless (legacy) apply must ignore ownership, not conflict (got {}: {})", + resp.status(), + resp.text().unwrap_or_default() + ); +}