fix: Apply suggestions

fix: Improve LtiConfiguration clean method test

docs/reusable_configuration.rst

@@ -0,0 +1,80 @@
+##########################
+LTI Reusable configuration
+##########################
+
+Currently, this library supports a mechanism that allows LTI configuration
+pluggability and re-usability, this allows instructors to be able to re-use
+LTI configuration values across multiple XBlocks, reducing the work a
+instructor needs to do to set up an LTI consumer XBlock. This feature works
+for both LTI 1.1 and LTI 1.3. This feature, in the case of LTI 1.3 greatly
+reduces the work an instructor needs to dedicate to setting up multiple
+XBlocks that use the same configuration, since all values, including the access
+token and keyset URL, are shared across all XBlocks using the same
+configuration, eliminating the need to have a tool deployment for each XBlock.
+
+***********************
+How to use this feature
+***********************
+
+Setup Openedx LTI Store
+=======================
+
+1. Install the openedx-ltistore plugin on the LMS and studio
+   (https://github.com/open-craft/openedx-ltistore):
+
+.. code-block:: bash
+
+    make lms-shell
+    pip install -e /edx/src/openedx-ltistore
+
+2. Setup any existing openedx-filters configurations for both LMS and studio:
+
+.. code-block:: python
+
+    OPEN_EDX_FILTERS_CONFIG = {
+        "org.openedx.xblock.lti_consumer.configuration.listed.v1": {
+            "fail_silently": False,
+            "pipeline": [
+                "lti_store.pipelines.GetLtiConfigurations"
+            ]
+        }
+    }
+
+3. Restart the LMS & Studio for the latest config to take effect.
+
+Setup course waffle flag
+========================
+
+1. Go to LMS admin > WAFFLE_UTILS > Waffle flag course override
+   (http://localhost:18010/admin/waffle_utils/waffleflagcourseoverridemodel/).
+2. Create a waffle flag course override with these values:
+    - Waffle flag: lti_consumer.enable_external_config_filter
+    - Course id: <your course id>
+    - Override choice: Force On
+    - Enabled: True
+
+Create reusable LTI configuration
+=================================
+
+1. Go to LMS admin > LTI_STORE > External lti configurations
+   (http://localhost:18010/admin/lti_store/externallticonfiguration/).
+2. Create a new external LTI configuration.
+3. On the list of external LTI configurations, note down the "Filter Key" value
+   of the newly created configuration (Example: lti_store:1).
+
+Setup LTI consumer XBlock on studio
+===================================
+
+1. Add "lti_consumer" to the "Advanced Module List" in
+   the "Advanced Settings" of your course.
+2. Add a new unit to the course and add "LTI Consumer"
+   from the "Advanced" blocks.
+3. Click the "Edit" button of the LTI Consumer block
+   and set "Configuration Type" to "Reusable Configuration".
+4. Set the "External configuration ID" to the filter key value we copied
+   when we created the LTI configuration (Example: lti_store:1).
+5. Click "Save".
+6. (Optional) On an LTI 1.3 consumer XBlock, note down all the values
+   you need to set up on the LTI tool.
+6. Go to your live course and execute a launch.
+7. That launch should work as expected.

lti_consumer/lti_xblock.py

@@ -82,6 +82,7 @@
     external_config_filter_enabled,
     external_user_id_1p1_launches_enabled,
     database_config_enabled,
+    EXTERNAL_ID_REGEX,
 )
 
 log = logging.getLogger(__name__)
@@ -107,8 +108,6 @@
 )
 # Catch a value enclosed by ${}, the value enclosed can contain any charater except "=".
 CUSTOM_PARAMETER_TEMPLATE_REGEX = re.compile(r'^(\${[^%s]+})$' % CUSTOM_PARAMETER_SEPARATOR)
-SLUG_CHARACTER_CLASS = '[-a-zA-Z0-9_]'
-EXTERNAL_ID_REGEX = re.compile(rf'^({SLUG_CHARACTER_CLASS}+:{SLUG_CHARACTER_CLASS}+)$')
 
 
 def parse_handler_suffix(suffix):
@@ -693,18 +692,14 @@ def validate_field_data(self, validation, data):
                 _('Custom Parameters should be strings in "x=y" format.'),
             )))
 
-        # Validate external config ID is not missing.
-        if data.config_type == 'external' and not data.external_config:
+        # Validate the external config ID.
+        if (
+            data.config_type == 'external' and not
+            (data.external_config and EXTERNAL_ID_REGEX.match(str(data.external_config)))
+        ):
             _ = self.runtime.service(self, 'i18n').ugettext
             validation.add(ValidationMessage(ValidationMessage.ERROR, str(
-                _('Reusable configuration ID must be set when using external config.'),
-            )))
-
-        # Validate external config ID format.
-        if data.config_type == 'external' and not EXTERNAL_ID_REGEX.match(str(data.external_config)):
-            _ = self.runtime.service(self, 'i18n').ugettext
-            validation.add(ValidationMessage(ValidationMessage.ERROR, str(
-                _('Reusable configuration ID should be a string in "x:y" format.'),
+                _('Reusable configuration ID must be set when using external config (Example: "x:y").'),
             )))
 
         # keyset URL and public key are mutually exclusive

lti_consumer/models.py

@@ -15,6 +15,7 @@
 from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField
 from opaque_keys.edx.keys import CourseKey
 from config_models.models import ConfigurationModel
+from django.utils.functional import cached_property
 from django.utils.translation import gettext_lazy as _
 from lti_consumer.filters import get_external_config_from_filter
 
@@ -31,6 +32,7 @@
     get_lti_nrps_context_membership_url,
     choose_lti_1p3_redirect_uris,
     model_to_dict,
+    EXTERNAL_ID_REGEX,
 )
 
 log = logging.getLogger(__name__)
@@ -251,9 +253,11 @@ def clean(self):
             raise ValidationError({
                 "config_store": _("LTI Configuration stores on XBlock needs a block location set."),
             })
-        if self.config_store == self.CONFIG_EXTERNAL and self.external_id is None:
+        if self.config_store == self.CONFIG_EXTERNAL and not EXTERNAL_ID_REGEX.match(str(self.external_id)):
             raise ValidationError({
-                "config_store": _("LTI Configuration using reusable configuration needs a external ID set."),
+                "config_store": _(
+                    'LTI Configuration using reusable configuration needs a external ID in "x:y" format.',
+                ),
             })
         if self.version == self.LTI_1P3 and self.config_store == self.CONFIG_ON_DB:
             if self.lti_1p3_tool_public_key == "" and self.lti_1p3_tool_keyset_url == "":
@@ -368,6 +372,13 @@ def lti_1p3_public_jwk(self):
         self._generate_lti_1p3_keys_if_missing()
         return json.loads(self.lti_1p3_internal_public_jwk)
 
+    @cached_property
+    def external_config(self):
+        """
+        Return the external resuable configuration.
+        """
+        return get_external_config_from_filter({}, self.external_id)
+
     def _get_lti_1p1_consumer(self):
         """
         Return a class of LTI 1.1 consumer.
@@ -378,10 +389,9 @@ def _get_lti_1p1_consumer(self):
             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")
-            secret = config.get("lti_1p1_client_secret")
-            launch_url = config.get("lti_1p1_launch_url")
+            key = self.external_config.get("lti_1p1_client_key")
+            secret = self.external_config.get("lti_1p1_client_secret")
+            launch_url = self.external_config.get("lti_1p1_launch_url")
         else:
             key = self.lti_1p1_client_key
             secret = self.lti_1p1_client_secret
@@ -396,8 +406,7 @@ def get_lti_advantage_ags_mode(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_ags_mode
         elif self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-            return config.get('lti_advantage_ags_mode')
+            return self.external_config.get('lti_advantage_ags_mode')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_advantage_ags_mode
@@ -409,8 +418,7 @@ def get_lti_advantage_deep_linking_enabled(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_deep_linking_enabled
         elif self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-            return config.get('lti_advantage_deep_linking_enabled')
+            return self.external_config.get('lti_advantage_deep_linking_enabled')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_advantage_deep_linking_enabled
@@ -422,8 +430,7 @@ 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
         elif self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-            return config.get('lti_advantage_deep_linking_launch_url')
+            return self.external_config.get('lti_advantage_deep_linking_launch_url')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_advantage_deep_linking_launch_url
@@ -435,8 +442,7 @@ def get_lti_advantage_nrps_enabled(self):
         if self.config_store == self.CONFIG_ON_DB:
             return self.lti_advantage_enable_nrps
         elif self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-            return config.get('lti_advantage_enable_nrps')
+            return self.external_config.get('lti_advantage_enable_nrps')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_1p3_enable_nrps
@@ -578,22 +584,20 @@ def _get_lti_1p3_consumer(self):
                 tool_keyset_url=self.lti_1p3_tool_keyset_url,
             )
         elif self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-
             consumer = consumer_class(
                 iss=get_lti_api_base(),
-                lti_oidc_url=config.get('lti_1p3_oidc_url'),
-                lti_launch_url=config.get('lti_1p3_launch_url'),
-                client_id=config.get('lti_1p3_client_id'),
+                lti_oidc_url=self.external_config.get('lti_1p3_oidc_url'),
+                lti_launch_url=self.external_config.get('lti_1p3_launch_url'),
+                client_id=self.external_config.get('lti_1p3_client_id'),
                 # Deployment ID hardcoded to 1 since
                 # we're not using multi-tenancy.
                 deployment_id='1',
-                rsa_key=config.get('lti_1p3_private_key'),
-                rsa_key_id=config.get('lti_1p3_private_key_id'),
+                rsa_key=self.external_config.get('lti_1p3_private_key'),
+                rsa_key_id=self.external_config.get('lti_1p3_private_key_id'),
                 # Registered redirect uris
                 redirect_uris=self.get_lti_1p3_redirect_uris(),
-                tool_key=config.get('lti_1p3_tool_public_key'),
-                tool_keyset_url=config.get('lti_1p3_tool_keyset_url'),
+                tool_key=self.external_config.get('lti_1p3_tool_public_key'),
+                tool_keyset_url=self.external_config.get('lti_1p3_tool_keyset_url'),
             )
         else:
             # This should not occur, but raise an error if self.config_store is not
@@ -621,10 +625,9 @@ def get_lti_1p3_redirect_uris(self):
         Return pre-registered redirect uris or sensible defaults
         """
         if self.config_store == self.CONFIG_EXTERNAL:
-            config = get_external_config_from_filter({}, self.external_id)
-            redirect_uris = config.get('lti_1p3_redirect_uris')
-            launch_url = config.get('lti_1p3_launch_url')
-            deep_link_launch_url = config.get('lti_advantage_deep_linking_launch_url')
+            redirect_uris = self.external_config.get('lti_1p3_redirect_uris')
+            launch_url = self.external_config.get('lti_1p3_launch_url')
+            deep_link_launch_url = self.external_config.get('lti_advantage_deep_linking_launch_url')
         elif self.config_store == self.CONFIG_ON_DB:
             redirect_uris = self.lti_1p3_redirect_uris
             launch_url = self.lti_1p3_launch_url

lti_consumer/tests/unit/test_lti_xblock.py

@@ -6,7 +6,7 @@
 import string
 from datetime import timedelta
 from itertools import product
-from unittest.mock import Mock, PropertyMock, patch, call
+from unittest.mock import Mock, PropertyMock, patch
 
 import ddt
 from Cryptodome.PublicKey import RSA
@@ -220,16 +220,12 @@ def test_validate_empty_external_config_with_external_config_type(
 
         self.xblock.validate()
 
-        add_mock.assert_has_calls([
-            call(mock_validation_message(
-                'error',
-                'Reusable configuration ID must be set when using external config.',
-            )),
-            call(mock_validation_message(
+        add_mock.assert_called_once_with(
+            mock_validation_message(
                 'error',
-                'Reusable configuration ID should be a string in "x:y" format.',
-            )),
-        ])
+                'Reusable configuration ID must be set when using external config (Example: "x:y").',
+            ),
+        )
 
     @ddt.data('apptest', 'app:', ':test', 'app::test', f'{string.punctuation}:{string.punctuation}')
     @patch('lti_consumer.lti_xblock.ValidationMessage')
@@ -250,7 +246,7 @@ def test_validate_invalid_external_config_with_external_config_type(
         add_mock.assert_called_once_with(
             mock_validation_message(
                 'error',
-                'Reusable configuration ID should be a string in "x:y" format.',
+                'Reusable configuration ID must be set when using external config (Example: "x:y").',
             ),
         )
 

lti_consumer/tests/unit/test_models.py

@@ -374,6 +374,11 @@ def test_clean(self):
         self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_EXTERNAL
         self.lti_1p3_config.external_id = None
 
+        with self.assertRaises(ValidationError):
+            self.lti_1p3_config.clean()
+
+        self.lti_1p3_config.external_id = 'invalid-external-id'
+
         with self.assertRaises(ValidationError):
             self.lti_1p3_config.clean()
 

lti_consumer/utils.py

@@ -3,6 +3,7 @@
 """
 import copy
 import logging
+import re
 from importlib import import_module
 from urllib.parse import urlencode
 
@@ -19,6 +20,9 @@
 
 log = logging.getLogger(__name__)
 
+SLUG_CHARACTER_CLASS = '[-a-zA-Z0-9_]'
+EXTERNAL_ID_REGEX = re.compile(rf'^({SLUG_CHARACTER_CLASS}+:{SLUG_CHARACTER_CLASS}+)$')
+
 
 def _(text):
     """