From 4418c661716db8996e9dc2575c64bd9fcfcfaa18 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Tue, 14 Dec 2021 14:54:42 -0500 Subject: [PATCH] feat: add masquerading support to learning sequences Previously, it had some basic manual masquerading by calling the endpoint with ?user=mytestuser. But this adds standard session masquerading support to the endpoint as well. This support is limited by LS's own partition group support. It only looks at the enrollment track partition currently. Further FBE and cohort partition support will come later. But this commit opens up normal session masquerading for: - Generic student - Specific student - Enrollment track AA-1151 --- lms/djangoapps/courseware/tests/helpers.py | 10 +-- .../courseware/tests/test_module_render.py | 2 - .../learning_sequences/api/permissions.py | 25 +------ .../learning_sequences/tests/test_views.py | 67 +++++++++++++++++-- .../content/learning_sequences/views.py | 47 ++++++++----- 5 files changed, 103 insertions(+), 48 deletions(-) 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: