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

Missing decorator for views? #12

Closed
shoooe opened this issue Oct 7, 2014 · 10 comments
Closed

Missing decorator for views? #12

shoooe opened this issue Oct 7, 2014 · 10 comments
Assignees

Comments

@shoooe
Copy link

shoooe commented Oct 7, 2014

In the documentation it's not specified how to limit a view to users that have object based permissions. Just like normally we can do:

@permission_required('app.action_object')
def my_view(request, ...):
    pass

I think that we should be able to somehow take incoming object IDs arguments and automatically block the request if the user does not meet the requirements.

For example, something along the lines of:

@permission_required('app.view_posts', 'post_id')
def view_post(request, post_id):
    pass

What do you think?

@dfunckt
Copy link
Owner

dfunckt commented Oct 7, 2014

How would the decorator know which model to use post_id with? Or do you suggest that it should pass it directly as the obj argument to has_perm()?=

@shoooe
Copy link
Author

shoooe commented Oct 7, 2014

@dfunckt Thanks for the reply. Yes, the first implementation I can think of would fetch the object from the database and pass it to has_perm. There are a lot of issues though, like: "how should the argument to the view be passed to the decorator?" or "how should the decorator know what model to fetch the object from?".

I'm just saying that having:

if not request.user.has_perm(...):
    # 403 or whatever you want

in every view, sounds like it's going against the DRY principle.

I'm open to other solutions if you have any.

@dfunckt
Copy link
Owner

dfunckt commented Oct 7, 2014

In principle, I’m all for adding such a decorator. The implementation is indeed tricky though and I’m not sure it can be generic enough and cover all edge cases. I have several questions:

  • How would the decorator know which model to use for the query? This is simple — we could require a queryset be passed to the decorator as a third argument.
  • Based on your example, what if post_id is not the model's PK, but its slug? That would require another argument for the field name to query on.
  • What arguments should the view eventually receive? Since the decorator will hit the database should the view expect to be given the initial post_id argument, or the fetched object itself?

What do you think? How do other projects implement this kind of decorators?

@shoooe
Copy link
Author

shoooe commented Oct 7, 2014

Based on your example, what if post_id is not the model's PK, but its slug? That would require another argument for the field name to query on.

It sounds reasonable to require some sort of convention on the argument name.

What arguments should the view eventually receive? Since the decorator will hit the database should the view expect to be given the initial post_id argument, or the fetched object itself?

Well, suddenly passing an object would break a lot of things; so I guess we should discard the object and pass a post_id to the view. That's obviously at the cost of performance.

What do you think? How do other projects implement this kind of decorators?

I think that there are a lot of issues. I've never seen any project trying to solve the same problem. I'll think about it for a while and then tell you my best guess. Before I do that, am I missing something obvious?

Is, currently, the only way to enforce object permissions:

object = # fetch object
if request.user.has_perm('permission', object):
    # 403

, inside a view?

@dfunckt
Copy link
Owner

dfunckt commented Oct 7, 2014

Yes, for function-based views this is what you have to do. You could use class-based views and implement a mixin specifically for what you need, but that's a totally different way of doing things.

I'll leave the issue open as the decorator is something I'd like to see in rules. Think over it, and let me know of any ideas you might have.

@shoooe
Copy link
Author

shoooe commented Oct 8, 2014

I haven't found anything satisfying yet. The main problem I see is the repetition of what you do inside that if. That can be solved by introducing a decorator in the form:

@object_permission_required( 'app.permission_name', 
                             lambda request, object_id: SomeObject.objects.get(id = object_id))

Which is suboptimal but slightly better than an if statement. This also takes into account that the id for the object to check might be inside the request object, maybe as a POST or GET parameter, instead of the url parameter.

The signature is basically:

@object_permission_required(<permission_name>, <object fetcher>)

The "fetcher" function can of course be just defined once and used in multiple places:

def some_object_fetcher(request, object_id, *args, **kargs):
    return SomeObject.objects.get(id = object_id)

and then used as:

@object_permission_required('app.permission_name', some_object_fetcher)
def my_view(request, object_id):
    # ...

See if you can go somewhere with this. I'll let you know if something else comes up by the end of the week. What I don't like about this solution is that the "fetcher" is not generic enough, it's coupled with the request and the order of the arguments of the specific view.

@dfunckt
Copy link
Owner

dfunckt commented Oct 9, 2014

Hi @Jefffrey, passing in a callable seems like the most flexible solution and would be indeed useful to support this. We should however, also provide some simple solution for the general case, similar to what we discussed above. Guardian implements a similar decorator to what we need. I will also have a dab at this this weekend.

@dfunckt dfunckt self-assigned this Oct 9, 2014
@shoooe
Copy link
Author

shoooe commented Oct 9, 2014

I agree. Guardian's solution using tuples is probably the best for most cases.

@Visgean
Copy link

Visgean commented Mar 16, 2015

Django guardian uses something similar: https://django-guardian.readthedocs.org/en/v1.2/userguide/check.html#using-decorators though it does kind of awkward...

@dfunckt
Copy link
Owner

dfunckt commented Jul 26, 2015

Fixed in 96e1212. Documentation forthcoming.

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

3 participants