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)
Change History (41)
comment:1 by , 14 years ago
comment:2 by , 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 , 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 , 14 years ago
Attachment: | requestfactory_for_login.diff added |
---|
Client.login to use RequestFactory
comment:4 by , 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 , 14 years ago
Test against Client.login request middlewares and params
comment:5 by , 14 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → 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 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:14 by , 12 years ago
Cc: | added |
---|
comment:17 by , 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 , 11 years ago
comment:19 by , 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 , 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 , 11 years ago
Yep, tests run smoothly, so self.request()._request
instead of self.handler(self._base_environ)._request
;-)
comment:22 by , 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.
comment:23 by , 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 , 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:29 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:30 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 11 years ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
Severity: | Normal → Release blocker |
Triage Stage: | Ready for checkin → 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:33 by , 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 , 11 years ago
Cc: | added |
---|
comment:35 by , 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 , 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 , 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 , 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 , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:42 by , 11 years ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Severity: | Release blocker → Normal |
Status: | closed → new |
Reverted from master/1.7 for now and removed release blocker flag.
comment:43 by , 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 , 8 months ago
Cc: | added |
---|
I'm too hasty here, sorry.
Apparently I also need to be able to define
request.META['HTTP_HOST']
to the fake request indjango.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 theClient.get()
where one can define theHTTP_HOST
of the request object etc. Naturally there has to be a backwards compatible way to runClient.login()
, maybe even different named method?