diff --git a/CHANGES/+fix-content-401-unauthenticated.bugfix b/CHANGES/+fix-content-401-unauthenticated.bugfix new file mode 100644 index 0000000000..8f56c73a22 --- /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 a1bf1ca114..f725964fd8 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 7de105a50b..c3ea99a27b 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 dcaef91060..62fd5dd0ac 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/functional/api/using_plugin/test_contentguard.py b/pulpcore/tests/functional/api/using_plugin/test_contentguard.py index 585ced47ec..768c682b56 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 diff --git a/pulpcore/tests/unit/test_content_guard.py b/pulpcore/tests/unit/test_content_guard.py index 9024020f23..284afe6344 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)