#37025: Deprecate the prefixing of HTTP_ to header names in
RemoteUserMiddleware.aprocess_request()
------------------------------+---------------------------------------
Reporter: Jacob Walls | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
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 Jacob Walls):
Thanks both. So the original proposal definitely involves breaking
RemoteUserMiddleware + ASGI + default configuration, on the theory that if
almost no one is using this, it's OK to make this "broken by default" ,
because "broken by default" = "secure by default". But the `header_prefix`
idea is interesting!
That said, the recent doc edits are sufficient, so my motivation here was
less about security and more "easier to reason about", given that if we
lean toward Sarah's proposed doc edit in comment:6, then I wonder if we're
doubling down on documenting a bug.
----
To Sarah's question in comment:3, to get the tests working, we would need
to adjust how we parameterize the header names in the tests. I recently
learned that the sync request factory expects prefixed header names, e.g.
`"HTTP_MYHEADER"`, but the async request factory expects unprefixed header
names, e.g. `"MYHEADER"`, since they go through `ASGIRequest`, which
applies prefixing.
{{{#!diff
diff --git a/django/contrib/auth/middleware.py
b/django/contrib/auth/middleware.py
index a28e1705a3..7136a64c44 100644
--- a/django/contrib/auth/middleware.py
+++ b/django/contrib/auth/middleware.py
@@ -184,7 +184,7 @@ class RemoteUserMiddleware:
" before the RemoteUserMiddleware class."
)
try:
- username = request.META["HTTP_" + self.header]
+ 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
diff --git a/tests/auth_tests/test_remote_user.py
b/tests/auth_tests/test_remote_user.py
index 4a97fc2120..6745009589 100644
--- a/tests/auth_tests/test_remote_user.py
+++ b/tests/auth_tests/test_remote_user.py
@@ -3,8 +3,9 @@ from datetime import UTC, datetime
from django.conf import settings
from django.contrib.auth import aauthenticate, authenticate
from django.contrib.auth.backends import RemoteUserBackend
-from django.contrib.auth.middleware import RemoteUserMiddleware
+from django.contrib.auth.middleware import
PersistentRemoteUserMiddleware, RemoteUserMiddleware
from django.contrib.auth.models import User
+from django.http.request import HttpHeaders
from django.middleware.csrf import _get_new_csrf_string,
_mask_cipher_secret
from django.test import (
AsyncClient,
@@ -17,15 +18,24 @@ from django.test import (
@override_settings(ROOT_URLCONF="auth_tests.urls")
class RemoteUserTest(TestCase):
- middleware = "django.contrib.auth.middleware.RemoteUserMiddleware"
+ # Use a custom middleware if we want this test case to run on both
WSGI & ASGI.
+ middleware = "auth_tests.test_remote_user.CustomHeaderMiddleware"
backend = "django.contrib.auth.backends.RemoteUserBackend"
- header = "REMOTE_USER"
+ header = "HTTP_AUTHUSER"
email_header = "REMOTE_EMAIL"
# Usernames to be passed in REMOTE_USER for the test_known_user test
case.
known_user = "knownuser"
known_user2 = "knownuser2"
+ @property
+ def async_header(self):
+ if self.header.startswith(HttpHeaders.HTTP_PREFIX):
+ # The tests like to parameterize headers, but if the defined
header
+ # is already prefixed, strip it before using it.
+ return self.header.lstrip(HttpHeaders.HTTP_PREFIX)
+ return self.header
+
@classmethod
def setUpClass(cls):
cls.enterClassContext(
@@ -65,7 +75,7 @@ class RemoteUserTest(TestCase):
self.assertTrue(response.context["user"].is_anonymous)
self.assertEqual(await User.objects.acount(), num_users)
- response = await self.async_client.get("/remote_user/",
**{self.header: ""})
+ response = await self.async_client.get("/remote_user/",
**{self.async_header: ""})
self.assertTrue(response.context["user"].is_anonymous)
self.assertEqual(await User.objects.acount(), num_users)
@@ -142,7 +152,7 @@ class RemoteUserTest(TestCase):
"""See test_unknown_user."""
num_users = await User.objects.acount()
response = await self.async_client.get(
- "/remote_user/", **{self.header: "newuser"}
+ "/remote_user/", **{self.async_header: "newuser"}
)
self.assertEqual(response.context["user"].username, "newuser")
self.assertEqual(await User.objects.acount(), num_users + 1)
@@ -150,7 +160,7 @@ class RemoteUserTest(TestCase):
# Another request with same user should not create any new users.
response = await self.async_client.get(
- "/remote_user/", **{self.header: "newuser"}
+ "/remote_user/", **{self.async_header: "newuser"}
)
self.assertEqual(await User.objects.acount(), num_users + 1)
@@ -176,14 +186,14 @@ class RemoteUserTest(TestCase):
await User.objects.acreate(username="knownuser2")
num_users = await User.objects.acount()
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(response.context["user"].username, "knownuser")
self.assertEqual(await User.objects.acount(), num_users)
# A different user passed in the headers causes the new user
# to be logged in.
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user2}
+ "/remote_user/", **{self.async_header: self.known_user2}
)
self.assertEqual(response.context["user"].username, "knownuser2")
self.assertEqual(await User.objects.acount(), num_users)
@@ -221,7 +231,7 @@ class RemoteUserTest(TestCase):
await user.asave()
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertNotEqual(default_login,
response.context["user"].last_login)
@@ -229,7 +239,7 @@ class RemoteUserTest(TestCase):
user.last_login = default_login
await user.asave()
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(default_login,
response.context["user"].last_login)
@@ -259,7 +269,7 @@ class RemoteUserTest(TestCase):
await User.objects.acreate(username="knownuser")
# Known user authenticates
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(response.context["user"].username, "knownuser")
# During the session, the REMOTE_USER header disappears. Should
trigger
@@ -295,12 +305,12 @@ class RemoteUserTest(TestCase):
await User.objects.acreate(username="knownuser")
# Known user authenticates
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(response.context["user"].username, "knownuser")
# During the session, the REMOTE_USER changes to a different
user.
response = await self.async_client.get(
- "/remote_user/", **{self.header: "newnewuser"}
+ "/remote_user/", **{self.async_header: "newnewuser"}
)
# The current user is not the prior remote_user.
# In backends that create a new user, username is "newnewuser"
@@ -315,7 +325,7 @@ class RemoteUserTest(TestCase):
async def test_inactive_user_async(self):
await User.objects.acreate(username="knownuser", is_active=False)
response = await self.async_client.get(
- "/remote_user/", **{self.header: "knownuser"}
+ "/remote_user/", **{self.async_header: "knownuser"}
)
self.assertTrue(response.context["user"].is_anonymous)
@@ -343,7 +353,7 @@ class RemoteUserNoCreateTest(RemoteUserTest):
async def test_unknown_user_async(self):
num_users = await User.objects.acount()
response = await self.async_client.get(
- "/remote_user/", **{self.header: "newuser"}
+ "/remote_user/", **{self.async_header: "newuser"}
)
self.assertTrue(response.context["user"].is_anonymous)
self.assertEqual(await User.objects.acount(), num_users)
@@ -362,7 +372,7 @@ class
AllowAllUsersRemoteUserBackendTest(RemoteUserTest):
async def test_inactive_user_async(self):
user = await User.objects.acreate(username="knownuser",
is_active=False)
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(response.context["user"].username,
user.username)
@@ -436,6 +446,10 @@ class RemoteUserCustomTest(RemoteUserTest):
self.assertEqual(newuser.email, "[email protected]")
+"""
+These tests are now duplicative in the current demonstration. We wouldn't
merge
+it in this state.
+"""
class CustomHeaderMiddleware(RemoteUserMiddleware):
"""
Middleware that overrides custom HTTP auth user header.
@@ -454,13 +468,17 @@ class CustomHeaderRemoteUserTest(RemoteUserTest):
header = "HTTP_AUTHUSER"
+class
CustomPersistentRemoteUserMiddleware(PersistentRemoteUserMiddleware):
+ header = "HTTP_AUTHUSER"
+
+
class PersistentRemoteUserTest(RemoteUserTest):
"""
PersistentRemoteUserMiddleware keeps the user logged in even if the
subsequent calls do not contain the header value.
"""
- middleware =
"django.contrib.auth.middleware.PersistentRemoteUserMiddleware"
+ middleware =
"auth_tests.test_remote_user.CustomPersistentRemoteUserMiddleware"
require_header = False
def test_header_disappears(self):
@@ -482,7 +500,7 @@ class PersistentRemoteUserTest(RemoteUserTest):
await User.objects.acreate(username="knownuser")
# Known user authenticates
response = await self.async_client.get(
- "/remote_user/", **{self.header: self.known_user}
+ "/remote_user/", **{self.async_header: self.known_user}
)
self.assertEqual(response.context["user"].username, "knownuser")
# Should stay logged in if the REMOTE_USER header disappears.
}}}
I had to be aware of that in caf90a971f09323775ed0cacf94eadaf39d040e0,
where I added a little change in the `AsyncRequestFactory` to smooth out
this difference with respect to underscores. It's possible we could
deprecate and undo that going forward, if we want to standardize how
headers are provided in tests? But that would be more disruptive than what
I'm proposing here.
----
I'll definitely support wontfix'ing this if there is no value, but
something seems off. I wonder if the quirks of `AsyncRequestFactory` put
a little design pressure on the decision to have
`RemoteUserMiddleware.aprocess_request` apply prefixing?
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019d72b7e61c-b467e250-73c1-4ea0-b8aa-9e18255e7be0-000000%40eu-central-1.amazonses.com.
