diff --git a/hive-c0re/src/topology.rs b/hive-c0re/src/topology.rs index 0a120ee3..662c7586 100644 --- a/hive-c0re/src/topology.rs +++ b/hive-c0re/src/topology.rs @@ -344,6 +344,11 @@ pub fn has_role_in(roles: &BTreeMap>, name: &str, role: &str /// Grant or revoke a role for `name`. Idempotent — no disk write when the /// state is already correct. +/// +/// Empty role lists are kept in the map (never removed). An absent key means +/// "never seen" (seed on next `reconcile_roles`); an empty list means +/// "explicitly revoked" (do not re-seed). Callers that want to remove an +/// agent from the map entirely should use `reconcile_roles` (agent departure). pub fn set_role(name: &str, role: &str, enabled: bool) -> Result<(), String> { let mut roles = read_roles(); let list = roles.entry(name.to_owned()).or_default(); @@ -353,9 +358,8 @@ pub fn set_role(name: &str, role: &str, enabled: bool) -> Result<(), String> { (false, true) => list.retain(|r| r != role), _ => return Ok(()), } - if roles.get(name).is_some_and(|l| l.is_empty()) { - roles.remove(name); - } + // Intentionally do NOT remove empty entries — an empty list signals an + // explicit revoke and prevents reconcile_roles from re-seeding the role. write_roles(&roles).map_err(|e| format!("write roles.json: {e}")) } @@ -618,4 +622,80 @@ mod tests { let topo = topo_three_level(); assert!(children_of_in(&topo, "nobody").is_empty()); } + + // ----------------------------------------------------------------------- + // Roles tests (no disk I/O — use the pure `has_role_in` / in-memory maps) + // ----------------------------------------------------------------------- + + #[test] + fn has_role_in_returns_true_when_role_held() { + let mut roles = BTreeMap::new(); + roles.insert( + "alice".to_owned(), + vec![ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS.to_owned()], + ); + assert!(has_role_in(&roles, "alice", ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS)); + } + + #[test] + fn has_role_in_returns_false_for_absent_agent() { + let roles: BTreeMap> = BTreeMap::new(); + assert!(!has_role_in(&roles, "alice", ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS)); + } + + #[test] + fn has_role_in_returns_false_for_empty_list() { + let mut roles = BTreeMap::new(); + roles.insert("alice".to_owned(), vec![]); + assert!(!has_role_in(&roles, "alice", ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS)); + } + + /// Revoking a role must leave the key present with an empty list so + /// `reconcile_roles` does not re-seed it. + #[test] + fn set_role_revoke_keeps_empty_entry_as_tombstone() { + // Build an in-memory roles map as set_role would see it after granting. + let mut roles: BTreeMap> = BTreeMap::new(); + roles.insert( + "root".to_owned(), + vec![ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS.to_owned()], + ); + + // Simulate the revoke path of set_role (in-memory, no disk). + let list = roles.entry("root".to_owned()).or_default(); + let held = list.iter().any(|r| r == ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS); + assert!(held); + list.retain(|r| r != ROLE_CAN_MANAGE_TOP_LEVEL_AGENTS); + + // Key must still be present (tombstone), just with an empty list. + assert!(roles.contains_key("root"), "empty entry must not be removed"); + assert!(roles["root"].is_empty()); + } + + /// `reconcile_roles` must not re-seed root when its entry exists but is + /// empty (operator explicitly revoked the role). + #[test] + fn reconcile_roles_in_does_not_reseed_after_explicit_revoke() { + let agent_names = vec!["root".to_owned(), "alice".to_owned()]; + let mut roles: BTreeMap> = BTreeMap::new(); + // Tombstone: root was seen before but all roles were revoked. + roles.insert("root".to_owned(), vec![]); + + let root = crate::lifecycle::MANAGER_NAME; + let root_present = agent_names.iter().any(|n| n == root); + let should_seed = root_present && !roles.contains_key(root); + // should_seed must be false because "root" key is present (tombstone). + assert!(!should_seed, "reconcile_roles must not re-seed an explicit revoke"); + } + + /// `reconcile_roles` seeds root on first appearance (no prior entry). + #[test] + fn reconcile_roles_in_seeds_root_when_absent() { + let agent_names = vec!["root".to_owned(), "alice".to_owned()]; + let roles: BTreeMap> = BTreeMap::new(); // empty + + let root = crate::lifecycle::MANAGER_NAME; + let should_seed = agent_names.iter().any(|n| n == root) && !roles.contains_key(root); + assert!(should_seed, "reconcile_roles must seed root when absent"); + } }