From 647da21191102e1749ccaafb7f8de8c6e40829a1 Mon Sep 17 00:00:00 2001
From: "zia.fazal@arbisoft.com" <zia.fazal@arbisoft.com>
Date: Thu, 7 May 2020 17:35:23 +0500
Subject: [PATCH] Apply same username restrictions during SSO pipeline
Apply same username restrictions during SSO pipeline as we have user registeration flow to avoid SSO flow breakage at the time of user creation.
ENT-2730
Code quality fixes
---
.../djangoapps/third_party_auth/pipeline.py | 11 +++--
.../djangoapps/third_party_auth/settings.py | 3 ++
.../third_party_auth/tests/test_pipeline.py | 48 +++++++++++++++++++
3 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py
index 84ecefabec3..18924f21ce8 100644
--- a/common/djangoapps/third_party_auth/pipeline.py
+++ b/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(
diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py
index 0f6a4536c3f..b28f810df6d 100644
--- a/common/djangoapps/third_party_auth/settings.py
+++ b/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
diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline.py b/common/djangoapps/third_party_auth/tests/test_pipeline.py
index 2374c4827e7..9613706cf65 100644
--- a/common/djangoapps/third_party_auth/tests/test_pipeline.py
+++ b/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'])
--
GitLab