From ba8e98c7109373583f91aff04567b0dabe3d6ebd Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 9 Jun 2022 04:26:59 +1000 Subject: [PATCH] refactor: move noauth rebind ModuleSystem argument to service (#30320) This removes the `rebind_noauth_module_to_user` argument from the ModuleSystem constructor and moves it to a separate service called "rebinder" in the class `RebindModuleService`. This is used in the LTI module to bind calls received by its noauth endpoint to bind the module the real_user. --- common/lib/xmodule/xmodule/lti_2_util.py | 8 +- common/lib/xmodule/xmodule/lti_module.py | 1 + common/lib/xmodule/xmodule/services.py | 104 ++++++++++++++++++ .../xmodule/xmodule/tests/test_lti20_unit.py | 16 +-- .../xmodule/xmodule/tests/test_lti_unit.py | 2 +- common/lib/xmodule/xmodule/x_module.py | 21 +++- lms/djangoapps/courseware/module_render.py | 81 +++----------- .../courseware/tests/test_module_render.py | 7 +- .../core/djangoapps/xblock/runtime/runtime.py | 17 ++- 9 files changed, 171 insertions(+), 86 deletions(-) diff --git a/common/lib/xmodule/xmodule/lti_2_util.py b/common/lib/xmodule/xmodule/lti_2_util.py index 3829139b7b..93faa807c7 100644 --- a/common/lib/xmodule/xmodule/lti_2_util.py +++ b/common/lib/xmodule/xmodule/lti_2_util.py @@ -79,7 +79,7 @@ class LTI20BlockMixin: except LTIError: return Response(status=401) # Unauthorized in this case. 401 is right - real_user = self.system.service(self, 'user').get_user_by_anonymous_id(anon_id) + real_user = self.runtime.service(self, 'user').get_user_by_anonymous_id(anon_id) if not real_user: # that means we can't save to database, as we do not have real user id. msg = f"[LTI]: Real user not found against anon_id: {anon_id}" log.info(msg) @@ -171,7 +171,7 @@ class LTI20BlockMixin: "@context": "http://purl.imsglobal.org/ctx/lis/v2/Result", "@type": "Result" } - self.system.rebind_noauth_module_to_user(self, real_user) + self.runtime.service(self, 'rebind_user').rebind_noauth_module_to_user(self, real_user) if self.module_score is None: # In this case, no score has been ever set return Response(json.dumps(base_json_obj).encode('utf-8'), content_type=LTI_2_0_JSON_CONTENT_TYPE) @@ -254,10 +254,10 @@ class LTI20BlockMixin: else: scaled_score = None - self.system.rebind_noauth_module_to_user(self, user) + self.runtime.service(self, 'rebind_user').rebind_noauth_module_to_user(self, user) # have to publish for the progress page... - self.system.publish( + self.runtime.publish( self, 'grade', { diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index 583cf54d65..71be6d6b75 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -275,6 +275,7 @@ class LTIFields: @XBlock.needs("i18n") @XBlock.needs("mako") @XBlock.needs("user") +@XBlock.needs("rebind_user") class LTIBlock( LTIFields, LTI20BlockMixin, diff --git a/common/lib/xmodule/xmodule/services.py b/common/lib/xmodule/xmodule/services.py index be5534e1c1..a45ad61a44 100644 --- a/common/lib/xmodule/xmodule/services.py +++ b/common/lib/xmodule/xmodule/services.py @@ -4,10 +4,26 @@ Module contains various XModule/XBlock services import inspect +import logging + +from functools import partial from config_models.models import ConfigurationModel from django.conf import settings +from edx_when.field_data import DateLookupFieldData from xmodule.modulestore.django import modulestore +from xblock.reference.plugins import Service +from xblock.runtime import KvsFieldData + + +from lms.djangoapps.courseware.field_overrides import OverrideFieldData +from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache +from lms.djangoapps.lms_xblock.field_data import LmsFieldData +from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig +from openedx.core.lib.courses import get_course_by_id + + +log = logging.getLogger(__name__) class SettingsService: @@ -119,3 +135,91 @@ class TeamsConfigurationService: if not self._course: self._course = self.get_course(course_id) return self._course.teams_configuration + + +class RebindUserServiceError(Exception): + pass + + +class RebindUserService(Service): + """ + An XBlock Service that allows modules to get rebound to real users if it was previously bound to an AnonymousUser. + + This used to be a local function inside the `lms.djangoapps.courseware.module_render.get_module_system_for_user` + method, and was passed as a constructor argument to x_module.ModuleSystem. This has been refactored out into a + service to simplify the ModuleSystem and lives in this module temporarily. + + TODO: Only the old LTI XBlock uses it in 2 places for LTI 2.0 integration. As the LTI XBlock is deprecated in + favour of the LTI Consumer XBlock, this should be removed when the LTI XBlock is removed. + + Arguments: + user (User) - A Django User object + course_id (str) - Course ID + course (Course) - Course Object + get_module_system_for_user (function) - The helper function that will be called to create a module system + for a specfic user. This is the parent function from which this service was reactored out. + `lms.djangoapps.courseware.module_render.get_module_system_for_user` + kwargs (dict) - all the keyword arguments that need to be passed to the `get_module_system_for_user` + function when it is called during rebinding + """ + def __init__(self, user, course_id, get_module_system_for_user, **kwargs): + super().__init__(**kwargs) + self.user = user + self.course_id = course_id + self._ref = { + "get_module_system_for_user": get_module_system_for_user + } + self._kwargs = kwargs + + def rebind_noauth_module_to_user(self, block, real_user): + """ + Function that rebinds the module to the real_user. + + Will only work within a module bound to an AnonymousUser, e.g. one that's instantiated by the noauth_handler. + + Arguments: + block (any xblock type): the module to rebind + real_user (django.contrib.auth.models.User): the user to bind to + + Returns: + nothing (but the side effect is that module is re-bound to real_user) + """ + if self.user.is_authenticated: + err_msg = "rebind_noauth_module_to_user can only be called from a module bound to an anonymous user" + log.error(err_msg) + raise RebindUserServiceError(err_msg) + + field_data_cache_real_user = FieldDataCache.cache_for_descriptor_descendents( + self.course_id, + real_user, + block, + asides=XBlockAsidesConfig.possible_asides(), + ) + student_data_real_user = KvsFieldData(DjangoKeyValueStore(field_data_cache_real_user)) + + with modulestore().bulk_operations(self.course_id): + course = modulestore().get_course(course_key=self.course_id) + + (inner_system, inner_student_data) = self._ref["get_module_system_for_user"]( + user=real_user, + student_data=student_data_real_user, # These have implicit user bindings, rest of args considered not to + descriptor=block, + course_id=self.course_id, + course=course, + **self._kwargs + ) + + block.bind_for_student( + inner_system, + real_user.id, + [ + partial(DateLookupFieldData, course_id=self.course_id, user=self.user), + partial(OverrideFieldData.wrap, real_user, course), + partial(LmsFieldData, student_data=inner_student_data), + ], + ) + + block.scope_ids = block.scope_ids._replace(user_id=real_user.id) + # now bind the module to the new ModuleSystem instance and vice-versa + block.runtime = inner_system + inner_system.xmodule_instance = block diff --git a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py index aca04ea955..253945cb6d 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti20_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti20_unit.py @@ -24,12 +24,12 @@ class LTI20RESTResultServiceTest(unittest.TestCase): def setUp(self): super().setUp() - self.system = get_test_system(user=self.USER_STANDIN) + self.runtime = get_test_system(user=self.USER_STANDIN) self.environ = {'wsgi.url_scheme': 'http', 'REQUEST_METHOD': 'POST'} - self.system.publish = Mock() - self.system.rebind_noauth_module_to_user = Mock() + self.runtime.publish = Mock() + self.runtime._services['rebind_user'] = Mock() # pylint: disable=protected-access - self.xmodule = LTIBlock(self.system, DictFieldData({}), Mock()) + self.xmodule = LTIBlock(self.runtime, DictFieldData({}), Mock()) self.lti_id = self.xmodule.lti_id self.xmodule.due = None self.xmodule.graceperiod = None @@ -250,7 +250,7 @@ class LTI20RESTResultServiceTest(unittest.TestCase): assert response.status_code == 200 assert self.xmodule.module_score is None assert self.xmodule.score_comment == '' - (_, evt_type, called_grade_obj), _ = self.system.publish.call_args # pylint: disable=unpacking-non-sequence + (_, evt_type, called_grade_obj), _ = self.runtime.publish.call_args # pylint: disable=unpacking-non-sequence assert called_grade_obj ==\ {'user_id': self.USER_STANDIN.id, 'value': None, 'max_value': None, 'score_deleted': True} assert evt_type == 'grade' @@ -271,7 +271,7 @@ class LTI20RESTResultServiceTest(unittest.TestCase): assert response.status_code == 200 assert self.xmodule.module_score is None assert self.xmodule.score_comment == '' - (_, evt_type, called_grade_obj), _ = self.system.publish.call_args # pylint: disable=unpacking-non-sequence + (_, evt_type, called_grade_obj), _ = self.runtime.publish.call_args # pylint: disable=unpacking-non-sequence assert called_grade_obj ==\ {'user_id': self.USER_STANDIN.id, 'value': None, 'max_value': None, 'score_deleted': True} assert evt_type == 'grade' @@ -288,7 +288,7 @@ class LTI20RESTResultServiceTest(unittest.TestCase): assert response.status_code == 200 assert self.xmodule.module_score == 0.1 assert self.xmodule.score_comment == 'ಠ益ಠ' - (_, evt_type, called_grade_obj), _ = self.system.publish.call_args # pylint: disable=unpacking-non-sequence + (_, evt_type, called_grade_obj), _ = self.runtime.publish.call_args # pylint: disable=unpacking-non-sequence assert evt_type == 'grade' assert called_grade_obj ==\ {'user_id': self.USER_STANDIN.id, 'value': 0.1, 'max_value': 1.0, 'score_deleted': False} @@ -370,7 +370,7 @@ class LTI20RESTResultServiceTest(unittest.TestCase): Test that we get a 404 when the supplied user does not exist """ self.setup_system_xmodule_mocks_for_lti20_request_test() - self.system._services['user'] = StubUserService(user=None) # pylint: disable=protected-access + self.runtime._services['user'] = StubUserService(user=None) # pylint: disable=protected-access mock_request = self.get_signed_lti20_mock_request(self.GOOD_JSON_PUT) response = self.xmodule.lti_2_0_result_rest_handler(mock_request, "user/abcd") assert response.status_code == 404 diff --git a/common/lib/xmodule/xmodule/tests/test_lti_unit.py b/common/lib/xmodule/xmodule/tests/test_lti_unit.py index cfda18a62f..2ca2a6492c 100644 --- a/common/lib/xmodule/xmodule/tests/test_lti_unit.py +++ b/common/lib/xmodule/xmodule/tests/test_lti_unit.py @@ -63,7 +63,7 @@ class LTIBlockTest(TestCase): """) self.system = get_test_system() self.system.publish = Mock() - self.system.rebind_noauth_module_to_user = Mock() + self.system._services['rebind_user'] = Mock() # pylint: disable=protected-access self.xmodule = LTIBlock( self.system, diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 7ec451ce54..adff5c8635 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1634,6 +1634,22 @@ class ModuleSystemShim: from django.conf import settings return settings.LMS_BASE + @property + def rebind_noauth_module_to_user(self): + """ + A function that was used to bind modules initialized by AnonymousUsers to real users. Mainly used + by the LTI Module to connect the right users with the requests from LTI tools. + + Deprecated in favour of the "rebind_user" service. + """ + warnings.warn( + "rebind_noauth_module_to_user is deprecated. Please use the 'rebind_user' service instead.", + DeprecationWarning, stacklevel=3 + ) + rebind_user_service = self._services.get('rebind_user') + if rebind_user_service: + return partial(rebind_user_service.rebind_noauth_module_to_user) + class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime): """ @@ -1658,7 +1674,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, course_id=None, error_descriptor_class=None, field_data=None, - rebind_noauth_module_to_user=None, **kwargs, ): """ @@ -1684,9 +1699,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, error_descriptor_class - The class to use to render XModules with errors field_data - the `FieldData` to use for backing XBlock storage. - - rebind_noauth_module_to_user - rebinds module bound to AnonymousUser to a real user...used in LTI - modules, which have an anonymous handler, to set legitimate users' data """ # Usage_store is unused, and field_data is often supplanted with an @@ -1706,7 +1718,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, self.xmodule_instance = None self.descriptor_runtime = descriptor_runtime - self.rebind_noauth_module_to_user = rebind_noauth_module_to_user def get(self, attr): """ provide uniform access to attributes (like etree).""" diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 6345011358..44b77f3eab 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -45,6 +45,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.util.sandboxing import SandboxService +from xmodule.services import RebindUserService from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from common.djangoapps.xblock_django.constants import ATTR_KEY_USER_ID @@ -63,7 +64,6 @@ from lms.djangoapps.courseware.services import UserStateService from lms.djangoapps.grades.api import GradesUtilService from lms.djangoapps.grades.api import signals as grades_signals from lms.djangoapps.lms_xblock.field_data import LmsFieldData -from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from lms.djangoapps.verify_student.services import XBlockVerificationService from openedx.core.djangoapps.bookmarks.services import BookmarksService @@ -483,12 +483,12 @@ def get_module_system_for_user( student_data=student_data, course_id=course_id, track_function=track_function, + request_token=request_token, position=position, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, static_asset_path=static_asset_path, user_location=user_location, - request_token=request_token, course=course, will_recheck_access=will_recheck_access, ) @@ -608,65 +608,20 @@ def get_module_system_for_user( completion=1.0, ) - def rebind_noauth_module_to_user(module, real_user): - """ - A function that allows a module to get re-bound to a real user if it was previously bound to an AnonymousUser. - - Will only work within a module bound to an AnonymousUser, e.g. one that's instantiated by the noauth_handler. - - Arguments: - module (any xblock type): the module to rebind - real_user (django.contrib.auth.models.User): the user to bind to - - Returns: - nothing (but the side effect is that module is re-bound to real_user) - """ - if user.is_authenticated: - err_msg = ("rebind_noauth_module_to_user can only be called from a module bound to " - "an anonymous user") - log.error(err_msg) - raise LmsModuleRenderError(err_msg) - - field_data_cache_real_user = FieldDataCache.cache_for_descriptor_descendents( - course_id, - real_user, - module, - asides=XBlockAsidesConfig.possible_asides(), - ) - student_data_real_user = KvsFieldData(DjangoKeyValueStore(field_data_cache_real_user)) - - (inner_system, inner_student_data) = get_module_system_for_user( - user=real_user, - student_data=student_data_real_user, # These have implicit user bindings, rest of args considered not to - descriptor=module, - course_id=course_id, - track_function=track_function, - position=position, - wrap_xmodule_display=wrap_xmodule_display, - grade_bucket_type=grade_bucket_type, - static_asset_path=static_asset_path, - user_location=user_location, - request_token=request_token, - course=course, - will_recheck_access=will_recheck_access, - ) - - module.bind_for_student( - inner_system, - real_user.id, - [ - partial(DateLookupFieldData, course_id=course_id, user=user), - partial(OverrideFieldData.wrap, real_user, course), - partial(LmsFieldData, student_data=inner_student_data), - ], - ) - - module.scope_ids = ( - module.scope_ids._replace(user_id=real_user.id) - ) - # now bind the module to the new ModuleSystem instance and vice-versa - module.runtime = inner_system - inner_system.xmodule_instance = module + # Rebind module service to deal with noauth modules getting attached to users + rebind_user_service = RebindUserService( + user, + course_id, + get_module_system_for_user, + track_function=track_function, + position=position, + wrap_xmodule_display=wrap_xmodule_display, + grade_bucket_type=grade_bucket_type, + static_asset_path=static_asset_path, + user_location=user_location, + request_token=request_token, + will_recheck_access=will_recheck_access, + ) # Build a list of wrapping functions that will be applied in order # to the Fragment content coming out of the xblocks that are about to be rendered. @@ -751,10 +706,10 @@ def get_module_system_for_user( 'cache': CacheService(cache), 'sandbox': SandboxService(contentstore=contentstore, course_id=course_id), 'xqueue': xqueue_service, - 'replace_urls': replace_url_service + 'replace_urls': replace_url_service, + 'rebind_user': rebind_user_service, }, descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access - rebind_noauth_module_to_user=rebind_noauth_module_to_user, request_token=request_token, ) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index d9d8aeae0e..d01d2e3f45 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -55,6 +55,7 @@ from xmodule.modulestore.tests.django_utils import ( ) from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, ToyCourseFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.test_asides import AsideTestType # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.services import RebindUserServiceError from xmodule.video_module import VideoBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import STUDENT_VIEW, CombinedSystem # lint-amnesty, pylint: disable=wrong-import-order from common.djangoapps import static_replace @@ -2175,10 +2176,10 @@ class TestRebindModule(TestSubmittingProblems): user2 = UserFactory() user2.id = 2 with self.assertRaisesRegex( - render.LmsModuleRenderError, + RebindUserServiceError, "rebind_noauth_module_to_user can only be called from a module bound to an anonymous user" ): - assert module.system.rebind_noauth_module_to_user(module, user2) + assert module.runtime.service(module, 'rebind_user').rebind_noauth_module_to_user(module, user2) def test_rebind_noauth_module_to_user_anonymous(self): """ @@ -2188,7 +2189,7 @@ class TestRebindModule(TestSubmittingProblems): module = self.get_module_for_user(self.anon_user) user2 = UserFactory() user2.id = 2 - module.system.rebind_noauth_module_to_user(module, user2) + module.runtime.service(module, 'rebind_user').rebind_noauth_module_to_user(module, user2) assert module assert module.system.anonymous_student_id == anonymous_id_for_user(user2, self.course.id) assert module.scope_ids.user_id == user2.id diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index ba06df6da5..d3f98e2cb9 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -23,6 +23,7 @@ from xblock.runtime import KvsFieldData, MemoryIdManager, Runtime from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import ModuleI18nService +from xmodule.services import RebindUserService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace.services import ReplaceURLService @@ -30,6 +31,7 @@ from common.djangoapps.track import contexts as track_contexts from common.djangoapps.track import views as track_views from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache +from lms.djangoapps.courseware import module_render from lms.djangoapps.grades.api import signals as grades_signals from openedx.core.djangoapps.xblock.apps import get_xblock_app_config from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreChildrenData, BlockstoreFieldData @@ -37,7 +39,7 @@ from openedx.core.djangoapps.xblock.runtime.ephemeral_field_data import Ephemera from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin from openedx.core.djangoapps.xblock.utils import get_xblock_id_for_anonymous_user from openedx.core.lib.cache_utils import CacheService -from openedx.core.lib.xblock_utils import wrap_fragment, xblock_local_resource_url +from openedx.core.lib.xblock_utils import wrap_fragment, xblock_local_resource_url, request_token from .id_managers import OpaqueKeyReader from .shims import RuntimeShim, XBlockShim @@ -217,6 +219,7 @@ class XBlockRuntime(RuntimeShim, Runtime): # TODO: Do these declarations actually help with anything? Maybe this check should # be removed from here and from XBlock.runtime declaration = block.service_declaration(service_name) + context_key = block.scope_ids.usage_id.context_key if declaration is None: raise NoSuchServiceError(f"Service {service_name!r} was not requested.") # Most common service is field-data so check that first: @@ -230,7 +233,6 @@ class XBlockRuntime(RuntimeShim, Runtime): raise return self.block_field_datas[block.scope_ids] elif service_name == "completion": - context_key = block.scope_ids.usage_id.context_key return CompletionService(user=self.user, context_key=context_key) elif service_name == "user": return DjangoXBlockUserService( @@ -253,6 +255,17 @@ class XBlockRuntime(RuntimeShim, Runtime): return CacheService(cache) elif service_name == 'replace_urls': return ReplaceURLService(xblock=block, lookup_asset_url=self._lookup_asset_url) + elif service_name == 'rebind_user': + # this service should ideally be initialized with all the arguments of get_module_system_for_user + # but only the positional arguments are passed here as the other arguments are too + # specific to the lms.module_render module + return RebindUserService( + self.user, + context_key, + module_render.get_module_system_for_user, + track_function=make_track_function(), + request_token=request_token(crum.get_current_request()), + ) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name)