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's secure_django_login https://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 keeps next intact, 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:

  1. Sign out of your Django instance
  2. Manually open an admin deeplink such as /admin/my_app/my_model/, which should redirect to /admin/login/?next=/my_app/my_model/
  3. In another tab, sign in to your Django instance as staff
  4. 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)

test.diff (1.2 KB ) - added by Benedict Etzel 3 weeks ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Benedict Etzel, 3 weeks ago

Cc: Benedict Etzel added

comment:2 by Benedict Etzel, 3 weeks ago

I've attached the failing test case. I'll be working on this over in https://github.com/beheh/django/tree/ticket_36717.

comment:3 by Benedict Etzel, 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 Benedict Etzel, 3 weeks ago

Attachment: test.diff added

comment:5 by Natalia Bidart, 3 weeks ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/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 Natalia Bidart, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:7 by nessita <124304+nessita@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 5401b12:

Fixed #36717 -- Redirect authenticated users on admin login view to next URL.

Co-authored-by: Natalia <124304+nessita@…>

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