feat: RBAC authentication contract and role library

[shrugs]

• Roles.sol is a library for convenience functions on the Role struct that maps addresses to whether or not they have this role
• RBAC.sol is a contract providing storage and accessors for the roles, indexed by strings (which are compacted in memory)

---

Questions

• is it worth indexing roles by an Enum instead? This would require some casting to uints
• are events something RBAC.sol should be concerned with?

[shrugs]

looks like a flaky test. is there a re-run option?

[frangio]

Thanks @shrugs!

I was thinking of having RBAC be Ownable and exposing admin functionality to the owner. I feel like it will always be needed and we should aim to reduce boilerplate. What do you think?

is it worth indexing roles by an Enum instead? This would require some casting to uints

I understand the motivation, but I think I prefer strings to uints. They both have issues though... If we go with strings we should probably recommend that users of the library define constant variables with their role names.

[frangio]

are events something RBAC.sol should be concerned with?

Do you mean events as in e.g. RoleAssigned(string, address)? Yes, absolutely. I think RBAC should be like a more general Ownable.

[shrugs]

@frangio what do you think about making RBAC ownable, but using its own access control scheme? i.e., not using Ownable, but providing a default "owner" role and making msg.sender owner by default, and providing an onlyOwner modifier and transferOwnership method? I think it'd be weird if RBAC depended on Ownership.sol.

I'll add the events 👍

[shrugs]

Although, how important is the owner public variable? If we want to match the Ownable interface, we'd need an owner() getter, which requires storing the current owner address.

We can either keep track of the owner (in which case we just inherit from Ownable and call it a day) or we can deviate from the interface and end up with something like:

// should we prefix this with Log to match current standards, or keep it as it is to match the previous interface? if we're going to deviate from the ownable interface at all, I'd prefer to update this to include Log as well.
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
function isOwner(address addr) external returns (bool);
function transferOwnership(address addr) onlyOwner public;
modifier onlyOwner();

and users can see if a specific address is owner by calling isOwner(address).

[frangio]

Hm, okay, it might be a bit weird/confusing to have two different permission mechanisms in one contract (Ownable + RBAC).

Let's not worry about matching the Ownable interface, and just have a specific role who can add and remove addresses from roles. Since we're deviating from Ownable, how about calling the role:
const ADMIN_ROLE = "admin";

I'm guessing this means making RBAC#addRole (etc.) not internal but public and only callable by an "admin". The view functions should also be public IMO. This makes it very easy to use I think.

[shrugs]

If we're going to bake in the ability for admin's to add/remove roles, there are two api surfaces; the internal, no-constraint CRUD operations and the onlyAdmin management operations.

So I've added adminAddRole and adminRemoveRole which are onlyAdmin public, which should cover most usecases.

I also rebased on master.

[chuawengkwong]

NO longer interest

[shrugs]

@frangio I've updated the event names and tested them, as well as rebasing on master to include the recent changes

[frangio]

Great! I'm ready to merge once the linter errors are fixed. 😃

[shrugs]

😅 good catch; I should set up a precommit hook instead of hoping my editor catches it.

[frangio]

Hehe. We should look into how to make that easy for other developers too. 🙂

Thanks @shrugs!

[chuawengkwong]

Xxcccc

…

On4 December 2017 23:30:47 GMT+08:00, Francisco Giordano ***@***.***> wrote: Merged #580. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #580 (comment)