Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#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?

  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.
  1. 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())

Change History (14)

comment:1 by Dylan Verheul, 7 years ago

I'd be happy to submit a PR for this after an acceptable solution has been agreed.

comment:2 by Tim Graham, 7 years ago

Triage Stage: UnreviewedSomeday/Maybe

Writing to the DevelopersMailingList is a better place to get feedback about how to proceed.

comment:3 by Dylan Verheul, 7 years ago

Triage Stage: Someday/MaybeUnreviewed
Type: Cleanup/optimizationBug

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 Tim Graham, 7 years ago

Triage Stage: UnreviewedSomeday/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 Dylan Verheul, 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.

Version 0, edited 7 years ago by Dylan Verheul (next)

comment:6 by Tim Graham, 7 years ago

Summary: Improve handling lack of permissions in AccessMixinFix redirect loop in AccessMixin if a user lack permissions
Triage Stage: Someday/MaybeAccepted

I agree the situation isn't ideal. Let's see what a patch looks like.

comment:7 by Dylan Verheul, 7 years ago

Owner: changed from nobody to Dylan Verheul
Status: newassigned

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:8 by Tim Graham, 7 years ago

Has patch: set

comment:9 by Dylan Verheul, 7 years ago

Should I leave this assigned to myself, or better to deassign so someone will review?

comment:10 by Tim Graham, 7 years ago

Patches needing review isn't affected by whether or not the ticket is assigned.

comment:11 by Carlton Gibson, 7 years ago

Cc: Carlton Gibson added
Triage Stage: AcceptedReady for checkin

comment:12 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 9b1125b:

Fixed #28379 -- Made AccessMixin raise Permissiondenied for authenticated users.

comment:13 by Udo Schochtert, 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 Carlton Gibson, 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.)

Note: See TracTickets for help on using tickets.
Back to Top