ensure media upload endpoints are one-off

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