Remove test client login()'s hardcoded rejection of inactive users

[Jon Dufresne]

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()?

[Tim Graham]

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.

[Jon Dufresne]

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.

[Jon Dufresne]

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.

[Markus Holtermann]

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.

[Tim Graham]

How about documenting this for now?

[Jon Dufresne]

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.

[Tim Graham <timograham@…>]

In fbc618c:

Refs #24987 -- Documented that Client.login() rejects inactive users.

[Tim Graham <timograham@…>]

In 8050e62:

[1.8.x] Refs #24987 -- Documented that Client.login() rejects inactive users.

Backport of fbc618c13cc72b9c2f4c8dfd5ef8b8ab5a5d7caa from master

[Tim Graham]

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).

[Jon Dufresne]

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.

[Tim Graham]

#25232 might allow removing the check in the test client login() method.

[Sasha Gaevsky]

​PR

[Hugo Osvaldo Barrera]

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.

[Tim Graham]

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.

[Tim Graham]

​Updated PR

[Tim Graham <timograham@…>]

In 107165c:

Fixed #24987 -- Allowed inactive users to login with the test client.