Opened 14 years ago
Closed 12 years ago
#15198 closed Cleanup/optimization (fixed)
AuthenticationForm.clean call does not have request set
Reported by: | Jari Pennanen | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | alej0varas@…, zbigniew@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
I'm working on per site login system currently.
It is required to get the site of the request in prior to calling authenticate()
so the calls to my auth backend should be either in form of:
authenticate(site_id, username, password)
or more general:
authenticate(request, username, password)
If I try to create own django.contrib.auth.forms.AuthenticationForm
e.g. by subclassing, then the login view never sets the request
attribute of auth form.
So currently I have to recreate whole login view and authentication form in order to make per site login. With this simple patch I could just subclass AuthenticationForm and override the clean()
functions call to authentication.
More about my per site endeavours in django-devs thread, and in my proposal for changes to django.
Attachments (3)
Change History (15)
by , 14 years ago
Attachment: | authform_clean__no_request.diff added |
---|
comment:1 by , 14 years ago
Patch needs improvement: | set |
---|
Apparently this patch breaks something, probably having something to do with cookie testing.
(There is this cookie testing feature embeded in AuthenticationForm
, and that needs to be isolated from request
attribute.)
follow-up: 6 comment:2 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Seems like a reasonable suggestion -- although the issue with the cookie check obviously still needs to be resolved.
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:4 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:5 by , 12 years ago
Easy pickings: | set |
---|---|
Type: | New feature → Cleanup/optimization |
The Problem is not that the authenticate function needs the request. The form has the request parameter already in the contructor. So that your interhited own AuthForm can utilize that. But, the request attribute is not ever set. It is only given on non POST requests.
This inconsitency can be solved by patching the auth/views.py login function in that code
if request.method == "POST": form = authentication_form(data=request.POST) if form.is_valid():
has to be changed so
if request.method == "POST": form = authentication_form(data=request.POST, request=request) if form.is_valid():
then it is also conistent to the bottom code in that function where the request is already is given into that form
else: form = authentication_form(request)
comment:6 by , 12 years ago
Replying to russellm:
Seems like a reasonable suggestion -- although the issue with the cookie check obviously still needs to be resolved.
the cookie issue can also easy be patched so that it behave the same as it was before
def check_for_test_cookie(self): if self.request and self.request.method != "POST" and not self.request.session.test_cookie_worked(): raise forms.ValidationError(self.error_messages['no_cookies'])
by , 12 years ago
Attachment: | safe-patch-cookie.diff added |
---|
this should avoid the cookie issue when the request is avaliable on both request types get and post
comment:7 by , 12 years ago
#19446 was a duplicate (addressing the cookie test issue). About that cookie test: in any case, when cookies are disabled in the browser, the csrf_protect
decorator will short-circuit the login process and return a 403 Forbidden response to the POST request. Therefore I think we could drop the cookie test altogether.
comment:8 by , 12 years ago
Patch needs improvement: | unset |
---|
comment:10 by , 12 years ago
Needs tests: | unset |
---|
Added tests. Pull request here: https://github.com/django/django/pull/644. One question: should we also remove the other test cookie checks in login(): https://github.com/django/django/blob/master/django/contrib/auth/views.py#L48?
comment:11 by , 12 years ago
Cc: | added |
---|---|
Triage Stage: | Accepted → Ready for checkin |
The patch from https://github.com/django/django/pull/644 applies cleanly against current master, all tests pass.
comment:12 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
AuthenticationForm is called without request