feat(api): double API throttles for membership promotion - #7508
Conversation
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
| @classmethod | ||
| def setUpTestData(cls) -> None: | ||
| cls.user = UserProfileWithParentsFactory.create().user | ||
| cls.user.set_password("password") |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
The password on 'cls.user' is being set without validating the password. Call django.contrib.auth.password_validation.validate_password() with validation functions before setting the password. See https://docs.djangoproject.com/en/3.0/topics/auth/passwords/ for more information.
To resolve this comment:
💡 Follow autofix suggestion
| cls.user.set_password("password") | |
| if django.contrib.auth.password_validation.validate_password("password", user=cls.user): | |
| cls.user.set_password("password") |
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by unvalidated-password.
You can view more details about this finding in the Semgrep AppSec Platform.
e756951 to
941eb6d
Compare
Adds a time-boxed x2 boost to user API rate limits, gated by the `double_api_throttles` waffle switch. Lifetime and daily usage stats are unaffected (separate Redis path).
941eb6d to
7c7d79c
Compare
albertisfu
left a comment
albertisfu
left a comment
There was a problem hiding this comment.
Thanks @ERosendo this looks good!
I just a couple of suggestions before we merge it.
|
|
||
| def promo_doubling_applies(user: User) -> bool: | ||
| """Return whether the x2 API promotion applies to this user.""" | ||
| if not switch_is_active(DOUBLE_API_THROTTLES_SWITCH): |
There was a problem hiding this comment.
This will hit the Redis cache on each request. Could we cache the switch_is_active status in memory to avoid hitting Redis too often? Maybe with a 10-minute cache?
It’s fine if it takes up to 10 minutes for the increase to be enabled or disabled after the switch is activated.
|
|
||
| overrides = get_all_throttle_overrides(self.throttle_type) | ||
| rates = overrides.get(request.user.username) or self.default_rates | ||
| if promo_doubling_applies(request.user): |
There was a problem hiding this comment.
In get_cache_key, we’re doing:
if self.scope == "user" and promo_doubling_applies(request.user):
But here, we only check:
if promo_doubling_applies(request.user):
To be consistent, could we use the same condition?
if self.scope == "user" and promo_doubling_applies(request.user):
Maybe we can create a helper method for it.
There was a problem hiding this comment.
Good catch! I extracted a _promo_applies(request) helper on ExceptionalUserRateThrottle that encapsulates self.scope == "user" and promo_doubling_applies(request.user), then used it in both get_cache_key() and allow_request(). That way the eligibility check is defined in one place and stays consistent across both methods.
Extract `_promo_applies(request)` into `ExceptionalUserRateThrottle` to centralize the x2 promo eligibility logic. Previously, `get_cache_key()` checked both `self.scope == "user"` and `promo_doubling_applies(...)`, while `allow_request()` only checked `promo_doubling_applies(...)`. Using a shared helper ensures the eligibility condition is defined once and applied consistently.
albertisfu
left a comment
albertisfu
left a comment
There was a problem hiding this comment.
Thanks @ERosendo this looks ready to go!
2 participants
Fixes
This PR fixes https://github.com/freelawproject/internal/issues/1050
Summary
This PR adds a time-boxed x2 boost to API rate limits, controlled by a new
double_api_throttleswaffle switch. This lets us enable the promotion and disable it when it ends without requiring a redeploy.Who gets doubled
The promotion is implemented as shared eligibility logic that's reused throughout the codebase:
ExceptionalUserRateThrottle.allow_requestdoubles the resolved rates for eligible users._promo2xthrottle cache window (via get_cache_key). When the promotion ends, users return to the normal limits with a clean slate instead of having a window full of requests counted against the lower limits.get_recent_api_request_countuse the same eligibility check, so thedisplayed limits always match the enforced limits.
Lifetime and daily usage stats are untouched, they're tracked on a separate Redis path (
_log_request), independent of the throttle history.Deployment
This PR should:
skip-deploy(skips everything below)skip-web-deployskip-celery-deployskip-cronjob-deployskip-daemon-deploy