Fix redirect loop in AccessMixin if a user lack permissions

[Dylan Verheul]

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

[Dylan Verheul]

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

[Tim Graham]

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

[Dylan Verheul]

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.

[Tim Graham]

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.

[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.

[Tim Graham]

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

[Dylan Verheul]

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

[Tim Graham]

​PR

[Dylan Verheul]

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

[Tim Graham]

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

[Tim Graham <timograham@…>]

In 9b1125b:

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

[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

[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.)