From 89927c7ca66d56ef2196eaacb273f6ff5e3ac8ba Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 19 Aug 2026 21:59:04 +0600 Subject: [PATCH 1/2] fix: seed miniShopManagerPolicy and relax order form field reads Package the default manager policy under its template, repair missing policies on install/upgrade, and allow authenticated mgr reads on model-fields/extra-fields without mssetting_save while keeping writes gated. Closes #613 --- _build/build.php | 71 +++++---- _build/elements/policies.php | 27 +--- _build/resolvers/resolver_09_policies.php | 79 +++++++++ .../config/manager_access_policy.php | 34 ++++ .../minishop3/config/routes/manager.php | 33 ++-- .../tests/ExtraFieldsRouteAclTest.php | 127 +++++++++++++++ .../tests/ModelFieldsRouteAclTest.php | 150 ++++++++++++++++++ .../minishop3/tests/PoliciesPackagingTest.php | 69 ++++++++ 8 files changed, 521 insertions(+), 69 deletions(-) create mode 100644 _build/resolvers/resolver_09_policies.php create mode 100644 core/components/minishop3/config/manager_access_policy.php create mode 100644 core/components/minishop3/tests/ExtraFieldsRouteAclTest.php create mode 100644 core/components/minishop3/tests/ModelFieldsRouteAclTest.php create mode 100644 core/components/minishop3/tests/PoliciesPackagingTest.php diff --git a/_build/build.php b/_build/build.php index cb4aa7110..ea4aee0e6 100644 --- a/_build/build.php +++ b/_build/build.php @@ -471,40 +471,15 @@ private function chunks(): void } /** - * Add access policy + * Access policies are nested under policy templates (see policyTemplates()). */ private function policies(): void { - /** @noinspection PhpIncludeInspection */ - $policies = include($this->config['elements'] . 'policies.php'); - if (!is_array($policies)) { - $this->modx->log(modX::LOG_LEVEL_ERROR, 'Could not package in Access Policies'); - return; - } - $attributes = [ - xPDOTransport::PRESERVE_KEYS => false, - xPDOTransport::UNIQUE_KEY => ['name'], - xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['policies']), - ]; - foreach ($policies as $name => $data) { - if (isset($data['data'])) { - $data['data'] = json_encode($data['data']); - } - /** @var $policy modAccessPolicy */ - $policy = $this->modx->newObject(modAccessPolicy::class); - $policy->fromArray(array_merge([ - 'name' => $name, - 'lexicon' => $this->config['name_lower'] . ':permissions', - ], $data) - , '', true, true); - $vehicle = $this->builder->createVehicle($policy, $attributes); - $this->builder->putVehicle($vehicle); - } - $this->modx->log(modX::LOG_LEVEL_INFO, 'Packaged in ' . count($policies) . ' Access Policies'); + $this->modx->log(modX::LOG_LEVEL_INFO, 'Access policies packaged via policyTemplates()'); } /** - * Add policy templates + * Add policy templates (and nested default policies). */ private function policyTemplates(): void { @@ -514,6 +489,11 @@ private function policyTemplates(): void $this->modx->log(modX::LOG_LEVEL_ERROR, 'Could not package in Policy Templates'); return; } + /** @noinspection PhpIncludeInspection */ + $policy_definitions = include($this->config['elements'] . 'policies.php'); + if (!is_array($policy_definitions)) { + $policy_definitions = []; + } $attributes = [ xPDOTransport::PRESERVE_KEYS => false, xPDOTransport::UNIQUE_KEY => ['name'], @@ -525,6 +505,11 @@ private function policyTemplates(): void xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['permission']), xPDOTransport::UNIQUE_KEY => ['template', 'name'], ], + 'Policies' => [ + xPDOTransport::PRESERVE_KEYS => false, + xPDOTransport::UPDATE_OBJECT => !empty($this->config['update']['policies']), + xPDOTransport::UNIQUE_KEY => ['name'], + ], ], ]; foreach ($policy_templates as $name => $data) { @@ -542,17 +527,37 @@ private function policyTemplates(): void $permissions[] = $permission; } } - /** @var $permission modAccessPolicyTemplate */ - $permission = $this->modx->newObject(modAccessPolicyTemplate::class); - $permission->fromArray(array_merge([ + /** @var modAccessPolicyTemplate $template */ + $template = $this->modx->newObject(modAccessPolicyTemplate::class); + $template->fromArray(array_merge([ 'name' => $name, 'lexicon' => $this->config['name_lower'] . ':permissions', ], $data) , '', true, true); if (!empty($permissions)) { - $permission->addMany($permissions); + $template->addMany($permissions); + } + if ($name === 'miniShopManagerPolicyTemplate' && $policy_definitions !== []) { + $policies = []; + foreach ($policy_definitions as $policyName => $policyData) { + $payload = $policyData; + if (isset($payload['data']) && is_array($payload['data'])) { + $payload['data'] = json_encode($payload['data']); + } + /** @var modAccessPolicy $policy */ + $policy = $this->modx->newObject(modAccessPolicy::class); + $policy->fromArray(array_merge([ + 'name' => $policyName, + 'lexicon' => $this->config['name_lower'] . ':permissions', + ], $payload) + , '', true, true); + $policies[] = $policy; + } + if ($policies !== []) { + $template->addMany($policies, 'Policies'); + } } - $vehicle = $this->builder->createVehicle($permission, $attributes); + $vehicle = $this->builder->createVehicle($template, $attributes); $this->builder->putVehicle($vehicle); } $this->modx->log(modX::LOG_LEVEL_INFO, 'Packaged in ' . count($policy_templates) . ' Access Policy Templates'); diff --git a/_build/elements/policies.php b/_build/elements/policies.php index f1a092703..cad37eba1 100644 --- a/_build/elements/policies.php +++ b/_build/elements/policies.php @@ -1,28 +1,5 @@ [ - 'description' => 'A policy for create and update MiniShop3 categories and products.', - 'parent' => 0, - 'class' => '', - 'lexicon' => 'minishop3:permissions', - 'data' => array_fill_keys($permissions, true), - ], -]; +return require dirname(__DIR__, 2) . '/core/components/minishop3/config/manager_access_policy.php'; diff --git a/_build/resolvers/resolver_09_policies.php b/_build/resolvers/resolver_09_policies.php new file mode 100644 index 000000000..d3eb5f143 --- /dev/null +++ b/_build/resolvers/resolver_09_policies.php @@ -0,0 +1,79 @@ + $options */ +if (!$transport->xpdo || !($transport instanceof xPDOTransport)) { + return false; +} + +$modx = $transport->xpdo; +$action = $options[xPDOTransport::PACKAGE_ACTION] ?? null; +if (!in_array($action, [xPDOTransport::ACTION_INSTALL, xPDOTransport::ACTION_UPGRADE], true)) { + return true; +} + +$definitionsFile = MODX_CORE_PATH . 'components/minishop3/config/manager_access_policy.php'; +if (!is_readable($definitionsFile)) { + $modx->log(modX::LOG_LEVEL_ERROR, '[MiniShop3] manager_access_policy.php not found'); + + return false; +} + +/** @var array> $definitions */ +$definitions = require $definitionsFile; +$template = $modx->getObject(modAccessPolicyTemplate::class, ['name' => 'miniShopManagerPolicyTemplate']); +if (!$template instanceof modAccessPolicyTemplate) { + $modx->log(modX::LOG_LEVEL_WARN, '[MiniShop3] miniShopManagerPolicyTemplate not found; skip policy repair'); + + return true; +} + +$templateId = (int) $template->get('id'); + +foreach ($definitions as $name => $data) { + /** @var modAccessPolicy|null $policy */ + $policy = $modx->getObject(modAccessPolicy::class, ['name' => $name]); + if ($policy instanceof modAccessPolicy) { + if ((int) $policy->get('template') !== $templateId) { + $policy->set('template', $templateId); + if (!$policy->save()) { + $modx->log(modX::LOG_LEVEL_ERROR, "[MiniShop3] Failed to link policy {$name} to template"); + + return false; + } + $modx->log(modX::LOG_LEVEL_INFO, "[MiniShop3] Linked existing policy {$name} to template"); + } + // Existing policy data is preserved (custom site ACL overrides). + continue; + } + + $payload = $data; + if (isset($payload['data']) && is_array($payload['data'])) { + $payload['data'] = json_encode($payload['data']); + } + + $policy = $modx->newObject(modAccessPolicy::class); + $policy->fromArray(array_merge([ + 'name' => $name, + 'lexicon' => 'minishop3:permissions', + 'template' => $templateId, + ], $payload), '', true, true); + + if ($policy->save()) { + $modx->log(modX::LOG_LEVEL_INFO, "[MiniShop3] Created access policy {$name}"); + continue; + } + + $modx->log(modX::LOG_LEVEL_ERROR, "[MiniShop3] Failed to create access policy {$name}"); + + return false; +} + +return true; diff --git a/core/components/minishop3/config/manager_access_policy.php b/core/components/minishop3/config/manager_access_policy.php new file mode 100644 index 000000000..7ad694fb0 --- /dev/null +++ b/core/components/minishop3/config/manager_access_policy.php @@ -0,0 +1,34 @@ + [ + 'description' => 'A policy for create and update MiniShop3 categories and products.', + 'parent' => 0, + 'class' => '', + 'lexicon' => 'minishop3:permissions', + 'data' => array_fill_keys($permissions, true), + ], +]; diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index b86435845..28066761a 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -189,6 +189,7 @@ new PermissionMiddleware($modx, 'view_document') ]); + // Extra fields reads: order/product forms load schema without settings perm (#613) $router->group('/extra-fields', function ($router) use ($modx) { $router->get('', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); @@ -198,6 +199,10 @@ $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); return $controller->get($params); }); + }); + + // Extra fields writes: schema mutations require mssetting_save (#381) + $router->group('/extra-fields', function ($router) use ($modx) { $router->post('', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ExtraFieldsController($modx); return $controller->create(); @@ -984,6 +989,7 @@ new PermissionMiddleware($modx, 'mssetting_save') ]); + // Model fields reads: order/product forms load schema without settings perm (#613) $router->group('/model-fields', function ($router) use ($modx) { $router->get('/models', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); @@ -1009,6 +1015,22 @@ $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); return $controller->getSections($params); }); + + // Field routes + $router->get('', function ($params) use ($modx) { + $allParams = array_merge($_GET, $params); + + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->getList($allParams); + }); + $router->get('/{id}', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->get($params); + }); + }); + + // Model fields writes: schema mutations require mssetting_save (#381) + $router->group('/model-fields', function ($router) use ($modx) { $router->post('/sections', function ($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; @@ -1036,17 +1058,6 @@ return $controller->deleteSection($params); }); - // Field routes - $router->get('', function ($params) use ($modx) { - $allParams = array_merge($_GET, $params); - - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->getList($allParams); - }); - $router->get('/{id}', function ($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->get($params); - }); $router->post('', function ($params) use ($modx) { $input = file_get_contents('php://input'); $data = json_decode($input, true) ?: []; diff --git a/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php b/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php new file mode 100644 index 000000000..e39ad9608 --- /dev/null +++ b/core/components/minishop3/tests/ExtraFieldsRouteAclTest.php @@ -0,0 +1,127 @@ + + */ +$extractRouteGroups = static function (string $src, string $path): array { + $needle = "\$router->group('{$path}'"; + $groups = []; + $offset = 0; + $len = strlen($src); + + while (($pos = strpos($src, $needle, $offset)) !== false) { + $open = strpos($src, 'function ($router)', $pos); + if ($open === false) { + break; + } + $braceStart = strpos($src, '{', $open); + if ($braceStart === false) { + break; + } + + $depth = 0; + $closedAt = null; + for ($i = $braceStart; $i < $len; $i++) { + $ch = $src[$i]; + if ($ch === '{') { + $depth++; + } elseif ($ch === '}') { + $depth--; + if ($depth === 0) { + $closedAt = $i; + break; + } + } + } + + if ($closedAt === null) { + break; + } + + $body = substr($src, $braceStart + 1, $closedAt - $braceStart - 1); + $perm = null; + if ( + preg_match( + '/^\}\s*,\s*\[\s*new\s+PermissionMiddleware\(\s*\$modx\s*,\s*\'([^\']+)\'\s*\)/', + substr($src, $closedAt, 200), + $permMatch + ) + ) { + $perm = $permMatch[1]; + } + + $groups[] = ['body' => $body, 'perm' => $perm]; + $offset = $closedAt + 1; + } + + return $groups; +}; + +$routesFile = dirname(__DIR__) . '/config/routes/manager.php'; +$src = file_get_contents($routesFile); +if ($src === false || $src === '') { + $fail('cannot read manager.php routes'); +} + +$groups = $extractRouteGroups($src, '/extra-fields'); +if (count($groups) !== 2) { + $fail('expected two /extra-fields route groups (read + write), got ' . count($groups)); +} + +/** @var array $routePerm */ +$routePerm = []; + +foreach ($groups as $group) { + if ( + preg_match_all( + "/\\\$router->(get|put|post|delete)\(\s*'([^']*)'/", + $group['body'], + $routeMatches, + PREG_SET_ORDER + ) + ) { + foreach ($routeMatches as $routeMatch) { + $key = strtoupper($routeMatch[1]) . ' ' . $routeMatch[2]; + $routePerm[$key] = $group['perm']; + } + } +} + +$expected = [ + 'GET ' => null, + 'GET /{id}' => null, + 'POST ' => 'mssetting_save', + 'PUT /{id}' => 'mssetting_save', + 'DELETE /{id}' => 'mssetting_save', +]; + +foreach ($expected as $key => $perm) { + if (!array_key_exists($key, $routePerm)) { + $fail("missing route {$key}"); + } + if ($routePerm[$key] !== $perm) { + $fail("{$key} ACL mismatch"); + } +} + +foreach ($routePerm as $key => $perm) { + if (!array_key_exists($key, $expected)) { + $fail("unexpected extra-fields route registered: {$key}"); + } +} + +fwrite(STDOUT, "OK ExtraFieldsRouteAclTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/ModelFieldsRouteAclTest.php b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php new file mode 100644 index 000000000..8d26e1d9f --- /dev/null +++ b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php @@ -0,0 +1,150 @@ + + */ +$extractRouteGroups = static function (string $src, string $path): array { + $needle = "\$router->group('{$path}'"; + $groups = []; + $offset = 0; + $len = strlen($src); + + while (($pos = strpos($src, $needle, $offset)) !== false) { + $open = strpos($src, 'function ($router)', $pos); + if ($open === false) { + break; + } + $braceStart = strpos($src, '{', $open); + if ($braceStart === false) { + break; + } + + $depth = 0; + $closedAt = null; + for ($i = $braceStart; $i < $len; $i++) { + $ch = $src[$i]; + if ($ch === '{') { + $depth++; + } elseif ($ch === '}') { + $depth--; + if ($depth === 0) { + $closedAt = $i; + break; + } + } + } + + if ($closedAt === null) { + break; + } + + $body = substr($src, $braceStart + 1, $closedAt - $braceStart - 1); + $perm = null; + if ( + preg_match( + '/^\}\s*,\s*\[\s*new\s+PermissionMiddleware\(\s*\$modx\s*,\s*\'([^\']+)\'\s*\)/', + substr($src, $closedAt, 200), + $permMatch + ) + ) { + $perm = $permMatch[1]; + } + + $groups[] = ['body' => $body, 'perm' => $perm]; + $offset = $closedAt + 1; + } + + return $groups; +}; + +$routesFile = dirname(__DIR__) . '/config/routes/manager.php'; +$src = file_get_contents($routesFile); +if ($src === false || $src === '') { + $fail('cannot read manager.php routes'); +} + +$groups = $extractRouteGroups($src, '/model-fields'); +if ($groups === []) { + $fail('no /model-fields route groups found'); +} + +/** @var array $routePerm */ +$routePerm = []; + +foreach ($groups as $group) { + if ( + preg_match_all( + "/\\\$router->(get|put|post|delete)\(\s*'([^']*)'/", + $group['body'], + $routeMatches, + PREG_SET_ORDER + ) + ) { + foreach ($routeMatches as $routeMatch) { + $key = strtoupper($routeMatch[1]) . ' ' . $routeMatch[2]; + if (array_key_exists($key, $routePerm) && $routePerm[$key] !== $group['perm']) { + $fail("conflicting permissions for {$key}"); + } + $routePerm[$key] = $group['perm']; + } + } +} + +$expectedReads = [ + 'GET /models', + 'GET /visible/{model}', + 'GET /combo-options/{model}', + 'GET /combo-options/{model}/{field_name}', + 'GET /sections/{model}', + 'GET ', + 'GET /{id}', +]; + +$expectedWrites = [ + 'POST /sections' => 'mssetting_save', + 'PUT /sections/ranks' => 'mssetting_save', + 'PUT /sections/{id}' => 'mssetting_save', + 'DELETE /sections/{id}' => 'mssetting_save', + 'POST ' => 'mssetting_save', + 'PUT /ranks' => 'mssetting_save', + 'PUT /{id}' => 'mssetting_save', + 'DELETE /{id}' => 'mssetting_save', +]; + +foreach ($expectedReads as $key) { + if (!array_key_exists($key, $routePerm)) { + $fail("missing read route {$key}"); + } + if ($routePerm[$key] !== null) { + $fail("{$key} must be auth-only, got: " . var_export($routePerm[$key], true)); + } +} + +foreach ($expectedWrites as $key => $perm) { + if (($routePerm[$key] ?? null) !== $perm) { + $fail("{$key} must require {$perm}, got: " . var_export($routePerm[$key] ?? null, true)); + } +} + +$expectedKeys = array_merge($expectedReads, array_keys($expectedWrites)); +foreach ($routePerm as $key => $perm) { + if (!in_array($key, $expectedKeys, true)) { + $fail("unexpected model-fields route registered: {$key}"); + } +} + +fwrite(STDOUT, "OK ModelFieldsRouteAclTest\n"); +exit(0); diff --git a/core/components/minishop3/tests/PoliciesPackagingTest.php b/core/components/minishop3/tests/PoliciesPackagingTest.php new file mode 100644 index 000000000..8f5cf8366 --- /dev/null +++ b/core/components/minishop3/tests/PoliciesPackagingTest.php @@ -0,0 +1,69 @@ + [")) { + $fail('policyTemplates() must declare Policies related object attributes'); +} + +if (!str_contains($buildSrc, "addMany(\$policies, 'Policies')")) { + $fail('miniShopManagerPolicy must be nested under miniShopManagerPolicyTemplate'); +} + +if (!preg_match('/private function policies\(\): void\s*\{[^}]*Packaged via policyTemplates/s', $buildSrc)) { + if (!str_contains($buildSrc, 'Access policies packaged via policyTemplates()')) { + $fail('standalone policies() must be no-op with packaging note'); + } +} + +$resolverPath = $repoRoot . '/_build/resolvers/resolver_09_policies.php'; +if (!is_readable($resolverPath)) { + $fail('resolver_09_policies.php missing'); +} + +$resolverSrc = file_get_contents($resolverPath); +if ($resolverSrc === false || !str_contains($resolverSrc, 'manager_access_policy.php')) { + $fail('resolver must load manager_access_policy.php'); +} + +$policyConfig = $repoRoot . '/core/components/minishop3/config/manager_access_policy.php'; +if (!is_readable($policyConfig)) { + $fail('manager_access_policy.php missing'); +} + +/** @var array> $definitions */ +$definitions = require $policyConfig; +if (!isset($definitions['miniShopManagerPolicy']['data']['msorder_save'])) { + $fail('miniShopManagerPolicy must grant msorder_save'); +} +if (!isset($definitions['miniShopManagerPolicy']['data']['mssetting_save'])) { + $fail('miniShopManagerPolicy must grant mssetting_save'); +} + +$elementsPolicies = $repoRoot . '/_build/elements/policies.php'; +$elementsSrc = file_get_contents($elementsPolicies); +if ($elementsSrc === false || !str_contains($elementsSrc, 'manager_access_policy.php')) { + $fail('_build/elements/policies.php must require manager_access_policy.php'); +} + +fwrite(STDOUT, "OK PoliciesPackagingTest\n"); +exit(0); From e90f852065cd7b37319ec623b05047c993fe309d Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 7 Sep 2026 20:13:54 +0600 Subject: [PATCH 2/2] fix(mgr-api): gate model-fields data reads behind form permissions combo-options and visible execute combo sources (customer PII), so they require AnyPermissionMiddleware instead of mgr-auth alone. Add router ACL smoke coverage and clarify the policy resolver data comment. --- _build/resolvers/resolver_09_policies.php | 3 +- .../minishop3/config/routes/manager.php | 47 ++-- .../tests/ModelFieldsRouteAclTest.php | 63 +++-- .../tests/ModelFieldsRoutePermissionsTest.php | 243 ++++++++++++++++++ 4 files changed, 318 insertions(+), 38 deletions(-) create mode 100644 core/components/minishop3/tests/ModelFieldsRoutePermissionsTest.php diff --git a/_build/resolvers/resolver_09_policies.php b/_build/resolvers/resolver_09_policies.php index d3eb5f143..caaef5e48 100644 --- a/_build/resolvers/resolver_09_policies.php +++ b/_build/resolvers/resolver_09_policies.php @@ -50,7 +50,8 @@ } $modx->log(modX::LOG_LEVEL_INFO, "[MiniShop3] Linked existing policy {$name} to template"); } - // Existing policy data is preserved (custom site ACL overrides). + // Existing policy data is left untouched by this resolver (no fromArray overwrite). + // Note: transport update.policies=true may still rewrite policy data on upgrade via xPDOObjectVehicle. continue; } diff --git a/core/components/minishop3/config/routes/manager.php b/core/components/minishop3/config/routes/manager.php index 28066761a..200312ff1 100644 --- a/core/components/minishop3/config/routes/manager.php +++ b/core/components/minishop3/config/routes/manager.php @@ -989,26 +989,13 @@ new PermissionMiddleware($modx, 'mssetting_save') ]); - // Model fields reads: order/product forms load schema without settings perm (#613) + // Model fields schema reads: order/product forms load layout without settings perm (#613). + // Pure metadata — no combo source execution. $router->group('/model-fields', function ($router) use ($modx) { $router->get('/models', function ($params) use ($modx) { $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); return $controller->getModels(); }); - $router->get('/visible/{model}', function ($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->getVisibleFields($params); - }); - - // Combo options routes - $router->get('/combo-options/{model}', function ($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->getComboOptions($params); - }); - $router->get('/combo-options/{model}/{field_name}', function ($params) use ($modx) { - $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); - return $controller->getFieldComboOptions($params); - }); // Section routes $router->get('/sections/{model}', function ($params) use ($modx) { @@ -1029,6 +1016,36 @@ }); }); + // Model fields data reads: visible embeds comboOptions; combo-options executes sources + // (e.g. msCustomer PII). Require a form/settings permission — not mgr-auth alone (#613). + $router->group('/model-fields', function ($router) use ($modx) { + $router->get('/visible/{model}', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->getVisibleFields($params); + }); + + $router->get('/combo-options/{model}', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->getComboOptions($params); + }); + $router->get('/combo-options/{model}/{field_name}', function ($params) use ($modx) { + $controller = new \MiniShop3\Controllers\Api\Manager\ModelFieldsController($modx); + return $controller->getFieldComboOptions($params); + }); + }, [ + new AnyPermissionMiddleware($modx, [ + 'msorder_list', + 'msorder_view', + 'msorder_save', + 'msproduct_save', + 'mscategory_save', + 'mssetting_list', + 'mssetting_view', + 'mssetting_save', + 'view_document', + ]), + ]); + // Model fields writes: schema mutations require mssetting_save (#381) $router->group('/model-fields', function ($router) use ($modx) { $router->post('/sections', function ($params) use ($modx) { diff --git a/core/components/minishop3/tests/ModelFieldsRouteAclTest.php b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php index 8d26e1d9f..2fbb97ae0 100644 --- a/core/components/minishop3/tests/ModelFieldsRouteAclTest.php +++ b/core/components/minishop3/tests/ModelFieldsRouteAclTest.php @@ -3,6 +3,9 @@ /** * Regression #613: /model-fields read vs write ACL map (no MODX). * + * Schema reads stay auth-only; data-bearing visible/combo-options use AnyPermissionMiddleware; + * writes require mssetting_save. + * * Запуск: php tests/ModelFieldsRouteAclTest.php */ @@ -14,7 +17,7 @@ }; /** - * @return list + * @return list */ $extractRouteGroups = static function (string $src, string $path): array { $needle = "\$router->group('{$path}'"; @@ -52,18 +55,21 @@ } $body = substr($src, $braceStart + 1, $closedAt - $braceStart - 1); - $perm = null; + $tail = substr($src, $closedAt, 400); + $gate = null; if ( preg_match( '/^\}\s*,\s*\[\s*new\s+PermissionMiddleware\(\s*\$modx\s*,\s*\'([^\']+)\'\s*\)/', - substr($src, $closedAt, 200), + $tail, $permMatch ) ) { - $perm = $permMatch[1]; + $gate = $permMatch[1]; + } elseif (preg_match('/^\}\s*,\s*\[\s*new\s+AnyPermissionMiddleware\(/', $tail)) { + $gate = 'any'; } - $groups[] = ['body' => $body, 'perm' => $perm]; + $groups[] = ['body' => $body, 'gate' => $gate]; $offset = $closedAt + 1; } @@ -81,8 +87,8 @@ $fail('no /model-fields route groups found'); } -/** @var array $routePerm */ -$routePerm = []; +/** @var array $routeGate */ +$routeGate = []; foreach ($groups as $group) { if ( @@ -95,24 +101,27 @@ ) { foreach ($routeMatches as $routeMatch) { $key = strtoupper($routeMatch[1]) . ' ' . $routeMatch[2]; - if (array_key_exists($key, $routePerm) && $routePerm[$key] !== $group['perm']) { + if (array_key_exists($key, $routeGate) && $routeGate[$key] !== $group['gate']) { $fail("conflicting permissions for {$key}"); } - $routePerm[$key] = $group['perm']; + $routeGate[$key] = $group['gate']; } } } -$expectedReads = [ +$expectedSchemaReads = [ 'GET /models', - 'GET /visible/{model}', - 'GET /combo-options/{model}', - 'GET /combo-options/{model}/{field_name}', 'GET /sections/{model}', 'GET ', 'GET /{id}', ]; +$expectedDataReads = [ + 'GET /visible/{model}' => 'any', + 'GET /combo-options/{model}' => 'any', + 'GET /combo-options/{model}/{field_name}' => 'any', +]; + $expectedWrites = [ 'POST /sections' => 'mssetting_save', 'PUT /sections/ranks' => 'mssetting_save', @@ -124,23 +133,33 @@ 'DELETE /{id}' => 'mssetting_save', ]; -foreach ($expectedReads as $key) { - if (!array_key_exists($key, $routePerm)) { - $fail("missing read route {$key}"); +foreach ($expectedSchemaReads as $key) { + if (!array_key_exists($key, $routeGate)) { + $fail("missing schema read route {$key}"); } - if ($routePerm[$key] !== null) { - $fail("{$key} must be auth-only, got: " . var_export($routePerm[$key], true)); + if ($routeGate[$key] !== null) { + $fail("{$key} must be auth-only, got: " . var_export($routeGate[$key], true)); + } +} + +foreach ($expectedDataReads as $key => $gate) { + if (($routeGate[$key] ?? null) !== $gate) { + $fail("{$key} must use AnyPermissionMiddleware, got: " . var_export($routeGate[$key] ?? null, true)); } } foreach ($expectedWrites as $key => $perm) { - if (($routePerm[$key] ?? null) !== $perm) { - $fail("{$key} must require {$perm}, got: " . var_export($routePerm[$key] ?? null, true)); + if (($routeGate[$key] ?? null) !== $perm) { + $fail("{$key} must require {$perm}, got: " . var_export($routeGate[$key] ?? null, true)); } } -$expectedKeys = array_merge($expectedReads, array_keys($expectedWrites)); -foreach ($routePerm as $key => $perm) { +$expectedKeys = array_merge( + $expectedSchemaReads, + array_keys($expectedDataReads), + array_keys($expectedWrites) +); +foreach ($routeGate as $key => $gate) { if (!in_array($key, $expectedKeys, true)) { $fail("unexpected model-fields route registered: {$key}"); } diff --git a/core/components/minishop3/tests/ModelFieldsRoutePermissionsTest.php b/core/components/minishop3/tests/ModelFieldsRoutePermissionsTest.php new file mode 100644 index 000000000..5ce0e820d --- /dev/null +++ b/core/components/minishop3/tests/ModelFieldsRoutePermissionsTest.php @@ -0,0 +1,243 @@ +loadRoutes(dirname(__DIR__) . '/config/routes/manager.php'); + $router->build(); + + return $router; +}; + +$assertDenied = static function ( + Router $router, + string $method, + string $uri, + string $label +) use ($assertSame): void { + $response = $router->dispatch($uri, $method); + $data = $response->getData(); + $assertSame(403, $response->getStatusCode(), "{$label}: status"); + $assertSame(false, $data['success'] ?? null, "{$label}: success"); + $assertSame(403, $data['code'] ?? null, "{$label}: body code"); +}; + +$assertAllowedPastPermissionGate = static function ( + Router $router, + string $method, + string $uri, + string $label +) use ($fail): void { + try { + $response = $router->dispatch($uri, $method); + } catch (\Throwable $e) { + // Handler may fail without full MODX DI; permission middleware already passed. + return; + } + if ($response->getStatusCode() === 403) { + $fail("{$label}: expected permission gate to pass, got 403"); + } +}; + +$_SERVER['HTTP_MODAUTH'] = 'test-modauth-token'; + +$modx = new modX(); +$router = $buildRouter($modx); + +$routesProperty = new ReflectionProperty(Router::class, 'routes'); +$registered = $routesProperty->getValue($router); + +$expectedAny = [ + 'GET /api/mgr/model-fields/visible/{model}', + 'GET /api/mgr/model-fields/combo-options/{model}', + 'GET /api/mgr/model-fields/combo-options/{model}/{field_name}', +]; + +$expectedAuthOnly = [ + 'GET /api/mgr/model-fields/models', + 'GET /api/mgr/model-fields/sections/{model}', + 'GET /api/mgr/model-fields', + 'GET /api/mgr/model-fields/{id}', +]; + +$expectedWrite = [ + 'POST /api/mgr/model-fields/sections' => 'mssetting_save', + 'PUT /api/mgr/model-fields/sections/ranks' => 'mssetting_save', + 'PUT /api/mgr/model-fields/sections/{id}' => 'mssetting_save', + 'DELETE /api/mgr/model-fields/sections/{id}' => 'mssetting_save', + 'POST /api/mgr/model-fields' => 'mssetting_save', + 'PUT /api/mgr/model-fields/ranks' => 'mssetting_save', + 'PUT /api/mgr/model-fields/{id}' => 'mssetting_save', + 'DELETE /api/mgr/model-fields/{id}' => 'mssetting_save', +]; + +$requiredAnyPermissions = [ + 'msorder_list', + 'msorder_view', + 'msorder_save', + 'msproduct_save', + 'mscategory_save', + 'mssetting_list', + 'mssetting_view', + 'mssetting_save', + 'view_document', +]; + +foreach ($registered as $route) { + $pattern = $route['pattern'] ?? ''; + if (!str_starts_with($pattern, '/api/mgr/model-fields')) { + continue; + } + $method = is_array($route['method'] ?? null) + ? implode('|', $route['method']) + : (string) ($route['method'] ?? ''); + $key = strtoupper($method) . ' ' . $pattern; + $middlewares = $route['middlewares'] ?? []; + + $hasAny = false; + $permission = null; + foreach ($middlewares as $middleware) { + if ($middleware instanceof AnyPermissionMiddleware) { + $hasAny = true; + $reflection = new ReflectionProperty(AnyPermissionMiddleware::class, 'permissions'); + $perms = $reflection->getValue($middleware); + sort($perms); + $expectedSorted = $requiredAnyPermissions; + sort($expectedSorted); + $assertSame($expectedSorted, $perms, "{$key}: AnyPermissionMiddleware set"); + } + if ($middleware instanceof PermissionMiddleware) { + $reflection = new ReflectionProperty(PermissionMiddleware::class, 'permission'); + $permission = $reflection->getValue($middleware); + } + } + + if (in_array($key, $expectedAny, true)) { + if (!$hasAny) { + $fail("{$key} must use AnyPermissionMiddleware"); + } + continue; + } + + if (in_array($key, $expectedAuthOnly, true)) { + if ($hasAny || $permission !== null) { + $fail("{$key} must be auth-only (no Permission/AnyPermission beyond Auth)"); + } + // AuthMiddleware is on the outer /api/mgr group — still present on the route. + continue; + } + + if (array_key_exists($key, $expectedWrite)) { + $assertSame($expectedWrite[$key], $permission, "{$key}: write permission"); + continue; + } + + $fail("unexpected model-fields route registered: {$key}"); +} + +foreach ($expectedAny as $routeKey) { + $found = false; + foreach ($registered as $route) { + $pattern = $route['pattern'] ?? ''; + $method = is_array($route['method'] ?? null) + ? implode('|', $route['method']) + : (string) ($route['method'] ?? ''); + if (strtoupper($method) . ' ' . $pattern === $routeKey) { + $found = true; + break; + } + } + if (!$found) { + $fail("missing registered route {$routeKey}"); + } +} + +// Runtime: mgr without form/settings perms cannot hit data-bearing reads +$modx->setPermissions([]); +$assertDenied( + $router, + 'GET', + '/api/mgr/model-fields/combo-options/msOrder/customer_id', + 'mgr-only combo-options field' +); +$assertDenied( + $router, + 'GET', + '/api/mgr/model-fields/combo-options/msOrder', + 'mgr-only combo-options model' +); +$assertDenied( + $router, + 'GET', + '/api/mgr/model-fields/visible/msOrder', + 'mgr-only visible' +); + +// Order manager without mssetting_save can pass the data-read gate (#613) +$modx->setPermissions(['msorder_save']); +$assertAllowedPastPermissionGate( + $router, + 'GET', + '/api/mgr/model-fields/combo-options/msOrder/customer_id', + 'msorder_save combo-options' +); +$assertAllowedPastPermissionGate( + $router, + 'GET', + '/api/mgr/model-fields/visible/msOrder', + 'msorder_save visible' +); + +// Same order manager still cannot mutate schema +$assertDenied( + $router, + 'POST', + '/api/mgr/model-fields', + 'msorder_save cannot POST model-fields' +); + +// Schema list stays reachable without shop permissions (auth-only) +$modx->setPermissions([]); +$assertAllowedPastPermissionGate( + $router, + 'GET', + '/api/mgr/model-fields/models', + 'auth-only models' +); + +fwrite(STDOUT, "OK ModelFieldsRoutePermissionsTest\n"); +exit(0);