Ratelimit password reset by email instead of IP.
Also changed `password_reset` endpoint rate limit configuration to 1/minute from 30/5 minutes. PROD-1427
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user