Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(password-reset): rate limit password reset emails #2619

Merged
merged 5 commits into from
Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/app/settings/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's now a question whether we should add a recurring task to delete all UserPasswordResetRequest entities older than this time frame. Probably one we can worry about in the future but we will end up with stale data in there that likely never gets deleted / used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, likely. I will create an issue for it once this is merged, sounds good?


# 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)
25 changes: 25 additions & 0 deletions api/custom_auth/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -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)),
],
),
]
Empty file.
10 changes: 10 additions & 0 deletions api/custom_auth/models.py
Original file line number Diff line number Diff line change
@@ -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)
15 changes: 14 additions & 1 deletion api/custom_auth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
"""
Expand Down Expand Up @@ -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)
85 changes: 85 additions & 0 deletions api/tests/unit/users/test_unit_users_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
14 changes: 14 additions & 0 deletions api/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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:
Expand All @@ -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")
Expand Down