From 4ac3971061ec644d81188b0217214be429913b69 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Thu, 30 Jul 2026 21:23:32 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein/team):=20revoke=20=D1=81=D0=B5?= =?UTF-8?q?=D1=81=D1=81=D0=B8=D0=B9=20=D0=BF=D1=80=D0=B8=20=D1=81=D0=BC?= =?UTF-8?q?=D0=B5=D0=BD=D0=B5=20=D0=BF=D0=B0=D1=80=D0=BE=D0=BB=D1=8F=20+?= =?UTF-8?q?=20Origin-check=20+=20=D0=B1=D0=B0=D1=82=D1=87-=D0=BA=D0=B2?= =?UTF-8?q?=D0=BE=D1=82=D1=8B=20(#2554)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review PR #2563: - HIGH: PATCH new_password не ревокал старые сессии сотрудника — sliding-refresh продлевал скомпрометированную/чужую сессию бесконечно. Теперь revoke_user_sessions вызывается при is_active=False ИЛИ new_password (self-lockout невозможен — _fetch_employee_row фильтрует role='employee'). - Medium1: Origin/Referer-check (защита CSRF) на POST/PATCH team-роутах против settings.cors_origins; оба заголовка отсутствуют (curl-смоук внутри контейнера) — пропускаем, не ломаем прод-смоук. - Medium2: батч-квота для GET /employees (_batch_quota_status) — было 2N+3 SQL-запросов на N сотрудников, стало 5 плоско (замерено: N=10 -> 5 запросов). Добавлена пагинация limit(default 50, max 200)/offset. - Заодно: статические WHERE-ветки вместо f-string-сборки в list_employees; monthly_limit теперь сбрасывает account_quota_overrides.unlimited=false и не затирает существующий note. --- tradein-mvp/backend/app/api/v1/team.py | 233 +++++++++++++++++---- tradein-mvp/backend/tests/test_team_api.py | 152 +++++++++++++- 2 files changed, 346 insertions(+), 39 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/team.py b/tradein-mvp/backend/app/api/v1/team.py index 8da7a6e1..f75b1624 100644 --- a/tradein-mvp/backend/app/api/v1/team.py +++ b/tradein-mvp/backend/app/api/v1/team.py @@ -25,6 +25,7 @@ from __future__ import annotations import logging from dataclasses import dataclass from typing import Annotated, Any +from urllib.parse import urlparse from fastapi import APIRouter, Depends, HTTPException, Query, Request from sqlalchemy import text @@ -32,6 +33,7 @@ from sqlalchemy.engine import RowMapping from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session +from app.core.auth import get_role from app.core.config import settings from app.core.db import get_db from app.core.password import hash_password @@ -92,6 +94,47 @@ async def current_team_actor( ) +def _origin_host_allowed(candidate: str) -> bool: + """True если scheme://netloc *candidate* совпадает с одним из `settings.cors_origins`. + + `cors_origins` уже является источником правды для «какие origin'ы это наш + фронт» (см. CORSMiddleware в app/main.py, ENV CORS_ORIGINS) — переиспользуем + его вместо нового хардкода.""" + try: + parsed = urlparse(candidate) + except ValueError: + return False + if not parsed.scheme or not parsed.netloc: + return False + origin = f"{parsed.scheme}://{parsed.netloc}" + return origin in settings.cors_origins + + +def _require_same_origin(request: Request) -> None: + """CSRF defense-in-depth (issue #2554 DoD) для state-changing team-роутов + (POST/PATCH): `Origin` (или `Referer` как fallback) обязан матчить один из + `settings.cors_origins`, иначе 403. + + Оба заголовка отсутствуют → ПРОПУСКАЕМ (не 403). Причина: это единственный + надёжный сигнал non-browser клиента в этом стеке — curl-смоуки внутри + контейнера (см. `.claude/rules/tradein.md` "Тестировать HTTP только ВНУТРИ + контейнера", `docker exec tradein-backend curl ...`) не шлют ни один из этих + заголовков, а реальный браузер (fetch/XHR/form) ВСЕГДА прикладывает Origin + на unsafe-методах (POST/PATCH) — так что "оба отсутствуют" практически + невозможно для настоящего кросс-сайтового CSRF через браузер. Session-cookie + уже стоит на `SameSite=Lax` (см. `app.api.v1.auth.login`) — это первый рубеж + против CSRF, Origin-check — второй. + """ + candidate = request.headers.get("origin") or request.headers.get("referer") + if candidate is None: + return + if not _origin_host_allowed(candidate): + logger.warning( + "team: Origin/Referer mismatch %r on %s — possible CSRF", candidate, request.url.path + ) + raise HTTPException(status_code=403, detail="origin not allowed") + + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -131,14 +174,22 @@ def _authorize_employee(actor: TeamActor, row: RowMapping | None) -> RowMapping: def _upsert_quota_override( db: Session, username: str, monthly_limit: int, actor_username: str ) -> None: + """Upsert персонального лимита. Явная установка monthly_limit — сигнал "хочу + numeric-квоту", поэтому ВСЕГДА сбрасывает `unlimited=false` (иначе лимит может + молча не применяться — прежний unlimited-грант выигрывал бы у нового limit). + `note` — НЕ затирается, если уже задан (`COALESCE`): не перезаписываем + человеко-читаемую причину прошлого гранта (напр. "пилот, грант ...") молча + сгенерированной строкой; note проставляется только при первом upsert записи. + """ db.execute( text( """ - INSERT INTO account_quota_overrides (username, monthly_limit, note) - VALUES (:username, CAST(:monthly_limit AS integer), :note) + INSERT INTO account_quota_overrides (username, monthly_limit, unlimited, note) + VALUES (:username, CAST(:monthly_limit AS integer), false, :note) ON CONFLICT (username) DO UPDATE SET monthly_limit = EXCLUDED.monthly_limit, - note = EXCLUDED.note, + unlimited = false, + note = COALESCE(account_quota_overrides.note, EXCLUDED.note), updated_at = now() """ ), @@ -150,6 +201,83 @@ def _upsert_quota_override( ) +def _batch_quota_status(db: Session, usernames: list[str]) -> dict[str, dict[str, Any]]: + """Батч-версия `account_quota.get_status` для N сотрудников — 2 SQL-запроса + вместо 2N (было 2N+3 на GET /employees, HIGH/Medium2 review PR #2563). + + Семантика ИДЕНТИЧНА `account_quota.is_unlimited`/`user_limit`/`get_status`: + unlimited = admin-роль (roles.yaml, in-memory, без похода в БД) ИЛИ + `account_quota_overrides.unlimited=true`; limit = override.monthly_limit, + иначе глобальный `account_quota.MONTHLY_LIMIT`. + """ + if not usernames: + return {} + + overrides = ( + db.execute( + text( + """ + SELECT username, monthly_limit, unlimited + FROM account_quota_overrides + WHERE username = ANY(CAST(:usernames AS text[])) + """ + ), + {"usernames": usernames}, + ) + .mappings() + .all() + ) + override_by_username = {r["username"]: r for r in overrides} + + period = account_quota.current_period() + usage_rows = ( + db.execute( + text( + """ + SELECT username, used + FROM account_estimate_usage + WHERE username = ANY(CAST(:usernames AS text[])) AND period_month = :period + """ + ), + {"usernames": usernames, "period": period}, + ) + .mappings() + .all() + ) + used_by_username = {r["username"]: r["used"] for r in usage_rows} + + result: dict[str, dict[str, Any]] = {} + for username in usernames: + override = override_by_username.get(username) + try: + is_admin_role = get_role(username) == "admin" + except KeyError: + is_admin_role = False + unlimited = is_admin_role or bool(override is not None and override["unlimited"]) + limit = ( + int(override["monthly_limit"]) + if override is not None and override["monthly_limit"] is not None + else account_quota.MONTHLY_LIMIT + ) + used = used_by_username.get(username, 0) + if unlimited: + result[username] = { + "limit": limit, + "used": used, + "remaining": limit, + "unlimited": True, + } + else: + remaining = max(0, limit - max(0, used)) + result[username] = { + "limit": limit, + "used": used, + "remaining": remaining, + "unlimited": False, + } + return result + + def _employee_out(row: RowMapping, quota: dict[str, Any]) -> EmployeeOut: return EmployeeOut( id=row["id"], @@ -174,6 +302,7 @@ async def create_employee( body: EmployeeCreateRequest, actor: Annotated[TeamActor, Depends(current_team_actor)], db: Annotated[Session, Depends(get_db)], + _origin_check: Annotated[None, Depends(_require_same_origin)], ) -> EmployeeOut: """Создать сотрудника. Роль всегда `employee`. @@ -277,12 +406,18 @@ async def update_employee( body: EmployeeUpdateRequest, actor: Annotated[TeamActor, Depends(current_team_actor)], db: Annotated[Session, Depends(get_db)], + _origin_check: Annotated[None, Depends(_require_same_origin)], ) -> EmployeeOut: """Частичное обновление сотрудника — block/unblock, лимит, профиль, пароль. manager может патчить ТОЛЬКО своих (manager_id == actor.user_id), иначе 404. - При is_active=False — обязательно revoke всех сессий (иначе блокировка не - подействует до истечения TTL текущей сессии сотрудника). + При is_active=False ИЛИ смене пароля (new_password) — обязательно revoke всех + сессий (HIGH, deep-review PR #2563): без этого блокировка/reset не подействуют + до истечения TTL текущей сессии сотрудника — хуже того, sliding-refresh + (`app.services.auth_session.get_session_user`) продлевает `expires_at` на + КАЖДОМ запросе, так что скомпрометированная/чужая сессия живёт неограниченно + долго, а не «до TTL». `revoke_user_sessions` сам называет смену пароля своим + use-case — см. его докстринг. """ row = _fetch_employee_row(db, employee_id) row = _authorize_employee(actor, row) @@ -320,10 +455,12 @@ async def update_employee( if body.monthly_limit is not None: _upsert_quota_override(db, row["username"], body.monthly_limit, actor.username) - if body.is_active is False: + if body.is_active is False or body.new_password is not None: # Обязательно ПОСЛЕ UPDATE, ДО финального commit — revoke_user_sessions # коммитит сам (см. app.services.auth_session), это флашит и наш - # предшествующий UPDATE/quota-upsert в той же сессии. + # предшествующий UPDATE/quota-upsert в той же сессии. Self-lockout + # невозможен: _fetch_employee_row фильтрует role='employee', actor + # (admin|manager) никогда не может патчить сам себя через этот роут. revoke_user_sessions(db, employee_id) db.commit() @@ -381,44 +518,68 @@ async def update_employee( # --------------------------------------------------------------------------- +# Два статических варианта WHERE (НЕ f-string/динамическая сборка — Medium/ +# "заодно" review PR #2563: значения биндятся параметрами и без того безопасны, +# но статические ветки не провоцируют будущие правки в сторону конкатенации SQL). +_LIST_EMPLOYEES_BY_MANAGER_SQL = text( + """ + SELECT id, username, display_name, org_name, email, is_active, manager_id, created_at + FROM tradein_users + WHERE role = 'employee' AND manager_id = :manager_id + ORDER BY created_at DESC + LIMIT :limit OFFSET :offset + """ +) + +_LIST_EMPLOYEES_ALL_SQL = text( + """ + SELECT id, username, display_name, org_name, email, is_active, manager_id, created_at + FROM tradein_users + WHERE role = 'employee' + ORDER BY created_at DESC + LIMIT :limit OFFSET :offset + """ +) + + @router.get("/employees", response_model=list[EmployeeOut]) async def list_employees( actor: Annotated[TeamActor, Depends(current_team_actor)], db: Annotated[Session, Depends(get_db)], manager_id: Annotated[int | None, Query()] = None, + limit: Annotated[int, Query(ge=1, le=200)] = 50, + offset: Annotated[int, Query(ge=0)] = 0, ) -> list[EmployeeOut]: - """Список сотрудников. manager видит только своих; admin — всех, опц. ?manager_id=.""" - params: dict[str, Any] = {} - where = "WHERE role = 'employee'" + """Список сотрудников. manager видит только своих; admin — всех, опц. ?manager_id=. + + Квота — ОДИН батч-запрос на всю страницу (`_batch_quota_status`), не N+1 + (Medium2, review PR #2563: было 2N+3 SQL-запросов на N сотрудников). + """ if actor.role == "manager": - where += " AND manager_id = :manager_id" - params["manager_id"] = actor.user_id - elif manager_id is not None: - where += " AND manager_id = :manager_id" - params["manager_id"] = manager_id - - rows = ( - db.execute( - text( - f""" - SELECT id, username, display_name, org_name, email, is_active, - manager_id, created_at - FROM tradein_users - {where} - ORDER BY created_at DESC - """ - ), - params, + rows = ( + db.execute( + _LIST_EMPLOYEES_BY_MANAGER_SQL, + {"manager_id": actor.user_id, "limit": limit, "offset": offset}, + ) + .mappings() + .all() + ) + elif manager_id is not None: + rows = ( + db.execute( + _LIST_EMPLOYEES_BY_MANAGER_SQL, + {"manager_id": manager_id, "limit": limit, "offset": offset}, + ) + .mappings() + .all() + ) + else: + rows = ( + db.execute(_LIST_EMPLOYEES_ALL_SQL, {"limit": limit, "offset": offset}).mappings().all() ) - .mappings() - .all() - ) - result: list[EmployeeOut] = [] - for row in rows: - quota = account_quota.get_status(db, row["username"]) - result.append(_employee_out(row, quota)) - return result + quota_by_username = _batch_quota_status(db, [row["username"] for row in rows]) + return [_employee_out(row, quota_by_username[row["username"]]) for row in rows] # --------------------------------------------------------------------------- diff --git a/tradein-mvp/backend/tests/test_team_api.py b/tradein-mvp/backend/tests/test_team_api.py index ae72124d..57a2c6b7 100644 --- a/tradein-mvp/backend/tests/test_team_api.py +++ b/tradein-mvp/backend/tests/test_team_api.py @@ -48,6 +48,7 @@ class _Store: self.estimates: dict[str, dict[str, Any]] = {} # estimate_id -> result fields self.events: list[dict[str, Any]] = [] # user_events rows (history source) self._next_id = 1 + self.query_count = 0 # db.execute() calls — N+1 regression guard (review PR #2563) def add_user( self, @@ -169,6 +170,7 @@ class _FakeDB: sql = str(stmt) p = params or {} s = self.store + s.query_count += 1 # ---- tradein_sessions ---- if "INSERT INTO tradein_sessions" in sql: @@ -258,6 +260,8 @@ class _FakeDB: if "manager_id" in p: rows = [u for u in rows if u["manager_id"] == p["manager_id"]] rows = sorted(rows, key=lambda u: u["created_at"], reverse=True) + offset, limit = p.get("offset", 0), p.get("limit", len(rows)) + rows = rows[offset : offset + limit] return _Result( [ { @@ -315,15 +319,29 @@ class _FakeDB: user["password_hash"] = p["password_hash"] return _Result([]) - # ---- account_quota_overrides upsert ---- + # ---- account_quota_overrides upsert: unlimited always reset to False, + # note preserved (COALESCE) if a row already existed — mirrors real SQL. if "INSERT INTO account_quota_overrides" in sql: + existing_override = s.quota_overrides.get(p["username"]) + preserved_note = ( + existing_override["note"] if existing_override is not None else None + ) or p["note"] s.quota_overrides[p["username"]] = { "monthly_limit": p["monthly_limit"], "unlimited": False, - "note": p["note"], + "note": preserved_note, } return _Result([]) + # ---- account_quota_overrides: batch (list_employees) ---- + if "SELECT username, monthly_limit, unlimited" in sql: + rows = [ + {"username": u, "monthly_limit": ov["monthly_limit"], "unlimited": ov["unlimited"]} + for u, ov in s.quota_overrides.items() + if u in p["usernames"] + ] + return _Result(rows) + if "SELECT unlimited FROM account_quota_overrides" in sql: override = s.quota_overrides.get(p["u"]) return _Result([{"unlimited": override["unlimited"]}] if override else []) @@ -332,7 +350,16 @@ class _FakeDB: override = s.quota_overrides.get(p["u"]) return _Result([{"monthly_limit": override["monthly_limit"]}] if override else []) - # ---- account_estimate_usage ---- + # ---- account_estimate_usage: batch (list_employees) ---- + if "SELECT username, used" in sql: + rows = [ + {"username": u, "used": used} + for (u, period), used in s.usage.items() + if u in p["usernames"] and period == p["period"] + ] + return _Result(rows) + + # ---- account_estimate_usage: single (account_quota.get_status) ---- if "SELECT used FROM account_estimate_usage" in sql: used = s.usage.get((p["u"], p["p"])) return _Result([{"used": used}] if used is not None else []) @@ -619,6 +646,28 @@ def test_block_employee_revokes_sessions(client: TestClient, store: _Store) -> N assert any(e["event_type"] == "employee_blocked" for e in _EVENTS) +def test_reset_password_revokes_old_sessions(client: TestClient, store: _Store) -> None: + """HIGH (deep-review PR #2563): смена пароля обязана ревокать ВСЕ существующие + сессии сотрудника — иначе скомпрометированная/чужая сессия переживает reset + (sliding-refresh в auth_session.py продлевает её бесконечно, а не «до TTL»).""" + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + emp_id = store.add_user( + "emp_a", hash_password("OldSecret1!"), role="employee", manager_id=mgr_id + ) + + emp_client = TestClient(client.app, base_url="https://testserver") + _login(emp_client, "emp_a", "OldSecret1!") + assert any(sess["user_id"] == emp_id for sess in store.sessions.values()) + + _login(client, "mgr_a", "Secret123!") + resp = client.patch(f"/api/v1/team/employees/{emp_id}", json={"new_password": "NewSecret2!"}) + assert resp.status_code == 200, resp.text + + # Password reset must have revoked ALL of emp_a's pre-existing sessions — + # not just when is_active is explicitly set to False. + assert not any(sess["user_id"] == emp_id for sess in store.sessions.values()) + + def test_unblock_employee_event(client: TestClient, store: _Store) -> None: mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") emp_id = store.add_user( @@ -700,6 +749,103 @@ def test_admin_sees_all_employees_and_filters_by_manager(client: TestClient, sto assert {e["username"] for e in resp_filtered.json()} == {"emp_a"} +def test_list_employees_query_count_is_not_n_plus_1(client: TestClient, store: _Store) -> None: + """Medium2 (deep-review PR #2563): pre-fix measured 2N+3 = 23 SQL queries for + N=10 employees (per-employee `account_quota.get_status`). Batch quota lookup + (`_batch_quota_status`) must keep the query count constant regardless of N.""" + store.add_user("admin1", hash_password("Secret123!"), role="admin") + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + for i in range(10): + store.add_user(f"emp_{i}", hash_password("Secret123!"), role="employee", manager_id=mgr_id) + + _login(client, "admin1", "Secret123!") + store.query_count = 0 # reset after login's own DB traffic + resp = client.get("/api/v1/team/employees") + assert resp.status_code == 200 + assert len(resp.json()) == 10 + # rbac_guard session lookup (1) + current_team_actor session lookup (1) + + # list query (1) + 2 batch quota queries = 5, flat regardless of N. + assert store.query_count <= 5, f"expected O(1) queries for N=10, got {store.query_count}" + + +def test_list_employees_pagination(client: TestClient, store: _Store) -> None: + store.add_user("admin1", hash_password("Secret123!"), role="admin") + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + for i in range(5): + store.add_user(f"emp_{i}", hash_password("Secret123!"), role="employee", manager_id=mgr_id) + + _login(client, "admin1", "Secret123!") + resp = client.get("/api/v1/team/employees", params={"limit": 2, "offset": 1}) + assert resp.status_code == 200 + assert len(resp.json()) == 2 + + +def test_list_employees_limit_max_200(client: TestClient, store: _Store) -> None: + store.add_user("admin1", hash_password("Secret123!"), role="admin") + _login(client, "admin1", "Secret123!") + resp = client.get("/api/v1/team/employees", params={"limit": 500}) + assert resp.status_code == 422 + + +# --------------------------------------------------------------------------- +# CSRF defense-in-depth — Origin/Referer check on state-changing team routes +# --------------------------------------------------------------------------- + + +def test_create_employee_origin_mismatch_403(client: TestClient, store: _Store) -> None: + store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + _login(client, "mgr_a", "Secret123!") + + resp = client.post( + "/api/v1/team/employees", + json={"username": "emp_evil", "password": "Secret123!"}, + headers={"Origin": "https://evil.example"}, + ) + assert resp.status_code == 403 + assert "emp_evil" not in store.users + + +def test_create_employee_origin_allowed(client: TestClient, store: _Store) -> None: + store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + _login(client, "mgr_a", "Secret123!") + + allowed_origin = config.settings.cors_origins[0] + resp = client.post( + "/api/v1/team/employees", + json={"username": "emp_ok", "password": "Secret123!"}, + headers={"Origin": allowed_origin}, + ) + assert resp.status_code == 201, resp.text + + +def test_create_employee_no_origin_no_referer_allowed(client: TestClient, store: _Store) -> None: + """curl-смоук внутри контейнера не шлёт ни Origin, ни Referer — не должен ломаться.""" + store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + _login(client, "mgr_a", "Secret123!") + + resp = client.post( + "/api/v1/team/employees", + json={"username": "emp_curl", "password": "Secret123!"}, + ) + assert resp.status_code == 201, resp.text + + +def test_patch_employee_origin_mismatch_403(client: TestClient, store: _Store) -> None: + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + emp_id = store.add_user( + "emp_a", hash_password("Secret123!"), role="employee", manager_id=mgr_id + ) + _login(client, "mgr_a", "Secret123!") + + resp = client.patch( + f"/api/v1/team/employees/{emp_id}", + json={"display_name": "hacked"}, + headers={"Referer": "https://evil.example/csrf.html"}, + ) + assert resp.status_code == 403 + assert store.users["emp_a"]["display_name"] != "hacked" + + # --------------------------------------------------------------------------- # GET /employees/{id}/history # ---------------------------------------------------------------------------