#28588: User.has_perm() with superusers hides nonexistent permissions
-------------------------------------+-------------------------------------
Reporter: Paul Hallett | Owner: Carlton
Type: | Gibson
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution:
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 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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/063.5d67fb8e064393ebc6eac6daace7cc64%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
