From 2d31e1820fe18c2cabc8fbe17ccef2324d49b789 Mon Sep 17 00:00:00 2001 From: Rich Wareham Date: Mon, 19 Nov 2018 15:36:25 +0000 Subject: [PATCH] ensure media upload endpoints are one-off JWP media upload endpoints are one-use-only URLs. Unfortunately we allowed the same URL to be retrieved multiple times from the upload endpoint which meant that a client may try to upload a media item multiple times to the same URL. Fix this by making the /media/{id}/upload endpoint a PUT-only endpoint. If the upload endpoint URL exists in the database to begin with, we return it and delete it from the database. If the endpoint does not exist, we create one first. This involves a bit of an unnecessary write and then delete from the database in the case where the endpoint does not exist but for the common case of "create media item" then "get upload endpoint", it Does The Right Thing (TM). Closes #367 --- README.md | 2 +- api/serializers.py | 25 +++++++ api/tests/test_views.py | 95 ++++++++++++++++++++---- api/views.py | 6 +- guides/uploading.md | 2 +- ui/frontend/src/api.ts | 6 +- ui/frontend/src/containers/UploadForm.js | 4 +- 7 files changed, 118 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index eb1133a5..9512c41b 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ $ ./compose.sh tox run -v $PWD:/tmp/workspace -e TOXINI_ARTEFACT_DIR=/tmp/worksp The following developer guides have been written: -* [guides/uploading.md](Uploading new media items). +* [Uploading new media items](guides/uploading.md). ## Dockerfile configuration diff --git a/api/serializers.py b/api/serializers.py index c54aa80f..a1ce0657 100644 --- a/api/serializers.py +++ b/api/serializers.py @@ -1,9 +1,11 @@ +import datetime import logging from urllib import parse as urlparse from django.conf import settings from django.http import QueryDict from django.urls import reverse +from django.utils import timezone from django.utils.http import urlencode from rest_framework import serializers @@ -309,8 +311,31 @@ class MediaUploadSerializer(serializers.Serializer): expires_at = serializers.DateTimeField(source='upload_endpoint.expires_at', read_only=True) def update(self, instance, verified_data): + """ + Ensure that the instance has an upload endpoint which has not expired. Delete it just + before returning it to the rest of the serialiser so that we are sure we only ever give an + upload URL back to a client once. + + """ + # If there is already a created upload endpoint which expires more than a day from now, + # we can use the instance as is. + if hasattr(instance, 'upload_endpoint'): + headroom = datetime.timedelta(days=1) + if instance.upload_endpoint.expires_at >= timezone.now() + headroom: + # Delete the endpoint because we're about to return it to the client + instance.upload_endpoint.delete() + return instance + + # Otherwise, delete the existing endpoint; we'll create another + instance.upload_endpoint.delete() + + # Create an upload endpoint and re-fetch the instance # TODO: abstract the creation of UploadEndpoint objects to be backend neutral management.create_upload_endpoint(instance) + instance.refresh_from_db() + + # Delete the endpoint because we're about to return it to the client + instance.upload_endpoint.delete() return instance diff --git a/api/tests/test_views.py b/api/tests/test_views.py index 65209f74..95a00694 100644 --- a/api/tests/test_views.py +++ b/api/tests/test_views.py @@ -7,10 +7,12 @@ from django.http import QueryDict from django.test import TestCase, override_settings from django.urls import reverse +from django.utils import timezone from rest_framework.authtoken.models import Token from rest_framework.test import APIRequestFactory, force_authenticate import mediaplatform_jwp.api.delivery as api +import mediaplatform_jwp.upload as jwp_upload import mediaplatform.models as mpmodels from . import create_stats_table, delete_stats_table, add_stat @@ -784,49 +786,104 @@ def setUp(self): self.item.channel.edit_permission.reset() self.item.channel.edit_permission.save() + # The upload endpoint has not expired + self.item.upload_endpoint.expires_at = timezone.now() + datetime.timedelta(days=200) + self.item.upload_endpoint.save() + + create_patch = mock.patch('mediaplatform_jwp.api.management.create_upload_endpoint') + self.mock_create_upload_endpoint = create_patch.start() + self.mock_create_upload_endpoint.side_effect = self.mock_create_upload_endpoint_side_effect + self.addCleanup(create_patch.stop) + + def test_no_get(self): + """Get request is not supported.""" + response = self.get_for_item() + self.assertEqual(response.status_code, 405) # method not allowed + def test_needs_view_permission(self): """Upload endpoint should 404 if user doesn't have view permission.""" - response = self.get_for_item() - self.assertEqual(response.status_code, 404) + response = self.put_for_item() + self.assertEqual(response.status_code, 403) self.client.force_login(self.user) - response = self.get_for_item() + response = self.put_for_item() self.assertEqual(response.status_code, 404) def test_needs_edit_permission(self): """If user has view but not edit permission, endpoint should deny permission.""" self.add_view_permission() self.client.force_login(self.user) - response = self.get_for_item() + response = self.put_for_item() self.assertEqual(response.status_code, 403) def test_allows_view_and_edit_permission(self): """If user has view *and* edit permission, endpoint should succeed.""" + # Get a reference to the upload endpoint before it is deleted + upload_endpoint = self.item.upload_endpoint + self.client.force_login(self.user) self.add_view_permission() self.add_edit_permission() - response = self.get_for_item() + + response = self.put_for_item() self.assertEqual(response.status_code, 200) + body = response.json() - self.assertEqual(body['url'], self.item.upload_endpoint.url) + self.assertEqual(body['url'], upload_endpoint.url) self.assertEqual( dateparser.parse(body['expires_at']), - self.item.upload_endpoint.expires_at + upload_endpoint.expires_at ) + # The upload endpoint should no longer be in the DB now it has been retrieved + self.item.refresh_from_db() + self.assertFalse(hasattr(self.item, 'upload_endpoint')) + + def test_existing_upload_endpoint(self): + """PUT-ing endpoint does not create a new upload endpoint if one exists.""" + self.assertTrue(hasattr(self.item, 'upload_endpoint')) + self.client.force_login(self.user) + self.add_view_permission() + self.add_edit_permission() + + response = self.put_for_item() + + self.assertEqual(response.status_code, 200) + self.mock_create_upload_endpoint.assert_not_called() + + def test_existing_expired_upload_endpoint(self): + """PUT-ing endpoint *does* create a new upload endpoint if one exists but it has + expired or is close to expiring.""" + # Expire the endpoint (or, rather, it expired in one hour) + self.item.upload_endpoint.expires_at = timezone.now() + datetime.timedelta(hours=1) + self.item.upload_endpoint.save() + + self.assertTrue(hasattr(self.item, 'upload_endpoint')) + self.client.force_login(self.user) + self.add_view_permission() + self.add_edit_permission() + + response = self.put_for_item() + + self.assertEqual(response.status_code, 200) + self.mock_create_upload_endpoint.assert_called() + item = self.mock_create_upload_endpoint.call_args[0][0] + self.assertEqual(item.id, self.item.id) + def test_create_upload_endpoint(self): - """PUT-ing endpoint creates a new upload endpoint.""" + """PUT-ing endpoint creates a new upload endpoint if one does not exist.""" + self.item.upload_endpoint.delete() + self.item.refresh_from_db() + self.assertFalse(hasattr(self.item, 'upload_endpoint')) self.client.force_login(self.user) self.add_view_permission() self.add_edit_permission() - with mock.patch('mediaplatform_jwp.api.management.create_upload_endpoint' - ) as mock_create: - response = self.put_for_item() + response = self.put_for_item() self.assertEqual(response.status_code, 200) - mock_create.assert_called_once() - item = mock_create.call_args[0][0] + self.mock_create_upload_endpoint.assert_called_once() + item = self.mock_create_upload_endpoint.call_args[0][0] self.assertEqual(item.id, self.item.id) def get_for_item(self, **kwargs): @@ -835,6 +892,18 @@ def get_for_item(self, **kwargs): def put_for_item(self, **kwargs): return self.client.put(reverse('api:media_upload', kwargs={'pk': self.item.pk}), **kwargs) + def mock_create_upload_endpoint_side_effect(self, item): + response = { + 'protocol': 'https', + 'address': 'mock.invalid', + 'path': '/some/upload/endpoint', + 'query': { + 'key': 'some-key', + 'token': 'some-token', + } + } + jwp_upload.record_link_response(response, item) + def add_view_permission(self): self.item.view_permission.crsids.append(self.user.username) self.item.view_permission.save() diff --git a/api/views.py b/api/views.py index 22980204..2c796eb6 100644 --- a/api/views.py +++ b/api/views.py @@ -308,11 +308,11 @@ class MediaItemView(MediaItemMixin, generics.RetrieveUpdateAPIView): serializer_class = serializers.MediaItemDetailSerializer -class MediaItemUploadView(MediaItemMixin, generics.RetrieveUpdateAPIView): +class MediaItemUploadView(MediaItemMixin, generics.UpdateAPIView): """ Endpoint for retrieving an upload URL for a media item. Requires that the user have the edit - permission for the media item. Should the upload URL be expired or otherwise unsuitable, a HTTP - POST/PUT to this endpoint refreshes the URL. + permission for the media item. A HTTP PUT to this endpoint can be used to retrieve an upload + URL which can then have the media file POST-ed to it. """ # To access the upload API, the user must always have the edit permission. diff --git a/guides/uploading.md b/guides/uploading.md index 2bc0c149..977a5a37 100644 --- a/guides/uploading.md +++ b/guides/uploading.md @@ -53,7 +53,7 @@ URL which accepts a HTTP POST with the media item source file form encoded as the "file" field. **Uploading media via a HTTP POST to the upload URL does not require further authentication beyond knowing the URL.** -To retrieve the upload URL, perform an authenticated HTTP POST to +To retrieve the upload URL, perform an authenticated HTTP PUT to ``${BASE}/media/{id}/upload`` replacing ``{id}`` with the ``id`` property from the JSON document describing the new media item. The request body may be empty. diff --git a/ui/frontend/src/api.ts b/ui/frontend/src/api.ts index e0efb2d0..06a1f935 100644 --- a/ui/frontend/src/api.ts +++ b/ui/frontend/src/api.ts @@ -282,9 +282,11 @@ export const mediaPatch = (item: IMediaPatchResource) : Promise }; /** Retrieve upload endpoint for a media item. */ -export const mediaUploadGet = (item: IMediaResource) : Promise => { +export const mediaUploadPut = (item: IMediaResource) : Promise => { // TODO: decide if we want to use the URL in @id rather than key here, - return apiFetch(API_ENDPOINTS.mediaList + item.id + '/upload'); + return apiFetch(API_ENDPOINTS.mediaList + item.id + '/upload', { + method: 'PUT', + }); }; /** Retrieve a media resource's analytics. */ diff --git a/ui/frontend/src/containers/UploadForm.js b/ui/frontend/src/containers/UploadForm.js index 0da6910e..ee06b09d 100644 --- a/ui/frontend/src/containers/UploadForm.js +++ b/ui/frontend/src/containers/UploadForm.js @@ -15,7 +15,7 @@ import { withStyles } from '@material-ui/core/styles'; import MediaDropzone from '../components/MediaDropzone'; import ItemMetadataForm from '../components/ItemMetadataForm'; -import { mediaCreate, mediaPatch, mediaUploadGet, } from '../api'; +import { mediaCreate, mediaPatch, mediaUploadPut } from '../api'; import ChannelSelect from "./ChannelSelect"; /** @@ -165,7 +165,7 @@ class UploadForm extends Component { /** Called when a new media item has been created to receive the upload. */ setMediaItem(item) { - mediaUploadGet(item).then(({ url }) => this.setUploadUrl(url)); + mediaUploadPut(item).then(({ url }) => this.setUploadUrl(url)); this.setState({ item, draftItem: { ...item, ...this.state.draftItem } }); }