Skip to content

Implement rate limiting for e-mail verifications#8419

Open
LawnGnome wants to merge 1 commit into
rust-lang:mainfrom
LawnGnome:e-mail-throttling
Open

Implement rate limiting for e-mail verifications#8419
LawnGnome wants to merge 1 commit into
rust-lang:mainfrom
LawnGnome:e-mail-throttling

Conversation

@LawnGnome
Copy link
Copy Markdown
Contributor

This applies a burst of 3 and a refill time of 30 minutes by default per user. (Not that I can imagine a scenario where we'd ever override this for a user, but using the same machinery as other user actions is obviously much simpler.)

I don't love that this ends up essentially prop-drilling the rate limiter into a bunch of new places, but I don't see an alternative other than prop-drilling the whole app struct through, which would be worse.

This doesn't address (sorry) per-address rate limiting, but is at least a reasonable starting point.

This applies a burst of 3 and a refill time of 30 minutes by default
per user. (Not that I can imagine a scenario where we'd ever override
this for a user, but using the same machinery as other user actions is
obviously much simpler.)

I don't love that this ends up essentially prop-drilling the rate
limiter into a bunch of new places, but I don't see an alternative other
than prop-drilling the whole app struct through, which would be worse.

This doesn't address (sorry) per-address rate limiting, but is at least
a reasonable starting point.
@LawnGnome LawnGnome added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Apr 9, 2024
@LawnGnome LawnGnome requested a review from a team April 9, 2024 00:42
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2024

Codecov Report

❌ Patch coverage is 89.61039% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.63%. Comparing base (61c0870) to head (27637ae).
⚠️ Report is 7216 commits behind head on main.

Files with missing lines Patch % Lines
src/controllers/user/me.rs 74.07% 7 Missing ⚠️
src/controllers/user/session.rs 41.66% 7 Missing ⚠️
src/rate_limiter.rs 94.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8419      +/-   ##
==========================================
+ Coverage   87.62%   87.63%   +0.01%     
==========================================
  Files         272      272              
  Lines       26333    26426      +93     
==========================================
+ Hits        23073    23158      +85     
- Misses       3260     3268       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Turbo87
Copy link
Copy Markdown
Member

Turbo87 commented Apr 9, 2024

I don't love that this ends up essentially prop-drilling the rate limiter into a bunch of new places, but I don't see an alternative other than prop-drilling the whole app struct through, which would be worse.

I think the root cause of this is that the model layer is currently sending emails, which IMHO is a bit questionable, especially because we're using this code path in a couple of places in the test suite too. It might be better to decouple these things first before introducing the rate limiting.

@LawnGnome
Copy link
Copy Markdown
Contributor Author

I think the root cause of this is that the model layer is currently sending emails, which IMHO is a bit questionable, especially because we're using this code path in a couple of places in the test suite too. It might be better to decouple these things first before introducing the rate limiting.

Yeah, I went back and forth on this yesterday — decoupling the e-mail sending out of the model layer is better from an architectural perspective, but it means the NewUser::create_or_update API is easier to misuse as it stands, since it now comes with an implicit need to figure out if the e-mail address needs verification after and action that.

The e-mail upsert code could be extracted into something that returns that state, but being in the same transaction as the user upsert is obviously a nice quality that would be bad to lose.

I'm sorta wondering if we end up with something like this (treat names as placeholders):

let new_user = NewUser::new(...);
let updater = new_user.create_or_update(&conn)?;  // returns some intermediate type holding a transaction
let verification_required = updater.set_email(&email)?;  // needs to be optional since not all updates might touch the e-mail?
updater.commit()?;  // consumes updater

// Has to be after the commit;
if verification_required {
  // send verification e-mail
}

Another, simpler option would obviously be to return something else from create_or_update than just the User — a struct enclosing the User along with a bool field indicating if verification is required.

Now I've typed this out, I probably have a slight preference for the second option (return something enclosing User and whatever other state we need), but I'm interested in what you think.

@bors
Copy link
Copy Markdown
Contributor

bors commented Apr 10, 2024

☔ The latest upstream changes (presumably 77d55e9) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 12, 2026

☔ The latest upstream changes (possibly #13157) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants