feat: added error msg to launch error template (#394)

* feat: added error msg to launch error template

* fix: removed debug prints

* temp: manual test of validate_lti_1p3_launch_data

- custom object made for launch_data to cause the errors to trigger

* temp: Commented out tests for error msgs

* fix: html and error msg fix

* chore: lint

* fix: tests + 1.1 err msg

* chore: bump version and changelog

* test: refined LTI launch error tests

* chore: debug cleanup

CHANGELOG.rst

@@ -16,6 +16,10 @@ Please See the `releases tab <https://github.com/openedx/xblock-lti-consumer/rel
 Unreleased
 ~~~~~~~~~~
 
+9.5.6 - 2023-07-25
+------------------
+* Added LTI launch error messages to the template returned when these errors occur.
+
 9.5.5 - 2023-07-13
 ------------------
 * Fix broken call to LMS `get_block_for_descriptor_internal` due to merge with `get_block_for_descriptor`.

README.rst

@@ -151,6 +151,7 @@ Unit Testing
 * To run all of the unit tests at once, run `make test`
 * To run tests on individual files in development, run `python ./test.py -k=[name of test file without .py]`
 * For example, if you want to run the tests in test_permissions.py, run `python ./test.py -k=test_permissions`
+* To run a specific test in a file, run something like `python ./test.py -k=test_permissions.TestClass.test_function`
 
 Testing Against an LTI Provider
 ===============================

lti_consumer/__init__.py

@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '9.5.5'
+__version__ = '9.5.6'

lti_consumer/lti_xblock.py

@@ -1235,9 +1235,11 @@ def lti_launch_handler(self, request, suffix=''):  # pylint: disable=unused-argu
             role = LTI_1P1_ROLE_MAP.get(role, 'Student,Learner')
 
             result_sourcedid = self.lis_result_sourcedid
-        except LtiError:
+        # Fails if extract_real_user_data() fails
+        except LtiError as err:
             loader = ResourceLoader(__name__)
-            template = loader.render_django_template('/templates/html/lti_launch_error.html')
+            template = loader.render_django_template('/templates/html/lti_launch_error.html',
+                                                     context={"error_msg": err})
             return Response(template, status=400, content_type='text/html')
 
         username = None

lti_consumer/plugin/views.py

@@ -139,30 +139,50 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
 
     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)
+        error_msg = 'The lti_message_hint is missing or empty.'
+        log.info(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            status=HTTP_400_BAD_REQUEST
+        )
 
     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)
+        error_msg = 'The login_hint is missing or empty.'
+        log.info(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            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 trying to fetch the launch data during an LTI 1.3 launch when using the cache'
-            f' key {lti_message_hint}. The login hint is {login_hint}.'
+        error_msg = (
+            f'Unable to find record of an OIDC launch for the provided lti_message_hint: {lti_message_hint}'
+        )
+        log.warning(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            status=HTTP_400_BAD_REQUEST
         )
-        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(
-            f"The Lti1p3LaunchData is not valid. {validation_message}"
+        error_msg = f"The Lti1p3LaunchData is not valid. {validation_message}"
+        log.error(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            status=HTTP_400_BAD_REQUEST
         )
-        return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST)
 
     config_id = launch_data.config_id
     try:
@@ -174,8 +194,14 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
         raise Http404 from exc
 
     if lti_config.version != LtiConfiguration.LTI_1P3:
-        log.error("The LTI Version of configuration %s is not LTI 1.3", lti_config)
-        return render(request, 'html/lti_launch_error.html', status=HTTP_404_NOT_FOUND)
+        error_msg = f"The LTI Version of the following configuration is not LTI 1.3: {lti_config}"
+        log.error(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            status=HTTP_404_NOT_FOUND
+        )
 
     context = {}
 
@@ -210,11 +236,17 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
             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
+                error_msg = (
+                    f"The context_type key {context_type} in the launch "
+                    f"data does not represent a valid context_type."
+                )
+                log.error(error_msg)
+                return render(
+                    request,
+                    'html/lti_launch_error.html',
+                    context={"error_msg": error_msg},
+                    status=HTTP_400_BAD_REQUEST
                 )
-                return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST)
 
         lti_consumer.set_context_claim(
             launch_data.context_id,
@@ -302,12 +334,14 @@ 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
+        error_msg = f"Error preparing LTI 1.3 launch for resource with resource_link_id {resource_link_id}: {exc}"
         log.warning(
             "Error preparing LTI 1.3 launch for resource with resource_link_id %r: %s",
             resource_link_id,
             exc,
             exc_info=True
         )
+        context.update({"error_msg": error_msg})
         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
@@ -485,13 +519,25 @@ def deep_linking_content_endpoint(request, lti_config_id):
     """
     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)
+        error_msg = 'The launch_data_key query param in the request is missing or empty.'
+        log.info(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            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)
+        error_msg = f'There was a cache miss during an LTI 1.3 launch when using the cache_key {launch_data_key}.'
+        log.warning(error_msg)
+        return render(
+            request,
+            'html/lti_launch_error.html',
+            context={"error_msg": error_msg},
+            status=HTTP_400_BAD_REQUEST
+        )
     try:
         # Get LTI Configuration
         lti_config = LtiConfiguration.objects.get(id=lti_config_id)

lti_consumer/templates/html/lti_launch_error.html

@@ -7,7 +7,10 @@
     </head>
     <body>
         <p>
-            <b>{% trans "There was an error while launching the LTI tool." %}</b>
+            <b>{% trans "There was an error while launching the LTI tool: " %}</b>
+        </p>
+        <p>
+            {{error_msg}}
         </p>
         <p>
             {% trans "If you're seeing this on a live course, please contact the course staff." %}

lti_consumer/tests/unit/plugin/test_views.py

@@ -34,6 +34,7 @@ class TestLti1p3KeysetEndpoint(TestCase):
     """
     Test `public_keyset_endpoint` method.
     """
+
     def setUp(self):
         super().setUp()
 
@@ -107,6 +108,7 @@ class TestLti1p3LaunchGateEndpoint(TestCase):
     """
     Tests for the `launch_gate_endpoint` method.
     """
+
     def setUp(self):
         super().setUp()
 
@@ -187,55 +189,108 @@ def test_non_existant_lti_config(self):
 
     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.
+        Check for the expected error message 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)
+        self.assertIn(
+            'The lti_message_hint is missing or empty.',
+            str(response.content)
+        )
 
     def test_missing_required_login_hint_param(self):
         """
-        Check that a 400 error is returned when required login_hint query parameter is not provided.
+        Check for the expected error message 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):
+        # Assert expected error msg in the HttpResponse
+        self.assertIn(
+            'The login_hint is missing or empty.',
+            str(response.content)
+        )
+
+    @patch('lti_consumer.plugin.views.get_data_from_cache')
+    def test_missing_launch_data(self, mock_get_data_from_cache):
         """
-        Check that a 400 error is returned when required lti_message_hint query parameter is not associated with
-        launch_data in the cache.
+        Check that the expected error message returned when required lti_message_hint query parameter
+        is not associated with launch_data in the cache.
         """
+        # Mock getting the launch_data from the cache.
+        mock_get_data_from_cache.return_value = None
         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')
+        # Assert expected error msg in the HttpResponse
+        self.assertIn(
+            'Unable to find record of an OIDC launch for the provided lti_message_hint:',
+            str(response.content)
+        )
+
+    @patch('lti_consumer.plugin.views.validate_lti_1p3_launch_data')
+    @patch('lti_consumer.plugin.views.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_get_data_from_cache.return_value = {"context_type": "invalid_context_type"}
 
         # Mock checking the launch_data for validity.
-        mock_validate_launch_data.return_value = (False, [])
+        mock_validate_launch_data.return_value = (False, [
+            'The context_id attribute is required in the launch data if any optional context properties are provided.',
+        ])
 
         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')
+        # Assert expected error msg in the HttpResponse
+        self.assertIn(
+            'The context_id attribute is required in the launch data if any optional context properties are provided.',
+            str(response.content)
+        )
+
+    @patch('lti_consumer.plugin.views.validate_lti_1p3_launch_data')
+    @patch('lti_consumer.plugin.views.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_get_data_from_cache.return_value = Lti1p3LaunchData(
+            user_id=3,
+            user_role='instructor',
+            config_id=self.config.config_id,
+            resource_link_id=(
+                'block-v1:edX+LTI-xblock+consumer+'
+                'type@lti_consumer+block@7f9f07d76ce440f39d892f7f0d312cf2'
+            ),
+            preferred_username=None,
+            name=None,
+            email=None,
+            external_user_id='cd6fafb6-2bf7-4196-9b23-a1361c1bf0ef',
+            launch_presentation_document_target='iframe',
+            launch_presentation_return_url=None,
+            message_type='LtiStartProctoring',  # 'LtiResourceLinkRequest',
+            # context_id='course-v1:edX+LTI-xblock+consumer',
+            context_type=['invalid_context_type'],  # ['course_offering'],
+            context_title='edx-LTI-xblock-thing - edX',
+            context_label='course-v1:edX+LTI-xblock+consumer',
+            deep_linking_content_item_id=None,
+            proctoring_launch_data=Lti1p3ProctoringLaunchData(attempt_number=1)  # None
+        )
 
         # 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)
 
+        # Assert expected error msg in the HttpResponse
+        self.assertIn(
+            # The error message in the HttpResponse contains a messy formatted string, so only test for part it.
+            'the launch data does not represent a valid context_type.',
+            str(response.content)
+        )
+
     def test_lti_launch_response(self):
         """
         Check that the right launch response is generated
@@ -266,7 +321,7 @@ def test_launch_callback_endpoint_fails(self):
         self.assertEqual(response.status_code, 400)
 
         response_body = response.content.decode('utf-8')
-        self.assertIn("There was an error while launching the LTI tool.", response_body)
+        self.assertIn("There was an error while launching the LTI tool: ", response_body)
         self.assertNotIn("% trans", response_body)
 
         with patch(
@@ -529,6 +584,7 @@ class TestLti1p3AccessTokenEndpoint(TestCase):
     """
     Test `access_token_endpoint` method.
     """
+
     def setUp(self):
         super().setUp()
 

lti_consumer/tests/unit/test_lti_xblock.py

@@ -919,7 +919,7 @@ def test_lti_launch_handler_unauthenticated(self, mock_course):
         self.assertEqual(response.content_type, 'text/html')
 
         response_body = response.body.decode('utf-8')
-        self.assertIn("There was an error while launching the LTI tool.", response_body)
+        self.assertIn("There was an error while launching the LTI tool: ", response_body)
 
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.course')
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.anonymous_user_id', PropertyMock(return_value=FAKE_USER_ID))