Refactoring Superuser contract to allow Owners to transfer ownership … (#978)

* Refactoring Superuser contract to allow Owners to transfer ownership when they are not superusers #50

* Refactoring tests to create a contract instance for each of them #50

contracts/ownership/Ownable.sol

@@ -33,16 +33,6 @@ contract Ownable {
     _;
   }
 
-  /**
-   * @dev Allows the current owner to transfer control of the contract to a newOwner.
-   * @param newOwner The address to transfer ownership to.
-   */
-  function transferOwnership(address newOwner) public onlyOwner {
-    require(newOwner != address(0));
-    emit OwnershipTransferred(owner, newOwner);
-    owner = newOwner;
-  }
-
   /**
    * @dev Allows the current owner to relinquish control of the contract.
    */
@@ -50,4 +40,22 @@ contract Ownable {
     emit OwnershipRenounced(owner);
     owner = address(0);
   }
+
+  /**
+   * @dev Allows the current owner to transfer control of the contract to a newOwner.
+   * @param _newOwner The address to transfer ownership to.
+   */
+  function transferOwnership(address _newOwner) public onlyOwner {
+    _transferOwnership(_newOwner);
+  }
+
+  /**
+   * @dev Transfers control of the contract to a newOwner.
+   * @param _newOwner The address to transfer ownership to.
+   */
+  function _transferOwnership(address _newOwner) internal {
+    require(_newOwner != address(0));
+    emit OwnershipTransferred(owner, _newOwner);
+    owner = _newOwner;
+  }
 }

contracts/ownership/Superuser.sol

@@ -26,6 +26,11 @@ contract Superuser is Ownable, RBAC {
     _;
   }
 
+  modifier onlyOwnerOrSuperuser() {
+    require(msg.sender == owner || isSuperuser(msg.sender));
+    _;
+  }
+
   /**
    * @dev getter to determine if address has superuser role
    */
@@ -41,22 +46,17 @@ contract Superuser is Ownable, RBAC {
    * @dev Allows the current superuser to transfer his role to a newSuperuser.
    * @param _newSuperuser The address to transfer ownership to.
    */
-  function transferSuperuser(address _newSuperuser) 
+  function transferSuperuser(address _newSuperuser) public onlySuperuser {
-    onlySuperuser
-    public
-  {
     require(_newSuperuser != address(0));
     removeRole(msg.sender, ROLE_SUPERUSER);
     addRole(_newSuperuser, ROLE_SUPERUSER);
   }
 
   /**
-   * @dev Allows the current superuser to transfer control of the contract to a newOwner.
+   * @dev Allows the current superuser or owner to transfer control of the contract to a newOwner.
    * @param _newOwner The address to transfer ownership to.
    */
-  function transferOwnership(address _newOwner) public onlySuperuser {
+  function transferOwnership(address _newOwner) public onlyOwnerOrSuperuser {
-    require(_newOwner != address(0));
+    _transferOwnership(_newOwner);
-    owner = _newOwner;
-    emit OwnershipTransferred(owner, _newOwner);
   }
 }

test/ownership/Superuser.test.js

@@ -15,7 +15,7 @@ contract('Superuser', function (accounts) {
     anyone,
   ] = accounts;
 
-  before(async function () {
+  beforeEach(async function () {
     this.superuser = await Superuser.new();
   });
 
@@ -31,11 +31,13 @@ contract('Superuser', function (accounts) {
       const ownerIsSuperuser = await this.superuser.isSuperuser(firstOwner);
       ownerIsSuperuser.should.be.equal(false);
 
-      const address1IsSuperuser = await this.superuser.isSuperuser(newSuperuser);
+      const newSuperuserIsSuperuser = await this.superuser.isSuperuser(newSuperuser);
-      address1IsSuperuser.should.be.equal(true);
+      newSuperuserIsSuperuser.should.be.equal(true);
     });
 
-    it('should change owner after transferring', async function () {
+    it('should change owner after the superuser transfers the ownership', async function () {
+      await this.superuser.transferSuperuser(newSuperuser, { from: firstOwner });
+
       await expectEvent.inTransaction(
         this.superuser.transferOwnership(newOwner, { from: newSuperuser }),
         'OwnershipTransferred'
@@ -44,6 +46,16 @@ contract('Superuser', function (accounts) {
       const currentOwner = await this.superuser.owner();
       currentOwner.should.be.equal(newOwner);
     });
+
+    it('should change owner after the owner transfers the ownership', async function () {
+      await expectEvent.inTransaction(
+        this.superuser.transferOwnership(newOwner, { from: firstOwner }),
+        'OwnershipTransferred'
+      );
+
+      const currentOwner = await this.superuser.owner();
+      currentOwner.should.be.equal(newOwner);
+    });
   });
 
   context('in adversarial conditions', () => {
@@ -53,7 +65,7 @@ contract('Superuser', function (accounts) {
       );
     });
 
-    it('should prevent non-superusers from setting a new owner', async function () {
+    it('should prevent users that are not superuser nor owner from setting a new owner', async function () {
       await expectThrow(
         this.superuser.transferOwnership(newOwner, { from: anyone })
       );