BID API: more generic way to check the user ID in API views
authorSybren A. Stüvel <sybren@stuvel.eu>
Tue, 21 May 2019 11:33:26 +0000 (13:33 +0200)
committerSybren A. Stüvel <sybren@stuvel.eu>
Fri, 31 May 2019 13:54:49 +0000 (15:54 +0200)
The same thing was done in a few different ways, so now there is one way
to check that the User ID on the URL matches the authentication token
used.

bid_api/tests/test_info.py
bid_api/views/abstract.py
bid_api/views/info.py

index de5761e919e106d9822a426df655d065a7b4cc0e..fda6e8ddc9c9b03ae4d6a17dee196df2f0f5c272 100644 (file)
@@ -391,7 +391,7 @@ class UserBadgeHTMLTest(AbstractBadgeTest):
         other_user_token.save()
 
         resp = self.get(self.target_user.id, size='', access_token='other-user-token')
-        self.assertEqual(400, resp.status_code)
+        self.assertEqual(403, resp.status_code)
 
     def test_hide_private_badges(self):
         # Mark the Cloud badge as private. This should hide the user's only badge, and
index 4ddd1ba6154930ac94af9996df8836b5616c6686..58b47ff1e7e886d38c19687eb2a3d5f4b26565fc 100644 (file)
@@ -1,11 +1,16 @@
+import logging
+import typing
 import urllib.parse
 
 from django.contrib.sites.models import Site
+from django.http import JsonResponse
 from django.utils.decorators import method_decorator
 from django.views.decorators.csrf import csrf_exempt
 from django.views.decorators.cache import never_cache
 from django.views.generic import View
 
+log = logging.getLogger(__name__)
+
 
 class AbstractAPIView(View):
     """Excempted from CSRF requests and never cached."""
@@ -20,3 +25,24 @@ class AbstractAPIView(View):
 
         current_site = Site.objects.get_current()
         return urllib.parse.urljoin(f'{request.scheme}://{current_site.domain}/', relative_url)
+
+    def check_user_id(self, request, user_id_from_url: str) -> typing.Optional[JsonResponse]:
+        """Check whether the user ID owns the authentication token.
+
+        :return: tuple (uid, err), where uid is the integer user ID, and err is
+            a JSON response with an error message, or None if all is ok.
+        """
+
+        request_user = getattr(request, 'user', None)
+        if request_user is None or str(request.user.id) != user_id_from_url:
+            my_log = getattr(self, 'log', log)
+            my_log.warning('Request from %s on %s for user %s did not match auth token owner %s',
+                           request.META.get('REMOTE_ADDR', '-unknown-'),
+                           request.path,
+                           user_id_from_url,
+                           request.user.id)
+
+            return JsonResponse(
+                {'message': 'user ID on URL did not match authentication token owner'},
+                status=403)
+        return None
index 7c6a1b91364d2df5b03e0f5f816d61c3254f0b64..d11a577af41c3c5d76efa76bbac5cc298ae384ac 100644 (file)
@@ -2,8 +2,7 @@ import logging
 import typing
 
 from django.contrib.auth import get_user_model
-from django.http import (JsonResponse, HttpResponseBadRequest, HttpResponseNotFound,
-                         HttpResponseForbidden)
+from django.http import (JsonResponse, HttpResponseBadRequest, HttpResponseNotFound)
 from django.shortcuts import render
 from django.utils.decorators import method_decorator
 from oauth2_provider.decorators import protected_resource
@@ -69,30 +68,16 @@ class UserBadgeView(AbstractAPIView):
 
     @method_decorator(protected_resource(scopes=['badge']))
     def get(self, request, user_id) -> JsonResponse:
+        err = self.check_user_id(request, user_id)
+        if err is not None:
+            return err
 
-        try:
-            uid = int(user_id)
-        except TypeError:
-            # This is unlikely to happen as the URL only matches digits.
-            return JsonResponse({'message': 'invalid user ID'}, status=400)
-
-        if request.user.id != uid:
-            return JsonResponse(
-                {'message': 'you can only request badges for the owner of this token'},
-                status=403)
-
-        log.debug('Fetching badges of user %d on behalf of API user %s', uid, request.user)
-        try:
-            user = UserModel.objects.get(id=uid)
-        except UserModel.DoesNotExist:
-            # This is very unlikely, as we could authenticate the user from a token.
-            return JsonResponse({'message': 'user not found'}, status=422)
-
+        log.debug('Fetching badges of user %s', request.user)
         badges = {
             role.name: self.badge_dict(request, role)
-            for role in user.public_badges()
+            for role in request.user.public_badges()
         }
-        return JsonResponse({'user_id': user.id,
+        return JsonResponse({'user_id': request.user.id,
                              'badges': badges})
 
     def badge_dict(self, request, role):
@@ -138,10 +123,9 @@ class BadgesHTMLView(AbstractAPIView):
         and without having to cache the authentication token too.
         """
 
-        if str(request.user.id) != user_id:
-            self.log.warning('Request from %s for user %s did not match auth token owner %s',
-                             request.META.get('REMOTE_ADDR', '-unknown-'), user_id, request.user.id)
-            return HttpResponseBadRequest(f'User ID {user_id} does not match auth token.')
+        err = self.check_user_id(request, user_id)
+        if err is not None:
+            return err
 
         badges: typing.Iterable[Role] = request.user.public_badges()
         if not badges: