Dark launch: Fix 1+N queries in third party auth config fetching

common/djangoapps/third_party_auth/provider.py

@@ -4,8 +4,11 @@
 
 
 from django.contrib.sites.models import Site
+import edx_django_utils.monitoring as monitoring_utils
+from more_itertools import unique_everseen
 
 from openedx.core.djangoapps.theming.helpers import get_current_request
+from openedx.core.lib.cache_utils import request_cached
 
 from .models import (
     _LTI_BACKENDS,
@@ -17,6 +20,29 @@
     SAMLProviderConfig
 )
 
+CACHE_NAMESPACE = 'third_party_auth.provider'
+
+
+@request_cached(namespace=CACHE_NAMESPACE)
+def current_configs_for_site(provider_cls, site_id):
+    """
+    Like ConfigurationModel.current() but returns the current config in every
+    partition as delineated by KEY_FIELDS, but only looking at configs in the
+    specified site.
+
+    This is a hack to support these provider configs, which do not include site
+    in their KEY_FIELDS but need to be partitioned on site. See comment in
+    ``_enabled_providers`` for more details.
+    """
+
+    def partition_key(obj):
+        """Compute the partition key for this object."""
+        return tuple(getattr(obj, k) for k in provider_cls.KEY_FIELDS)
+
+    configs = provider_cls.objects.filter(site_id=str(site_id))
+    # Get the most recent record in each partition
+    return unique_everseen(configs.order_by('-change_date'), partition_key)
+
 
 class Registry(object):
     """
@@ -27,9 +53,40 @@ class Registry(object):
     @classmethod
     def _enabled_providers(cls):
         """
-        Helper method that returns a generator used to iterate over all providers
-        of the current site.
+        Generator returning all enabled providers of the current site.
+
+        Provider configurations are partitioned on site + some key (backend
+        name in the case of OAuth, slug for SAML, and consumer key for LTI).
         """
+        try:
+            old_values = set(cls._enabled_providers_old())
+        except:
+            # Make sure we have an error baseline to compare to
+            monitoring_utils.increment("temp_tpa_enabled_all_dark_launch_old_threw")
+            raise
+
+        # Dark launch call and metrics
+        try:
+            new_values = set(cls._enabled_providers_new())
+
+            try:
+                if old_values != new_values:
+                    data = "old[%s]new[%s]" % (
+                        ','.join([str(p.id) for p in old_values]),
+                        ','.join([str(p.id) for p in new_values]))
+                    monitoring_utils.set_custom_metric("temp_tpa_enabled_all_dark_launch_mismatch", data)
+            except:  # pylint: disable=bare-except
+                pass
+        except:  # pylint: disable=bare-except
+            monitoring_utils.increment("temp_tpa_enabled_all_dark_launch_new_threw")
+
+        # Could return old_values, but lets keep it a generator for consistency
+        for config in old_values:
+            yield config
+
+    @classmethod
+    def _enabled_providers_old(cls):
+        """Old implementation of _enabled_providers"""
         oauth2_backend_names = OAuth2ProviderConfig.key_values('backend_name', flat=True)
         for oauth2_backend_name in oauth2_backend_names:
             provider = OAuth2ProviderConfig.current(oauth2_backend_name)
@@ -46,6 +103,45 @@ def _enabled_providers(cls):
             if provider.enabled_for_current_site and provider.backend_name in _LTI_BACKENDS:
                 yield provider
 
+    @classmethod
+    def _enabled_providers_new(cls):
+        """New implementation of _enabled_providers for dark launch"""
+        site = Site.objects.get_current(get_current_request())
+
+        # Note that site is added as an explicit filter. 'site_id' isn't in the
+        # KEY_FIELDS for these models, but it should be, since we want "the most
+        # recent version of the config" to be most recent for a given key *and*
+        # site. Since site is not in KEY_FIELDS, looking up a config by just its
+        # key could result in finding a config for a different site. (The
+        # previous code here would then have returned nothing, having had a
+        # site-filtering step.) In short, we need to be careful not to let two
+        # configs belonging to two different sites but with the same key
+        # "shadow" one another.
+        #
+        # It's not clear if we can safely add site_id to the KEY_FIELDS, and
+        # it's also not clear if we're even going to want to keep support for
+        # sites here long term, so in the meantime we just only ask for
+        # configs within the current request's site.
+
+        # Get all OAuth2 provider configs for this site...
+        for config in current_configs_for_site(OAuth2ProviderConfig, site.pk):
+            # Ignore the config if it's disabled or we don't have a backend
+            # class for it. (Not having a backend class for a config should
+            # be pretty rare if it happens at all.)
+            if config.enabled and config.backend_name in _PSA_OAUTH2_BACKENDS:
+                yield config
+
+        if SAMLConfiguration.is_enabled(site, 'default'):
+            # ...followed by SAML configs, if feature is enabled...
+            for config in current_configs_for_site(SAMLProviderConfig, site.pk):
+                if config.enabled and config.backend_name in _PSA_SAML_BACKENDS:
+                    yield config
+
+        # ...and finally LTI configs
+        for config in current_configs_for_site(LTIProviderConfig, site.pk):
+            if config.enabled and config.backend_name in _LTI_BACKENDS:
+                yield config
+
     @classmethod
     def enabled(cls):
         """Returns list of enabled providers."""
@@ -113,6 +209,36 @@ def get_enabled_by_backend_name(cls, backend_name):
         Yields:
             Instances of ProviderConfig.
         """
+        try:
+            old_values = set(cls._get_enabled_by_backend_name_old(backend_name))
+        except:
+            # Make sure we have an error baseline to compare to
+            monitoring_utils.increment("temp_tpa_by_backend_dark_launch_old_threw")
+            raise
+
+        # Dark launch call and metrics
+        try:
+            new_values = set(cls._get_enabled_by_backend_name_new(backend_name))
+
+            try:
+                if old_values != new_values:
+                    data = "backend[%s]old[%s]new[%s]" % (
+                        backend_name,
+                        ','.join([str(p.id) for p in old_values]),
+                        ','.join([str(p.id) for p in new_values]))
+                    monitoring_utils.set_custom_metric("temp_tpa_by_backend_dark_launch_mismatch", data)
+            except:  # pylint: disable=bare-except
+                pass
+        except:  # pylint: disable=bare-except
+            monitoring_utils.increment("temp_tpa_by_backend_dark_launch_new_threw")
+
+        # Could return old_values, but lets keep it a generator for consistency
+        for config in old_values:
+            yield config
+
+    @classmethod
+    def _get_enabled_by_backend_name_old(cls, backend_name):
+        """Old implementation of get_enabled_by_backend_name"""
         if backend_name in _PSA_OAUTH2_BACKENDS:
             oauth2_backend_names = OAuth2ProviderConfig.key_values('backend_name', flat=True)
             for oauth2_backend_name in oauth2_backend_names:
@@ -131,3 +257,10 @@ def get_enabled_by_backend_name(cls, backend_name):
                 provider = LTIProviderConfig.current(consumer_key)
                 if provider.backend_name == backend_name and provider.enabled_for_current_site:
                     yield provider
+
+    @classmethod
+    def _get_enabled_by_backend_name_new(cls, backend_name):
+        """New implementation of get_enabled_by_backend_name for dark launch"""
+        for provider in cls._enabled_providers():
+            if provider.backend_name == backend_name:
+                yield provider

common/djangoapps/third_party_auth/tests/test_provider.py

@@ -147,6 +147,48 @@ def test_provider_disabled_for_mismatching_site(self):
         prov = self.configure_google_provider(visible=True, enabled=True, site=site_b)
         self.assertEqual(prov.enabled_for_current_site, False)
 
+    @with_site_configuration(SITE_DOMAIN_A)
+    @patch('edx_django_utils.monitoring.set_custom_metric')
+    def test_providers_with_same_key_independent_across_sites(self, mock_set_custom_metric):
+        """
+        Verify that having two providers configured with the same key
+        but for different sites do not shadow each other on retrieval.
+        """
+        site_a = Site.objects.get_or_create(domain=SITE_DOMAIN_A, name=SITE_DOMAIN_A)[0]
+        site_b = Site.objects.get_or_create(domain=SITE_DOMAIN_B, name=SITE_DOMAIN_B)[0]
+
+        self.enable_saml(site=site_a)
+        self.enable_saml(site=site_b)
+        configs = [
+            self.configure_saml_provider(enabled=True, site=site_b, slug="x", name="Site B first"),
+            self.configure_saml_provider(enabled=True, site=site_a, slug="x", name="Site A first"),
+            self.configure_saml_provider(enabled=True, site=site_a, slug="x", name="Site A second"),
+            self.configure_saml_provider(enabled=True, site=site_b, slug="x", name="Site B second"),
+        ]
+
+        # If sites are not partitioned in retrieval, a site_b record can appear
+        # to be the "current" config, and then be ignored by site_a (resulting
+        # in zero enabled providers for site_a).
+
+        # TODO(ARCHBOM-1139) Uncomment when finished with dark launch in provider.py
+        # self.assertEqual([p.name for p in provider.Registry.enabled()], ["Site A second"])
+        # Until then, just run the generator to its end and show that the dark launch would complain here
+        list(provider.Registry.enabled())
+        mock_set_custom_metric.assert_any_call('temp_tpa_enabled_all_dark_launch_mismatch',
+                                               'old[]new[%s]' % configs[2].id)
+
+    def test_mixed_providers(self):
+        """
+        Verify that multiple providers types can be configured at same time.
+        """
+        self.enable_saml()
+        self.configure_saml_provider(enabled=True, slug="saml-slug", name="Some SAML")
+        self.configure_google_provider(enabled=True)
+        self.configure_lti_provider(enabled=True)
+
+        configs = provider.Registry.enabled()
+        self.assertEqual({p.backend_name for p in configs}, {'tpa-saml', 'google-oauth2', 'lti'})
+
     def test_get_returns_enabled_provider(self):
         google_provider = self.configure_google_provider(enabled=True)
         self.assertEqual(google_provider.id, provider.Registry.get(google_provider.provider_id).id)

requirements/edx/base.in

@@ -103,6 +103,7 @@ mailsnake                           # Needed for mailchimp (mailing djangoapp)
 mako==1.0.2                         # Primary template language used for server-side page rendering
 Markdown                            # Convert text markup to HTML; used in capa problems, forums, and course wikis
 mongoengine==0.10.0                 # Object-document mapper for MongoDB, used in the LMS dashboard
+more-itertools                      # Additional utilities for working with iterators
 mysqlclient                         # Driver for the default production relational database
 newrelic                            # New Relic agent for performance monitoring
 nodeenv                             # Utility for managing Node.js environments; we use this for deployments and testing

requirements/edx/base.txt

@@ -160,7 +160,7 @@ maxminddb==1.5.2          # via geoip2
 mock==3.0.5               # via -c requirements/edx/../constraints.txt, -r requirements/edx/paver.txt, xblock-drag-and-drop-v2, xblock-poll
 git+https://github.com/edx/MongoDBProxy.git@d92bafe9888d2940f647a7b2b2383b29c752f35a#egg=MongoDBProxy==0.1.0+edx.2  # via -r requirements/edx/github.in
 mongoengine==0.10.0       # via -r requirements/edx/base.in
-more-itertools==8.2.0     # via -r requirements/edx/paver.txt, zipp
+more-itertools==8.2.0     # via -r requirements/edx/base.in, -r requirements/edx/paver.txt, zipp
 mpmath==1.1.0             # via sympy
 mysqlclient==1.4.6        # via -r requirements/edx/base.in
 newrelic==5.12.0.140      # via -r requirements/edx/base.in, edx-django-utils