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

[Ramiro Morales]

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

[Ramiro Morales]

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.

[Florian Apolloner]

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)

[Florian Apolloner]

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…

[Florian Apolloner]

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

[virtual]

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.

[Florian Apolloner]

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

[Ramiro Morales]

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

[Ramiro Morales]

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