diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index fe2b6c91d689..ff6be033f9f5 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -7,6 +7,7 @@ import logging from abc import ABCMeta, abstractmethod from collections import defaultdict +from contextlib import contextmanager from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from opaque_keys.edx.django.models import CourseKeyField @@ -44,6 +45,17 @@ def register_access_role(cls): return cls +@contextmanager +def strict_role_checking(): + """ + Context manager that temporarily disables role inheritance. + """ + OLD_ACCESS_ROLES_INHERITANCE = ACCESS_ROLES_INHERITANCE.copy() + ACCESS_ROLES_INHERITANCE.clear() + yield + ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE) + + class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring CACHE_NAMESPACE = "student.roles.BulkRoleCache" CACHE_KEY = 'roles_by_user' @@ -78,7 +90,7 @@ def __init__(self, user): ) @staticmethod - def _get_roles(role): + def get_roles(role): """ Return the roles that should have the same permissions as the specified role. """ @@ -90,7 +102,7 @@ def has_role(self, role, course_id, org): or a role that inherits from the specified role, course_id and org. """ return any( - access_role.role in self._get_roles(role) and + access_role.role in self.get_roles(role) and access_role.course_id == course_id and access_role.org == org for access_role in self._roles @@ -286,6 +298,14 @@ class CourseLimitedStaffRole(CourseStaffRole): BASE_ROLE = CourseStaffRole.ROLE +@register_access_role +class eSHEInstructorRole(CourseStaffRole): + """A Staff member of a course without access to Studio.""" + + ROLE = 'eshe_instructor' + BASE_ROLE = CourseStaffRole.ROLE + + @register_access_role class CourseInstructorRole(CourseRole): """A course Instructor""" @@ -463,11 +483,11 @@ def remove_courses(self, *course_keys): def courses_with_role(self): """ - Return a django QuerySet for all of the courses with this user x role. You can access + Return a django QuerySet for all of the courses with this user x (or derived from x) role. You can access any of these properties on each result record: * user (will be self.user--thus uninteresting) * org * course_id * role (will be self.role--thus uninteresting) """ - return CourseAccessRole.objects.filter(role=self.role, user=self.user) + return CourseAccessRole.objects.filter(role__in=RoleCache.get_roles(self.role), user=self.user) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index c670c7c2b75b..831d33045d66 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -16,6 +16,7 @@ CourseStaffRole, CourseFinanceAdminRole, CourseSalesAdminRole, + eSHEInstructorRole, LibraryUserRole, CourseDataResearcherRole, GlobalStaff, @@ -168,6 +169,7 @@ class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-clas ROLES = ( (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), (CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')), + (eSHEInstructorRole(IN_KEY), ('eshe_instructor', IN_KEY, 'edX')), (CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')), (OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')), (CourseFinanceAdminRole(IN_KEY), ('finance_admin', IN_KEY, 'edX')), diff --git a/lms/djangoapps/courseware/rules.py b/lms/djangoapps/courseware/rules.py index 8202418f6f4c..07cbbab9022c 100644 --- a/lms/djangoapps/courseware/rules.py +++ b/lms/djangoapps/courseware/rules.py @@ -18,7 +18,7 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.enrollments.api import is_enrollment_valid_for_proctoring from common.djangoapps.student.models import CourseAccessRole -from common.djangoapps.student.roles import CourseRole, OrgRole +from common.djangoapps.student.roles import CourseRole, OrgRole, strict_role_checking from xmodule.course_block import CourseBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.error_block import ErrorBlock # lint-amnesty, pylint: disable=wrong-import-order @@ -47,10 +47,14 @@ class HasAccessRule(Rule): """ A rule that calls `has_access` to determine whether it passes """ - def __init__(self, action): + def __init__(self, action, strict=False): self.action = action + self.strict = strict def check(self, user, instance=None): + if self.strict: + with strict_role_checking(): + return has_access(user, self.action, instance) return has_access(user, self.action, instance) def query(self, user): diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 9255d113f038..343b6923ccee 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -19,6 +19,7 @@ CourseInstructorRole, CourseLimitedStaffRole, CourseStaffRole, + eSHEInstructorRole, ) from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params from openedx.core.djangoapps.django_comment_common.models import Role @@ -30,6 +31,7 @@ 'instructor': CourseInstructorRole, 'staff': CourseStaffRole, 'limited_staff': CourseLimitedStaffRole, + 'eshe_instructor': eSHEInstructorRole, 'ccx_coach': CourseCcxCoachRole, 'data_researcher': CourseDataResearcherRole, } diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 20426761e66b..ac9b430acec1 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -44,7 +44,13 @@ # only global staff or those with the data_researcher role can access the data download tab # HasAccessRule('staff') also includes course staff perms[CAN_RESEARCH] = is_staff | HasRolesRule('data_researcher') -perms[CAN_ENROLL] = HasAccessRule('staff') +# eshe_instructor implicitly gets staff access, but shouldn't be able to enroll +perms[CAN_ENROLL] = ( + # can enroll if a user is an eshe_instructor and has an explicit staff role + (HasRolesRule('eshe_instructor') & HasAccessRule('staff', strict=True)) | + # can enroll if a user is just staff + (~HasRolesRule('eshe_instructor') & HasAccessRule('staff')) +) perms[CAN_BETATEST] = HasAccessRule('instructor') perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher') diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index bca16977a98c..bdbffbde7f68 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -29,6 +29,7 @@ from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase +from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK from lms.djangoapps.instructor.toggles import DATA_DOWNLOAD_V2 from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info @@ -37,6 +38,7 @@ ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND, OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG ) +from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -308,6 +310,56 @@ def test_membership_reason_field_visibility(self, enbale_reason_field): else: self.assertNotContains(response, reason_field) + def test_membership_tab_for_eshe_instructor(self): + """ + Verify that eSHE instructors don't have access to membership tab and + work correctly with other roles. + """ + + membership_section = ( + '
  • ' + '' + '
    • ' + ) + batch_enrollment = ( + '
    ' + ) + + user = UserFactory.create() + self.client.login(username=user.username, password="test") + + # eSHE instructors shouldn't have access to membership tab + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='eshe_instructor', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertNotContains(response, membership_section) + + # However if combined with forum_admin, they should have access to this + # tab, but not to batch enrollment + forum_admin_role = RoleFactory(name=FORUM_ROLE_ADMINISTRATOR, course_id=self.course.id) + forum_admin_role.users.add(user) + response = self.client.get(self.url) + self.assertContains(response, membership_section) + self.assertNotContains(response, batch_enrollment) + + # Combined with course staff, should have union of all three roles + # permissions sets + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='staff', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertContains(response, membership_section) + self.assertContains(response, batch_enrollment) + def test_student_admin_staff_instructor(self): """ Verify that staff users are not able to see course-wide options, while still diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 579b8d035f90..9ff82a3e1c5d 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -35,7 +35,9 @@ CourseFinanceAdminRole, CourseInstructorRole, CourseSalesAdminRole, - CourseStaffRole + CourseStaffRole, + eSHEInstructorRole, + strict_role_checking, ) from common.djangoapps.util.json_request import JsonResponse from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled @@ -126,6 +128,7 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable access = { 'admin': request.user.is_staff, 'instructor': bool(has_access(request.user, 'instructor', course)), + 'eshe_instructor': eSHEInstructorRole(course_key).has_user(request.user), 'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user), 'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user), 'staff': bool(has_access(request.user, 'staff', course)), @@ -133,6 +136,9 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable 'data_researcher': request.user.has_perm(permissions.CAN_RESEARCH, course_key), } + with strict_role_checking(): + access['explicit_staff'] = bool(has_access(request.user, 'staff', course)) + if not request.user.has_perm(permissions.VIEW_DASHBOARD, course_key): raise Http404() @@ -490,7 +496,15 @@ def _section_membership(course, access): 'update_forum_role_membership', kwargs={'course_id': str(course_key)} ), - 'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False) + 'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False), + + # Membership section should be hidden for eSHE instructors. + # Since they get Course Staff role implicitly, we need to hide this + # section if the user doesn't have the Course Staff role set explicitly + # or have the Discussion Admin role. + 'is_hidden': ( + not access['forum_admin'] and (access['eshe_instructor'] and not access['explicit_staff']) + ), } return section_data diff --git a/lms/envs/common.py b/lms/envs/common.py index 23c335cee154..c5ecfa68af21 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1048,6 +1048,16 @@ # .. toggle_target_removal_date: None # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, + + # .. toggle_name: FEATURES['ENABLE_ESHE_INSTRUCTOR_ROLE'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the ESHE Instructor role + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2023-07-31 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/561/files' + 'ENABLE_ESHE_INSTRUCTOR_ROLE': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API diff --git a/lms/static/js/fixtures/instructor_dashboard/membership.html b/lms/static/js/fixtures/instructor_dashboard/membership.html index 962bfac5b4f1..6b77a148df69 100644 --- a/lms/static/js/fixtures/instructor_dashboard/membership.html +++ b/lms/static/js/fixtures/instructor_dashboard/membership.html @@ -11,6 +11,7 @@

    Course Team Management