Opened 9 years ago
Closed 9 years ago
#24987 closed Bug (fixed)
Remove test client login()'s hardcoded rejection of inactive users
Reported by: | Jon Dufresne | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | sasha@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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()
?
Attachments (1)
Change History (18)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Has patch: | set |
---|
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.
comment:3 by , 9 years ago
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.
comment:4 by , 9 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Someday/Maybe |
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.
by , 9 years ago
Attachment: | 24987-doc.diff added |
---|
comment:6 by , 9 years ago
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.
comment:9 by , 9 years ago
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).
comment:10 by , 9 years ago
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.
comment:11 by , 9 years ago
#25232 might allow removing the check in the test client login()
method.
comment:12 by , 9 years ago
Cc: | added |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
Patch needs improvement: | unset |
comment:13 by , 9 years ago
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.
comment:14 by , 9 years ago
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.
comment:15 by , 9 years ago
Summary: | django.test.client.Client.login() rejects user with is_active=False → Remove test client login()'s hardcoded rejection of inactive users |
---|---|
Triage Stage: | Someday/Maybe → Accepted |
comment:16 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.