Skip to content

Commit

Permalink
fix: ignore rate limits for autoconfirm (#1810)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* #1800 introduced a bug which
applied the rate limiting setting even if `AUTOCONFIRM` was enabled.
Although this is unintended, it is a breaking change so we need to give
users time to update their rate limit settings before applying it

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
  • Loading branch information
kangmingtay authored Oct 22, 2024
1 parent 39459c1 commit 9ce2340
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 11 deletions.
19 changes: 11 additions & 8 deletions internal/api/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -627,14 +627,17 @@ func (a *API) sendEmail(r *http.Request, tx *storage.Connection, u *models.User,
}
}

// apply rate limiting before the email is sent out
if ok := a.limiterOpts.Email.Allow(); !ok {
emailRateLimitCounter.Add(
ctx,
1,
metric.WithAttributeSet(attribute.NewSet(attribute.String("path", r.URL.Path))),
)
return EmailRateLimitExceeded
// TODO(km): Deprecate this behaviour - rate limits should still be applied to autoconfirm
if !config.Mailer.Autoconfirm {
// apply rate limiting before the email is sent out
if ok := a.limiterOpts.Email.Allow(); !ok {
emailRateLimitCounter.Add(
ctx,
1,
metric.WithAttributeSet(attribute.NewSet(attribute.String("path", r.URL.Path))),
)
return EmailRateLimitExceeded
}
}

if config.Hook.SendEmail.Enabled {
Expand Down
9 changes: 6 additions & 3 deletions internal/api/phone.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,12 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use

// not using test OTPs
if otp == "" {
// apply rate limiting before the sms is sent out
if ok := a.limiterOpts.Phone.Allow(); !ok {
return "", tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "SMS rate limit exceeded")
// TODO(km): Deprecate this behaviour - rate limits should still be applied to autoconfirm
if !config.Sms.Autoconfirm {
// apply rate limiting before the sms is sent out
if ok := a.limiterOpts.Phone.Allow(); !ok {
return "", tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "SMS rate limit exceeded")
}
}
otp, err = crypto.GenerateOtp(config.Sms.OtpLength)
if err != nil {
Expand Down

0 comments on commit 9ce2340

Please sign in to comment.