From c14aa1c4bf2009302d81c1a08254f6940fc34dc6 Mon Sep 17 00:00:00 2001
From: Andy Shultz <ashultz@edx.org>
Date: Thu, 22 Sep 2022 12:03:53 -0400
Subject: [PATCH] feat: only use anonymous user if there is no other choice
Sometimes we need to load the block. Current code always rebinds the
block to the anonymous user because we might not have a user.
But in many cases we do have a user and may have already loaded and
bound the block in question. Check for user and request and if the
block is already bound and just use that block if possible or at least
load the block with the user you actually have.
---
lti_consumer/models.py | 2 +-
lti_consumer/plugin/compat.py | 42 ++++++++++++++++---
lti_consumer/signals.py | 2 +-
lti_consumer/tests/unit/plugin/test_views.py | 2 +-
.../tests/unit/plugin/test_views_lti_ags.py | 16 +++----
.../plugin/test_views_lti_deep_linking.py | 2 +-
lti_consumer/tests/unit/test_lti_xblock.py | 4 +-
lti_consumer/tests/unit/test_models.py | 4 +-
8 files changed, 52 insertions(+), 22 deletions(-)
diff --git a/lti_consumer/models.py b/lti_consumer/models.py
index e8736d2..acf8eb2 100644
--- a/lti_consumer/models.py
+++ b/lti_consumer/models.py
@@ -264,7 +264,7 @@ class LtiConfiguration(models.Model):
if block is None:
if self.location is None:
raise ValueError(_("Block location not set, it's not possible to retrieve the block."))
- block = self._block = compat.load_block_as_anonymous_user(self.location)
+ block = self._block = compat.load_block_as_user(self.location)
return block
@block.setter
diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py
index e7a1b15..7335627 100644
--- a/lti_consumer/plugin/compat.py
+++ b/lti_consumer/plugin/compat.py
@@ -78,9 +78,43 @@ def run_xblock_handler_noauth(*args, **kwargs):
return handle_xblock_callback_noauth(*args, **kwargs)
-def load_block_as_anonymous_user(location):
+def load_block_as_user(location):
"""
- Load a block as anonymous user.
+ Load a block as the current user, or load as the anonymous user if no user is available.
+ """
+ # pylint: disable=import-error,import-outside-toplevel
+ from crum import get_current_user, get_current_request
+ from xmodule.modulestore.django import modulestore
+ from lms.djangoapps.courseware.module_render import get_module_for_descriptor_internal
+ from openedx.core.lib.xblock_utils import request_token
+
+ # Retrieve descriptor from modulestore
+ descriptor = modulestore().get_item(location)
+ user = get_current_user()
+ request = get_current_request()
+ if user and request:
+ # If we're in request scope, the descriptor may already be a block bound to a user
+ # and we don't need to do any more loading
+ if descriptor.scope_ids.user_id is not None and user.id == descriptor.scope_ids.user_id:
+ return descriptor
+
+ # If not load this block to bind it onto the user
+ get_module_for_descriptor_internal(
+ user=user,
+ descriptor=descriptor,
+ student_data=None,
+ course_id=location.course_key,
+ track_function=None,
+ request_token=request_token(request),
+ )
+ return descriptor
+ else:
+ return _load_block_as_anonymous_user(location, descriptor)
+
+
+def _load_block_as_anonymous_user(location, descriptor):
+ """
+ Load a block as the anonymous user because no user is available.
This uses a few internal courseware methods to retrieve the descriptor
and bind an AnonymousUser to it, in a similar fashion as a `noauth` XBlock
@@ -89,12 +123,8 @@ def load_block_as_anonymous_user(location):
# pylint: disable=import-error,import-outside-toplevel
from crum import impersonate
from django.contrib.auth.models import AnonymousUser
- from xmodule.modulestore.django import modulestore
from lms.djangoapps.courseware.module_render import get_module_for_descriptor_internal
- # Retrieve descriptor from modulestore
- descriptor = modulestore().get_item(location)
-
# ensure `crum.get_current_user` returns AnonymousUser. It returns None when outside
# of request scope which causes error during block loading.
user = AnonymousUser()
diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py
index b239349..e9d928e 100644
--- a/lti_consumer/signals.py
+++ b/lti_consumer/signals.py
@@ -31,7 +31,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl
and instance.score_given:
try:
# Load block using LMS APIs and check if the block is graded and still accept grades.
- block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id)
+ block = compat.load_block_as_user(instance.line_item.resource_link_id)
if block.has_score and (not block.is_past_due() or block.accept_grades_past_due):
# Map external ID to platform user
user = compat.get_user_from_external_user_id(instance.user_id)
diff --git a/lti_consumer/tests/unit/plugin/test_views.py b/lti_consumer/tests/unit/plugin/test_views.py
index 17db1dc..4d7e9e9 100644
--- a/lti_consumer/tests/unit/plugin/test_views.py
+++ b/lti_consumer/tests/unit/plugin/test_views.py
@@ -144,7 +144,7 @@ class TestLti1p3LaunchGateEndpoint(TestCase):
self.compat.get_external_id_for_user.return_value = "12345"
model_compat_patcher = patch("lti_consumer.models.compat")
model_compat = model_compat_patcher.start()
- model_compat.load_block_as_anonymous_user.return_value = self.xblock
+ model_compat.load_block_as_user.return_value = self.xblock
self.addCleanup(model_compat_patcher.stop)
def test_invalid_lti_version(self):
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 7a3e7f9..856419b 100644
--- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py
+++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py
@@ -71,7 +71,7 @@ class LtiAgsLineItemViewSetTestCase(APITransactionTestCase):
self.addCleanup(compat_mock.stop)
self._compat_mock = compat_mock.start()
self._compat_mock.get_user_from_external_user_id.return_value = self._mock_user
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
def _set_lti_token(self, scopes=None):
"""
@@ -431,7 +431,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
"gradingProgress": grading_progress,
})
- self._compat_mock.load_block_as_anonymous_user.assert_not_called()
+ self._compat_mock.load_block_as_user.assert_not_called()
self._compat_mock.get_user_from_external_user_id.assert_not_called()
def test_xblock_grade_publish_on_score_save(self):
@@ -439,7 +439,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
Test that the grade is submitted when gradingProgress is `FullyGraded`.
"""
# Set up LMS mocks
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
self.xblock.set_user_module_score = Mock()
@@ -452,7 +452,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
# Check if publish grade was called
self.xblock.set_user_module_score.assert_called_once()
self._compat_mock.get_user_from_external_user_id.assert_called_once()
- self._compat_mock.load_block_as_anonymous_user.assert_called_once()
+ self._compat_mock.load_block_as_user.assert_called_once()
call_args = self.xblock.set_user_module_score.call_args.args
self.assertEqual(call_args, ('user_mock', 0.83, 1, 'This is exceptional work.'))
@@ -462,7 +462,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
Test when given score is bigger than maximum score.
"""
# Return block bypassing LMS API
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
self.xblock.set_user_module_score = Mock()
@@ -494,7 +494,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
self.xblock.runtime.publish.side_effect = LmsException
# Return block bypassing LMS API
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
# Check that the except statement catches the exception
with self.assertRaises(LmsException):
@@ -511,7 +511,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
self._post_lti_score()
# Check that the block wasn't set if due date is past
- self._compat_mock.load_block_as_anonymous_user.assert_called_once()
+ self._compat_mock.load_block_as_user.assert_called_once()
self._compat_mock.get_user_from_external_user_id.assert_not_called()
self.xblock.set_user_module_score.assert_not_called()
@@ -521,7 +521,7 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
Test grade publish after due date when accept_grades_past_due is True. Grade should publish.
"""
# Return block bypassing LMS API
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
self.xblock.set_user_module_score = Mock()
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 b19794d..445730f 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
@@ -80,7 +80,7 @@ class LtiDeepLinkingTestCase(APITransactionTestCase):
self.addCleanup(compat_mock.stop)
self._compat_mock = compat_mock.start()
self._compat_mock.get_user_from_external_user_id.return_value = self._mock_user
- self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.load_block_as_user.return_value = self.xblock
@ddt.ddt
diff --git a/lti_consumer/tests/unit/test_lti_xblock.py b/lti_consumer/tests/unit/test_lti_xblock.py
index 7f535df..59c27f4 100644
--- a/lti_consumer/tests/unit/test_lti_xblock.py
+++ b/lti_consumer/tests/unit/test_lti_xblock.py
@@ -1576,7 +1576,7 @@ class TestLti1p3AccessTokenEndpoint(TestLtiConsumerXBlock):
patcher = patch(
'lti_consumer.models.compat',
- **{'load_block_as_anonymous_user.return_value': self.xblock}
+ **{'load_block_as_user.return_value': self.xblock}
)
patcher.start()
self.addCleanup(patcher.stop)
@@ -1751,7 +1751,7 @@ class TestLti1p3AccessTokenJWK(TestCase):
self.request = make_jwt_request(jwt)
patcher = patch(
'lti_consumer.models.compat',
- **{'load_block_as_anonymous_user.return_value': self.xblock}
+ **{'load_block_as_user.return_value': self.xblock}
)
patcher.start()
self.addCleanup(patcher.stop)
diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py
index aa06952..f1562bc 100644
--- a/lti_consumer/tests/unit/test_models.py
+++ b/lti_consumer/tests/unit/test_models.py
@@ -318,7 +318,7 @@ class TestLtiConfigurationModel(TestCase):
"""
Check if a block is properly loaded when calling the `block` property.
"""
- compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ compat_mock.load_block_as_user.return_value = self.xblock
block = self.lti_1p3_config.block
self.assertEqual(block, self.xblock)
@@ -435,7 +435,7 @@ class TestLtiAgsScoreModel(TestCase):
compat_mock = patch("lti_consumer.signals.compat")
self.addCleanup(compat_mock.stop)
self._compat_mock = compat_mock.start()
- self._compat_mock.load_block_as_anonymous_user.return_value = make_xblock(
+ self._compat_mock.load_block_as_user.return_value = make_xblock(
'lti_consumer', LtiConsumerXBlock, {
'due': datetime.now(timezone.utc),
'graceperiod': timedelta(days=2),
--
GitLab