Redesigning Access Control for the OpenZeppelin Contracts

[albertocuestacanada]

Perhaps we could not have our Roles library decide this for the users, but instead provide examples of how to use the library for a few of the most common access/revoke patterns.

This is what I wanted to explain with the code samples above. As I see it, there are three levels in designing an application that includes permissioning:

1. The base role data types and the functions that manage them.
2. The rules on how to add and remove roles and permissions, which change for each business.
3. The actual business functions, that only use RBAC to check if bearers hold specific roles.

The base rbac contract should be common to all applications, but rarely used as is. This should be provided as part of your packages.

The business rbac contract implements how the business sees roles. There are multiple variations of these, all extending from the base contract. You can provide some common implementations for reuse or as example (SuperAdminRBAC, TransferrableRBAC, etc...). However, many businesses will code their own contracts at this level.

The business functions contract implements the business, and only calls view functions from RBAC. I think that this separation between business and permissioning is important.

[frangio]

@Austin-Williams

Perhaps the ability to transfer roles should not be allowed by default. Perhaps, by default, if you want to transfer your role then the admin would need to perform the transfer.

I like this idea, I think we should seriously consider it.

I also think it could be reasonable to provide multiple options, in the same way that we currently provide Ownable but we also provide Roles. I wouldn't want there to be an explotion of alternatives though.

[albertocuestacanada]

I thought I would reframe the conversation since it has been a bit quiet lately. I’ll expand on my latest comment.

So there are three layers in coding this access control system, as I see it.

The data layer - In this layer belong questions such as what is a role, what does mean to bear a role, should we use EnumerableSet, should we use bytes32 for role ids. This layer is about data integrity, not about who can do what.

The organization layer - There can be multiple organizations and each one will implement access control in a different way. There could be a community where everyone can add members (like MintableRole), other organizations would have two tiers of admins and users, other organizations will be hierarchical. You could build Ownable in this layer. You could even think of a DAO organization where permissions are given by voting.

I don’t think we need to agree on a single organization to implement, and I don’t think that we could anyway. We would need to narrow down to a small subset of people that will use this. In my opinion we should narrow down to three or four organization types that seem to be the most common, and implement them reusing the base layer.

Questions belonging in this layer are things like which roles exist? can users renounce their roles? can roles be transferred? who can make a member bear a role?. The agreements and disagreements in these questions will lead to how many organizations should be offered.

The business layer - In this layer lie the specific applications of the access control system. An existing example is ERC20Mintable. We will have to reimplement a number of contracts already offered, which will serve as examples of implementation. The users of the package will build most of the applications at this layer.

As usual, I’ve coded an example for this. There is a base layer that uses EnumerableSet, there are three organization contracts (Community, TwoTiered, Hierarchy), two contracts adding optional organizational features (Renounceable, Transferrable), and somewhere else a reimplementation of ERC20Mintable using TwoTiered. These are just an example and not expected to replace the discussion above on the data representation and preferred organizations.

[nventuro]

Thanks everyone for your feedback and suggestions! They have been super valuable in evaluating this complex topic.

I am now leaning towards the following idea, inspired by @albertocuestacanada’s proposals, incorporating @Austin-Williams and @frangio’s comments about authorization vs identification, and @MicahZoltu and @abcoathup’s notes about enumerability and a safe API.

@jonathan’s comment about permissions vs roles is also very interesting, though I think such a system might be better codified at the user level instead of at the library level (e.g. by having action X require that the user has either role A or B,).

The proposed solution attempts to provide flexibility while keeping usage simple for simple scenarios:

• Roles cannot be tranferred
• Each role has an associated admin role, and a role can only be granted or revoked by accounts that have it’s admin role
• DEFAULT_ADMIN_ROLE is the default admin role for all roles
• A role’s admin role can be changed via an internal function
• The external API is kept minimal: contract’s that use AccessControl only add an external pure getter for each newly defined role

It does not:

• Separate grant and revoke concerns: a role’s admin can perform both tasks. I haven’t found scenarios in which this distinction is important.

For improved readability, I split the interface from the code:

• IAccessControl holds the entire external API
• AccessControl has the implementation, plus the internal functions _grantRole and _revokeRole. I removed Roles.sol here and replaced it with usage of EnumerableSet
• ERC20Flavors shows usage examples for different system configurations

The great thing about this is that the default admin role lets us greatly simplify how the library is used, opting-in to complexity as the system grows more sophisticated (see the snippets in ERC20Flavors).

It can also be easily used as a standalone contract queried by many other contracts in a system (as suggested by @albertocuestacanada) by simply inheriting from it, setting up initial roles in a constructor and exposing _setRoleAdmin externally. We might provide such a contract in the library.

I’d love to hear your thoughts on the matter! I’ll be offline on vacation for a couple weeks, but plan to wrap this up and include it on the v3.0 release by mid-March. While I’m gone, @frangio will reply to questions as my sidekick in Contracts. Thanks again to everyone for participating in the process!

[albertocuestacanada]

Great job @nventuro, I like how you have simplified a lot of my preconceptions, and using 0x0 as the default admin role id is a very elegant touch.

My only comment at this point is that renounceRole might not be something that everyone wants. I can’t see many use cases in which a user might want to renounce a role (now that we don’t have transfer), and I think this might result mostly in unwanted use that leads to request admins to grant the role back.

I’ve got some concerns about getRoleMember(), but they relate to EnumerableSet so I’ve opened a different topic for that one.

I’ll try AccessControl as the base for a number of my contracts, and revert back if I have more feedback.

Enjoy your holidays!

[n44o]

@nventuro Overall, I really like the direction this is going in.
Regarding this point:

Each role has an associated admin role , and a role can only be granted or revoked by accounts that have its admin role

The concern I have is about bytecode size. Currently, a project I'm working on (Unlock-Protocol) is making use of Roles.sol and 2 custom roles. The total bytecode size for Roles.sol + the 2 custom roles contracts accounts for close to 18% of the maximum bytecode size for a contract from what I can tell. I'm wondering about the cost of adding an admin role for each role? I suppose it comes down to how this is implemented.

[nventuro]

Thanks @albertocuestacanada! Did you get a chance to try AccessControl out over these last few weeks? I'd love to hear about your experience.

@n44o

The concern I have is about bytecode size. Currently, a project I’m working on (Unlock-Protocol) is making use of Roles.sol and 2 custom roles. The total bytecode size for Roles.sol + the 2 custom roles contracts accounts for close to 18% of the maximum bytecode size for a contract from what I can tell. I’m wondering about the cost of adding an admin role for each role? I suppose it comes down to how this is implemented.

The idea behind this new approach is to make the system more dynamic, in the sense that all derived contracts have to do is provide a new public constant variable for each role, so overhead is kept low.

I haven't yet measured bytecode size, but if that ends up becoming an issue, you could also deploy AccessControl as a standalone contract, and have other the other contracts in your system use it with external calls. This is however much more complex and harder to manage, and wouldn't necessarily be what I'd suggest to most projects.

[frangio]

I think the issue that causes bytecode bloat with the current Roles.sol is that every new role must expose a new set of functions externally and this necessarily adds a bunch of code for dispatching, etc. We should not have this same issue with the design proposed by @nventuro since there is a single external interface that is used for all roles.

I really like the proposal! It feels really clean and general.

I also like that it’s generic in the way that the bytes32 ids are generated. This will allow us to update the recommendation as the language evolves.

[ethicraul]

Adding my 2 cents, we use a similar setup than @albertocuestacanada RBAC, we have a dumb "EternalStorage" contract that uses keccak256 hashes as keys in mappings, with limited write permissions to known contracts.

One of them is UserManager, that defines our business roles (lender, borrower, etc) and the rest of the contracts ask it if msg.sender has X role to do stuff.

@albertocuestacanada

In another example, DeFi applications that aim to operate within the real economy need to comply with KYC and AML laws. Under those rules only properly identified individuals can use the offered financial services, and allowing transfer of permissions to unidentified accounts would certainly be forbidden. My best assumption is that DeFi applications would prefer to have a process to add additional accounts to an individual ( grant ) than to allow transfer .

Yes, but there is use cases for transferIMO.

A transfer use case could be account recovery (an approver has to validate new account, then transfer privileges) or migration (user signs with old address a transfer request, new address signs and transfer is executed)

[albertocuestacanada]

Hi @nventuro, I just tested the functionality in AccessControl.sol and it is as much as I’ve ever needed in creating access control structures.

The only thing I would suggest is that given that renounceRole is a refinement of revokeRole, and that some people might not need it or need it with a different behaviour, I would put that on a contract to be used as a module (check Renounceable.sol and Transferrable.sol in my repo).

Other than that, I think this is ready to get released into the wild. Great job

[nventuro]

Thanks for the evaluation @albertocuestacanada! I'm working on an initial PR so we can hopefully release this with v3.0 of OpenZeppelin Contracts soon.

@albertocuestacanada

The only thing I would suggest is that given that renounceRole is a refinement of revokeRole , and that some people might not need it or need it with a different behaviour

We think renounce is important for security reasons: if the keys for an account you control are compromised (say, if your phone or laptop is stolen), being able to drop roles associated to said account on your own is a very useful thing to have.