From 77f8d3b2e16687b60e932afa40400f8d76acbbd9 Mon Sep 17 00:00:00 2001 From: Andrea Corna Date: Tue, 7 Jul 2026 13:43:55 +0200 Subject: [PATCH 1/2] Return 401 for unauthenticated content requests, not 403 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Distinguish authentication failures (no/invalid credentials) from authorization failures (authenticated but not permitted) by introducing an AuthenticationRequired exception. This allows the content app to return HTTP 401 Unauthorized for requests that need authentication, enabling tools like pip to properly detect and retry with credentials. - RBACContentGuard catches NotAuthenticated and AuthenticationFailed → 401 - CompositeContentGuard re-raises authentication errors immediately - Handler maps AuthenticationRequired → HTTPUnauthorized (401) - Other PermissionErrors continue to map to HTTPForbidden (403) Fixes: content app returning 403 for all access denials regardless of auth state. Co-Authored-By: Claude Haiku 4.5 --- .../+fix-content-401-unauthenticated.bugfix | 1 + pulpcore/app/models/__init__.py | 2 + pulpcore/app/models/publication.py | 24 +++++++++- pulpcore/content/handler.py | 12 ++++- pulpcore/tests/unit/test_content_guard.py | 45 ++++++++++++++++++- 5 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 CHANGES/+fix-content-401-unauthenticated.bugfix diff --git a/CHANGES/+fix-content-401-unauthenticated.bugfix b/CHANGES/+fix-content-401-unauthenticated.bugfix new file mode 100644 index 00000000000..8f56c73a224 --- /dev/null +++ b/CHANGES/+fix-content-401-unauthenticated.bugfix @@ -0,0 +1 @@ +Fixed the content app returning `403 Forbidden` instead of `401 Unauthorized` when a request to a guarded distribution had no (or invalid) credentials at all. diff --git a/pulpcore/app/models/__init__.py b/pulpcore/app/models/__init__.py index a1bf1ca1147..f725964fd85 100644 --- a/pulpcore/app/models/__init__.py +++ b/pulpcore/app/models/__init__.py @@ -51,6 +51,7 @@ ) from .publication import ( + AuthenticationRequired, ContentGuard, Distribution, DistributedPublication, @@ -139,6 +140,7 @@ "Importer", "PulpImport", "PulpImporter", + "AuthenticationRequired", "ContentGuard", "Distribution", "DistributedPublication", diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py index 7de105a50b2..c3ea99a27b6 100644 --- a/pulpcore/app/models/publication.py +++ b/pulpcore/app/models/publication.py @@ -17,7 +17,7 @@ from django.db import DatabaseError, IntegrityError, models, transaction from django.utils import timezone from django_lifecycle import AFTER_CREATE, AFTER_UPDATE, BEFORE_DELETE, hook -from rest_framework.exceptions import APIException +from rest_framework.exceptions import APIException, AuthenticationFailed, NotAuthenticated from url_normalize import url_normalize from pulpcore.app.files import PulpTemporaryUploadedFile @@ -352,6 +352,17 @@ class Meta: unique_together = ("publication", "relative_path") +class AuthenticationRequired(PermissionError): + """ + Raised by a :class:`ContentGuard` when the request has no (or invalid) credentials at all. + + This is distinct from a plain :class:`PermissionError`, which signals that the request was + authenticated but is not authorized to access the content. The content app maps this + exception to an HTTP 401 response (with a ``WWW-Authenticate`` header), while a plain + ``PermissionError`` is mapped to an HTTP 403, matching normal HTTP authentication semantics. + """ + + class ContentGuard(MasterModel): """ Defines a named content guard. @@ -385,6 +396,9 @@ def permit(self, request): Raises: PermissionError: When not authorized. + AuthenticationRequired: When no (or invalid) credentials were provided at all. This + is a subclass of ``PermissionError``, so guards that don't distinguish the two + cases can keep raising a plain ``PermissionError`` and will keep getting a 403. """ raise NotImplementedError() @@ -419,8 +433,11 @@ def permit(self, request): setattr(view, "action", "download") try: view.check_permissions(drequest) + except (NotAuthenticated, AuthenticationFailed) as e: + # No (or invalid) credentials were provided at all -> 401, not 403. + raise AuthenticationRequired(e) from e except APIException as e: - raise PermissionError(e) + raise PermissionError(e) from e class Meta: default_related_name = "%(app_label)s_%(model_name)s" @@ -585,6 +602,9 @@ def permit(self, request): try: detail_guard.permit(request) return # success on first-pass + except AuthenticationRequired: + # Re-raise immediately to preserve 401 status, don't aggregate with other denials + raise except PermissionError as pe: guard_error = _("Guard: '{}', HREF: '{}', class: '{}', denial: [{}].").format( detail_guard.name, get_url(detail_guard), type(detail_guard), str(pe) diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py index dcaef910600..62fd5dd0ac0 100644 --- a/pulpcore/content/handler.py +++ b/pulpcore/content/handler.py @@ -18,6 +18,7 @@ HTTPMovedPermanently, HTTPNotFound, HTTPRequestRangeNotSatisfiable, + HTTPUnauthorized, ) from asgiref.sync import sync_to_async from django.utils import timezone @@ -48,6 +49,7 @@ from pulpcore.app.models import ( # noqa: E402 Artifact, ArtifactDistribution, + AuthenticationRequired, ContentArtifact, Distribution, Publication, @@ -262,7 +264,7 @@ async def auth_cached(cls, request, cached, base_key): ) try: guard = await sync_to_async(cls._permit)(request, distro) - except HTTPForbidden: + except (HTTPForbidden, HTTPUnauthorized): guard = True raise finally: @@ -490,12 +492,20 @@ def _permit(request, distribution): Raises: [aiohttp.web_exceptions.HTTPForbidden][]: When not permitted. + [aiohttp.web_exceptions.HTTPUnauthorized][]: When no (or invalid) credentials were + provided at all. """ guard = distribution.content_guard if not guard: return False try: guard.cast().permit(request) + except AuthenticationRequired as ar: + log.debug( + 'Path: %(p)s not authenticated for guard: "%(g)s" reason: %(r)s', + {"p": request.path, "g": guard.name, "r": str(ar)}, + ) + raise HTTPUnauthorized(reason=str(ar)) except PermissionError as pe: log.debug( 'Path: %(p)s not permitted by guard: "%(g)s" reason: %(r)s', diff --git a/pulpcore/tests/unit/test_content_guard.py b/pulpcore/tests/unit/test_content_guard.py index 9024020f239..284afe63445 100644 --- a/pulpcore/tests/unit/test_content_guard.py +++ b/pulpcore/tests/unit/test_content_guard.py @@ -5,7 +5,15 @@ import pytest -from pulpcore.app.models import ContentRedirectContentGuard, HeaderContentGuard +from rest_framework.exceptions import NotAuthenticated, PermissionDenied + +from pulpcore.app.models import ( + AuthenticationRequired, + ContentRedirectContentGuard, + HeaderContentGuard, + RBACContentGuard, +) +from pulpcore.app.viewsets import RBACContentGuardViewSet def test_preauthenticate_urls(): @@ -142,3 +150,38 @@ def test_header_content_guard(db): encoded_value = b64encode(b"somevalue") request.headers = {"x-header-name": encoded_value} assert not content_guard_without_jq_filter.permit(request) + + +def test_rbac_content_guard_no_credentials_raises_authentication_required(monkeypatch): + """ + No credentials at all must surface as AuthenticationRequired (-> HTTP 401), not a plain + PermissionError (-> HTTP 403). Regression test for the content app returning 403 for + unauthenticated requests. + """ + + def raise_not_authenticated(self, request): + raise NotAuthenticated() + + monkeypatch.setattr(RBACContentGuardViewSet, "check_permissions", raise_not_authenticated) + + content_guard = RBACContentGuard(name="rbac_guard") + request = {"drf_request": Mock()} + + with pytest.raises(AuthenticationRequired): + content_guard.permit(request) + + +def test_rbac_content_guard_bad_permissions_raises_permission_error(monkeypatch): + """An authenticated-but-unauthorized request should still raise a plain PermissionError.""" + + def raise_permission_denied(self, request): + raise PermissionDenied() + + monkeypatch.setattr(RBACContentGuardViewSet, "check_permissions", raise_permission_denied) + + content_guard = RBACContentGuard(name="rbac_guard") + request = {"drf_request": Mock()} + + with pytest.raises(PermissionError) as exc_info: + content_guard.permit(request) + assert not isinstance(exc_info.value, AuthenticationRequired) From 08ab8c328387790fa62fdab142b08902f9cc7553 Mon Sep 17 00:00:00 2001 From: Andrea Corna Date: Thu, 9 Jul 2026 22:41:33 +0200 Subject: [PATCH 2/2] Update RBAC content guard functional test for 401 vs 403 test_rbac_content_guard_full_workflow asserted 403 for every unauthorized user, including the anonymous one, which matched the previous (buggy) behavior of the content app. Split the expectation so that: - authorized users still get 404 (no change) - the anonymous user (no credentials at all) now expects 401 - authenticated-but-unauthorized users still expect 403 Co-Authored-By: Claude Sonnet 4.5 --- .../functional/api/using_plugin/test_contentguard.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pulpcore/tests/functional/api/using_plugin/test_contentguard.py b/pulpcore/tests/functional/api/using_plugin/test_contentguard.py index 585ced47ec9..768c682b56e 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_contentguard.py +++ b/pulpcore/tests/functional/api/using_plugin/test_contentguard.py @@ -46,7 +46,14 @@ def _assert_access(authorized_users): else: auth = None response = get_from_url(distribution_base_url(distro.base_url), auth=auth) - expected_status = 404 if user in authorized_users else 403 + if user in authorized_users: + expected_status = 404 + elif user is anonymous_user: + # No credentials at all -> Unauthorized, not Forbidden. + expected_status = 401 + else: + # Authenticated, but not permitted. + expected_status = 403 assert response.status == expected_status, f"Failed on {user.username=}" # Make sure all users can access the distribution URL without a content guard