Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#25163 closed Cleanup/optimization (fixed)

Add a hint to the admin login page when a user is redirected there due to lack of permissions

Reported by: Jan Pazdziora Owned by: Tim Graham <timograham@…>
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Assume application which uses django.contrib.auth.views.login with some custom template to allow the users to log in. Even users that are not staff can therefore log in.

While authenticated with this non-staff user, access to /admin gets redirected to /admin/login which shows the Django administration logon form. So that page (and any access to /admin) behaves as if the user was not authenticated. No information clarifying that "while you are authenticated as david, you are unfortunately not authorized to access this page -- would you care to re-login?" What's more, the user stays authenticated, so when they edit the location in their browser to access some non-admin site, they are back as authenticated user.

Maybe when the user is not authorized, it should be clearly spelled out on the admin login screen, giving the user a chance to logout and re-login?

I was able to reproduce this behaviour without any remote user authentication set up, even if that is eventually the environment where I'd like the authentication to also work.

Note: Not sure if this is more about django.contrib.admin or django.contrib.auth, filing under contrib.admin because there's where I can demonstrate it easily.

Change History (21)

comment:1 Changed 5 years ago by Tim Graham

Rather than redirecting to login, it might be better to raise a 404 on access to unauthorized pages so as not to leak what pages of the admin exist. Thoughts?

comment:2 Changed 5 years ago by Carl Meyer

That's what I generally do when an authenticated user accesses a page they don't have permissions for. There's no reason they should even be able to tell whether that page exists. +1 to returning a 404 instead.

comment:3 Changed 5 years ago by Jan Pazdziora

Not sure if I'd be in favour of 404. If we'd wanted to strictly follow HTTP RFCs, 403 would probably be a better call.

But the important thing here is probably not the status but the content of that 404/403 page. We probably want to preserve the chance for the user to recover from the situation -- log in as different user, for example.

comment:4 Changed 5 years ago by Tim Graham

Summary: When user does not have permission, /admin redirects to /admin/login but user is still authenticatedWhen a user doesn't have permission to an admin page, raise 404 instead of redirecting to login
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

As to 403 vs 404, as Carl said, "There's no reason they should even be able to tell whether that page exists."

I would expect a user to reach such a page only by typing in a URL manually, therefore there's not really a need to be able to recover gracefully from it. Of course, you can use a custom handler404 to customize that page if you like.

comment:5 Changed 5 years ago by Konrad Świat

Owner: changed from nobody to Konrad Świat
Status: newassigned

comment:6 in reply to:  4 Changed 5 years ago by Jan Pazdziora

Replying to timgraham:

I would expect a user to reach such a page only by typing in a URL manually, ...

I'm not sure I agree here. The /admin link is really no secret and projects are likely to have it shown all over the places, in the form of "and by the way, if you think you are the admin, here's the link, don't worry, we will ask for your credentials if you are currently not logged in". And if the user is not currently logged in, that's exactly what happens.

The problem starts when the user is logged in, just not as staff.

Yes, you can say that in that case the link to /admin shouldn't be displayed at all but we have no way to enforce that in code that people write, and besides, why should authenticated user see less than anonymous?

Or is the general recommendation to never place link to /admin anywhere?

comment:7 Changed 5 years ago by Tim Graham

The point about authenticated users seeing less than anonymous users seems to be a good one. It seems raising 404 instead of redirecting wouldn't really give any security improvement unless we also raised 404s on all admin pages (except login) for anonymous users. That could be fairly inconvenient for users who have their session timeout and try to access one of the admin pages with a deeper URL. They'd have to navigate back the login page manually instead of being redirected to login with a "next" query string parameter.

comment:8 Changed 5 years ago by Konrad Świat

Owner: Konrad Świat deleted
Status: assignednew

comment:9 Changed 5 years ago by Carl Meyer

Yes, the 404 idea would require more work, and would have some negative consequences.

I hope that the redirect to login in the admin is already unconditional for any url under admin/ and doesn't currently reveal information about the available admin urls. Need to check that.

I think the best resolution for this bug is probably some ux hint on the admin login screen for a user who is already authenticated, as mentioned in the ticket description. I may be missing something, but I don't see how that would be a problem or reveal anything that shouldn't be revealed.

comment:10 Changed 5 years ago by Carl Meyer

(Fwiw I don't believe that I have ever included a public admin link in any Django project I've worked on. Usually I put the admin at a less predictable url, and give that url directly to the few people who need to use it.)

Last edited 5 years ago by Carl Meyer (previous) (diff)

comment:11 Changed 5 years ago by Konrad Świat

Hiding urls might be tricky, e.g. i18n_patterns can reveal url existance.

Last edited 5 years ago by Konrad Świat (previous) (diff)

comment:12 Changed 5 years ago by Luis Visintini

Owner: set to Luis Visintini
Status: newassigned

comment:13 Changed 5 years ago by Tim Graham

lvisintini, what's your plan to resolve this ticket? We need to make sure the solution is agreeable.

comment:14 Changed 5 years ago by Luis Visintini

Like carljm, I think that a hint in the login page is a good fix for this.

I have been overly enthusiastic and coded in something for this already, along with a test.

If the user is authenticated, then the message in this ticket description is displayed as a <p class="errornote">

Should I push that branch for review?

comment:15 Changed 5 years ago by Luis Visintini

happy to hold of on doing that or torch my branch if not required/needed

comment:16 Changed 5 years ago by Tim Graham

Sure, happy to look at your work.

comment:17 Changed 5 years ago by Luis Visintini

Has patch: set

comment:18 Changed 5 years ago by Luis Visintini

Owner: Luis Visintini deleted
Status: assignednew

comment:19 Changed 5 years ago by Tim Graham

Summary: When a user doesn't have permission to an admin page, raise 404 instead of redirecting to loginAdd a hint to the admin login page when a user is redirected there due to lack of permissions

comment:20 Changed 5 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 635ffc3c:

Fixed #25163 -- Added hint for non-staff users to admin login page.

comment:21 Changed 5 years ago by Claude Paroz <claude@…>

In 6ed613b2:

Refs #25163 -- Added trimmed option to recent blocktrans addition

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