Pass the resource into the getRoleId() function

[spruce-bruce]

This is a very small technical change, but one that deserves a little bit of discussion. I'd like to pass the resource into the getRoleId() function on the user object.

My use case is getting more complicated as the requirements for my project change. I have a structure now where a user can have different 'roles' on different resources. I can support this requirement by passing getRoleId() the resource and allowing the getRoleId() function to return different roles depending on the resource that its passed.

This might be a philosophical change to how the library is intended to be used, though, so I'm curious if there are any thoughts on this change.

[spruce-bruce]

Haven't heard from you, but I went ahead and threw together the PR for this change. Let me know if you're not into it or would like some changes

[djvirgen]

This is interesting. Could this not be solved with a custom assertion? The assertion callback has access to the role, resource, and requested action, which means you should be able to allow or deny based on that. If custom assertions can't solve this, I'm interested to see how you're using this.

[spruce-bruce]

It can be, and we are using it that way currently.

There are a couple of things that are kind of clunky in our particular use case, though, when using arrays of roles. And I totally understand if our use case seems fringe and you'd rather not support it.

Here's an example of a case we have that is simplified by the change in the PR:

If we imagine a blog, we define a Post resource. We define 3 roles, create, edit, and view. Edit inherits from view, create inherits from edit.

So take this snippet:

var user = {
  getRoleId: function() {
    // this user has the create role on Post:1, the edit role on Post:2 and 
    // the view role on Post:3 I have no context for which resource I'm 
    // acting on so I must return all the roles that this user has
    return [
      'create',
      'edit',
      'view',
    ];
  }
}

acl.allow(user, post, 'edit', function(err, user, resource, aciton, result, next) {
  // The user has the 'create' and 'edit' roles so we're going to call this function 
  // for every Post resource.
  
  // We've used role inheritance to determine that this is the assertion  
  // we should be running, but in order to correctly determine that I 
  // have access to edit Post:1 I will have to do a lookup to determine 
  // if this particular user has the create OR edit roles for this particular 
  // resource
});

So, using the ability to return arrays of roles from getRoleId I'm attempting to support a permissions model where my users have different roles per resource, ie, I have 2 Blog posts and I have different roles depending on which of those 2 Blog posts I'm taking action on.

We currently support this permissions model with the assertions as they are, but it requires me to write custom assertions for all of my .allow calls that check up the hierarchy of roles every time. If I can instead return the roles the user has based on the context of the resource, I can eliminate all (or most) of my custom assertions and transplant my logic for determining roles into the user model, and will allow me to be able to rely on role inheritance more completely.

[djvirgen]

Oh, that's interesting. I hadn't thought of using roles like that. I usually think of them as things like admin, member, guest etc. That is, what the object is vs what it can do.

When using them in this way, what are the actions you're passing in to the ACL? e.g.

acl.query('edit', 'blog', ???, function(err, allowed) { /* ... */ });

The way I'd do this would be a little different:

var user = {
  id: null,
  getRoleId: function() {
    if (1 === this.id) return 'admin';
    if (this.id > 1) return 'member';
    return 'guest';
  }
}

var blog = {
  authorId: 7,
  getResourceId: function() {
    return 'blog';
  }
}

acl.allow('admin', 'blog', 'edit'); // admins can edit any blog
acl.allow('member', 'blog', 'edit', function(err, role, resource, action, result, next) {
  if (resource.authorId === role.id) return result(null, true);
  next();
});
acl.allow('guest', 'blog', 'view'); // everyone can view any blog

Could something like this work for you?

[spruce-bruce]

No, there's no natural relationship between user -> resource in my use case that I can use to determine access.

I can't say "because this user created this particular post they get to edit it". I need to be able to assign posts to users explicitly. It's the case where an admin comes into the system and says "allow this member to edit these 10 posts but not these 5".

So I have to link up users as editors for some blog posts and not others, but not only that I have two or three other roles that they may have that determine their access to a particular post. So I need to say something like: User:1 can edit Post:1, and User:1 can edit and delete Post:2, and User:1 can create new Posts.

In order to represent this relationship I have to store some information about the relationship about this user to Post:1 and Post:2, and since (in this example) the user did not create those posts and otherwise has no natural relationship, I have to decide how to relate them.

The crux is that any user can be assigned to edit any post. So I promote my "member" to "editor" and now I have to say in my assertion callback "does this person have edit privileges on this specific resource?" (this information is stored in the database).

Then after that I have one more complication: I want multiple levels of privileges per Post. Not only do I want to say a user is an editor, but that user can delete also. I want the "deleter" role to inherit from the "editor" role. User:1 can be an editor on Post:1 and User:2 can be a deleter on Post:2. So I grant User:2 the deleter role and User:1 the editor role. My assertions look like this now:

acl.allow('admin', 'blog', null); // admins can edit any blog

// editors can edit some posts
acl.allow('editor', 'blog', 'edit', function(err, role, resource, action, result, next) {
  if (role.hasEditPermOn(resource)) {
    return result(null, true)
  }

  // this logic could be done in "hasEditPermOn", but the point is the same
  // I have to do this check.
  if (role.hasDeletePermOn(resource)) {
    return result(null, true);
  }

  // It's at this point that I'm thinking: how can I simplify this call?
  // Is there a way to allow virgen-acl to take back control of the logic
  // of determining whether or not this user has access to this
  // resource?
  
  next();
}

// members can edit their own posts
acl.allow('member', 'blog', 'edit', function(err, role, resource, action, result, next) {
  if (resource.authorId === role.id) return result(null, true);
  next();
});
acl.allow('guest', 'blog', 'view'); // everyone can view any blog

In the scenario above, anybody will have the deleter role if they are allowed to delete even one Post. Now I have to make things more complicated: we have Pages as well as Posts. I also need to be able to assign users arbitrary permissions to Pages (this is why I proposed the multiple roles change). Now I need roles like this: admin, post-deleter, post-editor, page-deleter, page-editor, member.

My assertion callbacks for posts look similar to my assertion callbacks for pages. I have to check up the permissions hierarchy every time. This works, but it's not as clean as I'd like. I've lost some of the value of role inheritance. I can't just check if this user has been assigned as an editor, but I must also check if this user has been assigned as a deleter on the given resource.

Now my real use case I have a lot more resources and a lot more roles. The logic in my assertions above is checking a hierarchy of 3 or 4 every time. In order to build roles in my User models getRoleId() function I'm pulling from the database all the permissions a user has on any resource and building a list of roles like above, and returning the full list of roles that a user has.

Then every assertion has to re-check roles to make sure that 1, do they have edit on this thing? and 2, do they have deleter on this thing?

This architecture is simplified if I have the resource available to me in the user object when I go to deliver the roles. I can simply say "oh you're checking access to Post:1? Well here is the role this user has for that resource". This results in fewer false positives (ie, executing assertion callbacks fewer times), and I never have to check up through a permissions hierarchy because I get to rely on virgen-acl to do that for me, which is desirable.

Now, that all being said, I think this is a more complex use case than virgen-acl was maybe intended for, but I also think that virgen-acl was very well designed and is flexible enough to handle very complex use cases. This change helps me more cleanly support my complex use case.

[spruce-bruce]

Hey @djvirgen i know i've written a lot on the topic.

I'm curious if you're still not convinced. I want this change enough that I'd probably maintain a fork with this feature if you'd prefer not to merge it. Before I go through the effort, though, I wanted to check in one more time.

[djvirgen]

Hi @spruce-bruce ,

After reading through your latest comments a few times, I think the issue mostly involves how you're using roles. The role name itself should not indicate any level of permission. For example, a role of editor implies permission to edit all posts, but you want them to be editor of a certain post, which makes things much harder to manage as you are experiencing. Generally, a user's role is static, and does not change once new permissions are granted. In many use cases, you could use the single role user for everything.

This allows Virgen ACL to look up the appropriate permissions based on the provided .allow() and .deny() rules. So in your example, when the author of a post grants a user permission to edit a post, that user's role (at least in Virgen ACL) does not change. It should remain user. When querying the ACL, it will see that a particular user wants to take an action on a specific resource. The callback function may grow a bit in complexity, but it doesn't need any more information than is provided. For example, your resource could contain a list of user IDs that are allowed to edit the post, retrievable via Resource#getApprovedEditors():

acl.allow('user', 'post', 'edit', function(err, role, resource, action, result, next) {
  // authors can always edit their own posts
  if (resource.authorId === role.id) return result(null, true);

  // privileged users can also edit the post
  if (resource.hasOwnProperties('getApprovedEditors') && 
    -1 !== resource.getApprovedEditors().indexOf(role.id)) return result(null, true);

  // if we got this far, this assertion can't make a determination either way, continue to next rule
  next();
});

If Virgen ACL starts to support dynamic roles based on the resource and/or action, I think that will move too much of the access-control rules to the domain entities, which kind of defeats the purpose of an ACL. I'd prefer the roles and resources would simply identify themselves, and let the ACL determine who can do what.

I hope this helps, let me know what you think.

[spruce-bruce]

I think that's fair, and I completely understand the rationale. My current system is using the .allow() callbacks as you suggest and this works .

It does prevent me from getting the use out of role inheritance that I would like, though, since (use your example) I would only have one role. Every user in my system will have the 'user' role and the user role will be allowed to do anything to any entity and the only thing preventing that will be the logic in allow callbacks.

The value of virgen-acl in such a system is negligable, then. I could remove virgen-acl and just use the logic in my allow callbacks instead of acl.query() to determine whether or not a use has access to a resource.

And that's fair, too. If you think that in a system like mine, where any user could potentially be given access to any resource, that virgen-acl isn't the right choice then it totally makes sense to prevent virgen-acl from moving in the direction of supporting such a system.

It remains true, though, that using virgen-acl with the change in this PR allows me to simplify my app code and maintain a permissions structure that is easy to understand because of expressiveness in writing .allow() calls like .allow('comment-editor', 'comment', ['view', 'edit']);

I'll just maintain a fork then for my purposes and stop harassing you about this. Thanks for getting back to me on this, and really, I like this library quite a lot.