Merge pull request #10046 from edx/cdyer/MA-1416-image-mimetype-uploads

MA-1416 image mimetype uploads

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

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

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

openedx/core/lib/api/parsers.py

 """
-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 `<Parser>.media_type`.
+
+To use these in an APIView, set `<View>.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 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(parsers.JSONParser):
+class MergePatchParser(JSONParser):
     """
     Custom parser to be used with the "merge patch" implementation (https://tools.ietf.org/html/rfc7396).
     """

openedx/core/lib/api/tests/test_parsers.py

+"""
+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)