Implement both IP and email based rate limiting.

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)

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
         )
 

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"))