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 Nick Sandford, 12 years ago

Cc: nick.sandford@… added
Easy pickings: set
Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

Seems reasonable to me, would be good for consistency.

comment:2 by Nick Sandford, 12 years ago

Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Nick Sandford
Status: newassigned

Added a pull request here: https://github.com/django/django/pull/718.

comment:3 by Claude Paroz, 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 Nick Sandford, 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 Claude Paroz, 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 DjangoPanngo, 11 years ago

Resolution: wontfix
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top