From d8e1b917efe505fca33c05cec238678a25d5c7e2 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 17 Apr 2017 14:18:01 -0400 Subject: [PATCH] ENT-320: Fix OAuth2ProviderConfig to be keyed by `provider_slug` This change fixes a bug where OAuth2 Provider Configs only show up on logistration if the provider's `provider_slug` matches a valid OAuth2 backend name. Closes ENT-320. --- common/djangoapps/third_party_auth/admin.py | 2 +- common/djangoapps/third_party_auth/provider.py | 15 +++++++++------ .../third_party_auth/tests/test_provider.py | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/third_party_auth/admin.py b/common/djangoapps/third_party_auth/admin.py index 8ba67183c3..7877e24778 100644 --- a/common/djangoapps/third_party_auth/admin.py +++ b/common/djangoapps/third_party_auth/admin.py @@ -33,7 +33,7 @@ class OAuth2ProviderConfigAdmin(KeyedConfigurationModelAdmin): def get_list_display(self, request): """ Don't show every single field in the admin change list """ return ( - 'name', 'enabled', 'site', 'backend_name', 'secondary', 'skip_registration_form', + 'name', 'enabled', 'provider_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/provider.py b/common/djangoapps/third_party_auth/provider.py index 0bfb602c89..1b3d04ac52 100644 --- a/common/djangoapps/third_party_auth/provider.py +++ b/common/djangoapps/third_party_auth/provider.py @@ -22,9 +22,10 @@ class Registry(object): Helper method that returns a generator used to iterate over all providers of the current site. """ - for backend_name in _PSA_OAUTH2_BACKENDS: - provider = OAuth2ProviderConfig.current(backend_name) - if provider.enabled_for_current_site: + oauth2_slugs = OAuth2ProviderConfig.key_values('provider_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())): idp_slugs = SAMLProviderConfig.key_values('idp_slug', flat=True) @@ -103,9 +104,11 @@ class Registry(object): Instances of ProviderConfig. """ if backend_name in _PSA_OAUTH2_BACKENDS: - provider = OAuth2ProviderConfig.current(backend_name) - if provider.enabled_for_current_site: - yield provider + oauth2_slugs = OAuth2ProviderConfig.key_values('provider_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())): idp_names = SAMLProviderConfig.key_values('idp_slug', flat=True) diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index b38bdc4ed2..bab94b01ee 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -148,6 +148,24 @@ 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): + """ + Regression test to ensure that the Registry properly fetches OAuth2ProviderConfigs that have a provider_slug + which doesn't match any of the possible backend_names. + """ + google_provider = self.configure_google_provider(enabled=True, provider_slug='custom_slug') + self.assertIn(google_provider, provider.Registry._enabled_providers()) + self.assertIn(google_provider, provider.Registry.get_enabled_by_backend_name('google-oauth2')) + + def test_oauth2_enabled_only_for_supplied_backend(self): + """ + Test to ensure that Registry.get_enabled_by_backend_name doesn't return OAuth2 providers with incorrect + backend_names. + """ + facebook_provider = self.configure_facebook_provider(enabled=True) + self.configure_google_provider(enabled=True) + self.assertNotIn(facebook_provider, provider.Registry.get_enabled_by_backend_name('google-oauth2')) + def test_get_returns_none_if_provider_not_enabled(self): linkedin_provider_id = "oa2-linkedin-oauth2" # At this point there should be no configuration entries at all so no providers should be enabled