From 780c1d982d417d6a4e7b46612f24b630157e82f2 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Tue, 12 Jun 2018 23:12:13 +0500 Subject: [PATCH] Rate limit course list API. This 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 --- lms/djangoapps/course_api/__init__.py | 6 +++ lms/djangoapps/course_api/tests/test_views.py | 38 ++++++++++++++++++- lms/djangoapps/course_api/views.py | 38 +++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) 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+: