Opened 2 years ago

Closed 2 years ago

#19327 closed Bug (fixed)

Admin doesn't handle double login attempts

Reported by: KJ Owned by: KJ
Component: contrib.admin Version: master
Severity: Normal Keywords: sensitive_post_parameters, login
Cc: KJ 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 2 years ago.
Test for KJ's patch. Should the message be tested, too?

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years ago by KJ

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 2 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 2 years ago by russellm

  • 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.

Changed 2 years ago by adupin

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

comment:4 Changed 2 years ago by adupin

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

  • Cc KJ 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 Changed 2 years ago by julien

  • 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 Changed 2 years ago by KJ

  • Patch needs improvement unset

Patch updated to satisfy above conditions.

comment:8 Changed 2 years ago by julien

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

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

comment:10 Changed 2 years ago by slurms

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks good, all tests pass under sqlite.

comment:11 Changed 2 years ago by ptone

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 2 years ago by Preston Holmes <preston@…>

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

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