Django-guardian: Global permissions vs object permissions

[brianmay]

There seems to be a number of bugs due to some routines using global permissions, and others only using object permissions. Will close the related bugs and reference them here.

[Pesticles]

Yes to this!

permission_required_or_403 has the accept_global_perms parameter, but there's no way in a view to do the same thing with get_obj_perms

I propose a get_obj_perms_with_global tag which includes global perms.

I don't know if you've already started down this road, but I'll start working up a patch to do it; if you've already made a start feel free to stop me :)

P.S. I think there's an argument to be made that the default behaviour should be to include global permissions, it seems to me that when you ask "can user X do Y to object Z" that implies checking all of user/object, group/object, user/global, group/global permissions. But that might just be because of the the way I use it.

[Pesticles]

Having said all that, it is pretty straight forward to check global permissions separately...Hrmmm.

[aidanlister]

Pesticles: there's enough issues on guardian as testament to how confusing this is. We get bit by it every six months ... if request.user.has_access('foo', obj) should be true if they have global permissions but not object level permissions. Writing the check twice is yucky.

What needs to happen to get this changed?

[brianmay]

I think somebody needs to write a pull request... Ideally in such a way that it doesn't break existing code (if this is sane and feasible or not I don't know).

[rvk86]

Old issue, but I don't see a pull request yet. Any news on this one? I'm running into the same behavior, which I think is not very predictable. Thanks a bunch!

[doganmeh]

I added some comments to #49 since that had more insight on the issue of whether and how to fallback to globals when object level fails. I am not sure if closed issues raise notification, so I am adding this here.

[doganmeh]

Depending on the design decisions being made I have intent to work on some of these issues.

[airstandley]

I would be happy to help with these issues.

[doganmeh]

327 seems fixed to me.

```
brandon=Person.objects.get(first_name='BRANDON')
brandon.is_superuser
False

school=School.objects.get(pk=1)

get_users_with_perms(school, attach_perms=True, with_superusers=False, with_group_users=False)
{}

assign_perm('add_school', brandon, school)

get_users_with_perms(school, attach_perms=True, with_superusers=False, with_group_users=False)
{: ['add_school']}

mehmet=Person.objects.get(first_name='Mehmet')
mehmet.is_superuser
True

get_users_with_perms(school, attach_perms=True, with_superusers=True, with_group_users=False)
{: [], : ['add_school', 'change_school', 'delete_school']}

assign_perm('add_school', mehmet, school)

get_users_with_perms(school, attach_perms=True, with_superusers=True, with_group_users=False)
{: [], : ['add_school', 'change_school', 'delete_school']}

get_users_with_perms(school, attach_perms=True, with_superusers=False, with_group_users=False)
{: ['add_school'], : ['add_school']}

[doganmeh]

On #155:

Addition of the following two lines is requested to get_obj_perms_field_choices (self):

        if self.exclude_default:
            choices = list(set(choices).intersection(self.obj._meta.permissions))

to exclude the Model's default permissions. First, I am not clear on how intersection would do that. Second, that does not seem to be related to object vs model permissions. Please correct me if I am missing anything.

But, while looking into that, I noticed there needs to be a separation of object vs model permissions in:

    def save_obj_perms(self):
        """
        Saves selected object permissions by creating new ones and removing
        those which were not selected but already exists.

        Should be called *after* form is validated.
        """
        perms = self.cleaned_data[self.get_obj_perms_field_name()]
        model_perms = [c[0] for c in self.get_obj_perms_field_choices()]

        to_remove = set(model_perms) - set(perms)
        for perm in to_remove:
            remove_perm(perm, self.user, self.obj)

        for perm in perms:
            assign_perm(perm, self.user, self.obj)

It seems like, it is saving at the object level whatever permission is missing, while in my opinion, any permission available at the model level should be skipped, or an option to that end be provided.

[doganmeh]

I think a global setting such as GUARDIAN_FALLBACK_TO_MODEL=False and an argument to applicable methods such as fallback_to_model=False needed to properly delineate object permissions and model permissions. Default value of False ensures backward compatibility. If either of the global or the argument setting is True, fallback should take place.

[airstandley]

+1 for GUARDIAN_FALLBACK_TO_MODEL.

Per the discussion on #49 I think that setting might would also work well to control whether the ObjectPermissionBackend was explicit or not.

[doganmeh]

I created a pull request (#546). It allows checker functions in core.py option to fallback to (or include) model level permissions. Other modules also need to be reviewed. I plan on reviewing them as I use them. If anyone would like to jump in, most welcome.

[doganmeh]

Seems #327 no longer exists. #332 is fixed by the pull request above if accepted. There is just one side issue related to #155 (mentioned above) left. If that was also taken care of (or moved out to a separate issue), this issue could be closed, I think.

[alfonsrv]

Global permissions are still not respected for object level permission if set, am I wrong here?

u = User.objects.get(id=1)
c = Client.objects.get(id=1)
assign_perm('core.change_client', u)
u = User.objects.get(id=1)  # reloading user for perm refresh
u.has_perm('core.change_client')  # True
u.has_perm('core.change_client', c)  # False
u.has_perm('change_client', c)  # False
get_perms(u, c)  # []

And what does setting GUARDIAN_GLOBAL_PERMISSIONS_CARRY_OVER or FALLBACK_TO_MODEL do? Neither seem to be documented.

[alfonsrv]

Wouldn't it be sufficient to add this to core.py?

get_user_perms:126 which seems to be the center of all user permission checking; alternatively triggerable using a boolean flag

def get_user_perms(self, obj):
    # ....
    user_global_perms = Permission.objects.filter(content_type=ctype)\
                                        .filter(user=self.user)\
                                        .values_list('codename', flat=True)
    user_object_perms = user_perms_qs.values_list("codename", flat=True)
    user_perms = list(chain(user_global_perms, user_object_perms))
    return user_perms