Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7776 closed (fixed)

Test cookie isn't deleted if persistent data in Newforms-Admin login

Reported by: Michael Newman Owned by: Brian Rosner
Component: contrib.admin Version: dev
Severity: Keywords: Admin auth cookies
Cc: rajesh.dhawan@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The test is probably the most simple way to see this. Right now a test cookie might be left in the users session due to it only being removed if there is no persistent data. The fix is simply moving the delete_test_cookie up a few lines and I added to the admin_view tests to check to make sure there isn't a cookie left after the client is logged in.

Attachments (1)

7776-delete-cookie.diff (2.3 KB ) - added by Michael Newman 16 years ago.

Download all attachments as: .zip

Change History (11)

by Michael Newman, 16 years ago

Attachment: 7776-delete-cookie.diff added

comment:1 by Brian Rosner, 16 years ago

Owner: changed from nobody to Brian Rosner
Status: newassigned
Triage Stage: UnreviewedReady for checkin

comment:2 by Michael Newman, 16 years ago

Version: newforms-adminSVN

comment:3 by Rajesh Dhawan, 16 years ago

session.test_cookie_delete() will raise an Exception if testcookie does not exist in the session. So, we should test that the cookie worked before calling test_cookie_delete:

if request.session.test_cookie_worked():
    request.session.delete_test_cookie()

comment:4 by Rajesh Dhawan, 16 years ago

Cc: rajesh.dhawan@… added

comment:5 by anonymous, 16 years ago

milestone: 1.0

rajeshdhawan: You can see on line 244:

if not request.session.test_cookie_worked():
     message = _("Looks like your browser isn't configured to accept cookies. Please enable cookies, reload this page, and try again.")
     return self.display_login_form(request, message)

which does the same check you are purposing and short circuits long before it reaches this code.

This is a bug in django and I am marking for 1.0 so it doesn't get forgotten, correct me if I am wrong.

comment:6 by Rajesh Dhawan, 16 years ago

anonymous: Yes, I see that it does the same check. It's strange, though, because I have been lately seeing the following trace (I got a few of these this morning using the latest svn checkout):

Traceback:
File "/home/rajesh/Development/default-django/django/core/handlers/base.py"
in get_response 86. response = callback(request, *callback_args, **callback_kwargs)
File "/home/rajesh/Development/default-django/django/contrib/admin/sites.py"
in root 156. return self.login(request)
File "/home/rajesh/Development/django-svn/django/views/decorators/cache.py"
in _wrapped_view_func 44. response = view_func(request, *args, **kwargs)
File "/home/rajesh/Development/default-django/django/contrib/admin/sites.py"
in login 280. request.session.delete_test_cookie()
File "/home/rajesh/Development/default-django/django/contrib/sessions/backends/base.py"
in delete_test_cookie 84. del self[self.TEST_COOKIE_NAME]
File "/home/rajesh/Development/default-django/django/contrib/sessions/backends/base.py"
in __delitem__ 53. del self._session[key]
Exception Type: KeyError at /admin/ Exception Value: 'testcookie'

I haven't traced this down to the session.delete_test_cookie function to be able to confirm that it's a bug in the sessions module (it happens very intermittently, so it's hard to debug).

comment:7 by Russell Keith-Magee, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [8484]) Fixed #7776: Ensured that the test cookie is always deleted once a login has succeeded. Thanks for the report and fix, Mnewman.

comment:8 by Rajesh Dhawan, 16 years ago

Resolution: fixed
Status: closedreopened

Russell: This still throws the error I mentioned above. Previously, I thought it was an intermittent problem but now I have been able to narrow it down to the case where an already logged in user is logging in to admin as a different user. In such a case, the login call on line 274 has the side effect of flushing/clearing out the session of the previously logged in user (as of [8343]). So, this empty session causes the delete_test_cookie call to fail with a KeyError.

I think that the test cookie should only be deleted after checking request.session.test_cookie_worked() again. The test_cookie_worked call at line 244 is not sufficient.

comment:9 by Michael Newman, 16 years ago

Resolution: fixed
Status: reopenedclosed

rajeshdhawan: The error you are having is clearly a different problem. If you are having a problem please open another ticket. the bug that I reported and described has been fixed. Thanks Russell

comment:10 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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