diff --git a/cms/envs/bok_choy.yml b/cms/envs/bok_choy.yml
index c6c90bddf88d9b203847bfcd3a63b4cf73e840f8..6aa54f0b849b4ef30696db1d799c8aa3c7ef7ebe 100644
--- a/cms/envs/bok_choy.yml
+++ b/cms/envs/bok_choy.yml
@@ -112,6 +112,9 @@ MODULESTORE:
- ENGINE: xmodule.modulestore.xml.XMLModuleStore
NAME: xml
OPTIONS: {data_dir: '** OVERRIDDEN **', default_class: xmodule.hidden_module.HiddenDescriptor}
+# We need to test different scenarios, following setting effectively disbale rate limiting
+PASSWORD_RESET_IP_RATE: '1/s'
+PASSWORD_RESET_EMAIL_RATE: '1/s'
SECRET_KEY: ''
SERVER_EMAIL: devops@example.com
SESSION_COOKIE_DOMAIN: null
diff --git a/cms/envs/common.py b/cms/envs/common.py
index edbfb8b849b13abbe0480dd973d78694a38da1a3..11dc5dd9cc22c4566fe1529840c82f198264289e 100644
--- a/cms/envs/common.py
+++ b/cms/envs/common.py
@@ -2254,3 +2254,7 @@ DISABLE_DEPRECATED_SIGNUP_URL = False
##### LOGISTRATION RATE LIMIT SETTINGS #####
LOGISTRATION_RATELIMIT_RATE = '100/5m'
+
+##### PASSWORD RESET RATE LIMIT SETTINGS #####
+PASSWORD_RESET_IP_RATE = '1/m'
+PASSWORD_RESET_EMAIL_RATE = '2/h'
diff --git a/common/djangoapps/util/request_rate_limiter.py b/common/djangoapps/util/request_rate_limiter.py
index 384ac28276935d6742c62a4b594484e42bc57383..015890963d815c37385b33a541e6779f37c31605 100644
--- a/common/djangoapps/util/request_rate_limiter.py
+++ b/common/djangoapps/util/request_rate_limiter.py
@@ -3,9 +3,6 @@ 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
@@ -32,72 +29,3 @@ class BadRequestRateLimiter(RequestRateLimiter):
Default rate limit is 30 requests for every 5 minutes.
"""
pass
-
-
-class PasswordResetEmailRateLimiter(RequestRateLimiter):
- """
- Rate limiting requests to send password reset emails.
- """
- email_rate_limit = getattr(settings, 'PASSWORD_RESET_EMAIL_RATE_LIMIT', {})
- requests = email_rate_limit.get('no_of_emails', 1)
- cache_timeout_seconds = email_rate_limit.get('per_seconds', 60)
- reset_email_cache_prefix = 'resetemail'
-
- def key(self, request, dt):
- """
- 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,
- self.get_email(request),
- dt.strftime('%Y%m%d%H%M'),
- )
-
- def expire_after(self):
- """
- 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
-
- def keys_to_check(self, request):
- """
- Return 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 limit has been reached.
- """
- for key in self.keys_to_check(request):
- self.cache_incr(key)
diff --git a/lms/envs/bok_choy.yml b/lms/envs/bok_choy.yml
index 9e555ebaaac9547eb7f64668f2e8094a49ccc707..6feab78ef8cecc42e48b9dd80ff17c05ee13babb 100644
--- a/lms/envs/bok_choy.yml
+++ b/lms/envs/bok_choy.yml
@@ -211,9 +211,8 @@ MODULESTORE:
NAME: xml
OPTIONS: {data_dir: '** OVERRIDDEN **', default_class: xmodule.hidden_module.HiddenDescriptor}
# We need to test different scenarios, following setting effectively disbale rate limiting
-PASSWORD_RESET_EMAIL_RATE_LIMIT:
- no_of_emails: 1
- per_seconds: 1
+PASSWORD_RESET_IP_RATE: '1/s'
+PASSWORD_RESET_EMAIL_RATE: '1/s'
PASSWORD_RESET_SUPPORT_LINK: https://support.example.com/password-reset-help.html
REGISTRATION_EXTENSION_FORM: openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm
REGISTRATION_EXTRA_FIELDS: {city: hidden, country: required, gender: optional, goals: optional,
@@ -237,7 +236,7 @@ TECH_SUPPORT_EMAIL: technical@example.com
THIRD_PARTY_AUTH_BACKENDS: [social_core.backends.google.GoogleOAuth2, social_core.backends.linkedin.LinkedinOAuth2,
social_core.backends.facebook.FacebookOAuth2, third_party_auth.dummy.DummyBackend,
third_party_auth.saml.SAMLAuthBackend]
-THIRD_PARTY_AUTH:
+THIRD_PARTY_AUTH:
Google:
SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "test"
SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "test"
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 38221127d86d1f35a78d3e7dac756fe40b1916c6..6792b1398f6f9fbc07de8d28513c00d05fb1e9c4 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -531,10 +531,6 @@ SOFTWARE_SECURE_REQUEST_RETRY_DELAY = 60 * 60
# Maximum of 6 retries before giving up.
SOFTWARE_SECURE_RETRY_MAX_ATTEMPTS = 6
-PASSWORD_RESET_EMAIL_RATE_LIMIT = {
- 'no_of_emails': 1,
- 'per_seconds': 60
-}
RETRY_CALENDAR_SYNC_EMAIL_MAX_ATTEMPTS = 5
# Deadline message configurations
COURSE_MESSAGE_ALERT_DURATION_IN_DAYS = 14
@@ -3777,6 +3773,10 @@ RATELIMIT_RATE = '120/m'
##### LOGISTRATION RATE LIMIT SETTINGS #####
LOGISTRATION_RATELIMIT_RATE = '100/5m'
+##### PASSWORD RESET RATE LIMIT SETTINGS #####
+PASSWORD_RESET_IP_RATE = '1/m'
+PASSWORD_RESET_EMAIL_RATE = '2/h'
+
############### Settings for Retirement #####################
RETIRED_USERNAME_PREFIX = 'retired__user_'
RETIRED_EMAIL_PREFIX = 'retired__user_'
diff --git a/openedx/core/djangoapps/user_authn/views/password_reset.py b/openedx/core/djangoapps/user_authn/views/password_reset.py
index 066ee2f47bda70ebc573cdb336f27c5a250a0ef8..17d5276b8aee1e7115c4b4498a162d6108f46ac4 100644
--- a/openedx/core/djangoapps/user_authn/views/password_reset.py
+++ b/openedx/core/djangoapps/user_authn/views/password_reset.py
@@ -24,6 +24,7 @@ from django.views.decorators.http import require_POST
from edx_ace import ace
from edx_ace.recipient import Recipient
from eventtracking import tracker
+from ratelimit.decorators import ratelimit
from rest_framework.views import APIView
from edxmako.shortcuts import render_to_string
@@ -43,8 +44,9 @@ 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 PasswordResetEmailRateLimiter
+POST_EMAIL_KEY = 'post:email'
+REAL_IP_KEY = 'openedx.core.djangoapps.util.ratelimit.real_ip'
SETTING_CHANGE_INITIATED = 'edx.user.settings.change_initiated'
# Maintaining this naming for backwards compatibility.
@@ -238,15 +240,19 @@ def request_password_change(email, is_secure):
@csrf_exempt
@require_POST
+@ratelimit(key=POST_EMAIL_KEY, rate=settings.PASSWORD_RESET_EMAIL_RATE)
+@ratelimit(key=REAL_IP_KEY, rate=settings.PASSWORD_RESET_IP_RATE)
def password_reset(request):
"""
Attempts to send a password reset e-mail.
"""
+ user = request.user
+ # Prefer logged-in user's email
+ email = user.email if user.is_authenticated else request.POST.get('email')
+ AUDIT_LOG.info("Password reset initiated for email %s.", email)
- password_reset_email_limiter = PasswordResetEmailRateLimiter()
-
- if password_reset_email_limiter.is_rate_limit_exceeded(request):
- AUDIT_LOG.warning("Password reset rate limit exceeded")
+ if getattr(request, 'limited', False):
+ AUDIT_LOG.warning("Password reset rate limit exceeded for email %s.", email)
return JsonResponse(
{
'success': False,
@@ -277,8 +283,6 @@ def password_reset(request):
# bad user? tick the rate limiter counter
AUDIT_LOG.info("Bad password_reset user passed in.")
- password_reset_email_limiter.tick_request_counter(request)
-
return JsonResponse({
'success': True,
'value': render_to_string('registration/password_reset_done.html', {}),
@@ -522,6 +526,8 @@ def _get_user_from_email(email):
@require_POST
+@ratelimit(key=POST_EMAIL_KEY, rate=settings.PASSWORD_RESET_EMAIL_RATE)
+@ratelimit(key=REAL_IP_KEY, rate=settings.PASSWORD_RESET_IP_RATE)
def password_change_request_handler(request):
"""Handle password change requests originating from the account page.
@@ -546,20 +552,18 @@ def password_change_request_handler(request):
POST /account/password
"""
+ user = request.user
+ # Prefer logged-in user's email
+ email = user.email if user.is_authenticated else request.POST.get('email')
+ AUDIT_LOG.info("Password reset initiated for user %s.", email)
- password_reset_email_limiter = PasswordResetEmailRateLimiter()
-
- if password_reset_email_limiter.is_rate_limit_exceeded(request):
- AUDIT_LOG.warning("Password reset rate limit exceeded")
+ if getattr(request, 'limited', False):
+ AUDIT_LOG.warning("Password reset rate limit exceeded for email %s.", email)
return HttpResponse(
_("Your previous request is in progress, please try again in a few moments."),
status=403
)
- user = request.user
- # Prefer logged-in user's email
- email = user.email if user.is_authenticated else request.POST.get('email')
-
if email:
try:
request_password_change(email, request.is_secure())
@@ -591,7 +595,6 @@ def password_change_request_handler(request):
.format(email=email, error=err))
return HttpResponse(_("Some error occured during password change. Please try again"), status=500)
- password_reset_email_limiter.tick_request_counter(request)
return HttpResponse(status=200)
else:
return HttpResponseBadRequest(_("No email address provided."))
diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_password.py
index 5bee8f52691b5e76b9e1473c7cbc2bc57fd33d39..f1c9666dc6a3982ae25d0ea6105582efb43f0b3e 100644
--- a/openedx/core/djangoapps/user_authn/views/tests/test_password.py
+++ b/openedx/core/djangoapps/user_authn/views/tests/test_password.py
@@ -5,17 +5,21 @@ Tests for user authorization password-related functionality.
import json
import logging
import re
+from datetime import datetime, timedelta
import ddt
from django.conf import settings
from django.contrib.auth import get_user_model
from django.core import mail
+from django.core.cache import cache
from django.test import TestCase
from django.test.client import RequestFactory
from django.urls import reverse
+from freezegun import freeze_time
from mock import Mock, patch
from oauth2_provider.models import AccessToken as dot_access_token
from oauth2_provider.models import RefreshToken as dot_refresh_token
+from pytz import UTC
from testfixtures import LogCapture
from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factories
@@ -110,6 +114,7 @@ class TestPasswordChange(CreateAccountMixin, CacheIsolationTestCase):
result = self.client.login(username=self.USERNAME, password=self.OLD_PASSWORD)
self.assertTrue(result)
mail.outbox = []
+ cache.clear()
def test_password_change(self):
# Request a password change while logged in, simulating
@@ -259,11 +264,16 @@ class TestPasswordChange(CreateAccountMixin, CacheIsolationTestCase):
# Send the view an email address not tied to any user
response = self._change_password(email=self.NEW_EMAIL)
self.assertEqual(response.status_code, 200)
- logger.check((LOGGER_NAME, 'INFO', 'Invalid password reset attempt'))
+
+ expected_logs = (
+ (LOGGER_NAME, 'INFO', 'Password reset initiated for user {}.'.format(self.NEW_EMAIL)),
+ (LOGGER_NAME, 'INFO', 'Invalid password reset attempt')
+ )
+ logger.check(*expected_logs)
def test_password_change_rate_limited(self):
"""
- Tests that consecutive password reset requests are rate limited.
+ Tests that password reset requests are rate limited as expected.
"""
# Log out the user created during test setup, to prevent the view from
# selecting the logged-in user's email address over the email provided
@@ -273,11 +283,11 @@ class TestPasswordChange(CreateAccountMixin, CacheIsolationTestCase):
response = self._change_password(email=self.NEW_EMAIL)
self.assertEqual(response.status_code, status)
- with patch(
- 'util.request_rate_limiter.PasswordResetEmailRateLimiter.is_rate_limit_exceeded',
- return_value=False
- ):
- response = self._change_password(email=self.NEW_EMAIL)
+ # now reset the time to 1 min from now in future and change the email and
+ # verify that it will allow another request from same IP
+ reset_time = datetime.now(UTC) + timedelta(seconds=61)
+ with freeze_time(reset_time):
+ response = self._change_password(email=self.OLD_EMAIL)
self.assertEqual(response.status_code, 200)
@ddt.data(
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 8450540a8b6830df5122457d20593a091c5ab154..1a83bc0d93a7406c8755887149c32972b111c88d 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
@@ -7,6 +7,7 @@ import json
import re
import unicodedata
import unittest
+from datetime import datetime, timedelta
import ddt
from django.conf import settings
@@ -22,8 +23,10 @@ from django.test.client import RequestFactory
from django.test.utils import override_settings
from django.urls import reverse
from django.utils.http import int_to_base36
+from freezegun import freeze_time
from mock import Mock, patch
from oauth2_provider import models as dot_models
+from pytz import UTC
from six.moves import range
from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factories
@@ -181,31 +184,41 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase):
def test_ratelimited_from_different_ips_with_same_email(self):
"""
- Test that password reset endpoint allow only one request per minute
+ Test that password reset endpoint allow only two requests per hour
per email address.
"""
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 IP and verify that the rate limiter should kick in and
- # give a Forbidden response if the request is for same email address.
+ self.request_password_reset(200)
+ # now reset the time to 1 min from now in future and change the email and
+ # verify that it will allow another request from same IP
+ for status in [200, 403]:
+ reset_time = datetime.now(UTC) + timedelta(seconds=61)
+ with freeze_time(reset_time):
+ self.request_password_reset(status)
+
+ # Even changing the IP will not allow more than two requests for same email.
new_ip = "8.8.8.8"
- self.assertNotEqual(good_req.META.get('REMOTE_ADDR'), new_ip)
+ self.request_password_reset(403, new_ip=new_ip)
+
+ cache.clear()
- bad_req = self.request_factory.post(
+ def request_password_reset(self, status, new_ip=None):
+ extra_args = {}
+ if new_ip:
+ extra_args = {'REMOTE_ADDR': new_ip}
+
+ reset_request = self.request_factory.post(
'/password_reset/',
{'email': 'thisdoesnotexist@foo.com'},
- REMOTE_ADDR=new_ip
+ **extra_args
)
- bad_req.user = AnonymousUser()
- bad_resp = password_reset(bad_req)
- self.assertEqual(bad_resp.status_code, 403)
- self.assertEqual(bad_req.META.get('REMOTE_ADDR'), new_ip)
- cache.clear()
+ if new_ip:
+ self.assertEqual(reset_request.META.get('REMOTE_ADDR'), new_ip)
+
+ reset_request.user = AnonymousUser()
+ response = password_reset(reset_request)
+ self.assertEqual(response.status_code, status)
@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"),