Merge pull request #301 from openedx/ashultz0/unblock

fix: remove general use of load_block_as_user

CHANGELOG.rst

@@ -15,6 +15,14 @@ Please See the [releases tab](https://github.com/edx/xblock-lti-consumer/release
 
 Unreleased
 ~~~~~~~~~~
+6.1.0 - 2022-11-08
+------------------
+* 6.0.0 broke studio functionality because it leaned more heavily on the xblock load which only worked in the LMS.
+
+  * Fix by greatly limiting when we attempt a full xblock load and bind
+
+
+
 6.0.0 - 2022-10-24
 ------------------
 BREAKING CHANGE:

lti_consumer/__init__.py

@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '6.0.0'
+__version__ = '6.1.0'

lti_consumer/api.py

@@ -83,11 +83,6 @@ def _get_lti_config_for_block(block):
             block.location,
             LtiConfiguration.CONFIG_ON_XBLOCK,
         )
-
-    # Since the block was passed, preload it to avoid
-    # having to instance the modulestore and fetch it again.
-    lti_config.block = block
-
     return lti_config
 
 

lti_consumer/models.py

@@ -227,17 +227,14 @@ class LtiConfiguration(models.Model):
                   'grades.'
     )
 
-    # Empty variable that'll hold the block once it's retrieved
-    # from the modulestore or preloaded
-    _block = None
-
     def clean(self):
         if self.config_store == self.CONFIG_ON_XBLOCK and self.location is None:
             raise ValidationError({
                 "config_store": _("LTI Configuration stores on XBlock needs a block location set."),
             })
         if self.version == self.LTI_1P3 and self.config_store == self.CONFIG_ON_DB:
-            if not database_config_enabled(self.block.location.course_key):
+            block = compat.load_enough_xblock(self.location)
+            if not database_config_enabled(block.location.course_key):
                 raise ValidationError({
                     "config_store": _("LTI Configuration stores on database is not enabled."),
                 })
@@ -255,25 +252,6 @@ def clean(self):
         if consumer is None:
             raise ValidationError(_("Invalid LTI configuration."))
 
-    @property
-    def block(self):
-        """
-        Return instance of block (either preloaded or directly from the modulestore).
-        """
-        block = getattr(self, '_block', None)
-        if block is None:
-            if self.location is None:
-                raise ValueError(_("Block location not set, it's not possible to retrieve the block."))
-            block = self._block = compat.load_block_as_user(self.location)
-        return block
-
-    @block.setter
-    def block(self, block):
-        """
-        Allows preloading the block instead of fetching it from the modulestore.
-        """
-        self._block = block
-
     def _generate_lti_1p3_keys_if_missing(self):
         """
         Generate LTI 1.3 RSA256 keys if missing.
@@ -336,8 +314,9 @@ def _get_lti_1p1_consumer(self):
         """
         # If LTI configuration is stored in the XBlock.
         if self.config_store == self.CONFIG_ON_XBLOCK:
-            key, secret = self.block.lti_provider_key_secret
-            launch_url = self.block.launch_url
+            block = compat.load_enough_xblock(self.location)
+            key, secret = block.lti_provider_key_secret
+            launch_url = block.launch_url
         elif self.config_store == self.CONFIG_EXTERNAL:
             config = get_external_config_from_filter({}, self.external_id)
             key = config.get("lti_1p1_client_key")
@@ -360,7 +339,8 @@ def get_lti_advantage_ags_mode(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_ags_mode
         else:
-            return self.block.lti_advantage_ags_mode
+            block = compat.load_enough_xblock(self.location)
+            return block.lti_advantage_ags_mode
 
     def get_lti_advantage_deep_linking_enabled(self):
         """
@@ -372,7 +352,8 @@ def get_lti_advantage_deep_linking_enabled(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_deep_linking_enabled
         else:
-            return self.block.lti_advantage_deep_linking_enabled
+            block = compat.load_enough_xblock(self.location)
+            return block.lti_advantage_deep_linking_enabled
 
     def get_lti_advantage_deep_linking_launch_url(self):
         """
@@ -384,7 +365,8 @@ def get_lti_advantage_deep_linking_launch_url(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_deep_linking_launch_url
         else:
-            return self.block.lti_advantage_deep_linking_launch_url
+            block = compat.load_enough_xblock(self.location)
+            return block.lti_advantage_deep_linking_launch_url
 
     def get_lti_advantage_nrps_enabled(self):
         """
@@ -396,7 +378,8 @@ def get_lti_advantage_nrps_enabled(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_enable_nrps
         else:
-            return self.block.lti_1p3_enable_nrps
+            block = compat.load_enough_xblock(self.location)
+            return block.lti_1p3_enable_nrps
 
     def _setup_lti_1p3_ags(self, consumer):
         """
@@ -419,21 +402,21 @@ def _setup_lti_1p3_ags(self, consumer):
         # and manage lineitems using the AGS endpoints.
         if not lineitem and lti_advantage_ags_mode == self.LTI_ADVANTAGE_AGS_DECLARATIVE:
             try:
-                block = self.block
+                block = compat.load_enough_xblock(self.location)
             except ValueError:  # There is no location to load the block
                 block = None
 
             if block:
                 default_values = {
                     'resource_id': self.location,
-                    'score_maximum': self.block.weight,
-                    'label': self.block.display_name,
+                    'score_maximum': block.weight,
+                    'label': block.display_name,
                 }
-                if hasattr(self.block, 'start'):
-                    default_values['start_date_time'] = self.block.start
+                if hasattr(block, 'start'):
+                    default_values['start_date_time'] = block.start
 
-                if hasattr(self.block, 'due'):
-                    default_values['end_date_time'] = self.block.due
+                if hasattr(block, 'due'):
+                    default_values['end_date_time'] = block.due
             else:
                 # TODO find a way to make these defaults more sensible
                 default_values = {
@@ -488,10 +471,11 @@ def _get_lti_1p3_consumer(self):
         look for the configuration and instance the class.
         """
         if self.config_store == self.CONFIG_ON_XBLOCK:
+            block = compat.load_enough_xblock(self.location)
             consumer = LtiAdvantageConsumer(
                 iss=get_lms_base(),
-                lti_oidc_url=self.block.lti_1p3_oidc_url,
-                lti_launch_url=self.block.lti_1p3_launch_url,
+                lti_oidc_url=block.lti_1p3_oidc_url,
+                lti_launch_url=block.lti_1p3_launch_url,
                 client_id=self.lti_1p3_client_id,
                 # Deployment ID hardcoded to 1 since
                 # we're not using multi-tenancy.
@@ -500,8 +484,8 @@ def _get_lti_1p3_consumer(self):
                 rsa_key=self.lti_1p3_private_key,
                 rsa_key_id=self.lti_1p3_private_key_id,
                 # LTI 1.3 Tool key/keyset url
-                tool_key=self.block.lti_1p3_tool_public_key,
-                tool_keyset_url=self.block.lti_1p3_tool_keyset_url,
+                tool_key=block.lti_1p3_tool_public_key,
+                tool_keyset_url=block.lti_1p3_tool_keyset_url,
             )
         elif self.config_store == self.CONFIG_ON_DB:
             consumer = LtiAdvantageConsumer(

lti_consumer/plugin/compat.py

@@ -60,18 +60,30 @@ def get_database_config_waffle_flag():
     return CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.{ENABLE_DATABASE_CONFIG}', __name__)
 
 
+def load_enough_xblock(location):  # pragma: nocover
+    """
+    Load enough of an xblock to read from for LTI values stored on the block.
+    The block may or may not be bound to the user for actual use depending on
+    what has happened in the request so far.
+    """
+    # pylint: disable=import-error,import-outside-toplevel
+    from xmodule.modulestore.django import modulestore
+
+    # Retrieve descriptor from modulestore
+    return modulestore().get_item(location)
+
+
 def load_block_as_user(location):  # pragma: nocover
     """
     Load a block as the current user, or load as the anonymous user if no user is available.
     """
     # pylint: disable=import-error,import-outside-toplevel
     from crum import get_current_user, get_current_request
-    from xmodule.modulestore.django import modulestore
     from lms.djangoapps.courseware.module_render import get_module_for_descriptor_internal
     from openedx.core.lib.xblock_utils import request_token
 
     # Retrieve descriptor from modulestore
-    descriptor = modulestore().get_item(location)
+    descriptor = load_enough_xblock(location)
     user = get_current_user()
     request = get_current_request()
     if user and request:

lti_consumer/plugin/views.py

@@ -480,7 +480,8 @@ def deep_linking_content_endpoint(request, lti_config_id):
         raise Http404 from exc
 
     # check if user has proper access
-    if not has_block_access(request.user, lti_config.block, lti_config.location.course_key):
+    block = compat.load_block_as_user(lti_config.location)
+    if not has_block_access(request.user, block, lti_config.location.course_key):
         log.warning(
             "Permission on LTI Config %r denied for user %r.",
             lti_config_id,
@@ -499,7 +500,7 @@ def deep_linking_content_endpoint(request, lti_config_id):
     # Render LTI-DL contents
     return render(request, 'html/lti-dl/render_dl_content.html', {
         'content_items': content_items,
-        'block': lti_config.block,
+        'block': block,
         'launch_data': launch_data,
     })
 

lti_consumer/tests/unit/plugin/test_views.py

@@ -133,19 +133,21 @@ def setUp(self):
         )
         self.launch_data_key = cache_lti_1p3_launch_data(self.launch_data)
 
-        self.compat_patcher = patch("lti_consumer.plugin.views.compat")
-        self.compat = self.compat_patcher.start()
-        self.addCleanup(self.compat.stop)
+        compat_patcher = patch("lti_consumer.plugin.views.compat")
+        self.addCleanup(compat_patcher.stop)
+        self.compat = compat_patcher.start()
         course = Mock(name="course")
         course.display_name_with_default = "course_display_name"
         course.display_org_with_default = "course_display_org"
         self.compat.get_course_by_id.return_value = course
         self.compat.get_user_role.return_value = "student"
         self.compat.get_external_id_for_user.return_value = "12345"
+
         model_compat_patcher = patch("lti_consumer.models.compat")
+        self.addCleanup(model_compat_patcher.stop)
         model_compat = model_compat_patcher.start()
+        model_compat.load_enough_xblock.return_value = self.xblock
         model_compat.load_block_as_user.return_value = self.xblock
-        self.addCleanup(model_compat_patcher.stop)
 
     def test_invalid_lti_version(self):
         """

lti_consumer/tests/unit/plugin/test_views_lti_ags.py

@@ -3,7 +3,7 @@
 """
 import json
 from datetime import timedelta
-from unittest.mock import patch, PropertyMock, Mock
+from unittest.mock import patch, Mock
 
 from Cryptodome.PublicKey import RSA
 import ddt
@@ -54,17 +54,14 @@ def setUp(self):
             location=self.xblock.location,  # pylint: disable=no-member
             version=LtiConfiguration.LTI_1P3,
         )
-        # Preload XBlock to avoid calls to modulestore
-        self.lti_config.block = self.xblock
 
         # Patch internal method to avoid calls to modulestore
         patcher = patch(
-            'lti_consumer.models.LtiConfiguration.block',
-            new_callable=PropertyMock,
-            return_value=self.xblock
+            'lti_consumer.plugin.compat.load_enough_xblock',
         )
         self.addCleanup(patcher.stop)
-        self._lti_block_patch = patcher.start()
+        self._load_block_patch = patcher.start()
+        self._load_block_patch.return_value = self.xblock
 
         self._mock_user = Mock()
         compat_mock = patch("lti_consumer.signals.compat")

lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py

@@ -1,7 +1,7 @@
 """
 Tests for LTI Advantage Assignments and Grades Service views.
 """
-from unittest.mock import patch, PropertyMock, Mock
+from unittest.mock import patch, Mock
 
 import re
 import ddt
@@ -63,24 +63,27 @@ def setUp(self):
         for key, value in self.xblock_attributes.items():
             setattr(self.xblock, key, value)
 
-        # Preload XBlock to avoid calls to modulestore
-        self.lti_config.block = self.xblock
+        # Patch internal methods to avoid calls to modulestore
+        enough_mock = patch(
+            'lti_consumer.plugin.compat.load_enough_xblock',
+        )
+        self.addCleanup(enough_mock.stop)
+        self._load_block_patch = enough_mock.start()
+        self._load_block_patch.return_value = self.xblock
 
-        # Patch internal method to avoid calls to modulestore
-        patcher = patch(
-            'lti_consumer.models.LtiConfiguration.block',
-            new_callable=PropertyMock,
-            return_value=self.xblock
+        # some deep linking endpoints still load the xblock as its user for access check
+        as_user_mock = patch(
+            'lti_consumer.plugin.compat.load_block_as_user',
         )
-        self.addCleanup(patcher.stop)
-        self._lti_block_patch = patcher.start()
+        self.addCleanup(as_user_mock.stop)
+        self._load_block_as_user_patch = as_user_mock.start()
+        self._load_block_as_user_patch.return_value = self.xblock
 
         self._mock_user = 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 = self._mock_user
-        self._compat_mock.load_block_as_user.return_value = self.xblock
+        get_user_mock = patch("lti_consumer.plugin.compat.get_user_from_external_user_id")
+        self.addCleanup(get_user_mock.stop)
+        self._get_user_patch = get_user_mock.start()
+        self._get_user_patch.return_value = self._mock_user
 
 
 @ddt.ddt
@@ -485,7 +488,7 @@ def test_dl_contents(self, has_block_access_patcher):  # pylint: disable=unused-
 
         resp = self.client.get(self.url)
         self.assertEqual(resp.status_code, 200)
-        expected_title = '{} | Deep Linking Contents'.format(self.lti_config.block.display_name)
+        expected_title = '{} | Deep Linking Contents'.format(self.xblock.display_name)
         self.assertContains(resp, expected_title)
 
     @ddt.data(

lti_consumer/tests/unit/plugin/test_views_lti_nrps.py

@@ -1,7 +1,7 @@
 """
 Tests for LTI Names and Role Provisioning Service views.
 """
-from unittest.mock import Mock, patch, PropertyMock
+from unittest.mock import Mock, patch
 from Cryptodome.PublicKey import RSA
 from jwkest.jwk import RSAKey
 from rest_framework.test import APITransactionTestCase
@@ -140,17 +140,14 @@ def setUp(self):
             location=self.xblock.location,  # pylint: disable=no-member
             version=LtiConfiguration.LTI_1P3,
         )
-        # Preload XBlock to avoid calls to modulestore
-        self.lti_config.block = self.xblock
 
         # Patch internal method to avoid calls to modulestore
         patcher = patch(
-            'lti_consumer.models.LtiConfiguration.block',
-            new_callable=PropertyMock,
-            return_value=self.xblock
+            'lti_consumer.plugin.compat.load_enough_xblock',
         )
         self.addCleanup(patcher.stop)
-        self._lti_block_patch = patcher.start()
+        self._load_block_patch = patcher.start()
+        self._load_block_patch.return_value = self.xblock
 
         self.context_membership_endpoint = reverse(
             'lti_consumer:lti-nrps-memberships-view-list',

lti_consumer/tests/unit/test_api.py

@@ -41,7 +41,7 @@ def setUp(self):
 
         # Patch compat method to avoid calls to modulestore
         patcher = patch(
-            'lti_consumer.plugin.compat.load_block_as_user',
+            'lti_consumer.plugin.compat.load_enough_xblock',
         )
         self.addCleanup(patcher.stop)
         self._load_block_patch = patcher.start()
@@ -72,7 +72,6 @@ def _setup_lti_block(self):
         self.lti_config = LtiConfiguration.objects.create(
             config_id=_test_config_id,
             location=self.xblock.location,  # pylint: disable=no-member
-            block=self.xblock,
             version=LtiConfiguration.LTI_1P3,
             config_store=LtiConfiguration.CONFIG_ON_XBLOCK,
         )