diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e0a76ab1deb42a0ab17e85eb002b1795279cb71c..39be79612f595306a31b686aef02ded4e0911598 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,27 @@ Please See the [releases tab](https://github.com/edx/xblock-lti-consumer/release Unreleased ~~~~~~~~~~ +5.0.0 - 2022-10-12 +------------------ +BREAKING CHANGE: + +Please note that additional breaking changes will be forthcoming in future versions of this library. + +* Modified Python API methods to take Lti1p3LaunchData as a required argument +** get_lti_1p3_launch_info +** get_lti_1p3_launch_start_url +** get_lti_1p3_content_url + +* Added an Lti1p3LaunchData data class +* Added caching for Lti1p3LaunchData to limit data sent in request query or form parameters +* Replaced references to LtiConsumerXBlock.location with Lti1p3LaunchData.config_id +* Removed definition of key LTI 1.3 claims from the launch_gate_endpoint and instantiated Lti1p3LaunchData from within + the LtiConsumerXBlock instead +* Added a required launch_data_key request query parameter to the deep_linking_content_endpoint and refactored + associated templates and template tags to pass this parameter in the request to the view +* Changed the access token URL and Keyset URL to use the LtiConfiguration.config_id in the URL instead of the + LtiConfiguration.location + 4.4.0 - 2022-08-17 ------------------ * Move the LTI 1.3 Access Token and Launch Callback endpoint logic from the XBlock to the Django views diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py index 0bcf3e49f7aa6cb28612ac2dae3e126504ecedc0..0e15340785ea2f0e2a1dbbcd7522144122c86585 100644 --- a/lti_consumer/__init__.py +++ b/lti_consumer/__init__.py @@ -4,4 +4,4 @@ Runtime will load the XBlock class from here. from .apps import LTIConsumerApp from .lti_xblock import LtiConsumerXBlock -__version__ = '4.5.0' +__version__ = '5.0.0' diff --git a/lti_consumer/api.py b/lti_consumer/api.py index d03be168810b0cdd9746ad018fbaba4fb513b7ef..3a4ff706a5449ea6d3f2a3470689dd4a675b2a3b 100644 --- a/lti_consumer/api.py +++ b/lti_consumer/api.py @@ -9,9 +9,11 @@ import json from opaque_keys.edx.keys import CourseKey +from lti_consumer.lti_1p3.constants import LTI_1P3_ROLE_MAP from .exceptions import LtiError from .models import CourseAllowPIISharingInLTIFlag, LtiConfiguration, LtiDlContentItem from .utils import ( + get_lti_1p3_context_types_claim, get_lti_deeplinking_content_url, get_lms_lti_keyset_link, get_lms_lti_launch_link, @@ -24,7 +26,7 @@ def _get_or_create_local_lti_config(lti_version, block_location, config_store=LtiConfiguration.CONFIG_ON_XBLOCK, external_id=None): """ Retrieve the LtiConfiguration for the block described by block_location, if one exists. If one does not exist, - create an LtiConfiguration with the LtiConfiguration.CONFIG_ON_DB config_store. + create an LtiConfiguration with the LtiConfiguration.CONFIG_ON_XBLOCK config_store. Treat the lti_version argument as the source of truth for LtiConfiguration.version and override the LtiConfiguration.version with lti_version. This allows, for example, for @@ -100,7 +102,11 @@ def get_lti_consumer(config_id=None, block=None): return _get_lti_config(config_id, block).get_lti_consumer() -def get_lti_1p3_launch_info(config_id=None, block=None): +def get_lti_1p3_launch_info( + launch_data, + config_id=None, + block=None, +): """ Retrieves the Client ID, Keyset URL and other urls used to configure a LTI tool. """ @@ -116,9 +122,9 @@ def get_lti_1p3_launch_info(config_id=None, block=None): deep_linking_enabled = lti_consumer.lti_dl_enabled() if deep_linking_enabled: + launch_data.message_type = "LtiDeepLinkingRequest" deep_linking_launch_url = lti_consumer.prepare_preflight_url( - hint=lti_config.location, - lti_hint="deep_linking_launch" + launch_data, ) # Retrieve LTI Content Items (if any was set up) @@ -129,22 +135,28 @@ def get_lti_1p3_launch_info(config_id=None, block=None): if dl_content_items.exists(): deep_linking_content_items = [item.attributes for item in dl_content_items] - link_location = lti_config.location if lti_config.location else lti_config.config_id + config_id = lti_config.config_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(link_location), + 'keyset_url': get_lms_lti_keyset_link(config_id), 'deployment_id': '1', 'oidc_callback': get_lms_lti_launch_link(), - 'token_url': get_lms_lti_access_token_link(link_location), + '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, } -def get_lti_1p3_launch_start_url(config_id=None, block=None, deep_link_launch=False, dl_content_id=None, hint=""): +def get_lti_1p3_launch_start_url( + launch_data, + config_id=None, + block=None, + deep_link_launch=False, + dl_content_id=None, +): """ Computes and retrieves the LTI URL that starts the OIDC flow. """ @@ -152,25 +164,25 @@ def get_lti_1p3_launch_start_url(config_id=None, block=None, deep_link_launch=Fa lti_config = _get_lti_config(config_id, block) lti_consumer = lti_config.get_lti_consumer() - # Change LTI hint depending on LTI launch type - lti_hint = "" + # Include a message hint in the launch_data depending on LTI launch type # Case 1: Performs Deep Linking configuration flow. Triggered by staff users to # configure tool options and select content to be presented. if deep_link_launch: - lti_hint = "deep_linking_launch" + launch_data.message_type = "LtiDeepLinkingRequest" # Case 2: Perform a LTI Launch for `ltiResourceLink` content types, since they # need to use the launch mechanism from the callback view. elif dl_content_id: - lti_hint = f"deep_linking_content_launch:{dl_content_id}" + launch_data.deep_linking_content_item_id = dl_content_id # Prepare and return OIDC flow start url - return lti_consumer.prepare_preflight_url( - hint=hint, - lti_hint=lti_hint - ) + return lti_consumer.prepare_preflight_url(launch_data) -def get_lti_1p3_content_url(config_id=None, block=None, hint=""): +def get_lti_1p3_content_url( + launch_data, + config_id=None, + block=None, +): """ Computes and returns which URL the LTI consumer should launch to. @@ -189,20 +201,20 @@ def get_lti_1p3_content_url(config_id=None, block=None, hint=""): # If there's no content items, return normal LTI launch URL if not content_items.count(): - return get_lti_1p3_launch_start_url(config_id, block, hint=hint) + return get_lti_1p3_launch_start_url(launch_data, config_id=config_id, block=block) # If there's a single `ltiResourceLink` content, return the launch - # url for that specif deep link + # url for that specific deep link if content_items.count() == 1 and content_items.get().content_type == LtiDlContentItem.LTI_RESOURCE_LINK: return get_lti_1p3_launch_start_url( - config_id, - block, + launch_data, + config_id=config_id, + block=block, dl_content_id=content_items.get().id, - hint=hint, ) # If there's more than one content item, return content presentation URL - return get_lti_deeplinking_content_url(lti_config.id) + return get_lti_deeplinking_content_url(lti_config.id, launch_data) def get_deep_linking_data(deep_linking_id, config_id=None, block=None): @@ -232,3 +244,40 @@ def get_lti_pii_sharing_state_for_course(course_key: CourseKey) -> bool: bool: The state of PII sharing for this course for LTI. """ return CourseAllowPIISharingInLTIFlag.current(course_key).enabled + + +def validate_lti_1p3_launch_data(launch_data): + """ + Validate that the data in Lti1p3LaunchData are valid and raise an LtiMessageHintValidationFailure exception if they + are not. + + The initializer of the Lti1p3LaunchData takes care of ensuring that required data is provided to the class. This + utility method verifies that other requirements of the data are met. + """ + validation_messages = [] + + # The context claim is an object that composes properties about the context. The claim itself is optional, but if it + # is provided, then the id property is required. Ensure that if any other of the other optional properties are + # provided that the id property is also provided. + if ((launch_data.context_type or launch_data.context_title or launch_data.context_label) and not + launch_data.context_id): + validation_messages.append( + "The context_id attribute is required in the launch data if any optional context properties are provided." + ) + + if launch_data.user_role not in LTI_1P3_ROLE_MAP: + validation_messages.append(f"The user_role attribute {launch_data.user_role} is not a valid user_role.") + + context_type = launch_data.context_type + if context_type: + try: + get_lti_1p3_context_types_claim(context_type) + except ValueError: + validation_messages.append( + f"The context_type attribute {context_type} in the launch data is not a valid context_type." + ) + + if validation_messages: + return False, validation_messages + else: + return True, [] diff --git a/lti_consumer/data.py b/lti_consumer/data.py new file mode 100644 index 0000000000000000000000000000000000000000..7d2bfcfe7c1a7fa42d5323d0660042ba3d8d0000 --- /dev/null +++ b/lti_consumer/data.py @@ -0,0 +1,46 @@ +""" +This modules provides public data structures to represent LTI 1.3 launch data that can be used within this library and +by users of this library. +""" + +from attrs import define, field + + +@define +class Lti1p3LaunchData: + """ + The Lti1p3LaunchData class contains data necessary and related to an LTI 1.3 launch. It is a mechanism to share + launch data between apps. Applications using this library should use the Lti1p3LaunchData class to supply + contextually defined or stored launch data to the generic LTI 1.3 launch. + + * user_id (required): the user's unique identifier + * user_role (required): the user's role as one of the keys in LTI_1P3_ROLE_MAP: staff, instructor, student, or + guest + * config_id (required): the config_id field of an LtiConfiguration to use for the launch + * resource_link_id (required): a unique identifier that is guaranteed to be unique for each placement of the LTI + link + * launch_presentation_document_target (optional): the document_target property of the launch_presentation claim; it + describes the kind of browser window or frame from which the user launched inside the message sender's system; + it is one of frame, iframe, or window; it defaults to iframe + * message_type (optional): the message type of the eventual LTI launch; defaults to LtiResourceLinkRequest + * context_id (conditionally required): the id property of the context claim; the stable, unique identifier for the + context; if any of the context properties are provided, context_id is required + * context_type (optional): the type property of the context claim; a list of some combination of the following valid + context_types: group, course_offering, course_section, or course_template + * context_title (optional): the title proerty of the context claim; a short, descriptive name for the context + * context_label (optional): the label property of the context claim; a full, descriptive name for the context + * deep_linking_context_item_id (optional): the database id of the LtiDlContentItem that should be used for the LTI + 1.3 Deep Linking launch; this is used when the LTI 1.3 Deep Linking launch is a regular LTI resource link + request using a content item that was configured via a previous LTI 1.3 Deep Linking request + """ + user_id = field() + user_role = field() + config_id = field() + resource_link_id = field() + launch_presentation_document_target = field(default="iframe") + message_type = field(default="LtiResourceLinkRequest") + context_id = field(default=None) + context_type = field(default=None) + context_title = field(default=None) + context_label = field(default=None) + deep_linking_content_item_id = field(default=None) diff --git a/lti_consumer/lti_1p1/tests/test_consumer.py b/lti_consumer/lti_1p1/tests/test_consumer.py index 88482b1c70f5f3710a2f6154f4ec994f0cfe3da5..0ae9d04aab52e8c7d406732a55a862a902cc88a0 100644 --- a/lti_consumer/lti_1p1/tests/test_consumer.py +++ b/lti_consumer/lti_1p1/tests/test_consumer.py @@ -8,7 +8,7 @@ from unittest.mock import Mock, patch from lti_consumer.lti_1p1.exceptions import Lti1p1Error from lti_consumer.lti_1p1.consumer import LtiConsumer1p1, parse_result_json -from lti_consumer.tests.unit.test_utils import make_request +from lti_consumer.tests.test_utils import make_request INVALID_JSON_INPUTS = [ ([ diff --git a/lti_consumer/lti_1p1/tests/test_oauth.py b/lti_consumer/lti_1p1/tests/test_oauth.py index 619dd6af2fc5c4faf9cb241b46694e91914440d9..a0899b4cde799a2bb077d1b891548b01005cdcc7 100644 --- a/lti_consumer/lti_1p1/tests/test_oauth.py +++ b/lti_consumer/lti_1p1/tests/test_oauth.py @@ -10,7 +10,7 @@ from lti_consumer.lti_1p1.exceptions import Lti1p1Error from lti_consumer.lti_1p1.oauth import (get_oauth_request_signature, log_authorization_header, verify_oauth_body_signature) -from lti_consumer.tests.unit.test_utils import make_request +from lti_consumer.tests.test_utils import make_request OAUTH_PARAMS = [ ('oauth_nonce', '80966668944732164491378916897'), diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index f2b2a4f6ab58e893d2764e93ecc5a871da922ea4..c45a608384a58e0a758727776f807193e8557f03 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -1,8 +1,11 @@ """ LTI 1.3 Consumer implementation """ +import logging from urllib.parse import urlencode +from lti_consumer.utils import cache_lti_1p3_launch_data, get_data_from_cache + from . import constants, exceptions from .constants import ( LTI_1P3_ROLE_MAP, @@ -16,6 +19,8 @@ from .ags import LtiAgs from .deep_linking import LtiDeepLinking from .nprs import LtiNrps +log = logging.getLogger(__name__) + class LtiConsumer1p3: """ @@ -53,6 +58,7 @@ class LtiConsumer1p3: # IMS LTI Claim data self.lti_claim_user_data = None + self.lti_claim_resource_link = None self.lti_claim_launch_presentation = None self.lti_claim_context = None self.lti_claim_custom_parameters = None @@ -90,20 +96,31 @@ class LtiConsumer1p3: def prepare_preflight_url( self, - hint="hint", - lti_hint="lti_hint" + launch_data, ): """ Generates OIDC url with parameters """ + user_id = launch_data.user_id + + # Set the launch_data in the cache. An LTI 1.3 launch involves two "legs" - the third party initiated + # login request (the preflight request) and the actual launch -, and this information must be shared between + # the two requests. A simple example is the intended LTI launch message of the LTI launch. This value is + # known at the time that preflight request is made, but it is not accessible when the tool responds to the + # preflight request and the platform must craft a launch request. This library stores the launch_data in the + # cache and includes the cache key as the lti_message_hint query or form parameter to retrieve it later. + launch_data_key = cache_lti_1p3_launch_data(launch_data) + oidc_url = self.oidc_url + "?" + + login_hint = user_id parameters = { "iss": self.iss, "client_id": self.client_id, "lti_deployment_id": self.deployment_id, "target_link_uri": self.launch_url, - "login_hint": hint, - "lti_message_hint": lti_hint + "login_hint": login_hint, + "lti_message_hint": launch_data_key, } return oidc_url + urlencode(parameters) @@ -143,6 +160,37 @@ class LtiConsumer1p3: "email": email_address, }) + def set_resource_link_claim( + self, + resource_link_id, + description=None, + title=None, + ): + """ + Set resource_link claim. The resource link must be stable and unique to each deployment_id. This value MUST + change if the link is copied or exported from one system or context and imported into another system or context + + https://www.imsglobal.org/spec/lti/v1p3#resource-link-claim + + Arguments: + * id (string): opaque, unique value identifying the placement of an LTI resource link + * description (string): description for the placement of an LTI resource link + * title (string): title for the placement of an LTI resource link + """ + resource_link_claim_data = { + "id": resource_link_id, + } + + if description: + resource_link_claim_data["description"] = description + + if title: + resource_link_claim_data["title"] = title + + self.lti_claim_resource_link = { + "https://purl.imsglobal.org/spec/lti/claim/resource_link": resource_link_claim_data + } + def set_launch_presentation_claim( self, document_target="iframe" @@ -233,7 +281,6 @@ class LtiConsumer1p3: def get_lti_launch_message( self, - resource_link, include_extra_claims=True, ): """ @@ -263,17 +310,6 @@ class LtiConsumer1p3: # MUST be the same value as the target_link_uri passed by the platform in the OIDC login request # http://www.imsglobal.org/spec/lti/v1p3/#target-link-uri "https://purl.imsglobal.org/spec/lti/claim/target_link_uri": self.launch_url, - - # Resource link: stable and unique to each deployment_id - # This value MUST change if the link is copied or exported from one system or - # context and imported into another system or context - # http://www.imsglobal.org/spec/lti/v1p3/#resource-link-claim - "https://purl.imsglobal.org/spec/lti/claim/resource_link": { - "id": resource_link, - # Optional claims - # "title": "Introduction Assignment" - # "description": "Assignment to introduce who you are", - }, }) # Check if user data is set, then append it to lti message @@ -283,6 +319,13 @@ class LtiConsumer1p3: else: raise ValueError("Required user data isn't set.") + # Check if the resource_link claim has been set and append it to the LTI message if it has. + # The resource_link claim is required, so raise an exception if it has not been set. + if self.lti_claim_resource_link: + lti_message.update(self.lti_claim_resource_link) + else: + raise ValueError("Required resource_link data isn't set.") + # Only used when doing normal LTI launches if include_extra_claims: # Set optional claims @@ -307,7 +350,6 @@ class LtiConsumer1p3: def generate_launch_request( self, preflight_response, - resource_link ): """ Build LTI message from class parameters @@ -319,7 +361,7 @@ class LtiConsumer1p3: self._validate_preflight_response(preflight_response) # Get LTI Launch Message - lti_launch_message = self.get_lti_launch_message(resource_link=resource_link) + lti_launch_message = self.get_lti_launch_message() # Nonce from OIDC preflight launch request lti_launch_message.update({ @@ -551,21 +593,25 @@ class LtiAdvantageConsumer(LtiConsumer1p3): def generate_launch_request( self, preflight_response, - resource_link ): """ Build LTI message for Deep linking launches. Overrides method from LtiConsumer1p3 to allow handling LTI Deep linking messages """ + lti_message_hint = preflight_response.get('lti_message_hint') + launch_data = get_data_from_cache(lti_message_hint) + + if not launch_data: + log.warning(f'There was a cache miss during an LTI 1.3 launch when using the cache_key {lti_message_hint}.') + # Check if Deep Linking is enabled and that this is a Deep Link Launch - if self.dl and preflight_response.get("lti_message_hint") == "deep_linking_launch": + if self.dl and launch_data.message_type == "LtiDeepLinkingRequest": # Validate preflight response self._validate_preflight_response(preflight_response) # Get LTI Launch Message lti_launch_message = self.get_lti_launch_message( - resource_link=resource_link, include_extra_claims=False, ) @@ -598,7 +644,6 @@ class LtiAdvantageConsumer(LtiConsumer1p3): # set up or this isn't a Deep Link Launch return super().generate_launch_request( preflight_response, - resource_link ) def check_and_decode_deep_linking_token(self, token): diff --git a/lti_consumer/lti_1p3/exceptions.py b/lti_consumer/lti_1p3/exceptions.py index bc9551ff4b2346bc7369809f566860273bb01c9b..7ef835b906b4346c79e10257f447ac1e9867d9d9 100644 --- a/lti_consumer/lti_1p3/exceptions.py +++ b/lti_consumer/lti_1p3/exceptions.py @@ -62,6 +62,10 @@ class PreflightRequestValidationFailure(Lti1p3Exception): message = "The preflight response is not valid." +class LtiLaunchDataValidationFailure(Lti1p3Exception): + message = "The Lti1p3LaunchData is not valid." + + class LtiAdvantageServiceNotSetUp(Lti1p3Exception): message = "The LTI Advantage Service is not set up." diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index 77881b6e31962f0cc0ba3245211cee8ec22963c5..423c414e667b55cd84f57be733688ae84b686f66 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -9,9 +9,11 @@ from urllib.parse import parse_qs, urlparse import ddt from Cryptodome.PublicKey import RSA from django.test.testcases import TestCase +from edx_django_utils.cache import get_cache_key, TieredCache from jwkest.jwk import load_jwks from jwkest.jws import JWS +from lti_consumer.data import Lti1p3LaunchData from lti_consumer.lti_1p3 import exceptions from lti_consumer.lti_1p3.ags import LtiAgs from lti_consumer.lti_1p3.deep_linking import LtiDeepLinking @@ -54,23 +56,22 @@ class TestLti1p3Consumer(TestCase): tool_key=RSA_KEY ) - def _setup_lti_user(self): + def _setup_lti_launch_data(self): """ Set up a minimal LTI message with only required parameters. - Currently, the only required parameters are the user data, - but using a helper function to keep the usage consistent accross - all tests. + Currently, the only required parameters are the user data and resource_link_data. """ self.lti_consumer.set_user_data( user_id="1", role="student", ) + self.lti_consumer.set_resource_link_claim("resource_link_id") + def _get_lti_message( self, preflight_response=None, - resource_link="link" ): """ Retrieves a base LTI message with fixed test parameters. @@ -88,7 +89,6 @@ class TestLti1p3Consumer(TestCase): return self.lti_consumer.generate_launch_request( preflight_response, - resource_link ) def _decode_token(self, token): @@ -149,11 +149,17 @@ class TestLti1p3Consumer(TestCase): """ Check if preflight request is properly formed and has all required keys. """ - preflight_request_data = self.lti_consumer.prepare_preflight_url( - hint="test-hint", - lti_hint="test-lti-hint" + user_id = "1" + resource_link_id = "resource_link_id" + launch_data = Lti1p3LaunchData( + user_id=user_id, + user_role="student", + config_id="1", + resource_link_id=resource_link_id, ) + preflight_request_data = self.lti_consumer.prepare_preflight_url(launch_data) + # Extract and check parameters from OIDC launch request url parameters = parse_qs(urlparse(preflight_request_data).query) self.assertCountEqual( @@ -169,11 +175,18 @@ class TestLti1p3Consumer(TestCase): ) self.assertEqual(parameters['iss'][0], ISS) self.assertEqual(parameters['client_id'][0], CLIENT_ID) - self.assertEqual(parameters['login_hint'][0], "test-hint") - self.assertEqual(parameters['lti_message_hint'][0], "test-lti-hint") + self.assertEqual(parameters['login_hint'][0], user_id) self.assertEqual(parameters['lti_deployment_id'][0], DEPLOYMENT_ID) self.assertEqual(parameters['target_link_uri'][0], LAUNCH_URL) + launch_data_key = get_cache_key( + app="lti", + key="launch_data", + user_id=user_id, + resource_link_id=resource_link_id + ) + self.assertEqual(parameters['lti_message_hint'][0], launch_data_key) + @ddt.data( # User with no roles ( @@ -203,7 +216,6 @@ class TestLti1p3Consumer(TestCase): "email": "jonh@example.com" } ), - ) @ddt.unpack def test_set_user_data(self, data, expected_output): @@ -216,6 +228,15 @@ class TestLti1p3Consumer(TestCase): expected_output ) + def test_check_no_user_data_error(self): + """ + Check if the launch request fails if no user data is set. + """ + with self.assertRaises(ValueError) as context_manager: + self._get_lti_message() + + self.assertEqual(str(context_manager.exception), "Required user data isn't set.") + @ddt.data( "iframe", "frame", @@ -225,7 +246,7 @@ class TestLti1p3Consumer(TestCase): """ Check if setting presentation claim data works """ - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_launch_presentation_claim(document_target=target) self.assertEqual( self.lti_consumer.lti_claim_launch_presentation, @@ -266,7 +287,7 @@ class TestLti1p3Consumer(TestCase): """ Check if setting context claim data works """ - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_context_claim( "context_id", context_types=[context_type], @@ -306,7 +327,7 @@ class TestLti1p3Consumer(TestCase): """ Check if setting invalid context claim type omits type attribute """ - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_context_claim( "context_id", context_types=["invalid"], @@ -345,7 +366,7 @@ class TestLti1p3Consumer(TestCase): """ Check if setting no context claim type works """ - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_context_claim( "context_id" ) @@ -375,19 +396,56 @@ class TestLti1p3Consumer(TestCase): expected_claim_data ) - def test_check_no_user_data_error(self): + @ddt.data( + ( + {"resource_link_id": "id"}, + { + "https://purl.imsglobal.org/spec/lti/claim/resource_link": { + "id": "id", + } + } + ), + ( + {"resource_link_id": "id", "description": "description", "title": "title"}, + { + "https://purl.imsglobal.org/spec/lti/claim/resource_link": { + "id": "id", + "description": "description", + "title": "title", + } + } + ), + ) + @ddt.unpack + def test_set_resource_link_claim(self, data, expected_output): """ - Check if the launch request fails if no user data is set. + Test that setting the lti_consumer.lti_claim_resource_link attribute with + the lti_consumer.set_resource_link_claim method works correctly. """ - with self.assertRaises(ValueError): + self.lti_consumer.set_resource_link_claim(**data) + self.assertEqual( + self.lti_consumer.lti_claim_resource_link, + expected_output + ) + + def test_check_no_resource_link_claim_error(self): + """ + Check if the launch request fails if no resource_link data is set. + """ + # In order to satisfy the user data check, set some user data. + self.lti_consumer.set_user_data("1", "student") + + with self.assertRaises(ValueError) as context_manager: self._get_lti_message() + self.assertEqual(str(context_manager.exception), "Required resource_link data isn't set.") + @patch('time.time', return_value=1000) def test_launch_request(self, mock_time): """ Check if the launch request works if user data is set. """ - self._setup_lti_user() + self._setup_lti_launch_data() launch_request = self._get_lti_message() self.assertEqual(mock_time.call_count, 2) @@ -405,7 +463,7 @@ class TestLti1p3Consumer(TestCase): "custom": "parameter", } - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_custom_parameters(custom_parameters) launch_request = self._get_lti_message() @@ -522,7 +580,7 @@ class TestLti1p3Consumer(TestCase): """ Check if extra claims are correctly added to the LTI message """ - self._setup_lti_user() + self._setup_lti_launch_data() self.lti_consumer.set_extra_claim({"fake_claim": "test"}) # Retrieve launch message @@ -571,21 +629,50 @@ class TestLtiAdvantageConsumer(TestCase): self.preflight_response = {} + def _setup_lti_message_hint(self): + """ + Instantiate Lti1p3LaunchData with the appropriate launch data and store it in the cache. + + Return the cache key that was used to store the Lti1p3LaunchData. + """ + user_id = "1" + resource_link_id = "resource_link_id" + launch_data = Lti1p3LaunchData( + user_id=user_id, + user_role="student", + config_id="1", + resource_link_id=resource_link_id, + message_type="LtiDeepLinkingRequest", + ) + + launch_data_key = get_cache_key( + app="lti", + key="launch_data", + user_id=user_id, + resource_link_id=resource_link_id, + ) + TieredCache.set_all_tiers(launch_data_key, launch_data) + + return launch_data_key + def _setup_deep_linking(self): """ Set's up deep linking class in LTI consumer. """ self.lti_consumer.enable_deep_linking("launch-url", "return-url") + lti_message_hint = self._setup_lti_message_hint() + # Set LTI Consumer parameters self.preflight_response = { "client_id": CLIENT_ID, "redirect_uri": LAUNCH_URL, "nonce": NONCE, "state": STATE, - "lti_message_hint": "deep_linking_launch", + "lti_message_hint": lti_message_hint, } self.lti_consumer.set_user_data("1", "student") + self.lti_consumer.set_resource_link_claim("resource_link_id") def test_enable_ags(self): """ @@ -639,7 +726,6 @@ class TestLtiAdvantageConsumer(TestCase): # Retrieve LTI Deep Link Launch Message token = self.lti_consumer.generate_launch_request( self.preflight_response, - "resourceLink" )['id_token'] # Decode and check diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py index be30ff3238c543bd2c6dfe961e4d54b897498d8c..c39387a09f4ab5299e6d6c9f4ca55238a622db3c 100644 --- a/lti_consumer/lti_xblock.py +++ b/lti_consumer/lti_xblock.py @@ -68,14 +68,15 @@ from xblock.validation import ValidationMessage from xblockutils.resources import ResourceLoader from xblockutils.studio_editable import StudioEditableXBlockMixin +from .data import Lti1p3LaunchData from .exceptions import LtiError from .lti_1p1.consumer import LtiConsumer1p1, parse_result_json, LTI_PARAMETERS from .lti_1p1.oauth import log_authorization_header from .outcomes import OutcomeService +from .plugin import compat from .track import track_event from .utils import _, resolve_custom_parameter_template, external_config_filter_enabled, database_config_enabled - log = logging.getLogger(__name__) DOCS_ANCHOR_TAG_OPEN = ( @@ -87,7 +88,7 @@ DOCS_ANCHOR_TAG_OPEN = ( "'>" ) RESULT_SERVICE_SUFFIX_PARSER = re.compile(r"^user/(?P<anon_id>\w+)", re.UNICODE) -ROLE_MAP = { +LTI_1P1_ROLE_MAP = { 'student': 'Student,Learner', 'staff': 'Administrator', 'instructor': 'Instructor', @@ -743,14 +744,13 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): @property def role(self): """ - Get system user role and convert it to LTI role. + Get system user role. """ user = self.runtime.service(self, 'user').get_current_user() if not user.opt_attrs["edx-platform.is_authenticated"]: raise LtiError(self.ugettext("Could not get user data for current request")) - role = user.opt_attrs.get('edx-platform.user_role', 'student') - return ROLE_MAP.get(role, 'Student,Learner') + return user.opt_attrs.get('edx-platform.user_role', 'student') @property def course(self): @@ -1031,7 +1031,13 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): from lti_consumer.api import get_lti_1p3_launch_info # Retrieve LTI 1.3 Launch information - context.update(get_lti_1p3_launch_info(block=self)) + launch_data = self.get_lti_1p3_launch_data() + context.update( + get_lti_1p3_launch_info( + launch_data=launch_data, + block=self + ) + ) # Render template fragment = Fragment() @@ -1097,6 +1103,10 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): real_user_data = self.extract_real_user_data() user_id = self.user_id role = self.role + + # Convert the LMS role into an LTI 1.1 role. + role = LTI_1P1_ROLE_MAP.get(role, 'Student,Learner') + result_sourcedid = self.lis_result_sourcedid except LtiError: loader = ResourceLoader(__name__) @@ -1405,6 +1415,47 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): return launch_url + def get_lti_1p3_launch_data(self): + """ + Return an instance of Lti1p3LaunchData, containing necessary data for an LTI 1.3 launch. + """ + # Runtime import since this will only run in the + # Open edX LMS/Studio environments. + # TODO: Replace this with a more appropriate API function that is intended for public use. + # pylint: disable=import-outside-toplevel + from lti_consumer.api import _get_lti_config + lti_config = _get_lti_config(block=self) + + location = self.location # pylint: disable=no-member + course_key = str(location.course_key) + + launch_data = Lti1p3LaunchData( + user_id=self.external_user_id, + user_role=self.role, + config_id=lti_config.config_id, + resource_link_id=str(location), + launch_presentation_document_target="iframe", + context_id=course_key, + context_type=["course_offering"], + context_title=self.get_context_title(), + context_label=course_key, + ) + + return launch_data + + def get_context_title(self): + """ + Return the title attribute of the context_claim for LTI 1.3 launches. This information is included in the + launch_data query or form parameter of the LTI 1.3 third-party login initiation request. + """ + course_key = self.location.course_key # pylint: disable=no-member + course = compat.get_course_by_id(course_key) + + return " - ".join([ + course.display_name_with_default, + course.display_org_with_default + ]) + def _get_lti_block_launch_handler(self): """ Return the LTI block launch handler. @@ -1415,13 +1466,14 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock): if self.lti_version == 'lti_1p1' or self.config_type == "external": 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_content_url # pylint: disable=import-outside-toplevel + launch_data = self.get_lti_1p3_launch_data() # Retrieve and set LTI 1.3 Launch start URL + # Runtime import since this will only run in the Open edX LMS/Studio environments. + from lti_consumer.api import get_lti_1p3_content_url # pylint: disable=import-outside-toplevel lti_block_launch_handler = get_lti_1p3_content_url( + launch_data, block=self, - hint=str(self.location) # pylint: disable=no-member ) return lti_block_launch_handler diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 5f3d1eea8f6bbbec864cb9ca179520668a47c6f3..e7a1b1561841072bd9fc2035c9059a800da0d8a7 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -258,22 +258,3 @@ def get_event_tracker(): return tracker except ModuleNotFoundError: return None - - -def get_user_role(user, course_key: CourseKey) -> str: - """ - Import the get_user_role from LMS and return the value. - """ - # pylint: disable=import-error,import-outside-toplevel - from lms.djangoapps.courseware.access import get_user_role as get_role - return get_role(user, course_key) - - -def get_external_id_for_user(user) -> str: - """ - Import and run `get_external_id_for_user` from LMS - """ - # pylint: disable=import-error,import-outside-toplevel - from openedx.core.djangoapps.external_user_ids.models import ExternalId - ext_user, _ = ExternalId.add_new_user_id(user, 'lti') - return str(ext_user.external_user_id) diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py index 7818bffb3218b1ed6d235a3b5b70eeca2b6c0a73..09c0bc5f19c97b1bf90c5d2a19ca0c070027f540 100644 --- a/lti_consumer/plugin/views.py +++ b/lti_consumer/plugin/views.py @@ -5,7 +5,7 @@ import logging import urllib from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist, PermissionDenied +from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError from django.http import JsonResponse, Http404 from django.db import transaction from django.views.decorators.csrf import csrf_exempt @@ -20,7 +20,7 @@ from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.status import HTTP_400_BAD_REQUEST, HTTP_403_FORBIDDEN, HTTP_404_NOT_FOUND -from lti_consumer.api import get_lti_pii_sharing_state_for_course +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.models import ( LtiConfiguration, @@ -28,7 +28,6 @@ from lti_consumer.models import ( LtiDlContentItem, ) -from lti_consumer.lti_1p3.consumer import LTI_1P3_CONTEXT_TYPE from lti_consumer.lti_1p3.exceptions import ( Lti1p3Exception, LtiDeepLinkingContentTypeNotSupported, @@ -65,7 +64,7 @@ from lti_consumer.lti_1p3.extensions.rest_framework.parsers import ( ) from lti_consumer.lti_1p3.extensions.rest_framework.utils import IgnoreContentNegotiation from lti_consumer.plugin import compat -from lti_consumer.utils import _ +from lti_consumer.utils import _, get_lti_1p3_context_types_claim, get_data_from_cache from lti_consumer.track import track_event @@ -143,32 +142,44 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume """ Gate endpoint that triggers LTI launch endpoint XBlock handler - This uses the location key from the "login_hint" query parameter + This uses the config_id key of the "lti_message_hint" query parameter to identify the LtiConfiguration and its consumer to generate the LTI 1.3 Launch Form. """ - usage_id = request.GET.get('login_hint') - if not usage_id: - log.info('The `login_hint` query param in the request is missing or empty.') + # pylint: disable=too-many-statements + request_params = request.GET if request.method == 'GET' else request.POST + + lti_message_hint = request_params.get('lti_message_hint') + if not lti_message_hint: + log.info('The lti_message_hint query param in the request is missing or empty.') return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) - try: - usage_key = UsageKey.from_string(usage_id) - except InvalidKeyError as exc: + login_hint = request_params.get('login_hint') + if not login_hint: + log.info('The login_hint query param in the request is missing or empty.') + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + + launch_data = get_data_from_cache(lti_message_hint) + if not launch_data: + log.warning(f'There was a cache miss during an LTI 1.3 launch when using the cache_key {lti_message_hint}.') + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + + # Validate the Lti1p3LaunchData. + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + if not is_valid: + validation_message = " ".join(validation_messages) log.error( - "The login_hint: %s is not a valid block location. Error: %s", - usage_id, - exc, - exc_info=True + f"The Lti1p3LaunchData is not valid. {validation_message}" ) - return render(request, 'html/lti_launch_error.html', status=HTTP_404_NOT_FOUND) + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + config_id = launch_data.config_id try: lti_config = LtiConfiguration.objects.get( - location=usage_key + config_id=config_id ) - except LtiConfiguration.DoesNotExist as exc: - log.error("Invalid usage_id '%s' for LTI 1.3 Launch callback", usage_id) + except (LtiConfiguration.DoesNotExist, ValidationError) as exc: + log.error("Invalid config_id '%s' for LTI 1.3 Launch callback", config_id) raise Http404 from exc if lti_config.version != LtiConfiguration.LTI_1P3: @@ -177,50 +188,58 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume context = {} - course_key = usage_key.course_key - course = compat.get_course_by_id(course_key) - user_role = compat.get_user_role(request.user, course_key) - external_user_id = compat.get_external_id_for_user(request.user) - lti_consumer = lti_config.get_lti_consumer() - try: - # Pass user data - # Pass django user role to library - lti_consumer.set_user_data(user_id=external_user_id, role=user_role) - - # Set launch context - # Hardcoded for now, but we need to translate from - # self.launch_target to one of the LTI compliant names, - # either `iframe`, `frame` or `window` - # This is optional though - lti_consumer.set_launch_presentation_claim('iframe') - - # Set context claim - # This is optional - context_title = " - ".join([ - course.display_name_with_default, - course.display_org_with_default - ]) - # Course ID is the context ID for the LTI for now. This can be changed to be - # more specific in the future for supporting other tools like discussions, etc. + lti_consumer = lti_config.get_lti_consumer() + + # Set sub and roles claims. + user_id = launch_data.user_id + user_role = launch_data.user_role + lti_consumer.set_user_data( + user_id=user_id, + role=user_role, + ) + + # Set resource_link claim. + lti_consumer.set_resource_link_claim(launch_data.resource_link_id) + + # Set launch_presentation claim. + launch_presentation_target = launch_data.launch_presentation_document_target + if launch_presentation_target: + lti_consumer.set_launch_presentation_claim(launch_presentation_target) + + # Set optional context claim, if supplied. + context_type = launch_data.context_type + context_types_claim = None + + if context_type: + try: + context_types_claim = get_lti_1p3_context_types_claim(context_type) + except ValueError: + log.error( + "The context_type key %s in the launch data does not represent a valid context_type.", + context_type + ) + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + lti_consumer.set_context_claim( - str(course_key), - context_types=[LTI_1P3_CONTEXT_TYPE.course_offering], - context_title=context_title, - context_label=str(course_key) + launch_data.context_id, + context_types_claim, + launch_data.context_title, + launch_data.context_label, ) - # Retrieve preflight response - preflight_response = request.GET.dict() - lti_message_hint = preflight_response.get('lti_message_hint', '') + # Retrieve preflight response. + preflight_response = request_params.dict() - # Set LTI Launch URL + # Set LTI Launch URL. context.update({'launch_url': lti_consumer.launch_url}) - # Modify LTI Launch URL dependind on launch type + # Modify LTI Launch URL depending on launch type. # Deep Linking Launch - Configuration flow launched by # course creators to set up content. - if lti_consumer.dl and lti_message_hint == 'deep_linking_launch': + deep_linking_content_item_id = launch_data.deep_linking_content_item_id + + if lti_consumer.dl and launch_data.message_type == 'LtiDeepLinkingRequest': # Check if the user is staff before LTI doing deep linking launch. # If not, raise exception and display error page if user_role not in ['instructor', 'staff']: @@ -231,10 +250,9 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume # Deep Linking ltiResourceLink content presentation # When content type is `ltiResourceLink`, the tool will be launched with # different parameters, set by instructors when running the DL configuration flow. - elif lti_consumer.dl and 'deep_linking_content_launch' in lti_message_hint: - # Retrieve Deep Linking parameters using lti_message_hint parameter. - deep_linking_id = lti_message_hint.split(':')[1] - content_item = lti_config.ltidlcontentitem_set.get(pk=deep_linking_id) + elif lti_consumer.dl and deep_linking_content_item_id: + # Retrieve Deep Linking parameters using the parameter. + content_item = lti_config.ltidlcontentitem_set.get(pk=deep_linking_content_item_id) # Only filter DL content item from content item set in the same LTI configuration. # This avoids a malicious user to input a random LTI id and perform LTI DL # content launches outside the scope of its configuration. @@ -250,8 +268,7 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume context.update({ "preflight_response": preflight_response, "launch_request": lti_consumer.generate_launch_request( - resource_link=usage_id, - preflight_response=preflight_response + preflight_response=preflight_response, ) }) event = { @@ -263,18 +280,20 @@ def launch_gate_endpoint(request, suffix=None): # pylint: disable=unused-argume return render(request, 'html/lti_1p3_launch.html', context) except Lti1p3Exception as exc: + resource_link_id = launch_data.resource_link_id log.warning( - "Error preparing LTI 1.3 launch for block %r: %s", - usage_id, + "Error preparing LTI 1.3 launch for resource with resource_link_id %r: %s", + resource_link_id, exc, exc_info=True ) return render(request, 'html/lti_launch_error.html', context, status=HTTP_400_BAD_REQUEST) except AssertionError as exc: + resource_link_id = launch_data.resource_link_id log.warning( - "Permission on LTI block %r denied for user %r: %s", - usage_id, - external_user_id, + "Permission on resource with resource_link_id %r denied for user %r: %s", + resource_link_id, + user_id, exc, exc_info=True ) @@ -438,10 +457,20 @@ def deep_linking_response_endpoint(request, lti_config_id=None): @require_http_methods(['GET']) @xframe_options_sameorigin -def deep_linking_content_endpoint(request, lti_config_id=None): +def deep_linking_content_endpoint(request, lti_config_id): """ Deep Linking endpoint for rendering Deep Linking Content Items. """ + launch_data_key = request.GET.get("launch_data_key") + if not launch_data_key: + log.info('The launch_data_key query param in the request is missing or empty.') + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + + launch_data = get_data_from_cache(launch_data_key) + if not launch_data: + log.warning(f'There was a cache miss during an LTI 1.3 launch when using the cache_key {launch_data_key}.') + return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST) + try: # Get LTI Configuration lti_config = LtiConfiguration.objects.get(id=lti_config_id) @@ -470,6 +499,7 @@ def deep_linking_content_endpoint(request, lti_config_id=None): return render(request, 'html/lti-dl/render_dl_content.html', { 'content_items': content_items, 'block': lti_config.block, + 'launch_data': launch_data, }) diff --git a/lti_consumer/templates/html/lti-dl/render_dl_content.html b/lti_consumer/templates/html/lti-dl/render_dl_content.html index 0afa083bac7b3eb7ee12b70947751563d999d7fe..ef333b8a20c246c9969b6b34decc142ba0a9220a 100644 --- a/lti_consumer/templates/html/lti-dl/render_dl_content.html +++ b/lti_consumer/templates/html/lti-dl/render_dl_content.html @@ -8,7 +8,7 @@ {% for item in content_items %} <div> {% if item.content_type == 'ltiResourceLink' %} - {% include "html/lti-dl/render_lti_resource_link.html" with item=item attrs=item.attributes %} + {% include "html/lti-dl/render_lti_resource_link.html" with item=item attrs=item.attributes launch_data=launch_data %} {% elif item.content_type == 'link' %} {% include "html/lti-dl/render_link.html" with item=item attrs=item.attributes %} {% elif item.content_type == 'html' %} diff --git a/lti_consumer/templates/html/lti-dl/render_lti_resource_link.html b/lti_consumer/templates/html/lti-dl/render_lti_resource_link.html index 8e5b10abace9c8b6305a7de109025e994648128d..9b6baf4c472ced52961432fb106de7babf512bbf 100644 --- a/lti_consumer/templates/html/lti-dl/render_lti_resource_link.html +++ b/lti_consumer/templates/html/lti-dl/render_lti_resource_link.html @@ -3,7 +3,9 @@ {% if attrs.title %}<h2>{{ attrs.title }}</h2>{% endif %} {% if attrs.text %}<p>{{ attrs.text }}</p>{% endif %} -<a href="{{ item | get_dl_lti_launch_url }}" target="_blank"> +{% get_dl_lti_launch_url item launch_data as launch_url %} + +<a href="{{ launch_url }}" target="_blank"> Click here to launch the LTI content in a new tab <i class="icon fa fa-external-link"></i> </a> diff --git a/lti_consumer/templatetags/get_dl_lti_launch_url.py b/lti_consumer/templatetags/get_dl_lti_launch_url.py index 1dd97c9cc49b9e9bf8ee2c797131399f19058857..d49f5858d6234fed14822b6430679ac54dd2f91c 100644 --- a/lti_consumer/templatetags/get_dl_lti_launch_url.py +++ b/lti_consumer/templatetags/get_dl_lti_launch_url.py @@ -4,23 +4,22 @@ Template tags and helper functions for sanitizing html. from django import template from lti_consumer.api import get_lti_1p3_launch_start_url - register = template.Library() -@register.filter() -def get_dl_lti_launch_url(content_item): +@register.simple_tag +def get_dl_lti_launch_url(content_item, launch_data): """ - Template tag to retrive the LTI launch URL for `ltiResourceLink` content. + Template tag to retrieve the LTI launch URL for `ltiResourceLink` content. - This uses the LTI Consumer API, but hardcodes the `hint` parameter to the - block id (used when LTI launches are tied to XBlocks). + This uses the LTI Consumer API to generate the LTI 1.3 third party login initiation + URL to start the LTI 1.3 launch flow. - TODO: Refactor `hint` to use generic ID once LTI launches out of XBlocks are - supported. + This template tag requires content_item and launch_data arguments, which are passed to + get_lti_1p3_launch_start_url to encode information necessary to the eventual LTI 1.3 launch. """ return get_lti_1p3_launch_start_url( + launch_data, config_id=content_item.lti_configuration.id, dl_content_id=content_item.id, - hint=str(content_item.lti_configuration.location), ) diff --git a/lti_consumer/tests/test_utils.py b/lti_consumer/tests/test_utils.py new file mode 100644 index 0000000000000000000000000000000000000000..192505b9d213926051fbce8e49d4d11ec3cc0506 --- /dev/null +++ b/lti_consumer/tests/test_utils.py @@ -0,0 +1,96 @@ +""" +Utility functions used within unit tests +""" + +from unittest.mock import Mock +import urllib + +from opaque_keys.edx.keys import UsageKey +from webob import Request +from workbench.runtime import WorkbenchRuntime +from xblock.fields import ScopeIds +from xblock.runtime import DictKeyValueStore, KvsFieldData + + +FAKE_USER_ID = 'fake_user_id' + + +def make_xblock(xblock_name, xblock_cls, attributes): + """ + Helper to construct XBlock objects + """ + runtime = WorkbenchRuntime() + key_store = DictKeyValueStore() + db_model = KvsFieldData(key_store) + ids = generate_scope_ids(runtime, xblock_name) + xblock = xblock_cls(runtime, db_model, scope_ids=ids) + xblock.category = Mock() + + xblock.location = UsageKey.from_string( + 'block-v1:edX+DemoX+Demo_Course+type@problem+block@466f474fa4d045a8b7bde1b911e095ca' + ) + + xblock.runtime = Mock( + hostname='localhost', + ) + xblock.course_id = 'course-v1:edX+DemoX+Demo_Course' + for key, value in attributes.items(): + setattr(xblock, key, value) + return xblock + + +def generate_scope_ids(runtime, block_type): + """ + Helper to generate scope IDs for an XBlock + """ + def_id = runtime.id_generator.create_definition(block_type) + usage_id = runtime.id_generator.create_usage(def_id) + return ScopeIds('user', block_type, def_id, usage_id) + + +def make_request(body, method='POST'): + """ + Helper to make a request + """ + request = Request.blank('/') + request.method = 'POST' + request.body = body.encode('utf-8') + request.method = method + 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. + """ + return { + 'custom_author_email': 'author@example.com', + 'custom_author_country': '', + } + + +def defaulting_processor(_xblock): + """ + A dummy LTI parameter processor with default params. + """ + + +defaulting_processor.lti_xblock_default_params = { + 'custom_name': 'Lex', + 'custom_country': '', +} diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py index 5a5b5d64360bd2e09abb2ff573a68b95e830fa13..17db1dcfbd32600ada6a8aba2fdb96c94e268bee 100644 --- a/lti_consumer/tests/unit/plugin/test_views.py +++ b/lti_consumer/tests/unit/plugin/test_views.py @@ -12,6 +12,7 @@ from django.urls import reverse from Cryptodome.PublicKey import RSA from jwkest.jwk import RSAKey from opaque_keys.edx.keys import UsageKey +from lti_consumer.data import Lti1p3LaunchData from lti_consumer.models import LtiConfiguration, LtiDlContentItem from lti_consumer.lti_1p3.exceptions import ( MissingRequiredClaim, @@ -24,7 +25,8 @@ from lti_consumer.lti_1p3.exceptions import ( ) from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.lti_1p3.tests.utils import create_jwt -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock +from lti_consumer.utils import cache_lti_1p3_launch_data class TestLti1p3KeysetEndpoint(TestCase): @@ -122,8 +124,14 @@ class TestLti1p3LaunchGateEndpoint(TestCase): 'lti_1p3_oidc_url': 'http://tool.example/oidc', } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = self.location + + self.launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id=self.config.config_id, + resource_link_id="resource_link_id", + ) + self.launch_data_key = cache_lti_1p3_launch_data(self.launch_data) self.compat_patcher = patch("lti_consumer.plugin.views.compat") self.compat = self.compat_patcher.start() @@ -139,13 +147,6 @@ class TestLti1p3LaunchGateEndpoint(TestCase): model_compat.load_block_as_anonymous_user.return_value = self.xblock self.addCleanup(model_compat_patcher.stop) - def test_invalid_usage_key(self): - """ - Check that passing a invalid login_hint yields HTTP code 404. - """ - response = self.client.get(self.url, {"login_hint": "useless-location"}) - self.assertEqual(response.status_code, 404) - def test_invalid_lti_version(self): """ Check that a LTI 1.1 tool accessing this endpoint is returned a 404. @@ -153,7 +154,13 @@ class TestLti1p3LaunchGateEndpoint(TestCase): self.config.version = LtiConfiguration.LTI_1P1 self.config.save() - response = self.client.get(self.url, {"login_hint": self.location}) + response = self.client.get( + self.url, + { + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key + } + ) self.assertEqual(response.status_code, 404) # Rollback @@ -164,19 +171,78 @@ class TestLti1p3LaunchGateEndpoint(TestCase): """ Check that a 404 is returned when LtiConfiguration for a location doesn't exist """ - response = self.client.get(self.url, {"login_hint": self.location + "extra"}) + self.launch_data.config_id = "1" + response = self.client.get( + self.url, + { + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key + } + ) self.assertEqual(response.status_code, 404) + def test_missing_required_lti_message_hint_param(self): + """ + Check that a 400 error is returned when required lti_message_hint query parameter is not provided. + """ + response = self.client.post(self.url, {"login_hint": "login_hint"}) + self.assertEqual(response.status_code, 400) + + def test_missing_required_login_hint_param(self): + """ + Check that a 400 error is returned when required login_hint query parameter is not provided. + """ + response = self.client.post(self.url, {"lti_message_hint": "lti_message_hint"}) + self.assertEqual(response.status_code, 400) + + def test_missing_launch_data(self): + """ + Check that a 400 error is returned when required lti_message_hint query parameter is not associated with + launch_data in the cache. + """ + response = self.client.post(self.url, {"lti_message_hint": "lti_message_hint", "login_hint": "login_hint"}) + self.assertEqual(response.status_code, 400) + + @patch('lti_consumer.api.validate_lti_1p3_launch_data') + @patch('lti_consumer.utils.get_data_from_cache') + def test_invalid_launch_data(self, mock_get_data_from_cache, mock_validate_launch_data): + """ + Check that a 400 error is returned when the launch_data stored in the cache is not valid. + """ + # Mock getting the launch_data from the cache. + mock_get_data_from_cache.return_value = {} + + # Mock checking the launch_data for validity. + mock_validate_launch_data.return_value = (False, []) + + response = self.client.post(self.url, {"lti_message_hint": "lti_message_hint", "login_hint": "login_hint"}) + self.assertEqual(response.status_code, 400) + + @patch('lti_consumer.api.validate_lti_1p3_launch_data') + @patch('lti_consumer.utils.get_data_from_cache') + def test_invalid_context_type(self, mock_get_data_from_cache, mock_validate_launch_data): + # Mock getting the launch_data from the cache. + mock_launch_data = Mock() + mock_launch_data.context_type = "invalid_context_type" + mock_get_data_from_cache.return_value = mock_launch_data + + # Mock checking the launch_data for validity. + mock_validate_launch_data.return_value = (True, []) + + response = self.client.post(self.url, {"lti_message_hint": "lti_message_hint", "login_hint": "login_hint"}) + self.assertEqual(response.status_code, 400) + def test_lti_launch_response(self): """ Check that the right launch response is generated """ params = { - "login_hint": self.location, "nonce": "nonce-value", "state": "hello-world", "redirect_uri": "https://tool.example", - "client_id": self.config.lti_1p3_client_id + "client_id": self.config.lti_1p3_client_id, + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key } response = self.client.get(self.url, params) self.assertEqual(response.status_code, 200) @@ -207,8 +273,8 @@ class TestLti1p3LaunchGateEndpoint(TestCase): "client_id": self.config.lti_1p3_client_id, "redirect_ur": "http://tool.example/launch", "state": "state_test_123", - "nonce": "nonce", - "login_hint": self.location + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key, } response = self.client.get(self.url, params) self.assertEqual(response.status_code, 400) @@ -225,6 +291,8 @@ class TestLti1p3LaunchGateEndpoint(TestCase): mock_user_service.get_external_user_id.return_value = 2 self.xblock.runtime.service.return_value = mock_user_service + self.launch_data.user_role = user_role + self.xblock.course.display_name_with_default = 'course_display_name' self.xblock.course.display_org_with_default = 'course_display_org' @@ -242,8 +310,8 @@ class TestLti1p3LaunchGateEndpoint(TestCase): "redirect_uri": "http://tool.example/launch", "state": "state_test_123", "nonce": "nonce", - "login_hint": self.location, - "lti_message_hint": "deep_linking_launch" + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key, } response = self.client.get(self.url, params) @@ -262,7 +330,7 @@ class TestLti1p3LaunchGateEndpoint(TestCase): self.xblock.config_type = 'database' LtiConfiguration.objects.filter(id=self.config.id).update( - location=self.xblock.location, + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_DB, lti_advantage_deep_linking_enabled=dl_enabled, @@ -270,14 +338,15 @@ class TestLti1p3LaunchGateEndpoint(TestCase): ) if dl_enabled: self.xblock.lti_advantage_deep_linking_launch_url = url + self.launch_data.message_type = "LtiDeepLinkingRequest" params = { "client_id": self.config.lti_1p3_client_id, "redirect_uri": "http://tool.example/launch", "state": "state_test_123", "nonce": "nonce", - "login_hint": self.location, - "lti_message_hint": "deep_linking_launch" + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key, } response = self.client.get(self.url, params) @@ -299,14 +368,15 @@ class TestLti1p3LaunchGateEndpoint(TestCase): self._setup_deep_linking(user_role='student') # Enable deep linking self.xblock.lti_advantage_deep_linking_enabled = True + self.launch_data.message_type = "LtiDeepLinkingRequest" params = { "client_id": self.config.lti_1p3_client_id, "redirect_uri": "http://tool.example/launch", "state": "state_test_123", "nonce": "nonce", - "login_hint": self.location, - "lti_message_hint": "deep_linking_launch" + "login_hint": self.launch_data.user_id, + "lti_message_hint": self.launch_data_key, } response = self.client.get(self.url, params) @@ -332,13 +402,17 @@ class TestLti1p3LaunchGateEndpoint(TestCase): }, } + # We need to re-cache the launch_data with a new cache key that is specific to the content item. + self.launch_data.deep_linking_content_item_id = "1" + launch_data_key = cache_lti_1p3_launch_data(self.launch_data) + params = { "client_id": self.config.lti_1p3_client_id, "redirect_uri": "http://tool.example/launch", "state": "state_test_123", "nonce": "nonce", - "login_hint": self.location, - "lti_message_hint": "deep_linking_content_launch:1" + "login_hint": self.launch_data.user_id, + "lti_message_hint": launch_data_key, } response = self.client.get(self.url, params) content = response.content.decode('utf-8') @@ -363,13 +437,18 @@ class TestLti1p3LaunchGateEndpoint(TestCase): content_type="link", attributes={"parameter": "custom"} ) + + # We need to re-cache the launch_data with a new cache key that is specific to the content item. + self.launch_data.deep_linking_content_item_id = dl_item.id + launch_data_key = cache_lti_1p3_launch_data(self.launch_data) + params = { "client_id": self.config.lti_1p3_client_id, "redirect_uri": "http://tool.example/launch", "state": "state_test_123", "nonce": "nonce", - "login_hint": self.location, - "lti_message_hint": f"deep_linking_content_launch:{dl_item.id}" + "login_hint": self.launch_data.user_id, + "lti_message_hint": launch_data_key, } response = self.client.get(self.url, params) # Check response 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 085816b3491c658d873d51ad1f969e99fb39b9b3..7a3e7f94441eb56327d7f93735752bd4547ce8c1 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -15,7 +15,7 @@ from rest_framework.test import APITransactionTestCase from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration, LtiAgsLineItem, LtiAgsScore -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock class LtiAgsLineItemViewSetTestCase(APITransactionTestCase): @@ -49,12 +49,9 @@ class LtiAgsLineItemViewSetTestCase(APITransactionTestCase): } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' - # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, ) # Preload XBlock to avoid calls to modulestore @@ -177,7 +174,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, + resource_link_id=self.xblock.location, # pylint: disable=no-member label="test label", score_maximum=100 ) @@ -198,7 +195,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): 'scoreMaximum': 100, 'label': 'test label', 'tag': '', - 'resourceLinkId': self.xblock.location, + 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member 'startDateTime': None, 'endDateTime': None, } @@ -215,7 +212,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, + resource_link_id=self.xblock.location, # pylint: disable=no-member label="test label", score_maximum=100 ) @@ -242,7 +239,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): 'scoreMaximum': 100, 'label': 'test label', 'tag': '', - 'resourceLinkId': self.xblock.location, + 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member 'startDateTime': None, 'endDateTime': None, } @@ -262,7 +259,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): 'scoreMaximum': 100, 'label': 'test', 'tag': 'score', - 'resourceLinkId': self.xblock.location, + 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member }), content_type="application/vnd.ims.lis.v2.lineitem+json", ) @@ -276,7 +273,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): 'scoreMaximum': 100, 'label': 'test', 'tag': 'score', - 'resourceLinkId': self.xblock.location, + 'resourceLinkId': str(self.xblock.location), # pylint: disable=no-member 'startDateTime': None, 'endDateTime': None, } @@ -287,7 +284,7 @@ class LtiAgsViewSetLineItemTests(LtiAgsLineItemViewSetTestCase): self.assertEqual(line_item.score_maximum, 100) self.assertEqual(line_item.label, 'test') self.assertEqual(line_item.tag, 'score') - self.assertEqual(str(line_item.resource_link_id), self.xblock.location) + self.assertEqual(str(line_item.resource_link_id), str(self.xblock.location)) # pylint: disable=no-member def test_create_lineitem_invalid_resource_link_id(self): """ @@ -324,7 +321,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase): self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, + resource_link_id=self.xblock.location, # pylint: disable=no-member label="test label", score_maximum=100 ) @@ -856,7 +853,7 @@ class LtiAgsViewSetResultsTests(LtiAgsLineItemViewSetTestCase): self.line_item = LtiAgsLineItem.objects.create( lti_configuration=self.lti_config, resource_id="test", - resource_link_id=self.xblock.location, + resource_link_id=self.xblock.location, # pylint: disable=no-member label="test label", score_maximum=100 ) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py index 77505f9a28c2fe20d171c97c75fc1839dd26c433..b19794d6bc831b93663b26ed9f7033299572ccd3 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_deep_linking.py @@ -3,6 +3,7 @@ Tests for LTI Advantage Assignments and Grades Service views. """ from unittest.mock import patch, PropertyMock, Mock +import re import ddt from Cryptodome.PublicKey import RSA from jwkest.jwk import RSAKey @@ -10,9 +11,11 @@ from rest_framework.test import APITransactionTestCase from rest_framework.exceptions import ValidationError +from lti_consumer.data import Lti1p3LaunchData +from lti_consumer.utils import cache_lti_1p3_launch_data from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration, LtiDlContentItem -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock class LtiDeepLinkingTestCase(APITransactionTestCase): @@ -22,11 +25,13 @@ class LtiDeepLinkingTestCase(APITransactionTestCase): def setUp(self): super().setUp() - self.location = 'block-v1:course+test+2020+type@problem+block@test' + # We define the XBlock first in order to create an LtiConfiguration instance, which is used to generate + # LTI 1.3 keys. Later, we set the necessary XBlock attributes. + self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, {}) # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=self.location, + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, ) @@ -35,7 +40,7 @@ class LtiDeepLinkingTestCase(APITransactionTestCase): self.key = RSAKey( # Using the same key ID as client id # This way we can easily serve multiple public - # keys on teh same endpoint and keep all + # keys on the same endpoint and keep all # LTI 1.3 blocks working kid=self.lti_config.lti_1p3_private_key_id, key=rsa_key @@ -54,10 +59,9 @@ class LtiDeepLinkingTestCase(APITransactionTestCase): 'lti_advantage_deep_linking_enabled': True, 'lti_advantage_deep_linking_launch_url': 'http://tool.example/deep_link_launch', } - self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = self.location + for key, value in self.xblock_attributes.items(): + setattr(self.xblock, key, value) # Preload XBlock to avoid calls to modulestore self.lti_config.block = self.xblock @@ -406,7 +410,18 @@ class LtiDeepLinkingContentEndpointTestCase(LtiDeepLinkingTestCase): def setUp(self): super().setUp() - self.url = '/lti_consumer/v1/lti/{}/lti-dl/content'.format(self.lti_config.id) + + self.launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id=self.lti_config.config_id, + resource_link_id="resource_link_id", + ) + self.launch_data_key = cache_lti_1p3_launch_data(self.launch_data) + + self.url = '/lti_consumer/v1/lti/{}/lti-dl/content?launch_data_key={}'.format( + self.lti_config.id, self.launch_data_key + ) @patch('lti_consumer.plugin.views.has_block_access', return_value=False) def test_forbidden_access(self, has_block_access_patcher): # pylint: disable=unused-argument @@ -420,9 +435,35 @@ class LtiDeepLinkingContentEndpointTestCase(LtiDeepLinkingTestCase): """ Test if throws 404 when lti configuration not found. """ - resp = self.client.get('/lti_consumer/v1/lti/200/lti-dl/content') + resp = self.client.get( + '/lti_consumer/v1/lti/200/lti-dl/content?launch_data_key={}'.format(self.launch_data_key) + ) self.assertEqual(resp.status_code, 404) + def test_missing_required_launch_data_key_param(self): + """ + Check that a 400 error is returned when required launch_data_key query parameter is not provided. + """ + # Use a new URL instead of self.url so that we do not include a launch_data_key. + url = '/lti_consumer/v1/lti/{}/lti-dl/content'.format( + self.lti_config.id + ) + response = self.client.get(url, {}) + self.assertEqual(response.status_code, 400) + + def test_missing_launch_data(self): + """ + Check that a 400 error is returned when required launch_data_key query parameter is not associated with + launch_data in the cache. + """ + # Use a new URL instead of self.url so that we include a launch_data_key that is not associated with any + # launch_data in the cache. + url = '/lti_consumer/v1/lti/{}/lti-dl/content'.format( + self.lti_config.id + ) + response = self.client.get(url, {"launch_data_key": "launch_data_key"}) + self.assertEqual(response.status_code, 400) + @patch('lti_consumer.plugin.views.has_block_access', return_value=True) def test_no_dl_contents(self, has_block_access_patcher): # pylint: disable=unused-argument """ @@ -630,9 +671,12 @@ class LtiDeepLinkingContentEndpointTestCase(LtiDeepLinkingTestCase): resp = self.client.get(self.url) self.assertEqual(resp.status_code, 200) - # Check that there's three LTI Resource links presented - for item in content_items: + # Check that there's three LTI Resource links presented with the correct launch_data_key query parameter and the + # correct deep_linking_content_item_id parameter in the launch_data. + lti_message_hints = re.findall('lti_message_hint=(\\w*)', str(resp.content)) + + for lti_message_hint in lti_message_hints: self.assertContains( resp, - f"lti_message_hint=deep_linking_content_launch%3A{item.id}" + f"lti_message_hint={lti_message_hint}" ) diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py index 4a9eabaf39282c425014197f69bd42ab85b68d5f..2a40014e2fefb7e86b6e2275326ad11a35b53281 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_nrps.py @@ -10,7 +10,7 @@ from rest_framework.reverse import reverse from lti_consumer.exceptions import LtiError from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock def generate_mock_members(num, role='student'): @@ -135,12 +135,9 @@ class LtiNrpsTestCase(APITransactionTestCase): self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' - # Create configuration self.lti_config = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, ) # Preload XBlock to avoid calls to modulestore diff --git a/lti_consumer/tests/unit/test_api.py b/lti_consumer/tests/unit/test_api.py index c034a49d1cfe9ceabcc79820efc3ab27a8ee2df6..94633a97ff28773dccde9c3309cc505fb8965fc6 100644 --- a/lti_consumer/tests/unit/test_api.py +++ b/lti_consumer/tests/unit/test_api.py @@ -2,10 +2,12 @@ Tests for LTI API. """ from unittest.mock import Mock, patch +from urllib.parse import parse_qs, urlparse import ddt from Cryptodome.PublicKey import RSA from django.test.testcases import TestCase +from edx_django_utils.cache import get_cache_key from opaque_keys.edx.locations import Location from lti_consumer.api import ( @@ -14,11 +16,14 @@ from lti_consumer.api import ( get_deep_linking_data, get_lti_1p3_launch_info, get_lti_1p3_launch_start_url, - get_lti_consumer + get_lti_consumer, + validate_lti_1p3_launch_data ) +from lti_consumer.data import Lti1p3LaunchData from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiConfiguration, LtiDlContentItem -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock +from lti_consumer.utils import get_data_from_cache class Lti1P3TestCase(TestCase): @@ -52,11 +57,19 @@ class Lti1P3TestCase(TestCase): 'lti_1p3_tool_public_key': public_key, } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' + # Create lti configuration self.lti_config = LtiConfiguration.objects.create( - location=self.xblock.location + location=self.xblock.location # pylint: disable=no-member + ) + + @staticmethod + def _get_lti_1p3_launch_data(): + return Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", ) @@ -228,9 +241,124 @@ class TestGetLtiConsumer(TestCase): self.assertEqual(LtiConfiguration.objects.all().count(), 1) +class TestValidateLti1p3LaunchData(TestCase): + """ + Unit tests for validate_lti_1p3_launch_data API method. + """ + def setUp(self): + # Patch internal method to avoid calls to modulestore + super().setUp() + patcher = patch( + 'lti_consumer.models.LtiConfiguration.get_lti_consumer', + ) + self.addCleanup(patcher.stop) + + def _assert_required_context_id_message(self, validation_messages): + """ + Assert that validation_messages is the correct list of validation_messages for the required context_id + attribute. + + Arguments: + validation_messages (list): a list of strings representing validation messages + """ + self.assertEqual( + validation_messages, + ["The context_id attribute is required in the launch data if any optional context properties are provided."] + ) + + def test_valid(self): + """ + Ensure that valid instances of Lti1p3LaunchData are appropriately validated. + """ + launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", + context_id="1", + context_type=["course_offering"], + ) + + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + + self.assertEqual(is_valid, True) + self.assertEqual(validation_messages, []) + + def test_invalid_context_values_context_id_required(self): + """ + Ensure that instances of Lti1p3LaunchData that are instantiated with optional context_* attributes also are + instantiated with the context_id attribute. + """ + launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", + ) + + launch_data.context_type = ["course_offering"] + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + self.assertEqual(is_valid, False) + self._assert_required_context_id_message(validation_messages) + + launch_data.context_title = "context_title" + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + self.assertEqual(is_valid, False) + self._assert_required_context_id_message(validation_messages) + + launch_data.context_label = "context_label" + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + self.assertEqual(is_valid, False) + self._assert_required_context_id_message(validation_messages) + + def test_invalid_user_role(self): + """ + Ensure that instances of Lti1p3LaunchData are instantiated with a user_role that is in the LTI_1P3_ROLE_MAP. + """ + user_role = "cat" + launch_data = Lti1p3LaunchData( + user_id="1", + user_role=user_role, + config_id="1", + resource_link_id="resource_link_id", + ) + + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + + self.assertEqual(is_valid, False) + self.assertEqual( + validation_messages, + [f"The user_role attribute {user_role} is not a valid user_role."] + ) + + def test_invalid_context_type(self): + """ + Ensure that instances of Lti1p3LaunchData are instantiated with a context_type that is one of group, + course_offering, course_section, or course_template. + """ + context_type = "invalid_context" + + launch_data = Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", + context_id="1", + context_type=context_type, + ) + + is_valid, validation_messages = validate_lti_1p3_launch_data(launch_data) + + self.assertEqual(is_valid, False) + self.assertEqual( + validation_messages, + [f"The context_type attribute {context_type} in the launch data is not a valid context_type."] + ) + + class TestGetLti1p3LaunchInfo(TestCase): """ - Unit tests for get_lti_consumer API method. + Unit tests for get_lti_1p3_launch_info API method. """ def setUp(self): # Patch internal method to avoid calls to modulestore @@ -245,12 +373,21 @@ class TestGetLti1p3LaunchInfo(TestCase): return super().setUp() + @staticmethod + def _get_lti_1p3_launch_data(): + return Lti1p3LaunchData( + user_id="1", + user_role="student", + config_id="1", + resource_link_id="resource_link_id", + ) + 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_info() + get_lti_1p3_launch_info({}) def test_retrieve_with_id(self): """ @@ -259,8 +396,10 @@ class TestGetLti1p3LaunchInfo(TestCase): location = 'block-v1:course+test+2020+type@problem+block@test' lti_config = LtiConfiguration.objects.create(location=location) + launch_data = self._get_lti_1p3_launch_data() + # Call and check returns - launch_info = get_lti_1p3_launch_info(config_id=lti_config.id) + launch_info = get_lti_1p3_launch_info(launch_data, config_id=lti_config.id) # Not checking all data here, there's a test specific for that self.assertEqual(launch_info['client_id'], lti_config.lti_1p3_client_id) @@ -273,6 +412,8 @@ class TestGetLti1p3LaunchInfo(TestCase): block.location = 'block-v1:course+test+2020+type@problem+block@test' block.lti_version = LtiConfiguration.LTI_1P3 + launch_data = self._get_lti_1p3_launch_data() + # Create LTI Config and Deep linking object lti_config = LtiConfiguration.objects.create(location=block.location) LtiDlContentItem.objects.create( @@ -282,7 +423,7 @@ class TestGetLti1p3LaunchInfo(TestCase): ) # Call API - launch_info = get_lti_1p3_launch_info(block=block) + launch_info = get_lti_1p3_launch_info(launch_data, block=block) # Retrieve created config and check full launch info data lti_config = LtiConfiguration.objects.get() @@ -291,12 +432,12 @@ class TestGetLti1p3LaunchInfo(TestCase): { 'client_id': lti_config.lti_1p3_client_id, 'keyset_url': 'https://example.com/api/lti_consumer/v1/public_keysets/{}'.format( - lti_config.location + lti_config.config_id ), 'deployment_id': '1', 'oidc_callback': 'https://example.com/api/lti_consumer/v1/launch/', 'token_url': 'https://example.com/api/lti_consumer/v1/token/{}'.format( - lti_config.location + lti_config.config_id ), 'deep_linking_launch_url': 'http://example.com', @@ -316,7 +457,10 @@ class TestGetLti1p3LaunchInfo(TestCase): content_type=LtiDlContentItem.IMAGE, attributes={"test": "this is a test attribute"} ) - launch_info = get_lti_1p3_launch_info(config_id=lti_config.id) + + launch_data = self._get_lti_1p3_launch_data() + + launch_info = get_lti_1p3_launch_info(launch_data, config_id=lti_config.id) self.assertEqual( launch_info, { @@ -346,22 +490,64 @@ class TestGetLti1p3LaunchUrl(Lti1P3TestCase): Check if the API creates a model if no object matching properties is found. """ with self.assertRaises(Exception): - get_lti_1p3_launch_start_url() + get_lti_1p3_launch_start_url({}) + + def test_get_normal_lti_launch_url(self): + """ + Check if the correct launch url is retrieved for a normal LTI 1.3 launch. + """ + self._setup_lti_block() + + launch_data = self._get_lti_1p3_launch_data() + + # Call API for normal LTI launch initiation. + launch_url = get_lti_1p3_launch_start_url(launch_data, block=self.xblock) + + parameters = parse_qs(urlparse(launch_url).query) + launch_data = get_data_from_cache(parameters.get("lti_message_hint")[0]) + + self.assertEqual(launch_data.message_type, "LtiResourceLinkRequest") + self.assertEqual(launch_data.deep_linking_content_item_id, None) - def test_retrieve_url(self): + def test_get_deep_linking_lti_launch_url(self): """ - Check if the correct launch url is retrieved + Check if the correct launch url is retrieved for a deep linking LTI 1.3 launch. """ self._setup_lti_block() - # Call API for normal LTI launch initiation - launch_url = get_lti_1p3_launch_start_url(block=self.xblock, hint="test_hint") - self.assertIn('login_hint=test_hint', launch_url) - self.assertIn('lti_message_hint=', launch_url) + launch_data = self._get_lti_1p3_launch_data() - # Call API for deep link launch - launch_url = get_lti_1p3_launch_start_url(block=self.xblock, deep_link_launch=True) - self.assertIn('lti_message_hint=deep_linking_launch', launch_url) + # Call API for normal LTI launch initiation. + launch_url = get_lti_1p3_launch_start_url(launch_data, block=self.xblock) + + parameters = parse_qs(urlparse(launch_url).query) + launch_url = get_lti_1p3_launch_start_url(launch_data, block=self.xblock, deep_link_launch=True) + + parameters = parse_qs(urlparse(launch_url).query) + launch_data = get_data_from_cache(parameters.get("lti_message_hint")[0]) + + self.assertEqual(launch_data.message_type, "LtiDeepLinkingRequest") + self.assertEqual(launch_data.deep_linking_content_item_id, None) + + def test_get_deep_linking_content_item_launch_url(self): + """ + Check if the correct launch url is retrieved for a deep linking content item LTI 1.3 launch. + """ + self._setup_lti_block() + + launch_data = self._get_lti_1p3_launch_data() + + # Call API for normal LTI launch initiation. + launch_url = get_lti_1p3_launch_start_url(launch_data, block=self.xblock) + + parameters = parse_qs(urlparse(launch_url).query) + launch_url = get_lti_1p3_launch_start_url(launch_data, block=self.xblock, dl_content_id="1") + + parameters = parse_qs(urlparse(launch_url).query) + launch_data = get_data_from_cache(parameters.get("lti_message_hint")[0]) + + self.assertEqual(launch_data.message_type, "LtiResourceLinkRequest") + self.assertEqual(launch_data.deep_linking_content_item_id, "1") class TestGetLti1p3ContentUrl(Lti1P3TestCase): @@ -373,9 +559,11 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): """ Check if the correct LTI content presentation is returned on a normal LTI Launch. """ + launch_data = self._get_lti_1p3_launch_data() + mock_get_launch_url.return_value = 'test_url' self._setup_lti_block() - self.assertEqual(get_lti_1p3_content_url(block=self.xblock), 'test_url') + self.assertEqual(get_lti_1p3_content_url(launch_data, block=self.xblock), 'test_url') def test_lti_content_presentation_single_link(self): """ @@ -384,6 +572,8 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): """ self._setup_lti_block() + launch_data = self._get_lti_1p3_launch_data() + # Create LTI DL content items lti_content = LtiDlContentItem.objects.create( lti_configuration=self.lti_config, @@ -392,13 +582,13 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): ) # Call API to retrieve content item URL - launch_url = get_lti_1p3_content_url(block=self.xblock) - self.assertIn( - # Checking for `deep_linking_content_launch:<content_item_id>` - # URL Encoded `:` is `%3A` - f'lti_message_hint=deep_linking_content_launch%3A{lti_content.id}', - launch_url, - ) + launch_url = get_lti_1p3_content_url(launch_data, block=self.xblock) + + parameters = parse_qs(urlparse(launch_url).query) + launch_data = get_data_from_cache(parameters.get("lti_message_hint")[0]) + + self.assertEqual(lti_content.id, launch_data.deep_linking_content_item_id) + self.assertEqual(launch_data.message_type, "LtiResourceLinkRequest") def test_lti_content_presentation_multiple_links(self): """ @@ -407,6 +597,15 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): """ self._setup_lti_block() + launch_data = self._get_lti_1p3_launch_data() + + launch_data_key = get_cache_key( + app="lti", + key="launch_data", + user_id=launch_data.user_id, + resource_link_id=launch_data.resource_link_id + ) + # Create LTI DL content items for _ in range(3): LtiDlContentItem.objects.create( @@ -418,8 +617,8 @@ class TestGetLti1p3ContentUrl(Lti1P3TestCase): # Call API to retrieve content item URL self.assertIn( # Checking for the content presentation URL - f"/api/lti_consumer/v1/lti/{self.lti_config.id}/lti-dl/content", - get_lti_1p3_content_url(block=self.xblock), + f"/api/lti_consumer/v1/lti/{self.lti_config.id}/lti-dl/content?launch_data_key={launch_data_key}", + get_lti_1p3_content_url(launch_data, block=self.xblock), ) diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py index 242924b929d56f1bad19e3b8eef28b1496829fd0..7f535df62952f7db0cc11cf9ebe6393bc40a3a48 100644 --- a/lti_consumer/tests/unit/test_lti_xblock.py +++ b/lti_consumer/tests/unit/test_lti_xblock.py @@ -15,14 +15,15 @@ from django.test import override_settings from django.test.testcases import TestCase from django.utils import timezone from jwkest.jwk import RSAKey, KEYS -from opaque_keys.edx.keys import UsageKey from lti_consumer.exceptions import LtiError +from lti_consumer.api import _get_lti_config +from lti_consumer.data import Lti1p3LaunchData from lti_consumer.lti_xblock import LtiConsumerXBlock, parse_handler_suffix, valid_config_type_values from lti_consumer.lti_1p3.tests.utils import create_jwt -from lti_consumer.tests.unit import test_utils -from lti_consumer.tests.unit.test_utils import FAKE_USER_ID, make_jwt_request, make_request, make_xblock +from lti_consumer.tests import test_utils +from lti_consumer.tests.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">' @@ -150,28 +151,28 @@ class TestProperties(TestLtiConsumerXBlock): 'edx-platform.is_authenticated': True, } self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user) - self.assertEqual(self.xblock.role, 'Student,Learner') + self.assertEqual(self.xblock.role, 'student') fake_user.opt_attrs = { 'edx-platform.user_role': 'guest', 'edx-platform.is_authenticated': True, } self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user) - self.assertEqual(self.xblock.role, 'Student,Learner') + self.assertEqual(self.xblock.role, 'guest') fake_user.opt_attrs = { 'edx-platform.user_role': 'staff', 'edx-platform.is_authenticated': True, } self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user) - self.assertEqual(self.xblock.role, 'Administrator') + self.assertEqual(self.xblock.role, 'staff') fake_user.opt_attrs = { 'edx-platform.user_role': 'instructor', 'edx-platform.is_authenticated': True, } self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user) - self.assertEqual(self.xblock.role, 'Instructor') + self.assertEqual(self.xblock.role, 'instructor') fake_user.opt_attrs = { 'edx-platform.user_role': 'student', @@ -276,13 +277,29 @@ class TestProperties(TestLtiConsumerXBlock): with self.assertRaises(LtiError): __ = self.xblock.user_id + def test_external_user_id(self): + """ + Test `external_user_id` returns the correct external user ID. + """ + external_user_id = "external_user_id" + self.xblock.runtime.service(self, 'user').get_external_user_id = Mock(return_value=external_user_id) + self.assertEqual(self.xblock.external_user_id, external_user_id) + + def test_external_user_id_none(self): + """ + Test `external_user_id` raises LtiError when the external user ID cannot be returned. + """ + self.xblock.runtime.service(self, 'user').get_external_user_id = Mock(return_value=None) + with self.assertRaises(LtiError): + __ = self.xblock.external_user_id + def test_resource_link_id(self): """ Test `resource_link_id` returns appropriate string """ self.assertEqual( self.xblock.resource_link_id, - f"{self.xblock.runtime.hostname}-{self.xblock.location.html_id()}" + f"{self.xblock.runtime.hostname}-{self.xblock.location.html_id()}" # pylint: disable=no-member ) @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.context_id') @@ -579,7 +596,6 @@ class TestGetLti1p1Consumer(TestLtiConsumerXBlock): key = 'test' secret = 'secret' self.xblock.lti_id = provider - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' type(mock_course).lti_passports = PropertyMock(return_value=[f"{provider}:{key}:{secret}"]) with patch('lti_consumer.models.LtiConfiguration.block', return_value=self.xblock): @@ -1275,7 +1291,8 @@ class TestGetContext(TestLtiConsumerXBlock): @ddt.data('lti_1p1', 'lti_1p3') @patch('lti_consumer.api.get_lti_1p3_content_url') - def test_context_keys(self, lti_version, lti_api_patch): + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.get_lti_1p3_launch_data') + def test_context_keys(self, lti_version, lti_api_patch, mock_get_lti_1p3_launch_data): """ Test `_get_context_for_template` returns dict with correct keys """ @@ -1287,6 +1304,11 @@ class TestGetContext(TestLtiConsumerXBlock): 'modal_vertical_offset', 'modal_horizontal_offset', 'modal_width', 'accept_grades_past_due' ) + + # This test isn't testing the value of any of the above keys. Calling _get_lti_block_launch_handler raises an + # error because the mocked XBlock location attribute does not act like a UsageKey, so mock out + # get_lti_1p3_launch_data to avoid accessing it. + mock_get_lti_1p3_launch_data.return_value = None context = self.xblock._get_context_for_template() # pylint: disable=protected-access for key in context_keys: @@ -1332,8 +1354,9 @@ class TestGetContext(TestLtiConsumerXBlock): context = self.xblock._get_context_for_template() # pylint: disable=protected-access self.assertEqual(context['launch_url'], lti_launch_url) + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.get_lti_1p3_launch_data') @patch('lti_consumer.api.get_lti_1p3_content_url') - def test_context_correct_origin_1p3(self, mock_get_lti_1p3_content_url): + def test_context_correct_origin_1p3(self, mock_get_lti_1p3_content_url, mock_get_lti_1p3_launch_data): """ Test that certain context keys relevant to 1.3 integrations that can be stored on different types of config_stores are pulled from the appropriate config_store. @@ -1348,6 +1371,10 @@ class TestGetContext(TestLtiConsumerXBlock): type(mock_lti_consumer).launch_url = PropertyMock(return_value=lti_1p3_launch_url) self.xblock._get_lti_consumer = Mock(return_value=mock_lti_consumer) # pylint: disable=protected-access + # Calling _get_lti_block_launch_handler raises an error because the mocked XBlock location attribute does not + # act like a UsageKey, so mock out get_lti_1p3_launch_data to avoid accessing it. + mock_get_lti_1p3_launch_data.return_value = None + context = self.xblock._get_context_for_template() # pylint: disable=protected-access self.assertEqual(context['lti_1p3_launch_url'], lti_1p3_launch_url) @@ -1358,7 +1385,7 @@ class TestProcessorSettings(TestLtiConsumerXBlock): Unit tests for the adding custom LTI parameters. """ settings = { - 'parameter_processors': ['lti_consumer.tests.unit.test_utils:dummy_processor'] + 'parameter_processors': ['lti_consumer.tests.test_utils:dummy_processor'] } def test_no_processors_by_default(self): @@ -1390,7 +1417,7 @@ class TestProcessorSettings(TestLtiConsumerXBlock): }, { # Non-existent processor 'parameter_processors': [ - 'lti_consumer.tests.unit.test_utils:non_existent', + 'lti_consumer.tests.test_utils:non_existent', ], }) @patch('lti_consumer.lti_xblock.log') @@ -1430,8 +1457,6 @@ class TestLtiConsumer1p3XBlock(TestCase): 'lti_1p3_oidc_url': 'http://tool.example/oidc', } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' self.mock_filter_enabled_patcher = patch("lti_consumer.lti_xblock.external_config_filter_enabled") self.mock_database_config_enabled_patcher = patch("lti_consumer.lti_xblock.database_config_enabled") @@ -1440,21 +1465,72 @@ class TestLtiConsumer1p3XBlock(TestCase): self.addCleanup(self.mock_filter_enabled_patcher.stop) self.addCleanup(self.mock_database_config_enabled_patcher.stop) + def test_get_lti_1p3_launch_data(self): + """ + Test that get_lti_1p3_launch_data returns an instance of Lti1p3LaunchData with the correct data. + """ + # Mock out the user role and external_user_id properties. + fake_user = Mock() + fake_user.opt_attrs = { + 'edx-platform.user_role': 'instructor', + 'edx-platform.is_authenticated': True, + } + self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user) + self.xblock.runtime.service(self, 'user').get_external_user_id = Mock(return_value="external_user_id") + + # Mock out get_context_title to avoid calling into the compatability layer. + self.xblock.get_context_title = Mock(return_value="context_title") + + launch_data = self.xblock.get_lti_1p3_launch_data() + + lti_config = _get_lti_config(block=self.xblock) + + course_key = str(self.xblock.location.course_key) # pylint: disable=no-member + expected_launch_data = Lti1p3LaunchData( + user_id="external_user_id", + user_role="instructor", + config_id=lti_config.config_id, + resource_link_id=str(self.xblock.location), # pylint: disable=no-member + launch_presentation_document_target="iframe", + message_type="LtiResourceLinkRequest", + context_id=course_key, + context_type=["course_offering"], + context_title="context_title", + context_label=course_key, + ) + + self.assertEqual( + launch_data, + expected_launch_data + ) + + @patch('lti_consumer.plugin.compat.get_course_by_id') + def test_get_context_title(self, mock_get_course_by_id): + """ + Test that get_context_title returns the correct context title + """ + mock_course = Mock() + mock_course.display_name_with_default = "DemoX" + mock_course.display_org_with_default = "edX" + + mock_get_course_by_id.return_value = mock_course + + self.assertEqual(self.xblock.get_context_title(), "DemoX - edX") + def test_studio_view(self): """ Test that the studio settings view load the custom js. """ - self.xblock.location = Mock() - self.xblock.location.__get__ = 'block-v1:course+test+2020+type@problem+block@test' - self.xblock.location.course_key = 'course-v1:Demo_Course' response = self.xblock.studio_view({}) self.assertEqual(response.js_init_fn, 'LtiConsumerXBlockInitStudio') + @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.get_lti_1p3_launch_data') @patch('lti_consumer.api.get_lti_1p3_launch_info') - def test_author_view(self, mock_get_launch_info): + def test_author_view(self, mock_get_launch_info, mock_lti_get_1p3_launch_data): """ Test that the studio view loads LTI 1.3 view. """ + mock_lti_get_1p3_launch_data.return_value = None mock_get_launch_info.return_value = { 'client_id': "mock-client_id", 'keyset_url': "mock-keyset_url", @@ -1497,8 +1573,7 @@ class TestLti1p3AccessTokenEndpoint(TestLtiConsumerXBlock): 'lti_1p3_tool_public_key': self.public_key, } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' + patcher = patch( 'lti_consumer.models.compat', **{'load_block_as_anonymous_user.return_value': self.xblock} @@ -1669,8 +1744,6 @@ class TestLti1p3AccessTokenJWK(TestCase): '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") @@ -1753,7 +1826,6 @@ class TestSubmitStudioEditsHandler(TestLtiConsumerXBlock): def setUp(self): super().setUp() - self.xblock.location = UsageKey.from_string('block-v1:course+test+2020+type@problem+block@test') self.xblock.lti_version = "lti_1p3" db_config_waffle_patcher = patch('lti_consumer.lti_xblock.database_config_enabled', return_value=True) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 58a2ccc40daf54747c7877fd2596d91e3c004303..aa06952c8ea2ed1fa7115ec9cb3621cc4873c528 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -3,7 +3,7 @@ Unit tests for LTI models. """ from contextlib import contextmanager from datetime import datetime, timedelta -from unittest.mock import patch, Mock, PropertyMock +from unittest.mock import patch import ddt from Cryptodome.PublicKey import RSA @@ -22,7 +22,7 @@ from lti_consumer.models import ( LtiConfiguration, LtiDlContentItem, ) -from lti_consumer.tests.unit.test_utils import make_xblock +from lti_consumer.tests.test_utils import make_xblock @ddt.ddt @@ -54,22 +54,20 @@ class TestLtiConfigurationModel(TestCase): 'lti_advantage_deep_linking_enabled': True, } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' # Creates an LTI configuration objects for testing self.lti_1p1_config = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P1 ) self.lti_1p3_config = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3 ) self.lti_1p3_config_db = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, config_store=LtiConfiguration.CONFIG_ON_DB, lti_advantage_ags_mode='programmatic', @@ -92,7 +90,7 @@ class TestLtiConfigurationModel(TestCase): Helper function to create a LtiConfiguration object with specific attributes """ return LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3, **kwargs ) @@ -385,10 +383,6 @@ class TestLtiConfigurationModel(TestCase): self.lti_1p3_config.config_store = self.lti_1p3_config.CONFIG_ON_DB self.lti_1p3_config.block = self.xblock - self.xblock.location = Mock() - course_key_mock = PropertyMock(return_value='course-v1:edX+DemoX+Demo_Course') - type(self.xblock.location).course_key = course_key_mock - self.lti_1p3_config_db.block = self.xblock with patch("lti_consumer.models.database_config_enabled", return_value=False),\ @@ -495,11 +489,9 @@ class TestLtiDlContentItemModel(TestCase): self.xblock_attributes = {'lti_version': 'lti_1p3'} self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) - # Set dummy location so that UsageKey lookup is valid - self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' self.lti_1p3_config = LtiConfiguration.objects.create( - location=str(self.xblock.location), + location=self.xblock.location, # pylint: disable=no-member version=LtiConfiguration.LTI_1P3 ) @@ -507,6 +499,7 @@ class TestLtiDlContentItemModel(TestCase): """ Test String representation of model. """ + content_item = LtiDlContentItem.objects.create( lti_configuration=self.lti_1p3_config, content_type=LtiDlContentItem.IMAGE, @@ -514,7 +507,8 @@ class TestLtiDlContentItemModel(TestCase): ) self.assertEqual( str(content_item), - "[CONFIG_ON_XBLOCK] lti_1p3 - block-v1:course+test+2020+type@problem+block@test: image" + "[CONFIG_ON_XBLOCK] lti_1p3 - " + "block-v1:edX+DemoX+Demo_Course+type@problem+block@466f474fa4d045a8b7bde1b911e095ca: image" ) diff --git a/lti_consumer/tests/unit/test_outcomes.py b/lti_consumer/tests/unit/test_outcomes.py index 9fe0337751e84e1b8a807c8438bba5e9e7a5fa21..d97d926f826436af4518df60645c79f69da9084a 100644 --- a/lti_consumer/tests/unit/test_outcomes.py +++ b/lti_consumer/tests/unit/test_outcomes.py @@ -11,7 +11,7 @@ from unittest.mock import Mock, PropertyMock, patch from lti_consumer.exceptions import LtiError from lti_consumer.outcomes import OutcomeService, parse_grade_xml_body from lti_consumer.tests.unit.test_lti_xblock import TestLtiConsumerXBlock -from lti_consumer.tests.unit.test_utils import make_request +from lti_consumer.tests.test_utils import make_request REQUEST_BODY_TEMPLATE_VALID = textwrap.dedent(""" <?xml version="1.0" encoding="UTF-8"?> diff --git a/lti_consumer/tests/unit/test_utils.py b/lti_consumer/tests/unit/test_utils.py index 214db164fe6133d2e1408dd370d105823354ed1c..439377228da7224170de84f72b47f356f8ee015c 100644 --- a/lti_consumer/tests/unit/test_utils.py +++ b/lti_consumer/tests/unit/test_utils.py @@ -1,91 +1,115 @@ """ -Utility functions used within unit tests +Unit tests for lti_consumer.utils module """ +from unittest.mock import Mock, patch -from unittest.mock import Mock -import urllib -from webob import Request -from workbench.runtime import WorkbenchRuntime -from xblock.fields import ScopeIds -from xblock.runtime import DictKeyValueStore, KvsFieldData +import ddt +from django.test.testcases import TestCase -FAKE_USER_ID = 'fake_user_id' +from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE +from lti_consumer.utils import ( + get_lti_1p3_context_types_claim, + get_lti_1p3_launch_data_cache_key, + cache_lti_1p3_launch_data, + get_data_from_cache, +) -def make_xblock(xblock_name, xblock_cls, attributes): +@ddt.ddt +class TestGetLti1p3ContextTypesClaim(TestCase): """ - Helper to construct XBlock objects + Tests for the get_lti_1p3_context_types_claim function of the utils module. """ - runtime = WorkbenchRuntime() - key_store = DictKeyValueStore() - db_model = KvsFieldData(key_store) - ids = generate_scope_ids(runtime, xblock_name) - xblock = xblock_cls(runtime, db_model, scope_ids=ids) - xblock.category = Mock() - xblock.location = Mock( - html_id=Mock(return_value='sample_element_id'), + + @ddt.data( + (["course_offering"], [LTI_1P3_CONTEXT_TYPE.course_offering]), + (["course_offering", "group"], [LTI_1P3_CONTEXT_TYPE.course_offering, LTI_1P3_CONTEXT_TYPE.group]), ) - xblock.runtime = Mock( - hostname='localhost', + @ddt.unpack + def test_get_lti_1p3_context_types_claim(self, argument, expected_output): + """ + Test that get_lti_1p3_context_types_claim returns the correct context_types. + """ + lti_context_types_claims = get_lti_1p3_context_types_claim(argument) + + self.assertEqual(lti_context_types_claims, expected_output) + + @ddt.data( + ["course_offering", "nonsense"], + ["nonsense"], ) - xblock.course_id = 'course-v1:edX+DemoX+Demo_Course' - for key, value in attributes.items(): - setattr(xblock, key, value) - return xblock - - -def generate_scope_ids(runtime, block_type): - """ - Helper to generate scope IDs for an XBlock - """ - def_id = runtime.id_generator.create_definition(block_type) - usage_id = runtime.id_generator.create_usage(def_id) - return ScopeIds('user', block_type, def_id, usage_id) - - -def make_request(body, method='POST'): - """ - Helper to make a request - """ - request = Request.blank('/') - request.method = 'POST' - request.body = body.encode('utf-8') - request.method = method - 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. - """ - return { - 'custom_author_email': 'author@example.com', - 'custom_author_country': '', - } + def test_get_lti_1p3_context_types_claim_invalid(self, argument): + """ + Test that get_lti_1p3_context_types_claim if any of the context_types are invalid. + """ + with self.assertRaises(ValueError): + get_lti_1p3_context_types_claim(argument) -def defaulting_processor(_xblock): +@ddt.ddt +class TestCacheUtilities(TestCase): """ - A dummy LTI parameter processor with default params. + Tests for the cache utilities in the utils module. """ - -defaulting_processor.lti_xblock_default_params = { - 'custom_name': 'Lex', - 'custom_country': '', -} + @patch('lti_consumer.utils.get_cache_key') + @ddt.data(None, "1") + def test_get_lti_1p3_launch_data_cache_key(self, deep_linking_content_item_id, mock_get_cache_key): + """ + Test that get_lti_1p3_launch_data_cache_key calls the get_cache_key function with the correct arguments. + """ + mock_launch_data = Mock() + mock_launch_data.user_id = "1" + mock_launch_data.resource_link_id = "1" + mock_launch_data.deep_linking_content_item_id = deep_linking_content_item_id + + get_lti_1p3_launch_data_cache_key(mock_launch_data) + + get_cache_key_kwargs = { + "app": "lti", + "key": "launch_data", + "user_id": "1", + "resource_link_id": "1" + } + + if deep_linking_content_item_id: + get_cache_key_kwargs['deep_linking_content_item_id'] = deep_linking_content_item_id + + mock_get_cache_key.assert_called_with( + **get_cache_key_kwargs + ) + + @patch('lti_consumer.utils.TieredCache.set_all_tiers') + @patch('lti_consumer.utils.get_lti_1p3_launch_data_cache_key') + def test_cache_lti_1p3_launch_data(self, mock_get_cache_key, mock_set_all_tiers): + """ + Test that cache_lti_1p3_launch_data caches the launch_data and returns the cache key. + """ + mock_launch_data = Mock() + + mock_get_cache_key.return_value = "launch_data_cache_key" + + cache_lti_1p3_launch_data(mock_launch_data) + + mock_get_cache_key.assert_called_with(mock_launch_data) + mock_set_all_tiers.assert_called_with("launch_data_cache_key", mock_launch_data, django_cache_timeout=600) + + @patch('lti_consumer.utils.TieredCache.get_cached_response') + @ddt.data(True, False) + def test_get_data_from_cache(self, is_found, mock_get_cached_response): + """ + Test that get_data_from_cache returns the data from the cache correctly or returns None if the data + is not in the cache. + """ + mock_cached_data = Mock() + mock_cached_data.value = "value" + mock_cached_data.is_found = is_found + + mock_get_cached_response.return_value = mock_cached_data + + value = get_data_from_cache("key") + + if is_found: + self.assertEqual(value, "value") + else: + self.assertIsNone(value) diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index c29eb45f6c9b3839b202ace7f5afeca30007d6f9..eba6abd7b0922db2018a89d89c9c055411cd675d 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -3,10 +3,13 @@ Utility functions for LTI Consumer block """ import logging from importlib import import_module +from urllib.parse import urlencode from django.conf import settings +from edx_django_utils.cache import get_cache_key, TieredCache from lti_consumer.plugin.compat import get_external_config_waffle_flag, get_database_config_waffle_flag +from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE log = logging.getLogger(__name__) @@ -30,15 +33,15 @@ def get_lms_base(): return settings.LMS_ROOT_URL -def get_lms_lti_keyset_link(location): +def get_lms_lti_keyset_link(config_id): """ Returns an LMS link to LTI public keyset endpoint - :param location: the location or config_id of the LtiConfiguration object + :param config_id: the config_id of the LtiConfiguration object """ - return "{lms_base}/api/lti_consumer/v1/public_keysets/{location}".format( + return "{lms_base}/api/lti_consumer/v1/public_keysets/{config_id}".format( lms_base=get_lms_base(), - location=str(location), + config_id=str(config_id), ) @@ -53,15 +56,15 @@ def get_lms_lti_launch_link(): ) -def get_lms_lti_access_token_link(location): +def get_lms_lti_access_token_link(config_id): """ Returns an LMS link to LTI Launch endpoint - :param location: the location or config_id of the LtiConfiguration object + :param config_id: the config_id of the LtiConfiguration object """ - return "{lms_base}/api/lti_consumer/v1/token/{location}".format( + return "{lms_base}/api/lti_consumer/v1/token/{config_id}".format( lms_base=get_lms_base(), - location=str(location), + config_id=str(config_id), ) @@ -97,16 +100,26 @@ def get_lti_deeplinking_response_url(lti_config_id): ) -def get_lti_deeplinking_content_url(lti_config_id): +def get_lti_deeplinking_content_url(lti_config_id, launch_data): """ Return the LTI Deep Linking content presentation endpoint :param lti_config_id: LTI configuration id + :param launch_data: (lti_consumer.data.Lti1p3LaunchData): a class containing data necessary for an LTI 1.3 launch """ - return "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-dl/content".format( + url = "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-dl/content".format( lms_base=get_lms_base(), lti_config_id=str(lti_config_id), ) + url += "?" + + launch_data_key = cache_lti_1p3_launch_data(launch_data) + + url += urlencode({ + "launch_data_key": launch_data_key, + }) + + return url def get_lti_nrps_context_membership_url(lti_config_id): @@ -172,3 +185,82 @@ def database_config_enabled(course_key): return False if it is not enabled. """ return get_database_config_waffle_flag().is_enabled(course_key) + + +def get_lti_1p3_context_types_claim(context_types): + """ + Return the LTI 1.3 context_type claim based on the context_type slug provided as an argument. + + Arguments: + context_type (list): a list of context_types + """ + lti_context_types = [] + + for context_type in context_types: + if context_type == "group": + lti_context_types.append(LTI_1P3_CONTEXT_TYPE.group) + elif context_type == "course_offering": + lti_context_types.append(LTI_1P3_CONTEXT_TYPE.course_offering) + elif context_type == "course_section": + lti_context_types.append(LTI_1P3_CONTEXT_TYPE.course_section) + elif context_type == "course_template": + lti_context_types.append(LTI_1P3_CONTEXT_TYPE.course_template) + else: + raise ValueError("context_type is not a valid type.") + + return lti_context_types + + +def get_lti_1p3_launch_data_cache_key(launch_data): + """ + Return the cache key for the instance of Lti1p3LaunchData. + + Arugments: + launch_data (lti_consumer.data.Lti1p3LaunchData): a class containing data necessary for an LTI 1.3 launch + """ + kwargs = { + "app": "lti", + "key": "launch_data", + "user_id": launch_data.user_id, + "resource_link_id": launch_data.resource_link_id + } + + # If the LTI 1.3 launch is a deep linking launch to a particular content item, then the launch data should be cached + # per content item to properly do an LTI 1.3 deep linking launch. Otherwise, deep linking launches to different + # items will not work via the deep_linking_content_endpoint view, because launch data is cached at the time that the + # preflight URL is generated. + content_item_id = launch_data.deep_linking_content_item_id + if content_item_id: + kwargs["deep_linking_content_item_id"] = content_item_id + + return get_cache_key(**kwargs) + + +def cache_lti_1p3_launch_data(launch_data): + """ + Insert the launch_data into the cache and return the cache key. + + Arguments: + launch_data (lti_consumer.data.Lti1p3LaunchData): a class containing data necessary for an LTI 1.3 launch + """ + launch_data_key = get_lti_1p3_launch_data_cache_key(launch_data) + + # Insert the data into the cache with a 600 second timeout. + TieredCache.set_all_tiers(launch_data_key, launch_data, django_cache_timeout=600) + + return launch_data_key + + +def get_data_from_cache(cache_key): + """ + Return data stored in the cache with the cache key, if it exists. If not, return none. + + Arguments: + cache_key: the key for the data in the cache + """ + cached_data = TieredCache.get_cached_response(cache_key) + + if cached_data.is_found: + return cached_data.value + + return None diff --git a/requirements/base.in b/requirements/base.in index 2cf6882603c6a56c41990de9614bf4e2582b0ac3..f946ff4b31361dc3c6a7209b26e7b51c1e0841ed 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -1,6 +1,7 @@ # Core requirements for using this package -c constraints.txt +attrs lxml bleach django diff --git a/requirements/base.txt b/requirements/base.txt index 771f6e8428fab387a8fa20ef8934fc0722ca718a..eaa9a19927f6e01023aa0c6320938e39cbf7cacd 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -8,6 +8,8 @@ appdirs==1.4.4 # via fs asgiref==3.5.2 # via django +attrs==22.1.0 + # via -r requirements/base.in bleach==5.0.1 # via -r requirements/base.in certifi==2022.9.24 diff --git a/requirements/ci.txt b/requirements/ci.txt index 4fe8d2af14d38e6de654ed57286bfc310ff444cd..81bcc0a7138deb0c1e341be23020391509e4f201 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -21,6 +21,8 @@ astroid==2.12.10 # -r requirements/test.txt # pylint # pylint-celery +attrs==22.1.0 + # via -r requirements/test.txt binaryornot==0.4.4 # via # -r requirements/test.txt @@ -49,7 +51,6 @@ certifi==2022.9.24 cffi==1.15.1 # via # -r requirements/test.txt - # cryptography # pynacl chardet==5.0.0 # via @@ -189,11 +190,6 @@ jaraco-classes==3.2.3 # via # -r requirements/test.txt # keyring -jeepney==0.8.0 - # via - # -r requirements/test.txt - # keyring - # secretstorage jinja2==3.1.2 # via # -r requirements/test.txt @@ -391,10 +387,6 @@ s3transfer==0.6.0 # via # -r requirements/test.txt # boto3 -secretstorage==3.3.3 - # via - # -r requirements/test.txt - # keyring simplejson==3.17.6 # via # -r requirements/test.txt diff --git a/requirements/dev.txt b/requirements/dev.txt index e89ce09246ad4d54b0d2787334a99e5f3c5f4fd1..772c68029fc1ae0e0d049df60db595c2b862f6e1 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -12,6 +12,8 @@ asgiref==3.5.2 # via # -r requirements/base.txt # django +attrs==22.1.0 + # via -r requirements/base.txt bleach==5.0.1 # via -r requirements/base.txt certifi==2022.9.24 diff --git a/requirements/quality.txt b/requirements/quality.txt index 2125e0a1f356bcaff9e6a6b4dbe49a75ecf472bb..08d716593d4a1fda4a0c9ba700ed3038b944448d 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -18,6 +18,8 @@ astroid==2.12.10 # via # pylint # pylint-celery +attrs==22.1.0 + # via -r requirements/base.txt binaryornot==0.4.4 # via cookiecutter bleach==5.0.1 diff --git a/requirements/test.txt b/requirements/test.txt index 392b3cfcff8dfc0db3c60a277377e2d4898a39d2..4252b81b46f6ebaac5aeadc76499f558affba58f 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -18,6 +18,8 @@ astroid==2.12.10 # via # pylint # pylint-celery +attrs==22.1.0 + # via -r requirements/base.txt binaryornot==0.4.4 # via cookiecutter bleach==5.0.1 @@ -39,7 +41,6 @@ certifi==2022.9.24 cffi==1.15.1 # via # -r requirements/base.txt - # cryptography # pynacl chardet==5.0.0 # via binaryornot @@ -144,10 +145,6 @@ isort==5.10.1 # via pylint jaraco-classes==3.2.3 # via keyring -jeepney==0.8.0 - # via - # keyring - # secretstorage jinja2==3.1.2 # via # code-annotations @@ -293,8 +290,6 @@ rich==12.5.1 # via twine s3transfer==0.6.0 # via boto3 -secretstorage==3.3.3 - # via keyring simplejson==3.17.6 # via # -r requirements/base.txt