#28379 closed Bug (fixed)
Fix redirect loop in AccessMixin if a user lack permissions
Reported by: | Dylan Verheul | Owned by: | Dylan Verheul |
---|---|---|---|
Component: | contrib.auth | Version: | 1.11 |
Severity: | Normal | Keywords: | permissions |
Cc: | Carlton Gibson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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?
- The anonymous user visits a URL, let's call it
show_me_something
- Permission is denied
- The user is redirected to a login page, without any explanation
- 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.
- The authenticated user visits a URL, let's call it
show_me_something
- Permission is denied
- 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!
- The only logical thing for the user to do is to provide login credentials again.
- 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:
- 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 forAccessMixin.raise_exception
toTrue
, and possible even deprecate it. Currently, settingraise_exception
to False is a recipe for writing views that confuse users and even make them end up in endless loops.
- If raising the exception is not the behavior in the AccessMixin, never redirect a logged in user to the
login_url
, but raisePermissionDenied
instead. So changehandle_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())
Change History (14)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Triage Stage: | Unreviewed → Someday/Maybe |
---|
Writing to the DevelopersMailingList is a better place to get feedback about how to proceed.
comment:3 by , 7 years ago
Triage Stage: | Someday/Maybe → Unreviewed |
---|---|
Type: | Cleanup/optimization → Bug |
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.
comment:4 by , 7 years ago
Triage Stage: | Unreviewed → Someday/Maybe |
---|
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 django-braces. I searched that project's issues briefly but didn't see this issue raised there.
comment:5 by , 7 years ago
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.
comment:6 by , 7 years ago
Summary: | Improve handling lack of permissions in AccessMixin → Fix redirect loop in AccessMixin if a user lack permissions |
---|---|
Triage Stage: | Someday/Maybe → Accepted |
I agree the situation isn't ideal. Let's see what a patch looks like.
comment:7 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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
comment:9 by , 7 years ago
Should I leave this assigned to myself, or better to deassign so someone will review?
comment:10 by , 7 years ago
Patches needing review isn't affected by whether or not the ticket is assigned.
comment:11 by , 7 years ago
Cc: | added |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:13 by , 6 years ago
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
comment:14 by , 6 years ago
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.)
I'd be happy to submit a PR for this after an acceptable solution has been agreed.