Merge pull request #15624 from open-craft/haikuginger/remove-sso-consent

[ENT-491] Remove SSO consent features; verify fully-populated consent URL generation

common/djangoapps/third_party_auth/middleware.py

@@ -23,32 +23,3 @@ class ExceptionMiddleware(SocialAuthExceptionMiddleware):
             redirect_uri = pipeline.AUTH_DISPATCH_URLS[auth_entry]
 
         return redirect_uri
-
-
-class PipelineQuarantineMiddleware(object):
-    """
-    Middleware flushes the session if a user agent with a quarantined session
-    attempts to leave the quarantined set of views.
-    """
-
-    def process_view(self, request, view_func, view_args, view_kwargs):  # pylint: disable=unused-argument
-        """
-        Check the session to see if we've quarantined the user to a particular
-        step of the authentication pipeline; if so, look up which modules the
-        user is allowed to browse to without breaking the pipeline. If the view
-        that's been requested is outside those modules, then flush the session.
-
-        In general, this middleware should be used in cases where allowing the
-        user to exit the running pipeline would be undesirable, and where it'd
-        be better to flush the session state rather than allow it. Pipeline
-        quarantining is utilized by the Enterprise application to enforce
-        collection of user consent for sharing data with a linked third-party
-        authentication provider.
-        """
-        if not pipeline.running(request):
-            return
-
-        view_module = view_func.__module__
-        quarantined_modules = request.session.get('third_party_auth_quarantined_modules')
-        if quarantined_modules is not None and not any(view_module.startswith(mod) for mod in quarantined_modules):
-            request.session.flush()

common/djangoapps/third_party_auth/settings.py

@@ -10,12 +10,9 @@ If true, it:
     b) calls apply_settings(), passing in the Django settings
 """
 
-from openedx.features.enterprise_support.api import insert_enterprise_pipeline_elements
-
 _FIELDS_STORED_IN_SESSION = ['auth_entry', 'next']
 _MIDDLEWARE_CLASSES = (
     'third_party_auth.middleware.ExceptionMiddleware',
-    'third_party_auth.middleware.PipelineQuarantineMiddleware',
 )
 _SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard'
 
@@ -58,9 +55,6 @@ def apply_settings(django_settings):
         'third_party_auth.pipeline.login_analytics',
     ]
 
-    # Add enterprise pipeline elements if the enterprise app is installed
-    insert_enterprise_pipeline_elements(django_settings.SOCIAL_AUTH_PIPELINE)
-
     # Required so that we can use unmodified PSA OAuth2 backends:
     django_settings.SOCIAL_AUTH_STRATEGY = 'third_party_auth.strategy.ConfigurationModelStrategy'
 

common/djangoapps/third_party_auth/tests/test_middleware.py

-"""
-Test the session-flushing middleware
-"""
-import unittest
-
-from django.conf import settings
-from django.test import Client
-from social_django.models import Partial
-
-
-@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class TestSessionFlushMiddleware(unittest.TestCase):
-    """
-    Ensure that if the pipeline is exited when it's been quarantined,
-    the entire session is flushed.
-    """
-    def setUp(self):
-        self.client = Client()
-        self.fancy_variable = 13025
-        self.token = 'pipeline_running'
-        self.tpa_quarantined_modules = ('fake_quarantined_module',)
-
-    def tearDown(self):
-        Partial.objects.all().delete()
-
-    def test_session_flush(self):
-        """
-        Test that a quarantined session is flushed when navigating elsewhere
-        """
-        session = self.client.session
-        session['fancy_variable'] = self.fancy_variable
-        session['partial_pipeline_token'] = self.token
-        session['third_party_auth_quarantined_modules'] = self.tpa_quarantined_modules
-        session.save()
-        Partial.objects.create(token=session.get('partial_pipeline_token'))
-        self.client.get('/')
-        self.assertEqual(self.client.session.get('fancy_variable', None), None)
-
-    def test_session_no_running_pipeline(self):
-        """
-        Test that a quarantined session without a running pipeline is not flushed
-        """
-        session = self.client.session
-        session['fancy_variable'] = self.fancy_variable
-        session['third_party_auth_quarantined_modules'] = self.tpa_quarantined_modules
-        session.save()
-        self.client.get('/')
-        self.assertEqual(self.client.session.get('fancy_variable', None), self.fancy_variable)
-
-    def test_session_no_quarantine(self):
-        """
-        Test that a session with a running pipeline but no quarantine is not flushed
-        """
-        session = self.client.session
-        session['fancy_variable'] = self.fancy_variable
-        session['partial_pipeline_token'] = self.token
-        session.save()
-        Partial.objects.create(token=session.get('partial_pipeline_token'))
-        self.client.get('/')
-        self.assertEqual(self.client.session.get('fancy_variable', None), self.fancy_variable)

common/djangoapps/third_party_auth/tests/test_settings.py

@@ -2,7 +2,6 @@
 
 import unittest
 
-from openedx.features.enterprise_support.api import enterprise_enabled
 from third_party_auth import provider, settings
 from third_party_auth.tests import testutil
 
@@ -56,8 +55,3 @@ class SettingsUnitTest(testutil.TestCase):
         # bad in prod.
         settings.apply_settings(self.settings)
         self.assertFalse(self.settings.SOCIAL_AUTH_RAISE_EXCEPTIONS)
-
-    @unittest.skipUnless(enterprise_enabled(), 'enterprise not enabled')
-    def test_enterprise_elements_inserted(self):
-        settings.apply_settings(self.settings)
-        self.assertIn('enterprise.tpa_pipeline.handle_enterprise_logistration', self.settings.SOCIAL_AUTH_PIPELINE)

lms/djangoapps/courseware/tests/test_course_info.py

@@ -388,7 +388,7 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest
         self.assertEqual(resp.status_code, 200)
 
     def test_num_queries_instructor_paced(self):
-        self.fetch_course_info_with_queries(self.instructor_paced_course, 25, 3)
+        self.fetch_course_info_with_queries(self.instructor_paced_course, 24, 3)
 
     def test_num_queries_self_paced(self):
-        self.fetch_course_info_with_queries(self.self_paced_course, 25, 3)
+        self.fetch_course_info_with_queries(self.self_paced_course, 24, 3)

lms/djangoapps/courseware/tests/test_views.py

@@ -211,8 +211,8 @@ class IndexQueryTestCase(ModuleStoreTestCase):
     NUM_PROBLEMS = 20
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 10, 143),
-        (ModuleStoreEnum.Type.split, 4, 143),
+        (ModuleStoreEnum.Type.mongo, 10, 142),
+        (ModuleStoreEnum.Type.split, 4, 142),
     )
     @ddt.unpack
     def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count):
@@ -1464,12 +1464,12 @@ class ProgressPageTests(ProgressPageBaseTests):
         """Test that query counts remain the same for self-paced and instructor-paced courses."""
         SelfPacedConfiguration(enabled=self_paced_enabled).save()
         self.setup_course(self_paced=self_paced)
-        with self.assertNumQueries(40, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1):
+        with self.assertNumQueries(39, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST), check_mongo_calls(1):
             self._get_progress_page()
 
     @ddt.data(
-        (False, 40, 26),
-        (True, 33, 22)
+        (False, 39, 25),
+        (True, 32, 21)
     )
     @ddt.unpack
     def test_progress_queries(self, enable_waffle, initial, subsequent):

lms/djangoapps/django_comment_client/base/tests.py

@@ -404,8 +404,8 @@ class ViewsQueryCountTestCase(
         return inner
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 4, 32),
-        (ModuleStoreEnum.Type.split, 3, 12, 32),
+        (ModuleStoreEnum.Type.mongo, 3, 4, 31),
+        (ModuleStoreEnum.Type.split, 3, 12, 31),
     )
     @ddt.unpack
     @count_queries
@@ -413,8 +413,8 @@ class ViewsQueryCountTestCase(
         self.create_thread_helper(mock_request)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 3, 28),
-        (ModuleStoreEnum.Type.split, 3, 9, 28),
+        (ModuleStoreEnum.Type.mongo, 3, 3, 27),
+        (ModuleStoreEnum.Type.split, 3, 9, 27),
     )
     @ddt.unpack
     @count_queries

openedx/core/djangoapps/bookmarks/tests/test_views.py

@@ -268,7 +268,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
         self.assertEqual(response.data['developer_message'], u'Parameter usage_id not provided.')
 
         # Send empty data dictionary.
-        with self.assertNumQueries(8):  # No queries for bookmark table.
+        with self.assertNumQueries(7):  # No queries for bookmark table.
             response = self.send_post(
                 client=self.client,
                 url=reverse('bookmarks'),

openedx/core/djangoapps/user_api/accounts/tests/test_views.py

@@ -174,7 +174,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase):
         Test that a client (logged in) can get her own username.
         """
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
-        self._verify_get_own_username(15)
+        self._verify_get_own_username(14)
 
     def test_get_username_inactive(self):
         """
@@ -184,7 +184,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase):
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
         self.user.is_active = False
         self.user.save()
-        self._verify_get_own_username(15)
+        self._verify_get_own_username(14)
 
     def test_get_username_not_logged_in(self):
         """
@@ -193,7 +193,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase):
         """
 
         # verify that the endpoint is inaccessible when not logged in
-        self._verify_get_own_username(13, expected_status=401)
+        self._verify_get_own_username(12, expected_status=401)
 
 
 @ddt.ddt
@@ -305,7 +305,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
         """
         self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
         self.create_mock_profile(self.user)
-        with self.assertNumQueries(19):
+        with self.assertNumQueries(18):
             response = self.send_get(self.different_client)
         self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY)
 
@@ -320,7 +320,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
         """
         self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
         self.create_mock_profile(self.user)
-        with self.assertNumQueries(19):
+        with self.assertNumQueries(18):
             response = self.send_get(self.different_client)
         self._verify_private_account_response(response, account_privacy=PRIVATE_VISIBILITY)
 
@@ -395,12 +395,12 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
             self.assertEqual(False, data["accomplishments_shared"])
 
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
-        verify_get_own_information(17)
+        verify_get_own_information(16)
 
         # Now make sure that the user can get the same information, even if not active
         self.user.is_active = False
         self.user.save()
-        verify_get_own_information(11)
+        verify_get_own_information(10)
 
     def test_get_account_empty_string(self):
         """
@@ -414,7 +414,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase):
         legacy_profile.save()
 
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
-        with self.assertNumQueries(17):
+        with self.assertNumQueries(16):
             response = self.send_get(self.client)
         for empty_field in ("level_of_education", "gender", "country", "bio"):
             self.assertIsNone(response.data[empty_field])

openedx/features/course_experience/tests/views/test_course_home.py

@@ -142,7 +142,7 @@ class TestCourseHomePage(CourseHomePageTestCase):
         course_home_url(self.course)
 
         # Fetch the view and verify the query counts
-        with self.assertNumQueries(38, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
+        with self.assertNumQueries(37, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
             with check_mongo_calls(4):
                 url = course_home_url(self.course)
                 self.client.get(url)

openedx/features/course_experience/tests/views/test_course_updates.py

@@ -127,7 +127,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase):
         course_updates_url(self.course)
 
         # Fetch the view and verify that the query counts haven't changed
-        with self.assertNumQueries(31, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
+        with self.assertNumQueries(30, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST):
             with check_mongo_calls(4):
                 url = course_updates_url(self.course)
                 self.client.get(url)

openedx/features/enterprise_support/api.py

@@ -26,7 +26,6 @@ from openedx.core.lib.token_utils import JwtBuilder
 try:
     from enterprise import utils as enterprise_utils
     from enterprise.models import EnterpriseCourseEnrollment, EnterpriseCustomer
-    from enterprise.tpa_pipeline import get_enterprise_customer_for_request
     from enterprise.utils import consent_necessary_for_course
 except ImportError:
     pass
@@ -240,9 +239,9 @@ def enterprise_customer_for_request(request, tpa_hint=None):
     if not enterprise_enabled():
         return None
 
-    ec = get_enterprise_customer_for_request(request)
+    ec = None
 
-    if not ec and tpa_hint:
+    if tpa_hint:
         try:
             ec = EnterpriseCustomer.objects.get(enterprise_customer_identity_provider__provider_id=tpa_hint)
         except EnterpriseCustomer.DoesNotExist:
@@ -308,24 +307,6 @@ def get_enterprise_consent_url(request, course_id, user=None, return_to=None):
     return full_url
 
 
-def insert_enterprise_pipeline_elements(pipeline):
-    """
-    If the enterprise app is enabled, insert additional elements into the
-    pipeline so that data sharing consent views are used.
-    """
-    if not enterprise_enabled():
-        return
-
-    additional_elements = (
-        'enterprise.tpa_pipeline.handle_enterprise_logistration',
-    )
-    # Find the item we need to insert the data sharing consent elements before
-    insert_point = pipeline.index('social_core.pipeline.social_auth.load_extra_data')
-
-    for index, element in enumerate(additional_elements):
-        pipeline.insert(insert_point + index, element)
-
-
 def get_cache_key(**kwargs):
     """
     Get MD5 encoded cache key for given arguments.

openedx/features/enterprise_support/tests/test_api.py

@@ -14,7 +14,6 @@ from openedx.features.enterprise_support.api import (
     enterprise_enabled,
     get_dashboard_consent_notification,
     get_enterprise_consent_url,
-    insert_enterprise_pipeline_elements
 )
 
 
@@ -24,33 +23,9 @@ class TestEnterpriseApi(unittest.TestCase):
     Test enterprise support APIs.
     """
 
-    @override_settings(ENABLE_ENTERPRISE_INTEGRATION=False)
-    def test_utils_with_enterprise_disabled(self):
-        """
-        Test that disabling the enterprise integration flag causes
-        the utilities to return the expected default values.
-        """
-        self.assertFalse(enterprise_enabled())
-        self.assertEqual(insert_enterprise_pipeline_elements(None), None)
-
-    @override_settings(ENABLE_ENTERPRISE_INTEGRATION=True)
-    def test_utils_with_enterprise_enabled(self):
-        """
-        Test that enabling enterprise integration (which is currently on by default) causes the
-        the utilities to return the expected values.
-        """
-        self.assertTrue(enterprise_enabled())
-        pipeline = ['abc', 'social_core.pipeline.social_auth.load_extra_data', 'def']
-        insert_enterprise_pipeline_elements(pipeline)
-        self.assertEqual(pipeline, ['abc',
-                                    'enterprise.tpa_pipeline.handle_enterprise_logistration',
-                                    'social_core.pipeline.social_auth.load_extra_data',
-                                    'def'])
-
     @override_settings(ENABLE_ENTERPRISE_INTEGRATION=True)
-    @mock.patch('openedx.features.enterprise_support.api.get_enterprise_customer_for_request')
     @mock.patch('openedx.features.enterprise_support.api.EnterpriseCustomer')
-    def test_enterprise_customer_for_request(self, ec_class_mock, get_ec_pipeline_mock):
+    def test_enterprise_customer_for_request(self, ec_class_mock):
         """
         Test that the correct EnterpriseCustomer, if any, is returned.
         """
@@ -68,8 +43,6 @@ class TestEnterpriseApi(unittest.TestCase):
         ec_class_mock.DoesNotExist = Exception
         ec_class_mock.objects.get.side_effect = get_ec_mock
 
-        get_ec_pipeline_mock.return_value = None
-
         request = mock.MagicMock()
         request.GET.get.return_value = 'real-uuid'
         self.assertEqual(enterprise_customer_for_request(request), 'this-is-actually-an-enterprise-customer')
@@ -85,10 +58,6 @@ class TestEnterpriseApi(unittest.TestCase):
         self.assertEqual(enterprise_customer_for_request(request, tpa_hint='fake-provider-id'), None)
         self.assertEqual(enterprise_customer_for_request(request, tpa_hint=None), None)
 
-        get_ec_pipeline_mock.return_value = 'also-a-real-enterprise'
-
-        self.assertEqual(enterprise_customer_for_request(request), 'also-a-real-enterprise')
-
     def check_data_sharing_consent(self, consent_required=False, consent_url=None):
         """
         Used to test the data_sharing_consent_required view decorator.

requirements/edx/base.txt

@@ -51,7 +51,7 @@ edx-lint==0.4.3
 astroid==1.3.8
 edx-django-oauth2-provider==1.1.4
 edx-django-sites-extensions==2.3.0
-edx-enterprise==0.38.5
+edx-enterprise==0.38.6
 edx-oauth2-provider==1.2.0
 edx-opaque-keys==0.4.0
 edx-organizations==0.4.5