Redundant is_active check in auth.backends.ModelBackend

[Tobias Bengfort]

See ​https://github.com/django/django/pull/11037

User.is_active is checked in ModelBackend on all of these methods:

• get_user_permissions()
• get_group_permissions()
• get_all_permissions()
• has_perm()
• has_module_perms()

I think the last three are redundant and should be removed.

timgraham had ​concerns though:

I'm unsure about removing the "redundant" is_active checks. It might be that some ModelBackend sublcasses rely on them. For example, if you subclass get_all_permissions() and omitting the is_active check (which wasn't there prior to Django 1.8 ​c33447a)... then your application would still have is_active checks in the other methods. This needs careful consideration and perhaps a discussion on the mailing list as to whether the benefits are worth the possible security issues.

My opinion is exactly the opposite: If a ModelBackend subclass does not check is_active in get_all_permissions() that is a bug and potentially even a security issue. The redundand checks hide these issues and therefore make it harder to find them.

I also think that the different methods should be consitent: If a permission is returned by get_all_permissions(), then checking that permission with has_perm() should return True. The only reason to do anything special in has_perm() is for performance optimizations.

[Tim Graham]

Both Mariusz and I raised doubts that this is a good idea. Please raise the issue on the DevelopersMailingList to see if there's consensus otherwise. If it's accepted (at which point we could reopen this ticket), the commit should be in a separate PR rather than lumped in with other features. Thanks.

[Tim Graham]

​django-developers discussion