diff --git a/src/config.default.toml b/src/config.default.toml index a9b9b1ff..5bc17c6b 100644 --- a/src/config.default.toml +++ b/src/config.default.toml @@ -45,3 +45,10 @@ client_id = "aiod-api" # a private client, used by the backend client_id_swagger = "aiod-api-swagger" # a public client, used by the Swagger Frontend openid_connect_url = "http://localhost/aiod-auth/realms/aiod/.well-known/openid-configuration" scopes = "openid profile roles" + +# Rate limiting configuration +[rate_limits] +# Maximum number of review submissions per user within the time window +max_submissions_per_user = 10 +# Time window in seconds (default: 3600 = 1 hour) +submission_window_seconds = 3600 diff --git a/src/config.py b/src/config.py index 6b14fb7e..f03fe830 100644 --- a/src/config.py +++ b/src/config.py @@ -75,3 +75,4 @@ def log_configuration(): KEYCLOAK_CONFIG = CONFIG.get("keycloak", {}) DEV_CONFIG = CONFIG.get("dev", {}) REQUEST_TIMEOUT = CONFIG.get("dev", {}).get("request_timeout", None) +RATE_LIMIT_CONFIG = CONFIG.get("rate_limits", {}) diff --git a/src/dependencies/rate_limiter.py b/src/dependencies/rate_limiter.py new file mode 100644 index 00000000..53cc7681 --- /dev/null +++ b/src/dependencies/rate_limiter.py @@ -0,0 +1,73 @@ +""" +Per-user rate limiting for review submission requests (issue #663). + +Uses an in-memory rolling window to track submissions per user. +Connectors (users with platform_* roles) bypass rate limiting as they use +a separate workflow for systematic data migrations. +""" + +import datetime +import logging +from collections import defaultdict, deque + +from fastapi import Depends, HTTPException, status + +from authentication import KeycloakUser, get_user_or_raise +from config import RATE_LIMIT_CONFIG + +logger = logging.getLogger(__name__) + +# In-memory store: maps user subject_identifier -> deque of submission timestamps +_submission_timestamps: dict[str, deque[datetime.datetime]] = defaultdict(deque) + +MAX_SUBMISSIONS: int = RATE_LIMIT_CONFIG.get("max_submissions_per_user", 10) +WINDOW_SECONDS: int = RATE_LIMIT_CONFIG.get("submission_window_seconds", 3600) + + +def _clean_expired_entries(user_id: str) -> None: + """Remove timestamps that fall outside the current rolling window.""" + cutoff = datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta( + seconds=WINDOW_SECONDS + ) + timestamps = _submission_timestamps[user_id] + while timestamps and timestamps[0] < cutoff: + timestamps.popleft() + + +def check_submission_rate_limit( + user: KeycloakUser = Depends(get_user_or_raise), +) -> None: + """ + FastAPI dependency that enforces per-user rate limiting on review submissions. + + Connectors (platform roles) are exempt — they use the connector workflow + designed for systematic uploads (see issue #663). + + Raises: + HTTPException 429 if the user has exceeded max_submissions_per_user + within the submission_window_seconds rolling window. + """ + if user.is_connector: + return # connectors bypass rate limiting + + user_id = user._subject_identifier + _clean_expired_entries(user_id) + + timestamps = _submission_timestamps[user_id] + if len(timestamps) >= MAX_SUBMISSIONS: + window_minutes = WINDOW_SECONDS // 60 + logger.warning( + f"Rate limit exceeded for user {user.name!r}: " + f"{len(timestamps)}/{MAX_SUBMISSIONS} submissions in {window_minutes} min window." + ) + raise HTTPException( + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + detail=( + f"Rate limit exceeded: you may submit at most {MAX_SUBMISSIONS} " + f"review requests per {window_minutes} minutes. " + "Please try again later." + ), + ) + + # Record this submission timestamp + timestamps.append(datetime.datetime.now(datetime.timezone.utc)) diff --git a/src/routers/review_router.py b/src/routers/review_router.py index 624b1d8e..9094b4b5 100644 --- a/src/routers/review_router.py +++ b/src/routers/review_router.py @@ -23,6 +23,7 @@ from database.model.concept.concept import AIoDConcept from routers.helper_functions import get_router_by_type from database.model.helper_functions import get_asset_type_by_abbreviation +from dependencies.rate_limiter import check_submission_rate_limit from versioning import Version @@ -141,6 +142,7 @@ def get_submission( def _submit_resource( submission: SubmissionCreate, user: KeycloakUser = Depends(get_user_or_raise), + _rate_check: None = Depends(check_submission_rate_limit), ): id_to_type = { identifier: get_asset_type_by_abbreviation().get(identifier.split("_")[0]) diff --git a/src/tests/test_review_rate_limit.py b/src/tests/test_review_rate_limit.py new file mode 100644 index 00000000..9a4dc27e --- /dev/null +++ b/src/tests/test_review_rate_limit.py @@ -0,0 +1,120 @@ +""" +Tests for per-user rate limiting on review submission requests (issue #663). +""" + +import datetime +from collections import deque + +import pytest +from fastapi import HTTPException + +from authentication import KeycloakUser +from dependencies.rate_limiter import ( + MAX_SUBMISSIONS, + WINDOW_SECONDS, + _submission_timestamps, + check_submission_rate_limit, +) + + +def _make_user( + subject_id: str = "user-001", + name: str = "testuser", + roles: set[str] | None = None, +) -> KeycloakUser: + if roles is None: + roles = {"offline_access", "uma_authorization", "default-roles-aiod"} + return KeycloakUser(name=name, roles=roles, _subject_identifier=subject_id) + + +@pytest.fixture(autouse=True) +def _clear_rate_limit_state(): + """Ensure a clean rate limiter state for every test.""" + _submission_timestamps.clear() + yield + _submission_timestamps.clear() + + +class TestSubmissionRateLimit: + + def test_allows_submissions_within_limit(self): + """Submissions at or below the limit should pass.""" + user = _make_user() + for _ in range(MAX_SUBMISSIONS): + check_submission_rate_limit(user) + assert len(_submission_timestamps[user._subject_identifier]) == MAX_SUBMISSIONS + + def test_rejects_submission_over_limit(self): + """One submission beyond the limit must raise HTTP 429.""" + user = _make_user() + for _ in range(MAX_SUBMISSIONS): + check_submission_rate_limit(user) + + with pytest.raises(HTTPException) as exc_info: + check_submission_rate_limit(user) + assert exc_info.value.status_code == 429 + assert "Rate limit exceeded" in exc_info.value.detail + + def test_allows_after_window_expires(self): + """Submissions from outside the window should be cleaned and new ones allowed.""" + user = _make_user() + expired = datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta( + seconds=WINDOW_SECONDS + 1 + ) + _submission_timestamps[user._subject_identifier] = deque( + [expired] * MAX_SUBMISSIONS + ) + # All expired — should succeed + check_submission_rate_limit(user) + assert len(_submission_timestamps[user._subject_identifier]) == 1 + + def test_connector_bypasses_rate_limit(self): + """Users with platform_* roles (connectors) are never rate-limited.""" + connector = _make_user( + subject_id="connector-001", + roles={"platform_huggingface", "default-roles-aiod"}, + ) + for _ in range(MAX_SUBMISSIONS + 5): + check_submission_rate_limit(connector) + assert "connector-001" not in _submission_timestamps + + def test_independent_counters_per_user(self): + """Each user has an independent rate limit counter.""" + alice = _make_user(subject_id="alice", name="alice") + bob = _make_user(subject_id="bob", name="bob") + + for _ in range(MAX_SUBMISSIONS): + check_submission_rate_limit(alice) + + # Bob should still be free + check_submission_rate_limit(bob) + assert len(_submission_timestamps["bob"]) == 1 + + # Alice should be blocked + with pytest.raises(HTTPException) as exc_info: + check_submission_rate_limit(alice) + assert exc_info.value.status_code == 429 + + def test_partial_expiry_keeps_recent_entries(self): + """Expired entries are cleaned but recent ones are preserved.""" + user = _make_user() + now = datetime.datetime.now(datetime.timezone.utc) + expired = now - datetime.timedelta(seconds=WINDOW_SECONDS + 1) + recent = now - datetime.timedelta(seconds=60) + # 5 expired + 4 recent = 9 — under the limit + _submission_timestamps[user._subject_identifier] = deque( + [expired] * 5 + [recent] * 4 + ) + check_submission_rate_limit(user) + # 5 expired cleaned, 4 recent + 1 new = 5 + assert len(_submission_timestamps[user._subject_identifier]) == 5 + + def test_error_message_includes_limits(self): + """The 429 detail must include the configured limit and window.""" + user = _make_user() + for _ in range(MAX_SUBMISSIONS): + check_submission_rate_limit(user) + with pytest.raises(HTTPException) as exc_info: + check_submission_rate_limit(user) + assert str(MAX_SUBMISSIONS) in exc_info.value.detail + assert str(WINDOW_SECONDS // 60) in exc_info.value.detail