Opened 6 years ago

Last modified 2 years ago

#15179 new Bug

django.test.client.Client.login fake HttpRequest is not run through middlewares

Reported by: Jari Pennanen Owned by: Anssi Kääriäinen
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: mmitar@…, k@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the django.test.client.Client.login the fake HttpRequest should run through MiddleWare lists process_request functions before login.

My authentication/login mechanism relies on few things in the request object and if the login is called with incorrect request object it does not work. Needles to say perhaps but it works in real client.

(I'm working on a per site login system, it seems I can make this fairly pluggable, but this is nagging me. If you are intersted see django-devs thread)

Attachments (3)

requestfactory_for_login.diff (602 bytes) - added by Jari Pennanen 6 years ago.
Client.login to use RequestFactory
tests.py (1.6 KB) - added by Jari Pennanen 6 years ago.
Test against Client.login request middlewares and params
15179.diff (2.3 KB) - added by Chris Beaven 6 years ago.
My take on a completely backwards compatible method

Download all attachments as: .zip

Change History (43)

comment:1 Changed 6 years ago by Jari Pennanen

I'm too hasty here, sorry.

Apparently I also need to be able to define request.META['HTTP_HOST'] to the fake request in django.test.client.Client.login. Since per site login is required to be verified against Host of the request.

So maybe making the Client.login() use more parameters? Like the Client.get() where one can define the HTTP_HOST of the request object etc. Naturally there has to be a backwards compatible way to run Client.login(), maybe even different named method?

comment:2 Changed 6 years ago by Jari Pennanen

Here is a little unit test where Client.get() test (test_client_request) works, but Client.login() (test_client_login) fails:

from django.conf.urls.defaults import * #@UnusedWildImport
from django.contrib.auth.models import User
from django.http import HttpResponse
from django.test import TestCase, Client
from django.contrib.auth.signals import user_logged_in
from django.dispatch import receiver

urlpatterns = patterns('',
    (r'^$', 'testing.tests.http_host_view'),
)

def http_host_view(request):
    return HttpResponse(request.get_host())

class DjangoClientRequestTests(TestCase):
    urls = 'testing.tests'

    def setUp(self):
        u = User.objects.create(username='tester')
        u.set_password('1234')
        u.save()

    def test_client_request(self):
        c = Client(enforce_csrf_checks=False, HTTP_HOST='myhost.com')
        self.assertEqual(c.get("/").content, 'myhost.com')

    def test_client_login(self):
        c = Client(enforce_csrf_checks=False, HTTP_HOST='myhost.com')
        
        @receiver(user_logged_in)
        def login(sender, signal, request, user):
            self.assertEqual(request.get_host(), 'myhost.com') # <---------- This fails!
            
        c.login(username='tester', password='1234')

This is probably a good way to start figuring out a patch for Client.login().

comment:3 Changed 6 years ago by Jari Pennanen

Has patch: set
milestone: 1.3
Needs tests: set

Sweet, I found (technically) a one line patch. The comment I put there is required since it is not obvious why one could not use super(Client, self).get("/login/")

Do we need tests for this? Disclaimer I have not run full django suite tests yet (since I forgot again how it is ran), but I'm hopeful.

Changed 6 years ago by Jari Pennanen

Client.login to use RequestFactory

comment:4 Changed 6 years ago by Jari Pennanen

My patch apparently fixes the second problem (which I mentioned in first comment), but the actual problem that middlewares are not run is not yet fixed. I'm working on that right now.

Changed 6 years ago by Jari Pennanen

Attachment: tests.py added

Test against Client.login request middlewares and params

comment:5 Changed 6 years ago by Russell Keith-Magee

Needs tests: unset
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Accepting the use case. However, the patch needs some work.

  • The patch should note that the reference to "/login" is arbitrary -- it's not actually using /login, it's just using that URL as a dummy location.
  • The test case isn't particularly good - it's checking a test condition that is a composite of multiple features. You should be checking that request has an attribute called "middlewares", and that the value is "+middlewares'. Checking a comparison of concatenated strings is just confusing the issue.
  • The test case should be integrated with Django's test suite (the test_client_regress tests would be a good location).

comment:6 Changed 6 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

Changed 6 years ago by Chris Beaven

Attachment: 15179.diff added

My take on a completely backwards compatible method

comment:7 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 4 years ago by Claude Paroz

#18973 was a duplicate with another use case.

comment:14 Changed 4 years ago by Mitar

Cc: mmitar@… added

comment:15 Changed 4 years ago by tim.dawborn

#20275 is another duplicate. Any chance of bumping this?

comment:16 Changed 3 years ago by Unai Zalakain

Has patch: unset
Owner: changed from nobody to Unai Zalakain
Patch needs improvement: unset
Status: newassigned

comment:17 Changed 3 years ago by Unai Zalakain

PR sent: https://github.com/django/django/pull/1824

Requests made with django.test.Client.login and
django.test.Client.logout respect defaults defined in django.test.Client
instantiation and are processed through middleware now.

comment:18 Changed 3 years ago by Unai Zalakain

Has patch: set

comment:19 Changed 3 years ago by loic84

I'm not too sure about this ticket.

login and logout would be the only requests that go through middleware. (refs. #9159)

Also going through the middleware is not necessarily sufficient to be useful, what if it the middleware tests for the HTTP verb (hardcoded to GET currently)? or for a specific IP address (hardcoded to '127.0.0.1') or even for a specific endpoint (hardcoded to '/'). For this to work, login and logout should accept **extra argument like the other methods.

comment:20 Changed 3 years ago by Unai Zalakain

It's kind of messy but all the get, post, etc. go through middleware.

This methods all call the self.generic method which in turn calls the self.request method. Now, this method doesn't do a big thing in RequestFactory but it's subclassed in Client and makes use of self.handler which builds the request through middleware.

Now, the self.request returns directly a response and in login and logout we need the source request also. I thought about returning (request, response) tuples in self.request but that wouldn't be backwards compatible so I directly call self.handler witch returns me the source request attached.

Now that I think it, if self.handler attaches the source request, I could also get it (I think) through self.request. Gonna look into it.

comment:21 Changed 3 years ago by Unai Zalakain

Yep, tests run smoothly, so self.request()._request instead of self.handler(self._base_environ)._request ;-)

comment:22 Changed 3 years ago by loic84

@unaizalakain, true for Client.request() ultimately being called.

The issue with the request environ remains though, what do you think?

It's worth noting that login already uses the **kwargs wildcard for **credentials, so it can't be used for **extra, IMO it's not a big deal if login and logout are not exactly consistent with the other methods as they operate at a higher level.

Last edited 3 years ago by loic84 (previous) (diff)

comment:23 Changed 3 years ago by Unai Zalakain

It's a pity that username and password kwargs cannot be assumed because of the auth backend plugability. Though it would be better not to, it seems to me that the only possible solution is to add a extra kwarg that accepts a dict although I don't know if it's a really needed feature (though it doesn't really hurt to have it) because of the possibility of setting custom env vars directly at the Client at instantiation or modifying Client.defaults.

comment:24 Changed 3 years ago by loic84

As mentioned on the PR, I would rename the newly added response._request to response.request_instance so it's not confusing that there is a response._request and a response._request which are two different things.

FTR: Ideally the old response.request would become a getter with a deprecation warning that proxies request_instance.environ, or be renamed to request_data; however making such property would require a custom Response class which is probably overkill.

comment:25 Changed 3 years ago by Unai Zalakain

Done, thanks for the review!!

comment:26 Changed 3 years ago by Unai Zalakain

Once again, thanks for your review, PR updated ;-)

comment:27 Changed 3 years ago by loic84

Triage Stage: AcceptedReady for checkin

LGTM.

comment:28 Changed 3 years ago by Anssi Kääriäinen

Owner: changed from Unai Zalakain to Anssi Kääriäinen

I am going to do some minor final polish & commit this one.

comment:29 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: assignedclosed

In 4fdd51b73240bf9c8d9472fcc45df699f0714755:

Fixed #15179 -- middlewares not applied for test client login()

Requests made with django.test.Client.login() and logout() respect
defaults defined in django.test.Client instantiation and are processed
through middleware.

Thanks to Loic for the reviews.

comment:30 Changed 3 years ago by Kevin Christopher Henry

Patch needs improvement: set
Resolution: fixed
Status: closednew

I think there's a serious problem with this approach.

The "fake" request is used to generate a real response, which means that any test that uses Client.login() will be coupled to the behavior of an arbitrary piece of view code (by default whatever handles a GET of '/'). So if that view happens to raise an exception, any test that uses Client.login() will fail. This kind of coupling does not seem like a good idea in the test suite (especially considering that such an exception needn't even be the result of a bug; it could just be that the test database wasn't put in an appropriate state, since that view isn't what's being tested).

comment:31 Changed 3 years ago by Tim Graham

Has patch: unset
Patch needs improvement: unset
Severity: NormalRelease blocker
Triage Stage: Ready for checkinAccepted

Seems like it could be a legitimate concern, although I'm not sure how often it will be a problem in practice.

It wasn't initially clear to me where the "real response" was generated; I believe that refers to the logic in the self.request() calls that were added.

Marking as a release blocker so we make a decision before the 1.7 release.

comment:32 Changed 3 years ago by Tim Graham

Version: master1.7-beta-2

comment:33 Changed 3 years ago by Kevin Christopher Henry

FWIW this isn't purely theoretical - I came across this after upgrading to 1.7b1 and getting a huge number of test failures on working code. (As I hinted at above, the failures were because I hadn't set up the test database to handle a GET on '/'.)

The relevant change is here. The call to request() does a full request / response cycle. Unfortunately I don't know this area of the code well enough to suggest a solution. Is there a way to point the code at a dummy view that just returns an HttpResponse?

Even with such a change there will still be some coupling with the other (non-authentication) middleware components. That doesn't seem like a problem, though, since it's hard to imagine any request-based test working if the middleware doesn't work. But if we care we could make this feature opt-in with a use_middleware keyword argument to login(). That would minimize coupling and ensure full backward-compatibility.

comment:34 Changed 3 years ago by Kevin Christopher Henry

Cc: k@… added

comment:35 Changed 3 years ago by loic84

How about taking advantage of https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.HttpRequest.urlconf.

We could set a custom urlconf that contains a login/logout view from contrib.auth and use these.

comment:36 Changed 3 years ago by Aymeric Augustin

Given the concerns expressed before committing this patch and the results seen during the beta, I would suggest to revert the change. The approach seems too fragile. It touches too many layers.

I would also have closed the ticket as wontfix.

Client.login() is a simple convenience feature. If it doesn't work, it isn't hard to write a LoginMixin providing a suitable login method for your website.

comment:37 Changed 3 years ago by loic84

I have a proof of concept, that makes Client.login() and logout() go through a normal request cycle:

https://github.com/loic/django/compare/ticket15179

This also unveiled a cache issue with @override_settings(ROOT_URLCONF).

comment:38 Changed 3 years ago by Tim Graham

Has patch: set

I like Loic's POC as it obsoletes a lot of the custom code in the Client.login() and logout() methods. My only concern is that there may be some subtle differences in behavior that are not tested. Worst case, I think we could revert this change in 1.7 and punt it to 1.8.

What do you think, Kevin?

comment:39 Changed 3 years ago by Kevin Christopher Henry

Agreed, I like the approach in Loic's patch. There's still an inherent tension here - anything that uses Client.login() is going to end up inadvertently testing the user's middleware. That seems OK to me, since I imagine that any test that uses Client.login() is going to end up making another request as well, so the middleware will be exercised in either case.

My imagination is limited, however. :-) Given where we are in the cycle (and Aymeric's objections), I would lean towards reverting this in 1.7 and taking Loic's approach going forward.

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

Resolution: fixed
Status: newclosed

In aabceadd7d7a61468b0dc7dc9d560a770abae0cf:

Revert "Fixed #15179 -- middlewares not applied for test client login()"

This reverts commit 4fdd51b73240bf9c8d9472fcc45df699f0714755.

See the ticket for concerns with this implementation; it will be revisited.

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

In 1d20693fa67f314de08e51af57af6fb5460b7b0c:

[1.7.x] Revert "Fixed #15179 -- middlewares not applied for test client login()"

This reverts commit 4fdd51b73240bf9c8d9472fcc45df699f0714755.

See the ticket for concerns with this implementation; it will be revisited.

Backport of aabceadd7d from master

comment:42 Changed 3 years ago by Tim Graham

Has patch: unset
Resolution: fixed
Severity: Release blockerNormal
Status: closednew

Reverted from master/1.7 for now and removed release blocker flag.

comment:43 Changed 2 years ago by Tim Graham

Version: 1.7-beta-2master
Note: See TracTickets for help on using tickets.
Back to Top