From 803a6c742c49b4d743fc417d3d67ba010213767a Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Tue, 12 Jun 2018 01:04:45 +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/tests/test_views.py | 12 +++++++++- lms/djangoapps/course_api/views.py | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index ad243c1a5a..f7b32550aa 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -7,13 +7,14 @@ from django.urls import reverse from django.test import RequestFactory from django.test.utils import override_settings from nose.plugins.attrib import attr +from rest_framework import status from search.tests.test_course_discovery import DemoCourse from search.tests.tests import TEST_INDEX_NAME from search.tests.utils import SearcherMixin from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase -from ..views import CourseDetailView +from ..views import CourseDetailView, CourseListUserThrottle from .mixins import TEST_PASSWORD, CourseApiFactoryMixin @@ -100,6 +101,15 @@ class CourseListViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): self.client.logout() self.verify_response() + def test_throttle_for_user(self): + """Make sure a user requests do not exceed the maximum number of requests""" + throttle = CourseListUserThrottle() + rate_limit, __ = throttle.parse_rate(throttle.rate) + self.setup_user(self.honor_user) + for attempt in xrange(rate_limit + 2): + expected_status = status.HTTP_429_TOO_MANY_REQUESTS if attempt >= rate_limit else status.HTTP_200_OK + self.verify_response(expected_status_code=expected_status, params={'username': self.honor_user.username}) + @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..9eb75bdfae 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -6,6 +6,7 @@ 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 @@ -123,6 +124,28 @@ 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': '2/minute', + 'staff': '10/minute', + } + + def allow_request(self, request, view): + # 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 +219,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+: