Skip to content

Replace database phone one-time password rate limiter with Redis#7554

Merged
mitchellhenke merged 8 commits intomainfrom
mitchellhenke/replace-otp-rate-limiter-with-redis
Jan 24, 2023
Merged

Replace database phone one-time password rate limiter with Redis#7554
mitchellhenke merged 8 commits intomainfrom
mitchellhenke/replace-otp-rate-limiter-with-redis

Conversation

@mitchellhenke
Copy link
Contributor

changelog: Internal, Rate Limiting, Replace database phone one-time password rate limiter with Redis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a very slight difference in calculating the number of max attempts where Throttle uses greater than or equal to and OtpRateLimiter uses greater than

I think we'll want to rename the config and update it to not use the + 1 once we've switched over.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM. Since the limits for these are relatively low, we don't need to worry about like a transition/rollover case right? we'll just give everybody a quick rate limit reset at deploy time?

@mitchellhenke
Copy link
Contributor Author

mitchellhenke commented Dec 30, 2022

LGTM. Since the limits for these are relatively low, we don't need to worry about like a transition/rollover case right? we'll just give everybody a quick rate limit reset at deploy time?

I think the risk is relatively low, but opted to include the redis_throttle_otp_rate_limiter_read_enabled config (defaulting to false for production) to avoid the reset. It allows us to write to both for a bit while continuing to read from the current implementation. We can switch pretty quickly though, yeah.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/replace-otp-rate-limiter-with-redis branch 2 times, most recently from 07aa424 to cd4dfaa Compare January 10, 2023 17:47
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/replace-otp-rate-limiter-with-redis branch 6 times, most recently from 085aff3 to 57d3452 Compare January 14, 2023 00:36
@mitchellhenke mitchellhenke marked this pull request as ready for review January 17, 2023 18:06
Mitchell Henke and others added 8 commits January 24, 2023 15:15
changelog: Internal, Rate Limiting, Replace database phone one-time password rate limiter with Redis
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/replace-otp-rate-limiter-with-redis branch from 57d3452 to f6028b3 Compare January 24, 2023 21:15
@mitchellhenke mitchellhenke merged commit 0cbae89 into main Jan 24, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/replace-otp-rate-limiter-with-redis branch January 24, 2023 21:41
class Throttle
attr_reader :throttle_type

THROTTLE_CONFIG = {
Copy link
Contributor

@aduth aduth Jan 25, 2023

Choose a reason for hiding this comment

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

Curious: What's the reason/context for refactoring this constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was having trouble with the tests and having multiple avenues to stub since it can be accessed through the constant and the Throttle.attempt_window_in_minutes/1 method. It was a change I'd been considering for a bit and I swear there was a good reason to bring it into this PR in particular, but I'm struggling to remember the specifics 😓

Definitely open to improvements/reversion.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants