#30284: Redundant is_active check in auth.backends.ModelBackend
-------------------------------------+-------------------------------------
Reporter: Tobias Bengfort | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Tobias Bengfort:
Old 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
> [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.
New description:
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
[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.
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#comment:1>
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/060.2cab0b4206494f4ff543300993392972%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
