get_objects_for_user behavior change

[xordoquy]

I tried running Django REST Framework test suite against the django-guardian 1.3 and we have a failing test case that was passing with 1.2.5

https://travis-ci.org/tomchristie/django-rest-framework/jobs/65945259
which corresponds to this test: https://github.com/tomchristie/django-rest-framework/blob/master/tests/test_permissions.py#L373

By investigating a bit, it seems the get_objects_for_user has changed.
For this test in 1.2.5 it returned an empty list, while it's now - 1.3 - a non empty list

[k4nar]

I have the same issue with 1.3.

It comes from the option accept_global_perms introduced by #293. If I set it to False it works as in 1.2.5.

I don't really understand if it comes from a misunderstanding of global permissions from my side, or from a shifting of paradigm from Guardian. Either way, I don't think we will be the only ones with this issue.

[xuhcc]

+1
New behavior breaks filtering when using DjangoObjectPermissionsFilter from DRF.

[v-erena]

If accept_global_perms is set to true, then all assigned global permissions will also be taken into account.

1. Scenario 1: a user has view permissions generally defined on the model 'books' but no object based permission on a single book instance.
   • If accept_global_perms is true: List of all books will be returned
   • if accept_global_perms is false: list will be empty
2. Scenario 2: a user has view permissions generally defined on the model 'books' and also has an object based permission to view book 'Whatever'.
   • If accept_global_perms is true: List of all books will be returned
   • if accept_global_perms is false: list will only contain book 'Whatever'
3. Scenario 3: a user only has object based permission on book 'Whatever'
   • If accept_global_perms is true: list will only contain book 'Whatever'
   • if accept_global_perms is false: list will only contain book 'Whatever'
4. Scenario 4: a user does not have any permission
   • If accept_global_perms is true: emtpy list
   • if accept_global_perms is false: emtpy list

This is how I interpreted the global permission vs object based permission handling in guardian. The flag accept_global_perms is also used in other functions. But maybe I am wrong?

[k4nar]

Your scenarios are right, although it gets a little bit more complicated when group permissions are involved.

My experience with Django permissions is that you use either global or per-object permissions. However, it can be useful to add global permissions to a group, in order to say "any user in this group should have that permission for every object related to this group". That way you can update the permissions automatically.
Before 1.3 doing that was working fine because only the object permissions were checked. However, now get_object_for_users will always return all the instances of the model as the group has the global permission.

The test case provided by @xordoquy is failing because a group is created with all global permissions (https://github.com/tomchristie/django-rest-framework/blob/master/tests/test_permissions.py#L274-L287). This is another example of a usage were there is a clear distinction between global and per-object permissions.

I think that accept_global_perms can be useful for some people, but it should be either False by default, or have its default in the settings.

[tomchristie]

Addressing this downstream in encode/django-rest-framework#3165

I think the new behavior probably makes sense - I believe when I originally came to integrating a guardian filter with REST framework that was what I was first expecting. However either could be reasonable.

Certainly it's subtle so worth documenting well.

I think that accept_global_perms can be useful for some people, but it should be either False by default, or have its default in the settings.

I guess that would be my preference too, given that it appears to be a behavioral change, but equally it's kinda too late now that 1.3 is released, so {shrug}.

WRT integration with REST framework - sticking with the existing behavior so as not to mess with folks upgrading existing deployments.

[edufelipe]

👍 I also think changing the behavior is counterintuitive at best. Got bit by the same issue when upgrading to 1.3 to get Django 1.8 compatibility.

[brianmay]

So if I understand this correctly, this is a documentation issue, correct? i.e. we should keep the current behavior and document it? How do we move forward on this?

[ghost]

It's been a while since this was closed, but I still missed this, and took a while to figure out I needed accept_global_perms=False as this is counter-intuitive IMHO (guardian is an object level permission library).
I disagree that this should be left as is and not changed in later versions, but if so, at least have an explicit example for this in the api docs / examples please.

[brianmay]

IMHO probably too late to change the defaults now. Having said said, any pull requests to address this issue (e.g. in documentation or examples) appreciated.