Redesigning Access Control for the OpenZeppelin Contracts

Hi all, chipping in here with some additions for the Roles library that may have affect Access Control design decisions. It has happened to me, and I foresee many others, would want to know how many accounts have a particular Role at a given moment. This could be accomplished with a public counter inside the _add and _remove functions of the library.

Moreover, with the advent of increased transparency and the Ethereum Name Service (i.e. clean account names instead of hexadecimal), applications may want to display all or some of the current bearers of a particular Role. An address[] public would do this. However, as a role grows in popularity it can get expensive to store all its addresses on-chain.

Thoughts?

2 Likes

I don’t think our roles solution should necessarily be built to satisfy those use cases, but this raises a very interesting point. I think so far we’ve been thinking about access control in a way that only includes authorization and not identification, so situations where it only matters that someone can do something and not who they are.

In that sense, voting and oracles are not purely about access control, because the notion of identity becomes relevant given that you need to differentiate who is voting or reporting a price.

Our reasoning that “everyone can grant” is a natural consequence of “everyone can transfer” only made sense in the context of pure authorization. However, since users of roles will have access to msg.sender anyway, we may want to design with identification in mind too. In that case, “everyone can grant” feels wrong.

On the other hand, if identification is important then transfer is also a potential issue. Voters or oracles could also transfer their role to a different account and that would likely result in a sybil attack as well. It’s only possible to prevent this if we use for identification an identifier other than the account address, that is preserved on a role transfer. Users still have ready access to msg.sender anyway so it is likely that they will use all of this incorrectly.

I think we have to reconsider the design with all of this in mind, but my conclusion is that roles should not be used for situations like voters or oracles in the way @Austin-Williams described. They’re different concepts that should be implemented on their own.

5 Likes

Not to confuse the conversation here, but for consideration:

Many applications separate the concepts of permissions and roles. This has the advantage to potentially simplify code maintenance while giving flexibility and improving ease-of-use for administrators.

Functions in this case check for permissions (essentially hard-coded), while roles are an arbitrary collection of permissions created and labeled by administrators. Then administrators assign roles to accounts (thereby assigning a collection of permissions to each account).

For example in a health care application, there might be separate functions with permissions to: (1) submit test results, (2) prescribe category 1 medicines, (3) prescribe category 2 medicines, (4) request an MRI, (5) approve an MRI, and (6) approve surgery.

In one health care facility they might have the following roles:
nurse: can do (1)(2)(4)
doctor: can do (1)(2)(3)(4)(5)

But in another health care facility they might have different roles:
junior nurse: can do (1)(2)(4)
senior nurse: can do (1)(2)(3)(4)
doctor: can do (1)(2)(3)(4)(5)

And in another they have even different roles and labels such as:
attending: can do (1)(2)(4)
resident: can do (1)(2)(3)(4)
surgeon: can do (1)(2)(3)(4)(5)

Another use-case mentioned in the comments is voting. Is an admin account allowed also to vote? Must an admin have a separate account for voting? In the permissions and roles model, there might be one permissions for “vote administration” and another for “voting”, then I can have a role for “admin” which has both permissions, and another role for “voter” which only has the voting permission.

Separating permissions and roles provides this flexibility. The implementation would need to include ability to create and define roles. Checking for a permission on an account would involve a lookup of the role for the account and then a lookup whether the role contains that permission.

In applications that have very limited number of roles and permissions (for example just super-admin, admin and general-user) then this flexibility and extra layer or implementation is not required. But for applications that will have increased functionality (more permissions) and more roles, especially those that might extend use to multiple groups that have different definition of roles (like example above), then this flexibility and extra layer becomes critical.

5 Likes

I think that is a great insight, @jonathan.

Following that model is easy to code a base RBAC contract that just records the Roles, and then segregate in different contracts the RBAC rules for a particular business and the Permissions for business functions.

Sample code

BaseRBAC:

pragma solidity ^0.5.10;

import "../GSN/Context.sol";


/**
 * @title BaseRoles
 * @author Alberto Cuesta Canada
 * @notice Implements runtime configurable Role Based Access Control.
 */
contract BaseRoles is Context {
    event RoleAdded(bytes32 roleId);
    event RoleRemoved(bytes32 roleId);
    event BearerAdded(address bearer, bytes32 roleId);
    event BearerRemoved(address bearer, bytes32 roleId);

    /**
     * @notice A role, which will be used to group users.
     * @dev The role id is its position in the roles array.
     * @param admin The only role that can add or remove bearers from this role. To have the role
     * bearers to be also the role admins you should pass roles.length as the admin role.
     * @param bearers Addresses belonging to this role.
     */
    struct Role {
        bool exists;
        mapping (address => bool) bearers;
    }

    mapping (bytes32 => Role) internal roles;

    /**
     * @notice A method to verify if a role exists.
     * @param roleId The id of the role being verified.
     * @return True or false.
     */
    function roleExists(bytes32 roleId)
        public
        view
        returns(bool)
    {
        return (roles[roleId].exists);
    }

    /**
     * @notice A method to verify whether an bearer is a bearer of a role
     * @param bearer The bearer to verify.
     * @param roleId The role to look into.
     * @return Whether the bearer is a bearer of the role.
     */
    function hasRole(address bearer, bytes32 roleId)
        public
        view
        returns(bool)
    {
        require(roleExists(roleId), "Role doesn't exist.");
        return roles[roleId].bearers[bearer];
    }

    /**
     * @notice A method to verify whether msg.sender is a bearer of a role
     * @param roleId The role to look into.
     * @return Whether the bearer is a bearer of the role.
     */
    function hasRole(bytes32 roleId)
        public
        view
        returns(bool)
    {
        return hasRole(_msgSender(), roleId);
    }


    /**
     * @notice A method to create a new role.
     * @param roleId The id for role that is being created
     * the role being created.
     */
    function _addRole(bytes32 roleId)
        internal
    {
        require(!roleExists(roleId), "Role already exists.");

        roles[roleId] = Role({ exists: true });
        emit RoleAdded(roleId);
    }

    /**
     * @notice A method to create a new role.
     * @param roleId The id for role that is being created
     * the role being created.
     */
    function _removeRole(bytes32 roleId)
        internal
    {
        require(roleExists(roleId), "Role doesn't exist.");

        delete roles[roleId];
        emit RoleRemoved(roleId);
    }


    /**
     * @notice A method to add a bearer to a role
     * @param bearer The address to add as a bearer.
     * @param roleId The role to add the bearer to.
     */
    function _addBearer(address bearer, bytes32 roleId)
        internal
    {
        require(
            !hasRole(bearer, roleId),
            "Address is bearer of role."
        );

        roles[roleId].bearers[bearer] = true;
        emit BearerAdded(bearer, roleId);
    }

    /**
     * @notice A method to remove a bearer from a role
     * @param bearer The address to remove as a bearer.
     * @param roleId The role to remove the bearer from.
     */
    function _removeBearer(address bearer, bytes32 roleId)
        internal
    {
        require(
            hasRole(bearer, roleId),
            "Address is not bearer of role."
        );

        delete roles[roleId].bearers[bearer];
        emit BearerRemoved(bearer, roleId);
    }
}

Then the specific business decides the RBAC rules, whether then need transfer, grant, superadmins, even dynamic roles can be created. For your scenarios something like this would do.

HealthRBAC:

pragma solidity ^0.5.10;

import "./BaseRoles.sol";


/**
 * @title HealthRBAC
 * @author Alberto Cuesta Canada
 * @notice Implements RBAC rules for an example Health business.
 */
contract HealthRBAC is BaseRoles {
    bytes32 constant ADMIN = "ADMIN";
    bytes32 constant NURSE = "NURSE";
    bytes32 constant DOCTOR = "DOCTOR";

    // Define initial roles and permissions
    constructor(address admin) public {
        _addRole(ADMIN);
        _addRole(NURSE);
        _addRole(DOCTOR);
        _addBearer(admin, ADMIN);
    }

    // Define RBAC rules
    function addBearer(address bearer, bytes32 roleId)
        public
    {
        require(hasRole(ADMIN), "Unauthorized");
        _addBearer(bearer, roleId);
    }

    function removeBearer(address bearer, bytes32 roleId)
        public
    {
        require(hasRole(ADMIN), "Unauthorized");
        _removeBearer(bearer, roleId);
    }
}

And then finally on that base we can code the actual business rules.

HealthBusiness:

pragma solidity ^0.5.10;

import "./HealthRBAC.sol";


/**
 * @title HealthBusiness
 * @author Alberto Cuesta Canada
 * @notice Implements a business using HealthRBAC.
 */
contract HealthBusiness is HealthRBAC {
    constructor(address admin) public HealthRBAC(admin) {}

    // Define business
    function submitTestResults()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function prescribeCat1()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function prescribeCat2()
        public
    {
        require(hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function requestMRI()
        public
    {
        require(hasRole(NURSE) || hasRole(DOCTOR), "Unauthorized");
        // ...
    }

    function approveSurgery()
        public
    {
        require(hasRole(DOCTOR), "Unauthorized");
        // ...
    }
}

This pattern is very flexible, and I don’t think it is too complicated to use successfully.

1 Like

Let’s focus the discussion on the design constraints for now. We can worry about the implementation details later. I think it’s also valuable to discuss potential APIs but with some minimal examples to keep things focused.

1 Like

That’s a good point.
To expand on this:

The ability to transfer your role implies the ability to trustlessly sell your role (or trustlessly rent out your role’s privileges).

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.

@nventuro, I think I’m struggling to reason about Roles without context.

Can you give an example of when someone might want to use our Roles library? What kinds of use cases are we designing for?

I’m thinking back through recent audits and struggling to think of any examples where there is more than one role (other than the use of Ownable, where there is only a single _owner).

1 Like

These are roles we’ve had in Contracts:

  • minter can arbitrarily mint new tokens
  • pauser can pause and unpause a contract (which functions are paused is up to the user)
  • a capper was able set a cap on how many tokens an account could purchase in a crowdsale
  • similarly, a whitelistee was able to participate in a crowdsale, and whitelisters granted the whitelistee role (this a departure from the previous examples, and is sort of tied to identity)

As you can see, most are associated with being able to perform administrative tasks. We decided to have multiple roles instead of a single owner to allow for more granular designs, you may for example only let a multisig pause the system, while a different contract might have minting permissions. There’s some relevant discussion here.

I was originally designing with use cases similar to these in mind (ability to pause, upgrade, selfdestruct, change system settings, etc.), and had not really considered voting or oracles. Like @frangio, my intuition is that these two contexts might be different enough that a single design won’t be able to cover both, but I’m not entirely sure we can’t make it work yet.

2 Likes

Thank you. Those examples help.

Additionally, we will also handle role management: how they are are granted and revoked.

Perhaps this goal goes to far.

It seems, from our discussion above, that granting and revoking conditions are heavily situation-dependent. If our Library is too opinionated about how this is done, then it may not be flexible enough to accomplish the first goal:

appropriate for the security needs of most if not all projects.

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.

2 Likes

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.

3 Likes

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.

1 Like

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.

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

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!

1 Like

@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.

1 Like

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

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.

2 Likes

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.

1 Like

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.

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)

1 Like

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 :+1:

1 Like

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.

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.

1 Like