From 136f58cb2d411866a66048e7cfa19c1e35ca6dd1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 24 Jul 2019 11:19:02 -0400 Subject: [PATCH] Add a staff-access specific bridgekeeper rule with full query support --- lms/djangoapps/courseware/permissions.py | 5 +- lms/djangoapps/courseware/rules.py | 108 ++++++++++++++++++++++- lms/djangoapps/courseware/views/index.py | 7 +- lms/djangoapps/courseware/views/views.py | 23 +++-- lms/templates/preview_menu.html | 2 +- requirements/edx/base.in | 1 + requirements/edx/base.txt | 1 + requirements/edx/development.txt | 1 + requirements/edx/testing.txt | 1 + 9 files changed, 134 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/courseware/permissions.py b/lms/djangoapps/courseware/permissions.py index 94ee2a0ee6..9e6cc08294 100644 --- a/lms/djangoapps/courseware/permissions.py +++ b/lms/djangoapps/courseware/permissions.py @@ -3,7 +3,10 @@ Permission definitions for the courseware djangoapp """ from bridgekeeper import perms -from .rules import HasAccessRule +from .rules import HasAccessRule, HasStaffAccessToContent VIEW_COURSE_HOME = 'courseware.view_course_home' +MASQUERADE_AS_STUDENT = 'courseware.masquerade_as_student' + perms[VIEW_COURSE_HOME] = HasAccessRule('load') +perms[MASQUERADE_AS_STUDENT] = HasStaffAccessToContent() diff --git a/lms/djangoapps/courseware/rules.py b/lms/djangoapps/courseware/rules.py index 9d92abd664..38bbc2eb1d 100644 --- a/lms/djangoapps/courseware/rules.py +++ b/lms/djangoapps/courseware/rules.py @@ -3,15 +3,26 @@ django-rules and Bridgekeeper rules for courseware related features """ from __future__ import absolute_import -from bridgekeeper.rules import Rule +import logging + +import laboratory +import rules +from bridgekeeper.rules import Rule, EMPTY from course_modes.models import CourseMode +from django.conf import settings from django.db.models import Q -from opaque_keys.edx.keys import CourseKey -from student.models import CourseEnrollment +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from student.models import CourseEnrollment, CourseAccessRole +from xblock.core import XBlock +from xmodule.course_module import CourseDescriptor +from xmodule.error_module import ErrorDescriptor +from xmodule.x_module import XModule from .access import has_access -import rules + +LOG = logging.getLogger(__name__) @rules.predicate @@ -46,3 +57,92 @@ class HasAccessRule(Rule): # that is used to determine if the rule should allow a user # into django admin return Q(pk__in=[]) + + +class StaffAccessExperiment(laboratory.Experiment): + def compare(self, control, candidate): + return bool(control) == candidate + + def publish(self, result): + if not result.match: + LOG.warning( + u"StaffAccessExperiment: control=%r, candidate=%r", + result.control, + result.candidates[0], + exc_info=True + ) + + +class HasStaffAccessToContent(Rule): + """ + Check whether a user has `staff` access in a course. + + Expects to be used to filter a CourseOverview queryset + """ + def check(self, user, instance=None): + """ + Return True if the supplied user has staff-level access to the supplied content. + """ + staff_sql_experiment = StaffAccessExperiment( + raise_on_mismatch=settings.DEBUG, + context={'userid': user.id, 'instance': repr(instance)} + ) + staff_sql_experiment.control(self._check_with_has_access, args=(user, instance)) + staff_sql_experiment.candidate(self._check_with_query, args=(user, instance)) + return staff_sql_experiment.conduct() + + def _check_with_has_access(self, user, instance=None): + return has_access(user, 'staff', instance) + + def _check_with_query(self, user, instance=None): + """ + Use the query method to check whether a single user has access to the supplied object. + """ + # delegate the work to type-specific functions. + # (start with more specific types, then get more general) + if isinstance(instance, (CourseDescriptor, CourseOverview)): + course_key = instance.id + elif isinstance(instance, (ErrorDescriptor, XModule, XBlock)): + course_key = instance.scope_ids.usage_id.course_key + elif isinstance(instance, CourseKey): + course_key = instance + elif isinstance(instance, UsageKey): + course_key = instance.course_key + elif isinstance(instance, basestring): + course_key = CourseKey.from_string(instance) + + return self.filter(user, CourseOverview.objects.filter(id=course_key)).exists() + + def query(self, user): + """ + Returns a Q object that expects to be used to filter CourseOverview queries. + """ + if not user.is_authenticated: + return EMPTY + + masq_settings = getattr(user, 'masquerade_settings', {}) + masq_as_student = [ + course_key for + (course_key, masq_setting) in masq_settings.items() + if masq_setting.role == 'student' + ] + + not_masquerading_as_student = ~Q(id__in=masq_as_student) + + is_global_staff = user.is_staff + course_staff_or_instructor_courses = CourseAccessRole.objects.filter( + user=user, + role__in=('staff', 'instructor'), + course_id__isnull=False + ).values('course_id') + org_staff_or_instructor_courses = CourseAccessRole.objects.filter( + user=user, + role__in=('staff', 'instructor'), + course_key__isnull=True, + org__isnull=False + ).values('org') + + query = not_masquerading_as_student + if not is_global_staff: + query &= Q(id__in=course_staff_or_instructor_courses) | Q(org__in=org_staff_or_instructor_courses) + return query diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 53bf4d4516..0dd7ce00a2 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -66,8 +66,11 @@ from ..entrance_exams import ( from ..masquerade import check_content_start_date_for_masquerade_user, setup_masquerade from ..model_data import FieldDataCache from ..module_render import get_module_for_descriptor, toc_for_course +from ..permissions import MASQUERADE_AS_STUDENT + from .views import CourseTabView + log = logging.getLogger("edx.courseware.views.index") TEMPLATE_IMPORTS = {'urllib': urllib} @@ -152,6 +155,7 @@ class CoursewareIndex(View): # If the user is considered enrolled show the default XBlock student_view. pass + self.can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, self.course) self.is_staff = has_access(request.user, 'staff', self.course) self._setup_masquerade_for_effective_user() @@ -168,7 +172,7 @@ class CoursewareIndex(View): self.masquerade, self.effective_user = setup_masquerade( self.request, self.course_key, - self.is_staff, + self.can_masquerade, reset_masquerade_data=True ) # Set the user in the request to the effective user. @@ -415,6 +419,7 @@ class CoursewareIndex(View): 'init': '', 'fragment': Fragment(), 'staff_access': self.is_staff, + 'can_masquerade': self.can_masquerade, 'masquerade': self.masquerade, 'supports_preview_menu': True, 'studio_url': get_studio_url(self.course, 'course'), diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index b215607283..3650d3b628 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -63,7 +63,7 @@ from courseware.courses import ( from courseware.masquerade import setup_masquerade from courseware.model_data import FieldDataCache from courseware.models import BaseStudentModuleHistory, StudentModule -from courseware.permissions import VIEW_COURSE_HOME +from courseware.permissions import VIEW_COURSE_HOME, MASQUERADE_AS_STUDENT from courseware.url_helpers import get_redirect_url from courseware.user_state_client import DjangoXBlockUserStateClient from edxmako.shortcuts import marketing_link, render_to_response, render_to_string @@ -355,8 +355,8 @@ def course_info(request, course_id): with modulestore().bulk_operations(course_key): course = get_course_with_access(request.user, 'load', course_key) - staff_access = has_access(request.user, 'staff', course) - masquerade, user = setup_masquerade(request, course_key, staff_access, reset_masquerade_data=True) + can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, course) + masquerade, user = setup_masquerade(request, course_key, can_masquerade, reset_masquerade_data=True) # LEARNER-612: CCX redirect handled by new Course Home (DONE) # LEARNER-1697: Transition banner messages to new Course Home (DONE) @@ -432,7 +432,7 @@ def course_info(request, course_id): 'course_subtitle': course_subtitle, 'show_subtitle': course_homepage_show_subtitle, 'show_org': course_homepage_show_org, - 'staff_access': staff_access, + 'can_masquerade': can_masquerade, 'masquerade': masquerade, 'supports_preview_menu': True, 'studio_url': get_studio_url(course, 'course_info'), @@ -645,11 +645,16 @@ class CourseTabView(EdxFragmentView): """ Creates the context for the fragment's template. """ - staff_access = has_access(request.user, 'staff', course) + can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, course) supports_preview_menu = tab.get('supports_preview_menu', False) uses_bootstrap = self.uses_bootstrap(request, course, tab=tab) if supports_preview_menu: - masquerade, masquerade_user = setup_masquerade(request, course.id, staff_access, reset_masquerade_data=True) + masquerade, masquerade_user = setup_masquerade( + request, + course.id, + can_masquerade, + reset_masquerade_data=True, + ) request.user = masquerade_user else: masquerade = None @@ -658,7 +663,7 @@ class CourseTabView(EdxFragmentView): 'course': course, 'tab': tab, 'active_page': tab.get('type', None), - 'staff_access': staff_access, + 'can_masquerade': can_masquerade, 'masquerade': masquerade, 'supports_preview_menu': supports_preview_menu, 'uses_bootstrap': uses_bootstrap, @@ -988,11 +993,12 @@ def _progress(request, course_key, student_id): course = get_course_with_access(request.user, 'load', course_key) staff_access = bool(has_access(request.user, 'staff', course)) + can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, course) masquerade = None if student_id is None or student_id == request.user.id: # This will be a no-op for non-staff users, returning request.user - masquerade, student = setup_masquerade(request, course_key, staff_access, reset_masquerade_data=True) + masquerade, student = setup_masquerade(request, course_key, can_masquerade, reset_masquerade_data=True) else: try: coach_access = has_ccx_coach_role(request.user, course_key) @@ -1035,6 +1041,7 @@ def _progress(request, course_key, student_id): 'courseware_summary': courseware_summary, 'studio_url': studio_url, 'grade_summary': course_grade.summary, + 'can_masquerade': can_masquerade, 'staff_access': staff_access, 'masquerade': masquerade, 'supports_preview_menu': True, diff --git a/lms/templates/preview_menu.html b/lms/templates/preview_menu.html index 0e64476799..7b239d78d8 100644 --- a/lms/templates/preview_menu.html +++ b/lms/templates/preview_menu.html @@ -11,7 +11,7 @@ from xmodule.partitions.partitions_service import get_all_partitions_for_course %> <% -show_preview_menu = course and staff_access and supports_preview_menu +show_preview_menu = course and can_masquerade and supports_preview_menu %> % if show_preview_menu: diff --git a/requirements/edx/base.in b/requirements/edx/base.in index c2ab37d4fc..3796bb23ba 100644 --- a/requirements/edx/base.in +++ b/requirements/edx/base.in @@ -100,6 +100,7 @@ help-tokens html5lib # HTML parser, used for capa problems ipaddress # Ip network support for Embargo feature jsonfield # Django model field for validated JSON; used in several apps +laboratory # Library for testing that code refactors/infrastructure changes produce identical results mailsnake # Needed for mailchimp (mailing djangoapp) mako==1.0.2 # Primary template language used for server-side page rendering Markdown # Convert text markup to HTML; used in capa problems, forums, and course wikis diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 3e4c3e5b45..b01f5ec8ce 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -147,6 +147,7 @@ jmespath==0.9.4 # via boto3, botocore jsondiff==1.2.0 # via edx-enterprise jsonfield==2.0.2 kombu==3.0.37 # via celery +laboratory==1.0.2 lazy==1.1 lepl==5.1.3 # via rfc6266-parser libsass==0.10.0 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 15f6b1675d..93ea8a0ca6 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -188,6 +188,7 @@ jmespath==0.9.4 jsondiff==1.2.0 jsonfield==2.0.2 kombu==3.0.37 +laboratory==1.0.2 lazy-object-proxy==1.4.1 lazy==1.1 lepl==5.1.3 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index b7d9bd5939..94dcd50f15 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -182,6 +182,7 @@ jmespath==0.9.4 jsondiff==1.2.0 jsonfield==2.0.2 kombu==3.0.37 +laboratory==1.0.2 lazy-object-proxy==1.4.1 # via astroid lazy==1.1 lepl==5.1.3