[Django] #26957: Inconsistency between doc and code: authenticate() when user.is_active == False
Django
False
--------------------------------------+--------------------
Reporter: ericls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
In the code:
https://github.com/django/django/blob/master/django/contrib/auth/backends.py#L12-L32
{{{
def authenticate(self, username=None, password=None, **kwargs):
UserModel = get_user_model()
if username is None:
username = kwargs.get(UserModel.USERNAME_FIELD)
try:
user = UserModel._default_manager.get_by_natural_key(username)
except UserModel.DoesNotExist:
# Run the default password hasher once to reduce the timing
# difference between an existing and a non-existing user
(#20760).
UserModel().set_password(password)
else:
if user.check_password(password) and
self.user_can_authenticate(user):
return user
def user_can_authenticate(self, user):
"""
Reject users with is_active=False. Custom user models that don't
have
that attribute are allowed.
"""
is_active = getattr(user, 'is_active', None)
return is_active or is_active is None
}}}
authenticate() returns None when user.is_active == False.
However, the documentation suggests that when user.is_active == False, the
authenticate() method returns the user normally.
{{{
from django.contrib.auth import authenticate, login
def my_view(request):
username = request.POST['username']
password = request.POST['password']
user = authenticate(username=username, password=password)
if user is not None:
if user.is_active:
login(request, user)
# Redirect to a success page.
else:
# Return a 'disabled account' error message
...
else:
# Return an 'invalid login' error message.
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26957>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
False
Reporter: ericls | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 1.10
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 ericls):
* needs_docs: => 0
* version: 1.9 => 1.10
* needs_tests: => 0
* needs_better_patch: => 0
Old description:
New description:
In the code:
https://github.com/django/django/blob/stable/1.10.x/django/contrib/auth/backends.py#L12-L32
{{{
def authenticate(self, username=None, password=None, **kwargs):
UserModel = get_user_model()
if username is None:
username = kwargs.get(UserModel.USERNAME_FIELD)
try:
user = UserModel._default_manager.get_by_natural_key(username)
except UserModel.DoesNotExist:
# Run the default password hasher once to reduce the timing
# difference between an existing and a non-existing user
(#20760).
UserModel().set_password(password)
else:
if user.check_password(password) and
self.user_can_authenticate(user):
return user
def user_can_authenticate(self, user):
"""
Reject users with is_active=False. Custom user models that don't
have
that attribute are allowed.
"""
is_active = getattr(user, 'is_active', None)
return is_active or is_active is None
}}}
authenticate() returns None when user.is_active == False.
However, the documentation suggests that when user.is_active == False, the
authenticate() method returns the user normally.
https://docs.djangoproject.com/en/1.10/topics/auth/default/#auth-web-
requests
{{{
from django.contrib.auth import authenticate, login
def my_view(request):
username = request.POST['username']
password = request.POST['password']
user = authenticate(username=username, password=password)
if user is not None:
if user.is_active:
login(request, user)
# Redirect to a success page.
else:
# Return a 'disabled account' error message
...
else:
# Return an 'invalid login' error message.
...
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:1>
Django
False
Reporter: ericls | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Yes, it would be good to adjust or clarify that example in light of
#25232.
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:2>
Django
False
Reporter: ericls | Owner: austin-
Type: | simmons
Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by austin-simmons):
* owner: nobody => austin-simmons
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:3>
Django
False
Reporter: ericls | Owner:
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by austin-simmons):
* owner: austin-simmons =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:4>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: new => assigned
* owner: => an0o0nym
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:5>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by an0o0nym):
I understand that the `user.is_active()` check is redundant after we run
`authenticate` method in the example code. However I would like to ask for
some advice, before I do anything(as a newbie I would like to make it all
right). I would add some information in the box that the `authenticate`
method returns `None` if the user is inactive, even if the user entered
correct login detail(thus checking if user is active after that is
redundant)? And then I would mention to use separately `user.is_active()`
and `user.check_password()` to display separate error messages, for each
case, as in the example code above ? Would that clear things up?
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:6>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by ericls):
Running `user.is_active()` and `user.check_password()` separately is a
good idea, but I think it would result in redundant SQL queries, right?
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:7>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by an0o0nym):
You are right. So, how about authenticate ''user'' using `authenticate()`
method (as in the example provided in the ticket description), then if
''user'' '''is not''' `None` login ''user'' without performing
`user.is_active()` check. Else, if ''user'' '''is''' `None` create an if
statement, using following expression: `user.is_active()`; which will
allow us to display custom error messages for 'disabled account' and
'invalid login'
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:8>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by an0o0nym):
Pull request created at [https://github.com/django/django/pull/7059].
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:9>
Django
False
Reporter: ericls | Owner: an0o0nym
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c52350bc6c0ce4e146db696d3a9772b6b9dc554f" c52350b]:
{{{
#!CommitTicketReference repository=""
revision="c52350bc6c0ce4e146db696d3a9772b6b9dc554f"
[1.10.x] Fixed #26957 -- Corrected authenticate() docs regarding
User.is_active.
Backport of c412aaca735c7cc1c766b85c1512f9ff434ce63a from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:10>
Django
False
Reporter: ericls | Owner: an0o0nym
Type: Cleanup/optimization | Status: closed
Component: contrib.auth | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"c412aaca735c7cc1c766b85c1512f9ff434ce63a" c412aac]:
{{{
#!CommitTicketReference repository=""
revision="c412aaca735c7cc1c766b85c1512f9ff434ce63a"
Fixed #26957 -- Corrected authenticate() docs regarding User.is_active.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26957#comment:11>