Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#11124 closed (fixed)

ModelAdmin's has_change_permission() and has_delete_permission() methods don't examine their obj parameter

Reported by: Ramiro Morales Owned by: nobody
Component: contrib.admin Version: dev
Severity: Keywords: has_change_permission has_delete_permission ModelAdmin
Cc: Florian Apolloner Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

ModelAdmin's has_change_permission(self, request, obj=None) and has_delete_permission(self, request, obj=None) don't examine their obj parameters, they always check the permission of the request user on the class registered with the ModelAdmin instance.

their docstrings, e.g.

"""
Returns True if the given request has permission to change the given
Django model instance.

If `obj` is None, this should return True if the given request has
permission to change *any* object of the given type.
"""

are slightly misleading.

This applies to trunk and 1.0.x

Attachments (1)

11124-ModelAdmin_has_x_permission_docstrings-r10793.diff (1.9 KB ) - added by Ramiro Morales 15 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by Ramiro Morales, 15 years ago

Summary: ModelAdmin's has_change_permission() and has_delete_permission(self,) don't examine their obj parameterModelAdmin's has_change_permission() and has_delete_permission() methods don't examine their obj parameter

comment:2 by Ramiro Morales, 15 years ago

Now that I see http://groups.google.com/group/django-users/browse_thread/thread/4c927ef1a679e417?hl=en I realize these methods are placeholders for user-provided sub-classes to implement these per-model instance checks.

Will upload a patch for the docstrings (to make clear these methods are intended to be overridden for users with advanced needs just like it is done in other ModelAdmin methods' docstrings and to make clear the obj argument isn't used in the base implementation) shortly.

comment:3 by Ramiro Morales, 15 years ago

Has patch: set

comment:4 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Florian Apolloner, 15 years ago

Cc: Florian Apolloner added

I guess the correct way (after #12462 got applied) would be something like this:

def has_change_permission(self, request, user, obj=None):
  opts = self.opts
  perm = opts.app_label + '.' + opts.get_change_permission()
  if obj is None:
    return request.user.has_perm(perm)
  else:
    return request.user.has_perm(perm, obj)

comment:6 by Florian Apolloner, 15 years ago

I should also notice, that we would need the if obj is None clause as long as there are backends which are allowed to not support obj as param…

comment:7 by Florian Apolloner, 15 years ago

Okay ignore my previous two posts, it was late yesterday :/ This is the current situation:
The ModelBackend kicks in even if an obj is passed in; which is kinda suboptimal I guess as according to the docs Backends will return False if not supporting rowlevel perms.

After that got fixed (#12462) the !Modelbackend will return False always for obj!=None. This means that as soon as a user updates his ModelAdmin to supply obj to the backends he will have to use a rowlevel-backend (Those who don't want to use row level perms could write a dummy backend which passes the check to the ModelBackend but without passing the obj to the ModelBackend).

comment:8 by virtual, 14 years ago

New Django user here, I ran into this problem. The proper fix to this, IMHO, is to use some logic like this:

   if **the user object supports two arguments***:
      return request.user.has_perm(perm, obj)
   else:
      return request.user.has_perm(perm)

The key is a test to figure out if the user object supports the two-argument version of perm.

We could test it by trying to call the two-argument version, and if an exception is thrown, reverting to the one-argument version. This of course would require checking to make sure the exception was thrown by our initial call, re-raising exceptions that come from user code.

But we don't have to be that complicated. There is a built-in Python module to solve this problem, the inspect module. In particular, inspect.getargs. So I am thinking something like this:

   args, varargs, varkw = inspect.getargs(user.has_perm)
   if len(args) >= 2 or varargs is not None:
      return request.user.has_perm(perm, obj)
   else:
      return request.user.has_perm(perm)

The only concern is whether inspect is portable to all the Python environments Django must support (e.g. Jython). But that is why testing frameworks were invented, we just need to write a test with a single-argument-only backend and see if the test fails in any supported environment :)

The really proper fix to this is to deprecate the one-argument call and unconditionally call the two-argument version. It should be put on the list for Django 2.0 or the next release with backwards-compatibility-breaking changes.

in reply to:  8 comment:9 by Florian Apolloner, 14 years ago

Replying to virtual:

The really proper fix to this is to deprecate the one-argument call and unconditionally call the two-argument version. It should be put on the list for Django 2.0 or the next release with backwards-compatibility-breaking changes.

That's already handled by the current code (see: http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/models.py#L148 and http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/__init__.py#L10). Actually I don't see what your "fix" is supposed to fix, the user currently supports an optional obj parameter for all methods (at least those where it makes sense).

comment:10 by Ramiro Morales, 14 years ago

Resolution: fixed
Status: newclosed

(In [15126]) Fixed #11124 -- Expanded docstrings of the ModelAdmin has_{change|delete}_permission methods to make it clear they can be overriden to implement per-instance permission checks. Refs #12642.

comment:11 by Ramiro Morales, 14 years ago

(In [15179]) [1.2.X] Fixed #11124 -- Expanded docstrings of the ModelAdmin has_{change|delete}_permission methods to make it clear they can be overriden to implement per-instance permission checks. Refs #12642.

Backport of [15126] from trunk.

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