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

Merge pull request #32356 from open-craft/agrendalath/fc-0026-user-checks

Browse Source 
feat: remove block-specific handling from runtime role checks [FC-0026]
This commit is contained in:
Piotr Surowiec
2023-06-15 15:32:59 +02:00
committed by GitHub
parent 2f8b7380fb bc17163258
commit e439b3f749
9 changed files with 187 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
common/djangoapps/xblock_django/constants.py
View File

@@ -3,12 +3,26 @@ Constants used by DjangoXBlockUserService
"""
# Optional attributes stored on the XBlockUser
# The anonymous user ID for the user in the course.
ATTR_KEY_ANONYMOUS_USER_ID = 'edx-platform.anonymous_user_id'
# The global (course-agnostic) anonymous user ID for the user.
ATTR_KEY_DEPRECATED_ANONYMOUS_USER_ID = 'edx-platform.deprecated_anonymous_user_id'
# The country code determined from the user's request IP address.
ATTR_KEY_REQUEST_COUNTRY_CODE = 'edx-platform.request_country_code'
# Whether the user is authenticated or anonymous.
ATTR_KEY_IS_AUTHENTICATED = 'edx-platform.is_authenticated'
# The personally identifiable user ID.
ATTR_KEY_USER_ID = 'edx-platform.user_id'
# The username.
ATTR_KEY_USERNAME = 'edx-platform.username'
# Whether the user is enrolled in the course as a Beta Tester.
ATTR_KEY_USER_IS_BETA_TESTER = 'edx-platform.user_is_beta_tester'
# Whether the user has staff access to the platform.
ATTR_KEY_USER_IS_GLOBAL_STAFF = 'edx-platform.user_is_global_staff'
# Whether the user is a course team member with 'Staff' or 'Admin' access.
ATTR_KEY_USER_IS_STAFF = 'edx-platform.user_is_staff'
# A dict containing user's entries from the `UserPreference` model.
ATTR_KEY_USER_PREFERENCES = 'edx-platform.user_preferences'
# The user's role in the course ('staff', 'instructor', or 'student').
ATTR_KEY_USER_ROLE = 'edx-platform.user_role'

39
common/djangoapps/xblock_django/tests/test_user_service.py
View File

@@ -16,6 +16,8 @@ from common.djangoapps.xblock_django.user_service import (
ATTR_KEY_ANONYMOUS_USER_ID,
ATTR_KEY_REQUEST_COUNTRY_CODE,
ATTR_KEY_USER_ID,
ATTR_KEY_USER_IS_BETA_TESTER,
ATTR_KEY_USER_IS_GLOBAL_STAFF,
ATTR_KEY_USER_IS_STAFF,
ATTR_KEY_USER_PREFERENCES,
ATTR_KEY_USER_ROLE,
@@ -49,11 +51,15 @@ class UserServiceTestCase(TestCase):
self.assertListEqual(xb_user.emails, [])
def assert_xblock_user_matches_django(
self, xb_user, dj_user,
self,
xb_user,
dj_user,
user_is_staff=False,
user_role=None,
anonymous_user_id=None,
request_country_code=None,
user_is_global_staff=False,
user_is_beta_tester=False,
):
"""
A set of assertions for comparing a XBlockUser to a django User
@@ -63,7 +69,9 @@ class UserServiceTestCase(TestCase):
assert xb_user.full_name == dj_user.profile.name
assert xb_user.opt_attrs[ATTR_KEY_USERNAME] == dj_user.username
assert xb_user.opt_attrs[ATTR_KEY_USER_ID] == dj_user.id
assert xb_user.opt_attrs[ATTR_KEY_USER_IS_BETA_TESTER] == user_is_beta_tester
assert xb_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] == user_is_staff
assert xb_user.opt_attrs[ATTR_KEY_USER_IS_GLOBAL_STAFF] == user_is_global_staff
assert xb_user.opt_attrs[ATTR_KEY_USER_ROLE] == user_role
assert xb_user.opt_attrs[ATTR_KEY_ANONYMOUS_USER_ID] == anonymous_user_id
assert xb_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] == request_country_code
@@ -80,14 +88,24 @@ class UserServiceTestCase(TestCase):
self.assert_is_anon_xb_user(xb_user, request_country_code=country_code)
@ddt.data(
(False, None, None, None),
(True, 'instructor', None, None),
(True, 'staff', None, None),
(False, 'student', 'abcdef0123', None),
(True, 'student', 'abcdef0123', 'uk'),
(False, None, None, None, False, False),
(True, 'instructor', None, None, False, False),
(True, 'staff', None, None, False, False),
(False, 'student', 'abcdef0123', None, False, False),
(True, 'student', 'abcdef0123', 'uk', False, False),
(False, None, None, None, True, False),
(False, None, None, None, False, True),
)
@ddt.unpack
def test_convert_authenticate_user(self, user_is_staff, user_role, anonymous_user_id, request_country_code):
def test_convert_authenticate_user(
self,
user_is_staff,
user_role,
anonymous_user_id,
request_country_code,
user_is_global_staff,
user_is_beta_tester,
):
"""
Tests for convert_django_user_to_xblock_user behavior when django user is User.
"""
@@ -97,15 +115,20 @@ class UserServiceTestCase(TestCase):
user_role=user_role,
anonymous_user_id=anonymous_user_id,
request_country_code=request_country_code,
user_is_global_staff=user_is_global_staff,
user_is_beta_tester=user_is_beta_tester,
)
xb_user = django_user_service.get_current_user()
assert xb_user.is_current_user
self.assert_xblock_user_matches_django(
xb_user, self.user,
xb_user,
self.user,
user_is_staff,
user_role,
anonymous_user_id,
request_country_code,
user_is_global_staff,
user_is_beta_tester,
)
def test_get_anonymous_user_id_returns_none_for_non_staff_users(self):

11
common/djangoapps/xblock_django/user_service.py
View File

@@ -18,6 +18,8 @@ from .constants import (
ATTR_KEY_REQUEST_COUNTRY_CODE,
ATTR_KEY_USER_ID,
ATTR_KEY_USERNAME,
ATTR_KEY_USER_IS_BETA_TESTER,
ATTR_KEY_USER_IS_GLOBAL_STAFF,
ATTR_KEY_USER_IS_STAFF,
ATTR_KEY_USER_PREFERENCES,
ATTR_KEY_USER_ROLE,
@@ -36,7 +38,10 @@ class DjangoXBlockUserService(UserService):
Constructs a DjangoXBlockUserService object.
Args:
user_is_staff(bool): optional - whether the user is staff in the course
django_user(User): optional - the user we are binding to the runtime. Is `None` for an anonymous user.
user_is_beta_tester(bool): optional - whether the user is enrolled in the course as a Beta Tester.
user_is_global_staff(bool): optional - whether the user has staff access to the platform.
user_is_staff(bool): optional - whether the user is a course team member with 'Staff' or 'Admin' access.
user_role(str): optional -- user's role in the course ('staff', 'instructor', or 'student')
anonymous_user_id(str): optional - anonymous_user_id for the user in the course
deprecated_anonymous_user_id(str): optional - There are XBlocks (CAPA and HTML) that use the per-student
@@ -46,6 +51,8 @@ class DjangoXBlockUserService(UserService):
"""
super().__init__(**kwargs)
self._django_user = django_user
self._user_is_beta_tester = kwargs.get('user_is_beta_tester', False)
self._user_is_global_staff = kwargs.get('user_is_global_staff', False)
self._user_is_staff = kwargs.get('user_is_staff', False)
self._user_role = kwargs.get('user_role', 'student')
self._anonymous_user_id = kwargs.get('anonymous_user_id', None)
@@ -121,6 +128,8 @@ class DjangoXBlockUserService(UserService):
xblock_user.opt_attrs[ATTR_KEY_REQUEST_COUNTRY_CODE] = self._request_country_code
xblock_user.opt_attrs[ATTR_KEY_USER_ID] = django_user.id
xblock_user.opt_attrs[ATTR_KEY_USERNAME] = django_user.username
xblock_user.opt_attrs[ATTR_KEY_USER_IS_BETA_TESTER] = self._user_is_beta_tester
xblock_user.opt_attrs[ATTR_KEY_USER_IS_GLOBAL_STAFF] = self._user_is_global_staff
xblock_user.opt_attrs[ATTR_KEY_USER_IS_STAFF] = self._user_is_staff
xblock_user.opt_attrs[ATTR_KEY_USER_ROLE] = self._user_role
user_preferences = get_user_preferences(django_user)

66
lms/djangoapps/courseware/block_render.py
View File

@@ -493,32 +493,6 @@ def prepare_runtime_for_user(
will_recheck_access=will_recheck_access,
)
user_is_staff = bool(has_access(user, 'staff', block.location, course_id))
user_service = DjangoXBlockUserService(
user,
user_is_staff=user_is_staff,
user_role=get_user_role(user, course_id),
anonymous_user_id=anonymous_id_for_user(user, course_id),
# See the docstring of `DjangoXBlockUserService`.
deprecated_anonymous_user_id=anonymous_id_for_user(user, None),
request_country_code=user_location,
)
# Rebind module service to deal with noauth modules getting attached to users
rebind_user_service = RebindUserService(
user,
course_id,
prepare_runtime_for_user,
track_function=track_function,
position=position,
wrap_xblock_display=wrap_xblock_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.
block_wrappers = []
@@ -555,6 +529,8 @@ def prepare_runtime_for_user(
block_wrappers.append(partial(course_expiration_wrapper, user))
block_wrappers.append(partial(offer_banner_wrapper, user))
user_is_staff = bool(has_access(user, 'staff', course_id))
if settings.FEATURES.get('DISPLAY_DEBUG_INFO_TO_STAFF'):
if is_masquerading_as_specific_student(user, course_id):
# When masquerading as a specific student, we want to show the debug button
@@ -568,7 +544,7 @@ def prepare_runtime_for_user(
del user.real_user.masquerade_settings
user.real_user.masquerade_settings = masquerade_settings
else:
staff_access = has_access(user, 'staff', block, course_id)
staff_access = user_is_staff
if staff_access:
block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info))
@@ -581,7 +557,17 @@ def prepare_runtime_for_user(
'fs': FSService(),
'field-data': field_data,
'mako': mako_service,
'user': user_service,
'user': DjangoXBlockUserService(
user,
user_is_beta_tester=CourseBetaTesterRole(course_id).has_user(user),
user_is_staff=user_is_staff,
user_is_global_staff=bool(has_access(user, 'staff', 'global')),
user_role=get_user_role(user, course_id),
anonymous_user_id=anonymous_id_for_user(user, course_id),
# See the docstring of `DjangoXBlockUserService`.
deprecated_anonymous_user_id=anonymous_id_for_user(user, None),
request_country_code=user_location,
),
'verification': XBlockVerificationService(),
'proctoring': ProctoringService(),
'milestones': milestones_helpers.get_service(),
@@ -595,10 +581,21 @@ def prepare_runtime_for_user(
'sandbox': SandboxService(contentstore=contentstore, course_id=course_id),
'xqueue': xqueue_service,
'replace_urls': replace_url_service,
'rebind_user': rebind_user_service,
'completion': CompletionService(user=user, context_key=course_id)
if user and user.is_authenticated
else None,
# Rebind module service to deal with noauth modules getting attached to users.
'rebind_user': RebindUserService(
user,
course_id,
prepare_runtime_for_user,
track_function=track_function,
position=position,
wrap_xblock_display=wrap_xblock_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,
),
'completion': CompletionService(user=user, context_key=course_id) if user and user.is_authenticated else None,
'i18n': XBlockI18nService,
'library_tools': LibraryToolsService(store, user_id=user.id if user else None),
'partitions': PartitionService(course_id=course_id, cache=DEFAULT_REQUEST_CACHE.data),
@@ -629,11 +626,6 @@ def prepare_runtime_for_user(
block.runtime.set('position', position)
block.runtime.set('user_is_staff', user_is_staff)
block.runtime.set('user_is_admin', bool(has_access(user, 'staff', 'global')))
block.runtime.set('user_is_beta_tester', CourseBetaTesterRole(course_id).has_user(user))
block.runtime.set('days_early_for_beta', block.days_early_for_beta)
return field_data

92
lms/djangoapps/courseware/tests/test_block_render.py
View File

@@ -61,12 +61,19 @@ from xmodule.video_block import VideoBlock # lint-amnesty, pylint: disable=wron
from xmodule.x_module import STUDENT_VIEW, DescriptorSystem # lint-amnesty, pylint: disable=wrong-import-order
from common.djangoapps import static_replace
from common.djangoapps.course_modes.models import CourseMode # lint-amnesty, pylint: disable=reimported
from common.djangoapps.student.tests.factories import GlobalStaffFactory
from common.djangoapps.student.tests.factories import RequestFactoryNoCsrf
from common.djangoapps.student.tests.factories import UserFactory
from common.djangoapps.student.tests.factories import (
BetaTesterFactory,
GlobalStaffFactory,
InstructorFactory,
RequestFactoryNoCsrf,
StaffFactory,
UserFactory,
)
from common.djangoapps.xblock_django.constants import (
ATTR_KEY_ANONYMOUS_USER_ID,
ATTR_KEY_DEPRECATED_ANONYMOUS_USER_ID,
ATTR_KEY_USER_IS_BETA_TESTER,
ATTR_KEY_USER_IS_GLOBAL_STAFF,
ATTR_KEY_USER_IS_STAFF,
ATTR_KEY_USER_ROLE,
)
@@ -2346,17 +2353,6 @@ class LMSXBlockServiceBindingTest(LMSXBlockServiceMixin):
service = self.block.runtime.service(self.block, expected_service)
assert service is not None
def test_beta_tester_fields_added(self):
"""
Tests that the beta tester fields are set on LMS runtime.
"""
self.block.days_early_for_beta = 5
self._prepare_runtime()
# pylint: disable=no-member
assert not self.block.runtime.user_is_beta_tester
assert self.block.runtime.days_early_for_beta == 5
def test_get_set_tag(self):
"""
Tests the user service interface.
@@ -2684,8 +2680,12 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase):
"""
assert getattr(self.block.runtime, attribute) == expected_value
@patch('lms.djangoapps.courseware.block_render.has_access', Mock(return_value=True, autospec=True))
def test_user_is_staff(self):
@ddt.data((True, 'staff'), (False, 'student'))
@ddt.unpack
def test_user_is_staff(self, is_staff, expected_role):
if is_staff:
self.user = StaffFactory(course_key=self.course.id)
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
@@ -2696,15 +2696,18 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase):
course=self.course,
)
block_user_info = self.block.runtime.service(self.block, "user").get_current_user()
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_IS_STAFF)
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_ROLE) == 'student'
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_IS_STAFF) == is_staff
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_ROLE) == expected_role
with warnings.catch_warnings(): # For now, also test the deprecated accessors for backwards compatibility:
warnings.simplefilter("ignore", category=DeprecationWarning)
assert self.block.runtime.user_is_staff
assert self.block.runtime.get_user_role() == 'student'
assert self.block.runtime.user_is_staff == is_staff
assert self.block.runtime.get_user_role() == expected_role
@ddt.data(True, False)
def test_user_is_admin(self, is_global_staff):
if is_global_staff:
self.user = GlobalStaffFactory.create()
@patch('lms.djangoapps.courseware.block_render.get_user_role', Mock(return_value='instructor', autospec=True))
def test_get_user_role(self):
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
@@ -2715,10 +2718,51 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase):
course=self.course,
)
block_user_info = self.block.runtime.service(self.block, "user").get_current_user()
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_ROLE) == 'instructor'
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_IS_GLOBAL_STAFF) == is_global_staff
with warnings.catch_warnings(): # For now, also test the deprecated accessors for backwards compatibility:
warnings.simplefilter("ignore", category=DeprecationWarning)
assert self.block.runtime.user_is_admin == is_global_staff
@ddt.data(True, False)
def test_user_is_beta_tester(self, is_beta_tester):
if is_beta_tester:
self.user = BetaTesterFactory(course_key=self.course.id)
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.course.id,
self.track_function,
self.request_token,
course=self.course,
)
block_user_info = self.block.runtime.service(self.block, "user").get_current_user()
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_IS_BETA_TESTER) == is_beta_tester
with warnings.catch_warnings(): # For now, also test the deprecated accessors for backwards compatibility:
warnings.simplefilter("ignore", category=DeprecationWarning)
assert self.block.runtime.user_is_beta_tester == is_beta_tester
@ddt.data((True, 'instructor'), (False, 'student'))
@ddt.unpack
def test_get_user_role(self, is_instructor, expected_role):
if is_instructor:
self.user = InstructorFactory(course_key=self.course.id)
_ = render.prepare_runtime_for_user(
self.user,
self.student_data,
self.block,
self.course.id,
self.track_function,
self.request_token,
course=self.course,
)
block_user_info = self.block.runtime.service(self.block, "user").get_current_user()
assert block_user_info.opt_attrs.get(ATTR_KEY_USER_ROLE) == expected_role
with warnings.catch_warnings(): # For now, also test the deprecated accessor for backwards compatibility:
warnings.simplefilter("ignore", category=DeprecationWarning)
assert self.block.runtime.get_user_role() == 'instructor'
assert self.block.runtime.get_user_role() == expected_role
def test_anonymous_student_id(self):
expected_anon_id = anonymous_id_for_user(self.user, self.course.id)

2
requirements/edx/base.txt
View File

@@ -793,7 +793,7 @@ openedx-mongodbproxy==0.2.0
# via -r requirements/edx/base.in
optimizely-sdk==4.1.1
# via -r requirements/edx/base.in
ora2==5.0.2
ora2==5.0.3
# via -r requirements/edx/base.in
oscrypto==1.3.0
# via snowflake-connector-python

2
requirements/edx/development.txt
View File

@@ -1051,7 +1051,7 @@ openedx-mongodbproxy==0.2.0
# via -r requirements/edx/testing.txt
optimizely-sdk==4.1.1
# via -r requirements/edx/testing.txt
ora2==5.0.2
ora2==5.0.3
# via -r requirements/edx/testing.txt
oscrypto==1.3.0
# via

2
requirements/edx/testing.txt
View File

@@ -999,7 +999,7 @@ openedx-mongodbproxy==0.2.0
# via -r requirements/edx/base.txt
optimizely-sdk==4.1.1
# via -r requirements/edx/base.txt
ora2==5.0.2
ora2==5.0.3
# via -r requirements/edx/base.txt
oscrypto==1.3.0
# via

32
xmodule/x_module.py
View File

@@ -43,6 +43,8 @@ from common.djangoapps.xblock_django.constants import (
ATTR_KEY_ANONYMOUS_USER_ID,
ATTR_KEY_REQUEST_COUNTRY_CODE,
ATTR_KEY_USER_ID,
ATTR_KEY_USER_IS_BETA_TESTER,
ATTR_KEY_USER_IS_GLOBAL_STAFF,
ATTR_KEY_USER_IS_STAFF,
ATTR_KEY_USER_ROLE,
)
@@ -1190,6 +1192,36 @@ class ModuleSystemShim:
if user_service:
return partial(user_service.get_current_user().opt_attrs.get, ATTR_KEY_USER_ROLE)
@property
def user_is_beta_tester(self):
"""
Returns whether the current user is enrolled in the course as a beta tester.
Deprecated in favor of the user service.
"""
warnings.warn(
'runtime.user_is_beta_tester is deprecated. Please use the user service instead.',
DeprecationWarning, stacklevel=2,
)
user_service = self._runtime_services.get('user') or self._services.get('user')
if user_service:
return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_BETA_TESTER)
@property
def user_is_admin(self):
"""
Returns whether the current user has global staff permissions.
Deprecated in favor of the user service.
"""
warnings.warn(
'runtime.user_is_admin is deprecated. Please use the user service instead.',
DeprecationWarning, stacklevel=2,
)
user_service = self._runtime_services.get('user') or self._services.get('user')
if user_service:
return user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_GLOBAL_STAFF)
@property
def render_template(self):
"""