Add user-specific rate limits with admin API management#440
Add user-specific rate limits with admin API management#440
Conversation
Introduces per-user submission rate limiting (hourly and daily caps)
configurable via the admin API. Users without an override are unrestricted.
- New migration: leaderboard.user_rate_limits table
- DB methods: get/set/delete user rate limits + submission rate checking
- Admin endpoints: GET/PUT/DELETE /admin/rate-limits/{user_id}
- Both submission endpoints check user rate limits (return 429 if exceeded)
- Tests for DB methods and all API endpoints
https://claude.ai/code/session_01LiSWKfcKe4riGZwMYdubiJ
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Adds per-user submission rate limiting (hourly/daily) with admin-managed overrides via new DB table + admin API endpoints, and enforces these limits on submission endpoints.
Changes:
- Added
leaderboard.user_rate_limitstable + DB CRUD/check helpers for per-user submission caps. - Enforced rate-limit checks in both submission endpoints (429 on exceeded limits).
- Added admin endpoints and tests for listing/getting/setting/deleting user rate limits.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_leaderboard_db.py |
Adds DB-level tests for rate limit CRUD and enforcement behavior. |
tests/test_admin_api.py |
Adds admin API tests for rate limit management endpoints. |
tests/conftest.py |
Ensures new table is truncated between tests. |
src/migrations/20260208_01_RkLmN-add-user-rate-limits.py |
Introduces the user_rate_limits table. |
src/libkernelbot/leaderboard_db.py |
Implements rate limit CRUD plus submission-rate checking logic. |
src/kernelbot/api/main.py |
Enforces per-user rate limits on submission endpoints and adds admin endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if max_per_hour is not None and (not isinstance(max_per_hour, int) or max_per_hour < 0): | ||
| raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer") | ||
|
|
||
| if max_per_day is not None and (not isinstance(max_per_day, int) or max_per_day < 0): |
There was a problem hiding this comment.
isinstance(x, int) also accepts booleans (True/False), which would silently become limits of 1/0. Use a stricter check (e.g., type(x) is int) to ensure only real integers are accepted.
| if max_per_hour is not None and (not isinstance(max_per_hour, int) or max_per_hour < 0): | |
| raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer") | |
| if max_per_day is not None and (not isinstance(max_per_day, int) or max_per_day < 0): | |
| if max_per_hour is not None and (type(max_per_hour) is not int or max_per_hour < 0): | |
| raise HTTPException(status_code=400, detail="max_submissions_per_hour must be a non-negative integer") | |
| if max_per_day is not None and (type(max_per_day) is not int or max_per_day < 0): |
| async def set_user_rate_limit( | ||
| user_id: str, | ||
| payload: dict, | ||
| _: Annotated[None, Depends(require_admin)], | ||
| db_context=Depends(get_db), | ||
| ) -> dict: |
There was a problem hiding this comment.
Using an untyped payload: dict loses FastAPI schema/validation and makes the API contract ambiguous. Prefer a Pydantic model (optional max_submissions_per_hour, max_submissions_per_day, note) so invalid types are rejected consistently and the OpenAPI docs reflect the request body.
| self.cursor.execute( | ||
| """ | ||
| INSERT INTO leaderboard.user_rate_limits | ||
| (user_id, max_submissions_per_hour, max_submissions_per_day, note) | ||
| VALUES (%s, %s, %s, %s) | ||
| ON CONFLICT (user_id) DO UPDATE SET | ||
| max_submissions_per_hour = EXCLUDED.max_submissions_per_hour, | ||
| max_submissions_per_day = EXCLUDED.max_submissions_per_day, | ||
| note = EXCLUDED.note, | ||
| updated_at = NOW() | ||
| RETURNING user_id, max_submissions_per_hour, max_submissions_per_day, | ||
| note, created_at, updated_at | ||
| """, | ||
| (user_id, max_submissions_per_hour, max_submissions_per_day, note), | ||
| ) |
There was a problem hiding this comment.
This insert is subject to the FK constraint (user_rate_limits.user_id → user_info.id). If an admin tries to set a limit for a non-existent user, Postgres will raise an integrity error that currently becomes a generic KernelBotError (likely surfacing as a 500). Catch integrity violations and return a domain-specific error so the API can respond with a clear 400/404.
| CREATE TABLE leaderboard.user_rate_limits ( | ||
| user_id TEXT PRIMARY KEY REFERENCES leaderboard.user_info(id), | ||
| max_submissions_per_hour INTEGER, | ||
| max_submissions_per_day INTEGER, | ||
| note TEXT, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
The schema allows negative limits, which would invert enforcement semantics and can slip in via non-API callers. Add CHECK constraints to enforce max_submissions_per_hour >= 0 / max_submissions_per_day >= 0 (while still allowing NULL).
| self.cursor.execute( | ||
| """ | ||
| SELECT | ||
| COUNT(*) FILTER (WHERE submission_time > NOW() - INTERVAL '1 hour') AS hourly_count, | ||
| COUNT(*) FILTER (WHERE submission_time > NOW() - INTERVAL '1 day') AS daily_count | ||
| FROM leaderboard.submission | ||
| WHERE user_id = %s | ||
| """, | ||
| (user_id,), | ||
| ) |
There was a problem hiding this comment.
This query runs on every submission for rate-limited users and will become expensive as leaderboard.submission grows unless it can use an index efficiently. Consider adding an index like (user_id, submission_time) (or confirm an existing one) to keep the hourly/daily counts fast.
| with db_context as db: | ||
| rate_check = db.check_user_submission_rate(user_info["user_id"]) | ||
| if not rate_check["allowed"]: | ||
| raise HTTPException(status_code=429, detail=f"Rate limit exceeded. {rate_check['retry_after']}") |
There was a problem hiding this comment.
For 429 responses, it’s best practice to include a Retry-After header (seconds or HTTP date). Since retry_after is currently a human string, consider also returning a machine-readable duration and setting HTTPException(..., headers={'Retry-After': ...}).
| raise HTTPException(status_code=429, detail=f"Rate limit exceeded. {rate_check['retry_after']}") | |
| retry_after_header = str(rate_check.get("retry_after_seconds", rate_check["retry_after"])) | |
| raise HTTPException( | |
| status_code=429, | |
| detail=f"Rate limit exceeded. {rate_check['retry_after']}", | |
| headers={"Retry-After": retry_after_header}, | |
| ) |
| def test_set_user_rate_limit(self, test_client, mock_backend): | ||
| """PUT /admin/rate-limits/{user_id} sets rate limit.""" | ||
| self._setup_db_mock(mock_backend) | ||
| mock_backend.db.set_user_rate_limit = MagicMock(return_value={ | ||
| "user_id": "123", "max_submissions_per_hour": 5, "max_submissions_per_day": 20, | ||
| "note": "restricted", "created_at": None, "updated_at": None, | ||
| }) |
There was a problem hiding this comment.
Given the FK constraint in the migration, setting a rate limit for a non-existent user_id is an important error path. Add an admin API test that PUT /admin/rate-limits/{user_id} returns a clear 400/404 when the user doesn’t exist (once the handler is updated to translate the DB integrity error).
Introduces per-user submission rate limiting (hourly and daily caps)
configurable via the admin API. Users without an override are unrestricted.
https://claude.ai/code/session_01LiSWKfcKe4riGZwMYdubiJ