diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index a376c16f4c..c8303a4152 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -244,7 +244,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 35 + QUERY_COUNT = 36 TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): ( # # of sql queries to default, @@ -273,7 +273,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 35 + QUERY_COUNT = 36 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 3), diff --git a/lms/djangoapps/courseware/permissions.py b/lms/djangoapps/courseware/permissions.py index 722a4a4bbf..e96644b2a5 100644 --- a/lms/djangoapps/courseware/permissions.py +++ b/lms/djangoapps/courseware/permissions.py @@ -3,14 +3,16 @@ Permission definitions for the courseware djangoapp """ from bridgekeeper import perms -from .rules import HasAccessRule +from .rules import HasAccessRule, HasStaffAccessToContent EDIT_BOOKMARK = 'courseware.edit_bookmark' +MASQUERADE_AS_STUDENT = 'courseware.masquerade_as_student' VIEW_COURSE_HOME = 'courseware.view_course_home' VIEW_COURSEWARE = 'courseware.view_courseware' VIEW_XQA_INTERFACE = 'courseware.view_xqa_interface' perms[EDIT_BOOKMARK] = HasAccessRule('staff') +perms[MASQUERADE_AS_STUDENT] = HasStaffAccessToContent() perms[VIEW_COURSE_HOME] = HasAccessRule('load') perms[VIEW_COURSEWARE] = HasAccessRule('load') perms[VIEW_XQA_INTERFACE] = HasAccessRule('staff') diff --git a/lms/djangoapps/courseware/rules.py b/lms/djangoapps/courseware/rules.py index d93b50b2aa..ed77878697 100644 --- a/lms/djangoapps/courseware/rules.py +++ b/lms/djangoapps/courseware/rules.py @@ -3,17 +3,30 @@ django-rules and Bridgekeeper rules for courseware related features """ from __future__ import absolute_import -from bridgekeeper.rules import Rule -from django.db.models import Q -from opaque_keys.edx.keys import CourseKey +import logging +import laboratory import rules +from bridgekeeper.rules import Rule, EMPTY from course_modes.models import CourseMode -from student.models import CourseEnrollment +from django.conf import settings +from django.db.models import Q +from opaque_keys.edx.django.models import CourseKeyField +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 +LOG = logging.getLogger(__name__) + + @rules.predicate def is_track_ok_for_exam(user, exam): """ @@ -46,3 +59,93 @@ 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') + ).exclude( + course_id=CourseKeyField.Empty, + ).values('course_id') + org_staff_or_instructor_courses = CourseAccessRole.objects.filter( + user=user, + role__in=('staff', 'instructor'), + course_id=CourseKeyField.Empty, + 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/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index d8cb1f19e8..414cf5b374 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -434,8 +434,8 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest def test_num_queries_instructor_paced(self): # TODO: decrease query count as part of REVO-28 - self.fetch_course_info_with_queries(self.instructor_paced_course, 44, 3) + self.fetch_course_info_with_queries(self.instructor_paced_course, 45, 3) def test_num_queries_self_paced(self): # TODO: decrease query count as part of REVO-28 - self.fetch_course_info_with_queries(self.self_paced_course, 44, 3) + self.fetch_course_info_with_queries(self.self_paced_course, 45, 3) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 02dc769a5b..4741995b2d 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -224,8 +224,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 181), - (ModuleStoreEnum.Type.split, 4, 175), + (ModuleStoreEnum.Type.mongo, 10, 182), + (ModuleStoreEnum.Type.split, 4, 176), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1466,8 +1466,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 55), - (False, 54) + (True, 56), + (False, 55) ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1480,8 +1480,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 63, 43), - (True, 54, 38) + (False, 64, 44), + (True, 55, 39) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): 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 3712411aa3..f9be6c3c15 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, VIEW_COURSEWARE, VIEW_XQA_INTERFACE +from courseware.permissions import MASQUERADE_AS_STUDENT, VIEW_COURSE_HOME, VIEW_COURSEWARE, VIEW_XQA_INTERFACE 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 @@ -356,8 +356,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) @@ -433,7 +433,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'), @@ -646,11 +646,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 @@ -659,7 +664,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, @@ -989,11 +994,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) @@ -1036,6 +1042,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/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 79ece8cfed..5dc36870db 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -217,7 +217,7 @@ class TestCourseHomePage(CourseHomePageTestCase): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(94, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(95, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 45d51a5448..4a00d6c886 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -134,7 +134,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(55, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(56, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url)