[Django] #28588: has_perm with superusers hides non-existent permissions
Django
-----------------------------------------+------------------------
Reporter: Paul Hallett | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
the {{{has_perm}}} method on the User model hides non-existent permissions
by always returning True is the user is a superuser:
{{{
# Legit permission
>>> super_user.has_perm('auth.can_add_user')
True
# Totally fake permission
>>> super_user.has_perm('rubbishrubbishrubbish')
True
}}}
Combined with sloppy testing (for example - most devs are superusers) this
can lead to poor assumptions about the integrity of the code.
I'd like to suggest that the {{{has_perm}}} attempts a look up for the
Permission before resolving is the user is a super user or not.
This has already been mentioned as one point in a long list in this
ticket: [https://code.djangoproject.com/ticket/26547].
I can't see what the resolution of the ticket was as it was just closed
with a reason.
--
Ticket URL: <https://code.djangoproject.com/ticket/28588>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Paul Hallett | Owner: moshe nahmias
Type: Uncategorized | Status: assigned
Component: Uncategorized | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+-----------------------------------------
Changes (by moshe nahmias):
* status: new => assigned
* owner: nobody => moshe nahmias
* easy: 0 => 1
Comment:
It seems like a easy enough thing to fix, I will do it if accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:1>
Django
Reporter: Paul Hallett | Owner: moshe nahmias
Type: Uncategorized | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+-----------------------------------------
* component: Uncategorized => contrib.auth
Comment:
I think we can do this in Debug mode (`settings.DEBUG = True`). Then, it
will not affect sanely-deployed sites. It won't affect testing either --
but I think it reasonable to expect that if a project has tests for its
views, they will not all use an administrative user... It will mostly
affect the developers who are testing by running `runserver` on their
local host and surfing the site with their single account.
I'm not sure doing this in Release mode is as valuable -- it would make no
difference for non-superusers, and for superusers, it can only break
something that currently work and should work (the superuser should have
the permission with the typo...).
I'm not sure if this should then count as a bug, a cleanup, or a new
feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Paul Hallett | Owner: moshe
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
From the mailing list:
Florian: "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."
Me: "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."
If a code solution can't be found, the documentation could note this
caveat.
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* owner: moshe nahmias => Carlton Gibson
* has_patch: 0 => 1
Comment:
Mailing list thread: https://groups.google.com/d/topic/django-
developers/ky98JpKJQOQ/discussion
Original PR: https://github.com/django/django/pull/11284
There's a few points on the mailing list and here that tell against
patching this directly.
'Given the philosophy, "complexity is the enemy of security,"...' for a
start — the existing `return True` for superusers is clear as day. The
complication just doesn't seem to be merited.
The suggested patch ends up calling `get_all_permissions()`. This already
raises the DB query count in the test suite but (as Florian pointed out in
the discussion) for third-party backends this may be prohibitive.
Additionally, there is no requirement to implement
`get_all_permissions()`, so it's not at all clear that the suggested test
is adequate. Even if it were, there would be a breaking change in the
default behaviour, which as Shai comments above should work, that a
superuser has a non-existing permission. (Arguably that last could change,
but for what purpose? — Again, not merited here.)
It seems to me that the search for a (DEBUG only?) check here is
misplaced. A programmer wants to catch a typo is a permission name. Fine.
But the way to do that is to use named constants rather than passing magic
strings. Let the interpreter catch it. A runtime check is (IMO) the wrong
place entirely to try and address this. And as Shai said, there's an
expectation ''"that if a project has tests for its views, they will not
all use an administrative user..."'' — I appreciate that doesn't help the
beginner who's doesn't know about named constants and is just testing
locally using a superuser and `runserver` but I don't see how we can help
that. (By the time you're using the permissions system tests are pretty
much required to know you've done it right.)
[https://github.com/django/django/pull/11530 PR implementing Tim's
suggestion just to document the behaviour].
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Paul Hallett | Owner: Carlton
Type: | Gibson
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* version: 1.11 => 2.2
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Paul Hallett | Owner: Carlton
Type: | Gibson
Component: contrib.auth | Version: 2.2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4b32d039dbb59b3c3e76587df5c58150e752d9ac" 4b32d039]:
{{{
#!CommitTicketReference repository=""
revision="4b32d039dbb59b3c3e76587df5c58150e752d9ac"
Fixed #28588 -- Doc'd User.has_perm() & co. behavior for active
superusers.
Equivalent note for PermissionsMixin was added in
d33864ed138f65070049a3ac20ee98e03a1442b9.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Paul Hallett | Owner: Carlton
Type: | Gibson
Cleanup/optimization | Status: closed
Component: contrib.auth | Version: 2.2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b6d8957356fd7f23e806c65657a72f7316d66b61" b6d8957]:
{{{
#!CommitTicketReference repository=""
revision="b6d8957356fd7f23e806c65657a72f7316d66b61"
[2.2.x] Fixed #28588 -- Doc'd User.has_perm() & co. behavior for active
superusers.
Equivalent note for PermissionsMixin was added in
d33864ed138f65070049a3ac20ee98e03a1442b9.
Backport of 4b32d039dbb59b3c3e76587df5c58150e752d9ac from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28588#comment:7>