redirect_unauthenticated_users will cause raise_exception to be ignored

[cornmander]

When chaining LoginRequiredMixin and another AccessMixin, setting raise_exception = True and redirect_unauthenticated_users = True doesn't seem to do the right thing.

[kennethlove]

I tried to replicate this but it passed the test (I got a 302). Did you expect the redirect to happen instead?

[gkeller2]

@kennethlove : I think this issue is related to issue #161 (redirect loop).

[snip]
[10/Aug/2015 16:21:56]"GET /profiles/list/ HTTP/1.1" 302 0
[10/Aug/2015 16:21:56]"GET /accounts/login/?next=/profiles/list/ HTTP/1.1" 302 0
[10/Aug/2015 16:21:56]"GET /profiles/list/ HTTP/1.1" 302 0
[10/Aug/2015 16:21:56]"GET /accounts/login/?next=/profiles/list/ HTTP/1.1" 302 0
[snip]

I've just stumbled upon the same situation. In the following view, I want to check that the user is authenticated and that it belongs to the appropriate group. If the user is not authenticated, I want it to be redirected to the LoginView. If it is authenticated, but does not belong to the right group, I want an exception to be rised.

class UserProfileListView(LoginRequiredMixin, GroupRequiredMixin, ListView):
    group_required = u"Academic Administrator"
    raise_exception = True
    redirect_unauthenticated_users = True

    pass

NOTE: Actually, this problem occurs even without having LoginRequiredMixin as one of the parents.

The problem resides in the method handle_no_permission from the class AccessMixin:

    def handle_no_permission(self, request):
        if self.raise_exception and not self.redirect_unauthenticated_users:
            if (inspect.isclass(self.raise_exception)
                    and issubclass(self.raise_exception, Exception)):
                raise self.raise_exception
            if callable(self.raise_exception):
                ret = self.raise_exception(request)
                if isinstance(ret, (HttpResponse, StreamingHttpResponse)):
                    return ret
            raise PermissionDenied

        return self.no_permissions_fail(request)

The problem is twofold: first, the fact that the default behaviour is a redirection to the login page, and second, how the flag redirect_unauthenticated_users is being used. Consider the following situations:

1. if the flag raise_exception is False, the default behaviour is to redirect the user to the login page. However, if the user is already authenticated, the user is redirected back to the page for which it didn't have permissions, thus closing the loop.

2. if the flag raise_exception is True and the flag redirect_unauthenticated_users is False, an exception is raised (as expected).

3. if the flag raise_exception is True and the flag redirect_unauthenticated_users is True, the user is redirected to the login page with independence of whether or not the user is authenticated. If the user was already authenticated, we run again into the situation described in (1) above.

The solution I'd propose is to refactor the method handle_no_permission to simply raise an exception or redirect the user somewhere other than the login page. This other page to redirect to should be indicated by the programmer by setting the variable redirect_to_url. One of the two behaviours has to be defined by the programmer (raising an exception or redirecting the user).

    def handle_no_permission(self, request):
        if not (self.raise_exception or redirect_to_url):
            raise ImproperlyConfigured(...)
        if self.raise_exception:
            if (inspect.isclass(self.raise_exception)
                    and issubclass(self.raise_exception, Exception)):
                raise self.raise_exception
            if callable(self.raise_exception):
                ret = self.raise_exception(request)
                if isinstance(ret, (HttpResponse, StreamingHttpResponse)):
                    return ret
            raise PermissionDenied
        else:
            return HttpResponseRedirect(redirect_to_url)

Now, if a programmer wants to also have unauthenticated users redirected to the login page, then they should use LoginRequiredMixin in addition to whatever permission or group they are trying to check.

Of course, the above refactoring of handle_no_permission also requires the refactoring of dispatch in LoginRequiredMixin, so that any user that is not authenticated is immediately redirected to the login page. There's no need to run any code from handle_no_permission. LoginRequiredMixin should do one thing only and do it well: if a user is not authenticated, redirect it to the login page. No more, no less.

    def dispatch(self, request, *args, **kwargs):
        if not request.user.is_authenticated():
#            return self.handle_no_permission(request)
            return self.no_permissions_fail(request)

        return super(LoginRequiredMixin, self).dispatch(
            request, *args, **kwargs)

[marksweb]

Just to add another to this, I've got a SuperuserRequiredMixin view which then hits the redirect view due to the ?next arg.

class ClientListView(LoginRequiredMixin, SuperuserRequiredMixin, FormView):
    """
    List of clients
    """
    http_method_names = ['get', 'post']
    form_class = ClientListForm
    raise_exception = True
    login_url = settings.LOGIN_URL
    redirect_unauthenticated_users = True

Oddly I was seeing the unauthenticated users redirected to a page they can access while I had redirect_unauthenticated_users set to False. This confused the hell out of me but it appears to be a redirect URL used when PermissionDenied is raised.

I wondered if a UserPassesTestMixin could provide a workaround to allow for checking that a user is logged in, a superuser etc, but then that only results in a True/False value which wouldn't help. Could a custom handle_no_permission be used to implement default redirect URLs for already authenticated users to prevent the loop?

[iopsthecloud]

I agree with @gkeller2 I've the same issue. If the user is authenticated but not authorized to view the content page, the call go in an infinite loop.

The solution proposed by @gkeller2 sounds good in my case.

[gkeller2]

@renaudManda : I've implemented the solution locally; let me know if you'd be interested in having a look at it.

[iopsthecloud]

@gkeller2 yes of course. I did myself as well. You can check on https://gist.github.com/renaudManda/9db2a8185966b01ee8e9

[kennethlove]

How about a pull request on this from @gkeller2 or @renaudManda ?

[gkeller2]

@kennethlove : done #196

I did it in a hurry, forking the project, creating a branch and making the changes I did in my local project. I'm a bit busy at the moment, but do contact me for anything and I'll try to assist. I didn't run any tests -- sorry for that!

[gkeller2]

@kennethlove : have you checked out the code?

[cornmander]

@gkeller2 @kennethlove I tried my own shot at making a fix -- see #200 -- I think this is a cleaner fix because there's no API changes. I tested the fix inline to my project and it does the trick, but I'm having problem getting my test case to fail and succeed when it should. Can you take a look?

[tylerjryan]

bumping for interest

[iopsthecloud]

+1 for @cornmander PR :)