From dd0218416b14582ba9289b73549226355edc20f3 Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Mon, 10 Aug 2015 13:51:33 +0200 Subject: [PATCH 1/3] Allow staff users to request a list of courses a student is enrolled in. The list only includes those courses the requesting user has staff access for. --- .../djangoapps/enrollment/tests/test_views.py | 75 +++++++++++++++---- common/djangoapps/enrollment/views.py | 23 ++++-- 2 files changed, 77 insertions(+), 21 deletions(-) diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index 4960c273b4..1da2df8af6 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -30,8 +30,9 @@ from enrollment.errors import CourseEnrollmentError from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.user_api.models import UserOrgTag from openedx.core.lib.django_test_client_utils import get_absolute_url -from student.tests.factories import UserFactory, CourseModeFactory from student.models import CourseEnrollment +from student.roles import CourseStaffRole +from student.tests.factories import UserFactory, CourseModeFactory from embargo.test_utils import restrict_course @@ -135,6 +136,9 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): EMAIL = "bob@example.com" PASSWORD = "edx" + OTHER_USERNAME = "Jane" + OTHER_EMAIL = "jane@example.com" + def setUp(self): """ Create a course and user, then log in. """ super(EnrollmentTest, self).setUp() @@ -151,8 +155,16 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): # miss; the modulestore is queried and course metadata is cached. __ = CourseOverview.get_from_id(self.course.id) - self.user = UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) - self.other_user = UserFactory.create() + self.user = UserFactory.create( + username=self.USERNAME, + email=self.EMAIL, + password=self.PASSWORD, + ) + self.other_user = UserFactory.create( + username=self.OTHER_USERNAME, + email=self.OTHER_EMAIL, + password=self.PASSWORD, + ) self.client.login(username=self.USERNAME, password=self.PASSWORD) @ddt.data( @@ -302,20 +314,53 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): self.client.logout() self.assert_enrollment_status(username=self.other_user.username, as_server=True) - def test_user_does_not_match_param_for_list(self): - CourseModeFactory.create( - course_id=self.course.id, - mode_slug=CourseMode.HONOR, - mode_display_name=CourseMode.HONOR, + def _assert_enrollments_visible_in_list(self, courses, use_server_key=False): + """ + Check that the list of enrollments of self.user returned for the currently logged in user + matches the list of courses passed in in 'courses'. + """ + kwargs = {} + if use_server_key: + kwargs.update(HTTP_X_EDX_API_KEY=self.API_KEY) + response = self.client.get(reverse('courseenrollments'), {'user': self.user.username}, **kwargs) + self.assertEqual(response.status_code, status.HTTP_200_OK) + data = json.loads(response.content) + self.assertItemsEqual( + [enrollment['course_details']['course_id'] for enrollment in data], + [unicode(course.id) for course in courses] ) - resp = self.client.get(reverse('courseenrollments'), {'user': self.other_user.username}) - self.assertEqual(resp.status_code, status.HTTP_404_NOT_FOUND) - # Verify that the server still has access to this endpoint. + + def test_enrollment_list_permissions(self): + """ + Test that the correct list of enrollments is returned, depending on the permissions of the + requesting user. + """ + # Create another course, and enroll self.user in both courses. + other_course = CourseFactory.create() + for course in self.course, other_course: + CourseModeFactory.create( + course_id=unicode(course.id), + mode_slug=CourseMode.HONOR, + mode_display_name=CourseMode.HONOR, + ) + self.assert_enrollment_status( + course_id=unicode(course.id), + max_mongo_calls=1, + ) + # Verify the user himself can see both of his enrollments. + self._assert_enrollments_visible_in_list([self.course, other_course]) + # Verify that self.other_user can't see any of the enrollments. + self.client.login(username=self.OTHER_USERNAME, password=self.PASSWORD) + self._assert_enrollments_visible_in_list([]) + # Create a staff user for self.course (but nor for other_course) and log her in. + staff_user = UserFactory.create(username='staff', email='staff@example.com', password=self.PASSWORD,) + CourseStaffRole(self.course.id).add_users(staff_user) + self.client.login(username='staff', password=self.PASSWORD) + # Verify that she can see only the enrollment in the course she has staff privileges for. + self._assert_enrollments_visible_in_list([self.course]) + # Verify the server can see all enrollments. self.client.logout() - resp = self.client.get( - reverse('courseenrollments'), {'username': self.other_user.username}, **{'HTTP_X_EDX_API_KEY': self.API_KEY} - ) - self.assertEqual(resp.status_code, status.HTTP_200_OK) + self._assert_enrollments_visible_in_list([self.course, other_course], use_server_key=True) def test_user_does_not_match_param(self): """ diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index faeb4d9369..c83b28135d 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -30,7 +30,9 @@ from enrollment.errors import ( CourseNotFoundError, CourseEnrollmentError, CourseModeNotFoundError, CourseEnrollmentExistsError ) +from student.auth import user_has_role from student.models import User +from student.roles import CourseStaffRole log = logging.getLogger(__name__) @@ -452,14 +454,15 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): # cross-domain CSRF. @method_decorator(ensure_csrf_cookie_cross_domain) def get(self, request): - """Gets a list of all course enrollments for the currently logged in user.""" + """Gets a list of all course enrollments for a user. + + Returns a list for the currently logged in user, or for the user named by the 'user' GET + parameter. If the username does not match the currently logged in user, only courses the + requesting user has staff permissions for are listed. + """ username = request.GET.get('user', request.user.username) - if request.user.username != username and not self.has_api_key_permissions(request): - # Return a 404 instead of a 403 (Unauthorized). If one user is looking up - # other users, do not let them deduce the existence of an enrollment. - return Response(status=status.HTTP_404_NOT_FOUND) try: - return Response(api.get_enrollments(username)) + enrollment_data = api.get_enrollments(username) except CourseEnrollmentError: return Response( status=status.HTTP_400_BAD_REQUEST, @@ -469,6 +472,14 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): ).format(username=username) } ) + if username == request.user.username or self.has_api_key_permissions(request): + return Response(enrollment_data) + filtered_data = [] + for enrollment in enrollment_data: + course_key = CourseKey.from_string(enrollment["course_details"]["course_id"]) + if user_has_role(request.user, CourseStaffRole(course_key)): + filtered_data.append(enrollment) + return Response(filtered_data) def post(self, request): """Enrolls the currently logged-in user in a course. From 4377981486225dbae810dac7ac2cc9d79ecf264f Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Wed, 2 Sep 2015 21:17:08 +0200 Subject: [PATCH 2/3] Optimise getting list of courses from Enrollment API for global staff. --- common/djangoapps/enrollment/tests/test_views.py | 8 ++++++-- common/djangoapps/enrollment/views.py | 5 +++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index 1da2df8af6..bd5a3ffbc2 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -32,7 +32,7 @@ from openedx.core.djangoapps.user_api.models import UserOrgTag from openedx.core.lib.django_test_client_utils import get_absolute_url from student.models import CourseEnrollment from student.roles import CourseStaffRole -from student.tests.factories import UserFactory, CourseModeFactory +from student.tests.factories import AdminFactory, CourseModeFactory, UserFactory from embargo.test_utils import restrict_course @@ -353,11 +353,15 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): self.client.login(username=self.OTHER_USERNAME, password=self.PASSWORD) self._assert_enrollments_visible_in_list([]) # Create a staff user for self.course (but nor for other_course) and log her in. - staff_user = UserFactory.create(username='staff', email='staff@example.com', password=self.PASSWORD,) + staff_user = UserFactory.create(username='staff', email='staff@example.com', password=self.PASSWORD) CourseStaffRole(self.course.id).add_users(staff_user) self.client.login(username='staff', password=self.PASSWORD) # Verify that she can see only the enrollment in the course she has staff privileges for. self._assert_enrollments_visible_in_list([self.course]) + # Create a global staff user, and verify she can see all enrollments. + AdminFactory(username='global_staff', email='global_staff@example.com', password=self.PASSWORD) + self.client.login(username='global_staff', password=self.PASSWORD) + self._assert_enrollments_visible_in_list([self.course, other_course]) # Verify the server can see all enrollments. self.client.logout() self._assert_enrollments_visible_in_list([self.course, other_course], use_server_key=True) diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index c83b28135d..99b64fffac 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -32,7 +32,7 @@ from enrollment.errors import ( ) from student.auth import user_has_role from student.models import User -from student.roles import CourseStaffRole +from student.roles import CourseStaffRole, GlobalStaff log = logging.getLogger(__name__) @@ -472,7 +472,8 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): ).format(username=username) } ) - if username == request.user.username or self.has_api_key_permissions(request): + if username == request.user.username or GlobalStaff().has_user(request.user) or \ + self.has_api_key_permissions(request): return Response(enrollment_data) filtered_data = [] for enrollment in enrollment_data: From 6ce7a60cf753450113ac6ef49e0613e92ae42ccc Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Tue, 15 Sep 2015 16:35:46 +0200 Subject: [PATCH 3/3] Clarify docstring of the enrollment list endpoint of the Enrollment API. --- common/djangoapps/enrollment/views.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index 99b64fffac..b3d084203e 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -459,6 +459,13 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): Returns a list for the currently logged in user, or for the user named by the 'user' GET parameter. If the username does not match the currently logged in user, only courses the requesting user has staff permissions for are listed. + + Only staff or instructor permissions on individual courses are taken into account when + deciding whether the requesting user is permitted to see a particular enrollment, i.e. + organizational staff access doesn't grant permission to see the enrollments in all courses + of the organization. This may change in the future. + + However, users with global staff access can see all enrollments of all students. """ username = request.GET.get('user', request.user.username) try: