ipam_control/docs/reviews/2026-09-27-changes-032-review.md

# Ревью и тестирование изменения 032 — 2026-09-27

**Объём:** реализация `docs/changes/032-role-model-org-scope/PLAN.md` исполнителем на Haiku 4.5 (агент `ae8bd63b5568cb6ee`), незакоммиченные изменения
поверх `5210ba3`. 12 изменённых файлов, 3 новые миграции.
**Метод:** построчное чтение diff по каждому файлу, затем сквозные сценарии на стенде `ipam_control_006` (API напрямую + прямые SQL-запросы для диагностики),
затем регрессия `pytest`. Все находки ниже воспроизведены, не гипотетичны. Временные данные удалены.

## Итог

Модель прав (`require_org`/`scope_org`, разделение ролей по роутерам) реализована **широко и в целом корректно** — это подтверждено 16 из 24 сценариев ниже.
Но найдено **три серьёзных бага**, один из которых — **полная поломка удаления организаций** (регрессия ранее стабильной функции, изменение 007) — и они
подтверждаются штатным regression-прогоном: `pytest` даёт **3 упавших теста из 14**, и все три — прямое проявление находок №1–3.

**Рекомендация: не коммитить как есть.** Нужен ещё один проход правок (аналогично циклам 024/030 в этой же сессии) на находки №1–3 минимум; №4 желательно
в том же заходе, №5–8 — по желанию.

| # | Серьёзность | Файл(ы) | Кратко |
|---|---|---|---|
| 1 | **Критическая** | `alembic/versions/0012_audit_log_organization.py`, `app/models.py` | `audit_log.organization_id → organizations.id` без `ON DELETE SET NULL` — удаление организации сломано полностью |
| 2 | **Высокая** | `app/schemas.py` | Кросс-полевая проверка `organization_id` в `UserIn`/`UserUpdate` через `@field_validator` не срабатывает, если поле просто не передано в теле запроса |
| 3 | **Высокая** | `app/api/v1/users.py`, `web/app.js` | Повышение до `superadmin` / понижение с `superadmin` не работает при естественном вызове (без ручной синхронизации `organization_id`) |
| 4 | Средняя | `app/services.py` (`refuse_delete`) | Записи `*.delete_blocked` всегда `organization_id=NULL` — невидимы для админа организации в его собственном журнале |
| 5 | Низкая | `web/app.js` | Кнопка «Добавить устройство»: мёртвый код (`disabled` не поддерживается `btn()`), логика инвертирована по смыслу, хотя фактически не влияет |
| 6 | Низкая | `web/app.js`, `web/styles.css` | Бейдж роли `superadmin` использует класс `purple`, которого нет в CSS — рендерится без цвета |
| 7 | Инфо | `app/services.py` | `require_org`/`scope_org` делают `from app.models import Role` локально, хотя модуль уже импортирует из `app.models` на верхнем уровне |
| 8 | Низкая | `web/app.js` (`userDialog`) | Поле выбора организации не реагирует на смену роли в открытом диалоге (видимость считается по старой роли записи) |

---

## Находки

### 1. Удаление организации сломано полностью ✅ КРИТИЧНО
**Где:** `alembic/versions/0012_audit_log_organization.py` — `op.create_foreign_key(..., "audit_log", "organizations", ["organization_id"], ["id"])`
без `ondelete`; то же в `app/models.py` — `AuditLog.organization_id: Mapped[int | None] = mapped_column(ForeignKey("organizations.id"), index=True)`.

**Механизм:** `create_org` пишет `audit(..., organization_id=o.id)` при создании организации — то есть у *любой* организации сразу же появляется
ссылающаяся строка в `audit_log`. FK без `ON DELETE SET NULL` (по умолчанию `NO ACTION`/`RESTRICT`) не даёт удалить организацию, пока существует
хоть одна такая запись — а она существует всегда.

**Воспроизведение (минимальное):** создать организацию → сразу `DELETE /organizations/{id}` → 409 «Конфликт с существующими данными…»
(общий обработчик `IntegrityError`, без списка блокираторов — проверка `blockers()` до этого момента их не нашла, ошибка на уровне самого `DELETE`).
В логе PostgreSQL: `violates foreign key constraint "fk_audit_log_organization_id_organizations" ... Key (id)=(442) is still referenced from table "audit_log"`.

**Подтверждено регрессией:** `pytest tests/test_api.py::test_delete_organization` падает именно на этом.

**Исправление:** `ondelete="SET NULL"` у FK — и в модели, и в миграции (`op.create_foreign_key(..., ondelete="SET NULL")`). Это согласуется с уже принятым
для этой колонки принципом «история удалённых сущностей остаётся, `organization_id` уходит в `NULL`» — тем же, что применён к `entity_id`
(без FK вообще) и описан в самом плане 032 для backfill.

### 2. Валидатор `organization_id` не срабатывает при отсутствии поля в теле запроса ✅ ВЫСОКАЯ
**Где:** `app/schemas.py`, `UserIn`/`UserUpdate` — `@field_validator("organization_id")`.

**Причина (проверено экспериментально на минимальном примере с pydantic 2.13):** `@field_validator` **не вызывается**, если поле отсутствует
в переданных данных и используется его значение по умолчанию (`None`) — только если оно передано явно (в том числе `null`). Это штатное поведение
Pydantic v2, не баг библиотеки, но означает, что кросс-полевая проверка «admin/viewer обязаны иметь организацию» реально работает только тогда,
когда клиент явно прислал `"organization_id": null` — а не когда просто не указал ключ (обычный способ «не задавать поле» в JSON API).

**Последствие:** `POST /users {"username": "...", "password": "...", "role": "admin"}` (без ключа `organization_id`) не даёт ожидаемых 422.
Запрос доходит до `db.add()`/`flush()`, ловится `CheckConstraint` в БД → `IntegrityError` → попадает в блок `flush(db, "Пользователь с таким
логином уже существует")`, который **любую** `IntegrityError` подписывает этим текстом. Клиент получает 409 «Пользователь с таким логином уже
существует» для **несуществующего** логина — вводящее в заблуждение сообщение, маскирующее реальную причину.

**Подтверждено регрессией:** `pytest tests/test_users.py::test_users_management` падает ровно на этом первом же вызове
(`client.post("/users", json={"username": name, "password": "start-pass-123", "role": "viewer"})` — без `organization_id`).
Также `pytest tests/test_journal.py::test_clear_requires_password_and_locks_out` падает с `CheckViolation` на прямой SQL-вставке — показывает,
что БД-ограничение единственная линия защиты и она долетает как сырая ошибка там, где путь не идёт через API вообще.

**Исправление:** заменить `@field_validator("organization_id")` на `@model_validator(mode="after")` в обеих схемах — он выполняется всегда,
по итоговому состоянию модели, независимо от того, какие поля были явно переданы.

### 3. Смена роли на/с `superadmin` не работает при обычном вызове ✅ ВЫСОКАЯ
**Где:** `app/api/v1/users.py::update_user`; сопутствующий пробел в `web/app.js::userDialog`.

**Воспроизведено дважды, в обе стороны:**
- Повышение: `PATCH /users/{id} {"role": "superadmin"}` (без `organization_id`) на существующем `admin` с заполненной организацией → 409
  «Конфликт с существующими данными…» (тот же `CheckConstraint`, `organization_id` в БД не очищен). Роль не меняется, пользователь остаётся как был.
- Понижение: `PATCH /users/{id} {"role": "admin"}` (без `organization_id`) на существующем `superadmin` → тот же 409 (`organization_id` не проставлен,
  остаётся `NULL`, что нарушает ограничение уже для роли `admin`).
- Контрольный, «правильный» вызов с явным `organization_id` в теле — проходит (200). То есть путь существует, но требует от клиента знания
  о внутреннем ограничении БД, которое нигде не задокументировано и не проверяется на уровне API до `commit()`.

**Причина:** `update_user` строит `data = body.model_dump(exclude_unset=True, exclude_none=True)` и применяет её через `apply_update` без
дополнительной реконсиляции: если `role` меняется на/с `superadmin`, а `organization_id` в этом же запросе не передан, старое значение поля
остаётся нетронутым — и это ровно то состояние, которое ограничение в БД запрещает (кроме случая `is_active=false`, который здесь не применим).

**В UI это дополнительно усугублено:** `userDialog` при создании нового пользователя корректно исключает `organization_id` из тела, если выбрана
роль `superadmin` (`if (v.role !== "superadmin" && v.organization_id) body.organization_id = ...`), но **при редактировании существующего**
такой же проверки нет (`if (v.organization_id) body.organization_id = Number(v.organization_id);` — без учёта роли). Хуже того, поле-селектор
организации в форме скрывается по **прежней** роли записи (`u?.role === "superadmin"`), а не по текущему выбору в открытом select — так что
даже если оператор переключит роль на «Суперадминистратор» в уже открытом диалоге, поле организации останется видимым со старым значением
и уйдёт в тело запроса. Через UI сейчас **невозможно** повысить существующего администратора организации до суперадминистратора.

**Исправление:** в `update_user` — при вычислении финальной роли принудительно приводить `organization_id`: если итоговая роль `superadmin`,
записывать `u.organization_id = None` независимо от того, что пришло в запросе; если роль уходит от `superadmin` и `organization_id` не передан
явно — требовать его (422 с понятным текстом), а не полагаться на молчаливое сохранение старого (в данном случае отсутствующего) значения.
В `web/app.js` — тот же guard `v.role !== "superadmin"` в ветке `edit`, и видимость поля организации должна реагировать на текущее значение
селектора роли в форме, а не на исходную роль записи.

### 4. Отказы в удалении не попадают в журнал организации ✅ СРЕДНЯЯ
**Где:** `app/services.py::refuse_delete` — не тронута агентом; по-прежнему вызывает `audit(...)` без `organization_id`, и ни один из пяти call-site'ов
(`refs.py` ×4, `prefixes.py` ×1, `users.py` ×2) не передаёт его.

**Воспроизведено:** `admin` организации A пытается удалить используемый VRF своей же организации → корректно получает 409 (бизнес-правило работает).
Но `GET /audit?event_type=vrf.delete_blocked` от его же имени возвращает `{"total": 0}` — событие существует (видно суперадминистратору), но
`organization_id = NULL`, и `scope_org` его отфильтровывает. Админ организации не видит в своём журнале собственные отклонённые попытки удаления.

**Исправление:** добавить `organization_id: int | None = None` в сигнатуру `refuse_delete` и передавать его на всех вызовах (там же, где рядом
уже передаётся `organization_id` в соседние вызовы `audit()` для тех же сущностей — паттерн уже есть в `prefixes.py`/`refs.py` для успешных операций).

### 5–8. Мелкие (UI/стиль) 🔎
- **№5:** `screens.devices` — `btn(..., { disabled: true })`; функция `btn()` (`web/app.js`) не поддерживает ключ `disabled` — он молча игнорируется.
  Ветвление `isSuperadmin() ? btn(...) : btn(..., {disabled:true})` фактически рендерит один и тот же (рабочий) элемент в обеих ветках, поэтому
  функционально org-admin по-прежнему может добавлять устройства (это подтверждено сценарием), но код вводит в заблуждение и должен быть упрощён
  до безусловного `btn(...)`, как было раньше — устройства создаёт любой `admin` своей организации, не только `superadmin`.
- **№6:** `badge(u.role === "superadmin" ? "purple" : ...)` — в `web/styles.css` определены только `.badge.green/blue/red/amber`; `purple` рендерится
  без модификатора (стандартный серый бейдж), роль суперадминистратора визуально не выделяется.
- **№7:** `require_org`/`scope_org` (`app/services.py`) делают `from app.models import Role` внутри тела функции при каждом вызове, хотя модуль уже
  импортирует из `app.models` на верхнем уровне (`Address, AuditLog, Base`) — нет циркулярной причины для локального импорта, чисто стилистически.
- **№8:** см. разбор в находке №3 — видимость поля организации в `userDialog` не реактивна к живому выбору роли в форме.

---

## Что проверено и работает корректно ✅
Двадцать четыре сценария на стенде (организации/VRF/префиксы/устройства/операторы/типы устройств/пользователи/журнал/обзор), из них 16 без замечаний:
- Бутстрап-администратор из `.env` после миграций стал `superadmin` с `organization_id=NULL`.
- Изоляция чтения: `admin`/`viewer` организации видят только свою организацию в списках и по прямому `id` (404 на чужую, без утечки существования);
  явный `organization_id` в query-параметрах чужой организации — тоже 404, не пустой список (осознанное и разумное отличие от плана, не баг).
- Запись в своей организации: префиксы, устройства — создаются; в чужой — 404.
- `viewer` читает, любая попытка записи — 403.
- Типы устройств: читают все роли; создание/правка/удаление — только `superadmin` (403 для `admin` организации).
- Управление пользователями, `/journal/settings`, `/journal/clear` — только `superadmin` (403 для остальных).
- «Обзор» — счётчики и `recent_changes` корректно урезаны до организации вызывающего (когда организация вообще видна в журнале — см. находку №4).
- Блокиратор `"users"` при удалении организации с привязанными пользователями — корректно собирается и полностью виден в `diff.blocked_by` записи
  журнала (со списком логинов и `total`) — сама механика `refuse_delete`/`blockers` для этой новой группы работает без замечаний.
- `alembic check` — «No new upgrade operations detected», миграции 0010–0012 применяются на стенде без ручного вмешательства.
- `import app.main`, `node --check web/app.js` — чисто.

## Регрессия (`pytest -q`)
```
3 failed, 11 passed in 8.32s
FAILED tests/test_api.py::test_delete_organization          — находка №1
FAILED tests/test_journal.py::test_clear_requires_password_and_locks_out  — находка №2 (CheckViolation на прямой SQL-вставке без organization_id)
FAILED tests/test_users.py::test_users_management            — находка №2
```
Все три падения — прямое проявление находок №1–2, не случайные/посторонние сбои. Обновление самих тестов под новую модель (добавление
`organization_id` в фикстуры пользователей и т. п.) — отдельный, последующий шаг после того, как находки №1–3 будут исправлены в коде.

## Рекомендация
Исправить находки №1–3 (и желательно №4) отдельным заходом — по аналогии с циклами 024/030 в этой сессии: план → агент на правки кода →
повторное ревью и тесты. До этого изменение 032 не готово к коммиту.