From 682ec886ed9ec2242c439392910b012619e6d1f4 Mon Sep 17 00:00:00 2001 From: Brittney Exline Date: Wed, 4 Apr 2018 10:46:50 -0400 Subject: [PATCH] ENT-943 Refactoring third_party_auth models --- common/djangoapps/third_party_auth/admin.py | 2 +- .../third_party_auth/api/tests/test_views.py | 8 +-- .../management/commands/tests/test_saml.py | 4 ++ .../migrations/0019_consolidate_slug.py | 55 +++++++++++++++++++ common/djangoapps/third_party_auth/models.py | 24 +++++--- .../djangoapps/third_party_auth/provider.py | 8 +-- common/djangoapps/third_party_auth/tasks.py | 2 +- .../third_party_auth/tests/factories.py | 3 +- .../tests/specs/test_testshib.py | 2 + .../third_party_auth/tests/test_provider.py | 7 ++- .../third_party_auth/tests/testutil.py | 1 + common/test/db_fixtures/third_party_auth.json | 3 + .../student_account/test/test_views.py | 1 + 13 files changed, 97 insertions(+), 23 deletions(-) create mode 100644 common/djangoapps/third_party_auth/migrations/0019_consolidate_slug.py diff --git a/common/djangoapps/third_party_auth/admin.py b/common/djangoapps/third_party_auth/admin.py index b102e2eacc..1cbd69fdf0 100644 --- a/common/djangoapps/third_party_auth/admin.py +++ b/common/djangoapps/third_party_auth/admin.py @@ -37,7 +37,7 @@ class OAuth2ProviderConfigAdmin(KeyedConfigurationModelAdmin): def get_list_display(self, request): """ Don't show every single field in the admin change list """ return ( - 'name', 'enabled', 'provider_slug', 'site', 'backend_name', 'secondary', 'skip_registration_form', + 'name', 'enabled', 'slug', 'site', 'backend_name', 'secondary', 'skip_registration_form', 'skip_email_verification', 'change_date', 'changed_by', 'edit_link', ) diff --git a/common/djangoapps/third_party_auth/api/tests/test_views.py b/common/djangoapps/third_party_auth/api/tests/test_views.py index 84d4d746b8..fa58a97f4f 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -54,7 +54,7 @@ class TpaAPITestCase(ThirdPartyAuthTestMixin, APITestCase): testshib = self.configure_saml_provider( name='TestShib', enabled=True, - idp_slug=IDP_SLUG_TESTSHIB + slug=IDP_SLUG_TESTSHIB ) # Create several users and link each user to Google and TestShib @@ -75,7 +75,7 @@ class TpaAPITestCase(ThirdPartyAuthTestMixin, APITestCase): UserSocialAuth.objects.create( user=user, provider=testshib.backend_name, - uid='{}:remote_{}'.format(testshib.idp_slug, username), + uid='{}:remote_{}'.format(testshib.slug, username), ) # Create another user not linked to any providers: UserFactory.create(username=CARL_USERNAME, password=PASSWORD) @@ -238,7 +238,7 @@ class UserMappingViewAPITests(TpaAPITestCase): self._verify_response(response, expect_code, expect_data) def test_user_mappings_only_return_requested_idp_mapping_by_provider_id(self): - testshib2 = self.configure_saml_provider(name='TestShib2', enabled=True, idp_slug='testshib2') + testshib2 = self.configure_saml_provider(name='TestShib2', enabled=True, slug='testshib2') username = 'testshib2user' user = UserFactory.create( username=username, @@ -249,7 +249,7 @@ class UserMappingViewAPITests(TpaAPITestCase): UserSocialAuth.objects.create( user=user, provider=testshib2.backend_name, - uid='{}:{}'.format(testshib2.idp_slug, username), + uid='{}:{}'.format(testshib2.slug, username), ) url = reverse('third_party_auth_user_mapping_api', kwargs={'provider_id': PROVIDER_ID_TESTSHIB}) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index b2148195cb..66ffcd3d86 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -153,6 +153,7 @@ class TestSAMLCommand(TestCase): "site__domain": "second.testserver.fake", "site__name": "testserver.fake", "idp_slug": "second-test-shib", + "slug": "second-test-shib", "entity_id": "https://idp.testshib.org/idp/another-shibboleth", "metadata_source": "https://www.testshib.org/metadata/another-testshib-providers.xml", } @@ -168,6 +169,7 @@ class TestSAMLCommand(TestCase): "site__domain": "third.testserver.fake", "site__name": "testserver.fake", "idp_slug": "third-test-shib", + "slug": "third-test-shib", # Note: This entity id will not be present in returned response and will cause failed update. "entity_id": "https://idp.testshib.org/idp/non-existent-shibboleth", "metadata_source": "https://www.testshib.org/metadata/third/testshib-providers.xml", @@ -189,6 +191,7 @@ class TestSAMLCommand(TestCase): "site__domain": "fourth.testserver.fake", "site__name": "testserver.fake", "idp_slug": "fourth-test-shib", + "slug": "fourth-test-shib", "automatic_refresh_enabled": False, # Note: This invalid entity id will not be present in the refresh set "entity_id": "https://idp.testshib.org/idp/fourth-shibboleth", @@ -243,6 +246,7 @@ class TestSAMLCommand(TestCase): saml_provider_config={ "site__domain": "third.testserver.fake", "idp_slug": "third-test-shib", + "slug": "third-test-shib", # Note: This entity id will not be present in returned response and will cause failed update. "entity_id": "https://idp.testshib.org/idp/non-existent-shibboleth", "metadata_source": "https://www.testshib.org/metadata/third/testshib-providers.xml", diff --git a/common/djangoapps/third_party_auth/migrations/0019_consolidate_slug.py b/common/djangoapps/third_party_auth/migrations/0019_consolidate_slug.py new file mode 100644 index 0000000000..a61a67bf37 --- /dev/null +++ b/common/djangoapps/third_party_auth/migrations/0019_consolidate_slug.py @@ -0,0 +1,55 @@ +# -*- coding: utf-8 -*- +""" +Custom migration script to add slug field to all ProviderConfig models. +""" +from __future__ import unicode_literals + +from django.db import migrations, models +from django.utils.text import slugify + + +def fill_slug_field(apps, schema_editor): + """ + Fill in the slug field for each ProviderConfig class for backwards compatability. + """ + OAuth2ProviderConfig = apps.get_model('third_party_auth', 'OAuth2ProviderConfig') + SAMLProviderConfig = apps.get_model('third_party_auth', 'SAMLProviderConfig') + LTIProviderConfig = apps.get_model('third_party_auth', 'LTIProviderConfig') + + for config in OAuth2ProviderConfig.objects.all(): + config.slug = config.provider_slug + config.save() + + for config in SAMLProviderConfig.objects.all(): + config.slug = config.idp_slug + config.save() + + for config in LTIProviderConfig.objects.all(): + config.slug = slugify(config.lti_consumer_key) + config.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('third_party_auth', '0018_auto_20180327_1631'), + ] + + operations = [ + migrations.AddField( + model_name='ltiproviderconfig', + name='slug', + field=models.SlugField(default=b'default', help_text=b'A short string uniquely identifying this provider. Cannot contain spaces and should be a usable as a CSS class. Examples: "ubc", "mit-staging"', max_length=30), + ), + migrations.AddField( + model_name='oauth2providerconfig', + name='slug', + field=models.SlugField(default=b'default', help_text=b'A short string uniquely identifying this provider. Cannot contain spaces and should be a usable as a CSS class. Examples: "ubc", "mit-staging"', max_length=30), + ), + migrations.AddField( + model_name='samlproviderconfig', + name='slug', + field=models.SlugField(default=b'default', help_text=b'A short string uniquely identifying this provider. Cannot contain spaces and should be a usable as a CSS class. Examples: "ubc", "mit-staging"', max_length=30), + ), + migrations.RunPython(fill_slug_field, reverse_code=migrations.RunPython.noop), + ] diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 8166e6f6b8..6c076ba82c 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -88,6 +88,8 @@ class ProviderConfig(ConfigurationModel): """ Abstract Base Class for configuring a third_party_auth provider """ + KEY_FIELDS = ('slug',) + icon_class = models.CharField( max_length=50, blank=True, @@ -109,6 +111,12 @@ class ProviderConfig(ConfigurationModel): ), ) name = models.CharField(max_length=50, blank=False, help_text="Name of this provider (shown to users)") + slug = models.SlugField( + max_length=30, db_index=True, default='default', + help_text=( + 'A short string uniquely identifying this provider. ' + 'Cannot contain spaces and should be a usable as a CSS class. Examples: "ubc", "mit-staging"' + )) secondary = models.BooleanField( default=False, help_text=_( @@ -312,7 +320,6 @@ class OAuth2ProviderConfig(ProviderConfig): Also works for OAuth1 providers. """ prefix = 'oa2' - KEY_FIELDS = ('provider_slug', ) # Backend name is unique backend_name = models.CharField( max_length=50, blank=False, db_index=True, help_text=( @@ -514,7 +521,6 @@ class SAMLProviderConfig(ProviderConfig): Configuration Entry for a SAML/Shibboleth provider. """ prefix = 'saml' - KEY_FIELDS = ('idp_slug', ) backend_name = models.CharField( max_length=50, default='tpa-saml', blank=False, help_text="Which python-social-auth provider backend to use. 'tpa-saml' is the standard edX SAML backend.") @@ -603,26 +609,26 @@ class SAMLProviderConfig(ProviderConfig): def get_url_params(self): """ Get a dict of GET parameters to append to login links for this provider """ - return {'idp': self.idp_slug} + return {'idp': self.slug} def is_active_for_pipeline(self, pipeline): """ Is this provider being used for the specified pipeline? """ - return self.backend_name == pipeline['backend'] and self.idp_slug == pipeline['kwargs']['response']['idp_name'] + return self.backend_name == pipeline['backend'] and self.slug == pipeline['kwargs']['response']['idp_name'] def match_social_auth(self, social_auth): """ Is this provider being used for this UserSocialAuth entry? """ - prefix = self.idp_slug + ":" + prefix = self.slug + ":" return self.backend_name == social_auth.provider and social_auth.uid.startswith(prefix) def get_remote_id_from_social_auth(self, social_auth): """ Given a UserSocialAuth object, return the remote ID used by this provider. """ assert self.match_social_auth(social_auth) # Remove the prefix from the UID - return social_auth.uid[len(self.idp_slug) + 1:] + return social_auth.uid[len(self.slug) + 1:] def get_social_auth_uid(self, remote_id): """ Get social auth uid from remote id by prepending idp_slug to the remote id """ - return '{}:{}'.format(self.idp_slug, remote_id) + return '{}:{}'.format(self.slug, remote_id) def get_config(self): """ @@ -648,7 +654,7 @@ class SAMLProviderConfig(ProviderConfig): log.error( 'No SAMLProviderData found for provider "%s" with entity id "%s" and IdP slug "%s". ' 'Run "manage.py saml pull" to fix or debug.', - self.name, self.entity_id, self.idp_slug + self.name, self.entity_id, self.slug ) raise AuthNotConfigured(provider_name=self.name) conf['x509cert'] = data.public_key @@ -660,7 +666,7 @@ class SAMLProviderConfig(ProviderConfig): SAMLConfiguration.current(self.site.id, 'default') ) idp_class = get_saml_idp_class(self.identity_provider_type) - return idp_class(self.idp_slug, **conf) + return idp_class(self.slug, **conf) class SAMLProviderData(models.Model): diff --git a/common/djangoapps/third_party_auth/provider.py b/common/djangoapps/third_party_auth/provider.py index c4dff5d1ca..6dd669cecc 100644 --- a/common/djangoapps/third_party_auth/provider.py +++ b/common/djangoapps/third_party_auth/provider.py @@ -28,13 +28,13 @@ class Registry(object): Helper method that returns a generator used to iterate over all providers of the current site. """ - oauth2_slugs = OAuth2ProviderConfig.key_values('provider_slug', flat=True) + oauth2_slugs = OAuth2ProviderConfig.key_values('slug', flat=True) for oauth2_slug in oauth2_slugs: provider = OAuth2ProviderConfig.current(oauth2_slug) if provider.enabled_for_current_site and provider.backend_name in _PSA_OAUTH2_BACKENDS: yield provider if SAMLConfiguration.is_enabled(Site.objects.get_current(get_current_request()), 'default'): - idp_slugs = SAMLProviderConfig.key_values('idp_slug', flat=True) + idp_slugs = SAMLProviderConfig.key_values('slug', flat=True) for idp_slug in idp_slugs: provider = SAMLProviderConfig.current(idp_slug) if provider.enabled_for_current_site and provider.backend_name in _PSA_SAML_BACKENDS: @@ -112,14 +112,14 @@ class Registry(object): Instances of ProviderConfig. """ if backend_name in _PSA_OAUTH2_BACKENDS: - oauth2_slugs = OAuth2ProviderConfig.key_values('provider_slug', flat=True) + oauth2_slugs = OAuth2ProviderConfig.key_values('slug', flat=True) for oauth2_slug in oauth2_slugs: provider = OAuth2ProviderConfig.current(oauth2_slug) if provider.backend_name == backend_name and provider.enabled_for_current_site: yield provider elif backend_name in _PSA_SAML_BACKENDS and SAMLConfiguration.is_enabled( Site.objects.get_current(get_current_request()), 'default'): - idp_names = SAMLProviderConfig.key_values('idp_slug', flat=True) + idp_names = SAMLProviderConfig.key_values('slug', flat=True) for idp_name in idp_names: provider = SAMLProviderConfig.current(idp_name) if provider.backend_name == backend_name and provider.enabled_for_current_site: diff --git a/common/djangoapps/third_party_auth/tasks.py b/common/djangoapps/third_party_auth/tasks.py index be62d13892..ad19eda2be 100644 --- a/common/djangoapps/third_party_auth/tasks.py +++ b/common/djangoapps/third_party_auth/tasks.py @@ -47,7 +47,7 @@ def fetch_saml_metadata(): """ # First make a list of all the metadata XML URLs: - saml_providers = SAMLProviderConfig.key_values('idp_slug', flat=True) + saml_providers = SAMLProviderConfig.key_values('slug', flat=True) num_total = len(saml_providers) num_skipped = 0 url_map = {} diff --git a/common/djangoapps/third_party_auth/tests/factories.py b/common/djangoapps/third_party_auth/tests/factories.py index a6e4243be4..682ef84ec3 100644 --- a/common/djangoapps/third_party_auth/tests/factories.py +++ b/common/djangoapps/third_party_auth/tests/factories.py @@ -25,12 +25,13 @@ class SAMLProviderConfigFactory(DjangoModelFactory): """ class Meta(object): model = SAMLProviderConfig - django_get_or_create = ('idp_slug', 'metadata_source', "entity_id") + django_get_or_create = ('slug', 'metadata_source', "entity_id") site = SubFactory(SiteFactory) enabled = True idp_slug = "test-shib" + slug = "test-shib" name = "TestShib College" entity_id = "https://idp.testshib.org/idp/shibboleth" diff --git a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py index a6f480ce33..21712382fd 100644 --- a/common/djangoapps/third_party_auth/tests/specs/test_testshib.py +++ b/common/djangoapps/third_party_auth/tests/specs/test_testshib.py @@ -92,6 +92,7 @@ class SamlIntegrationTestUtilities(object): kwargs.setdefault('enabled', True) kwargs.setdefault('visible', True) kwargs.setdefault('idp_slug', self.PROVIDER_IDP_SLUG) + kwargs.setdefault('slug', self.PROVIDER_IDP_SLUG) kwargs.setdefault('entity_id', TESTSHIB_ENTITY_ID) kwargs.setdefault('metadata_source', TESTSHIB_METADATA_URL) kwargs.setdefault('icon_class', 'fa-university') @@ -200,6 +201,7 @@ class TestShibIntegrationTest(SamlIntegrationTestUtilities, IntegrationTestMixin kwargs.setdefault('enabled', True) kwargs.setdefault('visible', True) kwargs.setdefault('idp_slug', self.PROVIDER_IDP_SLUG) + kwargs.setdefault('slug', self.PROVIDER_IDP_SLUG) kwargs.setdefault('entity_id', TESTSHIB_ENTITY_ID) kwargs.setdefault('metadata_source', TESTSHIB_METADATA_URL_WITH_CACHE_DURATION) kwargs.setdefault('icon_class', 'fa-university') diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index 71c489ea57..f17f31f0d2 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -59,6 +59,7 @@ class RegistryTest(testutil.TestCase): enabled=True, name="Disallowed", idp_slug="test", + slug="test", backend_name="disallowed" ) self.assertEqual(len(provider.Registry.enabled()), 0) @@ -150,12 +151,12 @@ class RegistryTest(testutil.TestCase): google_provider = self.configure_google_provider(enabled=True) self.assertEqual(google_provider.id, provider.Registry.get(google_provider.provider_id).id) - def test_oauth2_provider_keyed_by_provider_slug(self): + def test_oauth2_provider_keyed_by_slug(self): """ - Regression test to ensure that the Registry properly fetches OAuth2ProviderConfigs that have a provider_slug + Regression test to ensure that the Registry properly fetches OAuth2ProviderConfigs that have a slug which doesn't match any of the possible backend_names. """ - google_provider = self.configure_google_provider(enabled=True, provider_slug='custom_slug') + google_provider = self.configure_google_provider(enabled=True, slug='custom_slug') self.assertIn(google_provider, provider.Registry._enabled_providers()) self.assertIn(google_provider, provider.Registry.get_enabled_by_backend_name('google-oauth2')) diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 4112d30b17..fdbc99d575 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -79,6 +79,7 @@ class ThirdPartyAuthTestMixin(object): def configure_oauth_provider(**kwargs): """ Update the settings for an OAuth2-based third party auth provider """ kwargs.setdefault('provider_slug', kwargs['backend_name']) + kwargs.setdefault('slug', kwargs['backend_name']) obj = OAuth2ProviderConfig(**kwargs) obj.save() return obj diff --git a/common/test/db_fixtures/third_party_auth.json b/common/test/db_fixtures/third_party_auth.json index 8bfc8855ac..2b8fe15bcc 100644 --- a/common/test/db_fixtures/third_party_auth.json +++ b/common/test/db_fixtures/third_party_auth.json @@ -11,6 +11,7 @@ "icon_image": null, "backend_name": "google-oauth2", "provider_slug": "google-oauth2", + "slug": "google-oauth2", "key": "test", "secret": "test", "site": 2, @@ -30,6 +31,7 @@ "icon_image": null, "backend_name": "facebook", "provider_slug": "facebook", + "slug": "facebook", "key": "test", "secret": "test", "site": 2, @@ -49,6 +51,7 @@ "icon_image": "test-icon.png", "backend_name": "dummy", "provider_slug": "dummy", + "slug": "dummy", "key": "", "secret": "", "site": 2, diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 0594a827ec..116833252a 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -500,6 +500,7 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi kwargs.setdefault('enabled', True) kwargs.setdefault('visible', True) kwargs.setdefault('idp_slug', idp_slug) + kwargs.setdefault('slug', idp_slug) kwargs.setdefault('entity_id', 'https://idp.testshib.org/idp/shibboleth') kwargs.setdefault('metadata_source', 'https://mock.testshib.org/metadata/testshib-providers.xml') kwargs.setdefault('icon_class', 'fa-university')