Opened 10 days ago

Closed 10 days ago

Last modified 10 days ago

#36300 closed Uncategorized (needsinfo)

request.META["HTTP_" + self.header] in RemoteUserMiddleware __acall__ does not sound correct

Reported by: Jan Pazdziora Owned by:
Component: contrib.auth Version: 5.2
Severity: Normal Keywords:
Cc: Jon Janzen, Carlton Gibson Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've been investigating why https://github.com/adelton/django-identity-external no longer works with Django 5.2. The https://docs.djangoproject.com/en/5.2/releases/5.2/#django-contrib-auth talks about new async auth functions. I have no idea if the async functions are part of the problem I try to solve but it made me look at the code changes.

The PR https://github.com/django/django/pull/18036 for https://code.djangoproject.com/ticket/35303 added __acall__ with code

+        try:
+            username = request.META["HTTP_" + self.header]
+        except KeyError:
+            # If specified header doesn't exist then remove any existing
+            # authenticated remote-user, or return (leaving request.user set to
+            # AnonymousUser by the AuthenticationMiddleware).

among others.

However, the code in __call__ (previously process_request) has code

        try:
            username = request.META[self.header]
        except KeyError:
            # If specified header doesn't exist then remove any existing
            # authenticated remote-user, or return (leaving request.user set to
            # AnonymousUser by the AuthenticationMiddleware).
            if self.force_logout_if_no_header and request.user.is_authenticated:

Since they implement the same logic, the discrepancy is worrying. I believe the "HTTP_" prefix is wrong -- if the user (admin) wants to consume some HTTP header, let them configure the value with the HTTP_ prefix already.

This also shows that there don't seem tests covering the RemoteUserMiddleware, or the problem would have been caught.

Change History (2)

comment:1 by Sarah Boyce, 10 days ago

Cc: Jon Janzen Carlton Gibson added
Resolution: needsinfo
Status: newclosed

There are tests such as tests.auth_tests.test_remote_user.RemoteUserTest.test_known_user_async
If the HTTP prefix is removed, the tests fail.

You will need to demonstrate an issue that we can replicate so that we can better understand the request here.

I think really this is a discussion around that WSGI adds a HTTP_ prefix but ASGI does not (see HttpHeaders.to_wsgi_name vs HttpHeaders.to_asgi_name)
I will cc some other folks to the ticket in case they have thoughts to add

comment:2 by Carlton Gibson, 10 days ago

I think the difference here is that WSGI servers generally set REMOTE_USER in the environment, where the ASGI scope is parsed as headers (so get the HTTP_ prefix).

needsinfo: Yes. A clearer demonstration of why Django is at fault would be needed here.

Unless django-identity-external is using the new __acall__ methods, it's not clear why it should be affected. (I didn't see a tracking issue on its repo.) Perhaps you're using it with an async app, and you're hitting the new pathway? Maybe the `CustomHeaderRemoteUserMiddleware` example in the docs would be what you need, as a workaround?

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