Code

Opened 6 years ago

Closed 6 years ago

#8061 closed (invalid)

Allow login view to check test cookie

Reported by: jcassee Owned by: jcassee
Component: contrib.sessions Version: master
Severity: Keywords:
Cc: joost@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

The solution to ticket #3393, change [7692], removed the check for the test cookie on the login form. This was because that check breaks POSTs from login forms in views that don't call session.set_test_cookie(). On the other hand it break useful functionality (as mentioned in the ticket log). The attached patch adds a keyword parameter to the login ('check_test_cookie', default False) that if True will cause the request object to be passed to the AuthenticationForm.

The problem of having to call session.set_test_cookie() can by alleviated by always setting the test cookie unless the session cookie is present. This would not have to be a large overhead, as in the common case the session cookie is already set. (If that part of the ticket is accepted, then check_test_cookie could be made True by default or change [7692] be reverted.)

Two patches are attached to this ticket:

  • login-view.diff adds the extra parameter to the login view.
  • session-middleware.diff adds the test cookie to the response unless the session cookie was received

These patches are still pretty rough, documentation and tests would have to be written.

Attachments (5)

login-view.diff (1.1 KB) - added by jcassee 6 years ago.
session-middleware.diff (691 bytes) - added by jcassee 6 years ago.
check-session-cookie.diff (2.4 KB) - added by jcassee 6 years ago.
temporary-session-key-login.diff (1.8 KB) - added by jcassee 6 years ago.
temporary-session-key.diff (4.1 KB) - added by jcassee 6 years ago.
Fixed silly 'self' bug

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by jcassee

Changed 6 years ago by jcassee

comment:1 Changed 6 years ago by jcassee

  • Cc joost@… added
  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Owner set to jcassee
  • Patch needs improvement unset
  • Status changed from new to assigned

The session-middleware.diff patch is incorrect. It does not handle all situations.

I just had another idea. Although set_test_cookie() may not have been called, the session cookie must have been set. The new patch check-session-cookie.diff changes the AuthenticationForm to check for the presence of the session cookie in the request. There are three cases where this logic fails:

  • When the login form is not hosted by Django. This is really a special case but if we can introduce the check_cookie parameter to the login view (like in login-view.diff) so that users can disable the check.
  • The browser rejects cookies, but has an old cookie from a previous session. The test_cookie_worked() method will catch this case. This is such a rare case that I think it's unimportant.

For the session cookie to always be present, it must have been modified at least once. The new patch automatically saves the session if it is new. This can be a nuisance because anonymous users without cookies will continuously create new sessions. That may be a big problem for large sites. I guess just using login-view.diff would be safest. Checking for cookies is trickier than I would have imagined!

comment:2 Changed 6 years ago by jcassee

  • Patch needs improvement set

Changed 6 years ago by jcassee

comment:3 Changed 6 years ago by jcassee

  • Component changed from Authentication to django.contrib.sessions
  • Patch needs improvement unset

The new version of the sessions patch, temporary-session-key.diff fixes the problems mentioned above. It always sets the session cookie, but leaves the key empty unless the session was modified. Whether the session cookie has been received back (and consequently whether cookies work) can be checked using session.accepts_cookies(). This allows the AuthenticationForm to check for cookies at any time.

Changing component to django.contrib.sessions to invite review on this patch.

I also updated the login patch to temporary-session-key-login.diff.

Changed 6 years ago by jcassee

Changed 6 years ago by jcassee

Fixed silly 'self' bug

comment:4 Changed 6 years ago by jcassee

  • Triage Stage changed from Unreviewed to Design decision needed

comment:5 Changed 6 years ago by jcassee

  • Resolution set to invalid
  • Status changed from assigned to closed

I have split off the session part of this ticket into #8122. If it is accepted I will create a new ticket for the AuthorizationForm change. Marking this ticket invalid.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.