diff --git a/common/djangoapps/util/request_rate_limiter.py b/common/djangoapps/util/request_rate_limiter.py index cb292ea1b2..78dc32e78f 100644 --- a/common/djangoapps/util/request_rate_limiter.py +++ b/common/djangoapps/util/request_rate_limiter.py @@ -48,7 +48,7 @@ class PasswordResetEmailRateLimiter(RequestRateLimiter): """ return '%s-%s-%s' % ( self.reset_email_cache_prefix, - self.get_ip(request), + self.get_email(request), dt.strftime('%Y%m%d%H%M'), ) @@ -57,3 +57,12 @@ class PasswordResetEmailRateLimiter(RequestRateLimiter): Returns timeout for cache keys. """ return self.cache_timeout_seconds + + def get_email(self, request): + """ + Returns email id for cache key. + """ + user = request.user + # Prefer logged-in user's email + email = user.email if user.is_authenticated else request.POST.get('email') + return email diff --git a/openedx/core/djangoapps/user_authn/views/password_reset.py b/openedx/core/djangoapps/user_authn/views/password_reset.py index 9715db6358..a6af532ecf 100644 --- a/openedx/core/djangoapps/user_authn/views/password_reset.py +++ b/openedx/core/djangoapps/user_authn/views/password_reset.py @@ -43,7 +43,7 @@ from student.forms import send_account_recovery_email_for_user from student.models import AccountRecovery from util.json_request import JsonResponse from util.password_policy_validators import normalize_password, validate_password -from util.request_rate_limiter import BadRequestRateLimiter, PasswordResetEmailRateLimiter +from util.request_rate_limiter import PasswordResetEmailRateLimiter SETTING_CHANGE_INITIATED = 'edx.user.settings.change_initiated' @@ -242,11 +242,15 @@ def password_reset(request): """ Attempts to send a password reset e-mail. """ - # Add some rate limiting here by re-using the RateLimitMixin as a helper class - limiter = BadRequestRateLimiter() - if limiter.is_rate_limit_exceeded(request): - AUDIT_LOG.warning("Rate limit exceeded in password_reset") - return HttpResponseForbidden() + + password_reset_email_limiter = PasswordResetEmailRateLimiter() + + 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."), + status=403 + ) form = PasswordResetFormNoActive(request.POST) if form.is_valid(): @@ -269,7 +273,7 @@ def password_reset(request): else: # bad user? tick the rate limiter counter AUDIT_LOG.info("Bad password_reset user passed in.") - limiter.tick_request_counter(request) + password_reset_email_limiter.tick_request_counter(request) return JsonResponse({ 'success': True, 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 3ea7d3c2a9..656d4c8564 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 @@ -85,6 +85,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): """ bad_pwd_req = self.request_factory.post('/password_reset/', {'email': self.user_bad_passwd.email}) + bad_pwd_req.user = AnonymousUser() bad_pwd_resp = password_reset(bad_pwd_req) # If they've got an unusable password, we return a successful response code self.assertEqual(bad_pwd_resp.status_code, 200) @@ -105,6 +106,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): """ bad_email_req = self.request_factory.post('/password_reset/', {'email': self.user.email + "makeItFail"}) + bad_email_req.user = AnonymousUser() bad_email_resp = password_reset(bad_email_req) # Note: even if the email is bad, we return a successful response code # This prevents someone potentially trying to "brute-force" find out which @@ -123,20 +125,17 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) def test_password_reset_ratelimited(self): """ - Try (and fail) resetting password 30 times in a row on an non-existant email address + Test that reset password endpoint only allow one request per minute. """ cache.clear() - for i in range(30): - good_req = self.request_factory.post('/password_reset/', { - 'email': 'thisdoesnotexist{0}@foo.com'.format(i) - }) - good_resp = password_reset(good_req) - self.assertEqual(good_resp.status_code, 200) + password_reset_req = self.request_factory.post('/password_reset/', {'email': 'thisdoesnotexist@foo.com'}) + password_reset_req.user = AnonymousUser() + good_resp = password_reset(password_reset_req) + self.assertEqual(good_resp.status_code, 200) # then the rate limiter should kick in and give a HttpForbidden response - bad_req = self.request_factory.post('/password_reset/', {'email': 'thisdoesnotexist@foo.com'}) - bad_resp = password_reset(bad_req) + bad_resp = password_reset(password_reset_req) self.assertEqual(bad_resp.status_code, 403) self.assert_no_events_were_emitted()