Merge pull request #310 from openedx/mroytman/MST-1718-fix-basic-outcomes-result-service-user-id

Fix LTI 1.1 Basic Outcomes Service and LTI 2.0 Rsult Service to Support External User IDs

Author: michaelroytman

CHANGELOG.rst

@@ -16,7 +16,16 @@ Please See the [releases tab](https://github.com/openedx/xblock-lti-consumer/rel
 Unreleased
 ~~~~~~~~~~
 
-=======
+7.0.2 - 2022-11-29
+------------------
+* Fix the LTI 1.1 Outcome Results Service to be able to tie an outcome pass back to a user when the user ID is an
+  `external_user_id`.
+* Fix the LTI 2.0 Result Service to be able to tie a result pass back to a user when the user ID is an
+  `external_user_id`.
+* Update the `RESULT_SERVICE_SUFFIX_PARSER` regex string to be able to parse UUIDs to accommodate `external_user_ids`.
+* Add a `get_lti_1p1_user_from_user_id` method to the `LtiConsumerXBlock` to get the user object associated with a user
+  ID.
+
 7.0.1 - 2022-11-29
 ------------------
 

lti_consumer/__init__.py

@@ -4,4 +4,4 @@
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '7.0.1'
+__version__ = '7.0.2'

lti_consumer/lti_xblock.py

@@ -93,7 +93,7 @@
     "/projects/open-edx-building-and-running-a-course/en/latest/exercises_tools/lti_component.html"
     "'>"
 )
-RESULT_SERVICE_SUFFIX_PARSER = re.compile(r"^user/(?P<anon_id>\w+)", re.UNICODE)
+RESULT_SERVICE_SUFFIX_PARSER = re.compile(r"^user/(?P<anon_id>[\w-]+)", re.UNICODE)
 LTI_1P1_ROLE_MAP = {
     'student': 'Student,Learner',
     'staff': 'Administrator',
@@ -835,7 +835,7 @@ def external_user_id(self):
 
     def get_lti_1p1_user_id(self):
         """
-        Returns the user ID to send to an LTI tool during an LTI 1.1 launch. If the
+        Returns the user ID to send to an LTI tool during an LTI 1.1/2.0 launch. If the
         enable_external_user_id_1p1_launches CourseWaffleFlag is enabled for the course, returns the external_user_id
         defined by the external_user_ids Djangoapp. Otherwise, returns the anonymous_user_id.
 
@@ -849,6 +849,23 @@ def get_lti_1p1_user_id(self):
 
         return self.anonymous_user_id
 
+    def get_lti_1p1_user_from_user_id(self, user_id):
+        """
+        Returns the user object associated with a user_id. This is used in LTI 1.1/2.0 integrations for calls to the
+        LTI 1.1 Basic Outcomes service and the LTI 2.0 Results service. Tool Providers may make calls to this library's
+        endpoints with a user identifier. This function returns a user object associated with that user identifier.
+
+        The user identifier may be a course-anonymized user ID (i.e. the anonymous_user_id) or the global, consistent
+        user ID (i.e. the external_user_id). This functions returns the correct User object.
+        """
+        if external_user_id_1p1_launches_enabled(self.scope_ids.usage_id.context_key):
+            try:
+                return compat.get_user_from_external_user_id(user_id)
+            except LtiError:
+                return None
+        else:
+            return self.runtime.service(self, 'user').get_user_by_anonymous_id(user_id)
+
     @property
     def resource_link_id(self):
         """
@@ -1293,7 +1310,7 @@ def result_service_handler(self, request, suffix=''):
         except LtiError:
             return Response(status=401)  # Unauthorized in this case.  401 is right
 
-        user = self.runtime.service(self, 'user').get_user_by_anonymous_id(anon_id)
+        user = self.get_lti_1p1_user_from_user_id(anon_id)
         if not user:  # that means we can't save to database, as we do not have real user id.
             msg = _("[LTI]: Real user not found against anon_id: {}").format(anon_id)
             log.info(msg)

lti_consumer/outcomes.py

@@ -183,8 +183,9 @@ def handle_request(self, request):
             log.debug("[LTI]: %s", error_message)
             return response_xml_template.format(**failure_values)
 
-        anon_id = unquote(sourced_id.split(':')[-1])
-        real_user = self.xblock.runtime.service(self, 'user').get_user_by_anonymous_id(anon_id)
+        user_id = unquote(sourced_id.split(':')[-1])
+        real_user = self.xblock.get_lti_1p1_user_from_user_id(user_id)
+
         if not real_user:  # that means we can't save to database, as we do not have real user id.
             failure_values['imsx_messageIdentifier'] = escape(imsx_message_identifier)
             failure_values['imsx_description'] = "User not found."

lti_consumer/tests/unit/test_lti_xblock.py

@@ -671,8 +671,8 @@ def test_unauthenticated_user(self):
 
 
 @ddt.ddt
-class TestGetLti1p1UserId(TestLtiConsumerXBlock):
-    """ Unit tests for the get_lti_1p1_user_id method"""
+class TestLti1p1UserId(TestLtiConsumerXBlock):
+    """ Unit tests for the get_lti_1p1_user_id and get_lti_1p1_user_from_user_id methods"""
     def setUp(self):
         super().setUp()
 
@@ -698,6 +698,40 @@ def test_external_user_id_flag_enabled(self, external_user_id_1p1_launches_enabl
             external_user_id_1p1_launches_enabled.return_value = external_user_id_1p1_launches_enabled_value
             self.assertEqual(self.xblock.get_lti_1p1_user_id(), expected_value)
 
+    @patch('lti_consumer.lti_xblock.compat')
+    @ddt.data(True, False)
+    def test_get_lti_1p1_user_from_user_id(
+            self,
+            external_user_id_1p1_launches_enabled,
+            compat_mock):
+
+        # Set the mock user objects for user objects associated with an anonymous_user_id and an external_user_id.
+        mock_anonymous_user = Mock()
+        mock_external_user = Mock()
+
+        self.xblock.runtime.service(self, 'user').get_user_by_anonymous_id = Mock(return_value=mock_anonymous_user)
+        compat_mock.get_user_from_external_user_id.return_value = mock_external_user
+
+        with patch('lti_consumer.lti_xblock.external_user_id_1p1_launches_enabled') as \
+                mock_external_user_id_1p1_launches_enabled:
+            mock_external_user_id_1p1_launches_enabled.return_value = external_user_id_1p1_launches_enabled
+
+            user = self.xblock.get_lti_1p1_user_from_user_id('user_id')
+
+            if external_user_id_1p1_launches_enabled:
+                self.assertEqual(user, mock_external_user)
+            else:
+                self.assertEqual(user, mock_anonymous_user)
+
+    @patch('lti_consumer.lti_xblock.external_user_id_1p1_launches_enabled')
+    @patch('lti_consumer.lti_xblock.compat')
+    def test_get_lti_1p1_user_from_user_id_lti_error(self, compat_mock, mock_external_user_id_1p1_launches_enabled):
+        mock_external_user_id_1p1_launches_enabled.return_value = True
+        compat_mock.get_user_from_external_user_id.side_effect = LtiError()
+
+        user = self.xblock.get_lti_1p1_user_from_user_id('user_id')
+        self.assertEqual(user, None)
+
 
 class TestStudentView(TestLtiConsumerXBlock):
     """
@@ -917,11 +951,15 @@ def setUp(self):
         super().setUp()
         self.lti_provider_key = 'test'
         self.lti_provider_secret = 'secret'
-        self.xblock.runtime.get_real_user = Mock()
         self.xblock.accept_grades_past_due = True
         self.mock_lti_consumer = Mock()
         self.xblock._get_lti_consumer = Mock(return_value=self.mock_lti_consumer)  # pylint: disable=protected-access
 
+        mock_user = Mock()
+        mock_id = PropertyMock(return_value=1)
+        type(mock_user).id = mock_id
+        self.xblock.get_lti_1p1_user_from_user_id = Mock(return_value=mock_user)
+
     @override_settings(DEBUG=True)
     @patch('lti_consumer.lti_xblock.log_authorization_header')
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret')
@@ -998,11 +1036,8 @@ def test_bad_user_id(self, mock_parse_suffix):
         Test 404 response returned when a user cannot be found
         """
         mock_parse_suffix.return_value = FAKE_USER_ID
-        self.xblock.runtime.service = Mock(
-            return_value=Mock(
-                get_user_by_anonymous_id=Mock(return_value=None)
-            )
-        )
+        self.xblock.get_lti_1p1_user_from_user_id.return_value = None
+
         response = self.xblock.result_service_handler(make_request('', 'GET'))
 
         self.assertEqual(response.status_code, 404)
@@ -1327,6 +1362,15 @@ def test_suffix_match(self):
         parsed = parse_handler_suffix(f"user/{FAKE_USER_ID}")
         self.assertEqual(parsed, FAKE_USER_ID)
 
+    def test_suffix_match_uuid(self):
+        """
+        Test `parse_handler_suffix` when `suffix` is a UUID. Note that we may send UUIDs as user IDs when the
+        lti_consumer.enable_external_user_id_1p1_launches CourseWaffleFlag is enabled, so we must be able to parse
+        UUID user IDs.
+        """
+        parsed = parse_handler_suffix("user/2e9ec4fa-e1cc-4591-9f19-cf1e94454c21")
+        self.assertEqual(parsed, "2e9ec4fa-e1cc-4591-9f19-cf1e94454c21")
+
 
 @ddt.ddt
 class TestGetContext(TestLtiConsumerXBlock):

lti_consumer/tests/unit/test_outcomes.py

@@ -5,13 +5,14 @@
 import textwrap
 import unittest
 from copy import copy
-
 from unittest.mock import Mock, PropertyMock, patch
 
+import ddt
+
 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.test_utils import make_request
+from lti_consumer.tests.unit.test_lti_xblock import TestLtiConsumerXBlock
 
 REQUEST_BODY_TEMPLATE_VALID = textwrap.dedent("""
     <?xml version="1.0" encoding="UTF-8"?>
@@ -326,14 +327,25 @@ def test_string_with_unicode_chars(self):
         self.assertEqual(action, 'ţéšţ_action')
 
 
+@ddt.ddt
 class TestOutcomeService(TestLtiConsumerXBlock):
     """
     Unit tests for OutcomeService
     """
 
     def setUp(self):
         super().setUp()
-        self.outcome_servce = OutcomeService(self.xblock)
+        self.outcome_service = OutcomeService(self.xblock)
+
+        # Set up user mock for LtiConsumerXBlock.get_lti_1p1_user_from_user_id method.
+        self.mock_get_user_id_patcher = patch('lti_consumer.lti_xblock.LtiConsumerXBlock.get_lti_1p1_user_from_user_id')
+        self.addCleanup(self.mock_get_user_id_patcher.stop)
+        self.mock_get_user_id_patcher_enabled = self.mock_get_user_id_patcher.start()
+
+        mock_user = Mock()
+        mock_id = PropertyMock(return_value=1)
+        type(mock_user).id = mock_id
+        self.mock_get_user_id_patcher_enabled.return_value = mock_user
 
     @patch('lti_consumer.outcomes.verify_oauth_body_signature', Mock(return_value=True))
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's')))
@@ -343,6 +355,7 @@ def test_handle_replace_result_success(self):
         Test replace result request returns with success indicator
         """
         request = make_request('')
+
         values = {
             'code': 'success',
             'description': 'Score for  is now 0.5',
@@ -351,7 +364,7 @@ def test_handle_replace_result_success(self):
         }
 
         self.assertEqual(
-            self.outcome_servce.handle_request(request).strip(),
+            self.outcome_service.handle_request(request).strip(),
             RESPONSE_BODY_TEMPLATE.format(**values).strip()
         )
 
@@ -362,7 +375,7 @@ def test_grade_past_due(self):
         """
         request = make_request('')
         self.xblock.accept_grades_past_due = False
-        response = self.outcome_servce.handle_request(request)
+        response = self.outcome_service.handle_request(request)
 
         self.assertIn('failure', response)
         self.assertIn('Grade is past due', response)
@@ -376,7 +389,7 @@ def test_lti_error_not_raises_type_error(self, mock_parse):
         request = make_request('test_string')
 
         mock_parse.side_effect = LtiError
-        response = self.outcome_servce.handle_request(request)
+        response = self.outcome_service.handle_request(request)
         self.assertNotIn('TypeError', response)
         self.assertNotIn('a bytes-like object is required', response)
         self.assertIn('Request body XML parsing error', response)
@@ -389,7 +402,7 @@ def test_xml_parse_lti_error(self, mock_parse):
         request = make_request('')
 
         mock_parse.side_effect = LtiError
-        response = self.outcome_servce.handle_request(request)
+        response = self.outcome_service.handle_request(request)
         self.assertIn('failure', response)
         self.assertIn('Request body XML parsing error', response)
 
@@ -403,10 +416,10 @@ def test_invalid_signature(self, mock_verify):
         request = make_request('')
 
         mock_verify.side_effect = ValueError
-        self.assertIn('failure', self.outcome_servce.handle_request(request))
+        self.assertIn('failure', self.outcome_service.handle_request(request))
 
         mock_verify.side_effect = LtiError
-        self.assertIn('failure', self.outcome_servce.handle_request(request))
+        self.assertIn('failure', self.outcome_service.handle_request(request))
 
     @patch('lti_consumer.outcomes.verify_oauth_body_signature', Mock(return_value=True))
     @patch('lti_consumer.lti_xblock.LtiConsumerXBlock.lti_provider_key_secret', PropertyMock(return_value=('t', 's')))
@@ -416,12 +429,9 @@ def test_user_not_found(self):
         Test user not found returns failure response
         """
         request = make_request('')
-        self.xblock.runtime.service = Mock(
-            return_value=Mock(
-                get_user_by_anonymous_id=Mock(return_value=None)
-            )
-        )
-        response = self.outcome_servce.handle_request(request)
+
+        self.mock_get_user_id_patcher_enabled.return_value = None
+        response = self.outcome_service.handle_request(request)
 
         self.assertIn('failure', response)
         self.assertIn('User not found', response)
@@ -434,7 +444,7 @@ def test_unsupported_action(self):
         Test unsupported action returns unsupported response
         """
         request = make_request('')
-        response = self.outcome_servce.handle_request(request)
+        response = self.outcome_service.handle_request(request)
 
         self.assertIn('unsupported', response)
         self.assertIn('Target does not support the requested operation.', response)