From 57194bf36acebc64332a91b155578e344343b1ee Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Thu, 16 Jul 2026 09:53:22 +0600 Subject: [PATCH 1/2] fix(manager-api): tighten ACL on customers and category products Replace group-level view_document with msorder_* / msproduct_* permissions aligned to legacy processors so content editors cannot mutate via mgr API. --- .../minishop3/config/routes/manager.php | 80 +++++++---- .../Manager/CategoryProductsController.php | 136 +++++++++++------- .../minishop3/src/Router/HttpStatus.php | 1 + .../Middleware/AnyPermissionMiddleware.php | 48 +++++++ .../Middleware/PermissionMiddleware.php | 3 +- .../CategoryProductActionPermissions.php | 27 ++++ .../CategoryProductActionPermissionsTest.php | 51 +++++++ 7 files changed, 266 insertions(+), 80 deletions(-) create mode 100644 core/components/minishop3/src/Router/Middleware/AnyPermissionMiddleware.php create mode 100644 core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php create mode 100644 core/components/minishop3/tests/CategoryProductActionPermissionsTest.php diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 71aa8978..4af28dc9 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -22,7 +22,9 @@ */ use MiniShop3\Router\Middleware\AuthMiddleware; +use MiniShop3\Router\Middleware\AnyPermissionMiddleware; use MiniShop3\Router\Middleware\PermissionMiddleware; +use MiniShop3\Services\Category\CategoryProductActionPermissions; use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; @@ -291,13 +293,16 @@ new PermissionMiddleware($modx, 'mssetting_save') ]); + // Customers: permissions align with legacy Customer* processors (#378) $router->group('/customers', function($router) use ($modx) { $router->get('', function($params) use ($modx) { $allParams = array_merge($_GET, $params); $controller = new \MiniShop3\Controllers\Api\Manager\CustomersController($modx); return $controller->getList($allParams); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_list') + ]); // Bulk delete - must be before /{id} route $router->delete('/bulk', function($params) use ($modx) { $input = file_get_contents('php://input'); @@ -305,11 +310,15 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CustomersController($modx); return $controller->bulkDelete($data); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_remove') + ]); $router->get('/{id}', function($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\CustomersController($modx); return $controller->get($params); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_view') + ]); $router->put('/{id}', function($params) use ($modx) { $input = file_get_contents('php://input'); @@ -318,15 +327,21 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CustomersController($modx); return $controller->update($data); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_save') + ]); $router->delete('/{id}', function($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\CustomersController($modx); return $controller->delete($params); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_remove') + ]); $router->get('/{id}/addresses', function($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\CustomerAddressesController($modx); return $controller->getList($params); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_list') + ]); $router->post('/{id}/addresses', function($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; @@ -334,7 +349,9 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CustomerAddressesController($modx); return $controller->create($data); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_save') + ]); $router->put('/{id}/addresses/{address_id}', function($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; @@ -343,19 +360,20 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CustomerAddressesController($modx); return $controller->update($data); - }); + }, [ + new PermissionMiddleware($modx, 'msorder_save') + ]); $router->delete('/{id}/addresses/{address_id}', function($params) use ($modx) { $params['address_id'] = $params['address_id'] ?? null; $controller = new \MiniShop3\Controllers\Api\Manager\CustomerAddressesController($modx); return $controller->delete($params); - }); - - }, [ - new PermissionMiddleware($modx, 'view_document') - ]); + }, [ + new PermissionMiddleware($modx, 'msorder_remove') + ]); + }); - // Category products routes + // Category products: read vs mutate permissions (#378) $router->group('/categories', function($router) use ($modx) { // Get products in category $router->get('/{id}/products', function($params) use ($modx) { @@ -363,12 +381,16 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->getList($allParams); - }); + }, [ + new PermissionMiddleware($modx, 'view_document') + ]); // Get filters configuration $router->get('/{id}/products/filters', function($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->getFilters($params); - }); + }, [ + new PermissionMiddleware($modx, 'view_document') + ]); // Sort products (drag-drop) $router->post('/{id}/products/sort', function($params) use ($modx) { $input = file_get_contents('php://input'); @@ -377,7 +399,9 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->sort($allParams); - }); + }, [ + new PermissionMiddleware($modx, 'msproduct_save') + ]); // Bulk delete products $router->delete('/{id}/products/bulk', function($params) use ($modx) { $input = file_get_contents('php://input'); @@ -386,8 +410,10 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->bulkDelete($allParams); - }); - // Multiple product actions + }, [ + new PermissionMiddleware($modx, 'msproduct_delete') + ]); + // Multiple product actions — gate any product mutation perm; exact check per method in controller $router->post('/{id}/products/multiple', function($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; @@ -395,7 +421,12 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->multiple($allParams); - }); + }, [ + new AnyPermissionMiddleware( + $modx, + CategoryProductActionPermissions::mutationPermissions() + ) + ]); // Toggle product publish status $router->post('/{id}/products/{productId}/publish', function($params) use ($modx) { $input = file_get_contents('php://input'); @@ -404,11 +435,10 @@ $controller = new \MiniShop3\Controllers\Api\Manager\CategoryProductsController($modx); return $controller->publish($allParams); - }); - - }, [ - new PermissionMiddleware($modx, 'view_document') - ]); + }, [ + new PermissionMiddleware($modx, 'msproduct_publish') + ]); + }); $router->group('/deliveries', function($router) use ($modx) { $router->get('', function($params) use ($modx) { diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php index 799b7024..400a9b41 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php @@ -6,6 +6,7 @@ use MiniShop3\Model\msProduct; use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; +use MiniShop3\Services\Category\CategoryProductActionPermissions; use MiniShop3\Services\Category\CategoryProductsListService; use MiniShop3\Services\FilterConfigManager; use MODX\Revolution\modX; @@ -167,6 +168,20 @@ public function multiple(array $params = []): array return Response::error('Method is required', HttpStatus::BAD_REQUEST)->getData(); } + $permission = CategoryProductActionPermissions::forMethod($method); + if ($permission === null) { + $this->modx->log( + modX::LOG_LEVEL_WARN, + "[CategoryProductsController] Unknown method: {$method}" + ); + + return Response::error('Unknown method', HttpStatus::BAD_REQUEST)->getData(); + } + + if ($denied = $this->denyWithoutPermission($permission)) { + return $denied; + } + if (empty($ids) || !is_array($ids)) { return Response::error('Product IDs array is required', HttpStatus::BAD_REQUEST)->getData(); } @@ -191,50 +206,14 @@ public function multiple(array $params = []): array continue; } - $result = false; - - switch ($method) { - case 'publish': - $product->set('published', 1); - $product->set('publishedon', time()); - $product->set('publishedby', $this->modx->user->get('id')); - $result = $product->save(); - break; - - case 'unpublish': - $product->set('published', 0); - $product->set('publishedon', 0); - $product->set('publishedby', 0); - $result = $product->save(); - break; - - case 'delete': - $product->set('deleted', 1); - $product->set('deletedon', time()); - $product->set('deletedby', $this->modx->user->get('id')); - $result = $product->save(); - break; - - case 'undelete': - $product->set('deleted', 0); - $product->set('deletedon', 0); - $product->set('deletedby', 0); - $result = $product->save(); - break; - - case 'show': - $product->set('hidemenu', 0); - $result = $product->save(); - break; - - case 'hide': - $product->set('hidemenu', 1); - $result = $product->save(); - break; - - default: - $this->modx->log(modX::LOG_LEVEL_WARN, "[CategoryProductsController] Unknown method: {$method}"); - } + $result = match ($method) { + 'publish' => $this->applyPublish($product, true), + 'unpublish' => $this->applyPublish($product, false), + 'delete' => $this->applyDelete($product, true), + 'undelete' => $this->applyDelete($product, false), + 'show' => $this->applyHideMenu($product, false), + 'hide' => $this->applyHideMenu($product, true), + }; if ($result) { $success++; @@ -283,6 +262,10 @@ public function publish(array $params = []): array return Response::error('Product ID is required', HttpStatus::BAD_REQUEST)->getData(); } + if ($denied = $this->denyWithoutPermission('msproduct_publish')) { + return $denied; + } + $product = $this->modx->getObject(msProduct::class, $productId); if (!$product) { @@ -294,16 +277,7 @@ public function publish(array $params = []): array $published = $product->get('published') ? 0 : 1; } - $product->set('published', $published); - if ($published) { - $product->set('publishedon', time()); - $product->set('publishedby', $this->modx->user->get('id')); - } else { - $product->set('publishedon', 0); - $product->set('publishedby', 0); - } - - if (!$product->save()) { + if (!$this->applyPublish($product, (bool) $published)) { return Response::error('Failed to update product', HttpStatus::INTERNAL_SERVER_ERROR)->getData(); } @@ -313,6 +287,60 @@ public function publish(array $params = []): array ], $published ? 'Product published' : 'Product unpublished')->getData(); } + private function denyWithoutPermission(string $permission): ?array + { + if ($this->modx->hasPermission($permission)) { + return null; + } + + $this->modx->log( + modX::LOG_LEVEL_WARN, + '[CategoryProductsController] Access denied for permission ' . $permission + . ' (user id ' . (int)($this->modx->user?->get('id') ?? 0) . ')' + ); + + return Response::error( + "Access denied. Required permission: {$permission}", + HttpStatus::FORBIDDEN + )->getData(); + } + + private function applyPublish(msProduct $product, bool $published): bool + { + return $this->setAuditedFlag($product, 'published', 'publishedon', 'publishedby', $published); + } + + private function applyDelete(msProduct $product, bool $deleted): bool + { + return $this->setAuditedFlag($product, 'deleted', 'deletedon', 'deletedby', $deleted); + } + + private function setAuditedFlag( + msProduct $product, + string $flag, + string $onField, + string $byField, + bool $enabled + ): bool { + $product->set($flag, $enabled ? 1 : 0); + if ($enabled) { + $product->set($onField, time()); + $product->set($byField, $this->modx->user->get('id')); + } else { + $product->set($onField, 0); + $product->set($byField, 0); + } + + return $product->save(); + } + + private function applyHideMenu(msProduct $product, bool $hidemenu): bool + { + $product->set('hidemenu', $hidemenu ? 1 : 0); + + return $product->save(); + } + /** * Get default filters configuration * diff --git a/core/components/minishop3/src/Router/HttpStatus.php b/core/components/minishop3/src/Router/HttpStatus.php index 26e24ea1..2bd2a87a 100644 --- a/core/components/minishop3/src/Router/HttpStatus.php +++ b/core/components/minishop3/src/Router/HttpStatus.php @@ -15,6 +15,7 @@ final class HttpStatus public const CREATED = 201; public const BAD_REQUEST = 400; public const UNAUTHORIZED = 401; + public const FORBIDDEN = 403; public const NOT_FOUND = 404; public const METHOD_NOT_ALLOWED = 405; public const CONFLICT = 409; diff --git a/core/components/minishop3/src/Router/Middleware/AnyPermissionMiddleware.php b/core/components/minishop3/src/Router/Middleware/AnyPermissionMiddleware.php new file mode 100644 index 00000000..88a874a5 --- /dev/null +++ b/core/components/minishop3/src/Router/Middleware/AnyPermissionMiddleware.php @@ -0,0 +1,48 @@ + */ + protected array $permissions; + + /** + * @param list $permissions + */ + public function __construct(modX $modx, array $permissions) + { + $this->modx = $modx; + $this->permissions = array_values(array_filter($permissions, static fn($p) => is_string($p) && $p !== '')); + } + + /** + * {@inheritdoc} + */ + public function handle(array $params) + { + foreach ($this->permissions as $permission) { + if ($this->modx->hasPermission($permission)) { + return null; + } + } + + $required = $this->permissions !== [] + ? implode(' or ', $this->permissions) + : '(none configured)'; + + return Response::error( + "Access denied. Required permission: {$required}", + HttpStatus::FORBIDDEN + ); + } +} diff --git a/core/components/minishop3/src/Router/Middleware/PermissionMiddleware.php b/core/components/minishop3/src/Router/Middleware/PermissionMiddleware.php index 32fea5ac..d8e82d50 100644 --- a/core/components/minishop3/src/Router/Middleware/PermissionMiddleware.php +++ b/core/components/minishop3/src/Router/Middleware/PermissionMiddleware.php @@ -2,6 +2,7 @@ namespace MiniShop3\Router\Middleware; +use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; use MODX\Revolution\modX; @@ -31,7 +32,7 @@ public function handle(array $params) if (!$this->modx->hasPermission($this->permission)) { return Response::error( "Access denied. Required permission: {$this->permission}", - 403 + HttpStatus::FORBIDDEN ); } diff --git a/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php b/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php new file mode 100644 index 00000000..1fa17699 --- /dev/null +++ b/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php @@ -0,0 +1,27 @@ + + */ + public static function mutationPermissions(): array + { + return ['msproduct_publish', 'msproduct_delete', 'msproduct_save']; + } + + public static function forMethod(string $method): ?string + { + return match ($method) { + 'publish', 'unpublish' => 'msproduct_publish', + 'delete', 'undelete' => 'msproduct_delete', + 'show', 'hide' => 'msproduct_save', + default => null, + }; + } +} diff --git a/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php b/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php new file mode 100644 index 00000000..c36db1c1 --- /dev/null +++ b/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php @@ -0,0 +1,51 @@ + Date: Thu, 16 Jul 2026 09:55:26 +0600 Subject: [PATCH 2/2] test(manager-api): cover 403/400 authz evaluate for category products Extract CategoryProductActionPermissions::evaluate() so controller deny paths are unit-tested without bootstrapping MODX. --- .../Manager/CategoryProductsController.php | 30 ++++++---- .../CategoryProductActionPermissions.php | 46 +++++++++++++++ .../CategoryProductActionPermissionsTest.php | 56 ++++++++++++++++++- 3 files changed, 120 insertions(+), 12 deletions(-) diff --git a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php index 400a9b41..6a2dab76 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php @@ -168,18 +168,26 @@ public function multiple(array $params = []): array return Response::error('Method is required', HttpStatus::BAD_REQUEST)->getData(); } - $permission = CategoryProductActionPermissions::forMethod($method); - if ($permission === null) { - $this->modx->log( - modX::LOG_LEVEL_WARN, - "[CategoryProductsController] Unknown method: {$method}" - ); - - return Response::error('Unknown method', HttpStatus::BAD_REQUEST)->getData(); - } + $access = CategoryProductActionPermissions::evaluate( + $method, + fn(string $permission): bool => $this->modx->hasPermission($permission) + ); + if (!$access['allowed']) { + if ($access['reason'] === 'unknown_method') { + $this->modx->log( + modX::LOG_LEVEL_WARN, + "[CategoryProductsController] Unknown method: {$method}" + ); + } else { + $this->modx->log( + modX::LOG_LEVEL_WARN, + '[CategoryProductsController] Access denied for permission ' + . ($access['permission'] ?? '') + . ' (user id ' . (int)($this->modx->user?->get('id') ?? 0) . ')' + ); + } - if ($denied = $this->denyWithoutPermission($permission)) { - return $denied; + return Response::error($access['message'], $access['status'])->getData(); } if (empty($ids) || !is_array($ids)) { diff --git a/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php b/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php index 1fa17699..265e451b 100644 --- a/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php +++ b/core/components/minishop3/src/Services/Category/CategoryProductActionPermissions.php @@ -2,6 +2,8 @@ namespace MiniShop3\Services\Category; +use MiniShop3\Router\HttpStatus; + /** Maps category product bulk/multiple actions to MODX permissions (#378). */ final class CategoryProductActionPermissions { @@ -24,4 +26,48 @@ public static function forMethod(string $method): ?string default => null, }; } + + /** + * Evaluate access for a bulk/multiple action before any product mutation. + * + * @param callable(string):bool $hasPermission + * @return array{ + * allowed: bool, + * status: int, + * message: string, + * permission: ?string, + * reason: 'ok'|'unknown_method'|'forbidden' + * } + */ + public static function evaluate(string $method, callable $hasPermission): array + { + $permission = self::forMethod($method); + if ($permission === null) { + return [ + 'allowed' => false, + 'status' => HttpStatus::BAD_REQUEST, + 'message' => 'Unknown method', + 'permission' => null, + 'reason' => 'unknown_method', + ]; + } + + if (!$hasPermission($permission)) { + return [ + 'allowed' => false, + 'status' => HttpStatus::FORBIDDEN, + 'message' => "Access denied. Required permission: {$permission}", + 'permission' => $permission, + 'reason' => 'forbidden', + ]; + } + + return [ + 'allowed' => true, + 'status' => HttpStatus::OK, + 'message' => '', + 'permission' => $permission, + 'reason' => 'ok', + ]; + } } diff --git a/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php b/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php index c36db1c1..4aee8695 100644 --- a/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php +++ b/core/components/minishop3/tests/CategoryProductActionPermissionsTest.php @@ -1,7 +1,10 @@ false; +$allowAll = static fn(string $permission): bool => true; +$allowOnly = static function (string $allowed) { + return static fn(string $permission): bool => $permission === $allowed; +}; + +// Unknown method → 400 (contract change vs old silent loop → 500) +$unknown = CategoryProductActionPermissions::evaluate('not-a-method', $denyAll); +$assertSame(false, $unknown['allowed'], 'unknown allowed'); +$assertSame(HttpStatus::BAD_REQUEST, $unknown['status'], 'unknown status'); +$assertSame('unknown_method', $unknown['reason'], 'unknown reason'); +$assertSame('Unknown method', $unknown['message'], 'unknown message'); + +// Missing permission → 403 before any product mutation +$forbidden = CategoryProductActionPermissions::evaluate('delete', $denyAll); +$assertSame(false, $forbidden['allowed'], 'forbidden allowed'); +$assertSame(HttpStatus::FORBIDDEN, $forbidden['status'], 'forbidden status'); +$assertSame('forbidden', $forbidden['reason'], 'forbidden reason'); +$assertSame('msproduct_delete', $forbidden['permission'], 'forbidden permission'); +$assertSame( + 'Access denied. Required permission: msproduct_delete', + $forbidden['message'], + 'forbidden message' +); + +// Wrong permission for method → still 403 (no escalation via method param) +$wrongPerm = CategoryProductActionPermissions::evaluate('publish', $allowOnly('msproduct_delete')); +$assertSame(false, $wrongPerm['allowed'], 'wrongPerm allowed'); +$assertSame(HttpStatus::FORBIDDEN, $wrongPerm['status'], 'wrongPerm status'); +$assertSame('msproduct_publish', $wrongPerm['permission'], 'wrongPerm permission'); + +// Authorized → ok (controller may proceed to load products) +$ok = CategoryProductActionPermissions::evaluate('delete', $allowOnly('msproduct_delete')); +$assertSame(true, $ok['allowed'], 'ok allowed'); +$assertSame(HttpStatus::OK, $ok['status'], 'ok status'); +$assertSame('ok', $ok['reason'], 'ok reason'); + +// bulkDelete path forces method=delete before evaluate (same 403 semantics) +$bulkMethod = 'delete'; +$bulk = CategoryProductActionPermissions::evaluate($bulkMethod, $denyAll); +$assertSame(HttpStatus::FORBIDDEN, $bulk['status'], 'bulkDelete-equivalent status'); +$assertSame('msproduct_delete', $bulk['permission'], 'bulkDelete-equivalent permission'); + +// publish() defense-in-depth uses the same forbidden shape for msproduct_publish +$publishDenied = CategoryProductActionPermissions::evaluate('publish', $denyAll); +$assertSame(HttpStatus::FORBIDDEN, $publishDenied['status'], 'publish denied status'); +$assertSame('msproduct_publish', $publishDenied['permission'], 'publish denied permission'); + +$assertSame(true, CategoryProductActionPermissions::evaluate('show', $allowAll)['allowed'], 'show allowAll'); + fwrite(STDOUT, "OK: CategoryProductActionPermissionsTest\n");