Opened 11 years ago

Closed 11 years ago

#19327 closed Bug (fixed)

Admin doesn't handle double login attempts

Reported by: Krzysztof Jurewicz Owned by: Krzysztof Jurewicz
Component: contrib.admin Version: dev
Severity: Normal Keywords: sensitive_post_parameters, login
Cc: Krzysztof Jurewicz Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

When sending login form on admin site when user is already logged in, admin view gets called as if no login attempt was being made. Login form POST data can then easily cause some of the admin views to break. Furthermore, sensitive_post_parameters decorator isn't applied because login view doesn't get called, so if an exception is raised, a traceback is emailed with username and password in plain text.

A real life example would be when user opens 2 tabs with login form, logs in on one of them and then forgets about it and tries to log in on the second.

Attachments (1)

19327.diff (949 bytes ) - added by adupin 11 years ago.
Test for KJ's patch. Should the message be tested, too?

Download all attachments as: .zip

Change History (13)

comment:1 by Krzysztof Jurewicz, 11 years ago

Has patch: set

I've created a pull request: https://github.com/django/django/pull/545

comment:2 by Russell Keith-Magee, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Russell Keith-Magee, 11 years ago

Needs tests: set
Patch needs improvement: set

Problem definitely exists, but:

  • The patch needs tests. This is something the test client can test really easily, so there's not reason not to.
  • There are more reliable checks for "is logged in" than checking the contents of POST -- you should be able to check request.user.is_authenticated to evaluate if the user is logged in.

by adupin, 11 years ago

Attachment: 19327.diff added

Test for KJ's patch. Should the message be tested, too?

comment:4 by adupin, 11 years ago

Regarding the check if the user is authenticated: If i got it right, the if-statement in line 198 checks the user's permissions (i.e. is_active and is_staff) so only authenticated users can reach the lines from KJ's patch. There, checking for LOGIN_FORM_KEY seems to be the right way to find out if it's a login attempt by an already authenticated user.

comment:5 by Krzysztof Jurewicz, 11 years ago

Cc: Krzysztof Jurewicz added
Needs tests: unset
Patch needs improvement: unset

+1 for adupin's argumentation about the if statement.

I think that tests for this ticket should be placed in admin_views, not in admin_custom_urls, so I've put one there and updated the pull request. Checking of error message is included. I didn't actually use adupin's patch because tests in admin_views are written in a different manner.

BTW, I didn't write a test at first because I didn't notice that those are actually present for the admin. The documentation says tests for contrib apps live in django/contrib/<app>/tests - I'll create another ticket about that.

comment:6 by Julien Phalip, 11 years ago

Patch needs improvement: set

Thanks for the patch. There is a related issue which I think should be fixed at the same time as this.

To reproduce:

  • Log out from the admin.
  • Open the admin index in two separate tabs, without logging in yet. You should see the login form in both tabs.
  • In the first tab, enter the correct credentials to log in successfully.
  • In the second tab, enter incorrect credentials (e.g. wrong password).

The problem is that you will get through in the second tab even though the credentials were incorrect.

We should make sure that the login form always checks the entered credentials regardless of whether or not a user is already logged in. Also, if the entered credentials are wrong, then the user should get logged out.

comment:7 by Krzysztof Jurewicz, 11 years ago

Patch needs improvement: unset

Patch updated to satisfy above conditions.

comment:8 by Julien Phalip, 11 years ago

I can't seem to find the updated patch. Did you forget to attach it or to update the pull request? Or am I missing something? Thanks!

comment:9 by Krzysztof Jurewicz, 11 years ago

Pull request is updated, but changes were added to the previous commit, with date unchanged. Commit hash is now e83ea372ff3f93ef53e2d037c5277b4bc60a5eb5.

comment:10 by Nick Sandford, 11 years ago

Triage Stage: AcceptedReady for checkin

Patch looks good, all tests pass under sqlite.

comment:11 by Preston Holmes, 11 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

One further situation, similar to the one Julien brings up, is that if the second login is valid, but a different user, the current user should be logged out. Currently the patch just issues a message and proceeds.

The truth is, both this scenario and the one Julien mentioned, the browser has been left by one user with a valid session - and so this isn't really a Django security issue per se, but if we are going to clean and tighten this up, might as well cover all the bases here with multiple tabs in a potentially shared environment.

Otherwise this is looking good.

comment:12 by Preston Holmes <preston@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 9d6ecc6bc668b5a243905486fa724d53508ad2b5:

Fixed #19327 -- Added handling of double login attempts in admin.

Thanks to Krzysztof Jurewicz for initial patch and
adupin for tests.

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