Merge pull request #25088 from edx/pwnage101/course-id-list-view-bypass-has-access

Bypass calls to has_access() when listing courses

lms/djangoapps/course_api/api.py

@@ -18,8 +18,10 @@ from lms.djangoapps.courseware.courses import (
     get_courses,
     get_permission_for_course_about
 )
+from opaque_keys.edx.django.models import CourseKeyField
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 from openedx.core.lib.api.view_utils import LazySequence
+from student.models import CourseAccessRole
 from student.roles import GlobalStaff
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError
@@ -170,19 +172,43 @@ def list_course_keys(request, username, role):
     """
     user = get_effective_user(request.user, username)
 
-    course_keys = CourseOverview.get_all_course_keys()
+    all_course_keys = CourseOverview.get_all_course_keys()
 
     # Global staff have access to all courses. Filter courses for non-global staff.
     if GlobalStaff().has_user(user):
-        return course_keys
+        return all_course_keys
 
-    return LazySequence(
+    if role == 'staff':
-        (
+        # This short-circuit implementation bypasses has_access() which we think is too slow for some users when
-            course_key for course_key in course_keys
+        # evaluating staff-level course access for Insights.  Various tickets have context on this issue: CR-2487,
-            if has_access(user, role, course_key)
+        # TNL-7448, DESUPPORT-416, and probably more.
-        ),
+        #
-        est_len=len(course_keys)
+        # This is a simplified implementation that does not consider org-level access grants (e.g. when course_id is
-    )
+        # empty).
+        filtered_course_keys = (
+            CourseAccessRole.objects.filter(
+                user=user,
+                # Having the instructor role implies staff access.
+                role__in=['staff', 'instructor'],
+            )
+            # We need to check against CourseOverview so that we don't return any Libraries.
+            .extra(tables=['course_overviews_courseoverview'], where=['course_id = course_overviews_courseoverview.id'])
+            # For good measure, make sure we don't return empty course IDs.
+            .exclude(course_id=CourseKeyField.Empty)
+            .order_by('course_id')
+            .values_list('course_id', flat=True)
+            .distinct()
+        )
+    else:
+        # This is the original implementation which still covers the case where role = "instructor":
+        filtered_course_keys = LazySequence(
+            (
+                course_key for course_key in all_course_keys
+                if has_access(user, role, course_key)
+            ),
+            est_len=len(all_course_keys)
+        )
+    return filtered_course_keys
 
 
 def get_due_dates(request, course_key, user):

lms/djangoapps/course_api/tests/test_views.py

@@ -22,6 +22,7 @@ from waffle.testutils import override_switch
 
 from course_modes.models import CourseMode
 from course_modes.tests.factories import CourseModeFactory
+from opaque_keys.edx.locator import LibraryLocator
 from openedx.core.lib.api.view_utils import LazySequence
 from openedx.features.content_type_gating.models import ContentTypeGatingConfig
 from openedx.features.course_duration_limits.models import CourseDurationLimitConfig
@@ -459,9 +460,15 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase):
         add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user)
 
         # Create a second course, along with an instructor user for it.
-        alternate_course = self.create_course(org='test')
+        alternate_course1 = self.create_course(org='test1')
         course_instructor_user = self.create_user(username='course_instructor', is_staff=False)
-        add_users(self.global_admin, CourseInstructorRole(alternate_course.id), course_instructor_user)
+        add_users(self.global_admin, CourseInstructorRole(alternate_course1.id), course_instructor_user)
+
+        # Create a third course, along with an user that has both staff and instructor for it.
+        alternate_course2 = self.create_course(org='test2')
+        course_instructor_staff_user = self.create_user(username='course_instructor_staff', is_staff=False)
+        add_users(self.global_admin, CourseInstructorRole(alternate_course2.id), course_instructor_staff_user)
+        add_users(self.global_admin, CourseStaffRole(alternate_course2.id), course_instructor_staff_user)
 
         # Requesting the courses for which the course staff user is staff should return *only* the single course.
         self.setup_user(self.staff_user)
@@ -485,7 +492,7 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase):
             'role': 'instructor'
         })
         self.assertEqual(len(filtered_response.data['results']), 1)
-        self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org))
+        self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course1.org))
 
         # The course instructor user has the inferred course staff role on one course.
         self.setup_user(course_instructor_user)
@@ -494,7 +501,38 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase):
             'role': 'staff'
         })
         self.assertEqual(len(filtered_response.data['results']), 1)
-        self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org))
+        self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course1.org))
+
+        # The user with both instructor AND staff on a course has the inferred course staff role on that one course.
+        self.setup_user(course_instructor_staff_user)
+        filtered_response = self.verify_response(params={
+            'username': course_instructor_staff_user.username,
+            'role': 'staff'
+        })
+        self.assertEqual(len(filtered_response.data['results']), 1)
+        self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course2.org))
+
+    def test_no_libraries(self):
+        """
+        Verify that only Course IDs are returned, not anything else like libraries.
+        """
+        # Make this user a course staff user for a course, AND a library.
+        course_staff_user = self.create_user(username='course_staff', is_staff=False)
+        add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user)
+        add_users(
+            self.global_admin,
+            CourseStaffRole(LibraryLocator.from_string('library-v1:library_org+library_name')),
+            course_staff_user,
+        )
+
+        # Requesting the courses should return *only* courses and not libraries.
+        self.setup_user(self.staff_user)
+        filtered_response = self.verify_response(params={
+            'username': course_staff_user.username,
+            'role': 'staff'
+        })
+        self.assertEqual(len(filtered_response.data['results']), 1)
+        self.assertTrue(filtered_response.data['results'][0].startswith(self.course.org))
 
 
 class LazyPageNumberPaginationTestCase(TestCase):