feat: add access control (#131)

Author: superical

contracts/BaseDocumentStore.sol

@@ -17,7 +17,7 @@ contract BaseDocumentStore is Initializable {
   event DocumentIssued(bytes32 indexed document);
   event DocumentRevoked(bytes32 indexed document);
 
-  function initialize(string memory _name) internal onlyInitializing {
+  function __BaseDocumentStore_init(string memory _name) internal onlyInitializing {
     version = "2.3.0";
     name = _name;
   }

contracts/DocumentStore.sol

@@ -6,32 +6,31 @@ import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
 import "./BaseDocumentStore.sol";
+import "./base/DocumentStoreAccessControl.sol";
 
-contract DocumentStore is BaseDocumentStore, OwnableUpgradeable {
+contract DocumentStore is BaseDocumentStore, DocumentStoreAccessControl {
   constructor(string memory _name, address owner) {
     initialize(_name, owner);
   }
 
   function initialize(string memory _name, address owner) internal initializer {
-    require(owner != address(0), "Owner is required");
-    super.__Ownable_init();
-    super.transferOwnership(owner);
-    BaseDocumentStore.initialize(_name);
+    __DocumentStoreAccessControl_init(owner);
+    __BaseDocumentStore_init(_name);
   }
 
-  function issue(bytes32 document) public onlyOwner onlyNotIssued(document) {
+  function issue(bytes32 document) public onlyRole(ISSUER_ROLE) onlyNotIssued(document) {
     BaseDocumentStore._issue(document);
   }
 
-  function bulkIssue(bytes32[] memory documents) public onlyOwner {
+  function bulkIssue(bytes32[] memory documents) public onlyRole(ISSUER_ROLE) {
     BaseDocumentStore._bulkIssue(documents);
   }
 
-  function revoke(bytes32 document) public onlyOwner onlyNotRevoked(document) returns (bool) {
+  function revoke(bytes32 document) public onlyRole(REVOKER_ROLE) onlyNotRevoked(document) returns (bool) {
     return BaseDocumentStore._revoke(document);
   }
 
-  function bulkRevoke(bytes32[] memory documents) public onlyOwner {
+  function bulkRevoke(bytes32[] memory documents) public onlyRole(REVOKER_ROLE) {
     return BaseDocumentStore._bulkRevoke(documents);
   }
 }

contracts/DocumentStoreWithRevokeReasons.sol

@@ -12,7 +12,12 @@ contract DocumentStoreWithRevokeReasons is DocumentStore {
 
   constructor(string memory _name, address owner) DocumentStore(_name, owner) {}
 
-  function revoke(bytes32 document, uint256 reason) public onlyOwner onlyNotRevoked(document) returns (bool) {
+  function revoke(bytes32 document, uint256 reason)
+    public
+    onlyRole(REVOKER_ROLE)
+    onlyNotRevoked(document)
+    returns (bool)
+  {
     revoke(document);
     revokeReason[document] = reason;
     emit DocumentRevokedWithReason(document, reason);

contracts/base/DocumentStoreAccessControl.sol

@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: Apache-2.0
+
+pragma solidity ^0.8.0;
+
+import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
+
+contract DocumentStoreAccessControl is AccessControlUpgradeable {
+  bytes32 public constant ISSUER_ROLE = keccak256("ISSUER_ROLE");
+  bytes32 public constant REVOKER_ROLE = keccak256("REVOKER_ROLE");
+
+  function __DocumentStoreAccessControl_init(address owner) internal onlyInitializing {
+    require(owner != address(0), "Owner is zero");
+    _setupRole(DEFAULT_ADMIN_ROLE, owner);
+    _setupRole(ISSUER_ROLE, owner);
+    _setupRole(REVOKER_ROLE, owner);
+  }
+}

src/index.test.ts

@@ -1,4 +1,4 @@
-import { providers } from "ethers";
+import { providers, ethers } from "ethers";
 import { deploy, deployAndWait, connect } from "./index";
 import { DocumentStoreCreator__factory as DocumentStoreCreatorFactory } from "./contracts";
 
@@ -7,6 +7,10 @@ const signer = provider.getSigner();
 let account: string;
 let documentStoreCreatorAddressOverride: string;
 
+const adminRole = ethers.constants.HashZero;
+const issuerRole = ethers.utils.id("ISSUER_ROLE");
+const revokerRole = ethers.utils.id("REVOKER_ROLE");
+
 beforeAll(async () => {
   // Deploy an instance of DocumentStoreFactory on the new blockchain
   const factory = new DocumentStoreCreatorFactory(signer);
@@ -25,8 +29,14 @@ describe("deploy", () => {
 describe("deployAndWait", () => {
   it("deploys a new DocumentStore contract", async () => {
     const instance = await deployAndWait("My Store", signer, { documentStoreCreatorAddressOverride });
-    const owner = await instance.owner();
-    expect(owner).toBe(account);
+
+    const hasAdminRole = await instance.hasRole(adminRole, account);
+    const hasIssuerRole = await instance.hasRole(issuerRole, account);
+    const hasRevokerRole = await instance.hasRole(revokerRole, account);
+    expect(hasAdminRole).toBe(true);
+    expect(hasIssuerRole).toBe(true);
+    expect(hasRevokerRole).toBe(true);
+
     const name = await instance.name();
     expect(name).toBe("My Store");
   });
@@ -37,8 +47,6 @@ describe("connect", () => {
     const { address } = await deployAndWait("My Store", signer, { documentStoreCreatorAddressOverride });
     console.log(address);
     const instance = await connect(address, signer);
-    const owner = await instance.owner();
-    expect(owner).toBe(account);
     const name = await instance.name();
     expect(name).toBe("My Store");
   });

test/DocumentStore.js

@@ -1,4 +1,4 @@
-const { expect } = require("chai").use(require("chai-as-promised"));
+const { expect, assert } = require("chai").use(require("chai-as-promised"));
 const { ethers } = require("hardhat");
 const { get } = require("lodash");
 const config = require("../config.js");
@@ -8,6 +8,10 @@ describe("DocumentStore", async () => {
   let DocumentStore;
   let DocumentStoreInstance;
 
+  const adminRole = ethers.constants.HashZero;
+  const issuerRole = ethers.utils.id("ISSUER_ROLE");
+  const revokerRole = ethers.utils.id("REVOKER_ROLE");
+
   beforeEach("", async () => {
     Accounts = await ethers.getSigners();
     DocumentStore = await ethers.getContractFactory("DocumentStore");
@@ -20,10 +24,35 @@ describe("DocumentStore", async () => {
       const name = await DocumentStoreInstance.name();
       expect(name).to.be.equal(config.INSTITUTE_NAME, "Name of institute does not match");
     });
+  });
+
+  describe("Access Control", () => {
+    describe("Initialisation", () => {
+      it("should revert if owner is zero address", async () => {
+        const tx = DocumentStore.connect(Accounts[0]).deploy(config.INSTITUTE_NAME, ethers.constants.AddressZero);
+
+        await expect(tx).to.be.revertedWith("Owner is zero");
+      });
+
+      describe("Owner Default Roles", () => {
+        it("should have default admin role", async () => {
+          const hasRole = await DocumentStoreInstance.hasRole(adminRole, Accounts[0].address);
+
+          expect(hasRole).to.be.true;
+        });
 
-    it("it should have the corrent owner", async () => {
-      const owner = await DocumentStoreInstance.owner();
-      expect(owner).to.be.equal(Accounts[0].address);
+        it("should have issuer role", async () => {
+          const hasRole = await DocumentStoreInstance.hasRole(issuerRole, Accounts[0].address);
+
+          expect(hasRole).to.be.true;
+        });
+
+        it("should have revoker role", async () => {
+          const hasRole = await DocumentStoreInstance.hasRole(revokerRole, Accounts[0].address);
+
+          expect(hasRole).to.be.true;
+        });
+      });
     });
   });
 
@@ -35,8 +64,9 @@ describe("DocumentStore", async () => {
   });
 
   describe("issue", () => {
+    const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
+
     it("should be able to issue a document", async () => {
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
       const tx = await DocumentStoreInstance.issue(documentMerkleRoot);
       const receipt = await tx.wait();
 
@@ -49,7 +79,6 @@ describe("DocumentStore", async () => {
     });
 
     it("should not allow duplicate issues", async () => {
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
       await DocumentStoreInstance.issue(documentMerkleRoot);
 
       // Check that reissue is rejected
@@ -59,13 +88,25 @@ describe("DocumentStore", async () => {
       );
     });
 
-    it("only allows the owner to issue", async () => {
-      const nonOwner = Accounts[1];
-      const owner = await DocumentStoreInstance.owner();
-      expect(nonOwner).to.not.be.equal(owner);
+    it("should revert when caller has no issuer role", async () => {
+      const account = Accounts[1];
+      const hasNoIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, account.address);
+      assert.isFalse(hasNoIssuerRole, "Non-Issuer Account has issuer role");
 
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
-      await expect(DocumentStoreInstance.connect(nonOwner).issue(documentMerkleRoot)).to.be.rejectedWith(/revert/);
+      await expect(DocumentStoreInstance.connect(account).issue(documentMerkleRoot)).to.be.rejectedWith(
+        /AccessControl/
+      );
+    });
+
+    it("should issue successfully when caller has issuer role", async () => {
+      const account = Accounts[0];
+      const hasIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, account.address);
+      assert.isTrue(hasIssuerRole, "Issuer Account has issuer role");
+
+      await DocumentStoreInstance.connect(account).issue(documentMerkleRoot);
+      const issued = await DocumentStoreInstance.isIssued(documentMerkleRoot);
+
+      expect(issued).to.be.true;
     });
   });
 
@@ -116,15 +157,30 @@ describe("DocumentStore", async () => {
       );
     });
 
-    it("only allows the owner to issue", async () => {
-      const nonOwner = Accounts[1];
-      const owner = await DocumentStoreInstance.owner();
-      expect(nonOwner).to.not.be.equal(owner);
+    it("should revert when caller has no issuer role", async () => {
+      const nonIssuerAccount = Accounts[1];
+      const hasNoIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, nonIssuerAccount.address);
+      assert.isFalse(hasNoIssuerRole, "Non-Issuer Account has issuer role");
 
       const documentMerkleRoots = ["0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330"];
 
       // FIXME:
-      await expect(DocumentStoreInstance.connect(nonOwner).bulkIssue(documentMerkleRoots)).to.be.rejectedWith(/revert/);
+      await expect(DocumentStoreInstance.connect(nonIssuerAccount).bulkIssue(documentMerkleRoots)).to.be.rejectedWith(
+        /AccessControl/
+      );
+    });
+
+    it("should bulk issue successfully when caller has issuer role", async () => {
+      const documentMerkleRoots = ["0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330"];
+
+      const account = Accounts[0];
+      const hasIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, account.address);
+      assert.isTrue(hasIssuerRole, "Issuer Account has no issuer role");
+
+      await DocumentStoreInstance.connect(account).bulkIssue(documentMerkleRoots);
+      const issued = await DocumentStoreInstance.isIssued(documentMerkleRoots[0]);
+
+      expect(issued).to.be.true;
     });
   });
 
@@ -166,8 +222,9 @@ describe("DocumentStore", async () => {
   });
 
   describe("revoke", () => {
+    const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
+
     it("should allow the revocation of a valid and issued document", async () => {
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
       const documentHash = "0x10327d7f904ee3ee0e69d592937be37a33692a78550bd100d635cdea2344e6c7";
 
       await DocumentStoreInstance.issue(documentMerkleRoot);
@@ -181,7 +238,6 @@ describe("DocumentStore", async () => {
     });
 
     it("should allow the revocation of an issued root", async () => {
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
       const documentHash = documentMerkleRoot;
 
       await DocumentStoreInstance.issue(documentMerkleRoot);
@@ -195,7 +251,6 @@ describe("DocumentStore", async () => {
     });
 
     it("should not allow repeated revocation of a valid and issued document", async () => {
-      const documentMerkleRoot = "0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330";
       const documentHash = "0x10327d7f904ee3ee0e69d592937be37a33692a78550bd100d635cdea2344e6c7";
 
       await DocumentStoreInstance.issue(documentMerkleRoot);
@@ -215,6 +270,27 @@ describe("DocumentStore", async () => {
       expect(receipt.events[0].event).to.be.equal("DocumentRevoked");
       expect(receipt.events[0].args.document).to.be.equal(documentHash);
     });
+
+    it("should revert when caller has no revoker role", async () => {
+      const nonRevokerAccount = Accounts[1];
+      const hasNoRevokerRole = await DocumentStoreInstance.hasRole(revokerRole, nonRevokerAccount.address);
+      assert.isFalse(hasNoRevokerRole, "Non-Revoker Account has revoker role");
+
+      await expect(DocumentStoreInstance.connect(nonRevokerAccount).revoke(documentMerkleRoot)).to.be.rejectedWith(
+        /AccessControl/
+      );
+    });
+
+    it("should revoke successfully when caller has issuer role", async () => {
+      const account = Accounts[0];
+      const hasIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, account.address);
+      assert.isTrue(hasIssuerRole, "Revoker Account has no revoker role");
+
+      await DocumentStoreInstance.connect(account).revoke(documentMerkleRoot);
+      const issued = await DocumentStoreInstance.isRevoked(documentMerkleRoot);
+
+      expect(issued).to.be.true;
+    });
   });
 
   describe("bulkRevoke", () => {
@@ -266,16 +342,29 @@ describe("DocumentStore", async () => {
       );
     });
 
-    it("only allows the owner to revoke", async () => {
-      const nonOwner = Accounts[1];
-      const owner = await DocumentStoreInstance.owner();
-      expect(nonOwner).to.not.be.equal(owner);
+    it("should revert when caller has no revoker role", async () => {
+      const nonRevokerAccount = Accounts[1];
+      const hasNoRevokerRole = await DocumentStoreInstance.hasRole(revokerRole, nonRevokerAccount.address);
+      assert.isFalse(hasNoRevokerRole, "Non-Revoker Account has revoker role");
 
       const documentMerkleRoots = ["0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330"];
-      await expect(DocumentStoreInstance.connect(nonOwner).bulkRevoke(documentMerkleRoots)).to.be.rejectedWith(
-        /revert/
+      await expect(DocumentStoreInstance.connect(nonRevokerAccount).bulkRevoke(documentMerkleRoots)).to.be.rejectedWith(
+        /AccessControl/
       );
     });
+
+    it("should bulk revoke successfully when caller has issuer role", async () => {
+      const documentMerkleRoots = ["0x3a267813bea8120f55a7b9ca814c34dd89f237502544d7c75dfd709a659f6330"];
+
+      const account = Accounts[0];
+      const hasIssuerRole = await DocumentStoreInstance.hasRole(issuerRole, account.address);
+      assert.isTrue(hasIssuerRole, "Revoker Account has no revoker role");
+
+      await DocumentStoreInstance.connect(account).bulkRevoke(documentMerkleRoots);
+      const issued = await DocumentStoreInstance.isRevoked(documentMerkleRoots[0]);
+
+      expect(issued).to.be.true;
+    });
   });
 
   describe("isRevoked", () => {

test/DocumentStoreCreator.js

@@ -24,16 +24,17 @@ describe("DocumentStoreCreator", async () => {
       // Test for events emitted by factory
       const tx = await DocumentStoreCreatorInstance.deploy(config.INSTITUTE_NAME);
       const receipt = await tx.wait();
-      expect(receipt.events[2].args.creator).to.be.equal(
+      expect(receipt.events[3].args.creator).to.be.equal(
         Accounts[0].address,
         "Emitted contract creator does not match"
       );
       // Test correctness of deployed DocumentStore
-      const deployedDocumentStore = await DocumentStore.attach(receipt.events[2].args.instance);
+      const deployedDocumentStore = await DocumentStore.attach(receipt.events[3].args.instance);
       const name = await deployedDocumentStore.name();
       expect(name).to.be.equal(config.INSTITUTE_NAME, "Name of institute does not match");
-      const owner = await deployedDocumentStore.owner();
-      expect(owner).to.be.equal(Accounts[0].address, "Owner of deployed contract does not match creator");
+
+      const hasAdminRole = await deployedDocumentStore.hasRole(ethers.constants.HashZero, Accounts[0].address);
+      expect(hasAdminRole).to.be.true;
     });
   });
 });