diff --git a/common/djangoapps/third_party_auth/management/commands/saml.py b/common/djangoapps/third_party_auth/management/commands/saml.py index afe369c2ade0..6865ebf69987 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,31 +70,28 @@ 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 null SAMLConfiguration references (informational) - - 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) 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 + - 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 site_mismatch_count = 0 slug_mismatch_count = 0 null_config_count = 0 + disabled_config_count = 0 error_count = 0 total_providers = 0 @@ -107,53 +103,74 @@ 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 = ", enabled=False" if provider_disabled else "" + provider_info = ( - f"Provider (id={provider_config.id}, name={provider_config.name}, " - f"slug={provider_config.slug}, site_id={provider_config.site_id})" + 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 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 + # Provider disabled status is already included in provider_info format try: + if not provider_config.saml_configuration: + null_config_count, disabled_config_count = self._check_no_config( + provider_config, provider_info, null_config_count, disabled_config_count + ) + continue + + # Check if SAML configuration is disabled + if not provider_config.saml_configuration.enabled: + # Resolution: Enable the SAML configuration in Django admin + # or assign a different configuration + self.stdout.write( + 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( 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: - 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 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 - provider_site_id = provider_config.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." + 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 - saml_configuration_slug = provider_config.saml_configuration.slug - provider_config_slug = provider_config.slug - - if saml_configuration_slug not in (provider_config_slug, 'default'): + # 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 + 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"[WARNING] {provider_info} " - f"SAML config (id={provider_config.saml_configuration_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 @@ -165,41 +182,64 @@ 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': False}, + 'disabled_config_count': {'count': disabled_config_count, 'requires_attention': True}, '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, 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 + # 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 + + 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( + f"[WARNING] {provider_info} has no direct SAML configuration and " + f"the default configuration (id={default_config.id}, enabled=False)." + ) + disabled_config_count += 1 + + return null_config_count, disabled_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(f" Null 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']}") + 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" Slug mismatches: {metrics['slug_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(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 6963d5dcd0d5..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 @@ -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,8 +338,30 @@ 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_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. """ @@ -346,31 +369,18 @@ def test_run_checks_outdated_configs(self, mock_set_custom_attribute): 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})', - output + 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}).' ) - self.assertIn('CHECK SUMMARY:', output) - self.assertIn('Providers checked: 2', output) + self.assertIn(expected_warning, output) self.assertIn('Outdated: 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) - # 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.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_site_mismatches(self, mock_set_custom_attribute): + def test_run_checks_site_mismatches(self): """ Test the --run-checks command identifies site ID mismatches. """ @@ -380,7 +390,7 @@ 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 @@ -388,25 +398,17 @@ def test_run_checks_site_mismatches(self, mock_set_custom_attribute): 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.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): + 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) + # 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): """ Test the --run-checks command identifies slug mismatches. """ @@ -416,7 +418,7 @@ 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 @@ -424,29 +426,23 @@ def test_run_checks_slug_mismatches(self, mock_set_custom_attribute): 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.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): + 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) + + 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. """ - SAMLProviderConfigFactory.create( + # 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 @@ -454,19 +450,101 @@ def test_run_checks_null_configurations(self, mock_set_custom_attribute): output = self._run_checks_command() - self.assertIn('[INFO]', output) - self.assertIn('null-provider', output) - self.assertIn('has no SAML configuration because a matching default was not found', 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', 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) + 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: 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): + """ + 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 + ) + + 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 the default configuration ' + 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) # 1 from this test + 1 from setUp data + + def test_run_checks_with_default_config(self): + """ + Test the --run-checks command correctly handles providers with default configurations. + """ + provider = SAMLProviderConfigFactory.create( + site=self.site, + slug='default-config-provider', + saml_configuration=None + ) + + default_config = SAMLConfigurationFactory.create( + site=self.site, + slug='default', + entity_id='https://default.example.com' + ) + + output = self._run_checks_command() + + 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): + """ + Test the --run-checks command handles disabled providers and configurations. + """ + disabled_provider = SAMLProviderConfigFactory.create( + site=self.site, + slug='disabled-provider', + enabled=False + ) + + disabled_config = SAMLConfigurationFactory.create( + site=self.site, + slug='disabled-config', + enabled=False + ) + + provider_with_disabled_config = SAMLProviderConfigFactory.create( + site=self.site, + slug='provider-with-disabled-config', + saml_configuration=disabled_config + ) + + output = self._run_checks_command() + + 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: 1', output) # disabled_provider has no config + self.assertIn('Disabled configs: 2', output) # setUp's provider + provider_with_disabled_config