Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#26988 closed Cleanup/optimization (fixed)

User is_authenticated callable property is confusing to check

Reported by: Mark Tranchant Owned by: nobody
Component: contrib.auth Version: 1.10
Severity: Release blocker Keywords: user is_authenticated property test
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mark Tranchant)

Just upgraded to 1.10, converted all is_authenticated() methods into is_authenticated properties as per the Release Notes and a test in my test suite failed.

It turns out I was checking for a logged in user with if request.user.is_authenticated is False:, but the is_authenticated property is actually a CallableBool which cannot be tested with {{== False}}, {{is False}}, {{== True}} or {{is True}}.

Checking this property only gives logical results with direct if user.is_authenticated or if not user.is_authenticated. This is very misleading and non-intuitive behaviour (at odds with PEP8 (bottom of linked section)) and should be fixed or strongly called out in the documentation. Example:

In [1]: from django.contrib.auth.models import AnonymousUser, AbstractBaseUser

In [2]: a = AnonymousUser()

In [3]: b = AbstractBaseUser()

In [4]: a.is_authenticated
Out[4]: CallableBool(False)

In [5]: b.is_authenticated
Out[5]: CallableBool(True)

In [6]: a.is_authenticated is False
Out[6]: False

In [7]: a.is_authenticated == False
Out[7]: False

In [8]: not a.is_authenticated
Out[8]: True

In [9]: not b.is_authenticated
Out[9]: False

In [10]: b.is_authenticated == False
Out[10]: False

In [11]: b.is_authenticated == True
Out[11]: False

Change History (9)

comment:1 Changed 3 years ago by Mark Tranchant

Summary: User is_authenticated callable property confusing if used with "is False"User is_authenticated callable property is confusing to check

comment:2 Changed 3 years ago by Mark Tranchant

Description: modified (diff)
Easy pickings: set

comment:3 Changed 3 years ago by Mark Tranchant

Description: modified (diff)

comment:4 Changed 3 years ago by Tim Graham

Easy pickings: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

I think it's impossible to fix the is comparison, but we can fix == and != as well as improve the release note. PR.

comment:5 Changed 3 years ago by Tim Graham

Has patch: set

comment:6 Changed 3 years ago by Mark Tranchant

Thanks, looks like the best we can do for a transitional period. In your release notes update:

Allowed User.is_authenticated() and User.is_anonymous() properties

you should not include the brackets.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 54afa960:

Fixed #26988 -- Improved/clarified User.is_authenticated/anonymous compatibility.

Thanks marktranchant for the report and review.

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

In fa4b5c1b:

[1.10.x] Fixed #26988 -- Improved/clarified User.is_authenticated/anonymous compatibility.

Thanks marktranchant for the report and review.

Backport of 54afa960d1ee8c63635225a0f0a2489971b5aab5 from master

comment:9 Changed 3 years ago by WeizhongTu

I think it's better to add a remark to the code, because someone only read the code

if request.user.is_authenticated is True:  # won't work
    ...
Note: See TracTickets for help on using tickets.
Back to Top