Skip to content

$owner Dynamic Role should be applied to find filter #2366

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

Closed
mitsos1os opened this issue May 23, 2016 · 9 comments
Closed

$owner Dynamic Role should be applied to find filter #2366

mitsos1os opened this issue May 23, 2016 · 9 comments

Comments

@mitsos1os
Copy link
Contributor

$owner as a dynamic role, gets triggered by instance methods that have the :id route parameter.
I believe that this should also happen in static methods in a similar way.

For example, let's say I have User and Post model. And they are connected through a User hasMany Posts and Posts belongTo User relationships through foreign key userId in Post model.

When requesting /find on Posts and have an ACL for $owner active, it should add to the query parameter the userId property (or one dynamically defined during model setup) in order to only return the logged in user's posts.

@richardpringle
Copy link
Contributor

Haha @mitsos1os... I can't get over that profile picture!

But I agree, this would be a good feature.

@kevintelford
Copy link

kevintelford commented Feb 28, 2017

Edit: I've created a separate feature request #3241

I'm having the same problem. To piggy back on @mitsos1os Posts example, I want to issue a query to find the users last 5 Posts, AND I don't want other users to be able to see that users Posts.
GET /api/posts?filter[where][ownerId]=123&[order]=updatedAt%20DESC&filter[limit]=5
But how can I craft the ACL? I want to say

{
  "accessType": "*",
  "principalType": "ROLE",
  "principalId": "$everyone",
  "permission": "DENY",
  "property": "*"
},
{
  "accessType": "*",
  "principalType": "ROLE",
  "principalId": "$owner",
  "permission": "ALLOW",
  "property": "*"
}

But the $owner ACL doesn't get applied to my find query (src)

Note:
To qualify a $owner, the target model needs to have a belongsTo relation to the User model (or a model extends from User) and property matching the foreign key of the target model instance. The check for $owner is only performed for a remote method that has ‘:id’ on the path, for example, GET /api/users/:id.

I could change and use $authenticated, but then any authenticated user could peek into the GET request being issued and craft their own using the Authenticate header and get someone else's Posts, which is not good.

@ebarault
Copy link
Contributor

ebarault commented Mar 1, 2017

Note that there are 2 slightly different demands here:

  • the first one (from @mitsos1os) covers the ability to automatically filter the query results based on the detection of the model instance ownership
  • the second one (from @kevintelford) covers strictly the access control part as the user id is passed in the query filter

There's currently WIP on the $owner dynamic role. I'll keep this in mind. At the time being i feel there might be collateral effects resulting from the ACL scoring computation that would prevent in some cases a correct coverage of all users expectations regarding access control (e.g. at the same time filter automatically the posts owned by a user and allow authenticated users to view all posts)

In the meantime have you considered implementing a custom role resolver to cover your needs?

@bajtos any thoughts on this feature?

@mitsos1os
Copy link
Contributor Author

Just for a hint I will tell you what I do to restrict user access to static $owner querying - interacting.
I set up a generic beforeRemote hook for all the static functions (ex. find, findOne, crate, count) that I want, which extracts the access token from the request, and then checks the ctx.args whether they have a data, where, or filter property and add the userId to the specific object... It is a little hardcoded but it works perfectly.

When I have some time I was thinking of implementing the owner role to check for the relation to the user document in the same way as $owner does, then add the property which would be dynamically retrieved from relation to the querying data... But if you say work is already being done... I will wait!

@ebarault
Copy link
Contributor

ebarault commented Mar 1, 2017

@mitsos1os : using beforeRemote hook makes perfect sense, I implemented this as well in some projects. I would not call that hardcoded. The way I did was to make it a reusable mixin so this can be activated on a per model basis, and in an optionnated manner.

@bajtos bajtos changed the title $owner Dynamic Role should be available in static methods too $owner Dynamic Role should be applied to find filter Mar 1, 2017
@bajtos
Copy link
Member

bajtos commented Mar 1, 2017

he ability to automatically filter the query results based on the detection of the model instance ownership

This feature request makes total sense. In my personal opinion, it should have been a part of the ACL system from the beginning.

@raymondfeng @ritch @superkhau ☝️ something to consider for the next LoopBack version

I set up a generic beforeRemote hook for all the static functions (ex. find, findOne, crate, count) that I want, which extracts the access token from the request, and then checks the ctx.args whether they have a data, where, or filter property and add the userId to the specific object... It is a little hardcoded but it works perfectly.

Now that accessToken is available in operation hooks via ctx.options.accessToken, it should be possible to register a single access hook observer instead of multiple beforeRemote hooks. Just an idea to consider.

The way I did was to make it a reusable mixin so this can be activated on a per model basis, and in an optionnated manner.

@ebarault Have you considered open-sourcing that mixin, so that other LoopBack users could use it too?

@mitsos1os
Copy link
Contributor Author

Now that accessToken is available in operation hooks via ctx.options.accessToken, it should be possible to register a single access hook observer instead of multiple beforeRemote hooks. Just an idea to consider.

Yes I agree, with the exception that you cannot have the access hook triggered for create operations and updateAttributes...

I also like the idea of a configurable mixin for accepting also some options on which actions it should restrict access... @bajtos where would be a good location to share this mixin?

@bajtos
Copy link
Member

bajtos commented Mar 1, 2017

where would be a good location to share this mixin?

I would create a new package developed on github.com and published to npmjs.org. You can promote it e.g. in https://github.com/pasindud/awesome-loopback and http://loopback.io/doc/en/community/index.html

@bajtos
Copy link
Member

bajtos commented Mar 6, 2017

I am closing this issue as a duplicate of much older #343.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants