From 648f8af4b808125616306197a2e6fa05e3baa111 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 19 Jun 2025 13:36:01 +0000 Subject: [PATCH] fix: Enhance SAML configuration retrieval logic in SAMLProviderConfig --- common/djangoapps/third_party_auth/models.py | 52 ++++++++- .../third_party_auth/tests/test_models.py | 107 +++++++++++++++++- 2 files changed, 152 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 6d244d96eddd..c7b4eb72e74f 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -21,6 +21,7 @@ from social_core.backends.saml import SAMLAuth from social_core.exceptions import SocialAuthBaseException from social_core.utils import module_member +from edx_django_utils.monitoring import set_custom_attribute from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming.helpers import get_current_request @@ -29,6 +30,21 @@ from .lti import LTI_PARAMS_KEY, LTIAuthBackend from .saml import STANDARD_SAML_PROVIDER_KEY, get_saml_idp_choices, get_saml_idp_class +from edx_toggles.toggles import SettingToggle + +# .. toggle_name: FEATURES['USE_LATEST_SAML_CONFIG'] +# .. toggle_implementation: SettingToggle +# .. toggle_default: False +# .. toggle_description: When enabled, the system uses the latest enabled SAML configuration with +# the same slug when authenticating users. When disabled, the system uses the directly +# referenced configuration. This is a temporary rollout toggle to get us to the +# enabled state. +# .. toggle_warning: Disabling this may affect authentication for users if providers have +# been updated with newer configurations. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2025-06-10 +# .. toggle_target_removal_date: 2025-09-01 +USE_LATEST_SAML_CONFIG = SettingToggle("USE_LATEST_SAML_CONFIG", default=False, module_name=__name__) log = logging.getLogger(__name__) @@ -759,6 +775,10 @@ class SAMLProviderConfig(ProviderConfig): ) ) archived = models.BooleanField(default=False) + # Ideally we would have stored the SAMLConfiguration keys of site_id and slug, rather than + # pointing to a specific record which may no longer be current when we try to use it. This + # has been compensated for elsewhere by retrieving the site_id and slug from the stored row, + # and using it to get the current (latest) row instead. saml_configuration = models.ForeignKey( SAMLConfiguration, on_delete=models.SET_NULL, @@ -880,12 +900,34 @@ def get_config(self): conf['x509certMulti'] = {'signing': public_keys} conf['x509cert'] = '' conf['url'] = sso_url + # This block determines which SAML configuration should be used during authentication + if self.saml_configuration: + direct_config = self.saml_configuration + conf['saml_sp_configuration'] = direct_config + # .. custom_attribute_name: saml_config.use_latest_toggle_enabled + # .. custom_attribute_description: True if USE_LATEST_SAML_CONFIG is enabled; false otherwise. + set_custom_attribute('saml_config.use_latest_toggle_enabled', USE_LATEST_SAML_CONFIG.is_enabled()) + if USE_LATEST_SAML_CONFIG.is_enabled(): + try: + slug = self.saml_configuration.slug + site_id = self.saml_configuration.site_id + latest_config = SAMLConfiguration.current(self.saml_configuration.site_id, self.saml_configuration.slug) + if latest_config: + # .. custom_attribute_name: saml_config.using + # .. custom_attribute_description: Describes which config is being used: direct (from db), latest, + # or default. + set_custom_attribute('saml_config.using', 'latest') + conf['saml_sp_configuration'] = latest_config + except Exception as e: + log.exception("Error finding latest SAML config for slug %s, site_id %s: %s", + self.saml_configuration.slug, self.saml_configuration.site_id, e) + else: + set_custom_attribute('saml_config.using', 'default') + else: + conf['saml_sp_configuration'] = SAMLConfiguration.current(self.site.id, 'default') + set_custom_attribute('saml_config.using', 'default') - # Add SAMLConfiguration appropriate for this IdP - conf['saml_sp_configuration'] = ( - self.saml_configuration or - SAMLConfiguration.current(self.site.id, 'default') - ) + # Create and return the appropriate IdP class with the configuration idp_class = get_saml_idp_class(self.identity_provider_type) return idp_class(self.slug, **conf) diff --git a/common/djangoapps/third_party_auth/tests/test_models.py b/common/djangoapps/third_party_auth/tests/test_models.py index ed0b74ebb396..2d79f800a057 100644 --- a/common/djangoapps/third_party_auth/tests/test_models.py +++ b/common/djangoapps/third_party_auth/tests/test_models.py @@ -2,10 +2,15 @@ Tests for third_party_auth/models.py. """ import unittest +from datetime import timedelta + from django.test import TestCase, override_settings +from django.utils import timezone +import mock -from .factories import SAMLProviderConfigFactory -from ..models import SAMLProviderConfig, clean_username +from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory +from .factories import SAMLProviderConfigFactory, SAMLConfigurationFactory +from ..models import SAMLProviderConfig, SAMLConfiguration, SAMLProviderData, USE_LATEST_SAML_CONFIG, clean_username class TestSamlProviderConfigModel(TestCase, unittest.TestCase): @@ -17,6 +22,35 @@ def setUp(self): super().setUp() self.saml_provider_config = SAMLProviderConfigFactory() + # Setup for SAML configuration dynamic lookup test + self.site = SiteFactory() + + # Create initial SAML configuration using the factory + self.initial_config = SAMLConfigurationFactory( + site=self.site, + slug='test-config', + entity_id='https://initial-entity-id.example.com', + private_key='initial_private_key', + public_key='initial_public_key', + ) + self.initial_config_id = self.initial_config.id + + # Create provider that points to the initial config + self.provider_config = SAMLProviderConfigFactory( + site=self.site, + entity_id='https://test-idp.example.com', + metadata_source='https://test-idp.example.com/metadata.xml', + saml_configuration=self.initial_config + ) + + # Create provider data to avoid AuthNotConfigured error + SAMLProviderData.objects.create( + entity_id=self.provider_config.entity_id, + sso_url='https://test-idp.example.com/SSO', + public_key='test_public_key', + fetched_at=timezone.now() + ) + def test_unique_entity_id_enforcement_for_non_current_configs(self): """ Test that the unique entity ID enforcement does not apply to noncurrent configs @@ -53,3 +87,72 @@ def test_clean_username_unicode_enabled(self): Test the username cleaner function with unicode enabled """ assert clean_username('ItJüstWòrks™') == 'ItJüstWòrks' + + def test_saml_configuration_dynamic_lookup(self): + """ + Test SAML configuration lookup functionality, including the toggle behavior and error handling. + """ + + # Update configuration creates a new record with the same slug + self.initial_config.entity_id = 'https://updated-entity-id.example.com' + self.initial_config.change_date = timezone.now() + timedelta(minutes=5) + self.initial_config.save() + self.initial_config.refresh_from_db() + + # Verify we have two configs with the same slug + config_count = SAMLConfiguration.objects.filter(slug='test-config').count() + self.assertEqual(config_count, 2) + + # Test with toggle ENABLED - should use latest config + with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=True): + provider_idp = self.provider_config.get_config() + latest_config = SAMLConfiguration.objects.filter( + slug='test-config', enabled=True + ).order_by('-change_date').first() + + # Compare the actual objects rather than just IDs + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration'), + latest_config + ) + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration').entity_id, + 'https://updated-entity-id.example.com' + ) + + # Test with toggle DISABLED - should use direct config + with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=False): + provider_idp = self.provider_config.get_config() + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration'), + self.provider_config.saml_configuration + ) + + # Test error handling + with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.objects.filter') as mock_filter: + mock_filter.side_effect = Exception("Test error") + with mock.patch.object(USE_LATEST_SAML_CONFIG, 'is_enabled', return_value=True): + provider_idp = self.provider_config.get_config() + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration'), + self.provider_config.saml_configuration + ) + + # Test default fallback + self.provider_config.saml_configuration = None + self.provider_config.save() + default_config = SAMLConfigurationFactory( + site=self.site, + slug='default', + enabled=True, + entity_id='https://default-entity-id.example.com', + ) + provider_idp = self.provider_config.get_config() + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration'), + default_config + ) + self.assertEqual( + provider_idp.conf.get('saml_sp_configuration').entity_id, + 'https://default-entity-id.example.com' + )