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

[Jari Pennanen]

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)

[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?

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

[Jari Pennanen]

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.

[Jari Pennanen]

Client.login to use RequestFactory

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

[Jari Pennanen]

Test against Client.login request middlewares and params

[Russell Keith-Magee]

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

[Chris Beaven]

My take on a completely backwards compatible method

[Jacob]

Milestone 1.3 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Claude Paroz]

#18973 was a duplicate with another use case.

[tim.dawborn]

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

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

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

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

[Unai Zalakain]

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

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

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

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

[Unai Zalakain]

Done, thanks for the review!!

[Unai Zalakain]

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

[loic84]

LGTM.

[Anssi Kääriäinen <akaariai@…>]

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.

[Kevin Christopher Henry]

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

[Tim Graham]

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.

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

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

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

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

[Tim Graham]

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?

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

[Tim Graham <timograham@…>]

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.

[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

[Tim Graham]

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

[Simon Charette]

With the introduction of force_login in 1.9 I think that the latest concerns raised about Loic's patch can be addressed by pointing users are using force_login if they really want to avoid going through middleware.