refactor(departments-edit): migrate to actionEditContext (step 2)

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) <noreply@anthropic.com>
This commit is contained in:
2026-04-25 23:52:27 +02:00
parent 3207d71244
commit 5378209fed
5 changed files with 408 additions and 78 deletions

View File

@@ -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<string, mixed> $args
* @return array{model: mixed, capabilities: array<string, mixed>, tenantScope: array{scope: 'all'|'list', ids: list<int>}, viewAuth: array{page: array<string, mixed>}}
@@ -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;
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);

View File

@@ -24,41 +24,37 @@ $directoryScopeGateway = app(\MintyPHP\Service\Tenant\TenantScopeService::class)
$uuid = trim((string) ($id ?? ''));
$editTarget = requestPathWithReturnTarget("admin/departments/edit/{$uuid}", $returnTarget);
$department = $uuid !== '' ? $departmentService->findByUuid($uuid) : null;
if (!$department) {
Flash::error('Department not found', $closeTarget, 'department_not_found');
Router::redirect($closeTarget);
}
// 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;
$departmentId = (int) ($department['id'] ?? 0);
$contextDecision = $authorizationService->authorize(DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_EDIT_CONTEXT, [
$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',
]);
if (!$contextDecision->isAllowed()) {
Router::redirect('error/forbidden');
return;
}
$capabilities = $contextDecision->attribute('capabilities', []);
if (!is_array($capabilities)) {
$capabilities = [];
}
$canViewPage = (bool) ($capabilities['can_view_page'] ?? false);
$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);
}
}

View File

@@ -0,0 +1,257 @@
<?php
namespace MintyPHP\Tests\Architecture;
use FilesystemIterator;
use PHPUnit\Framework\TestCase;
use RecursiveDirectoryIterator;
use RecursiveIteratorIterator;
/**
* GR-SEC-001 extension: every page action that calls one of the
* action-context aggregators (actionEditContext, actionCreateContext,
* actionFragmentContext) AND handles POST data MUST also call
* actionRequireCsrf() before it accesses the request body.
*
* Closes plan-stage finding SEC-PLAN-004 from Run
* 2026-04-25-action-context-helper. Replaces the obsolete
* ActionContextHelperContractTest::testNoProductionCallSitesYet which became
* stale once Step 2 introduced the first production caller (Departments-Edit
* pilot, Run 2026-04-25-action-context-rollout-step2-departments).
*
* Order requirement: actionRequireCsrf() must appear BEFORE the first
* POST-body indicator (bodyAll(), bodyInt(), body( etc.). It does NOT need to
* appear before the aggregator call itself — the aggregator only resolves
* model + capabilities for the GET-render and is safe to run before CSRF
* verification, mirroring the canonical Departments-Edit ordering preserved by
* Run 2026-04-25-action-context-rollout-step2-departments
* (Verhaltens-Identitäts-Pflicht).
*
* Detection is conservative: when an aggregator call cannot be statically
* located (e.g. wrapped in a heredoc or dynamic dispatch), the test fails
* explicitly with "Cannot verify CSRF pairing — review manually" rather than
* silently passing. Pattern reference: DetailDrawerFragmentContractTest.
*/
class ActionContextCsrfPairingContractTest extends TestCase
{
use ProjectFileAssertionSupport;
private const AGGREGATORS = [
'actionEditContext',
'actionCreateContext',
'actionFragmentContext',
];
/**
* POST-handling presence indicators (does the file handle POST at all?).
* Mirrors PostEndpointCsrfContractTest::handlesPostData.
*/
private const POST_PRESENCE_INDICATORS = [
"isMethod('POST')",
'requestInput()->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<int>|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<string> $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 <<<TAG
// and we haven't seen the closing tag yet, we'd be inside. Skip — the
// test fails-loud on unverifiable cases.
return false;
}
}

View File

@@ -17,6 +17,12 @@ use ReflectionNamedType;
* This test also asserts that the Tenant-Scope and Fragment-Resolver helpers
* carry their PHPStan array-shape return docblocks. Acceptance check SC-004
* relies on the shape staying stable.
*
* Step 2 (Pilot-Migration Departments-Edit, Run
* 2026-04-25-action-context-rollout-step2-departments) is the first
* production caller. The previous testNoProductionCallSitesYet assertion
* has been removed; the CSRF↔Aggregator pairing is enforced by
* tests/Architecture/ActionContextCsrfPairingContractTest.php instead.
*/
class ActionContextHelperContractTest extends TestCase
{
@@ -190,49 +196,6 @@ class ActionContextHelperContractTest extends TestCase
$this->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.

View File

@@ -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<string, mixed>|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
* ---------------------------------------------------------------- */