lekss361/gendesign

Fork 0

feat(tradein/team): team-management API — employees CRUD, quotas, stats (#2554) #2563

Merged

lekss361 merged 2 commits from feat/tradein-team-api into main

2026-07-30 18:38:26 +00:00

lekss361 commented

2026-07-30 17:53:20 +00:00

Owner

Closes #2554 (эпик #2549, шаг 4/8).

app/api/v1/team.py — POST /employees, PATCH /employees/{id}, GET /employees, GET /employees/{id}/history; identity только из session-cookie (legacy X-Authenticated-User на уровне идентичности не принимается — барьер поверх rbac_guard)
app/schemas/team.py — pydantic-модели, ASCII-regex на username (^[A-Za-z0-9._-]{3,64}$ — закрывает latin-1 коллизию из ревью #2561)
Org-изоляция: _authorize_employee() — manager + чужой manager_id → 404 (не 403, не палим существование); в POST manager_id из тела для manager молча игнорируется → принудительно свой id; для admin — валидация что указанный id существует и role='manager'
Блокировка (is_active=false) → revoke_user_sessions(), сессия отзывается немедленно (покрыто тестом)
Квоты — upsert в account_quota_overrides существующим паттерном, account_quota.py не тронут
История — user_events (estimate_request) LEFT JOIN trade_in_estimates, пагинация limit≤200/offset

Тесты: +22 (org-изоляция негативно, employee → 403, без сессии → 401, не-ASCII → 422, дубль → 409, ревок сессий при блокировке, квота в статусе). uv run pytest -q: 2807 passed, 1 failed — pre-existing test_search_cache_hit (локальный env, на CI зелёный). rbac.py/auth_session.py/account_quota.py не менялись.

Closes #2554 (эпик #2549, шаг 4/8). - `app/api/v1/team.py` — `POST /employees`, `PATCH /employees/{id}`, `GET /employees`, `GET /employees/{id}/history`; identity только из session-cookie (legacy `X-Authenticated-User` на уровне идентичности не принимается — барьер поверх rbac_guard) - `app/schemas/team.py` — pydantic-модели, ASCII-regex на username (`^[A-Za-z0-9._-]{3,64}$` — закрывает latin-1 коллизию из ревью #2561) - **Org-изоляция**: `_authorize_employee()` — manager + чужой `manager_id` → **404** (не 403, не палим существование); в POST `manager_id` из тела для manager молча игнорируется → принудительно свой id; для admin — валидация что указанный id существует и role='manager' - Блокировка (`is_active=false`) → `revoke_user_sessions()`, сессия отзывается немедленно (покрыто тестом) - Квоты — upsert в `account_quota_overrides` существующим паттерном, `account_quota.py` не тронут - История — `user_events` (estimate_request) LEFT JOIN `trade_in_estimates`, пагинация limit≤200/offset Тесты: +22 (org-изоляция негативно, employee → 403, без сессии → 401, не-ASCII → 422, дубль → 409, ревок сессий при блокировке, квота в статусе). `uv run pytest -q`: 2807 passed, 1 failed — pre-existing `test_search_cache_hit` (локальный env, на CI зелёный). `rbac.py`/`auth_session.py`/`account_quota.py` не менялись.

lekss361 added 1 commit 2026-07-30 17:53:21 +00:00

feat(tradein/team): team-management API — employees CRUD, quotas, stats (#2554 )

CI Trade-In / changes (pull_request) Successful in 10s

Details

CI / changes (pull_request) Successful in 8s

Details

CI Trade-In / frontend-checks (pull_request) Has been skipped

Details

CI / backend-tests (pull_request) Has been skipped

Details

CI / frontend-tests (pull_request) Has been skipped

Details

CI / openapi-codegen-check (pull_request) Has been skipped

Details

CI Trade-In / backend-tests (pull_request) Successful in 1m47s

Details

712c56f456

Session-only identity (current_team_actor, admin|manager) поверх tradein_users/
tradein_sessions (#2552 foundation). Org-изоляция manager <-> employee через
manager_id: чужой/несуществующий employee_id -> 404 (не 403 — не палим
существование), POST с чужим manager_id в теле от manager игнорируется
(принудительно свой id). Квота — upsert в account_quota_overrides (существующий
паттерн, без правки account_quota.py). История оценок — user_events LEFT JOIN
trade_in_estimates. Team-события (employee_created/blocked/unblocked/
password_reset/quota_changed) без пароля в payload.

lekss361 reviewed 2026-07-30 18:08:47 +00:00

lekss361 left a comment

Author

Owner

Deep code review — PR #2563 (head `712c56f4`)

Verdict: CHANGES REQUESTED (1 HIGH). Не мержу.

Проверка не по тестам автора: собран независимый харнесс, где реальный SQL из team.py исполняется на настоящем движке (in-memory SQLite), а не матчится по строке — фильтры, бинды и upsert реально выполняются. 27 adversarial-кейсов, 26 зелёных, 1 красный — он ниже. Полный pytest tests на ветке: 2807 passed / 9 skipped (единственный local-fail test_search_cache_hit — env-артефакт порядка тестов, на CI зелёный). CI на 712c56f4 зелёный, включая CI Trade-In / backend-tests.

HIGH — reset пароля не убивает сессии сотрудника

app/api/v1/team.py:323-327 — revoke_user_sessions вызывается ТОЛЬКО при body.is_active is False. При new_password (строки 291-295, 314-315) хеш меняется, а строки в tradein_sessions остаются.

Эмпирика: PATCH {"new_password": "BrandNew1!"} → 200, хеш обновлён (verify_password новым паролем = True), при этом SELECT * FROM tradein_sessions WHERE token='tok_emp_stolen' возвращает живую строку.

Почему это HIGH, а не «косметика»:

смена пароля сотруднику — штатная реакция на компрометацию/увольнение; сейчас она не выкидывает уже открытую сессию;
get_session_user (auth_session.py:113-131) делает sliding-refresh: активная сессия продлевает себе expires_at на каждый запрос, т.е. чужая сессия живёт бесконечно, а не «до конца TTL»;
докстринг самой функции (auth_session.py:184-186) прямо называет смену пароля своим use-case.

Фикс (self-lockout невозможен: _fetch_employee_row фильтрует role='employee', свою/админскую строку не пропатчить):

    if body.is_active is False or body.new_password is not None:
        revoke_user_sessions(db, employee_id)

Плюс регресс-тест: сессия сотрудника исчезает после reset пароля.

MEDIUM

DoD «Origin/Referer-check на POST/PATCH» (#2554) не выполнен. Ни в team.py, ни глобально в приложении нет проверки Origin/Referer/CSRF-токена, а auth — cookie-based. Практический риск низкий: cookie ставится с samesite="lax" (auth.py:130), а тело обязано быть JSON — cross-site POST/PATCH куку не донесёт. Но это надо либо реализовать, либо явно зафиксировать SameSite-обоснование (докстринг + комментарий в issue), иначе следующая ручка потеряет и это.
N+1 + отсутствие пагинации в GET /employees (team.py:417-421). Замер: 10 сотрудников → 23 запроса (2N+3: сессия ×2, список, затем user_limit + account_estimate_usage на каждого). Список ничем не ограничен — admin без ?manager_id вытянет всех. Предложение: один запрос с LEFT JOIN account_quota_overrides + LEFT JOIN account_estimate_usage по текущему периоду, либо limit/offset как в history.
RBAC-слой team-роуты фактически не гейтит — расходится с докстрингом team.py:3-14 и с DoD «gate в rbac.py». rbac_guard сверяет ВНЕШНИЙ путь (_EXTERNAL_PREFIX + path, rbac.py:235), поэтому /api/v1/team/** в списке manager'а (auth_session.py:208) не матчится никогда, а employee проходит по своему /trade-in/**. Реально employee режется вторым барьером current_team_actor → 403 (проверено на всех 4 роутах). Т.е. дыры нет, но defense-in-depth ровно один слой: новая team-ручка без Depends(current_team_actor) окажется доступна employee. Одна строка в DB_ROLE_PATHS (deny "/trade-in/api/v1/team/**" для employee) закрывает это.

LOW

team.py:403 — f-string в text() для сборки WHERE. Инъекции нет (интерполируются только литералы, manager_id идёт биндом — проверено «1 OR 1=1» / «1;DROP TABLE …» → 422), но правило проекта «никаких f-string в SQL» нарушено и грепу такие места не видны. Лучше два статических запроса.
team.py:301-303 — COALESCE не даёт очистить display_name/org_name/email в NULL (только перезаписать). Фронту (шаг 6) может понадобиться сброс поля.
_upsert_quota_override (team.py:131-150) перетирает note строкой team-api: set by <actor> — прежняя пометка гранта теряется. Флаг unlimited при этом сохраняется (проверено: был unlimited=1, после PATCH monthly_limit=25 остался 1) — то есть менеджер может выставить лимит, который молча не применится; в ответе это видно через quota.unlimited, так что не блокер.
Нет аудит-события на попытку кросс-орг доступа (404 в _authorize_employee) — это как раз тот сигнал, который стоит видеть в user_events.

Что проверено эмпирически (зелёное)

Org-изоляция — все четыре роута:

PATCH чужого employee → 404, и в БД строка не изменилась: is_active остался true, display_name NULL, старый пароль валиден;
GET /employees менеджером A → только его сотрудники (чужие и «сироты» с manager_id IS NULL не видны); ?manager_id=<B> игнорируется, выдача не расширяется;
GET /employees/{чужой}/history → 404;
POST с manager_id менеджера B → создан под A (проверено в БД, не только в ответе).

Эскалация:

role/is_active в теле POST игнорируются → в БД role='employee';
PATCH по id админа / другого менеджера / самого себя → 404, пароли админа и менеджера B не изменились, account_quota_overrides для менеджера не появился (квоту себе не поднять);
PATCH с manager_id/role/username в теле → поля не меняются;
POST с username существующего админа → 409 до какого-либо quota-upsert.

Authn: без cookie → 401 на всех 4 роутах; протухшая сессия → 401; сессия деактивированного менеджера → 401; legacy X-Authenticated-User: admin без cookie → 401 (session-only заявка докстринга подтверждена); employee → 403 на всех 4 роутах.

Инъекции/границы: manager_id = 1 OR 1=1 / 1;DROP TABLE tradein_users / '-- → 422, таблица цела; limit 0/-1/201/1e9 → 422, limit=200 → 200; offset=-1 → 422; username аб_вг/ab/65 символов/adm'--/<script> → 422. Паттерн :x::type в файлах PR отсутствует, ruff чистый.

Квоты: unlimited-грант переживает upsert; account_estimate_usage.used не сбрасывается сменой лимита (было 7 — осталось 7, и ответ отдаёт used=7); monthly_limit 0/-5 → 422. account_quota.py не тронут (diff = ровно 4 файла), логика 191-й миграции не задета.

Пароли: hash_password применяется в обоих путях, длинный (>72 байт) и пустой пароль → 422 без создания строки; в ответе POST нет ни пароля, ни слова password; в аудит-событиях пароля нет.

История: возвращаются только события своего сотрудника (события другого юзера и не-estimate_request отфильтрованы), LEFT JOIN подтягивает результат только своей оценки. Индексы под оба запроса есть (user_events_username_created_at_idx, tradein_users_manager_id_idx).

Дальше

Один фикс (HIGH) + регресс-тест — и PR мержится. MEDIUM 1-3 можно отдельными issue, но 3 — одна строка, дешевле сделать здесь же.

## Deep code review — PR #2563 (head 712c56f4) Verdict: **CHANGES REQUESTED** (1 HIGH). Не мержу. Проверка не по тестам автора: собран независимый харнесс, где **реальный SQL из `team.py` исполняется на настоящем движке (in-memory SQLite)**, а не матчится по строке — фильтры, бинды и upsert реально выполняются. 27 adversarial-кейсов, 26 зелёных, 1 красный — он ниже. Полный `pytest tests` на ветке: 2807 passed / 9 skipped (единственный local-fail `test_search_cache_hit` — env-артефакт порядка тестов, на CI зелёный). CI на 712c56f4 зелёный, включая `CI Trade-In / backend-tests`. ### HIGH — reset пароля не убивает сессии сотрудника `app/api/v1/team.py:323-327` — `revoke_user_sessions` вызывается ТОЛЬКО при `body.is_active is False`. При `new_password` (строки 291-295, 314-315) хеш меняется, а строки в `tradein_sessions` остаются. Эмпирика: PATCH `{"new_password": "BrandNew1!"}` → 200, хеш обновлён (`verify_password` новым паролем = True), при этом `SELECT * FROM tradein_sessions WHERE token='tok_emp_stolen'` возвращает живую строку. Почему это HIGH, а не «косметика»: - смена пароля сотруднику — штатная реакция на компрометацию/увольнение; сейчас она не выкидывает уже открытую сессию; - `get_session_user` (`auth_session.py:113-131`) делает sliding-refresh: активная сессия продлевает себе `expires_at` на каждый запрос, т.е. чужая сессия живёт **бесконечно**, а не «до конца TTL»; - докстринг самой функции (`auth_session.py:184-186`) прямо называет смену пароля своим use-case. Фикс (self-lockout невозможен: `_fetch_employee_row` фильтрует `role='employee'`, свою/админскую строку не пропатчить): ```python if body.is_active is False or body.new_password is not None: revoke_user_sessions(db, employee_id) ``` Плюс регресс-тест: сессия сотрудника исчезает после reset пароля. ### MEDIUM 1. **DoD «Origin/Referer-check на POST/PATCH» (#2554) не выполнен.** Ни в `team.py`, ни глобально в приложении нет проверки Origin/Referer/CSRF-токена, а auth — cookie-based. Практический риск низкий: cookie ставится с `samesite="lax"` (`auth.py:130`), а тело обязано быть JSON — cross-site POST/PATCH куку не донесёт. Но это надо либо реализовать, либо явно зафиксировать SameSite-обоснование (докстринг + комментарий в issue), иначе следующая ручка потеряет и это. 2. **N+1 + отсутствие пагинации в `GET /employees`** (`team.py:417-421`). Замер: 10 сотрудников → **23 запроса** (`2N+3`: сессия ×2, список, затем `user_limit` + `account_estimate_usage` на каждого). Список ничем не ограничен — admin без `?manager_id` вытянет всех. Предложение: один запрос с `LEFT JOIN account_quota_overrides` + `LEFT JOIN account_estimate_usage` по текущему периоду, либо `limit/offset` как в history. 3. **RBAC-слой team-роуты фактически не гейтит** — расходится с докстрингом `team.py:3-14` и с DoD «gate в `rbac.py`». `rbac_guard` сверяет ВНЕШНИЙ путь (`_EXTERNAL_PREFIX + path`, `rbac.py:235`), поэтому `/api/v1/team/**` в списке manager'а (`auth_session.py:208`) не матчится никогда, а employee проходит по своему `/trade-in/**`. Реально employee режется вторым барьером `current_team_actor` → 403 (проверено на всех 4 роутах). Т.е. дыры нет, но defense-in-depth ровно один слой: новая team-ручка без `Depends(current_team_actor)` окажется доступна employee. Одна строка в `DB_ROLE_PATHS` (deny `"/trade-in/api/v1/team/**"` для `employee`) закрывает это. ### LOW - `team.py:403` — f-string в `text()` для сборки `WHERE`. Инъекции нет (интерполируются только литералы, `manager_id` идёт биндом — проверено «1 OR 1=1» / «1;DROP TABLE …» → 422), но правило проекта «никаких f-string в SQL» нарушено и грепу такие места не видны. Лучше два статических запроса. - `team.py:301-303` — `COALESCE` не даёт очистить `display_name`/`org_name`/`email` в NULL (только перезаписать). Фронту (шаг 6) может понадобиться сброс поля. - `_upsert_quota_override` (`team.py:131-150`) перетирает `note` строкой `team-api: set by <actor>` — прежняя пометка гранта теряется. Флаг `unlimited` при этом сохраняется (проверено: был `unlimited=1`, после PATCH `monthly_limit=25` остался `1`) — то есть менеджер может выставить лимит, который молча не применится; в ответе это видно через `quota.unlimited`, так что не блокер. - Нет аудит-события на попытку кросс-орг доступа (404 в `_authorize_employee`) — это как раз тот сигнал, который стоит видеть в `user_events`. ### Что проверено эмпирически (зелёное) Org-изоляция — все четыре роута: - PATCH чужого employee → 404, и в БД строка не изменилась: `is_active` остался true, `display_name` NULL, старый пароль валиден; - `GET /employees` менеджером A → только его сотрудники (чужие и «сироты» с `manager_id IS NULL` не видны); `?manager_id=<B>` игнорируется, выдача не расширяется; - `GET /employees/{чужой}/history` → 404; - POST с `manager_id` менеджера B → создан под A (проверено в БД, не только в ответе). Эскалация: - `role`/`is_active` в теле POST игнорируются → в БД `role='employee'`; - PATCH по id админа / другого менеджера / самого себя → 404, пароли админа и менеджера B не изменились, `account_quota_overrides` для менеджера не появился (квоту себе не поднять); - PATCH с `manager_id`/`role`/`username` в теле → поля не меняются; - POST с username существующего админа → 409 до какого-либо quota-upsert. Authn: без cookie → 401 на всех 4 роутах; протухшая сессия → 401; сессия деактивированного менеджера → 401; legacy `X-Authenticated-User: admin` без cookie → 401 (session-only заявка докстринга подтверждена); employee → 403 на всех 4 роутах. Инъекции/границы: `manager_id` = `1 OR 1=1` / `1;DROP TABLE tradein_users` / `'--` → 422, таблица цела; `limit` 0/-1/201/1e9 → 422, `limit=200` → 200; `offset=-1` → 422; username `аб_вг`/`ab`/65 символов/`adm'--`/`<script>` → 422. Паттерн `:x::type` в файлах PR отсутствует, ruff чистый. Квоты: `unlimited`-грант переживает upsert; `account_estimate_usage.used` не сбрасывается сменой лимита (было 7 — осталось 7, и ответ отдаёт `used=7`); `monthly_limit` 0/-5 → 422. `account_quota.py` не тронут (diff = ровно 4 файла), логика 191-й миграции не задета. Пароли: `hash_password` применяется в обоих путях, длинный (>72 байт) и пустой пароль → 422 без создания строки; в ответе POST нет ни пароля, ни слова password; в аудит-событиях пароля нет. История: возвращаются только события своего сотрудника (события другого юзера и не-`estimate_request` отфильтрованы), LEFT JOIN подтягивает результат только своей оценки. Индексы под оба запроса есть (`user_events_username_created_at_idx`, `tradein_users_manager_id_idx`). ### Дальше Один фикс (HIGH) + регресс-тест — и PR мержится. MEDIUM 1-3 можно отдельными issue, но 3 — одна строка, дешевле сделать здесь же.

bot-backend referenced this pull request from a commit

2026-07-30 18:26:43 +00:00

fix(tradein/team): revoke сессий при смене пароля + Origin-check + батч-квоты (#2554)

bot-backend added 1 commit 2026-07-30 18:26:43 +00:00

fix(tradein/team): revoke сессий при смене пароля + Origin-check + батч-квоты (#2554 )

CI Trade-In / changes (pull_request) Successful in 10s

Details

CI / changes (pull_request) Successful in 11s

Details

CI Trade-In / frontend-checks (pull_request) Has been skipped

Details

CI / backend-tests (pull_request) Has been skipped

Details

CI / frontend-tests (pull_request) Has been skipped

Details

CI / openapi-codegen-check (pull_request) Has been skipped

Details

CI Trade-In / backend-tests (pull_request) Successful in 1m56s

Details

4ac3971061

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.

lekss361 reviewed 2026-07-30 18:38:20 +00:00

lekss361 left a comment

Author

Owner

Deep code review — fixup `4ac39710` (PR #2563)

Verdict: APPROVE, мержу. HIGH закрыт, регрессий не нашёл. Остаются два 🟡 на follow-up (ниже) — оба latent, не блокируют.

Проверял тем же независимым харнессом (реальный SQL роутера на настоящем движке), +31 новый кейс на дельту. Полный pytest tests на 4ac39710: 2815 passed / 9 skipped, ruff чистый, CI зелёный включая явный CI Trade-In / backend-tests.

HIGH — закрыт, проверено end-to-end

Не только «строка удалена»: сессия сотрудника ДО reset'а резолвится (GET team-роута → 403 по роли, т.е. rbac её видит), ПОСЛЕ PATCH {"new_password": …} та же cookie даёт 401 — доступ реально разорван, а не «строка пропала из таблицы».

Дополнительно проверено:

self-lockout невозможен: manager PATCH'ит свой id → 404, admin свой id → 404, сессии обоих живы и продолжают работать (role='employee'-фильтр в _fetch_employee_row отсекает раньше revoke);
нет кросс-орг DoS: manager A шлёт {"new_password", "is_active": false} на сотрудника manager B → 404, и сессия чужого сотрудника НЕ снесена (revoke строго после _authorize_employee);
блокировка по-прежнему ревокает (регресс не поехал).

Origin-check — дыр не нашёл, легитимный поток не сломан

Матрица из 12 векторов, все как ожидалось:

заголовок	значение	итог
Origin	`https://gendsgn.ru`	201
Origin	`https://gendsgn.ru.evil.com`	403
Origin	`https://evil.com/?x=https://gendsgn.ru`	403
Origin	`http://gendsgn.ru` (scheme)	403
Origin	`https://gendsgn.ru:443`	403
Origin	`https://user@gendsgn.ru`	403
Origin	`null` / `not-a-url`	403
Referer	`https://gendsgn.ru/trade-in/team`	201
Referer	`https://gendsgn.ru.evil.com/x`, `https://evil.com/https://gendsgn.ru`	403

Префиксных/суффиксных обходов нет — сравнение точное (scheme://netloc vs whitelist), а не startswith. Origin выигрывает у Referer (подделать Referer при живом Origin нельзя). 403 отдаётся ДО мутации: после отбитого PATCH is_active, пароль, сессии и quota-override остались нетронутыми.

Про «оба заголовка отсутствуют → пропускаем» — согласен, дыры не создаёт:

на unsafe-методах (POST/PATCH) браузер шлёт Origin всегда, в т.ч. при mode:'no-cors' и при form-POST; подавить его через Referrer-Policy нельзя (это про Referer);
PATCH вообще недоступен в no-cors, а form-POST не может выставить application/json — такой запрос отвалится на 422 в FastAPI, не дойдя до логики;
значит «оба отсутствуют» = не браузер, а у не-браузера нет и cookie.

Легитимный поток не ломается: www.gendsgn.ru в Caddyfile — 301 на apex, приложение живёт только на https://gendsgn.ru, ровно это и лежит в прод CORS_ORIGINS. SSR/curl изнутри контейнера идут по skip-ветке.

GET-роуты намеренно не гейтятся Origin'ом — корректно (нет мутации, кросс-сайтовое чтение ответа режет CORS).

Батч-квоты — 5 запросов вместо 23, но одно расхождение семантики (🟡 follow-up)

Замер подтверждён: 5 запросов на N=10 (было 23). Эквивалентность _batch_quota_status vs account_quota.get_status прогнал попарно на кейсах: без override, с numeric-лимитом, used>0, used>limit (remaining=0, не отрицательный), unlimited-грант, admin-роль, пустой список (без похода в БД), спецсимволы в username (o'brien, кириллица, a;DROP TABLE …--, %_wild — бинд держит, таблица цела).

Расхождение ровно одно, и оно из-за особенности старого is_unlimited:

# account_quota.is_unlimited
try:
    role = get_role(username)
except KeyError:
    return False          # <- в БД вообще не идёт

Для юзера, которого НЕТ в auth/roles.yaml (а это каждый созданный через team-API сотрудник), is_unlimited возвращает False, не заглядывая в account_quota_overrides. _batch_quota_status же честно читает override.unlimited для любого. Итог: при unlimited=true у DB-only сотрудника список покажет quota.unlimited=true, а реальный энфорсмент (check_and_raise/increment) продолжит считать лимит.

Почему не блокирую: направление fail-safe (показываем свободу, а ограничиваем строже — не наоборот), и сегодня недостижимо — unlimited=true стоит только у kopylov/praktika (миграция 191), оба есть в roles.yaml, а сам team-API теперь всегда пишет unlimited=false. Но докстринг _batch_quota_status утверждает «семантика ИДЕНТИЧНА» — это неправда, и грабли выстрелят, когда безлимит впервые выдадут DB-сотруднику. Фикс на 4 строки:

        try:
            role = get_role(username)
        except KeyError:
            unlimited = False          # как в is_unlimited: нет в roles.yaml → limited
        else:
            unlimited = role == "admin" or bool(override is not None and override["unlimited"])

Пагинация — границы ок, сортировка без тай-брейкера (🟡 follow-up)

limit 0/-1/201/1e9 → 422, limit=200 → 200, offset=-1 → 422, offset за пределами → [], страницы бьются корректно и не пересекаются.

Но ORDER BY created_at DESC без тай-брейкера при LIMIT/OFFSET неоднозначен, а created_at DEFAULT now() — это timestamp транзакции: сид #2557, вставляющий пачку юзеров одной транзакцией, даст всем БАЙТ-В-БАЙТ одинаковый created_at. Тогда порядок между страницами в PG не гарантирован → строки могут дублироваться/теряться при листании. На SQLite мой кейс с одинаковым created_at прошёл (там порядок детерминирован по rowid) — то есть это риск, а не воспроизведённый баг. Лечится одним словом: ORDER BY created_at DESC, id DESC.

Мелочи (не требуют действий)

unlimited=false при явной установке лимита — семантика правильная (иначе лимит молча не применялся бы); note через COALESCE сохраняет прежнюю причину гранта («пилот, грант 2026-07-27» пережил PATCH), новый override получает сгенерированный note. Путь до praktika/kopylov недостижим: они не role='employee' → 404, их override'ы не тронуты (проверено).
Мой прежний тест «unlimited переживает upsert» теперь красный — ожидаемо, поведение изменено намеренно.
= ANY(CAST(:x AS text[])) со списком в бинде — прод-проверенный паттерн репозитория (search_query.py:88, deactivate_stale_avito.py:76, analysis_runs/repository.py:178), на SQLite не проверяем by design.

Итог

Мержу. Два 🟡 выше — отдельным follow-up (предлагаю приклеить к шагу #2557/#2558 эпика, оба по 1-4 строки).

## Deep code review — fixup 4ac39710 (PR #2563) Verdict: **APPROVE**, мержу. HIGH закрыт, регрессий не нашёл. Остаются два 🟡 на follow-up (ниже) — оба latent, не блокируют. Проверял тем же независимым харнессом (реальный SQL роутера на настоящем движке), +31 новый кейс на дельту. Полный `pytest tests` на 4ac39710: **2815 passed / 9 skipped**, ruff чистый, CI зелёный включая явный `CI Trade-In / backend-tests`. ### HIGH — закрыт, проверено end-to-end Не только «строка удалена»: сессия сотрудника ДО reset'а резолвится (GET team-роута → 403 по роли, т.е. rbac её видит), ПОСЛЕ `PATCH {"new_password": …}` та же cookie даёт **401** — доступ реально разорван, а не «строка пропала из таблицы». Дополнительно проверено: - **self-lockout невозможен**: manager PATCH'ит свой id → 404, admin свой id → 404, сессии обоих живы и продолжают работать (`role='employee'`-фильтр в `_fetch_employee_row` отсекает раньше revoke); - **нет кросс-орг DoS**: manager A шлёт `{"new_password", "is_active": false}` на сотрудника manager B → 404, и сессия чужого сотрудника НЕ снесена (revoke строго после `_authorize_employee`); - блокировка по-прежнему ревокает (регресс не поехал). ### Origin-check — дыр не нашёл, легитимный поток не сломан Матрица из 12 векторов, все как ожидалось: | заголовок | значение | итог | |---|---|---| | Origin | `https://gendsgn.ru` | 201 | | Origin | `https://gendsgn.ru.evil.com` | 403 | | Origin | `https://evil.com/?x=https://gendsgn.ru` | 403 | | Origin | `http://gendsgn.ru` (scheme) | 403 | | Origin | `https://gendsgn.ru:443` | 403 | | Origin | `https://user@gendsgn.ru` | 403 | | Origin | `null` / `not-a-url` | 403 | | Referer | `https://gendsgn.ru/trade-in/team` | 201 | | Referer | `https://gendsgn.ru.evil.com/x`, `https://evil.com/https://gendsgn.ru` | 403 | Префиксных/суффиксных обходов нет — сравнение точное (`scheme://netloc` vs whitelist), а не `startswith`. `Origin` выигрывает у `Referer` (подделать Referer при живом Origin нельзя). 403 отдаётся ДО мутации: после отбитого PATCH `is_active`, пароль, сессии и quota-override остались нетронутыми. Про «оба заголовка отсутствуют → пропускаем» — согласен, дыры не создаёт: - на unsafe-методах (POST/PATCH) браузер шлёт `Origin` всегда, в т.ч. при `mode:'no-cors'` и при form-POST; подавить его через `Referrer-Policy` нельзя (это про Referer); - PATCH вообще недоступен в no-cors, а form-POST не может выставить `application/json` — такой запрос отвалится на 422 в FastAPI, не дойдя до логики; - значит «оба отсутствуют» = не браузер, а у не-браузера нет и cookie. Легитимный поток не ломается: `www.gendsgn.ru` в Caddyfile — 301 на apex, приложение живёт только на `https://gendsgn.ru`, ровно это и лежит в прод `CORS_ORIGINS`. SSR/curl изнутри контейнера идут по skip-ветке. GET-роуты намеренно не гейтятся Origin'ом — корректно (нет мутации, кросс-сайтовое чтение ответа режет CORS). ### Батч-квоты — 5 запросов вместо 23, но одно расхождение семантики (🟡 follow-up) Замер подтверждён: **5 запросов** на N=10 (было 23). Эквивалентность `_batch_quota_status` vs `account_quota.get_status` прогнал попарно на кейсах: без override, с numeric-лимитом, `used>0`, `used>limit` (remaining=0, не отрицательный), unlimited-грант, admin-роль, пустой список (без похода в БД), спецсимволы в username (`o'brien`, кириллица, `a;DROP TABLE …--`, `%_wild` — бинд держит, таблица цела). Расхождение ровно одно, и оно из-за особенности старого `is_unlimited`: ```python # account_quota.is_unlimited try: role = get_role(username) except KeyError: return False # <- в БД вообще не идёт ``` Для юзера, которого НЕТ в `auth/roles.yaml` (а это каждый созданный через team-API сотрудник), `is_unlimited` возвращает False, не заглядывая в `account_quota_overrides`. `_batch_quota_status` же честно читает `override.unlimited` для любого. Итог: при `unlimited=true` у DB-only сотрудника список покажет `quota.unlimited=true`, а реальный энфорсмент (`check_and_raise`/`increment`) продолжит считать лимит. Почему не блокирую: направление fail-safe (показываем свободу, а ограничиваем строже — не наоборот), и сегодня недостижимо — `unlimited=true` стоит только у `kopylov`/`praktika` (миграция 191), оба есть в `roles.yaml`, а сам team-API теперь всегда пишет `unlimited=false`. Но докстринг `_batch_quota_status` утверждает «семантика ИДЕНТИЧНА» — это неправда, и грабли выстрелят, когда безлимит впервые выдадут DB-сотруднику. Фикс на 4 строки: ```python try: role = get_role(username) except KeyError: unlimited = False # как в is_unlimited: нет в roles.yaml → limited else: unlimited = role == "admin" or bool(override is not None and override["unlimited"]) ``` ### Пагинация — границы ок, сортировка без тай-брейкера (🟡 follow-up) `limit` 0/-1/201/1e9 → 422, `limit=200` → 200, `offset=-1` → 422, `offset` за пределами → `[]`, страницы бьются корректно и не пересекаются. Но `ORDER BY created_at DESC` без тай-брейкера при `LIMIT/OFFSET` неоднозначен, а `created_at DEFAULT now()` — это **timestamp транзакции**: сид #2557, вставляющий пачку юзеров одной транзакцией, даст всем БАЙТ-В-БАЙТ одинаковый `created_at`. Тогда порядок между страницами в PG не гарантирован → строки могут дублироваться/теряться при листании. На SQLite мой кейс с одинаковым `created_at` прошёл (там порядок детерминирован по rowid) — то есть это риск, а не воспроизведённый баг. Лечится одним словом: `ORDER BY created_at DESC, id DESC`. ### Мелочи (не требуют действий) - `unlimited=false` при явной установке лимита — семантика правильная (иначе лимит молча не применялся бы); `note` через `COALESCE` сохраняет прежнюю причину гранта («пилот, грант 2026-07-27» пережил PATCH), новый override получает сгенерированный note. Путь до praktika/kopylov недостижим: они не `role='employee'` → 404, их override'ы не тронуты (проверено). - Мой прежний тест «unlimited переживает upsert» теперь красный — ожидаемо, поведение изменено намеренно. - `= ANY(CAST(:x AS text[]))` со списком в бинде — прод-проверенный паттерн репозитория (`search_query.py:88`, `deactivate_stale_avito.py:76`, `analysis_runs/repository.py:178`), на SQLite не проверяем by design. ### Итог Мержу. Два 🟡 выше — отдельным follow-up (предлагаю приклеить к шагу #2557/#2558 эпика, оба по 1-4 строки).