From 3bbbdd5ebec6cd3b9984e21071f6ed221e0e8b30 Mon Sep 17 00:00:00 2001
From: michaelroytman <mroytman@edx.org>
Date: Tue, 29 Nov 2022 12:57:56 -0500
Subject: [PATCH] feat: fix LTI 1.1 Basic Outcomes Service and LTI 2.0 Rsult
Service to support external user IDs
In #307, we added the ability to send a stable, static user identifier (i.e. external user ID) to fix failed launches with the QwikLabs tool. This is because the QwikLabs tool did not work with the course-anonymized user IDs we used to send (i.e. anonymous user IDs). Inadvertently, this change broke the LTI 1.1 Basic Outcomes Service and the LTI 2.0 Result Service for courses that use the external user ID (i.e. they have the lti_consumer.enable_external_user_id_1p1_launches CourseWaffleFlag enabled). The Basic Outcomes Service and Result Service handle grade pass backs. Because we now have two ways to identify a user in LTI 1.1/2.0, we must update the Basic Outcomes Service and Result Service to support both. This commit fixes this bug.
---
CHANGELOG.rst | 11 +++-
lti_consumer/__init__.py | 2 +-
lti_consumer/lti_xblock.py | 23 +++++++--
lti_consumer/outcomes.py | 5 +-
lti_consumer/tests/unit/test_lti_xblock.py | 60 +++++++++++++++++++---
lti_consumer/tests/unit/test_outcomes.py | 42 +++++++++------
6 files changed, 112 insertions(+), 31 deletions(-)
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 7b70f25..b92ea47 100644
--- a/CHANGELOG.rst
+++ b/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
------------------
diff --git a/lti_consumer/__init__.py b/lti_consumer/__init__.py
index a920083..73e477f 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__ = '7.0.1'
+__version__ = '7.0.2'
diff --git a/lti_consumer/lti_xblock.py b/lti_consumer/lti_xblock.py
index fb3e1d9..39e9dcd 100644
--- a/lti_consumer/lti_xblock.py
+++ b/lti_consumer/lti_xblock.py
@@ -93,7 +93,7 @@ DOCS_ANCHOR_TAG_OPEN = (
"/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 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
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 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
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 @@ class LtiConsumerXBlock(StudioEditableXBlockMixin, XBlock):
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)
diff --git a/lti_consumer/outcomes.py b/lti_consumer/outcomes.py
index b05ae0d..94112ec 100644
--- a/lti_consumer/outcomes.py
+++ b/lti_consumer/outcomes.py
@@ -183,8 +183,9 @@ class OutcomeService:
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."
diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py
index 0b8631c..1068285 100644
--- a/lti_consumer/tests/unit/test_lti_xblock.py
+++ b/lti_consumer/tests/unit/test_lti_xblock.py
@@ -671,8 +671,8 @@ class TestExtractRealUserData(TestLtiConsumerXBlock):
@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 @@ class TestGetLti1p1UserId(TestLtiConsumerXBlock):
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 @@ class TestResultServiceHandler(TestLtiConsumerXBlock):
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 @@ class TestResultServiceHandler(TestLtiConsumerXBlock):
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 @@ class TestParseSuffix(TestLtiConsumerXBlock):
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):
diff --git a/lti_consumer/tests/unit/test_outcomes.py b/lti_consumer/tests/unit/test_outcomes.py
index dcc1e5d..8008f9f 100644
--- a/lti_consumer/tests/unit/test_outcomes.py
+++ b/lti_consumer/tests/unit/test_outcomes.py
@@ -5,13 +5,14 @@ Unit tests for lti_consumer.outcomes module
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,6 +327,7 @@ class TestParseGradeXmlBody(unittest.TestCase):
self.assertEqual(action, 'ţéšţ_action')
+@ddt.ddt
class TestOutcomeService(TestLtiConsumerXBlock):
"""
Unit tests for OutcomeService
@@ -333,7 +335,17 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
}
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
"""
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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 @@ class TestOutcomeService(TestLtiConsumerXBlock):
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)
--
GitLab