Merge pull request #278 from openedx/mroytman/MST-1585-LtiError-user_id-LTI-1.1-launch

michaelroytman · web-flow · commit 682c90ee78a4 · 2022-08-22T14:48:38.000-04:00
Handle LtiError Error During LTI 1.1 Launch When Calling user_id for Unauthenticated User
diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py
@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '4.4.0'
+__version__ = '4.5.0'
diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py
@@ -745,7 +745,11 @@ def role(self):
         """
         Get system user role and convert it to LTI role.
         """
-        role = self.runtime.service(self, 'user').get_current_user().opt_attrs.get('edx-platform.user_role', 'student')
+        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')
 
     @property
@@ -791,27 +795,7 @@ def user_id(self):
             'edx-platform.anonymous_user_id', None)
 
         if user_id is None:
-            # TODO: Remove the supplemental log messaging. The logs were amended to help diagnose the bug in MST-1540:
-            # https://2u-internal.atlassian.net/browse/MST-1540. The bug is not easily reproducible in production or
-            # development, so these logs were added to determine whether the currently requesting user is an
-            # AnonymousUser or a currently authenticated user.
-            # pylint: disable=import-outside-toplevel
-            from crum import get_current_request
-            request = get_current_request()
-
-            # A test (lti_consumer.tests.unit.test_lti_xblock.TestProperties.test_user_id_none) calls this function
-            # directly, outside the scope of a request, causing it to fail if we assume this function runs within the
-            # context of a request. It's easier to modify the code here to handle the case when there is no request
-            # object to appease the test than to modify the test, because these changes to the log are temporary and
-            # will be removed shortly.
-            request_user_id = None
-            request_user_authenticated = None
-            if request:
-                request_user_id = request.user.id
-                request_user_authenticated = request.user.is_authenticated
-            raise LtiError(self.ugettext(f"Could not get user id for current request"
-                                         f" and currently requesting user id is: {request_user_id}"
-                                         f" and is_authenticated is: {request_user_authenticated}"))
+            raise LtiError(self.ugettext("Could not get user id for current request"))
         return str(user_id)
 
     def get_icon_class(self):
@@ -998,6 +982,10 @@ def extract_real_user_data(self):
         Extract and return real user data from the runtime
         """
         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"))
+
         user_data = {
             'user_email': None,
             'user_username': None,
@@ -1100,10 +1088,21 @@ def lti_launch_handler(self, request, suffix=''):  # pylint: disable=unused-argu
         Returns:
             webob.response: HTML LTI launch form
         """
-        real_user_data = self.extract_real_user_data()
-
         lti_consumer = self._get_lti_consumer()
 
+        # Occassionally, users try to do an LTI launch while they are unauthenticated. It is not known why this occurs.
+        # Sometimes, it is due to a web crawlers; other times, it is due to actual users of the platform. Regardless,
+        # return a 400 response with an appropriate error template.
+        try:
+            real_user_data = self.extract_real_user_data()
+            user_id = self.user_id
+            role = self.role
+            result_sourcedid = self.lis_result_sourcedid
+        except LtiError:
+            loader = ResourceLoader(__name__)
+            template = loader.render_django_template('/templates/html/lti_launch_error.html')
+            return Response(template, status=400, content_type='text/html')
+
         username = None
         email = None
         if self.ask_to_send_username and real_user_data['user_username']:
@@ -1112,12 +1111,13 @@ def lti_launch_handler(self, request, suffix=''):  # pylint: disable=unused-argu
             email = real_user_data['user_email']
 
         lti_consumer.set_user_data(
-            self.user_id,
-            self.role,
-            result_sourcedid=self.lis_result_sourcedid,
+            user_id,
+            role,
+            result_sourcedid=result_sourcedid,
             person_sourcedid=username,
             person_contact_email_primary=email
         )
+
         lti_consumer.set_context_data(
             self.context_id,
             self.course.display_name_with_default,
diff --git a/lti_consumer/plugin/views.py b/lti_consumer/plugin/views.py
@@ -150,7 +150,7 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
     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.')
-        return render(request, 'html/lti_1p3_launch_error.html', status=HTTP_400_BAD_REQUEST)
+        return render(request, 'html/lti_launch_error.html', status=HTTP_400_BAD_REQUEST)
 
     try:
         usage_key = UsageKey.from_string(usage_id)
@@ -161,7 +161,7 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
             exc,
             exc_info=True
         )
-        return render(request, 'html/lti_1p3_launch_error.html', status=HTTP_404_NOT_FOUND)
+        return render(request, 'html/lti_launch_error.html', status=HTTP_404_NOT_FOUND)
 
     try:
         lti_config = LtiConfiguration.objects.get(
@@ -173,7 +173,7 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
 
     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_1p3_launch_error.html', status=HTTP_404_NOT_FOUND)
+        return render(request, 'html/lti_launch_error.html', status=HTTP_404_NOT_FOUND)
 
     context = {}
 
@@ -269,7 +269,7 @@ def launch_gate_endpoint(request, suffix=None):  # pylint: disable=unused-argume
             exc,
             exc_info=True
         )
-        return render(request, 'html/lti_1p3_launch_error.html', context, status=HTTP_400_BAD_REQUEST)
+        return render(request, 'html/lti_launch_error.html', context, status=HTTP_400_BAD_REQUEST)
     except AssertionError as exc:
         log.warning(
             "Permission on LTI block %r denied for user %r: %s",
diff --git a/lti_consumer/templates/html/lti_launch_error.html b/lti_consumer/templates/html/lti_launch_error.html
@@ -7,7 +7,7 @@
     </head>
     <body>
         <p>
-            <b>{% trans "There was an error while launching the LTI 1.3 tool." %}</b>
+            <b>{% trans "There was an error while launching the LTI tool." %}</b>
         </p>
         <p>
             {% trans "If you're seeing this on a live course, please contact the course staff." %}
diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py
@@ -196,7 +196,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 1.3 tool.", response_body)
+        self.assertIn("There was an error while launching the LTI tool.", response_body)
         self.assertNotIn("% trans", response_body)
 
         with patch(
diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py
@@ -146,29 +146,40 @@ def test_role(self):
         """
         fake_user = Mock()
         fake_user.opt_attrs = {
-            'edx-platform.user_role': 'student'
+            'edx-platform.user_role': 'student',
+            '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')
 
         fake_user.opt_attrs = {
-            'edx-platform.user_role': 'guest'
+            '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')
 
         fake_user.opt_attrs = {
-            'edx-platform.user_role': 'staff'
+            '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')
 
         fake_user.opt_attrs = {
-            'edx-platform.user_role': 'instructor'
+            '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')
 
+        fake_user.opt_attrs = {
+            'edx-platform.user_role': 'student',
+            'edx-platform.is_authenticated': False,
+        }
+        with self.assertRaises(LtiError):
+            _ = self.xblock.role
+
     def test_course(self):
         """
         Test `course` calls modulestore.get_course
@@ -593,7 +604,8 @@ def test_get_real_user_callable(self):
         fake_user.emails = [fake_user_email]
         fake_username = 'fake'
         fake_user.opt_attrs = {
-            "edx-platform.username": fake_username
+            "edx-platform.username": fake_username,
+            "edx-platform.is_authenticated": True,
         }
 
         self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user)
@@ -616,14 +628,29 @@ def test_get_real_user_callable_with_language_preference(self):
         fake_user.opt_attrs = {
             "edx-platform.user_preferences": {
                 "pref-lang": "en"
-            }
+            },
+            "edx-platform.is_authenticated": True,
         }
 
         self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user)
 
         real_user_data = self.xblock.extract_real_user_data()
         self.assertEqual(real_user_data['user_language'], pref_language)
 
+    def test_unauthenticated_user(self):
+        """
+        Test that an LtiError is raised when the user is unauthenticated.
+        """
+        fake_user = Mock()
+        fake_user.opt_attrs = {
+            "edx-platform.is_authenticated": False,
+        }
+
+        self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user)
+
+        with self.assertRaises(LtiError):
+            self.xblock.extract_real_user_data()
+
 
 class TestStudentView(TestLtiConsumerXBlock):
     """
@@ -734,7 +761,8 @@ def setUp(self):
         fake_user.emails = [fake_user_email]
         fake_username = 'fake'
         fake_user.opt_attrs = {
-            "edx-platform.username": fake_username
+            "edx-platform.username": fake_username,
+            "edx-platform.is_authenticated": True,
         }
 
         self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user)
@@ -757,6 +785,36 @@ def test_generate_launch_request_called(self, mock_course):
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.content_type, 'text/html')
 
+    @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.course')
+    def test_lti_launch_handler_unauthenticated(self, mock_course):
+        """
+        Test that a 400 response an an appropriate template is rendered when a user is unauthenticated
+        during an LTI launch according to the LMS's user service.
+        """
+        provider = 'lti_provider'
+        key = 'test'
+        secret = 'secret'
+        type(mock_course).lti_passports = PropertyMock(return_value=[f"{provider}:{key}:{secret}"])
+
+        fake_user = Mock()
+        fake_user_email = 'abc@example.com'
+        fake_user.emails = [fake_user_email]
+        fake_username = 'fake'
+        fake_user.opt_attrs = {
+            "edx-platform.username": fake_username,
+            "edx-platform.is_authenticated": True,
+        }
+        self.xblock.runtime.service(self, 'user').get_current_user = Mock(return_value=fake_user)
+
+        request = make_request('', 'GET')
+        response = self.xblock.lti_launch_handler(request)
+
+        self.assertEqual(response.status_code, 400)
+        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)
+
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.course')
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.user_id', PropertyMock(return_value=FAKE_USER_ID))
     def test_publish_tracking_event(self, mock_course):
