Redundant is_active check in auth.backends.ModelBackend

36 views
Skip to first unread message

Tobias Bengfort

unread,
Mar 23, 2019, 8:58:58 PM3/23/19
to django-d...@googlegroups.com
See https://github.com/django/django/pull/11037

User.is_active is checked in contrib.auth.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.

Tim Graham has 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 redundant checks hide these
issues and therefore make it harder to find them.

I also think that the different methods should be consistent: 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.

tobias
Reply all
Reply to author
Forward
0 new messages