Django-guardian: user.has_perm("perm", obj) behaves unexpectedly

[Dzejkob]

If I use standard user.has_perm("perm") method, then it will return True only, if user has a global permission "perm".
And if user.has_perm("perm", obj) is used, it'll teturn True if user have permission to access this particular object.
But it will return False, even if user has a global permission "perm", which is quite unexpected for me, because I assume, that having global permission should give user access to all objects. Am I right?

[Dzejkob]

I digged out little more and found out, that each permission backend should work independently, so there should be no situation as I described above. But for some reason calling user.has_perm providing object instance, looks to only check object level permissions, and skips the global permission check. I have no idea, what is the reason of that behavior. I'm using Django 1.2.5.

[lukaszb]

Hey, can you paste your _AUTHENTICATION_BACKENDS_ setting?

As you've already read Django documentation, you know that the order of specified backends IS important.

I suppose you have something like following in your application:

AUTHENTICATION_BACKENDS = (
    'guardian.backends.ObjectPermissionBackend',
    'django.contrib.auth.backends.ModelBackend',
)

Or this:

from django.conf import global_settings
AUTHENTICATION_BACKENDS = (
    'guardian.backends.ObjectPermissionBackend',
) + global_settings.AUTHENTICATION_BACKENDS

Just make sure that default backend is specified first.

Can you confirm that this is the issue? If not, please add some more information (maybe you use some other backends as well, or some other app monkey patches that _User.has_perm_ method?).

[lukaszb]

Ok, seems I'm too tired. Order should not affect the _has_perm_ result. So please add more information about application settings you're using. Additionally, you can ensure that guardian works properly by running test suite (_python manage.py test guardian_).

[lukaszb]

Oh, ok, I've read your issue one more time. Default _auth.ModelBackend_ does NOT support _supports_object_permissions_ (that attribute is _False_). According to Django docs, they'd add that support for default backend from 1.4.

So, for your situation, behavior is absolutely correct and expected. Default backend is simply omitted.

You should check global permissions before you check object level permissions in your app. That's simplest solution I can think of.

Closing as _invalid_, however if you'd like to reopen it with new comment - feel free to do so!

[Dzejkob]

Ok, looks like You're right. But not checking global permissions brings some serious difficulties for me. For example in guardian.decorators.permission_required, which, as I assumed, should extend functionality of ordinary django.contrib.auth.decorators.permission_required with additional object level permission check.
Problem is, that I have a working aplication that uses global permissions, and I wanted to add some additional object level permissions to it, so I've changed default permission_required decorators to object level ones from django-guardian, but then users had lost their access rights based on global permissions.

[coagulant]

Looks like an unexpected behavior to me, could you please at least add this case to documentation, because it's not obvious without looking into the code.

As for me, this is a design flaw, resulting in extra check all over the code where I need to verify permission both globally and per-object.

[lukaszb]

@coagulant: it would not be flexible to actually _extend_ decorators from Django's auth. This app intets to implement _object permissions_, not extending original one. I.e. what if you'd like to use some other app that uses guardian and object level permissions only? What if you'd like to use global permissions at admin only, and object level permissions in apps given to regular users? What if app would require both global permission and object level permission for user to be able to perform some action on the object? There are many cases, guardian would not cover all of them.

On the other hand, I can admit that this particular use case, might be more common than the others. For anyone interested in this - please file another issue specifying the requirements. I believe this can be easily achieved without backwords incompatibility - i.e. with new configuration setting.

[Dzejkob]

It would be useful to me, if there was added just another permission_required decorator, that check for both object level permissions and global ones. That would cover my issue.

[lukaszb]

@Dzejkob, can you please check out last commit and say if this would be enough (I've added flag for accepting global permissions). Also, let me know if extending docstring at decorator itself is enough, or should I add more examples/more descriptive docs?

Note: commit is at new branch: _feature/add-accept_global_perms-flag-for-decorators_

[Dzejkob]

@lukaszb Yes, I've installed new branch version of Guardian into my project, added parameter in permission_required decorators accept_global_perms=True, and it seems to work as it should. So thank you for making this feature. I think it's good idea to merge it into the trunk.
About docs, they're quite clear to me, so I think it's enough.

[lukaszb]

This was fixed long time ago.

[xoen]

Hello, I found this issue as I'm trying jango-guardian and I found this behaviour buggy/very confusing.

I added a global permission (let's call it view_user) to a user (let's call it joe) and then checked if Joe can view a specific user (let's call it other_user) and surprisingly it returned False.

joe = User.objects.get(username="joe")
other_user = User.objects.get(username="other_user")
assign_perm("myapp.view_user", joe)
joe.has_perm("myapp.view_user") # True as expected
joe.has_perm("myapp.view_user", other_user) # False, whaaaat?

Now, I'm not using the permission_required decorator explicitly as my view sets are "automatically" checking the permissions because of my REST_FRAMEWORK/DEFAULT_PERMISSION_CLASSES and AUTHENTICATION_BACKENDS in settings.py. How do I tell guardian to behave as expected then?

(Apologies if I'm missing something really stupid, this is kind of day 2/3 in djangoland)

[xoen]

Sorry for the noise today, even more interesting/confusing that guardian.shortcuts.get_objects_for_user() honours the auth/global permissions by default (accept_global_perms).

accept_global_perms: if True takes global permissions into account. 
[...]
Default is True.

[doganmeh]

Although, this issue was closed in favor of another, there are a lot of insights here, so I am writing here to re-stir the conversation. Zen of the Python says:

There need to be one --and preferably only one-- obvious way to do it.

And to me that is to _fallback to global when local (object level) is not specified_. The order does not change the result (since Django returns False if obj is not None), but may change the performance.

I do agree that having double checks all over the code is not a good design, and in some sense against the DRY principle too. However, guardian implementing functionality that is available within Django default backend is not DRY either. That Django allowing multiple backends, and checking one by one in order indicates that backends are supposed to play together, not replace one another. If this is right,then Django refusing to check globals when an obj is not None is _wrong_. If Django ignored the obj, it could be the global fallback for the object checkers.

I think we should open a ticket with Django, asking whether, and how, authorization backends play with each other, and then go from there.

[doganmeh]

With that being said, I think Django changing its behavior is quite unlikely (although I do still think we should ask), so the status quo indicates that, guardian needs to evolve to be able to provide both depending upon which one is desired (which might be set either via settings or an argument to the function). But the default behavior I think should be the _local fallback to global when false_ accross the board.

[doganmeh]

I would rather love it if Django preferred a tri-state permission system: True, False, and None. In that case, local checkers could _override_ the globals via False; and via None each backend could get to say: "I don't know it, ask the next in line". In that case, Django would stop checking after getting a True or False in one of the backends, and stop wasting processing power.

This would give each backend more power: can give a definitive answer, or refer to others.

[airstandley]

@doganmeh Django does implement a tri-state permissions system (at least as of 1.10). The options are True, None, and raising PermissionDenied. Doing the later will cause Django to stop checking and return False.

As to the issue at hand, I believe it is a Contrib.Auth issue. That is the backend that deals with 'global permissions'. The problem is that they have established an indirect convention that has_perm with obj=None only checks 'table permissions' and has_perm with obj only checks 'row permissions'. They are unlikely to change that, but _should_ be open to extending their API to support checking both.

(At least I would hope they would be open to adding behavior to check both. It seems a clear flaw in their system.)

What would be your suggestions for the 'check both' API? The best I can think of is a kwarg.

[doganmeh]

I was talking about an explicitly tristate system where there would be a NullBooleanField on the Permission table. True, if you take the lack of True (or lack of the permission) as a None it could be considered as tristate. In that case permissions not specified by guardian should be taken as None, and the decision should be delegated to the global. If I had a choice, though, I would take the explicit design.

[doganmeh]

@airstandley I guess you mean an addition of a kwarg to user.has_perm such as:

def has_perm(self, perm, obj=None, fallback=False)

I think that would help. In the case when fallback=True, guardian would call Django backend and return the global. If I had a choice, though, I would prefer the fallback to Django be naturally handled by the framework, not by hijacking its internals.

[airstandley]

@doganmeh Sorry I misspoke. That should have read "True, False, and raising PermissionDenied". Not:

The options are True, None, and raising PermissionDenied

The later is how I think of the returns in my head...

I think I may have also misunderstood what you meant by

I would rather love it if Django preferred a tri-state permission system

To clarify, you were talking about the Permission Model implementation specifically and not the system as a whole?

My point was that the auth backend system does allow the exact short-cutting you described (At least for the has_perm, has_module_perms api). It is explicit, if not straightforward:
Any backend can either make a decision itself (return True or PermissionDenied), or delegate the decision to the next backed in the chain (return False). That is an implementation specific decision, and not a quality of the system itself.

An explicit ObjectPermissionBackend would be an issue for my project, so I would choose not to be explicit. ("Explicitness" makes integration with other permission checking backends cumbersome.) I suspect that, like you, others would prefer it be explicit. So having the 'explicitness' as setting makes sense to me.

[airstandley]

@doganmeh
So about the kwarg.

First off I keep worrying we are not on the same page as to how the behaviour of auth's backend system was intended to work. So to be clear:
My understanding is that the intention was that backends work together. If an app wanted to use auth's Permission system (ie 'global permissions', although technically they would be 'table permissions' but potatoe, potato) it would install the auth ModelBackend in AUTH_BACKENDS. If that app wanted to use guardian's ObjectPermission system (ie 'row permission') it would isntall guardian's ObjectPermissionBackend in AUTH_BACKENDS.
ModelBackend would handle global permission.
ObjectPermissionBackend would handle object permissions.
if a user only had an object permission, ModelBackend would never return True for has_perm. If a user only had a global permission ObjectPermissionBackend would never return True for has_perm. In both cases a call to user.has_perm(perm, obj) should still return true, as the user does have the permission for _one_ of the installed backends. (This is were we hit a problem because ModelBackend fails on this account)

Ok so given all that.

In an ideal world where compatibility was not an issue, I too would prefer a simple change to contrib.auth's ModelBackend. ModelBackend.has_perm(user_obj, perm, obj=None) should return True if the user has the 'global permission' specified by perm; regardless of whether obj is None or not.

However compatibility is an issue, which is why I ask if you have a solution to the issue.

The only _backwards compatible_ solutions I can think of are either add a kwarg to the API or add an AUTH setting.

So kwargs:
Let's call it obj_shortcut because that's the best I can come up with off the top of my head. object_shortcut would default to True. If object_shortcut is True then backends should behave as the ModelBackend does now: they should _only_ check 'table/global' permissions if obj is None, otherwise they should _only_ check 'row/object' permissions. However if object_shortcut is False then backends should behave as we would both prefer: they would check _both_ global and object permissions when object was not None. Then add-ons like Guardian could always supply a mixin with a has_perm method with object_shortcut=False as the default. user.has_perm(perm, obj, object_shortcut=False) would correctly return True if the Permission represented by perm was assigned to User, while legacy calls to user.has_perm(perm, obj) would continue to return False.

A setting would have the same result but would basically result in a switch in ModelBackend._get_permissions.

if not user_obj.is_active or user_obj.is_anonymous or obj is not None:
    return set()

would become something like:

if not user_obj.is_active or user_obj.is_anonymous or (obj is not None and legacy_behaviour_setting_is _on):
    return set()

Not sure which is better. I think a setting is cleaner, but I'm sure the Django devs would be more qualified to determine that.

Point is the issue is possible to fix, and I strongly believe that the issue is a Django bug, not Guardian.

[doganmeh]

@airstandley You are right, False or None does not matter. They both mean I don't know. Eventually if no one knows, permission is not granted.

I am generally on the same page with you, although django forcing backends to behave one way or another seems to degrade loosely coupledness. I think it should have mechanisms to allow each backend get the information they needed to get their jobs done. I am with you on the keyword argument(s) though, but not just forcing one way or another.

This conversation seems like a waste of time, but it helped me clarify to myself that there is not permission granted in guardian is not a denial, it is just lack of information.

Anyways, I made a pull request #546. Please check it out and let me know what you think.

[doganmeh]

I created a ticket with django: https://code.djangoproject.com/ticket/29012, and wonder what would they say.

[doganmeh]

Seems there were some other tickets with django: https://code.djangoproject.com/ticket/20218

I closed mine.

[airstandley]

@doganmeh
I like the direction you are headed with #546. If it would help, I should have time to look over the rest of the units and write some tests for these fallbacks this weekend.

On a tangent. Your comment about "Django forcing backends" to behave a specific way confuses me, but it also gave me a thought. Backends can behave however they would like; my initial concern about Guardian's ObjectPermissionBackend stems from the documentation suggesting it was designed to run alongside Auth's ModelBackend. Guardian could offer multiple backends, one designed to work with ModelBackend, and one designed to work alone. (I.E. one that just checks Guardian's UserObjectPermission/GroupObjectPermission tables, the other checks both UserObjectPermission/GroupObjectPermission tables and Auth's Permission table.)
Personally I prefer the current approach with a setting and kwargs. I think the major downside to a multiple backend approach would be that it becomes unclear how the short-cuts and convenience functions should behave.

[airstandley]

Saw the post to the Django dev mailing list. I hope they accept that, or agree to change the behaviour in an incompatible way. The current limitation on the API is just clunky.

[doganmeh]

Can you support it there? 😊

Sent from Mail for Windows 10

From: airstandley
Sent: Friday, January 12, 2018 12:06 PM
To: django-guardian/django-guardian
Cc: Mehmet Dogan; Mention
Subject: Re: [django-guardian/django-guardian] user.has_perm("perm", obj)behaves unexpectedly (#49)

Saw the post to the Django dev mailing list. I hope they accept that, or agree to change the behaviour in an incompatible way. The current limitation on the API is just clunky.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[doganmeh]

After dealing with this issue for about a week now, I came to more firmly believe that each backend needs to do just one thing, which is object permissions for Guardian. For that to happen Django must treat objs more nicely.

There is the patch I sent to Django for that: https://github.com/django/django/pull/9581 (please comment if you can). Ones that makes it in, we can clean up retrieval of model permissions whereever there is in Guardian, and just make a call to the default backend.