From 5296d260308fc4957d3d8f6972f295dbe45980d8 Mon Sep 17 00:00:00 2001
From: Giovanni Cimolin da Silva <giovannicimolin@gmail.com>
Date: Mon, 7 Dec 2020 12:15:45 -0300
Subject: [PATCH] Simplify LTI 1.3 Launches
Remove the OIDC view from XBlock and return OIDC url directly.
---
README.rst | 5 ++
lti_consumer/api.py | 56 ++++++++++++-------
lti_consumer/lti_1p3/consumer.py | 4 +-
lti_consumer/lti_1p3/tests/test_consumer.py | 2 +-
lti_consumer/lti_xblock.py | 56 ++++++++-----------
lti_consumer/templates/html/lti_1p3_oidc.html | 20 -------
lti_consumer/tests/unit/test_api.py | 46 +++++++++++++++
lti_consumer/tests/unit/test_lti_xblock.py | 31 +++-------
setup.py | 2 +-
test_settings.py | 7 ++-
10 files changed, 127 insertions(+), 102 deletions(-)
delete mode 100644 lti_consumer/templates/html/lti_1p3_oidc.html
diff --git a/README.rst b/README.rst
index 99e4c68..9007d3e 100644
--- a/README.rst
+++ b/README.rst
@@ -298,6 +298,11 @@ Please do not report security issues in public. Send security concerns via email
Changelog
=========
+2.4.1 - 2020-12-07
+------------------
+
+* Simplify LTI 1.3 launches by removing OIDC launch start view.
+
2.4.0 - 2020-12-02
------------------
diff --git a/lti_consumer/api.py b/lti_consumer/api.py
index 0a793eb..ce544f3 100644
--- a/lti_consumer/api.py
+++ b/lti_consumer/api.py
@@ -4,6 +4,7 @@ Python APIs used to handle LTI configuration and launches.
Some methods are meant to be used inside the XBlock, so they
return plaintext to allow easy testing/mocking.
"""
+from .exceptions import LtiError
from .models import LtiConfiguration
from .utils import (
get_lms_lti_keyset_link,
@@ -36,12 +37,12 @@ def _get_or_create_local_lti_config(lti_version, block_location):
return lti_config
-def get_lti_consumer(config_id=None, block=None):
+def _get_lti_config(config_id=None, block=None):
"""
- Retrieves an LTI Consumer instance for a given configuration.
+ Retrieves or creates a LTI Configuration using either block or LTI Config ID.
- Returns an instance of LtiConsumer1p1 or LtiConsumer1p3 depending
- on the configuration.
+ This wraps around `_get_or_create_local_lti_config` and handles the block and modulestore
+ bits of configuration.
"""
if config_id:
lti_config = LtiConfiguration.objects.get(pk=config_id)
@@ -53,31 +54,33 @@ def get_lti_consumer(config_id=None, block=None):
# Since the block was passed, preload it to avoid
# having to instance the modulestore and fetch it again.
lti_config.block = block
+ else:
+ raise LtiError('Either a config_id or block is required to get or create an LTI Configuration.')
+ return lti_config
+
+
+def get_lti_consumer(config_id=None, block=None):
+ """
+ Retrieves an LTI Consumer instance for a given configuration.
+
+ Returns an instance of LtiConsumer1p1 or LtiConsumer1p3 depending
+ on the configuration.
+ """
# Return an instance of LTI 1.1 or 1.3 consumer, depending
# on the configuration stored in the model.
- return lti_config.get_lti_consumer()
+ return _get_lti_config(config_id, block).get_lti_consumer()
def get_lti_1p3_launch_info(config_id=None, block=None):
"""
Retrieves the Client ID, Keyset URL and other urls used to configure a LTI tool.
"""
- if not config_id and not block:
- raise Exception("You need to pass either a config_id or a block.")
+ # Retrieve LTI Config
+ lti_config = _get_lti_config(config_id, block)
- if config_id:
- lti_config = LtiConfiguration.objects.get(pk=config_id)
- elif block:
- lti_config = _get_or_create_local_lti_config(
- block.lti_version,
- block.location,
- )
- # Since the block was passed, preload it to avoid
- # having to instance the modulestore and fetch it again.
- lti_config.block = block
-
- launch_info = {
+ # Return LTI launch information for end user configuration
+ return {
'client_id': lti_config.lti_1p3_client_id,
'keyset_url': get_lms_lti_keyset_link(lti_config.location),
'deployment_id': '1',
@@ -85,4 +88,17 @@ def get_lti_1p3_launch_info(config_id=None, block=None):
'token_url': get_lms_lti_access_token_link(lti_config.location),
}
- return launch_info
+
+def get_lti_1p3_launch_start_url(config_id=None, block=None, deep_link_launch=False, hint=""):
+ """
+ Computes and retrieves the LTI URL that starts the OIDC flow.
+ """
+ # Retrieve LTI consumer
+ lti_consumer = get_lti_consumer(config_id, block)
+
+ # Prepare and return OIDC flow start url
+ return lti_consumer.prepare_preflight_url(
+ callback_url=get_lms_lti_launch_link(),
+ hint=hint,
+ lti_hint=("deep_linking_launch" if deep_link_launch else "")
+ )
diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py
index ca641d1..7f15873 100644
--- a/lti_consumer/lti_1p3/consumer.py
+++ b/lti_consumer/lti_1p3/consumer.py
@@ -105,9 +105,7 @@ class LtiConsumer1p3:
"lti_message_hint": lti_hint
}
- return {
- "oidc_url": oidc_url + urlencode(parameters),
- }
+ return oidc_url + urlencode(parameters)
def set_user_data(
self,
diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py
index edfc248..65e7956 100644
--- a/lti_consumer/lti_1p3/tests/test_consumer.py
+++ b/lti_consumer/lti_1p3/tests/test_consumer.py
@@ -158,7 +158,7 @@ class TestLti1p3Consumer(TestCase):
)
# Extract and check parameters from OIDC launch request url
- parameters = parse_qs(urlparse(preflight_request_data['oidc_url']).query)
+ parameters = parse_qs(urlparse(preflight_request_data).query)
self.assertCountEqual(
parameters.keys(),
[
diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py
index 7fd6aca..8bb2454 100644
--- a/lti_consumer/lti_xblock.py
+++ b/lti_consumer/lti_xblock.py
@@ -83,7 +83,6 @@ from .lti_1p3.constants import LTI_1P3_CONTEXT_TYPE
from .outcomes import OutcomeService
from .utils import (
_,
- get_lms_lti_launch_link,
lti_1p3_enabled,
)
@@ -983,32 +982,6 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
template = loader.render_mako_template('/templates/html/lti_launch.html', context)
return Response(template, content_type='text/html')
- @XBlock.handler
- def lti_1p3_launch_handler(self, request, suffix=''): # pylint: disable=unused-argument
- """
- XBlock handler for launching the LTI 1.3 tools.
-
- Displays a form with the OIDC preflight request and
- submits it to the tool.
-
- Arguments:
- request (xblock.django.request.DjangoWebobRequest): Request object for current HTTP request
-
- Returns:
- webob.response: HTML LTI launch form
- """
- lti_consumer = self._get_lti_consumer()
-
- context = lti_consumer.prepare_preflight_url(
- callback_url=get_lms_lti_launch_link(),
- hint=str(self.location), # pylint: disable=no-member
- lti_hint=str(self.location) # pylint: disable=no-member
- )
-
- loader = ResourceLoader(__name__)
- template = loader.render_mako_template('/templates/html/lti_1p3_oidc.html', context)
- return Response(template, content_type='text/html')
-
@XBlock.handler
def lti_1p3_launch_callback(self, request, suffix=''): # pylint: disable=unused-argument
"""
@@ -1055,11 +1028,18 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
context_label=self.context_id
)
+ # Retrieve preflight response
+ preflight_response = dict(request.GET)
+
+ # Set LTI Launch URL
+ context.update({'launch_url': self.lti_1p3_launch_url})
+
+ # Update context with LTI launch parameters
context.update({
- "preflight_response": dict(request.GET),
+ "preflight_response": preflight_response,
"launch_request": lti_consumer.generate_launch_request(
resource_link=str(self.location), # pylint: disable=no-member
- preflight_response=dict(request.GET)
+ preflight_response=preflight_response
)
})
@@ -1357,10 +1337,20 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
allowed_attributes = dict(bleach.sanitizer.ALLOWED_ATTRIBUTES, **{'img': ['src', 'alt']})
sanitized_comment = bleach.clean(self.score_comment, tags=allowed_tags, attributes=allowed_attributes)
- # Set launch handler depending on LTI version
- lti_block_launch_handler = self.runtime.handler_url(self, 'lti_launch_handler').rstrip('/?')
- if self.lti_version == 'lti_1p3':
- lti_block_launch_handler = self.runtime.handler_url(self, 'lti_1p3_launch_handler').rstrip('/?')
+ if self.lti_version == 'lti_1p1':
+ # Set launch handler depending on LTI version
+ lti_block_launch_handler = self.runtime.handler_url(self, 'lti_launch_handler').rstrip('/?')
+ else:
+ # Runtime import since this will only run in the
+ # Open edX LMS/Studio environments.
+ from lti_consumer.api import get_lti_1p3_launch_start_url # pylint: disable=import-outside-toplevel
+
+ # Retrieve and set LTI 1.3 Launch start URL
+ lti_block_launch_handler = get_lti_1p3_launch_start_url(
+ block=self,
+ deep_link_launch=False,
+ hint=str(self.location) # pylint: disable=no-member
+ )
return {
'launch_url': self.launch_url.strip(),
diff --git a/lti_consumer/templates/html/lti_1p3_oidc.html b/lti_consumer/templates/html/lti_1p3_oidc.html
deleted file mode 100644
index 861b4f5..0000000
--- a/lti_consumer/templates/html/lti_1p3_oidc.html
+++ /dev/null
@@ -1,20 +0,0 @@
-<!DOCTYPE HTML>
-<html>
- <head>
- <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
- <title>LTI</title>
- </head>
- <body>
- <a id="launch-link" href="${oidc_url}">
- Click here to launch activity.
- </a>
- <script type="text/javascript">
- (function (d) {
- var element = d.getElementById("launch-link");
- if (element) {
- element.click();
- }
- }(document));
- </script>
- </body>
-</html>
diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py
index 0f705b3..e0b92e2 100644
--- a/lti_consumer/tests/unit/test_api.py
+++ b/lti_consumer/tests/unit/test_api.py
@@ -1,6 +1,7 @@
"""
Tests for LTI API.
"""
+from Cryptodome.PublicKey import RSA
from django.test.testcases import TestCase
from mock import Mock, patch
@@ -8,8 +9,11 @@ from lti_consumer.api import (
_get_or_create_local_lti_config,
get_lti_consumer,
get_lti_1p3_launch_info,
+ get_lti_1p3_launch_start_url,
)
+from lti_consumer.lti_xblock import LtiConsumerXBlock
from lti_consumer.models import LtiConfiguration
+from lti_consumer.tests.unit.test_utils import make_xblock
class TestGetOrCreateLocalLtiConfiguration(TestCase):
@@ -166,3 +170,45 @@ class TestGetLti1p3LaunchInfo(TestCase):
),
}
)
+
+
+class TestGetLti1p3LaunchUrl(TestCase):
+ """
+ Unit tests for get_lti_1p3_launch_start_url API method.
+ """
+ def test_no_parameters(self):
+ """
+ Check if the API creates a model if no object matching properties is found.
+ """
+ with self.assertRaises(Exception):
+ get_lti_1p3_launch_start_url()
+
+ def test_retrieve_url(self):
+ """
+ Check if the correct launch url is retrieved
+ """
+ # Generate RSA and save exports
+ rsa_key = RSA.generate(1024)
+ public_key = rsa_key.publickey().export_key()
+
+ xblock_attributes = {
+ 'lti_version': 'lti_1p3',
+ 'lti_1p3_launch_url': 'http://tool.example/launch',
+ '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
+ # Studio configuration view.
+ 'lti_1p3_tool_public_key': public_key,
+ }
+ xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attributes)
+ # Set dummy location so that UsageKey lookup is valid
+ xblock.location = 'block-v1:course+test+2020+type@problem+block@test'
+
+ # Call API for normal LTI launch initiation
+ launch_url = get_lti_1p3_launch_start_url(block=xblock, hint="test_hint")
+ self.assertIn('login_hint=test_hint', launch_url)
+ self.assertIn('lti_message_hint=', launch_url)
+
+ # Call API for deep link launch
+ launch_url = get_lti_1p3_launch_start_url(block=xblock, deep_link_launch=True)
+ self.assertIn('lti_message_hint=deep_linking_launch', launch_url)
diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py
index ebd60f8..bfbac79 100644
--- a/lti_consumer/tests/unit/test_lti_xblock.py
+++ b/lti_consumer/tests/unit/test_lti_xblock.py
@@ -388,8 +388,7 @@ class TestEditableFields(TestLtiConsumerXBlock):
"""
return all(field in self.xblock.editable_fields for field in fields)
- @patch('lti_consumer.lti_xblock.lti_1p3_enabled', return_value=False)
- def test_editable_fields_with_no_config(self, lti_1p3_enabled_mock):
+ def test_editable_fields_with_no_config(self):
"""
Test that LTI XBlock's fields (i.e. 'ask_to_send_username' and 'ask_to_send_email')
are editable when lti-configuration service is not provided.
@@ -397,10 +396,8 @@ class TestEditableFields(TestLtiConsumerXBlock):
self.xblock.runtime.service.return_value = None
# Assert that 'ask_to_send_username' and 'ask_to_send_email' are editable.
self.assertTrue(self.are_fields_editable(fields=['ask_to_send_username', 'ask_to_send_email']))
- lti_1p3_enabled_mock.assert_called()
- @patch('lti_consumer.lti_xblock.lti_1p3_enabled', return_value=False)
- def test_editable_fields_when_editing_allowed(self, lti_1p3_enabled_mock):
+ def test_editable_fields_when_editing_allowed(self):
"""
Test that LTI XBlock's fields (i.e. 'ask_to_send_username' and 'ask_to_send_email')
are editable when this XBlock is configured to allow it.
@@ -409,10 +406,8 @@ class TestEditableFields(TestLtiConsumerXBlock):
self.xblock.runtime.service.return_value = self.get_mock_lti_configuration(editable=True)
# Assert that 'ask_to_send_username' and 'ask_to_send_email' are editable.
self.assertTrue(self.are_fields_editable(fields=['ask_to_send_username', 'ask_to_send_email']))
- lti_1p3_enabled_mock.assert_called()
- @patch('lti_consumer.lti_xblock.lti_1p3_enabled', return_value=False)
- def test_editable_fields_when_editing_not_allowed(self, lti_1p3_enabled_mock):
+ def test_editable_fields_when_editing_not_allowed(self):
"""
Test that LTI XBlock's fields (i.e. 'ask_to_send_username' and 'ask_to_send_email')
are not editable when this XBlock is configured to not to allow it.
@@ -421,7 +416,6 @@ class TestEditableFields(TestLtiConsumerXBlock):
self.xblock.runtime.service.return_value = self.get_mock_lti_configuration(editable=False)
# Assert that 'ask_to_send_username' and 'ask_to_send_email' are not editable.
self.assertFalse(self.are_fields_editable(fields=['ask_to_send_username', 'ask_to_send_email']))
- lti_1p3_enabled_mock.assert_called()
@patch('lti_consumer.lti_xblock.lti_1p3_enabled', return_value=True)
def test_lti_1p3_fields_appear_when_enabled(self, lti_1p3_enabled_mock):
@@ -1068,7 +1062,8 @@ class TestGetContext(TestLtiConsumerXBlock):
"""
@ddt.data('lti_1p1', 'lti_1p3')
- def test_context_keys(self, lti_version):
+ @patch('lti_consumer.api.get_lti_1p3_launch_start_url')
+ def test_context_keys(self, lti_version, lti_api_patch):
"""
Test `_get_context_for_template` returns dict with correct keys
"""
@@ -1085,6 +1080,9 @@ class TestGetContext(TestLtiConsumerXBlock):
for key in context_keys:
self.assertIn(key, context)
+ if lti_version == 'lti_1p3':
+ lti_api_patch.assert_called_once()
+
@ddt.data('a', 'abbr', 'acronym', 'b', 'blockquote', 'code', 'em', 'i', 'li', 'ol', 'strong', 'ul', 'img')
def test_comment_allowed_tags(self, tag):
"""
@@ -1188,19 +1186,6 @@ class TestLtiConsumer1p3XBlock(TestCase):
# Set dummy location so that UsageKey lookup is valid
self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test'
- def test_launch_request(self):
- """
- Test LTI 1.3 launch request
- """
- response = self.xblock.lti_1p3_launch_handler(make_request('', 'GET'))
- self.assertEqual(response.status_code, 200)
-
- # Check if tool OIDC url is on page
- self.assertIn(
- self.xblock_attributes['lti_1p3_oidc_url'],
- response.body.decode('utf-8')
- )
-
def test_launch_callback_endpoint(self):
"""
Test the LTI 1.3 callback endpoint.
diff --git a/setup.py b/setup.py
index 658fb32..f82a42b 100644
--- a/setup.py
+++ b/setup.py
@@ -49,7 +49,7 @@ with open('README.rst') as _f:
setup(
name='lti-consumer-xblock',
- version='2.5.0',
+ version='2.5.1',
description='This XBlock implements the consumer side of the LTI specification.',
long_description=long_description,
long_description_content_type='text/x-rst',
diff --git a/test_settings.py b/test_settings.py
index 47f7852..0e12c81 100644
--- a/test_settings.py
+++ b/test_settings.py
@@ -11,4 +11,9 @@ USAGE_ID_PATTERN = r'(?P<usage_id>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|
ROOT_URLCONF = 'test_urls'
# LMS Urls - for LTI 1.3 testing
-LMS_ROOT_URL = "https://example.com"
\ No newline at end of file
+LMS_ROOT_URL = "https://example.com"
+
+# Dummy FEATURES dict
+FEATURES = {
+ 'LTI_1P3_ENABLED': False,
+}
--
GitLab