diff --git a/django/contrib/auth/__init__.py b/django/contrib/auth/__init__.py index aa56df9a3a14..d5d9acff886a 100644 --- a/django/contrib/auth/__init__.py +++ b/django/contrib/auth/__init__.py @@ -1,5 +1,7 @@ import re +from asgiref.sync import sync_to_async + from django.apps import apps as django_apps from django.conf import settings from django.core.exceptions import ImproperlyConfigured, PermissionDenied @@ -407,6 +409,6 @@ def check_password_with_timing_attack_mitigation(user, password): async def acheck_password_with_timing_attack_mitigation(user, password): """See check_user_with_timing_attack_mitigation.""" if user is None: - get_user_model()().set_password(password) + await sync_to_async(get_user_model()().set_password)(password) else: return await user.acheck_password(password) diff --git a/django/contrib/auth/middleware.py b/django/contrib/auth/middleware.py index ca8187a364b1..09e3d3e6bbc6 100644 --- a/django/contrib/auth/middleware.py +++ b/django/contrib/auth/middleware.py @@ -238,7 +238,10 @@ async def aclean_username(self, username, request): return username def get_username(self, request): - if isinstance(request, ASGIRequest): + if ( + isinstance(request, ASGIRequest) + and self.header == RemoteUserMiddleware.header + ): return request.META["HTTP_" + self.header] return request.META[self.header] diff --git a/docs/howto/auth-remote-user.txt b/docs/howto/auth-remote-user.txt index fe48bb3dc86a..4aad4f616bf2 100644 --- a/docs/howto/auth-remote-user.txt +++ b/docs/howto/auth-remote-user.txt @@ -2,24 +2,24 @@ How to authenticate using ``REMOTE_USER`` ========================================= -This document describes how to make use of external authentication sources -(where the web server sets the ``REMOTE_USER`` environment variable) in your -Django applications. This type of authentication solution is typically seen on -intranet sites, with single sign-on solutions such as IIS and Integrated -Windows Authentication or Apache and `mod_authnz_ldap`_, `CAS`_, `WebAuth`_, -`mod_auth_sspi`_, etc. +This document describes how to make use of external authentication sources in +your Django applications. This type of authentication solution is typically +seen on intranet sites, with single sign-on solutions such as IIS and +Integrated Windows Authentication or Apache and `mod_authnz_ldap`_, `CAS`_, +`WebAuth`_, `mod_auth_sspi`_, etc. .. _mod_authnz_ldap: https://httpd.apache.org/docs/current/mod/mod_authnz_ldap.html .. _CAS: https://www.apereo.org/projects/cas .. _WebAuth: https://uit.stanford.edu/service/authentication .. _mod_auth_sspi: https://sourceforge.net/projects/mod-auth-sspi -When the web server takes care of authentication it typically sets the -``REMOTE_USER`` environment variable for use in the underlying application. In -Django, ``REMOTE_USER`` is made available in the :attr:`request.META -` attribute. Django can be configured to make -use of the ``REMOTE_USER`` value using the ``RemoteUserMiddleware`` -or ``PersistentRemoteUserMiddleware``, and +When the web server takes care of authentication it typically provides the +authenticated user as ``REMOTE_USER``. In Django, this value is made available +in :attr:`request.META ` (as ``REMOTE_USER`` when +supplied as an environment variable, as in WSGI, or ``HTTP_REMOTE_USER`` when +supplied via an HTTP header, as in ASGI). Django can be configured to make use +of the ``REMOTE_USER`` value using the ``RemoteUserMiddleware`` or +``PersistentRemoteUserMiddleware``, and :class:`~django.contrib.auth.backends.RemoteUserBackend` classes found in :mod:`django.contrib.auth`. @@ -47,7 +47,8 @@ with :class:`~django.contrib.auth.backends.RemoteUserBackend` in the ] With this setup, ``RemoteUserMiddleware`` will detect the username in -``request.META['REMOTE_USER']`` and will authenticate and auto-login that user +``request.META['REMOTE_USER']`` (or ``request.META['HTTP_REMOTE_USER']`` under +ASGI) and will authenticate and auto-login that user using the :class:`~django.contrib.auth.backends.RemoteUserBackend`. Be aware that this particular setup disables authentication with the default @@ -103,9 +104,7 @@ instead of :class:`django.contrib.auth.middleware.RemoteUserMiddleware`:: client can supply the header. You must be sure that your web server or reverse proxy always sets or strips that header based on the appropriate authentication checks, never permitting an end user to submit a fake (or - "spoofed") header value. In particular, ASGI deployments cannot be exposed - directly to the internet (that is, without a reverse proxy) when using this - middleware. + "spoofed") header value. Since the HTTP headers ``X-Auth-User`` and ``X-Auth_User`` (for example) both normalize to the ``HTTP_X_AUTH_USER`` key in ``request.META``, you @@ -115,10 +114,12 @@ instead of :class:`django.contrib.auth.middleware.RemoteUserMiddleware`:: Under WSGI, this warning doesn't apply to ``RemoteUserMiddleware`` in its default configuration with ``header = "REMOTE_USER"``, since a key that doesn't start with ``HTTP_`` in ``request.META`` can only be set by your - WSGI server, not directly from an HTTP request header. This warning *does* - apply by default on ASGI, because in the async path, the middleware - prepends ``HTTP_`` to the defined header name before looking it up in - ``request.META``. + WSGI server, not directly from an HTTP request header. + + This warning applies under ASGI in all configurations, because there is + no equivalent for a WSGI server's ability to place a trusted value in the + environ. ASGI deployments *must* use a reverse proxy as described above + when using this middleware. If you need more control, you can create your own authentication backend that inherits from :class:`~django.contrib.auth.backends.RemoteUserBackend` and diff --git a/docs/releases/6.1.txt b/docs/releases/6.1.txt index 7af3c6af7205..50e4f63f5e1b 100644 --- a/docs/releases/6.1.txt +++ b/docs/releases/6.1.txt @@ -467,6 +467,15 @@ backends. * The undocumented ``InclusionAdminNode.__init__()`` now takes the template tag ``name`` as the first positional argument. +:mod:`django.contrib.auth` +-------------------------- + +* Under ASGI, :class:`~django.contrib.auth.middleware.RemoteUserMiddleware` no + longer prefixes ``HTTP_`` when looking up custom values in ``request.META``. + For example, to send ``-H "AuthUser": ...``, the ``header`` attribute should + be ``HTTP_AUTHUSER``. This restores the behavior prior to Django 5.2. (The + default value of ``REMOTE_USER`` is not affected.) + :mod:`django.contrib.gis` ------------------------- diff --git a/scripts/pr_quality/check_pr.py b/scripts/pr_quality/check_pr.py index 51d106e87537..e1991e46d361 100644 --- a/scripts/pr_quality/check_pr.py +++ b/scripts/pr_quality/check_pr.py @@ -30,6 +30,7 @@ import urllib.parse import urllib.request from datetime import date, datetime, timedelta, timezone +from http import HTTPStatus from pr_quality.errors import ( CHECKS_FOOTER, @@ -102,7 +103,8 @@ def github_request(method, path, token, repo, data=None, params=None): headers["Content-Type"] = "application/json" req = urllib.request.Request(url, data=body, headers=headers, method=method) with urllib.request.urlopen(req, timeout=URLOPEN_TIMEOUT_SECONDS) as response: - return json.loads(response.read()) + if response.status != HTTPStatus.NO_CONTENT: + return json.loads(response.read()) def get_recent_commit_count(pr_author, repo, token, since_days, max_count): @@ -144,6 +146,26 @@ def get_pr_total_changes(pr_number, repo, token): return total_changes +def get_comment_ids_to_delete(pr_number, repo, token): + ids = [] + page = 1 + while True: + comments = github_request( + "GET", + f"/issues/{pr_number}/comments", + token, + repo, + {"per_page": GITHUB_PER_PAGE, "page": page}, + ) + for comment in comments: + if CHECKS_HEADER in comment["body"]: + ids.append(comment["id"]) + if len(comments) < GITHUB_PER_PAGE: + break + page += 1 + return ids + + def strip_html_comments(text): """Return text with all HTML comments removed.""" return re.sub(r"", "", text, flags=re.DOTALL) @@ -506,6 +528,14 @@ def main( logger.info("PR #%s passed all quality checks.", pr_number) return + for id_to_delete in get_comment_ids_to_delete(pr_number, repo, token): + github_request( + "DELETE", + f"/issues/comments/{id_to_delete}", + token, + repo, + ) + github_request( "POST", f"/issues/{pr_number}/comments", diff --git a/scripts/pr_quality/tests/test_check_pr.py b/scripts/pr_quality/tests/test_check_pr.py index c7565df5ab5c..30bfa26512aa 100644 --- a/scripts/pr_quality/tests/test_check_pr.py +++ b/scripts/pr_quality/tests/test_check_pr.py @@ -781,8 +781,16 @@ def test_trusted_author_failures_no_close(self): body = make_pr_body(ticket="", checked_items=0) result, _, mock_gh = self.call_main(pr_body=body, commit_count=1) self.assertEqual(result, 1) - mock_gh.assert_called_once_with( - "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + self.assertEqual( + mock_gh.call_args_list, + [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + mock.call( + "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + ], ) def test_untrusted_author_failures_posts_comment_and_closes(self): @@ -790,8 +798,11 @@ def test_untrusted_author_failures_posts_comment_and_closes(self): result, _, mock_gh = self.call_main(pr_body=body) self.assertEqual(result, 1) self.assertEqual( - mock_gh.mock_calls, + mock_gh.call_args_list, [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), mock.call( "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY ), @@ -806,8 +817,11 @@ def test_missing_pr_author_treated_as_untrusted(self): result, _, mock_gh = self.call_main(pr_body=body, pr_author="") self.assertEqual(result, 1) self.assertEqual( - mock_gh.mock_calls, + mock_gh.call_args_list, [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), mock.call( "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY ), @@ -821,8 +835,16 @@ def test_autoclose_false_posts_comment_does_not_close(self): body = make_pr_body(ticket="", checked_items=0) result, _, mock_gh = self.call_main(pr_body=body, autoclose=False) self.assertEqual(result, 1) - mock_gh.assert_called_once_with( - "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + self.assertEqual( + mock_gh.call_args_list, + [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + mock.call( + "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + ], ) def test_warnings_only_posts_comment_does_not_close(self): @@ -833,8 +855,16 @@ def test_warnings_only_posts_comment_does_not_close(self): pr_body=VALID_PR_BODY, pr_title="", trac_data=trac_data ) self.assertIsNone(result) - mock_gh.assert_called_once_with( - "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + self.assertEqual( + mock_gh.call_args_list, + [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + mock.call( + "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), + ], ) def test_warnings_alongside_failures_still_closes(self): @@ -844,8 +874,11 @@ def test_warnings_alongside_failures_still_closes(self): result, _, mock_gh = self.call_main(pr_body=body, trac_data=trac_data) self.assertEqual(result, 1) self.assertEqual( - mock_gh.mock_calls, + mock_gh.call_args_list, [ + mock.call( + "GET", "/issues/10/comments", "test-token", "test/repo", mock.ANY + ), mock.call( "POST", "/issues/10/comments", "test-token", "test/repo", mock.ANY ), diff --git a/tests/auth_tests/test_auth_backends.py b/tests/auth_tests/test_auth_backends.py index 77eeed3d60f0..40d686dfc51f 100644 --- a/tests/auth_tests/test_auth_backends.py +++ b/tests/auth_tests/test_auth_backends.py @@ -498,6 +498,22 @@ async def test_aauthentication_timing(self): await aauthenticate(username="no_such_user", password="test") self.assertEqual(CountingMD5PasswordHasher.calls, 1) + async def test_aauthentication_timing_async_bridge(self): + with patch( + "django.contrib.auth.hashers.sync_to_async", + return_value=mock.AsyncMock(return_value=(True, False)), + ) as mocked_adapter: + username = getattr(self.user, self.UserModel.USERNAME_FIELD) + await aauthenticate(username=username, password="test") + mocked_adapter.assert_called_once() + + with patch( + "django.contrib.auth.sync_to_async", return_value=mock.AsyncMock() + ) as mocked_adapter: + username = getattr(self.user, self.UserModel.USERNAME_FIELD) + await aauthenticate(username="no_such_user", password="test") + mocked_adapter.assert_called_once() + @override_settings( PASSWORD_HASHERS=["auth_tests.test_auth_backends.CountingMD5PasswordHasher"] ) diff --git a/tests/auth_tests/test_remote_user.py b/tests/auth_tests/test_remote_user.py index 8c85c651968a..69b4b5e10fa6 100644 --- a/tests/auth_tests/test_remote_user.py +++ b/tests/auth_tests/test_remote_user.py @@ -31,8 +31,15 @@ def middleware(request): class RemoteUserTest(TestCase): middleware = "django.contrib.auth.middleware.RemoteUserMiddleware" backend = "django.contrib.auth.backends.RemoteUserBackend" + + # ASGI tests always provide this value via `headers`. + # WSGI tests provide this value directly into the environ via `**extra`. + # When subclassing to provide a custom header, implement a property to + # strip the prefix, and always pass via `headers`, since the only case + # where it should be passed via the environ is under WSGI, and only when + # the value is exactly "REMOTE_USER". header = "REMOTE_USER" - email_header = "REMOTE_EMAIL" + email_header = "HTTP_REMOTE_EMAIL" # Usernames to be passed in REMOTE_USER for the test_known_user test case. known_user = "knownuser" @@ -77,7 +84,9 @@ async def test_no_remote_user_async(self): 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/", headers={self.header: ""} + ) self.assertTrue(response.context["user"].is_anonymous) self.assertEqual(await User.objects.acount(), num_users) @@ -154,7 +163,7 @@ async def test_unknown_user_async(self): """See test_unknown_user.""" num_users = await User.objects.acount() response = await self.async_client.get( - "/remote_user/", **{self.header: "newuser"} + "/remote_user/", headers={self.header: "newuser"} ) self.assertEqual(response.context["user"].username, "newuser") self.assertEqual(await User.objects.acount(), num_users + 1) @@ -162,7 +171,7 @@ async def test_unknown_user_async(self): # Another request with same user should not create any new users. response = await self.async_client.get( - "/remote_user/", **{self.header: "newuser"} + "/remote_user/", headers={self.header: "newuser"} ) self.assertEqual(await User.objects.acount(), num_users + 1) @@ -188,14 +197,14 @@ async def test_known_user_async(self): 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/", headers={self.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/", headers={self.header: self.known_user2} ) self.assertEqual(response.context["user"].username, "knownuser2") self.assertEqual(await User.objects.acount(), num_users) @@ -233,7 +242,7 @@ async def test_last_login_async(self): await user.asave() response = await self.async_client.get( - "/remote_user/", **{self.header: self.known_user} + "/remote_user/", headers={self.header: self.known_user} ) self.assertNotEqual(default_login, response.context["user"].last_login) @@ -241,7 +250,7 @@ async def test_last_login_async(self): user.last_login = default_login await user.asave() response = await self.async_client.get( - "/remote_user/", **{self.header: self.known_user} + "/remote_user/", headers={self.header: self.known_user} ) self.assertEqual(default_login, response.context["user"].last_login) @@ -271,7 +280,7 @@ async def test_header_disappears_async(self): await User.objects.acreate(username="knownuser") # Known user authenticates response = await self.async_client.get( - "/remote_user/", **{self.header: self.known_user} + "/remote_user/", headers={self.header: self.known_user} ) self.assertEqual(response.context["user"].username, "knownuser") # During the session, the REMOTE_USER header disappears. Should trigger @@ -307,12 +316,12 @@ async def test_user_switch_forces_new_login_async(self): await User.objects.acreate(username="knownuser") # Known user authenticates response = await self.async_client.get( - "/remote_user/", **{self.header: self.known_user} + "/remote_user/", headers={self.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/", headers={self.header: "newnewuser"} ) # The current user is not the prior remote_user. # In backends that create a new user, username is "newnewuser" @@ -327,7 +336,7 @@ def test_inactive_user(self): 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/", headers={self.header: "knownuser"} ) self.assertTrue(response.context["user"].is_anonymous) @@ -355,7 +364,7 @@ def test_unknown_user(self): 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/", headers={self.header: "newuser"} ) self.assertTrue(response.context["user"].is_anonymous) self.assertEqual(await User.objects.acount(), num_users) @@ -374,7 +383,7 @@ def test_inactive_user(self): 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/", headers={self.header: self.known_user} ) self.assertEqual(response.context["user"].username, user.username) @@ -402,7 +411,7 @@ def configure_user(self, request, user, created=True): return user async def aconfigure_user(self, request, user, created=True): - user.email = request.META.get("HTTP_" + RemoteUserTest.email_header, "") + user.email = request.META.get(RemoteUserTest.email_header, "") if not created: user.last_name = user.username await user.asave() @@ -468,9 +477,9 @@ async def test_unknown_user_async(self): num_users = await User.objects.acount() response = await self.async_client.get( "/remote_user/", - **{ + headers={ self.header: "newuser", - self.email_header: "user@example.com", + self.email_header.removeprefix("HTTP_"): "user@example.com", }, ) self.assertEqual(response.context["user"].username, "newuser") @@ -503,19 +512,51 @@ class CustomHeaderMiddleware(RemoteUserMiddleware): header = "HTTP_AUTHUSER" -class CustomHeaderRemoteUserTest(RemoteUserTest): +class CustomHeaderMixin: + middleware = "auth_tests.test_remote_user.CustomHeaderMiddleware" + auth_header = CustomHeaderMiddleware.header + + @property + def header(self): + """Return the unprefixed header the server or proxy should provide.""" + return self.auth_header.removeprefix("HTTP_") + + def setUp(self): + """Force any **kwargs to be passed via `headers`.""" + super().setUp() + original_get = self.client.get + original_async_get = self.async_client.get + + def get(self, *args, headers={}, **kwargs): + headers = {**headers} + headers.update(kwargs) + return original_get(self, *args, headers=headers) + + async def aget(self, *args, headers={}, **kwargs): + headers = {**headers} + headers.update(kwargs) + return await original_async_get(self, *args, headers=headers) + + self.client.get = get + self.async_client.get = aget + self.addCleanup(setattr, self.client, "get", original_get) + self.addCleanup(setattr, self.async_client, "get", original_async_get) + + +class CustomHeaderRemoteUserTest(CustomHeaderMixin, RemoteUserTest): """ Tests a custom RemoteUserMiddleware subclass with custom HTTP auth user header. """ - middleware = "auth_tests.test_remote_user.CustomHeaderMiddleware" - header = "HTTP_AUTHUSER" - -class CustomHeaderASGISyncPathRemoteUserTest(ASGISyncPathRemoteUserTest): - middleware = "auth_tests.test_remote_user.CustomHeaderMiddleware" - header = "HTTP_AUTHUSER" +class CustomHeaderASGISyncPathRemoteUserTest( + CustomHeaderMixin, ASGISyncPathRemoteUserTest +): + """ + Tests sync execution under ASGI of a custom RemoteUserMiddleware subclass + with custom HTTP auth user header. + """ class PersistentRemoteUserTest(RemoteUserTest): @@ -546,7 +587,7 @@ async def test_header_disappears_async(self): await User.objects.acreate(username="knownuser") # Known user authenticates response = await self.async_client.get( - "/remote_user/", **{self.header: self.known_user} + "/remote_user/", headers={self.header: self.known_user} ) self.assertEqual(response.context["user"].username, "knownuser") # Should stay logged in if the REMOTE_USER header disappears.