From 6d5dcdf77a82f9f4ed5d478cfa63a128d0680255 Mon Sep 17 00:00:00 2001 From: John Nagro Date: Fri, 30 Jun 2023 09:47:02 -0400 Subject: [PATCH] fix: improve username generation (#32613) --- .../djangoapps/third_party_auth/pipeline.py | 25 ++++++++----- .../djangoapps/third_party_auth/settings.py | 2 +- .../third_party_auth/tests/test_pipeline.py | 35 ++++++++++++------- .../djangoapps/user_api/accounts/utils.py | 9 +++-- 4 files changed, 48 insertions(+), 23 deletions(-) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 601b411b71..a93f09b6ae 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -66,6 +66,7 @@ from collections import OrderedDict from logging import getLogger from smtplib import SMTPException from uuid import uuid4 +from random import randint import six import social_django @@ -997,22 +998,30 @@ def get_username(strategy, details, backend, user=None, *args, **kwargs): # lin else: username = uuid4().hex - short_username = (username[:max_length - uuid_length] - if max_length is not None - else username) + input_username = username final_username = slug_func(clean_func(username[:max_length])) # Generate a unique username for current user using username # as base but adding a unique hash at the end. Original # username is cut to avoid any field max_length. - # 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}): + # We will attempt different length random suffixes + while len(final_username) < min_length or user_exists({'username': final_username}): + # try to use at least 3 characters to avoid weird '-1' '-a' etc + # use less than 3 if that is whats being asked for + this_uuid_length = randint(min(uuid_length, 3), uuid_length) + short_username = (input_username[:max_length - (this_uuid_length + 1)] # +1 for the '-' + if max_length is not None + else username) # adding a dash between user-supplied and system-generated values to avoid weird combinations - username = short_username + '-' + username_suffix_generator(uuid_length) + username = short_username + '-' + username_suffix_generator(this_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)) + logger.info( + '[THIRD_PARTY_AUTH] New username candidnate generated: ' + f'input_username={input_username}, ' + f'suffix_length={this_uuid_length}, ' + f'final_username={final_username}' + ) else: final_username = storage.user.get_username(user) return {'username': final_username} diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 20ee8b9935..0aeb94fbd4 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -100,7 +100,7 @@ def apply_settings(django_settings): django_settings.SOCIAL_AUTH_INACTIVE_USER_URL = '/auth/inactive' # Context processors required under Django. - django_settings.SOCIAL_AUTH_UUID_LENGTH = 6 + django_settings.SOCIAL_AUTH_UUID_LENGTH = 10 django_settings.DEFAULT_TEMPLATE_ENGINE['OPTIONS']['context_processors'] += ( 'social_django.context_processors.backends', 'social_django.context_processors.login_redirect', diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline.py b/common/djangoapps/third_party_auth/tests/test_pipeline.py index a8310bf0e7..dc27f53f2b 100644 --- a/common/djangoapps/third_party_auth/tests/test_pipeline.py +++ b/common/djangoapps/third_party_auth/tests/test_pipeline.py @@ -72,17 +72,26 @@ class PipelineOverridesTest(SamlIntegrationTestUtilities, IntegrationTestMixin, ) @ddt.data( - ('S', 'S-9fe2', False), - ('S', 'S-9fe2', True), - ('S.K', 'S_K', False), - ('S.K.', 'S_K', False), - ('S.K.', 'S_K_-9fe2', True), - ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengthofm', False), - ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlen-9fe2', True), + ('S', 'S-b', False, False, False), + ('S', 'S-3f', True, False, False), + ('S', 'S-9fe2', True, True, False), + ('S.K', 'S_K', False, True, True), + ('S.K.', 'S_K_-9fe2', True, True, True), + ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengthofm', False, False, False), + ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengtho-b', True, False, False), + ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlength-3f', True, True, False), ) @ddt.unpack @mock.patch('common.djangoapps.third_party_auth.pipeline.user_exists') - def test_get_username_in_pipeline(self, idp_username, expected_username, already_exists, mock_user_exists): + def test_get_username_in_pipeline( + self, + idp_username, + expected_username, + already_exists_one, + already_exists_two, + already_exists_three, + mock_user_exists, + ): """ Test get_username method of running pipeline """ @@ -90,9 +99,11 @@ class PipelineOverridesTest(SamlIntegrationTestUtilities, IntegrationTestMixin, "username": idp_username, "email": "test@example.com" } - mock_user_exists.side_effect = [already_exists, False] + mock_user_exists.side_effect = [already_exists_one, already_exists_two, already_exists_three, False] __, strategy = self.get_request_and_strategy() 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'] + mock_suffix.side_effect = ['b', '3f', '9fe2'] + with mock.patch('common.djangoapps.third_party_auth.pipeline.randint') as mock_randint: + mock_randint.side_effect = [1, 2, 4] + 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 c9c8cf085b..1326c36a2e 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -229,13 +229,18 @@ 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. + Whether the suffix starts with a letter or number is also randomized. """ + # pick from letters, or numbers + choice_collections = [string.ascii_lowercase, string.digits] + # randomize which collection to pick from first + random.shuffle(choice_collections) output = '' for i in range(suffix_length): if (i % 2) == 0: - output += random.choice(string.ascii_lowercase) + output += random.choice(choice_collections[0]) else: - output += random.choice(string.digits) + output += random.choice(choice_collections[1]) return output