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

@@ -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
 

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
 
 

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()

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.

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.
 

ui/frontend/src/api.ts

@@ -282,9 +282,11 @@ export const mediaPatch = (item: IMediaPatchResource) : Promise<IMediaResource>
 };
 
 /** Retrieve upload endpoint for a media item. */
-export const mediaUploadGet = (item: IMediaResource) : Promise<IMediaUploadResource> => {
+export const mediaUploadPut = (item: IMediaResource) : Promise<IMediaUploadResource> => {
   // 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. */

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 } });
   }