From 46b63164a083e26b065d86cb45db4e31cb14392e Mon Sep 17 00:00:00 2001
From: jsa <jsa@edx.org>
Date: Mon, 9 Mar 2015 11:29:42 -0400
Subject: [PATCH] Implement profile_image upload and remove endpoints
TNL-1537
Co-Authored-By: Andy Armstrong <andya@edx.org>
Co-Authored-By: cahrens <christina@edx.org>
---
common/djangoapps/enrollment/views.py | 5 +-
lms/djangoapps/commerce/views.py | 2 +-
lms/djangoapps/mobile_api/utils.py | 8 +-
lms/envs/common.py | 11 +-
lms/envs/test.py | 2 +
lms/urls.py | 4 +
.../djangoapps/profile_images/__init__.py | 0
.../core/djangoapps/profile_images/images.py | 132 ++++++++
.../profile_images/tests/__init__.py | 0
.../profile_images/tests/helpers.py | 55 ++++
.../profile_images/tests/test_images.py | 182 +++++++++++
.../profile_images/tests/test_views.py | 304 ++++++++++++++++++
.../core/djangoapps/profile_images/urls.py | 22 ++
.../core/djangoapps/profile_images/views.py | 152 +++++++++
.../djangoapps/user_api/accounts/helpers.py | 57 ----
.../user_api/accounts/image_helpers.py | 115 +++++++
.../user_api/accounts/serializers.py | 9 +-
.../user_api/accounts/tests/test_api.py | 5 +-
.../user_api/accounts/tests/test_helpers.py | 69 ----
.../accounts/tests/test_image_helpers.py | 60 ++++
.../user_api/accounts/tests/test_views.py | 9 +-
.../djangoapps/user_api/accounts/views.py | 17 +-
.../djangoapps/user_api/preferences/api.py | 1 -
.../user_api/preferences/tests/test_views.py | 10 +-
.../djangoapps/user_api/preferences/views.py | 45 ++-
.../djangoapps/user_api/tests/test_views.py | 1 +
openedx/core/djangoapps/user_api/views.py | 11 +-
.../core/lib/api}/authentication.py | 0
openedx/core/lib/api/permissions.py | 21 +-
openedx/core/lib/api/tests/__init__.py | 0
.../lib/api}/tests/test_authentication.py | 0
31 files changed, 1135 insertions(+), 174 deletions(-)
create mode 100644 openedx/core/djangoapps/profile_images/__init__.py
create mode 100644 openedx/core/djangoapps/profile_images/images.py
create mode 100644 openedx/core/djangoapps/profile_images/tests/__init__.py
create mode 100644 openedx/core/djangoapps/profile_images/tests/helpers.py
create mode 100644 openedx/core/djangoapps/profile_images/tests/test_images.py
create mode 100644 openedx/core/djangoapps/profile_images/tests/test_views.py
create mode 100644 openedx/core/djangoapps/profile_images/urls.py
create mode 100644 openedx/core/djangoapps/profile_images/views.py
delete mode 100644 openedx/core/djangoapps/user_api/accounts/helpers.py
create mode 100644 openedx/core/djangoapps/user_api/accounts/image_helpers.py
delete mode 100644 openedx/core/djangoapps/user_api/accounts/tests/test_helpers.py
create mode 100644 openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py
rename {common/djangoapps/util => openedx/core/lib/api}/authentication.py (100%)
create mode 100644 openedx/core/lib/api/tests/__init__.py
rename {common/djangoapps/util => openedx/core/lib/api}/tests/test_authentication.py (100%)
diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py
index 57301a9ec08..6ec535bae12 100644
--- a/common/djangoapps/enrollment/views.py
+++ b/common/djangoapps/enrollment/views.py
@@ -18,7 +18,10 @@ from opaque_keys.edx.keys import CourseKey
from embargo import api as embargo_api
from cors_csrf.authentication import SessionAuthenticationCrossDomainCsrf
from cors_csrf.decorators import ensure_csrf_cookie_cross_domain
-from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser
+from openedx.core.lib.api.authentication import (
+ SessionAuthenticationAllowInactiveUser,
+ OAuth2AuthenticationAllowInactiveUser,
+)
from util.disable_rate_limit import can_disable_rate_limit
from enrollment import api
from enrollment.errors import (
diff --git a/lms/djangoapps/commerce/views.py b/lms/djangoapps/commerce/views.py
index 46f0b925744..44b17efaa10 100644
--- a/lms/djangoapps/commerce/views.py
+++ b/lms/djangoapps/commerce/views.py
@@ -15,7 +15,7 @@ from course_modes.models import CourseMode
from courseware import courses
from enrollment.api import add_enrollment
from student.models import CourseEnrollment
-from util.authentication import SessionAuthenticationAllowInactiveUser
+from openedx.core.lib.api.authentication import SessionAuthenticationAllowInactiveUser
log = logging.getLogger(__name__)
diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py
index 4d4eed8d266..57dc67c73c6 100644
--- a/lms/djangoapps/mobile_api/utils.py
+++ b/lms/djangoapps/mobile_api/utils.py
@@ -2,14 +2,16 @@
Common utility methods and decorators for Mobile APIs.
"""
-
import functools
from rest_framework import permissions
-
-from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.django import modulestore
+
from courseware.courses import get_course_with_access
+from openedx.core.lib.api.authentication import (
+ SessionAuthenticationAllowInactiveUser,
+ OAuth2AuthenticationAllowInactiveUser,
+)
from openedx.core.lib.api.permissions import IsUserInUrl
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 6a271030214..14fe067eaf0 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -2259,7 +2259,13 @@ FIELD_OVERRIDE_PROVIDERS = ()
# PROFILE IMAGE CONFIG
# TODO: add these settings to aws.py as well
-PROFILE_IMAGE_BACKEND = 'django.core.files.storage.FileSystemStorage'
+# WARNING: Certain django storage backends do not support atomic
+# file overwrites (including the default, specified below) - instead
+# there are separate calls to delete and then write a new file in the
+# storage backend. This introduces the risk of a race condition
+# occurring when a user uploads a new profile image to replace an
+# earlier one (the file will temporarily be deleted).
+PROFILE_IMAGE_BACKEND = 'storages.backends.overwrite.OverwriteStorage'
# PROFILE_IMAGE_DOMAIN points to the domain from which we serve image
# files from. When this is '/', it refers to the same domain as the
# app server. If serving from a different domain, specify that here
@@ -2272,4 +2278,5 @@ PROFILE_IMAGE_DEFAULT_FILENAME = 'default_profile_image' # TODO: determine fina
# platform unaware of current image URLs, resulting in reverting all
# users' profile images to the default placeholder image.
PROFILE_IMAGE_SECRET_KEY = 'placeholder secret key'
-
+PROFILE_IMAGE_MAX_BYTES = 1024 * 1024
+PROFILE_IMAGE_MIN_BYTES = 100
diff --git a/lms/envs/test.py b/lms/envs/test.py
index 58e1e7fef41..fdcf919dbe1 100644
--- a/lms/envs/test.py
+++ b/lms/envs/test.py
@@ -483,3 +483,5 @@ PROFILE_IMAGE_DOMAIN = 'http://example-storage.com/'
PROFILE_IMAGE_URL_PATH = 'profile_images/'
PROFILE_IMAGE_DEFAULT_FILENAME = 'default'
PROFILE_IMAGE_SECRET_KEY = 'secret'
+PROFILE_IMAGE_MAX_BYTES = 1024 * 1024
+PROFILE_IMAGE_MIN_BYTES = 100
diff --git a/lms/urls.py b/lms/urls.py
index bb041f9876b..7822ea75132 100644
--- a/lms/urls.py
+++ b/lms/urls.py
@@ -91,6 +91,7 @@ urlpatterns = (
if settings.FEATURES["ENABLE_USER_REST_API"]:
urlpatterns += (
url(r'^api/user/', include('openedx.core.djangoapps.user_api.urls')),
+ url(r'^api/profile_images/', include('openedx.core.djangoapps.profile_images.urls')),
)
if settings.FEATURES["ENABLE_COMBINED_LOGIN_REGISTRATION"]:
@@ -637,6 +638,9 @@ urlpatterns = patterns(*urlpatterns)
if settings.DEBUG:
urlpatterns += static(settings.STATIC_URL, document_root=settings.STATIC_ROOT)
+ urlpatterns += static(
+ settings.PROFILE_IMAGE_DOMAIN + settings.PROFILE_IMAGE_URL_PATH, document_root=settings.MEDIA_ROOT
+ )
# in debug mode, allow any template to be rendered (most useful for UX reference templates)
urlpatterns += url(r'^template/(?P<template>.+)$', 'debug.views.show_reference_template'),
diff --git a/openedx/core/djangoapps/profile_images/__init__.py b/openedx/core/djangoapps/profile_images/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/openedx/core/djangoapps/profile_images/images.py b/openedx/core/djangoapps/profile_images/images.py
new file mode 100644
index 00000000000..ab2e974cd03
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/images.py
@@ -0,0 +1,132 @@
+"""
+Image file manipulation functions related to profile images.
+"""
+from cStringIO import StringIO
+
+from django.conf import settings
+from django.core.files.base import ContentFile
+from django.utils.translation import ugettext as _, ugettext_noop as _noop
+from PIL import Image
+
+from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_storage
+
+
+FILE_UPLOAD_TOO_LARGE = _noop('Maximum file size exceeded.')
+FILE_UPLOAD_TOO_SMALL = _noop('Minimum file size not met.')
+FILE_UPLOAD_BAD_TYPE = _noop('Unsupported file type.')
+FILE_UPLOAD_BAD_EXT = _noop('File extension does not match data.')
+FILE_UPLOAD_BAD_MIMETYPE = _noop('Content-Type header does not match data.')
+
+
+class ImageValidationError(Exception):
+ """
+ Exception to use when the system rejects a user-supplied source image.
+ """
+ @property
+ def user_message(self):
+ """
+ Translate the developer-facing exception message for API clients.
+ """
+ # pylint: disable=translation-of-non-string
+ return _(self.message)
+
+
+def validate_uploaded_image(uploaded_file):
+ """
+ Raises ImageValidationError if the server should refuse to use this
+ uploaded file as the source image for a user's profile image. Otherwise,
+ returns nothing.
+ """
+ # validation code by @pmitros,
+ # adapted from https://github.com/pmitros/ProfileXBlock
+ # see also: http://en.wikipedia.org/wiki/Magic_number_%28programming%29
+ 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"]
+ }
+ }
+
+ if uploaded_file.size > settings.PROFILE_IMAGE_MAX_BYTES:
+ raise ImageValidationError(FILE_UPLOAD_TOO_LARGE)
+ elif uploaded_file.size < settings.PROFILE_IMAGE_MIN_BYTES:
+ raise ImageValidationError(FILE_UPLOAD_TOO_SMALL)
+
+ # 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'])]
+ 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']:
+ raise ImageValidationError(FILE_UPLOAD_BAD_MIMETYPE)
+
+ # check magic number matches expected file type
+ 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
+ uploaded_file.seek(0)
+
+
+def _get_scaled_image_file(image_obj, size):
+ """
+ Given a PIL.Image object, get a resized copy using `size` (square) and
+ return a file-like object containing the data saved as a JPEG.
+
+ Note that the file object returned is a django ContentFile which holds
+ data in memory (not on disk).
+ """
+ if image_obj.mode != "RGB":
+ image_obj = image_obj.convert("RGB")
+ scaled = image_obj.resize((size, size), Image.ANTIALIAS)
+ string_io = StringIO()
+ scaled.save(string_io, format='JPEG')
+ image_file = ContentFile(string_io.getvalue())
+ return image_file
+
+
+def create_profile_images(image_file, profile_image_names):
+ """
+ Generates a set of image files based on image_file and
+ stores them according to the sizes and filenames specified
+ in `profile_image_names`.
+ """
+ image_obj = Image.open(image_file)
+
+ # first center-crop the image if needed (but no scaling yet).
+ width, height = image_obj.size
+ if width != height:
+ side = width if width < height else height
+ image_obj = image_obj.crop(((width - side) / 2, (height - side) / 2, (width + side) / 2, (height + side) / 2))
+
+ storage = get_profile_image_storage()
+ for size, name in profile_image_names.items():
+ scaled_image_file = _get_scaled_image_file(image_obj, size)
+ # Store the file.
+ try:
+ storage.save(name, scaled_image_file)
+ finally:
+ scaled_image_file.close()
+
+
+def remove_profile_images(profile_image_names):
+ """
+ Physically remove the image files specified in `profile_image_names`
+ """
+ storage = get_profile_image_storage()
+ for name in profile_image_names.values():
+ storage.delete(name)
diff --git a/openedx/core/djangoapps/profile_images/tests/__init__.py b/openedx/core/djangoapps/profile_images/tests/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/openedx/core/djangoapps/profile_images/tests/helpers.py b/openedx/core/djangoapps/profile_images/tests/helpers.py
new file mode 100644
index 00000000000..e1bf2cf8342
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/tests/helpers.py
@@ -0,0 +1,55 @@
+"""
+Helper methods for use in profile image tests.
+"""
+from contextlib import contextmanager
+import os
+from tempfile import NamedTemporaryFile
+
+from django.core.files.uploadedfile import UploadedFile
+from PIL import Image
+
+
+@contextmanager
+def make_image_file(dimensions=(320, 240), extension=".jpeg", force_size=None):
+ """
+ Yields a named temporary file created with the specified image type and
+ options.
+
+ Note the default dimensions are unequal (not a square) ensuring that center-square
+ cropping logic will be exercised during tests.
+
+ The temporary file will be closed and deleted automatically upon exiting
+ the `with` block.
+ """
+ image = Image.new('RGB', dimensions, "green")
+ image_file = NamedTemporaryFile(suffix=extension)
+ try:
+ image.save(image_file)
+ if force_size is not None:
+ image_file.seek(0, os.SEEK_END)
+ bytes_to_pad = force_size - image_file.tell()
+ # write in hunks of 256 bytes
+ hunk, byte_ = bytearray([0] * 256), bytearray([0])
+ num_hunks, remainder = divmod(bytes_to_pad, 256)
+ for _ in xrange(num_hunks):
+ image_file.write(hunk)
+ for _ in xrange(remainder):
+ image_file.write(byte_)
+ image_file.flush()
+ image_file.seek(0)
+ yield image_file
+ finally:
+ image_file.close()
+
+
+@contextmanager
+def make_uploaded_file(content_type, *a, **kw):
+ """
+ Wrap the result of make_image_file in a django UploadedFile.
+ """
+ with make_image_file(*a, **kw) as image_file:
+ yield UploadedFile(
+ image_file,
+ content_type=content_type,
+ size=os.path.getsize(image_file.name),
+ )
diff --git a/openedx/core/djangoapps/profile_images/tests/test_images.py b/openedx/core/djangoapps/profile_images/tests/test_images.py
new file mode 100644
index 00000000000..3e65bd06d05
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/tests/test_images.py
@@ -0,0 +1,182 @@
+"""
+Test cases for image processing functions in the profile image package.
+"""
+from contextlib import closing
+from itertools import product
+import os
+from tempfile import NamedTemporaryFile
+import unittest
+
+from django.conf import settings
+from django.core.files.uploadedfile import UploadedFile
+from django.test import TestCase
+from django.test.utils import override_settings
+import ddt
+import mock
+from PIL import Image
+
+from ..images import (
+ FILE_UPLOAD_TOO_LARGE,
+ FILE_UPLOAD_TOO_SMALL,
+ FILE_UPLOAD_BAD_TYPE,
+ FILE_UPLOAD_BAD_EXT,
+ FILE_UPLOAD_BAD_MIMETYPE,
+ create_profile_images,
+ ImageValidationError,
+ remove_profile_images,
+ validate_uploaded_image,
+)
+from .helpers import make_image_file, make_uploaded_file
+
+
+@ddt.ddt
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+class TestValidateUploadedImage(TestCase):
+ """
+ Test validate_uploaded_image
+ """
+ def check_validation_result(self, uploaded_file, expected_failure_message):
+ """
+ Internal DRY helper.
+ """
+ if expected_failure_message is not None:
+ with self.assertRaises(ImageValidationError) as ctx:
+ validate_uploaded_image(uploaded_file)
+ self.assertEqual(ctx.exception.message, expected_failure_message)
+ else:
+ validate_uploaded_image(uploaded_file)
+ self.assertEqual(uploaded_file.tell(), 0)
+
+ @ddt.data(
+ (99, FILE_UPLOAD_TOO_SMALL),
+ (100, ),
+ (1024, ),
+ (1025, FILE_UPLOAD_TOO_LARGE),
+ )
+ @ddt.unpack
+ @override_settings(PROFILE_IMAGE_MIN_BYTES=100, PROFILE_IMAGE_MAX_BYTES=1024)
+ def test_file_size(self, upload_size, expected_failure_message=None):
+ """
+ Ensure that files outside the accepted size range fail validation.
+ """
+ with make_uploaded_file(
+ dimensions=(1, 1), extension=".png", content_type="image/png", force_size=upload_size
+ ) as uploaded_file:
+ self.check_validation_result(uploaded_file, expected_failure_message)
+
+ @ddt.data(
+ (".gif", "image/gif"),
+ (".jpg", "image/jpeg"),
+ (".jpeg", "image/jpeg"),
+ (".png", "image/png"),
+ (".bmp", "image/bmp", FILE_UPLOAD_BAD_TYPE),
+ (".tif", "image/tiff", FILE_UPLOAD_BAD_TYPE),
+ )
+ @ddt.unpack
+ def test_extension(self, extension, content_type, expected_failure_message=None):
+ """
+ Ensure that files whose extension is not supported fail validation.
+ """
+ with make_uploaded_file(extension=extension, content_type=content_type) as uploaded_file:
+ self.check_validation_result(uploaded_file, expected_failure_message)
+
+ def test_extension_mismatch(self):
+ """
+ Ensure that validation fails when the file extension does not match the
+ file data.
+ """
+ # make a bmp, try to fool the function into thinking it's a jpeg
+ with make_image_file(extension=".bmp") as bmp_file:
+ with closing(NamedTemporaryFile(suffix=".jpeg")) as fake_jpeg_file:
+ fake_jpeg_file.write(bmp_file.read())
+ fake_jpeg_file.seek(0)
+ uploaded_file = UploadedFile(
+ fake_jpeg_file,
+ content_type="image/jpeg",
+ size=os.path.getsize(fake_jpeg_file.name)
+ )
+ with self.assertRaises(ImageValidationError) as ctx:
+ validate_uploaded_image(uploaded_file)
+ self.assertEqual(ctx.exception.message, FILE_UPLOAD_BAD_EXT)
+
+ def test_content_type(self):
+ """
+ Ensure that validation fails when the content_type header and file
+ extension do not match
+ """
+ with make_uploaded_file(extension=".jpeg", content_type="image/gif") as uploaded_file:
+ with self.assertRaises(ImageValidationError) as ctx:
+ validate_uploaded_image(uploaded_file)
+ self.assertEqual(ctx.exception.message, FILE_UPLOAD_BAD_MIMETYPE)
+
+
+@ddt.ddt
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+class TestGenerateProfileImages(TestCase):
+ """
+ Test create_profile_images
+ """
+ @ddt.data(
+ *product(
+ ["gif", "jpg", "png"],
+ [(1, 1), (10, 10), (100, 100), (1000, 1000), (1, 10), (10, 100), (100, 1000), (1000, 999)],
+ )
+ )
+ @ddt.unpack
+ def test_generation(self, image_type, dimensions):
+ """
+ Ensure that regardless of the input format or dimensions, the outcome
+ of calling the function is square jpeg files with explicitly-requested
+ dimensions being saved to the profile image storage backend.
+ """
+ extension = "." + image_type
+ content_type = "image/" + image_type
+ requested_sizes = {
+ 10: "ten.jpg",
+ 100: "hundred.jpg",
+ 1000: "thousand.jpg",
+ }
+ mock_storage = mock.Mock()
+ with make_uploaded_file(dimensions=dimensions, extension=extension, content_type=content_type) as uploaded_file:
+ with mock.patch(
+ "openedx.core.djangoapps.profile_images.images.get_profile_image_storage",
+ return_value=mock_storage,
+ ):
+ create_profile_images(uploaded_file, requested_sizes)
+ names_and_files = [v[0] for v in mock_storage.save.call_args_list]
+ actual_sizes = {}
+ for name, file_ in names_and_files:
+ # get the size of the image file and ensure it's square jpeg
+ with closing(Image.open(file_)) as image_obj:
+ width, height = image_obj.size
+ self.assertEqual(width, height)
+ self.assertEqual(image_obj.format, 'JPEG')
+ actual_sizes[width] = name
+ self.assertEqual(requested_sizes, actual_sizes)
+ mock_storage.save.reset_mock()
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+class TestRemoveProfileImages(TestCase):
+ """
+ Test remove_profile_images
+ """
+ def test_remove(self):
+ """
+ Ensure that the outcome of calling the function is that the named images
+ are deleted from the profile image storage backend.
+ """
+ requested_sizes = {
+ 10: "ten.jpg",
+ 100: "hundred.jpg",
+ 1000: "thousand.jpg",
+ }
+ mock_storage = mock.Mock()
+ with mock.patch(
+ "openedx.core.djangoapps.profile_images.images.get_profile_image_storage",
+ return_value=mock_storage,
+ ):
+ remove_profile_images(requested_sizes)
+ deleted_names = [v[0][0] for v in mock_storage.delete.call_args_list]
+ self.assertEqual(requested_sizes.values(), deleted_names)
+ mock_storage.save.reset_mock()
diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py
new file mode 100644
index 00000000000..80c349d9c7d
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/tests/test_views.py
@@ -0,0 +1,304 @@
+"""
+Test cases for the HTTP endpoints of the profile image api.
+"""
+from contextlib import closing
+import unittest
+
+import ddt
+from django.conf import settings
+from django.core.urlresolvers import reverse
+import mock
+from mock import patch
+
+from PIL import Image
+from rest_framework.test import APITestCase, APIClient
+
+from student.tests.factories import UserFactory
+
+from ...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 .helpers import make_image_file
+
+TEST_PASSWORD = "test"
+
+
+class ProfileImageEndpointTestCase(APITestCase):
+ """
+ Base class / shared infrastructure for tests of profile_image "upload" and
+ "remove" endpoints.
+ """
+ # subclasses should override this with the name of the view under test, as
+ # per the urls.py configuration.
+ _view_name = None
+
+ def setUp(self):
+ super(ProfileImageEndpointTestCase, self).setUp()
+ self.user = UserFactory.create(password=TEST_PASSWORD)
+ # Ensure that parental controls don't apply to this user
+ self.user.profile.year_of_birth = 1980
+ self.user.profile.save()
+ self.url = reverse(self._view_name, kwargs={'username': self.user.username})
+ self.client.login(username=self.user.username, password=TEST_PASSWORD)
+ self.storage = get_profile_image_storage()
+ # this assertion is made here as a sanity check because all tests
+ # assume user.profile.has_profile_image is False by default
+ self.assertFalse(self.user.profile.has_profile_image)
+
+ def tearDown(self):
+ super(ProfileImageEndpointTestCase, self).tearDown()
+ for name in get_profile_image_names(self.user.username).values():
+ self.storage.delete(name)
+
+ def check_images(self, exist=True):
+ """
+ If exist is True, make sure the images physically exist in storage
+ with correct sizes and formats.
+
+ If exist is False, make sure none of the images exist.
+ """
+ for size, name in get_profile_image_names(self.user.username).items():
+ if exist:
+ self.assertTrue(self.storage.exists(name))
+ with closing(Image.open(self.storage.path(name))) as img:
+ self.assertEqual(img.size, (size, size))
+ self.assertEqual(img.format, 'JPEG')
+ else:
+ self.assertFalse(self.storage.exists(name))
+
+ def check_response(self, response, expected_code, expected_developer_message=None, expected_user_message=None):
+ """
+ Make sure the response has the expected code, and if that isn't 204,
+ optionally check the correctness of a developer-facing message.
+ """
+ self.assertEqual(expected_code, response.status_code)
+ if expected_code == 204:
+ self.assertIsNone(response.data)
+ else:
+ if expected_developer_message is not None:
+ self.assertEqual(response.data.get('developer_message'), expected_developer_message)
+ if expected_user_message is not None:
+ self.assertEqual(response.data.get('user_message'), expected_user_message)
+
+ def check_has_profile_image(self, has_profile_image=True):
+ """
+ Make sure the value of self.user.profile.has_profile_image is what we
+ expect.
+ """
+ # it's necessary to reload this model from the database since save()
+ # would have been called on another instance.
+ profile = self.user.profile.__class__.objects.get(user=self.user)
+ self.assertEqual(profile.has_profile_image, has_profile_image)
+
+
+@ddt.ddt
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
+ """
+ Tests for the profile_image upload endpoint.
+ """
+ _view_name = "profile_image_upload"
+
+ def test_unsupported_methods(self):
+ """
+ Test that GET, PUT, PATCH, and DELETE are not supported.
+ """
+ self.assertEqual(405, self.client.get(self.url).status_code)
+ self.assertEqual(405, self.client.put(self.url).status_code)
+ self.assertEqual(405, self.client.patch(self.url).status_code)
+ self.assertEqual(405, self.client.delete(self.url).status_code)
+
+ def test_anonymous_access(self):
+ """
+ Test that an anonymous client (not logged in) cannot POST.
+ """
+ anonymous_client = APIClient()
+ response = anonymous_client.post(self.url)
+ self.assertEqual(401, response.status_code)
+
+ def test_upload_self(self):
+ """
+ Test that an authenticated user can POST to their own upload endpoint.
+ """
+ with make_image_file() as image_file:
+ response = self.client.post(self.url, {'file': image_file}, format='multipart')
+ self.check_response(response, 204)
+ self.check_images()
+ self.check_has_profile_image()
+
+ def test_upload_other(self):
+ """
+ Test that an authenticated user cannot POST to another user's upload endpoint.
+ """
+ different_user = UserFactory.create(password=TEST_PASSWORD)
+ different_client = APIClient()
+ different_client.login(username=different_user.username, password=TEST_PASSWORD)
+ with make_image_file() as image_file:
+ response = different_client.post(self.url, {'file': image_file}, format='multipart')
+ self.check_response(response, 404)
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ def test_upload_staff(self):
+ """
+ Test that an authenticated staff cannot POST to another user's upload endpoint.
+ """
+ staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD)
+ staff_client = APIClient()
+ staff_client.login(username=staff_user.username, password=TEST_PASSWORD)
+ with make_image_file() as image_file:
+ response = staff_client.post(self.url, {'file': image_file}, format='multipart')
+ self.check_response(response, 403)
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ def test_upload_missing_file(self):
+ """
+ Test that omitting the file entirely from the POST results in HTTP 400.
+ """
+ response = self.client.post(self.url, {}, format='multipart')
+ self.check_response(
+ response, 400,
+ expected_developer_message=u"No file provided for profile image",
+ expected_user_message=u"No file provided for profile image",
+ )
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ def test_upload_not_a_file(self):
+ """
+ Test that sending unexpected data that isn't a file results in HTTP
+ 400.
+ """
+ response = self.client.post(self.url, {'file': 'not a file'}, format='multipart')
+ self.check_response(
+ response, 400,
+ expected_developer_message=u"No file provided for profile image",
+ expected_user_message=u"No file provided for profile image",
+ )
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ def test_upload_validation(self):
+ """
+ Test that when upload validation fails, the proper HTTP response and
+ messages are returned.
+ """
+ with make_image_file() as image_file:
+ with mock.patch(
+ 'openedx.core.djangoapps.profile_images.views.validate_uploaded_image',
+ side_effect=ImageValidationError(u"test error message")
+ ):
+ response = self.client.post(self.url, {'file': image_file}, format='multipart')
+ self.check_response(
+ response, 400,
+ expected_developer_message=u"test error message",
+ expected_user_message=u"test error message",
+ )
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ @patch('PIL.Image.open')
+ def test_upload_failure(self, image_open):
+ """
+ Test that when upload validation fails, the proper HTTP response and
+ messages are returned.
+ """
+ image_open.side_effect = [Exception(u"whoops"), None]
+ with make_image_file() as image_file:
+ response = self.client.post(self.url, {'file': image_file}, format='multipart')
+ self.check_response(
+ response, 400,
+ expected_developer_message=u"Upload failed for profile image: whoops",
+ expected_user_message=u"Upload failed for profile image",
+ )
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
+ """
+ Tests for the profile_image remove endpoint.
+ """
+ _view_name = "profile_image_remove"
+
+ def setUp(self):
+ super(ProfileImageRemoveTestCase, self).setUp()
+ with make_image_file() as image_file:
+ create_profile_images(image_file, get_profile_image_names(self.user.username))
+ self.check_images()
+ set_has_profile_image(self.user.username, True)
+
+ def test_unsupported_methods(self):
+ """
+ Test that GET, PUT, PATCH, and DELETE are not supported.
+ """
+ self.assertEqual(405, self.client.get(self.url).status_code)
+ self.assertEqual(405, self.client.put(self.url).status_code)
+ self.assertEqual(405, self.client.patch(self.url).status_code)
+ self.assertEqual(405, self.client.delete(self.url).status_code)
+
+ def test_anonymous_access(self):
+ """
+ Test that an anonymous client (not logged in) cannot call GET or POST.
+ """
+ anonymous_client = APIClient()
+ for request in (anonymous_client.get, anonymous_client.post):
+ response = request(self.url)
+ self.assertEqual(401, response.status_code)
+
+ def test_remove_self(self):
+ """
+ Test that an authenticated user can POST to remove their own profile
+ images.
+ """
+ response = self.client.post(self.url)
+ self.check_response(response, 204)
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ def test_remove_other(self):
+ """
+ Test that an authenticated user cannot POST to remove another user's
+ profile images.
+ """
+ different_user = UserFactory.create(password=TEST_PASSWORD)
+ different_client = APIClient()
+ different_client.login(username=different_user.username, password=TEST_PASSWORD)
+ response = different_client.post(self.url)
+ self.check_response(response, 404)
+ self.check_images(True) # thumbnails should remain intact.
+ self.check_has_profile_image(True)
+
+ def test_remove_staff(self):
+ """
+ Test that an authenticated staff user can POST to remove another user's
+ profile images.
+ """
+ staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD)
+ staff_client = APIClient()
+ staff_client.login(username=staff_user.username, password=TEST_PASSWORD)
+ response = self.client.post(self.url)
+ self.check_response(response, 204)
+ self.check_images(False)
+ self.check_has_profile_image(False)
+
+ @patch('student.models.UserProfile.save')
+ def test_remove_failure(self, user_profile_save):
+ """
+ Test that when upload validation fails, the proper HTTP response and
+ messages are returned.
+ """
+ user_profile_save.side_effect = [Exception(u"whoops"), None]
+ response = self.client.post(self.url)
+ self.check_response(
+ response, 400,
+ expected_developer_message=u"Delete failed for profile image: whoops",
+ expected_user_message=u"Delete failed for profile image",
+ )
+ self.check_images(True) # thumbnails should remain intact.
+ self.check_has_profile_image(True)
diff --git a/openedx/core/djangoapps/profile_images/urls.py b/openedx/core/djangoapps/profile_images/urls.py
new file mode 100644
index 00000000000..a2e73c73ac8
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/urls.py
@@ -0,0 +1,22 @@
+"""
+Defines the URL routes for this app.
+"""
+from .views import ProfileImageUploadView, ProfileImageRemoveView
+
+from django.conf.urls import patterns, url
+
+USERNAME_PATTERN = r'(?P<username>[\w.+-]+)'
+
+urlpatterns = patterns(
+ '',
+ url(
+ r'^v0/' + USERNAME_PATTERN + '/upload$',
+ ProfileImageUploadView.as_view(),
+ name="profile_image_upload"
+ ),
+ url(
+ r'^v0/' + USERNAME_PATTERN + '/remove$',
+ ProfileImageRemoveView.as_view(),
+ name="profile_image_remove"
+ ),
+)
diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py
new file mode 100644
index 00000000000..f2aaf748f79
--- /dev/null
+++ b/openedx/core/djangoapps/profile_images/views.py
@@ -0,0 +1,152 @@
+"""
+This module implements the upload and remove endpoints of the profile image api.
+"""
+from contextlib import closing
+
+from django.utils.translation import ugettext as _
+from rest_framework import permissions, status
+from rest_framework.parsers import MultiPartParser, FormParser
+from rest_framework.response import Response
+from rest_framework.views import APIView
+
+from openedx.core.djangoapps.user_api.errors import UserNotFound
+from openedx.core.lib.api.authentication import (
+ OAuth2AuthenticationAllowInactiveUser,
+ SessionAuthenticationAllowInactiveUser,
+)
+from openedx.core.lib.api.permissions import IsUserInUrl, IsUserInUrlOrStaff
+from openedx.core.djangoapps.user_api.accounts.image_helpers import set_has_profile_image, get_profile_image_names
+from .images import validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError
+
+
+class ProfileImageUploadView(APIView):
+ """
+ **Use Cases**
+
+ Uploads an image to be used for the user's profile.
+
+ **Example Requests**:
+
+ POST /api/profile_images/v0/{username}/upload
+
+ **Response for POST**
+
+ Users can only upload their own profile image. If the requesting user does not have username
+ "username", this method will return with a status of 403 for staff access but a 404 for ordinary
+ users to avoid leaking the existence of the account.
+
+ This method will also return a 404 if no user exists with username "username".
+
+ If the upload could not be performed then this method returns a 400 with specific errors
+ in the returned JSON.
+
+ If the update is successful, a 204 status is returned with no additional content.
+
+ """
+ parser_classes = (MultiPartParser, FormParser,)
+
+ authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
+ permission_classes = (permissions.IsAuthenticated, IsUserInUrl)
+
+ def post(self, request, username):
+ """
+ POST /api/profile_images/v0/{username}/upload
+ """
+ # validate request:
+ # verify that the user's
+ # ensure any file was sent
+ if 'file' not in request.FILES:
+ return Response(
+ {
+ "developer_message": u"No file provided for profile image",
+ "user_message": _(u"No file provided for profile image"),
+
+ },
+ status=status.HTTP_400_BAD_REQUEST
+ )
+
+ try:
+ # process the upload.
+ uploaded_file = request.FILES['file']
+
+ # no matter what happens, delete the temporary file when we're done
+ with closing(uploaded_file):
+
+ # image file validation.
+ try:
+ validate_uploaded_image(uploaded_file)
+ except ImageValidationError as error:
+ return Response(
+ {"developer_message": error.message, "user_message": error.user_message},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ # generate profile pic and thumbnails and store them
+ create_profile_images(uploaded_file, get_profile_image_names(username))
+
+ # update the user account to reflect that a profile image is available.
+ set_has_profile_image(username, True)
+ except Exception as error:
+ return Response(
+ {
+ "developer_message": u"Upload failed for profile image: {error}".format(error=error),
+ "user_message": _(u"Upload failed for profile image"),
+
+ },
+ status=status.HTTP_400_BAD_REQUEST
+ )
+
+ # send client response.
+ return Response(status=status.HTTP_204_NO_CONTENT)
+
+
+class ProfileImageRemoveView(APIView):
+ """
+ **Use Cases**
+
+ Removes all of the profile images associated with the user's account.
+
+ **Example Requests**:
+
+ POST /api/profile_images/v0/{username}/remove
+
+ **Response for POST**
+
+ Users are authorized to delete their own profile images, while staff can delete images for
+ any account. All other users will receive a 404 to avoid leaking the existence of the account.
+
+ This method will also return a 404 if no user exists with username "username".
+
+ If the delete could not be performed then this method returns a 400 with specific errors
+ in the returned JSON.
+
+ If the delete is successful, a 204 status is returned with no additional content.
+
+ """
+ authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
+ permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff)
+
+ def post(self, request, username): # pylint: disable=unused-argument
+ """
+ POST /api/profile_images/v0/{username}/remove
+ """
+ try:
+ # update the user account to reflect that the images were removed.
+ set_has_profile_image(username, False)
+
+ # remove physical files from storage.
+ remove_profile_images(get_profile_image_names(username))
+ except UserNotFound:
+ return Response(status=status.HTTP_404_NOT_FOUND)
+ except Exception as error:
+ return Response(
+ {
+ "developer_message": u"Delete failed for profile image: {error}".format(error=error),
+ "user_message": _(u"Delete failed for profile image"),
+
+ },
+ status=status.HTTP_400_BAD_REQUEST
+ )
+
+ # send client response.
+ return Response(status=status.HTTP_204_NO_CONTENT)
diff --git a/openedx/core/djangoapps/user_api/accounts/helpers.py b/openedx/core/djangoapps/user_api/accounts/helpers.py
deleted file mode 100644
index 836ace7d942..00000000000
--- a/openedx/core/djangoapps/user_api/accounts/helpers.py
+++ /dev/null
@@ -1,57 +0,0 @@
-"""
-Helper functions for the accounts API.
-"""
-import hashlib
-
-from django.conf import settings
-from django.core.files.storage import FileSystemStorage, get_storage_class
-
-PROFILE_IMAGE_SIZES_MAP = {
- 'full': 500,
- 'large': 120,
- 'medium': 50,
- 'small': 30
-}
-_PROFILE_IMAGE_SIZES = PROFILE_IMAGE_SIZES_MAP.values()
-PROFILE_IMAGE_FORMAT = 'jpg'
-
-
-def get_profile_image_url_for_user(user, size):
- """Return the URL to a user's profile image for a given size.
- Note that based on the value of
- django.conf.settings.PROFILE_IMAGE_DOMAIN, the URL may be relative,
- and in that case the caller is responsible for constructing the full
- URL.
-
- If the user has not yet uploaded a profile image, return the URL to
- the default edX user profile image.
-
- Arguments:
- user (django.auth.User): The user for whom we're generating a
- profile image URL.
-
- Returns:
- string: The URL for the user's profile image.
-
- Raises:
- ValueError: The caller asked for an unsupported image size.
- """
- if size not in _PROFILE_IMAGE_SIZES:
- raise ValueError('Unsupported profile image size: {size}'.format(size=size))
-
- if user.profile.has_profile_image:
- name = hashlib.md5(settings.PROFILE_IMAGE_SECRET_KEY + user.username).hexdigest()
- else:
- name = settings.PROFILE_IMAGE_DEFAULT_FILENAME
-
- filename = '{name}_{size}.{format}'.format(name=name, size=size, format=PROFILE_IMAGE_FORMAT)
-
- # Note that, for now, the backend will be FileSystemStorage. When
- # we eventually support s3 storage, we'll need to pass a parameter
- # to the storage class indicating the s3 bucket which we're using
- # for profile picture uploads.
- storage_class = get_storage_class(settings.PROFILE_IMAGE_BACKEND)
- if storage_class == FileSystemStorage:
- kwargs = {'base_url': (settings.PROFILE_IMAGE_DOMAIN + settings.PROFILE_IMAGE_URL_PATH)}
- storage = storage_class(**kwargs)
- return storage.url(filename)
diff --git a/openedx/core/djangoapps/user_api/accounts/image_helpers.py b/openedx/core/djangoapps/user_api/accounts/image_helpers.py
new file mode 100644
index 00000000000..64514db4c3b
--- /dev/null
+++ b/openedx/core/djangoapps/user_api/accounts/image_helpers.py
@@ -0,0 +1,115 @@
+"""
+Helper functions for the accounts API.
+"""
+import hashlib
+
+from django.conf import settings
+from django.core.exceptions import ObjectDoesNotExist
+from django.core.files.storage import get_storage_class
+
+from student.models import UserProfile
+from ..errors import UserNotFound
+
+
+PROFILE_IMAGE_SIZES_MAP = {
+ 'full': 500,
+ 'large': 120,
+ 'medium': 50,
+ 'small': 30
+}
+_PROFILE_IMAGE_SIZES = PROFILE_IMAGE_SIZES_MAP.values()
+
+
+def get_profile_image_storage():
+ """
+ Configures and returns a django Storage instance that can be used
+ to physically locate, read and write profile images.
+ """
+ storage_class = get_storage_class(settings.PROFILE_IMAGE_BACKEND)
+ return storage_class(base_url=(settings.PROFILE_IMAGE_DOMAIN + settings.PROFILE_IMAGE_URL_PATH))
+
+
+def _make_profile_image_name(username):
+ """
+ Returns the user-specific part of the image filename, based on a hash of
+ the username.
+ """
+ return hashlib.md5(settings.PROFILE_IMAGE_SECRET_KEY + username).hexdigest()
+
+
+def _get_profile_image_filename(name, size):
+ """
+ Returns the full filename for a profile image, given the name and size.
+ """
+ return '{name}_{size}.jpg'.format(name=name, size=size)
+
+
+def _get_profile_image_urls(name):
+ """
+ Returns a dict containing the urls for a complete set of profile images,
+ keyed by "friendly" name (e.g. "full", "large", "medium", "small").
+ """
+ storage = get_profile_image_storage()
+ return {
+ size_display_name: storage.url(_get_profile_image_filename(name, size))
+ for size_display_name, size in PROFILE_IMAGE_SIZES_MAP.items()
+ }
+
+
+def get_profile_image_names(username):
+ """
+ Returns a dict containing the filenames for a complete set of profile
+ images, keyed by pixel size.
+ """
+ name = _make_profile_image_name(username)
+ return {size: _get_profile_image_filename(name, size) for size in _PROFILE_IMAGE_SIZES}
+
+
+def get_profile_image_urls_for_user(user):
+ """
+ Return a dict {size:url} for each profile image for a given user.
+ Notes:
+ - this function does not determine whether the set of profile images
+ exists, only what the URLs will be if they do exist. It is assumed that
+ callers will use `_get_default_profile_image_urls` instead to provide
+ a set of urls that point to placeholder images, when there are no user-
+ submitted images.
+ - based on the value of django.conf.settings.PROFILE_IMAGE_DOMAIN,
+ the URL may be relative, and in that case the caller is responsible for
+ constructing the full URL if needed.
+
+ Arguments:
+ user (django.contrib.auth.User): the user for whom we are getting urls.
+
+ Returns:
+ dictionary of {size_display_name: url} for each image.
+
+ """
+ if user.profile.has_profile_image:
+ return _get_profile_image_urls(_make_profile_image_name(user.username))
+ else:
+ return _get_default_profile_image_urls()
+
+
+def _get_default_profile_image_urls():
+ """
+ Returns a dict {size:url} for a complete set of default profile images,
+ used as a placeholder when there are no user-submitted images.
+
+ TODO The result of this function should be memoized, but not in tests.
+ """
+ return _get_profile_image_urls(settings.PROFILE_IMAGE_DEFAULT_FILENAME)
+
+
+def set_has_profile_image(username, has_profile_image=True):
+ """
+ System (not user-facing) API call used to store whether the user has
+ uploaded a profile image. Used by profile_image API.
+ """
+ try:
+ profile = UserProfile.objects.get(user__username=username)
+ except ObjectDoesNotExist:
+ raise UserNotFound()
+
+ profile.has_profile_image = has_profile_image
+ profile.save()
diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py
index d8a187e3ee3..ceacd3e5e60 100644
--- a/openedx/core/djangoapps/user_api/accounts/serializers.py
+++ b/openedx/core/djangoapps/user_api/accounts/serializers.py
@@ -4,7 +4,8 @@ from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
from openedx.core.djangoapps.user_api.serializers import ReadOnlyFieldsSerializerMixin
from student.models import UserProfile, LanguageProficiency
-from .helpers import get_profile_image_url_for_user, PROFILE_IMAGE_SIZES_MAP
+from .image_helpers import get_profile_image_urls_for_user
+
PROFILE_IMAGE_KEY_PREFIX = 'image_url'
@@ -100,10 +101,10 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer, Rea
def get_profile_image(self, user_profile):
""" Returns metadata about a user's profile image. """
data = {'has_image': user_profile.has_profile_image}
+ urls = get_profile_image_urls_for_user(user_profile.user)
data.update({
- '{image_key_prefix}_{size}'.format(image_key_prefix=PROFILE_IMAGE_KEY_PREFIX, size=size_display_name):
- get_profile_image_url_for_user(user_profile.user, size_value)
- for size_display_name, size_value in PROFILE_IMAGE_SIZES_MAP.items()
+ '{image_key_prefix}_{size}'.format(image_key_prefix=PROFILE_IMAGE_KEY_PREFIX, size=size_display_name): url
+ for size_display_name, url in urls.items()
})
return data
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py
index 29ba9c03f72..729553706c9 100644
--- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py
+++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py
@@ -192,12 +192,13 @@ class TestAccountApi(TestCase):
self.assertEqual(0, len(pending_change))
-@patch('openedx.core.djangoapps.user_api.accounts.helpers._PROFILE_IMAGE_SIZES', [50, 10])
+@patch('openedx.core.djangoapps.user_api.accounts.image_helpers._PROFILE_IMAGE_SIZES', [50, 10])
@patch.dict(
- 'openedx.core.djangoapps.user_api.accounts.helpers.PROFILE_IMAGE_SIZES_MAP', {'full': 50, 'small': 10}, clear=True
+ 'openedx.core.djangoapps.user_api.accounts.image_helpers.PROFILE_IMAGE_SIZES_MAP', {'full': 50, 'small': 10}, clear=True
)
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
class AccountSettingsOnCreationTest(TestCase):
+ # pylint: disable=missing-docstring
USERNAME = u'frank-underwood'
PASSWORD = u'ṕáśśẃőŕd'
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_helpers.py b/openedx/core/djangoapps/user_api/accounts/tests/test_helpers.py
deleted file mode 100644
index 92abeaff01e..00000000000
--- a/openedx/core/djangoapps/user_api/accounts/tests/test_helpers.py
+++ /dev/null
@@ -1,69 +0,0 @@
-"""
-Tests for helpers.py
-"""
-from ddt import ddt, data
-import hashlib
-from mock import patch
-from unittest import skipUnless
-
-from django.conf import settings
-from django.test import TestCase
-
-from student.tests.factories import UserFactory
-
-from ...models import UserProfile
-from ..helpers import get_profile_image_url_for_user
-
-
-@ddt
-@patch('openedx.core.djangoapps.user_api.accounts.helpers._PROFILE_IMAGE_SIZES', [50, 10])
-@patch.dict(
- 'openedx.core.djangoapps.user_api.accounts.helpers.PROFILE_IMAGE_SIZES_MAP', {'full': 50, 'small': 10}, clear=True
-)
-@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class ProfileImageUrlTestCase(TestCase):
- """
- Tests for `get_profile_image_url_for_user`.
- """
- def setUp(self):
- super(ProfileImageUrlTestCase, self).setUp()
- self.user = UserFactory()
-
- # Ensure that parental controls don't apply to this user
- self.user.profile.year_of_birth = 1980
- self.user.profile.save()
-
- def verify_url(self, user, pixels, filename):
- """
- Helper method to verify that we're correctly generating profile
- image URLs.
- """
- self.assertEqual(
- get_profile_image_url_for_user(user, pixels),
- 'http://example-storage.com/profile_images/{filename}_{pixels}.jpg'.format(filename=filename, pixels=pixels)
- )
-
- @data(10, 50)
- def test_profile_image_urls(self, pixels):
- """
- Verify we get the URL to the default image if the user does not
- have a profile image.
- """
- # By default new users will have no profile image.
- self.verify_url(self.user, pixels, 'default')
- # A user can add an image, then remove it. We should get the
- # default image URL in that case.
- self.user.profile.has_profile_image = True
- self.user.profile.save()
- self.verify_url(self.user, pixels, hashlib.md5('secret' + self.user.username).hexdigest())
- self.user.profile.has_profile_image = False
- self.user.profile.save()
- self.verify_url(self.user, pixels, 'default')
-
- @data(1, 5000)
- def test_unsupported_sizes(self, image_size):
- """
- Verify that we cannot ask for image sizes which are unsupported.
- """
- with self.assertRaises(ValueError):
- get_profile_image_url_for_user(self.user, image_size)
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py b/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py
new file mode 100644
index 00000000000..8fda6808a48
--- /dev/null
+++ b/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py
@@ -0,0 +1,60 @@
+"""
+Tests for helpers.py
+"""
+import hashlib
+from mock import patch
+from unittest import skipUnless
+
+from django.conf import settings
+from django.test import TestCase
+
+from ..image_helpers import get_profile_image_urls_for_user
+from student.tests.factories import UserFactory
+
+TEST_SIZES = {'full': 50, 'small': 10}
+
+
+@patch.dict('openedx.core.djangoapps.user_api.accounts.image_helpers.PROFILE_IMAGE_SIZES_MAP', TEST_SIZES, clear=True)
+@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class ProfileImageUrlTestCase(TestCase):
+ """
+ Tests for profile image URL generation helpers.
+ """
+ def setUp(self):
+ super(ProfileImageUrlTestCase, self).setUp()
+ self.user = UserFactory()
+ # Ensure that parental controls don't apply to this user
+ self.user.profile.year_of_birth = 1980
+ self.user.profile.has_profile_image = False
+ self.user.profile.save()
+
+ def verify_url(self, actual_url, expected_name, expected_pixels):
+ """
+ Verify correct url structure.
+ """
+ self.assertEqual(
+ actual_url,
+ 'http://example-storage.com/profile_images/{0}_{1}.jpg'.format(expected_name, expected_pixels),
+ )
+
+ def verify_urls(self, expected_name, actual_urls):
+ """
+ Verify correct url dictionary structure.
+ """
+ self.assertEqual(set(TEST_SIZES.keys()), set(actual_urls.keys()))
+ for size_display_name, url in actual_urls.items():
+ self.verify_url(url, expected_name, TEST_SIZES[size_display_name])
+
+ def test_get_profile_image_urls(self):
+ """
+ Tests `get_profile_image_urls_for_user`
+ """
+ self.user.profile.has_profile_image = True
+ self.user.profile.save()
+ expected_name = hashlib.md5('secret' + self.user.username).hexdigest()
+ actual_urls = get_profile_image_urls_for_user(self.user)
+ self.verify_urls(expected_name, actual_urls)
+
+ self.user.profile.has_profile_image = False
+ self.user.profile.save()
+ self.verify_urls('default', get_profile_image_urls_for_user(self.user))
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
index 6486e95df19..76ead6b47a9 100644
--- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
@@ -97,9 +97,9 @@ class UserAPITestCase(APITestCase):
@ddt.ddt
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
-@patch('openedx.core.djangoapps.user_api.accounts.helpers._PROFILE_IMAGE_SIZES', [50, 10])
+@patch('openedx.core.djangoapps.user_api.accounts.image_helpers._PROFILE_IMAGE_SIZES', [50, 10])
@patch.dict(
- 'openedx.core.djangoapps.user_api.accounts.helpers.PROFILE_IMAGE_SIZES_MAP', {'full': 50, 'small': 10}, clear=True
+ 'openedx.core.djangoapps.user_api.accounts.image_helpers.PROFILE_IMAGE_SIZES_MAP', {'full': 50, 'small': 10}, clear=True
)
class TestAccountAPI(UserAPITestCase):
"""
@@ -200,7 +200,7 @@ class TestAccountAPI(UserAPITestCase):
"""
client = self.login_client(api_client, user)
response = client.get(reverse("accounts_api", kwargs={'username': "does_not_exist"}))
- self.assertEqual(404, response.status_code)
+ self.assertEqual(403 if user == "staff_user" else 404, response.status_code)
# Note: using getattr so that the patching works even if there is no configuration.
# This is needed when testing CMS as the patching is still executed even though the
@@ -279,7 +279,6 @@ class TestAccountAPI(UserAPITestCase):
for empty_field in ("year_of_birth", "level_of_education", "mailing_address", "bio"):
self.assertIsNone(data[empty_field])
self.assertIsNone(data["country"])
- # TODO: what should the format of this be?
self.assertEqual("m", data["gender"])
self.assertEqual("Learn a lot", data["goals"])
self.assertEqual(self.user.email, data["email"])
@@ -323,7 +322,7 @@ class TestAccountAPI(UserAPITestCase):
is_staff access).
"""
client = self.login_client(api_client, user)
- self.send_patch(client, {}, expected_status=404)
+ self.send_patch(client, {}, expected_status=403 if user == "staff_user" else 404)
@ddt.data(
("client", "user"),
diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py
index 1bd0696f54f..19c86490305 100644
--- a/openedx/core/djangoapps/user_api/accounts/views.py
+++ b/openedx/core/djangoapps/user_api/accounts/views.py
@@ -8,9 +8,12 @@ from django.db import transaction
from rest_framework.views import APIView
from rest_framework.response import Response
from rest_framework import status
-from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser
from rest_framework import permissions
+from openedx.core.lib.api.authentication import (
+ SessionAuthenticationAllowInactiveUser,
+ OAuth2AuthenticationAllowInactiveUser,
+)
from ..errors import UserNotFound, UserNotAuthorized, AccountUpdateError, AccountValidationError
from openedx.core.lib.api.parsers import MergePatchParser
from .api import get_account_settings, update_account_settings
@@ -123,8 +126,10 @@ class AccountView(APIView):
**Response Values for PATCH**
- Users can modify only their own account information. If the user
- attempts to modify another user's account, a 404 error is returned.
+ Users can only modify their own account information. If the requesting
+ user does not have username "username", this method will return with
+ a status of 403 for staff access but a 404 for ordinary users
+ to avoid leaking the existence of the account.
If no user exists with the specified username, a 404 error is
returned.
@@ -158,7 +163,7 @@ class AccountView(APIView):
if key.startswith(PROFILE_IMAGE_KEY_PREFIX):
account_settings['profile_image'][key] = request.build_absolute_uri(value)
except UserNotFound:
- return Response(status=status.HTTP_404_NOT_FOUND)
+ return Response(status=status.HTTP_403_FORBIDDEN if request.user.is_staff else status.HTTP_404_NOT_FOUND)
return Response(account_settings)
@@ -173,7 +178,9 @@ class AccountView(APIView):
try:
with transaction.commit_on_success():
update_account_settings(request.user, request.DATA, username=username)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN if request.user.is_staff else status.HTTP_404_NOT_FOUND)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
except AccountValidationError as err:
return Response({"field_errors": err.field_errors}, status=status.HTTP_400_BAD_REQUEST)
diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py
index 2c40f8515e7..8c2a3860ac2 100644
--- a/openedx/core/djangoapps/user_api/preferences/api.py
+++ b/openedx/core/djangoapps/user_api/preferences/api.py
@@ -256,7 +256,6 @@ def update_email_opt_in(user, org, opt_in):
log.warn(u"Could not update organization wide preference due to IntegrityError: {}".format(err.message))
-
def _track_update_email_opt_in(user_id, organization, opt_in):
"""Track an email opt-in preference change.
diff --git a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
index d3f4830edc7..8ebbf282e91 100644
--- a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
@@ -160,7 +160,7 @@ class TestPreferencesAPI(UserAPITestCase):
"dict_pref": {"int_key": 10},
"string_pref": "value",
},
- expected_status=404,
+ expected_status=403 if user == "staff_user" else 404,
)
def test_update_preferences(self):
@@ -288,7 +288,7 @@ class TestPreferencesAPI(UserAPITestCase):
"new_pref": "new_value",
"extra_pref": None,
},
- expected_status=404
+ expected_status=403 if user == "staff_user" else 404
)
@@ -503,7 +503,7 @@ class TestPreferencesDetailAPI(UserAPITestCase):
self._set_url("new_key")
client = self.login_client(api_client, user)
new_value = "new value"
- self.send_put(client, new_value, expected_status=404)
+ self.send_put(client, new_value, expected_status=403 if user == "staff_user" else 404)
@ddt.data(
(u"new value",),
@@ -531,7 +531,7 @@ class TestPreferencesDetailAPI(UserAPITestCase):
"""
client = self.login_client(api_client, user)
new_value = "new value"
- self.send_put(client, new_value, expected_status=404)
+ self.send_put(client, new_value, expected_status=403 if user == "staff_user" else 404)
@ddt.data(
(None,),
@@ -578,4 +578,4 @@ class TestPreferencesDetailAPI(UserAPITestCase):
Test that a client (logged in) cannot delete a preference for another user.
"""
client = self.login_client(api_client, user)
- self.send_delete(client, expected_status=404)
+ self.send_delete(client, expected_status=403 if user == "staff_user" else 404)
diff --git a/openedx/core/djangoapps/user_api/preferences/views.py b/openedx/core/djangoapps/user_api/preferences/views.py
index 1974ff0ccad..695d811f4d3 100644
--- a/openedx/core/djangoapps/user_api/preferences/views.py
+++ b/openedx/core/djangoapps/user_api/preferences/views.py
@@ -7,13 +7,17 @@ https://openedx.atlassian.net/wiki/display/TNL/User+API
from rest_framework.views import APIView
from rest_framework.response import Response
from rest_framework import status
-from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser
from rest_framework import permissions
from django.db import transaction
from django.utils.translation import ugettext as _
+from openedx.core.lib.api.authentication import (
+ SessionAuthenticationAllowInactiveUser,
+ OAuth2AuthenticationAllowInactiveUser,
+)
from openedx.core.lib.api.parsers import MergePatchParser
+from openedx.core.lib.api.permissions import IsUserInUrlOrStaff
from ..errors import UserNotFound, UserNotAuthorized, PreferenceValidationError, PreferenceUpdateError
from .api import (
get_user_preference, get_user_preferences, set_user_preference, update_user_preferences, delete_user_preference
@@ -45,7 +49,8 @@ class PreferencesView(APIView):
**Response for PATCH**
Users can only modify their own preferences. If the requesting user does not have username
- "username", this method will return with a status of 404.
+ "username", this method will return with a status of 403 for staff access but a 404 for ordinary
+ users to avoid leaking the existence of the account.
This method will also return a 404 if no user exists with username "username".
@@ -61,7 +66,7 @@ class PreferencesView(APIView):
"""
authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
- permission_classes = (permissions.IsAuthenticated,)
+ permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff)
parser_classes = (MergePatchParser,)
def get(self, request, username):
@@ -70,7 +75,9 @@ class PreferencesView(APIView):
"""
try:
user_preferences = get_user_preferences(request.user, username=username)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
return Response(user_preferences)
@@ -91,7 +98,9 @@ class PreferencesView(APIView):
try:
with transaction.commit_on_success():
update_user_preferences(request.user, request.DATA, username=username)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
except PreferenceValidationError as error:
return Response(
@@ -136,17 +145,25 @@ class PreferencesDetailView(APIView):
A successful put returns a 204 and no content.
- If the specified username or preference does not exist, this method returns a 404.
+ Users can only update their own preferences. If the requesting user does not have username
+ "username", this method will return with a status of 403 for staff access but a 404 for ordinary
+ users to avoid leaking the existence of the account.
+
+ If the specified preference does not exist, this method returns a 404.
**Response for DELETE**
A successful delete returns a 204 and no content.
- If the specified username or preference does not exist, this method returns a 404.
+ Users can only delete their own preferences. If the requesting user does not have username
+ "username", this method will return with a status of 403 for staff access but a 404 for ordinary
+ users to avoid leaking the existence of the account.
+
+ If the specified preference does not exist, this method returns a 404.
"""
authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
- permission_classes = (permissions.IsAuthenticated,)
+ permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff)
def get(self, request, username, preference_key):
"""
@@ -157,7 +174,9 @@ class PreferencesDetailView(APIView):
# There was no preference with that key, raise a 404.
if value is None:
return Response(status=status.HTTP_404_NOT_FOUND)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
return Response(value)
@@ -167,7 +186,9 @@ class PreferencesDetailView(APIView):
"""
try:
set_user_preference(request.user, preference_key, request.DATA, username=username)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
except PreferenceValidationError as error:
return Response(
@@ -193,7 +214,9 @@ class PreferencesDetailView(APIView):
"""
try:
preference_existed = delete_user_preference(request.user, preference_key, username=username)
- except (UserNotFound, UserNotAuthorized):
+ except UserNotAuthorized:
+ return Response(status=status.HTTP_403_FORBIDDEN)
+ except UserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
except PreferenceUpdateError as error:
return Response(
diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py
index 464b5c9ed7b..0337b2cbcf0 100644
--- a/openedx/core/djangoapps/user_api/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/tests/test_views.py
@@ -1693,6 +1693,7 @@ class TestGoogleRegistrationView(
"""Tests the User API registration endpoint with Google authentication."""
pass
+
@ddt.ddt
class UpdateEmailOptInTestCase(ApiTestCase, ModuleStoreTestCase):
"""Tests the UpdateEmailOptInPreference view. """
diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py
index d1844b119fd..dc9ad7c5c15 100644
--- a/openedx/core/djangoapps/user_api/views.py
+++ b/openedx/core/djangoapps/user_api/views.py
@@ -1,9 +1,6 @@
"""HTTP end-points for the User API. """
import copy
-from openedx.core.lib.api.permissions import ApiKeyHeaderPermission
from opaque_keys import InvalidKeyError
-import third_party_auth
-
from django.conf import settings
from django.contrib.auth.models import User
from django.http import HttpResponse
@@ -21,12 +18,14 @@ from rest_framework import viewsets
from rest_framework.views import APIView
from rest_framework.exceptions import ParseError
from django_countries import countries
-from django_comment_common.models import Role
from opaque_keys.edx.locations import SlashSeparatedCourseKey
-from edxmako.shortcuts import marketing_link
+from openedx.core.lib.api.permissions import ApiKeyHeaderPermission
+import third_party_auth
+from django_comment_common.models import Role
+from edxmako.shortcuts import marketing_link
from student.views import create_account_with_params, set_marketing_cookie
-from util.authentication import SessionAuthenticationAllowInactiveUser
+from openedx.core.lib.api.authentication import SessionAuthenticationAllowInactiveUser
from util.json_request import JsonResponse
from .preferences.api import update_email_opt_in
from .helpers import FormDescription, shim_student_view, require_post_params
diff --git a/common/djangoapps/util/authentication.py b/openedx/core/lib/api/authentication.py
similarity index 100%
rename from common/djangoapps/util/authentication.py
rename to openedx/core/lib/api/authentication.py
diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py
index e02b190a5cd..83734fc0646 100644
--- a/openedx/core/lib/api/permissions.py
+++ b/openedx/core/lib/api/permissions.py
@@ -52,9 +52,26 @@ class IsUserInUrl(permissions.BasePermission):
Permission that checks to see if the request user matches the user in the URL.
"""
def has_permission(self, request, view):
- # Return a 404 instead of a 403 (Unauthorized). If one user is looking up
- # other users, do not let them deduce the existence of an account.
+ """
+ Returns true if the current request is by the user themselves.
+
+ Note: a 404 is returned for non-staff instead of a 403. This is to prevent
+ users from being able to detect the existence of accounts.
+ """
url_username = request.parser_context.get('kwargs', {}).get('username', '')
if request.user.username.lower() != url_username.lower():
+ if request.user.is_staff:
+ return False # staff gets 403
raise Http404()
return True
+
+
+class IsUserInUrlOrStaff(IsUserInUrl):
+ """
+ Permission that checks to see if the request user matches the user in the URL or has is_staff access.
+ """
+ def has_permission(self, request, view):
+ if request.user.is_staff:
+ return True
+
+ return super(IsUserInUrlOrStaff, self).has_permission(request, view)
diff --git a/openedx/core/lib/api/tests/__init__.py b/openedx/core/lib/api/tests/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/common/djangoapps/util/tests/test_authentication.py b/openedx/core/lib/api/tests/test_authentication.py
similarity index 100%
rename from common/djangoapps/util/tests/test_authentication.py
rename to openedx/core/lib/api/tests/test_authentication.py
--
GitLab