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]
 
 
 # ---------------------------------------------------------------------------

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
 # ---------------------------------------------------------------------------