From 7fc07789ed2dc1a0d276d812e0b49f96f094f8fb Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Thu, 21 Feb 2019 22:37:07 -0500 Subject: [PATCH 01/20] Adds username replacement API New API replaces all copies of username in LMS a desired username. Requires user be in username_replacement_admin group. Should only be ran as a larger job to update usernames across all services, otherwise the system will be left in a broken state for those users. --- .../user_api/accounts/permissions.py | 9 + .../djangoapps/user_api/accounts/views.py | 161 +++++++++++++++++- openedx/core/djangoapps/user_api/urls.py | 8 +- 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index fd7b197f9c..78eb36f271 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -5,6 +5,7 @@ from __future__ import unicode_literals from rest_framework import permissions +USERNAME_REPLACEMENT_GROUP = "username_replacement_admin" class CanDeactivateUser(permissions.BasePermission): """ @@ -23,3 +24,11 @@ class CanRetireUser(permissions.BasePermission): """ def has_permission(self, request, view): return request.user.has_perm('accounts.can_retire_user') + +class CanReplaceUsername(permissions.BasePermission): + """ + Grants access to the Username Replacement API for anyone in the group, + including the service user. + """ + def has_permission(self, request, view): + return request.user.groups.filter(name=USERNAME_REPLACEMENT_GROUP).exists() diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index d3a6a8457a..37866701da 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -7,9 +7,11 @@ https://openedx.atlassian.net/wiki/display/TNL/User+API import datetime import logging from functools import wraps +import uuid import pytz from consent.models import DataSharingConsent +from django.apps import apps from django.conf import settings from django.contrib.auth import authenticate, get_user_model, logout from django.contrib.sites.models import Site @@ -27,6 +29,7 @@ from rest_framework import permissions, status from rest_framework.authentication import SessionAuthentication from rest_framework.parsers import JSONParser from rest_framework.response import Response +from rest_framework.serializers import ValidationError from rest_framework.views import APIView from rest_framework.viewsets import ViewSet from six import iteritems, text_type @@ -71,7 +74,7 @@ from ..models import ( UserRetirementStatus ) from .api import get_account_settings, update_account_settings -from .permissions import CanDeactivateUser, CanRetireUser +from .permissions import CanDeactivateUser, CanReplaceUsername, CanRetireUser from .serializers import UserRetirementPartnerReportSerializer, UserRetirementStatusSerializer from .signals import ( USER_RETIRE_LMS_CRITICAL, @@ -1002,3 +1005,159 @@ class AccountRetirementView(ViewSet): """ for entitlement in CourseEntitlement.objects.filter(user_id=user.id): entitlement.courseentitlementsupportdetail_set.all().update(comments='') + +class UsernameReplacementView(APIView): + """ + WARNING: This API is only meant to be used as part of a larger job that + updates usernames across all services. DO NOT run this alone or users will + not match across the system and things will be broken. + + API will recieve a list of current usernames and their requested new + username. If their new username is taken, it will randomly assign a new username. + """ + authentication_classes = (JwtAuthentication, ) + permission_classes = (permissions.IsAuthenticated, CanReplaceUsername) + + def post(self, request): + """ + POST /api/user/v1/accounts/replace_usernames/ + { + "username_mappings": [ + {"current_username_1": "desired_username_1"}, + {"current_username_2": "desired_username_2"} + ] + } + + **POST Parameters** + + A POST request must include the following parameter. + + * username_mappings: Required. A list of objects that map the current username (key) + to the desired username (value) + + **POST Response Values** + + As long as data validation passes, the request will return a 200 with a new mapping + of old usernames (key) to new username (value) + + { + "successful_replacements": [ + {"old_username_1": "new_username_1"} + ], + "failed_replacements": [ + {"old_username_2": "new_username_2"} + ] + } + + TODO: Determine if we need an audit trail outside of logging and API response. + """ + + # (model_name, column_name) + MODELS_WITH_USERNAME = ( + ('auth.user', 'username'), + ('consent.DataSharingConsent', 'username'), + ('consent.HistoricalDataSharingConsent', 'username'), + ('credit.CreditEligibility', 'username'), + ('credit.CreditRequest', 'username'), + ('credit.CreditRequirementStatus', 'username'), + ('user_api.UserRetirementPartnerReportingStatus', 'original_username'), + ('user_api.UserRetirementStatus', 'original_username') + ) + UNIQUE_SUFFIX_LENGTH = getattr(settings, 'SOCIAL_AUTH_UUID_LENGTH', 4) + + username_mappings = request.data.get("username_mappings") + replacement_locations = self._load_models(MODELS_WITH_USERNAME) + + if not self._has_valid_schema(username_mappings): + raise ValidationError("Request data does not match schema") + + successful_replacements, failed_replacements = [], [] + + for username_pair in username_mappings: + current_username = list(username_pair.keys())[0] + desired_username = list(username_pair.values())[0] + new_username = self._generate_unique_username(desired_username, suffix_length=UNIQUE_SUFFIX_LENGTH) + successfully_replaced = self._replace_username_for_all_models( + current_username, + new_username, + replacement_locations + ) + if successfully_replaced: + successful_replacements.append({current_username: new_username}) + else: + failed_replacements.append({current_username: new_username}) + return Response( + status=status.HTTP_200_OK, + data={ + "successful_replacements": successful_replacements, + "failed_replacements": failed_replacements + } + ) + + def _load_models(self, models_with_fields): + """ Takes tuples that contain a model path and returns the list with a loaded version of the model """ + try: + replacement_locations = [(apps.get_model(model), column) for (model, column) in models_with_fields] + except LookupError: + log.exception("Unable to load models for username replacement") + raise + return replacement_locations + + def _has_valid_schema(self, post_data): + """ Verifies the data is a list of objects with a single key:value pair """ + if not isinstance(post_data, list): + return False + for obj in post_data: + if not (isinstance(obj, dict) and len(obj) == 1): + return False + return True + + def _generate_unique_username(self, desired_username, suffix_length=4): + """ Accepts a username and returns a unique username if the requested is taken """ + User = apps.get_model('auth.user') + new_username = desired_username + # Keep checking usernames in case desired_username + random suffix is already taken + while True: + if User.objects.filter(username=new_username).exists(): + unique_suffix = uuid.uuid4().hex[:suffix_length] + new_username = desired_username + unique_suffix + else: + break + return new_username + + def _replace_username_for_all_models(self, current_username, new_username, replacement_locations): + """ + Replaces current_username with new_username for all (model, column) pairs in replacement locations. + Returns if it was successful or not. Will return successful even if no matching + + TODO: Determine if logs of username are a PII issue. + """ + try: + with transaction.atomic(): + num_rows_changed = 0 + for (model, column) in replacement_locations: + num_rows_changed += model.objects.filter( + **{column: current_username} + ).update( + **{column: new_username} + ) + except Exception as exc: + log.exception("Unable to change username from {current} to {new}. Reason: {error}".format( + current=current_username, + new=new_username, + error=exc + )) + return False + if num_rows_changed == 0: + log.warning("Unable to change username from {current} to {new} because {current} doesn't exist.".format( + current=current_username, + new=new_username, + )) + return False + + log.info("Successfully changed username from {current} to {new}.".format( + current=current_username, + new=new_username, + )) + return True + diff --git a/openedx/core/djangoapps/user_api/urls.py b/openedx/core/djangoapps/user_api/urls.py index bdea5668ca..334ffc7e8c 100644 --- a/openedx/core/djangoapps/user_api/urls.py +++ b/openedx/core/djangoapps/user_api/urls.py @@ -13,7 +13,8 @@ from .accounts.views import ( AccountRetirementView, AccountViewSet, DeactivateLogoutView, - LMSAccountRetirementView + LMSAccountRetirementView, + UsernameReplacementView ) from .preferences.views import PreferencesDetailView, PreferencesView from .verification_api.views import IDVerificationStatusView @@ -150,6 +151,11 @@ urlpatterns = [ RETIREMENT_UPDATE, name='accounts_retirement_update' ), + url( + r'^v1/accounts/replace_usernames/$', + UsernameReplacementView.as_view(), + name='username_replacement' + ), url( r'^v1/validation/registration$', RegistrationValidationView.as_view(), From 5a0659ed9e4f8085009c04ade4f66cbd5d3c94bd Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Tue, 26 Feb 2019 14:19:44 -0500 Subject: [PATCH 02/20] Replace group with static username --- openedx/core/djangoapps/user_api/accounts/permissions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index 78eb36f271..a7f09ea9a7 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -3,6 +3,7 @@ Permissions classes for User accounts API views. """ from __future__ import unicode_literals +from django.conf import settings from rest_framework import permissions USERNAME_REPLACEMENT_GROUP = "username_replacement_admin" @@ -31,4 +32,4 @@ class CanReplaceUsername(permissions.BasePermission): including the service user. """ def has_permission(self, request, view): - return request.user.groups.filter(name=USERNAME_REPLACEMENT_GROUP).exists() + return request.user.username == getattr(settings, "USERNAME_REPLACEMENT_WORKER") From eec95cf538d18a8f14a1516c82882962626762e8 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Thu, 28 Feb 2019 16:35:09 -0500 Subject: [PATCH 03/20] Add discussion api --- .../discussion_api/tests/test_views.py | 76 +++++++++++++++++++ lms/djangoapps/discussion_api/tests/utils.py | 9 +++ lms/djangoapps/discussion_api/urls.py | 2 + lms/djangoapps/discussion_api/views.py | 54 ++++++++++++- lms/envs/common.py | 2 + lms/lib/comment_client/user.py | 17 +++++ .../user_api/accounts/permissions.py | 2 - .../djangoapps/user_api/accounts/views.py | 3 + 8 files changed, 162 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index aafad0df20..1c4a8bdddc 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -10,6 +10,7 @@ from urlparse import urlparse import ddt import httpretty import mock +from django.conf import settings from django.urls import reverse from edx_oauth2_provider.tests.factories import ClientFactory, AccessTokenFactory from opaque_keys.edx.keys import CourseKey @@ -235,6 +236,81 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """ pass +@httpretty.activate +@mock.patch('django.conf.settings.USERNAME_REPLACEMENT_WORKER', 'test_replace_username_service_worker') +@mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) +class ReplaceUsernameViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): + """Tests for ReplaceUsernameView""" + def setUp(self): + super(ReplaceUsernameViewTest, self).setUp() + self.client_user = UserFactory() + self.client_user.username = "test_replace_username_service_worker" + self.new_username = "test_username_replacement" + self.url = reverse("replace_discussion_username") + + def assert_response_correct(self, response, expected_status, expected_content): + """ + Assert that the response has the given status code and content + """ + self.assertEqual(response.status_code, expected_status) + + if expected_content: + self.assertEqual(text_type(response.content), expected_content) + + def build_jwt_headers(self, user): + """ + Helper function for creating headers for the JWT authentication. + """ + token = create_jwt_for_user(user) + headers = {'HTTP_AUTHORIZATION': 'JWT ' + token} + return headers + + def test_missing_params(self): + headers = self.build_jwt_headers(self.client_user) + # Using this instead of ddt so we can access self.user + bad_post_contents = ( + {}, + {"current_username": self.user.username}, + {"new_username": "test_username_replacement"}, + ) + for data in bad_post_contents: + response = self.client.post(self.url, data, **headers) + self.assert_response_correct(response, 400, "") + + def test_basic(self): + """ Check successful replacement """ + self.register_get_username_replacement_response(self.user) + headers = self.build_jwt_headers(self.client_user) + data = {"current_username": self.user.username, "new_username": self.new_username} + response = self.client.post(self.url, data, **headers) + self.assert_response_correct(response, 204, "") + + def test_nonexistant_user(self): + self.register_get_username_replacement_response(self.user) + headers = self.build_jwt_headers(self.client_user) + data = {"current_username": "non-existant-user", "new_username": self.new_username} + response = self.client.post(self.url, data, **headers) + self.assert_response_correct(response, 404, "") + + def test_client_404(self): + self.register_get_username_replacement_response(self.user, status=404) + headers = self.build_jwt_headers(self.client_user) + data = {"current_username": self.user.username, "new_username": self.new_username} + response = self.client.post(self.url, data, **headers) + self.assert_response_correct(response, 404, "") + + def test_client_500(self): + self.register_get_username_replacement_response(self.user, status=500) + headers = self.build_jwt_headers(self.client_user) + data = {"current_username": self.user.username, "new_username": self.new_username} + response = self.client.post(self.url, data, **headers) + self.assert_response_correct(response, 500, "") + + def test_not_authenticated(self): + """ + Override the parent implementation of this, we JWT auth for this API + """ + pass @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 39902d2669..4641ccc807 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -226,6 +226,15 @@ class CommentsServiceMockMixin(object): status=status ) + def register_get_username_replacement_response(self, user, status=200, body=""): + assert httpretty.is_enabled(), 'httpretty must be enabled to mock calls.' + httpretty.register_uri( + httpretty.POST, + "http://localhost:4567/api/v1/users/{id}/replace_username".format(id=user.id), + body=body, + status=status + ) + def register_subscribed_threads_response(self, user, threads, page, num_pages): """Register a mock response for GET on the CS user instance endpoint""" assert httpretty.is_enabled(), 'httpretty must be enabled to mock calls.' diff --git a/lms/djangoapps/discussion_api/urls.py b/lms/djangoapps/discussion_api/urls.py index f138bfcf41..b154f2fbb0 100644 --- a/lms/djangoapps/discussion_api/urls.py +++ b/lms/djangoapps/discussion_api/urls.py @@ -13,6 +13,7 @@ from discussion_api.views import ( CourseView, ThreadViewSet, RetireUserView, + ReplaceUsernameView, ) ROUTER = SimpleRouter() @@ -40,6 +41,7 @@ urlpatterns = [ name="discussion_course" ), url(r"^v1/accounts/retire_forum", RetireUserView.as_view(), name="retire_discussion_user"), + url(r"^v1/accounts/replace_username", ReplaceUsernameView.as_view(), name="replace_discussion_username"), url( r"^v1/course_topics/{}".format(settings.COURSE_ID_PATTERN), CourseTopicsView.as_view(), diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 872897ab20..d43ab98f3e 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -1,6 +1,7 @@ """ Discussion API views """ +from django.contrib.auth.models import User from django.core.exceptions import ValidationError from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser @@ -49,7 +50,7 @@ from discussion_api.serializers import ( from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser from openedx.core.lib.api.parsers import MergePatchParser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes -from openedx.core.djangoapps.user_api.accounts.permissions import CanRetireUser +from openedx.core.djangoapps.user_api.accounts.permissions import CanReplaceUsername, CanRetireUser from openedx.core.djangoapps.user_api.models import UserRetirementStatus from util.json_request import JsonResponse from xmodule.modulestore.django import modulestore @@ -587,6 +588,57 @@ class RetireUserView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) +class ReplaceUsernameView(APIView): + """ + A request from the settings.USERNAME_REPLACEMENT_WORKER user can replace + the username for a user. This will change their username and update all of + their comments to have the new username + + POST /api/discussion/v1/accounts/replace_username/ + { + "current_username": "users_current_username", + "new_username": "name_to_change_it_to" + } + + Returns empty response if successful + """ + + authentication_classes = (JwtAuthentication,) + permission_classes = (permissions.IsAuthenticated, CanReplaceUsername) + + def post(self, request): + """ + Implements the username replacement endpoint + """ + current_username = request.data.get("current_username") + new_username = request.data.get("new_username") + + if not (current_username and new_username): + return Response(status=status.HTTP_400_BAD_REQUEST) + + try: + current_user = User.objects.get(username=current_username) + cc_user = comment_client.User.from_django_user(current_user) + cc_user.replace_username(new_username) + except User.DoesNotExist: + return Response(status=status.HTTP_404_NOT_FOUND) + except comment_client.CommentClientRequestError as exc: + if exc.status_code == 404: + return Response(status=status.HTTP_404_NOT_FOUND) + raise + except Exception as exc: + return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) + return Response(status=status.HTTP_204_NO_CONTENT) + + + + + + + + + + class CourseDiscussionSettingsAPIView(DeveloperErrorViewMixin, APIView): """ diff --git a/lms/envs/common.py b/lms/envs/common.py index 5688bf9a0c..af88b564b0 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3450,6 +3450,8 @@ RETIREMENT_STATES = [ 'COMPLETE', ] +USERNAME_REPLACEMENT_WORKER = "REPLACE WITH VALID USERNAME" + ############## Settings for Microfrontends ######################### # If running a Gradebook container locally, # modify lms/envs/private.py to give it a non-null value diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index d358f09c40..d854d2848b 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -180,6 +180,17 @@ class User(models.Model): metric_tags=self._metric_tags ) + def replace_username(self, new_username): + url = _url_for_username_replacement(self.id) + params = {"new_username": new_username} + + utils.perform_request( + 'post', + url, + params, + raw=True, + ) + def _url_for_vote_comment(comment_id): return "{prefix}/comments/{comment_id}/votes".format(prefix=settings.PREFIX, comment_id=comment_id) @@ -213,3 +224,9 @@ def _url_for_retire(user_id): Returns cs_comments_service url endpoint to retire a user (remove all post content, etc.) """ return "{prefix}/users/{user_id}/retire".format(prefix=settings.PREFIX, user_id=user_id) + +def _url_for_username_replacement(user_id): + """ + Returns cs_comments_servuce url endpoint to replace the username of a user + """ + return "{prefix}/users/{user_id}/replace_username".format(prefix=settings.PREFIX, user_id=user_id) diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index a7f09ea9a7..84d4155011 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -6,8 +6,6 @@ from __future__ import unicode_literals from django.conf import settings from rest_framework import permissions -USERNAME_REPLACEMENT_GROUP = "username_replacement_admin" - class CanDeactivateUser(permissions.BasePermission): """ Grants access to AccountDeactivationView if the requesting user is a superuser diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 37866701da..d32c214448 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1014,6 +1014,9 @@ class UsernameReplacementView(APIView): API will recieve a list of current usernames and their requested new username. If their new username is taken, it will randomly assign a new username. + + This API will be called first, before calling the APIs in other services as this + one handles the checks on the usernames provided. """ authentication_classes = (JwtAuthentication, ) permission_classes = (permissions.IsAuthenticated, CanReplaceUsername) From 1cd727762ddf0fae359b479eca6193b86ef09ed1 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Fri, 8 Mar 2019 16:42:34 -0500 Subject: [PATCH 04/20] fix quality --- lms/djangoapps/discussion_api/tests/test_views.py | 2 ++ lms/djangoapps/discussion_api/views.py | 10 +--------- lms/lib/comment_client/user.py | 1 + .../core/djangoapps/user_api/accounts/permissions.py | 2 ++ openedx/core/djangoapps/user_api/accounts/views.py | 2 +- 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 1c4a8bdddc..6a50baaa08 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -236,6 +236,7 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """ pass + @httpretty.activate @mock.patch('django.conf.settings.USERNAME_REPLACEMENT_WORKER', 'test_replace_username_service_worker') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -312,6 +313,7 @@ class ReplaceUsernameViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """ pass + @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index d43ab98f3e..7af8820071 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -588,6 +588,7 @@ class RetireUserView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) + class ReplaceUsernameView(APIView): """ A request from the settings.USERNAME_REPLACEMENT_WORKER user can replace @@ -631,15 +632,6 @@ class ReplaceUsernameView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) - - - - - - - - - class CourseDiscussionSettingsAPIView(DeveloperErrorViewMixin, APIView): """ **Use Cases** diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index d854d2848b..a007bd1d75 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -225,6 +225,7 @@ def _url_for_retire(user_id): """ return "{prefix}/users/{user_id}/retire".format(prefix=settings.PREFIX, user_id=user_id) + def _url_for_username_replacement(user_id): """ Returns cs_comments_servuce url endpoint to replace the username of a user diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index 84d4155011..b402478f6a 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -6,6 +6,7 @@ from __future__ import unicode_literals from django.conf import settings from rest_framework import permissions + class CanDeactivateUser(permissions.BasePermission): """ Grants access to AccountDeactivationView if the requesting user is a superuser @@ -24,6 +25,7 @@ class CanRetireUser(permissions.BasePermission): def has_permission(self, request, view): return request.user.has_perm('accounts.can_retire_user') + class CanReplaceUsername(permissions.BasePermission): """ Grants access to the Username Replacement API for anyone in the group, diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index d32c214448..8f243ac863 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1006,6 +1006,7 @@ class AccountRetirementView(ViewSet): for entitlement in CourseEntitlement.objects.filter(user_id=user.id): entitlement.courseentitlementsupportdetail_set.all().update(comments='') + class UsernameReplacementView(APIView): """ WARNING: This API is only meant to be used as part of a larger job that @@ -1163,4 +1164,3 @@ class UsernameReplacementView(APIView): new=new_username, )) return True - From d19536e7962896d01ab5e133c6ab8a052adcb96e Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Mon, 11 Mar 2019 10:47:06 -0400 Subject: [PATCH 05/20] qiality --- lms/djangoapps/discussion_api/tests/test_views.py | 1 - lms/djangoapps/discussion_api/views.py | 2 +- openedx/core/djangoapps/user_api/accounts/views.py | 3 +-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 6a50baaa08..c5ee2eeb59 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -10,7 +10,6 @@ from urlparse import urlparse import ddt import httpretty import mock -from django.conf import settings from django.urls import reverse from edx_oauth2_provider.tests.factories import ClientFactory, AccessTokenFactory from opaque_keys.edx.keys import CourseKey diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 7af8820071..316c0e22b5 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -627,7 +627,7 @@ class ReplaceUsernameView(APIView): if exc.status_code == 404: return Response(status=status.HTTP_404_NOT_FOUND) raise - except Exception as exc: + except Exception as exc: #pylint disable-broad-except return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 8f243ac863..94913343eb 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1118,7 +1118,6 @@ class UsernameReplacementView(APIView): def _generate_unique_username(self, desired_username, suffix_length=4): """ Accepts a username and returns a unique username if the requested is taken """ - User = apps.get_model('auth.user') new_username = desired_username # Keep checking usernames in case desired_username + random suffix is already taken while True: @@ -1145,7 +1144,7 @@ class UsernameReplacementView(APIView): ).update( **{column: new_username} ) - except Exception as exc: + except Exception as exc: #pylint: disable-broad-except log.exception("Unable to change username from {current} to {new}. Reason: {error}".format( current=current_username, new=new_username, From 7458cd8820df795dd57b50f9fd52443187f86029 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Mon, 11 Mar 2019 11:21:19 -0400 Subject: [PATCH 06/20] quality --- lms/djangoapps/discussion_api/views.py | 2 +- openedx/core/djangoapps/user_api/accounts/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 316c0e22b5..0a23b61777 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -627,7 +627,7 @@ class ReplaceUsernameView(APIView): if exc.status_code == 404: return Response(status=status.HTTP_404_NOT_FOUND) raise - except Exception as exc: #pylint disable-broad-except + except Exception as exc: # pylint disable-broad-except return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 94913343eb..b90e92d02a 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1144,7 +1144,7 @@ class UsernameReplacementView(APIView): ).update( **{column: new_username} ) - except Exception as exc: #pylint: disable-broad-except + except Exception as exc: # pylint: disable-broad-except log.exception("Unable to change username from {current} to {new}. Reason: {error}".format( current=current_username, new=new_username, From 58d6213939d953e82a6614dc108328d4b8d44198 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Mon, 11 Mar 2019 12:54:45 -0400 Subject: [PATCH 07/20] quality --- lms/djangoapps/discussion_api/views.py | 2 +- .../djangoapps/user_api/accounts/views.py | 30 +++++++++++-------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 0a23b61777..cc9b32d5e9 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -627,7 +627,7 @@ class ReplaceUsernameView(APIView): if exc.status_code == 404: return Response(status=status.HTTP_404_NOT_FOUND) raise - except Exception as exc: # pylint disable-broad-except + except Exception as exc: # pylint: disable=broad-except return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index b90e92d02a..9c84eea431 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1145,21 +1145,25 @@ class UsernameReplacementView(APIView): **{column: new_username} ) except Exception as exc: # pylint: disable-broad-except - log.exception("Unable to change username from {current} to {new}. Reason: {error}".format( - current=current_username, - new=new_username, - error=exc - )) + log.exception( + "Unable to change username from %s to %s. Reason: %s", + current_username, + new_username, + exc + ) return False if num_rows_changed == 0: - log.warning("Unable to change username from {current} to {new} because {current} doesn't exist.".format( - current=current_username, - new=new_username, - )) + log.warning( + "Unable to change username from %s to %s because %s doesn't exist.", + current_username, + new_username, + current_username, + ) return False - log.info("Successfully changed username from {current} to {new}.".format( - current=current_username, - new=new_username, - )) + log.info( + "Successfully changed username from %s to %s.", + current_username, + new_username, + ) return True From 906faba8fb3e51b2321d80124fb354e64cba6138 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Mon, 11 Mar 2019 13:34:23 -0400 Subject: [PATCH 08/20] quality --- openedx/core/djangoapps/user_api/accounts/permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index b402478f6a..9092ca002c 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -32,4 +32,4 @@ class CanReplaceUsername(permissions.BasePermission): including the service user. """ def has_permission(self, request, view): - return request.user.username == getattr(settings, "USERNAME_REPLACEMENT_WORKER") + return request.user.username == getattr(settings, "USERNAME_REPLACEMENT_WORKER", False) From cbe466fe61d58cfcfb90ab042667092a70e50278 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Mon, 11 Mar 2019 14:56:39 -0400 Subject: [PATCH 09/20] quality --- openedx/core/djangoapps/user_api/accounts/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 9c84eea431..a24fa22c9a 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1144,9 +1144,9 @@ class UsernameReplacementView(APIView): ).update( **{column: new_username} ) - except Exception as exc: # pylint: disable-broad-except + except Exception as exc: # pylint: disable=broad-except log.exception( - "Unable to change username from %s to %s. Reason: %s", + u"Unable to change username from %s to %s. Reason: %s", current_username, new_username, exc @@ -1154,7 +1154,7 @@ class UsernameReplacementView(APIView): return False if num_rows_changed == 0: log.warning( - "Unable to change username from %s to %s because %s doesn't exist.", + u"Unable to change username from %s to %s because %s doesn't exist.", current_username, new_username, current_username, @@ -1162,7 +1162,7 @@ class UsernameReplacementView(APIView): return False log.info( - "Successfully changed username from %s to %s.", + u"Successfully changed username from %s to %s.", current_username, new_username, ) From ab74bb4c97e7ab389497a8b88a0890ed518988d9 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 13 Mar 2019 15:59:28 -0400 Subject: [PATCH 10/20] Make discussion API bulk friendly --- .../discussion_api/tests/test_views.py | 87 ++++++++++------- lms/djangoapps/discussion_api/urls.py | 4 +- lms/djangoapps/discussion_api/views.py | 93 +++++++++++++------ 3 files changed, 119 insertions(+), 65 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index c5ee2eeb59..c350ed5d63 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -236,13 +236,14 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): pass +@ddt.ddt @httpretty.activate @mock.patch('django.conf.settings.USERNAME_REPLACEMENT_WORKER', 'test_replace_username_service_worker') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class ReplaceUsernameViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): - """Tests for ReplaceUsernameView""" +class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): + """Tests for ReplaceUsernamesView""" def setUp(self): - super(ReplaceUsernameViewTest, self).setUp() + super(ReplaceUsernamesViewTest, self).setUp() self.client_user = UserFactory() self.client_user.username = "test_replace_username_service_worker" self.new_username = "test_username_replacement" @@ -265,46 +266,60 @@ class ReplaceUsernameViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): headers = {'HTTP_AUTHORIZATION': 'JWT ' + token} return headers - def test_missing_params(self): + @ddt.data( + [{}, {}], + {}, + [{"test_key": "test_value", "test_key_2": "test_value_2"}] + ) + def test_bad_schema(self, mapping_data): + """ Verify the endpoint rejects bad data schema """ headers = self.build_jwt_headers(self.client_user) - # Using this instead of ddt so we can access self.user - bad_post_contents = ( - {}, - {"current_username": self.user.username}, - {"new_username": "test_username_replacement"}, - ) - for data in bad_post_contents: - response = self.client.post(self.url, data, **headers) - self.assert_response_correct(response, 400, "") + data = { + "username_mappings": mapping_data + } + response = self.client.post(self.url, data, **headers) + self.assertEqual(response.status_code, 400) + + def test_auth(self): + """ Verify the endpoint only works with the service worker """ + data = { + "username_mappings": [ + {"test_username_1": "test_new_username_1"}, + {"test_username_2": "test_new_username_2"} + ] + } + + # Test unauthenticated + response = self.client.post(self.url, data) + self.assertEqual(response.status_code, 401) + + # Test non-service worker + random_user = UserFactory() + headers = self.build_jwt_headers(random_user) + response = self.client.post(self.url, data, **headers) + self.assertEqual(response.status_code, 403) + + # Test service worker + headers = self.build_jwt_headers(self.service_user) + response = self.client.post(self.url, data, **headers) + self.assertEqual(response.status_code, 200) def test_basic(self): """ Check successful replacement """ + data = { + "username_mappings": [ + {self.user.username: self.new_username}, + ] + } + expected_response = { + 'failed_replacements': [], + 'successful_replacements': data["username_mappings"] + } self.register_get_username_replacement_response(self.user) headers = self.build_jwt_headers(self.client_user) - data = {"current_username": self.user.username, "new_username": self.new_username} response = self.client.post(self.url, data, **headers) - self.assert_response_correct(response, 204, "") - - def test_nonexistant_user(self): - self.register_get_username_replacement_response(self.user) - headers = self.build_jwt_headers(self.client_user) - data = {"current_username": "non-existant-user", "new_username": self.new_username} - response = self.client.post(self.url, data, **headers) - self.assert_response_correct(response, 404, "") - - def test_client_404(self): - self.register_get_username_replacement_response(self.user, status=404) - headers = self.build_jwt_headers(self.client_user) - data = {"current_username": self.user.username, "new_username": self.new_username} - response = self.client.post(self.url, data, **headers) - self.assert_response_correct(response, 404, "") - - def test_client_500(self): - self.register_get_username_replacement_response(self.user, status=500) - headers = self.build_jwt_headers(self.client_user) - data = {"current_username": self.user.username, "new_username": self.new_username} - response = self.client.post(self.url, data, **headers) - self.assert_response_correct(response, 500, "") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, expected_response) def test_not_authenticated(self): """ diff --git a/lms/djangoapps/discussion_api/urls.py b/lms/djangoapps/discussion_api/urls.py index b154f2fbb0..76a6f156e3 100644 --- a/lms/djangoapps/discussion_api/urls.py +++ b/lms/djangoapps/discussion_api/urls.py @@ -13,7 +13,7 @@ from discussion_api.views import ( CourseView, ThreadViewSet, RetireUserView, - ReplaceUsernameView, + ReplaceUsernamesView, ) ROUTER = SimpleRouter() @@ -41,7 +41,7 @@ urlpatterns = [ name="discussion_course" ), url(r"^v1/accounts/retire_forum", RetireUserView.as_view(), name="retire_discussion_user"), - url(r"^v1/accounts/replace_username", ReplaceUsernameView.as_view(), name="replace_discussion_username"), + url(r"^v1/accounts/replace_username", ReplaceUsernamesView.as_view(), name="replace_discussion_username"), url( r"^v1/course_topics/{}".format(settings.COURSE_ID_PATTERN), CourseTopicsView.as_view(), diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index cc9b32d5e9..50bed02046 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -1,6 +1,8 @@ """ Discussion API views """ +import logging + from django.contrib.auth.models import User from django.core.exceptions import ValidationError from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication @@ -55,6 +57,8 @@ from openedx.core.djangoapps.user_api.models import UserRetirementStatus from util.json_request import JsonResponse from xmodule.modulestore.django import modulestore +log = logging.getLogger(__name__) + @view_auth_classes() class CourseView(DeveloperErrorViewMixin, APIView): @@ -589,19 +593,21 @@ class RetireUserView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) -class ReplaceUsernameView(APIView): +class ReplaceUsernamesView(APIView): """ - A request from the settings.USERNAME_REPLACEMENT_WORKER user can replace - the username for a user. This will change their username and update all of - their comments to have the new username + WARNING: This API is only meant to be used as part of a larger job that + updates usernames across all services. DO NOT run this alone or users will + not match across the system and things will be broken. - POST /api/discussion/v1/accounts/replace_username/ - { - "current_username": "users_current_username", - "new_username": "name_to_change_it_to" - } + API will recieve a list of current usernames and their new username. - Returns empty response if successful + POST /api/discussion/v1/accounts/replace_usernames/ + { + "username_mappings": [ + {"current_username_1": "desired_username_1"}, + {"current_username_2": "desired_username_2"} + ] + } """ authentication_classes = (JwtAuthentication,) @@ -611,26 +617,59 @@ class ReplaceUsernameView(APIView): """ Implements the username replacement endpoint """ - current_username = request.data.get("current_username") - new_username = request.data.get("new_username") - if not (current_username and new_username): - return Response(status=status.HTTP_400_BAD_REQUEST) + username_mappings = request.data.get("username_mappings") - try: - current_user = User.objects.get(username=current_username) - cc_user = comment_client.User.from_django_user(current_user) - cc_user.replace_username(new_username) - except User.DoesNotExist: - return Response(status=status.HTTP_404_NOT_FOUND) - except comment_client.CommentClientRequestError as exc: - if exc.status_code == 404: - return Response(status=status.HTTP_404_NOT_FOUND) - raise - except Exception as exc: # pylint: disable=broad-except - return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR) - return Response(status=status.HTTP_204_NO_CONTENT) + if not self._has_valid_schema(username_mappings): + raise ValidationError("Request data does not match schema") + successful_replacements, failed_replacements = [], [] + + for username_pair in username_mappings: + current_username = list(username_pair.keys())[0] + new_username = list(username_pair.values())[0] + try: + # This API will be called after the regular LMS API, so the username in + # the DB will have already been updated to new_username + current_user = User.objects.get(username=new_username) + cc_user = comment_client.User.from_django_user(current_user) + cc_user.replace_username(new_username) + except User.DoesNotExist: + log.warning( + u"Unable to change username from %s to %s in forums because %s doesn't exist in LMS DB.", + current_username, + new_username, + new_username, + ) + failed_replacements += {current_username: new_username} + except comment_client.CommentClientRequestError as exc: + log.exception( + u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", + current_username, + new_username, + exc, + ) + failed_replacements += {current_username: new_username} + log.info( + u"Successfully changed username from %s to %s in forums.", + current_username, + new_username, + ) + successful_replacements += {current_username: new_username} + + return JsonResponse({ + "successful_replacements": successful_replacements, + "failed_replacements": failed_replacements, + }) + + def _has_valid_schema(self, post_data): + """ Verifies the data is a list of objects with a single key:value pair """ + if not isinstance(post_data, list): + return False + for obj in post_data: + if not (isinstance(obj, dict) and len(obj) == 1): + return False + return True class CourseDiscussionSettingsAPIView(DeveloperErrorViewMixin, APIView): """ From 26a4093cb55b0d75e8f69bec79f1d9914391095b Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 13 Mar 2019 16:17:10 -0400 Subject: [PATCH 11/20] Add tests to User API --- .../user_api/accounts/tests/test_views.py | 79 +++++++++++++++++++ .../djangoapps/user_api/accounts/views.py | 15 ++-- 2 files changed, 86 insertions(+), 8 deletions(-) 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 b0a66dcbc0..c4b4c0a305 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -17,6 +17,7 @@ import mock import pytz from rest_framework.test import APIClient, APITestCase +from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KEY from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangoapps.user_api.preferences.api import set_user_preference @@ -932,3 +933,81 @@ class TestAccountAPITransactions(TransactionTestCase): data = response.data self.assertEqual(old_email, data["email"]) self.assertEqual(u"m", data["gender"]) + + +@ddt.ddt +@mock.patch('django.conf.settings.USERNAME_REPLACEMENT_WORKER', 'test_replace_username_service_worker') +class UsernameReplacementViewTests(APITestCase): + """ Tests UsernameReplacementView """ + SERVICE_USERNAME = 'test_replace_username_service_worker' + + def setUp(self): + super(UsernameReplacementViewTests, self).setUp() + self.service_user = UserFactory(username=self.SERVICE_USERNAME) + self.url = reverse("username_replacement") + + def build_jwt_headers(self, user): + """ + Helper function for creating headers for the JWT authentication. + """ + token = create_jwt_for_user(user) + headers = {'HTTP_AUTHORIZATION': token} + return headers + + def call_api(self, user, data): + """ Helper function to call API with data """ + data = json.dumps(data) + headers = self.build_jwt_headers(user) + return self.client.post(self.url, data, content_type='application/json', **headers) + + def test_auth(self): + """ Verify the endpoint only works with the service worker """ + data = { + "username_mappings": [ + {"test_username_1": "test_new_username_1"}, + {"test_username_2": "test_new_username_2"} + ] + } + + # Test unauthenticated + response = self.client.post(self.url) + self.assertEqual(response.status_code, 401) + + # Test non-service worker + random_user = UserFactory() + response = self.call_api(random_user, data) + self.assertEqual(response.status_code, 403) + + # Test service worker + response = self.call_api(self.service_user, data) + self.assertEqual(response.status_code, 200) + + @ddt.data( + [{}, {}], + {}, + [{"test_key": "test_value", "test_key_2": "test_value_2"}] + ) + def test_bad_schema(self, mapping_data): + """ Verify the endpoint rejects bad data schema """ + data = { + "username_mappings": mapping_data + } + response = self.call_api(self.service_user, data) + self.assertEqual(response.status_code, 400) + + def test_existing_and_non_existing_users(self): + """ Tests a mix of existing and non existing users """ + random_users = [UserFactory() for _ in range(5)] + fake_usernames = ["myname_" + str(x) for x in range(5)] + existing_users = [{user.username: user.username + '_new'} for user in random_users] + non_existing_users = [{username: username + '_new'} for username in fake_usernames] + data = { + "username_mappings": existing_users + non_existing_users + } + expected_response = { + 'failed_replacements': [], + 'successful_replacements': existing_users + non_existing_users + } + response = self.call_api(self.service_user, data) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, expected_response) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index a24fa22c9a..b6d7b28580 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1153,17 +1153,16 @@ class UsernameReplacementView(APIView): ) return False if num_rows_changed == 0: - log.warning( + log.info( u"Unable to change username from %s to %s because %s doesn't exist.", current_username, new_username, current_username, ) - return False - - log.info( - u"Successfully changed username from %s to %s.", - current_username, - new_username, - ) + else: + log.info( + u"Successfully changed username from %s to %s.", + current_username, + new_username, + ) return True From bb1964f9ef5c5502eb531a21655859b0a6623f9f Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 13 Mar 2019 19:04:46 -0400 Subject: [PATCH 12/20] make discussion api match others --- lms/djangoapps/discussion_api/views.py | 63 ++++++++++++++------------ 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 50bed02046..8af1995270 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -628,40 +628,47 @@ class ReplaceUsernamesView(APIView): for username_pair in username_mappings: current_username = list(username_pair.keys())[0] new_username = list(username_pair.values())[0] - try: - # This API will be called after the regular LMS API, so the username in - # the DB will have already been updated to new_username - current_user = User.objects.get(username=new_username) - cc_user = comment_client.User.from_django_user(current_user) - cc_user.replace_username(new_username) - except User.DoesNotExist: - log.warning( - u"Unable to change username from %s to %s in forums because %s doesn't exist in LMS DB.", - current_username, - new_username, - new_username, - ) + successfully_replaced = self._replace_username(current_username, new_username) + if successfully_replaced: + successful_replacements += {current_username: new_username} + else: failed_replacements += {current_username: new_username} - except comment_client.CommentClientRequestError as exc: - log.exception( - u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", - current_username, - new_username, - exc, - ) - failed_replacements += {current_username: new_username} - log.info( - u"Successfully changed username from %s to %s in forums.", - current_username, - new_username, - ) - successful_replacements += {current_username: new_username} - return JsonResponse({ "successful_replacements": successful_replacements, "failed_replacements": failed_replacements, }) + def _replace_username(self, current_username, new_username): + try: + # This API will be called after the regular LMS API, so the username in + # the DB will have already been updated to new_username + current_user = User.objects.get(username=new_username) + cc_user = comment_client.User.from_django_user(current_user) + cc_user.replace_username(new_username) + except User.DoesNotExist: + log.warning( + u"Unable to change username from %s to %s in forums because %s doesn't exist in LMS DB.", + current_username, + new_username, + new_username, + ) + return False + except comment_client.CommentClientRequestError as exc: + log.exception( + u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", + current_username, + new_username, + exc, + ) + return False + + log.info( + u"Successfully changed username from %s to %s in forums.", + current_username, + new_username, + ) + return True + def _has_valid_schema(self, post_data): """ Verifies the data is a list of objects with a single key:value pair """ if not isinstance(post_data, list): From f224f38a21a0d1e784da6d042095b4bde33216c4 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 13 Mar 2019 19:12:21 -0400 Subject: [PATCH 13/20] testing --- .../discussion_api/tests/test_views.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index c350ed5d63..1d78136ada 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -266,6 +266,12 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): headers = {'HTTP_AUTHORIZATION': 'JWT ' + token} return headers + def call_api(self, user, data): + """ Helper function to call API with data """ + data = json.dumps(data) + headers = self.build_jwt_headers(user) + return self.client.post(self.url, data, content_type='application/json', **headers) + @ddt.data( [{}, {}], {}, @@ -273,11 +279,10 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) def test_bad_schema(self, mapping_data): """ Verify the endpoint rejects bad data schema """ - headers = self.build_jwt_headers(self.client_user) data = { "username_mappings": mapping_data } - response = self.client.post(self.url, data, **headers) + response = self.call_api(self.url, self.client_user, data) self.assertEqual(response.status_code, 400) def test_auth(self): @@ -295,13 +300,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): # Test non-service worker random_user = UserFactory() - headers = self.build_jwt_headers(random_user) - response = self.client.post(self.url, data, **headers) + response = self.call_api(self.url, random_user, data) self.assertEqual(response.status_code, 403) # Test service worker - headers = self.build_jwt_headers(self.service_user) - response = self.client.post(self.url, data, **headers) + response = self.call_api(self.url, self.client_user, data) self.assertEqual(response.status_code, 200) def test_basic(self): @@ -316,8 +319,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): 'successful_replacements': data["username_mappings"] } self.register_get_username_replacement_response(self.user) - headers = self.build_jwt_headers(self.client_user) - response = self.client.post(self.url, data, **headers) + response = self.call_api(self.url, self.client_user, data) self.assertEqual(response.status_code, 200) self.assertEqual(response.data, expected_response) From cda681e2f1d383ee38767fc45197ca47b9207d88 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 13 Mar 2019 23:04:41 -0400 Subject: [PATCH 14/20] fix python tests --- cms/envs/common.py | 1 + .../discussion_api/tests/test_views.py | 8 +++---- lms/djangoapps/discussion_api/views.py | 22 +++++++++++-------- .../user_api/accounts/tests/test_views.py | 2 +- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 2463a9e8a8..d2390996cb 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1361,6 +1361,7 @@ ADVANCED_PROBLEM_TYPES = [ } ] +USERNAME_REPLACEMENT_WORKER = "REPLACE WITH VALID USERNAME" # Files and Uploads type filter values diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 1d78136ada..6c0335c2b9 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -282,7 +282,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): data = { "username_mappings": mapping_data } - response = self.call_api(self.url, self.client_user, data) + response = self.call_api(self.client_user, data) self.assertEqual(response.status_code, 400) def test_auth(self): @@ -300,11 +300,11 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): # Test non-service worker random_user = UserFactory() - response = self.call_api(self.url, random_user, data) + response = self.call_api(random_user, data) self.assertEqual(response.status_code, 403) # Test service worker - response = self.call_api(self.url, self.client_user, data) + response = self.call_api(self.client_user, data) self.assertEqual(response.status_code, 200) def test_basic(self): @@ -319,7 +319,7 @@ class ReplaceUsernamesViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): 'successful_replacements': data["username_mappings"] } self.register_get_username_replacement_response(self.user) - response = self.call_api(self.url, self.client_user, data) + response = self.call_api(self.client_user, data) self.assertEqual(response.status_code, 200) self.assertEqual(response.data, expected_response) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 8af1995270..53d9f99948 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -10,7 +10,7 @@ from edx_rest_framework_extensions.auth.session.authentication import SessionAut from opaque_keys.edx.keys import CourseKey from rest_framework import permissions from rest_framework import status -from rest_framework.exceptions import UnsupportedMediaType +from rest_framework.exceptions import ParseError, UnsupportedMediaType from rest_framework.parsers import JSONParser from rest_framework.response import Response from rest_framework.views import APIView @@ -621,7 +621,7 @@ class ReplaceUsernamesView(APIView): username_mappings = request.data.get("username_mappings") if not self._has_valid_schema(username_mappings): - raise ValidationError("Request data does not match schema") + raise ParseError("Request data does not match schema") successful_replacements, failed_replacements = [], [] @@ -630,13 +630,17 @@ class ReplaceUsernamesView(APIView): new_username = list(username_pair.values())[0] successfully_replaced = self._replace_username(current_username, new_username) if successfully_replaced: - successful_replacements += {current_username: new_username} + successful_replacements.append({current_username: new_username}) else: - failed_replacements += {current_username: new_username} - return JsonResponse({ - "successful_replacements": successful_replacements, - "failed_replacements": failed_replacements, - }) + failed_replacements.append({current_username: new_username}) + + return Response( + status=status.HTTP_200_OK, + data={ + "successful_replacements": successful_replacements, + "failed_replacements": failed_replacements + } + ) def _replace_username(self, current_username, new_username): try: @@ -652,7 +656,7 @@ class ReplaceUsernamesView(APIView): new_username, new_username, ) - return False + return True except comment_client.CommentClientRequestError as exc: log.exception( u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", 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 c4b4c0a305..1f664bd456 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -951,7 +951,7 @@ class UsernameReplacementViewTests(APITestCase): Helper function for creating headers for the JWT authentication. """ token = create_jwt_for_user(user) - headers = {'HTTP_AUTHORIZATION': token} + headers = {'HTTP_AUTHORIZATION': 'JWT {}'.format(token)} return headers def call_api(self, user, data): From 5a164df3b7949c6e59b0909918558a4ad752ab0a Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Thu, 14 Mar 2019 10:44:40 -0400 Subject: [PATCH 15/20] quality --- lms/djangoapps/discussion_api/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 53d9f99948..49817e620b 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -682,6 +682,7 @@ class ReplaceUsernamesView(APIView): return False return True + class CourseDiscussionSettingsAPIView(DeveloperErrorViewMixin, APIView): """ **Use Cases** From 634866dd0cecb0190d591cda019ba52ce0b9c5a8 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Thu, 14 Mar 2019 11:15:25 -0400 Subject: [PATCH 16/20] Don't count a non-existing forums user as a failure --- lms/djangoapps/discussion_api/views.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 49817e620b..7655f82cb5 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -658,12 +658,20 @@ class ReplaceUsernamesView(APIView): ) return True except comment_client.CommentClientRequestError as exc: - log.exception( - u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", - current_username, - new_username, - exc, - ) + if exc.status_code == 404: + log.info( + u"Unable to change username from %s to %s in forums because user doesn't exist in forums", + current_username, + new_username, + ) + return True + else: + log.exception( + u"Unable to change username from %s to %s in forums because forums API call failed with: %s.", + current_username, + new_username, + exc, + ) return False log.info( From 510f877408b25597b260e0ca7be204cee57b2922 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 20 Mar 2019 10:52:10 -0400 Subject: [PATCH 17/20] quality --- lms/djangoapps/discussion_api/views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 7655f82cb5..0000bb1563 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -643,6 +643,9 @@ class ReplaceUsernamesView(APIView): ) def _replace_username(self, current_username, new_username): + """ + Replaces the current username with the new username in the forums service + """ try: # This API will be called after the regular LMS API, so the username in # the DB will have already been updated to new_username From c2e1cef508b2a1ad5a63366675ffdfbe49e15fa0 Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 20 Mar 2019 10:55:17 -0400 Subject: [PATCH 18/20] quality --- openedx/core/djangoapps/user_api/accounts/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1f664bd456..1c3a480bd7 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -951,7 +951,7 @@ class UsernameReplacementViewTests(APITestCase): Helper function for creating headers for the JWT authentication. """ token = create_jwt_for_user(user) - headers = {'HTTP_AUTHORIZATION': 'JWT {}'.format(token)} + headers = {'HTTP_AUTHORIZATION': u'JWT {}'.format(token)} return headers def call_api(self, user, data): From 41a5ebb0a199896776841920eb2a589c3ba2884e Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Wed, 20 Mar 2019 11:09:46 -0400 Subject: [PATCH 19/20] Review --- openedx/core/djangoapps/user_api/accounts/views.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index b6d7b28580..a5974c9584 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1052,8 +1052,6 @@ class UsernameReplacementView(APIView): {"old_username_2": "new_username_2"} ] } - - TODO: Determine if we need an audit trail outside of logging and API response. """ # (model_name, column_name) @@ -1117,7 +1115,11 @@ class UsernameReplacementView(APIView): return True def _generate_unique_username(self, desired_username, suffix_length=4): - """ Accepts a username and returns a unique username if the requested is taken """ + """ + Generates a unique username. + If the desired username is available, that will be returned. + Otherwise it will generate unique suffixs to the desired username until it is an available username. + """ new_username = desired_username # Keep checking usernames in case desired_username + random suffix is already taken while True: From 5f3bfd1ae458a5883d550ab3a462c16cfa1116dd Mon Sep 17 00:00:00 2001 From: Matt Tuchfarber Date: Thu, 21 Mar 2019 11:16:01 -0400 Subject: [PATCH 20/20] Fix documentation and logging --- openedx/core/djangoapps/user_api/accounts/permissions.py | 3 +-- openedx/core/djangoapps/user_api/accounts/views.py | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index 9092ca002c..7b14ce00c6 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -28,8 +28,7 @@ class CanRetireUser(permissions.BasePermission): class CanReplaceUsername(permissions.BasePermission): """ - Grants access to the Username Replacement API for anyone in the group, - including the service user. + Grants access to the Username Replacement API for the service user. """ def has_permission(self, request, view): return request.user.username == getattr(settings, "USERNAME_REPLACEMENT_WORKER", False) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index a5974c9584..4ee7fbaa2e 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1148,9 +1148,10 @@ class UsernameReplacementView(APIView): ) except Exception as exc: # pylint: disable=broad-except log.exception( - u"Unable to change username from %s to %s. Reason: %s", + u"Unable to change username from %s to %s. Failed on table %s because %s", current_username, new_username, + model.__class__.__name__, # Retrieves the model name that it failed on exc ) return False