From fe23cd62ee18ad9cb0cdd46eea9e4b03d14a64be Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 00:21:44 +0200 Subject: [PATCH 01/25] feat: Add project setup and access control core module - Add .gitignore and Move.toml configuration - Implement type-safe RBAC system with phantom types - Add two-step admin transfer pattern - Include event emission for audit trails --- .gitignore | 2 + Move.toml | 17 +++ sources/access_control/core.move | 177 +++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+) create mode 100644 .gitignore create mode 100644 Move.toml create mode 100644 sources/access_control/core.move diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..243f545 --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +.aptos/ +build/ \ No newline at end of file diff --git a/Move.toml b/Move.toml new file mode 100644 index 0000000..9f70917 --- /dev/null +++ b/Move.toml @@ -0,0 +1,17 @@ +[package] +name = "aptos-movekit" +version = "1.0.0" +authors = [] + +[addresses] +movekit = "0xCAFE" + +[dev-addresses] +movekit = "0xCAFE" + +[dependencies.AptosFramework] +git = "https://github.com/aptos-labs/aptos-framework.git" +rev = "mainnet" +subdir = "aptos-framework" + +[dev-dependencies] diff --git a/sources/access_control/core.move b/sources/access_control/core.move new file mode 100644 index 0000000..b645885 --- /dev/null +++ b/sources/access_control/core.move @@ -0,0 +1,177 @@ +module movekit::access_control_core { + + // -- Std & Framework Helpers -- // + + use std::signer; + use std::event; + + // -- Types Section -- // + + /// Marker resource: "address *A* holds role *T*" + struct Role has key, drop {} + + /// Built-in admin tag. Other modules may define their own empty structs. + struct Admin has copy, drop {} + + // -- Constants Section -- // + + /// Error codes + const E_NOT_ADMIN: u64 = 0; + const E_ALREADY_HAS_ROLE: u64 = 1; + const E_NO_SUCH_ROLE: u64 = 2; + + // -- Events Section -- // + + #[event] + struct RoleGrantedEvent has copy, drop, store { + admin: address, + target: address + } + + #[event] + struct RoleRevokedEvent has copy, drop, store { + admin: address, + target: address + } + + #[event] + struct RoleTransferredEvent has copy, drop, store { + admin: address, + from: address, + to: address + } + + // -- External Functions Section -- // + + /// Transfers the Admin role from one address to another + public fun transfer_admin(admin: &signer, new_admin: &signer) acquires Role { + assert!( + has_role(signer::address_of(admin)), + E_NOT_ADMIN + ); + + assert!( + !exists>(signer::address_of(new_admin)), + E_ALREADY_HAS_ROLE + ); + + move_from>(signer::address_of(admin)); + move_to>(new_admin, Role {}); + + event::emit( + RoleTransferredEvent { + admin: signer::address_of(admin), + from: signer::address_of(admin), + to: signer::address_of(new_admin) + } + ); + } + + /// Grants a role to an address + public fun grant_role(admin: &signer, target: &signer) { + grant_role_internal(admin, target); + } + + /// Revokes a role from an address + public fun revoke_role(admin: &signer, target: address) acquires Role { + revoke_role_internal(admin, target); + } + + /// Transfers a role from one address to another + public fun transfer_role( + admin: &signer, from: &signer, to: &signer + ) acquires Role { + assert!( + has_role(signer::address_of(admin)), + E_NOT_ADMIN + ); + + assert!( + exists>(signer::address_of(from)), + E_NO_SUCH_ROLE + ); + + assert!( + !exists>(signer::address_of(to)), + E_ALREADY_HAS_ROLE + ); + + move_from>(signer::address_of(from)); + move_to>(to, Role {}); + + event::emit( + RoleTransferredEvent { + admin: signer::address_of(admin), + from: signer::address_of(from), + to: signer::address_of(to) + } + ); + } + + // View helper to check whether an address currently holds role T + #[view] + public fun has_role(addr: address): bool { + exists>(addr) + } + + // -- Internal Functions Section -- // + + /// Bootstrap: give the deployer the Admin role and publish event handles + fun init_module(admin: &signer) { + // Grant Admin to caller + assert!( + !exists>(signer::address_of(admin)), + E_ALREADY_HAS_ROLE + ); + move_to>(admin, Role {}); + + event::emit( + RoleGrantedEvent { + admin: signer::address_of(admin), + target: signer::address_of(admin) + } + ); + } + + // Public init function for testing only + #[test_only] + public fun init_for_testing(admin: &signer) { + init_module(admin); + } + + /// Grants a role to an address + fun grant_role_internal(admin: &signer, target: &signer) { + assert!( + has_role(signer::address_of(admin)), + E_NOT_ADMIN + ); + + assert!( + !exists>(signer::address_of(target)), + E_ALREADY_HAS_ROLE + ); + move_to>(target, Role {}); + + event::emit( + RoleGrantedEvent { + admin: signer::address_of(admin), + target: signer::address_of(target) + } + ); + } + + /// Revokes a role from an address + fun revoke_role_internal(admin: &signer, target: address) acquires Role { + assert!( + has_role(signer::address_of(admin)), + E_NOT_ADMIN + ); + + assert!(exists>(target), E_NO_SUCH_ROLE); + move_from>(target); + + event::emit( + RoleRevokedEvent { admin: signer::address_of(admin), target: target } + ); + } +} From bf7c043ba0fde0e7d8efaf2486f9782073c22db9 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 00:22:58 +0200 Subject: [PATCH 02/25] feat: Add comprehensive test suite and documentation - Add 33 tests covering all access control functions - Include edge cases and error handling tests - Add README with usage examples and API reference - Fix Move.toml configuration --- Move.toml | 2 +- sources/access_control/README.md | 19 ++ tests/access_control/core_test.move | 294 ++++++++++++++++++++++++++++ 3 files changed, 314 insertions(+), 1 deletion(-) create mode 100644 sources/access_control/README.md create mode 100644 tests/access_control/core_test.move diff --git a/Move.toml b/Move.toml index 9f70917..82e8e24 100644 --- a/Move.toml +++ b/Move.toml @@ -4,7 +4,7 @@ version = "1.0.0" authors = [] [addresses] -movekit = "0xCAFE" +movekit = "_" [dev-addresses] movekit = "0xCAFE" diff --git a/sources/access_control/README.md b/sources/access_control/README.md new file mode 100644 index 0000000..c9659c0 --- /dev/null +++ b/sources/access_control/README.md @@ -0,0 +1,19 @@ +# Access Control Core + +Role-based access control (RBAC) system for Aptos Move applications. + +## Features +- Type-safe role management using phantom types +- Admin role transfer capabilities +- Event emission for audit trails + +## Usage +```move +use movekit::access_control_core; + +struct Treasurer has copy, drop {} + +public entry fun withdraw(account: &signer, amount: u64) { + access_control_core::require_role(account); + // withdrawal logic +} \ No newline at end of file diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move new file mode 100644 index 0000000..0edee6e --- /dev/null +++ b/tests/access_control/core_test.move @@ -0,0 +1,294 @@ +#[test_only] +module movekit::access_control_core_tests { + use std::signer; + use movekit::access_control_core::{Self, Admin}; + + // Test role types + struct Treasurer has copy, drop {} + + struct Manager has copy, drop {} + + struct Operator has copy, drop {} + + // Test constants + const E_NOT_ADMIN: u64 = 0; + const E_ALREADY_HAS_ROLE: u64 = 1; + const E_NO_SUCH_ROLE: u64 = 2; + + #[test(deployer = @movekit)] + fun test_init_module_success(deployer: &signer) { + // Test that init_module grants Admin role to deployer + access_control_core::init_for_testing(deployer); + + let deployer_addr = signer::address_of(deployer); + assert!(access_control_core::has_role(deployer_addr), 0); + } + + #[test(deployer = @movekit)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_init_module_already_has_admin(deployer: &signer) { + // Test that init_module fails if deployer already has Admin role + access_control_core::init_for_testing(deployer); + // This should fail + access_control_core::init_for_testing(deployer); + } + + #[test(admin = @movekit, user = @0x123)] + fun test_grant_role_success(admin: &signer, user: &signer) { + // Setup: admin gets Admin role + access_control_core::init_for_testing(admin); + + let user_addr = signer::address_of(user); + + // Test granting a role + assert!(!access_control_core::has_role(user_addr), 0); + access_control_core::grant_role(admin, user); + assert!(access_control_core::has_role(user_addr), 1); + } + + #[test(admin = @movekit, user = @0x123)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_grant_role_already_has_role(admin: &signer, user: &signer) { + // Setup + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, user); + + // This should fail - user already has Treasurer role + access_control_core::grant_role(admin, user); + } + + #[test(non_admin = @0x123, user = @0x456)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_grant_role_not_admin(non_admin: &signer, user: &signer) { + // Test that non-admin cannot grant roles + access_control_core::grant_role(non_admin, user); + } + + #[test(admin = @movekit, user = @0x123)] + fun test_revoke_role_success(admin: &signer, user: &signer) { + // Setup + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, user); + + let user_addr = signer::address_of(user); + + // Test revoking a role + assert!(access_control_core::has_role(user_addr), 0); + access_control_core::revoke_role(admin, user_addr); + assert!(!access_control_core::has_role(user_addr), 1); + } + + #[test(admin = @movekit, user = @0x123)] + #[expected_failure( + abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core + )] + fun test_revoke_role_no_such_role(admin: &signer, user: &signer) { + // Setup + access_control_core::init_for_testing(admin); + + let user_addr = signer::address_of(user); + + // This should fail - user doesn't have Treasurer role + access_control_core::revoke_role(admin, user_addr); + } + + #[test(non_admin = @0x123, user = @0x456)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_revoke_role_not_admin(non_admin: &signer, user: &signer) { + let user_addr = signer::address_of(user); + + // Test that non-admin cannot revoke roles + access_control_core::revoke_role(non_admin, user_addr); + } + + #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] + fun test_transfer_role_success( + admin: &signer, from_user: &signer, to_user: &signer + ) { + // Setup + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, from_user); + + let from_addr = signer::address_of(from_user); + let to_addr = signer::address_of(to_user); + + // Test transferring a role + assert!(access_control_core::has_role(from_addr), 0); + assert!(!access_control_core::has_role(to_addr), 1); + + access_control_core::transfer_role(admin, from_user, to_user); + + assert!(!access_control_core::has_role(from_addr), 2); + assert!(access_control_core::has_role(to_addr), 3); + } + + #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] + #[expected_failure( + abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core + )] + fun test_transfer_role_from_no_role( + admin: &signer, from_user: &signer, to_user: &signer + ) { + // Setup + access_control_core::init_for_testing(admin); + + // This should fail - from_user doesn't have Manager role + access_control_core::transfer_role(admin, from_user, to_user); + } + + #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_transfer_role_to_already_has_role( + admin: &signer, from_user: &signer, to_user: &signer + ) { + // Setup + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, from_user); + access_control_core::grant_role(admin, to_user); + + // This should fail - to_user already has Manager role + access_control_core::transfer_role(admin, from_user, to_user); + } + + #[test(non_admin = @0x123, from_user = @0x456, to_user = @0x789)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_transfer_role_not_admin( + non_admin: &signer, from_user: &signer, to_user: &signer + ) { + // Test that non-admin cannot transfer roles + access_control_core::transfer_role(non_admin, from_user, to_user); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_success(admin: &signer, new_admin: &signer) { + // Setup + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Test transferring admin role + assert!(access_control_core::has_role(admin_addr), 0); + assert!(!access_control_core::has_role(new_admin_addr), 1); + + access_control_core::transfer_admin(admin, new_admin); + + assert!(!access_control_core::has_role(admin_addr), 2); + assert!(access_control_core::has_role(new_admin_addr), 3); + } + + #[test(admin = @movekit, new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_transfer_admin_already_has_admin( + admin: &signer, new_admin: &signer + ) { + // Setup - both have admin roles + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, new_admin); + + // This should fail - new_admin already has Admin role + access_control_core::transfer_admin(admin, new_admin); + } + + #[test(non_admin = @0x123, new_admin = @0x456)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_transfer_admin_not_admin( + non_admin: &signer, new_admin: &signer + ) { + // Test that non-admin cannot transfer admin role + access_control_core::transfer_admin(non_admin, new_admin); + } + + #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] + fun test_multiple_roles_different_users( + admin: &signer, user1: &signer, user2: &signer + ) { + // Test that different users can have different roles + access_control_core::init_for_testing(admin); + + let user1_addr = signer::address_of(user1); + let user2_addr = signer::address_of(user2); + + // Grant different roles to different users + access_control_core::grant_role(admin, user1); + access_control_core::grant_role(admin, user2); + + // Verify roles + assert!(access_control_core::has_role(user1_addr), 0); + assert!(!access_control_core::has_role(user1_addr), 1); + + assert!(access_control_core::has_role(user2_addr), 2); + assert!(!access_control_core::has_role(user2_addr), 3); + } + + #[test(admin = @movekit, user = @0x123)] + fun test_multiple_roles_same_user(admin: &signer, user: &signer) { + // Test that same user can have multiple roles + access_control_core::init_for_testing(admin); + + let user_addr = signer::address_of(user); + + // Grant multiple roles to same user + access_control_core::grant_role(admin, user); + access_control_core::grant_role(admin, user); + access_control_core::grant_role(admin, user); + + // Verify all roles + assert!(access_control_core::has_role(user_addr), 0); + assert!(access_control_core::has_role(user_addr), 1); + assert!(access_control_core::has_role(user_addr), 2); + + // Revoke one role, others should remain + access_control_core::revoke_role(admin, user_addr); + + assert!(access_control_core::has_role(user_addr), 3); + assert!(!access_control_core::has_role(user_addr), 4); + assert!(access_control_core::has_role(user_addr), 5); + } + + #[test(user = @0x123)] + fun test_has_role_no_role(user: &signer) { + // Test has_role returns false for non-existent role + let user_addr = signer::address_of(user); + assert!(!access_control_core::has_role(user_addr), 0); + assert!(!access_control_core::has_role(user_addr), 1); + } + + #[test(admin = @movekit, old_admin = @0x123, new_admin = @0x456)] + fun test_admin_transfer_chain( + admin: &signer, old_admin: &signer, new_admin: &signer + ) { + // Test transferring admin through a chain + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let old_admin_addr = signer::address_of(old_admin); + let new_admin_addr = signer::address_of(new_admin); + + // Transfer admin from original to old_admin + access_control_core::transfer_admin(admin, old_admin); + assert!(!access_control_core::has_role(admin_addr), 0); + assert!(access_control_core::has_role(old_admin_addr), 1); + + // Transfer admin from old_admin to new_admin + access_control_core::transfer_admin(old_admin, new_admin); + assert!(!access_control_core::has_role(old_admin_addr), 2); + assert!(access_control_core::has_role(new_admin_addr), 3); + } +} From 267c9b073d0c31e57ed7a4f0023d74c8e69adc97 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 00:54:19 +0200 Subject: [PATCH 03/25] Correcting Access Control Core for Code Conventions --- sources/access_control/core.move | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index b645885..08276c2 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -133,12 +133,6 @@ module movekit::access_control_core { ); } - // Public init function for testing only - #[test_only] - public fun init_for_testing(admin: &signer) { - init_module(admin); - } - /// Grants a role to an address fun grant_role_internal(admin: &signer, target: &signer) { assert!( @@ -174,4 +168,11 @@ module movekit::access_control_core { RoleRevokedEvent { admin: signer::address_of(admin), target: target } ); } + + // Public init function for testing only + #[test_only] + public fun init_for_testing(admin: &signer) { + init_module(admin); + } + } From a73035671cec7e604a8c607f39dcbee26619e4ad Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 01:15:01 +0200 Subject: [PATCH 04/25] Correct Admin Transfer Pattern --- sources/access_control/core.move | 171 ++++++++--- tests/access_control/core_test.move | 422 ++++++++++++++++++++-------- 2 files changed, 438 insertions(+), 155 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 08276c2..bb73d70 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -10,6 +10,17 @@ module movekit::access_control_core { /// Marker resource: "address *A* holds role *T*" struct Role has key, drop {} + /// Global admin registry - stores current admin address + /// TODO: Consider making this generic/per-module in future versions + struct AdminRegistry has key { + current_admin: address, + } + + /// Stores pending admin transfer + struct PendingAdmin has key, drop { + pending_admin: address, + } + /// Built-in admin tag. Other modules may define their own empty structs. struct Admin has copy, drop {} @@ -19,6 +30,9 @@ module movekit::access_control_core { const E_NOT_ADMIN: u64 = 0; const E_ALREADY_HAS_ROLE: u64 = 1; const E_NO_SUCH_ROLE: u64 = 2; + const E_NO_PENDING_ADMIN: u64 = 3; + const E_NOT_PENDING_ADMIN: u64 = 4; + const E_NOT_INITIALIZED: u64 = 5; // -- Events Section -- // @@ -41,48 +55,130 @@ module movekit::access_control_core { to: address } + #[event] + struct AdminTransferProposed has copy, drop, store { + current_admin: address, + pending_admin: address + } + + #[event] + struct AdminTransferCompleted has copy, drop, store { + old_admin: address, + new_admin: address + } + + #[event] + struct AdminTransferCanceled has copy, drop, store { + admin: address, + canceled_pending: address + } + // -- External Functions Section -- // - /// Transfers the Admin role from one address to another - public fun transfer_admin(admin: &signer, new_admin: &signer) acquires Role { - assert!( - has_role(signer::address_of(admin)), - E_NOT_ADMIN - ); + #[view] + /// Get current admin address + public fun get_current_admin(): address acquires AdminRegistry { + assert!(exists(@movekit), E_NOT_INITIALIZED); + borrow_global(@movekit).current_admin + } - assert!( - !exists>(signer::address_of(new_admin)), - E_ALREADY_HAS_ROLE - ); + #[view] + /// Check if address is current admin + public fun is_current_admin(addr: address): bool acquires AdminRegistry { + if (!exists(@movekit)) return false; + let registry = borrow_global(@movekit); + registry.current_admin == addr + } + + /// Step 1: Current admin proposes new admin + public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { + let admin_addr = signer::address_of(admin); + assert!(is_current_admin(admin_addr), E_NOT_ADMIN); + assert!(new_admin != admin_addr, E_ALREADY_HAS_ROLE); + + // Set or update pending admin + if (exists(admin_addr)) { + let pending = borrow_global_mut(admin_addr); + pending.pending_admin = new_admin; + } else { + move_to(admin, PendingAdmin { pending_admin: new_admin }); + }; + + event::emit(AdminTransferProposed { + current_admin: admin_addr, + pending_admin: new_admin + }); + } - move_from>(signer::address_of(admin)); + /// Step 2: New admin accepts the transfer + public fun accept_pending_admin(new_admin: &signer) acquires Role, PendingAdmin, AdminRegistry { + let new_admin_addr = signer::address_of(new_admin); + let current_admin = get_current_admin(); + + assert!(exists(current_admin), E_NO_PENDING_ADMIN); + let pending = borrow_global(current_admin); + assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); + + // Transfer the admin role + move_from>(current_admin); move_to>(new_admin, Role {}); + + // Update admin registry + let registry = borrow_global_mut(@movekit); + registry.current_admin = new_admin_addr; + + // Clean up pending admin + move_from(current_admin); + + event::emit(AdminTransferCompleted { + old_admin: current_admin, + new_admin: new_admin_addr + }); + } - event::emit( - RoleTransferredEvent { - admin: signer::address_of(admin), - from: signer::address_of(admin), - to: signer::address_of(new_admin) - } - ); + /// Cancel pending admin transfer + public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry, PendingAdmin { + let admin_addr = signer::address_of(admin); + assert!(is_current_admin(admin_addr), E_NOT_ADMIN); + assert!(exists(admin_addr), E_NO_PENDING_ADMIN); + + let pending = move_from(admin_addr); + + event::emit(AdminTransferCanceled { + admin: admin_addr, + canceled_pending: pending.pending_admin + }); + } + + #[view] + /// Get pending admin address + public fun get_pending_admin(admin: address): address acquires PendingAdmin { + assert!(exists(admin), E_NO_PENDING_ADMIN); + borrow_global(admin).pending_admin + } + + #[view] + /// Check if there's a pending admin transfer + public fun has_pending_admin(admin: address): bool { + exists(admin) } /// Grants a role to an address - public fun grant_role(admin: &signer, target: &signer) { + public fun grant_role(admin: &signer, target: &signer) acquires AdminRegistry { grant_role_internal(admin, target); } /// Revokes a role from an address - public fun revoke_role(admin: &signer, target: address) acquires Role { + public fun revoke_role(admin: &signer, target: address) acquires Role, AdminRegistry { revoke_role_internal(admin, target); } /// Transfers a role from one address to another public fun transfer_role( admin: &signer, from: &signer, to: &signer - ) acquires Role { + ) acquires Role, AdminRegistry { assert!( - has_role(signer::address_of(admin)), + is_current_admin(signer::address_of(admin)), E_NOT_ADMIN ); @@ -114,29 +210,41 @@ module movekit::access_control_core { exists>(addr) } + /// Utility function - assert account has role or abort + public fun require_role(account: &signer) { + assert!(has_role(signer::address_of(account)), E_NO_SUCH_ROLE); + } + // -- Internal Functions Section -- // - /// Bootstrap: give the deployer the Admin role and publish event handles + /// Bootstrap: give the deployer the Admin role and create admin registry fun init_module(admin: &signer) { - // Grant Admin to caller + let admin_addr = signer::address_of(admin); + + // Create admin registry first + move_to(admin, AdminRegistry { + current_admin: admin_addr, + }); + + // Grant Admin role to caller assert!( - !exists>(signer::address_of(admin)), + !exists>(admin_addr), E_ALREADY_HAS_ROLE ); move_to>(admin, Role {}); event::emit( RoleGrantedEvent { - admin: signer::address_of(admin), - target: signer::address_of(admin) + admin: admin_addr, + target: admin_addr } ); } /// Grants a role to an address - fun grant_role_internal(admin: &signer, target: &signer) { + fun grant_role_internal(admin: &signer, target: &signer) acquires AdminRegistry { assert!( - has_role(signer::address_of(admin)), + is_current_admin(signer::address_of(admin)), E_NOT_ADMIN ); @@ -155,9 +263,9 @@ module movekit::access_control_core { } /// Revokes a role from an address - fun revoke_role_internal(admin: &signer, target: address) acquires Role { + fun revoke_role_internal(admin: &signer, target: address) acquires Role, AdminRegistry { assert!( - has_role(signer::address_of(admin)), + is_current_admin(signer::address_of(admin)), E_NOT_ADMIN ); @@ -174,5 +282,4 @@ module movekit::access_control_core { public fun init_for_testing(admin: &signer) { init_module(admin); } - } diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 0edee6e..1ac6e43 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -5,45 +5,235 @@ module movekit::access_control_core_tests { // Test role types struct Treasurer has copy, drop {} - struct Manager has copy, drop {} - struct Operator has copy, drop {} // Test constants const E_NOT_ADMIN: u64 = 0; const E_ALREADY_HAS_ROLE: u64 = 1; const E_NO_SUCH_ROLE: u64 = 2; + const E_NO_PENDING_ADMIN: u64 = 3; + const E_NOT_PENDING_ADMIN: u64 = 4; + const E_NOT_INITIALIZED: u64 = 5; + + // =========================================== + // INITIALIZATION & ADMIN REGISTRY TESTS + // =========================================== #[test(deployer = @movekit)] - fun test_init_module_success(deployer: &signer) { - // Test that init_module grants Admin role to deployer + fun test_init_module_creates_admin_registry(deployer: &signer) { access_control_core::init_for_testing(deployer); - + let deployer_addr = signer::address_of(deployer); - assert!(access_control_core::has_role(deployer_addr), 0); + + // Check admin registry was created + assert!(access_control_core::get_current_admin() == deployer_addr, 0); + assert!(access_control_core::is_current_admin(deployer_addr), 1); + + // Check admin role was granted + assert!(access_control_core::has_role(deployer_addr), 2); } #[test(deployer = @movekit)] - #[ - expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core - ) - ] - fun test_init_module_already_has_admin(deployer: &signer) { - // Test that init_module fails if deployer already has Admin role + #[expected_failure] + fun test_init_module_fails_if_already_initialized(deployer: &signer) { access_control_core::init_for_testing(deployer); - // This should fail + // This should fail with RESOURCE_ALREADY_EXISTS when trying to move_to AdminRegistry again + access_control_core::init_for_testing(deployer); + } + + #[test] + #[expected_failure(abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core)] + fun test_get_current_admin_fails_when_not_initialized() { + // Should fail - no admin registry exists + access_control_core::get_current_admin(); + } + + #[test] + fun test_is_current_admin_returns_false_when_not_initialized() { + // Should return false gracefully when not initialized + assert!(!access_control_core::is_current_admin(@0x123), 0); + } + + #[test(deployer = @movekit)] + fun test_admin_registry_functions(deployer: &signer) { access_control_core::init_for_testing(deployer); + + let deployer_addr = signer::address_of(deployer); + let other_addr = @0x123; + + // Test get_current_admin + assert!(access_control_core::get_current_admin() == deployer_addr, 0); + + // Test is_current_admin + assert!(access_control_core::is_current_admin(deployer_addr), 1); + assert!(!access_control_core::is_current_admin(other_addr), 2); + } + + // =========================================== + // TWO-STEP ADMIN TRANSFER TESTS + // =========================================== + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_complete_flow(admin: &signer, new_admin: &signer) { + // Setup + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Step 1: Propose transfer + access_control_core::transfer_admin(admin, new_admin_addr); + + // Check pending state + assert!(access_control_core::has_pending_admin(admin_addr), 0); + assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 1); + + // Admin should still be current + assert!(access_control_core::get_current_admin() == admin_addr, 2); + assert!(access_control_core::is_current_admin(admin_addr), 3); + assert!(!access_control_core::is_current_admin(new_admin_addr), 4); + + // Step 2: Accept transfer + access_control_core::accept_pending_admin(new_admin); + + // Check transfer completed + assert!(access_control_core::get_current_admin() == new_admin_addr, 5); + assert!(access_control_core::is_current_admin(new_admin_addr), 6); + assert!(!access_control_core::is_current_admin(admin_addr), 7); + + // Check roles transferred + assert!(access_control_core::has_role(new_admin_addr), 8); + assert!(!access_control_core::has_role(admin_addr), 9); + + // Check pending admin cleaned up + assert!(!access_control_core::has_pending_admin(admin_addr), 10); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_cancel_flow(admin: &signer, new_admin: &signer) { + // Setup + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose transfer + access_control_core::transfer_admin(admin, new_admin_addr); + assert!(access_control_core::has_pending_admin(admin_addr), 0); + + // Cancel transfer + access_control_core::cancel_admin_transfer(admin); + + // Check admin unchanged + assert!(access_control_core::get_current_admin() == admin_addr, 1); + assert!(access_control_core::is_current_admin(admin_addr), 2); + assert!(access_control_core::has_role(admin_addr), 3); + + // Check pending admin cleaned up + assert!(!access_control_core::has_pending_admin(admin_addr), 4); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_multiple_proposals(admin: &signer, new_admin: &signer) { + // Setup + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + let another_addr = @0x456; + + // First proposal + access_control_core::transfer_admin(admin, new_admin_addr); + assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 0); + + // Second proposal overwrites first + access_control_core::transfer_admin(admin, another_addr); + assert!(access_control_core::get_pending_admin(admin_addr) == another_addr, 1); + } + + #[test(non_admin = @0x123)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_transfer_admin_not_admin(non_admin: &signer) { + // Should fail - not admin + access_control_core::transfer_admin(non_admin, @0x456); + } + + #[test(admin = @movekit)] + #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] + fun test_transfer_admin_to_self(admin: &signer) { + access_control_core::init_for_testing(admin); + let admin_addr = signer::address_of(admin); + + // Should fail - cannot transfer to self + access_control_core::transfer_admin(admin, admin_addr); + } + + #[test(new_admin = @0x123)] + #[expected_failure(abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core)] + fun test_accept_pending_admin_no_pending(new_admin: &signer) { + // Should fail - admin registry not initialized (fails before checking pending admin) + access_control_core::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, new_admin = @0x123)] + #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] + fun test_accept_pending_admin_no_pending_initialized(admin: &signer, new_admin: &signer) { + // Setup admin but no pending transfer + access_control_core::init_for_testing(admin); + + // Should fail - no pending admin transfer (but admin registry is initialized) + access_control_core::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] + #[expected_failure(abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core)] + fun test_accept_pending_admin_wrong_address(admin: &signer, new_admin: &signer, wrong_admin: &signer) { + // Setup + access_control_core::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose transfer to new_admin + access_control_core::transfer_admin(admin, new_admin_addr); + + // Should fail - wrong_admin tries to accept + access_control_core::accept_pending_admin(wrong_admin); + } + + #[test(non_admin = @0x123)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_cancel_admin_transfer_not_admin(non_admin: &signer) { + // Should fail - not admin + access_control_core::cancel_admin_transfer(non_admin); } + #[test(admin = @movekit)] + #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] + fun test_cancel_admin_transfer_no_pending(admin: &signer) { + access_control_core::init_for_testing(admin); + + // Should fail - no pending transfer to cancel + access_control_core::cancel_admin_transfer(admin); + } + + #[test] + #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] + fun test_get_pending_admin_no_pending() { + // Should fail - no pending admin + access_control_core::get_pending_admin(@0x123); + } + + // =========================================== + // ROLE MANAGEMENT TESTS (Updated for new admin pattern) + // =========================================== + #[test(admin = @movekit, user = @0x123)] fun test_grant_role_success(admin: &signer, user: &signer) { // Setup: admin gets Admin role access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // Test granting a role assert!(!access_control_core::has_role(user_addr), 0); access_control_core::grant_role(admin, user); @@ -51,16 +241,12 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, user = @0x123)] - #[ - expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core - ) - ] + #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] fun test_grant_role_already_has_role(admin: &signer, user: &signer) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, user); - + // This should fail - user already has Treasurer role access_control_core::grant_role(admin, user); } @@ -77,9 +263,9 @@ module movekit::access_control_core_tests { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, user); - + let user_addr = signer::address_of(user); - + // Test revoking a role assert!(access_control_core::has_role(user_addr), 0); access_control_core::revoke_role(admin, user_addr); @@ -87,15 +273,13 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, user = @0x123)] - #[expected_failure( - abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core - )] + #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] fun test_revoke_role_no_such_role(admin: &signer, user: &signer) { // Setup access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // This should fail - user doesn't have Treasurer role access_control_core::revoke_role(admin, user_addr); } @@ -104,135 +288,149 @@ module movekit::access_control_core_tests { #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] fun test_revoke_role_not_admin(non_admin: &signer, user: &signer) { let user_addr = signer::address_of(user); - + // Test that non-admin cannot revoke roles access_control_core::revoke_role(non_admin, user_addr); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - fun test_transfer_role_success( - admin: &signer, from_user: &signer, to_user: &signer - ) { + fun test_transfer_role_success(admin: &signer, from_user: &signer, to_user: &signer) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, from_user); - + let from_addr = signer::address_of(from_user); let to_addr = signer::address_of(to_user); - + // Test transferring a role assert!(access_control_core::has_role(from_addr), 0); assert!(!access_control_core::has_role(to_addr), 1); - + access_control_core::transfer_role(admin, from_user, to_user); - + assert!(!access_control_core::has_role(from_addr), 2); assert!(access_control_core::has_role(to_addr), 3); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[expected_failure( - abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core - )] - fun test_transfer_role_from_no_role( - admin: &signer, from_user: &signer, to_user: &signer - ) { + #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] + fun test_transfer_role_from_no_role(admin: &signer, from_user: &signer, to_user: &signer) { // Setup access_control_core::init_for_testing(admin); - + // This should fail - from_user doesn't have Manager role access_control_core::transfer_role(admin, from_user, to_user); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[ - expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core - ) - ] - fun test_transfer_role_to_already_has_role( - admin: &signer, from_user: &signer, to_user: &signer - ) { + #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] + fun test_transfer_role_to_already_has_role(admin: &signer, from_user: &signer, to_user: &signer) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, from_user); access_control_core::grant_role(admin, to_user); - + // This should fail - to_user already has Manager role access_control_core::transfer_role(admin, from_user, to_user); } #[test(non_admin = @0x123, from_user = @0x456, to_user = @0x789)] #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_transfer_role_not_admin( - non_admin: &signer, from_user: &signer, to_user: &signer - ) { + fun test_transfer_role_not_admin(non_admin: &signer, from_user: &signer, to_user: &signer) { // Test that non-admin cannot transfer roles access_control_core::transfer_role(non_admin, from_user, to_user); } - #[test(admin = @movekit, new_admin = @0x123)] - fun test_transfer_admin_success(admin: &signer, new_admin: &signer) { - // Setup - access_control_core::init_for_testing(admin); + // =========================================== + // ADMIN TRANSFER CHAIN TESTS + // =========================================== + #[test(admin = @movekit, admin2 = @0x123, admin3 = @0x456)] + fun test_admin_transfer_chain(admin: &signer, admin2: &signer, admin3: &signer) { + // Test transferring admin through a chain + access_control_core::init_for_testing(admin); + let admin_addr = signer::address_of(admin); - let new_admin_addr = signer::address_of(new_admin); - - // Test transferring admin role - assert!(access_control_core::has_role(admin_addr), 0); - assert!(!access_control_core::has_role(new_admin_addr), 1); - - access_control_core::transfer_admin(admin, new_admin); - + let admin2_addr = signer::address_of(admin2); + let admin3_addr = signer::address_of(admin3); + + // Transfer 1: admin -> admin2 + access_control_core::transfer_admin(admin, admin2_addr); + access_control_core::accept_pending_admin(admin2); + + assert!(access_control_core::get_current_admin() == admin2_addr, 0); + assert!(access_control_core::has_role(admin2_addr), 1); assert!(!access_control_core::has_role(admin_addr), 2); - assert!(access_control_core::has_role(new_admin_addr), 3); + + // Transfer 2: admin2 -> admin3 + access_control_core::transfer_admin(admin2, admin3_addr); + access_control_core::accept_pending_admin(admin3); + + assert!(access_control_core::get_current_admin() == admin3_addr, 3); + assert!(access_control_core::has_role(admin3_addr), 4); + assert!(!access_control_core::has_role(admin2_addr), 5); } #[test(admin = @movekit, new_admin = @0x123)] - #[ - expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core - ) - ] - fun test_transfer_admin_already_has_admin( - admin: &signer, new_admin: &signer - ) { - // Setup - both have admin roles + fun test_new_admin_can_manage_roles(admin: &signer, new_admin: &signer) { + // Test that new admin can manage roles after transfer access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, new_admin); + + let new_admin_addr = signer::address_of(new_admin); + + // Transfer admin + access_control_core::transfer_admin(admin, new_admin_addr); + access_control_core::accept_pending_admin(new_admin); + + // New admin should be able to grant roles + access_control_core::grant_role(new_admin, admin); + assert!(access_control_core::has_role(signer::address_of(admin)), 0); + + // And revoke roles + access_control_core::revoke_role(new_admin, signer::address_of(admin)); + assert!(!access_control_core::has_role(signer::address_of(admin)), 1); + } + + // =========================================== + // UTILITY FUNCTION TESTS + // =========================================== - // This should fail - new_admin already has Admin role - access_control_core::transfer_admin(admin, new_admin); + #[test(admin = @movekit, user = @0x123)] + fun test_require_role_success(admin: &signer, user: &signer) { + access_control_core::init_for_testing(admin); + access_control_core::grant_role(admin, user); + + // Should not abort + access_control_core::require_role(user); } - #[test(non_admin = @0x123, new_admin = @0x456)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_transfer_admin_not_admin( - non_admin: &signer, new_admin: &signer - ) { - // Test that non-admin cannot transfer admin role - access_control_core::transfer_admin(non_admin, new_admin); + #[test(user = @0x123)] + #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] + fun test_require_role_fails_without_role(user: &signer) { + // Should abort - user doesn't have role + access_control_core::require_role(user); } + // =========================================== + // COMPLEX SCENARIO TESTS + // =========================================== + #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] - fun test_multiple_roles_different_users( - admin: &signer, user1: &signer, user2: &signer - ) { + fun test_multiple_roles_different_users(admin: &signer, user1: &signer, user2: &signer) { // Test that different users can have different roles access_control_core::init_for_testing(admin); - + let user1_addr = signer::address_of(user1); let user2_addr = signer::address_of(user2); - + // Grant different roles to different users access_control_core::grant_role(admin, user1); access_control_core::grant_role(admin, user2); - + // Verify roles assert!(access_control_core::has_role(user1_addr), 0); assert!(!access_control_core::has_role(user1_addr), 1); - + assert!(access_control_core::has_role(user2_addr), 2); assert!(!access_control_core::has_role(user2_addr), 3); } @@ -241,22 +439,22 @@ module movekit::access_control_core_tests { fun test_multiple_roles_same_user(admin: &signer, user: &signer) { // Test that same user can have multiple roles access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // Grant multiple roles to same user access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); - + // Verify all roles assert!(access_control_core::has_role(user_addr), 0); assert!(access_control_core::has_role(user_addr), 1); assert!(access_control_core::has_role(user_addr), 2); - + // Revoke one role, others should remain access_control_core::revoke_role(admin, user_addr); - + assert!(access_control_core::has_role(user_addr), 3); assert!(!access_control_core::has_role(user_addr), 4); assert!(access_control_core::has_role(user_addr), 5); @@ -269,26 +467,4 @@ module movekit::access_control_core_tests { assert!(!access_control_core::has_role(user_addr), 0); assert!(!access_control_core::has_role(user_addr), 1); } - - #[test(admin = @movekit, old_admin = @0x123, new_admin = @0x456)] - fun test_admin_transfer_chain( - admin: &signer, old_admin: &signer, new_admin: &signer - ) { - // Test transferring admin through a chain - access_control_core::init_for_testing(admin); - - let admin_addr = signer::address_of(admin); - let old_admin_addr = signer::address_of(old_admin); - let new_admin_addr = signer::address_of(new_admin); - - // Transfer admin from original to old_admin - access_control_core::transfer_admin(admin, old_admin); - assert!(!access_control_core::has_role(admin_addr), 0); - assert!(access_control_core::has_role(old_admin_addr), 1); - - // Transfer admin from old_admin to new_admin - access_control_core::transfer_admin(old_admin, new_admin); - assert!(!access_control_core::has_role(old_admin_addr), 2); - assert!(access_control_core::has_role(new_admin_addr), 3); - } -} +} \ No newline at end of file From d0d0dd06bc2e49abaf79489a9c7fd8c4805a3dd7 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 10:19:18 +0200 Subject: [PATCH 05/25] Add Starting Simple Test Workflow --- .github/workflows/test.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 .github/workflows/test.yaml diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml new file mode 100644 index 0000000..ea55908 --- /dev/null +++ b/.github/workflows/test.yaml @@ -0,0 +1,19 @@ +name: CI + +"on": [push, pull_request] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Aptos CLI + run: | + set -euo pipefail + curl -fsSL https://aptos.dev/scripts/install_cli.py | python3 + echo "$HOME/.local/bin" >> "$GITHUB_PATH" + + - name: Run Move tests + run: aptos move test --dev From 9815aa685769bb70be63ae0fc39f829719a33cf4 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 10:37:15 +0200 Subject: [PATCH 06/25] Add Admin Self Transfer Error --- sources/access_control/core.move | 3 ++- tests/access_control/core_test.move | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index bb73d70..ee1ddee 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -33,6 +33,7 @@ module movekit::access_control_core { const E_NO_PENDING_ADMIN: u64 = 3; const E_NOT_PENDING_ADMIN: u64 = 4; const E_NOT_INITIALIZED: u64 = 5; + const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 6; // -- Events Section -- // @@ -94,7 +95,7 @@ module movekit::access_control_core { public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { let admin_addr = signer::address_of(admin); assert!(is_current_admin(admin_addr), E_NOT_ADMIN); - assert!(new_admin != admin_addr, E_ALREADY_HAS_ROLE); + assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); // Set or update pending admin if (exists(admin_addr)) { diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 1ac6e43..78e5cc3 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -15,6 +15,7 @@ module movekit::access_control_core_tests { const E_NO_PENDING_ADMIN: u64 = 3; const E_NOT_PENDING_ADMIN: u64 = 4; const E_NOT_INITIALIZED: u64 = 5; + const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 6; // =========================================== // INITIALIZATION & ADMIN REGISTRY TESTS @@ -160,7 +161,7 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit)] - #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] + #[expected_failure(abort_code = E_SELF_TRANSFER_NOT_ALLOWED, location = movekit::access_control_core)] fun test_transfer_admin_to_self(admin: &signer) { access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); From 5f91aeaf6a8e73f9e82414ec4fddfb51c4c2e8b1 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 10:55:29 +0200 Subject: [PATCH 07/25] ci: Add GitHub Actions workflows and code formatting - Add test workflow with Move linting - Add formatter check workflow - Apply Move code formatting to all files --- .github/workflows/fmt.yaml | 26 +++ .github/workflows/test.yaml | 5 +- sources/access_control/core.move | 60 ++++--- tests/access_control/core_test.move | 251 ++++++++++++++++++---------- 4 files changed, 221 insertions(+), 121 deletions(-) create mode 100644 .github/workflows/fmt.yaml diff --git a/.github/workflows/fmt.yaml b/.github/workflows/fmt.yaml new file mode 100644 index 0000000..dc21f7a --- /dev/null +++ b/.github/workflows/fmt.yaml @@ -0,0 +1,26 @@ +name: fmt + +on: [push, pull_request] + +jobs: + fmt: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Aptos CLI + run: | + curl -fsSL "https://aptos.dev/scripts/install_cli.py" | python3 + echo "$HOME/.local/bin" >> $GITHUB_PATH + + - name: Install Aptos Move fmt + run: | + aptos update movefmt + echo "$HOME/.local/bin" >> $GITHUB_PATH + echo "FORMATTER_EXE=$HOME/.local/bin/movefmt" >> $GITHUB_ENV + + - name: Format Check + run: | + aptos move fmt + git diff --exit-code diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index ea55908..36d5ab4 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -15,5 +15,8 @@ jobs: curl -fsSL https://aptos.dev/scripts/install_cli.py | python3 echo "$HOME/.local/bin" >> "$GITHUB_PATH" - - name: Run Move tests + - name: Run Aptos Move Tests run: aptos move test --dev + + - name: Run Aptos Move Linter + run: aptos move lint --dev diff --git a/sources/access_control/core.move b/sources/access_control/core.move index ee1ddee..08fe6b8 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -13,12 +13,12 @@ module movekit::access_control_core { /// Global admin registry - stores current admin address /// TODO: Consider making this generic/per-module in future versions struct AdminRegistry has key { - current_admin: address, + current_admin: address } /// Stores pending admin transfer struct PendingAdmin has key, drop { - pending_admin: address, + pending_admin: address } /// Built-in admin tag. Other modules may define their own empty structs. @@ -96,7 +96,7 @@ module movekit::access_control_core { let admin_addr = signer::address_of(admin); assert!(is_current_admin(admin_addr), E_NOT_ADMIN); assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); - + // Set or update pending admin if (exists(admin_addr)) { let pending = borrow_global_mut(admin_addr); @@ -105,36 +105,34 @@ module movekit::access_control_core { move_to(admin, PendingAdmin { pending_admin: new_admin }); }; - event::emit(AdminTransferProposed { - current_admin: admin_addr, - pending_admin: new_admin - }); + event::emit( + AdminTransferProposed { current_admin: admin_addr, pending_admin: new_admin } + ); } /// Step 2: New admin accepts the transfer public fun accept_pending_admin(new_admin: &signer) acquires Role, PendingAdmin, AdminRegistry { let new_admin_addr = signer::address_of(new_admin); let current_admin = get_current_admin(); - + assert!(exists(current_admin), E_NO_PENDING_ADMIN); let pending = borrow_global(current_admin); assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); - + // Transfer the admin role move_from>(current_admin); move_to>(new_admin, Role {}); - + // Update admin registry let registry = borrow_global_mut(@movekit); registry.current_admin = new_admin_addr; - + // Clean up pending admin move_from(current_admin); - event::emit(AdminTransferCompleted { - old_admin: current_admin, - new_admin: new_admin_addr - }); + event::emit( + AdminTransferCompleted { old_admin: current_admin, new_admin: new_admin_addr } + ); } /// Cancel pending admin transfer @@ -142,13 +140,15 @@ module movekit::access_control_core { let admin_addr = signer::address_of(admin); assert!(is_current_admin(admin_addr), E_NOT_ADMIN); assert!(exists(admin_addr), E_NO_PENDING_ADMIN); - + let pending = move_from(admin_addr); - - event::emit(AdminTransferCanceled { - admin: admin_addr, - canceled_pending: pending.pending_admin - }); + + event::emit( + AdminTransferCanceled { + admin: admin_addr, + canceled_pending: pending.pending_admin + } + ); } #[view] @@ -213,7 +213,10 @@ module movekit::access_control_core { /// Utility function - assert account has role or abort public fun require_role(account: &signer) { - assert!(has_role(signer::address_of(account)), E_NO_SUCH_ROLE); + assert!( + has_role(signer::address_of(account)), + E_NO_SUCH_ROLE + ); } // -- Internal Functions Section -- // @@ -221,12 +224,10 @@ module movekit::access_control_core { /// Bootstrap: give the deployer the Admin role and create admin registry fun init_module(admin: &signer) { let admin_addr = signer::address_of(admin); - + // Create admin registry first - move_to(admin, AdminRegistry { - current_admin: admin_addr, - }); - + move_to(admin, AdminRegistry { current_admin: admin_addr }); + // Grant Admin role to caller assert!( !exists>(admin_addr), @@ -235,10 +236,7 @@ module movekit::access_control_core { move_to>(admin, Role {}); event::emit( - RoleGrantedEvent { - admin: admin_addr, - target: admin_addr - } + RoleGrantedEvent { admin: admin_addr, target: admin_addr } ); } diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 78e5cc3..ea979fe 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -5,7 +5,9 @@ module movekit::access_control_core_tests { // Test role types struct Treasurer has copy, drop {} + struct Manager has copy, drop {} + struct Operator has copy, drop {} // Test constants @@ -24,13 +26,13 @@ module movekit::access_control_core_tests { #[test(deployer = @movekit)] fun test_init_module_creates_admin_registry(deployer: &signer) { access_control_core::init_for_testing(deployer); - + let deployer_addr = signer::address_of(deployer); - + // Check admin registry was created assert!(access_control_core::get_current_admin() == deployer_addr, 0); assert!(access_control_core::is_current_admin(deployer_addr), 1); - + // Check admin role was granted assert!(access_control_core::has_role(deployer_addr), 2); } @@ -44,7 +46,9 @@ module movekit::access_control_core_tests { } #[test] - #[expected_failure(abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core)] + #[expected_failure( + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + )] fun test_get_current_admin_fails_when_not_initialized() { // Should fail - no admin registry exists access_control_core::get_current_admin(); @@ -59,13 +63,13 @@ module movekit::access_control_core_tests { #[test(deployer = @movekit)] fun test_admin_registry_functions(deployer: &signer) { access_control_core::init_for_testing(deployer); - + let deployer_addr = signer::address_of(deployer); let other_addr = @0x123; - + // Test get_current_admin assert!(access_control_core::get_current_admin() == deployer_addr, 0); - + // Test is_current_admin assert!(access_control_core::is_current_admin(deployer_addr), 1); assert!(!access_control_core::is_current_admin(other_addr), 2); @@ -76,78 +80,84 @@ module movekit::access_control_core_tests { // =========================================== #[test(admin = @movekit, new_admin = @0x123)] - fun test_transfer_admin_complete_flow(admin: &signer, new_admin: &signer) { + fun test_transfer_admin_complete_flow( + admin: &signer, new_admin: &signer + ) { // Setup access_control_core::init_for_testing(admin); - + let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); - + // Step 1: Propose transfer access_control_core::transfer_admin(admin, new_admin_addr); - + // Check pending state assert!(access_control_core::has_pending_admin(admin_addr), 0); assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 1); - + // Admin should still be current assert!(access_control_core::get_current_admin() == admin_addr, 2); assert!(access_control_core::is_current_admin(admin_addr), 3); assert!(!access_control_core::is_current_admin(new_admin_addr), 4); - + // Step 2: Accept transfer access_control_core::accept_pending_admin(new_admin); - + // Check transfer completed assert!(access_control_core::get_current_admin() == new_admin_addr, 5); assert!(access_control_core::is_current_admin(new_admin_addr), 6); assert!(!access_control_core::is_current_admin(admin_addr), 7); - + // Check roles transferred assert!(access_control_core::has_role(new_admin_addr), 8); assert!(!access_control_core::has_role(admin_addr), 9); - + // Check pending admin cleaned up assert!(!access_control_core::has_pending_admin(admin_addr), 10); } #[test(admin = @movekit, new_admin = @0x123)] - fun test_transfer_admin_cancel_flow(admin: &signer, new_admin: &signer) { + fun test_transfer_admin_cancel_flow( + admin: &signer, new_admin: &signer + ) { // Setup access_control_core::init_for_testing(admin); - + let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); - + // Propose transfer access_control_core::transfer_admin(admin, new_admin_addr); assert!(access_control_core::has_pending_admin(admin_addr), 0); - + // Cancel transfer access_control_core::cancel_admin_transfer(admin); - + // Check admin unchanged assert!(access_control_core::get_current_admin() == admin_addr, 1); assert!(access_control_core::is_current_admin(admin_addr), 2); assert!(access_control_core::has_role(admin_addr), 3); - + // Check pending admin cleaned up assert!(!access_control_core::has_pending_admin(admin_addr), 4); } #[test(admin = @movekit, new_admin = @0x123)] - fun test_transfer_admin_multiple_proposals(admin: &signer, new_admin: &signer) { + fun test_transfer_admin_multiple_proposals( + admin: &signer, new_admin: &signer + ) { // Setup access_control_core::init_for_testing(admin); - + let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); let another_addr = @0x456; - + // First proposal access_control_core::transfer_admin(admin, new_admin_addr); assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 0); - + // Second proposal overwrites first access_control_core::transfer_admin(admin, another_addr); assert!(access_control_core::get_pending_admin(admin_addr) == another_addr, 1); @@ -161,42 +171,61 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit)] - #[expected_failure(abort_code = E_SELF_TRANSFER_NOT_ALLOWED, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_SELF_TRANSFER_NOT_ALLOWED, + location = movekit::access_control_core + ) + ] fun test_transfer_admin_to_self(admin: &signer) { access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); - + // Should fail - cannot transfer to self access_control_core::transfer_admin(admin, admin_addr); } #[test(new_admin = @0x123)] - #[expected_failure(abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core)] + #[expected_failure( + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + )] fun test_accept_pending_admin_no_pending(new_admin: &signer) { // Should fail - admin registry not initialized (fails before checking pending admin) access_control_core::accept_pending_admin(new_admin); } #[test(admin = @movekit, new_admin = @0x123)] - #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] - fun test_accept_pending_admin_no_pending_initialized(admin: &signer, new_admin: &signer) { + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + ) + ] + fun test_accept_pending_admin_no_pending_initialized( + admin: &signer, new_admin: &signer + ) { // Setup admin but no pending transfer access_control_core::init_for_testing(admin); - + // Should fail - no pending admin transfer (but admin registry is initialized) access_control_core::accept_pending_admin(new_admin); } #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] - #[expected_failure(abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core)] - fun test_accept_pending_admin_wrong_address(admin: &signer, new_admin: &signer, wrong_admin: &signer) { + #[ + expected_failure( + abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core + ) + ] + fun test_accept_pending_admin_wrong_address( + admin: &signer, new_admin: &signer, wrong_admin: &signer + ) { // Setup access_control_core::init_for_testing(admin); let new_admin_addr = signer::address_of(new_admin); - + // Propose transfer to new_admin access_control_core::transfer_admin(admin, new_admin_addr); - + // Should fail - wrong_admin tries to accept access_control_core::accept_pending_admin(wrong_admin); } @@ -209,16 +238,24 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit)] - #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + ) + ] fun test_cancel_admin_transfer_no_pending(admin: &signer) { access_control_core::init_for_testing(admin); - + // Should fail - no pending transfer to cancel access_control_core::cancel_admin_transfer(admin); } #[test] - #[expected_failure(abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + ) + ] fun test_get_pending_admin_no_pending() { // Should fail - no pending admin access_control_core::get_pending_admin(@0x123); @@ -232,9 +269,9 @@ module movekit::access_control_core_tests { fun test_grant_role_success(admin: &signer, user: &signer) { // Setup: admin gets Admin role access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // Test granting a role assert!(!access_control_core::has_role(user_addr), 0); access_control_core::grant_role(admin, user); @@ -242,12 +279,16 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, user = @0x123)] - #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] fun test_grant_role_already_has_role(admin: &signer, user: &signer) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, user); - + // This should fail - user already has Treasurer role access_control_core::grant_role(admin, user); } @@ -264,9 +305,9 @@ module movekit::access_control_core_tests { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, user); - + let user_addr = signer::address_of(user); - + // Test revoking a role assert!(access_control_core::has_role(user_addr), 0); access_control_core::revoke_role(admin, user_addr); @@ -274,13 +315,15 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, user = @0x123)] - #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] + #[expected_failure( + abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core + )] fun test_revoke_role_no_such_role(admin: &signer, user: &signer) { // Setup access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // This should fail - user doesn't have Treasurer role access_control_core::revoke_role(admin, user_addr); } @@ -289,55 +332,69 @@ module movekit::access_control_core_tests { #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] fun test_revoke_role_not_admin(non_admin: &signer, user: &signer) { let user_addr = signer::address_of(user); - + // Test that non-admin cannot revoke roles access_control_core::revoke_role(non_admin, user_addr); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - fun test_transfer_role_success(admin: &signer, from_user: &signer, to_user: &signer) { + fun test_transfer_role_success( + admin: &signer, from_user: &signer, to_user: &signer + ) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, from_user); - + let from_addr = signer::address_of(from_user); let to_addr = signer::address_of(to_user); - + // Test transferring a role assert!(access_control_core::has_role(from_addr), 0); assert!(!access_control_core::has_role(to_addr), 1); - + access_control_core::transfer_role(admin, from_user, to_user); - + assert!(!access_control_core::has_role(from_addr), 2); assert!(access_control_core::has_role(to_addr), 3); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] - fun test_transfer_role_from_no_role(admin: &signer, from_user: &signer, to_user: &signer) { + #[expected_failure( + abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core + )] + fun test_transfer_role_from_no_role( + admin: &signer, from_user: &signer, to_user: &signer + ) { // Setup access_control_core::init_for_testing(admin); - + // This should fail - from_user doesn't have Manager role access_control_core::transfer_role(admin, from_user, to_user); } #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[expected_failure(abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core)] - fun test_transfer_role_to_already_has_role(admin: &signer, from_user: &signer, to_user: &signer) { + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_transfer_role_to_already_has_role( + admin: &signer, from_user: &signer, to_user: &signer + ) { // Setup access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, from_user); access_control_core::grant_role(admin, to_user); - + // This should fail - to_user already has Manager role access_control_core::transfer_role(admin, from_user, to_user); } #[test(non_admin = @0x123, from_user = @0x456, to_user = @0x789)] #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_transfer_role_not_admin(non_admin: &signer, from_user: &signer, to_user: &signer) { + fun test_transfer_role_not_admin( + non_admin: &signer, from_user: &signer, to_user: &signer + ) { // Test that non-admin cannot transfer roles access_control_core::transfer_role(non_admin, from_user, to_user); } @@ -347,49 +404,61 @@ module movekit::access_control_core_tests { // =========================================== #[test(admin = @movekit, admin2 = @0x123, admin3 = @0x456)] - fun test_admin_transfer_chain(admin: &signer, admin2: &signer, admin3: &signer) { + fun test_admin_transfer_chain( + admin: &signer, admin2: &signer, admin3: &signer + ) { // Test transferring admin through a chain access_control_core::init_for_testing(admin); - + let admin_addr = signer::address_of(admin); let admin2_addr = signer::address_of(admin2); let admin3_addr = signer::address_of(admin3); - + // Transfer 1: admin -> admin2 access_control_core::transfer_admin(admin, admin2_addr); access_control_core::accept_pending_admin(admin2); - + assert!(access_control_core::get_current_admin() == admin2_addr, 0); assert!(access_control_core::has_role(admin2_addr), 1); assert!(!access_control_core::has_role(admin_addr), 2); - - // Transfer 2: admin2 -> admin3 + + // Transfer 2: admin2 -> admin3 access_control_core::transfer_admin(admin2, admin3_addr); access_control_core::accept_pending_admin(admin3); - + assert!(access_control_core::get_current_admin() == admin3_addr, 3); assert!(access_control_core::has_role(admin3_addr), 4); assert!(!access_control_core::has_role(admin2_addr), 5); } #[test(admin = @movekit, new_admin = @0x123)] - fun test_new_admin_can_manage_roles(admin: &signer, new_admin: &signer) { + fun test_new_admin_can_manage_roles( + admin: &signer, new_admin: &signer + ) { // Test that new admin can manage roles after transfer access_control_core::init_for_testing(admin); - + let new_admin_addr = signer::address_of(new_admin); - + // Transfer admin access_control_core::transfer_admin(admin, new_admin_addr); access_control_core::accept_pending_admin(new_admin); - + // New admin should be able to grant roles access_control_core::grant_role(new_admin, admin); - assert!(access_control_core::has_role(signer::address_of(admin)), 0); - + assert!( + access_control_core::has_role(signer::address_of(admin)), + 0 + ); + // And revoke roles - access_control_core::revoke_role(new_admin, signer::address_of(admin)); - assert!(!access_control_core::has_role(signer::address_of(admin)), 1); + access_control_core::revoke_role( + new_admin, signer::address_of(admin) + ); + assert!( + !access_control_core::has_role(signer::address_of(admin)), + 1 + ); } // =========================================== @@ -400,13 +469,15 @@ module movekit::access_control_core_tests { fun test_require_role_success(admin: &signer, user: &signer) { access_control_core::init_for_testing(admin); access_control_core::grant_role(admin, user); - + // Should not abort access_control_core::require_role(user); } #[test(user = @0x123)] - #[expected_failure(abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core)] + #[expected_failure( + abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core + )] fun test_require_role_fails_without_role(user: &signer) { // Should abort - user doesn't have role access_control_core::require_role(user); @@ -417,21 +488,23 @@ module movekit::access_control_core_tests { // =========================================== #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] - fun test_multiple_roles_different_users(admin: &signer, user1: &signer, user2: &signer) { + fun test_multiple_roles_different_users( + admin: &signer, user1: &signer, user2: &signer + ) { // Test that different users can have different roles access_control_core::init_for_testing(admin); - + let user1_addr = signer::address_of(user1); let user2_addr = signer::address_of(user2); - + // Grant different roles to different users access_control_core::grant_role(admin, user1); access_control_core::grant_role(admin, user2); - + // Verify roles assert!(access_control_core::has_role(user1_addr), 0); assert!(!access_control_core::has_role(user1_addr), 1); - + assert!(access_control_core::has_role(user2_addr), 2); assert!(!access_control_core::has_role(user2_addr), 3); } @@ -440,22 +513,22 @@ module movekit::access_control_core_tests { fun test_multiple_roles_same_user(admin: &signer, user: &signer) { // Test that same user can have multiple roles access_control_core::init_for_testing(admin); - + let user_addr = signer::address_of(user); - + // Grant multiple roles to same user access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); - + // Verify all roles assert!(access_control_core::has_role(user_addr), 0); assert!(access_control_core::has_role(user_addr), 1); assert!(access_control_core::has_role(user_addr), 2); - + // Revoke one role, others should remain access_control_core::revoke_role(admin, user_addr); - + assert!(access_control_core::has_role(user_addr), 3); assert!(!access_control_core::has_role(user_addr), 4); assert!(access_control_core::has_role(user_addr), 5); @@ -468,4 +541,4 @@ module movekit::access_control_core_tests { assert!(!access_control_core::has_role(user_addr), 0); assert!(!access_control_core::has_role(user_addr), 1); } -} \ No newline at end of file +} From 952828f1f1852e93a7b9458ef3f8a88198338ff1 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 12:23:38 +0200 Subject: [PATCH 08/25] Formatting --- sources/access_control/core.move | 4 +--- tests/access_control/core_test.move | 16 ++++++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 08fe6b8..8099303 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -235,9 +235,7 @@ module movekit::access_control_core { ); move_to>(admin, Role {}); - event::emit( - RoleGrantedEvent { admin: admin_addr, target: admin_addr } - ); + event::emit(RoleGrantedEvent { admin: admin_addr, target: admin_addr }); } /// Grants a role to an address diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index ea979fe..bd21658 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -46,9 +46,11 @@ module movekit::access_control_core_tests { } #[test] - #[expected_failure( - abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core - )] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + ) + ] fun test_get_current_admin_fails_when_not_initialized() { // Should fail - no admin registry exists access_control_core::get_current_admin(); @@ -186,9 +188,11 @@ module movekit::access_control_core_tests { } #[test(new_admin = @0x123)] - #[expected_failure( - abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core - )] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + ) + ] fun test_accept_pending_admin_no_pending(new_admin: &signer) { // Should fail - admin registry not initialized (fails before checking pending admin) access_control_core::accept_pending_admin(new_admin); From b8b4b548ae40bc1a36b72d3c0244e16566ccaeae Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 16:03:42 +0200 Subject: [PATCH 09/25] Add .coverage_map to .gitignore --- .gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 243f545..5441311 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ .aptos/ -build/ \ No newline at end of file +build/ +.coverage_map* \ No newline at end of file From fabb455db67f435954431ffba2a73beac7cdf456 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 16:56:01 +0200 Subject: [PATCH 10/25] refactor: migrate to global registry and drop double-signer grant/revoke --- sources/access_control/core.move | 219 ++++++++++++++++++------------- 1 file changed, 126 insertions(+), 93 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 8099303..7f292e7 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -4,14 +4,18 @@ module movekit::access_control_core { use std::signer; use std::event; + use std::type_info::{Self, TypeInfo}; + use aptos_std::table::{Self, Table}; + use std::vector; // -- Types Section -- // - /// Marker resource: "address *A* holds role *T*" - struct Role has key, drop {} + /// Global role registry - stores all roles for all addresses + struct RoleRegistry has key { + roles: Table> // address -> list of role types they have + } /// Global admin registry - stores current admin address - /// TODO: Consider making this generic/per-module in future versions struct AdminRegistry has key { current_admin: address } @@ -21,7 +25,7 @@ module movekit::access_control_core { pending_admin: address } - /// Built-in admin tag. Other modules may define their own empty structs. + /// Built-in admin tag struct Admin has copy, drop {} // -- Constants Section -- // @@ -49,13 +53,6 @@ module movekit::access_control_core { target: address } - #[event] - struct RoleTransferredEvent has copy, drop, store { - admin: address, - from: address, - to: address - } - #[event] struct AdminTransferProposed has copy, drop, store { current_admin: address, @@ -91,7 +88,9 @@ module movekit::access_control_core { registry.current_admin == addr } - /// Step 1: Current admin proposes new admin + // -- Admin Transfer Functions -- + + /// Current admin proposes new admin public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { let admin_addr = signer::address_of(admin); assert!(is_current_admin(admin_addr), E_NOT_ADMIN); @@ -110,8 +109,10 @@ module movekit::access_control_core { ); } - /// Step 2: New admin accepts the transfer - public fun accept_pending_admin(new_admin: &signer) acquires Role, PendingAdmin, AdminRegistry { + /// New admin accepts the transfer + public fun accept_pending_admin( + new_admin: &signer + ) acquires PendingAdmin, AdminRegistry, RoleRegistry { let new_admin_addr = signer::address_of(new_admin); let current_admin = get_current_admin(); @@ -119,9 +120,10 @@ module movekit::access_control_core { let pending = borrow_global(current_admin); assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); - // Transfer the admin role - move_from>(current_admin); - move_to>(new_admin, Role {}); + // Transfer the admin role in registry + grant_role_internal(new_admin_addr); + // Grant first then revoke + revoke_role_internal(current_admin); // Update admin registry let registry = borrow_global_mut(@movekit); @@ -164,119 +166,150 @@ module movekit::access_control_core { exists(admin) } - /// Grants a role to an address - public fun grant_role(admin: &signer, target: &signer) acquires AdminRegistry { - grant_role_internal(admin, target); - } + /// Admin grants role immediately + public fun grant_role(admin: &signer, target: address) acquires AdminRegistry, RoleRegistry { + assert!(is_current_admin(signer::address_of(admin)), E_NOT_ADMIN); + assert!(!has_role(target), E_ALREADY_HAS_ROLE); - /// Revokes a role from an address - public fun revoke_role(admin: &signer, target: address) acquires Role, AdminRegistry { - revoke_role_internal(admin, target); - } + grant_role_internal(target); - /// Transfers a role from one address to another - public fun transfer_role( - admin: &signer, from: &signer, to: &signer - ) acquires Role, AdminRegistry { - assert!( - is_current_admin(signer::address_of(admin)), - E_NOT_ADMIN - ); - - assert!( - exists>(signer::address_of(from)), - E_NO_SUCH_ROLE + event::emit( + RoleGrantedEvent { admin: signer::address_of(admin), target: target } ); + } - assert!( - !exists>(signer::address_of(to)), - E_ALREADY_HAS_ROLE - ); + /// Admin revokes role immediately (standard practice) + public fun revoke_role(admin: &signer, target: address) acquires AdminRegistry, RoleRegistry { + assert!(is_current_admin(signer::address_of(admin)), E_NOT_ADMIN); + assert!(has_role(target), E_NO_SUCH_ROLE); - move_from>(signer::address_of(from)); - move_to>(to, Role {}); + revoke_role_internal(target); event::emit( - RoleTransferredEvent { - admin: signer::address_of(admin), - from: signer::address_of(from), - to: signer::address_of(to) - } + RoleRevokedEvent { admin: signer::address_of(admin), target: target } ); } - // View helper to check whether an address currently holds role T + // -- View Functions -- + + #[view] + /// Check if address has specific role + public fun has_role(addr: address): bool acquires RoleRegistry { + if (!exists(@movekit)) return false; + + let registry = borrow_global(@movekit); + if (!table::contains(®istry.roles, addr)) + return false; + + let user_roles = table::borrow(®istry.roles, addr); + let target_type = type_info::type_of(); + + vector_contains(user_roles, &target_type) + } + + #[view] + /// Get all roles for an address + public fun get_roles(addr: address): vector acquires RoleRegistry { + if (!exists(@movekit)) return vector::empty(); + + let registry = borrow_global(@movekit); + if (!table::contains(®istry.roles, addr)) + return vector::empty(); + + *table::borrow(®istry.roles, addr) + } + #[view] - public fun has_role(addr: address): bool { - exists>(addr) + /// Count total roles for an address + public fun get_role_count(addr: address): u64 acquires RoleRegistry { + vector::length(&get_roles(addr)) } /// Utility function - assert account has role or abort - public fun require_role(account: &signer) { + public fun require_role(account: &signer) acquires RoleRegistry { assert!( has_role(signer::address_of(account)), E_NO_SUCH_ROLE ); } - // -- Internal Functions Section -- // + // -- Internal Functions -- - /// Bootstrap: give the deployer the Admin role and create admin registry - fun init_module(admin: &signer) { - let admin_addr = signer::address_of(admin); + /// Internal function to grant role using global registry + fun grant_role_internal(target: address) acquires RoleRegistry { + let registry = borrow_global_mut(@movekit); + let role_type = type_info::type_of(); - // Create admin registry first - move_to(admin, AdminRegistry { current_admin: admin_addr }); + if (!table::contains(®istry.roles, target)) { + table::add(&mut registry.roles, target, vector::empty()); + }; - // Grant Admin role to caller - assert!( - !exists>(admin_addr), - E_ALREADY_HAS_ROLE - ); - move_to>(admin, Role {}); + let user_roles = table::borrow_mut(&mut registry.roles, target); - event::emit(RoleGrantedEvent { admin: admin_addr, target: admin_addr }); + if (!vector_contains(user_roles, &role_type)) { + vector::push_back(user_roles, role_type); + } } - /// Grants a role to an address - fun grant_role_internal(admin: &signer, target: &signer) acquires AdminRegistry { - assert!( - is_current_admin(signer::address_of(admin)), - E_NOT_ADMIN - ); + /// Internal function to revoke role using global registry + fun revoke_role_internal(target: address) acquires RoleRegistry { + let registry = borrow_global_mut(@movekit); + let role_type = type_info::type_of(); - assert!( - !exists>(signer::address_of(target)), - E_ALREADY_HAS_ROLE - ); - move_to>(target, Role {}); + let user_roles = table::borrow_mut(&mut registry.roles, target); + let (found, index) = vector_find(user_roles, &role_type); + assert!(found, E_NO_SUCH_ROLE); - event::emit( - RoleGrantedEvent { - admin: signer::address_of(admin), - target: signer::address_of(target) - } - ); + vector::remove(user_roles, index); + + if (vector::is_empty(user_roles)) { + table::remove(&mut registry.roles, target); + } } - /// Revokes a role from an address - fun revoke_role_internal(admin: &signer, target: address) acquires Role, AdminRegistry { - assert!( - is_current_admin(signer::address_of(admin)), - E_NOT_ADMIN - ); + /// Helper function to check if vector contains element + fun vector_contains(vec: &vector, item: &TypeInfo): bool { + let (found, _) = vector_find(vec, item); + found + } - assert!(exists>(target), E_NO_SUCH_ROLE); - move_from>(target); + /// Helper function to find element in vector + fun vector_find(vec: &vector, item: &TypeInfo): (bool, u64) { + let len = vector::length(vec); + let i = 0; + while (i < len) { + if (vector::borrow(vec, i) == item) { + return (true, i) + }; + i = i + 1; + }; + (false, 0) + } - event::emit( - RoleRevokedEvent { admin: signer::address_of(admin), target: target } + /// Bootstrap: give the deployer the Admin role and create registries + fun init_module(admin: &signer) acquires RoleRegistry { + let admin_addr = signer::address_of(admin); + + // Create admin registry + move_to(admin, AdminRegistry { current_admin: admin_addr }); + + // Create global role registry + move_to( + admin, + RoleRegistry { + roles: table::new>() + } ); + + // Grant Admin role to deployer + grant_role_internal(admin_addr); + + event::emit(RoleGrantedEvent { admin: admin_addr, target: admin_addr }); } // Public init function for testing only #[test_only] - public fun init_for_testing(admin: &signer) { + public fun init_for_testing(admin: &signer) acquires RoleRegistry { init_module(admin); } } From 0f102d60ee7d579e291e26026e93fb38193e29e5 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 20 Jun 2025 16:57:03 +0200 Subject: [PATCH 11/25] test: Add test for new pattern implementation --- tests/access_control/core_test.move | 547 +++++++++++++++++++++------- 1 file changed, 407 insertions(+), 140 deletions(-) diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index bd21658..4b3e0d7 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -1,6 +1,7 @@ #[test_only] module movekit::access_control_core_tests { use std::signer; + use std::vector; use movekit::access_control_core::{Self, Admin}; // Test role types @@ -33,8 +34,11 @@ module movekit::access_control_core_tests { assert!(access_control_core::get_current_admin() == deployer_addr, 0); assert!(access_control_core::is_current_admin(deployer_addr), 1); - // Check admin role was granted + // Check admin role was granted in global registry assert!(access_control_core::has_role(deployer_addr), 2); + + // Check role count + assert!(access_control_core::get_role_count(deployer_addr) == 1, 3); } #[test(deployer = @movekit)] @@ -111,7 +115,7 @@ module movekit::access_control_core_tests { assert!(access_control_core::is_current_admin(new_admin_addr), 6); assert!(!access_control_core::is_current_admin(admin_addr), 7); - // Check roles transferred + // Check roles transferred in global registry assert!(access_control_core::has_role(new_admin_addr), 8); assert!(!access_control_core::has_role(admin_addr), 9); @@ -187,6 +191,26 @@ module movekit::access_control_core_tests { access_control_core::transfer_admin(admin, admin_addr); } + #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] + #[ + expected_failure( + abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core + ) + ] + fun test_accept_pending_admin_wrong_address( + admin: &signer, new_admin: &signer, wrong_admin: &signer + ) { + // Setup + access_control_core::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose transfer to new_admin + access_control_core::transfer_admin(admin, new_admin_addr); + + // Should fail - wrong_admin tries to accept + access_control_core::accept_pending_admin(wrong_admin); + } + #[test(new_admin = @0x123)] #[ expected_failure( @@ -214,26 +238,6 @@ module movekit::access_control_core_tests { access_control_core::accept_pending_admin(new_admin); } - #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] - #[ - expected_failure( - abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core - ) - ] - fun test_accept_pending_admin_wrong_address( - admin: &signer, new_admin: &signer, wrong_admin: &signer - ) { - // Setup - access_control_core::init_for_testing(admin); - let new_admin_addr = signer::address_of(new_admin); - - // Propose transfer to new_admin - access_control_core::transfer_admin(admin, new_admin_addr); - - // Should fail - wrong_admin tries to accept - access_control_core::accept_pending_admin(wrong_admin); - } - #[test(non_admin = @0x123)] #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] fun test_cancel_admin_transfer_not_admin(non_admin: &signer) { @@ -266,20 +270,187 @@ module movekit::access_control_core_tests { } // =========================================== - // ROLE MANAGEMENT TESTS (Updated for new admin pattern) + // SECURITY ATTACK PREVENTION TESTS + // =========================================== + + #[test(attacker = @0x999)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_privilege_escalation_prevention_grant(attacker: &signer) { + // Attacker tries to grant themselves admin role without proper authority + access_control_core::grant_role(attacker, signer::address_of(attacker)); + } + + #[test(attacker = @0x999)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_privilege_escalation_prevention_transfer(attacker: &signer) { + // Attacker tries to transfer admin to themselves + access_control_core::transfer_admin(attacker, signer::address_of(attacker)); + } + + #[test(admin = @movekit, attacker = @0x999)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_unauthorized_role_grant_prevention( + admin: &signer, attacker: &signer + ) { + access_control_core::init_for_testing(admin); + + // Attacker tries to grant roles to others + access_control_core::grant_role(attacker, @0x123); + } + + #[test(admin = @movekit, attacker = @0x999)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_unauthorized_role_revoke_prevention( + admin: &signer, attacker: &signer + ) { + access_control_core::init_for_testing(admin); + + // Grant role to someone + access_control_core::grant_role(admin, @0x123); + + // Attacker tries to revoke roles from others + access_control_core::revoke_role(attacker, @0x123); + } + + #[test(admin = @movekit, attacker = @0x999)] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + fun test_unauthorized_admin_cancel_prevention( + admin: &signer, attacker: &signer + ) { + access_control_core::init_for_testing(admin); + + // Admin proposes transfer + access_control_core::transfer_admin(admin, @0x123); + + // Attacker tries to cancel admin transfer + access_control_core::cancel_admin_transfer(attacker); + } + + // =========================================== + // STATE CONSISTENCY TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_role_registry_admin_sync(admin: &signer) { + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Verify AdminRegistry and RoleRegistry are in sync + assert!(access_control_core::get_current_admin() == admin_addr, 0); + assert!(access_control_core::has_role(admin_addr), 1); + assert!(access_control_core::is_current_admin(admin_addr), 2); + + // Test consistency after granting other roles + access_control_core::grant_role(admin, admin_addr); + + // Admin should still be admin + assert!(access_control_core::get_current_admin() == admin_addr, 3); + assert!(access_control_core::has_role(admin_addr), 4); + assert!(access_control_core::has_role(admin_addr), 5); + assert!(access_control_core::get_role_count(admin_addr) == 2, 6); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_admin_transfer_maintains_role_consistency( + admin: &signer, new_admin: &signer + ) { + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Grant admin some additional roles + access_control_core::grant_role(admin, admin_addr); + access_control_core::grant_role(admin, admin_addr); + + // Verify initial state + assert!(access_control_core::get_role_count(admin_addr) == 3, 0); // Admin + Treasurer + Manager + assert!(access_control_core::get_role_count(new_admin_addr) == 0, 1); + + // Transfer admin role + access_control_core::transfer_admin(admin, new_admin_addr); + access_control_core::accept_pending_admin(new_admin); + + // Verify role consistency after transfer + assert!(access_control_core::get_role_count(admin_addr) == 2, 2); // Treasurer + Manager (lost Admin) + assert!(access_control_core::get_role_count(new_admin_addr) == 1, 3); // Admin only + + assert!(!access_control_core::has_role(admin_addr), 4); + assert!(access_control_core::has_role(new_admin_addr), 5); + assert!(access_control_core::has_role(admin_addr), 6); // Should keep other roles + assert!(access_control_core::has_role(admin_addr), 7); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_admin_role_consistency_during_transfer( + admin: &signer, new_admin: &signer + ) { + // Test that admin role count remains exactly 1 throughout transfer + access_control_core::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Initially: 1 admin role (in old admin) + assert!(access_control_core::has_role(admin_addr), 0); + assert!(!access_control_core::has_role(new_admin_addr), 1); + + // Propose transfer - still 1 admin role + access_control_core::transfer_admin(admin, new_admin_addr); + assert!(access_control_core::has_role(admin_addr), 2); + assert!(!access_control_core::has_role(new_admin_addr), 3); + + // Accept transfer - still 1 admin role (but transferred) + access_control_core::accept_pending_admin(new_admin); + assert!(!access_control_core::has_role(admin_addr), 4); + assert!(access_control_core::has_role(new_admin_addr), 5); + + // AdminRegistry and RoleRegistry should be in sync + assert!(access_control_core::get_current_admin() == new_admin_addr, 6); + assert!(access_control_core::is_current_admin(new_admin_addr), 7); + } + + // =========================================== + // USEFUL EDGE CASE TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_role_operations_on_nonexistent_users(admin: &signer) { + access_control_core::init_for_testing(admin); + + // Test granting roles to addresses that don't exist in table yet + let nonexistent = @0xDEADBEEF; + + // Should be able to grant role to new address + access_control_core::grant_role(admin, nonexistent); + assert!(access_control_core::has_role(nonexistent), 0); + assert!(access_control_core::get_role_count(nonexistent) == 1, 1); + + // Should be able to grant multiple roles + access_control_core::grant_role(admin, nonexistent); + assert!(access_control_core::has_role(nonexistent), 2); + assert!(access_control_core::get_role_count(nonexistent) == 2, 3); + } + + // =========================================== + // REAL-WORLD ROLE MANAGEMENT TESTS // =========================================== #[test(admin = @movekit, user = @0x123)] fun test_grant_role_success(admin: &signer, user: &signer) { - // Setup: admin gets Admin role access_control_core::init_for_testing(admin); let user_addr = signer::address_of(user); // Test granting a role assert!(!access_control_core::has_role(user_addr), 0); - access_control_core::grant_role(admin, user); - assert!(access_control_core::has_role(user_addr), 1); + assert!(access_control_core::get_role_count(user_addr) == 0, 1); + + access_control_core::grant_role(admin, user_addr); + + assert!(access_control_core::has_role(user_addr), 2); + assert!(access_control_core::get_role_count(user_addr) == 1, 3); } #[test(admin = @movekit, user = @0x123)] @@ -289,33 +460,35 @@ module movekit::access_control_core_tests { ) ] fun test_grant_role_already_has_role(admin: &signer, user: &signer) { - // Setup access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, user); + let user_addr = signer::address_of(user); + access_control_core::grant_role(admin, user_addr); // This should fail - user already has Treasurer role - access_control_core::grant_role(admin, user); + access_control_core::grant_role(admin, user_addr); } - #[test(non_admin = @0x123, user = @0x456)] + #[test(non_admin = @0x123)] #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_grant_role_not_admin(non_admin: &signer, user: &signer) { + fun test_grant_role_not_admin(non_admin: &signer) { // Test that non-admin cannot grant roles - access_control_core::grant_role(non_admin, user); + access_control_core::grant_role(non_admin, @0x456); } #[test(admin = @movekit, user = @0x123)] fun test_revoke_role_success(admin: &signer, user: &signer) { - // Setup access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, user); - let user_addr = signer::address_of(user); + access_control_core::grant_role(admin, user_addr); // Test revoking a role assert!(access_control_core::has_role(user_addr), 0); + assert!(access_control_core::get_role_count(user_addr) == 1, 1); + access_control_core::revoke_role(admin, user_addr); - assert!(!access_control_core::has_role(user_addr), 1); + + assert!(!access_control_core::has_role(user_addr), 2); + assert!(access_control_core::get_role_count(user_addr) == 0, 3); } #[test(admin = @movekit, user = @0x123)] @@ -323,84 +496,111 @@ module movekit::access_control_core_tests { abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core )] fun test_revoke_role_no_such_role(admin: &signer, user: &signer) { - // Setup access_control_core::init_for_testing(admin); - let user_addr = signer::address_of(user); // This should fail - user doesn't have Treasurer role access_control_core::revoke_role(admin, user_addr); } - #[test(non_admin = @0x123, user = @0x456)] + #[test(non_admin = @0x123)] #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_revoke_role_not_admin(non_admin: &signer, user: &signer) { - let user_addr = signer::address_of(user); - + fun test_revoke_role_not_admin(non_admin: &signer) { // Test that non-admin cannot revoke roles - access_control_core::revoke_role(non_admin, user_addr); + access_control_core::revoke_role(non_admin, @0x456); } - #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - fun test_transfer_role_success( - admin: &signer, from_user: &signer, to_user: &signer + // =========================================== + // GLOBAL REGISTRY TESTS + // =========================================== + + #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] + fun test_multiple_roles_different_users( + admin: &signer, user1: &signer, user2: &signer ) { - // Setup access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, from_user); - let from_addr = signer::address_of(from_user); - let to_addr = signer::address_of(to_user); + let user1_addr = signer::address_of(user1); + let user2_addr = signer::address_of(user2); - // Test transferring a role - assert!(access_control_core::has_role(from_addr), 0); - assert!(!access_control_core::has_role(to_addr), 1); + // Grant different roles to different users + access_control_core::grant_role(admin, user1_addr); + access_control_core::grant_role(admin, user2_addr); - access_control_core::transfer_role(admin, from_user, to_user); + // Verify roles + assert!(access_control_core::has_role(user1_addr), 0); + assert!(!access_control_core::has_role(user1_addr), 1); + assert!(access_control_core::get_role_count(user1_addr) == 1, 2); - assert!(!access_control_core::has_role(from_addr), 2); - assert!(access_control_core::has_role(to_addr), 3); + assert!(access_control_core::has_role(user2_addr), 3); + assert!(!access_control_core::has_role(user2_addr), 4); + assert!(access_control_core::get_role_count(user2_addr) == 1, 5); } - #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[expected_failure( - abort_code = E_NO_SUCH_ROLE, location = movekit::access_control_core - )] - fun test_transfer_role_from_no_role( - admin: &signer, from_user: &signer, to_user: &signer - ) { - // Setup + #[test(admin = @movekit, user = @0x123)] + fun test_multiple_roles_same_user(admin: &signer, user: &signer) { access_control_core::init_for_testing(admin); - // This should fail - from_user doesn't have Manager role - access_control_core::transfer_role(admin, from_user, to_user); + let user_addr = signer::address_of(user); + + // Grant multiple roles to same user + access_control_core::grant_role(admin, user_addr); + access_control_core::grant_role(admin, user_addr); + access_control_core::grant_role(admin, user_addr); + + // Verify all roles + assert!(access_control_core::has_role(user_addr), 0); + assert!(access_control_core::has_role(user_addr), 1); + assert!(access_control_core::has_role(user_addr), 2); + assert!(access_control_core::get_role_count(user_addr) == 3, 3); + + // Revoke one role, others should remain + access_control_core::revoke_role(admin, user_addr); + + assert!(access_control_core::has_role(user_addr), 4); + assert!(!access_control_core::has_role(user_addr), 5); + assert!(access_control_core::has_role(user_addr), 6); + assert!(access_control_core::get_role_count(user_addr) == 2, 7); } - #[test(admin = @movekit, from_user = @0x123, to_user = @0x456)] - #[ - expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core - ) - ] - fun test_transfer_role_to_already_has_role( - admin: &signer, from_user: &signer, to_user: &signer - ) { - // Setup + #[test(admin = @movekit, user = @0x123)] + fun test_get_roles_function(admin: &signer, user: &signer) { access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, from_user); - access_control_core::grant_role(admin, to_user); - // This should fail - to_user already has Manager role - access_control_core::transfer_role(admin, from_user, to_user); + let user_addr = signer::address_of(user); + + // Initially no roles + let roles = access_control_core::get_roles(user_addr); + assert!(vector::length(&roles) == 0, 0); + + // Grant some roles + access_control_core::grant_role(admin, user_addr); + access_control_core::grant_role(admin, user_addr); + + // Check roles vector + let roles = access_control_core::get_roles(user_addr); + assert!(vector::length(&roles) == 2, 1); + + // Revoke one role + access_control_core::revoke_role(admin, user_addr); + + let roles = access_control_core::get_roles(user_addr); + assert!(vector::length(&roles) == 1, 2); } - #[test(non_admin = @0x123, from_user = @0x456, to_user = @0x789)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] - fun test_transfer_role_not_admin( - non_admin: &signer, from_user: &signer, to_user: &signer - ) { - // Test that non-admin cannot transfer roles - access_control_core::transfer_role(non_admin, from_user, to_user); + #[test(user = @0x123)] + fun test_has_role_no_role(user: &signer) { + let user_addr = signer::address_of(user); + assert!(!access_control_core::has_role(user_addr), 0); + assert!(!access_control_core::has_role(user_addr), 1); + assert!(access_control_core::get_role_count(user_addr) == 0, 2); + } + + #[test] + fun test_get_roles_no_registry() { + let roles = access_control_core::get_roles(@0x123); + assert!(vector::length(&roles) == 0, 0); + assert!(access_control_core::get_role_count(@0x123) == 0, 1); } // =========================================== @@ -411,7 +611,6 @@ module movekit::access_control_core_tests { fun test_admin_transfer_chain( admin: &signer, admin2: &signer, admin3: &signer ) { - // Test transferring admin through a chain access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); @@ -439,9 +638,9 @@ module movekit::access_control_core_tests { fun test_new_admin_can_manage_roles( admin: &signer, new_admin: &signer ) { - // Test that new admin can manage roles after transfer access_control_core::init_for_testing(admin); + let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Transfer admin @@ -449,20 +648,12 @@ module movekit::access_control_core_tests { access_control_core::accept_pending_admin(new_admin); // New admin should be able to grant roles - access_control_core::grant_role(new_admin, admin); - assert!( - access_control_core::has_role(signer::address_of(admin)), - 0 - ); + access_control_core::grant_role(new_admin, admin_addr); + assert!(access_control_core::has_role(admin_addr), 0); // And revoke roles - access_control_core::revoke_role( - new_admin, signer::address_of(admin) - ); - assert!( - !access_control_core::has_role(signer::address_of(admin)), - 1 - ); + access_control_core::revoke_role(new_admin, admin_addr); + assert!(!access_control_core::has_role(admin_addr), 1); } // =========================================== @@ -472,7 +663,8 @@ module movekit::access_control_core_tests { #[test(admin = @movekit, user = @0x123)] fun test_require_role_success(admin: &signer, user: &signer) { access_control_core::init_for_testing(admin); - access_control_core::grant_role(admin, user); + let user_addr = signer::address_of(user); + access_control_core::grant_role(admin, user_addr); // Should not abort access_control_core::require_role(user); @@ -487,62 +679,137 @@ module movekit::access_control_core_tests { access_control_core::require_role(user); } + #[test(admin = @movekit, user = @0x123)] + fun test_require_role_admin(admin: &signer, user: &signer) { + access_control_core::init_for_testing(admin); + + // Admin should have Admin role + access_control_core::require_role(admin); + + // User should not have Admin role + let user_addr = signer::address_of(user); + assert!(!access_control_core::has_role(user_addr), 0); + } + // =========================================== - // COMPLEX SCENARIO TESTS + // EDGE CASES AND ERROR HANDLING // =========================================== - #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] - fun test_multiple_roles_different_users( - admin: &signer, user1: &signer, user2: &signer - ) { - // Test that different users can have different roles - access_control_core::init_for_testing(admin); + #[test] + fun test_has_role_with_no_registry() { + // Test has_role when RoleRegistry doesn't exist + assert!(!access_control_core::has_role(@0x123), 0); + assert!(!access_control_core::has_role(@0x456), 1); + } - let user1_addr = signer::address_of(user1); - let user2_addr = signer::address_of(user2); + #[test(admin = @movekit)] + fun test_grant_revoke_same_role_multiple_times(admin: &signer) { + access_control_core::init_for_testing(admin); + let target = @0x123; - // Grant different roles to different users - access_control_core::grant_role(admin, user1); - access_control_core::grant_role(admin, user2); + // Grant, revoke, grant again + access_control_core::grant_role(admin, target); + assert!(access_control_core::has_role(target), 0); - // Verify roles - assert!(access_control_core::has_role(user1_addr), 0); - assert!(!access_control_core::has_role(user1_addr), 1); + access_control_core::revoke_role(admin, target); + assert!(!access_control_core::has_role(target), 1); - assert!(access_control_core::has_role(user2_addr), 2); - assert!(!access_control_core::has_role(user2_addr), 3); + access_control_core::grant_role(admin, target); + assert!(access_control_core::has_role(target), 2); } - #[test(admin = @movekit, user = @0x123)] - fun test_multiple_roles_same_user(admin: &signer, user: &signer) { - // Test that same user can have multiple roles + #[test(admin = @movekit)] + fun test_large_number_of_roles(admin: &signer) { access_control_core::init_for_testing(admin); + let user = @0x123; - let user_addr = signer::address_of(user); - - // Grant multiple roles to same user + // Grant multiple different role types to same user access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); + access_control_core::grant_role(admin, user); - // Verify all roles - assert!(access_control_core::has_role(user_addr), 0); - assert!(access_control_core::has_role(user_addr), 1); - assert!(access_control_core::has_role(user_addr), 2); + // Verify all roles exist + assert!(access_control_core::has_role(user), 0); + assert!(access_control_core::has_role(user), 1); + assert!(access_control_core::has_role(user), 2); + assert!(access_control_core::has_role(user), 3); + assert!(access_control_core::get_role_count(user) == 4, 4); + } - // Revoke one role, others should remain - access_control_core::revoke_role(admin, user_addr); + #[test(admin = @movekit, new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + ) + ] + fun test_double_accept_pending_admin_fails( + admin: &signer, new_admin: &signer + ) { + // Normal transfer + access_control_core::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + access_control_core::transfer_admin(admin, new_admin_addr); - assert!(access_control_core::has_role(user_addr), 3); - assert!(!access_control_core::has_role(user_addr), 4); - assert!(access_control_core::has_role(user_addr), 5); + // First accept succeeds + access_control_core::accept_pending_admin(new_admin); + // Second accept must fail → E_NO_PENDING_ADMIN + access_control_core::accept_pending_admin(new_admin); } - #[test(user = @0x123)] - fun test_has_role_no_role(user: &signer) { - // Test has_role returns false for non-existent role - let user_addr = signer::address_of(user); - assert!(!access_control_core::has_role(user_addr), 0); - assert!(!access_control_core::has_role(user_addr), 1); + #[test(admin = @movekit)] + #[ + expected_failure( + abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + ) + ] + fun test_admin_cannot_grant_admin_twice(admin: &signer) { + // Bootstrap + access_control_core::init_for_testing(admin); + let admin_addr = signer::address_of(admin); + + // Current admin **already** owns the Admin role; trying again must abort + access_control_core::grant_role(admin, admin_addr); + } + + #[test(current_admin = @movekit, future_admin = @0x123)] + fun test_admin_role_not_duplicated_after_transfer( + current_admin: &signer, future_admin: &signer + ) { + access_control_core::init_for_testing(current_admin); + + let fut_addr = signer::address_of(future_admin); + + // Pre-grant Admin to the future admin + access_control_core::grant_role(current_admin, fut_addr); + assert!(access_control_core::get_role_count(fut_addr) == 1, 0); + + // Transfer governance + access_control_core::transfer_admin(current_admin, fut_addr); + access_control_core::accept_pending_admin(future_admin); + + assert!(access_control_core::get_role_count(fut_addr) == 1, 1); + } + + #[test(current_admin = @movekit, future_admin = @0x123)] + fun test_single_revoke_clears_admin_role( + current_admin: &signer, future_admin: &signer + ) { + // Setup identical to duplication test, except duplicates can’t occur. + access_control_core::init_for_testing(current_admin); + let fut_addr = signer::address_of(future_admin); + + // Pre-grant Admin, then transfer governance. + access_control_core::grant_role(current_admin, fut_addr); + access_control_core::transfer_admin(current_admin, fut_addr); + access_control_core::accept_pending_admin(future_admin); + + assert!(access_control_core::get_role_count(fut_addr) == 1, 0); + + // Revoke once. + access_control_core::revoke_role(future_admin, fut_addr); + + assert!(!access_control_core::has_role(fut_addr), 1); + assert!(access_control_core::get_role_count(fut_addr) == 0, 2); } } From 11a2ac2e01ce6017c1a3e62c61c51cf4302c23d2 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sat, 21 Jun 2025 16:28:03 +0200 Subject: [PATCH 12/25] feat: add secure admin registry with two-step transfer --- sources/access_control/admin_registry.move | 155 ++++ tests/access_control/admin_registry_test.move | 821 ++++++++++++++++++ 2 files changed, 976 insertions(+) create mode 100644 sources/access_control/admin_registry.move create mode 100644 tests/access_control/admin_registry_test.move diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move new file mode 100644 index 0000000..676014e --- /dev/null +++ b/sources/access_control/admin_registry.move @@ -0,0 +1,155 @@ +module movekit::access_control_admin_registry { + + use std::signer; + use std::event; + + friend movekit::access_control_core; + + struct AdminRegistry has key { + current_admin: address + } + + struct PendingAdmin has key, drop { + pending_admin: address + } + + // -- Constants -- // + + const E_NOT_INITIALIZED: u64 = 0; + const E_NOT_ADMIN: u64 = 1; + const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 2; + const E_NO_PENDING_ADMIN: u64 = 3; + const E_NOT_PENDING_ADMIN: u64 = 4; + const E_ALREADY_INITIALIZED: u64 = 5; + + // -- Events -- // + + #[event] + struct AdminTransferProposed has copy, drop, store { + current_admin: address, + pending_admin: address + } + + #[event] + struct AdminTransferCompleted has copy, drop, store { + old_admin: address, + new_admin: address + } + + #[event] + struct AdminTransferCanceled has copy, drop, store { + admin: address, + canceled_pending: address + } + + // -- Public Functions -- // + + #[view] + public fun get_current_admin(): address acquires AdminRegistry { + assert!(exists(@movekit), E_NOT_INITIALIZED); + borrow_global(@movekit).current_admin + } + + #[view] + public fun is_current_admin(addr: address): bool acquires AdminRegistry { + get_current_admin() == addr + } + + public fun require_admin(admin: &signer) acquires AdminRegistry { + assert!(exists(@movekit), E_NOT_INITIALIZED); + let registry = borrow_global(@movekit); + assert!(registry.current_admin == signer::address_of(admin), E_NOT_ADMIN); + } + + /// Current admin proposes new admin + public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { + require_admin(admin); + let admin_addr = signer::address_of(admin); + assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); + + // Set or update pending admin + if (exists(admin_addr)) { + let pending = borrow_global_mut(admin_addr); + pending.pending_admin = new_admin; + } else { + move_to(admin, PendingAdmin { pending_admin: new_admin }); + }; + + event::emit( + AdminTransferProposed { current_admin: admin_addr, pending_admin: new_admin } + ); + } + + /// New admin accepts the transfer + public fun accept_pending_admin(new_admin: &signer) acquires AdminRegistry, PendingAdmin { + let new_admin_addr = signer::address_of(new_admin); + let current_admin_addr = get_current_admin(); + + // Check that there's a pending admin transfer + assert!(exists(current_admin_addr), E_NO_PENDING_ADMIN); + + // Get the pending admin info + let pending = borrow_global(current_admin_addr); + + // Verify that the caller is the intended new admin + assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); + + // Update AdminRegistry to point to new admin + let registry = borrow_global_mut(@movekit); + let old_admin = registry.current_admin; + registry.current_admin = new_admin_addr; + + // Clean up: Remove PendingAdmin resource from old admin's address + let _removed_pending = move_from(current_admin_addr); + + // Emit completion event + event::emit( + AdminTransferCompleted { old_admin: old_admin, new_admin: new_admin_addr } + ); + } + + /// Cancel pending admin transfer + public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry, PendingAdmin { + let admin_addr = signer::address_of(admin); + require_admin(admin); + assert!(exists(admin_addr), E_NO_PENDING_ADMIN); + + let pending = move_from(admin_addr); + + event::emit( + AdminTransferCanceled { + admin: admin_addr, + canceled_pending: pending.pending_admin + } + ); + } + + #[view] + /// Get pending admin address + public fun get_pending_admin(): address acquires AdminRegistry, PendingAdmin { + let current_admin = get_current_admin(); + assert!(exists(current_admin), E_NO_PENDING_ADMIN); + borrow_global(current_admin).pending_admin + } + + #[view] + /// Check if there's a pending admin transfer + public fun has_pending_admin(admin: address): bool { + exists(admin) + } + + // -- Private Functions -- // + + fun init_module(admin: &signer) { + let admin_addr = signer::address_of(admin); + // admin signer represents @movekit during deployment + move_to(admin, AdminRegistry { current_admin: admin_addr }); + } + + #[test_only] + public fun init_for_testing(admin: &signer) { + //let admin_addr = signer::address_of(admin); + //move_to(admin, AdminRegistry { current_admin: admin_addr }); + init_module(admin); + } +} diff --git a/tests/access_control/admin_registry_test.move b/tests/access_control/admin_registry_test.move new file mode 100644 index 0000000..f2d263e --- /dev/null +++ b/tests/access_control/admin_registry_test.move @@ -0,0 +1,821 @@ +#[test_only] +module movekit::access_control_admin_registry_tests { + use std::signer; + use movekit::access_control_admin_registry; + + // Test constants matching the module + const E_NOT_INITIALIZED: u64 = 0; + const E_NOT_ADMIN: u64 = 1; + const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 2; + const E_NO_PENDING_ADMIN: u64 = 3; + const E_NOT_PENDING_ADMIN: u64 = 4; + const E_ALREADY_INITIALIZED: u64 = 5; + + // =========================================== + // INITIALIZATION TESTS + // =========================================== + + #[test(deployer = @movekit)] + fun test_init_module_success(deployer: &signer) { + access_control_admin_registry::init_for_testing(deployer); + + let deployer_addr = signer::address_of(deployer); + + // Should set deployer as current admin + assert!(access_control_admin_registry::get_current_admin() == deployer_addr, 0); + assert!(access_control_admin_registry::is_current_admin(deployer_addr), 1); + + // No pending admin initially + assert!(!access_control_admin_registry::has_pending_admin(deployer_addr), 2); + } + + #[test(deployer = @movekit)] + #[expected_failure] + fun test_double_initialization_fails(deployer: &signer) { + access_control_admin_registry::init_for_testing(deployer); + // Second initialization should fail + access_control_admin_registry::init_for_testing(deployer); + } + + #[test] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] + fun test_get_current_admin_not_initialized() { + // Should fail when not initialized + access_control_admin_registry::get_current_admin(); + } + + #[test] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] + fun test_is_current_admin_not_initialized() { + // Should fail when not initialized (now that is_current_admin calls get_current_admin) + access_control_admin_registry::is_current_admin(@0x123); + } + + // =========================================== + // ADMIN CHECK TESTS + // =========================================== + + #[test(deployer = @movekit)] + fun test_require_admin_success(deployer: &signer) { + access_control_admin_registry::init_for_testing(deployer); + + // Should not abort for actual admin + access_control_admin_registry::require_admin(deployer); + } + + #[test(deployer = @movekit)] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] + fun test_require_admin_not_initialized(deployer: &signer) { + access_control_admin_registry::require_admin(deployer); + } + + #[test(deployer = @movekit, non_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry + ) + ] + fun test_require_admin_fails_for_non_admin( + deployer: &signer, non_admin: &signer + ) { + access_control_admin_registry::init_for_testing(deployer); + + // Should fail for non-admin + access_control_admin_registry::require_admin(non_admin); + } + + #[test(deployer = @movekit)] + fun test_is_current_admin_various_addresses(deployer: &signer) { + access_control_admin_registry::init_for_testing(deployer); + + let deployer_addr = signer::address_of(deployer); + + assert!(access_control_admin_registry::is_current_admin(deployer_addr), 0); + assert!(!access_control_admin_registry::is_current_admin(@0x123), 1); + assert!(!access_control_admin_registry::is_current_admin(@0x456), 2); + } + + // =========================================== + // TWO-STEP TRANSFER TESTS + // =========================================== + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_complete_flow( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Step 1: Propose transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + + // Check pending state + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 1); + + // Admin should still be current + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 2); + assert!(access_control_admin_registry::is_current_admin(admin_addr), 3); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 4); + + // Step 2: Accept transfer + access_control_admin_registry::accept_pending_admin(new_admin); + + // Check transfer completed + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 5); + assert!(access_control_admin_registry::is_current_admin(new_admin_addr), 6); + assert!(!access_control_admin_registry::is_current_admin(admin_addr), 7); + + // Check pending admin cleaned up + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 8); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_transfer_admin_overwrite_proposal( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let new_admin_addr = signer::address_of(new_admin); + let another_addr = @0x456; + + // First proposal + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 0); + + // Second proposal overwrites first + access_control_admin_registry::transfer_admin(admin, another_addr); + assert!(access_control_admin_registry::get_pending_admin() == another_addr, 1); + + // Old proposal is no longer valid + assert!(access_control_admin_registry::get_pending_admin() != new_admin_addr, 2); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_cancel_admin_transfer(admin: &signer, new_admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // Cancel transfer + access_control_admin_registry::cancel_admin_transfer(admin); + + // Check admin unchanged + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); + assert!(access_control_admin_registry::is_current_admin(admin_addr), 2); + + // Check pending admin cleaned up + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + } + + // =========================================== + // ERROR CONDITION TESTS + // =========================================== + + #[test(admin = @movekit, non_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry + ) + ] + fun test_transfer_admin_not_admin(admin: &signer, non_admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + access_control_admin_registry::transfer_admin(non_admin, @0x456); + } + + #[test(admin = @movekit)] + #[ + expected_failure( + abort_code = E_SELF_TRANSFER_NOT_ALLOWED, + location = movekit::access_control_admin_registry + ) + ] + fun test_transfer_admin_to_self(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + let admin_addr = signer::address_of(admin); + + // Should fail - cannot transfer to self + access_control_admin_registry::transfer_admin(admin, admin_addr); + } + + #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] + #[ + expected_failure( + abort_code = E_NOT_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_accept_pending_admin_wrong_address( + admin: &signer, new_admin: &signer, wrong_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose transfer to new_admin + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + + // Should fail - wrong_admin tries to accept + access_control_admin_registry::accept_pending_admin(wrong_admin); + } + + #[test(new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] + fun test_accept_pending_admin_not_initialized(new_admin: &signer) { + // Should fail - admin registry not initialized + access_control_admin_registry::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_accept_pending_admin_no_pending( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + // Should fail - no pending admin transfer + access_control_admin_registry::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, non_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry + ) + ] + fun test_cancel_admin_transfer_not_admin( + admin: &signer, non_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + // Should fail - not admin + access_control_admin_registry::cancel_admin_transfer(non_admin); + } + + #[test(admin = @movekit)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_cancel_admin_transfer_no_pending(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // Should fail - no pending transfer to cancel + access_control_admin_registry::cancel_admin_transfer(admin); + } + + #[test(admin = @movekit)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_get_pending_admin_no_pending(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + // Should fail - no pending admin + access_control_admin_registry::get_pending_admin(); + } + + // =========================================== + // CORNER CASE TESTS + // =========================================== + + #[test(admin = @movekit, new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NO_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_double_accept_pending_admin_fails( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Normal transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + + // First accept succeeds + access_control_admin_registry::accept_pending_admin(new_admin); + + // Second accept must fail + access_control_admin_registry::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, new_admin = @0x123)] + #[ + expected_failure( + abort_code = E_NOT_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_accept_after_overwrite_fails( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Original proposal + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + + // Admin overwrites with different address + access_control_admin_registry::transfer_admin(admin, @0x456); + + // Original new_admin can no longer accept + access_control_admin_registry::accept_pending_admin(new_admin); + } + + #[test(admin = @movekit, new_admin1 = @0x123, new_admin2 = @0x456)] + fun test_transfer_chain( + admin: &signer, new_admin1: &signer, new_admin2: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin1_addr = signer::address_of(new_admin1); + let new_admin2_addr = signer::address_of(new_admin2); + + // Transfer 1: admin -> new_admin1 + access_control_admin_registry::transfer_admin(admin, new_admin1_addr); + access_control_admin_registry::accept_pending_admin(new_admin1); + + assert!(access_control_admin_registry::get_current_admin() == new_admin1_addr, 0); + assert!(!access_control_admin_registry::is_current_admin(admin_addr), 1); + + // Transfer 2: new_admin1 -> new_admin2 + access_control_admin_registry::transfer_admin(new_admin1, new_admin2_addr); + access_control_admin_registry::accept_pending_admin(new_admin2); + + assert!(access_control_admin_registry::get_current_admin() == new_admin2_addr, 2); + assert!(!access_control_admin_registry::is_current_admin(new_admin1_addr), 3); + assert!(!access_control_admin_registry::is_current_admin(admin_addr), 4); + } + + // =========================================== + // SPECIAL ADDRESS TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_transfer_to_zero_address(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // Should allow transfer to zero address (for renouncing) + access_control_admin_registry::transfer_admin(admin, @0x0); + + let admin_addr = signer::address_of(admin); + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::get_pending_admin() == @0x0, 1); + } + + #[test(admin = @movekit)] + #[ + expected_failure( + abort_code = E_SELF_TRANSFER_NOT_ALLOWED, + location = movekit::access_control_admin_registry + ) + ] + fun test_self_transfer_module_address(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // Should fail - transferring to @movekit when admin is @movekit is self-transfer + access_control_admin_registry::transfer_admin(admin, @movekit); + } + + // =========================================== + // VIEW FUNCTION TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_has_pending_admin_various_states(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Initially no pending admin + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // After proposing transfer + access_control_admin_registry::transfer_admin(admin, @0x123); + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 1); + + // After canceling + access_control_admin_registry::cancel_admin_transfer(admin); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + } + + #[test] + fun test_has_pending_admin_nonexistent_address() { + // Should return false for addresses that never had pending admin + assert!(!access_control_admin_registry::has_pending_admin(@0x123), 0); + assert!(!access_control_admin_registry::has_pending_admin(@0x456), 1); + } + + // =========================================== + // SECURITY TESTS + // =========================================== + + #[test(admin = @movekit, attacker = @0x999)] + #[ + expected_failure( + abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry + ) + ] + fun test_privilege_escalation_prevention_transfer( + admin: &signer, attacker: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + // Attacker tries to transfer admin to themselves without being admin + access_control_admin_registry::transfer_admin( + attacker, signer::address_of(attacker) + ); + } + + #[test(admin = @movekit, attacker = @0x999)] + #[ + expected_failure( + abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry + ) + ] + fun test_privilege_escalation_prevention_cancel( + admin: &signer, attacker: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + // Attacker tries to cancel admin transfer without being admin + access_control_admin_registry::cancel_admin_transfer(attacker); + } + + #[test(admin = @movekit, attacker = @0x999)] + #[ + expected_failure( + abort_code = E_NOT_PENDING_ADMIN, + location = movekit::access_control_admin_registry + ) + ] + fun test_unauthorized_accept_prevention( + admin: &signer, attacker: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + // Admin proposes transfer to someone else + access_control_admin_registry::transfer_admin(admin, @0x123); + + // Attacker tries to accept transfer meant for someone else + access_control_admin_registry::accept_pending_admin(attacker); + } + + // =========================================== + // EDGE CASE TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_multiple_proposal_overwrites(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // Multiple proposals should overwrite each other + access_control_admin_registry::transfer_admin(admin, @0x111); + assert!(access_control_admin_registry::get_pending_admin() == @0x111, 0); + + access_control_admin_registry::transfer_admin(admin, @0x222); + assert!(access_control_admin_registry::get_pending_admin() == @0x222, 1); + + access_control_admin_registry::transfer_admin(admin, @0x333); + assert!(access_control_admin_registry::get_pending_admin() == @0x333, 2); + + // Only the last proposal is valid + assert!(access_control_admin_registry::get_pending_admin() != @0x111, 3); + assert!(access_control_admin_registry::get_pending_admin() != @0x222, 4); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_pending_admin_cleanup_after_accept( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose and accept transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + access_control_admin_registry::accept_pending_admin(new_admin); + + // Old admin should no longer have pending admin + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // New admin should not have pending admin either + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 1); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_pending_admin_cleanup_after_cancel( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Propose and cancel transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + access_control_admin_registry::cancel_admin_transfer(admin); + + // Admin should no longer have pending admin + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // New admin should not have pending admin + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 1); + } + + // =========================================== + // STORAGE LEAK CHECKS + // =========================================== + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_no_pending_admin_leaks_after_accept( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Setup transfer + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + + // Verify pending admin exists + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // Accept transfer + access_control_admin_registry::accept_pending_admin(new_admin); + + // Verify NO pending admin resources exist anywhere + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 2); + assert!(!access_control_admin_registry::has_pending_admin(@movekit), 3); + + // Test random addresses too + assert!(!access_control_admin_registry::has_pending_admin(@0x999), 4); + assert!(!access_control_admin_registry::has_pending_admin(@0x888), 5); + } + + #[test(admin = @movekit)] + fun test_no_pending_admin_leaks_after_cancel(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Setup transfer + access_control_admin_registry::transfer_admin(admin, @0x123); + + // Verify pending admin exists + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + + // Cancel transfer + access_control_admin_registry::cancel_admin_transfer(admin); + + // Verify NO pending admin resources exist anywhere + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + assert!(!access_control_admin_registry::has_pending_admin(@0x123), 2); + assert!(!access_control_admin_registry::has_pending_admin(@movekit), 3); + } + + #[test(admin = @movekit)] + fun test_no_pending_admin_leaks_after_overwrite(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Multiple overwrites + access_control_admin_registry::transfer_admin(admin, @0x111); + access_control_admin_registry::transfer_admin(admin, @0x222); + access_control_admin_registry::transfer_admin(admin, @0x333); + + // Should only have ONE pending admin (the latest) + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::get_pending_admin() == @0x333, 1); + + // Cancel and verify clean slate + access_control_admin_registry::cancel_admin_transfer(admin); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + } + + // =========================================== + // CONCURRENT/RAPID SUCCESSION TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_rapid_proposal_overwrites(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // Rapid succession proposals (testing internal state consistency) + access_control_admin_registry::transfer_admin(admin, @0x111); + assert!(access_control_admin_registry::get_pending_admin() == @0x111, 0); + + access_control_admin_registry::transfer_admin(admin, @0x222); + assert!(access_control_admin_registry::get_pending_admin() == @0x222, 1); + + access_control_admin_registry::transfer_admin(admin, @0x333); + assert!(access_control_admin_registry::get_pending_admin() == @0x333, 2); + + // Verify only the last one is valid + assert!(access_control_admin_registry::get_pending_admin() != @0x111, 3); + assert!(access_control_admin_registry::get_pending_admin() != @0x222, 4); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_propose_cancel_propose_sequence( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Rapid sequence: propose -> cancel -> propose -> accept + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + access_control_admin_registry::cancel_admin_transfer(admin); + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + access_control_admin_registry::accept_pending_admin(new_admin); + + // Verify final state is correct + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 0); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + } + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_interleaved_operations(admin: &signer, new_admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Complex sequence testing state machine robustness + access_control_admin_registry::transfer_admin(admin, @0x999); // Propose to someone else + access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Overwrite + + // Verify state is correct before accept + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 0); + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); + + // Accept should work + access_control_admin_registry::accept_pending_admin(new_admin); + + // Verify final state + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 2); + } + + // =========================================== + // EDGE CASE OPERATIONAL TESTS + // =========================================== + + #[test(admin = @movekit)] + fun test_view_functions_consistency_under_state_changes( + admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Initial state verification + assert!(access_control_admin_registry::is_current_admin(admin_addr), 0); + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + + // After proposal + access_control_admin_registry::transfer_admin(admin, @0x123); + assert!(access_control_admin_registry::is_current_admin(admin_addr), 3); // Still current + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 4); // Still current + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 5); // Now has pending + + // After cancel + access_control_admin_registry::cancel_admin_transfer(admin); + assert!(access_control_admin_registry::is_current_admin(admin_addr), 6); // Still current + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 7); // Still current + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 8); // No longer pending + } + + #[test(admin = @movekit)] + fun test_resource_existence_consistency(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + // AdminRegistry should always exist after initialization + assert!( + access_control_admin_registry::is_current_admin(signer::address_of(admin)), + 0 + ); + + // These operations shouldn't affect AdminRegistry existence + access_control_admin_registry::transfer_admin(admin, @0x123); + assert!( + access_control_admin_registry::is_current_admin(signer::address_of(admin)), + 1 + ); + + access_control_admin_registry::cancel_admin_transfer(admin); + assert!( + access_control_admin_registry::is_current_admin(signer::address_of(admin)), + 2 + ); + + // PendingAdmin should be created and destroyed properly + let admin_addr = signer::address_of(admin); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + + access_control_admin_registry::transfer_admin(admin, @0x123); + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 4); + + access_control_admin_registry::cancel_admin_transfer(admin); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 5); + } + + // =========================================== + // SYSTEM INVARIANT TESTS + // =========================================== + + #[test(admin = @movekit, new_admin = @0x123)] + fun test_admin_uniqueness_invariant( + admin: &signer, new_admin: &signer + ) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + let new_admin_addr = signer::address_of(new_admin); + + // Throughout the entire transfer process, exactly one address should be admin + + // Initial: admin is admin, new_admin is not + assert!(access_control_admin_registry::is_current_admin(admin_addr), 0); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 1); + + // During proposal: admin still admin, new_admin still not + access_control_admin_registry::transfer_admin(admin, new_admin_addr); + assert!(access_control_admin_registry::is_current_admin(admin_addr), 2); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 3); + + // After transfer: new_admin is admin, admin is not + access_control_admin_registry::accept_pending_admin(new_admin); + assert!(!access_control_admin_registry::is_current_admin(admin_addr), 4); + assert!(access_control_admin_registry::is_current_admin(new_admin_addr), 5); + + // Verify no other addresses are admin + assert!(!access_control_admin_registry::is_current_admin(@0x999), 6); + assert!(!access_control_admin_registry::is_current_admin(@movekit), 7); + } + + #[test(admin = @movekit)] + fun test_state_machine_invariants(admin: &signer) { + access_control_admin_registry::init_for_testing(admin); + + let admin_addr = signer::address_of(admin); + + // Invariant: AdminRegistry always exists after initialization + assert!(access_control_admin_registry::get_current_admin() == admin_addr, 0); + + // Invariant: PendingAdmin exists iff there's an active proposal + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + + access_control_admin_registry::transfer_admin(admin, @0x123); + assert!(access_control_admin_registry::has_pending_admin(admin_addr), 2); + + access_control_admin_registry::cancel_admin_transfer(admin); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + + // Invariant: get_current_admin() always returns a valid address + let current = access_control_admin_registry::get_current_admin(); + assert!(current == admin_addr, 4); // Should be a concrete address, not null + } +} From 42f7bef0baf0fc284ffe3d2a74d15d75ddfc84d3 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sat, 21 Jun 2025 17:39:10 +0200 Subject: [PATCH 13/25] feat: integrate admin registry with role synchronization --- sources/access_control/admin_registry.move | 13 +- sources/access_control/core.move | 313 ++++++++++++--------- tests/access_control/core_test.move | 249 ++++++++-------- 3 files changed, 319 insertions(+), 256 deletions(-) diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move index 676014e..ca763f9 100644 --- a/sources/access_control/admin_registry.move +++ b/sources/access_control/admin_registry.move @@ -1,5 +1,4 @@ module movekit::access_control_admin_registry { - use std::signer; use std::event; @@ -14,7 +13,6 @@ module movekit::access_control_admin_registry { } // -- Constants -- // - const E_NOT_INITIALIZED: u64 = 0; const E_NOT_ADMIN: u64 = 1; const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 2; @@ -23,7 +21,6 @@ module movekit::access_control_admin_registry { const E_ALREADY_INITIALIZED: u64 = 5; // -- Events -- // - #[event] struct AdminTransferProposed has copy, drop, store { current_admin: address, @@ -138,6 +135,14 @@ module movekit::access_control_admin_registry { exists(admin) } + /// Allow friend modules to initialize admin registry (idempotent) + public(friend) fun init_admin_registry(admin: &signer) { + if (!exists(@movekit)) { + let admin_addr = signer::address_of(admin); + move_to(admin, AdminRegistry { current_admin: admin_addr }); + } + } + // -- Private Functions -- // fun init_module(admin: &signer) { @@ -148,8 +153,6 @@ module movekit::access_control_admin_registry { #[test_only] public fun init_for_testing(admin: &signer) { - //let admin_addr = signer::address_of(admin); - //move_to(admin, AdminRegistry { current_admin: admin_addr }); init_module(admin); } } diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 7f292e7..bf80aa3 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -1,203 +1,193 @@ module movekit::access_control_core { - // -- Std & Framework Helpers -- // + // -- Dependencies -- // use std::signer; use std::event; use std::type_info::{Self, TypeInfo}; use aptos_std::table::{Self, Table}; use std::vector; + use movekit::access_control_admin_registry; - // -- Types Section -- // + // -- Core Types -- // - /// Global role registry - stores all roles for all addresses + /// Global role registry mapping addresses to role types struct RoleRegistry has key { - roles: Table> // address -> list of role types they have + /// Maps addresses to their assigned role types + roles: Table> } - /// Global admin registry - stores current admin address - struct AdminRegistry has key { - current_admin: address - } - - /// Stores pending admin transfer - struct PendingAdmin has key, drop { - pending_admin: address - } - - /// Built-in admin tag + /// Built-in Admin role type (managed via transfer only) struct Admin has copy, drop {} - // -- Constants Section -- // + // -- Error Codes -- // - /// Error codes + /// Unauthorized access attempt - caller lacks required permissions const E_NOT_ADMIN: u64 = 0; + /// Role already assigned to target address const E_ALREADY_HAS_ROLE: u64 = 1; + /// Attempted to revoke non-existent role const E_NO_SUCH_ROLE: u64 = 2; - const E_NO_PENDING_ADMIN: u64 = 3; - const E_NOT_PENDING_ADMIN: u64 = 4; - const E_NOT_INITIALIZED: u64 = 5; - const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 6; + /// Attempted operation on uninitialized system + const E_NOT_INITIALIZED: u64 = 3; + /// Admin role cannot be manually managed - use admin transfer instead + const E_ADMIN_ROLE_PROTECTED: u64 = 4; + /// State corruption detected between admin registry and role registry + const E_STATE_CORRUPTION: u64 = 5; - // -- Events Section -- // + // -- Events -- // #[event] + /// Emitted when a role is successfully granted to an address struct RoleGrantedEvent has copy, drop, store { + /// Address of admin who granted the role admin: address, + /// Address that received the role target: address } #[event] + /// Emitted when a role is successfully revoked from an address struct RoleRevokedEvent has copy, drop, store { + /// Address of admin who revoked the role admin: address, + /// Address that lost the role target: address } #[event] - struct AdminTransferProposed has copy, drop, store { - current_admin: address, - pending_admin: address - } - - #[event] - struct AdminTransferCompleted has copy, drop, store { + /// Emitted when admin role synchronization occurs during transfer + struct AdminRoleSynchronized has copy, drop, store { + /// Previous admin who lost Admin role old_admin: address, + /// New admin who gained Admin role new_admin: address } - #[event] - struct AdminTransferCanceled has copy, drop, store { - admin: address, - canceled_pending: address - } + // -- Public Interface -- // - // -- External Functions Section -- // + // === Admin Delegation Functions === #[view] - /// Get current admin address - public fun get_current_admin(): address acquires AdminRegistry { - assert!(exists(@movekit), E_NOT_INITIALIZED); - borrow_global(@movekit).current_admin + /// Get current admin address from admin registry + public fun get_current_admin(): address { + access_control_admin_registry::get_current_admin() } #[view] - /// Check if address is current admin - public fun is_current_admin(addr: address): bool acquires AdminRegistry { - if (!exists(@movekit)) return false; - let registry = borrow_global(@movekit); - registry.current_admin == addr + /// Check if given address is the current admin + public fun is_current_admin(addr: address): bool { + access_control_admin_registry::is_current_admin(addr) } - // -- Admin Transfer Functions -- - - /// Current admin proposes new admin - public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { - let admin_addr = signer::address_of(admin); - assert!(is_current_admin(admin_addr), E_NOT_ADMIN); - assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); - - // Set or update pending admin - if (exists(admin_addr)) { - let pending = borrow_global_mut(admin_addr); - pending.pending_admin = new_admin; - } else { - move_to(admin, PendingAdmin { pending_admin: new_admin }); - }; - - event::emit( - AdminTransferProposed { current_admin: admin_addr, pending_admin: new_admin } - ); + /// Propose admin transfer - delegates to admin registry + public fun transfer_admin(admin: &signer, new_admin: address) { + access_control_admin_registry::transfer_admin(admin, new_admin) } - /// New admin accepts the transfer - public fun accept_pending_admin( - new_admin: &signer - ) acquires PendingAdmin, AdminRegistry, RoleRegistry { + /// Accept pending admin transfer and synchronize role assignments + public fun accept_pending_admin(new_admin: &signer) acquires RoleRegistry { let new_admin_addr = signer::address_of(new_admin); - let current_admin = get_current_admin(); - - assert!(exists(current_admin), E_NO_PENDING_ADMIN); - let pending = borrow_global(current_admin); - assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); - - // Transfer the admin role in registry - grant_role_internal(new_admin_addr); - // Grant first then revoke - revoke_role_internal(current_admin); - // Update admin registry - let registry = borrow_global_mut(@movekit); - registry.current_admin = new_admin_addr; + // Capture current state before any modifications + let current_admin_addr = access_control_admin_registry::get_current_admin(); - // Clean up pending admin - move_from(current_admin); - - event::emit( - AdminTransferCompleted { old_admin: current_admin, new_admin: new_admin_addr } + // Validate pending transfer exists and matches caller + assert!( + access_control_admin_registry::has_pending_admin(current_admin_addr), + E_NOT_INITIALIZED + ); + assert!( + access_control_admin_registry::get_pending_admin() == new_admin_addr, + E_NOT_ADMIN ); - } - /// Cancel pending admin transfer - public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry, PendingAdmin { - let admin_addr = signer::address_of(admin); - assert!(is_current_admin(admin_addr), E_NOT_ADMIN); - assert!(exists(admin_addr), E_NO_PENDING_ADMIN); + // Execute admin transfer atomically + access_control_admin_registry::accept_pending_admin(new_admin); - let pending = move_from(admin_addr); + // Synchronize Admin role assignments to maintain consistency + synchronize_admin_role(current_admin_addr, new_admin_addr); + // Emit synchronization event for audit trail event::emit( - AdminTransferCanceled { - admin: admin_addr, - canceled_pending: pending.pending_admin + AdminRoleSynchronized { + old_admin: current_admin_addr, + new_admin: new_admin_addr } ); } + /// Cancel pending admin transfer - delegates to admin registry + public fun cancel_admin_transfer(admin: &signer) { + access_control_admin_registry::cancel_admin_transfer(admin) + } + #[view] - /// Get pending admin address - public fun get_pending_admin(admin: address): address acquires PendingAdmin { - assert!(exists(admin), E_NO_PENDING_ADMIN); - borrow_global(admin).pending_admin + /// Get pending admin address from admin registry + public fun get_pending_admin(): address { + access_control_admin_registry::get_pending_admin() } #[view] - /// Check if there's a pending admin transfer + /// Check if admin has pending transfer public fun has_pending_admin(admin: address): bool { - exists(admin) + access_control_admin_registry::has_pending_admin(admin) } - /// Admin grants role immediately - public fun grant_role(admin: &signer, target: address) acquires AdminRegistry, RoleRegistry { - assert!(is_current_admin(signer::address_of(admin)), E_NOT_ADMIN); + // === Role Management Functions === + + /// Grant role to target address (Admin role; admin-only) + public fun grant_role(admin: &signer, target: address) acquires RoleRegistry { + // Security: Prevent manual Admin role manipulation + assert_admin_role_not_protected(); + + // Authorize admin access + require_admin(admin); + + // Validate role assignment assert!(!has_role(target), E_ALREADY_HAS_ROLE); + // Execute role grant grant_role_internal(target); + // Emit audit event event::emit( RoleGrantedEvent { admin: signer::address_of(admin), target: target } ); } - /// Admin revokes role immediately (standard practice) - public fun revoke_role(admin: &signer, target: address) acquires AdminRegistry, RoleRegistry { - assert!(is_current_admin(signer::address_of(admin)), E_NOT_ADMIN); + /// Revoke role from target address (Admin role; admin-only) + public fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { + // Security: Prevent manual Admin role manipulation + assert_admin_role_not_protected(); + + // Authorize admin access + require_admin(admin); + + // Validate role exists assert!(has_role(target), E_NO_SUCH_ROLE); + // Execute role revocation revoke_role_internal(target); + // Emit audit event event::emit( RoleRevokedEvent { admin: signer::address_of(admin), target: target } ); } - // -- View Functions -- + // === View Functions === #[view] /// Check if address has specific role + /// Returns false gracefully for uninitialized system or non-existent users public fun has_role(addr: address): bool acquires RoleRegistry { + // Handle uninitialized system gracefully if (!exists(@movekit)) return false; let registry = borrow_global(@movekit); + + // Handle non-existent user gracefully if (!table::contains(®istry.roles, addr)) return false; @@ -208,11 +198,15 @@ module movekit::access_control_core { } #[view] - /// Get all roles for an address + /// Get all roles assigned to an address + /// Returns empty vector for uninitialized system or non-existent users public fun get_roles(addr: address): vector acquires RoleRegistry { + // Handle uninitialized system gracefully if (!exists(@movekit)) return vector::empty(); let registry = borrow_global(@movekit); + + // Handle non-existent user gracefully if (!table::contains(®istry.roles, addr)) return vector::empty(); @@ -220,12 +214,13 @@ module movekit::access_control_core { } #[view] - /// Count total roles for an address + /// Count total roles assigned to an address public fun get_role_count(addr: address): u64 acquires RoleRegistry { vector::length(&get_roles(addr)) } - /// Utility function - assert account has role or abort + /// Assert caller has required role or abort with clear error + /// Useful for other modules requiring specific role authorization public fun require_role(account: &signer) acquires RoleRegistry { assert!( has_role(signer::address_of(account)), @@ -233,47 +228,86 @@ module movekit::access_control_core { ); } - // -- Internal Functions -- + // -- Internal Implementation -- // + + /// Synchronize Admin role during admin transfer + /// Ensures exactly one Admin role exists and belongs to current admin + fun synchronize_admin_role(old_admin: address, new_admin: address) acquires RoleRegistry { + // Validate registry is initialized + assert!(exists(@movekit), E_NOT_INITIALIZED); - /// Internal function to grant role using global registry + // Grant Admin role to new admin (safe - handles duplicates) + grant_role_internal(new_admin); + + // Revoke Admin role from old admin (safe - handles non-existence) + revoke_role_internal(old_admin); + + // Verify state consistency after synchronization + assert!(has_role(new_admin), E_STATE_CORRUPTION); + assert!(!has_role(old_admin), E_STATE_CORRUPTION); + } + + /// Internal role granting with duplicate protection fun grant_role_internal(target: address) acquires RoleRegistry { let registry = borrow_global_mut(@movekit); let role_type = type_info::type_of(); + // Initialize user's role vector if needed if (!table::contains(®istry.roles, target)) { table::add(&mut registry.roles, target, vector::empty()); }; let user_roles = table::borrow_mut(&mut registry.roles, target); + // Only add role if not already present (idempotent operation) if (!vector_contains(user_roles, &role_type)) { vector::push_back(user_roles, role_type); } } - /// Internal function to revoke role using global registry + /// Internal role revocation with non-existence protection fun revoke_role_internal(target: address) acquires RoleRegistry { let registry = borrow_global_mut(@movekit); let role_type = type_info::type_of(); + // Handle non-existent user gracefully + if (!table::contains(®istry.roles, target)) + return; + let user_roles = table::borrow_mut(&mut registry.roles, target); let (found, index) = vector_find(user_roles, &role_type); - assert!(found, E_NO_SUCH_ROLE); - vector::remove(user_roles, index); + // Only remove if role exists (idempotent operation) + if (found) { + vector::remove(user_roles, index); - if (vector::is_empty(user_roles)) { - table::remove(&mut registry.roles, target); + // Clean up empty role vectors to save storage + if (vector::is_empty(user_roles)) { + table::remove(&mut registry.roles, target); + } } } - /// Helper function to check if vector contains element + /// Require admin authorization with clear error messaging + fun require_admin(admin: &signer) { + access_control_admin_registry::require_admin(admin); + } + + /// Security check: prevent manual Admin role manipulation + fun assert_admin_role_not_protected() { + assert!( + type_info::type_of() != type_info::type_of(), + E_ADMIN_ROLE_PROTECTED + ); + } + + /// Check if vector contains specific element fun vector_contains(vec: &vector, item: &TypeInfo): bool { let (found, _) = vector_find(vec, item); found } - /// Helper function to find element in vector + /// Find element in vector with safe indexing fun vector_find(vec: &vector, item: &TypeInfo): (bool, u64) { let len = vector::length(vec); let i = 0; @@ -283,33 +317,44 @@ module movekit::access_control_core { }; i = i + 1; }; - (false, 0) + (false, 0) // Return 0 as dummy index when not found } - /// Bootstrap: give the deployer the Admin role and create registries + /// System initialization - creates role registry and grants initial Admin role fun init_module(admin: &signer) acquires RoleRegistry { let admin_addr = signer::address_of(admin); - // Create admin registry - move_to(admin, AdminRegistry { current_admin: admin_addr }); - - // Create global role registry - move_to( - admin, - RoleRegistry { - roles: table::new>() - } - ); + // Initialize admin registry first (idempotent operation) + access_control_admin_registry::init_admin_registry(admin); + + // Create role registry if not already exists + if (!exists(@movekit)) { + move_to( + admin, + RoleRegistry { + roles: table::new>() + } + ); + }; - // Grant Admin role to deployer + // Grant initial Admin role to deployer grant_role_internal(admin_addr); + // Emit initial role grant event event::emit(RoleGrantedEvent { admin: admin_addr, target: admin_addr }); } - // Public init function for testing only + // -- Testing Support -- // + #[test_only] + /// Initialize system for testing purposes public fun init_for_testing(admin: &signer) acquires RoleRegistry { init_module(admin); } + + #[test_only] + /// Test-only function to verify Admin role protection + public fun test_admin_role_protection(): bool { + type_info::type_of() == type_info::type_of() + } } diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 4b3e0d7..5e1fe73 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -11,30 +11,35 @@ module movekit::access_control_core_tests { struct Operator has copy, drop {} - // Test constants + // Core module error codes const E_NOT_ADMIN: u64 = 0; const E_ALREADY_HAS_ROLE: u64 = 1; const E_NO_SUCH_ROLE: u64 = 2; - const E_NO_PENDING_ADMIN: u64 = 3; - const E_NOT_PENDING_ADMIN: u64 = 4; - const E_NOT_INITIALIZED: u64 = 5; - const E_SELF_TRANSFER_NOT_ALLOWED: u64 = 6; + const E_NOT_INITIALIZED: u64 = 3; + const E_ADMIN_ROLE_PROTECTED: u64 = 4; + + // Admin registry error codes (for delegated functions) + const E_ADMIN_NOT_INITIALIZED: u64 = 0; + const E_ADMIN_NOT_ADMIN: u64 = 1; + const E_ADMIN_SELF_TRANSFER_NOT_ALLOWED: u64 = 2; + const E_ADMIN_NO_PENDING_ADMIN: u64 = 3; + const E_ADMIN_NOT_PENDING_ADMIN: u64 = 4; // =========================================== // INITIALIZATION & ADMIN REGISTRY TESTS // =========================================== #[test(deployer = @movekit)] - fun test_init_module_creates_admin_registry(deployer: &signer) { + fun test_init_module_creates_registries(deployer: &signer) { access_control_core::init_for_testing(deployer); let deployer_addr = signer::address_of(deployer); - // Check admin registry was created + // Check admin registry was created (delegated functions work) assert!(access_control_core::get_current_admin() == deployer_addr, 0); assert!(access_control_core::is_current_admin(deployer_addr), 1); - // Check admin role was granted in global registry + // Check admin role was granted in role registry assert!(access_control_core::has_role(deployer_addr), 2); // Check role count @@ -42,28 +47,39 @@ module movekit::access_control_core_tests { } #[test(deployer = @movekit)] - #[expected_failure] - fun test_init_module_fails_if_already_initialized(deployer: &signer) { + fun test_init_module_idempotent_on_double_call(deployer: &signer) { access_control_core::init_for_testing(deployer); - // This should fail with RESOURCE_ALREADY_EXISTS when trying to move_to AdminRegistry again + // Second initialization should be idempotent (no failure) access_control_core::init_for_testing(deployer); + + // Should still work correctly + let deployer_addr = signer::address_of(deployer); + assert!(access_control_core::get_current_admin() == deployer_addr, 0); + assert!(access_control_core::has_role(deployer_addr), 1); } #[test] #[ expected_failure( - abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry ) ] fun test_get_current_admin_fails_when_not_initialized() { - // Should fail - no admin registry exists + // Should fail - no admin registry exists (error comes from admin registry) access_control_core::get_current_admin(); } #[test] - fun test_is_current_admin_returns_false_when_not_initialized() { - // Should return false gracefully when not initialized - assert!(!access_control_core::is_current_admin(@0x123), 0); + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] + fun test_is_current_admin_fails_when_not_initialized() { + // Should fail when not initialized (since core delegates to admin registry) + access_control_core::is_current_admin(@0x123); } #[test(deployer = @movekit)] @@ -73,10 +89,8 @@ module movekit::access_control_core_tests { let deployer_addr = signer::address_of(deployer); let other_addr = @0x123; - // Test get_current_admin + // Test delegated admin functions assert!(access_control_core::get_current_admin() == deployer_addr, 0); - - // Test is_current_admin assert!(access_control_core::is_current_admin(deployer_addr), 1); assert!(!access_control_core::is_current_admin(other_addr), 2); } @@ -89,25 +103,24 @@ module movekit::access_control_core_tests { fun test_transfer_admin_complete_flow( admin: &signer, new_admin: &signer ) { - // Setup access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); - // Step 1: Propose transfer + // Step 1: Propose transfer (delegated to admin registry) access_control_core::transfer_admin(admin, new_admin_addr); - // Check pending state + // Check pending state (delegated functions) assert!(access_control_core::has_pending_admin(admin_addr), 0); - assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 1); + assert!(access_control_core::get_pending_admin() == new_admin_addr, 1); // Admin should still be current assert!(access_control_core::get_current_admin() == admin_addr, 2); assert!(access_control_core::is_current_admin(admin_addr), 3); assert!(!access_control_core::is_current_admin(new_admin_addr), 4); - // Step 2: Accept transfer + // Step 2: Accept transfer (core coordinates admin registry + role management) access_control_core::accept_pending_admin(new_admin); // Check transfer completed @@ -115,11 +128,11 @@ module movekit::access_control_core_tests { assert!(access_control_core::is_current_admin(new_admin_addr), 6); assert!(!access_control_core::is_current_admin(admin_addr), 7); - // Check roles transferred in global registry + // Check roles transferred in role registry assert!(access_control_core::has_role(new_admin_addr), 8); assert!(!access_control_core::has_role(admin_addr), 9); - // Check pending admin cleaned up + // Check pending admin cleaned up (delegated to admin registry) assert!(!access_control_core::has_pending_admin(admin_addr), 10); } @@ -127,7 +140,6 @@ module movekit::access_control_core_tests { fun test_transfer_admin_cancel_flow( admin: &signer, new_admin: &signer ) { - // Setup access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); @@ -137,7 +149,7 @@ module movekit::access_control_core_tests { access_control_core::transfer_admin(admin, new_admin_addr); assert!(access_control_core::has_pending_admin(admin_addr), 0); - // Cancel transfer + // Cancel transfer (delegated to admin registry) access_control_core::cancel_admin_transfer(admin); // Check admin unchanged @@ -153,120 +165,125 @@ module movekit::access_control_core_tests { fun test_transfer_admin_multiple_proposals( admin: &signer, new_admin: &signer ) { - // Setup access_control_core::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); let another_addr = @0x456; // First proposal access_control_core::transfer_admin(admin, new_admin_addr); - assert!(access_control_core::get_pending_admin(admin_addr) == new_admin_addr, 0); + assert!(access_control_core::get_pending_admin() == new_admin_addr, 0); // Second proposal overwrites first access_control_core::transfer_admin(admin, another_addr); - assert!(access_control_core::get_pending_admin(admin_addr) == another_addr, 1); + assert!(access_control_core::get_pending_admin() == another_addr, 1); } #[test(non_admin = @0x123)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] fun test_transfer_admin_not_admin(non_admin: &signer) { - // Should fail - not admin + // Should fail - not admin (error from admin registry) access_control_core::transfer_admin(non_admin, @0x456); } #[test(admin = @movekit)] #[ expected_failure( - abort_code = E_SELF_TRANSFER_NOT_ALLOWED, - location = movekit::access_control_core + abort_code = E_ADMIN_SELF_TRANSFER_NOT_ALLOWED, + location = movekit::access_control_admin_registry ) ] fun test_transfer_admin_to_self(admin: &signer) { access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); - // Should fail - cannot transfer to self + // Should fail - cannot transfer to self (error from admin registry) access_control_core::transfer_admin(admin, admin_addr); } #[test(admin = @movekit, new_admin = @0x123, wrong_admin = @0x456)] - #[ - expected_failure( - abort_code = E_NOT_PENDING_ADMIN, location = movekit::access_control_core - ) - ] + #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] fun test_accept_pending_admin_wrong_address( admin: &signer, new_admin: &signer, wrong_admin: &signer ) { - // Setup access_control_core::init_for_testing(admin); let new_admin_addr = signer::address_of(new_admin); // Propose transfer to new_admin access_control_core::transfer_admin(admin, new_admin_addr); - // Should fail - wrong_admin tries to accept + // Should fail - wrong_admin tries to accept (core validates this) access_control_core::accept_pending_admin(wrong_admin); } #[test(new_admin = @0x123)] #[ expected_failure( - abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry ) ] fun test_accept_pending_admin_no_pending(new_admin: &signer) { - // Should fail - admin registry not initialized (fails before checking pending admin) + // Should fail - admin registry not initialized (error from admin registry) access_control_core::accept_pending_admin(new_admin); } #[test(admin = @movekit, new_admin = @0x123)] #[ expected_failure( - abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core ) ] fun test_accept_pending_admin_no_pending_initialized( admin: &signer, new_admin: &signer ) { - // Setup admin but no pending transfer access_control_core::init_for_testing(admin); - // Should fail - no pending admin transfer (but admin registry is initialized) + // Should fail - no pending admin transfer (core validates this) access_control_core::accept_pending_admin(new_admin); } #[test(non_admin = @0x123)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] fun test_cancel_admin_transfer_not_admin(non_admin: &signer) { - // Should fail - not admin + // Should fail - not admin (error from admin registry) access_control_core::cancel_admin_transfer(non_admin); } #[test(admin = @movekit)] #[ expected_failure( - abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + abort_code = E_ADMIN_NO_PENDING_ADMIN, + location = movekit::access_control_admin_registry ) ] fun test_cancel_admin_transfer_no_pending(admin: &signer) { access_control_core::init_for_testing(admin); - // Should fail - no pending transfer to cancel + // Should fail - no pending transfer to cancel (error from admin registry) access_control_core::cancel_admin_transfer(admin); } #[test] #[ expected_failure( - abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry ) ] fun test_get_pending_admin_no_pending() { - // Should fail - no pending admin - access_control_core::get_pending_admin(@0x123); + // Should fail - no pending admin (error from admin registry) + access_control_core::get_pending_admin(); } // =========================================== @@ -274,21 +291,35 @@ module movekit::access_control_core_tests { // =========================================== #[test(attacker = @0x999)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_ROLE_PROTECTED, location = movekit::access_control_core + ) + ] fun test_privilege_escalation_prevention_grant(attacker: &signer) { - // Attacker tries to grant themselves admin role without proper authority + // Attacker tries to grant themselves admin role - blocked by role protection access_control_core::grant_role(attacker, signer::address_of(attacker)); } #[test(attacker = @0x999)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] fun test_privilege_escalation_prevention_transfer(attacker: &signer) { - // Attacker tries to transfer admin to themselves + // Attacker tries to transfer admin to themselves (error from admin registry) access_control_core::transfer_admin(attacker, signer::address_of(attacker)); } #[test(admin = @movekit, attacker = @0x999)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_ADMIN, + location = movekit::access_control_admin_registry + ) + ] fun test_unauthorized_role_grant_prevention( admin: &signer, attacker: &signer ) { @@ -299,7 +330,12 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, attacker = @0x999)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_ADMIN, + location = movekit::access_control_admin_registry + ) + ] fun test_unauthorized_role_revoke_prevention( admin: &signer, attacker: &signer ) { @@ -313,7 +349,12 @@ module movekit::access_control_core_tests { } #[test(admin = @movekit, attacker = @0x999)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_ADMIN, + location = movekit::access_control_admin_registry + ) + ] fun test_unauthorized_admin_cancel_prevention( admin: &signer, attacker: &signer ) { @@ -322,7 +363,7 @@ module movekit::access_control_core_tests { // Admin proposes transfer access_control_core::transfer_admin(admin, @0x123); - // Attacker tries to cancel admin transfer + // Attacker tries to cancel admin transfer (error from admin registry) access_control_core::cancel_admin_transfer(attacker); } @@ -368,7 +409,7 @@ module movekit::access_control_core_tests { assert!(access_control_core::get_role_count(admin_addr) == 3, 0); // Admin + Treasurer + Manager assert!(access_control_core::get_role_count(new_admin_addr) == 0, 1); - // Transfer admin role + // Transfer admin role (core coordinates between admin registry and role registry) access_control_core::transfer_admin(admin, new_admin_addr); access_control_core::accept_pending_admin(new_admin); @@ -412,14 +453,13 @@ module movekit::access_control_core_tests { } // =========================================== - // USEFUL EDGE CASE TESTS + // ROLE MANAGEMENT TESTS // =========================================== #[test(admin = @movekit)] fun test_role_operations_on_nonexistent_users(admin: &signer) { access_control_core::init_for_testing(admin); - // Test granting roles to addresses that don't exist in table yet let nonexistent = @0xDEADBEEF; // Should be able to grant role to new address @@ -433,10 +473,6 @@ module movekit::access_control_core_tests { assert!(access_control_core::get_role_count(nonexistent) == 2, 3); } - // =========================================== - // REAL-WORLD ROLE MANAGEMENT TESTS - // =========================================== - #[test(admin = @movekit, user = @0x123)] fun test_grant_role_success(admin: &signer, user: &signer) { access_control_core::init_for_testing(admin); @@ -469,7 +505,12 @@ module movekit::access_control_core_tests { } #[test(non_admin = @0x123)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] fun test_grant_role_not_admin(non_admin: &signer) { // Test that non-admin cannot grant roles access_control_core::grant_role(non_admin, @0x456); @@ -504,16 +545,17 @@ module movekit::access_control_core_tests { } #[test(non_admin = @0x123)] - #[expected_failure(abort_code = E_NOT_ADMIN, location = movekit::access_control_core)] + #[ + expected_failure( + abort_code = E_ADMIN_NOT_INITIALIZED, + location = movekit::access_control_admin_registry + ) + ] fun test_revoke_role_not_admin(non_admin: &signer) { // Test that non-admin cannot revoke roles access_control_core::revoke_role(non_admin, @0x456); } - // =========================================== - // GLOBAL REGISTRY TESTS - // =========================================== - #[test(admin = @movekit, user1 = @0x123, user2 = @0x456)] fun test_multiple_roles_different_users( admin: &signer, user1: &signer, user2: &signer @@ -727,89 +769,62 @@ module movekit::access_control_core_tests { access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); access_control_core::grant_role(admin, user); - access_control_core::grant_role(admin, user); + // Note: Admin role is reserved for actual admins // Verify all roles exist assert!(access_control_core::has_role(user), 0); assert!(access_control_core::has_role(user), 1); assert!(access_control_core::has_role(user), 2); - assert!(access_control_core::has_role(user), 3); - assert!(access_control_core::get_role_count(user) == 4, 4); + assert!(access_control_core::get_role_count(user) == 3, 3); } #[test(admin = @movekit, new_admin = @0x123)] #[ expected_failure( - abort_code = E_NO_PENDING_ADMIN, location = movekit::access_control_core + abort_code = E_NOT_INITIALIZED, location = movekit::access_control_core ) ] fun test_double_accept_pending_admin_fails( admin: &signer, new_admin: &signer ) { - // Normal transfer access_control_core::init_for_testing(admin); let new_admin_addr = signer::address_of(new_admin); access_control_core::transfer_admin(admin, new_admin_addr); // First accept succeeds access_control_core::accept_pending_admin(new_admin); - // Second accept must fail → E_NO_PENDING_ADMIN + // Second accept must fail (core validates pending transfer exists) access_control_core::accept_pending_admin(new_admin); } #[test(admin = @movekit)] #[ expected_failure( - abort_code = E_ALREADY_HAS_ROLE, location = movekit::access_control_core + abort_code = E_ADMIN_ROLE_PROTECTED, location = movekit::access_control_core ) ] fun test_admin_cannot_grant_admin_twice(admin: &signer) { - // Bootstrap access_control_core::init_for_testing(admin); let admin_addr = signer::address_of(admin); - // Current admin **already** owns the Admin role; trying again must abort + // Admin role is protected - cannot be granted manually access_control_core::grant_role(admin, admin_addr); } #[test(current_admin = @movekit, future_admin = @0x123)] - fun test_admin_role_not_duplicated_after_transfer( + #[ + expected_failure( + abort_code = E_ADMIN_ROLE_PROTECTED, location = movekit::access_control_core + ) + ] + fun test_admin_role_coordinated_during_transfer( current_admin: &signer, future_admin: &signer ) { access_control_core::init_for_testing(current_admin); let fut_addr = signer::address_of(future_admin); - // Pre-grant Admin to the future admin - access_control_core::grant_role(current_admin, fut_addr); - assert!(access_control_core::get_role_count(fut_addr) == 1, 0); - - // Transfer governance - access_control_core::transfer_admin(current_admin, fut_addr); - access_control_core::accept_pending_admin(future_admin); - - assert!(access_control_core::get_role_count(fut_addr) == 1, 1); - } - - #[test(current_admin = @movekit, future_admin = @0x123)] - fun test_single_revoke_clears_admin_role( - current_admin: &signer, future_admin: &signer - ) { - // Setup identical to duplication test, except duplicates can’t occur. - access_control_core::init_for_testing(current_admin); - let fut_addr = signer::address_of(future_admin); - - // Pre-grant Admin, then transfer governance. + // Cannot pre-grant Admin role - it's protected access_control_core::grant_role(current_admin, fut_addr); - access_control_core::transfer_admin(current_admin, fut_addr); - access_control_core::accept_pending_admin(future_admin); - - assert!(access_control_core::get_role_count(fut_addr) == 1, 0); - - // Revoke once. - access_control_core::revoke_role(future_admin, fut_addr); - - assert!(!access_control_core::has_role(fut_addr), 1); - assert!(access_control_core::get_role_count(fut_addr) == 0, 2); } } From de7a9877e431780a32cf4883153e08669b618c98 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 11:16:50 +0200 Subject: [PATCH 14/25] refactor: Add swag Move 2 syntax --- sources/access_control/admin_registry.move | 14 +- sources/access_control/core.move | 45 ++--- tests/access_control/admin_registry_test.move | 186 +++++++++--------- tests/access_control/core_test.move | 21 +- 4 files changed, 130 insertions(+), 136 deletions(-) diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move index ca763f9..dfbb4fa 100644 --- a/sources/access_control/admin_registry.move +++ b/sources/access_control/admin_registry.move @@ -44,7 +44,7 @@ module movekit::access_control_admin_registry { #[view] public fun get_current_admin(): address acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); - borrow_global(@movekit).current_admin + (&AdminRegistry[@movekit]).current_admin } #[view] @@ -54,7 +54,7 @@ module movekit::access_control_admin_registry { public fun require_admin(admin: &signer) acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); - let registry = borrow_global(@movekit); + let registry = &AdminRegistry[@movekit]; assert!(registry.current_admin == signer::address_of(admin), E_NOT_ADMIN); } @@ -66,7 +66,7 @@ module movekit::access_control_admin_registry { // Set or update pending admin if (exists(admin_addr)) { - let pending = borrow_global_mut(admin_addr); + let pending = &mut PendingAdmin[admin_addr]; pending.pending_admin = new_admin; } else { move_to(admin, PendingAdmin { pending_admin: new_admin }); @@ -86,13 +86,13 @@ module movekit::access_control_admin_registry { assert!(exists(current_admin_addr), E_NO_PENDING_ADMIN); // Get the pending admin info - let pending = borrow_global(current_admin_addr); + let pending = &PendingAdmin[current_admin_addr]; // Verify that the caller is the intended new admin assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); // Update AdminRegistry to point to new admin - let registry = borrow_global_mut(@movekit); + let registry = &mut AdminRegistry[@movekit]; let old_admin = registry.current_admin; registry.current_admin = new_admin_addr; @@ -126,7 +126,7 @@ module movekit::access_control_admin_registry { public fun get_pending_admin(): address acquires AdminRegistry, PendingAdmin { let current_admin = get_current_admin(); assert!(exists(current_admin), E_NO_PENDING_ADMIN); - borrow_global(current_admin).pending_admin + (&PendingAdmin[current_admin]).pending_admin } #[view] @@ -136,7 +136,7 @@ module movekit::access_control_admin_registry { } /// Allow friend modules to initialize admin registry (idempotent) - public(friend) fun init_admin_registry(admin: &signer) { + friend fun init_admin_registry(admin: &signer) { if (!exists(@movekit)) { let admin_addr = signer::address_of(admin); move_to(admin, AdminRegistry { current_admin: admin_addr }); diff --git a/sources/access_control/core.move b/sources/access_control/core.move index bf80aa3..4da7870 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -185,13 +185,12 @@ module movekit::access_control_core { // Handle uninitialized system gracefully if (!exists(@movekit)) return false; - let registry = borrow_global(@movekit); + let registry = &RoleRegistry[@movekit]; // Handle non-existent user gracefully - if (!table::contains(®istry.roles, addr)) - return false; + if (!registry.roles.contains(addr)) return false; - let user_roles = table::borrow(®istry.roles, addr); + let user_roles = registry.roles.borrow(addr); let target_type = type_info::type_of(); vector_contains(user_roles, &target_type) @@ -204,19 +203,18 @@ module movekit::access_control_core { // Handle uninitialized system gracefully if (!exists(@movekit)) return vector::empty(); - let registry = borrow_global(@movekit); + let registry = &RoleRegistry[@movekit]; // Handle non-existent user gracefully - if (!table::contains(®istry.roles, addr)) - return vector::empty(); + if (!registry.roles.contains(addr)) return vector::empty(); - *table::borrow(®istry.roles, addr) + *registry.roles.borrow(addr) } #[view] /// Count total roles assigned to an address public fun get_role_count(addr: address): u64 acquires RoleRegistry { - vector::length(&get_roles(addr)) + get_roles(addr).length() } /// Assert caller has required role or abort with clear error @@ -249,41 +247,40 @@ module movekit::access_control_core { /// Internal role granting with duplicate protection fun grant_role_internal(target: address) acquires RoleRegistry { - let registry = borrow_global_mut(@movekit); + let registry = &mut RoleRegistry[@movekit]; let role_type = type_info::type_of(); // Initialize user's role vector if needed - if (!table::contains(®istry.roles, target)) { - table::add(&mut registry.roles, target, vector::empty()); + if (!registry.roles.contains(target)) { + registry.roles.add(target, vector::empty()); }; - let user_roles = table::borrow_mut(&mut registry.roles, target); + let user_roles = registry.roles.borrow_mut(target); // Only add role if not already present (idempotent operation) if (!vector_contains(user_roles, &role_type)) { - vector::push_back(user_roles, role_type); + user_roles.push_back(role_type); } } /// Internal role revocation with non-existence protection fun revoke_role_internal(target: address) acquires RoleRegistry { - let registry = borrow_global_mut(@movekit); + let registry = &mut RoleRegistry[@movekit]; let role_type = type_info::type_of(); // Handle non-existent user gracefully - if (!table::contains(®istry.roles, target)) - return; + if (!registry.roles.contains(target)) return; - let user_roles = table::borrow_mut(&mut registry.roles, target); + let user_roles = registry.roles.borrow_mut(target); let (found, index) = vector_find(user_roles, &role_type); // Only remove if role exists (idempotent operation) if (found) { - vector::remove(user_roles, index); + user_roles.remove(index); // Clean up empty role vectors to save storage - if (vector::is_empty(user_roles)) { - table::remove(&mut registry.roles, target); + if (user_roles.is_empty()) { + registry.roles.remove(target); } } } @@ -309,13 +306,13 @@ module movekit::access_control_core { /// Find element in vector with safe indexing fun vector_find(vec: &vector, item: &TypeInfo): (bool, u64) { - let len = vector::length(vec); + let len = vec.length(); let i = 0; while (i < len) { - if (vector::borrow(vec, i) == item) { + if (vec.borrow(i) == item) { return (true, i) }; - i = i + 1; + i += 1; }; (false, 0) // Return 0 as dummy index when not found } diff --git a/tests/access_control/admin_registry_test.move b/tests/access_control/admin_registry_test.move index f2d263e..ee68e25 100644 --- a/tests/access_control/admin_registry_test.move +++ b/tests/access_control/admin_registry_test.move @@ -22,11 +22,11 @@ module movekit::access_control_admin_registry_tests { let deployer_addr = signer::address_of(deployer); // Should set deployer as current admin - assert!(access_control_admin_registry::get_current_admin() == deployer_addr, 0); - assert!(access_control_admin_registry::is_current_admin(deployer_addr), 1); + assert!(access_control_admin_registry::get_current_admin() == deployer_addr); + assert!(access_control_admin_registry::is_current_admin(deployer_addr)); // No pending admin initially - assert!(!access_control_admin_registry::has_pending_admin(deployer_addr), 2); + assert!(!access_control_admin_registry::has_pending_admin(deployer_addr)); } #[test(deployer = @movekit)] @@ -105,9 +105,9 @@ module movekit::access_control_admin_registry_tests { let deployer_addr = signer::address_of(deployer); - assert!(access_control_admin_registry::is_current_admin(deployer_addr), 0); - assert!(!access_control_admin_registry::is_current_admin(@0x123), 1); - assert!(!access_control_admin_registry::is_current_admin(@0x456), 2); + assert!(access_control_admin_registry::is_current_admin(deployer_addr)); + assert!(!access_control_admin_registry::is_current_admin(@0x123)); + assert!(!access_control_admin_registry::is_current_admin(@0x456)); } // =========================================== @@ -127,24 +127,24 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Check pending state - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); - assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 1); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr); // Admin should still be current - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 2); - assert!(access_control_admin_registry::is_current_admin(admin_addr), 3); - assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 4); + assert!(access_control_admin_registry::get_current_admin() == admin_addr); + assert!(access_control_admin_registry::is_current_admin(admin_addr)); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr)); // Step 2: Accept transfer access_control_admin_registry::accept_pending_admin(new_admin); // Check transfer completed - assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 5); - assert!(access_control_admin_registry::is_current_admin(new_admin_addr), 6); - assert!(!access_control_admin_registry::is_current_admin(admin_addr), 7); + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr); + assert!(access_control_admin_registry::is_current_admin(new_admin_addr)); + assert!(!access_control_admin_registry::is_current_admin(admin_addr)); // Check pending admin cleaned up - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 8); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } #[test(admin = @movekit, new_admin = @0x123)] @@ -158,14 +158,14 @@ module movekit::access_control_admin_registry_tests { // First proposal access_control_admin_registry::transfer_admin(admin, new_admin_addr); - assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 0); + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr); // Second proposal overwrites first access_control_admin_registry::transfer_admin(admin, another_addr); - assert!(access_control_admin_registry::get_pending_admin() == another_addr, 1); + assert!(access_control_admin_registry::get_pending_admin() == another_addr); // Old proposal is no longer valid - assert!(access_control_admin_registry::get_pending_admin() != new_admin_addr, 2); + assert!(access_control_admin_registry::get_pending_admin() != new_admin_addr); } #[test(admin = @movekit, new_admin = @0x123)] @@ -177,17 +177,17 @@ module movekit::access_control_admin_registry_tests { // Propose transfer access_control_admin_registry::transfer_admin(admin, new_admin_addr); - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // Cancel transfer access_control_admin_registry::cancel_admin_transfer(admin); // Check admin unchanged - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); - assert!(access_control_admin_registry::is_current_admin(admin_addr), 2); + assert!(access_control_admin_registry::get_current_admin() == admin_addr); + assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Check pending admin cleaned up - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } // =========================================== @@ -200,7 +200,9 @@ module movekit::access_control_admin_registry_tests { abort_code = E_NOT_ADMIN, location = movekit::access_control_admin_registry ) ] - fun test_transfer_admin_not_admin(admin: &signer, non_admin: &signer) { + fun test_transfer_admin_not_admin( + admin: &signer, non_admin: &signer + ) { access_control_admin_registry::init_for_testing(admin); access_control_admin_registry::transfer_admin(non_admin, @0x456); } @@ -373,16 +375,16 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, new_admin1_addr); access_control_admin_registry::accept_pending_admin(new_admin1); - assert!(access_control_admin_registry::get_current_admin() == new_admin1_addr, 0); - assert!(!access_control_admin_registry::is_current_admin(admin_addr), 1); + assert!(access_control_admin_registry::get_current_admin() == new_admin1_addr); + assert!(!access_control_admin_registry::is_current_admin(admin_addr)); // Transfer 2: new_admin1 -> new_admin2 access_control_admin_registry::transfer_admin(new_admin1, new_admin2_addr); access_control_admin_registry::accept_pending_admin(new_admin2); - assert!(access_control_admin_registry::get_current_admin() == new_admin2_addr, 2); - assert!(!access_control_admin_registry::is_current_admin(new_admin1_addr), 3); - assert!(!access_control_admin_registry::is_current_admin(admin_addr), 4); + assert!(access_control_admin_registry::get_current_admin() == new_admin2_addr); + assert!(!access_control_admin_registry::is_current_admin(new_admin1_addr)); + assert!(!access_control_admin_registry::is_current_admin(admin_addr)); } // =========================================== @@ -397,8 +399,8 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, @0x0); let admin_addr = signer::address_of(admin); - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); - assert!(access_control_admin_registry::get_pending_admin() == @0x0, 1); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::get_pending_admin() == @0x0); } #[test(admin = @movekit)] @@ -426,22 +428,22 @@ module movekit::access_control_admin_registry_tests { let admin_addr = signer::address_of(admin); // Initially no pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // After proposing transfer access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 1); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // After canceling access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } #[test] fun test_has_pending_admin_nonexistent_address() { // Should return false for addresses that never had pending admin - assert!(!access_control_admin_registry::has_pending_admin(@0x123), 0); - assert!(!access_control_admin_registry::has_pending_admin(@0x456), 1); + assert!(!access_control_admin_registry::has_pending_admin(@0x123)); + assert!(!access_control_admin_registry::has_pending_admin(@0x456)); } // =========================================== @@ -507,17 +509,17 @@ module movekit::access_control_admin_registry_tests { // Multiple proposals should overwrite each other access_control_admin_registry::transfer_admin(admin, @0x111); - assert!(access_control_admin_registry::get_pending_admin() == @0x111, 0); + assert!(access_control_admin_registry::get_pending_admin() == @0x111); access_control_admin_registry::transfer_admin(admin, @0x222); - assert!(access_control_admin_registry::get_pending_admin() == @0x222, 1); + assert!(access_control_admin_registry::get_pending_admin() == @0x222); access_control_admin_registry::transfer_admin(admin, @0x333); - assert!(access_control_admin_registry::get_pending_admin() == @0x333, 2); + assert!(access_control_admin_registry::get_pending_admin() == @0x333); // Only the last proposal is valid - assert!(access_control_admin_registry::get_pending_admin() != @0x111, 3); - assert!(access_control_admin_registry::get_pending_admin() != @0x222, 4); + assert!(access_control_admin_registry::get_pending_admin() != @0x111); + assert!(access_control_admin_registry::get_pending_admin() != @0x222); } #[test(admin = @movekit, new_admin = @0x123)] @@ -534,10 +536,10 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::accept_pending_admin(new_admin); // Old admin should no longer have pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // New admin should not have pending admin either - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 1); + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); } #[test(admin = @movekit, new_admin = @0x123)] @@ -554,10 +556,10 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::cancel_admin_transfer(admin); // Admin should no longer have pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // New admin should not have pending admin - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 1); + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); } // =========================================== @@ -577,19 +579,19 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Verify pending admin exists - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // Accept transfer access_control_admin_registry::accept_pending_admin(new_admin); // Verify NO pending admin resources exist anywhere - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr), 2); - assert!(!access_control_admin_registry::has_pending_admin(@movekit), 3); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin(@movekit)); // Test random addresses too - assert!(!access_control_admin_registry::has_pending_admin(@0x999), 4); - assert!(!access_control_admin_registry::has_pending_admin(@0x888), 5); + assert!(!access_control_admin_registry::has_pending_admin(@0x999)); + assert!(!access_control_admin_registry::has_pending_admin(@0x888)); } #[test(admin = @movekit)] @@ -602,15 +604,15 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, @0x123); // Verify pending admin exists - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // Cancel transfer access_control_admin_registry::cancel_admin_transfer(admin); // Verify NO pending admin resources exist anywhere - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); - assert!(!access_control_admin_registry::has_pending_admin(@0x123), 2); - assert!(!access_control_admin_registry::has_pending_admin(@movekit), 3); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin(@0x123)); + assert!(!access_control_admin_registry::has_pending_admin(@movekit)); } #[test(admin = @movekit)] @@ -625,12 +627,12 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, @0x333); // Should only have ONE pending admin (the latest) - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 0); - assert!(access_control_admin_registry::get_pending_admin() == @0x333, 1); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::get_pending_admin() == @0x333); // Cancel and verify clean slate access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } // =========================================== @@ -643,17 +645,17 @@ module movekit::access_control_admin_registry_tests { // Rapid succession proposals (testing internal state consistency) access_control_admin_registry::transfer_admin(admin, @0x111); - assert!(access_control_admin_registry::get_pending_admin() == @0x111, 0); + assert!(access_control_admin_registry::get_pending_admin() == @0x111); access_control_admin_registry::transfer_admin(admin, @0x222); - assert!(access_control_admin_registry::get_pending_admin() == @0x222, 1); + assert!(access_control_admin_registry::get_pending_admin() == @0x222); access_control_admin_registry::transfer_admin(admin, @0x333); - assert!(access_control_admin_registry::get_pending_admin() == @0x333, 2); + assert!(access_control_admin_registry::get_pending_admin() == @0x333); // Verify only the last one is valid - assert!(access_control_admin_registry::get_pending_admin() != @0x111, 3); - assert!(access_control_admin_registry::get_pending_admin() != @0x222, 4); + assert!(access_control_admin_registry::get_pending_admin() != @0x111); + assert!(access_control_admin_registry::get_pending_admin() != @0x222); } #[test(admin = @movekit, new_admin = @0x123)] @@ -672,8 +674,8 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::accept_pending_admin(new_admin); // Verify final state is correct - assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 0); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } #[test(admin = @movekit, new_admin = @0x123)] @@ -688,14 +690,14 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Overwrite // Verify state is correct before accept - assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr, 0); - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); + assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr); + assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Accept should work access_control_admin_registry::accept_pending_admin(new_admin); // Verify final state - assert!(access_control_admin_registry::get_current_admin() == new_admin_addr, 2); + assert!(access_control_admin_registry::get_current_admin() == new_admin_addr); } // =========================================== @@ -711,21 +713,21 @@ module movekit::access_control_admin_registry_tests { let admin_addr = signer::address_of(admin); // Initial state verification - assert!(access_control_admin_registry::is_current_admin(admin_addr), 0); - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 1); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 2); + assert!(access_control_admin_registry::is_current_admin(admin_addr)); + assert!(access_control_admin_registry::get_current_admin() == admin_addr); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // After proposal access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::is_current_admin(admin_addr), 3); // Still current - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 4); // Still current - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 5); // Now has pending + assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Still current + assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Still current + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // Now has pending // After cancel access_control_admin_registry::cancel_admin_transfer(admin); - assert!(access_control_admin_registry::is_current_admin(admin_addr), 6); // Still current - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 7); // Still current - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 8); // No longer pending + assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Still current + assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Still current + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // No longer pending } #[test(admin = @movekit)] @@ -753,13 +755,13 @@ module movekit::access_control_admin_registry_tests { // PendingAdmin should be created and destroyed properly let admin_addr = signer::address_of(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 4); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 5); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); } // =========================================== @@ -778,22 +780,22 @@ module movekit::access_control_admin_registry_tests { // Throughout the entire transfer process, exactly one address should be admin // Initial: admin is admin, new_admin is not - assert!(access_control_admin_registry::is_current_admin(admin_addr), 0); - assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 1); + assert!(access_control_admin_registry::is_current_admin(admin_addr)); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr)); // During proposal: admin still admin, new_admin still not access_control_admin_registry::transfer_admin(admin, new_admin_addr); - assert!(access_control_admin_registry::is_current_admin(admin_addr), 2); - assert!(!access_control_admin_registry::is_current_admin(new_admin_addr), 3); + assert!(access_control_admin_registry::is_current_admin(admin_addr)); + assert!(!access_control_admin_registry::is_current_admin(new_admin_addr)); // After transfer: new_admin is admin, admin is not access_control_admin_registry::accept_pending_admin(new_admin); - assert!(!access_control_admin_registry::is_current_admin(admin_addr), 4); - assert!(access_control_admin_registry::is_current_admin(new_admin_addr), 5); + assert!(!access_control_admin_registry::is_current_admin(admin_addr)); + assert!(access_control_admin_registry::is_current_admin(new_admin_addr)); // Verify no other addresses are admin - assert!(!access_control_admin_registry::is_current_admin(@0x999), 6); - assert!(!access_control_admin_registry::is_current_admin(@movekit), 7); + assert!(!access_control_admin_registry::is_current_admin(@0x999)); + assert!(!access_control_admin_registry::is_current_admin(@movekit)); } #[test(admin = @movekit)] @@ -803,19 +805,19 @@ module movekit::access_control_admin_registry_tests { let admin_addr = signer::address_of(admin); // Invariant: AdminRegistry always exists after initialization - assert!(access_control_admin_registry::get_current_admin() == admin_addr, 0); + assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Invariant: PendingAdmin exists iff there's an active proposal - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 1); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr), 2); + assert!(access_control_admin_registry::has_pending_admin(admin_addr)); access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr), 3); + assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // Invariant: get_current_admin() always returns a valid address let current = access_control_admin_registry::get_current_admin(); - assert!(current == admin_addr, 4); // Should be a concrete address, not null + assert!(current == admin_addr); // Should be a concrete address, not null } } diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 5e1fe73..784c64b 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -1,7 +1,6 @@ #[test_only] module movekit::access_control_core_tests { use std::signer; - use std::vector; use movekit::access_control_core::{Self, Admin}; // Test role types @@ -36,14 +35,10 @@ module movekit::access_control_core_tests { let deployer_addr = signer::address_of(deployer); // Check admin registry was created (delegated functions work) - assert!(access_control_core::get_current_admin() == deployer_addr, 0); - assert!(access_control_core::is_current_admin(deployer_addr), 1); - - // Check admin role was granted in role registry - assert!(access_control_core::has_role(deployer_addr), 2); - - // Check role count - assert!(access_control_core::get_role_count(deployer_addr) == 1, 3); + assert!(access_control_core::get_current_admin() == deployer_addr); + assert!(access_control_core::is_current_admin(deployer_addr)); + assert!(access_control_core::has_role(deployer_addr)); + assert!(access_control_core::get_role_count(deployer_addr) == 1); } #[test(deployer = @movekit)] @@ -613,7 +608,7 @@ module movekit::access_control_core_tests { // Initially no roles let roles = access_control_core::get_roles(user_addr); - assert!(vector::length(&roles) == 0, 0); + assert!(roles.length() == 0, 0); // Grant some roles access_control_core::grant_role(admin, user_addr); @@ -621,13 +616,13 @@ module movekit::access_control_core_tests { // Check roles vector let roles = access_control_core::get_roles(user_addr); - assert!(vector::length(&roles) == 2, 1); + assert!(roles.length() == 2, 1); // Revoke one role access_control_core::revoke_role(admin, user_addr); let roles = access_control_core::get_roles(user_addr); - assert!(vector::length(&roles) == 1, 2); + assert!(roles.length() == 1, 2); } #[test(user = @0x123)] @@ -641,7 +636,7 @@ module movekit::access_control_core_tests { #[test] fun test_get_roles_no_registry() { let roles = access_control_core::get_roles(@0x123); - assert!(vector::length(&roles) == 0, 0); + assert!(roles.length() == 0, 0); assert!(access_control_core::get_role_count(@0x123) == 0, 1); } From 484ee7c5a23ded18bf161272f443133df4634f3e Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 11:20:32 +0200 Subject: [PATCH 15/25] refactor: Refactor CI Workflow --- .github/workflows/{fmt.yaml => CI.yaml} | 21 ++++++++++++++++++++- .github/workflows/test.yaml | 22 ---------------------- 2 files changed, 20 insertions(+), 23 deletions(-) rename .github/workflows/{fmt.yaml => CI.yaml} (55%) delete mode 100644 .github/workflows/test.yaml diff --git a/.github/workflows/fmt.yaml b/.github/workflows/CI.yaml similarity index 55% rename from .github/workflows/fmt.yaml rename to .github/workflows/CI.yaml index dc21f7a..b040533 100644 --- a/.github/workflows/fmt.yaml +++ b/.github/workflows/CI.yaml @@ -1,4 +1,4 @@ -name: fmt +name: CI on: [push, pull_request] @@ -11,6 +11,7 @@ jobs: - name: Install Aptos CLI run: | + set -euo pipefail curl -fsSL "https://aptos.dev/scripts/install_cli.py" | python3 echo "$HOME/.local/bin" >> $GITHUB_PATH @@ -24,3 +25,21 @@ jobs: run: | aptos move fmt git diff --exit-code + + test: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Aptos CLI + run: | + set -euo pipefail + curl -fsSL "https://aptos.dev/scripts/install_cli.py" | python3 + echo "$HOME/.local/bin" >> $GITHUB_PATH + + - name: Run Aptos Move Tests + run: aptos move test --dev + + - name: Run Aptos Move Linter + run: aptos move lint --dev \ No newline at end of file diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml deleted file mode 100644 index 36d5ab4..0000000 --- a/.github/workflows/test.yaml +++ /dev/null @@ -1,22 +0,0 @@ -name: CI - -"on": [push, pull_request] - -jobs: - test: - runs-on: ubuntu-latest - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Install Aptos CLI - run: | - set -euo pipefail - curl -fsSL https://aptos.dev/scripts/install_cli.py | python3 - echo "$HOME/.local/bin" >> "$GITHUB_PATH" - - - name: Run Aptos Move Tests - run: aptos move test --dev - - - name: Run Aptos Move Linter - run: aptos move lint --dev From 1067c1094701acd88932a8f432d7e61417544a89 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 11:56:39 +0200 Subject: [PATCH 16/25] refactor: Refactored Events --- sources/access_control/core.move | 55 ++++++++++++++------------------ 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 4da7870..af8b0a7 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -39,16 +39,18 @@ module movekit::access_control_core { #[event] /// Emitted when a role is successfully granted to an address - struct RoleGrantedEvent has copy, drop, store { + struct RoleGranted has copy, drop, store { /// Address of admin who granted the role admin: address, /// Address that received the role - target: address + target: address, + /// Type information of the granted role + role: TypeInfo } #[event] /// Emitted when a role is successfully revoked from an address - struct RoleRevokedEvent has copy, drop, store { + struct RoleRevoked has copy, drop, store { /// Address of admin who revoked the role admin: address, /// Address that lost the role @@ -56,8 +58,8 @@ module movekit::access_control_core { } #[event] - /// Emitted when admin role synchronization occurs during transfer - struct AdminRoleSynchronized has copy, drop, store { + /// Emitted when admin role is transferred to a new admin + struct AdminRoleTransferred has copy, drop, store { /// Previous admin who lost Admin role old_admin: address, /// New admin who gained Admin role @@ -110,7 +112,7 @@ module movekit::access_control_core { // Emit synchronization event for audit trail event::emit( - AdminRoleSynchronized { + AdminRoleTransferred { old_admin: current_admin_addr, new_admin: new_admin_addr } @@ -152,7 +154,11 @@ module movekit::access_control_core { // Emit audit event event::emit( - RoleGrantedEvent { admin: signer::address_of(admin), target: target } + RoleGranted { + admin: signer::address_of(admin), + target: target, + role: type_info::type_of() + } ); } @@ -172,7 +178,7 @@ module movekit::access_control_core { // Emit audit event event::emit( - RoleRevokedEvent { admin: signer::address_of(admin), target: target } + RoleRevoked { admin: signer::address_of(admin), target: target } ); } @@ -193,7 +199,7 @@ module movekit::access_control_core { let user_roles = registry.roles.borrow(addr); let target_type = type_info::type_of(); - vector_contains(user_roles, &target_type) + user_roles.contains(&target_type) } #[view] @@ -258,7 +264,7 @@ module movekit::access_control_core { let user_roles = registry.roles.borrow_mut(target); // Only add role if not already present (idempotent operation) - if (!vector_contains(user_roles, &role_type)) { + if (!user_roles.contains(&role_type)) { user_roles.push_back(role_type); } } @@ -272,7 +278,7 @@ module movekit::access_control_core { if (!registry.roles.contains(target)) return; let user_roles = registry.roles.borrow_mut(target); - let (found, index) = vector_find(user_roles, &role_type); + let (found, index) = user_roles.find(|role| role == &role_type); // Only remove if role exists (idempotent operation) if (found) { @@ -298,25 +304,6 @@ module movekit::access_control_core { ); } - /// Check if vector contains specific element - fun vector_contains(vec: &vector, item: &TypeInfo): bool { - let (found, _) = vector_find(vec, item); - found - } - - /// Find element in vector with safe indexing - fun vector_find(vec: &vector, item: &TypeInfo): (bool, u64) { - let len = vec.length(); - let i = 0; - while (i < len) { - if (vec.borrow(i) == item) { - return (true, i) - }; - i += 1; - }; - (false, 0) // Return 0 as dummy index when not found - } - /// System initialization - creates role registry and grants initial Admin role fun init_module(admin: &signer) acquires RoleRegistry { let admin_addr = signer::address_of(admin); @@ -338,7 +325,13 @@ module movekit::access_control_core { grant_role_internal(admin_addr); // Emit initial role grant event - event::emit(RoleGrantedEvent { admin: admin_addr, target: admin_addr }); + event::emit( + RoleGranted { + admin: admin_addr, + target: admin_addr, + role: type_info::type_of() + } + ); } // -- Testing Support -- // From 56a66f3ea5c31b0f117810f3f3fb7968be0282bf Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 12:05:33 +0200 Subject: [PATCH 17/25] refactor: Changed Table to Ordered Map --- sources/access_control/core.move | 48 +++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index af8b0a7..a507af8 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -6,6 +6,7 @@ module movekit::access_control_core { use std::event; use std::type_info::{Self, TypeInfo}; use aptos_std::table::{Self, Table}; + use aptos_std::ordered_map::{Self, OrderedMap}; use std::vector; use movekit::access_control_admin_registry; @@ -13,8 +14,8 @@ module movekit::access_control_core { /// Global role registry mapping addresses to role types struct RoleRegistry has key { - /// Maps addresses to their assigned role types - roles: Table> + /// Maps addresses to their assigned roles + roles: Table> } /// Built-in Admin role type (managed via transfer only) @@ -203,7 +204,7 @@ module movekit::access_control_core { } #[view] - /// Get all roles assigned to an address + /// Get all roles assigned to an address in sorted order /// Returns empty vector for uninitialized system or non-existent users public fun get_roles(addr: address): vector acquires RoleRegistry { // Handle uninitialized system gracefully @@ -214,13 +215,26 @@ module movekit::access_control_core { // Handle non-existent user gracefully if (!registry.roles.contains(addr)) return vector::empty(); - *registry.roles.borrow(addr) + let user_roles = registry.roles.borrow(addr); + + // Extract keys from OrderedMap (automatically sorted) + ordered_map::keys(user_roles) } #[view] /// Count total roles assigned to an address public fun get_role_count(addr: address): u64 acquires RoleRegistry { - get_roles(addr).length() + // Handle uninitialized system gracefully + if (!exists(@movekit)) return 0; + + let registry = &RoleRegistry[@movekit]; + + // Handle non-existent user gracefully + if (!registry.roles.contains(addr)) return 0; + + let user_roles = registry.roles.borrow(addr); + + ordered_map::length(user_roles) } /// Assert caller has required role or abort with clear error @@ -232,6 +246,8 @@ module movekit::access_control_core { ); } + + // -- Internal Implementation -- // /// Synchronize Admin role during admin transfer @@ -256,16 +272,16 @@ module movekit::access_control_core { let registry = &mut RoleRegistry[@movekit]; let role_type = type_info::type_of(); - // Initialize user's role vector if needed + // Initialize user's role map if needed if (!registry.roles.contains(target)) { - registry.roles.add(target, vector::empty()); + registry.roles.add(target, ordered_map::new()); }; let user_roles = registry.roles.borrow_mut(target); // Only add role if not already present (idempotent operation) if (!user_roles.contains(&role_type)) { - user_roles.push_back(role_type); + user_roles.add(role_type, true); } } @@ -278,15 +294,15 @@ module movekit::access_control_core { if (!registry.roles.contains(target)) return; let user_roles = registry.roles.borrow_mut(target); - let (found, index) = user_roles.find(|role| role == &role_type); // Only remove if role exists (idempotent operation) - if (found) { - user_roles.remove(index); + if (user_roles.contains(&role_type)) { + user_roles.remove(&role_type); - // Clean up empty role vectors to save storage - if (user_roles.is_empty()) { - registry.roles.remove(target); + // Clean up empty role maps to save storage + if (ordered_map::is_empty(user_roles)) { + let empty_map = registry.roles.remove(target); + ordered_map::destroy_empty(empty_map); } } } @@ -316,7 +332,7 @@ module movekit::access_control_core { move_to( admin, RoleRegistry { - roles: table::new>() + roles: table::new>() } ); }; @@ -347,4 +363,4 @@ module movekit::access_control_core { public fun test_admin_role_protection(): bool { type_info::type_of() == type_info::type_of() } -} +} \ No newline at end of file From 4641a2ef3ce4dad78cbfb7ed4c8a45b1119699c7 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 12:23:04 +0200 Subject: [PATCH 18/25] refactor: Grouped Togheter View Functions --- sources/access_control/core.move | 61 +++++++++++++++----------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index a507af8..ffa8111 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -71,18 +71,6 @@ module movekit::access_control_core { // === Admin Delegation Functions === - #[view] - /// Get current admin address from admin registry - public fun get_current_admin(): address { - access_control_admin_registry::get_current_admin() - } - - #[view] - /// Check if given address is the current admin - public fun is_current_admin(addr: address): bool { - access_control_admin_registry::is_current_admin(addr) - } - /// Propose admin transfer - delegates to admin registry public fun transfer_admin(admin: &signer, new_admin: address) { access_control_admin_registry::transfer_admin(admin, new_admin) @@ -125,24 +113,12 @@ module movekit::access_control_core { access_control_admin_registry::cancel_admin_transfer(admin) } - #[view] - /// Get pending admin address from admin registry - public fun get_pending_admin(): address { - access_control_admin_registry::get_pending_admin() - } - - #[view] - /// Check if admin has pending transfer - public fun has_pending_admin(admin: address): bool { - access_control_admin_registry::has_pending_admin(admin) - } - // === Role Management Functions === /// Grant role to target address (Admin role; admin-only) public fun grant_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation - assert_admin_role_not_protected(); + assert_not_admin_role(); // Authorize admin access require_admin(admin); @@ -166,7 +142,7 @@ module movekit::access_control_core { /// Revoke role from target address (Admin role; admin-only) public fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation - assert_admin_role_not_protected(); + assert_not_admin_role(); // Authorize admin access require_admin(admin); @@ -186,8 +162,8 @@ module movekit::access_control_core { // === View Functions === #[view] - /// Check if address has specific role - /// Returns false gracefully for uninitialized system or non-existent users + /// Check if address has a specific role + /// Returns false if the registry or user entry does not exist public fun has_role(addr: address): bool acquires RoleRegistry { // Handle uninitialized system gracefully if (!exists(@movekit)) return false; @@ -204,6 +180,17 @@ module movekit::access_control_core { } #[view] + /// Get current admin address from admin registry + public fun get_current_admin(): address { + access_control_admin_registry::get_current_admin() + } + + #[view] + /// Check if given address is the current admin + public fun is_current_admin(addr: address): bool { + access_control_admin_registry::is_current_admin(addr) + } + /// Get all roles assigned to an address in sorted order /// Returns empty vector for uninitialized system or non-existent users public fun get_roles(addr: address): vector acquires RoleRegistry { @@ -216,7 +203,7 @@ module movekit::access_control_core { if (!registry.roles.contains(addr)) return vector::empty(); let user_roles = registry.roles.borrow(addr); - + // Extract keys from OrderedMap (automatically sorted) ordered_map::keys(user_roles) } @@ -233,7 +220,7 @@ module movekit::access_control_core { if (!registry.roles.contains(addr)) return 0; let user_roles = registry.roles.borrow(addr); - + ordered_map::length(user_roles) } @@ -246,7 +233,17 @@ module movekit::access_control_core { ); } + #[view] + /// Get pending admin address from admin registry + public fun get_pending_admin(): address { + access_control_admin_registry::get_pending_admin() + } + #[view] + /// Check if admin has pending transfer + public fun has_pending_admin(admin: address): bool { + access_control_admin_registry::has_pending_admin(admin) + } // -- Internal Implementation -- // @@ -313,7 +310,7 @@ module movekit::access_control_core { } /// Security check: prevent manual Admin role manipulation - fun assert_admin_role_not_protected() { + fun assert_not_admin_role() { assert!( type_info::type_of() != type_info::type_of(), E_ADMIN_ROLE_PROTECTED @@ -363,4 +360,4 @@ module movekit::access_control_core { public fun test_admin_role_protection(): bool { type_info::type_of() == type_info::type_of() } -} \ No newline at end of file +} From 594708b3d6f7c5d73116f2fdb949165bd209f2e6 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 14:28:47 +0200 Subject: [PATCH 19/25] refactor: Admin Registry Struct Now Has Pending Admin --- sources/access_control/admin_registry.move | 78 ++++++------- sources/access_control/core.move | 6 +- tests/access_control/admin_registry_test.move | 105 +++++++----------- tests/access_control/core_test.move | 8 +- 4 files changed, 88 insertions(+), 109 deletions(-) diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move index dfbb4fa..3223213 100644 --- a/sources/access_control/admin_registry.move +++ b/sources/access_control/admin_registry.move @@ -1,15 +1,13 @@ module movekit::access_control_admin_registry { use std::signer; use std::event; + use std::option::{Self, Option}; friend movekit::access_control_core; struct AdminRegistry has key { - current_admin: address - } - - struct PendingAdmin has key, drop { - pending_admin: address + current_admin: address, + pending_admin: Option
} // -- Constants -- // @@ -59,18 +57,14 @@ module movekit::access_control_admin_registry { } /// Current admin proposes new admin - public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry, PendingAdmin { + public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry { require_admin(admin); let admin_addr = signer::address_of(admin); assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); - // Set or update pending admin - if (exists(admin_addr)) { - let pending = &mut PendingAdmin[admin_addr]; - pending.pending_admin = new_admin; - } else { - move_to(admin, PendingAdmin { pending_admin: new_admin }); - }; + // Set pending admin in registry + let registry = &mut AdminRegistry[@movekit]; + registry.pending_admin = option::some(new_admin); event::emit( AdminTransferProposed { current_admin: admin_addr, pending_admin: new_admin } @@ -78,26 +72,22 @@ module movekit::access_control_admin_registry { } /// New admin accepts the transfer - public fun accept_pending_admin(new_admin: &signer) acquires AdminRegistry, PendingAdmin { + public fun accept_pending_admin(new_admin: &signer) acquires AdminRegistry { let new_admin_addr = signer::address_of(new_admin); - let current_admin_addr = get_current_admin(); + assert!(exists(@movekit), E_NOT_INITIALIZED); + let registry = &mut AdminRegistry[@movekit]; // Check that there's a pending admin transfer - assert!(exists(current_admin_addr), E_NO_PENDING_ADMIN); - - // Get the pending admin info - let pending = &PendingAdmin[current_admin_addr]; + assert!(option::is_some(®istry.pending_admin), E_NO_PENDING_ADMIN); // Verify that the caller is the intended new admin - assert!(pending.pending_admin == new_admin_addr, E_NOT_PENDING_ADMIN); + let pending_admin_addr = *option::borrow(®istry.pending_admin); + assert!(pending_admin_addr == new_admin_addr, E_NOT_PENDING_ADMIN); - // Update AdminRegistry to point to new admin - let registry = &mut AdminRegistry[@movekit]; + // Update registry let old_admin = registry.current_admin; registry.current_admin = new_admin_addr; - - // Clean up: Remove PendingAdmin resource from old admin's address - let _removed_pending = move_from(current_admin_addr); + registry.pending_admin = option::none(); // Emit completion event event::emit( @@ -106,40 +96,47 @@ module movekit::access_control_admin_registry { } /// Cancel pending admin transfer - public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry, PendingAdmin { - let admin_addr = signer::address_of(admin); + public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry { require_admin(admin); - assert!(exists(admin_addr), E_NO_PENDING_ADMIN); + let registry = &mut AdminRegistry[@movekit]; + assert!(option::is_some(®istry.pending_admin), E_NO_PENDING_ADMIN); - let pending = move_from(admin_addr); + let canceled_pending = *option::borrow(®istry.pending_admin); + registry.pending_admin = option::none(); event::emit( AdminTransferCanceled { - admin: admin_addr, - canceled_pending: pending.pending_admin + admin: signer::address_of(admin), + canceled_pending: canceled_pending } ); } #[view] /// Get pending admin address - public fun get_pending_admin(): address acquires AdminRegistry, PendingAdmin { - let current_admin = get_current_admin(); - assert!(exists(current_admin), E_NO_PENDING_ADMIN); - (&PendingAdmin[current_admin]).pending_admin + public fun get_pending_admin(): address acquires AdminRegistry { + assert!(exists(@movekit), E_NOT_INITIALIZED); + let registry = &AdminRegistry[@movekit]; + assert!(option::is_some(®istry.pending_admin), E_NO_PENDING_ADMIN); + *option::borrow(®istry.pending_admin) } #[view] /// Check if there's a pending admin transfer - public fun has_pending_admin(admin: address): bool { - exists(admin) + public fun has_pending_admin(): bool acquires AdminRegistry { + if (!exists(@movekit)) return false; + let registry = &AdminRegistry[@movekit]; + option::is_some(®istry.pending_admin) } /// Allow friend modules to initialize admin registry (idempotent) friend fun init_admin_registry(admin: &signer) { if (!exists(@movekit)) { let admin_addr = signer::address_of(admin); - move_to(admin, AdminRegistry { current_admin: admin_addr }); + move_to( + admin, + AdminRegistry { current_admin: admin_addr, pending_admin: option::none() } + ); } } @@ -148,7 +145,10 @@ module movekit::access_control_admin_registry { fun init_module(admin: &signer) { let admin_addr = signer::address_of(admin); // admin signer represents @movekit during deployment - move_to(admin, AdminRegistry { current_admin: admin_addr }); + move_to( + admin, + AdminRegistry { current_admin: admin_addr, pending_admin: option::none() } + ); } #[test_only] diff --git a/sources/access_control/core.move b/sources/access_control/core.move index ffa8111..8c42ce4 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -85,7 +85,7 @@ module movekit::access_control_core { // Validate pending transfer exists and matches caller assert!( - access_control_admin_registry::has_pending_admin(current_admin_addr), + access_control_admin_registry::has_pending_admin(), E_NOT_INITIALIZED ); assert!( @@ -241,8 +241,8 @@ module movekit::access_control_core { #[view] /// Check if admin has pending transfer - public fun has_pending_admin(admin: address): bool { - access_control_admin_registry::has_pending_admin(admin) + public fun has_pending_admin(): bool { + access_control_admin_registry::has_pending_admin() } // -- Internal Implementation -- // diff --git a/tests/access_control/admin_registry_test.move b/tests/access_control/admin_registry_test.move index ee68e25..34c842a 100644 --- a/tests/access_control/admin_registry_test.move +++ b/tests/access_control/admin_registry_test.move @@ -26,7 +26,7 @@ module movekit::access_control_admin_registry_tests { assert!(access_control_admin_registry::is_current_admin(deployer_addr)); // No pending admin initially - assert!(!access_control_admin_registry::has_pending_admin(deployer_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(deployer = @movekit)] @@ -127,7 +127,7 @@ module movekit::access_control_admin_registry_tests { access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Check pending state - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr); // Admin should still be current @@ -144,7 +144,7 @@ module movekit::access_control_admin_registry_tests { assert!(!access_control_admin_registry::is_current_admin(admin_addr)); // Check pending admin cleaned up - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(admin = @movekit, new_admin = @0x123)] @@ -177,7 +177,7 @@ module movekit::access_control_admin_registry_tests { // Propose transfer access_control_admin_registry::transfer_admin(admin, new_admin_addr); - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); // Cancel transfer access_control_admin_registry::cancel_admin_transfer(admin); @@ -187,7 +187,7 @@ module movekit::access_control_admin_registry_tests { assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Check pending admin cleaned up - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } // =========================================== @@ -398,8 +398,7 @@ module movekit::access_control_admin_registry_tests { // Should allow transfer to zero address (for renouncing) access_control_admin_registry::transfer_admin(admin, @0x0); - let admin_addr = signer::address_of(admin); - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); assert!(access_control_admin_registry::get_pending_admin() == @0x0); } @@ -425,25 +424,22 @@ module movekit::access_control_admin_registry_tests { fun test_has_pending_admin_various_states(admin: &signer) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); - // Initially no pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); // After proposing transfer access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); // After canceling access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test] fun test_has_pending_admin_nonexistent_address() { - // Should return false for addresses that never had pending admin - assert!(!access_control_admin_registry::has_pending_admin(@0x123)); - assert!(!access_control_admin_registry::has_pending_admin(@0x456)); + // Should return false for uninitialized registry + assert!(!access_control_admin_registry::has_pending_admin()); } // =========================================== @@ -528,18 +524,15 @@ module movekit::access_control_admin_registry_tests { ) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Propose and accept transfer access_control_admin_registry::transfer_admin(admin, new_admin_addr); access_control_admin_registry::accept_pending_admin(new_admin); - // Old admin should no longer have pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); - - // New admin should not have pending admin either - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); + // No longer has pending admin after transfer + assert!(!access_control_admin_registry::has_pending_admin()); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(admin = @movekit, new_admin = @0x123)] @@ -548,18 +541,15 @@ module movekit::access_control_admin_registry_tests { ) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Propose and cancel transfer access_control_admin_registry::transfer_admin(admin, new_admin_addr); access_control_admin_registry::cancel_admin_transfer(admin); - // Admin should no longer have pending admin - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); - - // New admin should not have pending admin - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); + // No longer has pending admin after cancellation + assert!(!access_control_admin_registry::has_pending_admin()); + assert!(!access_control_admin_registry::has_pending_admin()); } // =========================================== @@ -572,67 +562,55 @@ module movekit::access_control_admin_registry_tests { ) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Setup transfer access_control_admin_registry::transfer_admin(admin, new_admin_addr); // Verify pending admin exists - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); // Accept transfer access_control_admin_registry::accept_pending_admin(new_admin); - // Verify NO pending admin resources exist anywhere - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); - assert!(!access_control_admin_registry::has_pending_admin(new_admin_addr)); - assert!(!access_control_admin_registry::has_pending_admin(@movekit)); - - // Test random addresses too - assert!(!access_control_admin_registry::has_pending_admin(@0x999)); - assert!(!access_control_admin_registry::has_pending_admin(@0x888)); + // Verify NO pending admin state exists + assert!(!access_control_admin_registry::has_pending_admin()); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(admin = @movekit)] fun test_no_pending_admin_leaks_after_cancel(admin: &signer) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); - // Setup transfer access_control_admin_registry::transfer_admin(admin, @0x123); // Verify pending admin exists - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); // Cancel transfer access_control_admin_registry::cancel_admin_transfer(admin); - // Verify NO pending admin resources exist anywhere - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); - assert!(!access_control_admin_registry::has_pending_admin(@0x123)); - assert!(!access_control_admin_registry::has_pending_admin(@movekit)); + // Verify NO pending admin state exists + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(admin = @movekit)] fun test_no_pending_admin_leaks_after_overwrite(admin: &signer) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); - // Multiple overwrites access_control_admin_registry::transfer_admin(admin, @0x111); access_control_admin_registry::transfer_admin(admin, @0x222); access_control_admin_registry::transfer_admin(admin, @0x333); // Should only have ONE pending admin (the latest) - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); assert!(access_control_admin_registry::get_pending_admin() == @0x333); // Cancel and verify clean slate access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } // =========================================== @@ -664,7 +642,6 @@ module movekit::access_control_admin_registry_tests { ) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Rapid sequence: propose -> cancel -> propose -> accept @@ -675,14 +652,13 @@ module movekit::access_control_admin_registry_tests { // Verify final state is correct assert!(access_control_admin_registry::get_current_admin() == new_admin_addr); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } #[test(admin = @movekit, new_admin = @0x123)] fun test_interleaved_operations(admin: &signer, new_admin: &signer) { access_control_admin_registry::init_for_testing(admin); - let admin_addr = signer::address_of(admin); let new_admin_addr = signer::address_of(new_admin); // Complex sequence testing state machine robustness @@ -691,7 +667,10 @@ module movekit::access_control_admin_registry_tests { // Verify state is correct before accept assert!(access_control_admin_registry::get_pending_admin() == new_admin_addr); - assert!(access_control_admin_registry::get_current_admin() == admin_addr); + assert!( + access_control_admin_registry::get_current_admin() + == signer::address_of(admin) + ); // Accept should work access_control_admin_registry::accept_pending_admin(new_admin); @@ -715,19 +694,19 @@ module movekit::access_control_admin_registry_tests { // Initial state verification assert!(access_control_admin_registry::is_current_admin(admin_addr)); assert!(access_control_admin_registry::get_current_admin() == admin_addr); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); // After proposal access_control_admin_registry::transfer_admin(admin, @0x123); assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Still current assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Still current - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); // Now has pending + assert!(access_control_admin_registry::has_pending_admin()); // Now has pending // After cancel access_control_admin_registry::cancel_admin_transfer(admin); assert!(access_control_admin_registry::is_current_admin(admin_addr)); // Still current assert!(access_control_admin_registry::get_current_admin() == admin_addr); // Still current - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); // No longer pending + assert!(!access_control_admin_registry::has_pending_admin()); // No longer pending } #[test(admin = @movekit)] @@ -753,15 +732,15 @@ module movekit::access_control_admin_registry_tests { 2 ); - // PendingAdmin should be created and destroyed properly + // Pending admin state should be created and destroyed properly let admin_addr = signer::address_of(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); } // =========================================== @@ -807,14 +786,14 @@ module movekit::access_control_admin_registry_tests { // Invariant: AdminRegistry always exists after initialization assert!(access_control_admin_registry::get_current_admin() == admin_addr); - // Invariant: PendingAdmin exists iff there's an active proposal - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + // Invariant: Pending admin state exists iff there's an active proposal + assert!(!access_control_admin_registry::has_pending_admin()); access_control_admin_registry::transfer_admin(admin, @0x123); - assert!(access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(access_control_admin_registry::has_pending_admin()); access_control_admin_registry::cancel_admin_transfer(admin); - assert!(!access_control_admin_registry::has_pending_admin(admin_addr)); + assert!(!access_control_admin_registry::has_pending_admin()); // Invariant: get_current_admin() always returns a valid address let current = access_control_admin_registry::get_current_admin(); diff --git a/tests/access_control/core_test.move b/tests/access_control/core_test.move index 784c64b..424918d 100644 --- a/tests/access_control/core_test.move +++ b/tests/access_control/core_test.move @@ -107,7 +107,7 @@ module movekit::access_control_core_tests { access_control_core::transfer_admin(admin, new_admin_addr); // Check pending state (delegated functions) - assert!(access_control_core::has_pending_admin(admin_addr), 0); + assert!(access_control_core::has_pending_admin(), 0); assert!(access_control_core::get_pending_admin() == new_admin_addr, 1); // Admin should still be current @@ -128,7 +128,7 @@ module movekit::access_control_core_tests { assert!(!access_control_core::has_role(admin_addr), 9); // Check pending admin cleaned up (delegated to admin registry) - assert!(!access_control_core::has_pending_admin(admin_addr), 10); + assert!(!access_control_core::has_pending_admin(), 10); } #[test(admin = @movekit, new_admin = @0x123)] @@ -142,7 +142,7 @@ module movekit::access_control_core_tests { // Propose transfer access_control_core::transfer_admin(admin, new_admin_addr); - assert!(access_control_core::has_pending_admin(admin_addr), 0); + assert!(access_control_core::has_pending_admin(), 0); // Cancel transfer (delegated to admin registry) access_control_core::cancel_admin_transfer(admin); @@ -153,7 +153,7 @@ module movekit::access_control_core_tests { assert!(access_control_core::has_role(admin_addr), 3); // Check pending admin cleaned up - assert!(!access_control_core::has_pending_admin(admin_addr), 4); + assert!(!access_control_core::has_pending_admin(), 4); } #[test(admin = @movekit, new_admin = @0x123)] From ba26fd17c0a3f9ce8af9a89e1d8a031d000405fb Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Sun, 22 Jun 2025 15:49:43 +0200 Subject: [PATCH 20/25] refactor: Changed Function Visibility --- sources/access_control/admin_registry.move | 24 +++++++++---------- sources/access_control/core.move | 14 +++++------ tests/access_control/admin_registry_test.move | 1 - 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move index 3223213..c6af302 100644 --- a/sources/access_control/admin_registry.move +++ b/sources/access_control/admin_registry.move @@ -3,8 +3,6 @@ module movekit::access_control_admin_registry { use std::event; use std::option::{Self, Option}; - friend movekit::access_control_core; - struct AdminRegistry has key { current_admin: address, pending_admin: Option
@@ -37,27 +35,27 @@ module movekit::access_control_admin_registry { canceled_pending: address } - // -- Public Functions -- // + // -- Package Functions -- // #[view] - public fun get_current_admin(): address acquires AdminRegistry { + package fun get_current_admin(): address acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); (&AdminRegistry[@movekit]).current_admin } #[view] - public fun is_current_admin(addr: address): bool acquires AdminRegistry { + package fun is_current_admin(addr: address): bool acquires AdminRegistry { get_current_admin() == addr } - public fun require_admin(admin: &signer) acquires AdminRegistry { + package fun require_admin(admin: &signer) acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); let registry = &AdminRegistry[@movekit]; assert!(registry.current_admin == signer::address_of(admin), E_NOT_ADMIN); } /// Current admin proposes new admin - public fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry { + package fun transfer_admin(admin: &signer, new_admin: address) acquires AdminRegistry { require_admin(admin); let admin_addr = signer::address_of(admin); assert!(new_admin != admin_addr, E_SELF_TRANSFER_NOT_ALLOWED); @@ -72,7 +70,7 @@ module movekit::access_control_admin_registry { } /// New admin accepts the transfer - public fun accept_pending_admin(new_admin: &signer) acquires AdminRegistry { + package fun accept_pending_admin(new_admin: &signer) acquires AdminRegistry { let new_admin_addr = signer::address_of(new_admin); assert!(exists(@movekit), E_NOT_INITIALIZED); let registry = &mut AdminRegistry[@movekit]; @@ -96,7 +94,7 @@ module movekit::access_control_admin_registry { } /// Cancel pending admin transfer - public fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry { + package fun cancel_admin_transfer(admin: &signer) acquires AdminRegistry { require_admin(admin); let registry = &mut AdminRegistry[@movekit]; assert!(option::is_some(®istry.pending_admin), E_NO_PENDING_ADMIN); @@ -114,7 +112,7 @@ module movekit::access_control_admin_registry { #[view] /// Get pending admin address - public fun get_pending_admin(): address acquires AdminRegistry { + package fun get_pending_admin(): address acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); let registry = &AdminRegistry[@movekit]; assert!(option::is_some(®istry.pending_admin), E_NO_PENDING_ADMIN); @@ -123,14 +121,14 @@ module movekit::access_control_admin_registry { #[view] /// Check if there's a pending admin transfer - public fun has_pending_admin(): bool acquires AdminRegistry { + package fun has_pending_admin(): bool acquires AdminRegistry { if (!exists(@movekit)) return false; let registry = &AdminRegistry[@movekit]; option::is_some(®istry.pending_admin) } /// Allow friend modules to initialize admin registry (idempotent) - friend fun init_admin_registry(admin: &signer) { + package fun init_admin_registry(admin: &signer) { if (!exists(@movekit)) { let admin_addr = signer::address_of(admin); move_to( @@ -152,7 +150,7 @@ module movekit::access_control_admin_registry { } #[test_only] - public fun init_for_testing(admin: &signer) { + package fun init_for_testing(admin: &signer) { init_module(admin); } } diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 8c42ce4..15aacf7 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -72,12 +72,12 @@ module movekit::access_control_core { // === Admin Delegation Functions === /// Propose admin transfer - delegates to admin registry - public fun transfer_admin(admin: &signer, new_admin: address) { + package fun transfer_admin(admin: &signer, new_admin: address) { access_control_admin_registry::transfer_admin(admin, new_admin) } /// Accept pending admin transfer and synchronize role assignments - public fun accept_pending_admin(new_admin: &signer) acquires RoleRegistry { + package fun accept_pending_admin(new_admin: &signer) acquires RoleRegistry { let new_admin_addr = signer::address_of(new_admin); // Capture current state before any modifications @@ -109,14 +109,14 @@ module movekit::access_control_core { } /// Cancel pending admin transfer - delegates to admin registry - public fun cancel_admin_transfer(admin: &signer) { + package fun cancel_admin_transfer(admin: &signer) { access_control_admin_registry::cancel_admin_transfer(admin) } // === Role Management Functions === /// Grant role to target address (Admin role; admin-only) - public fun grant_role(admin: &signer, target: address) acquires RoleRegistry { + package fun grant_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation assert_not_admin_role(); @@ -140,7 +140,7 @@ module movekit::access_control_core { } /// Revoke role from target address (Admin role; admin-only) - public fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { + package fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation assert_not_admin_role(); @@ -351,13 +351,13 @@ module movekit::access_control_core { #[test_only] /// Initialize system for testing purposes - public fun init_for_testing(admin: &signer) acquires RoleRegistry { + package fun init_for_testing(admin: &signer) acquires RoleRegistry { init_module(admin); } #[test_only] /// Test-only function to verify Admin role protection - public fun test_admin_role_protection(): bool { + package fun test_admin_role_protection(): bool { type_info::type_of() == type_info::type_of() } } diff --git a/tests/access_control/admin_registry_test.move b/tests/access_control/admin_registry_test.move index 34c842a..3297aa6 100644 --- a/tests/access_control/admin_registry_test.move +++ b/tests/access_control/admin_registry_test.move @@ -733,7 +733,6 @@ module movekit::access_control_admin_registry_tests { ); // Pending admin state should be created and destroyed properly - let admin_addr = signer::address_of(admin); assert!(!access_control_admin_registry::has_pending_admin()); access_control_admin_registry::transfer_admin(admin, @0x123); From 39840973689806cf09aedd4831f3c6a9ba8ae83c Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Tue, 24 Jun 2025 15:41:16 +0200 Subject: [PATCH 21/25] refactor: Comments correction --- sources/access_control/admin_registry.move | 40 +++++++++++----------- sources/access_control/core.move | 36 +++++++++---------- 2 files changed, 36 insertions(+), 40 deletions(-) diff --git a/sources/access_control/admin_registry.move b/sources/access_control/admin_registry.move index c6af302..1a4f1e0 100644 --- a/sources/access_control/admin_registry.move +++ b/sources/access_control/admin_registry.move @@ -37,17 +37,6 @@ module movekit::access_control_admin_registry { // -- Package Functions -- // - #[view] - package fun get_current_admin(): address acquires AdminRegistry { - assert!(exists(@movekit), E_NOT_INITIALIZED); - (&AdminRegistry[@movekit]).current_admin - } - - #[view] - package fun is_current_admin(addr: address): bool acquires AdminRegistry { - get_current_admin() == addr - } - package fun require_admin(admin: &signer) acquires AdminRegistry { assert!(exists(@movekit), E_NOT_INITIALIZED); let registry = &AdminRegistry[@movekit]; @@ -110,6 +99,17 @@ module movekit::access_control_admin_registry { ); } + /// Allow friend modules to initialize admin registry (idempotent) + package fun init_admin_registry(admin: &signer) { + if (!exists(@movekit)) { + let admin_addr = signer::address_of(admin); + move_to( + admin, + AdminRegistry { current_admin: admin_addr, pending_admin: option::none() } + ); + } + } + #[view] /// Get pending admin address package fun get_pending_admin(): address acquires AdminRegistry { @@ -127,15 +127,15 @@ module movekit::access_control_admin_registry { option::is_some(®istry.pending_admin) } - /// Allow friend modules to initialize admin registry (idempotent) - package fun init_admin_registry(admin: &signer) { - if (!exists(@movekit)) { - let admin_addr = signer::address_of(admin); - move_to( - admin, - AdminRegistry { current_admin: admin_addr, pending_admin: option::none() } - ); - } + #[view] + package fun get_current_admin(): address acquires AdminRegistry { + assert!(exists(@movekit), E_NOT_INITIALIZED); + (&AdminRegistry[@movekit]).current_admin + } + + #[view] + package fun is_current_admin(addr: address): bool acquires AdminRegistry { + get_current_admin() == addr } // -- Private Functions -- // diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 15aacf7..3ec1e56 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -67,9 +67,7 @@ module movekit::access_control_core { new_admin: address } - // -- Public Interface -- // - - // === Admin Delegation Functions === + // -- Package Functions -- // /// Propose admin transfer - delegates to admin registry package fun transfer_admin(admin: &signer, new_admin: address) { @@ -113,7 +111,7 @@ module movekit::access_control_core { access_control_admin_registry::cancel_admin_transfer(admin) } - // === Role Management Functions === + // -- Role Management Functions -- // /// Grant role to target address (Admin role; admin-only) package fun grant_role(admin: &signer, target: address) acquires RoleRegistry { @@ -155,11 +153,22 @@ module movekit::access_control_core { // Emit audit event event::emit( - RoleRevoked { admin: signer::address_of(admin), target: target } + RoleRevoked { admin: signer::address_of(admin), target: target } + ); + } + + // -- Public functions -- // + + /// Assert caller has required role or abort with clear error + /// Useful for other modules requiring specific role authorization + public fun require_role(account: &signer) acquires RoleRegistry { + assert!( + has_role(signer::address_of(account)), + E_NO_SUCH_ROLE ); } - // === View Functions === + // -- View Functions -- // #[view] /// Check if address has a specific role @@ -191,6 +200,7 @@ module movekit::access_control_core { access_control_admin_registry::is_current_admin(addr) } + #[view] /// Get all roles assigned to an address in sorted order /// Returns empty vector for uninitialized system or non-existent users public fun get_roles(addr: address): vector acquires RoleRegistry { @@ -224,14 +234,6 @@ module movekit::access_control_core { ordered_map::length(user_roles) } - /// Assert caller has required role or abort with clear error - /// Useful for other modules requiring specific role authorization - public fun require_role(account: &signer) acquires RoleRegistry { - assert!( - has_role(signer::address_of(account)), - E_NO_SUCH_ROLE - ); - } #[view] /// Get pending admin address from admin registry @@ -354,10 +356,4 @@ module movekit::access_control_core { package fun init_for_testing(admin: &signer) acquires RoleRegistry { init_module(admin); } - - #[test_only] - /// Test-only function to verify Admin role protection - package fun test_admin_role_protection(): bool { - type_info::type_of() == type_info::type_of() - } } From 2a7bcd7ca14e4dd7c66320f64a34ff75d6dbd7e4 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Tue, 24 Jun 2025 15:42:06 +0200 Subject: [PATCH 22/25] Formatting --- sources/access_control/core.move | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 3ec1e56..8da0d0f 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -153,7 +153,7 @@ module movekit::access_control_core { // Emit audit event event::emit( - RoleRevoked { admin: signer::address_of(admin), target: target } + RoleRevoked { admin: signer::address_of(admin), target: target } ); } @@ -234,7 +234,6 @@ module movekit::access_control_core { ordered_map::length(user_roles) } - #[view] /// Get pending admin address from admin registry public fun get_pending_admin(): address { From 7bb251700868d62dcaeabaad2b329157ac209745 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Tue, 24 Jun 2025 17:17:04 +0200 Subject: [PATCH 23/25] doc: Add ARCHITECTURE.md and Update README.md --- sources/access_control/ARCHITECTURE.md | 194 +++++++++++++++++++++++++ sources/access_control/README.md | 79 ++++++++-- 2 files changed, 263 insertions(+), 10 deletions(-) create mode 100644 sources/access_control/ARCHITECTURE.md diff --git a/sources/access_control/ARCHITECTURE.md b/sources/access_control/ARCHITECTURE.md new file mode 100644 index 0000000..5edc365 --- /dev/null +++ b/sources/access_control/ARCHITECTURE.md @@ -0,0 +1,194 @@ +# Access Control Architecture + +## Overview + +The Aptos MoveKit Access Control system provides secure, type-safe role-based access control (RBAC) for smart contracts. It consists of two core modules that work together to manage admin privileges and role assignments. + +## Core Components + +### Modules + +1. **`access_control_admin_registry`** - Manages admin transfers using a secure two-step process +2. **`access_control_core`** - Coordinates role management and delegates admin operations + +### Data Structures + +```mermaid +classDiagram + class AdminRegistry { + +address current_admin + +Option pending_admin + } + + class RoleRegistry { + +Table roles + } + + class Admin { + <> + } + + AdminRegistry -- RoleRegistry : synchronized + RoleRegistry -- Admin : manages +``` + +## System Architecture + +```mermaid +graph TB + subgraph "Access Control Core" + Core[access_control_core] + Registry[access_control_admin_registry] + + Core --> Registry + end + + subgraph "Storage" + AdminReg[(AdminRegistry)] + RoleReg[(RoleRegistry)] + end + + subgraph "External" + Client[Client Contracts] + Admin[Admin Users] + end + + Core --> RoleReg + Registry --> AdminReg + Client --> Core + Admin --> Core + + Core -.->|Events| EventSystem[Event System] +``` + +## Admin Transfer Flow + +The system uses a secure two-step transfer process to prevent accidental admin loss: + +```mermaid +sequenceDiagram + participant CurrentAdmin + participant AdminRegistry + participant RoleRegistry + participant NewAdmin + + CurrentAdmin->>AdminRegistry: transfer_admin(new_admin) + AdminRegistry->>AdminRegistry: Set pending_admin + AdminRegistry-->>CurrentAdmin: AdminTransferProposed event + + Note over AdminRegistry: Pending state + + NewAdmin->>AdminRegistry: accept_pending_admin() + AdminRegistry->>AdminRegistry: Update current_admin + AdminRegistry->>RoleRegistry: Synchronize Admin role + RoleRegistry->>RoleRegistry: Grant Admin to new_admin + RoleRegistry->>RoleRegistry: Revoke Admin from old_admin + AdminRegistry-->>NewAdmin: AdminTransferCompleted event + RoleRegistry-->>NewAdmin: AdminRoleTransferred event +``` + +## Role Management Flow + +```mermaid +sequenceDiagram + participant Admin + participant Core + participant Registry as AdminRegistry + participant RoleRegistry + participant Target + + Admin->>Core: grant_role(target) + Core->>Core: assert_not_admin_role() + Core->>Registry: require_admin(admin) + Core->>RoleRegistry: Check !has_role(target) + Core->>RoleRegistry: grant_role_internal(target) + Core-->>Admin: RoleGranted event + + Note over Core: Similar flow for revoke_role +``` + +## Security Model + +### Protection Mechanisms + +1. **Admin Role Protection** - Admin role cannot be manually granted/revoked +2. **Two-Step Transfer** - Prevents accidental admin loss +3. **Authorization Checks** - All operations validate admin permissions +4. **Type Safety** - Phantom types ensure compile-time role verification +5. **State Validation** - Prevents duplicate assignments and missing roles + +### Access Control Matrix + +```mermaid +graph LR + subgraph "Permissions" + Admin[Admin Role] + CustomRole[Custom Roles] + end + + subgraph "Operations" + Transfer[Admin Transfer] + Grant[Grant Roles] + Revoke[Revoke Roles] + Query[Query Functions] + end + + Admin --> Transfer + Admin --> Grant + Admin --> Revoke + Admin --> Query + CustomRole --> Query +``` + +## Event System + +All operations emit events for audit trails: + +- `AdminTransferProposed` - Admin transfer initiated +- `AdminTransferCompleted` - Admin transfer completed +- `AdminTransferCanceled` - Admin transfer canceled +- `AdminRoleTransferred` - Admin role synchronized in RoleRegistry +- `RoleGranted` - Custom role granted +- `RoleRevoked` - Custom role revoked + +## Usage Patterns + +### Role Definition +```move +struct Treasurer has copy, drop {} +struct Manager has copy, drop {} +``` + +### Permission Checks +```move +public entry fun sensitive_operation(account: &signer) { + access_control_core::require_role(account); + // operation logic +} +``` + +### Admin Operations +```move +// Grant role (admin only) +access_control_core::grant_role(admin, target_address); + +// Transfer admin (two-step) +access_control_core::transfer_admin(admin, new_admin_address); +access_control_core::accept_pending_admin(new_admin); +``` + +## Design Principles + +1. **Separation of Concerns** - Admin management separate from role management +2. **Fail-Safe Defaults** - Graceful handling of uninitialized states +3. **Atomic Operations** - State changes are consistent across registries +4. **Storage Efficiency** - Cleanup of empty role maps +5. **Auditability** - Comprehensive event emission + +## Integration Points + +External contracts integrate through: +- `require_role()` for permission checks +- `has_role()` for conditional logic +- View functions for role queries +- Event listening for audit systems \ No newline at end of file diff --git a/sources/access_control/README.md b/sources/access_control/README.md index c9659c0..494e74b 100644 --- a/sources/access_control/README.md +++ b/sources/access_control/README.md @@ -1,19 +1,78 @@ -# Access Control Core +# Access Control System -Role-based access control (RBAC) system for Aptos Move applications. +Role-based access control (RBAC) for Aptos Move contracts using phantom types. -## Features -- Type-safe role management using phantom types -- Admin role transfer capabilities -- Event emission for audit trails +## Architecture + +Two modules: +- `access_control_admin_registry` - Manages admin transfers with two-step verification +- `access_control_core` - Handles role assignments and authorization ## Usage -```move -use movekit::access_control_core; +### Define Roles +```move struct Treasurer has copy, drop {} +struct Manager has copy, drop {} +``` +### Protect Functions +```move public entry fun withdraw(account: &signer, amount: u64) { access_control_core::require_role(account); - // withdrawal logic -} \ No newline at end of file + // protected logic +} +``` + +### Manage Roles (Admin Only) +```move +// Grant role +access_control_core::grant_role(admin, user_address); + +// Revoke role +access_control_core::revoke_role(admin, user_address); + +// Check role +let has_role = access_control_core::has_role(user_address); +``` + +### Transfer Admin +```move +// Step 1: Current admin proposes transfer +access_control_core::transfer_admin(admin, new_admin_address); + +// Step 2: New admin accepts +access_control_core::accept_pending_admin(new_admin); +``` + +## Key Functions + +| Function | Description | +|----------|-------------| +| `require_role(account)` | Assert account has role T | +| `has_role(address)` | Check if address has role T | +| `grant_role(admin, target)` | Grant role T to target | +| `revoke_role(admin, target)` | Revoke role T from target | +| `get_roles(address)` | Get all roles for address | +| `transfer_admin(admin, new_admin)` | Propose admin transfer | +| `accept_pending_admin(new_admin)` | Accept admin transfer | + +## Events + +- `RoleGranted` - Role granted +- `RoleRevoked` - Role revoked +- `AdminTransferProposed` - Admin transfer initiated +- `AdminTransferCompleted` - Admin transfer completed + +## Error Codes + +- `E_NOT_ADMIN` (0) - Caller not admin +- `E_ALREADY_HAS_ROLE` (1) - Role already assigned +- `E_NO_SUCH_ROLE` (2) - Role not found +- `E_ADMIN_ROLE_PROTECTED` (4) - Admin role cannot be manually managed + +## Security Notes + +- Admin role is protected - only transferable via two-step process +- Built-in Admin role type cannot be granted/revoked manually +- All operations emit events for audit trails \ No newline at end of file From e7033141bf8440c1c41351c42284d4fdb83b7828 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 27 Jun 2025 12:27:46 +0200 Subject: [PATCH 24/25] fix: Corrected Logic --- sources/access_control/core.move | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 8da0d0f..1dbe106 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -114,7 +114,7 @@ module movekit::access_control_core { // -- Role Management Functions -- // /// Grant role to target address (Admin role; admin-only) - package fun grant_role(admin: &signer, target: address) acquires RoleRegistry { + public fun grant_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation assert_not_admin_role(); From 601a915549929ba5ad91f216f79dc49a0c642443 Mon Sep 17 00:00:00 2001 From: PersonaNormale Date: Fri, 27 Jun 2025 14:43:53 +0200 Subject: [PATCH 25/25] fix: Corrected Logic --- sources/access_control/core.move | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sources/access_control/core.move b/sources/access_control/core.move index 1dbe106..394a967 100644 --- a/sources/access_control/core.move +++ b/sources/access_control/core.move @@ -138,7 +138,7 @@ module movekit::access_control_core { } /// Revoke role from target address (Admin role; admin-only) - package fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { + public fun revoke_role(admin: &signer, target: address) acquires RoleRegistry { // Security: Prevent manual Admin role manipulation assert_not_admin_role();