Merge pull request #23934 from edx/ziafazal/ENT-2730

ENT-2730: Apply same username restrictions during SSO pipeline

common/djangoapps/third_party_auth/pipeline.py

@@ -85,6 +85,7 @@ from edxmako.shortcuts import render_to_string
 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_authn import cookies as user_authn_cookies
 from third_party_auth.utils import user_exists
 from track import segment
@@ -850,7 +851,10 @@ def set_id_verification_status(auth_entry, strategy, details, user=None, *args,
 
 def get_username(strategy, details, backend, user=None, *args, **kwargs):
     """
-    Copy of social_core.pipeline.user.get_username with additional logging and case insensitive username checks.
+    Copy of social_core.pipeline.user.get_username to achieve
+    1. additional logging
+    2. case insensitive username checks
+    3. enforce same maximum and minimum length restrictions we have in `user_api/accounts`
     """
     if 'username' not in backend.setting('USER_FIELDS', USER_FIELDS):
         return
@@ -859,7 +863,8 @@ def get_username(strategy, details, backend, user=None, *args, **kwargs):
     if not user:
         email_as_username = strategy.setting('USERNAME_IS_FULL_EMAIL', False)
         uuid_length = strategy.setting('UUID_LENGTH', 16)
-        max_length = storage.user.username_max_length()
+        min_length = strategy.setting('USERNAME_MIN_LENGTH', accounts.USERNAME_MIN_LENGTH)
+        max_length = strategy.setting('USERNAME_MAX_LENGTH', accounts.USERNAME_MAX_LENGTH)
         do_slugify = strategy.setting('SLUGIFY_USERNAMES', False)
         do_clean = strategy.setting('CLEAN_USERNAMES', True)
 
@@ -898,7 +903,7 @@ def get_username(strategy, details, backend, user=None, *args, **kwargs):
         # 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 user_exists({'username': final_username}):
+        while not final_username or len(final_username) < min_length or user_exists({'username': final_username}):
             username = short_username + uuid4().hex[:uuid_length]
             final_username = slug_func(clean_func(username[:max_length]))
             logger.info(u'[THIRD_PARTY_AUTH] New username generated. Username: {username}'.format(

common/djangoapps/third_party_auth/settings.py

@@ -81,6 +81,9 @@ def apply_settings(django_settings):
     # enable this when you want to get stack traces rather than redirections.
     django_settings.SOCIAL_AUTH_RAISE_EXCEPTIONS = False
 
+    # Clean username to make sure username is compatible with our system requirements
+    django_settings.SOCIAL_AUTH_CLEAN_USERNAME_FUNCTION = 'third_party_auth.models.clean_username'
+
     # Allow users to login using social auth even if their account is not verified yet
     # This is required since we [ab]use django's 'is_active' flag to indicate verified
     # accounts; without this set to True, python-social-auth won't allow us to link the

common/djangoapps/third_party_auth/tests/test_pipeline.py

@@ -9,6 +9,8 @@ import mock
 
 from third_party_auth import pipeline
 from third_party_auth.tests import testutil
+from third_party_auth.tests.specs.base import IntegrationTestMixin
+from third_party_auth.tests.specs.test_testshib import SamlIntegrationTestUtilities
 from third_party_auth.tests.testutil import simulate_running_pipeline
 
 
@@ -50,3 +52,49 @@ class ProviderUserStateTestCase(testutil.TestCase):
         with simulate_running_pipeline("third_party_auth.pipeline", backend_name, **kwargs):
             logout_url = pipeline.get_idp_logout_url_from_running_pipeline(request)
             self.assertEqual(idp_config['logout_url'], logout_url)
+
+
+@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, testutil.AUTH_FEATURES_KEY + ' not enabled')
+@ddt.ddt
+class PipelineOverridesTest(SamlIntegrationTestUtilities, IntegrationTestMixin, testutil.SAMLTestCase):
+    """
+    Tests for pipeline overrides
+    """
+
+    def setUp(self):
+        super(PipelineOverridesTest, self).setUp()
+        self.enable_saml()
+        self.provider = self.configure_saml_provider(
+            enabled=True,
+            name="Test Provider",
+            slug='test',
+            backend_name='tpa-saml'
+        )
+
+    @ddt.data(
+        ('S', 'S9fe2', False),
+        ('S', 'S9fe2', True),
+        ('S.K', 'S_K', False),
+        ('S.K.', 'S_K_', False),
+        ('S.K.', 'S_K_9fe2', True),
+        ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengthofm', False),
+        ('usernamewithcharacterlengthofmorethan30chars', 'usernamewithcharacterlengt9fe2', True),
+    )
+    @ddt.unpack
+    @mock.patch('third_party_auth.pipeline.user_exists')
+    def test_get_username_in_pipeline(self, idp_username, expected_username, already_exists, mock_user_exists):
+        """
+        Test get_username method of running pipeline
+        """
+        details = {
+            "username": idp_username,
+            "email": "test@example.com"
+        }
+        mock_user_exists.side_effect = [already_exists, False]
+        __, strategy = self.get_request_and_strategy()
+        with mock.patch('third_party_auth.pipeline.uuid4') as mock_uuid:
+            uuid4 = mock.Mock()
+            type(uuid4).hex = mock.PropertyMock(return_value='9fe2c4e93f654fdbb24c02b15259716c')
+            mock_uuid.return_value = uuid4
+            final_username = pipeline.get_username(strategy, details, self.provider.backend_class())
+            self.assertEqual(expected_username, final_username['username'])