From a28dcc1e1b88e0013d7a3dec7594df8fc1cf00f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Mon, 10 Jul 2023 16:03:31 -0400 Subject: [PATCH 1/8] feat: LTI 1.3 reusable configuration fix: Improve external config fields validation fix: Improve hide/show fields toggle Co-authored-by: Squirrel18 Co-authored-by: alexjmpb --- lti_consumer/api.py | 26 ++++- lti_consumer/lti_xblock.py | 18 +-- .../migrations/0018_add_ags_multi_tenancy.py | 28 +++++ lti_consumer/models.py | 104 +++++++++++++----- lti_consumer/plugin/views.py | 20 ++-- lti_consumer/static/js/xblock_studio_view.js | 20 ++-- .../tests/unit/plugin/test_views_lti_ags.py | 13 +++ lti_consumer/tests/unit/test_api.py | 48 ++++++++ lti_consumer/tests/unit/test_lti_xblock.py | 45 ++++++++ lti_consumer/tests/unit/test_models.py | 96 ++++++++++------ 10 files changed, 329 insertions(+), 89 deletions(-) create mode 100644 lti_consumer/migrations/0018_add_ags_multi_tenancy.py diff --git a/lti_consumer/api.py b/lti_consumer/api.py index dc1b29d2..0eb8c4c8 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -141,14 +141,32 @@ def get_lti_1p3_launch_info( deep_linking_content_items = [item.attributes for item in dl_content_items] 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) + # We set the deployment ID to a default value of 1, + # this will be used on a configuration with a CONFIG_EXTERNAL config store + # if no deployment ID is set on the external configuration. + deployment_id = '1' + + # 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) + client_id = external_config.get('lti_1p3_client_id') + token_url = external_config.get('lti_1p3_access_token_url') + keyset_url = external_config.get('lti_1p3_keyset_url') + # Show default harcoded deployment ID if no deployment ID + # is set on the external configuration. + deployment_id = external_config.get('lti_1p3_deployment_id') or deployment_id # Return LTI launch information for end user configuration return { - 'client_id': lti_config.lti_1p3_client_id, - 'keyset_url': get_lms_lti_keyset_link(config_id), - 'deployment_id': '1', + 'client_id': client_id, + 'keyset_url': keyset_url, + 'deployment_id': deployment_id, 'oidc_callback': get_lms_lti_launch_link(), - 'token_url': get_lms_lti_access_token_link(config_id), + 'token_url': token_url, '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, diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 5b04de70..a732e055 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -677,6 +677,13 @@ def validate_field_data(self, validation, data): _("Custom Parameters must be a list") ))) + # Validate external config ID is not missing. + if data.config_type == 'external' and not 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.'), + ))) + # keyset URL and public key are mutually exclusive if data.lti_1p3_tool_key_mode == 'keyset_url': data.lti_1p3_tool_public_key = '' @@ -1614,10 +1621,7 @@ def _get_lti_block_launch_handler(self): """ Return the LTI block launch handler. """ - # The "external" config_type is not supported for LTI 1.3, only LTI 1.1. Therefore, ensure that we define - # the lti_block_launch_handler using LTI 1.1 logic for "external" config_types. - # TODO: This needs to change when the LTI 1.3 support is added to the external config_type in the future. - if self.lti_version == 'lti_1p1' or self.config_type == "external": + if self.lti_version == 'lti_1p1': lti_block_launch_handler = self.runtime.handler_url(self, 'lti_launch_handler').rstrip('/?') else: launch_data = self.get_lti_1p3_launch_data() @@ -1637,10 +1641,8 @@ def _get_lti_1p3_launch_url(self, consumer): """ lti_1p3_launch_url = self.lti_1p3_launch_url.strip() - # The "external" config_type is not supported for LTI 1.3, only LTI 1.1. Therefore, ensure that we define - # the lti_1p3_launch_url using the LTI 1.3 consumer only for config_types that support LTI 1.3. - # TODO: This needs to change when the LTI 1.3 support is added to the external config_type in the future. - if consumer and self.lti_version == "lti_1p3" and self.config_type == "database": + # Get LTI launch URL from consumer if using database or external configuration type. + if consumer and self.lti_version == 'lti_1p3' and self.config_type in ('database', 'external'): lti_1p3_launch_url = consumer.launch_url return lti_1p3_launch_url diff --git a/lti_consumer/migrations/0018_add_ags_multi_tenancy.py b/lti_consumer/migrations/0018_add_ags_multi_tenancy.py new file mode 100644 index 00000000..a403f2ab --- /dev/null +++ b/lti_consumer/migrations/0018_add_ags_multi_tenancy.py @@ -0,0 +1,28 @@ +# Generated by Django 3.2.17 on 2023-07-07 02:57 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_consumer', '0017_lticonfiguration_lti_1p3_redirect_uris'), + ] + + operations = [ + migrations.AddField( + model_name='ltiagslineitem', + name='client_id', + field=models.CharField(blank=True, max_length=255, null=True), + ), + migrations.AddField( + model_name='ltiagslineitem', + name='deployment_id', + field=models.CharField(blank=True, max_length=255, null=True), + ), + migrations.AddField( + model_name='ltiagslineitem', + name='oidc_url', + field=models.CharField(blank=True, max_length=255, null=True), + ), + ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 86b913b8..69215c9c 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -351,9 +351,6 @@ def get_lti_advantage_ags_mode(self): """ Return LTI 1.3 Advantage Assignment and Grade Services mode. """ - if self.config_store == self.CONFIG_EXTERNAL: - # TODO: Add support for CONFIG_EXTERNAL for LTI 1.3. - raise NotImplementedError if self.config_store == self.CONFIG_ON_DB: return self.lti_advantage_ags_mode else: @@ -364,9 +361,6 @@ def get_lti_advantage_deep_linking_enabled(self): """ Return whether LTI 1.3 Advantage Deep Linking is enabled. """ - if self.config_store == self.CONFIG_EXTERNAL: - # TODO: Add support for CONFIG_EXTERNAL for LTI 1.3. - raise NotImplementedError("CONFIG_EXTERNAL is not supported for LTI 1.3 Advantage services: %s") if self.config_store == self.CONFIG_ON_DB: return self.lti_advantage_deep_linking_enabled else: @@ -377,9 +371,6 @@ def get_lti_advantage_deep_linking_launch_url(self): """ Return the LTI 1.3 Advantage Deep Linking launch URL. """ - if self.config_store == self.CONFIG_EXTERNAL: - # TODO: Add support for CONFIG_EXTERNAL for LTI 1.3. - raise NotImplementedError("CONFIG_EXTERNAL is not supported for LTI 1.3 Advantage services: %s") if self.config_store == self.CONFIG_ON_DB: return self.lti_advantage_deep_linking_launch_url else: @@ -390,9 +381,6 @@ def get_lti_advantage_nrps_enabled(self): """ Return whether LTI 1.3 Advantage Names and Role Provisioning Services is enabled. """ - if self.config_store == self.CONFIG_EXTERNAL: - # TODO: Add support for CONFIG_EXTERNAL for LTI 1.3. - raise NotImplementedError("CONFIG_EXTERNAL is not supported for LTI 1.3 Advantage services: %s") if self.config_store == self.CONFIG_ON_DB: return self.lti_advantage_enable_nrps else: @@ -414,7 +402,32 @@ def _setup_lti_1p3_ags(self, consumer): log.info('LTI Advantage AGS is disabled for %s', self) return - lineitem = self.ltiagslineitem_set.first() + # We set the deployment ID to a default value of 1, + # this will be used on a configuration with a CONFIG_EXTERNAL config store + # if no deployment ID is set on the external configuration. + deployment_id = 1 + + # Get OIDC URL, Client ID and Deployment ID (On CONFIG_EXTERNAL) + # to use it to retrieve/save the tool deployment LineItem. + if self.config_store == self.CONFIG_ON_XBLOCK: + block = compat.load_enough_xblock(self.location) + oidc_url = block.lti_1p3_oidc_url + client_id = block.lti_1p3_client_id + elif self.config_store == self.CONFIG_EXTERNAL: + config = get_external_config_from_filter({}, self.external_id) + oidc_url = config.get('lti_1p3_oidc_url') + client_id = config.get('lti_1p3_client_id') + deployment_id = config.get('lti_1p3_deployment_id') or deployment_id + else: + oidc_url = self.lti_1p3_oidc_url + client_id = self.lti_1p3_client_id + + lineitem = self.ltiagslineitem_set.filter( + oidc_url=oidc_url, + client_id=client_id, + deployment_id=deployment_id, + ).first() + # If using the declarative approach, we should create a LineItem if it # doesn't exist. This is because on this mode the tool is not able to create # and manage lineitems using the AGS endpoints. @@ -447,6 +460,9 @@ def _setup_lti_1p3_ags(self, consumer): lineitem = LtiAgsLineItem.objects.create( lti_configuration=self, resource_link_id=self.location, + oidc_url=oidc_url, + client_id=client_id, + deployment_id=deployment_id, **default_values ) @@ -534,9 +550,30 @@ def _get_lti_1p3_consumer(self): tool_key=self.lti_1p3_tool_public_key, tool_keyset_url=self.lti_1p3_tool_keyset_url, ) + elif self.config_store == self.CONFIG_EXTERNAL: + block = compat.load_enough_xblock(self.location) + external = get_external_config_from_filter({}, self.external_id) + + consumer = consumer_class( + iss=get_lti_api_base(), + # Use the xblock values if lti_1p3_oidc_url or lti_1p3_launch_url + # is not set on the external configuration. + lti_oidc_url=block.lti_1p3_oidc_url or external.get('lti_1p3_oidc_url'), + lti_launch_url=block.lti_1p3_launch_url or external.get('lti_1p3_launch_url'), + client_id=external.get('lti_1p3_client_id'), + # Deployment ID hardcoded to 1 if no deployment ID is set + # on the external configuration. + deployment_id=external.get('lti_1p3_deployment_id') or '1', + rsa_key=external.get('lti_1p3_private_key'), + rsa_key_id=external.get('lti_1p3_private_key_id'), + # Registered redirect uris + redirect_uris=self.get_lti_1p3_redirect_uris(), + tool_key=external.get('lti_1p3_tool_public_key'), + tool_keyset_url=external.get('lti_1p3_tool_keyset_url'), + ) else: - # This should not occur, but raise an error if self.config_store is not CONFIG_ON_XBLOCK - # or CONFIG_ON_DB. + # This should not occur, but raise an error if self.config_store is not + # CONFIG_ON_XBLOCK, CONFIG_ON_DB or CONFIG_EXTERNAL. raise NotImplementedError if isinstance(consumer, LtiAdvantageConsumer): @@ -560,10 +597,14 @@ def get_lti_1p3_redirect_uris(self): Return pre-registered redirect uris or sensible defaults """ if self.config_store == self.CONFIG_EXTERNAL: - # TODO: Add support for CONFIG_EXTERNAL for LTI 1.3. - raise NotImplementedError - - if self.config_store == self.CONFIG_ON_DB: + block = compat.load_enough_xblock(self.location) + config = get_external_config_from_filter({}, self.external_id) + redirect_uris = config.get('lti_1p3_redirect_uris') or block.lti_1p3_redirect_uris + launch_url = config.get('lti_1p3_launch_url') or block.lti_1p3_launch_url + deep_link_launch_url = config.get( + 'lti_advantage_deep_linking_launch_url', + ) or block.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 deep_link_launch_url = self.lti_advantage_deep_linking_launch_url @@ -609,10 +650,6 @@ class LtiAgsLineItem(models.Model): LTI-AGS Specification: https://www.imsglobal.org/spec/lti-ags/v2p0 The platform MUST NOT modify the 'resourceId', 'resourceLinkId' and 'tag' values. - Note: When implementing multi-tenancy support, this needs to be changed - and be tied to a deployment ID, because each deployment should isolate - it's resources. - .. no_pii: """ # LTI Configuration link @@ -626,6 +663,24 @@ class LtiAgsLineItem(models.Model): blank=True ) + # OIDC URL, Client ID and Deployment ID + # This allows us to isolate the LineItem per tool deployment + oidc_url = models.CharField( + max_length=255, + blank=True, + null=True, + ) + client_id = models.CharField( + max_length=255, + blank=True, + null=True, + ) + deployment_id = models.CharField( + max_length=255, + blank=True, + null=True, + ) + # Tool resource identifier, not used by the LMS. resource_id = models.CharField(max_length=100, blank=True) @@ -663,9 +718,6 @@ class LtiAgsScore(models.Model): Model to store LineItem Score data for LTI Assignments and Grades service. LTI-AGS Specification: https://www.imsglobal.org/spec/lti-ags/v2p0 - Note: When implementing multi-tenancy support, this needs to be changed - and be tied to a deployment ID, because each deployment should isolate - it's resources. .. no_pii: """ diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 48406ada..3bce7dfb 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -556,19 +556,25 @@ class LtiAgsLineItemViewset(viewsets.ModelViewSet): def get_queryset(self): lti_configuration = self.request.lti_configuration + consumer_dict = lti_configuration.get_lti_consumer().__dict__ - # Return all LineItems related to the LTI configuration. - # TODO: - # Note that each configuration currently maps 1:1 - # to each resource link (block), and this filter needs - # improved once we start reusing LTI configurations. return LtiAgsLineItem.objects.filter( - lti_configuration=lti_configuration + lti_configuration=lti_configuration, + oidc_url=consumer_dict.get('oidc_url'), + client_id=consumer_dict.get('client_id'), + deployment_id=consumer_dict.get('deployment_id'), ) def perform_create(self, serializer): lti_configuration = self.request.lti_configuration - serializer.save(lti_configuration=lti_configuration) + consumer_dict = lti_configuration.get_lti_consumer().__dict__ + + serializer.save( + lti_configuration=lti_configuration, + oidc_url=consumer_dict.get('oidc_url'), + client_id=consumer_dict.get('client_id'), + deployment_id=consumer_dict.get('deployment_id'), + ) @action( detail=True, diff --git a/lti_consumer/static/js/xblock_studio_view.js b/lti_consumer/static/js/xblock_studio_view.js index b920a12e..488872f7 100644 --- a/lti_consumer/static/js/xblock_studio_view.js +++ b/lti_consumer/static/js/xblock_studio_view.js @@ -72,33 +72,33 @@ 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 only the External Config ID field + * external - Show all LTI 1.3 config fields except LTI 1.3 tool fields */ function getFieldsToHideForLtiConfigType() { const configType = $(element).find('#xb-field-edit-config_type').val(); - const configFields = lti1P1FieldList.concat(lti1P3FieldList); + const databaseConfigHiddenFields = lti1P1FieldList.concat(lti1P3FieldList); + const externalConfigHiddenFields = lti1P1FieldList.concat([ + 'lti_1p3_tool_key_mode', + 'lti_1p3_tool_keyset_url', + 'lti_1p3_tool_public_key', + ]); const fieldsToHide = []; if (configType === "external") { - // Hide the lti_version field and all the LTI 1.1 and LTI 1.3 fields. - configFields.forEach(function (field) { + // Hide LTI 1.1 and LTI 1.3 tool fields. + externalConfigHiddenFields.forEach(function (field) { fieldsToHide.push(field); }) - fieldsToHide.push("lti_version"); } else if (configType === "database") { // Hide the LTI 1.1 and LTI 1.3 fields. The XBlock will remain the source of truth for the lti_version, // so do not hide it and continue to allow editing it from the XBlock edit menu in Studio. - configFields.forEach(function (field) { + databaseConfigHiddenFields.forEach(function (field) { fieldsToHide.push(field); }) } else { // No fields should be hidden based on a config_type of 'new'. } - if (configType === "external") { - fieldsToHide.push("external_config"); - } - return fieldsToHide; } diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index 7a9e850f..825b7b62 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -69,6 +69,7 @@ def setUp(self): 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 + self.consumer_dict = self.lti_config.get_lti_consumer().__dict__ def _set_lti_token(self, scopes=None): """ @@ -170,6 +171,9 @@ def test_lti_ags_list(self): # Create LineItem line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, + oidc_url=self.consumer_dict.get('oidc_url'), + client_id=self.consumer_dict.get('client_id'), + deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -208,6 +212,9 @@ def test_lti_ags_retrieve(self): # Create LineItem line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, + oidc_url=self.consumer_dict.get('oidc_url'), + client_id=self.consumer_dict.get('client_id'), + deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -317,6 +324,9 @@ def setUp(self): # Create LineItem self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, + oidc_url=self.consumer_dict.get('oidc_url'), + client_id=self.consumer_dict.get('client_id'), + deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -849,6 +859,9 @@ def setUp(self): # Create LineItem self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, + oidc_url=self.consumer_dict.get('oidc_url'), + client_id=self.consumer_dict.get('client_id'), + deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 083f850e..8e935b6d 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -416,6 +416,7 @@ def test_required_start_assessment_url_for_start_proctoring_message_type(self, s ) +@ddt.ddt class TestGetLti1p3LaunchInfo(TestCase): """ Unit tests for get_lti_1p3_launch_info API method. @@ -538,6 +539,53 @@ def test_launch_info_for_lti_config_without_location(self): } ) + @ddt.data( + { + 'lti_1p3_client_id': 'test-client-id', + 'lti_1p3_access_token_url': 'test-access-token-url', + 'lti_1p3_keyset_url': 'test-keyset-url', + 'lti_1p3_deployment_id': 'test-deployment-id', + }, + { + 'lti_1p3_client_id': 'test-client-id', + 'lti_1p3_access_token_url': 'test-access-token-url', + 'lti_1p3_keyset_url': 'test-keyset-url', + }, + ) + @patch('lti_consumer.api.get_external_config_from_filter') + def test_launch_info_for_lti_config_with_external_configuration( + self, + external_config, + filter_mock, + ): + """ + Check if the API can return launch info for LtiConfiguration using + an external configuration. + """ + filter_mock.return_value = external_config + LtiConfiguration.objects.create( + version=LtiConfiguration.LTI_1P3, + config_id=_test_config_id, + config_store=LtiConfiguration.CONFIG_EXTERNAL, + external_id='test-external-id', + ) + + launch_info = get_lti_1p3_launch_info(self._get_lti_1p3_launch_data()) + + self.assertEqual( + launch_info, + { + 'client_id': external_config.get('lti_1p3_client_id'), + 'keyset_url': external_config.get('lti_1p3_keyset_url'), + 'deployment_id': external_config.get('lti_1p3_deployment_id', '1'), + 'oidc_callback': 'https://example.com/api/lti_consumer/v1/launch/', + 'token_url': external_config.get('lti_1p3_access_token_url'), + 'deep_linking_launch_url': 'http://example.com', + 'deep_linking_content_items': None, + }, + ) + filter_mock.assert_called_once_with({}, 'test-external-id') + class TestGetLti1p3LaunchUrl(Lti1P3TestCase): """ diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 674a9f5a..b88bd8c8 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -15,6 +15,7 @@ from django.test.testcases import TestCase from django.utils import timezone from jwkest.jwk import RSAKey, KEYS +from xblock.validation import Validation from lti_consumer.exceptions import LtiError @@ -87,6 +88,7 @@ def test_indexibility(self): ) +@ddt.ddt class TestProperties(TestLtiConsumerXBlock): """ Unit tests for LtiConsumerXBlock properties @@ -146,6 +148,49 @@ def test_validate(self): validation = self.xblock.validate() self.assertFalse(validation.empty) + def test_validate_external_config_with_external_config_type(self): + """Test external config ID with external config type.""" + self.xblock.config_type = 'external' + self.xblock.external_config = '1' + + validation = self.xblock.validate() + + self.assertEqual(validation.messages, []) + + @ddt.data('new', 'database') + def test_validate_external_config_without_external_config_type( + self, + config_type, + ): + """Test with external config ID without using an external config type.""" + self.xblock.config_type = config_type + self.xblock.external_config = None + + validation = self.xblock.validate() + + self.assertEqual(validation.messages, []) + + @patch('lti_consumer.lti_xblock.ValidationMessage') + @patch.object(Validation, 'add') + def test_validate_empty_external_config_with_external_config_type( + self, + add_mock, + mock_validation_message, + ): + """Test with empty external config ID using an external config type.""" + mock_validation_message.ERROR.return_value = 'error' + self.xblock.config_type = 'external' + self.xblock.external_config = None + + self.xblock.validate() + + add_mock.assert_called_once_with( + mock_validation_message( + 'error', + 'Reusable configuration ID must be set when using external config.', + ), + ) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.course') def test_validate_lti_id(self, mock_course): """ diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 74cb48af..d70980f3 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -153,7 +153,11 @@ def test_repr(self): f"[CONFIG_ON_XBLOCK] lti_1p3 - {dummy_location}" ) - @ddt.data(LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB) + @ddt.data( + LtiConfiguration.CONFIG_ON_XBLOCK, + LtiConfiguration.CONFIG_ON_DB, + LtiConfiguration.CONFIG_EXTERNAL, + ) def test_lti_consumer_ags_enabled(self, config_store): """ Check if LTI AGS is properly included when block is graded. @@ -185,7 +189,7 @@ def test_lti_consumer_ags_enabled(self, config_store): @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'disabled'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': None}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'XBlock'}, ) @ddt.unpack def test_get_lti_advantage_ags_mode(self, config_store, expected_value): @@ -196,13 +200,13 @@ def test_get_lti_advantage_ags_mode(self, config_store, expected_value): self.xblock.lti_advantage_ags_mode = 'XBlock' - if config_store in (LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB): - self.assertEqual(config.get_lti_advantage_ags_mode(), expected_value) - else: - with self.assertRaises(NotImplementedError): - config.get_lti_advantage_ags_mode() + self.assertEqual(config.get_lti_advantage_ags_mode(), expected_value) - @ddt.data(LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB) + @ddt.data( + LtiConfiguration.CONFIG_ON_XBLOCK, + LtiConfiguration.CONFIG_ON_DB, + LtiConfiguration.CONFIG_EXTERNAL, + ) def test_lti_consumer_ags_declarative(self, config_store): """ Check that a LineItem is created if AGS is set to the declarative mode. @@ -235,7 +239,11 @@ def test_lti_consumer_ags_declarative(self, config_store): ags_claim.get('scope') ) - @ddt.data(LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB) + @ddt.data( + LtiConfiguration.CONFIG_ON_XBLOCK, + LtiConfiguration.CONFIG_ON_DB, + LtiConfiguration.CONFIG_EXTERNAL, + ) def test_lti_consumer_deep_linking_enabled(self, config_store): """ Check if LTI DL is properly instanced when configured. @@ -254,7 +262,7 @@ def test_lti_consumer_deep_linking_enabled(self, config_store): @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': None}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': False}, ) @ddt.unpack def test_get_lti_advantage_deep_linking_enabled(self, config_store, expected_value): @@ -265,16 +273,12 @@ def test_get_lti_advantage_deep_linking_enabled(self, config_store, expected_val self.xblock.lti_advantage_deep_linking_enabled = False - if config_store in (LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB): - self.assertEqual(config.get_lti_advantage_deep_linking_enabled(), expected_value) - else: - with self.assertRaises(NotImplementedError): - config.get_lti_advantage_deep_linking_enabled() + self.assertEqual(config.get_lti_advantage_deep_linking_enabled(), expected_value) @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'database'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': None}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'XBlock'}, ) @ddt.unpack def test_get_lti_advantage_deep_linking_launch_url(self, config_store, expected_value): @@ -285,16 +289,12 @@ def test_get_lti_advantage_deep_linking_launch_url(self, config_store, expected_ self.xblock.lti_advantage_deep_linking_launch_url = 'XBlock' - if config_store in (LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB): - self.assertEqual(config.get_lti_advantage_deep_linking_launch_url(), expected_value) - else: - with self.assertRaises(NotImplementedError): - config.get_lti_advantage_deep_linking_launch_url() + self.assertEqual(config.get_lti_advantage_deep_linking_launch_url(), expected_value) @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': None}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': False}, ) @ddt.unpack def test_get_lti_advantage_nrps_enabled(self, config_store, expected_value): @@ -305,11 +305,7 @@ def test_get_lti_advantage_nrps_enabled(self, config_store, expected_value): self.xblock.lti_advantage_enable_nrps = False - if config_store in (LtiConfiguration.CONFIG_ON_XBLOCK, LtiConfiguration.CONFIG_ON_DB): - self.assertEqual(config.get_lti_advantage_nrps_enabled(), expected_value) - else: - with self.assertRaises(NotImplementedError): - config.get_lti_advantage_nrps_enabled() + self.assertEqual(config.get_lti_advantage_nrps_enabled(), expected_value) def test_generate_private_key(self): """ @@ -375,13 +371,6 @@ def test_clean(self): with self.assertRaises(ValidationError): self.lti_1p3_config.clean() - def test_get_redirect_uris_raises_error_for_external_config(self): - """ - An external config raises NotImplementedError - """ - with self.assertRaises(NotImplementedError): - self.lti_1p3_config_external.get_lti_1p3_redirect_uris() - @ddt.data( (LAUNCH_URL, DEEP_LINK_URL, [], [LAUNCH_URL, DEEP_LINK_URL]), (LAUNCH_URL, DEEP_LINK_URL, ["http://other.url"], ["http://other.url"]), @@ -415,6 +404,45 @@ def test_get_redirect_uris_for_db_model_returns_expected( assert self.lti_1p3_config_db.get_lti_1p3_redirect_uris() == expected + @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_redirect_uris_with_external_config( + self, + get_external_config_from_filter_mock, + choose_lti_1p3_redirect_uris, + ): + """ + Test get_redirect_uris with external configuration. + """ + get_external_config_from_filter_mock.return_value = { + 'lti_1p3_redirect_uris': [], + 'lti_1p3_launch_url': LAUNCH_URL, + 'lti_advantage_deep_linking_launch_url': DEEP_LINK_URL, + } + + self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) + get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) + choose_lti_1p3_redirect_uris.assert_called_once_with([], LAUNCH_URL, DEEP_LINK_URL) + + @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_redirect_uris_with_empty_external_config( + self, + get_external_config_from_filter_mock, + choose_lti_1p3_redirect_uris, + ): + """ + Test get_redirect_uris with empty external configuration. + """ + get_external_config_from_filter_mock.return_value = {} + self.xblock.lti_1p3_redirect_uris = [] + self.xblock.lti_1p3_launch_url = LAUNCH_URL + self.xblock.lti_advantage_deep_linking_launch_url = DEEP_LINK_URL + + self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) + get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) + choose_lti_1p3_redirect_uris.assert_called_once_with([], LAUNCH_URL, DEEP_LINK_URL) + class TestLtiAgsLineItemModel(TestCase): """ From 681abaf9d4db56665a9e4160650c23da4a7beee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Thu, 31 Aug 2023 02:25:58 +0000 Subject: [PATCH 2/8] fix: CONFIG_EXTERNAL XBlock or DB values fallback --- lti_consumer/models.py | 57 +++++++++++++++++++++----- lti_consumer/tests/unit/test_models.py | 40 ++++++++++++++---- 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 69215c9c..72977492 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -373,6 +373,15 @@ 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: + block = compat.load_enough_xblock(self.location) + config = get_external_config_from_filter({}, self.external_id) + return ( + # Use the deep linking launch URL set in XBlock or DB has a fallback. + block.lti_advantage_deep_linking_launch_url or + self.lti_advantage_deep_linking_launch_url or + config.get('lti_advantage_deep_linking_launch_url') + ) else: block = compat.load_enough_xblock(self.location) return block.lti_advantage_deep_linking_launch_url @@ -405,7 +414,7 @@ def _setup_lti_1p3_ags(self, consumer): # We set the deployment ID to a default value of 1, # this will be used on a configuration with a CONFIG_EXTERNAL config store # if no deployment ID is set on the external configuration. - deployment_id = 1 + deployment_id = '1' # Get OIDC URL, Client ID and Deployment ID (On CONFIG_EXTERNAL) # to use it to retrieve/save the tool deployment LineItem. @@ -414,9 +423,17 @@ def _setup_lti_1p3_ags(self, consumer): oidc_url = block.lti_1p3_oidc_url client_id = block.lti_1p3_client_id elif self.config_store == self.CONFIG_EXTERNAL: + block = compat.load_enough_xblock(self.location) config = get_external_config_from_filter({}, self.external_id) - oidc_url = config.get('lti_1p3_oidc_url') + # Use the OIDC URL set in XBlock or DB has a fallback. + oidc_url = ( + block.lti_1p3_oidc_url or + self.lti_1p3_oidc_url or + config.get('lti_1p3_oidc_url') + ) client_id = config.get('lti_1p3_client_id') + # Deployment ID hardcoded to 1 if no deployment ID is set + # on the external configuration. deployment_id = config.get('lti_1p3_deployment_id') or deployment_id else: oidc_url = self.lti_1p3_oidc_url @@ -556,10 +573,18 @@ def _get_lti_1p3_consumer(self): consumer = consumer_class( iss=get_lti_api_base(), - # Use the xblock values if lti_1p3_oidc_url or lti_1p3_launch_url - # is not set on the external configuration. - lti_oidc_url=block.lti_1p3_oidc_url or external.get('lti_1p3_oidc_url'), - lti_launch_url=block.lti_1p3_launch_url or external.get('lti_1p3_launch_url'), + # Use the OIDC URL set in XBlock or DB has a fallback. + lti_oidc_url=( + block.lti_1p3_oidc_url or + self.lti_1p3_oidc_url or + external.get('lti_1p3_oidc_url') + ), + # Use the launch URL set in XBlock or DB has a fallback. + lti_launch_url=( + block.lti_1p3_launch_url or + self.lti_1p3_launch_url or + external.get('lti_1p3_launch_url') + ), client_id=external.get('lti_1p3_client_id'), # Deployment ID hardcoded to 1 if no deployment ID is set # on the external configuration. @@ -599,11 +624,21 @@ def get_lti_1p3_redirect_uris(self): if self.config_store == self.CONFIG_EXTERNAL: block = compat.load_enough_xblock(self.location) config = get_external_config_from_filter({}, self.external_id) - redirect_uris = config.get('lti_1p3_redirect_uris') or block.lti_1p3_redirect_uris - launch_url = config.get('lti_1p3_launch_url') or block.lti_1p3_launch_url - deep_link_launch_url = config.get( - 'lti_advantage_deep_linking_launch_url', - ) or block.lti_advantage_deep_linking_launch_url + redirect_uris = ( + block.lti_1p3_redirect_uris or + self.lti_1p3_redirect_uris or + config.get('lti_1p3_redirect_uris') + ) + launch_url = ( + block.lti_1p3_launch_url or + self.lti_1p3_launch_url or + config.get('lti_1p3_launch_url') + ) + deep_link_launch_url = ( + block.lti_advantage_deep_linking_launch_url or + self.lti_advantage_deep_linking_launch_url or + 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 diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index d70980f3..2d551e2b 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -414,34 +414,60 @@ def test_get_redirect_uris_with_external_config( """ Test get_redirect_uris with external configuration. """ - get_external_config_from_filter_mock.return_value = { - 'lti_1p3_redirect_uris': [], + external_config = { + 'lti_1p3_redirect_uris': ['external-redirect-uris'], 'lti_1p3_launch_url': LAUNCH_URL, 'lti_advantage_deep_linking_launch_url': DEEP_LINK_URL, } + get_external_config_from_filter_mock.return_value = external_config self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) - choose_lti_1p3_redirect_uris.assert_called_once_with([], LAUNCH_URL, DEEP_LINK_URL) + choose_lti_1p3_redirect_uris.assert_called_once_with( + external_config['lti_1p3_redirect_uris'], + external_config['lti_1p3_launch_url'], + external_config['lti_advantage_deep_linking_launch_url'], + ) @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) @patch('lti_consumer.models.get_external_config_from_filter') - def test_get_redirect_uris_with_empty_external_config( + def test_get_redirect_uris_with_external_config_and_xblock_override( self, get_external_config_from_filter_mock, choose_lti_1p3_redirect_uris, ): """ - Test get_redirect_uris with empty external configuration. + Test get_redirect_uris with external configuration and XBlock override. """ + redirect_uris = ['xblock-redirect-uris'] get_external_config_from_filter_mock.return_value = {} - self.xblock.lti_1p3_redirect_uris = [] + self.xblock.lti_1p3_redirect_uris = redirect_uris self.xblock.lti_1p3_launch_url = LAUNCH_URL self.xblock.lti_advantage_deep_linking_launch_url = DEEP_LINK_URL self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) - choose_lti_1p3_redirect_uris.assert_called_once_with([], LAUNCH_URL, DEEP_LINK_URL) + choose_lti_1p3_redirect_uris.assert_called_once_with(redirect_uris, LAUNCH_URL, DEEP_LINK_URL) + + @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_redirect_uris_with_external_config_and_db_override( + self, + get_external_config_from_filter_mock, + choose_lti_1p3_redirect_uris, + ): + """ + Test get_redirect_uris with external configuration and DB override. + """ + redirect_uris = ['db-redirect-uris'] + get_external_config_from_filter_mock.return_value = {} + self.lti_1p3_config_external.lti_1p3_redirect_uris = redirect_uris + self.lti_1p3_config_external.lti_1p3_launch_url = LAUNCH_URL + self.lti_1p3_config_external.lti_advantage_deep_linking_launch_url = DEEP_LINK_URL + + self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) + get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) + choose_lti_1p3_redirect_uris.assert_called_once_with(redirect_uris, LAUNCH_URL, DEEP_LINK_URL) class TestLtiAgsLineItemModel(TestCase): From 3c20fab88f2d619941daf174728254a7811d815c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Thu, 31 Aug 2023 02:47:12 +0000 Subject: [PATCH 3/8] fix: CI linting errors --- lti_consumer/lti_xblock.py | 2 +- lti_consumer/tests/unit/test_models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 4f343249..c58c4070 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -690,7 +690,7 @@ def validate_field_data(self, validation, data): validation.add(ValidationMessage(ValidationMessage.ERROR, str( _('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: _ = self.runtime.service(self, 'i18n').ugettext diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 7b1e8f82..bd2accee 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -469,7 +469,7 @@ def test_get_redirect_uris_with_external_config_and_db_override( self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) choose_lti_1p3_redirect_uris.assert_called_once_with(redirect_uris, LAUNCH_URL, DEEP_LINK_URL) - + @patch.object(LtiConfiguration, 'sync_configurations') def test_save(self, sync_configurations_mock): """Test save method.""" From f15a78c6054f34411b0a4610083cba91fe5dad7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Wed, 6 Sep 2023 08:45:54 +0000 Subject: [PATCH 4/8] feat: Get keyset and access token using external ID fix: Remove multi-tenancy related changes fix: Remove external configuration value overrides --- lti_consumer/api.py | 13 +- .../migrations/0018_add_ags_multi_tenancy.py | 28 ---- lti_consumer/models.py | 122 ++++------------- lti_consumer/plugin/urls.py | 10 ++ lti_consumer/plugin/views.py | 124 +++++++++++++----- lti_consumer/static/js/xblock_studio_view.js | 3 + lti_consumer/tests/unit/plugin/test_views.py | 95 +++++++++++++- .../tests/unit/plugin/test_views_lti_ags.py | 13 -- lti_consumer/tests/unit/test_api.py | 31 ++--- lti_consumer/tests/unit/test_models.py | 46 +------ 10 files changed, 237 insertions(+), 248 deletions(-) delete mode 100644 lti_consumer/migrations/0018_add_ags_multi_tenancy.py diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 0eb8c4c8..569f285d 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -144,27 +144,20 @@ def get_lti_1p3_launch_info( 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) - # We set the deployment ID to a default value of 1, - # this will be used on a configuration with a CONFIG_EXTERNAL config store - # if no deployment ID is set on the external configuration. - deployment_id = '1' # 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) client_id = external_config.get('lti_1p3_client_id') - token_url = external_config.get('lti_1p3_access_token_url') - keyset_url = external_config.get('lti_1p3_keyset_url') - # Show default harcoded deployment ID if no deployment ID - # is set on the external configuration. - deployment_id = external_config.get('lti_1p3_deployment_id') or deployment_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, - 'deployment_id': deployment_id, + 'deployment_id': '1', 'oidc_callback': get_lms_lti_launch_link(), 'token_url': token_url, 'deep_linking_launch_url': deep_linking_launch_url, diff --git a/lti_consumer/migrations/0018_add_ags_multi_tenancy.py b/lti_consumer/migrations/0018_add_ags_multi_tenancy.py deleted file mode 100644 index a403f2ab..00000000 --- a/lti_consumer/migrations/0018_add_ags_multi_tenancy.py +++ /dev/null @@ -1,28 +0,0 @@ -# Generated by Django 3.2.17 on 2023-07-07 02:57 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('lti_consumer', '0017_lticonfiguration_lti_1p3_redirect_uris'), - ] - - operations = [ - migrations.AddField( - model_name='ltiagslineitem', - name='client_id', - field=models.CharField(blank=True, max_length=255, null=True), - ), - migrations.AddField( - model_name='ltiagslineitem', - name='deployment_id', - field=models.CharField(blank=True, max_length=255, null=True), - ), - migrations.AddField( - model_name='ltiagslineitem', - name='oidc_url', - field=models.CharField(blank=True, max_length=255, null=True), - ), - ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 5d5f15a3..09572bae 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -412,14 +412,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 elif self.config_store == self.CONFIG_EXTERNAL: - block = compat.load_enough_xblock(self.location) config = get_external_config_from_filter({}, self.external_id) - return ( - # Use the deep linking launch URL set in XBlock or DB has a fallback. - block.lti_advantage_deep_linking_launch_url or - self.lti_advantage_deep_linking_launch_url or - config.get('lti_advantage_deep_linking_launch_url') - ) + return config.get('lti_advantage_deep_linking_launch_url') else: block = compat.load_enough_xblock(self.location) return block.lti_advantage_deep_linking_launch_url @@ -449,39 +443,7 @@ def _setup_lti_1p3_ags(self, consumer): log.info('LTI Advantage AGS is disabled for %s', self) return - # We set the deployment ID to a default value of 1, - # this will be used on a configuration with a CONFIG_EXTERNAL config store - # if no deployment ID is set on the external configuration. - deployment_id = '1' - - # Get OIDC URL, Client ID and Deployment ID (On CONFIG_EXTERNAL) - # to use it to retrieve/save the tool deployment LineItem. - if self.config_store == self.CONFIG_ON_XBLOCK: - block = compat.load_enough_xblock(self.location) - oidc_url = block.lti_1p3_oidc_url - client_id = block.lti_1p3_client_id - elif self.config_store == self.CONFIG_EXTERNAL: - block = compat.load_enough_xblock(self.location) - config = get_external_config_from_filter({}, self.external_id) - # Use the OIDC URL set in XBlock or DB has a fallback. - oidc_url = ( - block.lti_1p3_oidc_url or - self.lti_1p3_oidc_url or - config.get('lti_1p3_oidc_url') - ) - client_id = config.get('lti_1p3_client_id') - # Deployment ID hardcoded to 1 if no deployment ID is set - # on the external configuration. - deployment_id = config.get('lti_1p3_deployment_id') or deployment_id - else: - oidc_url = self.lti_1p3_oidc_url - client_id = self.lti_1p3_client_id - - lineitem = self.ltiagslineitem_set.filter( - oidc_url=oidc_url, - client_id=client_id, - deployment_id=deployment_id, - ).first() + lineitem = self.ltiagslineitem_set.first() # If using the declarative approach, we should create a LineItem if it # doesn't exist. This is because on this mode the tool is not able to create @@ -515,9 +477,6 @@ def _setup_lti_1p3_ags(self, consumer): lineitem = LtiAgsLineItem.objects.create( lti_configuration=self, resource_link_id=self.location, - oidc_url=oidc_url, - client_id=client_id, - deployment_id=deployment_id, **default_values ) @@ -606,33 +565,22 @@ def _get_lti_1p3_consumer(self): tool_keyset_url=self.lti_1p3_tool_keyset_url, ) elif self.config_store == self.CONFIG_EXTERNAL: - block = compat.load_enough_xblock(self.location) - external = get_external_config_from_filter({}, self.external_id) + config = get_external_config_from_filter({}, self.external_id) consumer = consumer_class( iss=get_lti_api_base(), - # Use the OIDC URL set in XBlock or DB has a fallback. - lti_oidc_url=( - block.lti_1p3_oidc_url or - self.lti_1p3_oidc_url or - external.get('lti_1p3_oidc_url') - ), - # Use the launch URL set in XBlock or DB has a fallback. - lti_launch_url=( - block.lti_1p3_launch_url or - self.lti_1p3_launch_url or - external.get('lti_1p3_launch_url') - ), - client_id=external.get('lti_1p3_client_id'), - # Deployment ID hardcoded to 1 if no deployment ID is set - # on the external configuration. - deployment_id=external.get('lti_1p3_deployment_id') or '1', - rsa_key=external.get('lti_1p3_private_key'), - rsa_key_id=external.get('lti_1p3_private_key_id'), + 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'), + # 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'), # Registered redirect uris redirect_uris=self.get_lti_1p3_redirect_uris(), - tool_key=external.get('lti_1p3_tool_public_key'), - tool_keyset_url=external.get('lti_1p3_tool_keyset_url'), + tool_key=config.get('lti_1p3_tool_public_key'), + tool_keyset_url=config.get('lti_1p3_tool_keyset_url'), ) else: # This should not occur, but raise an error if self.config_store is not @@ -660,23 +608,10 @@ def get_lti_1p3_redirect_uris(self): Return pre-registered redirect uris or sensible defaults """ if self.config_store == self.CONFIG_EXTERNAL: - block = compat.load_enough_xblock(self.location) config = get_external_config_from_filter({}, self.external_id) - redirect_uris = ( - block.lti_1p3_redirect_uris or - self.lti_1p3_redirect_uris or - config.get('lti_1p3_redirect_uris') - ) - launch_url = ( - block.lti_1p3_launch_url or - self.lti_1p3_launch_url or - config.get('lti_1p3_launch_url') - ) - deep_link_launch_url = ( - block.lti_advantage_deep_linking_launch_url or - self.lti_advantage_deep_linking_launch_url or - config.get('lti_advantage_deep_linking_launch_url') - ) + 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') elif self.config_store == self.CONFIG_ON_DB: redirect_uris = self.lti_1p3_redirect_uris launch_url = self.lti_1p3_launch_url @@ -723,6 +658,10 @@ class LtiAgsLineItem(models.Model): LTI-AGS Specification: https://www.imsglobal.org/spec/lti-ags/v2p0 The platform MUST NOT modify the 'resourceId', 'resourceLinkId' and 'tag' values. + Note: When implementing multi-tenancy support, this needs to be changed + and be tied to a deployment ID, because each deployment should isolate + it's resources. + .. no_pii: """ # LTI Configuration link @@ -736,24 +675,6 @@ class LtiAgsLineItem(models.Model): blank=True ) - # OIDC URL, Client ID and Deployment ID - # This allows us to isolate the LineItem per tool deployment - oidc_url = models.CharField( - max_length=255, - blank=True, - null=True, - ) - client_id = models.CharField( - max_length=255, - blank=True, - null=True, - ) - deployment_id = models.CharField( - max_length=255, - blank=True, - null=True, - ) - # Tool resource identifier, not used by the LMS. resource_id = models.CharField(max_length=100, blank=True) @@ -791,6 +712,9 @@ class LtiAgsScore(models.Model): Model to store LineItem Score data for LTI Assignments and Grades service. LTI-AGS Specification: https://www.imsglobal.org/spec/lti-ags/v2p0 + Note: When implementing multi-tenancy support, this needs to be changed + and be tied to a deployment ID, because each deployment should isolate + it's resources. .. no_pii: """ diff --git a/lti_consumer/plugin/urls.py b/lti_consumer/plugin/urls.py index 69780718..d53f570b 100644 --- a/lti_consumer/plugin/urls.py +++ b/lti_consumer/plugin/urls.py @@ -31,6 +31,11 @@ public_keyset_endpoint, name='lti_consumer.public_keyset_endpoint_via_location' ), + path( + 'lti_consumer/v1/public_keysets//', + public_keyset_endpoint, + name='lti_consumer.public_keyset_endpoint_via_external_id' + ), re_path( 'lti_consumer/v1/launch/(?:/(?P.*))?$', launch_gate_endpoint, @@ -46,6 +51,11 @@ access_token_endpoint, name='lti_consumer.access_token_via_location' ), + path( + 'lti_consumer/v1/token//', + access_token_endpoint, + name='lti_consumer.access_token_via_external_id' + ), re_path( r'lti_consumer/v1/lti/(?P[-\w]+)/lti-dl/response', deep_linking_response_endpoint, diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 15f71e2f..bb30244f 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -6,7 +6,7 @@ from django.conf import settings from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError +from django.core.exceptions import PermissionDenied, ValidationError from django.db import transaction from django.http import Http404, JsonResponse from django.shortcuts import render @@ -26,7 +26,7 @@ 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.lti_1p3.consumer import LtiProctoringConsumer +from lti_consumer.lti_1p3.consumer import LtiConsumer1p3, LtiProctoringConsumer from lti_consumer.lti_1p3.exceptions import (BadJwtSignature, InvalidClaimValue, Lti1p3Exception, LtiDeepLinkingContentTypeNotSupported, MalformedJwtToken, MissingRequiredClaim, NoSuitableKeys, TokenSignatureExpired, @@ -48,7 +48,8 @@ from lti_consumer.plugin import compat from lti_consumer.signals.signals import LTI_1P3_PROCTORING_ASSESSMENT_STARTED from lti_consumer.track import track_event -from lti_consumer.utils import _, get_data_from_cache, get_lti_1p3_context_types_claim +from lti_consumer.utils import _, get_data_from_cache, get_lti_1p3_context_types_claim, get_lti_api_base +from lti_consumer.filters import get_external_config_from_filter log = logging.getLogger(__name__) @@ -83,21 +84,50 @@ def has_block_access(user, block, course_key): @require_http_methods(["GET"]) -def public_keyset_endpoint(request, usage_id=None, lti_config_id=None): +def public_keyset_endpoint( + request, + usage_id=None, + lti_config_id=None, + external_app=None, + external_slug=None, +): """ Gate endpoint to fetch public keysets from a problem This is basically a passthrough function that uses the OIDC response parameter `login_hint` to locate the block and run the proper handler. + + Arguments: + lti_config_id (UUID): config_id of the LtiConfiguration + usage_id (UsageKey): location of the Block + external_app (str): App name of the external LTI configuration + external_slug (str): Slug of the external LTI configuration. + + Returns: + JsonResponse or Http404 """ + external_id = f"{external_app}:{external_slug}" + try: if usage_id: lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) + version = lti_config.version + public_jwk = lti_config.lti_1p3_public_jwk elif lti_config_id: lti_config = LtiConfiguration.objects.get(config_id=lti_config_id) + version = lti_config.version + public_jwk = lti_config.lti_1p3_public_jwk + elif external_app and external_slug: + lti_config = get_external_config_from_filter({}, external_id) + + if not lti_config: + raise ValueError("External LTI configuration not found") - if lti_config.version != lti_config.LTI_1P3: + version = lti_config.get("version") + public_jwk = lti_config.get("lti_1p3_public_jwk", {}) + + if version != LtiConfiguration.LTI_1P3: raise LtiError( "LTI Error: LTI 1.1 blocks do not have a public keyset endpoint." ) @@ -105,14 +135,13 @@ def public_keyset_endpoint(request, usage_id=None, lti_config_id=None): # Retrieve block's Public JWK # The underlying method will generate a new Private-Public Pair if one does # not exist, and retrieve the values. - response = JsonResponse(lti_config.lti_1p3_public_jwk) + response = JsonResponse(public_jwk) response['Content-Disposition'] = 'attachment; filename=keyset.json' return response - except (LtiError, InvalidKeyError, ObjectDoesNotExist) as exc: + except (InvalidKeyError, LtiConfiguration.DoesNotExist, ValueError, LtiError) as exc: log.info( - "Error while retrieving keyset for usage_id (%r) or lit_config_id (%s): %s", - usage_id, - lti_config_id, + "Error while retrieving keyset for ID %s: %s", + usage_id or lti_config_id or external_id, exc, exc_info=True, ) @@ -362,7 +391,13 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume @csrf_exempt @xframe_options_sameorigin @require_http_methods(["POST"]) -def access_token_endpoint(request, lti_config_id=None, usage_id=None): +def access_token_endpoint( + request, + lti_config_id=None, + usage_id=None, + external_app=None, + external_slug=None, +): """ Gate endpoint to enable tools to retrieve access tokens for the LTI 1.3 tool. @@ -371,6 +406,8 @@ def access_token_endpoint(request, lti_config_id=None, usage_id=None): Arguments: lti_config_id (UUID): config_id of the LtiConfiguration usage_id (UsageKey): location of the Block + external_app (str): App name of the external LTI configuration + external_slug (str): Slug of the external LTI configuration. Returns: JsonResponse or Http404 @@ -379,21 +416,50 @@ def access_token_endpoint(request, lti_config_id=None, usage_id=None): Sucess: https://tools.ietf.org/html/rfc6749#section-4.4.3 Failure: https://tools.ietf.org/html/rfc6749#section-5.2 """ + external_id = f"{external_app}:{external_slug}" try: - if lti_config_id: + if usage_id: + lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id)) + version = lti_config.version + lti_consumer = lti_config.get_lti_consumer() + elif lti_config_id: lti_config = LtiConfiguration.objects.get(config_id=lti_config_id) - else: - usage_key = UsageKey.from_string(usage_id) - lti_config = LtiConfiguration.objects.get(location=usage_key) - except LtiConfiguration.DoesNotExist as exc: - log.warning("Error getting the LTI configuration with id %r: %s", lti_config_id, exc, exc_info=True) + version = lti_config.version + lti_consumer = lti_config.get_lti_consumer() + elif external_app and external_slug: + lti_config = get_external_config_from_filter({}, external_id) + + if not lti_config: + raise ValueError("External LTI configuration not found") + + version = lti_config.get("version") + # External LTI configurations don't have a get_lti_consumer method + # so we initialize the LtiConsumer1p3 class using the external config data. + lti_consumer = LtiConsumer1p3( + iss=get_lti_api_base(), + lti_oidc_url=None, + lti_launch_url=None, + client_id=lti_config.get("lti_1p3_client_id"), + deployment_id=None, + rsa_key=lti_config.get("lti_1p3_private_key"), + rsa_key_id=lti_config.get("lti_1p3_private_key_id"), + redirect_uris=None, + 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: + log.info( + "Error while retrieving access token for ID %s: %s", + usage_id or lti_config_id or external_id, + exc, + exc_info=True, + ) raise Http404 from exc - if lti_config.version != lti_config.LTI_1P3: + if version != LtiConfiguration.LTI_1P3: return JsonResponse({"error": "invalid_lti_version"}, status=HTTP_404_NOT_FOUND) - lti_consumer = lti_config.get_lti_consumer() try: token = lti_consumer.access_token( dict(urllib.parse.parse_qsl( @@ -606,25 +672,19 @@ class LtiAgsLineItemViewset(viewsets.ModelViewSet): def get_queryset(self): lti_configuration = self.request.lti_configuration - consumer_dict = lti_configuration.get_lti_consumer().__dict__ + # Return all LineItems related to the LTI configuration. + # TODO: + # Note that each configuration currently maps 1:1 + # to each resource link (block), and this filter needs + # improved once we start reusing LTI configurations. return LtiAgsLineItem.objects.filter( - lti_configuration=lti_configuration, - oidc_url=consumer_dict.get('oidc_url'), - client_id=consumer_dict.get('client_id'), - deployment_id=consumer_dict.get('deployment_id'), + lti_configuration=lti_configuration ) def perform_create(self, serializer): lti_configuration = self.request.lti_configuration - consumer_dict = lti_configuration.get_lti_consumer().__dict__ - - serializer.save( - lti_configuration=lti_configuration, - oidc_url=consumer_dict.get('oidc_url'), - client_id=consumer_dict.get('client_id'), - deployment_id=consumer_dict.get('deployment_id'), - ) + serializer.save(lti_configuration=lti_configuration) @action( detail=True, diff --git a/lti_consumer/static/js/xblock_studio_view.js b/lti_consumer/static/js/xblock_studio_view.js index 488872f7..bece55a6 100644 --- a/lti_consumer/static/js/xblock_studio_view.js +++ b/lti_consumer/static/js/xblock_studio_view.js @@ -78,9 +78,12 @@ function LtiConsumerXBlockInitStudio(runtime, element) { 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 fieldsToHide = []; diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index a2c701a2..7c62ce1a 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -30,6 +30,7 @@ from lti_consumer.utils import cache_lti_1p3_launch_data +@ddt.ddt class TestLti1p3KeysetEndpoint(TestCase): """ Test `public_keyset_endpoint` method. @@ -74,6 +75,19 @@ def test_invalid_usage_key(self): response = self.client.get('/lti_consumer/v1/public_keysets/invalid-key') self.assertEqual(response.status_code, 404) + @ddt.data( + ('lti_consumer:lti_consumer.public_keyset_endpoint', ['075194d3-6885-417e-a8a8-6c931e272f00']), + ('lti_consumer:lti_consumer.public_keyset_endpoint_via_location', ['block-v1:x+x+x+type@problem+block@x']), + ('lti_consumer:lti_consumer.public_keyset_endpoint_via_external_id', ['x', 'x']), + ) + @ddt.unpack + def test_non_existant_configuration(self, url, args): + """ + Check that 404 is returned when there is no configuration found. + """ + response = self.client.get(reverse(url, args=args)) + self.assertEqual(response.status_code, 404) + def test_wrong_lti_version(self): """ Check if trying to fetch the public keyset for LTI 1.1 yields a HTTP code 404. @@ -102,6 +116,25 @@ def test_public_keyset_endpoint_using_lti_config_id_in_url(self): json.loads(response.content.decode('utf-8')) ) + @patch('lti_consumer.plugin.views.get_external_config_from_filter') + def test_public_keyset_endpoint_using_external_id_in_url(self, get_external_config_from_filter): + """ + Check that the endpoint is accessible using the external LTI configuration ID. + """ + external_config = {'version': LtiConfiguration.LTI_1P3, 'lti_1p3_public_jwk': {}} + get_external_config_from_filter.return_value = external_config + + response = self.client.get(reverse( + 'lti_consumer:lti_consumer.public_keyset_endpoint_via_external_id', + args=['x', 'x'] + )) + + get_external_config_from_filter.assert_called_once_with({}, "x:x") + self.assertEqual(response.status_code, 200) + self.assertEqual(response['Content-type'], 'application/json') + self.assertEqual(response['Content-Disposition'], 'attachment; filename=keyset.json') + self.assertEqual(external_config["lti_1p3_public_jwk"], json.loads(response.content.decode('utf-8'))) + @ddt.ddt class TestLti1p3LaunchGateEndpoint(TestCase): @@ -620,6 +653,7 @@ def test_launch_non_existing_custom_parameters(self, mock_set_custom_parameters) mock_set_custom_parameters.assert_not_called() +@ddt.ddt class TestLti1p3AccessTokenEndpoint(TestCase): """ Test `access_token_endpoint` method. @@ -687,12 +721,65 @@ def test_access_token_endpoint_with_location_in_url(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.json(), token) - def test_non_existant_configuration_for_given_id(self): + @patch('lti_consumer.plugin.views.get_lti_api_base') + @patch('lti_consumer.plugin.views.LtiConsumer1p3') + @patch('lti_consumer.plugin.views.get_external_config_from_filter') + def test_access_token_endpoint_with_external_id_in_url( + self, + get_external_config_from_filter, + lti_consumer, + get_lti_api_base, + ): + """ + Check that the access_token generated by the lti_consumer is returned. + """ + external_config = { + 'version': LtiConfiguration.LTI_1P3, + 'lti_1p3_client_id': 'client-id', + 'lti_1p3_private_key': 'private-key', + 'lti_1p3_private_key_id': 'private-key-id', + 'lti_1p3_tool_public_key': 'public-key', + 'lti_1p3_tool_keyset_url': 'keyset-url', + } + token = {'access_token': 'test-token'} + get_external_config_from_filter.return_value = external_config + lti_consumer.return_value.access_token.return_value = token + + body = self.get_body(create_jwt(self.key, {})) + response = self.client.post( + reverse('lti_consumer:lti_consumer.access_token_via_external_id', args=['x', 'x']), + data=body, + ) + + get_external_config_from_filter.assert_called_once_with({}, 'x:x') + get_lti_api_base.assert_called_once_with() + lti_consumer.assert_called_once_with( + iss=get_lti_api_base(), + lti_oidc_url=None, + lti_launch_url=None, + client_id=external_config['lti_1p3_client_id'], + deployment_id=None, + rsa_key=external_config['lti_1p3_private_key'], + rsa_key_id=external_config['lti_1p3_private_key_id'], + redirect_uris=None, + tool_key=external_config['lti_1p3_tool_public_key'], + tool_keyset_url=external_config['lti_1p3_tool_keyset_url'], + ) + lti_consumer().access_token.called_once_with(body) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), token) + + @ddt.data( + ('lti_consumer:lti_consumer.access_token', ['075194d3-6885-417e-a8a8-6c931e272f00']), + ('lti_consumer:lti_consumer.access_token_via_location', ['block-v1:x+x+x+type@problem+block@x']), + ('lti_consumer:lti_consumer.access_token_via_external_id', ['x', 'x']), + ) + @ddt.unpack + def test_non_existant_configuration(self, url, args): """ - Check that 404 is returned when there is no configuration for a given id + Check that 404 is returned when there is no configuration found. """ - url = reverse('lti_consumer:lti_consumer.access_token', args=['075194d3-6885-417e-a8a8-6c931e272f00']) - response = self.client.post(url) + response = self.client.post(reverse(url, args=args)) self.assertEqual(response.status_code, 404) def test_verify_lti_version_is_1p3(self): diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py index 825b7b62..7a9e850f 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -69,7 +69,6 @@ def setUp(self): 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 - self.consumer_dict = self.lti_config.get_lti_consumer().__dict__ def _set_lti_token(self, scopes=None): """ @@ -171,9 +170,6 @@ def test_lti_ags_list(self): # Create LineItem line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, - oidc_url=self.consumer_dict.get('oidc_url'), - client_id=self.consumer_dict.get('client_id'), - deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -212,9 +208,6 @@ def test_lti_ags_retrieve(self): # Create LineItem line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, - oidc_url=self.consumer_dict.get('oidc_url'), - client_id=self.consumer_dict.get('client_id'), - deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -324,9 +317,6 @@ def setUp(self): # Create LineItem self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, - oidc_url=self.consumer_dict.get('oidc_url'), - client_id=self.consumer_dict.get('client_id'), - deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", @@ -859,9 +849,6 @@ def setUp(self): # Create LineItem self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, - oidc_url=self.consumer_dict.get('oidc_url'), - client_id=self.consumer_dict.get('client_id'), - deployment_id=self.consumer_dict.get('deployment_id'), resource_id="test", resource_link_id=self.xblock.scope_ids.usage_id, label="test label", diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index 8e935b6d..5b3095a1 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -416,7 +416,6 @@ def test_required_start_assessment_url_for_start_proctoring_message_type(self, s ) -@ddt.ddt class TestGetLti1p3LaunchInfo(TestCase): """ Unit tests for get_lti_1p3_launch_info API method. @@ -539,35 +538,23 @@ def test_launch_info_for_lti_config_without_location(self): } ) - @ddt.data( - { - 'lti_1p3_client_id': 'test-client-id', - 'lti_1p3_access_token_url': 'test-access-token-url', - 'lti_1p3_keyset_url': 'test-keyset-url', - 'lti_1p3_deployment_id': 'test-deployment-id', - }, - { - 'lti_1p3_client_id': 'test-client-id', - 'lti_1p3_access_token_url': 'test-access-token-url', - 'lti_1p3_keyset_url': 'test-keyset-url', - }, - ) @patch('lti_consumer.api.get_external_config_from_filter') def test_launch_info_for_lti_config_with_external_configuration( self, - external_config, filter_mock, ): """ Check if the API can return launch info for LtiConfiguration using an external configuration. """ + external_id = 'test-app:test-slug' + external_config = {'lti_1p3_client_id': 'test-client-id'} filter_mock.return_value = external_config - LtiConfiguration.objects.create( + lti_config = LtiConfiguration.objects.create( version=LtiConfiguration.LTI_1P3, config_id=_test_config_id, config_store=LtiConfiguration.CONFIG_EXTERNAL, - external_id='test-external-id', + external_id=external_id, ) launch_info = get_lti_1p3_launch_info(self._get_lti_1p3_launch_data()) @@ -576,15 +563,19 @@ def test_launch_info_for_lti_config_with_external_configuration( launch_info, { 'client_id': external_config.get('lti_1p3_client_id'), - 'keyset_url': external_config.get('lti_1p3_keyset_url'), + 'keyset_url': 'https://example.com/api/lti_consumer/v1/public_keysets/{}'.format( + lti_config.external_id.replace(':', '/'), + ), 'deployment_id': external_config.get('lti_1p3_deployment_id', '1'), 'oidc_callback': 'https://example.com/api/lti_consumer/v1/launch/', - 'token_url': external_config.get('lti_1p3_access_token_url'), + 'token_url': 'https://example.com/api/lti_consumer/v1/token/{}'.format( + lti_config.external_id.replace(':', '/'), + ), 'deep_linking_launch_url': 'http://example.com', 'deep_linking_content_items': None, }, ) - filter_mock.assert_called_once_with({}, 'test-external-id') + filter_mock.assert_called_once_with({}, external_id) class TestGetLti1p3LaunchUrl(Lti1P3TestCase): diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index bd2accee..011679ae 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -279,13 +279,15 @@ def test_get_lti_advantage_deep_linking_enabled(self, config_store, expected_val @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'database'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'XBlock'}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'external'}, ) @ddt.unpack - def test_get_lti_advantage_deep_linking_launch_url(self, config_store, expected_value): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_lti_advantage_deep_linking_launch_url(self, filter_mock, config_store, expected_value): """ Check if LTI Deep Linking launch URL is properly returned. """ + filter_mock.return_value = {'lti_advantage_deep_linking_launch_url': 'external'} config = self._get_1p3_config(config_store=config_store, lti_advantage_deep_linking_launch_url='database') self.xblock.lti_advantage_deep_linking_launch_url = 'XBlock' @@ -430,46 +432,6 @@ def test_get_redirect_uris_with_external_config( external_config['lti_advantage_deep_linking_launch_url'], ) - @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) - @patch('lti_consumer.models.get_external_config_from_filter') - def test_get_redirect_uris_with_external_config_and_xblock_override( - self, - get_external_config_from_filter_mock, - choose_lti_1p3_redirect_uris, - ): - """ - Test get_redirect_uris with external configuration and XBlock override. - """ - redirect_uris = ['xblock-redirect-uris'] - get_external_config_from_filter_mock.return_value = {} - self.xblock.lti_1p3_redirect_uris = redirect_uris - self.xblock.lti_1p3_launch_url = LAUNCH_URL - self.xblock.lti_advantage_deep_linking_launch_url = DEEP_LINK_URL - - self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) - get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) - choose_lti_1p3_redirect_uris.assert_called_once_with(redirect_uris, LAUNCH_URL, DEEP_LINK_URL) - - @patch('lti_consumer.models.choose_lti_1p3_redirect_uris', return_value=None) - @patch('lti_consumer.models.get_external_config_from_filter') - def test_get_redirect_uris_with_external_config_and_db_override( - self, - get_external_config_from_filter_mock, - choose_lti_1p3_redirect_uris, - ): - """ - Test get_redirect_uris with external configuration and DB override. - """ - redirect_uris = ['db-redirect-uris'] - get_external_config_from_filter_mock.return_value = {} - self.lti_1p3_config_external.lti_1p3_redirect_uris = redirect_uris - self.lti_1p3_config_external.lti_1p3_launch_url = LAUNCH_URL - self.lti_1p3_config_external.lti_advantage_deep_linking_launch_url = DEEP_LINK_URL - - self.assertEqual(self.lti_1p3_config_external.get_lti_1p3_redirect_uris(), None) - get_external_config_from_filter_mock.assert_called_once_with({}, self.lti_1p3_config_external.external_id) - choose_lti_1p3_redirect_uris.assert_called_once_with(redirect_uris, LAUNCH_URL, DEEP_LINK_URL) - @patch.object(LtiConfiguration, 'sync_configurations') def test_save(self, sync_configurations_mock): """Test save method.""" From 0af37538f471f2b7259cc2a2f255c7f25f9335c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Wed, 27 Sep 2023 04:53:22 +0000 Subject: [PATCH 5/8] fix: Apply suggestions --- lti_consumer/api.py | 9 ++--- lti_consumer/exceptions.py | 7 ++++ lti_consumer/lti_xblock.py | 9 +++++ lti_consumer/models.py | 13 +++++++ lti_consumer/plugin/urls.py | 8 +++-- lti_consumer/plugin/views.py | 10 +++--- lti_consumer/static/js/xblock_studio_view.js | 11 ++---- lti_consumer/tests/unit/plugin/test_views.py | 6 ++-- lti_consumer/tests/unit/test_lti_xblock.py | 36 ++++++++++++++++--- lti_consumer/tests/unit/test_models.py | 37 +++++++++++++++----- 10 files changed, 108 insertions(+), 38 deletions(-) diff --git a/lti_consumer/api.py b/lti_consumer/api.py index 569f285d..768b3649 100644 --- a/lti_consumer/api.py +++ b/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, diff --git a/lti_consumer/exceptions.py b/lti_consumer/exceptions.py index dec54692..8d3c0c5a 100644 --- a/lti_consumer/exceptions.py +++ b/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. + """ diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index c58c4070..cc4f86cf 100644 --- a/lti_consumer/lti_xblock.py +++ b/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 = '' diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 09572bae..4d1b422f 100644 --- a/lti_consumer/models.py +++ b/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 diff --git a/lti_consumer/plugin/urls.py b/lti_consumer/plugin/urls.py index d53f570b..a6117b71 100644 --- a/lti_consumer/plugin/urls.py +++ b/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//', + 'lti_consumer/v1/public_keysets//', 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//', + 'lti_consumer/v1/token//', access_token_endpoint, name='lti_consumer.access_token_via_external_id' ), diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index bb30244f..c5436c2d 100644 --- a/lti_consumer/plugin/views.py +++ b/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, diff --git a/lti_consumer/static/js/xblock_studio_view.js b/lti_consumer/static/js/xblock_studio_view.js index bece55a6..e8331be9 100644 --- a/lti_consumer/static/js/xblock_studio_view.js +++ b/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") { diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 7c62ce1a..02baa3cb 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/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. """ diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 0a512f2a..24cacbc0 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/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.', ), ) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 011679ae..4c004ac9 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -159,10 +159,12 @@ def test_repr(self): LtiConfiguration.CONFIG_ON_DB, LtiConfiguration.CONFIG_EXTERNAL, ) - def test_lti_consumer_ags_enabled(self, config_store): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_lti_consumer_ags_enabled(self, config_store, filter_mock): """ Check if LTI AGS is properly included when block is graded. """ + filter_mock.return_value = {'lti_advantage_ags_mode': 'programmatic'} config = self._get_1p3_config( config_store=config_store, lti_advantage_ags_mode='programmatic' @@ -190,13 +192,15 @@ def test_lti_consumer_ags_enabled(self, config_store): @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': 'XBlock'}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': 'disabled'}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'XBlock'}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': 'external'}, ) @ddt.unpack - def test_get_lti_advantage_ags_mode(self, config_store, expected_value): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_lti_advantage_ags_mode(self, filter_mock, config_store, expected_value): """ Check if LTI AGS is properly returned. """ + filter_mock.return_value = {'lti_advantage_ags_mode': 'external'} config = self._get_1p3_config(config_store=config_store, lti_advantage_ags_mode='disabled') self.xblock.lti_advantage_ags_mode = 'XBlock' @@ -208,10 +212,12 @@ def test_get_lti_advantage_ags_mode(self, config_store, expected_value): LtiConfiguration.CONFIG_ON_DB, LtiConfiguration.CONFIG_EXTERNAL, ) - def test_lti_consumer_ags_declarative(self, config_store): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_lti_consumer_ags_declarative(self, config_store, filter_mock): """ Check that a LineItem is created if AGS is set to the declarative mode. """ + filter_mock.return_value = {'lti_advantage_ags_mode': 'declarative'} self.xblock.lti_advantage_ags_mode = 'declarative' # Include `start` and `due` dates @@ -245,10 +251,12 @@ def test_lti_consumer_ags_declarative(self, config_store): LtiConfiguration.CONFIG_ON_DB, LtiConfiguration.CONFIG_EXTERNAL, ) - def test_lti_consumer_deep_linking_enabled(self, config_store): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_lti_consumer_deep_linking_enabled(self, config_store, filter_mock): """ Check if LTI DL is properly instanced when configured. """ + filter_mock.return_value = {'lti_advantage_deep_linking_enabled': True} config = self._get_1p3_config( config_store=config_store, lti_advantage_deep_linking_enabled=True @@ -263,13 +271,15 @@ def test_lti_consumer_deep_linking_enabled(self, config_store): @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': False}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': True}, ) @ddt.unpack - def test_get_lti_advantage_deep_linking_enabled(self, config_store, expected_value): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_lti_advantage_deep_linking_enabled(self, filter_mock, config_store, expected_value): """ Check if LTI Deep Linking enabled is properly returned. """ + filter_mock.return_value = {'lti_advantage_deep_linking_enabled': True} config = self._get_1p3_config(config_store=config_store, lti_advantage_deep_linking_enabled=True) self.xblock.lti_advantage_deep_linking_enabled = False @@ -297,13 +307,15 @@ def test_get_lti_advantage_deep_linking_launch_url(self, filter_mock, config_sto @ddt.data( {'config_store': LtiConfiguration.CONFIG_ON_XBLOCK, 'expected_value': False}, {'config_store': LtiConfiguration.CONFIG_ON_DB, 'expected_value': True}, - {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': False}, + {'config_store': LtiConfiguration.CONFIG_EXTERNAL, 'expected_value': True}, ) @ddt.unpack - def test_get_lti_advantage_nrps_enabled(self, config_store, expected_value): + @patch('lti_consumer.models.get_external_config_from_filter') + def test_get_lti_advantage_nrps_enabled(self, filter_mock, config_store, expected_value): """ Check if LTI Deep Linking launch URL is properly returned. """ + filter_mock.return_value = {'lti_advantage_enable_nrps': True} config = self._get_1p3_config(config_store=config_store, lti_advantage_enable_nrps=True) self.xblock.lti_advantage_enable_nrps = False @@ -356,6 +368,12 @@ def test_clean(self): self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_ON_XBLOCK self.lti_1p3_config.location = None + with self.assertRaises(ValidationError): + self.lti_1p3_config.clean() + + 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() @@ -368,6 +386,7 @@ def test_clean(self): self.lti_1p3_config_db.clean() self.lti_1p3_config.lti_1p3_proctoring_enabled = True + self.lti_1p3_config.external_id = 'test_id' for config_store in [self.lti_1p3_config.CONFIG_ON_XBLOCK, self.lti_1p3_config.CONFIG_EXTERNAL]: self.lti_1p3_config.config_store = config_store From c8031ebab8846bb6bb90d2ebf9dea133a2d83443 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Wed, 11 Oct 2023 04:15:59 +0000 Subject: [PATCH 6/8] fix: Apply suggestions fix: Improve LtiConfiguration clean method test --- docs/reusable_configuration.rst | 80 ++++++++++++++++++++++ lti_consumer/lti_xblock.py | 19 ++--- lti_consumer/models.py | 57 +++++++-------- lti_consumer/tests/unit/test_lti_xblock.py | 18 ++--- lti_consumer/tests/unit/test_models.py | 5 ++ lti_consumer/utils.py | 4 ++ 6 files changed, 133 insertions(+), 50 deletions(-) create mode 100644 docs/reusable_configuration.rst diff --git a/docs/reusable_configuration.rst b/docs/reusable_configuration.rst new file mode 100644 index 00000000..8177be49 --- /dev/null +++ b/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: + - 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. diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index cc4f86cf..50ec65f7 100644 --- a/lti_consumer/lti_xblock.py +++ b/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 diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 4d1b422f..3c7d87ea 100644 --- a/lti_consumer/models.py +++ b/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 diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 24cacbc0..c088b012 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/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").', ), ) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 4c004ac9..f95daf21 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/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() diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index 192c2bc1..a6acbd4d 100644 --- a/lti_consumer/utils.py +++ b/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): """ From 808734691161889bd0a5122be260ba18edc62070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Thu, 19 Oct 2023 18:03:16 +0000 Subject: [PATCH 7/8] fix: Apply suggestions, sync_configurations method --- README.rst | 24 ++++++++ docs/reusable_configuration.rst | 80 -------------------------- lti_consumer/models.py | 2 +- lti_consumer/tests/unit/test_models.py | 10 +++- 4 files changed, 33 insertions(+), 83 deletions(-) delete mode 100644 docs/reusable_configuration.rst diff --git a/README.rst b/README.rst index 6a207778..a55f4565 100644 --- a/README.rst +++ b/README.rst @@ -275,6 +275,30 @@ This XBlock supports `LTI 2.0 Result Service 2.0 `_ for testing the LTI 2.0 Result Service 2.0 implementation. +LTI Reusable configuration +************************** + +The LTI Consumer XBlock supports configuration reusability via plugins. +It is compatible with both LTI 1.1 and LTI 1.3. +All values (including the access token and keyset URL for LTI 1.3) +are shared across the XBlocks with the same external configuration ID. +This eliminates the need to have a tool deployment for each XBlock. + +How to Setup +============ + +1. Install and setup the openedx-ltistore plugin on the LMS and studio + (https://github.com/open-craft/openedx-ltistore): +2. Go to LMS admin > WAFFLE_UTILS > Waffle flag course override + (http://localhost:18010/admin/waffle_utils/waffleflagcourseoverridemodel/). +3. Create a waffle flag course override with these values: + - Waffle flag: lti_consumer.enable_external_config_filter + - Course id: + - Override choice: Force On + - Enabled: True +4. Create a new external LTI configuration and use it on the XBlock. + (This is explaioned on the README of the openedx-ltistore repository). + Getting Help ************ diff --git a/docs/reusable_configuration.rst b/docs/reusable_configuration.rst deleted file mode 100644 index 8177be49..00000000 --- a/docs/reusable_configuration.rst +++ /dev/null @@ -1,80 +0,0 @@ -########################## -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: - - 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. diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 3c7d87ea..0f39e212 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -288,7 +288,7 @@ def sync_configurations(self): otherwise, it will try to query any children configuration and update their fields using the current configuration values. """ - EXCLUDED_FIELDS = ['id', 'config_id', 'location'] + EXCLUDED_FIELDS = ['id', 'config_id', 'location', 'external_config'] if isinstance(self.location, CCXBlockUsageLocator): # Query main configuration using main location. diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index f95daf21..78d10536 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -485,7 +485,10 @@ def test_sync_configurations_with_ccx_location( call(location=self.lti_1p3_config.location.to_block_locator()), call().first(), ]) - model_to_dict_mock.assert_called_once_with(filter_mock.return_value.first(), ['id', 'config_id', 'location']) + model_to_dict_mock.assert_called_once_with( + filter_mock.return_value.first(), + ['id', 'config_id', 'location', 'external_config'], + ) setattr_mock.assert_called_once_with(self.lti_1p3_config, 'test', 'test') @patch('lti_consumer.models.isinstance', return_value=False) @@ -508,7 +511,10 @@ def test_sync_configurations_with_location( call().filter().exclude(id=self.lti_1p3_config.pk), call().filter().exclude().update(**model_to_dict_mock), ]) - model_to_dict_mock.assert_called_once_with(self.lti_1p3_config, ['id', 'config_id', 'location']) + model_to_dict_mock.assert_called_once_with( + self.lti_1p3_config, + ['id', 'config_id', 'location', 'external_config'], + ) @patch('lti_consumer.models.isinstance', return_value=False) @patch.object(LtiConfiguration.objects, 'filter', side_effect=IndexError()) From c9a25213d55c42321e5de8ba52d2329586812118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Montilla?= Date: Mon, 23 Oct 2023 23:14:53 +0000 Subject: [PATCH 8/8] feat: Update version, changelog and readme --- CHANGELOG.rst | 4 ++++ README.rst | 13 +++++++------ lti_consumer/__init__.py | 2 +- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ac64813c..96983ea7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,10 @@ Please See the `releases tab WAFFLE_UTILS > Waffle flag course override - (http://localhost:18010/admin/waffle_utils/waffleflagcourseoverridemodel/). +1. Install and setup the `openedx-ltistore`_ plugin on the LMS and Studio. +2. Go to LMS admin -> WAFFLE_UTILS -> Waffle flag course override + (http://localhost:18000/admin/waffle_utils/waffleflagcourseoverridemodel/). 3. Create a waffle flag course override with these values: - Waffle flag: lti_consumer.enable_external_config_filter - Course id: - Override choice: Force On - Enabled: True -4. Create a new external LTI configuration and use it on the XBlock. - (This is explaioned on the README of the openedx-ltistore repository). +4. Create a new external LTI configuration and use it in the XBlock. + This is explained in the README of the `openedx-ltistore`_ repository. + +.. _openedx-ltistore: https://github.com/open-craft/openedx-ltistore Getting Help ************ diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py index 030511cb..22663674 100644 --- a/lti_consumer/__init__.py +++ b/lti_consumer/__init__.py @@ -4,4 +4,4 @@ from .apps import LTIConsumerApp from .lti_xblock import LtiConsumerXBlock -__version__ = '9.6.2' +__version__ = '9.7.0'