-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Rate limit devices per user on short window #35613
Merged
Merged
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
569a15a
Create limiter to track device usage for each user
gherceg c8e4f92
Return 406 response when device is rate limited
gherceg 74ac239
Added datadog metrics for rate limiter
gherceg 6d270bb
Add setting to enable/disable device rate limiter
gherceg bc9d976
Fix failing test using mock
gherceg 384f700
Use sismember, not srem redis function
gherceg c90a5bd
Use pipelines to reduce calls to redis
gherceg 4014ac8
Modify bucket boundaries
gherceg d3e70de
Pass json converted instance into SubmissionPost
gherceg 4c8c734
Add comment explaining rationale for 2 minute timeout
gherceg 04e2ce7
Fix bug tracking wrong user id
gherceg b5c3aeb
Only use login as user if available
gherceg 0812b53
Fix metric to report device count
gherceg 881c35d
Fix bug introduced with previous change
gherceg 963cb42
Address minor feedback
gherceg 0aa7482
Convert xform to json if instance_json IS None
gherceg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
from datetime import datetime, timezone | ||
|
||
from django.conf import settings | ||
from django_redis import get_redis_connection | ||
|
||
from corehq import toggles | ||
from corehq.apps.cloudcare.const import DEVICE_ID as CLOUDCARE_DEVICE_ID | ||
from corehq.util.metrics import metrics_counter, metrics_histogram | ||
|
||
DEVICE_RATE_LIMIT_MESSAGE = "Current usage for this user is too high. Please try again in a minute." | ||
DEVICE_SET_CACHE_TIMEOUT = 2 * 60 # 2 minutes | ||
gherceg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class DeviceRateLimiter: | ||
""" | ||
Operates on a time window of 1 minute | ||
""" | ||
|
||
def __init__(self): | ||
# need to use raw redis connection to use srem and scard functions | ||
self.client = get_redis_connection() | ||
|
||
def device_limit_per_user(self, domain): | ||
if toggles.INCREASE_DEVICE_LIMIT_PER_USER.enabled(domain): | ||
return settings.INCREASED_DEVICE_LIMIT_PER_USER | ||
return settings.DEVICE_LIMIT_PER_USER | ||
|
||
def rate_limit_device(self, domain, user_id, device_id): | ||
""" | ||
Returns boolean representing if this user_id + device_id combo is rate limited or not | ||
NOTE: calling this method will result in the device_id being added to the list of used device_ids | ||
""" | ||
if not device_id or self._is_formplayer(device_id): | ||
# do not track formplayer activity | ||
return False | ||
|
||
key = self._get_redis_key(user_id) | ||
|
||
if not self._exists(key): | ||
self._track_usage(key, device_id, is_key_new=True) | ||
return False | ||
|
||
if self._device_has_been_used(key, device_id): | ||
return False | ||
|
||
device_count = self._device_count(key) | ||
if device_count < self.device_limit_per_user(domain): | ||
self._track_usage(key, device_id) | ||
gherceg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# this intentionally doesn't capture users with 1 device, only those with multiple | ||
metrics_histogram( | ||
'commcare.devices_per_user.device_count', | ||
device_count + 1, | ||
bucket_tag='count', | ||
buckets=[3, 5, 8, 10], | ||
tags={'domain': domain, 'user_id': user_id}, | ||
) | ||
return False | ||
|
||
metrics_counter( | ||
'commcare.devices_per_user.rate_limit_exceeded', tags={'domain': domain, 'user_id': user_id} | ||
) | ||
return settings.ENABLE_DEVICE_RATE_LIMITER | ||
|
||
def _get_redis_key(self, user_id): | ||
""" | ||
Create a redis key using the user_id and current time to the floored minute | ||
This ensures a new key is used every minute | ||
""" | ||
time = datetime.now(timezone.utc) | ||
formatted_time = time.strftime('%Y-%m-%d_%H:%M') | ||
key = f"device-limiter_{user_id}_{formatted_time}" | ||
return key | ||
|
||
def _track_usage(self, redis_key, device_id, is_key_new=False): | ||
self.client.sadd(redis_key, device_id) | ||
if is_key_new: | ||
self.client.expire(redis_key, DEVICE_SET_CACHE_TIMEOUT) | ||
gherceg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def _device_has_been_used(self, redis_key, device_id): | ||
# check if device_id is member of the set for this key | ||
return self.client.srem(redis_key, device_id) | ||
gherceg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def _device_count(self, redis_key): | ||
return self.client.scard(redis_key) | ||
|
||
def _exists(self, redis_key): | ||
return self.client.exists(redis_key) | ||
|
||
def _is_formplayer(self, device_id): | ||
return device_id.startswith("WebAppsLogin") or device_id == CLOUDCARE_DEVICE_ID | ||
|
||
|
||
device_rate_limiter = DeviceRateLimiter() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from django.test import SimpleTestCase, override_settings | ||
from freezegun import freeze_time | ||
|
||
from corehq.apps.cloudcare.const import DEVICE_ID as CLOUDCARE_DEVICE_ID | ||
from corehq.apps.users.device_rate_limiter import device_rate_limiter | ||
from corehq.tests.pytest_plugins.reusedb import clear_redis | ||
from corehq.util.test_utils import flag_enabled | ||
|
||
|
||
@freeze_time("2024-12-10 12:05:43") | ||
@override_settings(DEVICE_LIMIT_PER_USER=1) | ||
@override_settings(ENABLE_DEVICE_RATE_LIMITER=True) | ||
class TestDeviceRateLimiter(SimpleTestCase): | ||
|
||
domain = 'device-rate-limit-test' | ||
|
||
def setUp(self): | ||
self.addCleanup(clear_redis) | ||
|
||
def test_allowed_if_no_devices_have_been_used_yet(self): | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
|
||
@override_settings(DEVICE_LIMIT_PER_USER=2) | ||
def test_allowed_if_device_count_is_under_limit(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
|
||
def test_rate_limited_if_device_count_exceeds_limit(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertTrue(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
|
||
def test_allowed_if_device_has_already_been_used(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id')) | ||
|
||
def test_allowed_if_different_user(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'new-user-id', 'existing-device-id')) | ||
|
||
def test_allowed_after_waiting_one_minute(self): | ||
with freeze_time("2024-12-10 12:05:43") as frozen_time: | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertTrue(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
frozen_time.move_to("2024-12-10 12:06:15") | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
gherceg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def test_formplayer_activity_is_always_allowed(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'WebAppsLogin*newlogin')) | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', CLOUDCARE_DEVICE_ID)) | ||
|
||
def test_formplayer_activity_does_not_count_towards_limit(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'WebAppsLogin*newlogin') | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', CLOUDCARE_DEVICE_ID) | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
|
||
@override_settings(DEVICE_LIMIT_PER_USER=1) | ||
@override_settings(INCREASED_DEVICE_LIMIT_PER_USER=2) | ||
def test_allowed_after_enabling_ff_to_increase_limit(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertTrue(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
with flag_enabled('INCREASE_DEVICE_LIMIT_PER_USER'): | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) | ||
|
||
@override_settings(ENABLE_DEVICE_RATE_LIMITER=False) | ||
def test_allowed_if_rate_limiter_is_disabled(self): | ||
device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'existing-device-id') | ||
self.assertFalse(device_rate_limiter.rate_limit_device(self.domain, 'user-id', 'new-device-id')) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be retrieved directly from the instance with
instance.metadata.deviceID
? Possibly it should be made it forgiving of missing attributes withgetattr()
? I'm concerned about the overhead of addingconvert_xform_to_json()
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gherceg pointed out in an offline discussion that
instance
is a byte string here, not a form object as it is inSubmissionPost
later on.It would be nice to pass
form_json
on from here to anything else that subsequently needs parsed form JSON to avoid having to re-parse in those places.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempted in d3e70de