Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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 Tim Graham, 9 years ago

Summary: is_authenticated is vulnerability-prone.Make User.is_authenticated() a "callable property"
Type: UncategorizedNew feature

I bumped an old django-developers discussion to get opinions about this.

comment:2 by Zach Borboa, 9 years ago

Cc: zachborboa@… added

comment:3 by Aymeric Augustin, 9 years ago

Triage Stage: UnreviewedAccepted

The discussion is leaning towards making this change.

A possible deprecation of the callable form wasn't discussed.

comment:4 by Dheerendra Rathor, 9 years ago

Cc: dheeru.rathor14@… added

comment:5 by Tim Graham, 9 years ago

#26388 suggested the same for is_anonymous().

comment:6 by Jeremy Lainé, 9 years ago

Owner: changed from nobody to Jeremy Lainé
Status: newassigned

comment:7 by Jeremy Lainé, 9 years ago

Summary: Make User.is_authenticated() a "callable property"Make User.is_authenticated() and User.is_anonymous() "callable properties"

comment:8 by Jeremy Lainé, 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 Tim Graham, 9 years ago

Alternatively, we could add a system check that raises a warning for an incompatible implementation.

comment:10 by Jeremy Lainé, 9 years ago

Has patch: set

comment:11 by Tim Graham, 9 years ago

Patch needs improvement: set

Left comments for improvement.

comment:12 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In c1aec0fe:

Fixed #25847 -- Made User.is_(anonymous|authenticated) properties.

comment:13 by Florian Apolloner, 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 Claude Paroz, 8 years ago

Resolution: fixed
Status: closednew

comment:15 by Tim Graham, 8 years ago

Has patch: unset
Keywords: 1.10 added
Patch needs improvement: unset
Version: 1.9master

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 Florian Apolloner, 8 years ago

Cc: Florian Apolloner 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 Tim Graham, 8 years ago

Has patch: set

PR for the system check to detect is_anonymous/is_authenticated as methods.

comment:18 by Tim Graham <timograham@…>, 8 years ago

In 03efa304:

Refs #25847 -- Added system check for UserModel.is_anonymous/is_authenticated methods.

comment:19 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed

comment:20 by Tim Graham <timograham@…>, 8 years ago

In eba093e:

Refs #25847 -- Removed support for User.is_(anonymous|authenticated) as methods.

Per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top