Opened 11 months ago

Closed 3 months 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 (12)

comment:1 Changed 11 months ago by Dylan Verheul

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

comment:2 Changed 11 months ago by Tim Graham

Triage Stage: UnreviewedSomeday/Maybe

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

comment:3 Changed 11 months ago by Dylan Verheul

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 Changed 11 months ago by Tim Graham

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 Changed 11 months ago 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.

Last edited 11 months ago by Dylan Verheul (previous) (diff)

comment:6 Changed 11 months ago by Tim Graham

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 Changed 10 months ago by Dylan Verheul

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 Changed 10 months ago by Tim Graham

Has patch: set

comment:9 Changed 10 months ago by Dylan Verheul

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

comment:10 Changed 10 months ago by Tim Graham

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

comment:11 Changed 3 months ago by Carlton Gibson

Cc: Carlton Gibson added
Triage Stage: AcceptedReady for checkin

comment:12 Changed 3 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 9b1125b:

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

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