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

Simplify permission parent model to a permission owner model #155

Closed
izqui opened this issue Nov 24, 2017 · 4 comments
Closed

Simplify permission parent model to a permission owner model #155

izqui opened this issue Nov 24, 2017 · 4 comments

Comments

@izqui
Copy link
Contributor

izqui commented Nov 24, 2017

In the current iteration of the Kernel ACL, every permission instance has its own 'parent'. A more detailed explanation can be found in the ACL section of the AragonOS doc. In this model the parent of a permission instance means two different things:

  • Parent can always revoke a permission instance.
  • If an entity has a permission and it is also its own parent, it can regrant the permission.

The problem with this permission model is that in case a permission has been granted to an entity that cannot do arbitrary calls (i.e. Finance or Fundraising), the process for granting the permission to another entity (i.e. wanting to install another app that needs vault access) involves revoking the permission in a complex workflow like this:

- Finance app has permission X, with parent Y.
- Y revokes Finance app permission X.
- An entity allowed to do `kernel.createPermission` creates the permission X to a temporary entity Z, with parent Z.
- Z grants the new entity the permission.
- Z grants Finance app permission X, with parent Y (reset initial state)
- Z revokes its own permission X.

Problems:

  • If parent Y doesn't have permission to do kernel.createPermission, two different entities need to coordinate their actions. The permission cannot be created again while the Finance app has it.
  • If the permission has been granted to more entities (and these entities don't have the ability to re-grant), all these entities need to be revoked the permission. If they need to be revoked by different entities, the coordination problem becomes even worse.
  • Z needs to be trusted to perform all three actions (easily solvable making Z a contract that executes a EVMCallScript)

Proposed new model

Introduce a permission owner at the permission level and remove the permission parent from the permission instance level. The owner is able to grant or revoke an arbitrary number of permission instances of that permission.

  • createPermission(address entity, address app, bytes32 role, address newOwner): protected behind the ACL. Creates a new permission setting an owner. Permission is granted to entity. If permission has already been created, it fails.
  • grantPermission(address entity, address app, bytes32 role): Grants a permission instance for entity. Only callable by permission owner.
  • revokePermission(address entity, address app, bytes32 role): Revokes a permission instance for entity. Only callable by permission owner.
  • transferOwnership(address app, bytes32 role, address newOwner): only callable by permission owner, new owner will be able to revoke all previous permission instances too (impossible under current system).

This model still allows every permission to have a different owner, which is a very important security feature since some permissions need to be more protected than others.

The only disadvantage compared to the current model is that it won't be possible to have two different permission instances anymore (i.e. token transfers can be done by entity A and B) that could be revoked by different entities (entity A can be revoked its permission by C and B by D).

@sohkai
Copy link
Contributor

sohkai commented Nov 28, 2017

I like the new owner model; it's much clearer who an owner is versus a parent. It also reflects real life (humans) more so than before; most of the time, you're granted a permission by an "owner" of that permission and that same permission can be revoked by anyone who's an "owner," not just the person who granted it to you.

The only disadvantage compared to the current model is that it won't be possible to have two different permission instances anymore (i.e. token transfers can be done by entity A and B) that could be revoked by different entities (entity A can be revoked its permission by C and B by D).

Was there a use case for this initially? The notion of two permission instances that grant the same capability but are different because they come from different parents feels very artificial.

And, as I'm understanding it, the current proposal would make each permission ownable by only a single entity; what about multiple owners of a permission?

@izqui
Copy link
Contributor Author

izqui commented Nov 30, 2017

The notion of two permission instances that grant the same capability but are different because they come from different parents feels very artificial.

I agree.

what about multiple owners of a permission?

That can be achieved at a second layer setting the permission owner as a Group app or another forwarder.

@sohkai
Copy link
Contributor

sohkai commented Dec 1, 2017

I'm really excited that we'll get to re-write this line (it even has a circular dependency in English!):

If an Entity is the parent Pa of a Permission P, it has the power to grant or revoke access to P for any other Entity that has been granted P with parent Pa.


Small nomenclature suggestion: does "manager" or "controller" make more sense than "owner"? I don't mind "owner" that much, but it does usually suggest a different concept in smart contracts ("owning" the actual contract; yet permissions are not contracts themselves) and may also be slightly confusing in the UNIX analogy.

@izqui
Copy link
Contributor Author

izqui commented Dec 1, 2017

That's true, I think "manager" conveys the concept well without confusion like "owner"

@izqui izqui closed this as completed in #156 Dec 1, 2017
izqui added a commit to aragon/aragon-wiki that referenced this issue Dec 4, 2017
Smokyish added a commit to aragon/aragon-wiki that referenced this issue Dec 6, 2017
Update aragonOS doc to reflect aragon/aragonOS#155 changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants