#3393 closed (fixed)
login view assumes 'set_test_cookie' has been called
Reported by: | James Bennett | Owned by: | Jacob |
---|---|---|---|
Component: | Contrib apps | Version: | dev |
Severity: | Keywords: | auth login set_test_cookie | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Currently, django.contrib.auth.views.login
instantiates AuthenticationForm
by passing it the request, which is optional; when this happens, AuthenticationForm
will fail on valid logins unless set_test_cookie
has been called previously. The login
view itself does this, but this prevents easy reusability -- it's a fairly common use case to want to display a login box on each page for non-authenticated users, which means that each view in use must call set_test_cookie
to ensure that the POST to login
won't fail.
A simple solution to this would be to stop passing the request to AuthenticationForm
. Perhaps a more flexible solution would be for login
to begin taking a keyword argument to determine whether it should pass the request or not (and hence, whether set_test_cookie
will be required or not)
Attachments (2)
Change History (13)
comment:1 by , 18 years ago
comment:2 by , 18 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 17 years ago
3393.diff have problems with the deletion of the test-cookie, when it has not been set.
by , 17 years ago
Attachment: | 3393_2.diff added |
---|
comment:5 by , 17 years ago
I have just faced this problem. I was doing a post from a custom view and had the exactly same problem described by ubernostrum.
I've applied 3393_2.diff against trunk and it worked fine for me.
Any chance to get this merged?
comment:6 by , 17 years ago
I was seeing odd problems of a single-page login only working in weird circumstances. This patch fixed it immediately. +1 by me.
(I don't bother testing for cookie support anywhere in my app. Maybe I should?)
comment:7 by , 17 years ago
I am not sure, but maybe we should un-tick the "Patch needs improvement" flag for the triagers to evaluate the new patch ?
Otherwise it would help if they can indicate what types of improvements that are needed.
comment:8 by , 17 years ago
milestone: | → 1.0 |
---|
comment:9 by , 17 years ago
I get the point of this ticket, but it is killing existing functionality.
Perhaps a better solution would be to have an attribute on the view which, if set, avoids the set_test_cookie
option. Then people can just pass this arg to the login view in their url conf.
comment:10 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
And Jacob has suggested just yanking the test out of
AuthenticationForm
; I'll work on a patch for that, since it sounds like a better idea (though it means that any code which usesAuthenticationForm
will now need to remember to test for cookies on its own).