Redesigning Access Control for the OpenZeppelin Contracts

General
24

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!

2 Likes