Merge branch 'master' into html-extracted

Author: irtazaakram

cms/djangoapps/contentstore/views/tests/test_block.py

@@ -33,7 +33,6 @@
 from xblock.runtime import DictKeyValueStore, KvsFieldData
 from xblock.test.tools import TestRuntime
 from xblock.validation import ValidationMessage
-from xmodule.capa_block import ProblemBlock
 from xmodule.course_block import DEFAULT_START_DATE
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
@@ -619,7 +618,9 @@ def test_create_nicely(self):
         prob_usage_key = self.response_usage_key(resp)
         problem = self.get_item_from_modulestore(prob_usage_key)
         # check against the template
-        template = ProblemBlock.get_template(template_id)
+        course = CourseFactory.create()
+        problem_block = BlockFactory.create(category="problem", parent_location=course.location)
+        template = problem_block.get_template(template_id)
         self.assertEqual(problem.data, template["data"])
         self.assertEqual(problem.display_name, template["metadata"]["display_name"])
         self.assertEqual(problem.markdown, template["metadata"]["markdown"])

common/djangoapps/student/models/course_enrollment.py

@@ -364,10 +364,8 @@ def __str__(self):
             "[CourseEnrollment] {}: {} ({}); active: ({})"
         ).format(self.user, self.course_id, self.created, self.is_active)
 
-    def save(self, force_insert=False, force_update=False, using=None, update_fields=None):
-        super().save(
-            force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields
-        )
+    def save(self, *args, **kwargs):
+        super().save(*args, **kwargs)
 
         # Delete the cached status hash, forcing the value to be recalculated the next time it is needed.
         cache.delete(self.enrollment_status_hash_cache_key(self.user))

common/djangoapps/third_party_auth/apps.py

@@ -9,6 +9,9 @@ class ThirdPartyAuthConfig(AppConfig):  # lint-amnesty, pylint: disable=missing-
     verbose_name = "Third-party authentication"
 
     def ready(self):
+        # Import signal handlers to register them
+        from .signals import handlers  # noqa: F401 pylint: disable=unused-import
+
         # To override the settings before loading social_django.
         if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH', False):
             self._enable_third_party_auth()

common/djangoapps/third_party_auth/management/commands/saml.py

@@ -8,6 +8,7 @@
 from django.core.management.base import BaseCommand, CommandError
 
 from common.djangoapps.third_party_auth.tasks import fetch_saml_metadata
+from common.djangoapps.third_party_auth.models import SAMLProviderConfig, SAMLConfiguration
 
 
 class Command(BaseCommand):
@@ -16,13 +17,41 @@ 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',
+            action='store_true',
+            help="Fix SAMLProviderConfig references to use current SAMLConfiguration versions"
+        )
+        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.'
+        )
 
     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)
 
-        if not should_pull_saml_metadata:
-            raise CommandError("Command can only be used with '--pull' option.")
+        if not should_pull_saml_metadata and not should_fix_references:
+            raise CommandError("Command must be used with '--pull' or '--fix-references' option.")
 
+        if should_pull_saml_metadata:
+            self._handle_pull_metadata()
+
+        if should_fix_references:
+            self._handle_fix_references(options, dry_run=dry_run)
+
+    def _handle_pull_metadata(self):
+        """
+        Handle the --pull option to fetch and update SAML metadata from external providers.
+        This sets up logging and calls the fetch_saml_metadata task.
+        """
         log_handler = logging.StreamHandler(self.stdout)
         log_handler.setLevel(logging.DEBUG)
         log = logging.getLogger('common.djangoapps.third_party_auth.tasks')
@@ -46,3 +75,46 @@ def handle(self, *args, **options):
                     failures="\n\n".join(failure_messages)
                 )
             )
+
+    def _handle_fix_references(self, options, dry_run=False):
+        """Handle the --fix-references option for fixing outdated SAML configuration references."""
+        site_id = options.get('site_id')
+        updated_count = 0
+        error_count = 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})"
+                        )
+
+                        if not dry_run:
+                            provider_config.saml_configuration = current_config
+                            provider_config.save()
+                        updated_count += 1
+
+                except Exception as e:  # pylint: disable=broad-except
+                    self.stderr.write(
+                        f"Error processing provider '{provider_config.slug}': {e}"
+                    )
+                    error_count += 1
+
+        style = self.style.SUCCESS
+        if dry_run:
+            msg = f"[DRY RUN] Would update {updated_count} provider configurations. {error_count} errors encountered."
+        else:
+            msg = f"Updated {updated_count} provider configurations. {error_count} errors encountered."
+        self.stdout.write(style(msg))

common/djangoapps/third_party_auth/management/commands/tests/test_saml.py

@@ -8,6 +8,8 @@
 from io import StringIO
 
 from unittest import mock
+from ddt import ddt, data, unpack
+from django.contrib.sites.models import Site
 from django.core.management import call_command
 from django.core.management.base import CommandError
 from requests import exceptions
@@ -16,6 +18,8 @@
 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):
     """
@@ -45,6 +49,7 @@ def _(url=None, *args, **kwargs):  # lint-amnesty, pylint: disable=keyword-arg-b
 
 
 @skip_unless_lms
+@ddt
 class TestSAMLCommand(CacheIsolationTestCase):
     """
     Test django management command for fetching saml metadata.
@@ -58,12 +63,17 @@ def setUp(self):
         super().setUp()
 
         self.stdout = StringIO()
+        self.site = Site.objects.get_current()
 
         # 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
         # not processed.
-        SAMLConfigurationFactory.create(enabled=False, site__domain='testserver.fake', site__name='testserver.fake')
-        SAMLProviderConfigFactory.create(
+        self.saml_config = SAMLConfigurationFactory.create(
+            enabled=False,
+            site__domain='testserver.fake',
+            site__name='testserver.fake'
+        )
+        self.provider_config = SAMLProviderConfigFactory.create(
             site__domain='testserver.fake',
             site__name='testserver.fake',
             slug='test-shib',
@@ -72,6 +82,44 @@ def setUp(self):
             metadata_source='https://www.testshib.org/metadata/testshib-providers.xml',
         )
 
+    def _setup_test_configs_for_fix_references(self):
+        """
+        Helper method to create SAML configurations for fix-references tests.
+
+        Returns tuple of (old_config, new_config, provider_config)
+
+        Using a separate method keeps test data isolated. Including these configs in
+        setUp would create 3 provider configs for all tests, breaking tests that expect
+        specific provider counts or try to access non-existent test XML files.
+        """
+        # Create a SAML config that will be outdated after the new config is created
+        old_config = SAMLConfigurationFactory.create(
+            enabled=False,
+            site=self.site,
+            slug='test-config',
+            entity_id='https://old.example.com'
+        )
+
+        # Create newer config with same slug
+        new_config = SAMLConfigurationFactory.create(
+            enabled=True,
+            site=self.site,
+            slug='test-config',
+            entity_id='https://updated.example.com'
+        )
+
+        # Create a provider config that references the old config for fix-references tests
+        test_provider_config = SAMLProviderConfigFactory.create(
+            site=self.site,
+            slug='test-provider',
+            name='Test Provider',
+            entity_id='https://test.provider/idp/shibboleth',
+            metadata_source='https://test.provider/metadata.xml',
+            saml_configuration=old_config
+        )
+
+        return old_config, new_config, test_provider_config
+
     def __create_saml_configurations__(self, saml_config=None, saml_provider_config=None):
         """
         Helper method to create SAMLConfiguration and AMLProviderConfig.
@@ -101,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 can only be used with '--pull' option."):
+        with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--fix-references' option."):
             call_command("saml")
 
         # Call `saml` command without any argument so that it raises a CommandError
-        with self.assertRaisesMessage(CommandError, "Command can only be used with '--pull' option."):
+        with self.assertRaisesMessage(CommandError, "Command must be used with '--pull' or '--fix-references' option."):
             call_command("saml", pull=False)
 
     def test_no_saml_configuration(self):
@@ -285,3 +333,60 @@ def test_xml_parse_exceptions(self, mocked_get):
         with self.assertRaisesRegex(CommandError, "XMLSyntaxError:"):
             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):
+        """
+        Test the --fix-references command with and without --dry-run option.
+
+        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
+        """
+        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
+
+        out = StringIO()
+        if dry_run:
+            call_command('saml', '--fix-references', '--dry-run', stdout=out)
+        else:
+            call_command('saml', '--fix-references', stdout=out)
+
+        output = out.getvalue()
+
+        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"
+            )

common/djangoapps/third_party_auth/signals/__init__.py

@@ -0,0 +1 @@
+# Signal handlers for third_party_auth app

common/djangoapps/third_party_auth/signals/handlers.py

@@ -0,0 +1,57 @@
+"""
+Signal handlers for third_party_auth app.
+"""
+
+from django.db.models.signals import post_save
+from django.dispatch import receiver
+from edx_django_utils.monitoring import set_custom_attribute
+
+from common.djangoapps.third_party_auth.models import SAMLConfiguration, SAMLProviderConfig
+from common.djangoapps.third_party_auth.toggles import ENABLE_SAML_CONFIG_SIGNAL_HANDLERS
+
+
+@receiver(post_save, sender=SAMLConfiguration)
+def update_saml_provider_configs_on_configuration_change(sender, instance, created, **kwargs):
+    """
+    Signal handler to create a new SAMLProviderConfig when SAMLConfiguration is updated.
+
+    When a SAMLConfiguration is updated and a new version is created, this handler
+    generates a corresponding SAMLProviderConfig that references the latest
+    configuration version, ensuring all providers remain aligned with the most
+    current settings.
+    """
+    # .. custom_attribute_name: saml_config_signal.enabled
+    # .. custom_attribute_description: Tracks whether the SAML config signal handler is enabled.
+    set_custom_attribute('saml_config_signal.enabled', ENABLE_SAML_CONFIG_SIGNAL_HANDLERS.is_enabled())
+
+    # .. custom_attribute_name: saml_config_signal.new_config_id
+    # .. custom_attribute_description: Records the ID of the new SAML configuration instance.
+    set_custom_attribute('saml_config_signal.new_config_id', instance.id)
+
+    # .. custom_attribute_name: saml_config_signal.slug
+    # .. custom_attribute_description: Records the slug of the SAML configuration instance.
+    set_custom_attribute('saml_config_signal.slug', instance.slug)
+
+    if ENABLE_SAML_CONFIG_SIGNAL_HANDLERS.is_enabled():
+        try:
+            # Find all existing SAMLProviderConfig instances (current_set) that should be
+            # pointing to this slug but are pointing to an older version
+            existing_providers = SAMLProviderConfig.objects.current_set().filter(
+                site_id=instance.site_id,
+                saml_configuration__slug=instance.slug
+            ).exclude(saml_configuration_id=instance.id)
+
+            updated_count = 0
+            for provider_config in existing_providers:
+                provider_config.saml_configuration = instance
+                provider_config.save()
+                updated_count += 1
+
+            # .. custom_attribute_name: saml_config_signal.updated_count
+            # .. custom_attribute_description: The number of SAMLProviderConfig records updated to point to the new configuration.
+            set_custom_attribute('saml_config_signal.updated_count', updated_count)
+
+        except Exception as e:  # pylint: disable=broad-except
+            # .. custom_attribute_name: saml_config_signal.error_message
+            # .. custom_attribute_description: Records any error message that occurs during SAML provider config updates.
+            set_custom_attribute('saml_config_signal.error_message', str(e))

common/djangoapps/third_party_auth/signals/tests/__init__.py

@@ -0,0 +1 @@
+# This file marks the directory as a Python package.