Skip to content

Add temporary per-day user throttling #392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
May 3, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cicd/3-app/javabuilder/config/production-demo.config.json
Original file line number Diff line number Diff line change
@@ -5,8 +5,8 @@
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU",
"ProvisionedConcurrentExecutions": "1",
"ReservedConcurrentExecutions": "5",
"LimitPerHour": "25",
"LimitPerDay": "100",
"LimitPerHour": "-1",
"LimitPerDay": "50",
"SilenceAlerts": "false"
},
"Tags": {
2 changes: 1 addition & 1 deletion cicd/3-app/javabuilder/config/production.config.json
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
"ProvisionedConcurrentExecutions": "50",
"ReservedConcurrentExecutions": "500",
"LimitPerHour": "1000",
"LimitPerDay": "5000",
"LimitPerDay": "-1",
"SilenceAlerts": "false",
"TeacherLimitPerHour": "25000"
},
8 changes: 4 additions & 4 deletions cicd/3-app/javabuilder/template.yml.erb
Original file line number Diff line number Diff line change
@@ -26,13 +26,13 @@ Parameters:
Default: 3
LimitPerHour:
Type: Number
Description: The number of Javabuilder invocations allowed per user per hour.
MinValue: 1
Description: The number of Javabuilder invocations allowed per user per hour. If the value is -1, then there is no limit on the number of invocations per hour.
MinValue: -1
Default: 50
LimitPerDay:
Type: Number
Description: The number of Javabuilder invocations allowed per user per day.
MinValue: 1
Description: The number of Javabuilder invocations allowed per user per day. If the value is -1, then there is no limit on the number of invocations per day.
MinValue: -1
Default: 150
TeacherLimitPerHour:
Type: Number
18 changes: 15 additions & 3 deletions javabuilder-authorizer/token_status.rb
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ module TokenStatus
### Token statuses used in HTTP authorizer (first step in token validation)
# Token was validated by HTTP authorizer
VALID_HTTP = 'VALID_HTTP'.freeze
# User has been blocked for violating hourly or daily throttle limits
# User has been blocked for violating hourly throttle limits
USER_BLOCKED = 'USER_BLOCKED'.freeze
# All of a user's teachers (or the teacher themselves, if the user is a teacher)
# has been blocked for violating hourly throttle limits
@@ -24,12 +24,15 @@ module TokenStatus
UNKNOWN_ID = 'UNKNOWN_ID'.freeze
# Token provided was not vetted by hTTP authorizer
NOT_VETTED = 'NOT_VETTED'.freeze
# User has been blocks for violating daily limits. They will be automatically unblocked
# once they are no longer over the limit.
USER_BLOCKED_TEMPORARY = 'USER_BLOCKED_TEMPORARY'.freeze

### Token status used by both authorizers
# Token provided to the authorizer has already been used
TOKEN_USED = 'TOKEN_USED'.freeze

ERROR_STATES = [USER_BLOCKED, CLASSROOM_BLOCKED, USER_OVER_DAILY_LIMIT, USER_OVER_HOURLY_LIMIT, TEACHERS_OVER_HOURLY_LIMIT, UNKNOWN_ID, NOT_VETTED, TOKEN_USED]
ERROR_STATES = [USER_BLOCKED, CLASSROOM_BLOCKED, USER_OVER_DAILY_LIMIT, USER_OVER_HOURLY_LIMIT, TEACHERS_OVER_HOURLY_LIMIT, UNKNOWN_ID, NOT_VETTED, TOKEN_USED, USER_BLOCKED_TEMPORARY]
WARNING_STATES = [NEAR_LIMIT]
VALID_STATES = [VALID_HTTP, VALID_WEBSOCKET]

@@ -38,10 +41,19 @@ module TokenStatus
CLASSROOM_BLOCKED => 'ClassroomBlocked',
UNKNOWN_ID => 'TokenUnknownId',
NOT_VETTED => 'TokenNotVetted',
TOKEN_USED => 'TokenUsed'
TOKEN_USED => 'TokenUsed',
USER_BLOCKED_TEMPORARY => 'UserBlockedTemporary',
}.freeze

NEW_USER_BLOCKED = 'NewUserBlocked'.freeze
NEW_CLASSROOM_BLOCKED = 'NewClassroomBlocked'.freeze
CLASSROOM_HOURLY_REQUEST_COUNT = 'ClassroomHourlyRequestCount'.freeze

# Lockout statuses
PERMANENT_LOCKOUT = 'PERMANENT'.freeze
TEMPORARY_LOCKOUT = 'TEMPORARY'.freeze

# Lockout periods
DAY = 'DAY'.freeze
HOUR = 'HOUR'.freeze
end
42 changes: 28 additions & 14 deletions javabuilder-authorizer/token_validator.rb
Original file line number Diff line number Diff line change
@@ -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 = -1

def initialize(payload, origin, context)
@token_id = payload['sid']
@@ -29,10 +30,14 @@ def validate
return error(CLASSROOM_BLOCKED) if teachers_blocked?
hourly_usage_response = user_usage(ONE_HOUR_SECONDS)
return error(USER_BLOCKED) if user_over_hourly_limit?(hourly_usage_response)
# return error(USER_BLOCKED) if user_over_daily_limit?
daily_usage_response = user_usage(ONE_DAY_SECONDS)
return error(USER_BLOCKED_TEMPORARY) if user_over_daily_limit?(daily_usage_response)
return error(CLASSROOM_BLOCKED) if teachers_over_hourly_limit?

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
Comment on lines 37 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though, the hourly limit triggers a permanent block, so it does make sense that it should go first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

log_requests
mark_token_as_vetted
set_token_warning(NEAR_LIMIT, near_limit_detail) if near_limit_detail
@@ -88,23 +93,32 @@ def teachers_blocked?
end

def user_over_hourly_limit?(hourly_usage_response)
limit_per_hour = ENV['limit_per_hour'].to_i
return false if limit_per_hour == NO_LIMIT
user_over_limit?(
hourly_usage_response,
ENV['limit_per_hour'].to_i,
USER_OVER_HOURLY_LIMIT
limit_per_hour,
USER_OVER_HOURLY_LIMIT,
true # Should block permanently if the limit has been reached.
)
end

def get_user_near_hourly_limit_detail(hourly_usage_count)
get_user_near_limit_detail(hourly_usage_count, ENV['limit_per_hour'].to_i)
get_user_near_limit_detail(hourly_usage_count, ENV['limit_per_hour'].to_i, HOUR, PERMANENT_LOCKOUT)
end

def get_user_near_daily_limit_detail(daily_usage_count)
get_user_near_limit_detail(daily_usage_count, ENV['limit_per_day'].to_i, DAY, TEMPORARY_LOCKOUT)
end

def user_over_daily_limit?
usage_response = user_usage(ONE_DAY_SECONDS)
def user_over_daily_limit?(daily_usage_response)
limit_per_day = ENV['limit_per_day'].to_i
return false if limit_per_day == NO_LIMIT
user_over_limit?(
usage_response,
ENV['limit_per_day'].to_i,
USER_OVER_DAILY_LIMIT
daily_usage_response,
limit_per_day,
USER_OVER_DAILY_LIMIT,
false # Should not block permanently if the limit has been reached.
)
end

@@ -226,17 +240,17 @@ def user_usage(time_range_seconds)
response
end

def get_user_near_limit_detail(count, limit)
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 != NO_LIMIT && count <= limit && count >= (limit - NEAR_LIMIT_BUFFER)
return {remaining: limit - count, period: period, lockout_type: lockout_type}
else
return nil
end
end

def user_over_limit?(query_response, limit, logging_message)
def user_over_limit?(query_response, limit, logging_message, should_block_permanently)
over_limit = query_response.count > limit
if over_limit
if over_limit && should_block_permanently
# logging could be improved,
# [{"ttl"=>0.1648766446e10, "user_id"=>"611", "issued_at"=>0.1648680046e10}, {...
begin