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