User has_perm Is Not Developer Friendly

[David Sanders]

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 - ​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 (​moztrap, ​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 ​this test, ​this test, and the ​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 ​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').

[Tim Graham]

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!