Adding new Superuser contract with test #952

pmosse · 2018-05-21T22:09:15Z

Fixes #50

🚀 Description

I am adding a new Superuser contract. This allows a contract to have a user in the superuser role. A superuser can set a new owner for the contract, in case that the original owner address becomes compromised. The creator of the contract is the first superuser and he can then transfer his role to a different user.

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

shrugs · 2018-05-22T00:06:18Z

contracts/ownership/Superuser.sol

+ * @dev A superuser can transfer his role to a new address. 
+ */
+contract Superuser is Ownable, RBAC {
+  event SuperuserTransferred(address indexed previousSuperuser, address indexed newSuperuser);


These events aren't necessary. RBAC emits RoleChanged events, so SuperuserTransferred isn't needed, and Ownable already emits OwnershipTransferred.

About OwnershipTransferred: I am overriding transferOwnership and therefore it will not be emitted by Ownable.

I need to override it because in Ownable that method is onlyOwner and I need it to be onlySuperuser (actually it should be both).

I think I could do "emit super.OwnershipTransferred..." though.

the events are inherited as well, so you don't have to declare it again. just emit OwnershipTransferred(...)

If you call super.transferOwnership(...) the contract would emit the event correctly regardless

@shrugs I removed the event declaration. As we discussed in Slack, I can't call super.transferOwnership because of the onlyOwner modifier.

shrugs · 2018-05-22T00:07:31Z

contracts/ownership/Superuser.sol

+
+  string public constant ROLE_SUPERUSER = "superuser";
+
+  constructor () {


constructor should have the public modifier

shrugs · 2018-05-22T00:07:42Z

contracts/ownership/Superuser.sol

+    _;
+  }
+
+    /**


nit: formatting

shrugs · 2018-05-22T00:08:04Z

contracts/ownership/Superuser.sol

+    /**
+   * @dev getter to determine if address has superuser role
+   */
+  function superuser(address addr)


let's change this to isSuperuser to be more explicit.

shrugs · 2018-05-22T00:08:49Z

contracts/ownership/Superuser.sol

+    public
+  {
+    require(newSuperuser != address(0));
+    emit SuperuserTransferred(msg.sender, newSuperuser);


events not needed here, but also, generally you'll emit events after something has occurred (even though this whole process is atomic, it's good practice)

Perfect! Actually I agree with that practice but I wanted to follow the pattern in Ownable.sol 😋

shrugs · 2018-05-22T00:09:33Z

contracts/ownership/Superuser.sol

+   * @dev Allows the current superuser to transfer his role to a newSuperuser.
+   * @param newSuperuser The address to transfer ownership to.
+   */
+  function transferSuperuser(address newSuperuser) 


The convention is to prefix arguments with _ to keep them visually separate from variables in the function body. make this _newSuperuser?

likewise for the various other arguments in this file

Perfect! Same here, trying to follow the style of Ownable.sol and Whitelist.sol :)

shrugs · 2018-05-22T00:09:58Z

contracts/ownership/Superuser.sol

+   * @dev Allows the current superuser to transfer control of the contract to a newOwner.
+   * @param newOwner The address to transfer ownership to.
+   */
+  function transferOwnership(address newOwner) public onlySuperuser {


you can just call super.transferOwnership() here instead of re-implementing the function.

See my comments at the top of the page about this. I need to override the function to make it onlySuperuser instead of onlyOwner (actually I think it should be both). Let me know your thoughts about this.

shrugs · 2018-05-24T22:25:32Z

contracts/ownership/Superuser.sol

+contract Superuser is Ownable, RBAC {
+  string public constant ROLE_SUPERUSER = "superuser";
+
+  constructor ()public {


space here

make sure to run npm run lint:sol to catch these

@shrugs Fixed. For some reason the linter is not catching it.

shrugs · 2018-05-28T01:25:39Z

waiting on a second review before merging 👍

pmosse · 2018-06-02T18:53:44Z

@shrugs Hey! Any updates on this? :)

shrugs · 2018-06-02T22:28:06Z

Thanks for the reminder, @pmosse, I'll grab a second review by monday :)

AugustoL

LGTM

frangio · 2018-06-04T18:36:39Z

Sorry it took me a while to get to this.

Re: transferOwnership

I need to override the function to make it onlySuperuser instead of onlyOwner (actually I think it should be both). Let me know your thoughts about this.

@pmosse I see that the contract was merged with transferOwnership as onlySuperuser. Shouldn't this be both the superuser and the owner?

shrugs · 2018-06-04T19:35:41Z

@frangio I could see it both ways

With onlySuperuser, this works when the owner is also the superuser, and when the superuser changes the owner shouldn't be authorized to change themselves (it's the weakly-stored private key).

pmosse · 2018-06-04T20:43:40Z

@shrugs Well, with the code as it is now, and assuming that the normal behaviour is to transfer the superuser role to another account as soon as the contract is deployed, we are not allowing the owner to transfer his ownership if he needs to do that. With the change proposed by @frangio, the owner will be able to transfer his ownership at any time if needed, and if the current owner's account gets compromised, the superuser can always regain control by assigning a new owner. Does that make sense?

shrugs · 2018-06-04T20:58:14Z

Yeah exactly; I could see the argument for either way; neither of them are specifically good or bad; we just have to pick the "intended behavior" and stick with it.

frangio · 2018-06-04T23:08:26Z

Personally I'm for making it only-owner-or-superuser, because I see this as Ownable + an escape mechanism via a cold-stored key.

With onlySuperuser this is more like an "owner" role that is managed by a "superuser" role.

shrugs · 2018-06-04T23:29:09Z

I support that. @pmosse ?

pmosse · 2018-06-04T23:37:21Z

Agreed! I will start working in a PR to update this. Thanks! @shrugs @frangio

pmosse · 2018-06-05T00:47:53Z

Created a new PR in #978 @shrugs @frangio

pmosse added 2 commits May 21, 2018 18:56

Adding new Superuser contract with test

2092ec1

Adding new Superuser contract + test OpenZeppelin#50

e8f9092

shrugs suggested changes May 22, 2018

View reviewed changes

pmosse added 3 commits May 22, 2018 10:03

Fixes after code review feedback

c59396b

Fixes after code review feedback

ae03a00

Removing event declaration as it is in Ownable OpenZeppelin#50

6186d97

shrugs reviewed May 24, 2018

View reviewed changes

Fixing missing space

a6cefef

shrugs requested a review from frangio May 27, 2018 20:53

shrugs approved these changes May 27, 2018

View reviewed changes

Merge branch 'master' into feature/superuser-contract-#50

75ae921

Merge branch 'master' into feature/superuser-contract-#50

98053b6

AugustoL approved these changes Jun 3, 2018

View reviewed changes

shrugs merged commit a0c03ee into OpenZeppelin:master Jun 3, 2018

pmosse deleted the feature/superuser-contract-#50 branch June 5, 2018 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new Superuser contract with test #952

Adding new Superuser contract with test #952

pmosse commented May 21, 2018

shrugs May 22, 2018

pmosse May 22, 2018

shrugs May 23, 2018

pmosse May 24, 2018

shrugs May 22, 2018

shrugs May 22, 2018

shrugs May 22, 2018

shrugs May 22, 2018

pmosse May 22, 2018

shrugs May 22, 2018

pmosse May 22, 2018

shrugs May 22, 2018

pmosse May 22, 2018

shrugs May 24, 2018

pmosse May 25, 2018

shrugs commented May 28, 2018

pmosse commented Jun 2, 2018

shrugs commented Jun 2, 2018

AugustoL left a comment

frangio commented Jun 4, 2018

shrugs commented Jun 4, 2018

pmosse commented Jun 4, 2018

shrugs commented Jun 4, 2018

frangio commented Jun 4, 2018

shrugs commented Jun 4, 2018

pmosse commented Jun 4, 2018

pmosse commented Jun 5, 2018


		string public constant ROLE_SUPERUSER = "superuser";

		constructor () {

+                  _;
+                }
+                  /**

Adding new Superuser contract with test #952

Adding new Superuser contract with test #952

Conversation

pmosse commented May 21, 2018

🚀 Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs commented May 28, 2018

pmosse commented Jun 2, 2018

shrugs commented Jun 2, 2018

AugustoL left a comment

Choose a reason for hiding this comment

frangio commented Jun 4, 2018

shrugs commented Jun 4, 2018

pmosse commented Jun 4, 2018

shrugs commented Jun 4, 2018

frangio commented Jun 4, 2018

shrugs commented Jun 4, 2018

pmosse commented Jun 4, 2018

pmosse commented Jun 5, 2018