diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index f1c737427..d773a24d6 100644 --- a/contracts/ownership/Ownable.sol +++ b/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; + } } diff --git a/contracts/ownership/Superuser.sol b/contracts/ownership/Superuser.sol index 196984f68..b1ed8ce58 100644 --- a/contracts/ownership/Superuser.sol +++ b/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) - onlySuperuser - public - { + function transferSuperuser(address _newSuperuser) public onlySuperuser { 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 { - require(_newOwner != address(0)); - owner = _newOwner; - emit OwnershipTransferred(owner, _newOwner); + function transferOwnership(address _newOwner) public onlyOwnerOrSuperuser { + _transferOwnership(_newOwner); } } diff --git a/test/ownership/Superuser.test.js b/test/ownership/Superuser.test.js index 4421a2cb3..3eb85f846 100644 --- a/test/ownership/Superuser.test.js +++ b/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); - address1IsSuperuser.should.be.equal(true); + const newSuperuserIsSuperuser = await this.superuser.isSuperuser(newSuperuser); + 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 }) );