From db98743d426c0ded932d5a624cf8bd00cf2c6a86 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Tue, 15 Aug 2023 16:46:30 +0530 Subject: [PATCH] fix(password-reset): rate limit password reset emails (#2619) --- api/app/settings/common.py | 6 ++ api/custom_auth/migrations/0001_initial.py | 25 ++++++ api/custom_auth/migrations/__init__.py | 0 api/custom_auth/models.py | 10 +++ api/custom_auth/views.py | 15 +++- api/tests/unit/users/test_unit_users_views.py | 85 +++++++++++++++++++ api/users/models.py | 14 +++ 7 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 api/custom_auth/migrations/0001_initial.py create mode 100644 api/custom_auth/migrations/__init__.py create mode 100644 api/custom_auth/models.py diff --git a/api/app/settings/common.py b/api/app/settings/common.py index e6b0419ecb72..20982d8a604c 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -903,3 +903,9 @@ DOMAIN_OVERRIDE = env.str("FLAGSMITH_DOMAIN", "") # Used when no Django site is specified. DEFAULT_DOMAIN = "app.flagsmith.com" + +# Define the cooldown duration, in seconds, for password reset emails +PASSWORD_RESET_EMAIL_COOLDOWN = env.int("PASSWORD_RESET_EMAIL_COOLDOWN", 60 * 60 * 24) + +# Limit the count of password reset emails that can be dispatched within the `PASSWORD_RESET_EMAIL_COOLDOWN` timeframe. +MAX_PASSWORD_RESET_EMAILS = env.int("MAX_PASSWORD_RESET_EMAILS", 5) diff --git a/api/custom_auth/migrations/0001_initial.py b/api/custom_auth/migrations/0001_initial.py new file mode 100644 index 000000000000..bddde746470f --- /dev/null +++ b/api/custom_auth/migrations/0001_initial.py @@ -0,0 +1,25 @@ +# Generated by Django 3.2.20 on 2023-08-15 05:15 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='UserPasswordResetRequest', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('requested_at', models.DateTimeField(auto_now_add=True)), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='password_reset_requests', to=settings.AUTH_USER_MODEL)), + ], + ), + ] diff --git a/api/custom_auth/migrations/__init__.py b/api/custom_auth/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/custom_auth/models.py b/api/custom_auth/models.py new file mode 100644 index 000000000000..1eb02e412ca2 --- /dev/null +++ b/api/custom_auth/models.py @@ -0,0 +1,10 @@ +from django.db import models + +from users.models import FFAdminUser + + +class UserPasswordResetRequest(models.Model): + user = models.ForeignKey( + FFAdminUser, related_name="password_reset_requests", on_delete=models.CASCADE + ) + requested_at = models.DateTimeField(auto_now_add=True) diff --git a/api/custom_auth/views.py b/api/custom_auth/views.py index 562edd9d2f83..211a224fdc44 100644 --- a/api/custom_auth/views.py +++ b/api/custom_auth/views.py @@ -4,7 +4,7 @@ from drf_yasg.utils import swagger_auto_schema from rest_framework import status from rest_framework.authtoken.models import Token -from rest_framework.decorators import api_view, permission_classes +from rest_framework.decorators import action, api_view, permission_classes from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.throttling import ScopedRateThrottle @@ -16,6 +16,8 @@ from custom_auth.serializers import CustomUserDelete from users.constants import DEFAULT_DELETE_ORPHAN_ORGANISATIONS_VALUE +from .models import UserPasswordResetRequest + class CustomAuthTokenLoginOrRequestMFACode(AuthTokenLoginOrRequestMFACode): """ @@ -68,3 +70,14 @@ def perform_destroy(self, instance): DEFAULT_DELETE_ORPHAN_ORGANISATIONS_VALUE, ) ) + + @action(["post"], detail=False) + def reset_password(self, request, *args, **kwargs): + serializer = self.get_serializer(data=request.data) + serializer.is_valid(raise_exception=True) + user = serializer.get_user() + if user and user.can_send_password_reset_email(): + super().reset_password(request, *args, **kwargs) + UserPasswordResetRequest.objects.create(user=user) + + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/api/tests/unit/users/test_unit_users_views.py b/api/tests/unit/users/test_unit_users_views.py index 211981660ad5..56f62d492cc7 100644 --- a/api/tests/unit/users/test_unit_users_views.py +++ b/api/tests/unit/users/test_unit_users_views.py @@ -2,7 +2,11 @@ import pytest from django.conf import settings +from django.contrib.auth.tokens import default_token_generator +from django.core import mail from django.urls import reverse +from djoser import utils +from djoser.email import PasswordResetEmail from rest_framework import status from rest_framework.test import APIClient @@ -117,3 +121,84 @@ def test_change_email_address_api(mocker): assert len(args) == 0 assert len(kwargs) == 1 assert kwargs["args"] == (first_name, settings.DEFAULT_FROM_EMAIL, old_email) + + +@pytest.mark.django_db +def test_send_reset_password_emails_rate_limit(settings, client, test_user): + # Given + settings.MAX_PASSWORD_RESET_EMAILS = 2 + settings.PASSWORD_RESET_EMAIL_COOLDOWN = 60 + + url = reverse("api-v1:custom_auth:ffadminuser-reset-password") + data = {"email": test_user.email} + + # When + for _ in range(5): + response = client.post( + url, data=json.dumps(data), content_type="application/json" + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Then - we should only have two emails + assert len(mail.outbox) == 2 + isinstance(mail.outbox[0], PasswordResetEmail) + isinstance(mail.outbox[1], PasswordResetEmail) + + # clear the outbox + mail.outbox.clear() + + # Next, let's reduce the cooldown to 1 second and try again + settings.PASSWORD_RESET_EMAIL_COOLDOWN = 0.001 + response = client.post(url, data=json.dumps(data), content_type="application/json") + + # Then - we should receive another email + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert len(mail.outbox) == 1 + isinstance(mail.outbox[0], PasswordResetEmail) + + +@pytest.mark.django_db +def test_send_reset_password_emails_rate_limit_resets_after_password_reset( + settings, client, test_user +): + # Given + settings.MAX_PASSWORD_RESET_EMAILS = 2 + settings.PASSWORD_RESET_EMAIL_COOLDOWN = 60 * 60 * 24 + + url = reverse("api-v1:custom_auth:ffadminuser-reset-password") + data = {"email": test_user.email} + + # First, let's hit the limit of emails we can send + for _ in range(5): + response = client.post( + url, data=json.dumps(data), content_type="application/json" + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Then - we should only have two emails + assert len(mail.outbox) == 2 + mail.outbox.clear() + + # Next, let's reset the password + reset_password_data = { + "new_password": "new_password", + "re_new_password": "new_password", + "uid": utils.encode_uid(test_user.pk), + "token": default_token_generator.make_token(test_user), + } + reset_password_confirm_url = reverse( + "api-v1:custom_auth:ffadminuser-reset-password-confirm" + ) + response = client.post( + reset_password_confirm_url, + data=json.dumps(reset_password_data), + content_type="application/json", + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Finally, let's try to send another email + client.post(url, data=json.dumps(data), content_type="application/json") + + # Then - we should receive another email + assert len(mail.outbox) == 1 diff --git a/api/users/models.py b/api/users/models.py index 07bdf1f595a9..3d8f5e2e0544 100644 --- a/api/users/models.py +++ b/api/users/models.py @@ -7,6 +7,7 @@ from django.core.mail import send_mail from django.db import models from django.db.models import Count, QuerySet +from django.utils import timezone from django.utils.translation import gettext_lazy as _ from django_lifecycle import AFTER_CREATE, LifecycleModel, hook @@ -135,6 +136,10 @@ def delete( self.delete_orphan_organisations() super().delete() + def set_password(self, raw_password): + super().set_password(raw_password) + self.password_reset_requests.all().delete() + @property def auth_type(self): if self.google_user_id: @@ -158,6 +163,15 @@ def get_full_name(self): return None return " ".join([self.first_name, self.last_name]).strip() + def can_send_password_reset_email(self) -> bool: + limit = timezone.now() - timezone.timedelta( + seconds=settings.PASSWORD_RESET_EMAIL_COOLDOWN + ) + return ( + self.password_reset_requests.filter(requested_at__gte=limit).count() + < settings.MAX_PASSWORD_RESET_EMAILS + ) + def join_organisation_from_invite_email(self, invite_email: "Invite"): if invite_email.email.lower() != self.email.lower(): raise InvalidInviteError("Registered email does not match invited email")