[Django] #24914: Include a LoginRequiredMixin to Django

[Django]

#24914: Include a LoginRequiredMixin to Django
------------------------------+--------------------
Reporter: raphaelm | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+--------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#24914: Include a LoginRequiredMixin to Django

------------------------------+--------------------------------------

Reporter: raphaelm | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+--------------------------------------
Changes (by raphaelm):

* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:1>

[Django]

#24914: Include a LoginRequiredMixin to Django

------------------------------+--------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+--------------------------------------
Changes (by raphaelm):

* status: new => assigned
* needs_docs: 0 => 1
* owner: nobody => raphaelm
* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:2>

[Django]

#24914: Include a LoginRequiredMixin to Django

------------------------------+--------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed

Has patch: 0 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+--------------------------------------

Comment (by ewjoachim):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:3>

[Django]

#24914: Include a LoginRequiredMixin to Django

------------------------------+------------------------------------

Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by ana-balica):

* stage: Unreviewed => Accepted

Comment:

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/

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:4>

[Django]

#24914: Include basic permission mixins into Django

------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:5>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by raphaelm):

* has_patch: 0 => 1
* needs_tests: 1 => 0

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:6>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by raphaelm):

* needs_docs: 1 => 0

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:7>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by MarkusH):

* cc: MarkusH (added)
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1

* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:8>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by raphaelm):

* needs_better_patch: 1 => 0

* needs_tests: 1 => 0

* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:9>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: raphaelm
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by MarkusH):

Updated PR: https://github.com/django/django/pull/4836

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:10>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------

Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by MarkusH):

* owner: raphaelm => MarkusH

* needs_better_patch: 0 => 1

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:11>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by Rundll):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:12>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by 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:

{{{#!python
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:13>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by raphaelm):

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

{{{#!python
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:14>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by MarkusH):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:15>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by MarkusH):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:16>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by MarkusH):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:17>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by jaysonsantos):

Replying to [comment:13 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:
>

> {{{#!python
> 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:
{{{#!python
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:18>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by jaysonsantos):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:19>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: assigned

Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------

Comment (by MarkusH):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:20>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by Markus Holtermann <info@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"e5cb4e14118f3a508e3bc00ee7cd50bb0f18a61d" e5cb4e1]:
{{{
#!CommitTicketReference repository=""
revision="e5cb4e14118f3a508e3bc00ee7cd50bb0f18a61d"
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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:21>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH
Type: New feature | Status: closed

Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------
Changes (by timgraham):

* keywords: => 1.9
* has_patch: 1 => 0
* easy: 0 => 1

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:22>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: new
Component: contrib.auth | Version: master

Severity: Normal | Resolution:

Keywords: 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------
Changes (by timgraham):

* status: closed => new
* resolution: fixed =>

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:23>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: new
Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: 1.9 | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------
Changes (by timgraham):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/5162 PR] for additional
documentation.

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:24>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: new
Component: contrib.auth | Version: master

Severity: Normal | Resolution:
Keywords: 1.9 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Tim Graham <timograham@…>):

In [changeset:"6c6eb8a6917a538968201267f943ef581db1c22d" 6c6eb8a6]:
{{{
#!CommitTicketReference repository=""
revision="6c6eb8a6917a538968201267f943ef581db1c22d"
Refs #24914 -- Added docs for more auth mixin methods.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:25>

[Django]

#24914: Include basic permission mixins into Django
------------------------------+------------------------------------
Reporter: raphaelm | Owner: MarkusH

Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed

Keywords: 1.9 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Changes (by timgraham):

* status: new => closed
* resolution: => fixed

--
Ticket URL: <https://code.djangoproject.com/ticket/24914#comment:26>