Opened 4 months ago

Last modified 4 months ago

#28588 assigned Cleanup/optimization

User.has_perm() with superusers hides nonexistent permissions

Reported by: Paul Hallett Owned by: moshe nahmias
Component: contrib.auth Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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')
# Totally fake permission
>>> super_user.has_perm('rubbishrubbishrubbish')

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:

I can't see what the resolution of the ticket was as it was just closed with a reason.

Change History (3)

comment:1 Changed 4 months ago by moshe nahmias

Easy pickings: set
Owner: changed from nobody to moshe nahmias
Status: newassigned

It seems like a easy enough thing to fix, I will do it if accepted

comment:2 Changed 4 months ago by Shai Berger

Component: Uncategorizedcontrib.auth

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.

comment:3 Changed 4 months ago by Tim Graham

Easy pickings: unset
Summary: has_perm with superusers hides non-existent permissionsUser.has_perm() with superusers hides nonexistent permissions
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

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.

Note: See TracTickets for help on using tickets.
Back to Top