onury / accesscontrol

Role and Attribute based Access Control for Node.js
https://onury.io/accesscontrol
MIT License
2.21k stars 178 forks source link

Combined own or any check #24

Open webstrap opened 7 years ago

webstrap commented 7 years ago

As far as i read, the recommended way to check for permissions is

(req.user.name === req.params.username)
   ? ac.can(role).updateOwn('photo')
   : ac.can(role).updateAny('photo');

Have you considered to provide combined own or any checks in the form of

const owned = req.user.name === req.params.username;
ac.can('user').create('profile', {owned} )
// or
ac.can('user').create('profile').isOwned(owned)
onury commented 7 years ago

Yes it'd be a clean shorthand.

Actually, .create() is an alias of .createAny() » under the hood, it defaults the possession to any..

I'm considering a very similar API for v3 which will introduce scopes/contexts.

// Query context
const context = {
    own: username === params.username, // if omitted or false, means "any"
    ...
};

So you would check with:

const permission = ac.can('user').create('photo', context);
// or
const permission = ac.can('user').create('photo').with(context);

Note that this is currently not available as of v2.x, suggested API might change...

webstrap commented 7 years ago

Sounds pretty good. Would that allow as well to check directly for attributes, like

ac.can('user').update('photo').with({attributes: ["price","paper"]});

Similar, but way more complex, if you are able to place constraints, like a Junior Salesman is only allowed to change the owner of houses below 10.000 $ and only between 8am and 10am. I guess complexity asks here rather for callback functions, which would not be ideal for database persistance.

 JuniorSalesman: {
        house: {
            "update:any": ["owner"],          
            "context": {"constraints": ["price < 10000","8 < time < 10"]}
        },
    },
SeniorSalesman: {
        house: {
            "update:any": ["owner"]
        },
   },
ac.can('juniorSalesman').update('house').with({time:now, price: expensiveHouse.price});

In the Callback case you would just pass the parameters to the callbacks, which can then execute the logic and return true or false.

EDIT: I'm not sure if the context would make sense on the resource level or rather the permission/action level.

onury commented 7 years ago

We won't lose the attribute filtering feature or anything if that's what you mean. On the contrary, will have more control on the attributes.

But (per your example) you don't need AccessControl to check if a single attribute is set and/or equal to some value. You can do that in plain JS.

What I indicated by time restriction in other threads has a bit different expression. Working on that for quite some time. I'll release a specification and another library for this soon, which is a bit complex and will be a dependency to AccessControl.

psi-4ward commented 6 years ago

I'm currently investigating in an permissions system for an upcoming project and found your solution.

First: looks great and understandable.

But the own and any possessions are really confusing. It can clearly solve the own-Problem like User can edit its own post but another example would be: User can edit posts from his group. It would need way more logic to determine if it's "own".

So a .with(resolve) could solve such cases.

onury commented 6 years ago

Possession, in AccessControl is actually an extra feature. If you don't really need them; you can simply ignore them... i.e. use .create() instead of both .createOwn() or .createAny(). But they can be very powerful if designed properly within the host application's logic.

But the own and any possessions are really confusing. It can clearly solve the own-Problem like User can edit its own post but another example would be: User can edit posts from his group. It would need way more logic to determine if it's "own".

You're limiting what possession can stand for. As I always try to remind; the application design and choices you make, can result in various semantics. Code examples can better demonstrate what I mean..

Grant Permissions:

ac
  .grant('user')
    .updateOwn('Post')
    .updateOwn('GroupPost')  // «—
  .grant('admin')
    .updateAny('Post')

Control of Access:

function handler(req, res) {
    let permission;
    if (isOwnPost(req.user.id, req.params.postId)) {
        permission = ac.can(req.user.role).updateOwn('Post');
    } else if (isMemberOfGroup(req.user.id, req.params.groupId)) {
        permission = ac.can(req.user.role).updateOwn('GroupPost');
    } else {
        permission = ac.can(req.user.role).updateAny('Post');
    }
    // respond or restrict...
}

app.update('/groups/:groupId/posts/:postId', handler);
app.update('/posts/:postId', handler);

Tip: I could also throw in another role, group-admin and slightly change the design...

So, it comes down to how you approach to the problem and the meaning — you give to your resources, actions, etc..

Hope this clarifies a bit.

psi-4ward commented 6 years ago

Thank you! Yes this makes something more clear.

But ... now some permission-logic lives in in ac and others in handler which makes it hard to reason about. For me, it would be much more clear if I had a user-acls.js which defines the rules and resolves the conditions like user.id === post.authorId

onury commented 6 years ago

@psi-4ward somehow I missed your last post.

You can move your AC initialization and permission checks to a separate file, if that's what you mean. The check has to be done per request so a handler is the typical place for that. You could write middlewares or abstract AC as a service depending on your design.

But as in your example (user.id === post.authorId) these parameters come from the request handler. So I don't really see how you would de-couple AC from this.

aggarwaldev commented 5 years ago

@onury Here's something for your reference: https://www.npmjs.com/package/accesscontrol-plus#context

onury commented 5 years ago

That's another repo copying my work without even forking or mentioning this project. MIT license is not enough for some people. I don't understand why people choose to simply take the idea instead of contributing to this project.

Regarding context and other features I'll gather my notes into this repo (project section) soon. Thanks.

Mayhem93 commented 5 years ago

@onury Can't you do something about this? Like, contact either github or npmjs on this matter (because obviously "legal" action is not an option here).