Opened 5 years ago

Closed 4 years ago

Last modified 4 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: master
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 Changed 5 years ago by Tim Graham

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 Changed 5 years ago by Zach Borboa

Cc: zachborboa@… added

comment:3 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

The discussion is leaning towards making this change.

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

comment:4 Changed 5 years ago by Dheerendra Rathor

Cc: dheeru.rathor14@… added

comment:5 Changed 4 years ago by Tim Graham

#26388 suggested the same for is_anonymous().

comment:6 Changed 4 years ago by Jeremy Lainé

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

comment:7 Changed 4 years ago by Jeremy Lainé

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

comment:8 Changed 4 years ago by Jeremy Lainé

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 Changed 4 years ago by Tim Graham

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

comment:10 Changed 4 years ago by Jeremy Lainé

Has patch: set

comment:11 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Left comments for improvement.

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In c1aec0fe:

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

comment:13 Changed 4 years ago by Florian Apolloner

@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 Changed 4 years ago by Claude Paroz

Resolution: fixed
Status: closednew

comment:15 Changed 4 years ago by Tim Graham

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 Changed 4 years ago by Florian Apolloner

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 Changed 4 years ago by Tim Graham

Has patch: set

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

comment:18 Changed 4 years ago by Tim Graham <timograham@…>

In 03efa304:

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

comment:19 Changed 4 years ago by Tim Graham

Resolution: fixed
Status: newclosed

comment:20 Changed 4 years ago by Tim Graham <timograham@…>

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