feat: LTI 1.3 reusable configuration

lti_consumer/api.py

@@ -142,24 +142,21 @@ def get_lti_1p3_launch_info(
 
     config_id = lti_config.config_id
     client_id = lti_config.lti_1p3_client_id
-    token_url = get_lms_lti_access_token_link(config_id)
-    keyset_url = get_lms_lti_keyset_link(config_id)
 
     # Display LTI launch information from external configuration.
     # if an external configuration is being used.
     if lti_config.config_store == lti_config.CONFIG_EXTERNAL:
         external_config = get_external_config_from_filter({}, lti_config.external_id)
+        config_id = lti_config.external_id.replace(':', '/')
         client_id = external_config.get('lti_1p3_client_id')
-        token_url = get_lms_lti_access_token_link(lti_config.external_id.replace(':', '/'))
-        keyset_url = get_lms_lti_keyset_link(lti_config.external_id.replace(':', '/'))
 
     # Return LTI launch information for end user configuration
     return {
         'client_id': client_id,
-        'keyset_url': keyset_url,
+        'keyset_url': get_lms_lti_keyset_link(config_id),
         'deployment_id': '1',
         'oidc_callback': get_lms_lti_launch_link(),
-        'token_url': token_url,
+        'token_url': get_lms_lti_access_token_link(config_id),
         'deep_linking_launch_url': deep_linking_launch_url,
         'deep_linking_content_items':
             json.dumps(deep_linking_content_items, indent=4) if deep_linking_content_items else None,

lti_consumer/exceptions.py

@@ -7,3 +7,10 @@ class LtiError(Exception):
     """
     General error class for LTI Consumer usage.
     """
+
+
+class ExternalConfigurationNotFound(Exception):
+    """
+    This exception is used  when a reusable external configuration
+    is not found for a given external ID.
+    """

lti_consumer/lti_xblock.py

@@ -107,6 +107,8 @@
 )
 # 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):
@@ -698,6 +700,13 @@ def validate_field_data(self, validation, data):
                 _('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.'),
+            )))
+
         # keyset URL and public key are mutually exclusive
         if data.lti_1p3_tool_key_mode == 'keyset_url':
             data.lti_1p3_tool_public_key = ''

lti_consumer/models.py

@@ -251,6 +251,10 @@ 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:
+            raise ValidationError({
+                "config_store": _("LTI Configuration using reusable configuration needs a external ID set."),
+            })
         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 == "":
                 raise ValidationError({
@@ -391,6 +395,9 @@ 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')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_advantage_ags_mode
@@ -401,6 +408,9 @@ 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')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_advantage_deep_linking_enabled
@@ -424,6 +434,9 @@ 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')
         else:
             block = compat.load_enough_xblock(self.location)
             return block.lti_1p3_enable_nrps

lti_consumer/plugin/urls.py

@@ -31,8 +31,10 @@
         public_keyset_endpoint,
         name='lti_consumer.public_keyset_endpoint_via_location'
     ),
+    # The external ID is split into slashes to make the URL more readable
+    # and avoid clashing with USAGE_ID_PATTERN.
     path(
-        'lti_consumer/v1/public_keysets/<str:external_app>/<slug:external_slug>',
+        'lti_consumer/v1/public_keysets/<slug:external_app>/<slug:external_slug>',
         public_keyset_endpoint,
         name='lti_consumer.public_keyset_endpoint_via_external_id'
     ),
@@ -51,8 +53,10 @@
         access_token_endpoint,
         name='lti_consumer.access_token_via_location'
     ),
+    # The external ID is split into slashes to make the URL more readable
+    # and avoid clashing with USAGE_ID_PATTERN.
     path(
-        'lti_consumer/v1/token/<str:external_app>/<slug:external_slug>',
+        'lti_consumer/v1/token/<slug:external_app>/<slug:external_slug>',
         access_token_endpoint,
         name='lti_consumer.access_token_via_external_id'
     ),

lti_consumer/plugin/views.py

@@ -25,7 +25,7 @@
 from rest_framework.status import HTTP_200_OK, HTTP_400_BAD_REQUEST, HTTP_403_FORBIDDEN, HTTP_404_NOT_FOUND
 
 from lti_consumer.api import get_lti_pii_sharing_state_for_course, validate_lti_1p3_launch_data
-from lti_consumer.exceptions import LtiError
+from lti_consumer.exceptions import LtiError, ExternalConfigurationNotFound
 from lti_consumer.lti_1p3.consumer import LtiConsumer1p3, LtiProctoringConsumer
 from lti_consumer.lti_1p3.exceptions import (BadJwtSignature, InvalidClaimValue, Lti1p3Exception,
                                              LtiDeepLinkingContentTypeNotSupported, MalformedJwtToken,
@@ -122,7 +122,7 @@ def public_keyset_endpoint(
             lti_config = get_external_config_from_filter({}, external_id)
 
             if not lti_config:
-                raise ValueError("External LTI configuration not found")
+                raise ExternalConfigurationNotFound("External LTI configuration not found")
 
             version = lti_config.get("version")
             public_jwk = lti_config.get("lti_1p3_public_jwk", {})
@@ -138,7 +138,7 @@ def public_keyset_endpoint(
         response = JsonResponse(public_jwk)
         response['Content-Disposition'] = 'attachment; filename=keyset.json'
         return response
-    except (InvalidKeyError, LtiConfiguration.DoesNotExist, ValueError, LtiError) as exc:
+    except (InvalidKeyError, LtiConfiguration.DoesNotExist, ExternalConfigurationNotFound, LtiError) as exc:
         log.info(
             "Error while retrieving keyset for ID %s: %s",
             usage_id or lti_config_id or external_id,
@@ -431,7 +431,7 @@ def access_token_endpoint(
             lti_config = get_external_config_from_filter({}, external_id)
 
             if not lti_config:
-                raise ValueError("External LTI configuration not found")
+                raise ExternalConfigurationNotFound("External LTI configuration not found")
 
             version = lti_config.get("version")
             # External LTI configurations don't have a get_lti_consumer method
@@ -448,7 +448,7 @@ def access_token_endpoint(
                 tool_key=lti_config.get("lti_1p3_tool_public_key"),
                 tool_keyset_url=lti_config.get("lti_1p3_tool_keyset_url"),
             )
-    except (InvalidKeyError, LtiConfiguration.DoesNotExist, ValueError) as exc:
+    except (InvalidKeyError, LtiConfiguration.DoesNotExist, ExternalConfigurationNotFound) as exc:
         log.info(
             "Error while retrieving access token for ID %s: %s",
             usage_id or lti_config_id or external_id,

lti_consumer/static/js/xblock_studio_view.js

@@ -72,19 +72,12 @@ function LtiConsumerXBlockInitStudio(runtime, element) {
      *
      *  new - Show all the LTI 1.1/1.3 config fields
      *  database - Do not show the LTI 1.1/1.3 config fields
-     *  external - Show all LTI 1.3 config fields except LTI 1.3 tool fields
+     *  external - Show only the External Config ID field
      */
     function getFieldsToHideForLtiConfigType() {
         const configType = $(element).find('#xb-field-edit-config_type').val();
         const databaseConfigHiddenFields = lti1P1FieldList.concat(lti1P3FieldList);
-        const externalConfigHiddenFields = lti1P1FieldList.concat([
-            'lti_1p3_oidc_url',
-            'lti_1p3_launch_url',
-            'lti_1p3_tool_key_mode',
-            'lti_1p3_tool_keyset_url',
-            'lti_1p3_tool_public_key',
-            'lti_advantage_deep_linking_launch_url',
-        ]);
+        const externalConfigHiddenFields = lti1P1FieldList.concat(lti1P3FieldList);
         const fieldsToHide = [];
 
         if (configType === "external") {

lti_consumer/tests/unit/plugin/test_views.py

@@ -81,7 +81,7 @@ def test_invalid_usage_key(self):
         ('lti_consumer:lti_consumer.public_keyset_endpoint_via_external_id', ['x', 'x']),
     )
     @ddt.unpack
-    def test_non_existant_configuration(self, url, args):
+    def test_nonexistent_configuration(self, url, args):
         """
         Check that 404 is returned when there is no configuration found.
         """
@@ -206,7 +206,7 @@ def test_invalid_lti_version(self):
         self.config.version = LtiConfiguration.LTI_1P3
         self.config.save()
 
-    def test_non_existant_lti_config(self):
+    def test_nonexistent_lti_config(self):
         """
         Check that a 404 is returned when LtiConfiguration for a location doesn't exist
         """
@@ -775,7 +775,7 @@ def test_access_token_endpoint_with_external_id_in_url(
         ('lti_consumer:lti_consumer.access_token_via_external_id', ['x', 'x']),
     )
     @ddt.unpack
-    def test_non_existant_configuration(self, url, args):
+    def test_nonexistent_configuration(self, url, args):
         """
         Check that 404 is returned when there is no configuration found.
         """

lti_consumer/tests/unit/test_lti_xblock.py

@@ -1,12 +1,12 @@
 """
 Unit tests for LtiConsumerXBlock
 """
-
 import json
 import logging
+import string
 from datetime import timedelta
 from itertools import product
-from unittest.mock import Mock, PropertyMock, patch
+from unittest.mock import Mock, PropertyMock, patch, call
 
 import ddt
 from Cryptodome.PublicKey import RSA
@@ -185,8 +185,9 @@ def test_validate_with_invalid_custom_parameters(self, custom_parameters, add_mo
 
     def test_validate_external_config_with_external_config_type(self):
         """Test external config ID with external config type."""
+        slug = f'{string.digits}{string.ascii_letters}_-'
         self.xblock.config_type = 'external'
-        self.xblock.external_config = '1'
+        self.xblock.external_config = f'{slug}:{slug}'
 
         validation = self.xblock.validate()
 
@@ -219,10 +220,37 @@ 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(
+                'error',
+                'Reusable configuration ID should be a string in "x:y" format.',
+            )),
+        ])
+
+    @ddt.data('apptest', 'app:', ':test', 'app::test', f'{string.punctuation}:{string.punctuation}')
+    @patch('lti_consumer.lti_xblock.ValidationMessage')
+    @patch.object(Validation, 'add')
+    def test_validate_invalid_external_config_with_external_config_type(
+        self,
+        external_id,
+        add_mock,
+        mock_validation_message,
+    ):
+        """Test with invalid external config ID using an external config type."""
+        mock_validation_message.ERROR.return_value = 'error'
+        self.xblock.config_type = 'external'
+        self.xblock.external_config = external_id
+
+        self.xblock.validate()
+
         add_mock.assert_called_once_with(
             mock_validation_message(
                 'error',
-                'Reusable configuration ID must be set when using external config.',
+                'Reusable configuration ID should be a string in "x:y" format.',
             ),
         )