Skip to content

feat: remove block-specific handling from runtime role checks [FC-0026] #32356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Agrendalath merged 2 commits into from
Jun 15, 2023
Merged

feat: remove block-specific handling from runtime role checks [FC-0026] #32356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fbc263
feat: remove block-specific handling from runtime role checks
Agrendalath Jun 2, 2023
bc17163
chore: update `ora2` XBlock
Agrendalath Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions common/djangoapps/xblock_django/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,26 @@
"""

# 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'
Comment thread
Agrendalath marked this conversation as resolved.
Outdated
# 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 changes: 31 additions & 8 deletions common/djangoapps/xblock_django/tests/test_user_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
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,
Expand Down Expand Up @@ -49,11 +51,15 @@ def assert_is_anon_xb_user(self, xb_user, request_country_code):
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
Expand All @@ -63,7 +69,9 @@ def assert_xblock_user_matches_django(
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
Expand All @@ -80,14 +88,24 @@ def test_convert_anon_user(self):
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.
"""
Expand All @@ -97,15 +115,20 @@ def test_convert_authenticate_user(self, user_is_staff, user_role, anonymous_use
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):
Expand Down
11 changes: 10 additions & 1 deletion common/djangoapps/xblock_django/user_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
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,
Expand All @@ -36,7 +38,10 @@ def __init__(self, django_user, **kwargs):
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
Expand All @@ -46,6 +51,8 @@ def __init__(self, django_user, **kwargs):
"""
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)
Expand Down Expand Up @@ -121,6 +128,8 @@ def _convert_django_user_to_xblock_user(self, django_user):
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)
Expand Down
66 changes: 29 additions & 37 deletions lms/djangoapps/courseware/block_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,32 +493,6 @@ def inner_get_block(block):
will_recheck_access=will_recheck_access,
)

user_is_staff = bool(has_access(user, 'staff', block.location, course_id))

@Agrendalath Agrendalath Jun 8, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no block-specific staff users, so passing the course_id is enough.

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 = []
Expand Down Expand Up @@ -555,6 +529,8 @@ def inner_get_block(block):
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
Expand All @@ -568,7 +544,7 @@ def inner_get_block(block):
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))

Expand All @@ -581,7 +557,17 @@ def inner_get_block(block):
'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(),
Expand All @@ -595,10 +581,21 @@ def inner_get_block(block):
'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,
),

@Agrendalath Agrendalath Jun 2, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the initialization of these services directly to the services dict to clarify that they are not used as dependencies elsewhere (e.g., in the wrappers).

'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),
Expand Down Expand Up @@ -629,11 +626,6 @@ def inner_get_block(block):

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


Expand Down
92 changes: 68 additions & 24 deletions lms/djangoapps/courseware/tests/test_block_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,19 @@
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,
)
Expand Down Expand Up @@ -2346,17 +2353,6 @@ def test_expected_services_exist(self, expected_service):
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.
Expand Down Expand Up @@ -2684,8 +2680,12 @@ def test_user_service_attributes(self, attribute, expected_value):
"""
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,
Expand All @@ -2696,15 +2696,59 @@ def test_user_is_staff(self):
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()

_ = 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_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)

@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,
Expand All @@ -2715,10 +2759,10 @@ def test_get_user_role(self):
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_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)
Expand Down
Loading