[Django] #26547: User has_perm Is Not Developer Friendly
Django
--------------------------------------+---------------------------
Reporter: dsanders11 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords: has_perm user
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+---------------------------
Hopefully this isn't a duplicate, I didn't see any describing this.
As it stands `has_perm` on the User object is not developer friendly in
the least, and can easily lead to bugs.
Unfriendly
1. Checking a non-existent permission will always return false - unless
the user is a superuser and it will always return true. Never warns the
developer that they're checking a non-existent permission, and no way to
turn on strict permission mode
2. Doesn't play nice with `AppConfig` relabeling of apps, makes the
developer do the work
3. The default permissions 'add', 'change', and 'delete' are magic - no
one else is
4. As #25281 states, permission strings don't identify a permission
uniquely -
[https://github.com/django/django/blob/master/django/contrib/auth/models.py#L68
codename is not unique] but `has_perm` only checks the app label and
codename, meaning two models in the same app with the same codename allow
grant each other permission accidentally.
With the first issue, bugs can easily creep into code due to there being
no warning or error if a non-existent permission is used. If a developer
is testing as a superuser to test all functionality, even with an
incorrect permission name used in the code they will be able to test that
functionality. If they then intend to test a user not being able to use
functionality without permission, they will use a user without that
permission, with the intention of it not being permitted. The permission
check with a typo in the permission name will happily say that user
doesn't have permission, so from the developer's perspective everything is
working as expected. Once in production it'll be a "works for me" for
superusers but tricky to debug for normal users who have that permission.
The second issue dovetails with the first - hardcoding an app label into a
permission name means that if the app is re-labeled using `AppConfig` to
say avoid a name conflict, all of the permission checking breaks. Again,
it breaks silently due to the first issue, meaning it's not immediately
clear why none of the permission checking seems to work, even though
superusers can do whatever. May lead frustrated inexperienced developers
to giving people superuser status who they shouldn't because "it works" to
fix the problem. While hardcoding the app label is bad practice, it's
likely a persistent one, especially from before apps were able to be re-
labeled. For example, several Mozilla projects
([https://github.com/mozilla/moztrap/blob/master/moztrap/view/views.py#L14
moztrap],
[https://github.com/mozilla/airmozilla/blob/master/airmozilla/manage/views/events.py#L89
airmozilla]) hardcode the app label into the permission check. In the
Django codebase there are a multitude of ways to generate the permission
string with the app label, such as
[https://github.com/django/django/blob/master/tests/admin_views/custom_has_permission_admin.py#L12
this test],
[https://github.com/django/django/blob/master/tests/admin_views/tests.py#L1311
this test], and the
[https://github.com/django/django/blob/master/django/contrib/admin/options.py#L445
admin code]. All three are slightly different ways of generating the
permission string, mostly getting the app label. For an app to be able to
be relabeled successfully the developer must ensure the app label is
dynamically retrieved for permission checks.
The third issue is that 'add', 'change' and 'delete' are treated
differently than other permissions. In the `default_permissions` for a
model Meta, they're listed by their simple name (which becomes
'change_<foo>' to avoid the lack of unique checks) but `permissions`
requires a unique name (but no error if you don't). A naive developer may
add a new permission 'view' to several models, not realizing the issue in
4. A more consistent approach would be to apply `get_permission_codename`
[https://github.com/django/django/blob/master/django/contrib/auth/__init__.py#L202
in auth] to custom permissions as well so that the user doesn't have to
suffix in the model name by hand. I don't know how inheriting permissions
from an abstract class would work since it needs to be unique, but
probably not well. I'm sure someone has inadvertently created that bug.
The final issue is covered by #25281, but I thought I'd mention it here
since it directly affects the above. Without unique checking of
permissions it's possible for a developer to forget to add a permission to
a model (not realizing they need to be unique) and use permission checks
for a different model that are actually checking the permission defined on
the different model. The 'view' permission is a good example of this,
defining it on one model, forgetting to define it on the second, and then
in the code checking 'foo.view' permission will succeed if the user has
the permission for the model with it actually defined. Due to the lack of
uniqueness even if it was defined on both models, the check would allow a
user with view permission on FooBar to view BarFoo. It seems like if
#25281 does not progress in a timely manner then a stop gap would be to
force app_label and codename to be unique together for `Permission` -
currently content type and codename must be unique together but no mention
of app_label.
Sorry for the wall of text, these are just several large stumbling blocks
I've found with the user permissions as they stand today which could
really use some addressing. They're all avoidable if the developer knows
about them, but they aren't avoidable if they're broken in third-party
code. An overall refactoring of permissions with these issues in mind
seems like a good idea, to make everything consistent and logical.
IMO the cleanest way it could end up looking for developers would be:
{{{
from django.contrib.auth import get_permission_codename
def User.has_perm(self, model, perm, obj=None):
opts = model._meta
full_perm_name = "%s.%s" % (opts.app_label,
get_permission_codename(perm, opts))
try:
...
except PermissionNotFound:
logger.warning("Tried to check non-existent permission")
return False
user.has_perm(Foo, 'change')
user.has_perm(Foo, 'change', obj=foo_obj)
user.has_perm(Foo, 'custom_perm')
user.has_perm(Bar, 'custom_perm')
}}}
That would make things clean and logical and address I think all of the
above issues, but it isn't backwards compatible. It would change the
signature of `has_perm` and would also change how custom permissions are
created (making them unique per model using `get_permission_codename`).
It's not the cleanest if you need permission names like "send_email" since
it would be transformed to "send_email_foo". If permission checking is
done using the full criteria like #25281 mentions (app_label,
content_type, codename) then codename wouldn't have to be unique and could
switch to being verbatim instead, so ('auth', User, 'add_user') would
become ('auth', User, 'add').
--
Ticket URL: <https://code.djangoproject.com/ticket/26547>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: dsanders11 | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: contrib.auth | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: has_perm user | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* needs_docs: => 0
* resolution: => needsinfo
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
Yes, Trac is not a good place for a wall of text like this. A course of
action that involves first writing to the DevelopersMailingList and trying
to form consensus around specific solutions, then creating tickets with
specific actions items is more ideal. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/26547#comment:1>
Django
Reporter: David Sanders | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution: needsinfo
| Unreviewed
Comment (by Mariatta):
I was pointed to this issue because I created a different and related
issue #37021.
I also need a way to check for permission using `user.has_perm` for any
given `Permission` object.
Currently the way we have to do it is by constructing the string as
follows.
{{{
perm = Permission.objects.get(...)
if user.has_perm(f"{perm.content_type.app_label}.{perm.codename}"):
# do stuff
}}}
I prefer getting the string from the `Permission` object instead of
hardcoding it.
My suggestion is to add a new helper function to the Permission object
like so:
{{{
class Permission():
def perm_string(self): # feel free to suggest other name
return f"{self.content_type.app_label}.{self.codename}"
}}}
so that the `user.has_perm()` does not have to change, and then I can use
it like this:
{{{
perm = Permission.objects.get(...)
if user.has_perm(perm.perm_string()):
# do stuff
}}}
I think adding the helper function to the `Permission` object is simpler
than making changes to the `has_perm` function. This way there is no
concern with backward incompatibility, and we are not hardcoding the
string during user permission check process.
If you agree, I would like to try creating the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/26547#comment:2>