Add temporary per-day user throttling #392
Conversation
molly-moen
requested review from
cat5inthecradle,
bencodeorg,
ebeastlake,
sanchitmalhotra126 and
a team
| ) | ||
| limit_per_hour = ENV['limit_per_hour'].to_i | ||
| # A limit of 0 means there is no limit. | ||
| if limit_per_hour > 0 |
There was a problem hiding this comment.
nit: early return false if limit_per_hour == 0?
| near_limit_detail = get_user_near_hourly_limit_detail(hourly_usage_response.count) | ||
| if !near_limit_detail | ||
| near_limit_detail = get_user_near_daily_limit_detail(daily_usage_response.count) | ||
| end |
There was a problem hiding this comment.
With the way our limits set up, seems like there's no way for a user to be near both an hourly limit and a daily limit - but if there was, I guess we're prioritizing the hourly limit warning? Seems fine to me, just wanted to clarify
There was a problem hiding this comment.
Would a downside of that be a user receiving a "hourly limit" warning message, so they come back in an hour only to receive the "daily limit" message on their first attempt? Only matters if the messages are unique.
There was a problem hiding this comment.
Though, the hourly limit triggers a permanent block, so it does make sense that it should go first.
There was a problem hiding this comment.
Good questions. Yes, in our prod environments only one of these will be set (demo has a daily limit, prod has an hourly). But if we want both, my presumption was the hourly would be much lower than the daily and therefore the user would hit that first. And the fact that hourly triggers the permanent block means it is more important to message. The messages to the user are unique--they say what the time frame was (hour or day) and if the block is permanent.
There was a problem hiding this comment.
It feels like overkill for now to make some combined message given we won't show it to users, but it could be a follow-up if we think we'll enable both limits at some point.
| if count <= limit && count >= (limit - NEAR_LIMIT_BUFFER) | ||
| return {remaining: limit - count} | ||
| def get_user_near_limit_detail(count, limit, period, lockout_type) | ||
| if limit > 0 && count <= limit && count >= (limit - NEAR_LIMIT_BUFFER) |
There was a problem hiding this comment.
minor: if we want to be super clear, maybe we can save 0 as a constant like "NO_LIMIT" or something, and check limit != NO_LIMIT here? The 0 is documented well enough in the template so maybe not necessary
There was a problem hiding this comment.
since this is slightly hacky, it doesn't hurt to be super clear. I'll update this
| @@ -11,6 +11,7 @@ class TokenValidator | |||
| USER_REQUEST_RECORD_TTL_SECONDS = 25 * ONE_HOUR_SECONDS | |||
| TEACHER_ASSOCIATED_REQUEST_TTL_SECONDS = 25 * ONE_HOUR_SECONDS | |||
| NEAR_LIMIT_BUFFER = 10 | |||
| NO_LIMIT = 0 | |||
There was a problem hiding this comment.
Are there any cases where we would set the limit to zero intending to "shut off" access? i.e. I think our guidance in a DDOS is to set reserved concurrency to zero. Any reason we'd do that here too?
There was a problem hiding this comment.
I think if we set reserved concurrency to 0 that would cover shutting off access. If we want to severely limit access we could set limits to 1. For hourly limit that would probably be more trouble than it's worth, because we would have to then go unblock everyone.
There was a problem hiding this comment.
Just had a thought (not blocking). Does this take negative numbers? -1 seems like a pretty common "no limit" key that's less likely to have us run into issues in the future.
There was a problem hiding this comment.
That makes sense too (using -1), but if we used that, we'd probably want to make 0 have some predictable defined behavior (automatic rejection with a 'limit exceed' error).
There was a problem hiding this comment.
I'll try it out, the easiest way to try it out is by committing to auto-deploy to my dev instance, so there may be a little churn on this pr in the meantime.
There was a problem hiding this comment.
-1 works and I agree it's a bit cleaner, so let's go with that!
As part of Java Lab Eval Mode, we want to add back in daily user throttling, but have it not block the user forever, only until they are back under the limit. In this PR I:
USER_BLOCKED_TEMPORARYthat we will send if the user reaches their limit in a day.This PR cannot be merged until the corresponding code.org PR has been merged and deployed.