Opened 14 years ago

Closed 9 years ago

#15215 closed New feature (duplicate)

API for simpler (permission or any) checks for generic view classes

Reported by: Jari Pennanen Owned by: nobody
Component: Generic views Version: dev
Severity: Normal Keywords:
Cc: Tom Christie, Selwin Ong, amirouche.boubekki@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The generic class based views are difficult and cumbersome to decorate, why even trying to reuse the decorator functions? Why not implement simple functions that would do the checking.

I propose following API for checking the class based views (in this case just permissions):

from django.contrib.auth.viewchecks import login_required, has_perms

class ProtectedView(TemplateView):
    template_name = 'secret.html'
    dispatch_checks = (login_required, has_perms('auth.change_user'),)

(Notice that I used hypothetical django.contrib.auth.viewchecks)

dispatch_checks is list of functions (request, *args, **kwargs) -> bool if allowed to dispatch. Thus the has_perms('auth.change_user') should return function.

Above would also allow overriding the dispatch_checks tuple in subclassed views which I think is very important.

I'm working on a simple patch for django.contrib.auth and to View class to allow this, these checker functions should be reusable in decorators.

Change History (18)

comment:1 by Jari Pennanen, 14 years ago

This could be implemented as Mixin too, could be more pythonic, kind of like:

class ProtectedView(TemplateView, ForbiddenMixin):
    template_name = 'secret.html'
    forbidden_checks = (login_required, has_perms('auth.change_user'),

I have not tested this, but it this ForbiddenMixin should be part of Django it is so general and useful.

comment:2 by Jari Pennanen, 14 years ago

Opened up also a thread to django-devs for this if anyone is interested.

comment:3 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedDesign decision needed

I'm not convinced that this is exactly the right approach, but after using CBVs in action for a bit, I will grant the premise that it's unnecessarily difficult to add a login_required decorator to a CBV on the class itself.

#14512 was an original swing at this with a specific solution aimed at making decorators work for class based views -- it turns out to be inherently complex to do so. The discussions related to that thread provide more detail (thread 1 thread 2).

comment:4 by Tom Christie, 14 years ago

Cc: Tom Christie added

comment:5 by Łukasz Rekucki, 14 years ago

Severity: Normal
Type: New feature

comment:6 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 by Selwin Ong, 13 years ago

This is (roughly) how I implemented permission checks in one of my projects:

def dispatch(self, request, *args, **kwargs):
    if hasattr(self, 'check_permissions'):
        self.check_permissions(request, *args, **kwargs)
    elif self.require_permissions:
        if not request.user.has_perms(self.require_permissions):
            raise PermissionDenied
    return super(TableView, self).dispatch(request, *args, **kwargs)

You can use the API in two different ways:

class MyView(View):
    require_permissions = ['app.view_model', 'app2.view_model2']

or:

class MyView(View):

   def check_permissions(self, request, *args, **kwargs):
       # Custom code

If this approach is acceptable, I'm more than happy to try writing a proper patch and tests for inclusion into django. Comments welcome :)

Version 0, edited 13 years ago by Selwin Ong (next)

comment:9 by Selwin Ong, 13 years ago

Cc: Selwin Ong added

comment:10 by Amirouche, 12 years ago

Cc: amirouche.boubekki@… added

comment:11 by Amirouche, 12 years ago

Cc: amirouche.boubekki@… removed

comment:12 by Amirouche, 12 years ago

Cc: amirouche.boubekki@… added

comment:13 by Florian Apolloner, 12 years ago

Triage Stage: Design decision neededAccepted

Accepting the ticket based on the fact that this stuff should be easier. Not sure that the suggested approach is the best way though.

comment:14 by Garry Polley, 11 years ago

I think this ticket and https://code.djangoproject.com/ticket/14512 are pretty much the same ticket.

Another good spot that has been talked about is here:

https://groups.google.com/forum/#!topic/django-developers/jrfbenCJYYU

With all the current comments on the issue I think there are 2 ideas getting mixed with "mixins".

1) A mixin is getting used to only apply decorators, I think that is the case for:

@my_decorator
class MyView(View):
    def get:
        # my code

2) A mixin is being used to define a clean API. (e.g. form_valid, form_invalid, get_context_data)

I think point 1 is the one that Django should support, and it should only get applied to dispatch.

Looking into a comment by Alex Gaynor I believe this is possible. https://groups.google.com/d/msg/django-developers/jrfbenCJYYU/thffw4vO1D8J

comment:15 by Garry Polley, 11 years ago

After spending some time on this, I don't think this is worth the effort at. I think this ticket and the related one

https://code.djangoproject.com/ticket/14512

should both be closed, in favor of mixins.

Thoughts?

comment:16 by Garry Polley, 11 years ago

Resolution: wontfix
Status: newclosed

Talked to Jacob, the solution here is to bring mixins into Django directly.

comment:17 by Marc Tamlyn, 11 years ago

Resolution: wontfix
Status: closednew

One of these should be kept open to track the issue.

comment:18 by Markus Holtermann, 9 years ago

Resolution: duplicate
Status: newclosed

There has been some discussion among core developers at DjangoCon Europe 2015 where we basically agreed on the implementation as suggested in #24914. I'm therefore closing this ticket as duplicate.

Note: See TracTickets for help on using tickets.
Back to Top