From 78e496882f70aede1b1a94b7f8da09be576a9a40 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 28 Aug 2025 06:40:32 +0000 Subject: [PATCH 01/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../third_party_auth/signals/handlers.py | 4 +- .../signals/tests/test_handlers.py | 225 +++++++++++++++++- 2 files changed, 226 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/third_party_auth/signals/handlers.py b/common/djangoapps/third_party_auth/signals/handlers.py index dc83a32162d4..6364c798f549 100644 --- a/common/djangoapps/third_party_auth/signals/handlers.py +++ b/common/djangoapps/third_party_auth/signals/handlers.py @@ -37,9 +37,9 @@ def update_saml_provider_configs_on_configuration_change(sender, instance, creat # 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( - site_id=instance.site_id, + saml_configuration__site_id=instance.site_id, saml_configuration__slug=instance.slug - ).exclude(saml_configuration_id=instance.id) + ).exclude(saml_configuration_id=instance.id).exclude(saml_configuration_id__isnull=True) updated_count = 0 for provider_config in existing_providers: 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 8c534ce06c5b..323eb2941660 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -6,7 +6,9 @@ from unittest import mock from unittest.mock import call from django.test import TestCase, override_settings -from common.djangoapps.third_party_auth.tests.factories import SAMLConfigurationFactory +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 @ddt.ddt @@ -115,3 +117,224 @@ def test_saml_config_signal_handlers( f"Expected '{error_message}' in error message for {description}, " f"got: {error_calls[0][1][1]}" ) + + +@ddt.ddt +class TestSAMLProviderConfigUpdates(TestCase): + """ + Test SAML provider config updates based on SAML configuration changes. + """ + + def setUp(self): + # Create test sites + self.site1 = Site.objects.get_or_create(domain='test-site1.com', name='Site 1')[0] + self.site2 = Site.objects.get_or_create(domain='test-site2.com', name='Site 2')[0] + + # Create SAML configs (same slug, different sites) + self.config1 = SAMLConfigurationFactory( + site=self.site1, + slug='default', + entity_id='https://site1.com' + ) + self.config2 = SAMLConfigurationFactory( + site=self.site2, + slug='default', + entity_id='https://site2.com' + ) + + # Create provider with NULL config (critical for fallback auth) + self.null_provider = SAMLProviderConfigFactory( + slug='null_provider', + site=self.site1, + saml_configuration=None + ) + + def _get_current_provider(self, slug): + """ + Helper to get current version of provider by slug. + """ + return SAMLProviderConfig.objects.current_set().get(slug=slug) + + def _create_new_config(self, site, slug='default'): + """ + Helper to create new SAML config and trigger signal. + """ + return SAMLConfigurationFactory( + site=site, + slug=slug, + entity_id=f'https://{site.domain}/updated' + ) + + @ddt.data( + # Case 1: Tests that NULL configurations are never updated by signal handler + # Verifies that providers with NULL SAML config remain unchanged while valid configs are updated + { + 'test_case': 'null_preserved', + 'description': 'NULL configuration preserved', + 'setup_data': { + 'create_valid_provider': True, + 'create_cross_site_provider': False, + 'create_site2_provider': False, + }, + }, + # Case 2: Tests that signal handler only updates providers from same site + # Verifies site isolation - only providers with configs from the updated site are changed + { + 'test_case': 'site_isolation', + 'description': 'site isolation respected', + 'setup_data': { + 'create_valid_provider': False, + 'create_cross_site_provider': False, + 'create_site2_provider': True, + }, + }, + # Case 3: Tests cross-site provider updates when SAML config site matches + # Verifies that cross-site provider references are handled correctly + { + 'test_case': 'cross_site', + 'description': 'cross-site provider updated', + 'setup_data': { + 'create_valid_provider': False, + 'create_cross_site_provider': True, + 'create_site2_provider': False, + }, + }, + ) + @ddt.unpack + @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) + def test_saml_provider_config_updates(self, test_case, description, setup_data): + """ + Test SAML provider config updates under different scenarios. + """ + if test_case == 'null_preserved': + # Create provider with valid config + valid_provider = SAMLProviderConfigFactory( + slug='valid_provider', + site=self.site1, + saml_configuration=self.config1 + ) + + # Trigger signal by creating new config + new_config = self._create_new_config(self.site1) + + # Get current states + current_valid = self._get_current_provider('valid_provider') + current_null = self._get_current_provider('null_provider') + + # Verify: valid provider updated, NULL provider unchanged + self.assertEqual(current_valid.saml_configuration_id, new_config.id) + self.assertIsNone(current_null.saml_configuration_id) + + elif test_case == 'site_isolation': + # Create providers on different sites + site1_provider = SAMLProviderConfigFactory( + slug='site1_provider', + site=self.site1, + saml_configuration=self.config1 + ) + site2_provider = SAMLProviderConfigFactory( + slug='site2_provider', + site=self.site2, + saml_configuration=self.config2 + ) + original_site2_config_id = site2_provider.saml_configuration_id + + # Update only site1 config + new_config = self._create_new_config(self.site1) + + # Get current states + current_site1 = self._get_current_provider('site1_provider') + current_site2 = self._get_current_provider('site2_provider') + + # Verify: only site1 provider updated, site2 provider unchanged + self.assertEqual(current_site1.saml_configuration_id, new_config.id) + self.assertEqual(current_site2.saml_configuration_id, original_site2_config_id) + + elif test_case == 'cross_site': + # Create provider on site2 but referencing site1's config + cross_site_provider = SAMLProviderConfigFactory( + slug='cross_site_provider', + site=self.site2, + saml_configuration=self.config1 + ) + + # Update site1's config + new_config = self._create_new_config(self.site1) + + # Provider should be updated because its SAML config is from site1 + current_provider = self._get_current_provider('cross_site_provider') + self.assertEqual(current_provider.saml_configuration_id, new_config.id) + + @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) + def test_null_configuration_preserved(self): + """ + Test that NULL configurations are never updated by signal handler. + """ + + # Create provider with valid config + valid_provider = SAMLProviderConfigFactory( + slug='valid_provider', + site=self.site1, + saml_configuration=self.config1 + ) + + # Trigger signal by creating new config + new_config = self._create_new_config(self.site1) + + # Get current states + current_valid = self._get_current_provider('valid_provider') + current_null = self._get_current_provider('null_provider') + + # Verify: valid provider updated, NULL provider unchanged + self.assertEqual(current_valid.saml_configuration_id, new_config.id) + self.assertIsNone(current_null.saml_configuration_id) + + @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) + def test_site_isolation_respected(self): + """ + Test that signal handler only updates providers from same site. + """ + + # Create providers on different sites + site1_provider = SAMLProviderConfigFactory( + slug='site1_provider', + site=self.site1, + saml_configuration=self.config1 + ) + site2_provider = SAMLProviderConfigFactory( + slug='site2_provider', + site=self.site2, + saml_configuration=self.config2 + ) + original_site2_config_id = site2_provider.saml_configuration_id + + # Update only site1 config + new_config = self._create_new_config(self.site1) + + # Get current states + current_site1 = self._get_current_provider('site1_provider') + current_site2 = self._get_current_provider('site2_provider') + + # Verify: only site1 provider updated, site2 provider unchanged + self.assertEqual(current_site1.saml_configuration_id, new_config.id) + self.assertEqual(current_site2.saml_configuration_id, original_site2_config_id) + + @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) + def test_cross_site_provider_updated(self): + """ + Test provider gets updated when its SAML config's site matches, regardless of provider's site. + """ + + # Create provider on site2 but referencing site1's config + cross_site_provider = SAMLProviderConfigFactory( + slug='cross_site_provider', + site=self.site2, + saml_configuration=self.config1 + ) + + # Update site1's config + new_config = self._create_new_config(self.site1) + + # Provider should be updated because its SAML config is from site1 + current_provider = self._get_current_provider('cross_site_provider') + self.assertEqual(current_provider.saml_configuration_id, new_config.id) From c4cd608f0dca5a98818827c12ef387b3fdcc369c Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 28 Aug 2025 11:45:24 +0000 Subject: [PATCH 02/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../signals/tests/test_handlers.py | 39 +++++-------------- 1 file changed, 10 insertions(+), 29 deletions(-) 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 323eb2941660..af4ded57247f 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -207,17 +207,14 @@ def test_saml_provider_config_updates(self, test_case, description, setup_data): Test SAML provider config updates under different scenarios. """ if test_case == 'null_preserved': - # Create provider with valid config - valid_provider = SAMLProviderConfigFactory( + valid_provider_config = SAMLProviderConfigFactory( slug='valid_provider', site=self.site1, saml_configuration=self.config1 ) - # Trigger signal by creating new config new_config = self._create_new_config(self.site1) - # Get current states current_valid = self._get_current_provider('valid_provider') current_null = self._get_current_provider('null_provider') @@ -226,23 +223,20 @@ def test_saml_provider_config_updates(self, test_case, description, setup_data): self.assertIsNone(current_null.saml_configuration_id) elif test_case == 'site_isolation': - # Create providers on different sites - site1_provider = SAMLProviderConfigFactory( + site1_provider_config = SAMLProviderConfigFactory( slug='site1_provider', site=self.site1, saml_configuration=self.config1 ) - site2_provider = SAMLProviderConfigFactory( + site2_provider_config = SAMLProviderConfigFactory( slug='site2_provider', site=self.site2, saml_configuration=self.config2 ) - original_site2_config_id = site2_provider.saml_configuration_id + original_site2_config_id = site2_provider_config.saml_configuration_id - # Update only site1 config new_config = self._create_new_config(self.site1) - # Get current states current_site1 = self._get_current_provider('site1_provider') current_site2 = self._get_current_provider('site2_provider') @@ -251,14 +245,12 @@ def test_saml_provider_config_updates(self, test_case, description, setup_data): self.assertEqual(current_site2.saml_configuration_id, original_site2_config_id) elif test_case == 'cross_site': - # Create provider on site2 but referencing site1's config - cross_site_provider = SAMLProviderConfigFactory( + cross_site_provider_config = SAMLProviderConfigFactory( slug='cross_site_provider', site=self.site2, saml_configuration=self.config1 ) - # Update site1's config new_config = self._create_new_config(self.site1) # Provider should be updated because its SAML config is from site1 @@ -270,18 +262,14 @@ def test_null_configuration_preserved(self): """ Test that NULL configurations are never updated by signal handler. """ - - # Create provider with valid config - valid_provider = SAMLProviderConfigFactory( + valid_provider_config = SAMLProviderConfigFactory( slug='valid_provider', site=self.site1, saml_configuration=self.config1 ) - # Trigger signal by creating new config new_config = self._create_new_config(self.site1) - # Get current states current_valid = self._get_current_provider('valid_provider') current_null = self._get_current_provider('null_provider') @@ -294,24 +282,20 @@ def test_site_isolation_respected(self): """ Test that signal handler only updates providers from same site. """ - - # Create providers on different sites - site1_provider = SAMLProviderConfigFactory( + site1_provider_config = SAMLProviderConfigFactory( slug='site1_provider', site=self.site1, saml_configuration=self.config1 ) - site2_provider = SAMLProviderConfigFactory( + site2_provider_config = SAMLProviderConfigFactory( slug='site2_provider', site=self.site2, saml_configuration=self.config2 ) - original_site2_config_id = site2_provider.saml_configuration_id + original_site2_config_id = site2_provider_config.saml_configuration_id - # Update only site1 config new_config = self._create_new_config(self.site1) - # Get current states current_site1 = self._get_current_provider('site1_provider') current_site2 = self._get_current_provider('site2_provider') @@ -324,15 +308,12 @@ def test_cross_site_provider_updated(self): """ Test provider gets updated when its SAML config's site matches, regardless of provider's site. """ - - # Create provider on site2 but referencing site1's config - cross_site_provider = SAMLProviderConfigFactory( + cross_site_provider_config = SAMLProviderConfigFactory( slug='cross_site_provider', site=self.site2, saml_configuration=self.config1 ) - # Update site1's config new_config = self._create_new_config(self.site1) # Provider should be updated because its SAML config is from site1 From f5d3d9801a4b4db5f06815ff427480c0aa8baba3 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 07:25:18 +0000 Subject: [PATCH 03/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../signals/tests/test_handlers.py | 264 +++++++----------- 1 file changed, 99 insertions(+), 165 deletions(-) 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 af4ded57247f..f7a4ed73efa5 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -23,6 +23,33 @@ def setUp(self): org_info_str='{"en-US": {"url": "http://test.com", "displayname": "Test", "name": "test"}}' ) + self.site1 = Site.objects.get_or_create(domain='test-site1.com', name='Site 1')[0] + self.site2 = Site.objects.get_or_create(domain='test-site2.com', name='Site 2')[0] + + self.config1 = SAMLConfigurationFactory( + site=self.site1, + slug='default', + entity_id='https://site1.com' + ) + self.config2 = SAMLConfigurationFactory( + site=self.site2, + slug='default', + entity_id='https://site2.com' + ) + + self.provider_with_null_saml_configuration = SAMLProviderConfigFactory( + slug='null_saml_configuration', + site=self.site1, + saml_configuration=None + ) + + # Existing SAML config used by provider update tests + self.existing_saml_config = SAMLConfigurationFactory( + site=self.site1, + slug='slug', + entity_id='https://existing.example.com' + ) + @ddt.data( # Case 1: Tests behavior when SAML config signal handlers are disabled # Verifies that basic attributes are set but no provider updates are attempted @@ -118,43 +145,18 @@ def test_saml_config_signal_handlers( f"got: {error_calls[0][1][1]}" ) - -@ddt.ddt -class TestSAMLProviderConfigUpdates(TestCase): - """ - Test SAML provider config updates based on SAML configuration changes. - """ - - def setUp(self): - # Create test sites - self.site1 = Site.objects.get_or_create(domain='test-site1.com', name='Site 1')[0] - self.site2 = Site.objects.get_or_create(domain='test-site2.com', name='Site 2')[0] - - # Create SAML configs (same slug, different sites) - self.config1 = SAMLConfigurationFactory( - site=self.site1, - slug='default', - entity_id='https://site1.com' - ) - self.config2 = SAMLConfigurationFactory( - site=self.site2, - slug='default', - entity_id='https://site2.com' - ) - - # Create provider with NULL config (critical for fallback auth) - self.null_provider = SAMLProviderConfigFactory( - slug='null_provider', - site=self.site1, - saml_configuration=None - ) - def _get_current_provider(self, slug): """ Helper to get current version of provider by slug. """ return SAMLProviderConfig.objects.current_set().get(slug=slug) + def _get_site(self, site_id): + """ + Helper to get site by ID (1 = site1, 2 = site2). + """ + return self.site1 if site_id == 1 else self.site2 + def _create_new_config(self, site, slug='default'): """ Helper to create new SAML config and trigger signal. @@ -166,156 +168,88 @@ def _create_new_config(self, site, slug='default'): ) @ddt.data( - # Case 1: Tests that NULL configurations are never updated by signal handler - # Verifies that providers with NULL SAML config remain unchanged while valid configs are updated - { - 'test_case': 'null_preserved', - 'description': 'NULL configuration preserved', - 'setup_data': { - 'create_valid_provider': True, - 'create_cross_site_provider': False, - 'create_site2_provider': False, - }, - }, - # Case 2: Tests that signal handler only updates providers from same site - # Verifies site isolation - only providers with configs from the updated site are changed - { - 'test_case': 'site_isolation', - 'description': 'site isolation respected', - 'setup_data': { - 'create_valid_provider': False, - 'create_cross_site_provider': False, - 'create_site2_provider': True, - }, - }, - # Case 3: Tests cross-site provider updates when SAML config site matches - # Verifies that cross-site provider references are handled correctly - { - 'test_case': 'cross_site', - 'description': 'cross-site provider updated', - 'setup_data': { - 'create_valid_provider': False, - 'create_cross_site_provider': True, - 'create_site2_provider': False, - }, - }, + # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug, is_provider_updated + # All tests: provider's saml_configuration has site_id=1, slug='slug' + # + # Signal matches provider's saml config and should update + (1, 'slug', 1, 'slug', True), # Same site, same slug + (2, 'slug', 1, 'slug', True), # Cross-site provider, matching saml config + (1, 'provider-slug', 1, 'slug', True), # Different provider slug, matching saml config + # Signal does not match provider's saml config and should not update + (1, 'slug', 2, 'slug', False), # Different saml config site + (2, 'slug', 2, 'slug', False), # Different saml config site (cross-site) + (1, 'provider-slug', 1, 'provider-slug', False), # Different saml config slug + (2, 'provider-slug', 1, 'provider-slug', False), # Different saml config slug (cross-site) ) @ddt.unpack @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) - def test_saml_provider_config_updates(self, test_case, description, setup_data): + def test_saml_provider_config_updates(self, provider_site_id, provider_slug, + signal_saml_site_id, signal_saml_slug, is_provider_updated): """ Test SAML provider config updates under different scenarios. - """ - if test_case == 'null_preserved': - valid_provider_config = SAMLProviderConfigFactory( - slug='valid_provider', - site=self.site1, - saml_configuration=self.config1 - ) - - new_config = self._create_new_config(self.site1) - - current_valid = self._get_current_provider('valid_provider') - current_null = self._get_current_provider('null_provider') - - # Verify: valid provider updated, NULL provider unchanged - self.assertEqual(current_valid.saml_configuration_id, new_config.id) - self.assertIsNone(current_null.saml_configuration_id) - - elif test_case == 'site_isolation': - site1_provider_config = SAMLProviderConfigFactory( - slug='site1_provider', - site=self.site1, - saml_configuration=self.config1 - ) - site2_provider_config = SAMLProviderConfigFactory( - slug='site2_provider', - site=self.site2, - saml_configuration=self.config2 - ) - original_site2_config_id = site2_provider_config.saml_configuration_id - - new_config = self._create_new_config(self.site1) - - current_site1 = self._get_current_provider('site1_provider') - current_site2 = self._get_current_provider('site2_provider') - - # Verify: only site1 provider updated, site2 provider unchanged - self.assertEqual(current_site1.saml_configuration_id, new_config.id) - self.assertEqual(current_site2.saml_configuration_id, original_site2_config_id) - - elif test_case == 'cross_site': - cross_site_provider_config = SAMLProviderConfigFactory( - slug='cross_site_provider', - site=self.site2, - saml_configuration=self.config1 - ) - - new_config = self._create_new_config(self.site1) - - # Provider should be updated because its SAML config is from site1 - current_provider = self._get_current_provider('cross_site_provider') - self.assertEqual(current_provider.saml_configuration_id, new_config.id) - @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) - def test_null_configuration_preserved(self): - """ - Test that NULL configurations are never updated by signal handler. + Tests that providers are updated only when the signal's SAML configuration + matches the provider's existing SAML configuration (by site and slug). """ - valid_provider_config = SAMLProviderConfigFactory( - slug='valid_provider', - site=self.site1, - saml_configuration=self.config1 - ) - - new_config = self._create_new_config(self.site1) - - current_valid = self._get_current_provider('valid_provider') - current_null = self._get_current_provider('null_provider') + provider_site = self._get_site(provider_site_id) + signal_saml_site = self._get_site(signal_saml_site_id) - # Verify: valid provider updated, NULL provider unchanged - self.assertEqual(current_valid.saml_configuration_id, new_config.id) - self.assertIsNone(current_null.saml_configuration_id) - - @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) - def test_site_isolation_respected(self): - """ - Test that signal handler only updates providers from same site. - """ - site1_provider_config = SAMLProviderConfigFactory( - slug='site1_provider', - site=self.site1, - saml_configuration=self.config1 + provider = SAMLProviderConfigFactory( + slug=provider_slug, + site=provider_site, + saml_configuration=self.existing_saml_config ) - site2_provider_config = SAMLProviderConfigFactory( - slug='site2_provider', - site=self.site2, - saml_configuration=self.config2 - ) - original_site2_config_id = site2_provider_config.saml_configuration_id + original_config_id = provider.saml_configuration_id - new_config = self._create_new_config(self.site1) + new_saml_config = SAMLConfigurationFactory( + site=signal_saml_site, + slug=signal_saml_slug, + entity_id='https://new.example.com' + ) - current_site1 = self._get_current_provider('site1_provider') - current_site2 = self._get_current_provider('site2_provider') + current_provider = self._get_current_provider(provider_slug) - # Verify: only site1 provider updated, site2 provider unchanged - self.assertEqual(current_site1.saml_configuration_id, new_config.id) - self.assertEqual(current_site2.saml_configuration_id, original_site2_config_id) + if is_provider_updated: + self.assertEqual(current_provider.saml_configuration_id, new_saml_config.id, + f"Provider should be updated when signal SAML config matches") + else: + self.assertEqual(current_provider.saml_configuration_id, original_config_id, + f"Provider should NOT be updated when signal SAML config doesn't match") + @ddt.data( + # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug + # + # All tests: provider's saml config is None and should never be updated + (1, 'slug', 1, 'default'), + (1, 'default', 1, 'default'), + (2, 'slug', 1, 'default'), + (2, 'default', 2, 'default'), + ) + @ddt.unpack @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) - def test_cross_site_provider_updated(self): + def test_saml_provider_with_null_config_not_updated(self, provider_site_id, provider_slug, + signal_saml_site_id, signal_saml_slug): """ - Test provider gets updated when its SAML config's site matches, regardless of provider's site. + Test that providers with NULL SAML configuration are never updated by signal handler. + + This is critical for fallback authentication scenarios where providers + intentionally have no SAML configuration. """ - cross_site_provider_config = SAMLProviderConfigFactory( - slug='cross_site_provider', - site=self.site2, - saml_configuration=self.config1 + provider_site = self._get_site(provider_site_id) + signal_saml_site = self._get_site(signal_saml_site_id) + + null_provider = SAMLProviderConfigFactory( + slug=provider_slug, + site=provider_site, + saml_configuration=None ) - new_config = self._create_new_config(self.site1) + new_saml_config = SAMLConfigurationFactory( + site=signal_saml_site, + slug=signal_saml_slug, + entity_id='https://new.example.com' + ) - # Provider should be updated because its SAML config is from site1 - current_provider = self._get_current_provider('cross_site_provider') - self.assertEqual(current_provider.saml_configuration_id, new_config.id) + current_provider = self._get_current_provider(provider_slug) + self.assertIsNone(current_provider.saml_configuration_id, + "Provider with NULL SAML config should never be updated") From 4adde4118036c2ec727bb9fff2b365aff99895b8 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 08:34:09 +0000 Subject: [PATCH 04/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../third_party_auth/signals/tests/test_handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f7a4ed73efa5..9d44d3de2021 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -211,10 +211,10 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, if is_provider_updated: self.assertEqual(current_provider.saml_configuration_id, new_saml_config.id, - f"Provider should be updated when signal SAML config matches") + "Provider should be updated when signal SAML config matches") else: self.assertEqual(current_provider.saml_configuration_id, original_config_id, - f"Provider should NOT be updated when signal SAML config doesn't match") + "Provider should NOT be updated when signal SAML config doesn't match") @ddt.data( # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug From 6d2e514f0e94125acc0bafd41df3ebcbe41e55ee Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 09:49:57 +0000 Subject: [PATCH 05/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../signals/tests/test_handlers.py | 39 +++---------------- 1 file changed, 5 insertions(+), 34 deletions(-) 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 9d44d3de2021..3e40a1d106ee 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -26,23 +26,6 @@ def setUp(self): self.site1 = Site.objects.get_or_create(domain='test-site1.com', name='Site 1')[0] self.site2 = Site.objects.get_or_create(domain='test-site2.com', name='Site 2')[0] - self.config1 = SAMLConfigurationFactory( - site=self.site1, - slug='default', - entity_id='https://site1.com' - ) - self.config2 = SAMLConfigurationFactory( - site=self.site2, - slug='default', - entity_id='https://site2.com' - ) - - self.provider_with_null_saml_configuration = SAMLProviderConfigFactory( - slug='null_saml_configuration', - site=self.site1, - saml_configuration=None - ) - # Existing SAML config used by provider update tests self.existing_saml_config = SAMLConfigurationFactory( site=self.site1, @@ -157,27 +140,16 @@ def _get_site(self, site_id): """ return self.site1 if site_id == 1 else self.site2 - def _create_new_config(self, site, slug='default'): - """ - Helper to create new SAML config and trigger signal. - """ - return SAMLConfigurationFactory( - site=site, - slug=slug, - entity_id=f'https://{site.domain}/updated' - ) - @ddt.data( # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug, is_provider_updated # All tests: provider's saml_configuration has site_id=1, slug='slug' - # # Signal matches provider's saml config and should update - (1, 'slug', 1, 'slug', True), # Same site, same slug - (2, 'slug', 1, 'slug', True), # Cross-site provider, matching saml config - (1, 'provider-slug', 1, 'slug', True), # Different provider slug, matching saml config + (1, 'slug', 1, 'slug', True), # Same site, same slug + (2, 'slug', 1, 'slug', True), # Cross-site provider, matching saml config + (1, 'provider-slug', 1, 'slug', True), # Different provider slug, matching saml config # Signal does not match provider's saml config and should not update - (1, 'slug', 2, 'slug', False), # Different saml config site - (2, 'slug', 2, 'slug', False), # Different saml config site (cross-site) + (1, 'slug', 2, 'slug', False), # Different saml config site + (2, 'slug', 2, 'slug', False), # Different saml config site (cross-site) (1, 'provider-slug', 1, 'provider-slug', False), # Different saml config slug (2, 'provider-slug', 1, 'provider-slug', False), # Different saml config slug (cross-site) ) @@ -218,7 +190,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, @ddt.data( # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug - # # All tests: provider's saml config is None and should never be updated (1, 'slug', 1, 'default'), (1, 'default', 1, 'default'), From ad5da327297010ada3f5ec83e791eafbe80b999e Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 12:09:14 +0000 Subject: [PATCH 06/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../djangoapps/third_party_auth/signals/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) 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 3e40a1d106ee..8454a3f2394a 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -193,7 +193,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, # All tests: provider's saml config is None and should never be updated (1, 'slug', 1, 'default'), (1, 'default', 1, 'default'), - (2, 'slug', 1, 'default'), (2, 'default', 2, 'default'), ) @ddt.unpack From 25508f2fd6d54e404c45c50a11b8876de8a2388f Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 13:07:02 +0000 Subject: [PATCH 07/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../djangoapps/third_party_auth/signals/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) 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 8454a3f2394a..625d960fea5b 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -193,7 +193,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, # All tests: provider's saml config is None and should never be updated (1, 'slug', 1, 'default'), (1, 'default', 1, 'default'), - (2, 'default', 2, 'default'), ) @ddt.unpack @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) From 2b794992756ca6fe63598e2270417680a01ffef9 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 2 Sep 2025 14:20:48 +0000 Subject: [PATCH 08/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../djangoapps/third_party_auth/signals/tests/test_handlers.py | 2 ++ 1 file changed, 2 insertions(+) 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 625d960fea5b..3e40a1d106ee 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -193,6 +193,8 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, # All tests: provider's saml config is None and should never be updated (1, 'slug', 1, 'default'), (1, 'default', 1, 'default'), + (2, 'slug', 1, 'default'), + (2, 'default', 2, 'default'), ) @ddt.unpack @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) From 0a8e09501e6921e93f48f07b50423279ce5d5193 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 3 Sep 2025 06:17:21 +0000 Subject: [PATCH 09/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../signals/tests/test_handlers.py | 145 ++++++++---------- 1 file changed, 63 insertions(+), 82 deletions(-) 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 3e40a1d106ee..9724614d1a88 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -33,100 +33,77 @@ def setUp(self): entity_id='https://existing.example.com' ) - @ddt.data( - # Case 1: Tests behavior when SAML config signal handlers are disabled - # Verifies that basic attributes are set but no provider updates are attempted - { - 'enabled': False, - 'simulate_error': False, - 'description': 'handlers disabled', - 'expected_calls': [ + @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', 'CONFIG_ID'), + call('saml_config_signal.new_config_id', self.saml_config.id), call('saml_config_signal.slug', 'test-config'), - ], - 'expected_call_count': 3, - }, - # Case 2: Tests behavior when SAML config signal handlers are enabled - # Verifies that attributes are set and provider updates are attempted successfully - { - 'enabled': True, - 'simulate_error': False, - 'description': 'handlers enabled', - 'expected_calls': [ - call('saml_config_signal.enabled', True), - call('saml_config_signal.new_config_id', 'CONFIG_ID'), - call('saml_config_signal.slug', 'test-config'), - call('saml_config_signal.updated_count', 0), - ], - 'expected_call_count': 4, - }, - # Case 3: Tests error handling when signal handlers are enabled but encounter an exception - # Verifies that error information is properly captured when provider updates fail - { - 'enabled': True, - 'simulate_error': True, - 'description': 'handlers enabled with exception', - 'expected_calls': [ - call('saml_config_signal.enabled', True), - call('saml_config_signal.new_config_id', 'CONFIG_ID'), - call('saml_config_signal.slug', 'test-config'), - ], - 'expected_call_count': 4, # includes error_message call - 'error_message': 'Test error', - }, - ) - @ddt.unpack + ] + + # Verify expected calls were made + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + + # Verify total call count + assert mock_set_custom_attribute.call_count == 3, ( + f"Expected 3 calls for disabled handlers, " + f"got {mock_set_custom_attribute.call_count}" + ) + @mock.patch('common.djangoapps.third_party_auth.signals.handlers.set_custom_attribute') - def test_saml_config_signal_handlers( - self, mock_set_custom_attribute, enabled, simulate_error, - description, expected_calls, expected_call_count, error_message=None): + def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute): """ - Test SAML configuration signal handlers under different conditions. + Test error handling when signal handlers encounter an exception. + + Verifies that error information is properly captured when provider updates fail. """ - with override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=enabled): - if simulate_error: - # 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() - else: + 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() - expected_calls_with_id = [] - for call_obj in expected_calls: - args = list(call_obj[1]) - if args[1] == 'CONFIG_ID': - args[1] = self.saml_config.id - expected_calls_with_id.append(call(args[0], args[1])) + 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'), + ] # Verify expected calls were made - mock_set_custom_attribute.assert_has_calls(expected_calls_with_id, any_order=False) + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - # Verify total call count - assert mock_set_custom_attribute.call_count == expected_call_count, ( - f"Expected {expected_call_count} calls for {description}, " + # Verify total call count (includes error_message call) + assert mock_set_custom_attribute.call_count == 4, ( + f"Expected 4 calls for error case, " f"got {mock_set_custom_attribute.call_count}" ) - # If error is expected, verify error message was logged - if error_message: - 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 for {description}, " - 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): """ @@ -138,7 +115,12 @@ def _get_site(self, site_id): """ Helper to get site by ID (1 = site1, 2 = site2). """ - return self.site1 if site_id == 1 else self.site2 + if site_id == 1: + return self.site1 + elif site_id == 2: + return self.site2 + else: + raise ValueError(f"Unexpected site_id: {site_id}.") @ddt.data( # Args: provider_site_id, provider_slug, signal_saml_site_id, signal_saml_slug, is_provider_updated @@ -194,7 +176,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, (1, 'slug', 1, 'default'), (1, 'default', 1, 'default'), (2, 'slug', 1, 'default'), - (2, 'default', 2, 'default'), ) @ddt.unpack @override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=True) From 3af11f35fd03466131b8579a143a8d7995886c30 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 3 Sep 2025 16:09:35 -0400 Subject: [PATCH 10/15] fixup! Test clean-up --- .../third_party_auth/signals/tests/test_handlers.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) 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 9724614d1a88..068e9e65bc7e 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -50,14 +50,8 @@ def test_saml_config_signal_handlers_disabled(self, mock_set_custom_attribute): call('saml_config_signal.slug', 'test-config'), ] - # Verify expected calls were made mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - # Verify total call count - assert mock_set_custom_attribute.call_count == 3, ( - f"Expected 3 calls for disabled handlers, " - f"got {mock_set_custom_attribute.call_count}" - ) + 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): From a07b26d6abe0a52f72ddd2fa3165eda0c7279991 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 3 Sep 2025 16:09:50 -0400 Subject: [PATCH 11/15] fixup! Test clean-up --- .../djangoapps/third_party_auth/signals/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) 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 068e9e65bc7e..99ed4760578c 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -76,7 +76,6 @@ def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute) call('saml_config_signal.slug', 'test-config'), ] - # Verify expected calls were made mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) # Verify total call count (includes error_message call) From 3c516ef58baa5c1451b8a26f3a97a2f98b6877e4 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 3 Sep 2025 16:10:05 -0400 Subject: [PATCH 12/15] fixup! Test clean-up --- .../djangoapps/third_party_auth/signals/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) 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 99ed4760578c..356108f90972 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -77,7 +77,6 @@ def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute) ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - # Verify total call count (includes error_message call) assert mock_set_custom_attribute.call_count == 4, ( f"Expected 4 calls for error case, " From f86d5b2447dd3b15fd855f5d92b6a83dbefde57d Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 3 Sep 2025 16:10:24 -0400 Subject: [PATCH 13/15] fixup! Test clean-up --- .../third_party_auth/signals/tests/test_handlers.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 356108f90972..c320411da3f9 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -77,11 +77,7 @@ def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute) ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - # Verify total call count (includes error_message call) - assert mock_set_custom_attribute.call_count == 4, ( - f"Expected 4 calls for error case, " - f"got {mock_set_custom_attribute.call_count}" - ) + assert mock_set_custom_attribute.call_count == 4 # Verify error message was logged mock_set_custom_attribute.assert_any_call( From e8d6ec1a52922a5087ec796b2547f16042f00774 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 3 Sep 2025 20:39:58 +0000 Subject: [PATCH 14/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../third_party_auth/signals/tests/test_handlers.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 c320411da3f9..ba4164b1ba8d 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -124,9 +124,11 @@ def _get_site(self, site_id): (2, 'provider-slug', 1, 'provider-slug', False), # Different saml config slug (cross-site) ) @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): + signal_saml_site_id, signal_saml_slug, is_provider_updated, + mock_set_custom_attribute): """ Test SAML provider config updates under different scenarios. @@ -150,11 +152,17 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, ) current_provider = self._get_current_provider(provider_slug) - + + mock_set_custom_attribute.assert_any_call('saml_config_signal.enabled', True) + mock_set_custom_attribute.assert_any_call('saml_config_signal.new_config_id', new_saml_config.id) + mock_set_custom_attribute.assert_any_call('saml_config_signal.slug', signal_saml_slug) + if is_provider_updated: + mock_set_custom_attribute.assert_any_call('saml_config_signal.updated_count', 1) self.assertEqual(current_provider.saml_configuration_id, new_saml_config.id, "Provider should be updated when signal SAML config matches") else: + mock_set_custom_attribute.assert_any_call('saml_config_signal.updated_count', 0) self.assertEqual(current_provider.saml_configuration_id, original_config_id, "Provider should NOT be updated when signal SAML config doesn't match") From a021f121382a38bb328c5aec2c60f12e0d424696 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 3 Sep 2025 20:46:25 +0000 Subject: [PATCH 15/15] fix: Update SAMLProviderConfig for site-specific configurations --- .../third_party_auth/signals/tests/test_handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 ba4164b1ba8d..7875f0fcfa57 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -152,11 +152,11 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, ) current_provider = self._get_current_provider(provider_slug) - + mock_set_custom_attribute.assert_any_call('saml_config_signal.enabled', True) mock_set_custom_attribute.assert_any_call('saml_config_signal.new_config_id', new_saml_config.id) mock_set_custom_attribute.assert_any_call('saml_config_signal.slug', signal_saml_slug) - + if is_provider_updated: mock_set_custom_attribute.assert_any_call('saml_config_signal.updated_count', 1) self.assertEqual(current_provider.saml_configuration_id, new_saml_config.id,