From a2dac21843fe12d7432e89cb59e58929d320e445 Mon Sep 17 00:00:00 2001 From: Julia Eskew Date: Fri, 2 Aug 2019 12:52:30 -0400 Subject: [PATCH] Add optional role parameter to course API endpoint. (#21059) * Add optional role parameter to course API endpoint. * fixup! Add optional role parameter to course API endpoint. --- lms/djangoapps/course_api/api.py | 65 +++++++++++++- lms/djangoapps/course_api/forms.py | 3 +- lms/djangoapps/course_api/tests/test_forms.py | 1 + lms/djangoapps/course_api/tests/test_views.py | 84 +++++++++++++++++++ lms/djangoapps/course_api/views.py | 40 ++------- 5 files changed, 159 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index e3295a4a04..3dcb08ea8c 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -4,14 +4,19 @@ Course API from __future__ import absolute_import +from django.conf import settings from django.contrib.auth.models import AnonymousUser, User from rest_framework.exceptions import PermissionDenied +import search +import six +from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.courses import ( get_course_overview_with_access, get_courses, get_permission_for_course_about ) +from openedx.core.lib.api.view_utils import LazySequence from .permissions import can_view_courses_for_username @@ -57,7 +62,54 @@ def course_detail(request, username, course_key): ) -def list_courses(request, username, org=None, filter_=None): +def _filter_by_role(course_queryset, user, roles): + """ + Filters a course queryset by the roles for which the user has access. + """ + # Global staff have access to all courses. Filter course roles for non-global staff only. + if not user.is_staff: + if roles: + for role in roles: + # Filter the courses again to return only the courses for which the user has each specified role. + course_queryset = LazySequence( + ( + course for course in course_queryset + if has_access(user, role, course.id) + ), + est_len=len(course_queryset) + ) + return course_queryset + + +def _filter_by_search(course_queryset, search_term): + """ + Filters a course queryset by the specified search term. + """ + if not settings.FEATURES['ENABLE_COURSEWARE_SEARCH'] or not search_term: + return course_queryset + + # Return all the results, 10K is the maximum allowed value for ElasticSearch. + # We should use 0 after upgrading to 1.1+: + # - https://github.com/elastic/elasticsearch/commit/8b0a863d427b4ebcbcfb1dcd69c996c52e7ae05e + results_size_infinity = 10000 + + search_courses = search.api.course_discovery_search( + search_term, + size=results_size_infinity, + ) + + search_courses_ids = {course['data']['id'] for course in search_courses['results']} + + return LazySequence( + ( + course for course in course_queryset + if six.text_type(course.id) in search_courses_ids + ), + est_len=len(course_queryset) + ) + + +def list_courses(request, username, org=None, roles=None, filter_=None, search_term=None): """ Yield all available courses. @@ -78,12 +130,21 @@ def list_courses(request, username, org=None, filter_=None): If specified, visible `CourseOverview` objects are filtered such that only those belonging to the organization with the provided org code (e.g., "HarvardX") are returned. Case-insensitive. + roles (list of strings): + If specified, visible `CourseOverview` objects are filtered + such that only those for which the user has the specified role(s) + are returned. Multiple role parameters can be specified. filter_ (dict): If specified, visible `CourseOverview` objects are filtered by the given key-value pairs. + search_term (string): + Search term to filter courses (used by ElasticSearch). Return value: Yield `CourseOverview` objects representing the collection of courses. """ user = get_effective_user(request.user, username) - return get_courses(user, org=org, filter_=filter_) + course_qs = get_courses(user, org=org, filter_=filter_) + course_qs = _filter_by_role(course_qs, user, roles) + course_qs = _filter_by_search(course_qs, search_term) + return course_qs diff --git a/lms/djangoapps/course_api/forms.py b/lms/djangoapps/course_api/forms.py index a77124096d..3bf8e3b014 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -12,7 +12,7 @@ from django.forms import CharField, Form from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.util.forms import ExtendedNullBooleanField +from openedx.core.djangoapps.util.forms import ExtendedNullBooleanField, MultiValueField class UsernameValidatorMixin(object): @@ -54,6 +54,7 @@ class CourseListGetForm(UsernameValidatorMixin, Form): search_term = CharField(required=False) username = CharField(required=False) org = CharField(required=False) + role = MultiValueField(required=False) # white list of all supported filter fields filter_type = namedtuple('filter_type', ['param_name', 'field_name']) diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index 6fadb0cebf..860c9d8dde 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -69,6 +69,7 @@ class TestCourseListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreT self.cleaned_data = { 'username': user.username, 'org': '', + 'role': set([]), 'mobile': None, 'search_term': '', 'filter_': None, diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 57d7059b03..3f809f3736 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -23,6 +23,9 @@ from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig +from student.auth import add_users +from student.roles import CourseInstructorRole, CourseStaffRole +from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -159,6 +162,7 @@ class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreT self.url = reverse('course-list') self.staff_user = self.create_user(username='staff', is_staff=True) self.honor_user = self.create_user(username='honor', is_staff=False) + self.global_admin = AdminFactory() def test_filter_by_org(self): """Verify that CourseOverviews are filtered by the provided org key.""" @@ -206,6 +210,86 @@ class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreT u"testing course_api.views.CourseListView with filter_={}".format(filter_), ) + def test_filter_by_roles_global_staff(self): + """ + Verify that global staff are always returned all courses irregardless of role filter. + """ + self.setup_user(self.staff_user) + + # Create a second course to be filtered out of queries. + alternate_course = self.create_course(org=md5(self.course.org).hexdigest()) + + # Request the courses as the staff user with the different roles specified. + for roles in ('', 'staff', 'staff,instructor'): + filtered_response = self.verify_response(params={'username': self.staff_user.username, 'role': roles}) + # Both courses should be returned in the course list. + for org in [self.course.org, alternate_course.org]: + self.assertTrue( + any(course['org'] == org for course in filtered_response.data['results']) + ) + + def test_filter_by_roles_non_staff(self): + """ + Verify that a non-staff user can't access CourseOverviews by role. + """ + + # Requesting the courses as the non-staff user should *not* be allowed. + self.setup_user(self.honor_user) + filtered_response = self.verify_response(params={'username': self.honor_user.username, 'role': 'staff'}) + self.assertEquals(len(filtered_response.data['results']), 0) + + def test_filter_by_roles_course_staff(self): + """ + Verify that CourseOverviews are filtered by the provided roles. + """ + # Make this user a course staff user for the course. + course_staff_user = self.create_user(username='course_staff', is_staff=False) + add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user) + + # Create a second course to be filtered out of queries, along with an instructor user for it. + alternate_course = self.create_course(org=md5(self.course.org).hexdigest()) + course_instructor_user = self.create_user(username='course_instructor', is_staff=False) + add_users(self.global_admin, CourseInstructorRole(alternate_course.id), course_instructor_user) + + # Requesting the courses for which the course staff user is staff should return *only* the single course. + self.setup_user(self.staff_user) + filtered_response = self.verify_response(params={ + 'username': course_staff_user.username, + 'role': 'staff' + }) + self.assertEquals(len(filtered_response.data['results']), 1) + self.assertEquals(filtered_response.data['results'][0]['org'], self.course.org) + + # The course staff user does *not* have the course instructor role on any courses. + filtered_response = self.verify_response(params={ + 'username': course_staff_user.username, + 'role': 'instructor' + }) + self.assertEquals(len(filtered_response.data['results']), 0) + + # The course instructor user only has the course instructor role on one course. + filtered_response = self.verify_response(params={ + 'username': course_instructor_user.username, + 'role': 'instructor' + }) + self.assertEquals(len(filtered_response.data['results']), 1) + self.assertEquals(filtered_response.data['results'][0]['org'], alternate_course.org) + + # The honor user does *not* have the course staff -or- instructor role on any courses. + filtered_response = self.verify_response(params={ + 'username': self.honor_user.username, + 'role': 'staff,instructor' + }) + self.assertEquals(len(filtered_response.data['results']), 0) + + # The course instructor user has the inferred course staff role on one course. + self.setup_user(course_instructor_user) + filtered_response = self.verify_response(params={ + 'username': course_instructor_user.username, + 'role': 'staff' + }) + self.assertEquals(len(filtered_response.data['results']), 1) + class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 0f2867cb8b..9f30d6517c 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -4,15 +4,12 @@ Course API Views from __future__ import absolute_import -import search -import six -from django.conf import settings from django.core.exceptions import ValidationError from edx_rest_framework_extensions.paginators import NamespacedPageNumberPagination from rest_framework.generics import ListAPIView, RetrieveAPIView from rest_framework.throttling import UserRateThrottle -from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, LazySequence, view_auth_classes +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from . import USE_RATE_LIMIT_2_FOR_COURSE_LIST_API, USE_RATE_LIMIT_10_FOR_COURSE_LIST_API from .api import course_detail, list_courses @@ -193,9 +190,11 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): provided org code (e.g., "HarvardX") are returned. Case-insensitive. - mobile (optional): - If specified, only visible `CourseOverview` objects that are - designated as mobile_available are returned. + role (optional): + If specified, visible `CourseOverview` objects are filtered + such that only those for which the user has the specified role + are returned. Multiple role parameters can be specified. + Case-insensitive. **Returns** @@ -239,11 +238,6 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): serializer_class = CourseSerializer throttle_classes = (CourseListUserThrottle,) - # Return all the results, 10K is the maximum allowed value for ElasticSearch. - # We should use 0 after upgrading to 1.1+: - # - https://github.com/elastic/elasticsearch/commit/8b0a863d427b4ebcbcfb1dcd69c996c52e7ae05e - results_size_infinity = 10000 - def get_queryset(self): """ Yield courses visible to the user. @@ -252,27 +246,11 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): if not form.is_valid(): raise ValidationError(form.errors) - db_courses = list_courses( + return list_courses( self.request, form.cleaned_data['username'], org=form.cleaned_data['org'], + roles=form.cleaned_data['role'], filter_=form.cleaned_data['filter_'], - ) - - if not settings.FEATURES['ENABLE_COURSEWARE_SEARCH'] or not form.cleaned_data['search_term']: - return db_courses - - search_courses = search.api.course_discovery_search( - form.cleaned_data['search_term'], - size=self.results_size_infinity, - ) - - search_courses_ids = {course['data']['id'] for course in search_courses['results']} - - return LazySequence( - ( - course for course in db_courses - if six.text_type(course.id) in search_courses_ids - ), - est_len=len(db_courses) + search_term=form.cleaned_data['search_term'] )