Skip to content

Commit

Permalink
Merge pull request #2 from Transient-Labs/feature/revoke-all-roles
Browse files Browse the repository at this point in the history 
added revoke all roles feature request
  • Loading branch information
@mpeyfuss
mpeyfuss authored Feb 1, 2023
2 parents 120e04e + 01ec9b4 commit 788f4d6
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 15 deletions.
26 changes: 19 additions & 7 deletions src/access/OwnableAccessControl.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ abstract contract OwnableAccessControl is Ownable {

using EnumerableSet for EnumerableSet.AddressSet;

mapping(bytes32 => mapping(address => bool)) private _roleStatus;
mapping(bytes32 => EnumerableSet.AddressSet) private _roleMembers;
uint256 private _c; // counter to be able to revoke all priviledges
mapping(uint256 => mapping(bytes32 => mapping(address => bool))) private _roleStatus;
mapping(uint256 => mapping(bytes32 => EnumerableSet.AddressSet)) private _roleMembers;

/*//////////////////////////////////////////////////////////////////////////
Events
Expand All @@ -59,6 +60,9 @@ abstract contract OwnableAccessControl is Ownable {
/// @param role - the bytes32 role created in the inheriting contract
event RoleChange(address indexed from, address indexed user, bool indexed approved, bytes32 role);

/// @param from - address that authorized the revoke
event AllRolesRevoked(address indexed from);

/*//////////////////////////////////////////////////////////////////////////
Modifiers
//////////////////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -87,6 +91,14 @@ abstract contract OwnableAccessControl is Ownable {
External Role Functions
//////////////////////////////////////////////////////////////////////////*/

/// @notice function to revoke all roles currently present
/// @dev increments the `_c` variables
/// @dev requires owner privileges
function revokeAllRoles() external onlyOwner {
_c++;
emit AllRolesRevoked(msg.sender);
}

/// @notice function to renounce role
/// @param role - bytes32 role created in inheriting contracts
function renounceRole(bytes32 role) external {
Expand All @@ -113,13 +125,13 @@ abstract contract OwnableAccessControl is Ownable {
/// @param role - bytes32 role created in inheriting contracts
/// @param potentialRoleMember - address to check for role membership
function hasRole(bytes32 role, address potentialRoleMember) public view returns (bool) {
return _roleStatus[role][potentialRoleMember];
return _roleStatus[_c][role][potentialRoleMember];
}

/// @notice function to get role members
/// @param role - bytes32 role created in inheriting contracts
function getRoleMembers(bytes32 role) public view returns (address[] memory) {
return _roleMembers[role].values();
return _roleMembers[_c][role].values();
}

/*//////////////////////////////////////////////////////////////////////////
Expand All @@ -132,11 +144,11 @@ abstract contract OwnableAccessControl is Ownable {
/// @param status - bool whether to remove or add `roleMembers` to the `role`
function _setRole(bytes32 role, address[] memory roleMembers, bool status) internal {
for (uint256 i = 0; i < roleMembers.length; i++) {
_roleStatus[role][roleMembers[i]] = status;
_roleStatus[_c][role][roleMembers[i]] = status;
if (status) {
_roleMembers[role].add(roleMembers[i]);
_roleMembers[_c][role].add(roleMembers[i]);
} else {
_roleMembers[role].remove(roleMembers[i]);
_roleMembers[_c][role].remove(roleMembers[i]);
}
emit RoleChange(msg.sender, roleMembers[i], status, role);
}
Expand Down
26 changes: 19 additions & 7 deletions src/upgradeable/access/OwnableAccessControlUpgradeable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ abstract contract OwnableAccessControlUpgradeable is Initializable, OwnableUpgra

using EnumerableSetUpgradeable for EnumerableSetUpgradeable.AddressSet;

mapping(bytes32 => mapping(address => bool)) private _roleStatus;
mapping(bytes32 => EnumerableSetUpgradeable.AddressSet) private _roleMembers;
uint256 private _c; // counter to be able to revoke all priviledges
mapping(uint256 => mapping(bytes32 => mapping(address => bool))) private _roleStatus;
mapping(uint256 => mapping(bytes32 => EnumerableSetUpgradeable.AddressSet)) private _roleMembers;

/*//////////////////////////////////////////////////////////////////////////
Events
Expand All @@ -59,6 +60,9 @@ abstract contract OwnableAccessControlUpgradeable is Initializable, OwnableUpgra
/// @param approved - boolean indicating the user's status in role
/// @param role - the bytes32 role created in the inheriting contract
event RoleChange(address indexed from, address indexed user, bool indexed approved, bytes32 role);

/// @param from - address that authorized the revoke
event AllRolesRevoked(address indexed from);

/*//////////////////////////////////////////////////////////////////////////
Modifiers
Expand Down Expand Up @@ -95,6 +99,14 @@ abstract contract OwnableAccessControlUpgradeable is Initializable, OwnableUpgra
External Role Functions
//////////////////////////////////////////////////////////////////////////*/

/// @notice function to revoke all roles currently present
/// @dev increments the `_c` variables
/// @dev requires owner privileges
function revokeAllRoles() external onlyOwner {
_c++;
emit AllRolesRevoked(msg.sender);
}

/// @notice function to renounce role
/// @param role - bytes32 role created in inheriting contracts
function renounceRole(bytes32 role) external {
Expand All @@ -121,13 +133,13 @@ abstract contract OwnableAccessControlUpgradeable is Initializable, OwnableUpgra
/// @param role - bytes32 role created in inheriting contracts
/// @param potentialRoleMember - address to check for role membership
function hasRole(bytes32 role, address potentialRoleMember) public view returns (bool) {
return _roleStatus[role][potentialRoleMember];
return _roleStatus[_c][role][potentialRoleMember];
}

/// @notice function to get role members
/// @param role - bytes32 role created in inheriting contracts
function getRoleMembers(bytes32 role) public view returns (address[] memory) {
return _roleMembers[role].values();
return _roleMembers[_c][role].values();
}

/*//////////////////////////////////////////////////////////////////////////
Expand All @@ -140,11 +152,11 @@ abstract contract OwnableAccessControlUpgradeable is Initializable, OwnableUpgra
/// @param status - bool whether to remove or add `roleMembers` to the `role`
function _setRole(bytes32 role, address[] memory roleMembers, bool status) internal {
for (uint256 i = 0; i < roleMembers.length; i++) {
_roleStatus[role][roleMembers[i]] = status;
_roleStatus[_c][role][roleMembers[i]] = status;
if (status) {
_roleMembers[role].add(roleMembers[i]);
_roleMembers[_c][role].add(roleMembers[i]);
} else {
_roleMembers[role].remove(roleMembers[i]);
_roleMembers[_c][role].remove(roleMembers[i]);
}
emit RoleChange(msg.sender, roleMembers[i], status, role);
}
Expand Down
30 changes: 30 additions & 0 deletions test/OwnableAccessControl.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ contract TestOwnableAccessControl is Test {
MockOwnableAccessControl public mockContract;

event RoleChange(address indexed from, address indexed user, bool indexed approved, bytes32 role);
event AllRolesRevoked(address indexed from);

function testInitialValues() public {
mockContract = new MockOwnableAccessControl();
Expand Down Expand Up @@ -51,6 +52,11 @@ contract TestOwnableAccessControl is Test {
emit RoleChange(address(this), address(5), true, mockContract.MINTER_ROLE());
mockContract.setMinterRole(address(5));

// expect owner can revoke all roles
vm.expectEmit(true, false, false, false);
emit AllRolesRevoked(address(this));
mockContract.revokeAllRoles();

// expect reverts on other access controlled functions
vm.expectRevert(abi.encodeWithSelector(NotSpecifiedRole.selector, mockContract.ADMIN_ROLE()));
mockContract.onlyAdminFunction(3);
Expand Down Expand Up @@ -93,6 +99,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyMinterFunction(newNumberOne);
}

// expect can't revoke all roles
if (admin != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// revoke admin functionality
Expand Down Expand Up @@ -121,6 +133,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyMinterFunction(newNumberOne);
}

// expect can't revoke all roles
if (admin != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// expect admin to renounce role
Expand Down Expand Up @@ -164,6 +182,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyOwnerFunction(newNumber);
}

// expect can't revoke all roles
if (minter != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// revoke minter role and expect proper event log
Expand Down Expand Up @@ -192,6 +216,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyOwnerFunction(newNumber);
}

// expect can't revoke all roles
if (minter != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// expect minter able to renounce role
Expand Down
32 changes: 31 additions & 1 deletion test/upgradeable/OwnableAccessControlUpgradeable.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ contract TestOwnableAccessControl is Test {
MockOwnableAccessControlUpgradeable public mockContract;

event RoleChange(address indexed from, address indexed user, bool indexed approved, bytes32 role);

event AllRolesRevoked(address indexed from);

function testInitialization(address owner) public {
mockContract = new MockOwnableAccessControlUpgradeable();
mockContract.initialize(address(this));
Expand Down Expand Up @@ -65,6 +66,11 @@ contract TestOwnableAccessControl is Test {
emit RoleChange(address(this), address(5), true, mockContract.MINTER_ROLE());
mockContract.setMinterRole(address(5));

// expect owner can revoke all roles
vm.expectEmit(true, false, false, false);
emit AllRolesRevoked(address(this));
mockContract.revokeAllRoles();

// expect reverts on other access controlled functions
vm.expectRevert(abi.encodeWithSelector(NotSpecifiedRole.selector, mockContract.ADMIN_ROLE()));
mockContract.onlyAdminFunction(3);
Expand Down Expand Up @@ -108,6 +114,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyMinterFunction(newNumberOne);
}

// expect can't revoke all roles
if (admin != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// revoke admin functionality
Expand Down Expand Up @@ -136,6 +148,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyMinterFunction(newNumberOne);
}

// expect can't revoke all roles
if (admin != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// expect admin to renounce role
Expand Down Expand Up @@ -180,6 +198,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyOwnerFunction(newNumber);
}

// expect can't revoke all roles
if (minter != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// revoke minter role and expect proper event log
Expand Down Expand Up @@ -208,6 +232,12 @@ contract TestOwnableAccessControl is Test {
mockContract.onlyOwnerFunction(newNumber);
}

// expect can't revoke all roles
if (minter != address(this)) {
vm.expectRevert(bytes("Ownable: caller is not the owner"));
mockContract.revokeAllRoles();
}

vm.stopPrank();

// expect minter able to renounce role
Expand Down

0 comments on commit 788f4d6

Please sign in to comment.