Opened 10 years ago

Closed 10 years ago

#8061 closed (invalid)

Allow login view to check test cookie

Reported by: Joost Cassee Owned by: Joost Cassee
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:


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 Joost Cassee 10 years ago.
session-middleware.diff (691 bytes) - added by Joost Cassee 10 years ago.
check-session-cookie.diff (2.4 KB) - added by Joost Cassee 10 years ago.
temporary-session-key-login.diff (1.8 KB) - added by Joost Cassee 10 years ago.
temporary-session-key.diff (4.1 KB) - added by Joost Cassee 10 years ago.
Fixed silly 'self' bug

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by Joost Cassee

Attachment: login-view.diff added

Changed 10 years ago by Joost Cassee

Attachment: session-middleware.diff added

comment:1 Changed 10 years ago by Joost Cassee

Cc: joost@… added
Has patch: set
Needs documentation: set
Needs tests: set
Owner: set to Joost Cassee
Status: newassigned

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 10 years ago by Joost Cassee

Patch needs improvement: set

Changed 10 years ago by Joost Cassee

Attachment: check-session-cookie.diff added

comment:3 Changed 10 years ago by Joost Cassee

Component: Authenticationdjango.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 10 years ago by Joost Cassee

Changed 10 years ago by Joost Cassee

Attachment: temporary-session-key.diff added

Fixed silly 'self' bug

comment:4 Changed 10 years ago by Joost Cassee

Triage Stage: UnreviewedDesign decision needed

comment:5 Changed 10 years ago by Joost Cassee

Resolution: invalid
Status: assignedclosed

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.

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