Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#25164 closed Bug (wontfix)

The django.contrib.auth.views.login does not observe authentication via RemoteUserMiddleware

Reported by: Jan Pazdziora Owned by: nobody
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no 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 and it works fine. Then django.contrib.auth.middleware.RemoteUserMiddleware is enabled. Even when the user is authenticated via the REMOTE_USER mechanism, the login page is still shown but any templates that check user.is_anonymous will see the user as authenticated, on that page. And with PersistentRemoteUserMiddleware on any page after that.

This is kind of related to https://code.djangoproject.com/ticket/25030 where it's been suggested that the solution should go to django.contrib.auth, and also https://code.djangoproject.com/ticket/25163 which might actually be just django.contrib.admin case of this more generic django.contrib.auth issue.

Change History (8)

comment:1 Changed 6 years ago by Jan Pazdziora

I wonder if the patch

+    if hasattr(request, 'user') and request.user.is_authenticated():
+        if not is_safe_url(url=redirect_to, host=request.get_host()):
+            redirect_to = resolve_url(settings.LOGIN_REDIRECT_URL)
+        return HttpResponseRedirect(redirect_to)
+

that I currently have as proposed in https://github.com/django/django/pull/4925 for https://code.djangoproject.com/ticket/25030 isn't actually the correct thing to do for this case as well.

comment:2 Changed 6 years ago by Tim Graham

I wouldn't expect the login view to work with RemoteUserMiddleware. My understanding is that you should write a separate view that handles the details of your particular remote user backend.

comment:3 Changed 6 years ago by Jan Pazdziora

If you say django.contrib.auth.views is not supposed to work out of box with RemoteUserMiddleware, I'll certainly respect that.

However, similar to our discussion in https://code.djangoproject.com/ticket/25029, if we actually want to make RemoteUserMiddleware / PersistentRemoteUserMiddleware more useful and more used, it'd be nice if django.contrib.auth.views.login it could handle any django.contrib.auth.middleware that the admin setting up the project decides to configure. The purpose of middlewares as I understand it is to be easily pluggable from admin's point of view -- just add them to MIDDLEWARE_CLASSES list and be done with it.

Yes, the admin certainly can write their own login view. But shouldn't we strive to make the out-of-box experience as easy as possible, making various bits work reasonably for various setups that are shipped with Django. Of course, situation is different with third-party software but RemoteUserMiddleware has been in the project since 2009.

comment:4 Changed 6 years ago by Tim Graham

I think one point is that we don't want to prevent a logged in user from being able to login again (without first logging out) as your patch does.

comment:5 Changed 6 years ago by Carl Meyer

I don't believe this ticket describes a bug at all. The current login view is simple and predictable, works for a variety of common use cases as-is, and can easily be wrapped to provide alternative behavior. The problem described in the ticket description can easily be addressed by such wrapping or by UX clues on the login page. I don't see any proposal here to change Django to improve this particular scenario without harming others. So I think this ticket should be closed.

It's not accurate to say that the login view doesn't work out of the box with RemoteUserMiddleware. They work fine together. The login view has the behavior that it allows an authenticated user to view the login page and log in again if they choose. That's unrelated to RemoteUserMiddleware, and it's a reasonable default choice which the project author can easily change if they like.

comment:6 Changed 6 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

comment:7 Changed 6 years ago by Jan Pazdziora

There are two things that I believe that view could do better: be more explicit that the user is already authentication and that re-authentication will happen (and we will need to verify if re-logging on that view will actually change the authentication user, overriding the identity that came from RemoteUserMiddleware). And add a mechanism to know that the RemoteUserMiddleware (and especially PersistentRemoteUserMiddleware) authentication just happened for the first time in the current HTTP request, in which case I'd really assume that the expected behaviour is to just accept that external authentication and skip the logon page.

I will try to come up with some code which would address those, it might just take me some time due to other work.

Thank you,

Jan

comment:8 Changed 6 years ago by Tim Graham

About "be more explicit that the user is already authentication and that re-authentication will happen" -- it seems like could be done in the project's login template (Django doesn't ship one, except for the admin, but it doesn't seem relevant there since it doesn't allow viewing the login page for logged in users).

About "I'd really assume that the expected behaviour is to just accept that external authentication and skip the logon page" -- as noted before, adding this behavior would be backwards incompatible and seems better left to a custom login view.

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