[Django] #24987: django.test.client.Client.login() rejects user with is_active=False
Django
-----------------------------------+--------------------
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
According to the documentation on the `User` attribute `is_active`:
https://docs.djangoproject.com/en/dev/ref/contrib/auth/
> This doesn’t necessarily control whether or not the user can log in.
Authentication backends aren’t required to check for the is_active flag,
and the default backends do not. If you want to reject a login based on
is_active being False, it’s up to you to check that in your own login view
or a custom authentication backend. However, the AuthenticationForm used
by the login() view (which is the default) does perform this check, as do
the permission-checking methods such as has_perm() and the authentication
in the Django admin. All of those functions/methods will return False for
inactive users.
My auth system takes advantage of this by allowing inactive user to login.
However, if I try to login an inactive user in a test, the login fails.
This happens due to the code in Client.login() in client.py:
{{{
user = authenticate(**credentials)
if (user and user.is_active and
apps.is_installed('django.contrib.sessions')):
...
return True
else:
return False
}}}
That is, after a successful authentication in a test, inactive users are
rejected. This seems to contradict the documentation.
How would you feel about dropping the `user.is_active` check in
`Client.login()`?
--
Ticket URL: <https://code.djangoproject.com/ticket/24987>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
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 timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Check was added in #4526 without much of an explanation, but it matches
the check in `AuthenticationForm` so that's probably the reasoning. I'm
not sure if we'd consider this a bug or a feature that requires backwards
compatibility, but another option to solve your use case might be #20916.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:1>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* has_patch: 0 => 1
Comment:
> I'm not sure if we'd consider this a bug or a feature that requires
backwards compatibility
If you decide on this let me know and I can change my approach. Until
then, I'll take the path of least resistance. I have created a PR that
treats this as a bug.
https://github.com/django/django/pull/4864
> but another option to solve your use case might be #20916.
This is interesting. I will take a look at this as well. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:2>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jdufresne):
Hmm. This may be a duplicate of previous ticket #19792. Sorry, my searches
didn't reveal this originally. Obviously, I disagree with the final
conclusion of wontfix in that ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:3>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_docs: 0 => 1
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Someday/Maybe
Comment:
I tend to follow Claude's reasoning in #19792. I, personally expect
`client.login()` to behave like `contrib.auth.login()`, thus checking for
`is_active=True`. I see your point though.
However, your current change is backwards incompatible and will break many
users' tests. I therefore don't consider it a bug but a feature request.
One idea that comes to mind, of how to solve the problem in a backwards
compatible manner, would be a flag on the client `check_is_active=True`
that would allow to bypass the check.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:4>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* Attachment "24987-doc.diff" added.
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by timgraham):
How about documenting this for now?
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:5>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by jdufresne):
> However, your current change is backwards incompatible and will break
many users' tests. I therefore don't consider it a bug but a feature
request.
>
> One idea that comes to mind, of how to solve the problem in a backwards
compatible manner, would be a flag on the client check_is_active=True that
would allow to bypass the check.
Understood about backwards incompatible concern. I can investigate coding
this idea if there is agreement that it is the best approach.
> How about documenting this for now?
So long as the limitation exists, makes sense. You're diff looks good to
me.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:6>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"fbc618c13cc72b9c2f4c8dfd5ef8b8ab5a5d7caa" fbc618c]:
{{{
#!CommitTicketReference repository=""
revision="fbc618c13cc72b9c2f4c8dfd5ef8b8ab5a5d7caa"
Refs #24987 -- Documented that Client.login() rejects inactive users.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:7>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"8050e6282e4daee24758a4a1c6c2fa938957bef2" 8050e62]:
{{{
#!CommitTicketReference repository=""
revision="8050e6282e4daee24758a4a1c6c2fa938957bef2"
[1.8.x] Refs #24987 -- Documented that Client.login() rejects inactive
users.
Backport of fbc618c13cc72b9c2f4c8dfd5ef8b8ab5a5d7caa from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:8>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by timgraham):
If the login solution proposed in #20916 will meet your use case, I find
that better than adding an attribute to `test.Client` as I think the
latter will be difficult to use (requiring initializing the test client on
your own).
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:9>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by jdufresne):
> If the login solution proposed in #20916 will meet your use case, I find
that better than adding an attribute to test.Client as I think the latter
will be difficult to use (requiring initializing the test client on your
own).
I agree. I will continue work on the other ticket/PR. If you prefer to
close this, I have no problem with that.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:10>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by timgraham):
#25232 might allow removing the check in the test client `login()` method.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:11>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Easy pickings: 0 | UI/UX: 0
Changes (by sasha0):
* cc: sasha@… (added)
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
Comment:
[https://github.com/django/django/pull/6124 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:12>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by hobarrera):
> However, the AuthenticationForm used by the login() view (which is the
default) does perform this check, as do the permission-checking methods
such as has_perm() and the authentication in the Django admin. All of
those functions/methods will return False for inactive users.
I think it makes sense for inactive users to get rejected; it's the result
you'd get while using the whole default stack (eg: backend + view + form).
My first thought is that your tests should use `force_login`, or maybe
override `login()` to use your own login view+form combination.
Another, more flexible, but more appropriate fix is for `login()` to log
in users by posting to `LOGIN_URL`, which makes sure that tests use the
exact same thing as what your application uses. This might be more effort,
but makes tests far more consistent for everyone.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:13>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
The idea is to change the default authentication backend to reject
inactive users (#25232). Then we'll proceed with this patch so that
someone who wants to allow inactive users to login won't be thwarted by
the existing check in the test client.
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:14>
Django
-----------------------------------+------------------------------------
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Someday/Maybe => Accepted
Comment:
[https://github.com/django/django/pull/6309 Updated PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:15>
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:16>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"107165c4b04f4e5a830a60b6c66b2e5d8fb1d242" 107165c]:
{{{
#!CommitTicketReference repository=""
revision="107165c4b04f4e5a830a60b6c66b2e5d8fb1d242"
Fixed #24987 -- Allowed inactive users to login with the test client.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24987#comment:17>