Always check group access when masquerading.

TNL-6050

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)

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

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

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.ensure_masquerade_as_group_member(0, 1)
-            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)
 
         # Verify that the masquerading group is returned
         group_id, user_partition_id = get_masquerading_group_info(self.test_user, self.course.id)

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
+        self.ensure_masquerade_as_group_member(  # pylint: disable=no-member
-        request_json = {
+            self.user_partition.id,
-            "role": "student",
+            group.id if group is not None else None
-            "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
         )
-        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),