diff --git a/apps/downloads/tests/test_views.py b/apps/downloads/tests/test_views.py index 4bae6676f..1a05dfd40 100644 --- a/apps/downloads/tests/test_views.py +++ b/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")) diff --git a/pydotorg/resources.py b/pydotorg/resources.py index 05c4a3665..c69223b51 100644 --- a/pydotorg/resources.py +++ b/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 diff --git a/pydotorg/tests/test_resources.py b/pydotorg/tests/test_resources.py index d3c7019fb..26faa0443 100644 --- a/pydotorg/tests/test_resources.py +++ b/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)