Opened 14 years ago

Last modified 8 months ago

#15179 new Bug

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

Reported by: Jari Pennanen Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: mmitar@…, k@…, Ülgen Sarıkavak 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 14 years ago.
Client.login to use RequestFactory
tests.py (1.6 KB ) - added by Jari Pennanen 14 years ago.
Test against Client.login request middlewares and params
15179.diff (2.3 KB ) - added by Chris Beaven 14 years ago.
My take on a completely backwards compatible method

Download all attachments as: .zip

Change History (41)

comment:1 by Jari Pennanen, 14 years ago

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 by Jari Pennanen, 14 years ago

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 by Jari Pennanen, 14 years ago

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.

by Jari Pennanen, 14 years ago

Client.login to use RequestFactory

comment:4 by Jari Pennanen, 14 years ago

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.

by Jari Pennanen, 14 years ago

Attachment: tests.py added

Test against Client.login request middlewares and params

comment:5 by Russell Keith-Magee, 14 years ago

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 by Łukasz Rekucki, 14 years ago

Severity: Normal
Type: Bug

by Chris Beaven, 14 years ago

Attachment: 15179.diff added

My take on a completely backwards compatible method

comment:7 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Claude Paroz, 12 years ago

#18973 was a duplicate with another use case.

comment:14 by Mitar, 12 years ago

Cc: mmitar@… added

comment:15 by tim.dawborn, 12 years ago

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

comment:17 by Unai Zalakain, 11 years ago

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 by Unai Zalakain, 11 years ago

comment:19 by loic84, 11 years ago

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 by Unai Zalakain, 11 years ago

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 by Unai Zalakain, 11 years ago

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

comment:22 by loic84, 11 years ago

@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 11 years ago by loic84 (previous) (diff)

comment:23 by Unai Zalakain, 11 years ago

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 by loic84, 11 years ago

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 by Unai Zalakain, 11 years ago

Done, thanks for the review!!

comment:26 by Unai Zalakain, 11 years ago

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

comment:27 by loic84, 11 years ago

Triage Stage: AcceptedReady for checkin

LGTM.

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

Resolution: fixed
Status: newclosed

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 by Kevin Christopher Henry, 11 years ago

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 by Tim Graham, 11 years ago

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:33 by Kevin Christopher Henry, 11 years ago

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 by Kevin Christopher Henry, 11 years ago

Cc: k@… added

comment:35 by loic84, 11 years ago

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 by Aymeric Augustin, 11 years ago

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 by loic84, 11 years ago

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 by Tim Graham, 11 years ago

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 by Kevin Christopher Henry, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham, 11 years ago

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

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

comment:43 by Simon Charette, 4 years ago

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.

comment:44 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top