Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#11124 closed (fixed)

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

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

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 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from ModelAdmin's has_change_permission() and has_delete_permission(self,) don't examine their obj parameter to ModelAdmin's has_change_permission() and has_delete_permission() methods don't examine their obj parameter

comment:2 Changed 5 years ago by ramiro

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

  • Has patch set

comment:4 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 4 years ago by apollo13

  • Cc apollo13 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 Changed 4 years ago by apollo13

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

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 follow-up: Changed 4 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 4 years ago by apollo13

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

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

(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 Changed 3 years ago by ramiro

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.