From dc661747b20108d0a56264c089650852fbb2987b Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Wed, 27 Apr 2022 13:18:01 +1000 Subject: [PATCH] refactor: move noauth rebind ModuleSystem argument to service 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 recieved by its noauth endpoint to bind the module the real_user. The original function is defined as a local function and calls its parent function recursively to perform rebinding. Due to the heavy dependency of local variables, this new service is defined in the same module to minimize imports. --- 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 3829139b7b6c..93faa807c7b5 100644 --- a/common/lib/xmodule/xmodule/lti_2_util.py +++ b/common/lib/xmodule/xmodule/lti_2_util.py @@ -79,7 +79,7 @@ def lti_2_0_result_rest_handler(self, request, suffix): 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 @@ def _lti_2_0_result_get_handler(self, request, real_user): "@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 @@ def set_user_module_score(self, user, score, max_score, comment="", score_delete 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 583cf54d6590..71be6d6b75bd 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 be5534e1c191..a45ad61a44f7 100644 --- a/common/lib/xmodule/xmodule/services.py +++ b/common/lib/xmodule/xmodule/services.py @@ -4,10 +4,26 @@ 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 @@ def get_teams_configuration(self, course_id): 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 aca04ea95561..253945cb6d30 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 @@ def test_lti20_put_like_delete_success(self): 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 @@ def test_lti20_delete_success(self): 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 @@ def test_lti20_put_set_score_success(self): 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 @@ def test_lti20_request_handler_bad_user(self): 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 cfda18a62fc4..2ca2a6492c19 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 @@ def setUp(self): """) 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 7ec451ce547f..adff5c86351c 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1634,6 +1634,22 @@ def hostname(self): 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 @@ def __init__( course_id=None, error_descriptor_class=None, field_data=None, - rebind_noauth_module_to_user=None, **kwargs, ): """ @@ -1684,9 +1699,6 @@ def __init__( 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 @@ def __init__( 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 634501135885..44b77f3eab8f 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -45,6 +45,7 @@ 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.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 inner_get_module(descriptor): 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 handle_deprecated_progress_event(block, event): 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 rebind_noauth_module_to_user(module, real_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 d9d8aeae0ef9..d01d2e3f4562 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.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 @@ def test_rebind_noauth_module_to_user_not_anonymous(self): 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 @@ def test_rebind_noauth_module_to_user_anonymous(self): 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 ba06df6da54b..d3f98e2cb9b4 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -23,6 +23,7 @@ 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 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.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 @@ def service(self, block, service_name): # 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 @@ def service(self, block, service_name): 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 @@ def service(self, block, service_name): 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)