Admin doesn't handle double login attempts

[Krzysztof Jurewicz]

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.

[Krzysztof Jurewicz]

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

[Russell Keith-Magee]

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.

[adupin]

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

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

[Krzysztof Jurewicz]

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

[Julien Phalip]

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.

[Krzysztof Jurewicz]

Patch updated to satisfy above conditions.

[Julien Phalip]

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!

[Krzysztof Jurewicz]

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

[Nick Sandford]

Patch looks good, all tests pass under sqlite.

[Preston Holmes]

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.

[Preston Holmes <preston@…>]

In 9d6ecc6bc668b5a243905486fa724d53508ad2b5:

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

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