Merge pull request #216 from open-craft/felipetrz/BB-5168-upstream-jwk-support

giovannicimolin · web-flow · commit 4588b7ac3ac9 · 2022-01-21T13:50:43.000-03:00
[BB-5168] Add support for JWK Keysets for LTI 1.3
diff --git a/README.rst b/README.rst
@@ -367,6 +367,11 @@ Changelog
 
 Please See the [releases tab](https://github.com/edx/xblock-lti-consumer/releases) for the complete changelog.
 
+3.3.0 - 2022-01-20
+-------------------
+
+* Added support for specifying LTI 1.3 JWK URLs.
+
 3.2.0 - 2022-01-18
 -------------------
 
diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py
@@ -4,4 +4,4 @@
 from .lti_xblock import LtiConsumerXBlock
 from .apps import LTIConsumerApp
 
-__version__ = '3.2.0'
+__version__ = '3.3.0'
diff --git a/lti_consumer/lti_1p3/key_handlers.py b/lti_consumer/lti_1p3/key_handlers.py
@@ -68,8 +68,15 @@ def _get_keyset(self, kid=None):
         keyset = []
 
         if self.keyset_url:
-            # TODO: Improve support for keyset handling, handle errors.
-            keyset.extend(load_jwks_from_url(self.keyset_url))
+            try:
+                keys = load_jwks_from_url(self.keyset_url)
+            except Exception as err:
+                # Broad Exception is required here because jwkest raises
+                # an Exception object explicitly.
+                # Beware that many different scenarios are being handled
+                # as an invalid key when the JWK loading fails.
+                raise exceptions.NoSuitableKeys() from err
+            keyset.extend(keys)
 
         if self.public_key and kid:
             # Fill in key id of stored key.
diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py
@@ -283,6 +283,33 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
             "prior to doing the launch request."
         ),
     )
+
+    lti_1p3_tool_key_mode = String(
+        display_name=_("Tool Public Key Mode"),
+        scope=Scope.settings,
+        values=[
+            {"display_name": "Public Key", "value": "public_key"},
+            {"display_name": "Keyset URL", "value": "keyset_url"},
+        ],
+        default="public_key",
+        help=_(
+            "Select how the tool's public key information will be specified."
+        ),
+    )
+    lti_1p3_tool_keyset_url = String(
+        display_name=_("Tool Keyset URL"),
+        default='',
+        scope=Scope.settings,
+        help=_(
+            "Enter the LTI 1.3 Tool's JWK keysets URL."
+            "<br />This link should retrieve a JSON file containing"
+            " public keys and signature algorithm information, so"
+            " that the LMS can check if the messages and launch"
+            " requests received have the signature from the tool."
+            "<br /><b>This is not required when doing LTI 1.3 Launches"
+            " without LTI Advantage nor Basic Outcomes requests.</b>"
+        ),
+    )
     lti_1p3_tool_public_key = String(
         display_name=_("Tool Public Key"),
         multiline_editor=True,
@@ -520,7 +547,9 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
     editable_field_names = (
         'display_name', 'description',
         # LTI 1.3 variables
-        'lti_version', 'lti_1p3_launch_url', 'lti_1p3_oidc_url', 'lti_1p3_tool_public_key', 'lti_1p3_enable_nrps',
+        'lti_version', 'lti_1p3_launch_url', '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_advantage_deep_linking_enabled', 'lti_advantage_deep_linking_launch_url',
         'lti_advantage_ags_mode',
@@ -574,6 +603,12 @@ def validate_field_data(self, validation, data):
                 _("Custom Parameters must be a list")
             )))
 
+        # keyset URL and public key are mutually exclusive
+        if data.lti_1p3_tool_key_mode == 'keyset_url':
+            data.lti_1p3_tool_public_key = ''
+        elif data.lti_1p3_tool_key_mode == 'public_key':
+            data.lti_1p3_tool_keyset_url = ''
+
     def get_settings(self):
         """
         Get the XBlock settings bucket via the SettingsService.
diff --git a/lti_consumer/models.py b/lti_consumer/models.py
@@ -266,7 +266,7 @@ def _get_lti_1p3_consumer(self):
                 rsa_key_id=self.lti_1p3_private_key_id,
                 # LTI 1.3 Tool key/keyset url
                 tool_key=self.block.lti_1p3_tool_public_key,
-                tool_keyset_url=None,
+                tool_keyset_url=self.block.lti_1p3_tool_keyset_url,
             )
 
             # Check if enabled and setup LTI-AGS
diff --git a/lti_consumer/static/js/xblock_studio_view.js b/lti_consumer/static/js/xblock_studio_view.js
@@ -14,6 +14,8 @@ function LtiConsumerXBlockInitStudio(runtime, element) {
     const lti1P3FieldList = [
         "lti_1p3_launch_url",
         "lti_1p3_oidc_url",
+        "lti_1p3_tool_key_mode",
+        "lti_1p3_tool_keyset_url",
         "lti_1p3_tool_public_key",
         "lti_advantage_ags_mode",
         "lti_advantage_deep_linking_enabled",
@@ -72,12 +74,37 @@ function LtiConsumerXBlockInitStudio(runtime, element) {
         });
     }
 
+    /**
+     * Only display the field appropriate for the selected key mode.
+     */
+    function toggleLtiToolKeyMode() {
+        const ltiKeyModeField = $(element).find('#xb-field-edit-lti_1p3_tool_key_mode');
+
+        // find the field containers
+        const ltiKeysetUrlField = $(element).find('[data-field-name=lti_1p3_tool_keyset_url]');
+        const ltiPublicKeyField = $(element).find('[data-field-name=lti_1p3_tool_public_key]');
+
+        const selectedKeyMode = ltiKeyModeField.children("option:selected").val();
+        if (selectedKeyMode === 'public_key') {
+            ltiKeysetUrlField.hide();
+            ltiPublicKeyField.show();
+        } else if (selectedKeyMode === 'keyset_url') {
+            ltiPublicKeyField.hide();
+            ltiKeysetUrlField.show();
+        }
+    }
+
     // Call once component is instanced to hide fields
     toggleLtiFields();
+    toggleLtiToolKeyMode();
 
     // Bind to onChange method of lti_version selector
     $(element).find('#xb-field-edit-lti_version').bind('change', function() {
         toggleLtiFields();
-     });
+    });
 
+    // Bind to onChange method of lti_1p3_tool_key_mode selector
+    $(element).find('#xb-field-edit-lti_1p3_tool_key_mode').bind('change', function() {
+        toggleLtiToolKeyMode();
+    });
 }
diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py
@@ -4,7 +4,6 @@
 
 import json
 import logging
-import urllib.parse
 from datetime import timedelta
 from unittest.mock import Mock, NonCallableMock, PropertyMock, patch
 
@@ -13,14 +12,14 @@
 from django.conf import settings as dj_settings
 from django.test.testcases import TestCase
 from django.utils import timezone
-from jwkest.jwk import RSAKey
+from jwkest.jwk import RSAKey, KEYS
 
 from lti_consumer.api import get_lti_1p3_launch_info
 from lti_consumer.exceptions import LtiError
 from lti_consumer.lti_1p3.tests.utils import create_jwt
 from lti_consumer.lti_xblock import LtiConsumerXBlock, parse_handler_suffix
 from lti_consumer.tests.unit import test_utils
-from lti_consumer.tests.unit.test_utils import FAKE_USER_ID, make_request, make_xblock
+from lti_consumer.tests.unit.test_utils import FAKE_USER_ID, make_jwt_request, make_request, make_xblock
 from lti_consumer.utils import resolve_custom_parameter_template
 
 HTML_PROBLEM_PROGRESS = '<div class="problem-progress">'
@@ -452,6 +451,8 @@ def test_lti_1p3_fields_appear(self):
                     'lti_version',
                     'lti_1p3_launch_url',
                     'lti_1p3_oidc_url',
+                    'lti_1p3_tool_key_mode',
+                    'lti_1p3_tool_keyset_url',
                     'lti_1p3_tool_public_key',
                     'lti_advantage_deep_linking_enabled',
                     'lti_advantage_deep_linking_launch_url',
@@ -1484,17 +1485,7 @@ def test_access_token_malformed(self):
         """
         Test request with invalid JWT.
         """
-        request = make_request(
-            urllib.parse.urlencode({
-                "grant_type": "client_credentials",
-                "client_assertion_type": "something",
-                "client_assertion": "invalid-jwt",
-                "scope": "",
-            }),
-            'POST'
-        )
-        request.content_type = 'application/x-www-form-urlencoded'
-
+        request = make_jwt_request("invalid-jwt")
         response = self.xblock.lti_1p3_access_token(request)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json_body, {'error': 'invalid_grant'})
@@ -1503,15 +1494,7 @@ def test_access_token_invalid_grant(self):
         """
         Test request with invalid grant.
         """
-        request = make_request(
-            urllib.parse.urlencode({
-                "grant_type": "password",
-                "client_assertion_type": "something",
-                "client_assertion": "invalit-jwt",
-                "scope": "",
-            }),
-            'POST'
-        )
+        request = make_jwt_request("invalid-jwt", grant_type="password")
         request.content_type = 'application/x-www-form-urlencoded'
 
         response = self.xblock.lti_1p3_access_token(request)
@@ -1526,17 +1509,7 @@ def test_access_token_invalid_client(self):
         self.xblock.save()
 
         jwt = create_jwt(self.key, {})
-        request = make_request(
-            urllib.parse.urlencode({
-                "grant_type": "client_credentials",
-                "client_assertion_type": "something",
-                "client_assertion": jwt,
-                "scope": "",
-            }),
-            'POST'
-        )
-        request.content_type = 'application/x-www-form-urlencoded'
-
+        request = make_jwt_request(jwt)
         response = self.xblock.lti_1p3_access_token(request)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json_body, {'error': 'invalid_client'})
@@ -1546,17 +1519,7 @@ def test_access_token(self):
         Test request with valid JWT.
         """
         jwt = create_jwt(self.key, {})
-        request = make_request(
-            urllib.parse.urlencode({
-                "grant_type": "client_credentials",
-                "client_assertion_type": "something",
-                "client_assertion": jwt,
-                "scope": "",
-            }),
-            'POST'
-        )
-        request.content_type = 'application/x-www-form-urlencoded'
-
+        request = make_jwt_request(jwt)
         response = self.xblock.lti_1p3_access_token(request)
         self.assertEqual(response.status_code, 200)
 
@@ -1633,3 +1596,87 @@ def test_lti_custom_param_templates_not_configured(self, mock_import_module, moc
         self.assertEqual(resolved_value, custom_parameter_template_value)
         assert mock_log.error.called
         mock_import_module.asser_not_called()
+
+
+class TestLti1p3AccessTokenJWK(TestCase):
+    """
+    Unit tests for LtiConsumerXBlock Access Token endpoint when using a
+    LTI 1.3 setup with JWK authentication.
+    """
+    def setUp(self):
+        super().setUp()
+        self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, {
+            'lti_version': 'lti_1p3',
+            'lti_1p3_launch_url': 'http://tool.example/launch',
+            'lti_1p3_oidc_url': 'http://tool.example/oidc',
+            'lti_1p3_tool_keyset_url': "http://tool.example/keyset",
+        })
+        self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test'
+        self.xblock.save()
+
+        self.key = RSAKey(key=RSA.generate(2048), kid="1")
+
+        jwt = create_jwt(self.key, {})
+        self.request = make_jwt_request(jwt)
+
+    def make_keyset(self, keys):
+        """
+        Builds a keyset object with the given keys.
+        """
+        jwks = KEYS()
+        jwks._keys = keys  # pylint: disable=protected-access
+        return jwks
+
+    @patch("lti_consumer.lti_1p3.key_handlers.load_jwks_from_url")
+    def test_access_token_using_keyset_url(self, load_jwks_from_url):
+        """
+        Test request using the provider's keyset URL instead of a public key.
+        """
+        load_jwks_from_url.return_value = self.make_keyset([self.key])
+        response = self.xblock.lti_1p3_access_token(self.request)
+        load_jwks_from_url.assert_called_once_with("http://tool.example/keyset")
+        self.assertEqual(response.status_code, 200)
+
+    @patch("lti_consumer.lti_1p3.key_handlers.load_jwks_from_url")
+    def test_access_token_using_keyset_url_with_empty_keys(self, load_jwks_from_url):
+        """
+        Test request where the provider's keyset URL returns an empty list of keys.
+        """
+        load_jwks_from_url.return_value = self.make_keyset([])
+        response = self.xblock.lti_1p3_access_token(self.request)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json_body, {"error": "invalid_client"})
+
+    @patch("lti_consumer.lti_1p3.key_handlers.load_jwks_from_url")
+    def test_access_token_using_keyset_url_with_wrong_keys(self, load_jwks_from_url):
+        """
+        Test request where the provider's keyset URL returns wrong keys.
+        """
+        key = RSAKey(key=RSA.generate(2048), kid="2")
+        load_jwks_from_url.return_value = self.make_keyset([key])
+        response = self.xblock.lti_1p3_access_token(self.request)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json_body, {"error": "invalid_client"})
+
+    @patch("jwkest.jwk.request")
+    def test_access_token_using_keyset_url_that_fails(self, request):
+        """
+        Test request where the provider's keyset URL request fails.
+        """
+        request.side_effect = Exception("request fails")
+        response = self.xblock.lti_1p3_access_token(self.request)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json_body, {'error': 'invalid_client'})
+
+    @patch("jwkest.jwk.request")
+    def test_access_token_using_keyset_url_with_invalid_contents(self, request):
+        """
+        Test request where the provider's keyset URL doesn't return valid JSON.
+        """
+        response_mock = Mock()
+        response_mock.status_code = 200
+        response_mock.text = b'this is not a valid json'
+        request.return_value = response_mock
+        response = self.xblock.lti_1p3_access_token(self.request)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json_body, {'error': 'invalid_client'})
diff --git a/lti_consumer/tests/unit/test_utils.py b/lti_consumer/tests/unit/test_utils.py
@@ -3,6 +3,7 @@
 """
 
 from unittest.mock import Mock
+import urllib
 from webob import Request
 from workbench.runtime import WorkbenchRuntime
 from xblock.fields import ScopeIds
@@ -53,6 +54,21 @@ def make_request(body, method='POST'):
     return request
 
 
+def make_jwt_request(token, **overrides):
+    """
+    Builds a Request with a JWT body.
+    """
+    body = {
+        "grant_type": "client_credentials",
+        "client_assertion_type": "something",
+        "client_assertion": token,
+        "scope": "",
+    }
+    request = make_request(urllib.parse.urlencode({**body, **overrides}), 'POST')
+    request.content_type = 'application/x-www-form-urlencoded'
+    return request
+
+
 def dummy_processor(_xblock):
     """
     A dummy LTI parameter processor.
diff --git a/lti_consumer/translations/en/LC_MESSAGES/text.po b/lti_consumer/translations/en/LC_MESSAGES/text.po

Original file line number	Diff line number	Diff line change
`@@ -266,7 +266,7 @@ def _get_lti_1p3_consumer(self):`
`266`	`266`	`rsa_key_id=self.lti_1p3_private_key_id,`
`267`	`267`	`# LTI 1.3 Tool key/keyset url`
`268`	`268`	`tool_key=self.block.lti_1p3_tool_public_key,`
`269`		`- tool_keyset_url=None,`
	`269`	`+ tool_keyset_url=self.block.lti_1p3_tool_keyset_url,`
`270`	`270`	`)`
`271`	`271`
`272`	`272`	`# Check if enabled and setup LTI-AGS`