Skip to content

Commit 3939be4

Browse files
IbochkarevIbochkarev
committed
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
1 parent ec6bb69 commit 3939be4

7 files changed

Lines changed: 384 additions & 20 deletions

File tree

‎core/components/minishop3/src/Controllers/Api/Manager/CategoryProductsController.php‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use MiniShop3\Router\HttpStatus;
88
use MiniShop3\Router\Response;
99
use MiniShop3\Services\Category\CategoryProductActionPermissions;
10+
use MiniShop3\Services\Category\CategoryProductDocumentPolicy;
1011
use MiniShop3\Services\Category\CategoryProductScopeService;
1112
use MiniShop3\Services\Category\CategoryProductsListService;
1213
use MiniShop3\Services\FilterConfigManager;
@@ -293,20 +294,26 @@ public function updateProductData(array $params = []): array
293294
return Response::error('Invalid request data', HttpStatus::BAD_REQUEST)->getData();
294295
}
295296

296-
/** @var CategoryProductsListService|null $listService */
297-
$listService = $this->modx->services->get('ms3_category_products_list');
298-
if (!$listService) {
297+
// Scope check + product fetch in one round-trip (CategoryProductScopePolicy).
298+
// Replaces the separate isProductInCategoryScope() bool-only lookup and yields
299+
// the product instance for the document ACL check below (#473 pattern).
300+
$product = $this->scopeService()->findInCategory($categoryId, $productId, $nested);
301+
302+
if (!$product) {
303+
$this->modx->lexicon->load('minishop3:default');
304+
299305
return Response::error(
300-
'Category products list service is not available',
301-
HttpStatus::INTERNAL_SERVER_ERROR
306+
$this->modx->lexicon('ms3_err_product_not_in_category_scope'),
307+
HttpStatus::FORBIDDEN
302308
)->getData();
303309
}
304310

305-
if (!$listService->isProductInCategoryScope($productId, $categoryId, $nested)) {
306-
$this->modx->lexicon->load('minishop3:default');
311+
$savePolicies = [CategoryProductDocumentPolicy::POLICY_SAVE];
312+
if (!CategoryProductDocumentPolicy::isAllowedAll($product, $savePolicies)) {
313+
$this->logDocumentPolicyDenied($product, $savePolicies);
307314

308315
return Response::error(
309-
$this->modx->lexicon('ms3_err_product_not_in_category_scope'),
316+
'Save permission denied for this document',
310317
HttpStatus::FORBIDDEN
311318
)->getData();
312319
}
@@ -395,6 +402,19 @@ private function scopeService(): CategoryProductScopeService
395402
: new CategoryProductScopeService($this->modx);
396403
}
397404

405+
/** @param list<string> $policies */
406+
private function logDocumentPolicyDenied(object $product, array $policies): void
407+
{
408+
$this->modx->log(
409+
modX::LOG_LEVEL_WARN,
410+
'[CategoryProductsController] Document policy denied ('
411+
. implode(',', $policies)
412+
. ') for product '
413+
. (int) $product->get('id')
414+
. ' (user id ' . (int) ($this->modx->user->get('id') ?? 0) . ')'
415+
);
416+
}
417+
398418
private function denyWithoutPermission(string $permission): ?array
399419
{
400420
if ($this->modx->hasPermission($permission)) {

‎core/components/minishop3/src/Services/Category/CategoryProductDocumentPolicy.php‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
<?php
2+
3+
namespace MiniShop3\Services\Category;
4+
5+
use MiniShop3\Model\msCategory;
6+
use MiniShop3\Model\msProduct;
7+
use MiniShop3\Router\HttpStatus;
8+
use MiniShop3\Router\Response;
9+
use MODX\Revolution\modX;
10+
11+
/**
12+
* Resource-level MODX ACL (checkPolicy) for category product Manager API (#445).
13+
*
14+
* Complements global msproduct_* / view_document route permissions (#378).
15+
*/
16+
final class CategoryProductDocumentPolicy
17+
{
18+
public const POLICY_VIEW = 'view';
19+
public const POLICY_SAVE = 'save';
20+
public const POLICY_PUBLISH = 'publish';
21+
public const POLICY_DELETE = 'delete';
22+
public const POLICY_UNPUBLISH = 'unpublish';
23+
public const POLICY_UNDELETE = 'undelete';
24+
25+
public static function categoryViewPolicy(): string
26+
{
27+
return self::POLICY_VIEW;
28+
}
29+
30+
/** @return list<string> */
31+
public static function sortPolicies(): array
32+
{
33+
return [self::POLICY_SAVE];
34+
}
35+
36+
/** @return list<string> */
37+
public static function policiesForPublish(bool $published): array
38+
{
39+
return $published
40+
? [self::POLICY_PUBLISH]
41+
: [self::POLICY_SAVE, self::POLICY_UNPUBLISH];
42+
}
43+
44+
public static function isAllowed(object $resource, string $policy): bool
45+
{
46+
return self::evaluate($resource, $policy) === null;
47+
}
48+
49+
/** @param list<string> $policies */
50+
public static function isAllowedAll(object $resource, array $policies): bool
51+
{
52+
return self::evaluateAll($resource, $policies) === null;
53+
}
54+
55+
/**
56+
* @return array{status: int, message: string}|null null when allowed
57+
*/
58+
public static function evaluate(object $resource, string $policy): ?array
59+
{
60+
if (method_exists($resource, 'checkPolicy') && $resource->checkPolicy($policy)) {
61+
return null;
62+
}
63+
64+
return [
65+
'status' => HttpStatus::FORBIDDEN,
66+
'message' => self::messageForPolicy($policy),
67+
];
68+
}
69+
70+
/**
71+
* @param list<string> $policies
72+
* @return array{status: int, message: string}|null null when allowed
73+
*/
74+
public static function evaluateAll(object $resource, array $policies): ?array
75+
{
76+
foreach ($policies as $policy) {
77+
$denied = self::evaluate($resource, $policy);
78+
if ($denied !== null) {
79+
return $denied;
80+
}
81+
}
82+
83+
return null;
84+
}
85+
86+
public static function denialResponse(object $resource, string $policy): ?array
87+
{
88+
return self::toErrorResponse(self::evaluate($resource, $policy));
89+
}
90+
91+
/** @param list<string> $policies */
92+
public static function denialResponseAll(object $resource, array $policies): ?array
93+
{
94+
return self::toErrorResponse(self::evaluateAll($resource, $policies));
95+
}
96+
97+
public static function canViewInCategoryGrid(modX $modx, msProduct $product, bool $nested): bool
98+
{
99+
if (!self::isAllowed($product, self::POLICY_VIEW)) {
100+
return false;
101+
}
102+
103+
if (!$nested) {
104+
return true;
105+
}
106+
107+
$parentId = (int) $product->get('parent');
108+
if ($parentId <= 0) {
109+
return false;
110+
}
111+
112+
$parent = $modx->getObject(msCategory::class, $parentId);
113+
if (!$parent instanceof msCategory) {
114+
return false;
115+
}
116+
117+
return self::isAllowed($parent, self::POLICY_VIEW);
118+
}
119+
120+
/**
121+
* @param array<int, msCategory> $parentsById
122+
*/
123+
public static function canViewInCategoryGridCached(
124+
msProduct $product,
125+
bool $nested,
126+
array $parentsById,
127+
): bool {
128+
if (!self::isAllowed($product, self::POLICY_VIEW)) {
129+
return false;
130+
}
131+
132+
if (!$nested) {
133+
return true;
134+
}
135+
136+
$parentId = (int) $product->get('parent');
137+
if ($parentId <= 0) {
138+
return false;
139+
}
140+
141+
$parent = $parentsById[$parentId] ?? null;
142+
if (!$parent instanceof msCategory) {
143+
return false;
144+
}
145+
146+
return self::isAllowed($parent, self::POLICY_VIEW);
147+
}
148+
149+
public static function toErrorResponse(?array $evaluation): ?array
150+
{
151+
if ($evaluation === null) {
152+
return null;
153+
}
154+
155+
return Response::error(
156+
$evaluation['message'],
157+
$evaluation['status']
158+
)->getData();
159+
}
160+
161+
private static function messageForPolicy(string $policy): string
162+
{
163+
return match ($policy) {
164+
self::POLICY_VIEW => 'View permission denied for this document',
165+
self::POLICY_SAVE => 'Save permission denied for this document',
166+
self::POLICY_PUBLISH => 'Publish permission denied for this document',
167+
self::POLICY_DELETE => 'Delete permission denied for this document',
168+
self::POLICY_UNPUBLISH => 'Unpublish permission denied for this document',
169+
self::POLICY_UNDELETE => 'Undelete permission denied for this document',
170+
default => 'Access denied for this document',
171+
};
172+
}
173+
}

‎core/components/minishop3/src/Services/Category/CategoryProductScopeService.php‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,6 @@ public function findInCategory(int $categoryId, int $productId, bool $nested = f
2525
return null;
2626
}
2727

28-
$allowedParents = $this->treeService()->productParentIds($categoryId, $nested);
29-
30-
if ($allowedParents === []) {
31-
return null;
32-
}
33-
3428
if (!$nested) {
3529
/** @var msProduct|null $product */
3630
$product = $this->modx->getObject(msProduct::class, [
@@ -48,7 +42,17 @@ public function findInCategory(int $categoryId, int $productId, bool $nested = f
4842
return null;
4943
}
5044

51-
return in_array((int) $product->get('parent'), $allowedParents, true) ? $product : null;
45+
$parentId = (int) $product->get('parent');
46+
if ($parentId <= 0) {
47+
return null;
48+
}
49+
50+
return CategoryProductScopePolicy::isParentInScope(
51+
$parentId,
52+
$categoryId,
53+
true,
54+
$this->treeService()->getDescendantCategoryIds($categoryId)
55+
) ? $product : null;
5256
}
5357

5458
private function treeService(): CategoryTreeService

‎core/components/minishop3/tests/CategoryProductDocumentPolicyTest.php‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
<?php
2+
3+
/**
4+
* Resource policy map for category product document ACL (#445).
5+
*
6+
* Run: php tests/CategoryProductDocumentPolicyTest.php
7+
*/
8+
9+
declare(strict_types=1);
10+
11+
require __DIR__ . '/../vendor/autoload.php';
12+
13+
use MiniShop3\Router\HttpStatus;
14+
use MiniShop3\Services\Category\CategoryProductDocumentPolicy;
15+
16+
$fail = static function (string $message): never {
17+
fwrite(STDERR, "FAIL: {$message}\n");
18+
exit(1);
19+
};
20+
21+
$assertSame = static function ($expected, $actual, string $case) use ($fail): void {
22+
if ($actual !== $expected) {
23+
$fail($case . ': expected ' . var_export($expected, true) . ', got ' . var_export($actual, true));
24+
}
25+
};
26+
27+
$assertSame(
28+
[CategoryProductDocumentPolicy::POLICY_PUBLISH],
29+
CategoryProductDocumentPolicy::policiesForPublish(true),
30+
'policiesForPublish(true)'
31+
);
32+
$assertSame(
33+
[CategoryProductDocumentPolicy::POLICY_SAVE, CategoryProductDocumentPolicy::POLICY_UNPUBLISH],
34+
CategoryProductDocumentPolicy::policiesForPublish(false),
35+
'policiesForPublish(false)'
36+
);
37+
$assertSame(
38+
[CategoryProductDocumentPolicy::POLICY_SAVE],
39+
CategoryProductDocumentPolicy::sortPolicies(),
40+
'sortPolicies'
41+
);
42+
$assertSame(
43+
CategoryProductDocumentPolicy::POLICY_VIEW,
44+
CategoryProductDocumentPolicy::categoryViewPolicy(),
45+
'categoryViewPolicy'
46+
);
47+
48+
$allowed = new class {
49+
public function checkPolicy(string $policy): bool
50+
{
51+
return $policy === 'view';
52+
}
53+
};
54+
55+
$denied = new class {
56+
public function checkPolicy(string $policy): bool
57+
{
58+
return false;
59+
}
60+
};
61+
62+
$partial = new class {
63+
public function checkPolicy(string $policy): bool
64+
{
65+
return $policy === 'save';
66+
}
67+
};
68+
69+
$assertSame(true, CategoryProductDocumentPolicy::isAllowed($allowed, 'view'), 'isAllowed view');
70+
$assertSame(false, CategoryProductDocumentPolicy::isAllowed($denied, 'view'), 'isAllowed denied');
71+
$assertSame(
72+
true,
73+
CategoryProductDocumentPolicy::isAllowedAll($partial, ['save']),
74+
'isAllowedAll single save'
75+
);
76+
$assertSame(
77+
false,
78+
CategoryProductDocumentPolicy::isAllowedAll($partial, ['save', 'unpublish']),
79+
'isAllowedAll compound partial failure'
80+
);
81+
82+
$viewDenied = CategoryProductDocumentPolicy::evaluate($denied, 'view');
83+
$assertSame(HttpStatus::FORBIDDEN, $viewDenied['status'] ?? null, 'evaluate denied status');
84+
$assertSame(
85+
'View permission denied for this document',
86+
$viewDenied['message'] ?? null,
87+
'evaluate denied message'
88+
);
89+
90+
$noPolicyObject = new stdClass();
91+
$noPolicyDenied = CategoryProductDocumentPolicy::evaluate($noPolicyObject, 'view');
92+
$assertSame(HttpStatus::FORBIDDEN, $noPolicyDenied['status'] ?? null, 'evaluate without checkPolicy status');
93+
94+
fwrite(STDOUT, "OK: CategoryProductDocumentPolicyTest\n");

0 commit comments

Comments
 (0)