fix: improve username generation (#32613)

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}

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',

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']

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