From 9dbac97cc7004be1d3b35ce6263c4744291826ba Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Sun, 7 Sep 2025 06:38:51 +0000 Subject: [PATCH 01/16] feat: update saml management command --- .../management/commands/saml.py | 156 +++++++++++----- .../management/commands/tests/test_saml.py | 173 ++++++++++++------ .../signals/tests/test_handlers.py | 9 +- 3 files changed, 235 insertions(+), 103 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index e3891cefa08f..83ea6f61bd0b 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -2,10 +2,10 @@ Management commands for third_party_auth """ - 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 @@ -18,34 +18,28 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument('--pull', action='store_true', help="Pull updated metadata from external IDPs") parser.add_argument( - '--fix-references', + '--run-checks', action='store_true', - help="Fix SAMLProviderConfig references to use current SAMLConfiguration versions" + help="Run checks on SAMLProviderConfig configurations and report potential issues" ) parser.add_argument( '--site-id', type=int, - help='Only fix configurations for a specific site ID (to be used with --fix-references)' - ) - parser.add_argument( - '--dry-run', - action='store_true', - help='Show what would be changed, but do not make any changes.' + help='Only check configurations for a specific site ID (to be used with --run-checks)' ) def handle(self, *args, **options): should_pull_saml_metadata = options.get('pull', False) - should_fix_references = options.get('fix_references', False) - dry_run = options.get('dry_run', False) + should_run_checks = options.get('run_checks', False) - if not should_pull_saml_metadata and not should_fix_references: - raise CommandError("Command must be used with '--pull' or '--fix-references' option.") + if not should_pull_saml_metadata and not should_run_checks: + raise CommandError("Command must be used with '--pull' or '--run-checks' option.") if should_pull_saml_metadata: self._handle_pull_metadata() - if should_fix_references: - self._handle_fix_references(options, dry_run=dry_run) + if should_run_checks: + self._handle_run_checks(options) def _handle_pull_metadata(self): """ @@ -76,45 +70,117 @@ def _handle_pull_metadata(self): ) ) - def _handle_fix_references(self, options, dry_run=False): - """Handle the --fix-references option for fixing outdated SAML configuration references.""" + def _handle_run_checks(self, options): + """ + Handle the --run-checks option for checking SAML configuration issues. + + This is a report-only command that identifies potential configuration problems: + - Outdated configuration references + - Site ID mismatches between provider and configuration + - Slug mismatches (except when slug is 'default' which may be intentional) + - Providers with null configurations (informational) + + Includes observability attributes for monitoring. + """ site_id = options.get('site_id') - updated_count = 0 + + # .. custom_attribute_name: saml_management_command.operation + # .. custom_attribute_description: Records the operation being performed by the management command. + set_custom_attribute('saml_management_command.operation', 'run_checks') + + # .. custom_attribute_name: saml_management_command.site_filter + # .. custom_attribute_description: Records the site filter applied, either specific site ID or 'all'. + set_custom_attribute('saml_management_command.site_filter', str(site_id) if site_id else 'all') + + outdated_count = 0 + site_mismatch_count = 0 + slug_mismatch_count = 0 + null_config_count = 0 error_count = 0 + total_providers = 0 - # Filter by site if specified provider_configs = SAMLProviderConfig.objects.current_set() if site_id: provider_configs = provider_configs.filter(site_id=site_id) - for provider_config in provider_configs: - if provider_config.saml_configuration: - try: - current_config = SAMLConfiguration.current( - provider_config.site_id, - provider_config.saml_configuration.slug - ) - - if current_config and current_config.id != provider_config.saml_configuration_id: - self.stdout.write( - f"Provider '{provider_config.slug}' (site {provider_config.site_id}) " - f"has outdated config (ID: {provider_config.saml_configuration_id} -> {current_config.id})" - ) + self.stdout.write(self.style.SUCCESS("SAML Configuration Check Report")) + self.stdout.write("=" * 50) - if not dry_run: - provider_config.saml_configuration = current_config - provider_config.save() - updated_count += 1 + for provider_config in provider_configs: + total_providers += 1 + provider_info = f"Provider '{provider_config.slug}' (site {provider_config.site_id})" + + if not provider_config.saml_configuration: + self.stdout.write(f"[INFO] {provider_info} has no SAML configuration (may be intentional)") + null_config_count += 1 + continue + + try: + current_config = SAMLConfiguration.current( + provider_config.site_id, + provider_config.saml_configuration.slug + ) - except Exception as e: # pylint: disable=broad-except - self.stderr.write( - f"Error processing provider '{provider_config.slug}': {e}" + # Check for outdated configuration references + if current_config and current_config.id != provider_config.saml_configuration_id: + self.stdout.write( + f"[OUTDATED] {provider_info} " + f"has outdated config (ID: {provider_config.saml_configuration_id} -> {current_config.id})" ) - error_count += 1 + outdated_count += 1 - style = self.style.SUCCESS - if dry_run: - msg = f"[DRY RUN] Would update {updated_count} provider configurations. {error_count} errors encountered." + if provider_config.saml_configuration.site_id != provider_config.site_id: + self.stdout.write( + f"[SITE_MISMATCH] {provider_info} " + f"config site ({provider_config.saml_configuration.site_id}) != provider site ({provider_config.site_id})" + ) + site_mismatch_count += 1 + + actual_slug = provider_config.saml_configuration.slug + expected_slug = provider_config.slug + if (actual_slug != expected_slug and + actual_slug != 'default' and + expected_slug != 'default'): + self.stdout.write( + f"[SLUG_MISMATCH] {provider_info} " + f"config slug ('{actual_slug}') != provider slug ('{expected_slug}')" + ) + slug_mismatch_count += 1 + + except Exception as e: # pylint: disable=broad-except + self.stderr.write(f"[ERROR] Error processing {provider_info}: {e}") + error_count += 1 + + metrics = { + 'total_providers': total_providers, + 'outdated_count': outdated_count, + 'site_mismatch_count': site_mismatch_count, + 'slug_mismatch_count': slug_mismatch_count, + 'null_config_count': null_config_count, + 'error_count': error_count, + } + + for key, value 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}', value) + + total_issues = outdated_count + site_mismatch_count + slug_mismatch_count + + # .. custom_attribute_name: saml_management_command.total_issues + # .. custom_attribute_description: The total number of configuration issues requiring attention. + set_custom_attribute('saml_management_command.total_issues', total_issues) + + self.stdout.write("\n" + "=" * 50) + self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) + self.stdout.write(f" Providers: {total_providers}") + self.stdout.write(f" Outdated: {outdated_count}") + self.stdout.write(f" Site mismatches: {site_mismatch_count}") + self.stdout.write(f" Slug mismatches: {slug_mismatch_count}") + self.stdout.write(f" Null configs: {null_config_count}") + self.stdout.write(f" Errors: {error_count}") + + if total_issues > 0: + self.stdout.write(f"\nTotal issues requiring attention: {total_issues}") else: - msg = f"Updated {updated_count} provider configurations. {error_count} errors encountered." - self.stdout.write(style(msg)) + self.stdout.write(self.style.SUCCESS("\nNo 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 168d88ae3b21..e5930fc3fbac 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 @@ -8,7 +8,7 @@ from io import StringIO from unittest import mock -from ddt import ddt, data, unpack +from ddt import ddt from django.contrib.sites.models import Site from django.core.management import call_command from django.core.management.base import CommandError @@ -82,9 +82,9 @@ def setUp(self): metadata_source='https://www.testshib.org/metadata/testshib-providers.xml', ) - def _setup_test_configs_for_fix_references(self): + def _setup_test_configs_for_run_checks(self): """ - Helper method to create SAML configurations for fix-references tests. + Helper method to create SAML configurations for run-checks tests. Returns tuple of (old_config, new_config, provider_config) @@ -108,7 +108,7 @@ def _setup_test_configs_for_fix_references(self): entity_id='https://updated.example.com' ) - # Create a provider config that references the old config for fix-references tests + # Create a provider config that references the old config for run-checks tests test_provider_config = SAMLProviderConfigFactory.create( site=self.site, slug='test-provider', @@ -149,11 +149,11 @@ def test_raises_command_error_for_invalid_arguments(self): This test would fail with an error if ValueError is raised. """ # Call `saml` command without any argument so that it raises a CommandError - with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--fix-references' option."): + with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--run-checks' option."): call_command("saml") # Call `saml` command without any argument so that it raises a CommandError - with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--fix-references' option."): + with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--run-checks' option."): call_command("saml", pull=False) def test_no_saml_configuration(self): @@ -334,59 +334,122 @@ def test_xml_parse_exceptions(self, mocked_get): call_command("saml", pull=True, stdout=self.stdout) assert expected in self.stdout.getvalue() - @data( - (True, '[DRY RUN]', 'should not update provider configs'), - (False, '', 'should create new provider config for new version') - ) - @unpack - def test_fix_references(self, dry_run, expected_output_marker, test_description): + def _run_checks_command(self, site_id=None): + """Helper method to run the --run-checks command and return output.""" + out = StringIO() + args = ['saml', '--run-checks'] + if site_id: + args.extend(['--site-id', str(site_id)]) + call_command(*args, stdout=out) + return out.getvalue() + + def _assert_observability_calls(self, mock_set_custom_attribute, expected_calls): + """Helper method to assert multiple observability calls.""" + for call_args in expected_calls: + mock_set_custom_attribute.assert_any_call(*call_args) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_outdated_configs(self, mock_set_custom_attribute): + """ + Test the --run-checks command identifies outdated configurations. """ - Test the --fix-references command with and without --dry-run option. + old_config, new_config, test_provider_config = self._setup_test_configs_for_run_checks() + + output = self._run_checks_command() - Args: - dry_run (bool): Whether to run with --dry-run flag - expected_output_marker (str): Expected marker in output - test_description (str): Description of what the test should do + self.assertIn('[OUTDATED]', output) + self.assertIn('test-provider', output) + self.assertIn(f'{old_config.id} -> {new_config.id}', output) + self.assertIn('CHECK SUMMARY:', output) + self.assertIn('Providers: 2', output) + self.assertIn('Outdated: 1', output) + + expected_calls = [ + ('saml_management_command.operation', 'run_checks'), + ('saml_management_command.outdated_count', 1), + ('saml_management_command.total_issues', 2) + ] + self._assert_observability_calls(mock_set_custom_attribute, expected_calls) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_site_mismatches(self, mock_set_custom_attribute): + """ + Test the --run-checks command identifies site ID mismatches. """ - old_config, new_config, test_provider_config = self._setup_test_configs_for_fix_references() - new_config_id = new_config.id - original_config_id = old_config.id + other_site = Site.objects.create(domain='other.example.com', name='Other Site') - out = StringIO() - if dry_run: - call_command('saml', '--fix-references', '--dry-run', stdout=out) - else: - call_command('saml', '--fix-references', stdout=out) + config = SAMLConfigurationFactory.create( + site=other_site, + slug='test-config', + entity_id='https://example.com' + ) + + SAMLProviderConfigFactory.create( + site=self.site, + slug='test-provider', + saml_configuration=config + ) - output = out.getvalue() + output = self._run_checks_command() + self.assertIn('[SITE_MISMATCH]', output) self.assertIn('test-provider', output) - if expected_output_marker: - self.assertIn(expected_output_marker, output) - - test_provider_config.refresh_from_db() - - if dry_run: - # For dry run, ensure the provider config was NOT updated - self.assertEqual( - test_provider_config.saml_configuration_id, - original_config_id, - "Provider config should not be updated in dry run mode" - ) - else: - # For actual run, check that a new provider config was created - new_provider = SAMLProviderConfig.objects.filter( - site=self.site, - slug='test-provider', - saml_configuration_id=new_config_id - ).exclude(id=test_provider_config.id).first() - - self.assertIsNotNone(new_provider, "New provider config should be created") - self.assertEqual(new_provider.saml_configuration_id, new_config_id) - - # Original provider config should still reference the old config - self.assertEqual( - test_provider_config.saml_configuration_id, - original_config_id, - "Original provider config should still reference old config" - ) + mock_set_custom_attribute.assert_any_call('saml_management_command.site_mismatch_count', 1) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): + """ + Test the --run-checks command identifies slug mismatches. + """ + config = SAMLConfigurationFactory.create( + site=self.site, + slug='config-slug', + entity_id='https://example.com' + ) + + SAMLProviderConfigFactory.create( + site=self.site, + slug='provider-slug', + saml_configuration=config + ) + + output = self._run_checks_command() + + self.assertIn('[SLUG_MISMATCH]', output) + self.assertIn('provider-slug', output) + mock_set_custom_attribute.assert_any_call('saml_management_command.slug_mismatch_count', 1) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_null_configurations(self, mock_set_custom_attribute): + """ + Test the --run-checks command identifies providers with null configurations. + """ + SAMLProviderConfigFactory.create( + site=self.site, + slug='null-provider', + saml_configuration=None + ) + + output = self._run_checks_command() + + self.assertIn('[INFO]', output) + self.assertIn('null-provider', output) + self.assertIn('has no SAML configuration', output) + mock_set_custom_attribute.assert_any_call('saml_management_command.null_config_count', 2) + + @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') + def test_run_checks_with_site_filter(self, mock_set_custom_attribute): + """ + Test the --run-checks command with --site-id filter. + """ + other_site = Site.objects.create(domain='other.example.com', name='Other Site') + + SAMLProviderConfigFactory.create(site=self.site, slug='site1-provider', saml_configuration=None) + SAMLProviderConfigFactory.create(site=other_site, slug='site2-provider', saml_configuration=None) + + output = self._run_checks_command(site_id=self.site.id) + + self.assertIn('site1-provider', output) + self.assertNotIn('site2-provider', output) + self.assertIn('Providers: 1', output) + mock_set_custom_attribute.assert_any_call('saml_management_command.site_filter', str(self.site.id)) diff --git a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py index 7875f0fcfa57..1dd2fd6d7d01 100644 --- a/common/djangoapps/third_party_auth/signals/tests/test_handlers.py +++ b/common/djangoapps/third_party_auth/signals/tests/test_handlers.py @@ -153,9 +153,12 @@ def test_saml_provider_config_updates(self, provider_site_id, provider_slug, current_provider = self._get_current_provider(provider_slug) - mock_set_custom_attribute.assert_any_call('saml_config_signal.enabled', True) - mock_set_custom_attribute.assert_any_call('saml_config_signal.new_config_id', new_saml_config.id) - mock_set_custom_attribute.assert_any_call('saml_config_signal.slug', signal_saml_slug) + expected_calls = [ + call('saml_config_signal.enabled', True), + call('saml_config_signal.new_config_id', new_saml_config.id), + call('saml_config_signal.slug', signal_saml_slug), + ] + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) if is_provider_updated: mock_set_custom_attribute.assert_any_call('saml_config_signal.updated_count', 1) From 86ed3586b6ad0faa6bd684b108b21a9b9c6c7c75 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Sun, 7 Sep 2025 13:24:04 +0000 Subject: [PATCH 02/16] feat: update saml management command --- .../management/commands/saml.py | 37 ++++++++++++++----- .../management/commands/saml.py.new | 0 .../management/commands/tests/test_saml.py | 2 - 3 files changed, 28 insertions(+), 11 deletions(-) create mode 100644 common/djangoapps/third_party_auth/management/commands/saml.py.new diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 83ea6f61bd0b..2cde752e7a9b 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -83,7 +83,15 @@ def _handle_run_checks(self, options): Includes observability attributes for monitoring. """ site_id = options.get('site_id') + self._set_check_attributes(site_id) + metrics = self._check_provider_configurations(site_id) + self._report_check_summary(metrics) + + def _set_check_attributes(self, site_id): + """ + Set custom attributes for monitoring the check operation. + """ # .. custom_attribute_name: saml_management_command.operation # .. custom_attribute_description: Records the operation being performed by the management command. set_custom_attribute('saml_management_command.operation', 'run_checks') @@ -92,6 +100,11 @@ def _handle_run_checks(self, options): # .. custom_attribute_description: Records the site filter applied, either specific site ID or 'all'. set_custom_attribute('saml_management_command.site_filter', str(site_id) if site_id else 'all') + def _check_provider_configurations(self, site_id): + """ + Check each provider configuration for potential issues. + Returns a dictionary of metrics about the found issues. + """ outdated_count = 0 site_mismatch_count = 0 slug_mismatch_count = 0 @@ -138,9 +151,9 @@ def _handle_run_checks(self, options): actual_slug = provider_config.saml_configuration.slug expected_slug = provider_config.slug + if (actual_slug != expected_slug and - actual_slug != 'default' and - expected_slug != 'default'): + not (actual_slug == 'default' or expected_slug == 'default')): self.stdout.write( f"[SLUG_MISMATCH] {provider_info} " f"config slug ('{actual_slug}') != provider slug ('{expected_slug}')" @@ -165,7 +178,13 @@ def _handle_run_checks(self, options): # .. custom_attribute_description: Records metrics from SAML configuration checks. set_custom_attribute(f'saml_management_command.{key}', value) - total_issues = outdated_count + site_mismatch_count + slug_mismatch_count + return metrics + + def _report_check_summary(self, metrics): + """ + Print a summary of the check results and set the total_issues custom attribute. + """ + total_issues = metrics['outdated_count'] + metrics['site_mismatch_count'] + metrics['slug_mismatch_count'] # .. custom_attribute_name: saml_management_command.total_issues # .. custom_attribute_description: The total number of configuration issues requiring attention. @@ -173,12 +192,12 @@ def _handle_run_checks(self, options): self.stdout.write("\n" + "=" * 50) self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) - self.stdout.write(f" Providers: {total_providers}") - self.stdout.write(f" Outdated: {outdated_count}") - self.stdout.write(f" Site mismatches: {site_mismatch_count}") - self.stdout.write(f" Slug mismatches: {slug_mismatch_count}") - self.stdout.write(f" Null configs: {null_config_count}") - self.stdout.write(f" Errors: {error_count}") + self.stdout.write(f" Providers: {metrics['total_providers']}") + self.stdout.write(f" Outdated: {metrics['outdated_count']}") + self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']}") + self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']}") + self.stdout.write(f" Null configs: {metrics['null_config_count']}") + self.stdout.write(f" Errors: {metrics['error_count']}") if total_issues > 0: self.stdout.write(f"\nTotal issues requiring attention: {total_issues}") diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py.new b/common/djangoapps/third_party_auth/management/commands/saml.py.new new file mode 100644 index 000000000000..e69de29bb2d1 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 e5930fc3fbac..a6c789791db6 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 @@ -18,8 +18,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 SAMLProviderConfig - def mock_get(status_code=200): """ From a18e10421e63af7be71cf3b0a35baa25038e4cdd Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 8 Sep 2025 03:58:22 +0000 Subject: [PATCH 03/16] feat: update saml management command --- .../djangoapps/third_party_auth/management/commands/saml.py | 4 +++- .../third_party_auth/management/commands/saml.py.new | 0 2 files changed, 3 insertions(+), 1 deletion(-) delete mode 100644 common/djangoapps/third_party_auth/management/commands/saml.py.new diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 2cde752e7a9b..07ae53f5e0dc 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -143,9 +143,11 @@ def _check_provider_configurations(self, site_id): outdated_count += 1 if provider_config.saml_configuration.site_id != provider_config.site_id: + config_site = provider_config.saml_configuration.site_id + provider_site = provider_config.site_id self.stdout.write( f"[SITE_MISMATCH] {provider_info} " - f"config site ({provider_config.saml_configuration.site_id}) != provider site ({provider_config.site_id})" + f"config site ({config_site}) != provider site ({provider_site})" ) site_mismatch_count += 1 diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py.new b/common/djangoapps/third_party_auth/management/commands/saml.py.new deleted file mode 100644 index e69de29bb2d1..000000000000 From 036c783f74e3d830cd1852482886880f54c4bb2f Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 8 Sep 2025 05:28:27 +0000 Subject: [PATCH 04/16] feat: update saml management command --- .../management/commands/saml.py | 13 +++++++------ .../management/commands/tests/test_saml.py | 17 +++++++++-------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 07ae53f5e0dc..f8ec4b465ff8 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -2,6 +2,7 @@ Management commands for third_party_auth """ + import logging from django.core.management.base import BaseCommand, CommandError @@ -32,14 +33,15 @@ def handle(self, *args, **options): should_pull_saml_metadata = options.get('pull', False) should_run_checks = options.get('run_checks', False) - if not should_pull_saml_metadata and not should_run_checks: - raise CommandError("Command must be used with '--pull' or '--run-checks' option.") - if should_pull_saml_metadata: self._handle_pull_metadata() + return if should_run_checks: self._handle_run_checks(options) + return + + raise CommandError("Command must be used with '--pull' or '--run-checks' option.") def _handle_pull_metadata(self): """ @@ -93,7 +95,7 @@ def _set_check_attributes(self, site_id): Set custom attributes for monitoring the check operation. """ # .. custom_attribute_name: saml_management_command.operation - # .. custom_attribute_description: Records the operation being performed by the management command. + # .. custom_attribute_description: Records current SAML operation ('run_checks'). set_custom_attribute('saml_management_command.operation', 'run_checks') # .. custom_attribute_name: saml_management_command.site_filter @@ -130,7 +132,7 @@ def _check_provider_configurations(self, site_id): try: current_config = SAMLConfiguration.current( - provider_config.site_id, + provider_config.saml_configuration.site_id, provider_config.saml_configuration.slug ) @@ -192,7 +194,6 @@ def _report_check_summary(self, metrics): # .. custom_attribute_description: The total number of configuration issues requiring attention. set_custom_attribute('saml_management_command.total_issues', total_issues) - self.stdout.write("\n" + "=" * 50) self.stdout.write(self.style.SUCCESS("CHECK SUMMARY:")) self.stdout.write(f" Providers: {metrics['total_providers']}") self.stdout.write(f" Outdated: {metrics['outdated_count']}") 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 a6c789791db6..2d272589e656 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 @@ -62,6 +62,7 @@ def setUp(self): self.stdout = StringIO() self.site = Site.objects.get_current() + self.other_site = Site.objects.create(domain='other.example.com', name='Other Site') # We are creating SAMLConfiguration instance here so that there is always at-least one # disabled saml configuration instance, this is done to verify that disabled configurations are @@ -333,7 +334,9 @@ def test_xml_parse_exceptions(self, mocked_get): assert expected in self.stdout.getvalue() def _run_checks_command(self, site_id=None): - """Helper method to run the --run-checks command and return output.""" + """ + Helper method to run the --run-checks command and return output. + """ out = StringIO() args = ['saml', '--run-checks'] if site_id: @@ -342,7 +345,9 @@ def _run_checks_command(self, site_id=None): return out.getvalue() def _assert_observability_calls(self, mock_set_custom_attribute, expected_calls): - """Helper method to assert multiple observability calls.""" + """ + Helper method to assert multiple observability calls. + """ for call_args in expected_calls: mock_set_custom_attribute.assert_any_call(*call_args) @@ -374,10 +379,8 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): """ Test the --run-checks command identifies site ID mismatches. """ - other_site = Site.objects.create(domain='other.example.com', name='Other Site') - config = SAMLConfigurationFactory.create( - site=other_site, + site=self.other_site, slug='test-config', entity_id='https://example.com' ) @@ -440,10 +443,8 @@ def test_run_checks_with_site_filter(self, mock_set_custom_attribute): """ Test the --run-checks command with --site-id filter. """ - other_site = Site.objects.create(domain='other.example.com', name='Other Site') - SAMLProviderConfigFactory.create(site=self.site, slug='site1-provider', saml_configuration=None) - SAMLProviderConfigFactory.create(site=other_site, slug='site2-provider', saml_configuration=None) + SAMLProviderConfigFactory.create(site=self.other_site, slug='site2-provider', saml_configuration=None) output = self._run_checks_command(site_id=self.site.id) From dc9a9875432745852cc1afdf2a76c6e10cf9c556 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 8 Sep 2025 09:08:39 +0000 Subject: [PATCH 05/16] feat: update saml management command --- .../third_party_auth/management/commands/tests/test_saml.py | 6 +----- 1 file changed, 1 insertion(+), 5 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 2d272589e656..a888c790eeef 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 @@ -147,14 +147,10 @@ def test_raises_command_error_for_invalid_arguments(self): This test would fail with an error if ValueError is raised. """ - # Call `saml` command without any argument so that it raises a CommandError + # Call `saml` command without any arguments so that it raises a CommandError with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--run-checks' option."): call_command("saml") - # Call `saml` command without any argument so that it raises a CommandError - with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--run-checks' option."): - call_command("saml", pull=False) - def test_no_saml_configuration(self): """ Test that management command completes without errors and logs correct information when no From a68d3e6434c41cc7da30db5a096143c9064391e0 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 9 Sep 2025 11:37:13 +0000 Subject: [PATCH 06/16] feat: update saml management command --- .../management/commands/saml.py | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index f8ec4b465ff8..52fee6ae5987 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -74,26 +74,19 @@ def _handle_pull_metadata(self): def _handle_run_checks(self, options): """ - Handle the --run-checks option for checking SAML configuration issues. + Handle the --run-checks option for checking SAMLProviderConfig configuration issues. - This is a report-only command that identifies potential configuration problems: - - Outdated configuration references - - Site ID mismatches between provider and configuration - - Slug mismatches (except when slug is 'default' which may be intentional) - - Providers with null configurations (informational) + 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 between SAMLProviderConfig and its SAMLConfiguration (except when slug is 'default' which may be intentional) + - SAMLProviderConfig objects with null SAMLConfiguration references (informational) Includes observability attributes for monitoring. """ site_id = options.get('site_id') - self._set_check_attributes(site_id) - - metrics = self._check_provider_configurations(site_id) - self._report_check_summary(metrics) - - def _set_check_attributes(self, site_id): - """ - Set custom attributes for monitoring the check operation. - """ + + # 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') @@ -102,6 +95,9 @@ def _set_check_attributes(self, site_id): # .. custom_attribute_description: Records the site filter applied, either specific site ID or 'all'. set_custom_attribute('saml_management_command.site_filter', str(site_id) if site_id else 'all') + metrics = self._check_provider_configurations(site_id) + self._report_check_summary(metrics) + def _check_provider_configurations(self, site_id): """ Check each provider configuration for potential issues. @@ -123,7 +119,7 @@ def _check_provider_configurations(self, site_id): for provider_config in provider_configs: total_providers += 1 - provider_info = f"Provider '{provider_config.slug}' (site {provider_config.site_id})" + provider_info = f"Provider '{provider_config.slug}' (ID: {provider_config.id}, site {provider_config.site_id})" if not provider_config.saml_configuration: self.stdout.write(f"[INFO] {provider_info} has no SAML configuration (may be intentional)") @@ -137,10 +133,18 @@ def _check_provider_configurations(self, site_id): ) # Check for outdated configuration references - if current_config and current_config.id != provider_config.saml_configuration_id: + if current_config: + if current_config.id != provider_config.saml_configuration_id: + self.stdout.write( + f"[OUTDATED] {provider_info} " + f"has outdated config (ID: {provider_config.saml_configuration_id} -> {current_config.id})" + ) + outdated_count += 1 + else: + # No current config found - this might indicate the referenced config is no longer valid self.stdout.write( - f"[OUTDATED] {provider_info} " - f"has outdated config (ID: {provider_config.saml_configuration_id} -> {current_config.id})" + f"[WARNING] {provider_info} " + f"references config (ID: {provider_config.saml_configuration_id}) but no current config found for site {provider_config.saml_configuration.site_id}, slug '{provider_config.saml_configuration.slug}'" ) outdated_count += 1 @@ -153,14 +157,14 @@ def _check_provider_configurations(self, site_id): ) site_mismatch_count += 1 - actual_slug = provider_config.saml_configuration.slug - expected_slug = provider_config.slug + saml_configuration_slug = provider_config.saml_configuration.slug + provider_config_slug = provider_config.slug - if (actual_slug != expected_slug and - not (actual_slug == 'default' or expected_slug == 'default')): + if (saml_configuration_slug != provider_config_slug and + not (saml_configuration_slug == 'default' or provider_config_slug == 'default')): self.stdout.write( f"[SLUG_MISMATCH] {provider_info} " - f"config slug ('{actual_slug}') != provider slug ('{expected_slug}')" + f"config slug ('{saml_configuration_slug}') != provider slug ('{provider_config_slug}')" ) slug_mismatch_count += 1 From daec94bce456fe193aa17da4a9d3f4fad4f3170c Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 9 Sep 2025 12:10:20 +0000 Subject: [PATCH 07/16] feat: update saml management command --- .../third_party_auth/management/commands/saml.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 52fee6ae5987..8bc30d39972e 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -79,14 +79,13 @@ def _handle_run_checks(self, options): 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 between SAMLProviderConfig and its SAMLConfiguration (except when slug is 'default' which may be intentional) + - Slug mismatches between SAMLProviderConfig and its SAMLConfiguration + (except when slug is 'default' which may be intentional) - SAMLProviderConfig objects with null SAMLConfiguration references (informational) Includes observability attributes for monitoring. """ - site_id = options.get('site_id') - - # Set custom attributes for monitoring the check operation + site_id = options.get('site_id') # 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') @@ -119,7 +118,10 @@ def _check_provider_configurations(self, site_id): for provider_config in provider_configs: total_providers += 1 - provider_info = f"Provider '{provider_config.slug}' (ID: {provider_config.id}, site {provider_config.site_id})" + provider_info = ( + f"Provider '{provider_config.slug}' " + f"(ID: {provider_config.id}, site {provider_config.site_id})" + ) if not provider_config.saml_configuration: self.stdout.write(f"[INFO] {provider_info} has no SAML configuration (may be intentional)") @@ -144,7 +146,9 @@ def _check_provider_configurations(self, site_id): # No current config found - this might indicate the referenced config is no longer valid self.stdout.write( f"[WARNING] {provider_info} " - f"references config (ID: {provider_config.saml_configuration_id}) but no current config found for site {provider_config.saml_configuration.site_id}, slug '{provider_config.saml_configuration.slug}'" + f"references config (ID: {provider_config.saml_configuration_id}) but no current config " + f"found for site {provider_config.saml_configuration.site_id}, " + f"slug '{provider_config.saml_configuration.slug}'" ) outdated_count += 1 From a58ec45239f3823ae97ef00f2e9e93bbc634aaeb Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 10 Sep 2025 06:18:53 +0000 Subject: [PATCH 08/16] feat: update saml management command --- .../third_party_auth/management/commands/saml.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 8bc30d39972e..d4ce73ac47f1 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -79,13 +79,14 @@ def _handle_run_checks(self, options): 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 between SAMLProviderConfig and its SAMLConfiguration - (except when slug is 'default' which may be intentional) + - Slug mismatches (except 'default' slugs) # noqa: E501 - SAMLProviderConfig objects with null SAMLConfiguration references (informational) Includes observability attributes for monitoring. """ - site_id = options.get('site_id') # Set custom attributes for monitoring the check operation + site_id = options.get('site_id') + + # 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') From 1afa96a7cef353349adfb42a39623a150c25483c Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 11 Sep 2025 06:50:33 +0000 Subject: [PATCH 09/16] feat: update saml management command --- .../management/commands/saml.py | 98 ++++++++----------- .../management/commands/tests/test_saml.py | 57 ++++------- 2 files changed, 60 insertions(+), 95 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index d4ce73ac47f1..133f474bb5e2 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -23,11 +23,6 @@ def add_arguments(self, parser): action='store_true', help="Run checks on SAMLProviderConfig configurations and report potential issues" ) - parser.add_argument( - '--site-id', - type=int, - help='Only check configurations for a specific site ID (to be used with --run-checks)' - ) def handle(self, *args, **options): should_pull_saml_metadata = options.get('pull', False) @@ -38,7 +33,7 @@ def handle(self, *args, **options): return if should_run_checks: - self._handle_run_checks(options) + self._handle_run_checks() return raise CommandError("Command must be used with '--pull' or '--run-checks' option.") @@ -72,7 +67,7 @@ def _handle_pull_metadata(self): ) ) - def _handle_run_checks(self, options): + def _handle_run_checks(self): """ Handle the --run-checks option for checking SAMLProviderConfig configuration issues. @@ -84,21 +79,15 @@ def _handle_run_checks(self, options): Includes observability attributes for monitoring. """ - site_id = options.get('site_id') - # 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') - # .. custom_attribute_name: saml_management_command.site_filter - # .. custom_attribute_description: Records the site filter applied, either specific site ID or 'all'. - set_custom_attribute('saml_management_command.site_filter', str(site_id) if site_id else 'all') - - metrics = self._check_provider_configurations(site_id) + metrics = self._check_provider_configurations() self._report_check_summary(metrics) - def _check_provider_configurations(self, site_id): + def _check_provider_configurations(self): """ Check each provider configuration for potential issues. Returns a dictionary of metrics about the found issues. @@ -111,8 +100,6 @@ def _check_provider_configurations(self, site_id): total_providers = 0 provider_configs = SAMLProviderConfig.objects.current_set() - if site_id: - provider_configs = provider_configs.filter(site_id=site_id) self.stdout.write(self.style.SUCCESS("SAML Configuration Check Report")) self.stdout.write("=" * 50) @@ -120,12 +107,12 @@ def _check_provider_configurations(self, site_id): for provider_config in provider_configs: total_providers += 1 provider_info = ( - f"Provider '{provider_config.slug}' " - f"(ID: {provider_config.id}, site {provider_config.site_id})" + f"Provider (id={provider_config.id}, name={provider_config.name}, " + 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 (may be intentional)") + self.stdout.write(f"[INFO] {provider_info} has no SAML configuration because a matching default was not found.") null_config_count += 1 continue @@ -139,26 +126,19 @@ def _check_provider_configurations(self, site_id): if current_config: if current_config.id != provider_config.saml_configuration_id: self.stdout.write( - f"[OUTDATED] {provider_info} " - f"has outdated config (ID: {provider_config.saml_configuration_id} -> {current_config.id})" + 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 - else: - # No current config found - this might indicate the referenced config is no longer valid - self.stdout.write( - f"[WARNING] {provider_info} " - f"references config (ID: {provider_config.saml_configuration_id}) but no current config " - f"found for site {provider_config.saml_configuration.site_id}, " - f"slug '{provider_config.saml_configuration.slug}'" - ) - outdated_count += 1 if provider_config.saml_configuration.site_id != provider_config.site_id: config_site = provider_config.saml_configuration.site_id provider_site = provider_config.site_id self.stdout.write( - f"[SITE_MISMATCH] {provider_info} " - f"config site ({config_site}) != provider site ({provider_site})" + f"[WARNING] {provider_info} " + f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site}) " + f"does not match the provider's site_id." ) site_mismatch_count += 1 @@ -166,10 +146,11 @@ def _check_provider_configurations(self, site_id): provider_config_slug = provider_config.slug if (saml_configuration_slug != provider_config_slug and - not (saml_configuration_slug == 'default' or provider_config_slug == 'default')): + saml_configuration_slug != 'default'): self.stdout.write( - f"[SLUG_MISMATCH] {provider_info} " - f"config slug ('{saml_configuration_slug}') != provider slug ('{provider_config_slug}')" + f"[WARNING] {provider_info} " + f"SAML config (id={provider_config.saml_configuration_id}, slug='{saml_configuration_slug}') " + f"does not match the provider's slug." ) slug_mismatch_count += 1 @@ -178,40 +159,43 @@ def _check_provider_configurations(self, site_id): error_count += 1 metrics = { - 'total_providers': total_providers, - 'outdated_count': outdated_count, - 'site_mismatch_count': site_mismatch_count, - 'slug_mismatch_count': slug_mismatch_count, - 'null_config_count': null_config_count, - 'error_count': error_count, + '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}, + 'null_config_count': {'count': null_config_count, 'requires_attention': False}, + 'error_count': {'count': error_count, 'requires_attention': True}, } - for key, value in metrics.items(): + 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}', value) + set_custom_attribute(f'saml_management_command.{key}', metric_data['count']) return metrics def _report_check_summary(self, metrics): """ - Print a summary of the check results and set the total_issues custom attribute. + Print a summary of the check results and set the total_requiring_attention custom attribute. """ - total_issues = metrics['outdated_count'] + metrics['site_mismatch_count'] + metrics['slug_mismatch_count'] + 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_issues + # .. 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_issues', total_issues) + 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: {metrics['total_providers']}") - self.stdout.write(f" Outdated: {metrics['outdated_count']}") - self.stdout.write(f" Site mismatches: {metrics['site_mismatch_count']}") - self.stdout.write(f" Slug mismatches: {metrics['slug_mismatch_count']}") - self.stdout.write(f" Null configs: {metrics['null_config_count']}") - self.stdout.write(f" Errors: {metrics['error_count']}") - - if total_issues > 0: - self.stdout.write(f"\nTotal issues requiring attention: {total_issues}") + self.stdout.write(f" Providers: {metrics['total_providers']['count']}") + 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" Null configs: {metrics['null_config_count']['count']}") + self.stdout.write(f" Errors: {metrics['error_count']['count']}") + + if total_requiring_attention > 0: + self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") else: self.stdout.write(self.style.SUCCESS("\nNo 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 a888c790eeef..bffe3a59f3f8 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 @@ -329,24 +329,14 @@ def test_xml_parse_exceptions(self, mocked_get): call_command("saml", pull=True, stdout=self.stdout) assert expected in self.stdout.getvalue() - def _run_checks_command(self, site_id=None): + def _run_checks_command(self): """ Helper method to run the --run-checks command and return output. """ out = StringIO() - args = ['saml', '--run-checks'] - if site_id: - args.extend(['--site-id', str(site_id)]) - call_command(*args, stdout=out) + call_command('saml', '--run-checks', stdout=out) return out.getvalue() - def _assert_observability_calls(self, mock_set_custom_attribute, expected_calls): - """ - Helper method to assert multiple observability calls. - """ - for call_args in expected_calls: - mock_set_custom_attribute.assert_any_call(*call_args) - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') def test_run_checks_outdated_configs(self, mock_set_custom_attribute): """ @@ -356,19 +346,23 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): output = self._run_checks_command() - self.assertIn('[OUTDATED]', output) + # Print the output for debugging + print("=== COMMAND OUTPUT ===") + print(output) + print("=== MOCK CALLS ===") + for call in mock_set_custom_attribute.call_args_list: + print(f"set_custom_attribute{call}") + + self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) - self.assertIn(f'{old_config.id} -> {new_config.id}', output) + self.assertIn(f'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: 2', output) self.assertIn('Outdated: 1', output) - expected_calls = [ - ('saml_management_command.operation', 'run_checks'), - ('saml_management_command.outdated_count', 1), - ('saml_management_command.total_issues', 2) - ] - self._assert_observability_calls(mock_set_custom_attribute, expected_calls) + # Check key observability calls + mock_set_custom_attribute.assert_any_call('saml_management_command.operation', 'run_checks') + mock_set_custom_attribute.assert_any_call('saml_management_command.outdated_count', 1) @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') def test_run_checks_site_mismatches(self, mock_set_custom_attribute): @@ -389,8 +383,9 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): output = self._run_checks_command() - self.assertIn('[SITE_MISMATCH]', output) + self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) + self.assertIn('does not match the provider\'s site_id', output) mock_set_custom_attribute.assert_any_call('saml_management_command.site_mismatch_count', 1) @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') @@ -412,8 +407,9 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): output = self._run_checks_command() - self.assertIn('[SLUG_MISMATCH]', output) + self.assertIn('[WARNING]', output) self.assertIn('provider-slug', output) + self.assertIn('does not match the provider\'s slug', output) mock_set_custom_attribute.assert_any_call('saml_management_command.slug_mismatch_count', 1) @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') @@ -431,20 +427,5 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): self.assertIn('[INFO]', output) self.assertIn('null-provider', output) - self.assertIn('has no SAML configuration', output) + self.assertIn('has no SAML configuration because a matching default was not found', output) mock_set_custom_attribute.assert_any_call('saml_management_command.null_config_count', 2) - - @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') - def test_run_checks_with_site_filter(self, mock_set_custom_attribute): - """ - Test the --run-checks command with --site-id filter. - """ - SAMLProviderConfigFactory.create(site=self.site, slug='site1-provider', saml_configuration=None) - SAMLProviderConfigFactory.create(site=self.other_site, slug='site2-provider', saml_configuration=None) - - output = self._run_checks_command(site_id=self.site.id) - - self.assertIn('site1-provider', output) - self.assertNotIn('site2-provider', output) - self.assertIn('Providers: 1', output) - mock_set_custom_attribute.assert_any_call('saml_management_command.site_filter', str(self.site.id)) From 23d58a4de12df68150e509c881448bb6ba712ce6 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 11 Sep 2025 07:38:34 +0000 Subject: [PATCH 10/16] feat: update saml management command --- .../third_party_auth/management/commands/saml.py | 8 +++++--- .../management/commands/tests/test_saml.py | 5 ++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 133f474bb5e2..fb01aa15fc74 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -112,7 +112,10 @@ def _check_provider_configurations(self): ) if not provider_config.saml_configuration: - self.stdout.write(f"[INFO] {provider_info} has no SAML configuration because a matching default was not found.") + self.stdout.write( + f"[INFO] {provider_info} has no SAML configuration because " + f"a matching default was not found." + ) null_config_count += 1 continue @@ -145,8 +148,7 @@ def _check_provider_configurations(self): saml_configuration_slug = provider_config.saml_configuration.slug provider_config_slug = provider_config.slug - if (saml_configuration_slug != provider_config_slug and - saml_configuration_slug != 'default'): + 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}') " 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 bffe3a59f3f8..fa2f455852da 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 @@ -355,7 +355,10 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): 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})', output) + self.assertIn( + f'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: 2', output) self.assertIn('Outdated: 1', output) From 2bcac28d12833c701d0c31dbcec6b0215f6eea44 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Tue, 16 Sep 2025 06:10:34 +0000 Subject: [PATCH 11/16] feat: update saml management command --- .../third_party_auth/management/commands/saml.py | 6 +++--- .../management/commands/tests/test_saml.py | 16 +++++++--------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index fb01aa15fc74..b9103283834a 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -136,11 +136,11 @@ def _check_provider_configurations(self): outdated_count += 1 if provider_config.saml_configuration.site_id != provider_config.site_id: - config_site = provider_config.saml_configuration.site_id - provider_site = 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}) " + f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " f"does not match the provider's site_id." ) site_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 fa2f455852da..dabcb375aada 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 @@ -346,13 +346,6 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): output = self._run_checks_command() - # Print the output for debugging - print("=== COMMAND OUTPUT ===") - print(output) - print("=== MOCK CALLS ===") - for call in mock_set_custom_attribute.call_args_list: - print(f"set_custom_attribute{call}") - self.assertIn('[WARNING]', output) self.assertIn('test-provider', output) self.assertIn( @@ -364,8 +357,13 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): self.assertIn('Outdated: 1', output) # Check key observability calls - mock_set_custom_attribute.assert_any_call('saml_management_command.operation', 'run_checks') - mock_set_custom_attribute.assert_any_call('saml_management_command.outdated_count', 1) + 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), + ] + for call in expected_calls: + self.assertIn(call, mock_set_custom_attribute.call_args_list) @mock.patch('common.djangoapps.third_party_auth.management.commands.saml.set_custom_attribute') def test_run_checks_site_mismatches(self, mock_set_custom_attribute): From fb01dafc2fbda457a8bc6616bdced9736443f1bf Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 16 Sep 2025 13:32:19 -0400 Subject: [PATCH 12/16] fixup! Update common/djangoapps/third_party_auth/management/commands/tests/test_saml.py --- .../third_party_auth/management/commands/tests/test_saml.py | 3 +-- 1 file changed, 1 insertion(+), 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 dabcb375aada..57bcaec71983 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 @@ -362,8 +362,7 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): mock.call('saml_management_command.total_providers', 2), mock.call('saml_management_command.outdated_count', 1), ] - for call in expected_calls: - self.assertIn(call, mock_set_custom_attribute.call_args_list) + 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): From 46c940818523f8730d60ccd5f05dbe513c2e165c Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 17 Sep 2025 07:17:28 +0000 Subject: [PATCH 13/16] feat: update saml management command --- .../management/commands/saml.py | 13 ++--- .../management/commands/tests/test_saml.py | 49 +++++++++++++++++-- 2 files changed, 52 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 b9103283834a..929485afd31f 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -190,14 +190,15 @@ def _report_check_summary(self, metrics): 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: {metrics['total_providers']['count']}") - 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" Providers checked: {metrics['total_providers']['count']}") self.stdout.write(f" Null configs: {metrics['null_config_count']['count']}") - self.stdout.write(f" Errors: {metrics['error_count']['count']}") if total_requiring_attention > 0: - self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") + self.stdout.write(f"\nErrors and warnings 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" Errors: {metrics['error_count']['count']}") + self.stdout.write(f"\nTotal errors and warnings: {total_requiring_attention}") else: self.stdout.write(self.style.SUCCESS("\nNo 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 57bcaec71983..6963d5dcd0d5 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 @@ -353,7 +353,7 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): output ) self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers: 2', output) + self.assertIn('Providers checked: 2', output) self.assertIn('Outdated: 1', output) # Check key observability calls @@ -361,6 +361,11 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): 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.error_count', 0), + mock.call('saml_management_command.total_requiring_attention', 2), ] mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) @@ -386,7 +391,19 @@ 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) - mock_set_custom_attribute.assert_any_call('saml_management_command.site_mismatch_count', 1) + + # 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.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_slug_mismatches(self, mock_set_custom_attribute): @@ -410,7 +427,19 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): self.assertIn('[WARNING]', output) self.assertIn('provider-slug', output) self.assertIn('does not match the provider\'s slug', output) - mock_set_custom_attribute.assert_any_call('saml_management_command.slug_mismatch_count', 1) + + # 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.error_count', 0), + mock.call('saml_management_command.total_requiring_attention', 1), + ] + 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): @@ -428,4 +457,16 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): self.assertIn('[INFO]', output) self.assertIn('null-provider', output) self.assertIn('has no SAML configuration because a matching default was not found', output) - mock_set_custom_attribute.assert_any_call('saml_management_command.null_config_count', 2) + + # 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', 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', 0), + ] + mock_set_custom_attribute.assert_has_calls(expected_calls, any_order=False) From 6f1da54781114f9380c087a85a2b559ab3088422 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 17 Sep 2025 07:37:14 +0000 Subject: [PATCH 14/16] feat: update saml management command --- .../third_party_auth/management/commands/saml.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 929485afd31f..31ecd9251a45 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -114,7 +114,7 @@ def _check_provider_configurations(self): if not provider_config.saml_configuration: self.stdout.write( f"[INFO] {provider_info} has no SAML configuration because " - f"a matching default was not found." + "a matching default was not found." ) null_config_count += 1 continue @@ -141,7 +141,7 @@ def _check_provider_configurations(self): self.stdout.write( f"[WARNING] {provider_info} " f"SAML config (id={provider_config.saml_configuration_id}, site_id={config_site_id}) " - f"does not match the provider's site_id." + "does not match the provider's site_id." ) site_mismatch_count += 1 @@ -152,7 +152,7 @@ def _check_provider_configurations(self): self.stdout.write( f"[WARNING] {provider_info} " f"SAML config (id={provider_config.saml_configuration_id}, slug='{saml_configuration_slug}') " - f"does not match the provider's slug." + "does not match the provider's slug." ) slug_mismatch_count += 1 @@ -194,7 +194,7 @@ def _report_check_summary(self, metrics): self.stdout.write(f" Null configs: {metrics['null_config_count']['count']}") if total_requiring_attention > 0: - self.stdout.write(f"\nErrors and warnings requiring attention:") + self.stdout.write("\nErrors and warnings 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']}") From eddd1efffb47d607f3a7ac42eb947e817c04f47b Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 17 Sep 2025 09:06:52 +0000 Subject: [PATCH 15/16] feat: update saml management command --- common/djangoapps/third_party_auth/management/commands/saml.py | 1 + 1 file changed, 1 insertion(+) diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index 31ecd9251a45..a6bd0705614c 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -103,6 +103,7 @@ def _check_provider_configurations(self): self.stdout.write(self.style.SUCCESS("SAML Configuration Check Report")) self.stdout.write("=" * 50) + self.stdout.write("") for provider_config in provider_configs: total_providers += 1 From c64f0492003c9c417a955386847ecf02b0fc03de Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Wed, 17 Sep 2025 09:53:07 +0000 Subject: [PATCH 16/16] feat: update saml management command --- .../djangoapps/third_party_auth/management/commands/saml.py | 4 ++-- 1 file changed, 2 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 a6bd0705614c..afe369c2ade0 100644 --- a/common/djangoapps/third_party_auth/management/commands/saml.py +++ b/common/djangoapps/third_party_auth/management/commands/saml.py @@ -195,11 +195,11 @@ def _report_check_summary(self, metrics): self.stdout.write(f" Null configs: {metrics['null_config_count']['count']}") if total_requiring_attention > 0: - self.stdout.write("\nErrors and warnings requiring attention:") + 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" Errors: {metrics['error_count']['count']}") - self.stdout.write(f"\nTotal errors and warnings: {total_requiring_attention}") + self.stdout.write(f"\nTotal issues requiring attention: {total_requiring_attention}") else: self.stdout.write(self.style.SUCCESS("\nNo configuration issues found!"))