From e931c1cbfcf92fa34b557c1d88c4f7218861e587 Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Fri, 24 Nov 2017 12:56:03 +0200 Subject: [PATCH 1/5] feat: RBAC authentication contract and role library --- contracts/examples/RBACExample.sol | 61 +++++++++++++++++ contracts/ownership/rbac/RBAC.sol | 101 +++++++++++++++++++++++++++++ contracts/ownership/rbac/Roles.sol | 55 ++++++++++++++++ test/RBAC.js | 77 ++++++++++++++++++++++ test/helpers/RBACMock.sol | 68 +++++++++++++++++++ 5 files changed, 362 insertions(+) create mode 100644 contracts/examples/RBACExample.sol create mode 100644 contracts/ownership/rbac/RBAC.sol create mode 100644 contracts/ownership/rbac/Roles.sol create mode 100644 test/RBAC.js create mode 100644 test/helpers/RBACMock.sol diff --git a/contracts/examples/RBACExample.sol b/contracts/examples/RBACExample.sol new file mode 100644 index 000000000..ddc765873 --- /dev/null +++ b/contracts/examples/RBACExample.sol @@ -0,0 +1,61 @@ +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 new file mode 100644 index 000000000..39c717c3b --- /dev/null +++ b/contracts/ownership/rbac/RBAC.sol @@ -0,0 +1,101 @@ +pragma solidity ^0.4.18; + +import './Roles.sol'; + + +/** + * @title RBAC (Role-Based Access Control) + * @author Matt Condon (@Shrugs) + * @dev Stores and provides setters and getters for roles and addresses. + * Supports unlimited numbers of roles and addresses. + * See //contracts/examples/RBACExample.sol for an example of usage. + * This RBAC method uses strings to key roles. It may be beneficial + * for you to write your own implementation of this interface using Enums or similar. + */ +contract RBAC { + using Roles for Roles.Role; + + mapping (string => Roles.Role) internal roles; + + /** + * @dev add a role to an address + * @param addr address + * @param roleName the name of the role + */ + function addRole(address addr, string roleName) + internal + { + roles[roleName].add(addr); + } + + /** + * @dev remove a role from an address + * @param addr address + * @param roleName the name of the role + */ + function removeRole(address addr, string roleName) + internal + { + roles[roleName].remove(addr); + } + + /** + * @dev reverts if addr does not have role + * @param addr address + * @param roleName the name of the role + * // reverts + */ + function checkRole(address addr, string roleName) + view + internal + { + roles[roleName].check(addr); + } + + /** + * @dev determine if addr has role + * @param addr address + * @param roleName the name of the role + * @return bool + */ + function hasRole(address addr, string roleName) + view + internal + returns (bool) + { + return roles[roleName].has(addr); + } + + /** + * @dev modifier to scope access to a single role (uses msg.sender as addr) + * @param roleName the name of the role + * // reverts + */ + modifier onlyRole(string roleName) + { + checkRole(msg.sender, roleName); + _; + } + + /** + * @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 + * see: https://github.com/ethereum/solidity/issues/2467 + */ + // modifier onlyRoles(string[] roleNames) { + // bool hasAnyRole = false; + // for (uint8 i = 0; i < roleNames.length; i++) { + // if (hasRole(msg.sender, roleNames[i])) { + // hasAnyRole = true; + // break; + // } + // } + + // require(hasAnyRole); + + // _; + // } +} diff --git a/contracts/ownership/rbac/Roles.sol b/contracts/ownership/rbac/Roles.sol new file mode 100644 index 000000000..3ef7e2716 --- /dev/null +++ b/contracts/ownership/rbac/Roles.sol @@ -0,0 +1,55 @@ +pragma solidity ^0.4.18; + + +/** + * @title Roles + * @author Francisco Giordano (@frangio) + * @dev Library for managing addresses assigned to a Role. + * See RBAC.sol for example usage. + */ +library Roles { + struct Role { + mapping (address => bool) bearer; + } + + /** + * @dev give an address access to this role + */ + function add(Role storage role, address addr) + internal + { + role.bearer[addr] = true; + } + + /** + * @dev remove an address' access to this role + */ + function remove(Role storage role, address addr) + internal + { + role.bearer[addr] = false; + } + + /** + * @dev check if an address has this role + * // reverts + */ + function check(Role storage role, address addr) + view + internal + { + require(has(role, addr)); + } + + /** + * @dev check if an address has this role + * @return bool + */ + function has(Role storage role, address addr) + view + internal + returns (bool) + { + return role.bearer[addr]; + } +} diff --git a/test/RBAC.js b/test/RBAC.js new file mode 100644 index 000000000..1846f5698 --- /dev/null +++ b/test/RBAC.js @@ -0,0 +1,77 @@ +const RBACMock = artifacts.require('./helpers/RBACMock.sol') + +import expectThrow from './helpers/expectThrow' + +require('chai') + .use(require('chai-as-promised')) + .should() + +contract('RBAC', function(accounts) { + let mock + + const [ + owner, + anyone, + ...advisors + ] = accounts + + before(async () => { + mock = await RBACMock.new(advisors, { from: owner }) + }) + + context('in normal conditions', () => { + it('allows owner to call #onlyOwnersCanDoThis', async () => { + await mock.onlyOwnersCanDoThis({ from: owner }) + .should.be.fulfilled + }) + it('allows owner to call #onlyAdvisorsCanDoThis', async () => { + await mock.onlyAdvisorsCanDoThis({ from: owner }) + .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 }) + .should.be.fulfilled + }) + it('allows advisors to call #eitherOwnerOrAdvisorCanDoThis', async () => { + await mock.eitherOwnerOrAdvisorCanDoThis({ from: advisors[0] }) + .should.be.fulfilled + }) + it('does not allow owners to call #nobodyCanDoThis', async () => { + expectThrow( + mock.nobodyCanDoThis({ from: owner }) + ) + }) + it('does not allow advisors to call #nobodyCanDoThis', async () => { + expectThrow( + mock.nobodyCanDoThis({ from: advisors[0] }) + ) + }) + it('does not allow anyone to call #nobodyCanDoThis', async () => { + expectThrow( + mock.nobodyCanDoThis({ from: anyone }) + ) + }) + it('allows an owner to remove an advisor\'s role', async () => { + await mock.removeAdvisor(advisors[0], { from: owner }) + .should.be.fulfilled + }) + }) + + context('in adversarial conditions', () => { + it('does not allow an advisor to remove another advisor', async () => { + expectThrow( + mock.removeAdvisor(advisors[1], { from: advisors[0] }) + ) + }) + it('does not allow "anyone" to remove an advisor', async () => { + expectThrow( + mock.removeAdvisor(advisors[0], { from: anyone }) + ) + }) + }) + +}) diff --git a/test/helpers/RBACMock.sol b/test/helpers/RBACMock.sol new file mode 100644 index 000000000..9cd8edead --- /dev/null +++ b/test/helpers/RBACMock.sol @@ -0,0 +1,68 @@ +pragma solidity ^0.4.8; + +import '../../contracts/ownership/rbac/RBAC.sol'; + + +contract RBACMock is RBAC { + + modifier onlyOwnerOrAdvisor() + { + require( + hasRole(msg.sender, "owner") || + hasRole(msg.sender, "advisor") + ); + _; + } + + function RBACMock(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 + { + } + + function nobodyCanDoThis() + onlyRole("unknown") + 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"); + } +} From 9bb2c958ec19156aad1234127ee8d265d64d576f Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Fri, 1 Dec 2017 15:36:54 +0200 Subject: [PATCH 2/5] feat: add adminAddRole, adminRemoveRole, and make hasRole/checkRole public --- contracts/examples/RBACExample.sol | 61 ------------------------------ contracts/ownership/rbac/RBAC.sol | 60 +++++++++++++++++++++++++++-- test/RBAC.js | 32 +++++++++------- test/helpers/RBACMock.sol | 18 ++++----- 4 files changed, 84 insertions(+), 87 deletions(-) delete mode 100644 contracts/examples/RBACExample.sol 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 From 5e55569db6b743f35577adabf49dfd1c9399186e Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Fri, 1 Dec 2017 17:53:24 +0200 Subject: [PATCH 3/5] fix: make roles private --- contracts/ownership/rbac/RBAC.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/ownership/rbac/RBAC.sol b/contracts/ownership/rbac/RBAC.sol index dc11b3990..7f3d3b81e 100644 --- a/contracts/ownership/rbac/RBAC.sol +++ b/contracts/ownership/rbac/RBAC.sol @@ -15,7 +15,7 @@ import './Roles.sol'; contract RBAC { using Roles for Roles.Role; - mapping (string => Roles.Role) internal roles; + mapping (string => Roles.Role) private roles; event LogRoleAdded(address addr, string roleName); event LogRoleRemoved(address addr, string roleName); From 677d05743c1df9344827d0159a97923d83adad7b Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Mon, 4 Dec 2017 13:42:55 +0200 Subject: [PATCH 4/5] feat: update event names for OZ standards and test them --- contracts/ownership/rbac/RBAC.sol | 10 ++++++---- test/{RBAC.js => RBAC.test.js} | 22 ++++++++++++++++++++-- test/helpers/expectEvent.js | 16 ++++++++++++++++ test/{helpers => mocks}/RBACMock.sol | 19 ++++++++++--------- 4 files changed, 52 insertions(+), 15 deletions(-) rename test/{RBAC.js => RBAC.test.js} (77%) create mode 100644 test/helpers/expectEvent.js rename test/{helpers => mocks}/RBACMock.sol (72%) diff --git a/contracts/ownership/rbac/RBAC.sol b/contracts/ownership/rbac/RBAC.sol index 7f3d3b81e..4c175c65b 100644 --- a/contracts/ownership/rbac/RBAC.sol +++ b/contracts/ownership/rbac/RBAC.sol @@ -11,14 +11,16 @@ import './Roles.sol'; * See //contracts/examples/RBACExample.sol for an example of usage. * This RBAC method uses strings to key roles. It may be beneficial * for you to write your own implementation of this interface using Enums or similar. + * It's also recommended that you define constants in the contract, like ROLE_ADMIN below, + * to avoid typos. */ contract RBAC { using Roles for Roles.Role; mapping (string => Roles.Role) private roles; - event LogRoleAdded(address addr, string roleName); - event LogRoleRemoved(address addr, string roleName); + event RoleAdded(address addr, string roleName); + event RoleRemoved(address addr, string roleName); /** * A constant role name for indicating admins. @@ -43,7 +45,7 @@ contract RBAC { internal { roles[roleName].add(addr); - LogRoleAdded(addr, roleName); + RoleAdded(addr, roleName); } /** @@ -55,7 +57,7 @@ contract RBAC { internal { roles[roleName].remove(addr); - LogRoleRemoved(addr, roleName); + RoleRemoved(addr, roleName); } /** diff --git a/test/RBAC.js b/test/RBAC.test.js similarity index 77% rename from test/RBAC.js rename to test/RBAC.test.js index ef3866132..19f72e1f5 100644 --- a/test/RBAC.js +++ b/test/RBAC.test.js @@ -1,17 +1,21 @@ -const RBACMock = artifacts.require('./helpers/RBACMock.sol') +const RBACMock = artifacts.require('./mocks/RBACMock.sol') import expectThrow from './helpers/expectThrow' +import expectEvent from './helpers/expectEvent' require('chai') .use(require('chai-as-promised')) .should() +const ROLE_ADVISOR = 'advisor'; + contract('RBAC', function(accounts) { let mock const [ admin, anyone, + futureAdvisor, ...advisors ] = accounts @@ -60,9 +64,23 @@ contract('RBAC', function(accounts) { .should.be.fulfilled }) it('allows admins to #adminRemoveRole', async () => { - await mock.adminRemoveRole(advisors[3], 'advisor', { from: admin }) + await mock.adminRemoveRole(advisors[3], ROLE_ADVISOR, { from: admin }) .should.be.fulfilled }) + + it('announces a RoleAdded event on addRole', async () => { + expectEvent.inTransaction( + mock.adminAddRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), + 'RoleAdded' + ) + }) + + it('announces a RoleRemoved event on removeRole', async () => { + expectEvent.inTransaction( + mock.adminRemoveRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), + 'RoleRemoved' + ) + }) }) context('in adversarial conditions', () => { diff --git a/test/helpers/expectEvent.js b/test/helpers/expectEvent.js new file mode 100644 index 000000000..8a9502c36 --- /dev/null +++ b/test/helpers/expectEvent.js @@ -0,0 +1,16 @@ +const assert = require('chai').assert; + +const inLogs = async (logs, eventName) => { + const event = logs.find(e => e.event === eventName); + assert.exists(event); +}; + +const inTransaction = async (tx, eventName) => { + const { logs } = await tx; + return inLogs(logs, eventName); +}; + +module.exports = { + inLogs, + inTransaction, +}; diff --git a/test/helpers/RBACMock.sol b/test/mocks/RBACMock.sol similarity index 72% rename from test/helpers/RBACMock.sol rename to test/mocks/RBACMock.sol index aff1374d9..ae768a96a 100644 --- a/test/helpers/RBACMock.sol +++ b/test/mocks/RBACMock.sol @@ -5,11 +5,13 @@ import '../../contracts/ownership/rbac/RBAC.sol'; contract RBACMock is RBAC { + string constant ROLE_ADVISOR = "advisor"; + modifier onlyAdminOrAdvisor() { require( - hasRole(msg.sender, "admin") || - hasRole(msg.sender, "advisor") + hasRole(msg.sender, ROLE_ADMIN) || + hasRole(msg.sender, ROLE_ADVISOR) ); _; } @@ -17,23 +19,22 @@ contract RBACMock is RBAC { function RBACMock(address[] _advisors) public { - addRole(msg.sender, "admin"); - addRole(msg.sender, "advisor"); + addRole(msg.sender, ROLE_ADVISOR); for (uint256 i = 0; i < _advisors.length; i++) { - addRole(_advisors[i], "advisor"); + addRole(_advisors[i], ROLE_ADVISOR); } } function onlyAdminsCanDoThis() - onlyRole("admin") + onlyAdmin view external { } function onlyAdvisorsCanDoThis() - onlyRole("advisor") + onlyRole(ROLE_ADVISOR) view external { @@ -60,9 +61,9 @@ contract RBACMock is RBAC { { // revert if the user isn't an advisor // (perhaps you want to soft-fail here instead?) - checkRole(_addr, "advisor"); + checkRole(_addr, ROLE_ADVISOR); // remove the advisor's role - removeRole(_addr, "advisor"); + removeRole(_addr, ROLE_ADVISOR); } } From 63c8751e066f6ad6aaf8f2ae90040674832944fd Mon Sep 17 00:00:00 2001 From: Matt Condon Date: Mon, 4 Dec 2017 17:08:28 +0200 Subject: [PATCH 5/5] fix: linter errors --- test/RBAC.test.js | 81 +++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/test/RBAC.test.js b/test/RBAC.test.js index 19f72e1f5..3db66ad9b 100644 --- a/test/RBAC.test.js +++ b/test/RBAC.test.js @@ -1,99 +1,98 @@ -const RBACMock = artifacts.require('./mocks/RBACMock.sol') +import expectThrow from './helpers/expectThrow'; +import expectEvent from './helpers/expectEvent'; -import expectThrow from './helpers/expectThrow' -import expectEvent from './helpers/expectEvent' +const RBACMock = artifacts.require('./mocks/RBACMock.sol'); require('chai') .use(require('chai-as-promised')) - .should() + .should(); const ROLE_ADVISOR = 'advisor'; -contract('RBAC', function(accounts) { - let mock +contract('RBAC', function (accounts) { + let mock; const [ admin, anyone, futureAdvisor, ...advisors - ] = accounts + ] = accounts; before(async () => { - mock = await RBACMock.new(advisors, { from: admin }) - }) + mock = await RBACMock.new(advisors, { from: admin }); + }); context('in normal conditions', () => { it('allows admin to call #onlyAdminsCanDoThis', async () => { await mock.onlyAdminsCanDoThis({ from: admin }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('allows admin to call #onlyAdvisorsCanDoThis', async () => { await mock.onlyAdvisorsCanDoThis({ from: admin }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('allows advisors to call #onlyAdvisorsCanDoThis', async () => { await mock.onlyAdvisorsCanDoThis({ from: advisors[0] }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('allows admin to call #eitherAdminOrAdvisorCanDoThis', async () => { await mock.eitherAdminOrAdvisorCanDoThis({ from: admin }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('allows advisors to call #eitherAdminOrAdvisorCanDoThis', async () => { await mock.eitherAdminOrAdvisorCanDoThis({ from: advisors[0] }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('does not allow admins to call #nobodyCanDoThis', async () => { expectThrow( mock.nobodyCanDoThis({ from: admin }) - ) - }) + ); + }); it('does not allow advisors to call #nobodyCanDoThis', async () => { expectThrow( mock.nobodyCanDoThis({ from: advisors[0] }) - ) - }) + ); + }); it('does not allow anyone to call #nobodyCanDoThis', async () => { expectThrow( mock.nobodyCanDoThis({ from: anyone }) - ) - }) + ); + }); it('allows an admin to remove an advisor\'s role', async () => { await mock.removeAdvisor(advisors[0], { from: admin }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('allows admins to #adminRemoveRole', async () => { await mock.adminRemoveRole(advisors[3], ROLE_ADVISOR, { from: admin }) - .should.be.fulfilled - }) + .should.be.fulfilled; + }); it('announces a RoleAdded event on addRole', async () => { expectEvent.inTransaction( mock.adminAddRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), 'RoleAdded' - ) - }) + ); + }); it('announces a RoleRemoved event on removeRole', async () => { expectEvent.inTransaction( mock.adminRemoveRole(futureAdvisor, ROLE_ADVISOR, { from: admin }), 'RoleRemoved' - ) - }) - }) + ); + }); + }); context('in adversarial conditions', () => { it('does not allow an advisor to remove another advisor', async () => { expectThrow( mock.removeAdvisor(advisors[1], { from: advisors[0] }) - ) - }) + ); + }); it('does not allow "anyone" to remove an advisor', async () => { expectThrow( mock.removeAdvisor(advisors[0], { from: anyone }) - ) - }) - }) - -}) + ); + }); + }); +});