Opened 12 years ago
Closed 11 years ago
#19792 closed Bug (wontfix)
django.test.client.Client.login() checking for is_active attribute on User objects
Reported by: | mrmagooey | Owned by: | Nick Sandford |
---|---|---|---|
Component: | Testing framework | Version: | 1.5-rc-1 |
Severity: | Normal | Keywords: | |
Cc: | nick.sandford@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
As per #13125 there has been a conscious decision to leave the is_active attribute as a hook for custom behaviour, and not used for built-in code. However, the login() method on django.test.client.Client objects checks for is_active. It would seem that this probably shouldn't be the case if the same logic used in the linked ticket for the login_required decorator is to be applied framework-wide.
Change History (6)
comment:1 by , 12 years ago
Cc: | added |
---|---|
Easy pickings: | set |
Needs documentation: | set |
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 12 years ago
Has patch: | set |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
Owner: | changed from | to
Status: | new → assigned |
Added a pull request here: https://github.com/django/django/pull/718.
comment:3 by , 12 years ago
On one side, I understand the issue, and the is_active
check might be inappropriate in the test client when using a custom auth backend.
On the other side, if the test client login
method is meant to be equivalent to the contrib.auth.login
view, we have a problem with this patch. The latter will refuse the login because it uses the AuthenticationForm
which checks the is_active
flag, while the former will let the user in (no more is_active
check).
This might be solved by documenting the discrepancy between Client.login()
and contrib.auth.login()
(https://docs.djangoproject.com/en/dev/topics/testing/overview/#django.test.client.Client.login). A release note would also be mandatory, as this would be slightly backwards-incompatible.
comment:4 by , 12 years ago
Hmm, on second thought, I guess the inconsistency lies with the @login_required
decorator. If we expect the test client to work like the contrib.auth.login
view, the is_active
check should remain. Even with custom auth backends the Client.login()
method works in much the same way as contrib.auth.login
.
Though it is a bit confusing that Jacob has said that is_active
is purely for custom auth sources and that the built-in stuff doesn't use it. Is that the expected behaviour and should the AuthenticationForm
is_active
check also be removed?
comment:5 by , 12 years ago
No, the is_active
check cannot be removed in AuthenticationForm
. I'm convinced that numerous sites are counting on this behaviour.
The login_required
decorator is basically checking request.user.is_authenticated()
, and a user with is_active=False
will not be authenticated with standard contrib.auth.login
.
I'd tend to won't fix this ticket, unless we get a convincing use case of removing this check.
comment:6 by , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
I also agree that removing the is_active check wouldn't be a good thing. As no convincing use cases has been raised, I'm setting this to wontfix.
Seems reasonable to me, would be good for consistency.