Opened 13 years ago

Closed 11 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)

authform_clean__no_request.diff (557 bytes ) - added by Jari Pennanen 13 years ago.
AuthenticationForm is called without request
safe-patch-cookie.diff (597 bytes ) - added by sven.assmann@… 12 years ago.
this should avoid the cookie issue when the request is avaliable on both request types get and post
15198-3.diff (3.2 KB ) - added by Claude Paroz 11 years ago.
Remove cookie test in login view

Download all attachments as: .zip

Change History (15)

by Jari Pennanen, 13 years ago

AuthenticationForm is called without request

comment:1 by Jari Pennanen, 13 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.)

comment:2 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

Seems like a reasonable suggestion -- although the issue with the cookie check obviously still needs to be resolved.

comment:3 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

comment:4 by Alejandro Varas, 13 years ago

Cc: alej0varas@… added
Easy pickings: unset
UI/UX: unset

comment:5 by sven.assmann@…, 12 years ago

Easy pickings: set
Type: New featureCleanup/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)

in reply to:  2 comment:6 by sven.assmann@…, 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 sven.assmann@…, 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 Claude Paroz, 11 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.

by Claude Paroz, 11 years ago

Attachment: 15198-3.diff added

Remove cookie test in login view

comment:8 by Claude Paroz, 11 years ago

Patch needs improvement: unset

comment:9 by anonymous, 11 years ago

Needs tests: set

Patch looks good, needs tests though.

comment:10 by Nick Sandford, 11 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 Zbigniew Siciarz, 11 years ago

Cc: zbigniew@… added
Triage Stage: AcceptedReady for checkin

The patch from https://github.com/django/django/pull/644 applies cleanly against current master, all tests pass.

comment:12 by Preston Holmes <preston@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 22d82a7742c3b091857fda8612273360459110ee:

Fixed #15198 -- pass request to AuthenticationForm

Thanks to Ciantic for the report, claudep and slurms for initial work

Note: See TracTickets for help on using tickets.
Back to Top