From a6a69224d1bf8d72d0d5120837ccd2ed37a68a44 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Tue, 7 Jul 2020 22:44:22 +0500 Subject: [PATCH] Ratelimit login_user endpoint. Ratelimited `login_user` endpoint using `django-ratelimit`, also decreased default value of logistration rate limit to 100 requests per five minutes per IP. PROD-1877 --- cms/envs/common.py | 2 +- cms/envs/test.py | 3 +++ .../instructor_task/tests/test_integration.py | 3 +++ lms/envs/common.py | 2 +- .../core/djangoapps/user_authn/views/login.py | 7 +++++ .../user_authn/views/tests/test_login.py | 27 +++++++++++++++++-- 6 files changed, 40 insertions(+), 4 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 2d977717a6..edbfb8b849 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2253,4 +2253,4 @@ DISABLE_DEPRECATED_SIGNIN_URL = False DISABLE_DEPRECATED_SIGNUP_URL = False ##### LOGISTRATION RATE LIMIT SETTINGS ##### -LOGISTRATION_RATELIMIT_RATE = '500/5m' +LOGISTRATION_RATELIMIT_RATE = '100/5m' diff --git a/cms/envs/test.py b/cms/envs/test.py index b0a0ca8ed9..ff22d4134a 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -298,3 +298,6 @@ SYSTEM_WIDE_ROLE_CLASSES = os.environ.get("SYSTEM_WIDE_ROLE_CLASSES", []) DEFAULT_MOBILE_AVAILABLE = True PROCTORING_SETTINGS = {} + +##### LOGISTRATION RATE LIMIT SETTINGS ##### +LOGISTRATION_RATELIMIT_RATE = '5/5m' diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index e9b3d9b2b4..487246c9ee 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -16,6 +16,7 @@ import ddt import six from celery.states import FAILURE, SUCCESS from django.contrib.auth.models import User +from django.test.utils import override_settings from django.urls import reverse from mock import patch from six import text_type @@ -70,6 +71,7 @@ class TestIntegrationTask(InstructorTaskModuleTestCase): @ddt.ddt +@override_settings(RATELIMIT_ENABLE=False) class TestRescoringTask(TestIntegrationTask): """ Integration-style tests for rescoring problems in a background task. @@ -432,6 +434,7 @@ class TestRescoringTask(TestIntegrationTask): self.check_state(user, descriptor, 0, 1, expected_attempts=2) +@override_settings(RATELIMIT_ENABLE=False) class TestResetAttemptsTask(TestIntegrationTask): """ Integration-style tests for resetting problem attempts in a background task. diff --git a/lms/envs/common.py b/lms/envs/common.py index 063bc0de92..38221127d8 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3775,7 +3775,7 @@ RATELIMIT_ENABLE = True RATELIMIT_RATE = '120/m' ##### LOGISTRATION RATE LIMIT SETTINGS ##### -LOGISTRATION_RATELIMIT_RATE = '500/5m' +LOGISTRATION_RATELIMIT_RATE = '100/5m' ############### Settings for Retirement ##################### RETIRED_USERNAME_PREFIX = 'retired__user_' diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 7d6068b311..d95fd2232e 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -24,6 +24,7 @@ from django.views.decorators.csrf import csrf_exempt, csrf_protect, ensure_csrf_ from django.views.decorators.debug import sensitive_post_parameters from django.views.decorators.http import require_http_methods from edx_django_utils.monitoring import set_custom_metric +from ratelimit.decorators import ratelimit from ratelimitbackend.exceptions import RateLimitException from rest_framework.views import APIView @@ -382,6 +383,12 @@ def finish_auth(request): # pylint: disable=unused-argument @ensure_csrf_cookie @require_http_methods(['POST']) +@ratelimit( + key='openedx.core.djangoapps.util.ratelimit.real_ip', + rate=settings.LOGISTRATION_RATELIMIT_RATE, + method='POST', + block=True +) def login_user(request): """ AJAX request to log in the user. diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 5948ad86ca..3feff222dc 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -18,6 +18,7 @@ from django.http import HttpResponse from django.test.client import Client from django.test.utils import override_settings from django.urls import NoReverseMatch, reverse +from freezegun import freeze_time from mock import patch from six.moves import range @@ -351,7 +352,8 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): self._assert_audit_log(mock_audit_log, 'info', [u'Logout']) self._assert_not_in_audit_log(mock_audit_log, 'info', [u'test']) - def test_login_ratelimited_success(self): + @override_settings(RATELIMIT_ENABLE=False) + def test_excessive_login_attempts_success(self): # Try (and fail) logging in with fewer attempts than the limit of 30 # and verify that you can still successfully log in afterwards. for i in range(20): @@ -362,7 +364,8 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): response, _audit_log = self._login_response(self.user_email, self.password) self._assert_response(response, success=True) - def test_login_ratelimited(self): + @override_settings(RATELIMIT_ENABLE=False) + def test_excessive_login_attempts(self): # try logging in 30 times, the default limit in the number of failed # login attempts in one 5 minute period before the rate gets limited for i in range(30): @@ -372,6 +375,26 @@ class LoginTest(SiteMixin, CacheIsolationTestCase): response, _audit_log = self._login_response(self.user_email, 'wrong_password') self._assert_response(response, success=False, value='Too many failed login attempts') + def test_login_ratelimited(self): + """ + Test that login endpoint is IP ratelimited and only allow 5 requests + per 5 minutes per IP. + """ + for i in range(5): + password = u'test_password{0}'.format(i) + response, _audit_log = self._login_response(self.user_email, password) + self._assert_response(response, success=False) + + response, _audit_log = self._login_response(self.user_email, self.password) + self.assertEqual(response.status_code, 403) + + # now reset the time to 6 min from now in future and verify that it will + # allow another request from same IP and user can successfully login + reset_time = datetime.datetime.utcnow() + datetime.timedelta(seconds=361) + with freeze_time(reset_time): + response, _audit_log = self._login_response(self.user_email, self.password) + self._assert_response(response, success=True) + @patch.dict("django.conf.settings.FEATURES", {"DISABLE_SET_JWT_COOKIES_FOR_TESTS": False}) def test_login_refresh(self): def _assert_jwt_cookie_present(response):