Opened 3 weeks ago
Closed 3 weeks ago
#36717 closed Cleanup/optimization (fixed)
Admin login should redirect already logged-in users to page specified in next parameter
| Reported by: | Benedict Etzel | Owned by: | Benedict Etzel |
|---|---|---|---|
| Component: | contrib.admin | Version: | 5.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Benedict Etzel | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The admin login page at /admin/login/ ignores the ?next= parameter when an already logged-in user visits the page. This differs from the behaviour when the user logs in on the admin login page itself, where they are redirected to the page indicated in the parameter.
Background
When visiting the contrib.admin, the AdminSite.login method redirects logged-out users to a URL that looks like this: /admin/login/?next=/admin/my_app/my_model/. The next field (which is the value of contrib.auth.REDIRECT_FIELD_NAME) is then used to populate a hidden field login form (contrib.auth.LoginView).
After signing in, the contrib.auth.RedirectURLMixin of the LoginView redirects the user to the initially requested URL.
However, sometimes may re-visit /admin/login/?next=/admin/my_app/my_model/ when they're already signed in. For example:
- by signing in to the site in another tab
- by using the browser's back button
- by following a bookmark
- after custom authenticating overriding
admin.site.login(a good example is django-allauth'ssecure_django_loginhttps://docs.allauth.org/en/dev/common/admin.html that redirects visitors to/admin/login/to a custom login and then back to/admin/login/. Even though it keepsnextintact, Django will always redirect the - already logged-in - user back to the admin index)
Currently, AdminSite.login method disregards any next param in the URL for logged-in staff users and always redirects these to admin:index. This is slightly annoying because the user could be redirected these to the requested URL instead.
To reproduce:
- Sign out of your Django instance
- Manually open an admin deeplink such as
/admin/my_app/my_model/, which should redirect to/admin/login/?next=/my_app/my_model/ - In another tab, sign in to your Django instance as staff
- Back in the original tab, refresh the page. Observe how you are sent to
/admin/, instead of/admin/my_app/my_model/
Possible fix:
AdminSite.login could respect the redirect URL, and should probably run checks like contrib.auth.views.RedirectURLMixin (specifically get_redirect_url), to ensure that the URL is safe. I don't think there's a security issue here where somebody malicious links an admin to /admin/login/?next=/attacker-provider, because the attacker could already link a user directly to that page (or to the login form that then redirects the admin to the page).
I'm assigning this to myself for now to "claim" implementing this. I already have a working prototype locally.
Attachments (1)
Change History (8)
comment:1 by , 3 weeks ago
| Cc: | added |
|---|
comment:2 by , 3 weeks ago
comment:3 by , 3 weeks ago
| Has patch: | set |
|---|
I have a patch now in the linked branch.
The url_if_safe check comes from contrib.auth.views.RedirectURLMixin (https://github.com/django/django/blob/77d455ae73b177a32102f0b248828b5d63c0aa24/django/contrib/auth/views.py#L48-L56), with the exception of the overridable success_url_allowed_hosts - I'm not sure how we could do that here (except for adding to success_url_allowed_hosts to AdminSite).
by , 3 weeks ago
comment:5 by , 3 weeks ago
| Triage Stage: | Unreviewed → Accepted |
|---|---|
| Type: | Bug → Cleanup/optimization |
Hello Benedict, thank you for your report! I have reproduced as you described and I have also noticed this behavior in the past. I agree is fairly unexpected and that we can do better. Accepting on that basis, I'll try to review the branch soon.
comment:6 by , 3 weeks ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
I've attached the failing test case. I'll be working on this over in https://github.com/beheh/django/tree/ticket_36717.