Merge pull request #331 from open-craft/v4.5.0-security-update

Merge pull request from GHSA-7j9p-67mm-5g87

lti_consumer/__init__.py

@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '4.5.0'
+__version__ = '4.5.1'

lti_consumer/signals.py

@@ -22,16 +22,34 @@ def publish_grade_on_score_update(sender, instance, **kwargs):  # pylint: disabl
     in the LMS. Trying to trigger this signal from Studio (from the Django-admin interface, for example)
     throw an exception.
     """
+    line_item = instance.line_item
+    lti_config = line_item.lti_configuration
+
+    # Only save score if the `line_item.resource_link_id` is the same as
+    # `lti_configuration.location` to prevent LTI tools to alter grades they don't
+    # have permissions to.
+    # TODO: This security mechanism will need to be reworked once we enable LTI 1.3
+    # reusability to allow one configuration to save scores on multiple placements,
+    # but still locking down access to the items that are using the LTI configurtion.
+    if line_item.resource_link_id != lti_config.location:
+        log.warning(
+            "LTI tool tried publishing score %r to block %s (outside allowed scope of: %s).",
+            instance,
+            line_item.resource_link_id,
+            lti_config.location,
+        )
+        return
+
     # Before starting to publish grades to the LMS, check that:
     # 1. The grade being submitted in the final one - `FullyGraded`
     # 2. This LineItem is linked to a LMS grade - the `LtiResouceLinkId` field is set
     # 3. There's a valid grade in this score - `scoreGiven` is set
     if instance.grading_progress == LtiAgsScore.FULLY_GRADED \
-            and instance.line_item.resource_link_id \
+            and line_item.resource_link_id \
             and instance.score_given:
         try:
             # Load block using LMS APIs and check if the block is graded and still accept grades.
-            block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id)
+            block = compat.load_block_as_anonymous_user(line_item.resource_link_id)
             if block.has_score and (not block.is_past_due() or block.accept_grades_past_due):
                 # Map external ID to platform user
                 user = compat.get_user_from_external_user_id(instance.user_id)
@@ -58,7 +76,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs):  # pylint: disabl
             log.exception(
                 "Error while publishing score %r to block %s to LMS: %s",
                 instance,
-                instance.line_item.resource_link_id,
+                line_item.resource_link_id,
                 exc,
             )
             raise exc

lti_consumer/tests/unit/test_models.py

@@ -412,7 +412,6 @@ def setUp(self):
 
         self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test'
         self.lti_ags_model = LtiAgsLineItem.objects.create(
-            lti_configuration=None,
             resource_id="test-id",
             label="this-is-a-test",
             resource_link_id=self.dummy_location,
@@ -449,8 +448,16 @@ def setUp(self):
         )
 
         self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test'
+
+        self.lti_config = LtiConfiguration.objects.create(
+            config_id='6c440bf4-face-beef-face-e8bcfb1e53bd',
+            location=self.dummy_location,
+            version=LtiConfiguration.LTI_1P3,
+            config_store=LtiConfiguration.CONFIG_ON_XBLOCK,
+        )
+
         self.line_item = LtiAgsLineItem.objects.create(
-            lti_configuration=None,
+            lti_configuration=self.lti_config,
             resource_id="test-id",
             label="this-is-a-test",
             resource_link_id=self.dummy_location,

lti_consumer/tests/unit/test_signals.py

@@ -0,0 +1,102 @@
+"""
+Tests for LTI Advantage Assignments and Grades Service views.
+"""
+from datetime import datetime
+from unittest.mock import patch, Mock
+
+from django.test import TestCase
+from opaque_keys.edx.keys import UsageKey
+
+from lti_consumer.models import LtiConfiguration, LtiAgsLineItem, LtiAgsScore
+
+
+class PublishGradeOnScoreUpdateTest(TestCase):
+    """
+    Test the `publish_grade_on_score_update` signal.
+    """
+
+    def setUp(self):
+        """
+        Set up resources for signal testing.
+        """
+        super().setUp()
+
+        self.location = UsageKey.from_string(
+            "block-v1:course+test+2020+type@problem+block@test"
+        )
+
+        # Create configuration
+        self.lti_config = LtiConfiguration.objects.create(
+            location=self.location,
+            version=LtiConfiguration.LTI_1P3,
+        )
+
+        # Patch internal method to avoid calls to modulestore
+        self._block_mock = Mock()
+        compat_mock = patch("lti_consumer.signals.compat")
+        self.addCleanup(compat_mock.stop)
+        self._compat_mock = compat_mock.start()
+        self._compat_mock.get_user_from_external_user_id.return_value = Mock()
+        self._compat_mock.load_block_as_anonymous_user.return_value = self._block_mock
+
+    def test_grade_publish_not_done_when_wrong_line_item(self):
+        """
+        Test grade publish after for a different UsageKey than set on
+        `lti_config.location`.
+        """
+        # Create LineItem with `resource_link_id` != `lti_config.id`
+        line_item = LtiAgsLineItem.objects.create(
+            lti_configuration=self.lti_config,
+            resource_id="test",
+            resource_link_id=UsageKey.from_string(
+                "block-v1:course+test+2020+type@problem+block@different"
+            ),
+            label="test label",
+            score_maximum=100
+        )
+
+        # Save score and check that LMS method wasn't called.
+        LtiAgsScore.objects.create(
+            line_item=line_item,
+            score_given=1,
+            score_maximum=1,
+            activity_progress=LtiAgsScore.COMPLETED,
+            grading_progress=LtiAgsScore.FULLY_GRADED,
+            user_id="test",
+            timestamp=datetime.now(),
+        )
+
+        # Check that methods to save grades are not called
+        self._block_mock.set_user_module_score.assert_not_called()
+        self._compat_mock.get_user_from_external_user_id.assert_not_called()
+        self._compat_mock.load_block_as_anonymous_user.assert_not_called()
+
+    def test_grade_publish(self):
+        """
+        Test grade publish after if the UsageKey is equal to
+        the one on `lti_config.location`.
+        """
+        # Create LineItem with `resource_link_id` != `lti_config.id`
+        line_item = LtiAgsLineItem.objects.create(
+            lti_configuration=self.lti_config,
+            resource_id="test",
+            resource_link_id=self.location,
+            label="test label",
+            score_maximum=100
+        )
+
+        # Save score and check that LMS method wasn't called.
+        LtiAgsScore.objects.create(
+            line_item=line_item,
+            score_given=1,
+            score_maximum=1,
+            activity_progress=LtiAgsScore.COMPLETED,
+            grading_progress=LtiAgsScore.FULLY_GRADED,
+            user_id="test",
+            timestamp=datetime.now(),
+        )
+
+        # Check that methods to save grades are called
+        self._block_mock.set_user_module_score.assert_called_once()
+        self._compat_mock.get_user_from_external_user_id.assert_called_once()
+        self._compat_mock.load_block_as_anonymous_user.assert_called_once()