Code

Opened 3 years ago

Last modified 10 months ago

#15215 new New feature

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

Reported by: Ciantic Owned by: nobody
Component: Generic views Version: master
Severity: Normal Keywords:
Cc: tomchristie, selwin, 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.

Attachments (0)

Change History (17)

comment:1 Changed 3 years ago by Ciantic

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 3 years ago by Ciantic

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

comment:3 Changed 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Design 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 Changed 3 years ago by tomchristie

  • Cc tomchristie added

comment:5 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:6 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:8 Changed 2 years ago by selwin

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 :)

If we simply require people to be logged in, we can simply check for login_required boolean attribute on the view:

class MyView(View):
    login_required = True

*Edit: added login required check API

Last edited 2 years ago by selwin (previous) (diff)

comment:9 Changed 2 years ago by selwin

  • Cc selwin added

comment:10 Changed 19 months ago by abki

  • Cc amirouche.boubekki@… added

comment:11 Changed 19 months ago by abki

  • Cc amirouche.boubekki@… removed

comment:12 Changed 19 months ago by abki

  • Cc amirouche.boubekki@… added

comment:13 Changed 16 months ago by apollo13

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 10 months ago by garrypolley

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 Changed 10 months ago by garrypolley

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 Changed 10 months ago by garrypolley

  • Resolution set to wontfix
  • Status changed from new to closed

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

comment:17 Changed 10 months ago by mjtamlyn

  • Resolution wontfix deleted
  • Status changed from closed to new

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.