ayurishchevandClaude Opus 5.5
744a025960
Пентест (docs/reviews/2026-09-27-pentest.md) и план 031 (Swagger, TLS) — план, не реализован.
032 Роль superadmin (без организации) и привязка admin/viewer к одной организации:
users.organization_id + CHECK, audit_log.organization_id (миграции 0010-0012);
require_org/scope_org во всех чтениях и записях, журнал и «Обзор» в границах
организации; пользователи, организации, типы устройств, настройки журнала — только superadmin.
033 Исправление находок ревью 032 (docs/reviews/2026-09-27-changes-032-review.md,
docs/reviews/2026-09-27-codebase-review.md):
- FK audit_log.organization_id ON DELETE SET NULL (миграция 0013) — удаление организаций;
- проверка организации в предпросмотре подсети;
- инвариант «роль — организация» по итоговому состоянию (повышение снимает организацию,
понижение требует её), 422/404 вместо обезличенных 409;
- одинаковый 404 для чужих и несуществующих объектов (VRF, устройство, parent_id, оператор);
- отказы удаления в журнале организации, счётчики типов в пределах организации;
- UI: живое поле «Организация» в диалоге пользователя, бейдж superadmin; род в текстах 404.
README актуализирован под ролевую модель.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
151 lines
20 KiB
Markdown
151 lines
20 KiB
Markdown
# Ревью и тестирование изменения 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 не готово к коммиту.
|