From 05d18effdecc48544c2e70bcfde7453111d417b6 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Thu, 30 Apr 2020 20:23:12 +0500 Subject: [PATCH] Implement both IP and email based rate limiting. --- .../djangoapps/util/request_rate_limiter.py | 37 ++++++++++++- .../user_authn/views/password_reset.py | 7 ++- .../views/tests/test_reset_password.py | 52 ++++++++++++++++--- 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/util/request_rate_limiter.py b/common/djangoapps/util/request_rate_limiter.py index 78dc32e78f..195117cbc3 100644 --- a/common/djangoapps/util/request_rate_limiter.py +++ b/common/djangoapps/util/request_rate_limiter.py @@ -3,6 +3,7 @@ A utility class which wraps the RateLimitMixin 3rd party class to do bad request which can be used for rate limiting """ +from datetime import datetime, timedelta from django.conf import settings from ratelimitbackend.backends import RateLimitMixin @@ -44,7 +45,17 @@ class PasswordResetEmailRateLimiter(RequestRateLimiter): def key(self, request, dt): """ - Returns cache key. + Returns IP based cache key. + """ + return '%s-%s-%s' % ( + self.reset_email_cache_prefix, + self.get_ip(request), + dt.strftime('%Y%m%d%H%M'), + ) + + def email_key(self, request, dt): + """ + Returns email based cache key. """ return '%s-%s-%s' % ( self.reset_email_cache_prefix, @@ -66,3 +77,27 @@ class PasswordResetEmailRateLimiter(RequestRateLimiter): # Prefer logged-in user's email email = user.email if user.is_authenticated else request.POST.get('email') return email + + def keys_to_check(self, request): + """ + Retun list of IP and email based keys. + """ + keys = super(PasswordResetEmailRateLimiter, self).keys_to_check(request) + + now = datetime.now() + email_keys = [ + self.email_key( + request, + now - timedelta(minutes=minute), + ) for minute in range(self.minutes + 1) + ] + keys.extend(email_keys) + + return keys + + def tick_request_counter(self, request): + """ + Ticks any counters used to compute when rate limt has been reached + """ + for key in self.keys_to_check(request): + self.cache_incr(key) diff --git a/openedx/core/djangoapps/user_authn/views/password_reset.py b/openedx/core/djangoapps/user_authn/views/password_reset.py index 7aca85d1aa..066ee2f47b 100644 --- a/openedx/core/djangoapps/user_authn/views/password_reset.py +++ b/openedx/core/djangoapps/user_authn/views/password_reset.py @@ -247,8 +247,11 @@ def password_reset(request): if password_reset_email_limiter.is_rate_limit_exceeded(request): AUDIT_LOG.warning("Password reset rate limit exceeded") - return HttpResponse( - _("Your previous request is in progress, please try again in a few moments."), + return JsonResponse( + { + 'success': False, + 'value': _("Your previous request is in progress, please try again in a few moments.") + }, status=403 ) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py index c477b84184..5d7bb78539 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py @@ -123,16 +123,33 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): 'openedx.core.djangoapps.user_authn.views.password_reset.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True) ) - @ddt.data(True, False) - def test_password_reset_ratelimited(self, existing_user): + def test_password_reset_ratelimited_for_non_existing_user(self): """ - Test that reset password endpoint only allow one request per minute for both - existing and non-existing users. + Test that reset password endpoint only allow one request per minute + for non-existing user. + """ + self.assert_password_reset_ratelimitted('thisdoesnotexist@foo.com', AnonymousUser()) + self.assert_no_events_were_emitted() + + @patch( + 'openedx.core.djangoapps.user_authn.views.password_reset.render_to_string', + Mock(side_effect=mock_render_to_string, autospec=True) + ) + def test_password_reset_ratelimited_for_existing_user(self): + """ + Test that reset password endpoint only allow one request per minute + for existing user. + """ + self.assert_password_reset_ratelimitted(self.user.email, self.user) + self.assert_event_emission_count(SETTING_CHANGE_INITIATED, 1) + + def assert_password_reset_ratelimitted(self, email, user): + """ + Assert that password reset endpoint allow one request per minute per email. """ cache.clear() - email = self.user.email if existing_user else 'thisdoesnotexist@foo.com' password_reset_req = self.request_factory.post('/password_reset/', {'email': email}) - password_reset_req.user = self.user if existing_user else AnonymousUser() + password_reset_req.user = user password_reset_req.site = Mock(domain='example.com') good_resp = password_reset(password_reset_req) self.assertEqual(good_resp.status_code, 200) @@ -143,6 +160,29 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): cache.clear() + @patch( + 'openedx.core.djangoapps.user_authn.views.password_reset.render_to_string', + Mock(side_effect=mock_render_to_string, autospec=True) + ) + def test_ratelimitted_from_same_ip_with_different_email(self): + """ + Test that password reset endpoint allow one request per minute per IP. + """ + cache.clear() + good_req = self.request_factory.post('/password_reset/', {'email': 'thisdoesnotexist@foo.com'}) + good_req.user = AnonymousUser() + good_resp = password_reset(good_req) + self.assertEqual(good_resp.status_code, 200) + + # change the email ID and verify that the rate limiter should kick in and + # give a Forbidden response if the request is from same IP. + bad_req = self.request_factory.post('/password_reset/', {'email': 'thisdoesnotexist2@foo.com'}) + bad_req.user = AnonymousUser() + bad_resp = password_reset(bad_req) + self.assertEqual(bad_resp.status_code, 403) + + cache.clear() + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") @ddt.data(('plain_text', "You're receiving this e-mail because you requested a password reset"), ('html', "You're receiving this e-mail because you requested a password reset"))