Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: move noauth rebind ModuleSystem argument to service (#30320)

Browse Source 
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.
This commit is contained in:
Arunmozhi
2022-06-09 04:26:59 +10:00
committed by GitHub
parent aedcd7910d
commit ba8e98c710
9 changed files with 171 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
common/lib/xmodule/xmodule/lti_2_util.py
View File

@@ -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',
{

1
common/lib/xmodule/xmodule/lti_module.py
View File

@@ -275,6 +275,7 @@ class LTIFields:
@XBlock.needs("i18n")
@XBlock.needs("mako")
@XBlock.needs("user")
@XBlock.needs("rebind_user")
class LTIBlock(
LTIFields,
LTI20BlockMixin,

104
common/lib/xmodule/xmodule/services.py
View File

@@ -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

16
common/lib/xmodule/xmodule/tests/test_lti20_unit.py
View File

@@ -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

2
common/lib/xmodule/xmodule/tests/test_lti_unit.py
View File

@@ -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,

21
common/lib/xmodule/xmodule/x_module.py
View File

@@ -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)."""

81
lms/djangoapps/courseware/module_render.py
View File

@@ -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,
)

7
lms/djangoapps/courseware/tests/test_module_render.py
View File

@@ -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

17
openedx/core/djangoapps/xblock/runtime/runtime.py
View File

@@ -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)