Document User.has_perm() behavior for active superusers.

[Paul Hallett]

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

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: https://code.djangoproject.com/ticket/26547.

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

[moshe nahmias]

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

[Shai Berger]

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.

[Tim Graham]

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.

[Carlton Gibson]

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.)

​PR implementing Tim's suggestion just to document the behaviour.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4b32d039:

Fixed #28588 -- Doc'd User.has_perm() & co. behavior for active superusers.

Equivalent note for PermissionsMixin was added in d33864ed138f65070049a3ac20ee98e03a1442b9.

[Mariusz Felisiak <felisiak.mariusz@…>]

In b6d8957:

[2.2.x] Fixed #28588 -- Doc'd User.has_perm() & co. behavior for active superusers.

Equivalent note for PermissionsMixin was added in d33864ed138f65070049a3ac20ee98e03a1442b9.
Backport of 4b32d039dbb59b3c3e76587df5c58150e752d9ac from master