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 d80c914666..625246d321 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 @@ -120,22 +120,22 @@ class TestSAMLCommand(CacheIsolationTestCase): return old_config, new_config, test_provider_config - def __create_saml_configurations__(self, saml_config=None, saml_provider_config=None): + def _create_saml_configurations(self, saml_config=None, saml_provider_config=None): """ - Helper method to create SAMLConfiguration and AMLProviderConfig. + Helper method to create SAMLConfiguration and SAMLProviderConfig. """ SAMLConfigurationFactory.create(enabled=True, **( saml_config or { - 'site__domain': 'testserver.fake', - 'site__name': 'testserver.fake', + 'site__domain': 'first.testserver.fake', + 'site__name': 'first.testserver.fake', } )) SAMLProviderConfigFactory.create(enabled=True, **( saml_provider_config or { - 'site__domain': 'testserver.fake', - 'site__name': 'testserver.fake', - 'slug': 'test-shib', - 'name': 'TestShib College', + 'site__domain': 'first.testserver.fake', + 'site__name': 'first.testserver.fake', + 'slug': 'first-test-shib', + 'name': 'First TestShib College', 'entity_id': 'https://idp.testshib.org/idp/shibboleth', 'metadata_source': 'https://www.testshib.org/metadata/testshib-providers.xml', } @@ -168,9 +168,9 @@ class TestSAMLCommand(CacheIsolationTestCase): one or more saml configurations are enabled. """ # Create enabled configurations - self.__create_saml_configurations__() + self._create_saml_configurations() - expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n1 updated and 0 failed.\n" + expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n1 updated and 0 failed.\n" call_command("saml", pull=True, stdout=self.stdout) assert expected in self.stdout.getvalue() @@ -181,9 +181,9 @@ class TestSAMLCommand(CacheIsolationTestCase): and logs correct information. """ # Create enabled configurations - self.__create_saml_configurations__() + self._create_saml_configurations() - expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n0 updated and 1 failed.\n" + expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n0 updated and 1 failed.\n" with self.assertRaisesRegex(CommandError, r"HTTPError: 404 Client Error"): call_command("saml", pull=True, stdout=self.stdout) @@ -196,10 +196,10 @@ class TestSAMLCommand(CacheIsolationTestCase): and logs correct information when there are multiple providers with their data. """ # Create enabled configurations - self.__create_saml_configurations__() + self._create_saml_configurations() # Add another set of configurations - self.__create_saml_configurations__( + self._create_saml_configurations( saml_config={ "site__domain": "second.testserver.fake", "site__name": "testserver.fake", @@ -214,7 +214,7 @@ class TestSAMLCommand(CacheIsolationTestCase): ) # Add another set of configurations - self.__create_saml_configurations__( + self._create_saml_configurations( saml_config={ "site__domain": "third.testserver.fake", "site__name": "testserver.fake", @@ -229,13 +229,13 @@ class TestSAMLCommand(CacheIsolationTestCase): } ) - expected = '\n3 provider(s) found in database.\n0 skipped and 3 attempted.\n2 updated and 1 failed.\n' + expected = '\n4 provider(s) found in database.\n1 skipped and 3 attempted.\n2 updated and 1 failed.\n' with self.assertRaisesRegex(CommandError, r"MetadataParseError: Can't find EntityDescriptor for entityID"): call_command("saml", pull=True, stdout=self.stdout) assert expected in self.stdout.getvalue() # Now add a fourth configuration, and indicate that it should not be included in the update - self.__create_saml_configurations__( + self._create_saml_configurations( saml_config={ "site__domain": "fourth.testserver.fake", "site__name": "testserver.fake", @@ -251,8 +251,8 @@ class TestSAMLCommand(CacheIsolationTestCase): } ) - # Four configurations -- one will be skipped and three attempted, with similar results. - expected = '\nDone.\n4 provider(s) found in database.\n1 skipped and 3 attempted.\n0 updated and 1 failed.\n' + # Four test configurations plus setUp -- two will be skipped and three attempted, with similar results. + expected = '\nDone.\n5 provider(s) found in database.\n2 skipped and 3 attempted.\n0 updated and 1 failed.\n' with self.assertRaisesRegex(CommandError, r"MetadataParseError: Can't find EntityDescriptor for entityID"): call_command("saml", pull=True, stdout=self.stdout) assert expected in self.stdout.getvalue() @@ -263,11 +263,11 @@ class TestSAMLCommand(CacheIsolationTestCase): Test that management command errors out in case of fatal exceptions instead of failing silently. """ # Create enabled configurations - self.__create_saml_configurations__() + self._create_saml_configurations() mocked_get.side_effect = exceptions.SSLError - expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n0 updated and 1 failed.\n" + expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n0 updated and 1 failed.\n" with self.assertRaisesRegex(CommandError, "SSLError:"): call_command("saml", pull=True, stdout=self.stdout) @@ -291,7 +291,7 @@ class TestSAMLCommand(CacheIsolationTestCase): Test that management command errors out in case of fatal exceptions instead of failing silently. """ # Create enabled configurations, this configuration will raise MetadataParseError. - self.__create_saml_configurations__( + self._create_saml_configurations( saml_config={ "site__domain": "third.testserver.fake", }, @@ -322,9 +322,9 @@ class TestSAMLCommand(CacheIsolationTestCase): mocked_get.return_value = response # create enabled configuration - self.__create_saml_configurations__() + self._create_saml_configurations() - expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n0 updated and 1 failed.\n" + expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n0 updated and 1 failed.\n" with self.assertRaisesRegex(CommandError, "XMLSyntaxError:"): call_command("saml", pull=True, stdout=self.stdout) diff --git a/common/djangoapps/third_party_auth/signals/handlers.py b/common/djangoapps/third_party_auth/signals/handlers.py index 6364c798f5..4eceef18c0 100644 --- a/common/djangoapps/third_party_auth/signals/handlers.py +++ b/common/djangoapps/third_party_auth/signals/handlers.py @@ -7,7 +7,6 @@ from django.dispatch import receiver from edx_django_utils.monitoring import set_custom_attribute from common.djangoapps.third_party_auth.models import SAMLConfiguration, SAMLProviderConfig -from common.djangoapps.third_party_auth.toggles import ENABLE_SAML_CONFIG_SIGNAL_HANDLERS @receiver(post_save, sender=SAMLConfiguration) @@ -20,10 +19,6 @@ def update_saml_provider_configs_on_configuration_change(sender, instance, creat configuration version, ensuring all providers remain aligned with the most current settings. """ - # .. custom_attribute_name: saml_config_signal.enabled - # .. custom_attribute_description: Tracks whether the SAML config signal handler is enabled. - set_custom_attribute('saml_config_signal.enabled', ENABLE_SAML_CONFIG_SIGNAL_HANDLERS.is_enabled()) - # .. custom_attribute_name: saml_config_signal.new_config_id # .. custom_attribute_description: Records the ID of the new SAML configuration instance. set_custom_attribute('saml_config_signal.new_config_id', instance.id) @@ -32,26 +27,25 @@ def update_saml_provider_configs_on_configuration_change(sender, instance, creat # .. custom_attribute_description: Records the slug of the SAML configuration instance. set_custom_attribute('saml_config_signal.slug', instance.slug) - if ENABLE_SAML_CONFIG_SIGNAL_HANDLERS.is_enabled(): - try: - # Find all existing SAMLProviderConfig instances (current_set) that should be - # pointing to this slug but are pointing to an older version - existing_providers = SAMLProviderConfig.objects.current_set().filter( - saml_configuration__site_id=instance.site_id, - saml_configuration__slug=instance.slug - ).exclude(saml_configuration_id=instance.id).exclude(saml_configuration_id__isnull=True) + try: + # Find all existing SAMLProviderConfig instances (current_set) that should be + # pointing to this slug but are pointing to an older version + existing_providers = SAMLProviderConfig.objects.current_set().filter( + saml_configuration__site_id=instance.site_id, + saml_configuration__slug=instance.slug + ).exclude(saml_configuration_id=instance.id).exclude(saml_configuration_id__isnull=True) - updated_count = 0 - for provider_config in existing_providers: - provider_config.saml_configuration = instance - provider_config.save() - updated_count += 1 + updated_count = 0 + for provider_config in existing_providers: + provider_config.saml_configuration = instance + provider_config.save() + updated_count += 1 - # .. custom_attribute_name: saml_config_signal.updated_count - # .. custom_attribute_description: The number of SAMLProviderConfig records updated to point to the new configuration. - set_custom_attribute('saml_config_signal.updated_count', updated_count) + # .. custom_attribute_name: saml_config_signal.updated_count + # .. custom_attribute_description: The number of SAMLProviderConfig records updated to point to the new configuration. + set_custom_attribute('saml_config_signal.updated_count', updated_count) - except Exception as e: # pylint: disable=broad-except - # .. custom_attribute_name: saml_config_signal.error_message - # .. custom_attribute_description: Records any error message that occurs during SAML provider config updates. - set_custom_attribute('saml_config_signal.error_message', str(e)) + except Exception as e: # pylint: disable=broad-except + # .. custom_attribute_name: saml_config_signal.error_message + # .. custom_attribute_description: Records any error message that occurs during SAML provider config updates. + set_custom_attribute('saml_config_signal.error_message', str(e)) diff --git a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py index 1dd2fd6d7d..a948dc5b8f 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -5,7 +5,7 @@ Tests for SAML configuration signal handlers. import ddt from unittest import mock from unittest.mock import call -from django.test import TestCase, override_settings +from django.test import TestCase from django.contrib.sites.models import Site from common.djangoapps.third_party_auth.tests.factories import SAMLConfigurationFactory, SAMLProviderConfigFactory from common.djangoapps.third_party_auth.models import SAMLProviderConfig @@ -33,26 +33,6 @@ class TestSAMLConfigurationSignalHandlers(TestCase): entity_id='https://existing.example.com' ) - @mock.patch('common.djangoapps.third_party_auth.signals.handlers.set_custom_attribute') - def test_saml_config_signal_handlers_disabled(self, mock_set_custom_attribute): - """ - Test behavior when SAML config signal handlers are disabled. - - Verifies that basic attributes are set but no provider updates are attempted. - """ - with override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=False): - self.saml_config.entity_id = 'https://updated.example.com' - self.saml_config.save() - - expected_calls = [ - call('saml_config_signal.enabled', False), - call('saml_config_signal.new_config_id', self.saml_config.id), - call('saml_config_signal.slug', 'test-config'), - ] - - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - assert mock_set_custom_attribute.call_count == 3 - @mock.patch('common.djangoapps.third_party_auth.signals.handlers.set_custom_attribute') def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute): """ @@ -61,37 +41,35 @@ class TestSAMLConfigurationSignalHandlers(TestCase): Verifies that error information is properly captured when provider updates fail. """ error_message = "Test error" - with override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True): - # Simulate an exception in the provider config update logic - with mock.patch( - 'common.djangoapps.third_party_auth.models.SAMLProviderConfig.objects.current_set', - side_effect=Exception(error_message) - ): - self.saml_config.entity_id = 'https://updated.example.com' - self.saml_config.save() + # Simulate an exception in the provider config update logic + with mock.patch( + 'common.djangoapps.third_party_auth.models.SAMLProviderConfig.objects.current_set', + side_effect=Exception(error_message) + ): + self.saml_config.entity_id = 'https://updated.example.com' + self.saml_config.save() - expected_calls = [ - call('saml_config_signal.enabled', True), - call('saml_config_signal.new_config_id', self.saml_config.id), - call('saml_config_signal.slug', 'test-config'), - ] + expected_calls = [ + call('saml_config_signal.new_config_id', self.saml_config.id), + call('saml_config_signal.slug', 'test-config'), + ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - assert mock_set_custom_attribute.call_count == 4 + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + assert mock_set_custom_attribute.call_count == 3 - # Verify error message was logged - mock_set_custom_attribute.assert_any_call( - 'saml_config_signal.error_message', - mock.ANY - ) - error_calls = [ - call for call in mock_set_custom_attribute.mock_calls - if call[1][0] == 'saml_config_signal.error_message' - ] - assert error_message in error_calls[0][1][1], ( - f"Expected '{error_message}' in error message, " - f"got: {error_calls[0][1][1]}" - ) + # Verify error message was logged + mock_set_custom_attribute.assert_any_call( + 'saml_config_signal.error_message', + mock.ANY + ) + error_calls = [ + call for call in mock_set_custom_attribute.mock_calls + if call[1][0] == 'saml_config_signal.error_message' + ] + assert error_message in error_calls[0][1][1], ( + f"Expected '{error_message}' in error message, " + f"got: {error_calls[0][1][1]}" + ) def _get_current_provider(self, slug): """ @@ -125,7 +103,6 @@ class TestSAMLConfigurationSignalHandlers(TestCase): ) @ddt.unpack @mock.patch('common.djangoapps.third_party_auth.signals.handlers.set_custom_attribute') - @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) def test_saml_provider_config_updates(self, provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug, is_provider_updated, mock_set_custom_attribute): @@ -154,7 +131,6 @@ class TestSAMLConfigurationSignalHandlers(TestCase): current_provider = self._get_current_provider(provider_slug) expected_calls = [ - call('saml_config_signal.enabled', True), call('saml_config_signal.new_config_id', new_saml_config.id), call('saml_config_signal.slug', signal_saml_slug), ] @@ -177,7 +153,6 @@ class TestSAMLConfigurationSignalHandlers(TestCase): (2, 'slug', 1, 'default'), ) @ddt.unpack - @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) def test_saml_provider_with_null_config_not_updated(self, provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug): """ diff --git a/common/djangoapps/third_party_auth/toggles.py b/common/djangoapps/third_party_auth/toggles.py index d8f77f0b1c..53c4edd295 100644 --- a/common/djangoapps/third_party_auth/toggles.py +++ b/common/djangoapps/third_party_auth/toggles.py @@ -2,7 +2,7 @@ Togglable settings for Third Party Auth """ -from edx_toggles.toggles import WaffleFlag, SettingToggle +from edx_toggles.toggles import WaffleFlag THIRD_PARTY_AUTH_NAMESPACE = 'thirdpartyauth' @@ -18,26 +18,6 @@ THIRD_PARTY_AUTH_NAMESPACE = 'thirdpartyauth' APPLE_USER_MIGRATION_FLAG = WaffleFlag(f'{THIRD_PARTY_AUTH_NAMESPACE}.apple_user_migration', __name__) -# .. toggle_name: ENABLE_SAML_CONFIG_SIGNAL_HANDLERS -# .. toggle_implementation: SettingToggle -# .. toggle_default: False -# .. toggle_description: Controls whether SAML configuration signal handlers are active. -# When enabled (True), signal handlers will automatically update SAMLProviderConfig -# references when the associated SAMLConfiguration is updated. -# When disabled (False), SAMLProviderConfigs point to outdated SAMLConfiguration. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2025-07-03 -# .. toggle_target_removal_date: 2026-01-01 -# .. toggle_warning: Disabling this toggle may result in SAMLProviderConfig instances -# pointing to outdated SAMLConfiguration records. Use the management command -# 'saml --fix-references' to fix outdated references. -ENABLE_SAML_CONFIG_SIGNAL_HANDLERS = SettingToggle( - "ENABLE_SAML_CONFIG_SIGNAL_HANDLERS", - default=False, - module_name=__name__ -) - - def is_apple_user_migration_enabled(): """ Returns a boolean if Apple users migration is in process.