From a6776f697397100717ea2862f738ca5044c845bf Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 17 Jan 2017 16:25:54 -0500 Subject: [PATCH] Always check group access when masquerading. TNL-6050 --- lms/djangoapps/courseware/access.py | 14 ++++- lms/djangoapps/courseware/tests/helpers.py | 41 +++++++++++++- .../courseware/tests/test_access.py | 56 ++++++++++++++++++- .../courseware/tests/test_masquerade.py | 35 ++++++------ .../tests/test_partition_scheme.py | 20 +------ 5 files changed, 126 insertions(+), 40 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 0c554ca4f8..b80fbe8998 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -469,6 +469,10 @@ def _has_group_access(descriptor, user, course_key): # via updating the children of the split_test module. return ACCESS_GRANTED + # Allow staff and instructors roles group access, as they are not masquerading as a student. + if get_user_role(user, course_key) in ['staff', 'instructor']: + return ACCESS_GRANTED + # use merged_group_access which takes group access on the block's # parents / ancestors into account merged_access = descriptor.merged_group_access @@ -550,14 +554,20 @@ def _has_access_descriptor(user, action, descriptor, course_key=None): students to see modules. If not, views should check the course, so we don't have to hit the enrollments table on every module load. """ + # If the user (or the role the user is currently masquerading as) does not have + # access to this content, then deny access. The problem with calling _has_staff_access_to_descriptor + # before this method is that _has_staff_access_to_descriptor short-circuits and returns True + # for staff users in preview mode. + if not _has_group_access(descriptor, user, course_key): + return ACCESS_DENIED + + # If the user has staff access, they can load the module and checks below are not needed. if _has_staff_access_to_descriptor(user, descriptor, course_key): return ACCESS_GRANTED - # if the user has staff access, they can load the module so this code doesn't need to run return ( _visible_to_nonstaff_users(descriptor) and _can_access_descriptor_with_milestones(user, descriptor, course_key) and - _has_group_access(descriptor, user, course_key) and ( _has_detached_class_tag(descriptor) or _can_access_descriptor_with_start_date(user, descriptor, course_key) diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index a7c76ef937..6aaf154b99 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -1,7 +1,6 @@ """ Helpers for courseware tests. """ -import crum import json from django.contrib.auth.models import User @@ -10,6 +9,10 @@ from django.test import TestCase from django.test.client import RequestFactory from courseware.access import has_access +from courseware.masquerade import ( + handle_ajax, + setup_masquerade +) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.models import Registration @@ -178,3 +181,39 @@ class CourseAccessTestMixin(TestCase): """ self.assertFalse(has_access(user, action, course)) self.assertFalse(has_access(user, action, CourseOverview.get_from_id(course.id))) + + +def masquerade_as_group_member(user, course, partition_id, group_id): + """ + Installs a masquerade for the specified user and course, to enable + the user to masquerade as belonging to the specific partition/group + combination. + + Arguments: + user (User): a user. + course (CourseDescriptor): a course. + partition_id (int): the integer partition id, referring to partitions already + configured in the course. + group_id (int); the integer group id, within the specified partition. + + Returns: the status code for the AJAX response to update the user's masquerade for + the specified course. + """ + request = _create_mock_json_request( + user, + data={"role": "student", "user_partition_id": partition_id, "group_id": group_id} + ) + response = handle_ajax(request, unicode(course.id)) + setup_masquerade(request, course.id, True) + return response.status_code + + +def _create_mock_json_request(user, data, method='POST'): + """ + Returns a mock JSON request for the specified user. + """ + factory = RequestFactory() + request = factory.generic(method, '/', content_type='application/json', data=json.dumps(data)) + request.user = user + request.session = {} + return request diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 5bf6956a21..c0e9b33b41 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -27,7 +27,7 @@ from courseware.tests.factories import ( StaffFactory, UserFactory, ) -from courseware.tests.helpers import LoginEnrollmentTestCase +from courseware.tests.helpers import LoginEnrollmentTestCase, masquerade_as_group_member from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from student.models import CourseEnrollment from student.roles import CourseCcxCoachRole, CourseStaffRole @@ -44,6 +44,9 @@ from xmodule.course_module import ( CATALOG_VISIBILITY_NONE, ) from xmodule.error_module import ErrorDescriptor +from xmodule.partitions.partitions import Group, UserPartition +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, @@ -293,6 +296,57 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase, MilestonesTes bool(access.has_access(self.student, 'staff', self.course, course_key=self.course.id)) ) + @patch('courseware.access.in_preview_mode', Mock(return_value=True)) + def test_has_access_in_preview_mode_with_group(self): + """ + Test that a user masquerading as a member of a group sees appropriate content in preview mode. + """ + partition_id = 0 + group_0_id = 0 + group_1_id = 1 + user_partition = UserPartition( + partition_id, 'Test User Partition', '', + [Group(group_0_id, 'Group 1'), Group(group_1_id, 'Group 2')], + scheme_id='cohort' + ) + self.course.user_partitions.append(user_partition) + self.course.cohort_config = {'cohorted': True} + + chapter = ItemFactory.create(category="chapter", parent_location=self.course.location) + chapter.group_access = {partition_id: [group_0_id]} + chapter.user_partitions = self.course.user_partitions + + modulestore().update_item(self.course, ModuleStoreEnum.UserID.test) + + # User should not be able to preview when masquerading as student (and not in the group above). + with patch('courseware.access.get_user_role') as mock_user_role: + mock_user_role.return_value = 'student' + self.assertFalse( + bool(access.has_access(self.global_staff, 'load', chapter, course_key=self.course.id)) + ) + + # Should be able to preview when in staff or instructor role. + for mocked_role in ['staff', 'instructor']: + with patch('courseware.access.get_user_role') as mock_user_role: + mock_user_role.return_value = mocked_role + self.assertTrue( + bool(access.has_access(self.global_staff, 'load', chapter, course_key=self.course.id)) + ) + + # Now install masquerade group and set staff as a member of that. + self.assertEqual(200, masquerade_as_group_member(self.global_staff, self.course, partition_id, group_0_id)) + # Can load the chapter since user is in the group. + self.assertTrue( + bool(access.has_access(self.global_staff, 'load', chapter, course_key=self.course.id)) + ) + + # Move the user to be a part of the second group. + self.assertEqual(200, masquerade_as_group_member(self.global_staff, self.course, partition_id, group_1_id)) + # Cannot load the chapter since user is in a different group. + self.assertFalse( + bool(access.has_access(self.global_staff, 'load', chapter, course_key=self.course.id)) + ) + def test_has_access_to_course(self): self.assertFalse(access._has_access_to_course( None, 'staff', self.course.id diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index a4ae44674b..3e577cab14 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -8,7 +8,7 @@ from nose.plugins.attrib import attr from datetime import datetime from django.core.urlresolvers import reverse -from django.test import TestCase, RequestFactory +from django.test import TestCase from django.utils.timezone import UTC from capa.tests.response_xml_factory import OptionResponseXMLFactory @@ -20,7 +20,7 @@ from courseware.masquerade import ( get_masquerading_group_info ) from courseware.tests.factories import StaffFactory -from courseware.tests.helpers import LoginEnrollmentTestCase +from courseware.tests.helpers import LoginEnrollmentTestCase, masquerade_as_group_member from courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin from student.tests.factories import UserFactory from xblock.runtime import DictKeyValueStore @@ -107,16 +107,6 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase): ) return self.client.get(url) - def _create_mock_json_request(self, user, data, method='POST', session=None): - """ - Returns a mock JSON request for the specified user - """ - factory = RequestFactory() - request = factory.generic(method, '/', content_type='application/json', data=json.dumps(data)) - request.user = user - request.session = session or {} - return request - def verify_staff_debug_present(self, staff_debug_expected): """ Verifies that the staff debug control visibility is as expected (for staff only). @@ -162,6 +152,19 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase): "Profile link should point to real user", ) + def ensure_masquerade_as_group_member(self, partition_id, group_id): + """ + Installs a masquerade for the test_user and test course, to enable the + user to masquerade as belonging to the specific partition/group combination. + Also verifies that the call to install the masquerade was successful. + + Arguments: + partition_id (int): the integer partition id, referring to partitions already + configured in the course. + group_id (int); the integer group id, within the specified partition. + """ + self.assertEqual(200, masquerade_as_group_member(self.test_user, self.course, partition_id, group_id)) + @attr(shard=1) class NormalStudentVisibilityTest(MasqueradeTestCase): @@ -405,13 +408,7 @@ class TestGetMasqueradingGroupId(StaffMasqueradeTestCase): self.assertIsNone(user_partition_id) # Install a masquerading group - request = self._create_mock_json_request( - self.test_user, - data={"role": "student", "user_partition_id": 0, "group_id": 1} - ) - response = handle_ajax(request, unicode(self.course.id)) - self.assertEquals(response.status_code, 200) - setup_masquerade(request, self.course.id, True) + self.ensure_masquerade_as_group_member(0, 1) # Verify that the masquerading group is returned group_id, user_partition_id = get_masquerading_group_info(self.test_user, self.course.id) diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py index bdab552cdf..1e28ad2afd 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py @@ -6,7 +6,6 @@ import django.test from mock import patch from nose.plugins.attrib import attr -from courseware.masquerade import handle_ajax, setup_masquerade from courseware.tests.test_masquerade import StaffMasqueradeTestCase from student.tests.factories import UserFactory from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError @@ -341,30 +340,17 @@ class TestMasqueradedGroup(StaffMasqueradeTestCase): scheme_id='cohort' ) self.course.user_partitions.append(self.user_partition) - self.session = {} modulestore().update_item(self.course, self.test_user.id) def _verify_masquerade_for_group(self, group): """ Verify that the masquerade works for the specified group id. """ - # Send the request to set the masquerade - request_json = { - "role": "student", - "user_partition_id": self.user_partition.id, - "group_id": group.id if group is not None else None - } - request = self._create_mock_json_request( - self.test_user, - data=request_json, - session=self.session + self.ensure_masquerade_as_group_member( # pylint: disable=no-member + self.user_partition.id, + group.id if group is not None else None ) - response = handle_ajax(request, unicode(self.course.id)) - # pylint has issues analyzing this class (maybe due to circular imports?) - self.assertEquals(response.status_code, 200) # pylint: disable=no-member - # Now setup the masquerade for the test user - setup_masquerade(request, self.course.id, True) scheme = self.user_partition.scheme self.assertEqual( scheme.get_group_for_user(self.course.id, self.test_user, self.user_partition),