about ticket 28588- has_perm hide non-existent permissions

[moshe nahmias]

Hi,

I am a python developer and like to use Django for web development.

Since I like the framework I want to contribute back, so I looked at the open tickets to find something I can start with contributing and found ticket 28588.

This ticket is about when checking if the user has permission for some action if the user is super user he/she gets it all the time, even when the permission doesn't exist, and this is not developer friendly because the developer can mistakenly think that everything is fine even when the permission doesn't exist.

As I understand (and correct me if I'm wrong) there should be a discussion about if we want to do this.

If accepted I would like to do this, I think it's an easy enough change for a new contributor like me.

As I understand the ticket the problem is that a developer gets confused on this behaviour (and it's illogical) that the super user is having a permission that doesn't exist.

What do you think? (I think I will discuss my solution or optional solutions after we decide if we want to change this behaviour)

[1] https://code.djangoproject.com/ticket/28588

[Dan Watson]

Seems like maybe it would be more helpful if has_perm logged a note about the permission not existing (probably only in debug), rather than just returning False. In fact, I'd argue it should still return True -- if the permission did exist, the superuser would have it. And there's a backwards-compatibility argument. Think of superusers more as "permissions don't apply to me" than "I have all permissions".

Dan

[Adam Johnson]

I agree with Shai's comment on the ticket, changing it to raise a DoesNotExist when DEBUG=True. I think it's an acceptable compromise between backwards compat and helping find bugs.

--
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+unsubscribe@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/d0430a0f-04d9-484d-b888-bddf6f53ff95%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Adam

[Curtis Maloney]

On 09/25/2017 08:56 PM, Dan Watson wrote:
> Seems like maybe it would be more helpful if has_perm logged a note
> about the permission not existing (probably only in debug), rather than
> just returning False. In fact, I'd argue it should still return True --
> if the permission did exist, the superuser would have it. And there's a
> backwards-compatibility argument. Think of superusers more as
> "permissions don't apply to me" than "I have all permissions".

I agree with the logging... however, I think has_perm should always
return False for non-existent permissions. This will mean any
half-decent level of testing will uncover a typo in a permission name,
since you will never trigger the True state.

This would also be an argument for is_superuser to equate to "has all
the perms" instead of "has_perm always says true".

--
Curtis


>
> Dan

[Florian Apolloner]

I do not think it would be feasible to check existing permissions. For one, not every backend uses the Permission class Django supplies and get_all_permissions can cause performance issues so it should be used sparingly.

Cheers,
Florian

[Tim Graham]

I suppose we can tentatively accept the ticket, but I looked at the code briefly and agree with Florian's assessment. If someone proposes a patch, we can evaluate it, however, I don't see a simple way forward that wouldn't have a security risk or an adverse effect on performance. Given the philosophy, "complexity is the enemy of security," I'd lean toward keeping the permissions checking code simple instead of adding some other logic based on DEBUG.

[Shai Berger]

Can we define a new API on the permission backend, "verify_permission_exists()"
or some such, and just call it if settings.DEBUG and it is provided? That
doesn't seem very complex to me, and doesn't necessarily imply a huge
performance hit (even in DEBUG).

[moshe nahmias]

Florian, now the implementation is if the backend doesn't implement the has_perm we use continue so the code is not checked at all and return False for a regular user.

The solutions suggested here are logging, raise an error on DEBUG = True and return False if the permission doesn't exist (if I missed anything let me know).

​I numbered the options suggested so far for easy reference.

1. ​The way I see it if we use logging the user will have to check if it got an error when working with has_perm, and if the problem started with the programmer is not doing tests well I don't think he/she will check the logs if they have them, and they might not have logs enabled, but it's a nice solution that won't need anything except this change (which i can implement as a function .​

2. Raise an error on DEBUG = True is my favorite, if there is a bug it will jump on the first time the user is working on it when running the app but it's not backwards compatible.

3. Return False if the permission doesn't exist means that we go through the same path as a regular user, since (at least on auth.backends.ModelBackend) we check already if the user is superuser and if so we return all the permissions (I suppose it's only permissions that exist) it means we only need to remove the check at the start to see if the user is superuser.

​I don't think the performance will be that much of a problem, but since you think it might I think i will need to check it and report the results back unless there is a preference for one of the other solutions. either way it will be a good thing to check.​

Thanks,

Moshe

[Florian Apolloner]

Hi,

On Friday, September 29, 2017 at 7:00:41 PM UTC+2, moshe nahmias wrote:

3. Return False if the permission doesn't exist means that we go through the same path as a regular user, since (at least on auth.backends.ModelBackend) we check already if the user is superuser and if so we return all the permissions (I suppose it's only permissions that exist) it means we only need to remove the check at the start to see if the user is superuser.

Removing this check would be highly backwards incompatible for 3rd party permission backends.

​I don't think the performance will be that much of a problem, but since you think it might I think i will need to check it and report the results back unless there is a preference for one of the other solutions. either way it will be a good thing to check.​

There will probably not be a big performance drop for the builtin ones, but we do not know anything about 3rd party (ie ldap etc)

[moshe nahmias]

Thanks for the corrections and input.

If I will keep the check and return False if the permission doesn't exist, will it be OK?

If yes I think I have a good solution, I will return on the check if perm in Permission.all()

So instead of:

return True

we will have:

return perm in Permission.objects.all()

What do you think?

If this seems reasonable I will make the PR in the next few days.

--

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+unsubscribe@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/e14b6ff5-598d-4605-9e76-26df86971366%40googlegroups.com.

[Florian Apolloner]

Hi,

On Monday, October 2, 2017 at 2:24:31 PM UTC+2, moshe nahmias wrote:

If yes I think I have a good solution, I will return on the check if perm in Permission.all()

Where exactly do you propose to do that?  Here: https://github.com/django/django/blob/master/django/contrib/auth/models.py#L263 ? You are not allowed to access the model there since there is no gurantee that an auth backend uses a database backed storage…

Cheers,

Florian