Opened 17 years ago

Closed 13 years ago

#4617 closed Bug (fixed)

permission_required decorator behaviour is odd

Reported by: cbrand@… Owned by: ctrochalakis
Component: contrib.auth Version: dev
Severity: Normal Keywords: easy-pickings dceu2011
Cc: Tom Christie Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (10)

django-auth-decorators.diff (1.2 KB ) - added by ctrochalakis 16 years ago.
django-auth-decorators-fix
django-auth-decorators-plus-tests.diff (3.4 KB ) - added by ctrochalakis 16 years ago.
django-auth-decorators+tests
decorator.diff (3.5 KB ) - added by MilosU 16 years ago.
redirect in case of test failed and user is already authenticated
decorator.2.diff (3.6 KB ) - added by milosu 16 years ago.
update - previous patch had one-liner typo
decorators.diff (4.0 KB ) - added by milosu 16 years ago.
no permission case handled via template
decorators.2.diff (1.9 KB ) - added by django@… 14 years ago.
Updated patch for Django 1.2 trunk / RC
decorators.3.diff (3.7 KB ) - added by narsilou 13 years ago.
Version using PermissionDenied as suggested in comment 20
documentation.diff (560 bytes ) - added by trez 13 years ago.
Add missing documentation about the fix. Doc depends on ticket #9847
permission_required.diff (4.7 KB ) - added by Roald de Vries <roald@…> 13 years ago.
permission_required_tinyfix.diff (4.4 KB ) - added by Sasha Romijn 13 years ago.
Tiny fix on Roalds version: removed old commented out code

Download all attachments as: .zip

Change History (39)

comment:2 by cbrand@…, 17 years ago

Yes, that snippet is exactly what I need.

comment:3 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by Jacob, 16 years ago

Triage Stage: Design decision neededAccepted

by ctrochalakis, 16 years ago

Attachment: django-auth-decorators.diff added

django-auth-decorators-fix

comment:5 by ctrochalakis, 16 years ago

Has patch: set
Version: 0.96SVN

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

by ctrochalakis, 16 years ago

django-auth-decorators+tests

comment:6 by ctrochalakis, 16 years ago

Owner: changed from nobody to ctrochalakis
Status: newassigned

New patch. Fixes test cases.

comment:7 by Matthijs Kooijman <matthijs@…>, 16 years ago

I think this is the same issue as #6310.

comment:8 by Chris Beaven, 16 years ago

Keywords: easy-pickings added
Patch needs improvement: set

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

comment:9 by ctrochalakis, 16 years ago

Patch needs improvement: unset

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

comment:10 by Chris Beaven, 16 years ago

Triage Stage: AcceptedReady for checkin

So they do, promoting.

comment:11 by anonymous, 16 years ago

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

comment:12 by Malcolm Tredinnick, 16 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

by MilosU, 16 years ago

Attachment: decorator.diff added

redirect in case of test failed and user is already authenticated

comment:13 by MilosU, 16 years ago

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?

by milosu, 16 years ago

Attachment: decorator.2.diff added

update - previous patch had one-liner typo

by milosu, 16 years ago

Attachment: decorators.diff added

no permission case handled via template

comment:14 by milosu, 16 years ago

Patch needs improvement: unset

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?

comment:15 by krystal, 15 years ago

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

Is it planned to integrate it in 1.1 or 1.2 ?

comment:16 by DarwinSurvivor, 15 years ago

*Bump*

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

comment:17 by Adam Nelson, 14 years ago

Patch needs improvement: set

Patch needs to be updated to current trunk.

by django@…, 14 years ago

Attachment: decorators.2.diff added

Updated patch for Django 1.2 trunk / RC

comment:18 by Łukasz Rekucki, 14 years ago

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.

comment:19 by Harro, 13 years ago

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)

comment:20 by alex_dev, 13 years ago

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.

comment:21 by Tom Christie, 13 years ago

Cc: Tom Christie added

comment:22 by Gabriel Hurley, 13 years ago

Component: Contrib appscontrib.auth

comment:23 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Bug

comment:24 by Jacob, 13 years ago

Easy pickings: set

by narsilou, 13 years ago

Attachment: decorators.3.diff added

Version using PermissionDenied as suggested in comment 20

comment:25 by narsilou, 13 years ago

Patch needs improvement: unset

by trez, 13 years ago

Attachment: documentation.diff added

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

comment:26 by anonymous, 13 years ago

Needs documentation: unset
UI/UX: unset

by Roald de Vries <roald@…>, 13 years ago

Attachment: permission_required.diff added

comment:27 by Roald de Vries <roald@…>, 13 years ago

Keywords: dceu2011 added

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

by Sasha Romijn, 13 years ago

Tiny fix on Roalds version: removed old commented out code

comment:28 by Sasha Romijn, 13 years ago

Triage Stage: AcceptedReady for checkin

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.

comment:29 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: assignedclosed

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.

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