[Django] #25847: is_authenticated is vulnerability-prone.
Django
-------------------------------+--------------------
Reporter: skorokithakis | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
There's an inconsistency between `is_authenticated` and `is_superuser`.
The former is a method and the latter is a property, leading many people
to do:
{{{
if request.user.is_authenticated:
return sensitive_information
}}}
which is, of course, always executed.
I propose that is_authenticated be turned into a property, while it can
also be a callable, for backwards-compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: skorokithakis | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Uncategorized => New feature
* needs_tests: => 0
Comment:
I bumped an old [https://groups.google.com/d/topic/django-
developers/7k6Z8JxKH5Q/discussion django-developers discussion] to get
opinions about this.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:1>
Django
-------------------------------+--------------------------------------
Type: New feature | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by zachborboa):
* cc: zachborboa@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:2>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
The discussion is leaning towards making this change.
A possible deprecation of the callable form wasn't discussed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:3>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by DheerendraRathor):
* cc: dheeru.rathor14@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:4>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
#26388 suggested the same for `is_anonymous()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:5>
Django
-------------------------------+------------------------------------
Type: New feature | Status: assigned
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jlaine):
* status: new => assigned
* owner: nobody => jlaine
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:6>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:7>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jlaine):
I have put up a pull request here:
https://github.com/django/django/pull/6376
@apollo13 mentioned that we might want to support the case where a custom
user model defines is_anonymous / is_authenticated as methods instead of
properties. In this case we could do something like this to monkey-patch
that user model to the new style:
{{{
def __new__(cls):
if not isinstance(cls.is_anonymous, property):
real_is_anonymous = cls.is_anonymous
cls.is_anonymous = property(lambda x:
CallableBool(real_is_anonymous(x)))
return super(AbstractBaseUser, cls).__new__(cls)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:8>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Alternatively, we could add a system check that raises a warning for an
incompatible implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:9>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jlaine):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:10>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:11>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c1aec0feda73ede09503192a66f973598aef901d" c1aec0fe]:
{{{
#!CommitTicketReference repository=""
revision="c1aec0feda73ede09503192a66f973598aef901d"
Fixed #25847 -- Made User.is_(anonymous|authenticated) properties.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:12>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by apollo13):
@timgraham: I feel this should get reverted for now. We have to consider
custom user models which did define this as a method. A warning is not
enough, either his is a hard error or we ensure that it is backwards
compatible. Allowing custom user models to have a method is a security
risk since all checks will now return true…
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:13>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Component: contrib.auth | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by claudep):
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:14>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* keywords: => 1.10
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* version: 1.9 => master
Comment:
I was thinking of adding a "compatibility" system check to detect that.
I'll do it before 1.10 alpha if no one else takes it.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:15>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* cc: apollo13 (added)
Comment:
Unless that system check is a hard error I am against it. (Ie it does have
to be ERROR, so we can ensure that we do not open ourself to security
issues) -- can we justify the backwards incompatibility?
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:16>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: 1.10 | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6562 PR] for the system check to
detect `is_anonymous`/`is_authenticated` as methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:17>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Type: New feature | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"03efa304bce5ef0924948a74ae01cdf817dd416a" 03efa304]:
{{{
#!CommitTicketReference repository=""
revision="03efa304bce5ef0924948a74ae01cdf817dd416a"
Refs #25847 -- Added system check for
UserModel.is_anonymous/is_authenticated methods.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:18>
Django
-------------------------------+------------------------------------
Reporter: skorokithakis | Owner: jlaine
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:19>
Django
Reporter: Stavros | Owner: Jeremy
Korokithakis | Lainé
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.10 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"eba093e8b02989af1857b1915907ca0897f565ff" eba093e]:
{{{
#!CommitTicketReference repository=""
revision="eba093e8b02989af1857b1915907ca0897f565ff"
Refs #25847 -- Removed support for User.is_(anonymous|authenticated) as
methods.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25847#comment:20>