diff --git a/tradein-mvp/backend/app/api/v1/team.py b/tradein-mvp/backend/app/api/v1/team.py index f75b1624..e3e30ffc 100644 --- a/tradein-mvp/backend/app/api/v1/team.py +++ b/tradein-mvp/backend/app/api/v1/team.py @@ -205,10 +205,25 @@ def _batch_quota_status(db: Session, usernames: list[str]) -> dict[str, dict[str """Батч-версия `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`. + Семантика ИДЕНТИЧНА `account_quota.is_unlimited`/`user_limit`/`get_status` + (follow-up review PR #2563 п.2 — предыдущая версия расходилась: батч ВСЕГДА + читал `account_quota_overrides.unlimited`, а `is_unlimited` — ТОЛЬКО для + username, присутствующего в roles.yaml): + - username НЕ в roles.yaml (`get_role` → KeyError) → unlimited=False ВСЕГДА, + `account_quota_overrides.unlimited` даже не проверяется (roles.yaml — + источник правды "кто вообще может быть unlimited", override — "у кого + именно из известных roles.yaml-юзеров"). Сегодня недостижимо для DB-only + сотрудников team-API (`_upsert_quota_override` всегда пишет + `unlimited=false`), но станет достижимым при ручном UPDATE + `account_quota_overrides` или расширении roles.yaml — расхождение с + реальным enforcement (`check_and_raise`/`increment`, тот же `is_unlimited`) + было бы честной ложью в списке: "без лимита", который движок всё равно + считает. + - username в roles.yaml и role == admin → unlimited=True (без похода в БД). + - username в roles.yaml, role != admin → unlimited = override.unlimited. + limit = override.monthly_limit (читается для ЛЮБОГО username, без gate по + roles.yaml — так же ведёт себя `account_quota.user_limit`), иначе глобальный + `account_quota.MONTHLY_LIMIT`. """ if not usernames: return {} @@ -250,10 +265,17 @@ def _batch_quota_status(db: Session, usernames: list[str]) -> dict[str, dict[str for username in usernames: override = override_by_username.get(username) try: - is_admin_role = get_role(username) == "admin" + role = get_role(username) except KeyError: - is_admin_role = False - unlimited = is_admin_role or bool(override is not None and override["unlimited"]) + role = None + if role == "admin": + unlimited = True + elif role is not None: + unlimited = bool(override is not None and override["unlimited"]) + else: + # username не в roles.yaml — is_unlimited() короткое замыкание на + # False, override НЕ проверяется (см. докстринг выше). + unlimited = False limit = ( int(override["monthly_limit"]) if override is not None and override["monthly_limit"] is not None @@ -521,12 +543,21 @@ async def update_employee( # Два статических варианта WHERE (НЕ f-string/динамическая сборка — Medium/ # "заодно" review PR #2563: значения биндятся параметрами и без того безопасны, # но статические ветки не провоцируют будущие правки в сторону конкатенации SQL). +# +# ORDER BY created_at DESC, id DESC — тай-брейкер по `id` ОБЯЗАТЕЛЕН (follow-up +# review PR #2563 п.1): `created_at DEFAULT now()` — время ТРАНЗАКЦИИ, а bulk-seed +# (#2557) вставляет много юзеров одной транзакцией → идентичный timestamp у N строк. +# Без тай-брейкера порядок между страницами (LIMIT/OFFSET) на PostgreSQL для +# строк-«близнецов» не гарантирован — сотрудники пропадали/дублировались бы при +# постраничном листании. `id` монотонно растёт (BIGINT IDENTITY) — детерминированный +# tie-break без доп. индекса (созданные позже = бОльший id, тот же порядок что и +# намерение DESC-сортировки по времени). _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 + ORDER BY created_at DESC, id DESC LIMIT :limit OFFSET :offset """ ) @@ -536,7 +567,7 @@ _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 + ORDER BY created_at DESC, id DESC LIMIT :limit OFFSET :offset """ ) diff --git a/tradein-mvp/backend/tests/test_team_api.py b/tradein-mvp/backend/tests/test_team_api.py index 57a2c6b7..6a446885 100644 --- a/tradein-mvp/backend/tests/test_team_api.py +++ b/tradein-mvp/backend/tests/test_team_api.py @@ -61,6 +61,7 @@ class _Store: display_name: str | None = None, org_name: str | None = None, email: str | None = None, + created_at: datetime | None = None, ) -> int: uid = self._next_id self._next_id += 1 @@ -74,7 +75,7 @@ class _Store: "org_name": org_name, "email": email, "is_active": is_active, - "created_at": datetime.now(UTC), + "created_at": created_at or datetime.now(UTC), } return uid @@ -259,7 +260,10 @@ class _FakeDB: rows = [u for u in s.users.values() if u["role"] == "employee"] 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) + # Mirrors real SQL `ORDER BY created_at DESC, id DESC` — `id` tiebreak + # is REQUIRED for deterministic paging when created_at ties (follow-up + # review PR #2563 п.1, bulk-seed #2557 inserts many rows in one tx). + rows = sorted(rows, key=lambda u: (u["created_at"], u["id"]), reverse=True) offset, limit = p.get("offset", 0), p.get("limit", len(rows)) rows = rows[offset : offset + limit] return _Result( @@ -787,6 +791,51 @@ def test_list_employees_limit_max_200(client: TestClient, store: _Store) -> None assert resp.status_code == 422 +def test_list_employees_pagination_stable_with_identical_created_at( + client: TestClient, store: _Store +) -> None: + """Follow-up review PR #2563 п.1: `created_at DEFAULT now()` — время ТРАНЗАКЦИИ, + bulk-seed (#2557) вставляет много юзеров одной транзакцией → идентичный + timestamp у N+ строк. Без `id DESC` тай-брейкера порядок между страницами + на PostgreSQL для строк-«близнецов» не гарантирован — сотрудники пропадали/ + дублировались бы при постраничном листании. Вставляем 5 сотрудников с + ОДИНАКОВЫМ created_at, листаем limit=2 постранично — объединение страниц + обязано дать полный набор без дублей и пропусков.""" + store.add_user("admin1", hash_password("Secret123!"), role="admin") + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + same_ts = datetime.now(UTC) + expected_usernames = set() + for i in range(5): + username = f"emp_tie_{i}" + store.add_user( + username, + hash_password("Secret123!"), + role="employee", + manager_id=mgr_id, + created_at=same_ts, + ) + expected_usernames.add(username) + + _login(client, "admin1", "Secret123!") + + seen: list[str] = [] + offset = 0 + while True: + resp = client.get("/api/v1/team/employees", params={"limit": 2, "offset": offset}) + assert resp.status_code == 200, resp.text + page = [e["username"] for e in resp.json()] + if not page: + break + seen.extend(page) + offset += 2 + + assert len(seen) == len(expected_usernames), ( + f"page union has {len(seen)} entries (dupes or gaps), expected " + f"{len(expected_usernames)}: {seen}" + ) + assert set(seen) == expected_usernames + + # --------------------------------------------------------------------------- # CSRF defense-in-depth — Origin/Referer check on state-changing team routes # --------------------------------------------------------------------------- @@ -846,6 +895,65 @@ def test_patch_employee_origin_mismatch_403(client: TestClient, store: _Store) - assert store.users["emp_a"]["display_name"] != "hacked" +# --------------------------------------------------------------------------- +# _batch_quota_status unlimited semantics — must match account_quota.is_unlimited +# --------------------------------------------------------------------------- + + +def test_batch_quota_unlimited_ignored_for_non_roles_yaml_username( + client: TestClient, store: _Store +) -> None: + """Follow-up review PR #2563 п.2: `account_quota.is_unlimited` short-circuits + to False for a username NOT in roles.yaml — it never even reads + `account_quota_overrides.unlimited`. The batch quota status used by + GET /employees must agree, or the list would show "unlimited" for a quota + that real enforcement (check_and_raise/increment, same is_unlimited) does + NOT honor — a misleading display. `emp_ghost_unlimited` is a fresh DB-only + username guaranteed absent from roles.yaml.""" + store.add_user("admin1", hash_password("Secret123!"), role="admin") + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + store.add_user( + "emp_ghost_unlimited", hash_password("Secret123!"), role="employee", manager_id=mgr_id + ) + store.quota_overrides["emp_ghost_unlimited"] = { + "monthly_limit": 15, + "unlimited": True, + "note": "manual grant via SQL runbook (not through team-api)", + } + + _login(client, "admin1", "Secret123!") + resp = client.get("/api/v1/team/employees") + assert resp.status_code == 200, resp.text + entry = next(e for e in resp.json() if e["username"] == "emp_ghost_unlimited") + # DB override says unlimited=true, but username is NOT in roles.yaml — real + # enforcement would never see it, so the list must NOT claim "unlimited". + assert entry["quota"]["unlimited"] is False + assert entry["quota"]["limit"] == 15 + + +def test_batch_quota_unlimited_honored_for_roles_yaml_username( + client: TestClient, store: _Store +) -> None: + """Symmetric positive case: a username actually present in roles.yaml + (non-admin role) — `account_quota_overrides.unlimited=true` IS honored, same + as `account_quota.is_unlimited`. Uses `kopylov` — real prod pilot-role entry + in auth/roles.yaml (see app/core/auth.py module docstring).""" + store.add_user("admin1", hash_password("Secret123!"), role="admin") + mgr_id = store.add_user("mgr_a", hash_password("Secret123!"), role="manager") + store.add_user("kopylov", hash_password("Secret123!"), role="employee", manager_id=mgr_id) + store.quota_overrides["kopylov"] = { + "monthly_limit": 999, + "unlimited": True, + "note": "existing prod grant", + } + + _login(client, "admin1", "Secret123!") + resp = client.get("/api/v1/team/employees") + assert resp.status_code == 200, resp.text + entry = next(e for e in resp.json() if e["username"] == "kopylov") + assert entry["quota"]["unlimited"] is True + + # --------------------------------------------------------------------------- # GET /employees/{id}/history # ---------------------------------------------------------------------------