Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#7776 closed (fixed)

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

Reported by: Mnewman Owned by: brosner
Component: contrib.admin Version: master
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: UI/UX:

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 Mnewman 7 years ago.

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Mnewman

comment:1 Changed 7 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to brosner
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 7 years ago by Mnewman

  • Version changed from newforms-admin to SVN

comment:3 Changed 7 years ago by rajeshdhawan

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 Changed 7 years ago by rajeshdhawan

  • Cc rajesh.dhawan@… added

comment:5 Changed 7 years ago by anonymous

  • milestone set to 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 Changed 7 years ago by rajeshdhawan

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 Changed 7 years ago by russellm

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

(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 Changed 7 years ago by rajeshdhawan

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 7 years ago by Mnewman

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

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 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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