diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index e92326934a..5247f71c3b 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -331,14 +331,16 @@ class MasqueradeMixin: to pass in the course parameter below. """ - def update_masquerade(self, course=None, role='student', group_id=None, username=None, user_partition_id=None): + def update_masquerade(self, course=None, course_id=None, role='student', group_id=None, username=None, + user_partition_id=None): """ Installs a masquerade for the specified user and course, to enable the user to masquerade as belonging to the specific partition/group combination. Arguments: - course (object): a course or None for self.course + course (object): a course or None for self.course (or you can pass course_id instead) + course_id (str|CourseKey): a course id, useful if you don't happen to have a full course object handy user_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. @@ -347,11 +349,11 @@ class MasqueradeMixin: Returns: the response object for the AJAX call to update the user's masquerade. """ - course = course or self.course + course_id = str(course_id or (course and course.id) or self.course.id) masquerade_url = reverse( 'masquerade_update', kwargs={ - 'course_key_string': str(course.id), + 'course_key_string': course_id, } ) response = self.client.post( diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 7d2cd0b70e..0ebcb2c979 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -2630,7 +2630,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) @@ -2703,7 +2702,6 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): self.descriptor, self.course.id, self.track_function, - self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) diff --git a/openedx/core/djangoapps/content/learning_sequences/api/permissions.py b/openedx/core/djangoapps/content/learning_sequences/api/permissions.py index 156e1d1678..747e9e6e17 100644 --- a/openedx/core/djangoapps/content/learning_sequences/api/permissions.py +++ b/openedx/core/djangoapps/content/learning_sequences/api/permissions.py @@ -7,11 +7,7 @@ to determine whether those processors even need to be run to filter the results. """ from opaque_keys.edx.keys import CourseKey -from common.djangoapps.student.roles import ( - GlobalStaff, - CourseInstructorRole, - CourseStaffRole, -) +from lms.djangoapps.courseware.access import has_access from openedx.core import types from ..toggles import USE_FOR_OUTLINES @@ -31,20 +27,5 @@ def can_see_all_content(requesting_user: types.User, course_key: CourseKey) -> b There's no need to run processors to restrict results for these users. """ - return ( - GlobalStaff().has_user(requesting_user) or - CourseStaffRole(course_key).has_user(requesting_user) or - CourseInstructorRole(course_key).has_user(requesting_user) - ) - - -def can_see_content_as_other_users(requesting_user: types.User, course_key: CourseKey) -> bool: - """ - Is this user allowed to view this content as other users? - - For now, this is the same set of people who are allowed to see all content - (i.e. some kind of course or global staff). It's possible that we'll want to - make more granular distinctions between different kinds of staff roles in - the future (e.g. CourseDataResearcher). - """ - return can_see_all_content(requesting_user, course_key) + # has_access handles all possible staff cases, including checking for masquerading + return has_access(requesting_user, 'staff', course_key).has_access diff --git a/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py b/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py index e953ac818e..c50e1cdff1 100644 --- a/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py +++ b/openedx/core/djangoapps/content/learning_sequences/tests/test_views.py @@ -17,12 +17,15 @@ from datetime import datetime, timezone import ddt from edx_toggles.toggles.testutils import override_waffle_flag -from opaque_keys.edx.keys import CourseKey # lint-amnesty, pylint: disable=unused-import +from opaque_keys.edx.keys import CourseKey from rest_framework.test import APITestCase, APIClient -from openedx.core.djangolib.testing.utils import CacheIsolationTestCase +from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from ..api import replace_course_outline from ..api.tests.test_data import generate_sections @@ -30,6 +33,7 @@ from ..data import CourseOutlineData, CourseVisibility from ..toggles import USE_FOR_OUTLINES +@skip_unless_lms @override_waffle_flag(USE_FOR_OUTLINES, active=True) class CourseOutlineViewTest(CacheIsolationTestCase, APITestCase): """ @@ -137,9 +141,10 @@ class CourseOutlineViewTest(CacheIsolationTestCase, APITestCase): @ddt.ddt -class CourseOutlineViewMasqueradingTest(CacheIsolationTestCase, APITestCase): +@skip_unless_lms +class CourseOutlineViewTargetUserTest(CacheIsolationTestCase, APITestCase): """ - Tests permissions of masquerading. + Tests permissions of specifying a target user via url parameter. """ @classmethod def setUpTestData(cls): # lint-amnesty, pylint: disable=super-method-not-called @@ -261,6 +266,60 @@ class CourseOutlineViewMasqueradingTest(CacheIsolationTestCase, APITestCase): assert masq_attempt_result.status_code == 403 +@ddt.ddt +@skip_unless_lms +@override_waffle_flag(USE_FOR_OUTLINES, active=True) +class CourseOutlineViewMasqueradingTest(MasqueradeMixin, CacheIsolationTestCase): + """ + Tests permissions of session masquerading. + """ + @classmethod + def setUpTestData(cls): + """Set up the basic course outline and our users.""" + super().setUpTestData() + + overview = CourseOverviewFactory() + cls.course_key = overview.id + + outline = CourseOutlineData( + course_key=cls.course_key, + title="Views Test Course!", + published_at=datetime(2020, 5, 20, tzinfo=timezone.utc), + published_version="5ebece4b69dd593d82fe2020", + entrance_exam_id=None, + days_early_for_beta=None, + sections=generate_sections(cls.course_key, [2, 2]), + self_paced=False, + course_visibility=CourseVisibility.PUBLIC + ) + replace_course_outline(outline) + + # Users + cls.staff = UserFactory(is_staff=True, password='test') + cls.student = UserFactory(username='student') + UserFactory(username='student2') + + CourseEnrollment.enroll(cls.student, cls.course_key) + + def setUp(self): + super().setUp() + self.client.login(username=self.staff.username, password='test') + + def test_masquerading_works(self): + """Confirm that session masquerading works as expected.""" + self.update_masquerade(course_id=self.course_key, username='student') + result = self.client.get(outline_url(self.course_key)) + assert result.status_code == 200 + assert result.data['username'] == 'student' + + def test_target_user_takes_precedence(self): + """Specifying a user should override any masquerading.""" + self.update_masquerade(course_id=self.course_key, username='student') + result = self.client.get(outline_url(self.course_key), {'user': 'student2'}) + assert result.status_code == 200 + assert result.data['username'] == 'student2' + + def outline_url(course_key): """Helper: Course outline URL for a given course key.""" return f'/api/learning_sequences/v1/course_outline/{course_key}' diff --git a/openedx/core/djangoapps/content/learning_sequences/views.py b/openedx/core/djangoapps/content/learning_sequences/views.py index e392e5877a..72169afbb5 100644 --- a/openedx/core/djangoapps/content/learning_sequences/views.py +++ b/openedx/core/djangoapps/content/learning_sequences/views.py @@ -16,11 +16,13 @@ from rest_framework.exceptions import NotAuthenticated, NotFound, PermissionDeni from rest_framework.response import Response from rest_framework.views import APIView +from lms.djangoapps.courseware.access import has_access +from lms.djangoapps.courseware.masquerade import setup_masquerade from openedx.core import types from openedx.core.lib.api.view_utils import validate_course_key from .api import get_user_course_outline_details -from .api.permissions import can_call_public_api, can_see_content_as_other_users +from .api.permissions import can_call_public_api from .data import CourseOutlineData User = get_user_model() @@ -162,6 +164,9 @@ class CourseOutlineView(APIView): course_key = validate_course_key(course_key_str) at_time = datetime.now(timezone.utc) + # Get target user (and override request user for the benefit of any waffle checks) + request.user = self._determine_user(request, course_key) + # We use can_call_public_api to slowly roll this feature out, and be # able to turn it off for a course. But it's not really a permissions # thing in that it doesn't give them elevated access. If I had it to do @@ -177,13 +182,11 @@ class CourseOutlineView(APIView): if (not force_on) and (not can_call_public_api(course_key)): raise PermissionDenied() - outline_user = self._determine_user(request, course_key) - try: # Grab the user's outline and send our response... - user_course_outline_details = get_user_course_outline_details(course_key, outline_user, at_time) + user_course_outline_details = get_user_course_outline_details(course_key, request.user, at_time) except CourseOutlineData.DoesNotExist as does_not_exist_err: - if not outline_user.id: + if not request.user.id: # Outline is private or doesn't exist. But don't leak whether a course exists or not to anonymous # users with a 404 - give a 401 instead. This mostly prevents drive-by crawlers from creating a bunch # of 404 errors in your error report dashboard. @@ -197,28 +200,40 @@ class CourseOutlineView(APIView): """ For which user should we get an outline? - Uses a combination of the user on the request object and a manually - passed in "user" parameter. Ensures that the requesting user has - permission to view course outline of target user. Raise request-level + Uses a combination of the user on the request object, session masquerading + data, and a manually passed in "user" parameter. Ensures that the requesting + user has permission to view course outline of target user. Raise request-level exceptions otherwise. The "user" querystring param is expected to be a username, with a blank - value being interpreted as the anonymous user. + value being interpreted as the anonymous user. It will take priority over + session masquerading, if provided. """ + has_staff_access = has_access(request.user, 'staff', course_key).has_access + target_username = request.GET.get("user") + if target_username is not None: + return self._get_target_user(request, course_key, has_staff_access, target_username) - # Sending no "user" querystring param at all defaults us to the user who - # is making the request. - if target_username is None: - return request.user + _course_masquerade, user = setup_masquerade(request, course_key, has_staff_access) + return user + @staticmethod + def _get_target_user(request, course_key: CourseKey, has_staff_access: bool, target_username: str) -> types.User: + """ + Load and return the requested user with permission checking. + + A blank username will return an anonymous user. + + This was designed for manual API testing and kept in, as it may be useful for future development. + """ # Users can always see the outline as themselves. if target_username == request.user.username: return request.user # Otherwise, do a permission check to see if they're allowed to view as # other users. - if not can_see_content_as_other_users(request.user, course_key): + if not has_staff_access: display_username = "the anonymous user" if target_username == "" else target_username raise PermissionDenied( f"User {request.user.username} does not have permission to " @@ -226,14 +241,14 @@ class CourseOutlineView(APIView): ) # If we've gotten this far, their permissions are fine. Now we handle - # the masquerade use case... + # the different-user use case... # Having a "user" querystring that is a blank string is interpreted as # "show me this outline as an anonymous user". if target_username == "": return AnonymousUser() - # Finally, the actual work of looking up a user to masquerade as. + # Finally, the actual work of looking up a user to target. try: target_user = User.objects.get(username=target_username) except User.DoesNotExist as err: