diff --git a/contracts/access_control/src/access_control.rs b/contracts/access_control/src/access_control.rs index 4933e85..d6b891f 100644 --- a/contracts/access_control/src/access_control.rs +++ b/contracts/access_control/src/access_control.rs @@ -964,6 +964,75 @@ impl AccessControlModule { ); } } + ProposalAction::TransferAdmin(new_admin) => { + // Transfer admin ownership via multisig proposal. + if Self::is_multisig_enabled(env) { + // In multisig mode, update the admins list: add new_admin + // and remove the proposer from the multisig admin list. + let mut multisig_config = Self::get_multisig_config(env) + .ok_or(AccessControlError::MultisigNotEnabled)?; + + if multisig_config.admins.contains(&new_admin) { + return Err(AccessControlError::DuplicateAdmin); + } + + // Rebuild the admins list without the proposer + let mut new_admins = Vec::new(env); + for admin in multisig_config.admins.iter() { + if admin != proposal.proposer { + new_admins.push_back(admin); + } + } + new_admins.push_back(new_admin.clone()); + multisig_config.admins = new_admins; + + if !multisig_config.validate() { + return Err(AccessControlError::InvalidMultisigConfig); + } + + env.storage() + .persistent() + .set(&DataKey::MultiSigConfig, &multisig_config); + + // Update roles + env.storage() + .persistent() + .set(&DataKey::UserRole(new_admin.clone()), &UserRole::Admin); + env.storage() + .persistent() + .set( + &DataKey::UserRole(proposal.proposer.clone()), + &UserRole::Guest, + ); + } else { + // Single-admin mode: transfer ownership via DataKey::Admin + let current_admin = + Self::get_admin(env).ok_or(AccessControlError::AdminRequired)?; + + if current_admin == new_admin { + return Err(AccessControlError::InvalidAddress); + } + + // Update the admin address + env.storage().persistent().set(&DataKey::Admin, &new_admin); + + // Update roles: new admin gets Admin, old admin gets Guest + env.storage() + .persistent() + .set(&DataKey::UserRole(new_admin.clone()), &UserRole::Admin); + env.storage() + .persistent() + .set( + &DataKey::UserRole(current_admin.clone()), + &UserRole::Guest, + ); + } + + env.events().publish( + (symbol_short!("adm_xfer"), new_admin.clone()), + (proposal.proposer.clone(), new_admin.clone()), + ); + } _ => return Err(AccessControlError::InvalidProposalType), } diff --git a/contracts/access_control/src/access_control_role_tests.rs b/contracts/access_control/src/access_control_role_tests.rs index 79bd790..702f757 100644 --- a/contracts/access_control/src/access_control_role_tests.rs +++ b/contracts/access_control/src/access_control_role_tests.rs @@ -1,32 +1,481 @@ #[cfg(test)] mod role_access_control_tests { + use crate::access_control::AccessControlModule; + use crate::errors::AccessControlError; + use crate::types::{AccessControlConfig, UserRole}; + use soroban_sdk::{ + testutils::{Address as _, Ledger, LedgerInfo}, + Address, Env, Vec, + }; + + fn setup_initialized_env() -> (Env, Address, Address, Address, Address) { + let env = Env::default(); + let contract_id = env.register(crate::AccessControl, ()); + let admin = Address::generate(&env); + let user1 = Address::generate(&env); + let user2 = Address::generate(&env); + env.as_contract(&contract_id, || { + AccessControlModule::initialize(&env, admin.clone(), None).unwrap(); + }); + (env, contract_id, admin, user1, user2) + } + + /// Admin should be able to perform all admin-only operations. #[test] fn test_admin_can_create_resource() { - assert!(true); + let (env, contract_id, admin, user1, _) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // Admin can set roles + let result = AccessControlModule::set_role( + &env, + admin.clone(), + user1.clone(), + UserRole::Member, + ); + assert!(result.is_ok(), "Admin should be able to set roles"); + assert_eq!( + AccessControlModule::get_role(&env, user1.clone()), + UserRole::Member + ); + + // Admin can blacklist users + let result = AccessControlModule::blacklist_user(&env, admin.clone(), user1.clone()); + assert!(result.is_ok(), "Admin should be able to blacklist users"); + + // Admin can pause/unpause + let result = AccessControlModule::pause(&env, admin.clone()); + assert!(result.is_ok(), "Admin should be able to pause the contract"); + assert!(AccessControlModule::is_paused(&env)); + + let result = AccessControlModule::unpause(&env, admin.clone()); + assert!(result.is_ok(), "Admin should be able to unpause the contract"); + + // Admin can update config + let result = AccessControlModule::update_config( + &env, + admin.clone(), + AccessControlConfig::default(), + ); + assert!(result.is_ok(), "Admin should be able to update config"); + + // Admin can remove roles + let result = AccessControlModule::remove_role(&env, admin.clone(), user1.clone()); + assert!(result.is_ok(), "Admin should be able to remove roles"); + }); } + /// Non-admin users should not be able to perform admin-only operations. + /// Permission failures must produce clear, specific errors. #[test] fn test_user_cannot_create_resource() { - assert!(true); + let (env, contract_id, admin, user1, user2) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // Guest cannot set roles + let result = AccessControlModule::set_role( + &env, + user1.clone(), + user2.clone(), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Non-admin should get AdminRequired error when setting roles" + ); + + // Guest cannot blacklist + let result = AccessControlModule::blacklist_user(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Non-admin should get AdminRequired error when blacklisting" + ); + + // Guest cannot pause + let result = AccessControlModule::pause(&env, user1.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Non-admin should get AdminRequired error when pausing" + ); + + // Guest cannot remove roles + let result = AccessControlModule::remove_role(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Non-admin should get AdminRequired error when removing roles" + ); + + // Member also cannot perform admin operations + AccessControlModule::set_role( + &env, + admin.clone(), + user1.clone(), + UserRole::Member, + ) + .unwrap(); + + let result = AccessControlModule::pause(&env, user1.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Members should not be able to pause" + ); + + // Member cannot set other users' roles + let result = AccessControlModule::set_role( + &env, + user1.clone(), + user2.clone(), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Members should not be able to assign roles" + ); + }); } + /// Unauthorized access attempts must produce clear, specific error messages. #[test] fn test_unauthorized_access_denied() { - assert!(true); + let (env, contract_id, admin, user1, _) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // Blacklisted users should get Unauthorized error + AccessControlModule::blacklist_user(&env, admin.clone(), user1.clone()).unwrap(); + let result = AccessControlModule::set_role( + &env, + admin.clone(), + user1.clone(), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::Unauthorized, + "Blacklisted user should get Unauthorized error" + ); + + // Insufficient role access should produce InsufficientRole error + AccessControlModule::unblacklist_user(&env, admin.clone(), user1.clone()).unwrap(); + let result = + AccessControlModule::require_access(&env, user1.clone(), UserRole::Admin); + assert_eq!( + result.unwrap_err(), + AccessControlError::InsufficientRole, + "User without admin role should get InsufficientRole error" + ); + + // Operations on uninitialized system should produce NotInitialized error + let env2 = Env::default(); + let contract_id2 = env2.register(crate::AccessControl, ()); + env2.as_contract(&contract_id2, || { + let result = AccessControlModule::set_role( + &env2, + Address::generate(&env2), + Address::generate(&env2), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::NotInitialized, + "Uninitialized system should get NotInitialized error" + ); + }); + + // Paused contract should produce ContractPaused error + AccessControlModule::pause(&env, admin.clone()).unwrap(); + let result = AccessControlModule::set_role( + &env, + admin.clone(), + Address::generate(&env), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::ContractPaused, + "Paused contract should get ContractPaused error" + ); + }); } + /// Role escalation must be prevented (e.g., member can't promote themselves to admin). #[test] fn test_role_escalation_prevented() { - assert!(true); + let (env, contract_id, admin, user1, _) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // Set user1 as Member + AccessControlModule::set_role( + &env, + admin.clone(), + user1.clone(), + UserRole::Member, + ) + .unwrap(); + + // Member cannot assign Admin role to anyone + let result = AccessControlModule::set_role( + &env, + user1.clone(), + Address::generate(&env), + UserRole::Admin, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Members should not be able to assign Admin role" + ); + + // Member cannot promote themselves + let result = AccessControlModule::set_role( + &env, + user1.clone(), + user1.clone(), + UserRole::Admin, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Members should not be able to self-promote to Admin" + ); + + // Admin cannot remove the main admin's role + let result = AccessControlModule::remove_role(&env, admin.clone(), admin.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::RoleHierarchyViolation, + "Admin cannot remove their own admin role" + ); + + // Old admin cannot re-assign roles after transfer + AccessControlModule::propose_admin_transfer( + &env, + admin.clone(), + user1.clone(), + ) + .unwrap(); + AccessControlModule::accept_admin_transfer(&env, user1.clone()).unwrap(); + + let result = AccessControlModule::set_role( + &env, + admin.clone(), + Address::generate(&env), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Former admin should no longer have admin privileges after transfer" + ); + }); } + /// Role hierarchy (Admin > Member > Guest) must be enforced for all access checks. #[test] fn test_permission_inheritance() { - assert!(true); + let (env, contract_id, admin, user1, _) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // Set user1 as Member + AccessControlModule::set_role( + &env, + admin.clone(), + user1.clone(), + UserRole::Member, + ) + .unwrap(); + + // Member inherits Guest permissions + assert!( + AccessControlModule::check_access(&env, user1.clone(), UserRole::Guest) + .unwrap(), + "Member should inherit Guest access" + ); + + // Member has Member-level access + assert!( + AccessControlModule::check_access(&env, user1.clone(), UserRole::Member) + .unwrap(), + "Member should have Member access" + ); + + // Member does NOT have Admin-level access + assert!( + !AccessControlModule::check_access(&env, user1.clone(), UserRole::Admin) + .unwrap(), + "Member should not have Admin access" + ); + + // Admin inherits both Guest and Member + assert!( + AccessControlModule::check_access(&env, admin.clone(), UserRole::Guest) + .unwrap(), + "Admin should inherit Guest access" + ); + assert!( + AccessControlModule::check_access(&env, admin.clone(), UserRole::Member) + .unwrap(), + "Admin should inherit Member access" + ); + assert!( + AccessControlModule::check_access(&env, admin.clone(), UserRole::Admin) + .unwrap(), + "Admin should have Admin access" + ); + + // Guest has only Guest access + let guest = Address::generate(&env); + assert!( + AccessControlModule::check_access(&env, guest.clone(), UserRole::Guest) + .unwrap(), + "Guest should have Guest access" + ); + assert!( + !AccessControlModule::check_access(&env, guest.clone(), UserRole::Member) + .unwrap(), + "Guest should not have Member access" + ); + assert!( + !AccessControlModule::check_access(&env, guest.clone(), UserRole::Admin) + .unwrap(), + "Guest should not have Admin access" + ); + }); } + /// Comprehensive access control enforcement across all scenarios. #[test] fn test_access_control_enforcement() { - assert!(true); + let (env, contract_id, admin, user1, user2) = setup_initialized_env(); + + env.as_contract(&contract_id, || { + // === Admin operations require admin privileges === + + // update_config requires admin + let result = AccessControlModule::update_config( + &env, + user1.clone(), + AccessControlConfig::default(), + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "update_config requires admin" + ); + + // pause requires admin + let result = AccessControlModule::pause(&env, user1.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "pause requires admin" + ); + + // unpause requires admin + let result = AccessControlModule::unpause(&env, user1.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "unpause requires admin" + ); + + // blacklist requires admin + let result = AccessControlModule::blacklist_user(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "blacklist_user requires admin" + ); + + // unblacklist requires admin + let result = + AccessControlModule::unblacklist_user(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "unblacklist_user requires admin" + ); + + // set_role requires admin + let result = + AccessControlModule::set_role(&env, user1.clone(), user2.clone(), UserRole::Member); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "set_role requires admin" + ); + + // remove_role requires admin + let result = AccessControlModule::remove_role(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "remove_role requires admin" + ); + + // propose_admin_transfer requires admin + let result = + AccessControlModule::propose_admin_transfer(&env, user1.clone(), user2.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "propose_admin_transfer requires admin" + ); + + // === Read-only operations should work for all users === + + // get_role works for anyone + let role = AccessControlModule::get_role(&env, user1.clone()); + assert_eq!(role, UserRole::Guest, "get_role should work for anyone"); + + // is_admin works for anyone + let is_admin = AccessControlModule::is_admin(&env, user1.clone()); + assert!(!is_admin, "is_admin should work for anyone"); + + // is_blacklisted works for anyone + let is_blacklisted = AccessControlModule::is_blacklisted(&env, &user1); + assert!(!is_blacklisted, "is_blacklisted should work for anyone"); + + // get_config works for anyone + let config = AccessControlModule::get_config(&env); + assert!(!config.require_membership_for_roles); + + // === Multisig-specific enforcement === + // In multisig mode, direct admin operations are blocked + let env2 = Env::default(); + let contract_id2 = env2.register(crate::AccessControl, ()); + let ms_admin1 = Address::generate(&env2); + let ms_admin2 = Address::generate(&env2); + + env2.as_contract(&contract_id2, || { + let admins = Vec::from_array(&env2, [ms_admin1.clone(), ms_admin2.clone()]); + AccessControlModule::initialize_multisig(&env2, admins, 2, None).unwrap(); + + // Direct update_config blocked in multisig + let result = AccessControlModule::update_config( + &env2, + ms_admin1.clone(), + AccessControlConfig::default(), + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "update_config requires proposal in multisig mode" + ); + + // Direct pause blocked in multisig + let result = AccessControlModule::pause(&env2, ms_admin1.clone()); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "pause requires proposal in multisig mode" + ); + }); + }); } } diff --git a/contracts/access_control/src/access_control_tests.rs b/contracts/access_control/src/access_control_tests.rs index 51ee17b..954401e 100644 --- a/contracts/access_control/src/access_control_tests.rs +++ b/contracts/access_control/src/access_control_tests.rs @@ -1328,6 +1328,151 @@ fn test_duplicate_admin_prevented() { }); } +// ==================== Multisig Admin Transfer Tests (Issue #98 - CT-19) ==================== + +#[test] +fn test_multisig_admin_transfer_via_proposal() { + let env = Env::default(); + let contract_id = env.register(crate::AccessControl, ()); + let admin1 = Address::generate(&env); + let admin2 = Address::generate(&env); + let new_admin = Address::generate(&env); + + env.as_contract(&contract_id, || { + let admins = Vec::from_array(&env, [admin1.clone(), admin2.clone()]); + AccessControlModule::initialize_multisig(&env, admins, 2, None).unwrap(); + + // Create proposal to transfer admin ownership to new_admin + let action = ProposalAction::TransferAdmin(new_admin.clone()); + let proposal_id = + AccessControlModule::create_proposal(&env, admin1.clone(), action).unwrap(); + + let proposal = AccessControlModule::get_proposal(&env, proposal_id).unwrap(); + assert_eq!(proposal.proposal_type, ProposalType::Critical); + + // Fast forward time past time-lock (24 hours + 1 second) + env.ledger().set(LedgerInfo { + timestamp: env.ledger().timestamp() + 86401, + protocol_version: 23, + sequence_number: 10, + network_id: [0; 32], + base_reserve: 10, + min_temp_entry_ttl: 10, + min_persistent_entry_ttl: 10, + max_entry_ttl: 6312000, + }); + + // Second approval should execute the transfer (critical threshold for 2 admins = 2) + AccessControlModule::approve_proposal(&env, admin2.clone(), proposal_id).unwrap(); + + // Verify new admin has admin role + assert_eq!( + AccessControlModule::get_role(&env, new_admin.clone()), + UserRole::Admin + ); + + // Verify old admin1 role was downgraded + assert_eq!( + AccessControlModule::get_role(&env, admin1.clone()), + UserRole::Guest + ); + + // Verify multisig config was updated - admin1 removed, new_admin added + let updated_config = AccessControlModule::get_multisig_config(&env).unwrap(); + assert!( + !updated_config.admins.contains(&admin1), + "admin1 should be removed from multisig admins" + ); + assert!( + updated_config.admins.contains(&new_admin), + "new_admin should be added to multisig admins" + ); + assert!( + updated_config.admins.contains(&admin2), + "admin2 should remain in multisig admins" + ); + + // New admin should be able to perform admin operations + let user = Address::generate(&env); + assert!(AccessControlModule::set_role( + &env, + new_admin.clone(), + user.clone(), + UserRole::Member + ) + .is_ok()); + + // Old admin (admin1) should not be able to perform admin operations + let result = AccessControlModule::set_role( + &env, + admin1.clone(), + user.clone(), + UserRole::Member, + ); + assert_eq!( + result.unwrap_err(), + AccessControlError::AdminRequired, + "Old admin should no longer have admin privileges" + ); + }); +} + +#[test] +fn test_multisig_transfer_admin_to_self_fails() { + let env = Env::default(); + let contract_id = env.register(crate::AccessControl, ()); + let admin1 = Address::generate(&env); + let admin2 = Address::generate(&env); + + env.as_contract(&contract_id, || { + let admins = Vec::from_array(&env, [admin1.clone(), admin2.clone()]); + AccessControlModule::initialize_multisig(&env, admins, 2, None).unwrap(); + + // Transferring to self should be blocked + let action = ProposalAction::TransferAdmin(admin1.clone()); + let result = + AccessControlModule::create_proposal(&env, admin1.clone(), action); + // The proposal should still be creatable, but execution should fail + assert!(result.is_ok()); + + let proposal_id = result.unwrap(); + + // Fast forward time past time-lock + env.ledger().set(LedgerInfo { + timestamp: env.ledger().timestamp() + 86401, + protocol_version: 23, + sequence_number: 10, + network_id: [0; 32], + base_reserve: 10, + min_temp_entry_ttl: 10, + min_persistent_entry_ttl: 10, + max_entry_ttl: 6312000, + }); + + // Execution should fail with DuplicateAdmin (admin1 is already in the admins list) + let result = AccessControlModule::approve_proposal(&env, admin2.clone(), proposal_id); + assert_eq!(result.unwrap_err(), AccessControlError::DuplicateAdmin); + }); +} + +#[test] +fn test_multisig_prevents_direct_admin_transfer() { + let env = Env::default(); + let contract_id = env.register(crate::AccessControl, ()); + let admin1 = Address::generate(&env); + let admin2 = Address::generate(&env); + let new_admin = Address::generate(&env); + + env.as_contract(&contract_id, || { + let admins = Vec::from_array(&env, [admin1.clone(), admin2.clone()]); + AccessControlModule::initialize_multisig(&env, admins, 2, None).unwrap(); + + // Direct admin transfer should be blocked in multisig mode + let result = AccessControlModule::propose_admin_transfer(&env, admin1.clone(), new_admin.clone()); + assert_eq!(result.unwrap_err(), AccessControlError::InvalidAddress); + }); +} + #[test] fn test_get_pending_proposals_list() { let env = Env::default(); diff --git a/contracts/manage_hub/src/errors.rs b/contracts/manage_hub/src/errors.rs index ecb22cc..cc81e5d 100644 --- a/contracts/manage_hub/src/errors.rs +++ b/contracts/manage_hub/src/errors.rs @@ -79,7 +79,6 @@ pub enum Error { /// Pagination parameters failed validation (e.g. limit = 0, /// limit > MAX_PAGE_SIZE). InvalidPaginationParams = 56, -} - SubscriptionAlreadyRevoked = 51, - SubscriptionInvalid = 52, + /// Subscription has been marked as invalid. + SubscriptionInvalid = 57, }