Changes between Initial Version and Version 1 of Ticket #30284


Ignore:
Timestamp:
Mar 23, 2019, 6:28:54 PM (6 years ago)
Author:
Tobias Bengfort
Comment:

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #30284 – Description

    initial v1  
    11See https://github.com/django/django/pull/11037
    22
    3 `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.
     3`User.is_active` is checked in `ModelBackend` on all of these methods:
     4
     5- `get_user_permissions()`
     6- `get_group_permissions()`
     7- `get_all_permissions()`
     8- `has_perm()`
     9- `has_module_perms()`
     10
     11I think the last three are redundant and should be removed.
    412
    513timgraham had [https://github.com/django/django/pull/11037#pullrequestreview-217534553 concerns] though:
    614
    7 > 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.
     15> 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.
    816
    9 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.
     17My 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.
    1018
    1119I 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.
Back to Top