Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: removal of temporary saml toggle (#37651)

Browse Source 
Removes temporary rollout toggle ENABLE_SAML_CONFIG_SIGNAL_HANDLERS. The
toggle was used to rollout a fix, and now the fix that uses the signal handlers is
enabled by default.

The only follow-up needed by anyone is to no longer set this toggle, which will no
longer do anything.
This commit is contained in:
Krish Tyagi
2026-01-07 03:08:41 +05:30
committed by GitHub
parent fd33d80fb7
commit 1ffa136d45
4 changed files with 71 additions and 122 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
View File

@@ -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)

44
common/djangoapps/third_party_auth/signals/handlers.py
View File

@@ -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))

79
common/djangoapps/third_party_auth/signals/tests/test_handlers.py
View File

@@ -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):
"""

22
common/djangoapps/third_party_auth/toggles.py
View File

@@ -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.