Opened 3 months ago

Last modified 2 months ago

#28379 assigned Bug

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: Triage Stage: Accepted
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 (10)

comment:1 Changed 3 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 3 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 2 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 2 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 2 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 2 months ago by Dylan Verheul (previous) (diff)

comment:6 Changed 2 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 2 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 2 months ago by Tim Graham

Has patch: set

comment:9 Changed 2 months ago by Dylan Verheul

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

comment:10 Changed 2 months ago by Tim Graham

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

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