Opened 12 years ago
Closed 12 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)
Change History (13)
comment:1 by , 12 years ago
Has patch: | set |
---|
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 12 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 , 12 years ago
Attachment: | 19327.diff added |
---|
Test for KJ's patch. Should the message be tested, too?
comment:4 by , 12 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 , 12 years ago
Cc: | 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 , 12 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 , 12 years ago
Patch needs improvement: | unset |
---|
Patch updated to satisfy above conditions.
comment:8 by , 12 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 , 12 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 , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Patch looks good, all tests pass under sqlite.
comment:11 by , 12 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I've created a pull request: https://github.com/django/django/pull/545