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

[Django]

#36300: request.META["HTTP_" + self.header] in RemoteUserMiddleware __acall__ does
not sound correct
-------------------------------+-----------------------------------------
Reporter: Jan Pazdziora | Type: Uncategorized
Status: new | Component: contrib.auth
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+-----------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36300>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

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

Reporter: Jan Pazdziora | Owner: (none)
Type: Uncategorized | Status: closed
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution: needsinfo

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Changes (by Sarah Boyce):

* cc: Jon Janzen, Carlton Gibson (added)
* resolution: => needsinfo
* status: new => closed

Comment:

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
--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:1>

[Django]

#36300: request.META["HTTP_" + self.header] in RemoteUserMiddleware __acall__ does
not sound correct
-------------------------------+--------------------------------------
Reporter: Jan Pazdziora | Owner: (none)
Type: Uncategorized | Status: closed
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Carlton Gibson):

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
[https://docs.djangoproject.com/en/5.1/howto/auth-remote-user/#id1
`CustomHeaderRemoteUserMiddleware` example] in the docs would be what you
need, as a workaround?
--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:2>

[Django]

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

Reporter: Jan Pazdziora | Owner: (none)

Type: Bug | Status: new

Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------

Changes (by Jacob Walls):

* resolution: needsinfo =>
* stage: Unreviewed => Accepted
* status: closed => new
* type: Uncategorized => Bug

Comment:

Replying to [ticket:36300 Jan Pazdziora]:

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

As developed in ticket:37025#comment:19, this is essentially right -- we
had tests, but they were written incorrectly.

It's a regression in 50f89ae850f6b4e35819fe725a08c7e579bfd099 with respect
to custom headers. (The effort to support the default header under ASGI
changed the semantic for custom headers.)

I repurposed #37025 to clean up the tests, which should show the problem
more clearly. I'll put up a fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:3>

[Django]

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

-------------------------------+---------------------------------------
Reporter: Jan Pazdziora | Owner: Jacob Walls
Type: Bug | Status: assigned

Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------+---------------------------------------
Changes (by Jacob Walls):

* has_patch: 0 => 1
* owner: (none) => Jacob Walls
* status: new => assigned

Comment:

[https://github.com/django/django/pull/21209 PR] -- this branch contains
some other related fixes, so we might hold this branch until the other
work is reviewed.
--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:4>

[Django]

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

-------------------------------------+-------------------------------------

Reporter: Jan Pazdziora | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:5>

[Django]

#36300: request.META["HTTP_" + self.header] in RemoteUserMiddleware __acall__ does
not sound correct
-------------------------------------+-------------------------------------
Reporter: Jan Pazdziora | Owner: Jacob
| Walls

Type: Bug | Status: closed
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"1085e5e17b7403b15c4eff397f3c98c15660f46d" 1085e5e]:
{{{#!CommitTicketReference repository=""
revision="1085e5e17b7403b15c4eff397f3c98c15660f46d"
Fixed #36300 -- Restored the semantic where RemoteUserMiddleware.header
corresponds to request.META under ASGI.

Because these tests always passed both WSGI environ values and HTTP
headers via `**extra`, this masked a behavior difference between WSGI
and ASGI.

What should happen: everything should be passed via `headers` but for
the default REMOTE_USER case on WSGI, which should be passed via
`**extra`.

Since that was not done, a regression made it into Django 5.2
(50f89ae850f6b4e35819fe725a08c7e579bfd099) where `.header` no longer
corresponded to the request.META key under ASGI. To cope, an ASGI user
would have started(*) sending HTTP headers that match the `.header`
attribute, which may or may not have been edited to remove the HTTP_
prefix. (Note: the default `REMOTE_USER` case did not work under ASGI,
so the change in Django 5.2 had the effect of fixing the default case
but changing the semantic of the custom case.)

(*): Unless they were getting the sync execution path, which didn't have
this bug. See the fix in 0f4fff79d33b7cc84822e66bd1fc16caf8222e3a.

Thanks Mykhailo Havelia and Sarah Boyce for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36300#comment:6>