Return correct error code on failed authentication

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.

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",

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)

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',

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

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)