Opened 4 years ago

Closed 2 years ago

#15198 closed Cleanup/optimization (fixed)

AuthenticationForm.clean call does not have request set

Reported by: Ciantic Owned by: nobody
Component: contrib.auth Version: master
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 Ciantic 4 years ago.
AuthenticationForm is called without request
safe-patch-cookie.diff (597 bytes) - added by sven.assmann@… 3 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 claudep 2 years ago.
Remove cookie test in login view

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by Ciantic

AuthenticationForm is called without request

comment:1 Changed 4 years ago by Ciantic

  • Needs documentation unset
  • Needs tests unset
  • 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 follow-up: Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:4 Changed 4 years ago by alej0

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

comment:5 Changed 3 years ago by sven.assmann@…

  • Easy pickings set
  • Type changed from New feature to 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 in reply to: ↑ 2 Changed 3 years ago by sven.assmann@…

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'])

Changed 3 years ago by sven.assmann@…

this should avoid the cookie issue when the request is avaliable on both request types get and post

comment:7 Changed 2 years ago by claudep

#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.

Changed 2 years ago by claudep

Remove cookie test in login view

comment:8 Changed 2 years ago by claudep

  • Patch needs improvement unset

comment:9 Changed 2 years ago by anonymous

  • Needs tests set

Patch looks good, needs tests though.

comment:10 Changed 2 years ago by slurms

  • 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 Changed 2 years ago by zsiciarz

  • Cc zbigniew@… added
  • Triage Stage changed from Accepted to Ready for checkin

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

comment:12 Changed 2 years ago by Preston Holmes <preston@…>

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

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