diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 9e96779e9b..6b0ae672c6 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -108,7 +108,12 @@ def _filter_by_search(course_queryset, search_term): ) -def list_courses(request, username, org=None, filter_=None, search_term=None): +def list_courses(request, + username, + org=None, + filter_=None, + search_term=None, + permissions=None): """ Yield all available courses. @@ -134,12 +139,15 @@ def list_courses(request, username, org=None, filter_=None, search_term=None): by the given key-value pairs. search_term (string): Search term to filter courses (used by ElasticSearch). + permissions (list[str]): + If specified, it filters visible `CourseOverview` objects by + checking if each permission specified is granted for the username. Return value: Yield `CourseOverview` objects representing the collection of courses. """ user = get_effective_user(request.user, username) - course_qs = get_courses(user, org=org, filter_=filter_) + course_qs = get_courses(user, org=org, filter_=filter_, permissions=permissions) 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 4e7afc5978..7111039bf6 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -10,7 +10,10 @@ 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: @@ -59,6 +62,7 @@ class CourseListGetForm(UsernameValidatorMixin, Form): filter_type(param_name='mobile', field_name='mobile_available'), ] mobile = ExtendedNullBooleanField(required=False) + permissions = MultiValueField(required=False) def clean(self): """ diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index 7f838a1b7e..fc037fb055 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -100,7 +100,12 @@ class CourseListTestMixin(CourseApiTestMixin): Common behavior for list_courses tests """ - def _make_api_call(self, requesting_user, specified_user, org=None, filter_=None): + def _make_api_call(self, + requesting_user, + specified_user, + org=None, + filter_=None, + permissions=None): """ Call the list_courses api endpoint to get information about `specified_user` on behalf of `requesting_user`. @@ -108,7 +113,13 @@ class CourseListTestMixin(CourseApiTestMixin): request = Request(self.request_factory.get('/')) request.user = requesting_user with check_mongo_calls(0): - return list_courses(request, specified_user.username, org=org, filter_=filter_) + return list_courses( + request, + specified_user.username, + org=org, + filter_=filter_, + permissions=permissions, + ) def verify_courses(self, courses): """ @@ -209,6 +220,28 @@ class TestGetCourseListMultipleCourses(CourseListTestMixin, ModuleStoreTestCase) assert {course.id for course in filtered_courses} == {course.id for course in expected_courses},\ f'testing course_api.api.list_courses with filter_={filter_}' + def test_permissions(self): + + # Create a second course to be filtered out of queries. + self.create_course(course='should-be-hidden-course') + + # Create instructor (non-staff), and enroll him in the course. + instructor_user = self.create_user('the-instructor', is_staff=False) + self.create_enrollment(user=instructor_user, course_id=self.course.id) + self.create_courseaccessrole( + user=instructor_user, + course_id=self.course.id, + role='instructor', + org='edX', + ) + + filtered_courses = self._make_api_call( + instructor_user, + instructor_user, + permissions={'instructor'}) + + self.assertEqual({c.id for c in filtered_courses}, {self.course.id}) + class TestGetCourseListExtras(CourseListTestMixin, ModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index 8eda36e97c..fe5db50637 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 'mobile': None, 'search_term': '', 'filter_': None, + 'permissions': set(), } def test_basic(self): diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 5f0a28ad27..e2cb2521c2 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -201,6 +201,36 @@ class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreT response = self.verify_response(params=params) assert {course['course_id'] for course in response.data['results']} == {str(course.id) for course in expected_courses}, f'testing course_api.views.CourseListView with filter_={filter_}' # pylint: disable=line-too-long + def test_get_when_no_permission_then_filters_correctly(self): + """ + Given a user that is instructor in a course + And another course he is not an instructor + When get + Then only the first course is returned + """ + + # Create a second course to be filtered out of queries. + self.create_course(course='should-be-hidden-course') + + # Create instructor (non-staff), and enroll him in the course. + instructor_user = self.create_user('the-instructor', is_staff=False) + self.setup_user(instructor_user) + self.create_enrollment(user=instructor_user, course_id=self.course.id) + self.create_courseaccessrole( + user=instructor_user, + course_id=self.course.id, + role='instructor', + org='edX', + ) + + params = {'permissions': 'instructor', + 'username': instructor_user.username} + response = self.verify_response(params=params) + + self.assertEqual(response.status_code, 200) + ids = {c['course_id'] for c in response.json()['results']} + self.assertEqual(ids, {str(self.course.id)}) + class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index a1e5ef845e..5e4239e6f1 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -271,6 +271,12 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): provided org code (e.g., "HarvardX") are returned. Case-insensitive. + permissions (optional): + If specified, it filters visible `CourseOverview` objects by + checking if each permission specified is granted for the username. + Notice that Staff users are always granted permission to list any + course. + **Returns** * 200 on success, with a list of course discovery objects as returned @@ -321,13 +327,13 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): form = CourseListGetForm(self.request.query_params, initial={'requesting_user': self.request.user}) if not form.is_valid(): raise ValidationError(form.errors) - return list_courses( self.request, form.cleaned_data['username'], org=form.cleaned_data['org'], filter_=form.cleaned_data['filter_'], - search_term=form.cleaned_data['search_term'] + search_term=form.cleaned_data['search_term'], + permissions=form.cleaned_data['permissions'] ) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 535293e1bb..e15566068e 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -702,10 +702,13 @@ def get_course_syllabus_section(course, section_key): @function_trace('get_courses') -def get_courses(user, org=None, filter_=None): +def get_courses(user, org=None, filter_=None, permissions=None): """ - Return a LazySequence of courses available, optionally filtered by org code (case-insensitive). + Return a LazySequence of courses available, optionally filtered by org code + (case-insensitive) or a set of permissions to be satisfied for the specified + user. """ + courses = branding.get_visible_courses( org=org, filter_=filter_, @@ -715,13 +718,15 @@ def get_courses(user, org=None, filter_=None): 'image_set' ) + permissions = set(permissions or '') permission_name = configuration_helpers.get_value( 'COURSE_CATALOG_VISIBILITY_PERMISSION', settings.COURSE_CATALOG_VISIBILITY_PERMISSION ) + permissions.add(permission_name) return LazySequence( - (c for c in courses if has_access(user, permission_name, c)), + (c for c in courses if all(has_access(user, p, c) for p in permissions)), est_len=courses.count() )