[Django] #28379: Improve handling lack of permissions in AccessMixin
Django
-------------------------------------+-------------------------------------
Reporter: Dylan | Owner: nobody
Verheul |
Type: | Status: new
Cleanup/optimization |
Component: | Version: 1.11
contrib.auth |
Severity: Normal | Keywords: permissions
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`AccessMixin` and derived classes offer a great way to handle permission
testing for class based views. However, the current default workflow for
handling failed permissions can lead to an endless loop of redirects, and
is hard to fix without resorting to writing a custom `AccessMixin` for
each project.
**Why is the current workflow wrong?**
The default way of handling "no permission" is to check for
`AccessMixin.raise_exception` (default: `False`). If
`AccessMixin.raise_exception` is `True`, then the `PermissionDenied`
exception is raised. Else (the default behavior), the user is redirected
to the `login_url`.
The `PermissionDenied` exception can be customized in Django by providing
a `403.html`. In this template, it is easy to explain to the user that
access was denied, and to check for possible solutions, such as loggin in
an authorized user. In my opinion, this should always be the default. Lack
of permissions in a Python application calls for an exception to be
raised.
However, the default behavior is to redirect to the `login_url`. What
happens to a **user that has not yet logged in** this scenario?
1. The anonymous user visits a URL, let's call it `show_me_something`
2. Permission is denied
3. The user is redirected to a login page, without any explanation
4. Upon successful login, the user will probably be redirected to the
original intent `show_me_something`
This isn't pretty, the user doesn't know what is going on. In my apps I
would always go for the exception to be raised and to show a 403 page that
explains that the uses has insufficient rights to view
`show_me_something`, offer the user a link to login and signup, and carry
over the url to `show_me_something` as a redirect upon succesful login.
What's worse is what happens to a user that is already logged in.
1. The authenticated user visits a URL, let's call it `show_me_something`
2. Permission is denied
3. The user is redirected to a login page, without any explanation, but
already logged in. Some login pages may even refuse access to a logged in
user and redirect them. More confusion!
4. The only logical thing for the user to do is to provide login
credentials again.
5. Upon successful login, the user will probably be redirected to the
original intent `show_me_something`, where permission will be denied
again, goto 1.
**What should be improved**
My suggestions are:
1. All in all I'd say always raise the `PermissionDenied` exception and
have one way to handle missing permissions (DRY). That would mean at least
changing the default value for `AccessMixin.raise_exception` to `True`,
and possible even deprecate it. Currently, setting `raise_exception` to
False is a recipe for writing views that confuse users and even make them
end up in endless loops.
2. If raising the exception is not the behavior in the AccessMixin, never
redirect a logged in user to the `login_url`, but raise `PermissionDenied`
instead. So change `handle_no_permission` to:
{{{
def handle_no_permission(self):
if self.raise_exception or self.request.user.is_authenticated:
raise PermissionDenied(self.get_permission_denied_message())
return redirect_to_login(self.request.get_full_path(),
self.get_login_url(), self.get_redirect_field_name())
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28379>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Dylan Verheul):
I'd be happy to submit a PR for this after an acceptable solution has been
agreed.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Someday/Maybe
Comment:
Writing to the DevelopersMailingList is a better place to get feedback
about how to proceed.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:2>
Django
Reporter: Dylan Verheul | Owner: nobody
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dylan Verheul):
* type: Cleanup/optimization => Bug
* stage: Someday/Maybe => Unreviewed
Comment:
Thanks for your feedback. Maybe I didn't explain correctly. I know how to
submit a PR for Django.
The current default behavior of Django sends an authenticated user that
accesses a `PermissionRequiredMixin` based view into an endless loop of
redirects (no permissions - login - no permissions - login, ad infinitum).
This is wrong. The best fix requires a change of a default value in
`PermissionRequiredMixin`.
I'd like confirmation of this bug and the proposed solution before
submitting my PR. This looks like the correct place for that. I'll set the
ticket to Bug instead Cleanup/Optimization.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:3>
Django
Reporter: Dylan Verheul | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Someday/Maybe
Comment:
What I meant is that to revisit the design decision of `raise_exception =
False` and change its default value (which has backwards compatibility
ramifications), a discussion on the mailing list is required.
`AccessMixin` was mostly copied from [https://github.com/brack3t/django-
braces/blob/cda572a798f9b8e72015515c8d00b4951a076888/braces/views/_access.py
django-braces]. I searched that project's issues briefly but didn't see
this issue raised there.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:4>
Django
Reporter: Dylan Verheul | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage: Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Dylan Verheul):
Thanks for clarifying Tim. I checked `django-braces`, no issues filed
there as far as I can see. Of course, core Django has a much wider
audience that django-braces ever had (with all due respect, former
`django-braces` user here, very happy to see their ideas pulled into the
main project).
Fact is that the default settings of Django now result in an infinite
redirect to the login view if an authenticated user hits an AccessMixin
and fails the conditions. That should be fixed at least. If changing the
default value is too big a change, I'd vote to at least raise
Permissiondenied when an authenticated user hits AccessMixin without
meeting the conditions. Redirecting an authenticated user to the login
page does not make any sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:5>
Django
-------------------------------+------------------------------------
Reporter: Dylan Verheul | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Someday/Maybe => Accepted
Comment:
I agree the situation isn't ideal. Let's see what a patch looks like.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:6>
Django
Reporter: Dylan Verheul | Owner: Dylan Verheul
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dylan Verheul):
* owner: nobody => Dylan Verheul
* status: new => assigned
Comment:
PR coming up. I also found precedence in Documentation for
`LoginRequiredMixin`:
''If a view is using this mixin, all requests by non-authenticated
users will be redirected to the login page or shown an HTTP 403 Forbidden
error, depending on the raise_exception parameter.''
source: https://docs.djangoproject.com/en/1.11/topics/auth/default/#the-
loginrequired-mixin
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:7>
Django
-------------------------------+-----------------------------------------
Reporter: Dylan Verheul | Owner: Dylan Verheul
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8741 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:8>
Django
-------------------------------+-----------------------------------------
Reporter: Dylan Verheul | Owner: Dylan Verheul
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Dylan Verheul):
Should I leave this assigned to myself, or better to deassign so someone
will review?
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:9>
Django
-------------------------------+-----------------------------------------
Reporter: Dylan Verheul | Owner: Dylan Verheul
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: permissions | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
[https://dashboard.djangoproject.com/ Patches needing review] isn't
affected by whether or not the ticket is assigned.
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:10>
Django
Reporter: Dylan Verheul | Owner: Dylan
| Verheul
Type: Bug | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Dylan Verheul | Owner: Dylan
| Verheul
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: permissions | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9b1125bfc7e2dc747128e6e7e8a2259ff1a7d39f" 9b1125b]:
{{{
#!CommitTicketReference repository=""
revision="9b1125bfc7e2dc747128e6e7e8a2259ff1a7d39f"
Fixed #28379 -- Made AccessMixin raise Permissiondenied for authenticated
users.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Dylan Verheul | Owner: Dylan
| Verheul
Type: Bug | Status: closed
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: permissions | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Udo Schochtert):
note: this change made it to v2.1.3
(https://github.com/django/django/releases/tag/2.1.3)
just raising a 403 for authenticated users is not ideal.
it might be better to give users the possibility to redirect to another
url instead of just throwing the 403.
in my case I am using a custom mixin in which I redirect authenticated
users to another page and show them a message (that they have been
redirected due to missing access rights), anonymous users are redirected
to login (the default behaviour). after my update to v2.1.3 this does not
work anymore...
it might be better if AccessMixin would have a method something like
"get_redirect_url" (defaults to "/") to which an authenticated user will
be redirected. an anonymous user is redirected to login.
cheers, udo
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Dylan Verheul | Owner: Dylan
| Verheul
Type: Bug | Status: closed
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: permissions | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Udo, I’ve opened #29977 to track this. If you were able to add a test
case demonstating the changed behaviour it would aid assessment. (Is you
use-case supported? If so, have we introduced a regression? If not, is
this a new feature request? And so on.)
--
Ticket URL: <https://code.djangoproject.com/ticket/28379#comment:14>