Revamped Access Control (#2112)

* Remove Roles

* Add AccessControl and tests

* Removed IAccessControl

* Add RoleGranted and RoleRevoked events

* Make roles grantable and revokable regardless of their previous status

* Fix typo

* Add documentation

* Cleanup tests

* Add enumeration tests

* Add _setRoleAdmin tests

* Fix lint error

* Fix AccessControl link in docs

* WIP on access control guide

* Rename getRoleMembersCount

* Add tests for new role admin

* Make AccessControl GSN compatible

* Update access control guide

* Rename admin to adminRole

* Rename roleIds to roles

* Add 'operator' to RoleGranted and RoleRevoked events.

* Only emit events if the roles were not previously granted/revoked

* Uncomment expectEvent.not tests

* Rename operator to sender

* Add changelog entry

CHANGELOG.md

@@ -2,7 +2,11 @@
 
 ## 3.0.0 (unreleased)
 
+### New features
+ * `AccessControl`: new contract for managing permissions in a system, replacement for `Ownable` and `Roles`. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
+
 ### Breaking changes
+ * `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
  * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114))
  * `Pausable`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))
  * `Strings`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))

contracts/access/AccessControl.sol

+pragma solidity ^0.6.0;
+
+import "../utils/EnumerableSet.sol";
+import "../GSN/Context.sol";
+
+/**
+ * @dev Contract module that allows children to implement role-based access
+ * control mechanisms.
+ *
+ * Roles are referred to by their `bytes32` identifier. These should be exposed
+ * in the external API and be unique. The best way to achieve this is by
+ * using `public constant` hash digests:
+ *
+ * ```
+ * bytes32 public constant MY_ROLE = keccak256("MY_ROLE");
+ * ```
+ *
+ * Roles can be used to represent a set of permissions. To restrict access to a
+ * function call, use {hasRole}:
+ *
+ * ```
+ * function foo() public {
+ *     require(hasRole(MY_ROLE, _msgSender()));
+ *     ...
+ * }
+ * ```
+ *
+ * Roles can be granted and revoked programatically by calling the `internal`
+ * {_grantRole} and {_revokeRole} functions.
+ *
+ * This can also be achieved dynamically via the `external` {grantRole} and
+ * {revokeRole} functions. Each role has an associated admin role, and only
+ * accounts that have a role's admin role can call {grantRole} and {revokeRoke}.
+ *
+ * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
+ * that only accounts with this role will be able to grant or revoke other
+ * roles. More complex role relationships can be created by using
+ * {_setRoleAdmin}.
+ */
+abstract contract AccessControl is Context {
+    using EnumerableSet for EnumerableSet.AddressSet;
+
+    struct RoleData {
+        EnumerableSet.AddressSet members;
+        bytes32 adminRole;
+    }
+
+    mapping (bytes32 => RoleData) private _roles;
+
+    bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;
+
+    /**
+     * @dev Emitted when `account` is granted `role`.
+     *
+     * `sender` is the account that originated the contract call:
+     *   - if using `grantRole`, it is the admin role bearer
+     *   - if using `_grantRole`, its meaning is system-dependent
+     */
+    event RoleGranted(bytes32 indexed role, address indexed account, address indexed sender);
+
+    /**
+     * @dev Emitted when `account` is revoked `role`.
+     *
+     * `sender` is the account that originated the contract call:
+     *   - if using `revokeRole`, it is the admin role bearer
+     *   - if using `renounceRole`, it is the role bearer (i.e. `account`)
+     *   - if using `_renounceRole`, its meaning is system-dependent
+     */
+    event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
+
+    /**
+     * @dev Returns `true` if `account` has been granted `role`.
+     */
+    function hasRole(bytes32 role, address account) public view returns (bool) {
+        return _roles[role].members.contains(account);
+    }
+
+    /**
+     * @dev Returns the number of accounts that have `role`. Can be used
+     * together with {getRoleMember} to enumerate all bearers of a role.
+     */
+    function getRoleMemberCount(bytes32 role) public view returns (uint256) {
+        return _roles[role].members.length();
+    }
+
+    /**
+     * @dev Returns one of the accounts that have `role`. `index` must be a
+     * value between 0 and {getRoleMemberCount}, non-inclusive.
+     *
+     * Role bearers are not sorted in any particular way, and their ordering may
+     * change at any point.
+     *
+     * WARNING: When using {getRoleMember} and {getRoleMemberCount}, make sure
+     * you perform all queries on the same block. See the following
+     * https://forum.openzeppelin.com/t/iterating-over-elements-on-enumerableset-in-openzeppelin-contracts/2296[forum post]
+     * for more information.
+     */
+    function getRoleMember(bytes32 role, uint256 index) public view returns (address) {
+        return _roles[role].members.get(index);
+    }
+
+    /**
+     * @dev Returns the admin role that controls `role`. See {grantRole} and
+     * {revokeRole}.
+     *
+     * To change a role's admin, use {_setRoleAdmin}.
+     */
+    function getRoleAdmin(bytes32 role) external view returns (bytes32) {
+        return _roles[role].adminRole;
+    }
+
+    /**
+     * @dev Grants `role` to `account`.
+     *
+     * Calls {_grantRole} internally.
+     *
+     * Requirements:
+     *
+     * - the caller must have `role`'s admin role.
+     */
+    function grantRole(bytes32 role, address account) external virtual {
+        require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
+
+        _grantRole(role, account);
+    }
+
+    /**
+     * @dev Revokes `role` from `account`.
+     *
+     * Calls {_revokeRole} internally.
+     *
+     * Requirements:
+     *
+     * - the caller must have `role`'s admin role.
+     */
+    function revokeRole(bytes32 role, address account) external virtual {
+        require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");
+
+        _revokeRole(role, account);
+    }
+
+    /**
+     * @dev Revokes `role` from the calling account.
+     *
+     * Roles are often managed via {grantRole} and {revokeRole}: this function's
+     * purpose is to provide a mechanism for accounts to lose their privileges
+     * if they are compromised (such as when a trusted device is misplaced).
+     *
+     * Requirements:
+     *
+     * - the caller must be `account`.
+     */
+    function renounceRole(bytes32 role, address account) external virtual {
+        require(account == _msgSender(), "AccessControl: can only renounce roles for self");
+
+        _revokeRole(role, account);
+    }
+
+    /**
+     * @dev Grants `role` to `account`.
+     *
+     * If `account` had not been already granted `role`, emits a {RoleGranted}
+     * event.
+     */
+    function _grantRole(bytes32 role, address account) internal virtual {
+        if (_roles[role].members.add(account)) {
+            emit RoleGranted(role, account, msg.sender);
+        }
+    }
+
+    /**
+     * @dev Revokes `role` from `account`.
+     *
+     * If `account` had been granted `role`, emits a {RoleRevoked} event.
+     */
+    function _revokeRole(bytes32 role, address account) internal virtual {
+        if (_roles[role].members.remove(account)) {
+            emit RoleRevoked(role, account, msg.sender);
+        }
+    }
+
+    /**
+     * @dev Sets `adminRole` as `role`'s admin role.
+     */
+    function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual {
+        _roles[role].adminRole = adminRole;
+    }
+}

contracts/access/README.adoc

@@ -6,4 +6,4 @@ Contract modules for authorization and access control mechanisms.
 
 {{Ownable}}
 
-{{Roles}}
+{{AccessControl}}

contracts/access/Roles.sol

-pragma solidity ^0.6.0;
-
-/**
- * @title Roles
- * @dev Library for managing addresses assigned to a Role.
- */
-library Roles {
-    struct Role {
-        mapping (address => bool) bearer;
-    }
-
-    /**
-     * @dev Give an account access to this role.
-     */
-    function add(Role storage role, address account) internal {
-        require(!has(role, account), "Roles: account already has role");
-        role.bearer[account] = true;
-    }
-
-    /**
-     * @dev Remove an account's access to this role.
-     */
-    function remove(Role storage role, address account) internal {
-        require(has(role, account), "Roles: account does not have role");
-        role.bearer[account] = false;
-    }
-
-    /**
-     * @dev Check if an account has this role.
-     * @return bool
-     */
-    function has(Role storage role, address account) internal view returns (bool) {
-        require(account != address(0), "Roles: account is the zero address");
-        return role.bearer[account];
-    }
-}

contracts/mocks/AccessControlMock.sol

+pragma solidity ^0.6.0;
+
+import "../access/AccessControl.sol";
+
+contract AccessControlMock is AccessControl {
+    constructor() public {
+        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
+    }
+
+    function setRoleAdmin(bytes32 roleId, bytes32 adminRoleId) public {
+        _setRoleAdmin(roleId, adminRoleId);
+    }
+}

contracts/mocks/RolesMock.sol

-pragma solidity ^0.6.0;
-
-import "../access/Roles.sol";
-
-contract RolesMock {
-    using Roles for Roles.Role;
-
-    Roles.Role private dummyRole;
-
-    function add(address account) public {
-        dummyRole.add(account);
-    }
-
-    function remove(address account) public {
-        dummyRole.remove(account);
-    }
-
-    function has(address account) public view returns (bool) {
-        return dummyRole.has(account);
-    }
-}

package-lock.json

@@ -4047,7 +4047,8 @@
     "acorn": {
       "version": "7.1.0",
       "resolved": "https://registry.npmjs.org/acorn/-/acorn-7.1.0.tgz",
-      "integrity": "sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ=="
+      "integrity": "sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ==",
+      "dev": true
     },
     "acorn-jsx": {
       "version": "5.1.0",
@@ -8383,6 +8384,11 @@
             "negotiator": "0.6.2"
           }
         },
+        "acorn": {
+          "version": "7.1.0",
+          "resolved": "https://registry.npmjs.org/acorn/-/acorn-7.1.0.tgz",
+          "integrity": "sha512-kL5CuoXA/dgxlBbVrflsflzQ3PAas7RYZB52NOm/6839iVYJgKMJ3cQJD+t2i5+qFa8h3MDpEOJiS64E8JLnSQ=="
+        },
         "acorn-jsx": {
           "version": "5.1.0",
           "resolved": "https://registry.npmjs.org/acorn-jsx/-/acorn-jsx-5.1.0.tgz",
@@ -21197,6 +21203,7 @@
             "@webassemblyjs/helper-module-context": "1.8.5",
             "@webassemblyjs/wasm-edit": "1.8.5",
             "@webassemblyjs/wasm-parser": "1.8.5",
+            "acorn": "^6.2.1",
             "ajv": "^6.10.2",
             "ajv-keywords": "^3.4.1",
             "chrome-trace-event": "^1.0.2",
@@ -21217,6 +21224,11 @@
             "webpack-sources": "^1.4.1"
           },
           "dependencies": {
+            "acorn": {
+              "version": "6.4.0",
+              "resolved": "https://registry.npmjs.org/acorn/-/acorn-6.4.0.tgz",
+              "integrity": "sha512-gac8OEcQ2Li1dxIEWGZzsp2BitJxwkwcOm0zHAJLcPJaVvm58FRnk6RkuLRpU1EujipU2ZFODv2P9DLMfnV8mw=="
+            },
             "cacache": {
               "version": "12.0.3",
               "resolved": "https://registry.npmjs.org/cacache/-/cacache-12.0.3.tgz",

test/access/AccessControl.test.js

+const { accounts, contract, web3 } = require('@openzeppelin/test-environment');
+
+const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
+
+const { expect } = require('chai');
+
+const AccessControlMock = contract.fromArtifact('AccessControlMock');
+
+describe('AccessControl', function () {
+  const [ admin, authorized, otherAuthorized, other, otherAdmin ] = accounts;
+
+  const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000';
+  const ROLE = web3.utils.soliditySha3('ROLE');
+  const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');
+
+  beforeEach(async function () {
+    this.accessControl = await AccessControlMock.new({ from: admin });
+  });
+
+  describe('default admin', function () {
+    it('deployer has default admin role', async function () {
+      expect(await this.accessControl.hasRole(DEFAULT_ADMIN_ROLE, admin)).to.equal(true);
+    });
+
+    it('other roles\'s admin is the default admin role', async function () {
+      expect(await this.accessControl.getRoleAdmin(ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
+    });
+
+    it('default admin role\'s admin is itself', async function () {
+      expect(await this.accessControl.getRoleAdmin(DEFAULT_ADMIN_ROLE)).to.equal(DEFAULT_ADMIN_ROLE);
+    });
+  });
+
+  describe('granting', function () {
+    it('admin can grant role to other accounts', async function () {
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, sender: admin });
+
+      expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(true);
+    });
+
+    it('non-admin cannot grant role to other accounts', async function () {
+      await expectRevert(
+        this.accessControl.grantRole(ROLE, authorized, { from: other }),
+        'AccessControl: sender must be an admin to grant'
+      );
+    });
+
+    it('accounts can be granted a role multiple times', async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleGranted');
+    });
+  });
+
+  describe('revoking', function () {
+    it('roles that are not had can be revoked', async function () {
+      expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
+
+      const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+    });
+
+    context('with granted role', function () {
+      beforeEach(async function () {
+        await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      });
+
+      it('admin can revoke role', async function () {
+        const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: admin });
+
+        expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
+      });
+
+      it('non-admin cannot revoke role', async function () {
+        await expectRevert(
+          this.accessControl.revokeRole(ROLE, authorized, { from: other }),
+          'AccessControl: sender must be an admin to revoke'
+        );
+      });
+
+      it('a role can be revoked multiple times', async function () {
+        await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+
+        const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: admin });
+        await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+      });
+    });
+  });
+
+  describe('renouncing', function () {
+    it('roles that are not had can be renounced', async function () {
+      const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+      await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+    });
+
+    context('with granted role', function () {
+      beforeEach(async function () {
+        await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      });
+
+      it('bearer can renounce role', async function () {
+        const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+        expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: authorized });
+
+        expect(await this.accessControl.hasRole(ROLE, authorized)).to.equal(false);
+      });
+
+      it('only the sender can renounce their roles', async function () {
+        await expectRevert(
+          this.accessControl.renounceRole(ROLE, authorized, { from: admin }),
+          'AccessControl: can only renounce roles for self'
+        );
+      });
+
+      it('a role can be renounced multiple times', async function () {
+        await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+
+        const receipt = await this.accessControl.renounceRole(ROLE, authorized, { from: authorized });
+        await expectEvent.not.inTransaction(receipt.tx, AccessControlMock, 'RoleRevoked');
+      });
+    });
+  });
+
+  describe('enumerating', function () {
+    it('role bearers can be enumerated', async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: admin });
+      await this.accessControl.grantRole(ROLE, otherAuthorized, { from: admin });
+
+      const memberCount = await this.accessControl.getRoleMemberCount(ROLE);
+      expect(memberCount).to.bignumber.equal('2');
+
+      const bearers = [];
+      for (let i = 0; i < memberCount; ++i) {
+        bearers.push(await this.accessControl.getRoleMember(ROLE, i));
+      }
+
+      expect(bearers).to.have.members([authorized, otherAuthorized]);
+    });
+  });
+
+  describe('setting role admin', function () {
+    beforeEach(async function () {
+      await this.accessControl.setRoleAdmin(ROLE, OTHER_ROLE);
+      await this.accessControl.grantRole(OTHER_ROLE, otherAdmin, { from: admin });
+    });
+
+    it('a role\'s admin role can be changed', async function () {
+      expect(await this.accessControl.getRoleAdmin(ROLE)).to.equal(OTHER_ROLE);
+    });
+
+    it('the new admin can grant roles', async function () {
+      const receipt = await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
+      expectEvent(receipt, 'RoleGranted', { account: authorized, role: ROLE, sender: otherAdmin });
+    });
+
+    it('the new admin can revoke roles', async function () {
+      await this.accessControl.grantRole(ROLE, authorized, { from: otherAdmin });
+      const receipt = await this.accessControl.revokeRole(ROLE, authorized, { from: otherAdmin });
+      expectEvent(receipt, 'RoleRevoked', { account: authorized, role: ROLE, sender: otherAdmin });
+    });
+
+    it('a role\'s previous admins no longer grant roles', async function () {
+      await expectRevert(
+        this.accessControl.grantRole(ROLE, authorized, { from: admin }),
+        'AccessControl: sender must be an admin to grant'
+      );
+    });
+
+    it('a role\'s previous admins no longer revoke roles', async function () {
+      await expectRevert(
+        this.accessControl.revokeRole(ROLE, authorized, { from: admin }),
+        'AccessControl: sender must be an admin to revoke'
+      );
+    });
+  });
+});

test/access/Roles.test.js

-const { accounts, contract } = require('@openzeppelin/test-environment');
-
-const { expectRevert, constants } = require('@openzeppelin/test-helpers');
-const { ZERO_ADDRESS } = constants;
-
-const { expect } = require('chai');
-
-const RolesMock = contract.fromArtifact('RolesMock');
-
-describe('Roles', function () {
-  const [ authorized, otherAuthorized, other ] = accounts;
-
-  beforeEach(async function () {
-    this.roles = await RolesMock.new();
-  });
-
-  it('reverts when querying roles for the zero account', async function () {
-    await expectRevert(this.roles.has(ZERO_ADDRESS), 'Roles: account is the zero address');
-  });
-
-  context('initially', function () {
-    it('doesn\'t pre-assign roles', async function () {
-      expect(await this.roles.has(authorized)).to.equal(false);
-      expect(await this.roles.has(otherAuthorized)).to.equal(false);
-      expect(await this.roles.has(other)).to.equal(false);
-    });
-
-    describe('adding roles', function () {
-      it('adds roles to a single account', async function () {
-        await this.roles.add(authorized);
-        expect(await this.roles.has(authorized)).to.equal(true);
-        expect(await this.roles.has(other)).to.equal(false);
-      });
-
-      it('reverts when adding roles to an already assigned account', async function () {
-        await this.roles.add(authorized);
-        await expectRevert(this.roles.add(authorized), 'Roles: account already has role');
-      });
-
-      it('reverts when adding roles to the zero account', async function () {
-        await expectRevert(this.roles.add(ZERO_ADDRESS), 'Roles: account is the zero address');
-      });
-    });
-  });
-
-  context('with added roles', function () {
-    beforeEach(async function () {
-      await this.roles.add(authorized);
-      await this.roles.add(otherAuthorized);
-    });
-
-    describe('removing roles', function () {
-      it('removes a single role', async function () {
-        await this.roles.remove(authorized);
-        expect(await this.roles.has(authorized)).to.equal(false);
-        expect(await this.roles.has(otherAuthorized)).to.equal(true);
-      });
-
-      it('reverts when removing unassigned roles', async function () {
-        await expectRevert(this.roles.remove(other), 'Roles: account does not have role');
-      });
-
-      it('reverts when removing roles from the zero account', async function () {
-        await expectRevert(this.roles.remove(ZERO_ADDRESS), 'Roles: account is the zero address');
-      });
-    });
-  });
-});