diff --git a/openedx/core/djangoapps/profile_images/images.py b/openedx/core/djangoapps/profile_images/images.py index 4e78295a98..cf8e8b4d34 100644 --- a/openedx/core/djangoapps/profile_images/images.py +++ b/openedx/core/djangoapps/profile_images/images.py @@ -2,6 +2,7 @@ Image file manipulation functions related to profile images. """ from cStringIO import StringIO +from collections import namedtuple from django.conf import settings from django.core.files.base import ContentFile @@ -11,22 +12,24 @@ from PIL import Image from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_storage +ImageType = namedtuple('ImageType', ('extensions', 'mimetypes', 'magic')) + IMAGE_TYPES = { - 'jpeg': { - 'extension': [".jpeg", ".jpg"], - 'mimetypes': ['image/jpeg', 'image/pjpeg'], - 'magic': ["ffd8"] - }, - 'png': { - 'extension': [".png"], - 'mimetypes': ['image/png'], - 'magic': ["89504e470d0a1a0a"] - }, - 'gif': { - 'extension': [".gif"], - 'mimetypes': ['image/gif'], - 'magic': ["474946383961", "474946383761"] - } + 'jpeg': ImageType( + extensions=['.jpeg', '.jpg'], + mimetypes=['image/jpeg', 'image/pjpeg'], + magic=['ffd8'], + ), + 'png': ImageType( + extensions=[".png"], + mimetypes=['image/png'], + magic=["89504e470d0a1a0a"], + ), + 'gif': ImageType( + extensions=[".gif"], + mimetypes=['image/gif'], + magic=["474946383961", "474946383761"], + ), } @@ -52,7 +55,7 @@ def get_valid_file_types(): """ Return comma separated string of valid file types. """ - return ', '.join([', '.join(IMAGE_TYPES[ft]['extension']) for ft in IMAGE_TYPES.keys()]) + return ', '.join([', '.join(IMAGE_TYPES[ft].extensions) for ft in IMAGE_TYPES.keys()]) FILE_UPLOAD_TOO_LARGE = _noop(u'The file must be smaller than {image_max_size} in size.'.format(image_max_size=user_friendly_size(settings.PROFILE_IMAGE_MAX_BYTES))) # pylint: disable=line-too-long @@ -93,17 +96,17 @@ def validate_uploaded_image(uploaded_file): # check the file extension looks acceptable filename = unicode(uploaded_file.name).lower() - filetype = [ft for ft in IMAGE_TYPES if any(filename.endswith(ext) for ext in IMAGE_TYPES[ft]['extension'])] + filetype = [ft for ft in IMAGE_TYPES if any(filename.endswith(ext) for ext in IMAGE_TYPES[ft].extensions)] if not filetype: raise ImageValidationError(FILE_UPLOAD_BAD_TYPE) filetype = filetype[0] # check mimetype matches expected file type - if uploaded_file.content_type not in IMAGE_TYPES[filetype]['mimetypes']: + if uploaded_file.content_type not in IMAGE_TYPES[filetype].mimetypes: raise ImageValidationError(FILE_UPLOAD_BAD_MIMETYPE) # check magic number matches expected file type - headers = IMAGE_TYPES[filetype]['magic'] + headers = IMAGE_TYPES[filetype].magic if uploaded_file.read(len(headers[0]) / 2).encode('hex') not in headers: raise ImageValidationError(FILE_UPLOAD_BAD_EXT) # avoid unexpected errors from subsequent modules expecting the fp to be at 0 diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py index 026d2739d5..1042d6318c 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_views.py +++ b/openedx/core/djangoapps/profile_images/tests/test_views.py @@ -10,6 +10,7 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.http import HttpResponse +import ddt import mock from mock import patch from PIL import Image @@ -17,12 +18,12 @@ from rest_framework.test import APITestCase, APIClient from student.tests.factories import UserFactory from student.tests.tests import UserSettingsEventTestMixin - -from ...user_api.accounts.image_helpers import ( +from openedx.core.djangoapps.user_api.accounts.image_helpers import ( set_has_profile_image, get_profile_image_names, get_profile_image_storage, ) + from ..images import create_profile_images, ImageValidationError from ..views import LOG_MESSAGE_CREATE, LOG_MESSAGE_DELETE from .helpers import make_image_file @@ -169,6 +170,7 @@ class ProfileImageViewGeneralTestCase(ProfileImageEndpointMixin, APITestCase): self.assert_no_events_were_emitted() +@ddt.ddt @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS') @mock.patch('openedx.core.djangoapps.profile_images.views.log') class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase): @@ -196,15 +198,16 @@ class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase): self.check_anonymous_request_rejected('post') self.assertFalse(mock_log.info.called) + @ddt.data('.jpg', '.jpeg', '.jpg', '.jpeg', '.png', '.gif', '.GIF') @patch( 'openedx.core.djangoapps.profile_images.views._make_upload_dt', - side_effect=[TEST_UPLOAD_DT, TEST_UPLOAD_DT2] + side_effect=[TEST_UPLOAD_DT, TEST_UPLOAD_DT2], ) - def test_upload_self(self, mock_make_image_version, mock_log): # pylint: disable=unused-argument + def test_upload_self(self, extension, _mock_make_image_version, mock_log): """ Test that an authenticated user can POST to their own upload endpoint. """ - with make_image_file() as image_file: + with make_image_file(extension=extension) as image_file: response = self.client.post(self.url, {'file': image_file}, format='multipart') self.check_response(response, 204) self.check_images() @@ -222,6 +225,57 @@ class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase): self.check_upload_event_emitted(old=TEST_UPLOAD_DT, new=TEST_UPLOAD_DT2) + @ddt.data( + ('image/jpeg', '.jpg'), + ('image/jpeg', '.jpeg'), + ('image/pjpeg', '.jpg'), + ('image/pjpeg', '.jpeg'), + ('image/png', '.png'), + ('image/gif', '.gif'), + ('image/gif', '.GIF'), + ) + @ddt.unpack + @patch('openedx.core.djangoapps.profile_images.views._make_upload_dt', return_value=TEST_UPLOAD_DT) + def test_upload_by_mimetype(self, content_type, extension, _mock_make_image_version, mock_log): + """ + Test that a user can upload raw content with the appropriate mimetype + """ + with make_image_file(extension=extension) as image_file: + data = image_file.read() + response = self.client.post( + self.url, + data, + content_type=content_type, + HTTP_CONTENT_DISPOSITION='attachment;filename=filename{}'.format(extension), + ) + self.check_response(response, 204) + self.check_images() + self.check_has_profile_image() + mock_log.info.assert_called_once_with( + LOG_MESSAGE_CREATE, + {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id} + ) + self.check_upload_event_emitted() + + def test_upload_unsupported_mimetype(self, mock_log): + """ + Test that uploading an unsupported image as raw content fails with an + HTTP 415 Error. + """ + with make_image_file() as image_file: + data = image_file.read() + response = self.client.post( + self.url, + data, + content_type='image/tiff', + HTTP_CONTENT_DISPOSITION='attachment;filename=filename.tiff', + ) + self.check_response(response, 415) + self.check_images(False) + self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) + self.assert_no_events_were_emitted() + def test_upload_other(self, mock_log): """ Test that an authenticated user cannot POST to another user's upload diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py index c6d8357e3e..235a23eb58 100644 --- a/openedx/core/djangoapps/profile_images/views.py +++ b/openedx/core/djangoapps/profile_images/views.py @@ -3,6 +3,7 @@ This module implements the upload and remove endpoints of the profile image api. """ from contextlib import closing import datetime +import itertools import logging from django.utils.translation import ugettext as _ @@ -17,9 +18,13 @@ from openedx.core.lib.api.authentication import ( OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser, ) +from openedx.core.lib.api.parsers import TypedFileUploadParser from openedx.core.lib.api.permissions import IsUserInUrl, IsUserInUrlOrStaff +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_names, set_has_profile_image -from .images import validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError +from .images import ( + IMAGE_TYPES, validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError +) log = logging.getLogger(__name__) @@ -35,7 +40,7 @@ def _make_upload_dt(): return datetime.datetime.utcnow().replace(tzinfo=utc) -class ProfileImageView(APIView): +class ProfileImageView(DeveloperErrorViewMixin, APIView): """ **Use Cases** @@ -105,10 +110,12 @@ class ProfileImageView(APIView): the user exists or not. """ - parser_classes = (MultiPartParser, FormParser) + parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser) permission_classes = (permissions.IsAuthenticated, IsUserInUrl) + upload_media_types = set(itertools.chain(*(image_type.mimetypes for image_type in IMAGE_TYPES.values()))) + def post(self, request, username): """ POST /api/user/v1/accounts/{username}/image diff --git a/openedx/core/lib/api/parsers.py b/openedx/core/lib/api/parsers.py index fb5470f496..9e1b9c6f44 100644 --- a/openedx/core/lib/api/parsers.py +++ b/openedx/core/lib/api/parsers.py @@ -1,10 +1,83 @@ """ -Custom Django REST Framework request/response pipeline parsers +Custom DRF request parsers. These can be used by views to handle different +content types, as specified by `.media_type`. + +To use these in an APIView, set `.parser_classes` to a list including the +desired parsers. See http://www.django-rest-framework.org/api-guide/parsers/ +for details. """ -from rest_framework import parsers + +from rest_framework.exceptions import ParseError, UnsupportedMediaType +from rest_framework.parsers import FileUploadParser, JSONParser -class MergePatchParser(parsers.JSONParser): +class TypedFileUploadParser(FileUploadParser): + """ + Handles upload of files, ensuring that the media type is supported, and + that the uploaded filename matches the Content-type. + + Requirements: + * The view must have an `upload_media_types` attribute which is a + set (or other container) enumerating the mimetypes of the supported + media formats + + Example: + + View.upload_media_types = {'audio/mp3', 'audio/ogg', 'audio/wav'} + + * Content-type must be set to a supported type (as + defined in View.upload_media_types above). + + Example: + + Content-type: audio/ogg + + * Content-disposition must include a filename with a valid extension + for the specified Content-type. + + Example: + + Content-disposition: attachment; filename="lecture-1.ogg" + """ + + media_type = '*/*' + + # Add more entries to this as needed. All extensions should be lowercase. + file_extensions = { + 'image/gif': {'.gif'}, + 'image/jpeg': {'.jpeg', '.jpg'}, + 'image/pjpeg': {'.jpeg', '.jpg'}, + 'image/png': {'.png'}, + 'image/svg': {'.svg'}, + } + + def parse(self, stream, media_type=None, parser_context=None): + """ + Parse the request, returning a DataAndFiles object with the data dict + left empty, and the body of the request placed in files['file']. + """ + + upload_media_types = getattr(parser_context['view'], 'upload_media_types', set()) + if media_type not in upload_media_types: + raise UnsupportedMediaType(media_type) + + filename = self.get_filename(stream, media_type, parser_context) + if media_type in self.file_extensions: + fileparts = filename.rsplit('.', 1) + if len(fileparts) < 2: + ext = '' + else: + ext = '.{}'.format(fileparts[1]) + if ext.lower() not in self.file_extensions[media_type]: + errmsg = ( + u'File extension does not match requested Content-type. ' + u'Filename: "{filename}", Content-type: "{contenttype}"' + ) + raise ParseError(errmsg.format(filename=filename, contenttype=media_type)) + return super(TypedFileUploadParser, self).parse(stream, media_type, parser_context) + + +class MergePatchParser(JSONParser): """ Custom parser to be used with the "merge patch" implementation (https://tools.ietf.org/html/rfc7396). """ diff --git a/openedx/core/lib/api/tests/test_parsers.py b/openedx/core/lib/api/tests/test_parsers.py new file mode 100644 index 0000000000..6245b23dc4 --- /dev/null +++ b/openedx/core/lib/api/tests/test_parsers.py @@ -0,0 +1,107 @@ +""" +TestCases verifying proper behavior of custom DRF request parsers. +""" + +from collections import namedtuple +from io import BytesIO + +from rest_framework import exceptions +from rest_framework.test import APITestCase, APIRequestFactory + +from openedx.core.lib.api import parsers + + +class TestTypedFileUploadParser(APITestCase): + """ + Tests that verify the behavior of TypedFileUploadParser + """ + def setUp(self): + super(TestTypedFileUploadParser, self).setUp() + self.parser = parsers.TypedFileUploadParser() + self.request_factory = APIRequestFactory() + upload_media_types = {'image/png', 'image/jpeg', 'application/octet-stream'} + self.view = namedtuple('view', ('upload_media_types',))(upload_media_types) + + def test_parse_supported_type(self): + """ + Test that TypedFileUploadParser returns empty data and content stored in + files['file']. + """ + request = self.request_factory.post( + '/', + content_type='image/png', + HTTP_CONTENT_DISPOSITION='attachment; filename="file.PNG"', + ) + context = {'view': self.view, 'request': request} + result = self.parser.parse(stream=BytesIO('abcdefgh'), media_type='image/png', parser_context=context) + self.assertEqual(result.data, {}) + self.assertIn('file', result.files) + self.assertEqual(result.files['file'].read(), 'abcdefgh') + + def test_parse_unsupported_type(self): + """ + Test that TypedFileUploadParser raises an exception when parsing an + unsupported image format. + """ + request = self.request_factory.post( + '/', + content_type='image/tiff', + HTTP_CONTENT_DISPOSITION='attachment; filename="file.tiff"', + ) + context = {'view': self.view, 'request': request} + with self.assertRaises(exceptions.UnsupportedMediaType): + self.parser.parse(stream=BytesIO('abcdefgh'), media_type='image/tiff', parser_context=context) + + def test_parse_unconstrained_type(self): + """ + Test that TypedFileUploader allows any extension for mimetypes without + specified extensions + """ + request = self.request_factory.post( + '/', + content_type='application/octet-stream', + HTTP_CONTENT_DISPOSITION='attachment; filename="VIRUS.EXE', + ) + context = {'view': self.view, 'request': request} + result = self.parser.parse( + stream=BytesIO('abcdefgh'), media_type='application/octet-stream', parser_context=context + ) + self.assertEqual(result.data, {}) + self.assertIn('file', result.files) + self.assertEqual(result.files['file'].read(), 'abcdefgh') + + def test_parse_mismatched_filename_and_mimetype(self): + """ + Test that TypedFileUploadParser raises an exception when the specified + content-type doesn't match the filename extension in the + content-disposition header. + """ + request = self.request_factory.post( + '/', + content_type='image/png', + HTTP_CONTENT_DISPOSITION='attachment; filename="file.jpg"', + ) + context = {'view': self.view, 'request': request} + with self.assertRaises(exceptions.ParseError) as err: + self.parser.parse(stream=BytesIO('abcdefgh'), media_type='image/png', parser_context=context) + self.assertIn('developer_message', err.detail) + self.assertNotIn('user_message', err.detail) + + def test_no_acceptable_types(self): + """ + If the view doesn't specify supported types, the parser rejects + everything. + """ + view = object() + self.assertFalse(hasattr(view, 'upload_media_types')) + + request = self.request_factory.post( + '/', + content_type='image/png', + HTTP_CONTENT_DISPOSITION='attachment; filename="file.png"', + ) + context = {'view': view, 'request': request} + with self.assertRaises(exceptions.UnsupportedMediaType) as err: + self.parser.parse(stream=BytesIO('abcdefgh'), media_type='image/png', parser_context=context) + self.assertIn('developer_message', err.detail) + self.assertIn('user_message', err.detail)