-
Notifications
You must be signed in to change notification settings - Fork 56
feat: implement plan-based API rate limits #696
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
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.
Pull request overview
This PR implements a plan-based API rate limiting system using Django Rest Framework's throttling capabilities. Rate limits are configured per organisation plan tier (Free, Pro, Enterprise) and stored in Redis. The implementation includes database query optimizations in the authentication layer to prefetch organisation data needed for rate limit checks.
- Adds configurable rate limits per plan tier (Free: 120/min, Pro: 240/min, Enterprise: 1000/min by default)
- Implements custom throttle class that dynamically applies rate limits based on organisation plan
- Optimizes authentication queries to prefetch organisation relationships and avoid N+1 queries
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| backend/backend/settings.py | Adds rate limit configuration per plan tier, configures Redis cache backend for throttling, and sets up DRF throttle classes |
| backend/api/throttling.py | Implements PlanBasedRateThrottle class that applies per-user/service-account rate limits based on organisation plan |
| backend/api/auth.py | Optimizes database queries with select_related() to prefetch environment/app/organisation relationships; removes trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
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.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Signed-off-by: rohan <rohan.chaturvedi@protonmail.com>
- Removed default values for rate limits to rely on environment variables. - Updated Redis SSL configuration to require SSL certificate validation and added CA certificates path from environment variables.
| # If self-hosted return the default rate limit. If not set, this will disable throttling | ||
| if not CLOUD_HOSTED: | ||
| return settings.PLAN_RATE_LIMITS["DEFAULT"] | ||
| return settings.PLAN_RATE_LIMITS.get(plan, settings.PLAN_RATE_LIMITS["DEFAULT"]) |
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.
Plan rate limit fallback fails for unconfigured plans
In get_rate_for_plan, the expression settings.PLAN_RATE_LIMITS.get(plan, settings.PLAN_RATE_LIMITS["DEFAULT"]) doesn't correctly fall back to the default rate when a plan's rate is None. Since PLAN_RATE_LIMITS is populated with os.getenv() calls that return None for unset environment variables, the keys (FR, PR, EN) always exist in the dictionary with None values. Python's dict.get() only uses the default when the key is missing, not when the value is None. In cloud mode, if RATE_LIMIT_FREE is unset but RATE_LIMIT_DEFAULT is configured, Free plan users would get None as their rate, effectively disabling rate limiting for them.
Additional Locations (1)
| elif request.auth.get("service_token"): | ||
| ident = f"st_{request.auth['service_token'].id}" | ||
| else: | ||
| ident = f"anon_{ident}" |
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.
Missing fallback in cache key for unmatched auth types
In get_cache_key, when request.user.is_authenticated and request.auth is truthy but none of the three identity checks (org_member, service_account, service_token) match, the ident variable remains as the raw IP address without any prefix. The else clause adding the anon_ prefix only executes when the outer condition is falsy, not when the inner if-elif chain exhausts without a match. While currently unreachable with PhaseTokenAuthentication (which always sets one of these keys), this structure is fragile. Future auth changes could cause authenticated users to share IP-based rate limits instead of identity-based ones.
- Updated REDIS_AUTH to URL-encode the password for better security. - Added validation to ensure REDIS_CA_CERTS_PATH is set when using SSL. - Configured RQ_SSL_OPTIONS to require SSL certificate validation and include CA certificates path.
- Added support for Redis username in REDIS_AUTH. - Updated REDIS_AUTH construction to handle cases with and without username and password.
- Set REDIS_USERNAME to None if not provided, ensuring consistent behavior. - Added REDIS_USERNAME to RQ_QUEUES configuration for better Redis authentication.
- Added SSL options to the DATABASES configuration for PostgreSQL. - Configured SSL mode to require and conditionally include SSL root certificate based on environment variables.
- Removed unnecessary comments related to development settings and security warnings. - Streamlined the configuration for better readability and maintainability.
- Added ca-certificates package alongside curl for improved security and functionality. - Removed unnecessary line break for cleaner formatting.
- Included the AWS RDS CA bundle for the eu-central-1 region to enhance SSL connectivity. - Set appropriate permissions for the CA bundle file.
- Eliminated the check for REDIS_CA_CERTS_PATH when REDIS_SSL is true, simplifying the configuration. - Streamlined the Redis settings for improved clarity and maintainability.
- Changed DATABASE_SSL_CA_PATH to be used for PostgreSQL SSL settings, enhancing security. - Updated Redis SSL configuration to consistently reference REDIS_SSL_CA_PATH for clarity. - Improved maintainability by ensuring SSL options are conditionally set based on environment variables.
- Updated DATABASE_SSL_CA_PATH usage for PostgreSQL to enhance security with conditional SSL mode. - Revised Redis SSL settings to consistently use REDIS_SSL_CA_PATH, improving clarity and maintainability. - Ensured SSL options are dynamically set based on environment variables for better flexibility.
|
@nimish-ks - consider changing |
🔍 Overview
Adds a rate-limiting system for the REST API based on the DRF Throttling. Redis is used as the cache backend. Rate limits are applied per account, based on the account org plan in Cloud Mode, and based on the value of
RATE_LIMIT_DEFAULTin self hosted mode. When rate limited, the response is a429with aretry-afterheader indicating how many seconds to wait before re-trying a request🖼️ Screenshots or Demo
📝 Release Notes
Adds support for API rate limiting
🧪 Testing
Describe the testing strategy. List new tests added, existing tests modified, and any testing gaps.
💚 Did You...
- [ ] Update migrations (if required)- [ ] Regenerate graphql schema and types (if required)Note
Implements plan-scoped API rate limiting and wires it into key endpoints, with supporting configuration and tests.
PlanBasedRateThrottle(per-user/service-account/service-token cache keys) selecting rates fromPLAN_RATE_LIMITS(cloud) orDEFAULT(self-hosted)E2EESecretsView,PublicSecretsView, andaws_iam_authbackend/settings.pywithPLAN_RATE_LIMITS,REST_FRAMEWORK['DEFAULT_THROTTLE_RATES'], Redis cache config (optional SSL), and RQ queues; makes DBPORTint and adds optional Postgres SSL optionsPhaseTokenAuthenticationlookups viaselect_relatedwhen resolvingenvironmentcurl/ca-certificatesand installs AWS RDS eu-central-1 CA bundlepytest-django,pytest.ini,conftest.py, andtests/api/test_throttling.pycovering cache keys and plan-based rate selectionWritten by Cursor Bugbot for commit 00548fa. This will update automatically on new commits. Configure here.