[Django] #20916: Provide a "simple_login" feature for the test client
Django
-----------------------------------+--------------------
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords: auth
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20916>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
Would it make sense to add a parameter to `login` rather than making it a
separate method?
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:1>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
The issue with that is that at present `login` takes `**kwargs` which get
passed to the auth backend.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:2>
Django
Reporter: mjtamlyn | Owner: jfilipe
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jfilipe):
* status: new => assigned
* owner: nobody => jfilipe
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:3>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by alasdair):
* owner: jfilipe => alasdair
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:4>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:5>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:6>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:7>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:8>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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/jfilipe/django/compare/django:master...simple-login
If you guys like this approach I can add some docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:9>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:10>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:11>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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!
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:12>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:13>
Django
Reporter: mjtamlyn | Owner: alasdair
Type: New feature | Status: assigned
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:14>
Django
Reporter: mjtamlyn | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by alasdairnicol):
* owner: alasdair =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:15>
Django
Reporter: mjtamlyn | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:16>
Django
Reporter: mjtamlyn | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* cc: jon.dufresne@… (added)
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
Comment:
Received a positive review from mjtamlyn, so removing some flags.
More reviews and feedback is welcome. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:17>
Django
Reporter: mjtamlyn | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: mjtamlyn | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: Testing framework | Version: master
Keywords: auth | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* owner: => Tim Graham <timograham@…>
* resolution: => fixed
Comment:
In [changeset:"b44dee16e60ff2600fee90576e5bf0083c266104" b44dee16]:
{{{
#!CommitTicketReference repository=""
revision="b44dee16e60ff2600fee90576e5bf0083c266104"
Fixed #20916 -- Added Client.force_login() to bypass authentication.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20916#comment:20>