From e200015cfc29df983300aa5ae2ab7e1c9fed47f0 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Tue, 29 Sep 2015 20:35:42 +0000 Subject: [PATCH] Make the profile_image api more restful. MA-1280 Deprecate the views at /api/profile_images/v1/{username}/upload and /api/profile_images/v1/{username}/remove and replace them with GET and POST methods at /api/user/v1/accounts/{username}/image --- .../profile_images/tests/test_views.py | 168 ++++++++++++------ .../core/djangoapps/profile_images/urls.py | 7 +- .../core/djangoapps/profile_images/views.py | 164 ++++++++++------- openedx/core/djangoapps/user_api/urls.py | 16 +- 4 files changed, 231 insertions(+), 124 deletions(-) diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py index 4e93a1ad25..eecf56e913 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_views.py +++ b/openedx/core/djangoapps/profile_images/tests/test_views.py @@ -8,6 +8,8 @@ import unittest from django.conf import settings from django.core.urlresolvers import reverse +from django.http import HttpResponse + import mock from mock import patch from PIL import Image @@ -64,7 +66,7 @@ class PatchedClient(APIClient): return super(PatchedClient, self).request(*args, **kwargs) -class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase): +class ProfileImageEndpointMixin(UserSettingsEventTestMixin): """ Base class / shared infrastructure for tests of profile_image "upload" and "remove" endpoints. @@ -72,9 +74,10 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase): # subclasses should override this with the name of the view under test, as # per the urls.py configuration. _view_name = None + client_class = PatchedClient def setUp(self): - super(ProfileImageEndpointTestCase, self).setUp() + super(ProfileImageEndpointMixin, 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 @@ -92,7 +95,7 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase): self.reset_tracker() def tearDown(self): - super(ProfileImageEndpointTestCase, self).tearDown() + super(ProfileImageEndpointMixin, self).tearDown() for name in get_profile_image_names(self.user.username).values(): self.storage.delete(name) @@ -136,18 +139,46 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase): profile = self.user.profile.__class__.objects.get(user=self.user) self.assertEqual(profile.has_profile_image, has_profile_image) + def check_anonymous_request_rejected(self, method): + """ + Make sure that the specified method rejects access by unauthorized users. + """ + anonymous_client = APIClient() + request_method = getattr(anonymous_client, method) + response = request_method(self.url) + self.check_response(response, 401) + self.assert_no_events_were_emitted() + @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 ProfileImageUploadTestCase(ProfileImageEndpointTestCase): +class ProfileImageViewGeneralTestCase(ProfileImageEndpointMixin, APITestCase): """ - Tests for the profile_image upload endpoint. + Tests for the profile image endpoint """ - _view_name = "profile_image_upload" + _view_name = "accounts_profile_image_api" + + def test_unsupported_methods(self, mock_log): + """ + Test that GET, PUT, and PATCH 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) # pylint: disable=no-member + self.assertFalse(mock_log.info.called) + self.assert_no_events_were_emitted() + + +@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): + """ + Tests for the POST method of the profile_image api endpoint. + """ + _view_name = "accounts_profile_image_api" # Use the patched version of the API client to workaround a unicode issue # with DRF 3.1 and Django 1.4. Remove this after we upgrade Django past 1.4! - client_class = PatchedClient def check_upload_event_emitted(self, old=None, new=TEST_UPLOAD_DT): """ @@ -158,26 +189,12 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): setting='profile_image_uploaded_at', old=old, new=new ) - def test_unsupported_methods(self, mock_log): - """ - 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) - self.assertFalse(mock_log.info.called) - self.assert_no_events_were_emitted() - def test_anonymous_access(self, mock_log): """ - Test that an anonymous client (not logged in) cannot POST. + Test that an anonymous client (not logged in) cannot call POST. """ - anonymous_client = APIClient() - response = anonymous_client.post(self.url) - self.assertEqual(401, response.status_code) + self.check_anonymous_request_rejected('post') self.assertFalse(mock_log.info.called) - self.assert_no_events_were_emitted() @patch('openedx.core.djangoapps.profile_images.views._make_upload_dt', side_effect=[TEST_UPLOAD_DT, TEST_UPLOAD_DT2]) def test_upload_self(self, mock_make_image_version, mock_log): # pylint: disable=unused-argument @@ -204,7 +221,8 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): def test_upload_other(self, mock_log): """ - Test that an authenticated user cannot POST to another user's upload endpoint. + Test that an authenticated user cannot POST to another user's upload + endpoint. """ different_user = UserFactory.create(password=TEST_PASSWORD) # Ignore UserProfileFactory creation events. @@ -221,7 +239,8 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): def test_upload_staff(self, mock_log): """ - Test that an authenticated staff cannot POST to another user's upload endpoint. + Test that an authenticated staff cannot POST to another user's upload + endpoint. """ staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD) # Ignore UserProfileFactory creation events. @@ -306,14 +325,14 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): @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 ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): +class ProfileImageViewDeleteTestCase(ProfileImageEndpointMixin, APITestCase): """ - Tests for the profile_image remove endpoint. + Tests for the DELETE method of the profile_image endpoint. """ - _view_name = "profile_image_remove" + _view_name = "accounts_profile_image_api" def setUp(self): - super(ProfileImageRemoveTestCase, self).setUp() + super(ProfileImageViewDeleteTestCase, self).setUp() with make_image_file() as image_file: create_profile_images(image_file, get_profile_image_names(self.user.username)) self.check_images() @@ -330,34 +349,19 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): setting='profile_image_uploaded_at', old=TEST_UPLOAD_DT, new=None ) - def test_unsupported_methods(self, mock_log): - """ - 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) - self.assertFalse(mock_log.info.called) - self.assert_no_events_were_emitted() - def test_anonymous_access(self, mock_log): """ - Test that an anonymous client (not logged in) cannot call GET or POST. + Test that an anonymous client (not logged in) cannot call DELETE. """ - anonymous_client = APIClient() - for request in (anonymous_client.get, anonymous_client.post): - response = request(self.url) - self.assertEqual(401, response.status_code) + self.check_anonymous_request_rejected('delete') self.assertFalse(mock_log.info.called) - self.assert_no_events_were_emitted() def test_remove_self(self, mock_log): """ - Test that an authenticated user can POST to remove their own profile + Test that an authenticated user can DELETE to remove their own profile images. """ - response = self.client.post(self.url) + response = self.client.delete(self.url) self.check_response(response, 204) self.check_images(False) self.check_has_profile_image(False) @@ -369,7 +373,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): def test_remove_other(self, mock_log): """ - Test that an authenticated user cannot POST to remove another user's + Test that an authenticated user cannot DELETE to remove another user's profile images. """ different_user = UserFactory.create(password=TEST_PASSWORD) @@ -377,7 +381,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): self.reset_tracker() different_client = APIClient() different_client.login(username=different_user.username, password=TEST_PASSWORD) - response = different_client.post(self.url) + response = different_client.delete(self.url) self.check_response(response, 404) self.check_images(True) # thumbnails should remain intact. self.check_has_profile_image(True) @@ -386,13 +390,13 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): def test_remove_staff(self, mock_log): """ - Test that an authenticated staff user can POST to remove another user's + Test that an authenticated staff user can DELETE 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) + response = self.client.delete(self.url) self.check_response(response, 204) self.check_images(False) self.check_has_profile_image(False) @@ -405,13 +409,69 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): @patch('student.models.UserProfile.save') def test_remove_failure(self, user_profile_save, mock_log): """ - Test that when upload validation fails, the proper HTTP response and + Test that when remove validation fails, the proper HTTP response and messages are returned. """ user_profile_save.side_effect = [Exception(u"whoops"), None] with self.assertRaises(Exception): - self.client.post(self.url) + self.client.delete(self.url) self.check_images(True) # thumbnails should remain intact. self.check_has_profile_image(True) self.assertFalse(mock_log.info.called) self.assert_no_events_were_emitted() + + +class DeprecatedProfileImageTestMixin(ProfileImageEndpointMixin): + """ + Actual tests for DeprecatedProfileImage.*TestCase classes defined here. + + Requires: + self._view_name + self._replacement_method + """ + + def test_unsupported_methods(self, mock_log): + """ + 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) + self.assertFalse(mock_log.info.called) + self.assert_no_events_were_emitted() + + def test_post_calls_replacement_view_method(self, mock_log): + """ + Test that calls to this view pass through the the new view. + """ + with patch(self._replacement_method) as mock_method: + mock_method.return_value = HttpResponse() + self.client.post(self.url) + assert mock_method.called + self.assertFalse(mock_log.info.called) + self.assert_no_events_were_emitted() + + +@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 DeprecatedProfileImageUploadTestCase(DeprecatedProfileImageTestMixin, APITestCase): + """ + Tests for the deprecated profile_image upload endpoint. + + Actual tests defined on DeprecatedProfileImageTestMixin + """ + _view_name = 'profile_image_upload' + _replacement_method = 'openedx.core.djangoapps.profile_images.views.ProfileImageView.post' + + +@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 DeprecatedProfileImageRemoveTestCase(DeprecatedProfileImageTestMixin, APITestCase): + """ + Tests for the deprecated profile_image remove endpoint. + + Actual tests defined on DeprecatedProfileImageTestMixin + """ + _view_name = "profile_image_remove" + _replacement_method = 'openedx.core.djangoapps.profile_images.views.ProfileImageView.delete' diff --git a/openedx/core/djangoapps/profile_images/urls.py b/openedx/core/djangoapps/profile_images/urls.py index 0f8dd3dbae..65f72a6458 100644 --- a/openedx/core/djangoapps/profile_images/urls.py +++ b/openedx/core/djangoapps/profile_images/urls.py @@ -1,10 +1,15 @@ """ Defines the URL routes for this app. + +NOTE: These views are deprecated. These routes are superseded by +``/api/user/v1/accounts/{username}/image``, found in +``openedx.core.djangoapps.user_api.urls``. """ -from .views import ProfileImageUploadView, ProfileImageRemoveView from django.conf.urls import patterns, url +from .views import ProfileImageUploadView, ProfileImageRemoveView + USERNAME_PATTERN = r'(?P[\w.+-]+)' urlpatterns = patterns( diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py index c0abce9109..c6d8357e3e 100644 --- a/openedx/core/djangoapps/profile_images/views.py +++ b/openedx/core/djangoapps/profile_images/views.py @@ -35,49 +35,85 @@ def _make_upload_dt(): return datetime.datetime.utcnow().replace(tzinfo=utc) -class ProfileImageUploadView(APIView): +class ProfileImageView(APIView): """ - **Use Case** + **Use Cases** - * Upload an image for the user's profile. + Add or remove profile images associated with user accounts. - The requesting user must be signed in. The signed in user can only - upload his or her own profile image. + The requesting user must be signed in. Users can only add profile + images to their own account. Users with staff access can remove + profile images for other user accounts. All other users can remove + only their own profile images. - **Example Request** + **Example Requests** - POST /api/profile_images/v1/{username}/upload + POST /api/user/v1/accounts/{username}/image - **Example Responses** + DELETE /api/user/v1/accounts/{username}/image - When the requesting user tries to upload the image for a different user, the - request returns one of the following responses. + **Example POST Responses** - * If the requesting user has staff access, the request returns an HTTP 403 - "Forbidden" response. + When the requesting user attempts to upload an image for their own + account, the request returns one of the following responses: - * If the requesting user does not have staff access, the request returns - an HTTP 404 "Not Found" response. + * If the upload could not be performed, the request returns an HTTP 400 + "Bad Request" response with information about why the request failed. - * If no user matches the "username" parameter, the request returns an HTTP - 404 "Not Found" response. + * If the upload is successful, the request returns an HTTP 204 "No + Content" response with no additional content. - * If the upload could not be performed, the request returns an HTTP 400 "Bad - Request" response with more information. + If the requesting user tries to upload an image for a different + user, the request returns one of the following responses: - * If the upload is successful, the request returns an HTTP 204 "No Content" - response with no additional content. + * If no user matches the "username" parameter, the request returns an + HTTP 404 "Not Found" response. + * If the user whose profile image is being uploaded exists, but the + requesting user does not have staff access, the request returns an + HTTP 404 "Not Found" response. + + * If the specified user exists, and the requesting user has staff + access, the request returns an HTTP 403 "Forbidden" response. + + **Example DELETE Responses** + + When the requesting user attempts to remove the profile image for + their own account, the request returns one of the following + responses: + + * If the image could not be removed, the request returns an HTTP 400 + "Bad Request" response with information about why the request failed. + + * If the request successfully removes the image, the request returns + an HTTP 204 "No Content" response with no additional content. + + When the requesting user tries to remove the profile image for a + different user, the view will return one of the following responses: + + * If the requesting user has staff access, and the "username" parameter + matches a user, the profile image for the specified user is deleted, + and the request returns an HTTP 204 "No Content" response with no + additional content. + + * If the requesting user has staff access, but no user is matched by + the "username" parameter, the request returns an HTTP 404 "Not Found" + response. + + * If the requesting user does not have staff access, the request + returns an HTTP 404 "Not Found" response, regardless of whether + the user exists or not. """ - parser_classes = (MultiPartParser, FormParser,) + parser_classes = (MultiPartParser, FormParser) authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser) permission_classes = (permissions.IsAuthenticated, IsUserInUrl) def post(self, request, username): """ - POST /api/profile_images/v1/{username}/upload + POST /api/user/v1/accounts/{username}/image """ + # validate request: # verify that the user's # ensure any file was sent @@ -121,50 +157,11 @@ class ProfileImageUploadView(APIView): # send client response. return Response(status=status.HTTP_204_NO_CONTENT) - -class ProfileImageRemoveView(APIView): - """ - **Use Case** - - * Remove all of the profile images associated with the user's account. - - The requesting user must be signed in. - - Users with staff access can remove profile images for other user - accounts. - - Users without staff access can only remove their own profile images. - - **Example Request** - - POST /api/profile_images/v1/{username}/remove - - **Example Responses** - - When the requesting user tries to remove the profile image for a - different user, the request returns one of the following responses. - - * If the user does not have staff access, the request returns an HTTP - 404 "Not Found" response. - - * If no user matches the "username" parameter, the request returns an - HTTP 404 "Not Found" response. - - * If the image could not be removed, the request returns an HTTP 400 - "Bad Request" response with more information. - - * If the request successfully removes the image, the request returns - an HTTP 204 "No Content" response with no additional content. - - - """ - authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser) - permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff) - - def post(self, request, username): # pylint: disable=unused-argument + def delete(self, request, username): # pylint: disable=unused-argument """ - POST /api/profile_images/v1/{username}/remove + DELETE /api/user/v1/accounts/{username}/image """ + try: # update the user account to reflect that the images were removed. set_has_profile_image(username, False) @@ -182,3 +179,42 @@ class ProfileImageRemoveView(APIView): # send client response. return Response(status=status.HTTP_204_NO_CONTENT) + + +class ProfileImageUploadView(APIView): + """ + **DEPRECATION WARNING** + + /api/profile_images/v1/{username}/upload is deprecated. + All requests should now be sent to + /api/user/v1/accounts/{username}/image + """ + + parser_classes = ProfileImageView.parser_classes + authentication_classes = ProfileImageView.authentication_classes + permission_classes = ProfileImageView.permission_classes + + def post(self, request, username): + """ + POST /api/profile_images/v1/{username}/upload + """ + return ProfileImageView().post(request, username) + + +class ProfileImageRemoveView(APIView): + """ + **DEPRECATION WARNING** + + /api/profile_images/v1/{username}/remove is deprecated. + This endpoint's POST is replaced by the DELETE method at + /api/user/v1/accounts/{username}/image. + """ + + authentication_classes = ProfileImageView.authentication_classes + permission_classes = ProfileImageView.permission_classes + + def post(self, request, username): + """ + POST /api/profile_images/v1/{username}/remove + """ + return ProfileImageView().delete(request, username) diff --git a/openedx/core/djangoapps/user_api/urls.py b/openedx/core/djangoapps/user_api/urls.py index 8ae0414941..8f143a741a 100644 --- a/openedx/core/djangoapps/user_api/urls.py +++ b/openedx/core/djangoapps/user_api/urls.py @@ -2,27 +2,33 @@ Defines the URL routes for this app. """ +from django.conf.urls import patterns, url + +from ..profile_images.views import ProfileImageView from .accounts.views import AccountView from .preferences.views import PreferencesView, PreferencesDetailView -from django.conf.urls import patterns, url - USERNAME_PATTERN = r'(?P[\w.+-]+)' urlpatterns = patterns( '', url( - r'^v1/accounts/' + USERNAME_PATTERN + '$', + r'^v1/accounts/{}$'.format(USERNAME_PATTERN), AccountView.as_view(), name="accounts_api" ), url( - r'^v1/preferences/' + USERNAME_PATTERN + '$', + r'^v1/accounts/{}/image$'.format(USERNAME_PATTERN), + ProfileImageView.as_view(), + name="accounts_profile_image_api" + ), + url( + r'^v1/preferences/{}$'.format(USERNAME_PATTERN), PreferencesView.as_view(), name="preferences_api" ), url( - r'^v1/preferences/' + USERNAME_PATTERN + '/(?P[a-zA-Z0-9_]+)$', + r'^v1/preferences/{}/(?P[a-zA-Z0-9_]+)$'.format(USERNAME_PATTERN), PreferencesDetailView.as_view(), name="preferences_detail_api" ),