Opened 9 years ago

Closed 5 years ago

#4617 closed Bug (fixed)

permission_required decorator behaviour is odd

Reported by: cbrand@… Owned by: ctrochalakis
Component: contrib.auth Version: master
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 9 years ago.
django-auth-decorators-fix
django-auth-decorators-plus-tests.diff (3.4 KB) - added by ctrochalakis 9 years ago.
django-auth-decorators+tests
decorator.diff (3.5 KB) - added by MilosU 8 years ago.
redirect in case of test failed and user is already authenticated
decorator.2.diff (3.6 KB) - added by milosu 8 years ago.
update - previous patch had one-liner typo
decorators.diff (4.0 KB) - added by milosu 8 years ago.
no permission case handled via template
decorators.2.diff (1.9 KB) - added by django@… 6 years ago.
Updated patch for Django 1.2 trunk / RC
decorators.3.diff (3.7 KB) - added by narsilou 5 years ago.
Version using PermissionDenied as suggested in comment 20
documentation.diff (560 bytes) - added by trez 5 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@…> 5 years ago.
permission_required_tinyfix.diff (4.4 KB) - added by Erik Romijn 5 years ago.
Tiny fix on Roalds version: removed old commented out code

Download all attachments as: .zip

Change History (39)

comment:1 Changed 9 years ago by anonymous

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 9 years ago by cbrand@…

Yes, that snippet is exactly what I need.

comment:3 Changed 9 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 9 years ago by Jacob

Triage Stage: Design decision neededAccepted

Changed 9 years ago by ctrochalakis

Attachment: django-auth-decorators.diff added

django-auth-decorators-fix

comment:5 Changed 9 years ago by ctrochalakis

Has patch: set
Version: 0.96SVN

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

Changed 9 years ago by ctrochalakis

django-auth-decorators+tests

comment:6 Changed 9 years ago by ctrochalakis

Owner: changed from nobody to ctrochalakis
Status: newassigned

New patch. Fixes test cases.

comment:7 Changed 9 years ago by Matthijs Kooijman <matthijs@…>

I think this is the same issue as #6310.

comment:8 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by ctrochalakis

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 Changed 9 years ago by Chris Beaven

Triage Stage: AcceptedReady for checkin

So they do, promoting.

comment:11 Changed 8 years ago by 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...

comment:12 Changed 8 years ago by Malcolm Tredinnick

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.

Changed 8 years ago by MilosU

Attachment: decorator.diff added

redirect in case of test failed and user is already authenticated

comment:13 Changed 8 years ago by 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?

Changed 8 years ago by milosu

Attachment: decorator.2.diff added

update - previous patch had one-liner typo

Changed 8 years ago by milosu

Attachment: decorators.diff added

no permission case handled via template

comment:14 Changed 8 years ago by milosu

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 Changed 7 years ago by krystal

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 Changed 7 years ago by DarwinSurvivor

*Bump*

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

comment:17 Changed 6 years ago by Adam Nelson

Patch needs improvement: set

Patch needs to be updated to current trunk.

Changed 6 years ago by django@…

Attachment: decorators.2.diff added

Updated patch for Django 1.2 trunk / RC

comment:18 Changed 6 years ago by Ł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.

comment:19 Changed 6 years ago by 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)

comment:20 Changed 6 years ago by 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.

comment:21 Changed 6 years ago by Tom Christie

Cc: Tom Christie added

comment:22 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.auth

comment:23 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: Bug

comment:24 Changed 5 years ago by Jacob

Easy pickings: set

Changed 5 years ago by narsilou

Attachment: decorators.3.diff added

Version using PermissionDenied as suggested in comment 20

comment:25 Changed 5 years ago by narsilou

Patch needs improvement: unset

Changed 5 years ago by trez

Attachment: documentation.diff added

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

comment:26 Changed 5 years ago by anonymous

Needs documentation: unset
UI/UX: unset

Changed 5 years ago by Roald de Vries <roald@…>

Attachment: permission_required.diff added

comment:27 Changed 5 years ago by Roald de Vries <roald@…>

Keywords: dceu2011 added

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

Changed 5 years ago by Erik Romijn

Tiny fix on Roalds version: removed old commented out code

comment:28 Changed 5 years ago by Erik Romijn

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 Changed 5 years ago by Jannis Leidel

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