diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index a245b30658..7842da64e2 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -720,7 +720,7 @@ def submit_photos_for_verification(request): # then try to do that before creating the attempt. if request.POST.get('full_name'): try: - AccountView.update_account(request.user, username, {"name": request.POST.get('full_name')}) + AccountView.update_account(username, {"name": request.POST.get('full_name')}) except AccountUserNotFound: return HttpResponseBadRequest(_("No profile found for user")) except AccountUpdateError: 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 cec245f01d..e20eebbede 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -181,6 +181,13 @@ class TestAccountAPI(UserAPITestCase): self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.send_patch(self.different_client, {}, expected_status=404) + def test_patch_account_is_staff(self): + """ + Test that a client (logged in) with is_staff privileges cannot account settings for other users. + """ + self.staff_client.login(username=self.staff_user.username, password=TEST_PASSWORD) + self.send_patch(self.staff_client, {}, expected_status=404) + @ddt.data( ("client", "user"), ("staff_client", "staff_user"), @@ -198,34 +205,25 @@ class TestAccountAPI(UserAPITestCase): self.assertEqual(404, response.status_code) @ddt.data( - ( - "client", "user", "gender", "f", "not a gender", - "Select a valid choice. not a gender is not one of the available choices." - ), - ( - "client", "user", "level_of_education", "none", "x", - "Select a valid choice. x is not one of the available choices." - ), - ("client", "user", "country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), - ("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."), - ("client", "user", "name", "bob", "z" * 256, "Ensure this value has at most 255 characters (it has 256)."), - ("client", "user", "name", u"ȻħȺɍłɇs", "z ", "The name field must be at least 2 characters long."), - ("client", "user", "language", "Creole"), - ("client", "user", "goals", "Smell the roses"), - ("client", "user", "mailing_address", "Sesame Street"), - # All of the fields can be edited by is_staff, but iterating through all of them again seems like overkill. - # Just test a representative field. - ("staff_client", "staff_user", "goals", "Smell the roses"), + ("gender", "f", "not a gender", "Select a valid choice. not a gender is not one of the available choices."), + ("level_of_education", "none", "x", "Select a valid choice. x is not one of the available choices."), + ("country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), + ("year_of_birth", 2009, "not_an_int", "Enter a whole number."), + ("name", "bob", "z" * 256, "Ensure this value has at most 255 characters (it has 256)."), + ("name", u"ȻħȺɍłɇs", "z ", "The name field must be at least 2 characters long."), + ("language", "Creole"), + ("goals", "Smell the roses"), + ("mailing_address", "Sesame Street"), # Note that email is tested below, as it is not immediately updated. ) @ddt.unpack def test_patch_account( - self, api_client, user, field, value, fails_validation_value=None, developer_validation_message=None + self, field, value, fails_validation_value=None, developer_validation_message=None ): """ Test the behavior of patch, when using the correct content_type. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") self.send_patch(client, {field: value}) get_response = self.send_get(client) @@ -248,16 +246,12 @@ class TestAccountAPI(UserAPITestCase): get_response = self.send_get(client) self.assertEqual("", get_response.data[field]) - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) @ddt.unpack - def test_patch_account_noneditable(self, api_client, user): + def test_patch_account_noneditable(self): """ Tests the behavior of patch when a read-only field is attempted to be edited. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") def verify_error_response(field_name, data): self.assertEqual( @@ -317,10 +311,10 @@ class TestAccountAPI(UserAPITestCase): self.assertEqual(expected_entries, len(name_change_info)) return name_change_info - def verify_change_info(change_info, old_name, requester, new_name): + def verify_change_info(change_info, old_name, new_name): self.assertEqual(3, len(change_info)) self.assertEqual(old_name, change_info[0]) - self.assertEqual("Name change requested through account API by {}".format(requester), change_info[1]) + self.assertEqual("Name change requested through account API", change_info[1]) self.assertIsNotNone(change_info[2]) # Verify the new name was also stored. get_response = self.send_get(self.client) @@ -334,27 +328,21 @@ class TestAccountAPI(UserAPITestCase): # First change the name as the user and verify meta information. self.send_patch(self.client, {"name": "Mickey Mouse"}) name_change_info = get_name_change_info(1) - verify_change_info(name_change_info[0], old_name, self.user.username, "Mickey Mouse") + verify_change_info(name_change_info[0], old_name, "Mickey Mouse") - # Now change the name as a different (staff) user and verify meta information. - self.staff_client.login(username=self.staff_user.username, password=TEST_PASSWORD) - self.send_patch(self.staff_client, {"name": "Donald Duck"}) + # Now change the name again and verify meta information. + self.send_patch(self.client, {"name": "Donald Duck"}) name_change_info = get_name_change_info(2) - verify_change_info(name_change_info[0], old_name, self.user.username, "Donald Duck",) - verify_change_info(name_change_info[1], "Mickey Mouse", self.staff_user.username, "Donald Duck") + verify_change_info(name_change_info[0], old_name, "Donald Duck",) + verify_change_info(name_change_info[1], "Mickey Mouse", "Donald Duck") - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - def test_patch_email(self, api_client, user): + def test_patch_email(self): """ - Test that the user (and anyone with an is_staff account) can request an email change through the accounts API. + Test that the user can request an email change through the accounts API. Full testing of the helper method used (do_email_change_request) exists in the package with the code. Here just do minimal smoke testing. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") old_email = self.user.email new_email = "newemail@example.com" self.send_patch(client, {"email": new_email, "goals": "change my email"}) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 358ed3cb36..f5fb2846a3 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -124,8 +124,12 @@ class AccountView(APIView): https://tools.ietf.org/html/rfc7396. The content_type must be "application/merge-patch+json" or else an error response with status code 415 will be returned. """ + # Disallow users with is_staff access from calling patch on any account. + if request.user.username != username: + return Response(status=status.HTTP_404_NOT_FOUND) + try: - AccountView.update_account(request.user, username, request.DATA) + AccountView.update_account(username, request.DATA) except AccountUserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) except AccountUpdateError as err: @@ -134,16 +138,15 @@ class AccountView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) @staticmethod - def update_account(requesting_user, username, update): + def update_account(username, update): """Update the account for the given username. Note: No authorization or permissions checks are done in this method. It is up to the caller - of this method to enforce the contract that this method is only called if - requesting_user.username == username or requesting_user.is_staff == True. + of this method to enforce the contract that this method is only called + by the user with the specified username. Arguments: - requesting_user (User): the user who initiated the request username (string): the username associated with the account to change update (dict): the updated account field values @@ -200,15 +203,14 @@ class AccountView(APIView): raise AccountUpdateError(validation_errors) serializer.save() - # If the name was changed, store information about the change operation. This is outside of the - # serializer so that we can store who requested the change. + # If the name was changed, store information about the change operation. if old_name: meta = existing_user_profile.get_meta() if 'old_names' not in meta: meta['old_names'] = [] meta['old_names'].append([ old_name, - "Name change requested through account API by {0}".format(requesting_user.username), + "Name change requested through account API", datetime.datetime.now(UTC).isoformat() ]) existing_user_profile.set_meta(meta)