diff --git a/lms/djangoapps/course_api/__init__.py b/lms/djangoapps/course_api/__init__.py index 263b2b7830..3e9d86230f 100644 --- a/lms/djangoapps/course_api/__init__.py +++ b/lms/djangoapps/course_api/__init__.py @@ -1 +1,7 @@ """ Course API """ +from openedx.core.djangoapps.waffle_utils import (WaffleSwitch, WaffleSwitchNamespace) + +WAFFLE_SWITCH_NAMESPACE = WaffleSwitchNamespace(name='course_list_api_rate_limit') + +USE_RATE_LIMIT_2_FOR_COURSE_LIST_API = WaffleSwitch(WAFFLE_SWITCH_NAMESPACE, 'rate_limit_2') +USE_RATE_LIMIT_10_FOR_COURSE_LIST_API = WaffleSwitch(WAFFLE_SWITCH_NAMESPACE, 'rate_limit_10') diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index ad243c1a5a..2f545521ca 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -1,8 +1,10 @@ """ Tests for Course API views. """ +import ddt from hashlib import md5 +from django.core.exceptions import ImproperlyConfigured from django.urls import reverse from django.test import RequestFactory from django.test.utils import override_settings @@ -12,8 +14,9 @@ from search.tests.tests import TEST_INDEX_NAME from search.tests.utils import SearcherMixin from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase +from waffle.testutils import override_switch -from ..views import CourseDetailView +from ..views import CourseDetailView, CourseListUserThrottle from .mixins import TEST_PASSWORD, CourseApiFactoryMixin @@ -53,6 +56,7 @@ class CourseApiTestViewMixin(CourseApiFactoryMixin): @attr(shard=9) +@ddt.ddt class CourseListViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): """ Test responses returned from CourseListView. @@ -100,6 +104,38 @@ class CourseListViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): self.client.logout() self.verify_response() + def assert_throttle_configured_correctly(self, user_scope, throws_exception, expected_rate): + """Helper to determine throttle configuration is correctly set.""" + throttle = CourseListUserThrottle() + throttle.check_for_switches() + throttle.scope = user_scope + try: + rate_limit, __ = throttle.parse_rate(throttle.get_rate()) + self.assertEqual(rate_limit, expected_rate) + self.assertFalse(throws_exception) + except ImproperlyConfigured: + self.assertTrue(throws_exception) + + @ddt.data(('staff', False, 40), ('user', False, 20), ('unknown', True, None)) + @ddt.unpack + def test_throttle_rate_default(self, user_scope, throws_exception, expected_rate): + """ Make sure throttle rate default is set correctly for different user scopes. """ + self.assert_throttle_configured_correctly(user_scope, throws_exception, expected_rate) + + @ddt.data(('staff', False, 10), ('user', False, 2), ('unknown', True, None)) + @ddt.unpack + @override_switch('course_list_api_rate_limit.rate_limit_2', active=True) + def test_throttle_rate_2(self, user_scope, throws_exception, expected_rate): + """ Make sure throttle rate 2 is set correctly for different user scopes. """ + self.assert_throttle_configured_correctly(user_scope, throws_exception, expected_rate) + + @ddt.data(('staff', False, 20), ('user', False, 10), ('unknown', True, None)) + @ddt.unpack + @override_switch('course_list_api_rate_limit.rate_limit_10', active=True) + def test_throttle_rate_20(self, user_scope, throws_exception, expected_rate): + """ Make sure throttle rate 20 is set correctly for different user scopes. """ + self.assert_throttle_configured_correctly(user_scope, throws_exception, expected_rate) + @attr(shard=9) class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreTestCase): diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 0caeeed376..09a568795c 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -6,10 +6,12 @@ import search from django.conf import settings from django.core.exceptions import ValidationError from rest_framework.generics import ListAPIView, RetrieveAPIView +from rest_framework.throttling import UserRateThrottle from edx_rest_framework_extensions.paginators import NamespacedPageNumberPagination 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 from .forms import CourseDetailGetForm, CourseListGetForm from .serializers import CourseDetailSerializer, CourseSerializer @@ -123,6 +125,41 @@ class CourseDetailView(DeveloperErrorViewMixin, RetrieveAPIView): ) +class CourseListUserThrottle(UserRateThrottle): + """Limit the number of requests users can make to the course list API.""" + # The course list endpoint is likely being inefficient with how it's querying + # various parts of the code and can take courseware down, it needs to be rate + # limited until optimized. LEARNER-5527 + + THROTTLE_RATES = { + 'user': '20/minute', + 'staff': '40/minute', + } + + def check_for_switches(self): + if USE_RATE_LIMIT_2_FOR_COURSE_LIST_API.is_enabled(): + self.THROTTLE_RATES = { + 'user': '2/minute', + 'staff': '10/minute', + } + elif USE_RATE_LIMIT_10_FOR_COURSE_LIST_API.is_enabled(): + self.THROTTLE_RATES = { + 'user': '10/minute', + 'staff': '20/minute', + } + + def allow_request(self, request, view): + self.check_for_switches() + # Use a special scope for staff to allow for a separate throttle rate + user = request.user + if user.is_authenticated and (user.is_staff or user.is_superuser): + self.scope = 'staff' + self.rate = self.get_rate() + self.num_requests, self.duration = self.parse_rate(self.rate) + + return super(CourseListUserThrottle, self).allow_request(request, view) + + @view_auth_classes(is_authenticated=False) class CourseListView(DeveloperErrorViewMixin, ListAPIView): """ @@ -196,6 +233,7 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): pagination_class = NamespacedPageNumberPagination 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+: