Delineate the guest and API key authentication branches (#2946)

Co-authored-by: Hugo van Kemenade <[email protected]>

Author: sethmlarson

apps/downloads/tests/test_views.py

@@ -194,10 +194,7 @@ def test_invalid_token(self):
 
         url = self.create_url("os")
         response = self.client.get(url, headers={"authorization": self.Authorization_invalid})
-        # TODO: API v1 returns 200 for a GET request even if token is invalid.
-        # 'StaffAuthorization.read_list` returns 'object_list' unconditionally,
-        # and 'StaffAuthorization.read_detail` returns 'True'.
-        self.assertIn(response.status_code, [200, 401])
+        self.assertEqual(response.status_code, 401)
 
     def test_get_os(self):
         response = self.client.get(self.create_url("os"))

pydotorg/resources.py

@@ -12,11 +12,6 @@
 class ApiKeyOrGuestAuthentication(ApiKeyAuthentication):
     """Authentication backend that falls back to guest access when no API key is provided."""
 
-    def _unauthorized(self):
-        """Allow guests anyway."""
-        # Allow guests anyway
-        return True
-
     def is_authenticated(self, request, **kwargs):
         """Authenticate via API key, handling custom user model.
 
@@ -26,19 +21,26 @@ def is_authenticated(self, request, **kwargs):
         User = get_user_model()  # noqa: N806 - Django convention for user model reference
         username_field = User.USERNAME_FIELD
 
+        # Note that it's only safe to return 'True'
+        # in the guest case. If there is an API key supplied
+        # then we must not return 'True' unless the
+        # API key is valid.
         try:
             username, api_key = self.extract_credentials(request)
         except ValueError:
-            return self._unauthorized()
-
+            return True  # Allow guests.
         if not username or not api_key:
-            return self._unauthorized()
+            return True  # Allow guests.
+
+        # IMPORTANT: Beyond this point we are no longer
+        # handling the guest case, so all incorrect usernames
+        # or credentials MUST return HttpUnauthorized()
 
         try:
             lookup_kwargs = {username_field: username}
             user = User.objects.get(**lookup_kwargs)
         except (User.DoesNotExist, User.MultipleObjectsReturned):
-            return self._unauthorized()
+            return HttpUnauthorized()
 
         if not self.check_active(user):
             return False

pydotorg/tests/test_resources.py

@@ -1,6 +1,7 @@
 from django.contrib.auth import get_user_model
 from django.http import HttpRequest
 from django.test import TestCase
+from tastypie.http import HttpUnauthorized
 
 from pydotorg.resources import ApiKeyOrGuestAuthentication
 
@@ -21,6 +22,21 @@ def test_authentication(self):
         auth = ApiKeyOrGuestAuthentication()
         self.assertTrue(auth.is_authenticated(request))
 
-        request.GET["username"] = self.staff_user.email
+        request.GET["username"] = self.staff_user.username
+        self.assertTrue(auth.is_authenticated(request))
+
         request.GET["api_key"] = self.staff_user.api_key.key
         self.assertTrue(auth.is_authenticated(request))
+
+    def test_authentication_staff_unauthorized(self):
+        auth = ApiKeyOrGuestAuthentication()
+
+        request = HttpRequest()
+        request.GET["username"] = self.staff_user.username
+        request.GET["api_key"] = "not-api-key"
+        self.assertIsInstance(auth.is_authenticated(request), HttpUnauthorized)
+
+        request = HttpRequest()
+        request.GET["username"] = "not-staff"
+        request.GET["api_key"] = self.staff_user.api_key.key
+        self.assertIsInstance(auth.is_authenticated(request), HttpUnauthorized)