#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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
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.
