permission_required decorator behaviour is odd

[cbrand@…]

The permission_required() decorator is a great idea, but in practice its behaviour is odd.

When used, it first checks whether the user is logged in. If they're not, it redirects to the login page. So far, so good.

If they are logged in, it then checks whether they have been granted the specified permission. If they have, it calls the view function and displays the result. Also good.

If they're logged in but don't have the specified permission, it redirects to the login page. This is odd. Sure, they might have another user id they can use, but that sounds unusual to me. In most cases, this is just going to confuse them because they're already logged in.

Surely it would make more sense to return a HttpResponseForbidden in this case, even if the code to achieve that is a little more complex.

[anonymous]

​http://www.djangosnippets.org/snippets/254/

[cbrand@…]

Yes, that snippet is exactly what I need.

[ctrochalakis]

django-auth-decorators-fix

[ctrochalakis]

Added a check for the user being already authenticated. In that case a HttpResponseForbidden object is returned.

[ctrochalakis]

django-auth-decorators+tests

[ctrochalakis]

New patch. Fixes test cases.

[Matthijs Kooijman <matthijs@…>]

I think this is the same issue as #6310.

[Chris Beaven]

Looks good. Should probably have a new test to show the behavior when a user is not logged in.

[ctrochalakis]

The existing tests already handle this. They first try to access the view without logging in, and assert a redirect to the login page.

[Chris Beaven]

So they do, promoting.

[anonymous]

Perhaps this could be made even better by having it render a 403.html template, similar to the current 404 and 500 behaviour. Apps could then display a styled error page, informing the user that they do not possess the required authorization to view that page.

Maybe even a handler403...

[Malcolm Tredinnick]

This is a terrible usability regression. With this patch, if you are logged in and the permission is denied you get a single, non-customisable, non-styled string back as a result. The current code lets you display a proper page at the "login URL" (which is not going to be just the login box).

If any change like this is going in, the user has to be able to specify what happens when the permission is denied. It shouldn't be a setting (we don't need another one of those). It shouldn't be anything like a handler403, since that's a slippery slope that will never end (and HTTP response codes are the final stage of the processing pipeline -- we don't need to turn them all into exceptions). So either allow a template to be specified or a callable or something.

The approach here is reasonable and not forcing a redirect upon failure isn't a bad idea (it's one reason I don't use the current permission_denied decorator), but the result must be controllable, otherwise it's not use to anybody.

[MilosU]

redirect in case of test failed and user is already authenticated

[MilosU]

How about this patch? It will redirect to a developer supplied url, that can be set in code like:

permission_required('polls.can_vote', noperm_url='custom_url')(my_view)

The question is where to redirect in case the noperm_url is set to None, patch assumes '/', which is again odd. Any suggestions?

[milosu]

update - previous patch had one-liner typo

[milosu]

no permission case handled via template

[milosu]

New patch. Now it is possible to specify template that should be rendered via 403 in case the user did not pass the test, but he is still authenticated.

Better now?

[krystal]

This patch will surely be usefull for a lot of people.

Is it planned to integrate it in 1.1 or 1.2 ?

[DarwinSurvivor]

*Bump*

Has there been any discussion about this (or a similar) patch being added to a future django release?

[Adam Nelson]

Patch needs to be updated to current trunk.

[django@…]

Updated patch for Django 1.2 trunk / RC

[Łukasz Rekucki]

This is a bit related to #5515 (adding handler403) and #10360 (custom forbidden messages). Fixing one of them should solve the problems that mtredinnick described.

[Harro]

To me it doesn't seem obvious that the permission required checks if you are logged in first.
With the whole change to authentication backends so anonymous users can have permissions the logic should be that you check the permission first and if the user does not have the permission and they are not logged in go to the login page, else show a 403 (Which really should be added together with a handler imho)

[alex_dev]

Please consider another approach:

from django.core.exceptions import PermissionDenied 

def permission_required(perm, login_url=None):
    """
    Decorator for views that checks whether a user has a particular permission
    enabled, redirecting to the log-in page if necessary.
    """
    def check_perms(user):
        if user.is_anonymous():
            return False
        if user.has_perm(perm):
            return True
        raise PermissionDenied

    return user_passes_test(check_perms, login_url=login_url) 

This one doesn't change semantics of 'user_passes_test'. It depends on #9847 for handling PermissionDenied exception.

[narsilou]

Version using PermissionDenied as suggested in comment 20

[trez]

Add missing documentation about the fix. Doc depends on ticket #9847

[Roald de Vries <roald@…>]

Improved the documentation and added it to decorators.3.diff

[Sasha Romijn]

Tiny fix on Roalds version: removed old commented out code

[Sasha Romijn]

I have made a very small fix to the patch, removing a commented line of old code. Other than that, it all looks good, and if I understand the comments properly all existing disputes are resolved with this patch.

RFCed.

[Jannis Leidel]

In [16607]:

Fixed #4617 -- Added raise_exception option to permission_required decorator to be able to raise a PermissionDenied exception instead of redirecting to the login page.