fix(password-reset): rate limit password reset emails #2619
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2619 +/- ##
=======================================
Coverage 95.43% 95.44%
=======================================
Files 975 977 +2
Lines 27245 27325 +80
=======================================
+ Hits 26002 26080 +78
- Misses 1243 1245 +2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hmm... this feels like a bit overkill to me? I think we can just store the last password reset request time (although I'd argue we'd be better off storing this in a separate table than directly on the user model, as it will give us more flexibility), and then just define an interval between resets of, say, 30 or 60 seconds?
I just wanted to completely get rid of this issue - emails generated per 30 sec or 1 min are still a lot of emails? |
api/users/models.py
Outdated
| @property | ||
| def can_send_password_reset_email(self) -> bool: | ||
| with suppress(TypeError): | ||
| if self.last_password_reset_email_at < timezone.now() - timezone.timedelta( | ||
| seconds=settings.PASSWORD_RESET_EMAIL_COOLDOWN | ||
| ): | ||
| return True | ||
| return ( | ||
| self.num_of_password_reset_emails_sent | ||
| < settings.MAX_PASSWORD_RESET_EMAILS | ||
| ) | ||
|
|
||
| return True |
There was a problem hiding this comment.
In response to the main thread on the PR, I tend to agree that we should do slightly more to prevent it but I do think that if we use a separate model this logic will become slightly cleaner / more flexible?
Something like:
class UserPasswordResetRequest(models.Model):
user = models.ForeignKey("users.FFAdminUser", related_name="password_reset_requests")
requested_at = models.DateTimeField(auto_now_add=True)Then the code above becomes something like:
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
)Then on a successful password reset, we just clear the table for that user.
(sidenote: as per my comment on another PR, I'd rather not query the database in properties if we can avoid it)
There was a problem hiding this comment.
Yeah, good point. Done
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, likely. I will create an issue for it once this is merged, sounds good?
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingChanges
Rate limit password reset emails
New environment variables:
How did you test this code?
Covered by unit tests