Skip to content

Commit

Permalink
Remove support for redundant request media types in the API
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
SpecLad committed Mar 28, 2023
1 parent dcb7fb2 commit 43f1e9e
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 420 deletions.
42 changes: 30 additions & 12 deletions cvat/apps/engine/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@
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
Expand All @@ -50,6 +52,7 @@
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,
Expand Down Expand Up @@ -78,6 +81,8 @@
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
Expand Down Expand Up @@ -306,7 +311,7 @@ def perform_create(self, serializer, **kwargs):
'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}"
Expand Down Expand Up @@ -367,7 +372,8 @@ def dataset(self, request, pk):
@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)
Expand Down Expand Up @@ -499,7 +505,8 @@ def export_backup(self, request, pk=None):
'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)

Expand All @@ -514,7 +521,7 @@ def import_backup(self, request, pk=None):
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)

Expand Down Expand Up @@ -731,7 +738,9 @@ def get_queryset(self):
'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)

Expand All @@ -745,7 +754,8 @@ def import_backup(self, request, pk=None):
@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)

Expand Down Expand Up @@ -909,7 +919,8 @@ def upload_finished(self, request):
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':
Expand Down Expand Up @@ -945,7 +956,8 @@ def data(self, request, pk):
@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)
Expand Down Expand Up @@ -1032,7 +1044,7 @@ def append_data_chunk(self, request, pk, file_id):
'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':
Expand Down Expand Up @@ -1106,7 +1118,8 @@ def annotations(self, request, pk):
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)
Expand Down Expand Up @@ -1428,7 +1441,7 @@ def upload_finished(self, request):
'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':
Expand Down Expand Up @@ -1506,7 +1519,8 @@ def annotations(self, request, pk):
@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)
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 43f1e9e

Please sign in to comment.