Remove support for redundant request media types in the API (#5874)

Currently, every API endpoint that takes a request body supports (or at
least declares to support) 4 media types:

* `application/json`
* `application/offset+octet-stream`
* `application/x-www-form-urlencoded`
* `multipart/form-data`

Supporting multiple media types has a cost. We need to test that the
various media types actually work, and we need to document their use
(e.g., providing examples for each supported type). In practice, we
mostly don't... but we still need to. In addition, the user, seeing the
list of supported types, has to decide which one to use.

Now, the cost could be worthwhile if the multiple type support provided
value. However, for the most part, it doesn't:

* `application/offset+octet-stream` only makes sense for the TUS
endpoints. Moreover, for those endpoints it's the only type that makes
sense.

* `application/x-www-form-urlencoded` is strictly inferior to JSON. It
doesn't support compound values, and it doesn't carry type information,
so you can't, for example, distinguish a string from a null. It's
potentially susceptible to CSRF attacks (we have protections against
those, but we could accidentally disable them and not notice). Its main
use is for form submissions, but we don't use HTML-based submissions.

* `multipart/form-data` shares the downsides of
`application/x-www-form-urlencoded`, however it does have a redeeming
quality: it allows to efficiently upload binary files. Therefore, it has
legitimate uses in endpoints that accept such files.

Therefore, I believe it is justified to reduce the API surface area as
follows:

* Restrict `application/offset+octet-stream` to TUS endpoints and remove
support for other types from those endpoints.

* Remove `application/x-www-form-urlencoded` support entirely.

* Restrict `multipart/form-data` support to endpoints dealing with file
uploads.

Note that I had to keep `multipart/form-data` support for `POST
/api/cloudstorages` and `PATCH /api/cloudstorages/<id>`. That's because
they accept a file-type parameter (`key_file`). I don't especially like
this. Key files are not big, so the efficiency benefits of
`multipart/form-data` don't matter. Therefore, I don't think we really
need to support this type here; it would be more elegant to just use
JSON and Base64-encode the key file contents. However, I don't have time
to make that change right now, so I'm
leaving it for another time.

CHANGELOG.md

@@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Removed
 - Cloud storage unique_together limitation (<https://github.com/opencv/cvat/pull/5855>)
+- Support for redundant request media types in the API
+  (<https://github.com/opencv/cvat/pull/5874>)
 
 ### Fixed
 - An invalid project/org handling in webhooks (<https://github.com/opencv/cvat/pull/5707>)

cvat/apps/engine/tests/test_rest_api.py

@@ -747,7 +747,7 @@ class UserGetAPITestCase(UserAPITestCase):
 class UserPartialUpdateAPITestCase(UserAPITestCase):
     def _run_api_v2_users_id(self, user, user_id, data):
         with ForceLogin(user, self.client):
-            response = self.client.patch('/api/users/{}'.format(user_id), data=data)
+            response = self.client.patch('/api/users/{}'.format(user_id), data=data, format='json')
 
         return response
 

cvat/apps/engine/views.py

@@ -33,8 +33,10 @@ from drf_spectacular.plumbing import build_array_type, build_basic_type
 from rest_framework import mixins, serializers, status, viewsets
 from rest_framework.decorators import action
 from rest_framework.exceptions import APIException, NotFound, ValidationError, PermissionDenied
+from rest_framework.parsers import MultiPartParser
 from rest_framework.permissions import SAFE_METHODS
 from rest_framework.response import Response
+from rest_framework.settings import api_settings
 from django_sendfile import sendfile
 
 import cvat.apps.dataset_manager as dm
@@ -50,6 +52,7 @@ from cvat.apps.engine.models import (
     CloudProviderChoice, Location
 )
 from cvat.apps.engine.models import CloudStorage as CloudStorageModel
+from cvat.apps.engine.parsers import TusUploadParser
 from cvat.apps.engine.serializers import (
     AboutSerializer, AnnotationFileSerializer, BasicUserSerializer,
     DataMetaReadSerializer, DataMetaWriteSerializer, DataSerializer,
@@ -78,6 +81,8 @@ from cvat.apps.iam.permissions import (CloudStoragePermission,
 from cvat.apps.engine.cache import MediaCache
 from cvat.apps.events.handlers import handle_annotations_patch
 
+_UPLOAD_PARSER_CLASSES = api_settings.DEFAULT_PARSER_CLASSES + [MultiPartParser]
+
 @extend_schema(tags=['server'])
 class ServerViewSet(viewsets.ViewSet):
     serializer_class = None
@@ -306,7 +311,7 @@ class ProjectViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
             '405': OpenApiResponse(description='Format is not available'),
         })
     @action(detail=True, methods=['GET', 'POST', 'OPTIONS'], serializer_class=None,
-        url_path=r'dataset/?$')
+        url_path=r'dataset/?$', parser_classes=_UPLOAD_PARSER_CLASSES)
     def dataset(self, request, pk):
         self._object = self.get_object() # force call of check_object_permissions()
         rq_id = f"import:dataset-for-project.id{pk}-by-{request.user}"
@@ -367,7 +372,8 @@ class ProjectViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
     @extend_schema(methods=['HEAD'],
         summary="Implements TUS file uploading protocol."
     )
-    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='dataset/'+UploadMixin.file_id_regex)
+    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='dataset/'+UploadMixin.file_id_regex,
+        parser_classes=[TusUploadParser])
     def append_dataset_chunk(self, request, pk, file_id):
         self._object = self.get_object()
         return self.append_tus_chunk(request, file_id)
@@ -499,7 +505,8 @@ class ProjectViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
             '202': OpenApiResponse(description='Importing a backup file has been started'),
         })
     @action(detail=False, methods=['OPTIONS', 'POST'], url_path=r'backup/?$',
-        serializer_class=ProjectFileSerializer(required=False))
+        serializer_class=ProjectFileSerializer(required=False),
+        parser_classes=_UPLOAD_PARSER_CLASSES)
     def import_backup(self, request, pk=None):
         return self.deserialize(request, backup.import_project)
 
@@ -514,7 +521,7 @@ class ProjectViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
         summary="Implements TUS file uploading protocol."
     )
     @action(detail=False, methods=['HEAD', 'PATCH'], url_path='backup/'+UploadMixin.file_id_regex,
-        serializer_class=None)
+        serializer_class=None, parser_classes=[TusUploadParser])
     def append_backup_chunk(self, request, file_id):
         return self.append_tus_chunk(request, file_id)
 
@@ -731,7 +738,9 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
             '201': OpenApiResponse(description='The task has been imported'), # or better specify {id: task_id}
             '202': OpenApiResponse(description='Importing a backup file has been started'),
         })
-    @action(detail=False, methods=['OPTIONS', 'POST'], url_path=r'backup/?$', serializer_class=TaskFileSerializer(required=False))
+    @action(detail=False, methods=['OPTIONS', 'POST'], url_path=r'backup/?$',
+        serializer_class=TaskFileSerializer(required=False),
+        parser_classes=_UPLOAD_PARSER_CLASSES)
     def import_backup(self, request, pk=None):
         return self.deserialize(request, backup.import_task)
 
@@ -745,7 +754,8 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
     @extend_schema(methods=['HEAD'],
         summary="Implements TUS file uploading protocol."
     )
-    @action(detail=False, methods=['HEAD', 'PATCH'], url_path='backup/'+UploadMixin.file_id_regex)
+    @action(detail=False, methods=['HEAD', 'PATCH'], url_path='backup/'+UploadMixin.file_id_regex,
+        parser_classes=[TusUploadParser])
     def append_backup_chunk(self, request, file_id):
         return self.append_tus_chunk(request, file_id)
 
@@ -909,7 +919,8 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
         responses={
             '200': OpenApiResponse(description='Data of a specific type'),
         })
-    @action(detail=True, methods=['OPTIONS', 'POST', 'GET'], url_path=r'data/?$')
+    @action(detail=True, methods=['OPTIONS', 'POST', 'GET'], url_path=r'data/?$',
+        parser_classes=_UPLOAD_PARSER_CLASSES)
     def data(self, request, pk):
         self._object = self.get_object() # call check_object_permissions as well
         if request.method == 'POST' or request.method == 'OPTIONS':
@@ -945,7 +956,8 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
     @extend_schema(methods=['HEAD'],
         summary="Implements TUS file uploading protocol."
     )
-    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='data/'+UploadMixin.file_id_regex)
+    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='data/'+UploadMixin.file_id_regex,
+        parser_classes=[TusUploadParser])
     def append_data_chunk(self, request, pk, file_id):
         self._object = self.get_object()
         return self.append_tus_chunk(request, file_id)
@@ -1032,7 +1044,7 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
             '204': OpenApiResponse(description='The annotation has been deleted'),
         })
     @action(detail=True, methods=['GET', 'DELETE', 'PUT', 'PATCH', 'POST', 'OPTIONS'], url_path=r'annotations/?$',
-        serializer_class=None)
+        serializer_class=None, parser_classes=_UPLOAD_PARSER_CLASSES)
     def annotations(self, request, pk):
         self._object = self.get_object() # force call of check_object_permissions()
         if request.method == 'GET':
@@ -1106,7 +1118,8 @@ class TaskViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
         operation_id='tasks_annotations_file_retrieve_status',
         summary="Implements TUS file uploading protocol."
     )
-    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='annotations/'+UploadMixin.file_id_regex)
+    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='annotations/'+UploadMixin.file_id_regex,
+        parser_classes=[TusUploadParser])
     def append_annotations_chunk(self, request, pk, file_id):
         self._object = self.get_object()
         return self.append_tus_chunk(request, file_id)
@@ -1428,7 +1441,7 @@ class JobViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
             '204': OpenApiResponse(description='The annotation has been deleted'),
         })
     @action(detail=True, methods=['GET', 'DELETE', 'PUT', 'PATCH', 'POST', 'OPTIONS'], url_path=r'annotations/?$',
-        serializer_class=LabeledDataSerializer)
+        serializer_class=LabeledDataSerializer, parser_classes=_UPLOAD_PARSER_CLASSES)
     def annotations(self, request, pk):
         self._object = self.get_object() # force call of check_object_permissions()
         if request.method == 'GET':
@@ -1506,7 +1519,8 @@ class JobViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
     @extend_schema(methods=['HEAD'],
         summary="Implements TUS file uploading protocol."
     )
-    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='annotations/'+UploadMixin.file_id_regex)
+    @action(detail=True, methods=['HEAD', 'PATCH'], url_path='annotations/'+UploadMixin.file_id_regex,
+        parser_classes=[TusUploadParser])
     def append_annotations_chunk(self, request, pk, file_id):
         self._object = self.get_object()
         return self.append_tus_chunk(request, file_id)
@@ -2077,6 +2091,10 @@ class CloudStorageViewSet(viewsets.GenericViewSet, mixins.ListModelMixin,
     lookup_fields = {'owner': 'owner__username', 'name': 'display_name'}
     iam_organization_field = 'organization'
 
+    # Multipart support is necessary here, as CloudStorageWriteSerializer
+    # contains a file field (key_file).
+    parser_classes = _UPLOAD_PARSER_CLASSES
+
     def get_serializer_class(self):
         if self.request.method in ('POST', 'PATCH'):
             return CloudStorageWriteSerializer

cvat/settings/base.py

@@ -147,9 +147,6 @@ SITE_ID = 1
 REST_FRAMEWORK = {
     'DEFAULT_PARSER_CLASSES': [
         'rest_framework.parsers.JSONParser',
-        'rest_framework.parsers.FormParser',
-        'rest_framework.parsers.MultiPartParser',
-        'cvat.apps.engine.parsers.TusUploadParser',
     ],
     'DEFAULT_RENDERER_CLASSES': [
         'cvat.apps.engine.renderers.CVATAPIRenderer',