Proposal to extend support for object permission

[Tobias Bengfort]

Hi,

as far as I understand the documentation[1], django's architecture
supports object permissions, but external apps like django-rules or
django-guardian are supposed to provide actual implementations.

I think this approach is great. However, I believe that there are some
key points where django core should provide more tooling so that the
external apps can concentrate more on building the actual authentication
backends.

# DefaultObjectBackend (#20218)

ModelBackend always returns `False` if an object is passed. Many
developers expect object permissions to be a superset of model
permissions. A DefaultObjectBackend could be added that does exactly
that. By implementing it as a separate backend it is fully optional and
does not break backwards compatibility.

```
class DefaultObjectBackend:
def authenticate(self, username, password):
return None

def get_user_permissions(self, user, obj=None):
if obj is None:
return set()
return user.get_user_permissions()

def get_group_permissions(self, user, obj=None):
if obj is None:
return set()
return user.get_group_permissions()

def get_all_permissions(self, user, obj=None):
if obj is None:
return set()
return user.get_all_permissions()

def has_perm(self, user, perm, obj=None):
if obj is not None:
return user.has_perm(perm)
```

# PermissionRequiredMixin.get_permission_object()

Currently, external apps have to ship their own mixin because the
default one does not support object permissions. A new method
`get_permission_object()` could be added. This is what django-guardian
does. However, in django core it should return None by default for
backwards compatibility (In django-guardian it falls back to
`get_object()`).

```
class PermissionRequiredMixin:
…

def get_permission_object(self):
return None

def has_permission(self):
perms = self.get_permission_required()
permission_object = self.get_permission_object()
return self.request.user.has_perms(perms, obj=permission_object)
```

# has_perm template tag

Just as with PermissionRequiredMixin, the current way to check for
permissions in templates does not support object permissions. A new
has_perm template tag could be added that simply calls
`user.has_perm()`. This approach is used by django-rules.

```
@register.simple_tag
def has_perm(perm, user, obj=None):
return user.has_perm(perm, obj)
```

# Add a BaseBackend

Currently, writing an authentication backend is not as straight-forward
as it could be. I propose to add a BaseBackend that implements some sane
defaults:

```
class BaseBackend:
def authenticate(self, username, password):
return None

def get_user_permissions(self, user, obj=None):
return set()

def get_group_permissions(self, user, obj=None):
return set()

def get_all_permissions(self, user, obj=None):
perms = set()
perms.update(self.get_user_permissions(user, obj=obj))
perms.update(self.get_group_permissions(user, obj=obj))
return perms

def has_perm(self, user, perm, obj=None):
perms = self.get_all_permissions(user, obj=obj)
return perm in perms
```

All of these changes can easily be implemented and have little to no
backwards incompatibility. However, they would provide a much stronger
base for object permission in core. I am not sure whether this is
desired from either the core team or the developers of external
authentication apps though.

What do you think? Should I proceed writing pull requests for the
proposed changes?

tobias


[1]:
https://docs.djangoproject.com/en/2.1/topics/auth/customizing/#handling-object-permissions

[Carlton Gibson]

Hi Tobias, 

Thank you for your effort here. Sorry for the slowish response: it's just a question of getting round to it. 

The basic issue here is people what to avoid this: 

    if user.has_perm('foo.change_bar', obj=bar) or user.has_perm('foo.change_bar'):

        ...

(Or they find it surprising or ...) 

Last time this came up (around #20218) there weren't very many or strong opinions, so I came down against a breaking change. 

(I don't mind the existing permissions: they've been the same ≈forever, so I see no reason to change.) 

What you've done in your two PRs is nice I think: 

* https://github.com/django/django/pull/10601

* https://github.com/django/django/pull/10600

* Added a `DefaultObjectBackend` on an opt-in basis which gives the alternate behaviour. 

* Added the `get_premission_object()` hook, so that can be overridden. 

Both of these have small footprints and no breaking changes. So I think +1 yes. 

My only concern thus far is bringing out the change well enough in the release notes and docs. 

(Split between the two PRs I'm not sure it quite does that.) 

Kind Regards,

Carlton

[Tobias Bengfort]

On 08/11/2018 11:43, Carlton Gibson wrote:
> My only concern thus far is bringing out the change well enough in the
> release notes and docs.
> (Split between the two PRs I'm not sure it quite does that.)

My impression is that authentication backends and object permissions
have already been underdocumented before these changes. I would like to
help improving the documentation in general. However, the content and
structure of the documentation could be very different depending on
which of my changes get accepted. So my idea was to rework the
documentation afterwards.

tobias

[Carlton Gibson]

OK, makes sense. 

My current thinking is to Accept both. Perhaps you could put docs changes you'd make in a third PR (or ticket if you like, to discuss the outline)?

(Maybe one PR with three commits makes it easier to review as a whole.) 

Thanks for the input! 🏆

[Tobias Bengfort]

On 08/11/2018 12:00, Carlton Gibson wrote:
> Perhaps you could put docs changes you'd make in a third PR (or
> ticket if you like, to discuss the outline)? (Maybe one PR with three
> commits makes it easier to review as a whole.)

I created a pull request with some changes, mostly related to documentation:

https://github.com/django/django/pull/10636

This is still pretty rough. I feel like I overdid it. But I tried to
split it into different commits so that the individual changes can be
cherry-picked.

tobias

[Hanne Moa]

How object permissions work in the admin is also a bit of an
undocumented surprise. You need 'change_*' on the model, not just on a
specific object in order to to be able to do just about anything at
all. 'delete_selected` cannot check for object permissions etc.

I just started using django-guardian and it's the first time ever (in
ten years) I've needed to write my own subclass of ModelAdmin. Its
get_queryset() checks for whether the user has access to an object
directly via an object permission, but it also calls an optional
method since I try to use as few object permission as possible. The
plan is that if you have access to, say, a bookcase, you have the same
access to all the books therein without being explicit about it, with
the usual caveat about lousy analogies being lousy.

> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0dd42320-0761-ef5d-4877-9aba8b814134%40posteo.de.
> For more options, visit https://groups.google.com/d/optout.