diff --git a/contracts/BaseDocumentStore.sol b/contracts/BaseDocumentStore.sol index 3c35c22..b2308a8 100644 --- a/contracts/BaseDocumentStore.sol +++ b/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; } diff --git a/contracts/DocumentStore.sol b/contracts/DocumentStore.sol index ed16dec..da1aae5 100644 --- a/contracts/DocumentStore.sol +++ b/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); } } diff --git a/contracts/DocumentStoreWithRevokeReasons.sol b/contracts/DocumentStoreWithRevokeReasons.sol index 0e3dfda..7546831 100644 --- a/contracts/DocumentStoreWithRevokeReasons.sol +++ b/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); diff --git a/contracts/base/DocumentStoreAccessControl.sol b/contracts/base/DocumentStoreAccessControl.sol new file mode 100644 index 0000000..ec5de46 --- /dev/null +++ b/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); + } +} diff --git a/src/index.test.ts b/src/index.test.ts index ec565aa..0ff908f 100644 --- a/src/index.test.ts +++ b/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"); }); diff --git a/test/DocumentStore.js b/test/DocumentStore.js index f9fa103..1453216 100644 --- a/test/DocumentStore.js +++ b/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", () => { diff --git a/test/DocumentStoreCreator.js b/test/DocumentStoreCreator.js index d11ccdb..f7c2dba 100644 --- a/test/DocumentStoreCreator.js +++ b/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; }); }); });