From 5dc39666e42bdea11a96625575f53a671f41bffa Mon Sep 17 00:00:00 2001 From: Kira Date: Mon, 14 Mar 2022 12:34:19 -0400 Subject: [PATCH 1/4] feat: add display name and make saml config draft --- .../migrations/0006_auto_20220314_1551.py | 58 +++++++++++++++++++ common/djangoapps/third_party_auth/models.py | 16 +++-- .../tests/test_samlproviderconfig.py | 1 + lms/djangoapps/program_enrollments/signals.py | 16 +++++ 4 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py diff --git a/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py b/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py new file mode 100644 index 0000000000..fad006897a --- /dev/null +++ b/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py @@ -0,0 +1,58 @@ +# Generated by Django 3.2.12 on 2022-03-14 15:51 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('third_party_auth', '0005_auto_20210723_1527'), + ] + + operations = [ + migrations.AddField( + model_name='samlproviderconfig', + name='display_name', + field=models.CharField(blank=True, help_text='A configuration nickname.', max_length=30), + ), + migrations.AlterField( + model_name='ltiproviderconfig', + name='name', + field=models.CharField(blank=True, help_text='Name of this provider (shown to users)', max_length=50), + ), + migrations.AlterField( + model_name='oauth2providerconfig', + name='name', + field=models.CharField(blank=True, help_text='Name of this provider (shown to users)', max_length=50), + ), + migrations.AlterField( + model_name='samlconfiguration', + name='slug', + field=models.SlugField(blank=True, default='default', help_text='A short string uniquely identifying this configuration. Cannot contain spaces. Examples: "ubc", "mit-staging"', max_length=30), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='backend_name', + field=models.CharField(blank=True, default='tpa-saml', help_text="Which python-social-auth provider backend to use. 'tpa-saml' is the standard edX SAML backend.", max_length=50), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='entity_id', + field=models.CharField(blank=True, help_text='Example: https://idp.testshib.org/idp/shibboleth', max_length=255, verbose_name='Entity ID'), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='identity_provider_type', + field=models.CharField(blank=True, choices=[('standard_saml_provider', 'Standard SAML provider'), ('sap_success_factors', 'SAP SuccessFactors provider')], default='standard_saml_provider', help_text='Some SAML providers require special behavior. For example, SAP SuccessFactors SAML providers require an additional API call to retrieve user metadata not provided in the SAML response. Select the provider type which best matches your use case. If in doubt, choose the Standard SAML Provider type.', max_length=128, verbose_name='Identity Provider Type'), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='metadata_source', + field=models.CharField(blank=True, help_text="URL to this provider's XML metadata. Should be an HTTPS URL. Example: https://www.testshib.org/metadata/testshib-providers.xml", max_length=255), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='name', + field=models.CharField(blank=True, help_text='Name of this provider (shown to users)', max_length=50), + ), + ] diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 16e722b30d..250b0c5004 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -111,7 +111,8 @@ class ProviderConfig(ConfigurationModel): 'SVG images are recommended as they can scale to any size.' ), ) - name = models.CharField(max_length=50, blank=False, help_text="Name of this provider (shown to users)") + name = models.CharField( + max_length=50, blank=True, help_text="Name of this provider (shown to users)") slug = models.SlugField( max_length=30, db_index=True, default='default', help_text=( @@ -429,6 +430,7 @@ class SAMLConfiguration(ConfigurationModel): slug = models.SlugField( max_length=30, default='default', + blank=True, help_text=( 'A short string uniquely identifying this configuration. ' 'Cannot contain spaces. Examples: "ubc", "mit-staging"' @@ -569,13 +571,17 @@ class SAMLProviderConfig(ProviderConfig): .. no_pii: """ prefix = 'saml' + display_name = models.CharField( + max_length=30, blank=True, + help_text=_("A configuration nickname.")) backend_name = models.CharField( - max_length=50, default='tpa-saml', blank=False, + max_length=50, default='tpa-saml', blank=True, help_text="Which python-social-auth provider backend to use. 'tpa-saml' is the standard edX SAML backend.") entity_id = models.CharField( - max_length=255, verbose_name="Entity ID", help_text="Example: https://idp.testshib.org/idp/shibboleth") + max_length=255, verbose_name="Entity ID", blank=True, + help_text="Example: https://idp.testshib.org/idp/shibboleth") metadata_source = models.CharField( - max_length=255, + max_length=255, blank=True, help_text=( "URL to this provider's XML metadata. Should be an HTTPS URL. " "Example: https://www.testshib.org/metadata/testshib-providers.xml" @@ -622,7 +628,7 @@ class SAMLProviderConfig(ProviderConfig): "in the automatic refresh job, if configured." ) identity_provider_type = models.CharField( - max_length=128, blank=False, verbose_name="Identity Provider Type", default=STANDARD_SAML_PROVIDER_KEY, + max_length=128, blank=True, verbose_name="Identity Provider Type", default=STANDARD_SAML_PROVIDER_KEY, choices=get_saml_idp_choices(), help_text=( "Some SAML providers require special behavior. For example, SAP SuccessFactors SAML providers require an " "additional API call to retrieve user metadata not provided in the SAML response. Select the provider type " diff --git a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py index e3b3426b45..49e169b62c 100644 --- a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py +++ b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py @@ -96,6 +96,7 @@ class SAMLProviderConfigTests(APITestCase): assert results[0]['entity_id'] == SINGLE_PROVIDER_CONFIG['entity_id'] assert results[0]['metadata_source'] == SINGLE_PROVIDER_CONFIG['metadata_source'] assert response.data['results'][0]['country'] == SINGLE_PROVIDER_CONFIG['country'] + assert results[0]['display_name'] == "saml-test-ep-1" assert SAMLProviderConfig.objects.count() == 1 def test_get_one_config_by_enterprise_uuid_invalid_uuid(self): diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py index 7c5f9aaa7f..d58aee55ef 100644 --- a/lms/djangoapps/program_enrollments/signals.py +++ b/lms/djangoapps/program_enrollments/signals.py @@ -43,6 +43,22 @@ def listen_for_social_auth_creation(sender, instance, created, **kwargs): # lin ) raise +def generate_default_display_name(self): + """ + Returns a default display namem which can be overriden by a subclass. + """ + return f'{self.prefix}-{self.slug}-{self.id}' + +@receiver(post_save, sender=SAMLProviderConfig) +def save_default_display_name(sender, instance, created, **kwargs): # lint-amnesty, pylint: disable=unused-argument + """ + Post-save signal that sets default display name if one is not provided + """ + this_display_name = instance.display_name + # check if display_name is None, empty, or just spaces + if not (this_display_name and this_display_name.strip()): + instance.display_name = generate_default_display_name(instance) + instance.save() def matriculate_learner(user, uid): """ From 97f05b4e4aa4f0f6ed7a746d3395ced55be9456b Mon Sep 17 00:00:00 2001 From: Kira Date: Mon, 14 Mar 2022 13:30:52 -0400 Subject: [PATCH 2/4] fix: lint errors --- common/djangoapps/third_party_auth/models.py | 2 +- lms/djangoapps/program_enrollments/signals.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 250b0c5004..dd92c839f8 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -575,7 +575,7 @@ class SAMLProviderConfig(ProviderConfig): max_length=30, blank=True, help_text=_("A configuration nickname.")) backend_name = models.CharField( - max_length=50, default='tpa-saml', blank=True, + max_length=50, default='tpa-saml', blank=True, help_text="Which python-social-auth provider backend to use. 'tpa-saml' is the standard edX SAML backend.") entity_id = models.CharField( max_length=255, verbose_name="Entity ID", blank=True, diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py index d58aee55ef..8d7b910207 100644 --- a/lms/djangoapps/program_enrollments/signals.py +++ b/lms/djangoapps/program_enrollments/signals.py @@ -43,12 +43,14 @@ def listen_for_social_auth_creation(sender, instance, created, **kwargs): # lin ) raise + def generate_default_display_name(self): """ - Returns a default display namem which can be overriden by a subclass. + Returns a default display name for SamlProviderConfig. """ return f'{self.prefix}-{self.slug}-{self.id}' + @receiver(post_save, sender=SAMLProviderConfig) def save_default_display_name(sender, instance, created, **kwargs): # lint-amnesty, pylint: disable=unused-argument """ @@ -60,6 +62,7 @@ def save_default_display_name(sender, instance, created, **kwargs): # lint-amne instance.display_name = generate_default_display_name(instance) instance.save() + def matriculate_learner(user, uid): """ Update any waiting program enrollments with a user, From 6d51428e4a5511f4422cb3b91f7e754bf9358c41 Mon Sep 17 00:00:00 2001 From: Kira Date: Mon, 14 Mar 2022 17:06:52 -0400 Subject: [PATCH 3/4] fix: duplicate entry fixes --- .../tests/test_samlproviderconfig.py | 3 ++- lms/djangoapps/program_enrollments/signals.py | 11 ++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py index 49e169b62c..e53de1b124 100644 --- a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py +++ b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py @@ -2,6 +2,7 @@ Tests for SAMLProviderConfig endpoints """ import copy +import re from uuid import uuid4 from django.urls import reverse from django.contrib.sites.models import Site @@ -96,7 +97,7 @@ class SAMLProviderConfigTests(APITestCase): assert results[0]['entity_id'] == SINGLE_PROVIDER_CONFIG['entity_id'] assert results[0]['metadata_source'] == SINGLE_PROVIDER_CONFIG['metadata_source'] assert response.data['results'][0]['country'] == SINGLE_PROVIDER_CONFIG['country'] - assert results[0]['display_name'] == "saml-test-ep-1" + assert re.match(r"saml-test-slug-\d{4}", results[0]['display_name']) assert SAMLProviderConfig.objects.count() == 1 def test_get_one_config_by_enterprise_uuid_invalid_uuid(self): diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py index 8d7b910207..78e23326a8 100644 --- a/lms/djangoapps/program_enrollments/signals.py +++ b/lms/djangoapps/program_enrollments/signals.py @@ -4,8 +4,9 @@ Signal handlers for program enrollments import logging +import datetime -from django.db.models.signals import post_save +from django.db.models.signals import pre_save, post_save from django.dispatch import receiver from social_django.models import UserSocialAuth @@ -48,11 +49,12 @@ def generate_default_display_name(self): """ Returns a default display name for SamlProviderConfig. """ - return f'{self.prefix}-{self.slug}-{self.id}' + t = datetime.datetime.now() + return f'{self.prefix}-{self.slug}-{t.minute}{t.second}' -@receiver(post_save, sender=SAMLProviderConfig) -def save_default_display_name(sender, instance, created, **kwargs): # lint-amnesty, pylint: disable=unused-argument +@receiver(pre_save, sender=SAMLProviderConfig) +def save_default_display_name(sender, instance, **kwargs): # lint-amnesty, pylint: disable=unused-argument """ Post-save signal that sets default display name if one is not provided """ @@ -60,7 +62,6 @@ def save_default_display_name(sender, instance, created, **kwargs): # lint-amne # check if display_name is None, empty, or just spaces if not (this_display_name and this_display_name.strip()): instance.display_name = generate_default_display_name(instance) - instance.save() def matriculate_learner(user, uid): From 0ba94409bd51c0e76d9578cd1099177baf45af81 Mon Sep 17 00:00:00 2001 From: Kira Date: Tue, 15 Mar 2022 09:28:23 -0400 Subject: [PATCH 4/4] fix: lengthening display name --- .../third_party_auth/migrations/0006_auto_20220314_1551.py | 2 +- common/djangoapps/third_party_auth/models.py | 2 +- .../samlproviderconfig/tests/test_samlproviderconfig.py | 4 +++- lms/djangoapps/program_enrollments/signals.py | 6 +++--- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py b/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py index fad006897a..8d5c38829e 100644 --- a/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py +++ b/common/djangoapps/third_party_auth/migrations/0006_auto_20220314_1551.py @@ -13,7 +13,7 @@ class Migration(migrations.Migration): migrations.AddField( model_name='samlproviderconfig', name='display_name', - field=models.CharField(blank=True, help_text='A configuration nickname.', max_length=30), + field=models.CharField(blank=True, help_text='A configuration nickname.', max_length=35), ), migrations.AlterField( model_name='ltiproviderconfig', diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index dd92c839f8..1421767cb8 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -572,7 +572,7 @@ class SAMLProviderConfig(ProviderConfig): """ prefix = 'saml' display_name = models.CharField( - max_length=30, blank=True, + max_length=35, blank=True, help_text=_("A configuration nickname.")) backend_name = models.CharField( max_length=50, default='tpa-saml', blank=True, diff --git a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py index e53de1b124..52de29d3f8 100644 --- a/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py +++ b/common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py @@ -33,6 +33,7 @@ SINGLE_PROVIDER_CONFIG = { SINGLE_PROVIDER_CONFIG_2 = copy.copy(SINGLE_PROVIDER_CONFIG) SINGLE_PROVIDER_CONFIG_2['name'] = 'name-of-config-2' SINGLE_PROVIDER_CONFIG_2['slug'] = 'test-slug-2' +SINGLE_PROVIDER_CONFIG_2['display_name'] = 'display-name' SINGLE_PROVIDER_CONFIG_3 = copy.copy(SINGLE_PROVIDER_CONFIG) SINGLE_PROVIDER_CONFIG_3['name'] = 'name-of-config-3' @@ -97,7 +98,7 @@ class SAMLProviderConfigTests(APITestCase): assert results[0]['entity_id'] == SINGLE_PROVIDER_CONFIG['entity_id'] assert results[0]['metadata_source'] == SINGLE_PROVIDER_CONFIG['metadata_source'] assert response.data['results'][0]['country'] == SINGLE_PROVIDER_CONFIG['country'] - assert re.match(r"saml-test-slug-\d{4}", results[0]['display_name']) + assert re.match(r"test-slug-\d{4}", results[0]['display_name']) assert SAMLProviderConfig.objects.count() == 1 def test_get_one_config_by_enterprise_uuid_invalid_uuid(self): @@ -149,6 +150,7 @@ class SAMLProviderConfigTests(APITestCase): assert provider_config.name == 'name-of-config-2' assert provider_config.country == SINGLE_PROVIDER_CONFIG_2['country'] assert provider_config.attr_username == SINGLE_PROVIDER_CONFIG['attr_first_name'] + assert provider_config.display_name == SINGLE_PROVIDER_CONFIG_2['display_name'] # check association has also been created assert EnterpriseCustomerIdentityProvider.objects.filter( diff --git a/lms/djangoapps/program_enrollments/signals.py b/lms/djangoapps/program_enrollments/signals.py index 78e23326a8..572370c248 100644 --- a/lms/djangoapps/program_enrollments/signals.py +++ b/lms/djangoapps/program_enrollments/signals.py @@ -4,7 +4,7 @@ Signal handlers for program enrollments import logging -import datetime +from datetime import datetime from django.db.models.signals import pre_save, post_save from django.dispatch import receiver @@ -49,8 +49,8 @@ def generate_default_display_name(self): """ Returns a default display name for SamlProviderConfig. """ - t = datetime.datetime.now() - return f'{self.prefix}-{self.slug}-{t.minute}{t.second}' + time = datetime.now().strftime('%M%S') + return f'{self.slug}-{time}' @receiver(pre_save, sender=SAMLProviderConfig)