From 5378209fed91693b975efb756bd4f42636d64c67 Mon Sep 17 00:00:00 2001 From: fs Date: Sat, 25 Apr 2026 23:52:27 +0200 Subject: [PATCH] refactor(departments-edit): migrate to actionEditContext (step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pilot migration of pages/admin/departments/edit($id).php onto the actionEditContext aggregator introduced in step 1. The CONTEXT-stage vorspiel (lookup → authorize → tenant-scope → can_view_page → viewAuth) collapses into a single declarative call; the POST branch (CSRF → SUBMIT-authorize → can_update gate → service call → PRG) stays callsite-specific as planned. Three deliberate touches beyond a 1:1 lift: * Additive aggregator extension: actionEditContext gains an optional notFoundFlashScopeKey arg so the dedup scope-key 'department_not_found' is preserved without widening the frozen actionResolveModelOrFail building-block signature. Pattern is documented as the forward-compatibility mechanism for future cluster migrations. * t() consistency: the not-found message now flows through t() via the aggregator. To avoid a partial-translation mix, Flash::success calls for 'Department updated' (×2) are also wrapped — German users now see fully translated messages instead of a German/English mix. * Defensive scope consumption: the action now consults $tenantScope['scope'] before falling through to the strict-mode fallback. The Departments policy never emits can_manage_all_tenants today (so behavior is identical), but the action is now resilient to future policies that might. New ActionContextCsrfPairingContractTest enforces actionRequireCsrf() before any POST body access for actions that use the aggregators — preventing CSRF-pairing regressions during the cluster-wide rollout (step 3). The step-1 testNoProductionCallSitesYet guard is removed, since departments-edit is now the first legitimate caller; the new pairing test takes over its protective role with a more substantive guarantee. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/Support/helpers/action_context.php | 35 ++- pages/admin/departments/edit($id).php | 68 ++--- .../ActionContextCsrfPairingContractTest.php | 257 ++++++++++++++++++ .../ActionContextHelperContractTest.php | 49 +--- .../Helpers/ActionContextHelperTest.php | 77 ++++++ 5 files changed, 408 insertions(+), 78 deletions(-) create mode 100644 tests/Architecture/ActionContextCsrfPairingContractTest.php diff --git a/core/Support/helpers/action_context.php b/core/Support/helpers/action_context.php index 246550d..4b76ea6 100644 --- a/core/Support/helpers/action_context.php +++ b/core/Support/helpers/action_context.php @@ -238,6 +238,15 @@ function actionFragmentResolveOrStatus( * Two-Level-Authorize (CONTEXT + SUBMIT, used by Roles/Permissions) is NOT * built in — actions that need it call the building blocks directly. * + * Forward-compatibility pattern: this aggregator's signature uses an assoc + * array $args to permit additive Step 2+ extension (e.g. notFoundFlashScopeKey + * override added with the Departments-Edit pilot in Run + * 2026-04-25-action-context-rollout-step2-departments). Building blocks + * (actionResolveModelOrFail et al.) remain frozen by + * ActionContextHelperContractTest::testBuildingBlocksExistWithStableSignatures; + * caller-specific behavior must be expressed via additive aggregator args, not + * by widening building-block signatures. + * * Required keys in $args: * - finder: callable(string): mixed * - rawId: string @@ -251,6 +260,14 @@ function actionFragmentResolveOrStatus( * - tenantScopeFlagKey: string * - tenantScopeListKey: string * - canViewPageFlagKey: string (default 'can_view_page') + * - notFoundFlashScopeKey: string|null — when provided, the aggregator + * inlines Flash::error(t($notFoundFlashKey), $notFoundRedirectPath, + * $notFoundFlashScopeKey) + Router::redirect instead of delegating to + * actionResolveModelOrFail (which hardcodes scope-key 'not_found'). Use + * this to preserve a domain-specific dedup-scope-key (e.g. + * 'department_not_found') without widening the frozen building-block + * signature. The scope-key is a flash-dedup identifier only — it must + * not contain PII (GR-SEC-002). * * @param array $args * @return array{model: mixed, capabilities: array, tenantScope: array{scope: 'all'|'list', ids: list}, viewAuth: array{page: array}} @@ -268,8 +285,24 @@ function actionEditContext(array $args): array $tenantScopeFlagKey = (string) ($args['tenantScopeFlagKey'] ?? 'can_manage_all_tenants'); $tenantScopeListKey = (string) ($args['tenantScopeListKey'] ?? 'allowed_tenant_ids'); $canViewPageFlagKey = (string) ($args['canViewPageFlagKey'] ?? 'can_view_page'); + $notFoundFlashScopeKey = $args['notFoundFlashScopeKey'] ?? null; + $notFoundFlashScopeKey = is_string($notFoundFlashScopeKey) && $notFoundFlashScopeKey !== '' + ? $notFoundFlashScopeKey + : null; - $model = actionResolveModelOrFail($finder, $rawId, $notFoundFlashKey, $notFoundRedirectPath); + if ($notFoundFlashScopeKey !== null) { + $model = is_callable($finder) ? $finder($rawId) : null; + if ($model === null) { + Flash::error(t($notFoundFlashKey), $notFoundRedirectPath, $notFoundFlashScopeKey); + Router::redirect($notFoundRedirectPath); + // Router::redirect() calls die() in production. Guard for tests + // where executeRedirect=false: caller MUST not rely on this being + // reached — return value mirrors the building-block fallthrough. + $model = null; + } + } else { + $model = actionResolveModelOrFail($finder, $rawId, $notFoundFlashKey, $notFoundRedirectPath); + } $capabilities = actionAuthorizeAndExtractCapabilities($abilityKey, $context, $forbiddenStrategy); $tenantScope = actionDeriveTenantScope($capabilities, $tenantScopeFlagKey, $tenantScopeListKey); actionEnforceCanViewPage($capabilities, $canViewPageFlagKey, $forbiddenStrategy); diff --git a/pages/admin/departments/edit($id).php b/pages/admin/departments/edit($id).php index 064633b..4db9fdb 100644 --- a/pages/admin/departments/edit($id).php +++ b/pages/admin/departments/edit($id).php @@ -24,41 +24,37 @@ $directoryScopeGateway = app(\MintyPHP\Service\Tenant\TenantScopeService::class) $uuid = trim((string) ($id ?? '')); $editTarget = requestPathWithReturnTarget("admin/departments/edit/{$uuid}", $returnTarget); + +// Resolve the department BEFORE the aggregator so the authorize-context can +// carry the real target_department_id (matches today's order: lookup → CONTEXT +// authorize → can_view_page). The aggregator receives a trivial finder that +// returns the already-loaded model so its not-found branch fires only when +// findByUuid returned null. $department = $uuid !== '' ? $departmentService->findByUuid($uuid) : null; -if (!$department) { - Flash::error('Department not found', $closeTarget, 'department_not_found'); - Router::redirect($closeTarget); -} - $departmentId = (int) ($department['id'] ?? 0); -$contextDecision = $authorizationService->authorize(DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_EDIT_CONTEXT, [ - 'actor_user_id' => $currentUserId, - 'target_department_id' => $departmentId, -]); -if (!$contextDecision->isAllowed()) { - Router::redirect('error/forbidden'); - return; -} -$capabilities = $contextDecision->attribute('capabilities', []); -if (!is_array($capabilities)) { - $capabilities = []; -} -$canViewPage = (bool) ($capabilities['can_view_page'] ?? false); +$context = actionEditContext([ + 'finder' => static fn (string $_id): mixed => $department, + 'rawId' => $uuid, + 'notFoundFlashKey' => 'Department not found', + 'notFoundRedirectPath' => $closeTarget, + 'notFoundFlashScopeKey' => 'department_not_found', + 'abilityKey' => DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_EDIT_CONTEXT, + 'context' => [ + 'actor_user_id' => $currentUserId, + 'target_department_id' => $departmentId, + ], + 'viewAuthFlags' => ['can_update_department', 'can_delete_department'], + 'forbiddenStrategy' => 'redirect', +]); +$department = is_array($context['model']) ? $context['model'] : $department; +$capabilities = $context['capabilities']; +$tenantScope = $context['tenantScope']; +$viewAuth = $context['viewAuth']; $canUpdateDepartment = (bool) ($capabilities['can_update_department'] ?? false); $canDeleteDepartment = (bool) ($capabilities['can_delete_department'] ?? false); -$viewAuth['page'] = [ - 'can_update_department' => $canUpdateDepartment, - 'can_delete_department' => $canDeleteDepartment, -]; -$allowedTenantIdsRaw = $capabilities['allowed_tenant_ids'] ?? []; -$allowedTenantIds = is_array($allowedTenantIdsRaw) - ? array_values(array_unique(array_filter(array_map('intval', $allowedTenantIdsRaw), static fn (int $tenantId): bool => $tenantId > 0))) - : []; -if (!$canViewPage) { - Router::redirect('error/forbidden'); - return; -} +$canManageAllTenants = $tenantScope['scope'] === 'all'; +$allowedTenantIds = $tenantScope['ids']; app(\MintyPHP\Service\Audit\AuditMetadataEnricherInterface::class)->enrich($department); @@ -67,7 +63,9 @@ $errors = []; $warnings = []; $form = $department; $tenants = $tenantService->list(); -if ($allowedTenantIds) { +if ($canManageAllTenants) { + // No filtering — actor may pick any tenant. +} elseif ($allowedTenantIds) { $tenants = array_values(array_filter($tenants, static function (array $tenant) use ($allowedTenantIds): bool { $tenantId = (int) ($tenant['id'] ?? 0); return $tenantId > 0 && in_array($tenantId, $allowedTenantIds, true); @@ -110,7 +108,9 @@ if ($request->isMethod('POST')) { } $selectedTenantId = $request->bodyInt('tenant_id'); - if ($allowedTenantIds) { + if ($canManageAllTenants) { + // No restriction — submitted tenant_id passes through. + } elseif ($allowedTenantIds) { $selectedTenantId = in_array($selectedTenantId, $allowedTenantIds, true) ? $selectedTenantId : 0; } elseif ($directoryScopeGateway->isStrict()) { $selectedTenantId = 0; @@ -134,13 +134,13 @@ if ($request->isMethod('POST')) { if ($warnings) { Flash::info(implode(' ', $warnings), $closeTarget, 'department_warning'); } - Flash::success('Department updated', $closeTarget, 'department_updated'); + Flash::success(t('Department updated'), $closeTarget, 'department_updated'); Router::redirect($closeTarget); } else { if ($warnings) { Flash::info(implode(' ', $warnings), $editTarget, 'department_warning'); } - Flash::success('Department updated', $editTarget, 'department_updated'); + Flash::success(t('Department updated'), $editTarget, 'department_updated'); Router::redirect($editTarget); } } diff --git a/tests/Architecture/ActionContextCsrfPairingContractTest.php b/tests/Architecture/ActionContextCsrfPairingContractTest.php new file mode 100644 index 0000000..ed23f2f --- /dev/null +++ b/tests/Architecture/ActionContextCsrfPairingContractTest.php @@ -0,0 +1,257 @@ +method()', + 'hasBody(', + 'bodyAll()', + 'bodyInt(', + 'bodyString(', + ]; + + /** + * Body-access indicators. These must NOT happen before + * actionRequireCsrf(). isMethod('POST')/method() are guards, not body + * accesses, so they're excluded here — guards may legitimately come + * before the CSRF call (in the very same line as actionRequireCsrf). + */ + private const BODY_ACCESS_INDICATORS = [ + 'hasBody(', + 'bodyAll()', + 'bodyInt(', + 'bodyString(', + ]; + + public function testActionsUsingAggregatorsHaveCsrfBeforeAggregator(): void + { + $root = $this->projectRootPath(); + + $missing = []; + $unverifiable = []; + $orderViolations = []; + + foreach (['pages', 'modules'] as $dir) { + $base = $root . '/' . $dir; + if (!is_dir($base)) { + continue; + } + + $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($base, FilesystemIterator::SKIP_DOTS)); + /** @var \SplFileInfo $file */ + foreach ($iterator as $file) { + if (!$file->isFile() || $file->getExtension() !== 'php') { + continue; + } + + $content = (string) file_get_contents($file->getPathname()); + $aggregatorOffsets = $this->locateAggregatorCalls($content); + if ($aggregatorOffsets === []) { + continue; + } + + $relativePath = str_replace($root . '/', '', $file->getPathname()); + + // Non-static-extractable aggregator pattern (heredoc, eval, …). + if ($aggregatorOffsets === false) { + $unverifiable[] = $relativePath; + continue; + } + + $isPostHandler = $this->hasAny($content, self::POST_PRESENCE_INDICATORS) + || preg_match('/->body\(/', $content) === 1; + + // Drawer-fragment allowlist: actionFragmentContext-only callers + // without any POST indicator are GET-only and don't need CSRF. + if (!$isPostHandler) { + continue; + } + + $csrfOffset = $this->locateActionRequireCsrf($content); + if ($csrfOffset === null) { + $missing[] = $relativePath; + continue; + } + + $bodyAccessOffset = $this->locateFirstBodyAccess($content); + if ($bodyAccessOffset !== null && $csrfOffset >= $bodyAccessOffset) { + $orderViolations[] = sprintf( + '%s — actionRequireCsrf() at offset %d but POST body access at offset %d (CSRF must precede body access)', + $relativePath, + $csrfOffset, + $bodyAccessOffset + ); + } + } + } + + sort($missing); + sort($unverifiable); + sort($orderViolations); + + $messages = []; + if ($missing !== []) { + $messages[] = "Aggregator-callers handling POST without actionRequireCsrf():\n " . implode("\n ", $missing); + } + if ($orderViolations !== []) { + $messages[] = "Aggregator-callers with actionRequireCsrf() AFTER body access:\n " . implode("\n ", $orderViolations); + } + if ($unverifiable !== []) { + $messages[] = "Cannot verify CSRF pairing — review manually:\n " . implode("\n ", $unverifiable); + } + + $this->assertSame( + [], + $messages, + "GR-SEC-001 / aggregator pairing violations:\n" . implode("\n", $messages) + ); + } + + /** + * @return list|false list of byte offsets of aggregator calls; + * false when at least one match is wrapped in a + * heredoc/string literal and cannot be statically + * verified. + */ + private function locateAggregatorCalls(string $content): array|false + { + $offsets = []; + foreach (self::AGGREGATORS as $name) { + $needle = $name . '('; + $pos = 0; + while (($found = strpos($content, $needle, $pos)) !== false) { + if ($this->isInsideStringLiteral($content, $found)) { + return false; + } + $offsets[] = $found; + $pos = $found + strlen($needle); + } + } + sort($offsets); + return $offsets; + } + + private function locateFirstBodyAccess(string $content): ?int + { + $earliest = null; + foreach (self::BODY_ACCESS_INDICATORS as $needle) { + $pos = strpos($content, $needle); + if ($pos !== false && ($earliest === null || $pos < $earliest)) { + $earliest = $pos; + } + } + if (preg_match('/->body\(/', $content, $m, PREG_OFFSET_CAPTURE) === 1) { + $pos = (int) $m[0][1]; + if ($earliest === null || $pos < $earliest) { + $earliest = $pos; + } + } + return $earliest; + } + + private function locateActionRequireCsrf(string $content): ?int + { + $pos = strpos($content, 'actionRequireCsrf('); + return $pos === false ? null : $pos; + } + + /** + * @param list $needles + */ + private function hasAny(string $content, array $needles): bool + { + foreach ($needles as $needle) { + if (str_contains($content, $needle)) { + return true; + } + } + return false; + } + + /** + * Heuristic: a match is considered inside a heredoc/string literal when + * the immediately preceding non-whitespace character is a backtick or an + * unescaped quote on the same line. Conservative — favours raising + * "review manually" over silent pass. + */ + private function isInsideStringLiteral(string $content, int $offset): bool + { + if ($offset <= 0) { + return false; + } + // Walk back to start of line. + $lineStart = strrpos(substr($content, 0, $offset), "\n"); + $lineStart = $lineStart === false ? 0 : $lineStart + 1; + $prefix = substr($content, $lineStart, $offset - $lineStart); + + // Count unescaped single/double quotes on the line so far. + $singles = 0; + $doubles = 0; + $len = strlen($prefix); + for ($i = 0; $i < $len; $i++) { + $ch = $prefix[$i]; + if ($ch === '\\') { + $i++; + continue; + } + if ($ch === "'") { + $singles++; + } elseif ($ch === '"') { + $doubles++; + } + } + // Odd counts → currently inside that quote type. + if (($singles % 2) === 1 || ($doubles % 2) === 1) { + return true; + } + // Heredoc detection — simplistic: if the line above ended with <<assertTrue(function_exists('actionFragmentContext')); } - public function testNoProductionCallSitesYet(): void - { - $root = $this->projectRootPath(); - $names = [ - 'actionResolveModelOrFail', - 'actionAuthorizeAndExtractCapabilities', - 'actionDeriveTenantScope', - 'actionEnforceCanViewPage', - 'actionBuildViewAuth', - 'actionFragmentResolveOrStatus', - 'actionEditContext', - 'actionCreateContext', - 'actionFragmentContext', - ]; - - $hits = []; - foreach (['pages', 'modules'] as $dir) { - $base = $root . '/' . $dir; - if (!is_dir($base)) { - continue; - } - $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($base, \FilesystemIterator::SKIP_DOTS)); - /** @var \SplFileInfo $file */ - foreach ($iterator as $file) { - if (!$file->isFile() || $file->getExtension() !== 'php') { - continue; - } - $content = (string) file_get_contents($file->getPathname()); - foreach ($names as $name) { - if (preg_match('/\b' . preg_quote($name, '/') . '\s*\(/', $content)) { - $hits[] = $name . ' in ' . str_replace($root . '/', '', $file->getPathname()); - } - } - } - } - - $this->assertSame( - [], - $hits, - "Step 1 must not introduce production call-sites — defer to Step 2 (Departments-Edit pilot):\n" . implode("\n", $hits) - ); - } - /** * Ensure the `mixed` return type of building blocks is preserved where * documented. Catches accidental tightening that would break Step 2. diff --git a/tests/Support/Helpers/ActionContextHelperTest.php b/tests/Support/Helpers/ActionContextHelperTest.php index 938f95f..87f6f50 100644 --- a/tests/Support/Helpers/ActionContextHelperTest.php +++ b/tests/Support/Helpers/ActionContextHelperTest.php @@ -335,6 +335,83 @@ class ActionContextHelperTest extends TestCase $this->assertStringContainsString('admin/users', $this->capturedRedirect()); } + public function testEditContextWithFlashScopeKeyOverride(): void + { + // Departments-Edit pilot use-case: aggregator must emit a flash with + // the caller-specific scope-key ('department_not_found') instead of + // the building-block default ('not_found') when notFoundFlashScopeKey + // is provided. Required for Step 2 behavior identity (Run + // 2026-04-25-action-context-rollout-step2-departments). + $this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([ + 'capabilities' => ['can_view_page' => true], + ])); + $_SESSION['flash_messages'] = []; + + actionEditContext([ + 'finder' => static fn (): mixed => null, + 'rawId' => 'missing', + 'notFoundFlashKey' => 'Department not found', + 'notFoundRedirectPath' => 'admin/departments', + 'notFoundFlashScopeKey' => 'department_not_found', + 'abilityKey' => 'ABILITY_DEPARTMENTS_EDIT_CONTEXT', + 'context' => [], + 'viewAuthFlags' => [], + ]); + + $this->assertStringContainsString('admin/departments', $this->capturedRedirect()); + + $entry = $this->findFlashEntryByKey('department_not_found'); + $this->assertNotNull($entry, "Expected flash with key 'department_not_found' to be set by aggregator override."); + $this->assertSame('error', $entry['type'] ?? null); + $this->assertSame('admin/departments', $entry['scope'] ?? null); + } + + public function testEditContextWithoutFlashScopeKeyKeepsBuildingBlockDefault(): void + { + // Without notFoundFlashScopeKey, the aggregator delegates to + // actionResolveModelOrFail which hardcodes scope-key 'not_found'. + // This test pins the default-path behavior so a future addition + // doesn't accidentally widen building-block semantics. + $this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([ + 'capabilities' => ['can_view_page' => true], + ])); + $_SESSION['flash_messages'] = []; + + actionEditContext([ + 'finder' => static fn (): mixed => null, + 'rawId' => 'missing', + 'notFoundFlashKey' => 'action.context.model_not_found', + 'notFoundRedirectPath' => 'admin/users', + 'abilityKey' => 'ABILITY_USERS_EDIT_CONTEXT', + 'context' => [], + 'viewAuthFlags' => [], + ]); + + $entry = $this->findFlashEntryByKey('not_found'); + $this->assertNotNull($entry, "Expected default building-block scope-key 'not_found' when notFoundFlashScopeKey is absent."); + } + + /** + * @return array|null + */ + private function findFlashEntryByKey(string $key): ?array + { + /** @var mixed $rawMessages */ + $rawMessages = $_SESSION['flash_messages'] ?? []; + if (!is_array($rawMessages)) { + return null; + } + foreach ($rawMessages as $entry) { + if (!is_array($entry)) { + continue; + } + if (($entry['key'] ?? null) === $key) { + return $entry; + } + } + return null; + } + /* ---------------------------------------------------------------- * Aggregator: actionCreateContext * ---------------------------------------------------------------- */