Skip to content

Commit

Permalink
feat: Add permission checks to role checks - courseware, learner home…
Browse files Browse the repository at this point in the history
…, teams, content (#33991)

* feat: courseware, learner home, teams, content role checks to permission checks
  • Loading branch information
hsinkoff committed Feb 16, 2024
1 parent bad23b8 commit 9f33396
Show file tree
Hide file tree
Showing 17 changed files with 163 additions and 37 deletions.
6 changes: 3 additions & 3 deletions lms/djangoapps/course_api/blocks/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,12 @@ def test_query_counts_cached(self, store_type, with_storage_backing):
self._get_blocks(
course,
expected_mongo_queries=0,
expected_sql_queries=22 if with_storage_backing else 21,
expected_sql_queries=21 if with_storage_backing else 20,
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, True, 31),
(ModuleStoreEnum.Type.split, 2, False, 21),
(ModuleStoreEnum.Type.split, 2, True, 30),
(ModuleStoreEnum.Type.split, 2, False, 20),
)
@ddt.unpack
def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.course.enable_subsection_gating = True
self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref])

with self.assertNumQueries(5):
with self.assertNumQueries(6):
self.get_blocks_and_check_against_expected(self.user, expected_blocks_before_completion)

# clear the request cache to simulate a new request
Expand All @@ -179,7 +179,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.user,
)

with self.assertNumQueries(6):
with self.assertNumQueries(7):
self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL)

def test_staff_access(self):
Expand Down
29 changes: 25 additions & 4 deletions lms/djangoapps/courseware/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
from lms.djangoapps.mobile_api.models import IgnoreMobileAvailableFlagConfig
from lms.djangoapps.courseware.toggles import course_is_invitation_only
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.core.djangoapps.course_roles.data import CourseRolesPermission
from openedx.features.course_duration_limits.access import check_course_expired
from common.djangoapps.student import auth
from common.djangoapps.student.models import CourseEnrollmentAllowed
Expand Down Expand Up @@ -83,7 +84,8 @@ def has_ccx_coach_role(user, course_key):
ccx_id = course_key.ccx
role = CourseCcxCoachRole(course_key)

if role.has_user(user):
# TODO: remove role check once course_roles is fully impelented and data is migrated
if role.has_user(user) or user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_key):
list_ccx = CustomCourseForEdX.objects.filter(
course_id=course_key.to_course_locator(),
coach=user
Expand Down Expand Up @@ -174,7 +176,12 @@ def has_staff_access_to_preview_mode(user, course_key):
"""
has_admin_access_to_course = any(administrative_accesses_to_course_for_user(user, course_key))

return has_admin_access_to_course or is_masquerading_as_student(user, course_key)
# TODO: remove role check once course_roles is fully impelented and data is migrated
return (
has_admin_access_to_course or
is_masquerading_as_student(user, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key)
)


def _can_view_courseware_with_prerequisites(user, course):
Expand Down Expand Up @@ -721,12 +728,12 @@ def _has_access_to_course(user, access_level, course_key):
return ACCESS_DENIED

global_staff, staff_access, instructor_access = administrative_accesses_to_course_for_user(user, course_key)

permissions_access = user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key)
if global_staff:
debug("Allow: user.is_staff")
return ACCESS_GRANTED

if access_level not in ('staff', 'instructor'):
if access_level not in ('staff', 'instructor') and not permissions_access:
log.debug("Error in access._has_access_to_course access_level=%s unknown", access_level)
debug("Deny: unknown access level")
return ACCESS_DENIED
Expand All @@ -739,6 +746,10 @@ def _has_access_to_course(user, access_level, course_key):
debug("Allow: user has course instructor access")
return ACCESS_GRANTED

if permissions_access:
debug("Allow: user has view all content permission")
return ACCESS_GRANTED

debug("Deny: user did not have correct access")
return ACCESS_DENIED

Expand Down Expand Up @@ -875,11 +886,21 @@ def is_mobile_available_for_user(user, block):
Checks:
mobile_available flag on the course
Beta User and staff access overrides the mobile_available flag
Permission to view_all_published_content or
view_only_published_content overrides mobile_available flag
Arguments:
block (CourseBlock|CourseOverview): course or overview of course in question
"""
permissions_access = (
user.has_perms([
CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name,
CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name
], block.id)
)
# TODO: remove role check once course_roles is fully impelented and data is migrated
return (
auth.user_has_role(user, CourseBetaTesterRole(block.id))
or permissions_access
or _has_staff_access_to_block(user, block, block.id)
or _is_block_mobile_available(block)
)
Expand Down
7 changes: 6 additions & 1 deletion lms/djangoapps/courseware/access_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
StartDateError
)
from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_student
from openedx.core.djangoapps.course_roles.data import CourseRolesPermission
from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, COURSE_PRE_START_ACCESS_FLAG
from xmodule.course_block import COURSE_VISIBILITY_PUBLIC # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.util.xmodule_django import get_current_request_hostname # lint-amnesty, pylint: disable=wrong-import-order
Expand Down Expand Up @@ -57,7 +58,11 @@ def adjust_start_date(user, days_early_for_beta, start, course_key):
# bail early if no beta testing is set up
return start

if CourseBetaTesterRole(course_key).has_user(user):
# TODO: remove role check once course_roles is fully impelented and data is migrated
if (
CourseBetaTesterRole(course_key).has_user(user) or
user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name, course_key)
):
debug("Adjust start time: user in beta role for %s", course_key)
delta = timedelta(days_early_for_beta)
effective = start - delta
Expand Down
26 changes: 25 additions & 1 deletion lms/djangoapps/courseware/block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
from lms.djangoapps.lms_xblock.runtime import UserTagsService, lms_wrappers_aside, lms_applicable_aside_types
from lms.djangoapps.verify_student.services import XBlockVerificationService
from openedx.core.djangoapps.bookmarks.api import BookmarksService
from openedx.core.djangoapps.course_roles.data import CourseRolesPermission
from openedx.core.djangoapps.crawlers.models import CrawlersConfig
from openedx.core.djangoapps.credit.services import CreditService
from openedx.core.djangoapps.util.user_utils import SystemUser
Expand Down Expand Up @@ -573,7 +574,17 @@ def inner_get_block(block: XBlock) -> XBlock | None:
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))
has_permission = (
user.has_perms([
CourseRolesPermission.VIEW_ALL_CONTENT.perm_name,
CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name
], course_id)
)
# TODO: remove role check once course_roles is fully impelented and data is migrated
user_is_staff = (
bool(has_access(user, 'staff', course_id)) or
has_permission
)

if settings.FEATURES.get('DISPLAY_DEBUG_INFO_TO_STAFF'):
if user_is_staff or is_masquerading_as_specific_student(user, course_id):
Expand All @@ -583,6 +594,9 @@ def inner_get_block(block: XBlock) -> XBlock | None:
block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info))

store = modulestore()
# The user_is_staff, user_is_beta_test, and user_role cannot be removed until
# all x-blocks have been updated to use permissions, once they have been updated
# remove role check in favor of permissions checks

services = {
'fs': FSService(),
Expand All @@ -597,6 +611,16 @@ def inner_get_block(block: XBlock) -> XBlock | None:
# See the docstring of `DjangoXBlockUserService`.
deprecated_anonymous_user_id=anonymous_id_for_user(user, None),
request_country_code=user_location,
user_has_manage_content=user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name, course_id),
user_has_manage_grades=user.has_perm(CourseRolesPermission.MANAGE_GRADES.perm_name, course_id),
user_has_access_data_downloads=user.has_perm(
CourseRolesPermission.ACCESS_DATA_DOWNLOADS.perm_name,
course_id
),
user_has_view_all_content=user.has_perm(
CourseRolesPermission.VIEW_ALL_CONTENT.perm_name,
course_id
)
),
'verification': XBlockVerificationService(),
'proctoring': ProctoringService(),
Expand Down
31 changes: 28 additions & 3 deletions lms/djangoapps/courseware/masquerade.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from xblock.runtime import KeyValueStore

from common.djangoapps.course_modes.models import CourseMode
from openedx.core.djangoapps.course_roles.data import CourseRolesPermission
from openedx.core.djangoapps.util.user_messages import PageLevelMessages
from openedx.core.djangolib.markup import HTML
from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITION_ID
Expand Down Expand Up @@ -98,8 +99,13 @@ def get(self, request, course_key_string):
Retrieve data on the active and available masquerade options
"""
course_key = CourseKey.from_string(course_key_string)
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(request.user, course_key)
if not is_staff:
has_permissions = (
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name, course_key) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key)
)
if not is_staff and not has_permissions:
return JsonResponse({
'success': False,
})
Expand Down Expand Up @@ -165,8 +171,13 @@ def post(self, request, course_key_string):
to CourseMasquerade objects.
"""
course_key = CourseKey.from_string(course_key_string)
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(request.user, course_key)
if not is_staff:
has_permissions = (
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name, course_key) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key)
)
if not is_staff and not has_permissions:
return JsonResponse({
'success': False,
})
Expand Down Expand Up @@ -395,8 +406,22 @@ def check_content_start_date_for_masquerade_user(course_key, user, request, cour
if now < most_future_date and _is_masquerading:
group_masquerade = is_masquerading_as_student(user, course_key)
specific_student_masquerade = is_masquerading_as_specific_student(user, course_key)
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(user, course_key)
if group_masquerade or (specific_student_masquerade and not is_staff):
has_permissions = (
user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key) and
(
user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_LIVE_AND_PUBLISHED_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name, course_key)
)
)
if group_masquerade or (
specific_student_masquerade and (
not is_staff and not has_permissions
)
):
PageLevelMessages.register_warning_message(
request,
HTML(_('This user does not have access to this content because \
Expand Down
4 changes: 2 additions & 2 deletions lms/djangoapps/courseware/tests/test_discussion_xblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def test_permissions_query_load(self):
# * django_comment_client_role
# * DiscussionsConfiguration

num_queries = 6
num_queries = 9

for discussion in discussions:
discussion_xblock = get_block_for_descriptor(
Expand All @@ -466,7 +466,7 @@ def test_permissions_query_load(self):

# query to check for provider_type
# query to check waffle flag discussions.enable_new_structure_discussions
num_queries = 2
num_queries = 5

html = fragment.content
assert 'data-user-create-comment="false"' in html
Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/grades/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ def test_block_structure_created_only_once(self):
assert mock_block_structure_create.call_count == 1

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49, True),
(ModuleStoreEnum.Type.split, 2, 49, False),
(ModuleStoreEnum.Type.split, 2, 48, True),
(ModuleStoreEnum.Type.split, 2, 48, False),
)
@ddt.unpack
def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
Expand All @@ -165,7 +165,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat
self._apply_recalculate_subsection_grade()

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49),
(ModuleStoreEnum.Type.split, 2, 48),
)
@ddt.unpack
def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
Expand Down Expand Up @@ -210,7 +210,7 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal):
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49),
(ModuleStoreEnum.Type.split, 2, 48),
)
@ddt.unpack
def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries):
Expand Down
5 changes: 4 additions & 1 deletion lms/djangoapps/learner_home/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
get_masquerade_user,
)
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.core.djangoapps.course_roles.data import CourseRolesPermission
from openedx.core.djangoapps.programs.utils import ProgramProgressMeter
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser
Expand Down Expand Up @@ -327,10 +328,12 @@ def check_course_access(user, course_enrollments):
"is_too_early_to_view": not check_course_open_for_learner(
user, course_enrollment.course
),
# TODO: remove role checks once course_roles is fully impelented and data is migrated
"user_has_staff_access": any(
administrative_accesses_to_course_for_user(
user, course_enrollment.course_id
)
) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_enrollment.course_id)
),
}

Expand Down
Loading

0 comments on commit 9f33396

Please sign in to comment.