Skip to content

feat(web-api): msCustomerGroup и ACL групп ресурсов для авторизованных - #677

Open
Ibochkarev wants to merge 4 commits into
feat/issue-659-catalog-resource-groupsfrom
feat/issue-669-customer-resource-groups
Open

feat(web-api): msCustomerGroup и ACL групп ресурсов для авторизованных#677
Ibochkarev wants to merge 4 commits into
feat/issue-659-catalog-resource-groupsfrom
feat/issue-669-customer-resource-groups

Conversation

@Ibochkarev

Copy link
Copy Markdown
Member

Описание

Продолжение #659/#666: анонимный фильтр групп ресурсов уже скрывает закрытые разделы, но авторизованный msCustomer для MODX всё ещё гость. Добавлена сущность msCustomerGroup со ссылкой на modUserGroup; видимость каталога считается через access_resource_groups и тот же SQL-контур, без второго реестра прав и без modUser на каждого покупателя.

Depends on #666 (база PR — ветка feat/issue-659-catalog-resource-groups).

Тип изменений

  • Исправление бага (non-breaking change)
  • Новая функциональность (non-breaking change)
  • Breaking change (изменение, ломающее обратную совместимость)
  • Рефакторинг (без изменения функциональности)
  • Документация
  • Другое (опишите):

Связанные Issues

Closes #669

Depends on #666 / Refs #659

Как это было протестировано?

cd core/components/minishop3
php -l src/Services/Catalog/CatalogResourceGroupVisibility.php
php -l src/Services/Catalog/CustomerResourceGroupResolver.php
# exit 0

php tests/CatalogResourceGroupVisibilityTest.php
# OK, exit 0

php tests/CustomerResourceGroupResolverTest.php
# OK, exit 0

php tests/ProfileQuickUpdateForbiddenTest.php
php tests/CustomerPublicDtoTest.php
# OK, exit 0

composer test:smoke
# OK smoke tests (101), exit 0

После деплоя: phinx migrate (таблица ms3_customer_groups + customer_group_id).

  • Ручное тестирование
  • Автоматические тесты (composer ci:php / composer test, npm run lint:ci, composer stan / GitHub Actions CI)
  • Тестирование на разных версиях PHP/MODX

Конфигурация тестирования:

Скриншоты (если применимо)

До После
n/a n/a

Чеклист

  • Код соответствует стилю проекта
  • Добавлены/обновлены комментарии в сложных местах
  • Изменения не ломают существующую функциональность
  • Лексиконы добавлены на двух языках (ru/en)
  • PHPStan проходит без новых ошибок (composer stan / CI job PHPStan)
  • ESLint проходит без ошибок (npm run lint:ci для Vue)
  • Обновлён CHANGELOG.md (для значимых изменений)

Дополнительные заметки

MVP: одна группа на покупателя, без Vue UI, без чистки корзины при потере доступа. Manager API: /api/mgr/customer-groups + customer_group_id в PUT /customers/{id}. Web profile не отдаёт и не пишет customer_group_id. Blocked/inactive покупатель остаётся на anonymous-гейте. После merge #666 перенацелить PR на beta.

@Ibochkarev Ibochkarev added priority: high Важно исправить в ближайшее время enhancement New feature or request labels Sep 9, 2026
@Ibochkarev
Ibochkarev requested a review from biz87 September 9, 2026 02:14
@Ibochkarev
Ibochkarev added this pull request to stack #678 September 9, 2026 02:20
@Ibochkarev

Copy link
Copy Markdown
Member Author

Подтянул фикс из #666 (anonymous principal = 0 + SQLite execution tests) в эту ветку. Protected-membership SQL для #669 использует ту же семантику.

@biz87

biz87 commented Sep 9, 2026

Copy link
Copy Markdown
Member

По существу PR готов — проверил все девять пунктов, критичных проблем нет. Не вливаю только потому, что база (#666) уходит на доработку, и есть неразрешённый конфликт с #681. Подробности ниже.

Что подтвердилось

Реализован именно выбранный вариант. msCustomerGroup содержит name, user_group_id, active и метки времени — никаких списков групп ресурсов внутри модели. Видимость считается запросом к access_resource_groups по principal:IN с теми же классами принципалов, что и в анонимном гейте. То есть права остаются в одном месте, в MODX, а второго реестра не появилось — это было главным опасением в #669.

modUser не создаётся — проверено грепом по всем изменённым файлам.

Аноним идёт ровно прежним путём. При отсутствии токен-сервиса, отсутствующем или истёкшем токене, заблокированном покупателе, покупателе без группы, неактивной группе и user_group_id = 0 резолвер возвращает пустой массив, а buildNotExistsSql() становится алиасом buildVisibilitySql(..., []). Второй ветки логики для анонима нет, все четыре потребителя переведены на единый applyForRequest().

Существующие покупатели не ломаются. customer_group_id добавлен как int unsigned NULL без дефолта, существующие записи получают NULL, что трактуется как отсутствие группы — обычный каталог остаётся доступен. Покрыто тестом.

В публичный ответ ничего не утекает. И customer_group_id, и user_group_id в блок-листе CustomerPublicDto, оба проверены тестами — тот же whitelist-паттерн, что мы проверяли в #639.

Миграция и схема согласованы. hasTable/hasColumn без префикса, проверки перед addColumn/addIndex, создание через createObjectContainer — тот же паттерн, что в 20260518120000_create_option_groups_and_migrate.php. Схема и mysql-модели совпадают по типам, null, дефолтам и индексам. Зарезервированных слов MySQL 8 в новых колонках нет.

Роуты закрыты PermissionMiddleware с теми же правами msorder_*, что уже используются для существующего /customers — переиспользование конвенции, а не новое ослабление.

Отдельно отмечу решение по кэшу — оно лучше того, что я предполагал в issue. Резолв не кэшируется персистентно, а сегментом ключа кэша фасетов служит хеш от текущего набора разрешённых групп. При смене группы покупателя или правке ACL ключ меняется сам на следующем запросе, ручная инвалидация не нужна. Вопрос из #669 про «где хранить и когда сбрасывать» закрыт элегантно.

N+1 нет — фильтр остаётся условием запроса, а resolveAllowedResourceGroupIdsForCustomer() мемоизирован по паре «покупатель + контекст».

Числа: мерж с актуальной beta чистый, smoke 101/101, PHPUnit 322 / 1397 assertions, WebApi 24/24, PHPStan по 404 файлам — 0 ошибок.

Что мешает влить прямо сейчас

База уходит на доработку. В #666 нашёлся баг: ресурс, состоящий в нескольких группах ресурсов, скрывается, даже если одна из групп открыта анониму — членство в MODX работает как ИЛИ, а SQL проверяет попарно внутри одной строки членства. Ваш buildProtectedMembershipExistsSql() выносит этот SQL дословно, то есть баг наследуется и распространяется ещё и на авторизованных покупателей через OR по разрешённым группам. Подробности в комментарии к #666.

Конфликт с #681. Вы оба стоите на ветке #666 и по-разному переписываете CartItemManager::validateProduct(): вы переводите на applyForRequest() с учётом группы покупателя, #681 — на isVisible(), который документирован как anonymous-only и списка разрешённых групп не принимает. Взять одну версию целиком нельзя ни в каком порядке: либо авторизованный покупатель увидит товар в каталоге, но не сможет положить его в корзину, либо потеряется то, ради чего #681 трогал этот метод.

То же и с самим CatalogResourceGroupVisibility: вы добавляете четвёртый параметр к apply() плюс applyForRequest() и buildVisibilitySql(), #681 добавляет buildWhereFragment() и isVisible() — написано независимо, друг про друга не знает.

Нужно ваше решение, как свести apply(), applyForRequest(), buildWhereFragment() и isVisible() в одну согласованную точку входа и в каком порядке мержить пару. Механически это не разруливается.

Мелочи, на усмотрение

  • resolveAllowedIdsForRequest() каждый вызов заново резолвит токен и делает getObject(msCustomerToken::class, ...), мемоизация есть только глубже. При фасетном запросе с двумя десятками ключей это до двух десятков лишних точечных SELECT по индексированному полю. Не N+1 по каталогу, но кэшировать customerId по контексту на уровне резолвера дёшево.
  • На ms3_customer_groups.user_group_id нет уникального индекса — несколько групп покупателей могут ссылаться на одну группу пользователей MODX. Возможно, так и задумано (разные сегменты на одну группу прав), но стоит записать это явно, чтобы следующий читатель не счёл упущением.

Ibochkarev added a commit that referenced this pull request Sep 10, 2026
Fetch::run() never reads cacheKey, so the _rg suffix was misleading.
validateProduct stays on apply() so #677 can upgrade to applyForRequest;
isVisible remains for Fenom &product= lookups. Rebased onto #666 multi-group SQL.
@Ibochkarev
Ibochkarev force-pushed the feat/issue-669-customer-resource-groups branch from 15355bc to f99931d Compare September 10, 2026 01:40
@Ibochkarev

Copy link
Copy Markdown
Member Author

Сведено с #681 и пофикшенным #666.

Порядок мержа: #666#681#677 (база PR переключена на fix/issue-670-fenom-resource-groups).

API в одной точке входа

Multi-group (#666): buildProtectedMembershipExistsSql() наследует document-level OR: restricted на любой группе + нет principal=0 на любой группе документа (dg_anon.document = alias.id). Smoke + CatalogResourceGroupVisibilitySqlTest зелёные.

Мелочи из ревью

  • resolveAllowedIdsForRequest() мемоизирует token → customer_id на инстансе резолвера
  • в модели/миграции явно: user_group_id не уникален намеренно (несколько сегментов → один modUserGroup)

@Ibochkarev

Copy link
Copy Markdown
Member Author

Уточнение по базе: GitHub не даёт сменить base у stacked PR (Cannot change the base branch because the pull request is part of a stack). База остаётся feat/issue-659-catalog-resource-groups (#666), tip ветки — поверх tip #681 (901fc0c7). После мержа #666#681 в #677 останется только коммит с customer groups. Mergeable сейчас зелёный.

Reuse CatalogResourceGroupVisibility for ms3_products (INNER JOIN ON
to avoid pdoTools where false-positives), gallery/options product
lookups, and cart validation. Extend pdoTools cache keys with RG+context.
PHPStan failed on #681: bare modX vs MODX\Revolution\modX for
CatalogResourceGroupVisibility, and (int) cast after @var msProduct
in ms3_gallery.
Fetch::run() never reads cacheKey, so the _rg suffix was misleading.
validateProduct stays on apply() so #677 can upgrade to applyForRequest;
isVisible remains for Fenom &product= lookups. Rebased onto #666 multi-group SQL.
Authenticated catalog visitors resolve allowed resource groups via a
customer group → modUserGroup link, reusing the #666 SQL visibility gate.
@Ibochkarev
Ibochkarev force-pushed the feat/issue-669-customer-resource-groups branch from f99931d to 0f20d00 Compare September 10, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: high Важно исправить в ближайшее время

Projects

None yet

Development

Successfully merging this pull request may close these issues.

msCustomerGroup: связать покупателей с группами ресурсов MODX

2 participants