Opened 6 years ago

Last modified 6 years ago

#30284 closed Cleanup/optimization

Redundant is_active check in auth.backends.ModelBackend — at Initial Version

Reported by: Tobias Bengfort Owned by: nobody
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

User.is_active is checked in ModelBackend.get_all_permissions(), ModelBackend.has_perm() and ModelBackend.has_module_perms(). I think the last two 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. At least a release note is required. I'm not sure if this is required as part of the "BaseBackend" change, but I think it merits its own ticket.

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.

Change History (0)

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