From e3cd6d55c405b09cb83ccc2e4d28731d84554837 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Thu, 24 Sep 2020 17:08:24 -0400 Subject: [PATCH 1/3] Bypass calls to has_access() when listing courses When listing courses for which a user has "staff" level access, bypass calls to has_access() and use a hopefully more performant and simpler access pattern that uses only a single ORM query and far less processing per course. Caution: this short-circuit implementation does not handle org-level access grants. --- lms/djangoapps/course_api/api.py | 41 +++++++++++++++---- lms/djangoapps/course_api/tests/test_views.py | 23 +++++++++-- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 349c05a5ba..b97881f409 100644 --- a/lms/djangoapps/course_api/api.py +++ b/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,40 @@ 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( - ( - course_key for course_key in course_keys - if has_access(user, role, course_key) - ), - est_len=len(course_keys) - ) + if role == 'staff': + # This short-circuit implementation bypasses has_access() which we think is too slow for some users when + # evaluating staff-level course access for Insights. Various tickets have context on this issue: CR-2487, + # TNL-7448, DESUPPORT-416, and probably more. + # + # This is a simplified implementation that does not consider org-level access grants (e.g. when course_id is + # blank). + filtered_course_keys = ( + CourseAccessRole.objects.filter( + user=user, + # Having the instructor role implies staff access. This fact was reverse-engineered from unit tests. + role__in=['staff', 'instructor'], + ) + .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): diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index efdd4678ad..5dfb489df2 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -459,9 +459,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 +491,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 +500,16 @@ 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)) class LazyPageNumberPaginationTestCase(TestCase): From 83a0327b929c72e288ab49af0ea7cf6a367cad32 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Fri, 25 Sep 2020 11:51:47 -0400 Subject: [PATCH 2/3] Also exclude libraries from the list_course_keys API --- lms/djangoapps/course_api/api.py | 4 +++- lms/djangoapps/course_api/tests/test_views.py | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index b97881f409..20c20a3802 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -188,8 +188,10 @@ def list_course_keys(request, username, role): filtered_course_keys = ( CourseAccessRole.objects.filter( user=user, - # Having the instructor role implies staff access. This fact was reverse-engineered from unit tests. + # 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. + course_id__in=CourseOverview.objects.all(), ) .exclude(course_id=CourseKeyField.Empty) .order_by('course_id') diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 5dfb489df2..8378481caf 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/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 @@ -511,6 +512,28 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): 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): From dded86d2aee004a727b17cecd8eb464ab84a73dc Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Fri, 25 Sep 2020 12:54:37 -0400 Subject: [PATCH 3/3] Change subquery to JOIN in list_course_keys API --- lms/djangoapps/course_api/api.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 20c20a3802..86aa3c9d39 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -184,15 +184,16 @@ def list_course_keys(request, username, role): # TNL-7448, DESUPPORT-416, and probably more. # # This is a simplified implementation that does not consider org-level access grants (e.g. when course_id is - # blank). + # 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. - course_id__in=CourseOverview.objects.all(), ) + # 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)