Hi Carlton,
Thanks for the thoughts. I just wanted to share my opinion on your
options.
1. "Won't Fix"
I have yet to find anywhere the original design decisions were
documented. Based on what I can find, it appears that object level
permissions where a bit of an after-though for the initial auth
framework. Unless we can find the motivation for the original design
decision it seems foolish to leave /unexpected/ behaviour simply because
that's how it has always been.
I understand the motivation for wanting to only check object level
permissions, but I would argue that the design of the API insinuates a
hierarchical behaviour which is part of the reason the current behaviour
is unexpected.
Specifically why use an 'obj' kwarg instead of having two methods?
To me if the API where:
```
has_perm('foo.change_bar')
has_obj_perm('foo.change_bar', bar_obj)
```
I would expect `has_perm` to only deal with model level permissions and
`has_obj_perm` to only deal with object level permissions
To me the current API:
```
has_perm('foo.change_bar', obj=bar_obj)
```
Implies that the method will always check model level permissions and
can /optionally/ check object level permissions.
Having to always call `if user.has_perm('foo.change_bar', obj=bar) or
user.has_perm('foo.change_bar')` both seems ugly, and relies on backend
authors following ModelBackend's example of caching permission look-ups.
Otherwise there may be a performance cost. The AUTH_BACKEND system's
strength is the ability to plug in third-party and custom backends, it
seems dangerous to make assumptions based on ModelBackend unless those
assumptions are clearly documented.
2. Change ModelBackend
Initially this would have been my preferred option. However having given
the issue significant thought, I think that it is important that users
are still given the option of the current behaviour.
Having an API return only object level permissions when called with obj,
while returning only model level permissions when called without obj is
a legitimate use case.
Which leads me to
3. Break out permissions aspects.
I agree this would require some planning for migration. It also requires
discussion on whether the desire would be to separate authentication
from authorization or if the auth framework should just offer more
default options.
However overall I think this is the best option. It leverages the
flexibility of the backend system and offers the option of maintaining
BC if desired.
For example it would make sense to me to plan on restructuring the
auth.backends to something like:
```
ModelAuthenticationBackend(object):
# defines get_user and authenticate
PermissionAuthorizationBackend(object):
# defines get_user_permissions, get_group_permissions,
get_all_permissions, has_perm, has_module_perms
# 'New' behaviour: checks model level permissions /even/ when obj is None
ModelOnlyPermissionAuthorizationBackend(object):
# defines get_user_permissions, get_group_permissions,
get_all_permissions, has_perm, has_module_perms
# Current behaviour: checks model level permissions /only/ when obj is Non
ModelPermissionBackend(ModelAuthenticationBackend,
PermissionAuthorizationBackend):
pass
ModelBackend(ModelAuthenticationBackend,
ModelOnlyPermissionAuthorizationBackend):
pass
```
This offers users more options for their authorization/authentication
needs, and should make migration easier as you could gradually
depreciate the name 'ModelBackend' in favour of something like
'ModelPermissionOnlyBackend'
Additionally if Mehmet wanted to finalize an API that allowed users to
specify a subset of backends to check against, this approach would
support that.
Cheers,
Andrew
On 1/17/2018 2:45 AM, Carlton Gibson wrote:
Hi Mehmet,
Due to the BC issues, this is fairly in-depth.
Having looked at the history, here are my initial thoughts.
The initial issue here is this behaviour from `ModelBackend`:
```
user.has_perm('foo.change_bar', obj)
False
user.has_perm('for.change_bar')
True
```
Although the long-standing behaviour, this is considered _unexpected_.
Ticket is: https://code.djangoproject.com/ticket/20218
There are two related tickets regarding permission-checking in the Admin:
* https://code.djangoproject.com/ticket/13539
* https://code.djangoproject.com/ticket/11383
Currently, I see three options:
1. Close as "Won't Fix", perhaps with a review of the documentation to
see if we
can't clarify/emphasise the behaviour somewhere.
This is the path of least resistance. It conforms to the original
design
decision. It preserves the long-standing behaviour. (Whilst, yes,
some find the
behaviour unexpected, it has a sense; it just depends how you look
at it.)
The objection is to this kind of check:
if user.has_perm('foo.change_bar', obj=bar) or
user.has_perm('foo.change_bar'):
...
* Whilst, granted, it's a little clumsy, there's no reason this
couldn't be
wrapped in a utility function. (There's a suggestion to that
effect on the
Django-Guardian issue tracker[^1]. This seems like a good idea,
simple enough
to live in user code.)
* `ModelBackend` permission lookups are cached[^2] so the
performance worry
here should be negligible.
2. Implement the (straight) backwards incompatible change.
The difficulty here is (we guess) why this ticket has been open so
long.
If we are convinced this is the right way to go — i.e. that the
current
behaviour is in fact **wrong** — then we should go ahead despite the
difficulty.
Working out what that entails is non-trivial. That's why it needs
a decent
discussion and consensus here.
Which leads to...
3. Break out the permissions aspect of `ModelBackend` in order to make it
pluggable in some way. Then allow users to opt-in to a new version
with the
adjusted behaviour.
There is some discussion of this on one of the related tickets[^3].
Again, exactly what the migration path is needs some planning.
I'm not sure what the correct answer is.
[^1]: https://github.com/django-guardian/django-guardian/issues/459
[^2]:
https://docs.djangoproject.com/en/2.0/topics/auth/default/#permission-caching
[^3]: f.f. https://code.djangoproject.com/ticket/13539#comment:16
Kind Regards,
Carlton
--
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-developers+unsubscr...@googlegroups.com
<mailto:django-developers+unsubscr...@googlegroups.com>.
To post to this group, send email to
django-developers@googlegroups.com
<mailto:django-developers@googlegroups.com>.
Visit this group at *MailScanner has detected definite fraud in the
website at "groups.google.com". Do /not/ trust this website:*
https://groups.google.com/group/django-developers
<https://groups.google.com/group/django-developers>.
To view this discussion on the web visit *MailScanner has detected
definite fraud in the website at "groups.google.com". Do /not/ trust
this website:*
https://groups.google.com/d/msgid/django-developers/cab981b8-7dc7-4e9d-9dcc-442b36820cdf%40googlegroups.com
<https://groups.google.com/d/msgid/django-developers/cab981b8-7dc7-4e9d-9dcc-442b36820cdf%40googlegroups.com?utm_medium=email&utm_source=footer>.
For more options, visit *MailScanner has detected definite fraud in
the website at "groups.google.com". Do /not/ trust this website:*
https://groups.google.com/d/optout <https://groups.google.com/d/optout>.
--
*Andrew Standley*
/Senior Software Engineer/
Linear Systems
(909) 899-4345 *225
/astand...@linear-systems.com <mailto:astand...@linear-systems.com> /
--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/cfcefc08-e8a6-71d3-6269-31ccd9eaa52d%40linear-systems.com.
For more options, visit https://groups.google.com/d/optout.
