From 72edceb8c5a1e10dff74a19865ce760331083eb3 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 08:21:07 +0600 Subject: [PATCH 1/2] feat(mgr-api): category-scoped PUT for grid inline-edit product data Add PUT /categories/{id}/products/{productId}/data with URL-bound scope check; switch CategoryProductsGrid to the new route (#455). --- .../minishop3/config/routes/manager.php | 11 ++++ .../Manager/CategoryProductsController.php | 64 +++++++++++++++++++ .../Category/CategoryProductScopePolicy.php | 21 +++++- .../Category/CategoryProductsListService.php | 13 ++-- .../tests/CategoryProductScopePolicyTest.php | 13 +++- .../useCategoryProductsInlineEdit.js | 22 ++++--- 6 files changed, 125 insertions(+), 19 deletions(-) diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 1b0f0ffe..27b5c28e 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -436,6 +436,17 @@ CategoryProductActionPermissions::mutationPermissions() ) ]); + // Category-scoped inline-edit product data (#455) + $router->put('/{id}/products/{productId}/data', function($params) use ($modx) { + $input = file_get_contents('php://input'); + $data = json_decode($input, true) ?: []; + $allParams = array_merge($data, $_GET, $params); + + $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); + return $controller->updateProductData($allParams); + }, [ + new PermissionMiddleware($modx, 'msproduct_save') + ]); // Toggle product publish status $router->post('/{id}/products/{productId}/publish', function($params) use ($modx) { $input = file_get_contents('php://input'); diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php index 9eb91679..59527992 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php @@ -265,6 +265,70 @@ public function bulkDelete(array $params = []): array return $this->multiple($params); } + /** + * Update product data from category grid inline-edit + * PUT /api/mgr/categories/{id}/products/{productId}/data + * + * @param array $params + * @return array Response + */ + public function updateProductData(array $params = []): array + { + $categoryId = (int) ($params['id'] ?? 0); + $productId = (int) ($params['productId'] ?? 0); + $nested = filter_var($params['nested'] ?? false, FILTER_VALIDATE_BOOLEAN); + + if (!$categoryId) { + return Response::error('Category ID is required', HttpStatus::BAD_REQUEST)->getData(); + } + + if (!$productId) { + return Response::error('Product ID is required', HttpStatus::BAD_REQUEST)->getData(); + } + + $data = $params; + unset($data['id'], $data['productId'], $data['nested']); + + if ($data === []) { + return Response::error('Invalid request data', HttpStatus::BAD_REQUEST)->getData(); + } + + /** @var CategoryProductsListService|null $listService */ + $listService = $this->modx->services->get('ms3_category_products_list'); + if (!$listService) { + return Response::error( + 'Category products list service is not available', + HttpStatus::INTERNAL_SERVER_ERROR + )->getData(); + } + + if (!$listService->isProductInCategoryScope($productId, $categoryId, $nested)) { + $this->modx->lexicon->load('minishop3:default'); + + return Response::error( + $this->modx->lexicon('ms3_err_product_not_in_category_scope'), + HttpStatus::FORBIDDEN + )->getData(); + } + + /** @var \MiniShop3\Services\Product\ProductDataService|null $productDataService */ + $productDataService = $this->modx->services->get('ms3_product_data_service'); + if (!$productDataService) { + return Response::error('Product data service is not available', HttpStatus::INTERNAL_SERVER_ERROR)->getData(); + } + + $result = $productDataService->updateProductData($productId, $data); + + if (!empty($result['ok']) && !empty($result['data'])) { + return Response::success($result['data'])->getData(); + } + + $code = $result['code'] ?? HttpStatus::INTERNAL_SERVER_ERROR; + $message = $result['message'] ?? 'Failed to save product data'; + + return Response::error($message, $code)->getData(); + } + /** * Toggle product publish status * POST /api/mgr/categories/{id}/products/{productId}/publish diff --git a/core/components/minishop3/src/Services/Category/CategoryProductScopePolicy.php b/core/components/minishop3/src/Services/Category/CategoryProductScopePolicy.php index 5dcd1152..041322e4 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductScopePolicy.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductScopePolicy.php @@ -22,13 +22,30 @@ public static function isParentInScope( return false; } + return in_array( + $productParentId, + self::allowedParentCategoryIds($categoryId, $nested, $descendantCategoryIds), + true + ); + } + + /** + * @param list $descendantCategoryIds Child category IDs (recursive, excluding root) + * + * @return list + */ + public static function allowedParentCategoryIds( + int $categoryId, + bool $nested, + array $descendantCategoryIds + ): array { if (!$nested) { - return $productParentId === $categoryId; + return [$categoryId]; } $allowed = $descendantCategoryIds; $allowed[] = $categoryId; - return in_array($productParentId, $allowed, true); + return $allowed; } } diff --git a/core/components/minishop3/src/Services/Category/CategoryProductsListService.php b/core/components/minishop3/src/Services/Category/CategoryProductsListService.php index 41d671f6..2f1b0dc0 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductsListService.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductsListService.php @@ -226,14 +226,11 @@ private function quoteOptionKeyForJoinCondition(string $key): string */ public function getAllowedProductParentCategoryIds(int $categoryId, bool $nested): array { - if (!$nested) { - return [$categoryId]; - } - - $ids = $this->treeService()->getDescendantCategoryIds($categoryId); - $ids[] = $categoryId; - - return $ids; + return CategoryProductScopePolicy::allowedParentCategoryIds( + $categoryId, + $nested, + $nested ? $this->treeService()->getDescendantCategoryIds($categoryId) : [] + ); } /** diff --git a/core/components/minishop3/tests/CategoryProductScopePolicyTest.php b/core/components/minishop3/tests/CategoryProductScopePolicyTest.php index d39c7e47..4d31d008 100644 --- a/core/components/minishop3/tests/CategoryProductScopePolicyTest.php +++ b/core/components/minishop3/tests/CategoryProductScopePolicyTest.php @@ -1,7 +1,7 @@ ({ - category_id: typeof categoryId === 'object' && categoryId !== null ? categoryId.value : categoryId, - nested: Boolean(typeof nested === 'object' && nested !== null ? nested.value : nested), - }) + const resolveCategoryId = () => + typeof categoryId === 'object' && categoryId !== null ? categoryId.value : categoryId + + const resolveNested = () => + Boolean(typeof nested === 'object' && nested !== null ? nested.value : nested) const editingCell = ref(null) const inlineEditValue = ref('') @@ -211,10 +212,15 @@ export function useCategoryProductsInlineEdit(deps) { } inlineEditSaving.value = true try { - const res = await request.put(`/api/mgr/product-data/${product.id}`, { - [column.name]: value, - ...resolveScopeContext(), - }) + const scopedCategoryId = resolveCategoryId() + const payload = { [column.name]: value } + if (resolveNested()) { + payload.nested = 1 + } + const res = await request.put( + `/api/mgr/categories/${scopedCategoryId}/products/${product.id}/data`, + payload + ) const idx = products.value.findIndex(p => p.id === product.id) if (idx >= 0) { if (res && typeof res === 'object') { From fed58c011bbdc3632b958c7f60dfa6392d1101fd Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 23:07:20 +0600 Subject: [PATCH 2/2] fix(mgr-api): document ACL + scope policy for inline-edit product data Apply CategoryProductDocumentPolicy save check on the updateProductData inline-edit path (#473 pattern) and resolve scope via CategoryProductScopePolicy through findInCategory() instead of the separate isProductInCategoryScope() bool-only getObject round-trip. - Import CategoryProductDocumentPolicy (cherry-picked from #473) - Refactor CategoryProductScopeService::findInCategory nested branch to delegate to CategoryProductScopePolicy::isParentInScope - Replace isProductInCategoryScope() call in updateProductData with scopeService()->findInCategory() (single lookup, yields product for ACL) - Add 403 save-policy guard + logDocumentPolicyDenied helper - Tests: CategoryProductDocumentPolicyTest + ACL denial / out-of-scope cases in CategoryProductsControllerScopeTest; stubs support per-product policies and lexicon --- .../Manager/CategoryProductsController.php | 36 +++- .../CategoryProductDocumentPolicy.php | 173 ++++++++++++++++++ .../Category/CategoryProductScopeService.php | 18 +- .../CategoryProductDocumentPolicyTest.php | 94 ++++++++++ .../CategoryProductsControllerScopeTest.php | 25 +++ .../stubs/CategoryProductScopeModxStub.php | 38 +++- .../minishop3/tests/stubs/StubMsProduct.php | 20 +- 7 files changed, 384 insertions(+), 20 deletions(-) create mode 100644 core/components/minishop3/src/Services/Category/CategoryProductDocumentPolicy.php create mode 100644 core/components/minishop3/tests/CategoryProductDocumentPolicyTest.php diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php index 59527992..62bd9dbe 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php @@ -7,6 +7,7 @@ use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; use MiniShop3\Services\Category\CategoryProductActionPermissions; +use MiniShop3\Services\Category\CategoryProductDocumentPolicy; use MiniShop3\Services\Category\CategoryProductScopeService; use MiniShop3\Services\Category\CategoryProductsListService; use MiniShop3\Services\FilterConfigManager; @@ -293,20 +294,26 @@ public function updateProductData(array $params = []): array return Response::error('Invalid request data', HttpStatus::BAD_REQUEST)->getData(); } - /** @var CategoryProductsListService|null $listService */ - $listService = $this->modx->services->get('ms3_category_products_list'); - if (!$listService) { + // Scope check + product fetch in one round-trip (CategoryProductScopePolicy). + // Replaces the separate isProductInCategoryScope() bool-only lookup and yields + // the product instance for the document ACL check below (#473 pattern). + $product = $this->scopeService()->findInCategory($categoryId, $productId, $nested); + + if (!$product) { + $this->modx->lexicon->load('minishop3:default'); + return Response::error( - 'Category products list service is not available', - HttpStatus::INTERNAL_SERVER_ERROR + $this->modx->lexicon('ms3_err_product_not_in_category_scope'), + HttpStatus::FORBIDDEN )->getData(); } - if (!$listService->isProductInCategoryScope($productId, $categoryId, $nested)) { - $this->modx->lexicon->load('minishop3:default'); + $savePolicies = [CategoryProductDocumentPolicy::POLICY_SAVE]; + if (!CategoryProductDocumentPolicy::isAllowedAll($product, $savePolicies)) { + $this->logDocumentPolicyDenied($product, $savePolicies); return Response::error( - $this->modx->lexicon('ms3_err_product_not_in_category_scope'), + 'Save permission denied for this document', HttpStatus::FORBIDDEN )->getData(); } @@ -395,6 +402,19 @@ private function scopeService(): CategoryProductScopeService : new CategoryProductScopeService($this->modx); } + /** @param list $policies */ + private function logDocumentPolicyDenied(object $product, array $policies): void + { + $this->modx->log( + modX::LOG_LEVEL_WARN, + '[CategoryProductsController] Document policy denied (' + . implode(',', $policies) + . ') for product ' + . (int) $product->get('id') + . ' (user id ' . (int) ($this->modx->user->get('id') ?? 0) . ')' + ); + } + private function denyWithoutPermission(string $permission): ?array { if ($this->modx->hasPermission($permission)) { diff --git a/core/components/minishop3/src/Services/Category/CategoryProductDocumentPolicy.php b/core/components/minishop3/src/Services/Category/CategoryProductDocumentPolicy.php new file mode 100644 index 00000000..8698208d --- /dev/null +++ b/core/components/minishop3/src/Services/Category/CategoryProductDocumentPolicy.php @@ -0,0 +1,173 @@ + */ + public static function sortPolicies(): array + { + return [self::POLICY_SAVE]; + } + + /** @return list */ + public static function policiesForPublish(bool $published): array + { + return $published + ? [self::POLICY_PUBLISH] + : [self::POLICY_SAVE, self::POLICY_UNPUBLISH]; + } + + public static function isAllowed(object $resource, string $policy): bool + { + return self::evaluate($resource, $policy) === null; + } + + /** @param list $policies */ + public static function isAllowedAll(object $resource, array $policies): bool + { + return self::evaluateAll($resource, $policies) === null; + } + + /** + * @return array{status: int, message: string}|null null when allowed + */ + public static function evaluate(object $resource, string $policy): ?array + { + if (method_exists($resource, 'checkPolicy') && $resource->checkPolicy($policy)) { + return null; + } + + return [ + 'status' => HttpStatus::FORBIDDEN, + 'message' => self::messageForPolicy($policy), + ]; + } + + /** + * @param list $policies + * @return array{status: int, message: string}|null null when allowed + */ + public static function evaluateAll(object $resource, array $policies): ?array + { + foreach ($policies as $policy) { + $denied = self::evaluate($resource, $policy); + if ($denied !== null) { + return $denied; + } + } + + return null; + } + + public static function denialResponse(object $resource, string $policy): ?array + { + return self::toErrorResponse(self::evaluate($resource, $policy)); + } + + /** @param list $policies */ + public static function denialResponseAll(object $resource, array $policies): ?array + { + return self::toErrorResponse(self::evaluateAll($resource, $policies)); + } + + public static function canViewInCategoryGrid(modX $modx, msProduct $product, bool $nested): bool + { + if (!self::isAllowed($product, self::POLICY_VIEW)) { + return false; + } + + if (!$nested) { + return true; + } + + $parentId = (int) $product->get('parent'); + if ($parentId <= 0) { + return false; + } + + $parent = $modx->getObject(msCategory::class, $parentId); + if (!$parent instanceof msCategory) { + return false; + } + + return self::isAllowed($parent, self::POLICY_VIEW); + } + + /** + * @param array $parentsById + */ + public static function canViewInCategoryGridCached( + msProduct $product, + bool $nested, + array $parentsById, + ): bool { + if (!self::isAllowed($product, self::POLICY_VIEW)) { + return false; + } + + if (!$nested) { + return true; + } + + $parentId = (int) $product->get('parent'); + if ($parentId <= 0) { + return false; + } + + $parent = $parentsById[$parentId] ?? null; + if (!$parent instanceof msCategory) { + return false; + } + + return self::isAllowed($parent, self::POLICY_VIEW); + } + + public static function toErrorResponse(?array $evaluation): ?array + { + if ($evaluation === null) { + return null; + } + + return Response::error( + $evaluation['message'], + $evaluation['status'] + )->getData(); + } + + private static function messageForPolicy(string $policy): string + { + return match ($policy) { + self::POLICY_VIEW => 'View permission denied for this document', + self::POLICY_SAVE => 'Save permission denied for this document', + self::POLICY_PUBLISH => 'Publish permission denied for this document', + self::POLICY_DELETE => 'Delete permission denied for this document', + self::POLICY_UNPUBLISH => 'Unpublish permission denied for this document', + self::POLICY_UNDELETE => 'Undelete permission denied for this document', + default => 'Access denied for this document', + }; + } +} diff --git a/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php b/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php index c670aef5..95425d5c 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductScopeService.php @@ -25,12 +25,6 @@ public function findInCategory(int $categoryId, int $productId, bool $nested = f return null; } - $allowedParents = $this->treeService()->productParentIds($categoryId, $nested); - - if ($allowedParents === []) { - return null; - } - if (!$nested) { /** @var msProduct|null $product */ $product = $this->modx->getObject(msProduct::class, [ @@ -48,7 +42,17 @@ public function findInCategory(int $categoryId, int $productId, bool $nested = f return null; } - return in_array((int) $product->get('parent'), $allowedParents, true) ? $product : null; + $parentId = (int) $product->get('parent'); + if ($parentId <= 0) { + return null; + } + + return CategoryProductScopePolicy::isParentInScope( + $parentId, + $categoryId, + true, + $this->treeService()->getDescendantCategoryIds($categoryId) + ) ? $product : null; } private function treeService(): CategoryTreeService diff --git a/core/components/minishop3/tests/CategoryProductDocumentPolicyTest.php b/core/components/minishop3/tests/CategoryProductDocumentPolicyTest.php new file mode 100644 index 00000000..eb538d44 --- /dev/null +++ b/core/components/minishop3/tests/CategoryProductDocumentPolicyTest.php @@ -0,0 +1,94 @@ + 999, 'parent' => 2, 'published' => 0], ['id' => 100, 'parent' => 1, 'published' => 0], ['id' => 101, 'parent' => 2, 'published' => 0], + ['id' => 200, 'parent' => 1, 'published' => 0, 'policies' => ['save' => false]], ]; $modx->categories = [ ['id' => 2, 'parent' => 1], @@ -88,4 +89,28 @@ $assertSame(true, $sort['success'] ?? null, 'sort success'); $assertSame(1, $sort['data']['updated'] ?? null, 'sort updated count'); +// updateProductData: in-scope product with document save policy denied → 403 (#473 pattern) +$modx->getObjectCalls = []; +$aclDenied = $controller->updateProductData([ + 'id' => 1, + 'productId' => 200, + 'pagetitle' => 'updated title', +]); +$assertSame(false, $aclDenied['success'] ?? null, 'updateProductData ACL denied success'); +$assertSame(HttpStatus::FORBIDDEN, $aclDenied['code'] ?? null, 'updateProductData ACL denied code'); +$assertSame( + 'Save permission denied for this document', + $aclDenied['message'] ?? null, + 'updateProductData ACL denied message' +); + +// updateProductData: out-of-scope product → 403 (scope guard still enforced) +$outOfScope = $controller->updateProductData([ + 'id' => 1, + 'productId' => 999, + 'pagetitle' => 'updated title', +]); +$assertSame(false, $outOfScope['success'] ?? null, 'updateProductData out-of-scope success'); +$assertSame(HttpStatus::FORBIDDEN, $outOfScope['code'] ?? null, 'updateProductData out-of-scope code'); + fwrite(STDOUT, "OK: CategoryProductsControllerScopeTest\n"); diff --git a/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php b/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php index 98d1d7d6..e7205194 100644 --- a/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php +++ b/core/components/minishop3/tests/stubs/CategoryProductScopeModxStub.php @@ -16,7 +16,10 @@ class CategoryProductScopeModxStub extends modX /** @var object */ public object $services; - /** @var list */ + /** @var object */ + public object $lexicon; + + /** @var list}> */ public array $products = []; /** @var list */ @@ -28,6 +31,27 @@ class CategoryProductScopeModxStub extends modX public function __construct() { parent::__construct(); + $this->user = new class { + public function isAuthenticated(string $context): bool + { + return $context === 'mgr'; + } + + public function getUserToken(string $contextKey): string + { + return 'test-modauth-token'; + } + + public function get(string $key): mixed + { + return $key === 'id' ? 0 : null; + } + }; + $this->lexicon = new class { + public function load(string ...$topics): void + { + } + }; $this->services = new class { public function get(string $key): null { @@ -41,6 +65,14 @@ public function has(string $key): bool }; } + /** + * @param array $params + */ + public function lexicon(string $key, array $params = [], string $language = ''): string + { + return $key; + } + public function getObject($className = '', $criteria = null, $cacheFlag = true) { $this->getObjectCalls[] = ['class' => $className, 'criteria' => $criteria]; @@ -55,7 +87,7 @@ public function getObject($className = '', $criteria = null, $cacheFlag = true) foreach ($this->products as $row) { if ((int) $row['id'] === $productId && (int) $row['parent'] === $parentId) { - return new StubMsProduct($row); + return new StubMsProduct($row, $row['policies'] ?? null); } } @@ -65,7 +97,7 @@ public function getObject($className = '', $criteria = null, $cacheFlag = true) $productId = (int) $criteria; foreach ($this->products as $row) { if ((int) $row['id'] === $productId) { - return new StubMsProduct($row); + return new StubMsProduct($row, $row['policies'] ?? null); } } diff --git a/core/components/minishop3/tests/stubs/StubMsProduct.php b/core/components/minishop3/tests/stubs/StubMsProduct.php index 6187eff4..17cb9e12 100644 --- a/core/components/minishop3/tests/stubs/StubMsProduct.php +++ b/core/components/minishop3/tests/stubs/StubMsProduct.php @@ -12,10 +12,17 @@ class StubMsProduct /** @var array */ private array $data; - /** @param array $data */ - public function __construct(array $data) + /** @var array|null null = allow all policies */ + private ?array $policies; + + /** + * @param array $data + * @param array|null $policies null allows every checkPolicy(); map denies/allows per policy + */ + public function __construct(array $data, ?array $policies = null) { $this->data = $data; + $this->policies = $policies; } public function get(string $key): mixed @@ -32,4 +39,13 @@ public function save(): bool { return true; } + + public function checkPolicy(string $policy): bool + { + if ($this->policies === null) { + return true; + } + + return !empty($this->policies[$policy]); + } }