From 1d3c94d4e306fc3752f21d7e1b81bbea1f85bf49 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 22 Sep 2025 10:52:22 +0000 Subject: [PATCH 01/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 80 ++++++++++--------- .../management/commands/tests/test_saml.py | 73 +++++++++++++---- 2 files changed, 99 insertions(+), 54 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index afe369c2ade0..541d0d32168d 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -75,7 +75,10 @@ def _handle_run_checks(self): - Outdated SAMLConfiguration references (provider pointing to old config version) - Site ID mismatches between SAMLProviderConfig and its SAMLConfiguration - Slug mismatches (except 'default' slugs) # noqa: E501 - - SAMLProviderConfig objects with null SAMLConfiguration references (informational) + - SAMLProviderConfig objects with no available configuration (no direct config AND no default) + + Uses get_config() to accurately determine if a provider has usable configuration, + eliminating false warnings for providers that correctly use default configurations. Includes observability attributes for monitoring. """ @@ -112,50 +115,52 @@ def _check_provider_configurations(self): f"slug={provider_config.slug}, site_id={provider_config.site_id})" ) - if not provider_config.saml_configuration: - self.stdout.write( - f"[INFO] {provider_info} has no SAML configuration because " - "a matching default was not found." - ) - null_config_count += 1 - continue - try: - current_config = SAMLConfiguration.current( - provider_config.saml_configuration.site_id, - provider_config.saml_configuration.slug - ) + # Use get_config() to get the actual configuration that would be used + # This includes both direct configuration and default fallback logic + actual_config = provider_config.get_config() + + if not actual_config: + self.stdout.write( + f"[WARNING] {provider_info} has no SAML configuration and " + "no matching default configuration was found." + ) + null_config_count += 1 + continue + + if provider_config.saml_configuration: + current_config = SAMLConfiguration.current( + provider_config.saml_configuration.site_id, + provider_config.saml_configuration.slug + ) - # Check for outdated configuration references - if current_config: - if current_config.id != provider_config.saml_configuration_id: + if current_config and current_config.id != provider_config.saml_configuration_id: self.stdout.write( f"[WARNING] {provider_info} " - f"has outdated SAML config (id={provider_config.saml_configuration_id} which " + f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " f"should be updated to the current SAML config (id={current_config.id})." ) outdated_count += 1 - if provider_config.saml_configuration.site_id != provider_config.site_id: - config_site_id = provider_config.saml_configuration.site_id - provider_site_id = provider_config.site_id - self.stdout.write( - f"[WARNING] {provider_info} " - f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " - "does not match the provider's site_id." - ) - site_mismatch_count += 1 + if provider_config.saml_configuration.site_id != provider_config.site_id: + config_site_id = provider_config.saml_configuration.site_id + self.stdout.write( + f"[WARNING] {provider_info} " + f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " + "does not match the provider's site_id." + ) + site_mismatch_count += 1 - saml_configuration_slug = provider_config.saml_configuration.slug - provider_config_slug = provider_config.slug + saml_configuration_slug = provider_config.saml_configuration.slug + provider_config_slug = provider_config.slug - if saml_configuration_slug not in (provider_config_slug, 'default'): - self.stdout.write( - f"[WARNING] {provider_info} " - f"SAML config (id={provider_config.saml_configuration_id}, slug='{saml_configuration_slug}') " - "does not match the provider's slug." - ) - slug_mismatch_count += 1 + if saml_configuration_slug not in (provider_config_slug, 'default'): + self.stdout.write( + f"[WARNING] {provider_info} " + f"SAML config (id={provider_config.saml_configuration_id}, slug='{saml_configuration_slug}') " + "does not match the provider's slug." + ) + slug_mismatch_count += 1 except Exception as e: # pylint: disable=broad-except self.stderr.write(f"[ERROR] Error processing {provider_info}: {e}") @@ -166,7 +171,7 @@ def _check_provider_configurations(self): 'outdated_count': {'count': outdated_count, 'requires_attention': True}, 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': True}, - 'null_config_count': {'count': null_config_count, 'requires_attention': False}, + 'null_config_count': {'count': null_config_count, 'requires_attention': True}, 'error_count': {'count': error_count, 'requires_attention': True}, } @@ -192,13 +197,14 @@ def _report_check_summary(self, metrics): self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers checked: {metrics['total_providers']['count']}") - self.stdout.write(f" Null configs: {metrics['null_config_count']['count']}") + self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") if total_requiring_attention > 0: self.stdout.write("\nIssues requiring attention:") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") + self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") self.stdout.write(f" Errors: {metrics['error_count']['count']}") self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") else: diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 6963d5dcd0d5..9e12c0aa45bd 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -344,26 +344,27 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): """ old_config, new_config, test_provider_config = self._setup_test_configs_for_run_checks() - output = self._run_checks_command() + with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', + return_value={'entity_id': 'test'}): + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) self.assertIn( - f'id={old_config.id} which should be updated to the current SAML config (id={new_config.id})', + f'has outdated SAML config (id={old_config.id}) which should be updated to the current SAML config (id={new_config.id})', output ) self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) self.assertIn('Outdated: 1', output) - # Check key observability calls expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), mock.call('saml_management_command.outdated_count', 1), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), + mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), mock.call('saml_management_command.total_requiring_attention', 2), ] @@ -380,26 +381,27 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): entity_id='https://example.com' ) - SAMLProviderConfigFactory.create( + provider = SAMLProviderConfigFactory.create( site=self.site, slug='test-provider', saml_configuration=config ) - output = self._run_checks_command() + with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', + return_value={'entity_id': 'test'}): + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) self.assertIn('does not match the provider\'s site_id', output) - # Check observability calls expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 1), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), + mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), mock.call('saml_management_command.total_requiring_attention', 2), ] @@ -416,26 +418,27 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): entity_id='https://example.com' ) - SAMLProviderConfigFactory.create( + provider = SAMLProviderConfigFactory.create( site=self.site, slug='provider-slug', saml_configuration=config ) - output = self._run_checks_command() + with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', + return_value={'entity_id': 'test'}): + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('provider-slug', output) self.assertIn('does not match the provider\'s slug', output) - # Check observability calls expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), + mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), mock.call('saml_management_command.total_requiring_attention', 1), ] @@ -446,19 +449,21 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): """ Test the --run-checks command identifies providers with null configurations. """ - SAMLProviderConfigFactory.create( + # Create a provider without a configuration + provider = SAMLProviderConfigFactory.create( site=self.site, slug='null-provider', saml_configuration=None ) - output = self._run_checks_command() + with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', + return_value=None): + output = self._run_checks_command() - self.assertIn('[INFO]', output) + self.assertIn('[WARNING]', output) self.assertIn('null-provider', output) - self.assertIn('has no SAML configuration because a matching default was not found', output) + self.assertIn('has no SAML configuration and no matching default configuration was found', output) - # Check observability calls expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), @@ -467,6 +472,40 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): mock.call('saml_management_command.slug_mismatch_count', 0), mock.call('saml_management_command.null_config_count', 2), mock.call('saml_management_command.error_count', 0), + mock.call('saml_management_command.total_requiring_attention', 2), + ] + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_with_default_config(self, mock_set_custom_attribute): + """ + Test the --run-checks command correctly handles providers with default configurations. + """ + # Create a provider without a direct configuration + provider = SAMLProviderConfigFactory.create( + site=self.site, + slug='default-config-provider', + saml_configuration=None + ) + + with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', + return_value={'entity_id': 'default-config'}): + output = self._run_checks_command() + + self.assertNotIn('default-config-provider has no SAML configuration', output) + + self.assertIn('Providers checked: 2', output) + self.assertIn('Missing configs: 0', output) + self.assertIn('No configuration issues found!', output) + + expected_calls = [ + mock.call('saml_management_command.operation', 'run_checks'), + mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.outdated_count', 0), + mock.call('saml_management_command.site_mismatch_count', 0), + mock.call('saml_management_command.slug_mismatch_count', 0), + mock.call('saml_management_command.null_config_count', 0), + mock.call('saml_management_command.error_count', 0), mock.call('saml_management_command.total_requiring_attention', 0), ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) From e3c1b6f006c2923eaca81ae7c72175474b69af40 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 22 Sep 2025 11:59:24 +0000 Subject: [PATCH 02/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/saml.py | 5 +++-- .../management/commands/tests/test_saml.py | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 541d0d32168d..c7d0b61c3c04 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -119,7 +119,7 @@ def _check_provider_configurations(self): # Use get_config() to get the actual configuration that would be used # This includes both direct configuration and default fallback logic actual_config = provider_config.get_config() - + if not actual_config: self.stdout.write( f"[WARNING] {provider_info} has no SAML configuration and " @@ -155,9 +155,10 @@ def _check_provider_configurations(self): provider_config_slug = provider_config.slug if saml_configuration_slug not in (provider_config_slug, 'default'): + config_id = provider_config.saml_configuration_id self.stdout.write( f"[WARNING] {provider_info} " - f"SAML config (id={provider_config.saml_configuration_id}, slug='{saml_configuration_slug}') " + f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " "does not match the provider's slug." ) slug_mismatch_count += 1 diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 9e12c0aa45bd..85a76995fe84 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -350,10 +350,11 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) - self.assertIn( - f'has outdated SAML config (id={old_config.id}) which should be updated to the current SAML config (id={new_config.id})', - output + outdated_msg = ( + f'has outdated SAML config (id={old_config.id}) which should be updated to ' + f'the current SAML config (id={new_config.id})' ) + self.assertIn(outdated_msg, output) self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) self.assertIn('Outdated: 1', output) From 99eee7ee671decd680382e62a1fa9109619e8705 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 23 Sep 2025 10:29:34 +0000 Subject: [PATCH 03/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 45 +++++++++---------- .../management/commands/tests/test_saml.py | 28 ++++++------ 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index c7d0b61c3c04..aed1d99e5fe3 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -71,16 +71,8 @@ def _handle_run_checks(self): """ Handle the --run-checks option for checking SAMLProviderConfig configuration issues. - This is a report-only command. It identifies potential configuration problems such as: - - Outdated SAMLConfiguration references (provider pointing to old config version) - - Site ID mismatches between SAMLProviderConfig and its SAMLConfiguration - - Slug mismatches (except 'default' slugs) # noqa: E501 - - SAMLProviderConfig objects with no available configuration (no direct config AND no default) - - Uses get_config() to accurately determine if a provider has usable configuration, - eliminating false warnings for providers that correctly use default configurations. - - Includes observability attributes for monitoring. + This is a report-only command that identifies potential configuration problems + and includes observability attributes for monitoring. """ # Set custom attributes for monitoring the check operation # .. custom_attribute_name: saml_management_command.operation @@ -92,7 +84,11 @@ def _handle_run_checks(self): def _check_provider_configurations(self): """ - Check each provider configuration for potential issues. + Check each provider configuration for potential issues: + - Outdated configuration references + - Site ID mismatches + - Slug mismatches + - Missing configurations (no direct config and no default) Returns a dictionary of metrics about the found issues. """ outdated_count = 0 @@ -116,18 +112,6 @@ def _check_provider_configurations(self): ) try: - # Use get_config() to get the actual configuration that would be used - # This includes both direct configuration and default fallback logic - actual_config = provider_config.get_config() - - if not actual_config: - self.stdout.write( - f"[WARNING] {provider_info} has no SAML configuration and " - "no matching default configuration was found." - ) - null_config_count += 1 - continue - if provider_config.saml_configuration: current_config = SAMLConfiguration.current( provider_config.saml_configuration.site_id, @@ -162,6 +146,21 @@ def _check_provider_configurations(self): "does not match the provider's slug." ) slug_mismatch_count += 1 + else: + try: + default_config = SAMLConfiguration.current(provider_config.site_id, 'default') + if not default_config: + self.stdout.write( + f"[WARNING] {provider_info} has no SAML configuration and " + "no matching default configuration was found." + ) + null_config_count += 1 + except SAMLConfiguration.DoesNotExist: + self.stdout.write( + f"[WARNING] {provider_info} has no SAML configuration and " + "no matching default configuration was found." + ) + null_config_count += 1 except Exception as e: # pylint: disable=broad-except self.stderr.write(f"[ERROR] Error processing {provider_info}: {e}") diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 85a76995fe84..fe1dcd6c0a62 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -17,6 +17,7 @@ from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from common.djangoapps.third_party_auth.tests.factories import SAMLConfigurationFactory, SAMLProviderConfigFactory +from common.djangoapps.third_party_auth.models import SAMLConfiguration def mock_get(status_code=200): @@ -344,9 +345,7 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): """ old_config, new_config, test_provider_config = self._setup_test_configs_for_run_checks() - with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', - return_value={'entity_id': 'test'}): - output = self._run_checks_command() + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) @@ -388,9 +387,7 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): saml_configuration=config ) - with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', - return_value={'entity_id': 'test'}): - output = self._run_checks_command() + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) @@ -425,9 +422,7 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): saml_configuration=config ) - with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', - return_value={'entity_id': 'test'}): - output = self._run_checks_command() + output = self._run_checks_command() self.assertIn('[WARNING]', output) self.assertIn('provider-slug', output) @@ -457,8 +452,8 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): saml_configuration=None ) - with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', - return_value=None): + with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.current', + side_effect=SAMLConfiguration.DoesNotExist("No default config")): output = self._run_checks_command() self.assertIn('[WARNING]', output) @@ -489,9 +484,14 @@ def test_run_checks_with_default_config(self, mock_set_custom_attribute): saml_configuration=None ) - with mock.patch('common.djangoapps.third_party_auth.models.SAMLProviderConfig.get_config', - return_value={'entity_id': 'default-config'}): - output = self._run_checks_command() + # Create a default SAML configuration for the site + default_config = SAMLConfigurationFactory.create( + site=self.site, + slug='default', + entity_id='https://default.example.com' + ) + + output = self._run_checks_command() self.assertNotIn('default-config-provider has no SAML configuration', output) From 3dd7d88b0ea6bc33371d6bfe1b949e01dee3ca10 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 23 Sep 2025 12:19:39 +0000 Subject: [PATCH 04/23] fix: Improve SAML configuration checks and update warning messages --- .../djangoapps/third_party_auth/management/commands/saml.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index aed1d99e5fe3..74c1d7d45467 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -140,11 +140,6 @@ def _check_provider_configurations(self): if saml_configuration_slug not in (provider_config_slug, 'default'): config_id = provider_config.saml_configuration_id - self.stdout.write( - f"[WARNING] {provider_info} " - f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " - "does not match the provider's slug." - ) slug_mismatch_count += 1 else: try: From e00af91fc63226d7abd927cb7a62970534fda5b5 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 23 Sep 2025 12:30:00 +0000 Subject: [PATCH 05/23] fix: Improve SAML configuration checks and update warning messages --- .../djangoapps/third_party_auth/management/commands/saml.py | 4 ++++ .../third_party_auth/management/commands/tests/test_saml.py | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 74c1d7d45467..42ee3caf6e3f 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -140,6 +140,10 @@ def _check_provider_configurations(self): if saml_configuration_slug not in (provider_config_slug, 'default'): config_id = provider_config.saml_configuration_id + self.stdout.write( + f"[INFO] {provider_info} " + f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " + ) slug_mismatch_count += 1 else: try: diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index fe1dcd6c0a62..729b043f8c4f 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -424,9 +424,9 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): output = self._run_checks_command() - self.assertIn('[WARNING]', output) + self.assertIn('[INFO]', output) self.assertIn('provider-slug', output) - self.assertIn('does not match the provider\'s slug', output) + self.assertIn('slug=\'config-slug\'', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), From c0f5c348b726ee80f30027e3cf0f095d361857f1 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 24 Sep 2025 13:24:18 +0000 Subject: [PATCH 06/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 19 +++++++++++-------- .../management/commands/tests/test_saml.py | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 42ee3caf6e3f..7ee268a1d5d7 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -135,29 +135,32 @@ def _check_provider_configurations(self): ) site_mismatch_count += 1 - saml_configuration_slug = provider_config.saml_configuration.slug - provider_config_slug = provider_config.slug - - if saml_configuration_slug not in (provider_config_slug, 'default'): + if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): config_id = provider_config.saml_configuration_id self.stdout.write( f"[INFO] {provider_info} " - f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " + f"SAML config (id={config_id}, slug='{provider_config.saml_configuration.slug}') " ) slug_mismatch_count += 1 else: + # Provider has no direct SAML configuration - check for a default one try: default_config = SAMLConfiguration.current(provider_config.site_id, 'default') if not default_config: self.stdout.write( - f"[WARNING] {provider_info} has no SAML configuration and " + f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found." ) null_config_count += 1 + else: + self.stdout.write( + f"[INFO] {provider_info} has no direct SAML configuration but " + f"is using default configuration (id={default_config.id})." + ) except SAMLConfiguration.DoesNotExist: self.stdout.write( - f"[WARNING] {provider_info} has no SAML configuration and " - "no matching default configuration was found." + f"[WARNING] {provider_info} has no direct SAML configuration and " + "no matching default configuration was found (DoesNotExist)." ) null_config_count += 1 diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 729b043f8c4f..fa932c415101 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -458,7 +458,7 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): self.assertIn('[WARNING]', output) self.assertIn('null-provider', output) - self.assertIn('has no SAML configuration and no matching default configuration was found', output) + self.assertIn('has no direct SAML configuration and no matching default configuration was found', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), From e37bf164aa854337738ff2200f648143ace5ee25 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Fri, 26 Sep 2025 05:21:55 +0000 Subject: [PATCH 07/23] fix: Improve SAML configuration checks and update warning messages --- common/djangoapps/third_party_auth/management/commands/saml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 7ee268a1d5d7..cfcce34862ea 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -118,7 +118,7 @@ def _check_provider_configurations(self): provider_config.saml_configuration.slug ) - if current_config and current_config.id != provider_config.saml_configuration_id: + if current_config and (current_config.id != provider_config.saml_configuration_id): self.stdout.write( f"[WARNING] {provider_info} " f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " From 92dee4ffeaf9919c885436f2ce93ac76f794d37b Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 29 Sep 2025 06:01:13 +0000 Subject: [PATCH 08/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/saml.py | 10 ++++++---- .../management/commands/tests/test_saml.py | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index cfcce34862ea..11bcb0e2ef03 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -87,8 +87,8 @@ def _check_provider_configurations(self): Check each provider configuration for potential issues: - Outdated configuration references - Site ID mismatches - - Slug mismatches - Missing configurations (no direct config and no default) + Also reports informational data such as slug mismatches. Returns a dictionary of metrics about the found issues. """ outdated_count = 0 @@ -137,9 +137,11 @@ def _check_provider_configurations(self): if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): config_id = provider_config.saml_configuration_id + saml_configuration_slug = provider_config.saml_configuration.slug self.stdout.write( f"[INFO] {provider_info} " - f"SAML config (id={config_id}, slug='{provider_config.saml_configuration.slug}') " + f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " + "does not match the provider's slug." ) slug_mismatch_count += 1 else: @@ -172,7 +174,7 @@ def _check_provider_configurations(self): 'total_providers': {'count': total_providers, 'requires_attention': False}, 'outdated_count': {'count': outdated_count, 'requires_attention': True}, 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, - 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': True}, + 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': False}, 'null_config_count': {'count': null_config_count, 'requires_attention': True}, 'error_count': {'count': error_count, 'requires_attention': True}, } @@ -199,13 +201,13 @@ def _report_check_summary(self, metrics): self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers checked: {metrics['total_providers']['count']}") + self.stdout.write(f" Slug mismatches (informational): {metrics['slug_mismatch_count']['count']}") self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") if total_requiring_attention > 0: self.stdout.write("\nIssues requiring attention:") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") - self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") self.stdout.write(f" Errors: {metrics['error_count']['count']}") self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index fa932c415101..e287240a2131 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -366,7 +366,7 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), + mock.call('saml_management_command.total_requiring_attention', 1), ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -401,7 +401,7 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), + mock.call('saml_management_command.total_requiring_attention', 1), ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -436,7 +436,7 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), + mock.call('saml_management_command.total_requiring_attention', 0), ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) From 9614bd5f4233cc1169700fd7b2ad9d7ad9748239 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 29 Sep 2025 07:10:20 +0000 Subject: [PATCH 09/23] fix: Improve SAML configuration checks and update warning messages --- common/djangoapps/third_party_auth/management/commands/saml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 11bcb0e2ef03..572ad63a6f42 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -201,7 +201,7 @@ def _report_check_summary(self, metrics): self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers checked: {metrics['total_providers']['count']}") - self.stdout.write(f" Slug mismatches (informational): {metrics['slug_mismatch_count']['count']}") + self.stdout.write(f" Slug mismatches (INFO): {metrics['slug_mismatch_count']['count']}") self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") if total_requiring_attention > 0: From 93b9c21ef9a5ab06f728ace48a52c5473de89688 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 29 Sep 2025 18:57:14 +0000 Subject: [PATCH 10/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 23 +++++-- .../management/commands/tests/test_saml.py | 68 +++++++++++++++++++ 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 572ad63a6f42..c060aa83050b 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -148,7 +148,7 @@ def _check_provider_configurations(self): # Provider has no direct SAML configuration - check for a default one try: default_config = SAMLConfiguration.current(provider_config.site_id, 'default') - if not default_config: + if not default_config or default_config.id is None: self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found." @@ -201,15 +201,24 @@ def _report_check_summary(self, metrics): self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers checked: {metrics['total_providers']['count']}") - self.stdout.write(f" Slug mismatches (INFO): {metrics['slug_mismatch_count']['count']}") - self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") + self.stdout.write("") + # Informational only section + self.stdout.write("Informational only:") + self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") + if metrics['null_config_count']['count'] == 0: + self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") + self.stdout.write("") + + # Issues requiring attention section if total_requiring_attention > 0: - self.stdout.write("\nIssues requiring attention:") + self.stdout.write("Issues requiring attention:") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") - self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") + if metrics['null_config_count']['count'] > 0: + self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") self.stdout.write(f" Errors: {metrics['error_count']['count']}") - self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") + self.stdout.write("") + self.stdout.write(f"Total issues requiring attention: {total_requiring_attention}") else: - self.stdout.write(self.style.SUCCESS("\nNo configuration issues found!")) + self.stdout.write(self.style.SUCCESS("No configuration issues found!")) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index e287240a2131..766fa47a060e 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -356,7 +356,11 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn(outdated_msg, output) self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Slug mismatches: 1', output) + self.assertIn('Issues requiring attention:', output) self.assertIn('Outdated: 1', output) + self.assertIn('Total issues requiring attention: 1', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), @@ -392,6 +396,13 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) self.assertIn('does not match the provider\'s site_id', output) + self.assertIn('CHECK SUMMARY:', output) + self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Slug mismatches: 1', output) + self.assertIn('Issues requiring attention:', output) + self.assertIn('Site mismatches: 1', output) + self.assertIn('Total issues requiring attention: 1', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), @@ -427,6 +438,11 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): self.assertIn('[INFO]', output) self.assertIn('provider-slug', output) self.assertIn('slug=\'config-slug\'', output) + self.assertIn('CHECK SUMMARY:', output) + self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Slug mismatches: 1', output) + self.assertIn('No configuration issues found!', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), @@ -459,6 +475,55 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): self.assertIn('[WARNING]', output) self.assertIn('null-provider', output) self.assertIn('has no direct SAML configuration and no matching default configuration was found', output) + self.assertIn('CHECK SUMMARY:', output) + self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Issues requiring attention:', output) + self.assertIn('Missing configs: 2', output) + self.assertIn('Total issues requiring attention: 2', output) + + expected_calls = [ + mock.call('saml_management_command.operation', 'run_checks'), + mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.outdated_count', 0), + mock.call('saml_management_command.site_mismatch_count', 0), + mock.call('saml_management_command.slug_mismatch_count', 0), + mock.call('saml_management_command.null_config_count', 2), + mock.call('saml_management_command.error_count', 0), + mock.call('saml_management_command.total_requiring_attention', 2), + ] + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_null_config_id(self, mock_set_custom_attribute): + """ + Test the --run-checks command identifies providers with configurations that have null IDs. + This tests the new logic that checks for default_config.id is None. + """ + # Create a provider without a configuration + provider = SAMLProviderConfigFactory.create( + site=self.site, + slug='null-id-provider', + saml_configuration=None + ) + + # Create a mock config object with id=None (simulates broken default config) + mock_config = mock.Mock() + mock_config.id = None + + with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.current', + return_value=mock_config): + output = self._run_checks_command() + + self.assertIn('[WARNING]', output) + self.assertIn('null-id-provider', output) + self.assertIn('has no direct SAML configuration and no matching default configuration was found', output) + self.assertIn('CHECK SUMMARY:', output) + self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Issues requiring attention:', output) + self.assertIn('Missing configs: 2', output) + self.assertIn('Total issues requiring attention: 2', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), @@ -495,7 +560,10 @@ def test_run_checks_with_default_config(self, mock_set_custom_attribute): self.assertNotIn('default-config-provider has no SAML configuration', output) + self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) + self.assertIn('Informational only:', output) + self.assertIn('Slug mismatches: 0', output) self.assertIn('Missing configs: 0', output) self.assertIn('No configuration issues found!', output) From 0148c0a4c1f594d6bcd9005b90a7bcca9c90fc07 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 30 Sep 2025 18:02:04 +0000 Subject: [PATCH 11/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 57 ++++++++++-- .../management/commands/tests/test_saml.py | 86 +++++++++++++++++-- 2 files changed, 130 insertions(+), 13 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index c060aa83050b..3358d6c312e0 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -88,7 +88,10 @@ def _check_provider_configurations(self): - Outdated configuration references - Site ID mismatches - Missing configurations (no direct config and no default) + - Disabled providers and configurations Also reports informational data such as slug mismatches. + + See code comments near each log output for possible resolution details. Returns a dictionary of metrics about the found issues. """ outdated_count = 0 @@ -96,6 +99,8 @@ def _check_provider_configurations(self): slug_mismatch_count = 0 null_config_count = 0 error_count = 0 + disabled_provider_count = 0 + disabled_config_count = 0 total_providers = 0 provider_configs = SAMLProviderConfig.objects.current_set() @@ -106,19 +111,43 @@ def _check_provider_configurations(self): for provider_config in provider_configs: total_providers += 1 + + # Check if provider is disabled + provider_disabled = not provider_config.enabled + disabled_status = "[DISABLED] " if provider_disabled else "" + provider_info = ( - f"Provider (id={provider_config.id}, name={provider_config.name}, " + f"{disabled_status}Provider (id={provider_config.id}, name={provider_config.name}, " f"slug={provider_config.slug}, site_id={provider_config.site_id})" ) + if provider_disabled: + disabled_provider_count += 1 + # Resolution: Enable the provider in Django admin if it should be active + self.stdout.write( + f"[INFO] {provider_info} is disabled." + ) + # Still check configuration issues for disabled providers as they may be relevant when re-enabling + try: if provider_config.saml_configuration: + # Check if SAML configuration is disabled + config_disabled = not provider_config.saml_configuration.enabled + if config_disabled: + disabled_config_count += 1 + # Resolution: Enable the SAML configuration in Django admin or assign a different configuration + self.stdout.write( + f"[WARNING] {provider_info} " + f"has DISABLED SAML config (id={provider_config.saml_configuration_id})." + ) + current_config = SAMLConfiguration.current( provider_config.saml_configuration.site_id, provider_config.saml_configuration.slug ) if current_config and (current_config.id != provider_config.saml_configuration_id): + # Resolution: Update the provider's saml_configuration_id to the current config ID self.stdout.write( f"[WARNING] {provider_info} " f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " @@ -128,6 +157,7 @@ def _check_provider_configurations(self): if provider_config.saml_configuration.site_id != provider_config.site_id: config_site_id = provider_config.saml_configuration.site_id + # Resolution: Create a new SAML configuration for the correct site or move the provider to the matching site self.stdout.write( f"[WARNING] {provider_info} " f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " @@ -138,6 +168,7 @@ def _check_provider_configurations(self): if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): config_id = provider_config.saml_configuration_id saml_configuration_slug = provider_config.saml_configuration.slug + # Resolution: This is informational only - provider can use a different slug configuration self.stdout.write( f"[INFO] {provider_info} " f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " @@ -149,17 +180,29 @@ def _check_provider_configurations(self): try: default_config = SAMLConfiguration.current(provider_config.site_id, 'default') if not default_config or default_config.id is None: + # Resolution: Create a SAML configuration for this provider or create a default configuration for the site self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found." ) null_config_count += 1 else: - self.stdout.write( - f"[INFO] {provider_info} has no direct SAML configuration but " - f"is using default configuration (id={default_config.id})." - ) + # Check if the default configuration is disabled + if not default_config.enabled: + disabled_config_count += 1 + # Resolution: Enable the default SAML configuration or create a specific configuration for this provider + self.stdout.write( + f"[WARNING] {provider_info} has no direct SAML configuration and " + f"the default configuration (id={default_config.id}) is DISABLED." + ) + else: + # Resolution: This is normal operation - no action needed + self.stdout.write( + f"[INFO] {provider_info} has no direct SAML configuration but " + f"is using default configuration (id={default_config.id})." + ) except SAMLConfiguration.DoesNotExist: + # Resolution: Create a SAML configuration for this provider or create a default configuration for the site self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found (DoesNotExist)." @@ -172,6 +215,8 @@ def _check_provider_configurations(self): metrics = { 'total_providers': {'count': total_providers, 'requires_attention': False}, + 'disabled_provider_count': {'count': disabled_provider_count, 'requires_attention': False}, + 'disabled_config_count': {'count': disabled_config_count, 'requires_attention': True}, 'outdated_count': {'count': outdated_count, 'requires_attention': True}, 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': False}, @@ -205,6 +250,7 @@ def _report_check_summary(self, metrics): # Informational only section self.stdout.write("Informational only:") + self.stdout.write(f" Disabled providers: {metrics['disabled_provider_count']['count']}") self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") if metrics['null_config_count']['count'] == 0: self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") @@ -213,6 +259,7 @@ def _report_check_summary(self, metrics): # Issues requiring attention section if total_requiring_attention > 0: self.stdout.write("Issues requiring attention:") + self.stdout.write(f" Disabled configurations: {metrics['disabled_config_count']['count']}") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") if metrics['null_config_count']['count'] > 0: diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 766fa47a060e..74543484e5f3 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -360,17 +360,19 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn('Slug mismatches: 1', output) self.assertIn('Issues requiring attention:', output) self.assertIn('Outdated: 1', output) - self.assertIn('Total issues requiring attention: 1', output) + self.assertIn('Total issues requiring attention: 3', output) # 1 outdated + 2 disabled configs expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 2), # 1 from setUp + 1 from test config mock.call('saml_management_command.outdated_count', 1), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), + mock.call('saml_management_command.total_requiring_attention', 3), # 1 outdated + 2 disabled configs ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -402,17 +404,19 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): self.assertIn('Slug mismatches: 1', output) self.assertIn('Issues requiring attention:', output) self.assertIn('Site mismatches: 1', output) - self.assertIn('Total issues requiring attention: 1', output) + self.assertIn('Total issues requiring attention: 2', output) # 1 site mismatch + 1 disabled config expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 1), mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), + mock.call('saml_management_command.total_requiring_attention', 2), # 1 site mismatch + 1 disabled config ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -442,17 +446,19 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 1', output) - self.assertIn('No configuration issues found!', output) + self.assertIn('Total issues requiring attention: 1', output) # 1 disabled config from setUp expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 0), + mock.call('saml_management_command.total_requiring_attention', 1), # 1 disabled config from setUp ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -485,6 +491,8 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 0), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), @@ -528,6 +536,8 @@ def test_run_checks_null_config_id(self, mock_set_custom_attribute): expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 0), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), @@ -565,16 +575,76 @@ def test_run_checks_with_default_config(self, mock_set_custom_attribute): self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 0', output) self.assertIn('Missing configs: 0', output) - self.assertIn('No configuration issues found!', output) + self.assertIn('Total issues requiring attention: 1', output) # 1 disabled config from setUp expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), + mock.call('saml_management_command.disabled_provider_count', 0), + mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), mock.call('saml_management_command.null_config_count', 0), mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 0), + mock.call('saml_management_command.total_requiring_attention', 1), # 1 disabled config from setUp + ] + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_disabled_functionality(self, mock_set_custom_attribute): + """ + Test the --run-checks command handles disabled providers and configurations. + """ + # Create a disabled provider + disabled_provider = SAMLProviderConfigFactory.create( + site=self.site, + slug='disabled-provider', + enabled=False + ) + + # Create a disabled SAML configuration + disabled_config = SAMLConfigurationFactory.create( + site=self.site, + slug='disabled-config', + enabled=False + ) + + # Create a provider that uses the disabled config + provider_with_disabled_config = SAMLProviderConfigFactory.create( + site=self.site, + slug='provider-with-disabled-config', + saml_configuration=disabled_config + ) + + output = self._run_checks_command() + + # Check disabled provider detection + self.assertIn('[DISABLED] Provider', output) + self.assertIn('disabled-provider', output) + self.assertIn('is disabled', output) + + # Check disabled config detection + self.assertIn('has DISABLED SAML config', output) + self.assertIn('provider-with-disabled-config', output) + + # Verify no Resolution: text appears + self.assertNotIn('Resolution:', output) + + # Check summary includes new metrics + self.assertIn('Disabled providers: 1', output) + self.assertIn('Disabled configurations: 2', output) # 1 from test + 1 from setUp + + expected_calls = [ + mock.call('saml_management_command.operation', 'run_checks'), + mock.call('saml_management_command.total_providers', 3), + mock.call('saml_management_command.disabled_provider_count', 1), + mock.call('saml_management_command.disabled_config_count', 2), # 1 from test + 1 from setUp + mock.call('saml_management_command.outdated_count', 0), + mock.call('saml_management_command.site_mismatch_count', 0), + mock.call('saml_management_command.slug_mismatch_count', 1), + mock.call('saml_management_command.null_config_count', 1), # 1 from disabled provider having no config + mock.call('saml_management_command.error_count', 0), + mock.call('saml_management_command.total_requiring_attention', 3), # 2 disabled configs + 1 null config ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) From 9e113d3972ace05b6e0e3fc5158d63f7d609b351 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 30 Sep 2025 18:37:06 +0000 Subject: [PATCH 12/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 178 ++++++++++-------- 1 file changed, 96 insertions(+), 82 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 3358d6c312e0..65314a772217 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -117,97 +117,72 @@ def _check_provider_configurations(self): disabled_status = "[DISABLED] " if provider_disabled else "" provider_info = ( - f"{disabled_status}Provider (id={provider_config.id}, name={provider_config.name}, " - f"slug={provider_config.slug}, site_id={provider_config.site_id})" + f"{disabled_status}Provider (id={provider_config.id}, " + f"name={provider_config.name}, slug={provider_config.slug}, " + f"site_id={provider_config.site_id})" ) if provider_disabled: disabled_provider_count += 1 # Resolution: Enable the provider in Django admin if it should be active - self.stdout.write( - f"[INFO] {provider_info} is disabled." - ) - # Still check configuration issues for disabled providers as they may be relevant when re-enabling + self.stdout.write(f"[INFO] {provider_info} is disabled.") try: - if provider_config.saml_configuration: - # Check if SAML configuration is disabled - config_disabled = not provider_config.saml_configuration.enabled - if config_disabled: - disabled_config_count += 1 - # Resolution: Enable the SAML configuration in Django admin or assign a different configuration - self.stdout.write( - f"[WARNING] {provider_info} " - f"has DISABLED SAML config (id={provider_config.saml_configuration_id})." - ) - - current_config = SAMLConfiguration.current( - provider_config.saml_configuration.site_id, - provider_config.saml_configuration.slug + if not provider_config.saml_configuration: + disabled_config_count, null_config_count = self._check_no_config( + provider_config, provider_info, disabled_config_count, null_config_count + ) + continue + + # Check if SAML configuration is disabled + if not provider_config.saml_configuration.enabled: + disabled_config_count += 1 + # Resolution: Enable the SAML configuration in Django admin + # or assign a different configuration + self.stdout.write( + f"[WARNING] {provider_info} " + f"has DISABLED SAML config (id={provider_config.saml_configuration_id})." ) - if current_config and (current_config.id != provider_config.saml_configuration_id): - # Resolution: Update the provider's saml_configuration_id to the current config ID - self.stdout.write( - f"[WARNING] {provider_info} " - f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " - f"should be updated to the current SAML config (id={current_config.id})." - ) - outdated_count += 1 - - if provider_config.saml_configuration.site_id != provider_config.site_id: - config_site_id = provider_config.saml_configuration.site_id - # Resolution: Create a new SAML configuration for the correct site or move the provider to the matching site - self.stdout.write( - f"[WARNING] {provider_info} " - f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " - "does not match the provider's site_id." - ) - site_mismatch_count += 1 - - if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): - config_id = provider_config.saml_configuration_id - saml_configuration_slug = provider_config.saml_configuration.slug - # Resolution: This is informational only - provider can use a different slug configuration - self.stdout.write( - f"[INFO] {provider_info} " - f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " - "does not match the provider's slug." - ) - slug_mismatch_count += 1 - else: - # Provider has no direct SAML configuration - check for a default one - try: - default_config = SAMLConfiguration.current(provider_config.site_id, 'default') - if not default_config or default_config.id is None: - # Resolution: Create a SAML configuration for this provider or create a default configuration for the site - self.stdout.write( - f"[WARNING] {provider_info} has no direct SAML configuration and " - "no matching default configuration was found." - ) - null_config_count += 1 - else: - # Check if the default configuration is disabled - if not default_config.enabled: - disabled_config_count += 1 - # Resolution: Enable the default SAML configuration or create a specific configuration for this provider - self.stdout.write( - f"[WARNING] {provider_info} has no direct SAML configuration and " - f"the default configuration (id={default_config.id}) is DISABLED." - ) - else: - # Resolution: This is normal operation - no action needed - self.stdout.write( - f"[INFO] {provider_info} has no direct SAML configuration but " - f"is using default configuration (id={default_config.id})." - ) - except SAMLConfiguration.DoesNotExist: - # Resolution: Create a SAML configuration for this provider or create a default configuration for the site - self.stdout.write( - f"[WARNING] {provider_info} has no direct SAML configuration and " - "no matching default configuration was found (DoesNotExist)." - ) - null_config_count += 1 + # Check configuration currency + current_config = SAMLConfiguration.current( + provider_config.saml_configuration.site_id, + provider_config.saml_configuration.slug + ) + + if current_config and (current_config.id != provider_config.saml_configuration_id): + # Resolution: Update the provider's saml_configuration_id to the current config ID + self.stdout.write( + f"[WARNING] {provider_info} " + f"has outdated SAML config (id={provider_config.saml_configuration_id}) which " + f"should be updated to the current SAML config (id={current_config.id})." + ) + outdated_count += 1 + + # Check site ID match + if provider_config.saml_configuration.site_id != provider_config.site_id: + config_site_id = provider_config.saml_configuration.site_id + # Resolution: Create a new SAML configuration for the correct site + # or move the provider to the matching site + self.stdout.write( + f"[WARNING] {provider_info} " + f"SAML config (id={provider_config.saml_configuration_id}, " + f"site_id={config_site_id}) does not match the provider's site_id." + ) + site_mismatch_count += 1 + + # Check slug match + if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): + config_id = provider_config.saml_configuration_id + saml_configuration_slug = provider_config.saml_configuration.slug + # Resolution: This is informational only - provider can use + # a different slug configuration + self.stdout.write( + f"[INFO] {provider_info} " + f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " + "does not match the provider's slug." + ) + slug_mismatch_count += 1 except Exception as e: # pylint: disable=broad-except self.stderr.write(f"[ERROR] Error processing {provider_info}: {e}") @@ -231,6 +206,45 @@ def _check_provider_configurations(self): return metrics + def _check_no_config(self, provider_config, provider_info, disabled_config_count, null_config_count): + """Helper to check providers with no direct SAML configuration.""" + try: + default_config = SAMLConfiguration.current(provider_config.site_id, 'default') + if not default_config or default_config.id is None: + # Resolution: Create a SAML configuration for this provider + # or create a default configuration for the site + self.stdout.write( + f"[WARNING] {provider_info} has no direct SAML configuration and " + "no matching default configuration was found." + ) + null_config_count += 1 + return disabled_config_count, null_config_count + + if not default_config.enabled: + disabled_config_count += 1 + # Resolution: Enable the default SAML configuration + # or create a specific configuration for this provider + self.stdout.write( + f"[WARNING] {provider_info} has no direct SAML configuration and " + f"the default configuration (id={default_config.id}) is DISABLED." + ) + else: + # Resolution: This is normal operation - no action needed + self.stdout.write( + f"[INFO] {provider_info} has no direct SAML configuration but " + f"is using default configuration (id={default_config.id})." + ) + except SAMLConfiguration.DoesNotExist: + # Resolution: Create a SAML configuration for this provider + # or create a default configuration for the site + self.stdout.write( + f"[WARNING] {provider_info} has no direct SAML configuration and " + "no matching default configuration was found (DoesNotExist)." + ) + null_config_count += 1 + + return disabled_config_count, null_config_count + def _report_check_summary(self, metrics): """ Print a summary of the check results and set the total_requiring_attention custom attribute. From c2fcffab82f289c561fb09721d6a4bad8000ecf2 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 1 Oct 2025 10:42:20 +0000 Subject: [PATCH 13/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 47 +++++---------- .../management/commands/tests/test_saml.py | 59 ++++++++----------- 2 files changed, 39 insertions(+), 67 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 65314a772217..9c311c627ffd 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -99,8 +99,6 @@ def _check_provider_configurations(self): slug_mismatch_count = 0 null_config_count = 0 error_count = 0 - disabled_provider_count = 0 - disabled_config_count = 0 total_providers = 0 provider_configs = SAMLProviderConfig.objects.current_set() @@ -114,34 +112,30 @@ def _check_provider_configurations(self): # Check if provider is disabled provider_disabled = not provider_config.enabled - disabled_status = "[DISABLED] " if provider_disabled else "" + disabled_status = ", enabled=False" if provider_disabled else "" provider_info = ( - f"{disabled_status}Provider (id={provider_config.id}, " + f"Provider (id={provider_config.id}, " f"name={provider_config.name}, slug={provider_config.slug}, " - f"site_id={provider_config.site_id})" + f"site_id={provider_config.site_id}{disabled_status})" ) - if provider_disabled: - disabled_provider_count += 1 - # Resolution: Enable the provider in Django admin if it should be active - self.stdout.write(f"[INFO] {provider_info} is disabled.") + # Provider disabled status is already included in provider_info format try: if not provider_config.saml_configuration: - disabled_config_count, null_config_count = self._check_no_config( - provider_config, provider_info, disabled_config_count, null_config_count + null_config_count = self._check_no_config( + provider_config, provider_info, null_config_count ) continue # Check if SAML configuration is disabled if not provider_config.saml_configuration.enabled: - disabled_config_count += 1 # Resolution: Enable the SAML configuration in Django admin # or assign a different configuration self.stdout.write( f"[WARNING] {provider_info} " - f"has DISABLED SAML config (id={provider_config.saml_configuration_id})." + f"has SAML config (id={provider_config.saml_configuration_id}, enabled=False)." ) # Check configuration currency @@ -175,12 +169,13 @@ def _check_provider_configurations(self): if provider_config.saml_configuration.slug not in (provider_config.slug, 'default'): config_id = provider_config.saml_configuration_id saml_configuration_slug = provider_config.saml_configuration.slug + config_disabled_status = ", enabled=False" if not provider_config.saml_configuration.enabled else "" # Resolution: This is informational only - provider can use # a different slug configuration self.stdout.write( - f"[INFO] {provider_info} " - f"SAML config (id={config_id}, slug='{saml_configuration_slug}') " - "does not match the provider's slug." + f"[INFO] {provider_info} has " + f"SAML config (id={config_id}, slug='{saml_configuration_slug}'{config_disabled_status}) " + "that does not match the provider's slug." ) slug_mismatch_count += 1 @@ -190,8 +185,6 @@ def _check_provider_configurations(self): metrics = { 'total_providers': {'count': total_providers, 'requires_attention': False}, - 'disabled_provider_count': {'count': disabled_provider_count, 'requires_attention': False}, - 'disabled_config_count': {'count': disabled_config_count, 'requires_attention': True}, 'outdated_count': {'count': outdated_count, 'requires_attention': True}, 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': False}, @@ -206,7 +199,7 @@ def _check_provider_configurations(self): return metrics - def _check_no_config(self, provider_config, provider_info, disabled_config_count, null_config_count): + def _check_no_config(self, provider_config, provider_info, null_config_count): """Helper to check providers with no direct SAML configuration.""" try: default_config = SAMLConfiguration.current(provider_config.site_id, 'default') @@ -218,22 +211,16 @@ def _check_no_config(self, provider_config, provider_info, disabled_config_count "no matching default configuration was found." ) null_config_count += 1 - return disabled_config_count, null_config_count + return null_config_count if not default_config.enabled: - disabled_config_count += 1 # Resolution: Enable the default SAML configuration # or create a specific configuration for this provider self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " - f"the default configuration (id={default_config.id}) is DISABLED." - ) - else: - # Resolution: This is normal operation - no action needed - self.stdout.write( - f"[INFO] {provider_info} has no direct SAML configuration but " - f"is using default configuration (id={default_config.id})." + f"the default configuration (id={default_config.id}, enabled=False)." ) + null_config_count += 1 except SAMLConfiguration.DoesNotExist: # Resolution: Create a SAML configuration for this provider # or create a default configuration for the site @@ -243,7 +230,7 @@ def _check_no_config(self, provider_config, provider_info, disabled_config_count ) null_config_count += 1 - return disabled_config_count, null_config_count + return null_config_count def _report_check_summary(self, metrics): """ @@ -264,7 +251,6 @@ def _report_check_summary(self, metrics): # Informational only section self.stdout.write("Informational only:") - self.stdout.write(f" Disabled providers: {metrics['disabled_provider_count']['count']}") self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") if metrics['null_config_count']['count'] == 0: self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") @@ -273,7 +259,6 @@ def _report_check_summary(self, metrics): # Issues requiring attention section if total_requiring_attention > 0: self.stdout.write("Issues requiring attention:") - self.stdout.write(f" Disabled configurations: {metrics['disabled_config_count']['count']}") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") if metrics['null_config_count']['count'] > 0: diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 74543484e5f3..08f31ce8365f 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -360,19 +360,17 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn('Slug mismatches: 1', output) self.assertIn('Issues requiring attention:', output) self.assertIn('Outdated: 1', output) - self.assertIn('Total issues requiring attention: 3', output) # 1 outdated + 2 disabled configs + self.assertIn('Total issues requiring attention: 2', output) # 1 outdated + 1 null from setUp expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 2), # 1 from setUp + 1 from test config mock.call('saml_management_command.outdated_count', 1), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 0), + mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 3), # 1 outdated + 2 disabled configs + mock.call('saml_management_command.total_requiring_attention', 2), # 1 outdated + 1 null ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -404,19 +402,17 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): self.assertIn('Slug mismatches: 1', output) self.assertIn('Issues requiring attention:', output) self.assertIn('Site mismatches: 1', output) - self.assertIn('Total issues requiring attention: 2', output) # 1 site mismatch + 1 disabled config + self.assertIn('Total issues requiring attention: 2', output) # 1 site mismatch + 1 null from setUp expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 1), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 0), + mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), # 1 site mismatch + 1 disabled config + mock.call('saml_management_command.total_requiring_attention', 2), # 1 site mismatch + 1 null ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -441,24 +437,24 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): self.assertIn('[INFO]', output) self.assertIn('provider-slug', output) + self.assertIn('has SAML config', output) self.assertIn('slug=\'config-slug\'', output) + self.assertIn('that does not match the provider\'s slug', output) self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 1', output) - self.assertIn('Total issues requiring attention: 1', output) # 1 disabled config from setUp + self.assertIn('Total issues requiring attention: 1', output) # 1 null from setUp expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 0), + mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), # 1 disabled config from setUp + mock.call('saml_management_command.total_requiring_attention', 1), # 1 null from setUp ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -491,8 +487,6 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 0), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), @@ -536,8 +530,6 @@ def test_run_checks_null_config_id(self, mock_set_custom_attribute): expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 0), mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), @@ -574,20 +566,18 @@ def test_run_checks_with_default_config(self, mock_set_custom_attribute): self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 0', output) - self.assertIn('Missing configs: 0', output) - self.assertIn('Total issues requiring attention: 1', output) # 1 disabled config from setUp + self.assertIn('Missing configs: 1', output) # 1 from setUp + self.assertIn('Total issues requiring attention: 1', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.disabled_provider_count', 0), - mock.call('saml_management_command.disabled_config_count', 1), # 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 0), - mock.call('saml_management_command.null_config_count', 0), + mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), # 1 disabled config from setUp + mock.call('saml_management_command.total_requiring_attention', 1), # 1 null from setUp ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -619,32 +609,29 @@ def test_run_checks_disabled_functionality(self, mock_set_custom_attribute): output = self._run_checks_command() - # Check disabled provider detection - self.assertIn('[DISABLED] Provider', output) + # Check disabled provider shows enabled=False in provider info self.assertIn('disabled-provider', output) - self.assertIn('is disabled', output) + self.assertIn('enabled=False', output) # Check disabled config detection - self.assertIn('has DISABLED SAML config', output) + self.assertIn('has SAML config', output) + self.assertIn('enabled=False', output) self.assertIn('provider-with-disabled-config', output) # Verify no Resolution: text appears self.assertNotIn('Resolution:', output) - # Check summary includes new metrics - self.assertIn('Disabled providers: 1', output) - self.assertIn('Disabled configurations: 2', output) # 1 from test + 1 from setUp + # Check that there are 2 issues requiring attention (null configs from setUp and disabled provider) + self.assertIn('Total issues requiring attention: 2', output) expected_calls = [ mock.call('saml_management_command.operation', 'run_checks'), mock.call('saml_management_command.total_providers', 3), - mock.call('saml_management_command.disabled_provider_count', 1), - mock.call('saml_management_command.disabled_config_count', 2), # 1 from test + 1 from setUp mock.call('saml_management_command.outdated_count', 0), mock.call('saml_management_command.site_mismatch_count', 0), mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), # 1 from disabled provider having no config + mock.call('saml_management_command.null_config_count', 2), # 1 from setUp + 1 from disabled provider mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 3), # 2 disabled configs + 1 null config + mock.call('saml_management_command.total_requiring_attention', 2), # 2 null configs ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) From f3d415ea9aac24df56cf3b459a28c8a904ea2fe6 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 6 Oct 2025 05:26:32 +0000 Subject: [PATCH 14/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/saml.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 9c311c627ffd..29d8c54c5eb3 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -112,12 +112,12 @@ def _check_provider_configurations(self): # Check if provider is disabled provider_disabled = not provider_config.enabled - disabled_status = ", enabled=False" if provider_disabled else "" + disabled_status = "enabled=False" if provider_disabled else "" provider_info = ( f"Provider (id={provider_config.id}, " f"name={provider_config.name}, slug={provider_config.slug}, " - f"site_id={provider_config.site_id}{disabled_status})" + f"site_id={provider_config.site_id}{', ' + disabled_status if provider_disabled else ''})" ) # Provider disabled status is already included in provider_info format @@ -204,8 +204,8 @@ def _check_no_config(self, provider_config, provider_info, null_config_count): try: default_config = SAMLConfiguration.current(provider_config.site_id, 'default') if not default_config or default_config.id is None: - # Resolution: Create a SAML configuration for this provider - # or create a default configuration for the site + # Resolution: Create/Link a SAML configuration for this provider + # or create/link a default configuration for the site self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found." @@ -214,16 +214,16 @@ def _check_no_config(self, provider_config, provider_info, null_config_count): return null_config_count if not default_config.enabled: - # Resolution: Enable the default SAML configuration - # or create a specific configuration for this provider + # Resolution: Enable the provider's linked SAML configuration + # or create/link a specific configuration for this provider self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " f"the default configuration (id={default_config.id}, enabled=False)." ) null_config_count += 1 except SAMLConfiguration.DoesNotExist: - # Resolution: Create a SAML configuration for this provider - # or create a default configuration for the site + # Resolution: Link this provider with a SAML configuration + # or link it with a default configuration for the site self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " "no matching default configuration was found (DoesNotExist)." From 07a090f1cf91d149b976fe62ae43e8f34c768d13 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 7 Oct 2025 07:09:03 +0000 Subject: [PATCH 15/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 70 +++------ .../management/commands/tests/test_saml.py | 146 ++++-------------- 2 files changed, 50 insertions(+), 166 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 29d8c54c5eb3..7f2f28110f82 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -6,7 +6,6 @@ import logging from django.core.management.base import BaseCommand, CommandError -from edx_django_utils.monitoring import set_custom_attribute from common.djangoapps.third_party_auth.tasks import fetch_saml_metadata from common.djangoapps.third_party_auth.models import SAMLProviderConfig, SAMLConfiguration @@ -71,14 +70,8 @@ def _handle_run_checks(self): """ Handle the --run-checks option for checking SAMLProviderConfig configuration issues. - This is a report-only command that identifies potential configuration problems - and includes observability attributes for monitoring. + This is a report-only command that identifies potential configuration problems. """ - # Set custom attributes for monitoring the check operation - # .. custom_attribute_name: saml_management_command.operation - # .. custom_attribute_description: Records current SAML operation ('run_checks'). - set_custom_attribute('saml_management_command.operation', 'run_checks') - metrics = self._check_provider_configurations() self._report_check_summary(metrics) @@ -112,12 +105,12 @@ def _check_provider_configurations(self): # Check if provider is disabled provider_disabled = not provider_config.enabled - disabled_status = "enabled=False" if provider_disabled else "" + disabled_status = ", enabled=False" if provider_disabled else "" provider_info = ( f"Provider (id={provider_config.id}, " f"name={provider_config.name}, slug={provider_config.slug}, " - f"site_id={provider_config.site_id}{', ' + disabled_status if provider_disabled else ''})" + f"site_id={provider_config.site_id}{disabled_status})" ) # Provider disabled status is already included in provider_info format @@ -188,45 +181,31 @@ def _check_provider_configurations(self): 'outdated_count': {'count': outdated_count, 'requires_attention': True}, 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': False}, - 'null_config_count': {'count': null_config_count, 'requires_attention': True}, + 'null_config_count': {'count': null_config_count, 'requires_attention': False}, 'error_count': {'count': error_count, 'requires_attention': True}, } - for key, metric_data in metrics.items(): - # .. custom_attribute_name: saml_management_command.{key} - # .. custom_attribute_description: Records metrics from SAML configuration checks. - set_custom_attribute(f'saml_management_command.{key}', metric_data['count']) - return metrics def _check_no_config(self, provider_config, provider_info, null_config_count): """Helper to check providers with no direct SAML configuration.""" - try: - default_config = SAMLConfiguration.current(provider_config.site_id, 'default') - if not default_config or default_config.id is None: - # Resolution: Create/Link a SAML configuration for this provider - # or create/link a default configuration for the site - self.stdout.write( - f"[WARNING] {provider_info} has no direct SAML configuration and " - "no matching default configuration was found." - ) - null_config_count += 1 - return null_config_count - - if not default_config.enabled: - # Resolution: Enable the provider's linked SAML configuration - # or create/link a specific configuration for this provider - self.stdout.write( - f"[WARNING] {provider_info} has no direct SAML configuration and " - f"the default configuration (id={default_config.id}, enabled=False)." - ) - null_config_count += 1 - except SAMLConfiguration.DoesNotExist: - # Resolution: Link this provider with a SAML configuration - # or link it with a default configuration for the site + default_config = SAMLConfiguration.current(provider_config.site_id, 'default') + if not default_config or default_config.id is None: + # Resolution: Create/Link a SAML configuration for this provider + # or create/link a default configuration for the site + self.stdout.write( + f"[WARNING] {provider_info} has no direct SAML configuration and " + "no matching default configuration was found." + ) + null_config_count += 1 + return null_config_count + + if not default_config.enabled: + # Resolution: Enable the provider's linked SAML configuration + # or create/link a specific configuration for this provider self.stdout.write( f"[WARNING] {provider_info} has no direct SAML configuration and " - "no matching default configuration was found (DoesNotExist)." + f"the default configuration (id={default_config.id}, enabled=False)." ) null_config_count += 1 @@ -234,17 +213,13 @@ def _check_no_config(self, provider_config, provider_info, null_config_count): def _report_check_summary(self, metrics): """ - Print a summary of the check results and set the total_requiring_attention custom attribute. + Print a summary of the check results. """ total_requiring_attention = sum( metric_data['count'] for metric_data in metrics.values() if metric_data['requires_attention'] ) - # .. custom_attribute_name: saml_management_command.total_requiring_attention - # .. custom_attribute_description: The total number of configuration issues requiring attention. - set_custom_attribute('saml_management_command.total_requiring_attention', total_requiring_attention) - self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers checked: {metrics['total_providers']['count']}") self.stdout.write("") @@ -252,8 +227,7 @@ def _report_check_summary(self, metrics): # Informational only section self.stdout.write("Informational only:") self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']['count']}") - if metrics['null_config_count']['count'] == 0: - self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") + self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") self.stdout.write("") # Issues requiring attention section @@ -261,8 +235,6 @@ def _report_check_summary(self, metrics): self.stdout.write("Issues requiring attention:") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") - if metrics['null_config_count']['count'] > 0: - self.stdout.write(f" Missing configs: {metrics['null_config_count']['count']}") self.stdout.write(f" Errors: {metrics['error_count']['count']}") self.stdout.write("") self.stdout.write(f"Total issues requiring attention: {total_requiring_attention}") diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 08f31ce8365f..48774ce53c3f 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -338,8 +338,7 @@ def _run_checks_command(self): call_command('saml', '--run-checks', stdout=out) return out.getvalue() - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_outdated_configs(self, mock_set_custom_attribute): + def test_run_checks_outdated_configs(self): """ Test the --run-checks command identifies outdated configurations. """ @@ -358,24 +357,12 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 1', output) + self.assertIn('Missing configs: 1', output) # 1 null from setUp self.assertIn('Issues requiring attention:', output) self.assertIn('Outdated: 1', output) - self.assertIn('Total issues requiring attention: 2', output) # 1 outdated + 1 null from setUp - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 1), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), # 1 outdated + 1 null - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_site_mismatches(self, mock_set_custom_attribute): + self.assertIn('Total issues requiring attention: 1', output) # 1 outdated only + + def test_run_checks_site_mismatches(self): """ Test the --run-checks command identifies site ID mismatches. """ @@ -400,24 +387,12 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 1', output) + self.assertIn('Missing configs: 1', output) # 1 null from setUp self.assertIn('Issues requiring attention:', output) self.assertIn('Site mismatches: 1', output) - self.assertIn('Total issues requiring attention: 2', output) # 1 site mismatch + 1 null from setUp - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 1), - mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), # 1 site mismatch + 1 null - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): + self.assertIn('Total issues requiring attention: 1', output) # 1 site mismatch only + + def test_run_checks_slug_mismatches(self): """ Test the --run-checks command identifies slug mismatches. """ @@ -444,22 +419,10 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 1', output) - self.assertIn('Total issues requiring attention: 1', output) # 1 null from setUp - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), # 1 null from setUp - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_null_configurations(self, mock_set_custom_attribute): + self.assertIn('Missing configs: 1', output) # 1 null from setUp + self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention + + def test_run_checks_null_configurations(self): """ Test the --run-checks command identifies providers with null configurations. """ @@ -471,7 +434,7 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): ) with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.current', - side_effect=SAMLConfiguration.DoesNotExist("No default config")): + return_value=None): output = self._run_checks_command() self.assertIn('[WARNING]', output) @@ -480,24 +443,10 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) - self.assertIn('Issues requiring attention:', output) - self.assertIn('Missing configs: 2', output) - self.assertIn('Total issues requiring attention: 2', output) - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 0), - mock.call('saml_management_command.null_config_count', 2), - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_null_config_id(self, mock_set_custom_attribute): + self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp + self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention + + def test_run_checks_null_config_id(self): """ Test the --run-checks command identifies providers with configurations that have null IDs. This tests the new logic that checks for default_config.id is None. @@ -523,24 +472,10 @@ def test_run_checks_null_config_id(self, mock_set_custom_attribute): self.assertIn('CHECK SUMMARY:', output) self.assertIn('Providers checked: 2', output) self.assertIn('Informational only:', output) - self.assertIn('Issues requiring attention:', output) - self.assertIn('Missing configs: 2', output) - self.assertIn('Total issues requiring attention: 2', output) - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 0), - mock.call('saml_management_command.null_config_count', 2), - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_with_default_config(self, mock_set_custom_attribute): + self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp + self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention + + def test_run_checks_with_default_config(self): """ Test the --run-checks command correctly handles providers with default configurations. """ @@ -567,22 +502,9 @@ def test_run_checks_with_default_config(self, mock_set_custom_attribute): self.assertIn('Informational only:', output) self.assertIn('Slug mismatches: 0', output) self.assertIn('Missing configs: 1', output) # 1 from setUp - self.assertIn('Total issues requiring attention: 1', output) - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 2), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 0), - mock.call('saml_management_command.null_config_count', 1), # 1 from setUp disabled config - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 1), # 1 null from setUp - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_disabled_functionality(self, mock_set_custom_attribute): + self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention + + def test_run_checks_disabled_functionality(self): """ Test the --run-checks command handles disabled providers and configurations. """ @@ -621,17 +543,7 @@ def test_run_checks_disabled_functionality(self, mock_set_custom_attribute): # Verify no Resolution: text appears self.assertNotIn('Resolution:', output) - # Check that there are 2 issues requiring attention (null configs from setUp and disabled provider) - self.assertIn('Total issues requiring attention: 2', output) - - expected_calls = [ - mock.call('saml_management_command.operation', 'run_checks'), - mock.call('saml_management_command.total_providers', 3), - mock.call('saml_management_command.outdated_count', 0), - mock.call('saml_management_command.site_mismatch_count', 0), - mock.call('saml_management_command.slug_mismatch_count', 1), - mock.call('saml_management_command.null_config_count', 2), # 1 from setUp + 1 from disabled provider - mock.call('saml_management_command.error_count', 0), - mock.call('saml_management_command.total_requiring_attention', 2), # 2 null configs - ] - mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) + # Check informational section + self.assertIn('Informational only:', output) + self.assertIn('Missing configs: 2', output) # 1 from setUp + 1 from disabled provider + self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention From 0ce4bef2ebf4983f9a66e065f925cf44e8252e44 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 7 Oct 2025 07:19:33 +0000 Subject: [PATCH 16/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/tests/test_saml.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 48774ce53c3f..414a9d693760 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -17,7 +17,6 @@ from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from common.djangoapps.third_party_auth.tests.factories import SAMLConfigurationFactory, SAMLProviderConfigFactory -from common.djangoapps.third_party_auth.models import SAMLConfiguration def mock_get(status_code=200): From d277d544a87def9597499fc8fcd089167336d0dd Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 9 Oct 2025 06:52:45 +0000 Subject: [PATCH 17/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/tests/test_saml.py | 128 +++++++----------- 1 file changed, 51 insertions(+), 77 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 414a9d693760..ffb5c36cbf47 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -345,21 +345,15 @@ def test_run_checks_outdated_configs(self): output = self._run_checks_command() - self.assertIn('[WARNING]', output) - self.assertIn('test-provider', output) - outdated_msg = ( + expected_warning = ( + f'[WARNING] Provider (id={test_provider_config.id}, name={test_provider_config.name}, ' + f'slug={test_provider_config.slug}, site_id={test_provider_config.site_id}) ' f'has outdated SAML config (id={old_config.id}) which should be updated to ' - f'the current SAML config (id={new_config.id})' + f'the current SAML config (id={new_config.id}).' ) - self.assertIn(outdated_msg, output) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) - self.assertIn('Slug mismatches: 1', output) - self.assertIn('Missing configs: 1', output) # 1 null from setUp - self.assertIn('Issues requiring attention:', output) + self.assertIn(expected_warning, output) self.assertIn('Outdated: 1', output) - self.assertIn('Total issues requiring attention: 1', output) # 1 outdated only + self.assertIn('Total issues requiring attention: 1', output) def test_run_checks_site_mismatches(self): """ @@ -379,17 +373,14 @@ def test_run_checks_site_mismatches(self): output = self._run_checks_command() - self.assertIn('[WARNING]', output) - self.assertIn('test-provider', output) - self.assertIn('does not match the provider\'s site_id', output) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) - self.assertIn('Slug mismatches: 1', output) - self.assertIn('Missing configs: 1', output) # 1 null from setUp - self.assertIn('Issues requiring attention:', output) + expected_warning = ( + f'[WARNING] Provider (id={provider.id}, name={provider.name}, ' + f'slug={provider.slug}, site_id={provider.site_id}) ' + f'SAML config (id={config.id}, site_id={config.site_id}) does not match the provider\'s site_id.' + ) + self.assertIn(expected_warning, output) self.assertIn('Site mismatches: 1', output) - self.assertIn('Total issues requiring attention: 1', output) # 1 site mismatch only + self.assertIn('Total issues requiring attention: 1', output) def test_run_checks_slug_mismatches(self): """ @@ -409,23 +400,19 @@ def test_run_checks_slug_mismatches(self): output = self._run_checks_command() - self.assertIn('[INFO]', output) - self.assertIn('provider-slug', output) - self.assertIn('has SAML config', output) - self.assertIn('slug=\'config-slug\'', output) - self.assertIn('that does not match the provider\'s slug', output) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) + expected_info = ( + f'[INFO] Provider (id={provider.id}, name={provider.name}, ' + f'slug={provider.slug}, site_id={provider.site_id}) ' + f'has SAML config (id={config.id}, slug=\'{config.slug}\') ' + f'that does not match the provider\'s slug.' + ) + self.assertIn(expected_info, output) self.assertIn('Slug mismatches: 1', output) - self.assertIn('Missing configs: 1', output) # 1 null from setUp - self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention def test_run_checks_null_configurations(self): """ Test the --run-checks command identifies providers with null configurations. """ - # Create a provider without a configuration provider = SAMLProviderConfigFactory.create( site=self.site, slug='null-provider', @@ -436,28 +423,26 @@ def test_run_checks_null_configurations(self): return_value=None): output = self._run_checks_command() - self.assertIn('[WARNING]', output) - self.assertIn('null-provider', output) - self.assertIn('has no direct SAML configuration and no matching default configuration was found', output) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) + expected_warning = ( + f'[WARNING] Provider (id={provider.id}, name={provider.name}, ' + f'slug={provider.slug}, site_id={provider.site_id}) ' + f'has no direct SAML configuration and no matching default configuration was found.' + ) + self.assertIn(expected_warning, output) self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp - self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention def test_run_checks_null_config_id(self): """ Test the --run-checks command identifies providers with configurations that have null IDs. This tests the new logic that checks for default_config.id is None. """ - # Create a provider without a configuration provider = SAMLProviderConfigFactory.create( site=self.site, slug='null-id-provider', saml_configuration=None ) - # Create a mock config object with id=None (simulates broken default config) + # Mock config with id=None (simulates broken default config) mock_config = mock.Mock() mock_config.id = None @@ -465,27 +450,24 @@ def test_run_checks_null_config_id(self): return_value=mock_config): output = self._run_checks_command() - self.assertIn('[WARNING]', output) - self.assertIn('null-id-provider', output) - self.assertIn('has no direct SAML configuration and no matching default configuration was found', output) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) + expected_warning = ( + f'[WARNING] Provider (id={provider.id}, name={provider.name}, ' + f'slug={provider.slug}, site_id={provider.site_id}) ' + f'has no direct SAML configuration and no matching default configuration was found.' + ) + self.assertIn(expected_warning, output) self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp - self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention def test_run_checks_with_default_config(self): """ Test the --run-checks command correctly handles providers with default configurations. """ - # Create a provider without a direct configuration provider = SAMLProviderConfigFactory.create( site=self.site, slug='default-config-provider', saml_configuration=None ) - # Create a default SAML configuration for the site default_config = SAMLConfigurationFactory.create( site=self.site, slug='default', @@ -494,34 +476,25 @@ def test_run_checks_with_default_config(self): output = self._run_checks_command() - self.assertNotIn('default-config-provider has no SAML configuration', output) - - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) - self.assertIn('Informational only:', output) - self.assertIn('Slug mismatches: 0', output) - self.assertIn('Missing configs: 1', output) # 1 from setUp - self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention + # Provider with valid default config doesn't get counted as missing + self.assertIn('Missing configs: 1', output) # Only 1 from setUp def test_run_checks_disabled_functionality(self): """ Test the --run-checks command handles disabled providers and configurations. """ - # Create a disabled provider disabled_provider = SAMLProviderConfigFactory.create( site=self.site, slug='disabled-provider', enabled=False ) - # Create a disabled SAML configuration disabled_config = SAMLConfigurationFactory.create( site=self.site, slug='disabled-config', enabled=False ) - # Create a provider that uses the disabled config provider_with_disabled_config = SAMLProviderConfigFactory.create( site=self.site, slug='provider-with-disabled-config', @@ -530,19 +503,20 @@ def test_run_checks_disabled_functionality(self): output = self._run_checks_command() - # Check disabled provider shows enabled=False in provider info - self.assertIn('disabled-provider', output) - self.assertIn('enabled=False', output) - - # Check disabled config detection - self.assertIn('has SAML config', output) - self.assertIn('enabled=False', output) - self.assertIn('provider-with-disabled-config', output) - - # Verify no Resolution: text appears - self.assertNotIn('Resolution:', output) - - # Check informational section - self.assertIn('Informational only:', output) + # Disabled provider shown with enabled=False + expected_disabled_provider = ( + f'Provider (id={disabled_provider.id}, name={disabled_provider.name}, ' + f'slug={disabled_provider.slug}, site_id={disabled_provider.site_id}, enabled=False)' + ) + self.assertIn(expected_disabled_provider, output) + + # Warning about disabled config + expected_warning = ( + f'[WARNING] Provider (id={provider_with_disabled_config.id}, ' + f'name={provider_with_disabled_config.name}, ' + f'slug={provider_with_disabled_config.slug}, ' + f'site_id={provider_with_disabled_config.site_id}) ' + f'has SAML config (id={disabled_config.id}, enabled=False).' + ) + self.assertIn(expected_warning, output) self.assertIn('Missing configs: 2', output) # 1 from setUp + 1 from disabled provider - self.assertNotIn('Issues requiring attention:', output) # No issues requiring attention From c68320a23e2e78e2c432c2f75be5570cda3aa4bb Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 9 Oct 2025 09:55:50 +0000 Subject: [PATCH 18/23] fix: marketing site configurations --- .../management/commands/tests/test_saml.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index ffb5c36cbf47..89329c5fa4fd 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -429,7 +429,7 @@ def test_run_checks_null_configurations(self): f'has no direct SAML configuration and no matching default configuration was found.' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp + self.assertIn('Missing configs: 2', output) def test_run_checks_null_config_id(self): """ @@ -456,7 +456,7 @@ def test_run_checks_null_config_id(self): f'has no direct SAML configuration and no matching default configuration was found.' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) # 1 from test + 1 from setUp + self.assertIn('Missing configs: 2', output) def test_run_checks_with_default_config(self): """ @@ -476,8 +476,7 @@ def test_run_checks_with_default_config(self): output = self._run_checks_command() - # Provider with valid default config doesn't get counted as missing - self.assertIn('Missing configs: 1', output) # Only 1 from setUp + self.assertIn('Missing configs: 1', output) def test_run_checks_disabled_functionality(self): """ @@ -503,14 +502,6 @@ def test_run_checks_disabled_functionality(self): output = self._run_checks_command() - # Disabled provider shown with enabled=False - expected_disabled_provider = ( - f'Provider (id={disabled_provider.id}, name={disabled_provider.name}, ' - f'slug={disabled_provider.slug}, site_id={disabled_provider.site_id}, enabled=False)' - ) - self.assertIn(expected_disabled_provider, output) - - # Warning about disabled config expected_warning = ( f'[WARNING] Provider (id={provider_with_disabled_config.id}, ' f'name={provider_with_disabled_config.name}, ' @@ -519,4 +510,4 @@ def test_run_checks_disabled_functionality(self): f'has SAML config (id={disabled_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) # 1 from setUp + 1 from disabled provider + self.assertIn('Missing configs: 2', output) From 54fc1d9cbd0d4c455597f6778c75710f022d3104 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 16 Oct 2025 07:51:25 +0000 Subject: [PATCH 19/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/saml.py | 7 +- .../management/commands/tests/test_saml.py | 75 ++++++++++++++----- 2 files changed, 61 insertions(+), 21 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 7f2f28110f82..e6a3ea02d428 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -91,6 +91,7 @@ def _check_provider_configurations(self): site_mismatch_count = 0 slug_mismatch_count = 0 null_config_count = 0 + disabled_config_count = 0 error_count = 0 total_providers = 0 @@ -130,6 +131,7 @@ def _check_provider_configurations(self): f"[WARNING] {provider_info} " f"has SAML config (id={provider_config.saml_configuration_id}, enabled=False)." ) + disabled_config_count += 1 # Check configuration currency current_config = SAMLConfiguration.current( @@ -182,6 +184,7 @@ def _check_provider_configurations(self): 'site_mismatch_count': {'count': site_mismatch_count, 'requires_attention': True}, 'slug_mismatch_count': {'count': slug_mismatch_count, 'requires_attention': False}, 'null_config_count': {'count': null_config_count, 'requires_attention': False}, + 'disabled_config_count': {'count': disabled_config_count, 'requires_attention': True}, 'error_count': {'count': error_count, 'requires_attention': True}, } @@ -198,9 +201,8 @@ def _check_no_config(self, provider_config, provider_info, null_config_count): "no matching default configuration was found." ) null_config_count += 1 - return null_config_count - if not default_config.enabled: + elif not default_config.enabled: # Resolution: Enable the provider's linked SAML configuration # or create/link a specific configuration for this provider self.stdout.write( @@ -235,6 +237,7 @@ def _report_check_summary(self, metrics): self.stdout.write("Issues requiring attention:") self.stdout.write(f" Outdated: {metrics['outdated_count']['count']}") self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']['count']}") + self.stdout.write(f" Disabled configs: {metrics['disabled_config_count']['count']}") self.stdout.write(f" Errors: {metrics['error_count']['count']}") self.stdout.write("") self.stdout.write(f"Total issues requiring attention: {total_requiring_attention}") diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 89329c5fa4fd..623d33d27d45 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -79,6 +79,7 @@ def setUp(self): 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, ) def _setup_test_configs_for_run_checks(self): @@ -337,6 +338,29 @@ def _run_checks_command(self): call_command('saml', '--run-checks', stdout=out) return out.getvalue() + def test_run_checks_setup_test_data(self): + """ + Test the --run-checks command against initial setup test data. + + 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). + """ + output = self._run_checks_command() + + # The setup data includes a provider with a disabled SAML 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).' + ) + self.assertIn(expected_warning, output) + self.assertIn('Missing configs: 0', output) # No missing configs from setUp + self.assertIn('Disabled configs: 1', output) # From setUp: provider_config with disabled saml_config + def test_run_checks_outdated_configs(self): """ Test the --run-checks command identifies outdated configurations. @@ -353,7 +377,8 @@ def test_run_checks_outdated_configs(self): ) self.assertIn(expected_warning, output) self.assertIn('Outdated: 1', output) - self.assertIn('Total issues requiring attention: 1', output) + # Total includes: 1 outdated + 2 disabled configs (setUp + test's old_config which is also disabled) + self.assertIn('Total issues requiring attention: 3', output) def test_run_checks_site_mismatches(self): """ @@ -380,7 +405,8 @@ def test_run_checks_site_mismatches(self): ) self.assertIn(expected_warning, output) self.assertIn('Site mismatches: 1', output) - self.assertIn('Total issues requiring attention: 1', output) + # Total includes: 1 site mismatch + 1 disabled config (from setUp) + self.assertIn('Total issues requiring attention: 2', output) def test_run_checks_slug_mismatches(self): """ @@ -412,16 +438,17 @@ def test_run_checks_slug_mismatches(self): def test_run_checks_null_configurations(self): """ Test the --run-checks command identifies providers with null configurations. + This test verifies that providers with no direct SAML configuration and no + default configuration available are properly reported. """ + # Create a provider with no SAML configuration on a site that has no default config provider = SAMLProviderConfigFactory.create( site=self.site, slug='null-provider', saml_configuration=None ) - with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.current', - return_value=None): - output = self._run_checks_command() + output = self._run_checks_command() expected_warning = ( f'[WARNING] Provider (id={provider.id}, name={provider.name}, ' @@ -429,34 +456,42 @@ def test_run_checks_null_configurations(self): f'has no direct SAML configuration and no matching default configuration was found.' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) + self.assertIn('Missing configs: 1', output) # This test's provider with no config + self.assertIn('Disabled configs: 1', output) # From setUp def test_run_checks_null_config_id(self): """ - Test the --run-checks command identifies providers with configurations that have null IDs. - This tests the new logic that checks for default_config.id is None. + Test the --run-checks command identifies providers with disabled default configurations. + When a provider has no direct SAML configuration and the default config is disabled, + it should be reported as a missing config issue. """ + # Create a disabled default configuration for this site + disabled_default_config = SAMLConfigurationFactory.create( + site=self.site, + slug='default', + entity_id='https://default.example.com', + enabled=False + ) + + # Create a provider with no direct SAML configuration + # It will fall back to the disabled default config provider = SAMLProviderConfigFactory.create( site=self.site, slug='null-id-provider', saml_configuration=None ) - # Mock config with id=None (simulates broken default config) - mock_config = mock.Mock() - mock_config.id = None - - with mock.patch('common.djangoapps.third_party_auth.models.SAMLConfiguration.current', - return_value=mock_config): - output = self._run_checks_command() + output = self._run_checks_command() expected_warning = ( f'[WARNING] Provider (id={provider.id}, name={provider.name}, ' f'slug={provider.slug}, site_id={provider.site_id}) ' - f'has no direct SAML configuration and no matching default configuration was found.' + f'has no direct SAML configuration and the default configuration ' + f'(id={disabled_default_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) + self.assertIn('Missing configs: 1', output) # This test's provider with disabled default config + self.assertIn('Disabled configs: 1', output) # From setUp def test_run_checks_with_default_config(self): """ @@ -476,7 +511,8 @@ def test_run_checks_with_default_config(self): output = self._run_checks_command() - self.assertIn('Missing configs: 1', output) + self.assertIn('Missing configs: 0', output) # This test's provider has valid default config + self.assertIn('Disabled configs: 1', output) # From setUp def test_run_checks_disabled_functionality(self): """ @@ -510,4 +546,5 @@ def test_run_checks_disabled_functionality(self): f'has SAML config (id={disabled_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 2', output) + self.assertIn('Missing configs: 1', output) # disabled_provider has no config + self.assertIn('Disabled configs: 2', output) # setUp's provider + provider_with_disabled_config From 8b76ef537ab395a5145dccbf24a9c7cf266d2727 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 22 Oct 2025 16:11:12 +0000 Subject: [PATCH 20/23] fix: Improve SAML configuration checks and update warning messages --- .../management/commands/tests/test_saml.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 623d33d27d45..f7dd1e69a0e6 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -456,7 +456,7 @@ def test_run_checks_null_configurations(self): f'has no direct SAML configuration and no matching default configuration was found.' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 1', output) # This test's provider with no config + self.assertIn('Missing configs: 1', output) # This tests provider with no config self.assertIn('Disabled configs: 1', output) # From setUp def test_run_checks_null_config_id(self): @@ -490,8 +490,8 @@ def test_run_checks_null_config_id(self): f'(id={disabled_default_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 1', output) # This test's provider with disabled default config - self.assertIn('Disabled configs: 1', output) # From setUp + self.assertIn('Missing configs: 0', output) # No missing configs since default config exists + self.assertIn('Disabled configs: 2', output) # From setUp + this tests provider with disabled default config def test_run_checks_with_default_config(self): """ @@ -511,7 +511,7 @@ def test_run_checks_with_default_config(self): output = self._run_checks_command() - self.assertIn('Missing configs: 0', output) # This test's provider has valid default config + self.assertIn('Missing configs: 0', output) # This tests provider has valid default config self.assertIn('Disabled configs: 1', output) # From setUp def test_run_checks_disabled_functionality(self): From 36c8bf5cc76af8a3118931dcaf95613e20ccf4e8 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 22 Oct 2025 16:33:09 +0000 Subject: [PATCH 21/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/tests/test_saml.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index f7dd1e69a0e6..9fb4facefa4a 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -490,8 +490,8 @@ def test_run_checks_null_config_id(self): f'(id={disabled_default_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 0', output) # No missing configs since default config exists - self.assertIn('Disabled configs: 2', output) # From setUp + this tests provider with disabled default config + self.assertIn('Missing configs: 1', output) # 1 from this test (provider with disabled default config) + self.assertIn('Disabled configs: 1', output) # 1 from setUp data def test_run_checks_with_default_config(self): """ From 510accdf8cf199fd05fbdae468fa055da36a391c Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 22 Oct 2025 16:45:56 +0000 Subject: [PATCH 22/23] fix: Improve SAML configuration checks and update warning messages --- .../third_party_auth/management/commands/saml.py | 12 ++++++------ .../management/commands/tests/test_saml.py | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index e6a3ea02d428..570ce6b55fb7 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -118,8 +118,8 @@ def _check_provider_configurations(self): try: if not provider_config.saml_configuration: - null_config_count = self._check_no_config( - provider_config, provider_info, null_config_count + null_config_count, disabled_config_count = self._check_no_config( + provider_config, provider_info, null_config_count, disabled_config_count ) continue @@ -190,8 +190,8 @@ def _check_provider_configurations(self): return metrics - def _check_no_config(self, provider_config, provider_info, null_config_count): - """Helper to check providers with no direct SAML configuration.""" + def _check_no_config(self, provider_config, provider_info, null_config_count, disabled_config_count): + """Helper to check providers with no direct SAML configuration.""" default_config = SAMLConfiguration.current(provider_config.site_id, 'default') if not default_config or default_config.id is None: # Resolution: Create/Link a SAML configuration for this provider @@ -209,9 +209,9 @@ def _check_no_config(self, provider_config, provider_info, null_config_count): f"[WARNING] {provider_info} has no direct SAML configuration and " f"the default configuration (id={default_config.id}, enabled=False)." ) - null_config_count += 1 + disabled_config_count += 1 - return null_config_count + return null_config_count, disabled_config_count def _report_check_summary(self, metrics): """ diff --git a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py index 9fb4facefa4a..d80c9146664b 100644 --- a/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py +++ b/common/djangoapps/third_party_auth/management/commands/tests/test_saml.py @@ -456,8 +456,8 @@ def test_run_checks_null_configurations(self): f'has no direct SAML configuration and no matching default configuration was found.' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 1', output) # This tests provider with no config - self.assertIn('Disabled configs: 1', output) # From setUp + self.assertIn('Missing configs: 1', output) # 1 from this test (provider with no config and no default) + self.assertIn('Disabled configs: 1', output) # 1 from setUp data def test_run_checks_null_config_id(self): """ @@ -490,8 +490,8 @@ def test_run_checks_null_config_id(self): f'(id={disabled_default_config.id}, enabled=False).' ) self.assertIn(expected_warning, output) - self.assertIn('Missing configs: 1', output) # 1 from this test (provider with disabled default config) - self.assertIn('Disabled configs: 1', output) # 1 from setUp data + self.assertIn('Missing configs: 0', output) # No missing configs since default config exists + self.assertIn('Disabled configs: 2', output) # 1 from this test + 1 from setUp data def test_run_checks_with_default_config(self): """ From bab30b3d2158e9ff7a9f9ba57ab46553dd201144 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 22 Oct 2025 16:51:10 +0000 Subject: [PATCH 23/23] fix: Improve SAML configuration checks and update warning messages --- common/djangoapps/third_party_auth/management/commands/saml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 570ce6b55fb7..6865ebf69987 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -191,7 +191,7 @@ def _check_provider_configurations(self): return metrics def _check_no_config(self, provider_config, provider_info, null_config_count, disabled_config_count): - """Helper to check providers with no direct SAML configuration.""" + """Helper to check providers with no direct SAML configuration.""" default_config = SAMLConfiguration.current(provider_config.site_id, 'default') if not default_config or default_config.id is None: # Resolution: Create/Link a SAML configuration for this provider