Re: about ticket 28588- has_perm hide non-existent permissions

[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

-- 
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+unsubscr...@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/c2d685bf-e4ce-4cec-a3f8-628ee729895c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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.

On Sat, Sep 30, 2017 at 1:45 PM, Florian Apolloner 
wrote:

> 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)
>
> --
> 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+unsubscr...@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
> 
> .
>
> For more options, visit https://groups.google.com/d/optout.
>

-- 
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+unsubscr...@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/CACf8pw7joMt29d%2Br9y3E6Kxfj9Jvm3g5J%2B1gqw6vmDWz1H5h2Q%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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) 

-- 
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+unsubscr...@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.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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


On Thu, Sep 28, 2017 at 4:10 PM, Shai Berger  wrote:

> 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).
>
> On Thursday 28 September 2017 15:50:04 Tim Graham wrote:
> > 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.
> >
> > On Wednesday, September 27, 2017 at 9:48:24 AM UTC-4, Florian Apolloner
> >
> > wrote:
> > > 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
> > >
> > > On Sunday, September 24, 2017 at 4:56:40 PM UTC+2, moshe nahmias wrote:
> > >> 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
>

-- 
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+unsubscr...@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/CACf8pw6j0sGjDEkNJuxtwDbtmrWiTHrfVJWJW4uCJLrgXTx2kA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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).

On Thursday 28 September 2017 15:50:04 Tim Graham wrote:
> 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.
> 
> On Wednesday, September 27, 2017 at 9:48:24 AM UTC-4, Florian Apolloner
> 
> wrote:
> > 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
> > 
> > On Sunday, September 24, 2017 at 4:56:40 PM UTC+2, moshe nahmias wrote:
> >> 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

Re: about ticket 28588- has_perm hide non-existent permissions

[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.

On Wednesday, September 27, 2017 at 9:48:24 AM UTC-4, Florian Apolloner 
wrote:
>
> 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
>
> On Sunday, September 24, 2017 at 4:56:40 PM UTC+2, moshe nahmias wrote:
>>
>> 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
>>
>
>

-- 
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+unsubscr...@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/7e34b477-46ff-4f48-a45a-e4d0f8132f54%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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


--
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+unsubscr...@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/ee6d4c13-97c8-09b5-49dd-b92bbab15616%40tinbrain.net.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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.

On 25 September 2017 at 11:56, 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".
>
> Dan
>
> On Sunday, September 24, 2017 at 10:56:40 AM UTC-4, moshe nahmias wrote:
>>
>> 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
>>
> --
> 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+unsubscr...@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

-- 
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+unsubscr...@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/CAMyDDM3VDPQh7kUgENk9vjntG3k%3D9EDucxfv8Heuqn9jMxpryw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: about ticket 28588- has_perm hide non-existent permissions

[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

On Sunday, September 24, 2017 at 10:56:40 AM UTC-4, moshe nahmias wrote:
>
> 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
>

-- 
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+unsubscr...@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.