299 validate redirect uris (#302)

* Adds lti_1p3_redirect_uris to XBlock
* Adds lti_1p3_redirect_uris to LTIConfiguration model
* Asserts redirect_uri from preflight response in valid redirect uris during preflight validation
* To minimize disruption for previous versions, uses current launch url and deep linking url as default registered redirect_uris if redirect_uris is not explicitly set

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 <https://github.com/openedx/xblock-lti-consumer/releases>`_ 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 <https://www.imsglobal.org/spec/security/v1p0/#step-3-authentication-response>`_.
+* ``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

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'

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
         )

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,
         )

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
         )

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
         )

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

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'),
+        ),
+    ]

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

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

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()
 

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):
     """