diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1a9b2ed1..8c281c0d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,11 +11,22 @@ Changelog .. There should always be an "Unreleased" section for changes pending release. -Please See the [releases tab](https://github.com/openedx/xblock-lti-consumer/releases) for the complete changelog. +Please See the `releases tab `_ for the complete changelog. Unreleased ~~~~~~~~~~ +9.0.0 - 2023-03-03 +------------------ +BREAKING CHANGE: + +* Validates Tool provided ``redirect_uri`` against new ``lti_1p3_redirect_uris`` field per + `LTI Specification `_. +* ``lti_1p3_redirect_uris`` defaults to ``{lti_1p3_launch_url, lti_1p3_redirect_uris}`` when empty to provide + minimal distruption to existing integrations. +* **NOTE:** Since the redirect URI was never validated in the past, there is always a chance it is something + other than the launch url/deep linking url, so you may have to explicitly set it as appropriate. + 8.0.1 - 2023-02-03 ------------------ * This releases fixes the PII sharing consent dialog for inline launches to no longer refer to a nonexistent diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py index 4926ee88..08684e4a 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__ = '8.0.1' +__version__ = '9.0.0' diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index 00a31bf1..3076e776 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -37,6 +37,7 @@ def __init__( deployment_id, rsa_key, rsa_key_id, + redirect_uris, tool_key=None, tool_keyset_url=None, ): @@ -48,6 +49,7 @@ def __init__( self.launch_url = lti_launch_url self.client_id = client_id self.deployment_id = deployment_id + self.redirect_uris = redirect_uris # Set up platform message signature class self.key_handler = PlatformKeyHandler(rsa_key, rsa_key_id) @@ -469,9 +471,11 @@ def _validate_preflight_response(self, response): :param response: the preflight response to be validated """ try: + redirect_uri = response.get("redirect_uri") assert response.get("nonce") assert response.get("state") - assert response.get("redirect_uri") + assert redirect_uri + assert redirect_uri in self.redirect_uris assert response.get("client_id") == self.client_id except AssertionError as err: raise exceptions.PreflightRequestValidationFailure() from err @@ -739,6 +743,7 @@ def __init__( deployment_id, rsa_key, rsa_key_id, + redirect_uris, tool_key=None, tool_keyset_url=None, ): @@ -753,6 +758,7 @@ def __init__( deployment_id, rsa_key, rsa_key_id, + redirect_uris, tool_key, tool_keyset_url ) diff --git a/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_authentication.py b/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_authentication.py index 97b4ac65..973c47aa 100644 --- a/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_authentication.py +++ b/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_authentication.py @@ -17,6 +17,7 @@ ISS = "http://test-platform.example/" OIDC_URL = "http://test-platform/oidc" LAUNCH_URL = "http://test-platform/launch" +REDIRECT_URIS = [LAUNCH_URL] CLIENT_ID = "1" DEPLOYMENT_ID = "1" NONCE = "1234" @@ -43,6 +44,7 @@ def setUp(self): deployment_id=DEPLOYMENT_ID, rsa_key=RSA_KEY, rsa_key_id=RSA_KEY_ID, + redirect_uris=REDIRECT_URIS, # Use the same key for testing purposes tool_key=RSA_KEY, ) diff --git a/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_permissions.py b/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_permissions.py index c4d5fb6f..461d5950 100644 --- a/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_permissions.py +++ b/lti_consumer/lti_1p3/tests/extensions/rest_framework/test_permissions.py @@ -20,6 +20,7 @@ ISS = "http://test-platform.example/" OIDC_URL = "http://test-platform/oidc" LAUNCH_URL = "http://test-platform/launch" +REDIRECT_URIS = [LAUNCH_URL] CLIENT_ID = "1" DEPLOYMENT_ID = "1" NONCE = "1234" @@ -46,6 +47,7 @@ def setUp(self): deployment_id=DEPLOYMENT_ID, rsa_key=RSA_KEY, rsa_key_id=RSA_KEY_ID, + redirect_uris=REDIRECT_URIS, # Use the same key for testing purposes tool_key=RSA_KEY ) diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index 7ba2e686..b702a883 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -27,6 +27,7 @@ ISS = "http://test-platform.example/" OIDC_URL = "http://test-platform/oidc" LAUNCH_URL = "http://test-platform/launch" +REDIRECT_URIS = [LAUNCH_URL] CLIENT_ID = "1" DEPLOYMENT_ID = "1" NONCE = "1234" @@ -54,6 +55,7 @@ def setUp(self): deployment_id=DEPLOYMENT_ID, rsa_key=RSA_KEY, rsa_key_id=RSA_KEY_ID, + redirect_uris=REDIRECT_URIS, # Use the same key for testing purposes tool_key=RSA_KEY ) @@ -107,6 +109,7 @@ def _decode_token(self, token): @ddt.data( ({"client_id": CLIENT_ID, "redirect_uri": LAUNCH_URL, "nonce": STATE, "state": STATE}, True), ({"client_id": "2", "redirect_uri": LAUNCH_URL, "nonce": STATE, "state": STATE}, False), + ({"client_id": CLIENT_ID, "redirect_uri": "http://other.url", "nonce": STATE, "state": STATE}, False), ({"redirect_uri": LAUNCH_URL, "nonce": NONCE, "state": STATE}, False), ({"client_id": CLIENT_ID, "nonce": NONCE, "state": STATE}, False), ({"client_id": CLIENT_ID, "redirect_uri": LAUNCH_URL, "state": STATE}, False), @@ -640,6 +643,7 @@ def setUp(self): deployment_id=DEPLOYMENT_ID, rsa_key=RSA_KEY, rsa_key_id=RSA_KEY_ID, + redirect_uris=REDIRECT_URIS, # Use the same key for testing purposes tool_key=RSA_KEY ) @@ -882,6 +886,7 @@ def setUp(self): deployment_id=DEPLOYMENT_ID, rsa_key=RSA_KEY, rsa_key_id=RSA_KEY_ID, + redirect_uris=REDIRECT_URIS, # Use the same key for testing purposes tool_key=RSA_KEY ) diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index 3fe04ff7..331fd56e 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -321,6 +321,17 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): "prior to doing the launch request." ), ) + lti_1p3_redirect_uris = List( + display_name=_("Registered Redirect URIs"), + help=_( + "Valid urls the Tool may request us to redirect the id token to. The redirect uris " + "are often the same as the launch url/deep linking url so if this field is " + "empty, it will use them as the default. If you need to use different redirect " + "uri's, enter them here. If you use this field you must enter all valid redirect " + "uri's the tool may request." + ), + scope=Scope.settings + ) lti_1p3_tool_key_mode = String( display_name=_("Tool Public Key Mode"), @@ -585,7 +596,7 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): editable_field_names = ( 'display_name', 'description', 'config_type', 'lti_version', 'external_config', # LTI 1.3 variables - 'lti_1p3_launch_url', 'lti_1p3_oidc_url', + 'lti_1p3_launch_url', 'lti_1p3_redirect_uris', 'lti_1p3_oidc_url', 'lti_1p3_tool_key_mode', 'lti_1p3_tool_keyset_url', 'lti_1p3_tool_public_key', 'lti_1p3_enable_nrps', # LTI Advantage variables diff --git a/lti_consumer/migrations/0017_lticonfiguration_lti_1p3_redirect_uris.py b/lti_consumer/migrations/0017_lticonfiguration_lti_1p3_redirect_uris.py new file mode 100644 index 00000000..02afbcbb --- /dev/null +++ b/lti_consumer/migrations/0017_lticonfiguration_lti_1p3_redirect_uris.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.15 on 2022-11-23 18:19 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_consumer', '0016_lticonfiguration_lti_1p3_proctoring_enabled'), + ] + + operations = [ + migrations.AddField( + model_name='lticonfiguration', + name='lti_1p3_redirect_uris', + field=models.JSONField(blank=True, default=list, help_text="Valid urls the Tool may request us to redirect the id token to. The redirect uris are often the same as the launch url/deep linking url so if this field is empty, it will use them as the default. If you need to use different redirect uri's, enter them here. If you use this field you must enter all valid redirect uri's the tool may request.", verbose_name='LTI 1.3 Redirect URIs'), + ), + ] diff --git a/lti_consumer/models.py b/lti_consumer/models.py index dce26c07..86b913b8 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -28,6 +28,7 @@ get_lti_ags_lineitems_url, get_lti_deeplinking_response_url, get_lti_nrps_context_membership_url, + choose_lti_1p3_redirect_uris, ) log = logging.getLogger(__name__) @@ -185,6 +186,17 @@ class LtiConfiguration(models.Model): 'Tool. One of either lti_1p3_tool_public_key or lti_1p3_tool_keyset_url must not be blank.' ) + lti_1p3_redirect_uris = models.JSONField( + "LTI 1.3 Redirect URIs", + default=list, + blank=True, + help_text="Valid urls the Tool may request us to redirect the id token to. The redirect uris " + "are often the same as the launch url/deep linking url so if this field is " + "empty, it will use them as the default. If you need to use different redirect " + "uri's, enter them here. If you use this field you must enter all valid redirect " + "uri's the tool may request." + ) + # LTI 1.3 Advantage Related Variables lti_advantage_enable_nrps = models.BooleanField( "Enable LTI Advantage Names and Role Provisioning Services", @@ -498,6 +510,8 @@ def _get_lti_1p3_consumer(self): # XBlock Private RSA Key rsa_key=self.lti_1p3_private_key, rsa_key_id=self.lti_1p3_private_key_id, + # Registered redirect uris + redirect_uris=self.get_lti_1p3_redirect_uris(), # LTI 1.3 Tool key/keyset url tool_key=block.lti_1p3_tool_public_key, tool_keyset_url=block.lti_1p3_tool_keyset_url, @@ -514,6 +528,8 @@ def _get_lti_1p3_consumer(self): # XBlock Private RSA Key rsa_key=self.lti_1p3_private_key, rsa_key_id=self.lti_1p3_private_key_id, + # Registered redirect uris + redirect_uris=self.get_lti_1p3_redirect_uris(), # LTI 1.3 Tool key/keyset url tool_key=self.lti_1p3_tool_public_key, tool_keyset_url=self.lti_1p3_tool_keyset_url, @@ -539,6 +555,30 @@ def get_lti_consumer(self): return self._get_lti_1p1_consumer() + 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: + 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 + else: + block = compat.load_enough_xblock(self.location) + redirect_uris = block.lti_1p3_redirect_uris + launch_url = block.lti_1p3_launch_url + deep_link_launch_url = block.lti_advantage_deep_linking_launch_url + + return choose_lti_1p3_redirect_uris( + redirect_uris, + launch_url, + deep_link_launch_url + ) + @property def pii_share_username(self): return self.lti_config.get('pii_share_username', False) # pylint: disable=no-member diff --git a/lti_consumer/static/js/xblock_studio_view.js b/lti_consumer/static/js/xblock_studio_view.js index 0be6b71a..b920a12e 100644 --- a/lti_consumer/static/js/xblock_studio_view.js +++ b/lti_consumer/static/js/xblock_studio_view.js @@ -13,6 +13,7 @@ function LtiConsumerXBlockInitStudio(runtime, element) { const lti1P3FieldList = [ "lti_1p3_launch_url", + "lti_1p3_redirect_uris", "lti_1p3_oidc_url", "lti_1p3_tool_key_mode", "lti_1p3_tool_keyset_url", @@ -60,14 +61,14 @@ function LtiConsumerXBlockInitStudio(runtime, element) { lti1P1FieldList.forEach(function (field) { fieldsToHide.push(field); }); - } else {} + } else { } return fieldsToHide; } /** - * Return fields that should be hidden based on the selected config type. + * Return fields that should be hidden based on the selected config type. * * new - Show all the LTI 1.1/1.3 config fields * database - Do not show the LTI 1.1/1.3 config fields @@ -152,7 +153,7 @@ function LtiConsumerXBlockInitStudio(runtime, element) { for (const field of hiddenFields) { toggleFieldVisibility(field, false); - } + } } // Call once component is instanced to hide fields diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 543c24c7..fd70dd60 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -115,7 +115,8 @@ def setUp(self): self.config = LtiConfiguration( version=LtiConfiguration.LTI_1P3, location=self.location, - config_store=LtiConfiguration.CONFIG_ON_DB + config_store=LtiConfiguration.CONFIG_ON_DB, + lti_1p3_redirect_uris=["https://tool.example", "http://tool.example/launch"] ) self.config.save() diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index c3c305a7..74cb48af 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -19,6 +19,9 @@ LtiDlContentItem) from lti_consumer.tests.test_utils import make_xblock +LAUNCH_URL = 'http://tool.example/launch' +DEEP_LINK_URL = 'http://tool.example/deep-link/launch' + @ddt.ddt class TestLtiConfigurationModel(TestCase): @@ -39,7 +42,7 @@ def setUp(self): self.xblock_attributes = { 'lti_version': 'lti_1p3', - 'lti_1p3_launch_url': 'http://tool.example/launch', + 'lti_1p3_launch_url': LAUNCH_URL, 'lti_1p3_oidc_url': 'http://tool.example/oidc', # We need to set the values below because they are not automatically # generated until the user selects `lti_version == 'lti_1p3'` on the @@ -372,6 +375,46 @@ 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"]), + ) + @ddt.unpack + def test_get_redirect_uris_for_xblock_model_returns_expected( + self, launch_url, deep_link_url, redirect_uris, expected): + """ + Returns expected redirect uris for xblock model + """ + self.xblock.lti_1p3_launch_url = launch_url + self.xblock.lti_advantage_deep_linking_launch_url = deep_link_url + self.xblock.lti_1p3_redirect_uris = redirect_uris + + assert self.lti_1p3_config.get_lti_1p3_redirect_uris() == expected + + @ddt.data( + (LAUNCH_URL, DEEP_LINK_URL, [], [LAUNCH_URL, DEEP_LINK_URL]), + (LAUNCH_URL, DEEP_LINK_URL, ["http://other.url"], ["http://other.url"]), + ) + @ddt.unpack + def test_get_redirect_uris_for_db_model_returns_expected( + self, launch_url, deep_link_url, redirect_uris, expected): + """ + Returns expected redirect uris for db model + """ + self.lti_1p3_config_db.lti_1p3_launch_url = launch_url + self.lti_1p3_config_db.lti_advantage_deep_linking_launch_url = deep_link_url + self.lti_1p3_config_db.lti_1p3_redirect_uris = redirect_uris + self.lti_1p3_config_db.save() + + assert self.lti_1p3_config_db.get_lti_1p3_redirect_uris() == expected + class TestLtiAgsLineItemModel(TestCase): """ diff --git a/lti_consumer/tests/unit/test_utils.py b/lti_consumer/tests/unit/test_utils.py index 43937722..53ecb3a1 100644 --- a/lti_consumer/tests/unit/test_utils.py +++ b/lti_consumer/tests/unit/test_utils.py @@ -8,12 +8,16 @@ from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE from lti_consumer.utils import ( + choose_lti_1p3_redirect_uris, get_lti_1p3_context_types_claim, get_lti_1p3_launch_data_cache_key, cache_lti_1p3_launch_data, get_data_from_cache, ) +LAUNCH_URL = "http://tool.launch" +DEEP_LINK_URL = "http://tool.deep.launch" + @ddt.ddt class TestGetLti1p3ContextTypesClaim(TestCase): @@ -113,3 +117,23 @@ def test_get_data_from_cache(self, is_found, mock_get_cached_response): self.assertEqual(value, "value") else: self.assertIsNone(value) + + @ddt.data( + ("", "", [], []), + (LAUNCH_URL, "", [], [LAUNCH_URL]), + ("", DEEP_LINK_URL, [], [DEEP_LINK_URL]), + (LAUNCH_URL, DEEP_LINK_URL, [], [LAUNCH_URL, DEEP_LINK_URL]), + (LAUNCH_URL, DEEP_LINK_URL, ["http://other.url"], ["http://other.url"]), + ) + @ddt.unpack + def test_choose_lti_1p3_redirect_uri_returns_expected(self, launch_url, deep_link_url, redirect_uris, expected): + """ + Returns redirect_uris if set, else returns launch/deep_link urls as defaults + """ + result = choose_lti_1p3_redirect_uris( + redirect_uris=redirect_uris, + launch_url=launch_url, + deep_link_url=deep_link_url + ) + + assert result == expected diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index e7a3e75d..8f186d7d 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -249,6 +249,19 @@ def get_lti_1p3_context_types_claim(context_types): return lti_context_types +def choose_lti_1p3_redirect_uris(redirect_uris, launch_url, deep_link_url): + """ + Return provided redirect_uris if set, else use launch/deep_link as defaults + """ + if redirect_uris: + return redirect_uris + + result = [launch_url] if launch_url else [] + if deep_link_url: + result.append(deep_link_url) + return result + + def get_lti_1p3_launch_data_cache_key(launch_data): """ Return the cache key for the instance of Lti1p3LaunchData.