#30284: Redundant is_active check in auth.backends.ModelBackend
------------------------------------------------+------------------------
Reporter: Tobias Bengfort | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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
[https://github.com/django/django/pull/11037#pullrequestreview-217534553
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
[https://github.com/django/django/commit/c33447a50c1b0a96c6e2261f7c45d2522a3fe28d
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30284>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/045.4112ceabd5a3384a84a2ebee6ea8bd86%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
