From 3873ff7de9c9d81a2ac1dcdd3f620341cc23d613 Mon Sep 17 00:00:00 2001 From: John Nagro Date: Mon, 13 Dec 2021 15:32:51 -0500 Subject: [PATCH] feat: avoid certain strings in the suffix appended to edX usernames (#29524) ENT-2824 --- common/djangoapps/third_party_auth/pipeline.py | 4 +++- .../third_party_auth/tests/test_pipeline.py | 14 ++++++-------- .../core/djangoapps/user_api/accounts/utils.py | 18 +++++++++++++++++- .../core/djangoapps/user_api/accounts/views.py | 7 +++---- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 54a9e25337..ec8c253556 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -87,6 +87,7 @@ from lms.djangoapps.verify_student.models import SSOVerification from lms.djangoapps.verify_student.utils import earliest_allowed_verification_date from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import accounts +from openedx.core.djangoapps.user_api.accounts.utils import username_suffix_generator from openedx.core.djangoapps.user_authn import cookies as user_authn_cookies from openedx.core.djangoapps.user_authn.toggles import is_require_third_party_auth_enabled from common.djangoapps.third_party_auth.utils import ( @@ -1007,7 +1008,8 @@ def get_username(strategy, details, backend, user=None, *args, **kwargs): # lin # The final_username may be empty and will skip the loop. # We are using our own version of user_exists to avoid possible case sensitivity issues. while not final_username or len(final_username) < min_length or user_exists({'username': final_username}): - username = short_username + uuid4().hex[:uuid_length] + # adding a dash between user-supplied and system-generated values to avoid weird combinations + username = short_username + '-' + username_suffix_generator(uuid_length) final_username = slug_func(clean_func(username[:max_length])) logger.info('[THIRD_PARTY_AUTH] New username generated. Username: {username}'.format( username=final_username)) diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline.py b/common/djangoapps/third_party_auth/tests/test_pipeline.py index 5e5c35d281..5922dfab99 100644 --- a/common/djangoapps/third_party_auth/tests/test_pipeline.py +++ b/common/djangoapps/third_party_auth/tests/test_pipeline.py @@ -72,13 +72,13 @@ class PipelineOverridesTest(SamlIntegrationTestUtilities, IntegrationTestMixin, ) @ddt.data( - ('S', 'S9fe2', False), - ('S', 'S9fe2', True), + ('S', 'S-9fe2', False), + ('S', 'S-9fe2', True), ('S.K', 'S_K', False), ('S.K.', 'S_K', False), - ('S.K.', 'S_K_9fe2', True), + ('S.K.', 'S_K_-9fe2', True), ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengthofm', False), - ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengt9fe2', True), + ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengt-9fe', True), ) @ddt.unpack @mock.patch('common.djangoapps.third_party_auth.pipeline.user_exists') @@ -92,9 +92,7 @@ class PipelineOverridesTest(SamlIntegrationTestUtilities, IntegrationTestMixin, } mock_user_exists.side_effect = [already_exists, False] __, strategy = self.get_request_and_strategy() - with mock.patch('common.djangoapps.third_party_auth.pipeline.uuid4') as mock_uuid: - uuid4 = mock.Mock() - type(uuid4).hex = mock.PropertyMock(return_value='9fe2c4e93f654fdbb24c02b15259716c') - mock_uuid.return_value = uuid4 + with mock.patch('common.djangoapps.third_party_auth.pipeline.username_suffix_generator') as mock_suffix: + mock_suffix.return_value = '9fe2' final_username = pipeline.get_username(strategy, details, self.provider.backend_class()) assert expected_username == final_username['username'] diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index 47608708e5..fd80ed82b8 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -2,8 +2,9 @@ Utility methods for the account settings. """ - +import random import re +import string from urllib.parse import urlparse # pylint: disable=import-error import waffle # lint-amnesty, pylint: disable=invalid-django-waffle-import @@ -215,3 +216,18 @@ def create_retirement_request_and_deactivate_account(user): # Delete OAuth tokens associated with the user. retire_dot_oauth2_models(user) AccountRecovery.retire_recovery_email(user.id) + + +def username_suffix_generator(suffix_length=4): + """ + Generates a random, alternating number and letter string for the purpose of + appending to non-unique usernames. Alternating is less likey to produce + a significant/meaningful substring like an offensive word. + """ + output = '' + for i in range(suffix_length): + if (i % 2) == 0: + output += random.choice(string.ascii_lowercase) + else: + output += random.choice(string.digits) + return output diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 7aed9e2b98..840e3db372 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -7,7 +7,6 @@ https://openedx.atlassian.net/wiki/display/TNL/User+API import datetime import logging -import uuid from functools import wraps import pytz @@ -82,7 +81,7 @@ from .serializers import ( UserSearchEmailSerializer ) from .signals import USER_RETIRE_LMS_CRITICAL, USER_RETIRE_LMS_MISC, USER_RETIRE_MAILINGS -from .utils import create_retirement_request_and_deactivate_account +from .utils import create_retirement_request_and_deactivate_account, username_suffix_generator try: from coaching.api import has_ever_consented_to_coaching @@ -1325,8 +1324,8 @@ class UsernameReplacementView(APIView): # 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 + # adding a dash between user-supplied and system-generated values to avoid weird combinations + new_username = desired_username + '-' + username_suffix_generator(suffix_length) else: break return new_username