Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RBAC and Ownable migration towards Roles #1291

Merged
merged 16 commits into from
Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .soliumrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"rules": {
"error-reason": "off",
"indentation": ["error", 2],
"lbrace": "off",
"linebreak-style": ["error", "unix"],
"max-len": ["error", 79],
"no-constant": ["error"],
Expand Down
3 changes: 1 addition & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ contract MyContract is Ownable {
## Architecture
The following provides visibility into how OpenZeppelin's contracts are organized:

- **access** - Smart contracts that enable functionality that can be used for selective restrictions and basic authorization control functions. Includes address whitelisting and signature-based permissions management.
- **rbac** - A library used to manage addresses assigned to different user roles and an example Role-Based Access Control (RBAC) interface that demonstrates how to handle setters and getters for roles and addresses.
- **access** - Smart contracts that enable functionality that can be used for selective restrictions and basic authorization control functions.
- **crowdsale** - A collection of smart contracts used to manage token crowdsales that allow investors to purchase tokens with ETH. Includes a base contract which implements fundamental crowdsale functionality in its simplest form. The base contract can be extended in order to satisfy your crowdsale’s specific requirements.
- **distribution** - Includes extensions of the base crowdsale contract which can be used to customize the completion of a crowdsale.
- **emission** - Includes extensions of the base crowdsale contract which can be used to mint and manage how tokens are issued to purchasers.
Expand Down
21 changes: 2 additions & 19 deletions contracts/access/rbac/Roles.sol → contracts/access/Roles.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ pragma solidity ^0.4.24;

/**
* @title Roles
* @author Francisco Giordano (@frangio)
* @dev Library for managing addresses assigned to a Role.
* See RBAC.sol for example usage.
*/
library Roles {
struct Role {
Expand All @@ -15,32 +13,17 @@ library Roles {
/**
* @dev give an account access to this role
*/
function add(Role storage _role, address _account)
internal
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at you Venturo! 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean D:

function add(Role storage _role, address _account) internal {
_role.bearer[_account] = true;
}

/**
* @dev remove an account's access to this role
*/
function remove(Role storage _role, address _account)
internal
{
function remove(Role storage _role, address _account) internal {
_role.bearer[_account] = false;
}

/**
* @dev check if an account has this role
* // reverts
*/
function check(Role storage _role, address _account)
nventuro marked this conversation as resolved.
Show resolved Hide resolved
internal
view
{
require(has(_role, _account));
}

/**
* @dev check if an account has this role
* @return bool
Expand Down
94 changes: 0 additions & 94 deletions contracts/access/Whitelist.sol

This file was deleted.

106 changes: 0 additions & 106 deletions contracts/access/rbac/RBAC.sol

This file was deleted.

35 changes: 35 additions & 0 deletions contracts/access/roles/CapperRole.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity ^0.4.24;

import "../Roles.sol";


contract CapperRole {
using Roles for Roles.Role;

Roles.Role private cappers;

constructor() public {
cappers.add(msg.sender);
}

modifier onlyCapper() {
require(isCapper(msg.sender));
_;
}

function isCapper(address _account) public view returns (bool) {
return cappers.has(_account);
nventuro marked this conversation as resolved.
Show resolved Hide resolved
}

function addCapper(address _account) public onlyCapper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it ok that a capper can remove other cappers? Shouldn't there be instead an admin that adds and removes cappers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point; a lot of the api surface for these runs into the same issues with the problem with RBAC and it's that you can't define a public api surface by default because then changing it is super hard. and you don't know what sort of process a project might want to be able to use to be able to change the cap, for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our rationale was that someone who has a role is already able to perform actions using that role on behalf of other people. In fact, with ownership, it was trivial to transfer ownership to a forwarder contract that would let any set of accounts use the ownership. But this could also be done informally, as an off-chain favor.

Since this was already possible, we decided to make it part of the API. Otherwise it just feels like obscuring what is really possible.

cappers.add(_account);
}

function renounceCapper() public {
cappers.remove(msg.sender);
}

function _removeCapper(address _account) internal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it weird that addCapper is not consistent with removeCapper. A good design principle is that everything you can do you, you will be able to undo. I guess you had a good reason to go this way, so again, just thinking out loud. We can do any changes later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed this extensively with @frangio. Our reasoning goes as follows:

An account with a role should always be able to transfer it. If the account is a contract, then the conditions under which a transfer will be executed will be clear from the code. If it's an EOA, then the user already has the liberty to do whatever she wants with it, from complying to transaction requests to giving up the private keys to someone else, effectively transferring control.

So, transferring can always be done. This includes transferring the role to a contract that will forward all calls from a whitelisted set of addresses, which effectively is an add to multiple addresses. We decided to formalize this mechanism, and simply remove transfer and make add public by default. Note that remove plays no part here: at no point does having a role give you the power to take it away from someone else, but it does let you act at someone else's behest.

TL;DR: EOAs shouldn't be trusted, since people can do anything they want, if you really need control over this kind of stuff, the role-havers should also be contracts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want a notion of a public remove function but we will wait and see what use-cases come up to add later. :)

cappers.remove(_account);
}
}
35 changes: 35 additions & 0 deletions contracts/access/roles/MinterRole.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity ^0.4.24;

import "../Roles.sol";


contract MinterRole {
using Roles for Roles.Role;

Roles.Role private minters;

constructor() public {
minters.add(msg.sender);
}

modifier onlyMinter() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we assign nice names to the functions. I don't like so much that the code for Minter is the same as the code for Capper. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably should've gone into some more detail in the PR description: the role contracts (everything under access/roles/, and associated tests) will be auto-generated in the future (see #1290), since they are all identical (we call it the 'role boilerplate'). This is sort of a limitation of how libraries work in Solidty, but I'm not yet convinced what'd be the best way to tackle this moving forward. For now, code repetition is what we have.

require(isMinter(msg.sender));
_;
}

function isMinter(address _account) public view returns (bool) {
return minters.has(_account);
}

function addMinter(address _account) public onlyMinter {
minters.add(_account);
}

function renounceMinter() public {
minters.remove(msg.sender);
}

function _removeMinter(address _account) internal {
minters.remove(_account);
}
}
35 changes: 35 additions & 0 deletions contracts/access/roles/PauserRole.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity ^0.4.24;

import "../Roles.sol";


contract PauserRole {
using Roles for Roles.Role;

Roles.Role private pausers;

constructor() public {
pausers.add(msg.sender);
}

modifier onlyPauser() {
require(isPauser(msg.sender));
_;
}

function isPauser(address _account) public view returns (bool) {
return pausers.has(_account);
}

function addPauser(address _account) public onlyPauser {
pausers.add(_account);
}

function renouncePauser() public {
pausers.remove(msg.sender);
}

function _removePauser(address _account) internal {
pausers.remove(_account);
}
}
Loading