Opened 8 years ago

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

Download all attachments as: .zip

Change History (39)

comment:1 Changed 8 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 8 years ago by cbrand@…

Yes, that snippet is exactly what I need.

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

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 8 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

Changed 8 years ago by ctrochalakis

django-auth-decorators-fix

comment:5 Changed 8 years ago by ctrochalakis

  • Has patch set
  • Version changed from 0.96 to SVN

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

Changed 8 years ago by ctrochalakis

django-auth-decorators+tests

comment:6 Changed 8 years ago by ctrochalakis

  • Owner changed from nobody to ctrochalakis
  • Status changed from new to assigned

New patch. Fixes test cases.

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

I think this is the same issue as #6310.

comment:8 Changed 7 years ago by SmileyChris

  • 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 7 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 7 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

So they do, promoting.

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

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 7 years ago by MilosU

redirect in case of test failed and user is already authenticated

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

update - previous patch had one-liner typo

Changed 7 years ago by milosu

no permission case handled via template

comment:14 Changed 7 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 6 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 6 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 5 years ago by adamnelson

  • Patch needs improvement set

Patch needs to be updated to current trunk.

Changed 5 years ago by django@…

Updated patch for Django 1.2 trunk / RC

comment:18 Changed 5 years ago by lrekucki

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 5 years ago by hvdklauw

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 5 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 4 years ago by tomchristie

  • Cc tomchristie added

comment:22 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.auth

comment:23 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:24 Changed 4 years ago by jacob

  • Easy pickings set

Changed 4 years ago by narsilou

Version using PermissionDenied as suggested in comment 20

comment:25 Changed 4 years ago by narsilou

  • Patch needs improvement unset

Changed 4 years ago by trez

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

comment:26 Changed 4 years ago by anonymous

  • Needs documentation unset
  • UI/UX unset

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

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

  • Keywords dceu2011 added

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

Changed 4 years ago by erikr

Tiny fix on Roalds version: removed old commented out code

comment:28 Changed 4 years ago by erikr

  • Triage Stage changed from Accepted to Ready 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 4 years ago by jezdez

  • Resolution set to fixed
  • Status changed from assigned to closed

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