diff --git a/contracts/examples/RBACExample.sol b/contracts/examples/RBACExample.sol deleted file mode 100644 index ddc765873..000000000 --- a/contracts/examples/RBACExample.sol +++ /dev/null @@ -1,61 +0,0 @@ -pragma solidity ^0.4.8; - -import '../ownership/rbac/RBAC.sol'; - - -contract RBACExample is RBAC { - - modifier onlyOwnerOrAdvisor() - { - require( - hasRole(msg.sender, "owner") || - hasRole(msg.sender, "advisor") - ); - _; - } - - function RBACExample(address[] _advisors) - public - { - addRole(msg.sender, "owner"); - addRole(msg.sender, "advisor"); - - for (uint256 i = 0; i < _advisors.length; i++) { - addRole(_advisors[i], "advisor"); - } - } - - function onlyOwnersCanDoThis() - onlyRole("owner") - view - external - { - } - - function onlyAdvisorsCanDoThis() - onlyRole("advisor") - view - external - { - } - - function eitherOwnerOrAdvisorCanDoThis() - onlyOwnerOrAdvisor - view - external - { - } - - // owners can remove advisor's role - function removeAdvisor(address _addr) - onlyRole("owner") - public - { - // revert if the user isn't an advisor - // (perhaps you want to soft-fail here instead?) - checkRole(_addr, "advisor"); - - // remove the advisor's role - removeRole(_addr, "advisor"); - } -} diff --git a/contracts/ownership/rbac/RBAC.sol b/contracts/ownership/rbac/RBAC.sol index 39c717c3b..dc11b3990 100644 --- a/contracts/ownership/rbac/RBAC.sol +++ b/contracts/ownership/rbac/RBAC.sol @@ -17,6 +17,23 @@ contract RBAC { mapping (string => Roles.Role) internal roles; + event LogRoleAdded(address addr, string roleName); + event LogRoleRemoved(address addr, string roleName); + + /** + * A constant role name for indicating admins. + */ + string public constant ROLE_ADMIN = "admin"; + + /** + * @dev constructor. Sets msg.sender as admin by default + */ + function RBAC() + public + { + addRole(msg.sender, ROLE_ADMIN); + } + /** * @dev add a role to an address * @param addr address @@ -26,6 +43,7 @@ contract RBAC { internal { roles[roleName].add(addr); + LogRoleAdded(addr, roleName); } /** @@ -37,6 +55,7 @@ contract RBAC { internal { roles[roleName].remove(addr); + LogRoleRemoved(addr, roleName); } /** @@ -47,7 +66,7 @@ contract RBAC { */ function checkRole(address addr, string roleName) view - internal + public { roles[roleName].check(addr); } @@ -60,12 +79,37 @@ contract RBAC { */ function hasRole(address addr, string roleName) view - internal + public returns (bool) { return roles[roleName].has(addr); } + /** + * @dev add a role to an address + * @param addr address + * @param roleName the name of the role + */ + function adminAddRole(address addr, string roleName) + onlyAdmin + public + { + addRole(addr, roleName); + } + + /** + * @dev remove a role from an address + * @param addr address + * @param roleName the name of the role + */ + function adminRemoveRole(address addr, string roleName) + onlyAdmin + public + { + removeRole(addr, roleName); + } + + /** * @dev modifier to scope access to a single role (uses msg.sender as addr) * @param roleName the name of the role @@ -77,12 +121,22 @@ contract RBAC { _; } + /** + * @dev modifier to scope access to admins + * // reverts + */ + modifier onlyAdmin() + { + checkRole(msg.sender, ROLE_ADMIN); + _; + } + /** * @dev modifier to scope access to a set of roles (uses msg.sender as addr) * @param roleNames the names of the roles to scope access to * // reverts * - * @TODO - when solidity supports dynamic arrays as arguments, provide this + * @TODO - when solidity supports dynamic arrays as arguments to modifiers, provide this * see: https://github.com/ethereum/solidity/issues/2467 */ // modifier onlyRoles(string[] roleNames) { diff --git a/test/RBAC.js b/test/RBAC.js index 1846f5698..ef3866132 100644 --- a/test/RBAC.js +++ b/test/RBAC.js @@ -10,39 +10,39 @@ contract('RBAC', function(accounts) { let mock const [ - owner, + admin, anyone, ...advisors ] = accounts before(async () => { - mock = await RBACMock.new(advisors, { from: owner }) + mock = await RBACMock.new(advisors, { from: admin }) }) context('in normal conditions', () => { - it('allows owner to call #onlyOwnersCanDoThis', async () => { - await mock.onlyOwnersCanDoThis({ from: owner }) + it('allows admin to call #onlyAdminsCanDoThis', async () => { + await mock.onlyAdminsCanDoThis({ from: admin }) .should.be.fulfilled }) - it('allows owner to call #onlyAdvisorsCanDoThis', async () => { - await mock.onlyAdvisorsCanDoThis({ from: owner }) + it('allows admin to call #onlyAdvisorsCanDoThis', async () => { + await mock.onlyAdvisorsCanDoThis({ from: admin }) .should.be.fulfilled }) it('allows advisors to call #onlyAdvisorsCanDoThis', async () => { await mock.onlyAdvisorsCanDoThis({ from: advisors[0] }) .should.be.fulfilled }) - it('allows owner to call #eitherOwnerOrAdvisorCanDoThis', async () => { - await mock.eitherOwnerOrAdvisorCanDoThis({ from: owner }) + it('allows admin to call #eitherAdminOrAdvisorCanDoThis', async () => { + await mock.eitherAdminOrAdvisorCanDoThis({ from: admin }) .should.be.fulfilled }) - it('allows advisors to call #eitherOwnerOrAdvisorCanDoThis', async () => { - await mock.eitherOwnerOrAdvisorCanDoThis({ from: advisors[0] }) + it('allows advisors to call #eitherAdminOrAdvisorCanDoThis', async () => { + await mock.eitherAdminOrAdvisorCanDoThis({ from: advisors[0] }) .should.be.fulfilled }) - it('does not allow owners to call #nobodyCanDoThis', async () => { + it('does not allow admins to call #nobodyCanDoThis', async () => { expectThrow( - mock.nobodyCanDoThis({ from: owner }) + mock.nobodyCanDoThis({ from: admin }) ) }) it('does not allow advisors to call #nobodyCanDoThis', async () => { @@ -55,8 +55,12 @@ contract('RBAC', function(accounts) { mock.nobodyCanDoThis({ from: anyone }) ) }) - it('allows an owner to remove an advisor\'s role', async () => { - await mock.removeAdvisor(advisors[0], { from: owner }) + it('allows an admin to remove an advisor\'s role', async () => { + await mock.removeAdvisor(advisors[0], { from: admin }) + .should.be.fulfilled + }) + it('allows admins to #adminRemoveRole', async () => { + await mock.adminRemoveRole(advisors[3], 'advisor', { from: admin }) .should.be.fulfilled }) }) diff --git a/test/helpers/RBACMock.sol b/test/helpers/RBACMock.sol index 9cd8edead..aff1374d9 100644 --- a/test/helpers/RBACMock.sol +++ b/test/helpers/RBACMock.sol @@ -5,10 +5,10 @@ import '../../contracts/ownership/rbac/RBAC.sol'; contract RBACMock is RBAC { - modifier onlyOwnerOrAdvisor() + modifier onlyAdminOrAdvisor() { require( - hasRole(msg.sender, "owner") || + hasRole(msg.sender, "admin") || hasRole(msg.sender, "advisor") ); _; @@ -17,7 +17,7 @@ contract RBACMock is RBAC { function RBACMock(address[] _advisors) public { - addRole(msg.sender, "owner"); + addRole(msg.sender, "admin"); addRole(msg.sender, "advisor"); for (uint256 i = 0; i < _advisors.length; i++) { @@ -25,8 +25,8 @@ contract RBACMock is RBAC { } } - function onlyOwnersCanDoThis() - onlyRole("owner") + function onlyAdminsCanDoThis() + onlyRole("admin") view external { @@ -39,8 +39,8 @@ contract RBACMock is RBAC { { } - function eitherOwnerOrAdvisorCanDoThis() - onlyOwnerOrAdvisor + function eitherAdminOrAdvisorCanDoThis() + onlyAdminOrAdvisor view external { @@ -53,9 +53,9 @@ contract RBACMock is RBAC { { } - // owners can remove advisor's role + // admins can remove advisor's role function removeAdvisor(address _addr) - onlyRole("owner") + onlyAdmin public { // revert if the user isn't an advisor