From 44c78c609cd9a5e3430c1ff5e0cd3e140ed78075 Mon Sep 17 00:00:00 2001 From: Daniel Friedman Date: Fri, 20 Mar 2015 16:25:23 -0400 Subject: [PATCH] Add logging to image uploads/deletes. Conflicts: openedx/core/djangoapps/profile_images/views.py --- .../profile_images/tests/test_views.py | 59 +++++++++++++------ .../core/djangoapps/profile_images/views.py | 22 ++++++- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py index 80c349d9c7..327f606218 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_views.py +++ b/openedx/core/djangoapps/profile_images/tests/test_views.py @@ -4,7 +4,6 @@ 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 @@ -21,6 +20,7 @@ from ...user_api.accounts.image_helpers import ( 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 TEST_PASSWORD = "test" @@ -94,15 +94,15 @@ class ProfileImageEndpointTestCase(APITestCase): 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') +@mock.patch('openedx.core.djangoapps.profile_images.views.log') class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): """ Tests for the profile_image upload endpoint. """ _view_name = "profile_image_upload" - def test_unsupported_methods(self): + def test_unsupported_methods(self, mock_log): """ Test that GET, PUT, PATCH, and DELETE are not supported. """ @@ -110,16 +110,18 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): 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) - def test_anonymous_access(self): + def test_anonymous_access(self, mock_log): """ 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) + self.assertFalse(mock_log.info.called) - def test_upload_self(self): + def test_upload_self(self, mock_log): """ Test that an authenticated user can POST to their own upload endpoint. """ @@ -128,8 +130,12 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): 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} + ) - def test_upload_other(self): + def test_upload_other(self, mock_log): """ Test that an authenticated user cannot POST to another user's upload endpoint. """ @@ -141,8 +147,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): self.check_response(response, 404) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) - def test_upload_staff(self): + def test_upload_staff(self, mock_log): """ Test that an authenticated staff cannot POST to another user's upload endpoint. """ @@ -154,8 +161,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): self.check_response(response, 403) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) - def test_upload_missing_file(self): + def test_upload_missing_file(self, mock_log): """ Test that omitting the file entirely from the POST results in HTTP 400. """ @@ -167,8 +175,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): ) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) - def test_upload_not_a_file(self): + def test_upload_not_a_file(self, mock_log): """ Test that sending unexpected data that isn't a file results in HTTP 400. @@ -181,8 +190,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): ) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) - def test_upload_validation(self): + def test_upload_validation(self, mock_log): """ Test that when upload validation fails, the proper HTTP response and messages are returned. @@ -200,9 +210,10 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): ) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) @patch('PIL.Image.open') - def test_upload_failure(self, image_open): + def test_upload_failure(self, image_open, mock_log): """ Test that when upload validation fails, the proper HTTP response and messages are returned. @@ -217,9 +228,11 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): ) self.check_images(False) self.check_has_profile_image(False) + self.assertFalse(mock_log.info.called) @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): """ Tests for the profile_image remove endpoint. @@ -233,7 +246,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): self.check_images() set_has_profile_image(self.user.username, True) - def test_unsupported_methods(self): + def test_unsupported_methods(self, mock_log): """ Test that GET, PUT, PATCH, and DELETE are not supported. """ @@ -241,8 +254,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): 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) - def test_anonymous_access(self): + def test_anonymous_access(self, mock_log): """ Test that an anonymous client (not logged in) cannot call GET or POST. """ @@ -250,8 +264,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): for request in (anonymous_client.get, anonymous_client.post): response = request(self.url) self.assertEqual(401, response.status_code) + self.assertFalse(mock_log.info.called) - def test_remove_self(self): + def test_remove_self(self, mock_log): """ Test that an authenticated user can POST to remove their own profile images. @@ -260,8 +275,12 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): self.check_response(response, 204) self.check_images(False) self.check_has_profile_image(False) + mock_log.info.assert_called_once_with( + LOG_MESSAGE_DELETE, + {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id} + ) - def test_remove_other(self): + def test_remove_other(self, mock_log): """ Test that an authenticated user cannot POST to remove another user's profile images. @@ -273,8 +292,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): self.check_response(response, 404) self.check_images(True) # thumbnails should remain intact. self.check_has_profile_image(True) + self.assertFalse(mock_log.info.called) - def test_remove_staff(self): + def test_remove_staff(self, mock_log): """ Test that an authenticated staff user can POST to remove another user's profile images. @@ -286,9 +306,13 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): self.check_response(response, 204) self.check_images(False) self.check_has_profile_image(False) + mock_log.info.assert_called_once_with( + LOG_MESSAGE_DELETE, + {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id} + ) @patch('student.models.UserProfile.save') - def test_remove_failure(self, user_profile_save): + def test_remove_failure(self, user_profile_save, mock_log): """ Test that when upload validation fails, the proper HTTP response and messages are returned. @@ -302,3 +326,4 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): ) self.check_images(True) # thumbnails should remain intact. self.check_has_profile_image(True) + self.assertFalse(mock_log.info.called) diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py index f2aaf748f7..8968cd459b 100644 --- a/openedx/core/djangoapps/profile_images/views.py +++ b/openedx/core/djangoapps/profile_images/views.py @@ -2,6 +2,7 @@ This module implements the upload and remove endpoints of the profile image api. """ from contextlib import closing +import logging from django.utils.translation import ugettext as _ from rest_framework import permissions, status @@ -18,6 +19,11 @@ 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 +log = logging.getLogger(__name__) + +LOG_MESSAGE_CREATE = 'Generated and uploaded images %(image_names)s for user %(user_id)s' +LOG_MESSAGE_DELETE = 'Deleted images %(image_names)s for user %(user_id)s' + class ProfileImageUploadView(APIView): """ @@ -82,10 +88,16 @@ class ProfileImageUploadView(APIView): ) # generate profile pic and thumbnails and store them - create_profile_images(uploaded_file, get_profile_image_names(username)) + profile_image_names = get_profile_image_names(username) + create_profile_images(uploaded_file, profile_image_names) # update the user account to reflect that a profile image is available. set_has_profile_image(username, True) + + log.info( + LOG_MESSAGE_CREATE, + {'image_names': profile_image_names.values(), 'user_id': request.user.id} + ) except Exception as error: return Response( { @@ -135,7 +147,13 @@ class ProfileImageRemoveView(APIView): set_has_profile_image(username, False) # remove physical files from storage. - remove_profile_images(get_profile_image_names(username)) + profile_image_names = get_profile_image_names(username) + remove_profile_images(profile_image_names) + + log.info( + LOG_MESSAGE_DELETE, + {'image_names': profile_image_names.values(), 'user_id': request.user.id} + ) except UserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) except Exception as error: