Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Improve password reset rate limit.

Browse Source 
Used django-ratelimit instead of django-ratelimit-backend
to configure two different rate limit configurations for same
endpoint.

PROD-1708
This commit is contained in:
Waheed Ahmed
2020-07-01 18:07:23 +05:00
parent a6a69224d1
commit 4f80fd6540
8 changed files with 81 additions and 121 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
cms/envs/bok_choy.yml
View File

@@ -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

4
cms/envs/common.py
View File

@@ -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'

72
common/djangoapps/util/request_rate_limiter.py
View File

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

7
lms/envs/bok_choy.yml
View File

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

8
lms/envs/common.py
View File

@@ -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_'

35
openedx/core/djangoapps/user_authn/views/password_reset.py
View File

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

24
openedx/core/djangoapps/user_authn/views/tests/test_password.py
View File

@@ -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(

49
openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py
View File

@@ -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,32 +184,42 @@ 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)
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)
# 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.
# 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)
bad_req = self.request_factory.post(
'/password_reset/',
{'email': 'thisdoesnotexist@foo.com'},
REMOTE_ADDR=new_ip
)
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)
self.request_password_reset(403, new_ip=new_ip)
cache.clear()
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'},
**extra_args
)
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"),
('html', "You're receiving this e-mail because you requested a password reset"))