Opened 2 years ago

Closed 2 months ago

#20916 closed New feature (fixed)

Provide a "force_login" feature for the test client

Reported by: mjtamlyn Owned by: Tim Graham <timograham@…>
Component: Testing framework Version: master
Severity: Normal Keywords: auth
Cc: jon.dufresne@… 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

At present client.login() takes the normal authentication parameters, runs them through the authentication backends and then logs the user in. A lot of my sites currently require the user to be logged in on every page. Consequently, every unit test which uses the client to check a page creates a user and then logs them in. When I create the user with a helper factory, I have to be careful to record the raw password of that user so I can pass it back through the hashing algorithms.

I'd like to provide client.simple_login(user), which simply sets up the test client's session so that the user in question is logged in, without going through the hashing algorithms. This would include most of the business logic from the current login() method, but without the authenticate() check. This would be a minor performance improvement which probably doesn't justify itself in most cases, the main benefit is ease-of-use.

Related in a way to #15179

Change History (20)

comment:1 Changed 2 years ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Would it make sense to add a parameter to login rather than making it a separate method?

comment:2 Changed 2 years ago by mjtamlyn

The issue with that is that at present login takes **kwargs which get passed to the auth backend.

comment:3 Changed 2 years ago by jfilipe

  • Owner changed from nobody to jfilipe
  • Status changed from new to assigned

comment:4 Changed 19 months ago by alasdair

  • Owner changed from jfilipe to alasdair

comment:5 Changed 19 months ago by alasdair

I had a go at this ticket at the Djangoweekend sprints. You can see my work so far in my branch on github: https://github.com/alasdairnicol/django/compare/ticket_20916. I'd welcome any comments, either here or on github.

As mjtamlyn says, authentication credentials are passed to the authenticate method, which makes it tricky to add the desired functionality to the existing login method. We could change the function signature to

def login(*args, **kwargs):
    if args:
        user = args[0]
    else:
        # use credentials
        credentials = **kwargs

but it feels hacky, so I decided to try implementing a new method simple_login.

I found that we have to specify user.backend, even though the authentication backend is not used to log the user in. I have documented that backend must be in AUTHENTICATION_BACKENDS (if not, simple_login currently returns True, but any requests to a login protected view will be redirected to the login page). Perhaps I should raise a ValueError if the backend is not in AUTHENTICATION_BACKENDS.

Last edited 19 months ago by alasdair (previous) (diff)

comment:6 Changed 19 months ago by mjtamlyn

  • Has patch set
  • Needs documentation set
  • Patch needs improvement set

The patch is a good approach, but there is now a large amount of duplication between login and simple_login. Also, the new functionality needs both documentation and release notes.

comment:7 Changed 19 months ago by Alasdair

That's a good point about the duplication. I'll factor out the common functionality.

There is already documentation and an entry in the release notes in my patch. I can work on these if they need improvement.

comment:8 Changed 19 months ago by timo

I wonder if it might be nice to complete #20915 (remove django.test.client dependency on django.contrib.auth) before adding this which is more of that.

comment:9 Changed 17 months ago by jfilipe

I've tried a different approach where a new authentication backend is added when setting up the test environment.

The changes are here: https://github.com/django/django/pull/2570

If you guys like this approach I can add some docs.

Last edited 17 months ago by jfilipe (previous) (diff)

comment:10 Changed 17 months ago by smeatonj

I really like the approach you've taken here with the custom backend rather than polluting the standard auth system. I'm not sure that you'd want to set that backend as a default backend for the test suite though. Perhaps some kind of context manager could be used or just the standard @override_settings.

comment:11 Changed 17 months ago by jfilipe

Technically it's not setting it as the default, it's just adding it as another backend the auth system will try and use.

comment:12 Changed 17 months ago by julien

Thank you for your work on this ticket.

I feel a bit uncomfortable adding an inherently insecure authenticate backend to core. This feature only really makes sense for testing, so I'd personally prefer to add it directly to the test client.

Also, the term "simple" seems a little vague to me. Something like force_login(user), or login(user, force=True) would seem more explicit.

I'd appreciate getting another core dev's opinion about the above.

By the way, could you re-create the PR against the official Django repository on github (instead of your own fork)?

Thanks!

comment:13 Changed 17 months ago by jfilipe

PR against Django's repo has been created: https://github.com/django/django/pull/2570.

That's a valid point, Exposing SimpleLoginBackend in the auth contrib package could open it up to be abused by others.

comment:14 Changed 17 months ago by alasdair

jfilipes approach looks promising, so I have unassigned the ticket from myself.

Also, the term "simple" seems a little vague to me. Something like force_login(user), or login(user, force=True) would seem more explicit.

Adding force=True to the login method would be backwards incompatible. As mjtamlyn says earlier in the comments, login currently takes **kwargs which get passed to the auth backend.

comment:15 Changed 3 months ago by alasdairnicol

  • Owner alasdair deleted
  • Status changed from assigned to new

comment:16 Changed 3 months ago by jdufresne

I started looking into this ticket after the recommendation in ticket #24987. I have started a WIP branch based on the ideas and PR of jfilipes. Early feedback and comments are welcome. If the approach is well received I'll try to continue down that path to completion.

https://github.com/django/django/pull/4865

Thanks.

comment:17 Changed 2 months ago by jdufresne

  • Cc jon.dufresne@… added
  • Needs documentation unset
  • Patch needs improvement unset

Received a positive review from mjtamlyn, so removing some flags.

More reviews and feedback is welcome. Thanks.

comment:18 Changed 2 months ago by mjtamlyn

  • Triage Stage changed from Accepted to Ready for checkin

comment:19 Changed 2 months ago by timgraham

  • Summary changed from Provide a "simple_login" feature for the test client to Provide a "force_login" feature for the test client

comment:20 Changed 2 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In b44dee16:

Fixed #20916 -- Added Client.force_login() to bypass authentication.

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