Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,16 @@ def setUp(self):
# not processed.
self.saml_config = SAMLConfigurationFactory.create(
enabled=False,
site__domain='testserver.fake',
site__name='testserver.fake'
site__domain='setup.testserver.fake',
site__name='setup.testserver.fake'
)
self.provider_config = SAMLProviderConfigFactory.create(
site__domain='testserver.fake',
site__name='testserver.fake',
slug='test-shib',
name='TestShib College',
entity_id='https://idp.testshib.org/idp/shibboleth',
metadata_source='https://www.testshib.org/metadata/testshib-providers.xml',
saml_configuration=self.saml_config,
site__domain='setup.testserver.fake',
site__name='setup.testserver.fake',
slug='setup-test-shib',
name='Setup TestShib College',
entity_id='https://idp.testshib.org/idp/setup-shibboleth',
metadata_source='https://www.testshib.org/metadata/setup-testshib-providers.xml',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help explain why these changes are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robrap Previously, the test setup used names like testserver.fake and test-shib for both the base test data (created in setUp) and for additional test data created in individual test methods. This caused problems because some tests would accidentally interact with or be affected by the base data, leading to confusing or flaky test results.

By changing the setup data to use unique names (like setup.testserver.fake and setup-test-shib), make sure that the base data created for all tests is clearly separated from any test-specific data.

)

def _setup_test_configs_for_run_checks(self):
Expand Down Expand Up @@ -170,7 +169,7 @@ def test_fetch_saml_metadata(self):
# Create enabled configurations
self.__create_saml_configurations__()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why does __create_saml_configurations__ use this naming convention? Is it overriding something I am not seeing? If we simply want to indicate it is private, it would be _create_saml_configurations instead (just one underscore as a prefix, and no suffix).
  • Additionally, that method has a typo in its docstring: AMLProviderConfig is missing the leading S.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Point, I will fix that change Thanks.


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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why removal of the toggle results in an additional provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, with the toggle gone, the base setup always creates a provider and configuration (the "setup" provider). When the test method also creates its own providers, the total number of providers in the database increases by one compared to before. That’s why, for example, the expected count changes from 1 to 2 providers in the test output.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm forgetting what "skipped" means and what "attempted" means in this message.
  • I'm still not clear what the base setup creates a provider/configuration now with the toggle gone, but didn't before the toggle was gone. I'm still missing something.

call_command("saml", pull=True, stdout=self.stdout)
assert expected in self.stdout.getvalue()

Expand All @@ -183,7 +182,7 @@ def test_fetch_saml_metadata_failure(self):
# Create enabled 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)
Expand Down Expand Up @@ -229,7 +228,7 @@ def test_fetch_multiple_providers_data(self):
}
)

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()
Expand All @@ -251,8 +250,8 @@ def test_fetch_multiple_providers_data(self):
}
)

# 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'
# Five configurations -- 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()
Expand All @@ -267,7 +266,7 @@ def test_saml_request_exceptions(self, mocked_get):

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)
Expand Down Expand Up @@ -324,7 +323,7 @@ def test_xml_parse_exceptions(self, mocked_get):
# create enabled configuration
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)
Expand All @@ -344,18 +343,20 @@ def test_run_checks_setup_test_data(self):

This test validates that the base setup data (from setUp) is correctly
identified as having configuration issues. The setup includes a provider
(self.provider_config) with a disabled SAML configuration (self.saml_config),
which is reported as a disabled config issue (not a missing config).
(self.provider_config) with no direct SAML configuration, falling back to
a disabled default configuration (self.saml_config), which is reported as
a disabled config issue (not a missing config).
"""
output = self._run_checks_command()

# The setup data includes a provider with a disabled SAML config
# The setup data includes a provider with no direct SAML config, using disabled default config
expected_warning = (
f'[WARNING] Provider (id={self.provider_config.id}, '
f'name={self.provider_config.name}, '
f'slug={self.provider_config.slug}, '
f'site_id={self.provider_config.site_id}) '
f'has SAML config (id={self.saml_config.id}, enabled=False).'
f'has no direct SAML configuration and the default configuration '
f'(id={self.saml_config.id}, enabled=False).'
)
self.assertIn(expected_warning, output)
self.assertIn('Missing configs: 0', output) # No missing configs from setUp
Expand Down
50 changes: 22 additions & 28 deletions common/djangoapps/third_party_auth/signals/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
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)
Expand All @@ -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)
Expand All @@ -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)

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)

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

# .. 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))
83 changes: 29 additions & 54 deletions common/djangoapps/third_party_auth/signals/tests/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
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
Expand Down Expand Up @@ -34,64 +34,42 @@ def setUp(self):
)

@mock.patch('common.djangoapps.third_party_auth.signals.handlers.set_custom_attribute')
def test_saml_config_signal_handlers_disabled(self, mock_set_custom_attribute):
def test_saml_config_signal_handlers_with_error(self, mock_set_custom_attribute):
"""
Test behavior when SAML config signal handlers are disabled.
Test error handling when signal handlers encounter an exception.

Verifies that basic attributes are set but no provider updates are attempted.
Verifies that error information is properly captured when provider updates fail.
"""
with override_settings(ENABLE_SAML_CONFIG_SIGNAL_HANDLERS=False):
error_message = "Test 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()

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
expected_calls = [
call('saml_config_signal.new_config_id', self.saml_config.id),
call('saml_config_signal.slug', 'test-config'),
]

@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):
"""
Test error handling when signal handlers encounter an exception.
mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False)
assert mock_set_custom_attribute.call_count == 3

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

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'),
]

mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False)
assert mock_set_custom_attribute.call_count == 4

# 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):
"""
Expand Down Expand Up @@ -125,7 +103,6 @@ def _get_site(self, site_id):
)
@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):
Expand Down Expand Up @@ -154,7 +131,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug,
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),
]
Expand All @@ -177,7 +153,6 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug,
(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):
"""
Expand Down
22 changes: 1 addition & 21 deletions common/djangoapps/third_party_auth/toggles.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -18,26 +18,6 @@
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.
Expand Down
Loading