Skip to content

Commit

Permalink
Consider user global permissions when checking for has_perm
Browse files Browse the repository at this point in the history
  • Loading branch information
@bellini666
bellini666 committed Jun 1, 2021
1 parent f1b8f36 commit a96ff32
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions graphene_django_plus/models.py
View file
Expand Up @@ -58,4 +58,10 @@ def has_perm(self, user, perms, any_perm=False, checker=None):
perms = [perms] if isinstance(perms, str) else perms

f = any if any_perm else all

# First try to check if the user has global permissions for this
# Otherwise we will check for the objeect itself bellow
if f(user.has_perm(p) for p in perms):
return True

return f(checker.has_perm(p, self) for p in perms)

3 comments on commit a96ff32

@Rainshaw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I am wondering why this should be changed like this. should we use user.has_perm(p, self) for p in perms?
As in docs and code of Django, it is the auth backend should be responsible for this, and for obj permission, IMO should use user.has_perm(p, self)

@bellini666
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @RainshawGao .

The use case here is for global permissions. For example, when you define a model like this:

class Foo(GuardedModel):
    class Meta:
        permissions = [
            ("can_edit", "Can edit this object"),
        ]

You can use guardian to give foo.can_edit permission to a single user to a single object.
But, you can also give the user global permission to foo.can_edit, which should translate that that user has foo.can_edit to all objects regardless.

This is also to make sure that this works like the for_user query because, in there, the get_objects_for_user will respect that global permission and this function wasn't.

There is a documented issue here regarding that: django-guardian/django-guardian#380 . Probably there'll be an easier way to do this because, right now, the checker isn't respecting the global permissions

@Rainshaw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, got it, thank you! 😊

Please sign in to comment.