Opened 4 years ago

Closed 3 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: master
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)

24987-doc.diff (913 bytes) - added by Tim Graham 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by 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.

comment:2 Changed 4 years ago by Jon Dufresne

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 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by Markus Holtermann

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedSomeday/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.

Changed 4 years ago by Tim Graham

Attachment: 24987-doc.diff added

comment:5 Changed 4 years ago by Tim Graham

How about documenting this for now?

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

In fbc618c:

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

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

In 8050e62:

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

Backport of fbc618c13cc72b9c2f4c8dfd5ef8b8ab5a5d7caa from master

comment:9 Changed 4 years ago by 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).

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by Tim Graham

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

comment:12 Changed 4 years ago by Sasha Gaevsky

Cc: sasha@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:13 Changed 4 years ago by 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.

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by Tim Graham

Summary: django.test.client.Client.login() rejects user with is_active=FalseRemove test client login()'s hardcoded rejection of inactive users
Triage Stage: Someday/MaybeAccepted

comment:16 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:17 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 107165c:

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

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