Code

Opened 3 years ago

Last modified 2 months ago

#15179 new Bug

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

Reported by: Ciantic Owned by: akaariai
Component: Testing framework Version: 1.7-beta-2
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 Ciantic 3 years ago.
Client.login to use RequestFactory
tests.py (1.6 KB) - added by Ciantic 3 years ago.
Test against Client.login request middlewares and params
15179.diff (2.3 KB) - added by SmileyChris 3 years ago.
My take on a completely backwards compatible method

Download all attachments as: .zip

Change History (42)

comment:1 Changed 3 years ago by Ciantic

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 3 years ago by Ciantic

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 3 years ago by Ciantic

  • Has patch set
  • milestone set to 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 3 years ago by Ciantic

Client.login to use RequestFactory

comment:4 Changed 3 years ago by Ciantic

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 3 years ago by Ciantic

Test against Client.login request middlewares and params

comment:5 Changed 3 years ago by russellm

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

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 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by SmileyChris

My take on a completely backwards compatible method

comment:7 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 20 months ago by claudep

#18973 was a duplicate with another use case.

comment:14 Changed 20 months ago by mitar

  • Cc mmitar@… added

comment:15 Changed 15 months ago by tim.dawborn

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

comment:16 Changed 9 months ago by unaizalakain

  • Has patch unset
  • Owner changed from nobody to unaizalakain
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:17 Changed 9 months ago by unaizalakain

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 9 months ago by unaizalakain

  • Has patch set

comment:19 Changed 8 months 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 8 months ago by unaizalakain

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 8 months ago by unaizalakain

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

comment:22 Changed 8 months 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 8 months ago by loic84 (previous) (diff)

comment:23 Changed 8 months ago by unaizalakain

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 8 months 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 8 months ago by unaizalakain

Done, thanks for the review!!

comment:26 Changed 8 months ago by unaizalakain

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

comment:27 Changed 8 months ago by loic84

  • Triage Stage changed from Accepted to Ready for checkin

LGTM.

comment:28 Changed 8 months ago by akaariai

  • Owner changed from unaizalakain to akaariai

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

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

  • Resolution set to fixed
  • Status changed from assigned to closed

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 months ago by marfire

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to new

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 months ago by timo

  • Has patch unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Ready for checkin to Accepted

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 months ago by timo

  • Version changed from master to 1.7-beta-2

comment:33 Changed 3 months ago by marfire

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 months ago by marfire

  • Cc k@… added

comment:35 Changed 3 months 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 months ago by aaugustin

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 months 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 2 months ago by timo

  • 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 2 months ago by marfire

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 2 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 2 months 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 2 months ago by timo

  • Has patch unset
  • Resolution fixed deleted
  • Severity changed from Release blocker to Normal
  • Status changed from closed to new

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from akaariai to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.