Opened 4 years ago

Closed 4 years ago

#24914 closed New feature (fixed)

Include basic permission mixins into Django

Reported by: Raphael Michel Owned by: Markus Holtermann
Component: contrib.auth Version: master
Severity: Normal Keywords: 1.9
Cc: Markus Holtermann Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Django does provide very convenient decorators in django.contrib.auth, most notably the login_required decorator. At the same time, Django encourages the use of class-based views, where this cannot be used directly. The de-facto standard to do this is a mixin, there even is an example on this mixin in the documentation: https://docs.djangoproject.com/en/1.8/topics/class-based-views/intro/#mixins-that-wrap-as-view

I'll now be working on a proposed pull request during djangocon.eu 2015 sprint.

It is to discuss whether the same is to be done with permission_required.

Change History (26)

comment:1 Changed 4 years ago by Raphael Michel

Development of the pull request is happening on https://github.com/raphaelm/django/tree/ticket_24914

comment:2 Changed 4 years ago by Raphael Michel

Needs documentation: set
Needs tests: set
Owner: changed from nobody to Raphael Michel
Status: newassigned

comment:3 Changed 4 years ago by Joachim Jablon

It's worth noting that it's a feature that exists in http://django-braces.readthedocs.org/en/latest/index.html

comment:4 Changed 4 years ago by Ana Balica

Triage Stage: UnreviewedAccepted

It is a good idea to provide a LoginRequiredMixin, which basically will act the same way as the login_required decorator. It might be implemented in a cleaner way by decorating dispatch method (in contrast with decorating as_view classmethod).

I would opt for getting started with LoginRequiredMixin and afterwards providing mixins for user_passes_test and permission_required from django.contrib.auth.decorators.

https://docs.djangoproject.com/en/dev/_modules/django/contrib/auth/decorators/

comment:5 Changed 4 years ago by Raphael Michel

Summary: Include a LoginRequiredMixin to DjangoInclude basic permission mixins into Django

comment:6 Changed 4 years ago by Raphael Michel

Has patch: set
Needs tests: unset

I created a pull request for this ticket. An initial version of implementation and tests is in the pull request, documentation is yet to be done: https://github.com/django/django/pull/4749

This proposal provides all the and only the functionality of all three authentication decorators from contrib.auth.

While it is slightly differently implemented, it tries to stay compatible with the widely-used 3rd-party package django-braces in terms of parameter and method names. django-braces does have more functionality than the decorators have, but implementing this functionality in django itself is outside the scope of this ticket.

comment:7 Changed 4 years ago by Raphael Michel

Needs documentation: unset

I added documentation. As I'm not a native speaker, I'd love someone to review the language.

I decieded to drop a section in the documentation on class-based view on wrapping as_view with decorators, as there no longer seems to be any decorator in django core where this would make any sense.

comment:8 Changed 4 years ago by Markus Holtermann

Cc: Markus Holtermann added
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:9 Changed 4 years ago by Raphael Michel

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:10 Changed 4 years ago by Markus Holtermann

comment:11 Changed 4 years ago by Markus Holtermann

Owner: changed from Raphael Michel to Markus Holtermann
Patch needs improvement: set

The current implementation has some drawbacks as pointed out on the PR. I'll fix the in aforementioned PR.

comment:12 Changed 4 years ago by Thomas Randle

What would be the recommend approach for stacking different permission mixins on a CBV?

e.g. using LoginRequiredMixin and a PermissionRequiredMixin on the same view.

comment:13 Changed 4 years ago by Markus Holtermann

Thanks for mentioning that, Rundll. The current implementation does not support stacking them (braces' LoginRequired and PermissionRequired mixins do, though). However, neither our UserPassesTestMixin nor braces' allows stacking inherited mixins, either.

That makes me wonder how one is supposed to combine different permission checks that are used separately and together:

class CheckOneMixin(UserPassesTestMixin):
    def test_func(self, user):
        return something_is_true_or_false

class CheckTwoMixin(UserPassesTestMixin):
    def test_func(self, user):
        return something_else_is_true_or_false

class MyView1(CheckOneMixin, View):
    pass

class MyView2(CheckTwoMixin, View):
    pass

class MyView3(CheckOneMixin, CheckTwoMixin, View):
    pass

Effectively, MyView3 will never bother about the value of something_else_is_true_or_false as that function is never called.

comment:14 Changed 4 years ago by Raphael Michel

I don't really see an easy and obvious way to support stacking of UserPassesTestMixin. You could either do something like

class CheckOneMixin(UserPassesTestMixin):
    def test_func(self, user):
        return something_is_true_or_false and super().test_func()

or make the API more complex by allowing each subclass to set/append to a list of test functions in some way.

comment:15 Changed 4 years ago by Markus Holtermann

I just added some tests to check for the mixin stacking.

comment:16 Changed 4 years ago by Markus Holtermann

Here's a PR that adopts django-braces' mixins: https://github.com/django/django/pull/4844

comment:17 Changed 4 years ago by Markus Holtermann

Patch needs improvement: unset

comment:18 in reply to:  13 Changed 4 years ago by Jayson Reis

Replying to MarkusH:

Thanks for mentioning that, Rundll. The current implementation does not support stacking them (braces' LoginRequired and PermissionRequired mixins do, though). However, neither our UserPassesTestMixin nor braces' allows stacking inherited mixins, either.

That makes me wonder how one is supposed to combine different permission checks that are used separately and together:

class CheckOneMixin(UserPassesTestMixin):
    def test_func(self, user):
        return something_is_true_or_false

class CheckTwoMixin(UserPassesTestMixin):
    def test_func(self, user):
        return something_else_is_true_or_false

class MyView1(CheckOneMixin, View):
    pass

class MyView2(CheckTwoMixin, View):
    pass

class MyView3(CheckOneMixin, CheckTwoMixin, View):
    pass

Effectively, MyView3 will never bother about the value of something_else_is_true_or_false as that function is never called.

About ensuring at least one valid response, I thought something like this:

class CheckOneMixin:
    def test_func(self):
        print(self)
        return True


class CheckTwoMixin:
    def test_func(self):
        print(self)
        return False


class Dispatcher:
    def dispatch(self):
        validators = [b.test_func for b in self.__class__.__bases__ if hasattr(b, 'test_func')]
        for validator in validators:
            if validator(self):
                return 'Dispatch for real'

        return 'Invalid permissions'



class MyView1(CheckOneMixin, Dispatcher):
    pass


class MyView2(CheckTwoMixin, Dispatcher):
    pass


class MyView3(CheckOneMixin, CheckTwoMixin, Dispatcher):
    pass


for klass in (MyView1, MyView2, MyView3):
    print(klass().dispatch(), '\n')

In this case the class that act like the real dispatcher could call every function on View's bases.

comment:19 Changed 4 years ago by Jayson Reis

Never mind I just saw that there is a pull request already :)

comment:20 Changed 4 years ago by Markus Holtermann

Thanks jaysonsantos, I had something like that in an experimental version, but dropped it because I disliked iterating over __bases__ or mro().

comment:21 Changed 4 years ago by Markus Holtermann <info@…>

Resolution: fixed
Status: assignedclosed

In e5cb4e1:

Fixed #24914 -- Added authentication mixins for CBVs

Added the mixins LoginRequiredMixin, PermissionRequiredMixin and
UserPassesTestMixin to contrib.auth as counterparts to the respective
view decorators.

The authentication mixins UserPassesTestMixin, LoginRequiredMixin and
PermissionRequiredMixin have been inspired by django-braces
<https://github.com/brack3t/django-braces/>

Thanks Raphael Michel for the initial patch, tests and docs on the PR
and Ana Balica, Kenneth Love, Marc Tamlyn, and Tim Graham for the
review.

comment:22 Changed 4 years ago by Tim Graham

Easy pickings: set
Has patch: unset
Keywords: 1.9 added

Reopening to add documentation for the new methods like get_redirect_field_name(), get_permission_denied_message(), and get_permission_required(). Anything in the changeset above that says " Override this method ..." should be documented.

comment:23 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: closednew

comment:24 Changed 4 years ago by Tim Graham

Has patch: set

PR for additional documentation.

comment:25 Changed 4 years ago by Tim Graham <timograham@…>

In 6c6eb8a6:

Refs #24914 -- Added docs for more auth mixin methods.

comment:26 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top