#25847 closed New feature (fixed)
Make User.is_authenticated() and User.is_anonymous() "callable properties"
Reported by: | Stavros Korokithakis | Owned by: | Jeremy Lainé |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | 1.10 |
Cc: | zachborboa@…, dheeru.rathor14@…, Florian Apolloner | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (20)
comment:1 by , 9 years ago
Summary: | is_authenticated is vulnerability-prone. → Make User.is_authenticated() a "callable property" |
---|---|
Type: | Uncategorized → New feature |
comment:2 by , 9 years ago
Cc: | added |
---|
comment:3 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
The discussion is leaning towards making this change.
A possible deprecation of the callable form wasn't discussed.
comment:4 by , 9 years ago
Cc: | added |
---|
comment:6 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 9 years ago
Summary: | Make User.is_authenticated() a "callable property" → Make User.is_authenticated() and User.is_anonymous() "callable properties" |
---|
comment:8 by , 9 years ago
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)
comment:9 by , 9 years ago
Alternatively, we could add a system check that raises a warning for an incompatible implementation.
comment:10 by , 9 years ago
Has patch: | set |
---|
comment:13 by , 8 years ago
@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…
comment:14 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
comment:15 by , 8 years ago
Has patch: | unset |
---|---|
Keywords: | 1.10 added |
Patch needs improvement: | unset |
Version: | 1.9 → master |
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.
comment:16 by , 8 years ago
Cc: | added |
---|
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?
comment:17 by , 8 years ago
Has patch: | set |
---|
PR for the system check to detect is_anonymous
/is_authenticated
as methods.
comment:19 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I bumped an old django-developers discussion to get opinions about this.