Merge pull request #178 from dDevAhmed/feature/issue-130-safe-role-revocation

mijinummi · web-flow · commit 6e2cae047ac0 · 2026-03-30T03:19:11.000+01:00
Implement safe role revocation for property token
diff --git a/apps/rust/property-token/src/lib.rs b/apps/rust/property-token/src/lib.rs
@@ -6,6 +6,7 @@ use cosmwasm_std::{
     entry_point, to_json_binary, Binary, Deps, DepsMut, Env, MessageInfo, Order,
     Response, StdError, StdResult, Uint128, BankMsg, Coin,
 };
+use std::collections::BTreeSet;
 use crate::msg::{
     Auth, BatchMsg, ExecuteMsg, InstantiateMsg, MigrateMsg,
     PropertyMetadata, QueryMsg, SelfCheckResponse, TreasuryAction,
@@ -16,6 +17,28 @@ use crate::state::{
 };
 use crate::security::{ensure_authorized, prevent_replay};
 
+enum RoleMutation {
+    Grant(String),
+    Revoke(String),
+}
+
+fn validate_principal(label: &str, principal: &str) -> StdResult<String> {
+    let normalized = principal.trim();
+    if normalized.is_empty() {
+        return Err(StdError::generic_err(format!("{} cannot be empty", label)));
+    }
+
+    Ok(normalized.to_string())
+}
+
+fn format_audit_list(entries: &[String]) -> String {
+    if entries.is_empty() {
+        "none".to_string()
+    } else {
+        entries.join(",")
+    }
+}
+
 // ── Entry points ─────────────────────────────────────────────────────────────
 
 #[entry_point]
@@ -271,26 +294,95 @@ pub fn execute_update_config(
     new_admin: Option<String>,
     authorized_roles: Option<Vec<(String, bool)>>,
 ) -> StdResult<Response> {
-    let admin = ADMIN.load(deps.storage)?;
-    if info.sender.as_str() != admin {
+    let current_admin = ADMIN.load(deps.storage)?;
+    if info.sender.as_str() != current_admin {
         return Err(StdError::generic_err("Only the current admin can update config"));
     }
-    if let Some(addr) = new_admin {
-        ADMIN.save(deps.storage, &addr)?;
-    }
+
+    let next_admin = new_admin
+        .as_deref()
+        .map(|addr| validate_principal("new_admin", addr))
+        .transpose()?;
+
+    let mut seen_roles = BTreeSet::new();
+    let mut role_mutations: Vec<RoleMutation> = Vec::new();
+
     if let Some(roles) = authorized_roles {
         for (addr, is_auth) in roles {
-            AUTHORIZED_ROLES.save(deps.storage, &addr, &is_auth)?;
+            let normalized = validate_principal("authorized role address", &addr)?;
+            if !seen_roles.insert(normalized.clone()) {
+                return Err(StdError::generic_err(format!(
+                    "Duplicate role update requested for {}",
+                    normalized
+                )));
+            }
+
+            let existing_state = AUTHORIZED_ROLES.may_load(deps.storage, &normalized)?;
+            match (existing_state, is_auth) {
+                (Some(true), true) => {}
+                (Some(false), true) | (None, true) => {
+                    role_mutations.push(RoleMutation::Grant(normalized));
+                }
+                (Some(true), false) | (Some(false), false) => {
+                    role_mutations.push(RoleMutation::Revoke(normalized));
+                }
+                (None, false) => {
+                    return Err(StdError::generic_err(format!(
+                        "Cannot revoke role for {} because no active permission exists",
+                        normalized
+                    )));
+                }
+            }
+        }
+    }
+
+    let mut response = Response::new()
+        .add_attribute("action", "update_config")
+        .add_attribute("actor", info.sender.as_str());
+
+    if let Some(addr) = next_admin {
+        let admin_changed = addr != current_admin;
+        if admin_changed {
+            ADMIN.save(deps.storage, &addr)?;
         }
+        response = response
+            .add_attribute("admin_changed", admin_changed.to_string())
+            .add_attribute("previous_admin", current_admin.clone())
+            .add_attribute("current_admin", addr);
+    } else {
+        response = response
+            .add_attribute("admin_changed", "false")
+            .add_attribute("current_admin", current_admin.clone());
     }
-    Ok(Response::new().add_attribute("action", "update_config"))
+
+    let mut granted_roles: Vec<String> = Vec::new();
+    let mut revoked_roles: Vec<String> = Vec::new();
+
+    for mutation in role_mutations {
+        match mutation {
+            RoleMutation::Grant(addr) => {
+                AUTHORIZED_ROLES.save(deps.storage, &addr, &true)?;
+                granted_roles.push(addr);
+            }
+            RoleMutation::Revoke(addr) => {
+                AUTHORIZED_ROLES.remove(deps.storage, &addr);
+                revoked_roles.push(addr);
+            }
+        }
+    }
+
+    Ok(response
+        .add_attribute("roles_granted_count", granted_roles.len().to_string())
+        .add_attribute("roles_revoked_count", revoked_roles.len().to_string())
+        .add_attribute("roles_granted", format_audit_list(&granted_roles))
+        .add_attribute("roles_revoked", format_audit_list(&revoked_roles)))
 }
 
 /// #136 — Storage Cleanup Utility
 ///
 /// Removes:
 /// - Zero-balance entries from `FEE_BALANCES` (saves storage rent)
-/// - Explicitly revoked role entries (value == false) from `AUTHORIZED_ROLES`
+/// - Legacy explicitly-revoked role entries (value == false) from `AUTHORIZED_ROLES`
 ///
 /// Admin-only. Does not affect treasury, metadata, or nonce state.
 pub fn execute_cleanup_storage(
@@ -316,7 +408,7 @@ pub fn execute_cleanup_storage(
         FEE_BALANCES.remove(deps.storage, key.as_str());
     }
 
-    // Collect revoked role keys (explicitly set to false — dead entries)
+    // Collect legacy revoked role keys (explicitly set to false — dead entries)
     let revoked_role_keys: Vec<String> = AUTHORIZED_ROLES
         .range(deps.storage, None, None, Order::Ascending)
         .filter_map(|item| {
@@ -400,7 +492,7 @@ pub fn query_self_check(deps: Deps) -> StdResult<SelfCheckResponse> {
         ));
     }
 
-    // 5. No explicitly-revoked role entries should remain (they waste storage)
+    // 5. No legacy explicitly-revoked role entries should remain (they waste storage)
     let revoked_role_count = AUTHORIZED_ROLES
         .range(deps.storage, None, None, Order::Ascending)
         .filter_map(|item| item.ok())
@@ -409,7 +501,7 @@ pub fn query_self_check(deps: Deps) -> StdResult<SelfCheckResponse> {
 
     if revoked_role_count > 0 {
         failures.push(format!(
-            "{} authorized_roles entries are explicitly false (consider running CleanupStorage)",
+            "{} authorized_roles entries use the legacy false tombstone format (consider running CleanupStorage)",
             revoked_role_count
         ));
     } else {
diff --git a/apps/rust/property-token/src/msg.rs b/apps/rust/property-token/src/msg.rs
@@ -55,7 +55,10 @@ pub enum ExecuteMsg {
 
     // Role-based Access Control
     UpdateConfig {
+        /// Replaces the current admin when provided. Empty values are rejected.
         new_admin: Option<String>,
+        /// Backward-compatible permission updates.
+        /// `true` grants a role; `false` revokes it by removing the map entry.
         authorized_roles: Option<Vec<(String, bool)>>,
     },
 
diff --git a/apps/rust/property-token/tests/rbac_revocation_tests.rs b/apps/rust/property-token/tests/rbac_revocation_tests.rs
@@ -0,0 +1,176 @@
+use cosmwasm_std::testing::{mock_dependencies, mock_env, mock_info};
+use property_token::{
+    execute_update_config,
+    instantiate,
+    query_self_check,
+    msg::InstantiateMsg,
+    state::AUTHORIZED_ROLES,
+};
+
+fn attr_value(response: &cosmwasm_std::Response, key: &str) -> Option<String> {
+    response
+        .attributes
+        .iter()
+        .find(|attr| attr.key == key)
+        .map(|attr| attr.value.clone())
+}
+
+#[test]
+fn revoking_a_role_removes_the_storage_entry_and_emits_audit_attributes() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+    AUTHORIZED_ROLES
+        .save(deps.as_mut().storage, "operator", &true)
+        .unwrap();
+
+    let response = execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        None,
+        Some(vec![("operator".to_string(), false)]),
+    )
+    .unwrap();
+
+    assert_eq!(AUTHORIZED_ROLES.may_load(deps.as_ref().storage, "operator").unwrap(), None);
+    assert_eq!(attr_value(&response, "roles_revoked_count").as_deref(), Some("1"));
+    assert_eq!(attr_value(&response, "roles_revoked").as_deref(), Some("operator"));
+    assert_eq!(attr_value(&response, "actor").as_deref(), Some("admin"));
+}
+
+#[test]
+fn revoking_a_missing_role_is_rejected() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+
+    let err = execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        None,
+        Some(vec![("ghost".to_string(), false)]),
+    )
+    .unwrap_err()
+    .to_string();
+
+    assert!(err.contains("Cannot revoke role for ghost"), "unexpected error: {}", err);
+}
+
+#[test]
+fn legacy_false_role_entries_are_cleaned_by_revocation_and_self_check_passes() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+    AUTHORIZED_ROLES
+        .save(deps.as_mut().storage, "legacy-role", &false)
+        .unwrap();
+
+    execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        None,
+        Some(vec![("legacy-role".to_string(), false)]),
+    )
+    .unwrap();
+
+    let report = query_self_check(deps.as_ref()).unwrap();
+    assert!(report.ok, "expected self check to pass, got failures: {:?}", report.failures);
+}
+
+#[test]
+fn unauthorized_callers_cannot_revoke_roles() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+    AUTHORIZED_ROLES
+        .save(deps.as_mut().storage, "operator", &true)
+        .unwrap();
+
+    let err = execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("attacker", &[]),
+        None,
+        Some(vec![("operator".to_string(), false)]),
+    )
+    .unwrap_err()
+    .to_string();
+
+    assert!(err.contains("Only the current admin can update config"), "unexpected error: {}", err);
+    assert_eq!(AUTHORIZED_ROLES.may_load(deps.as_ref().storage, "operator").unwrap(), Some(true));
+}
+
+#[test]
+fn empty_admin_updates_are_rejected_to_preserve_admin_control() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+
+    let err = execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        Some("   ".to_string()),
+        None,
+    )
+    .unwrap_err()
+    .to_string();
+
+    assert!(err.contains("new_admin cannot be empty"), "unexpected error: {}", err);
+}
+
+#[test]
+fn duplicate_role_updates_are_rejected_before_state_changes() {
+    let mut deps = mock_dependencies();
+    instantiate(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        InstantiateMsg { admin: None },
+    )
+    .unwrap();
+
+    let err = execute_update_config(
+        deps.as_mut(),
+        mock_env(),
+        mock_info("admin", &[]),
+        None,
+        Some(vec![
+            ("operator".to_string(), true),
+            ("operator".to_string(), false),
+        ]),
+    )
+    .unwrap_err()
+    .to_string();
+
+    assert!(err.contains("Duplicate role update requested for operator"), "unexpected error: {}", err);
+    assert_eq!(AUTHORIZED_ROLES.may_load(deps.as_ref().storage, "operator").unwrap(), None);
+}
diff --git a/docs/ROLE_REVOCATION_RULES.md b/docs/ROLE_REVOCATION_RULES.md
@@ -0,0 +1,38 @@
+# Role Revocation Rules
+
+This document describes the role revocation behavior for the Soroban property-token contract.
+
+## Scope
+
+The contract maintains two permission layers:
+
+- `ADMIN`: the single administrative authority stored as a dedicated state item
+- `AUTHORIZED_ROLES`: a map of additional addresses that may perform privileged actions
+
+## Revocation Rules
+
+1. Only the current `ADMIN` may change permissions.
+2. Revoking a role removes the address from `AUTHORIZED_ROLES` entirely.
+3. The contract rejects revocation requests for addresses that were never granted a role.
+4. Legacy `AUTHORIZED_ROLES[address] = false` tombstones are treated as revocable stale state and are removed when revoked again.
+5. The admin role cannot be cleared. Admin control can only change through an explicit `new_admin` replacement value.
+6. Empty or whitespace-only principals are rejected for both role updates and admin changes.
+7. Duplicate updates for the same address in a single config change are rejected to avoid ambiguous final state.
+
+## Auditability
+
+Every successful `UpdateConfig` execution emits attributes for:
+
+- the actor performing the change
+- whether the admin changed
+- the current admin after the change
+- granted role count and address list
+- revoked role count and address list
+
+These attributes provide an on-chain audit trail for permission changes.
+
+## Operational Notes
+
+- `CleanupStorage` remains available to remove legacy `false` tombstones that may still exist from older deployments.
+- `SelfCheck` reports legacy tombstones as a failure until they are removed.
+- Revoking an address from `AUTHORIZED_ROLES` does not clear the dedicated `ADMIN` value. Admin transfer must be done explicitly.
