refactor(actions): introduce action-context helpers (step 1)
Six orthogonal building blocks plus three cluster aggregators in
core/Support/helpers/action_context.php, preparing consolidation of the
~40-60 line vorspiel duplicated across edit/create/view-fragment actions.
Step 1 of a planned 3-step rollout: no production call sites yet —
pages/ and modules/ are untouched. Architecture tests freeze the
building-block signatures and verify drawer-fragment AuthZ parity.
GR-SEC-009 is structurally enforced via the actionDeriveTenantScope
return shape (PHPStan array{scope: 'all'|'list', ids: list<int>});
'all' is unreachable without an explicit can_manage_all_tenants flag.
Aggregator docblocks carry a mandatory CSRF-pairing warning per
GR-SEC-001; actionBuildViewAuth flags the e()-escape obligation per
GR-SEC-010.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
248
tests/Architecture/ActionContextHelperContractTest.php
Normal file
248
tests/Architecture/ActionContextHelperContractTest.php
Normal file
@@ -0,0 +1,248 @@
|
||||
<?php
|
||||
|
||||
namespace MintyPHP\Tests\Architecture;
|
||||
|
||||
use PHPUnit\Framework\TestCase;
|
||||
use ReflectionFunction;
|
||||
use ReflectionNamedType;
|
||||
|
||||
/**
|
||||
* Freezes the API of the 6 orthogonal building blocks in
|
||||
* core/Support/helpers/action_context.php.
|
||||
*
|
||||
* Aggregators (actionEditContext, actionCreateContext, actionFragmentContext)
|
||||
* are intentionally NOT frozen — they may evolve additively in Step 2 of the
|
||||
* rollout.
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
class ActionContextHelperContractTest extends TestCase
|
||||
{
|
||||
use ProjectFileAssertionSupport;
|
||||
|
||||
/**
|
||||
* @return array<int, array{0: string, 1: list<array{0: string, 1?: mixed, 2?: bool}>}>
|
||||
*/
|
||||
public static function buildingBlocks(): array
|
||||
{
|
||||
// [function-name, [[paramName, ?defaultValue, hasDefault], ...]]
|
||||
return [
|
||||
['actionResolveModelOrFail', [
|
||||
['finder', null, false],
|
||||
['rawId', null, false],
|
||||
['notFoundFlashKey', null, false],
|
||||
['redirectPath', null, false],
|
||||
]],
|
||||
['actionAuthorizeAndExtractCapabilities', [
|
||||
['abilityKey', null, false],
|
||||
['context', null, false],
|
||||
['forbiddenStrategy', 'redirect', true],
|
||||
]],
|
||||
['actionDeriveTenantScope', [
|
||||
['capabilities', null, false],
|
||||
['allTenantsFlagKey', 'can_manage_all_tenants', true],
|
||||
['allowedListKey', 'allowed_tenant_ids', true],
|
||||
]],
|
||||
['actionEnforceCanViewPage', [
|
||||
['capabilities', null, false],
|
||||
['flagKey', 'can_view_page', true],
|
||||
['forbiddenStrategy', 'redirect', true],
|
||||
]],
|
||||
['actionBuildViewAuth', [
|
||||
['capabilities', null, false],
|
||||
['whitelistedFlags', null, false],
|
||||
]],
|
||||
['actionFragmentResolveOrStatus', [
|
||||
['finder', null, false],
|
||||
['rawId', null, false],
|
||||
['abilityKey', null, false],
|
||||
['context', null, false],
|
||||
]],
|
||||
];
|
||||
}
|
||||
|
||||
public function testHelperFileExistsAndIsRegistered(): void
|
||||
{
|
||||
$this->assertFileExists($this->projectRootPath() . '/core/Support/helpers/action_context.php');
|
||||
|
||||
$helpersFile = $this->readProjectFile('core/Support/helpers.php');
|
||||
$this->assertStringContainsString(
|
||||
"require __DIR__ . '/helpers/action_context.php';",
|
||||
$helpersFile,
|
||||
'helpers/action_context.php must be required from core/Support/helpers.php'
|
||||
);
|
||||
}
|
||||
|
||||
public function testBuildingBlocksExistWithStableSignatures(): void
|
||||
{
|
||||
foreach (self::buildingBlocks() as [$fnName, $expectedParams]) {
|
||||
$this->assertTrue(
|
||||
function_exists($fnName),
|
||||
"Helper function '{$fnName}' must exist (frozen by Step 1)."
|
||||
);
|
||||
|
||||
$reflection = new ReflectionFunction($fnName);
|
||||
$actualParams = $reflection->getParameters();
|
||||
|
||||
$this->assertSameSize(
|
||||
$expectedParams,
|
||||
$actualParams,
|
||||
"Parameter count for '{$fnName}' has changed."
|
||||
);
|
||||
|
||||
foreach ($expectedParams as $i => [$expectedName, $expectedDefault, $hasDefault]) {
|
||||
$this->assertSame(
|
||||
$expectedName,
|
||||
$actualParams[$i]->getName(),
|
||||
"Parameter #{$i} of '{$fnName}' has been renamed."
|
||||
);
|
||||
$this->assertSame(
|
||||
$hasDefault,
|
||||
$actualParams[$i]->isDefaultValueAvailable(),
|
||||
"Parameter '{$expectedName}' of '{$fnName}' default-availability has changed."
|
||||
);
|
||||
if ($hasDefault) {
|
||||
$this->assertSame(
|
||||
$expectedDefault,
|
||||
$actualParams[$i]->getDefaultValue(),
|
||||
"Parameter '{$expectedName}' of '{$fnName}' default has changed."
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
public function testTenantScopeReturnDocblockIsFrozen(): void
|
||||
{
|
||||
$reflection = new ReflectionFunction('actionDeriveTenantScope');
|
||||
$doc = (string) $reflection->getDocComment();
|
||||
|
||||
$this->assertStringContainsString(
|
||||
"@return array{scope: 'all'|'list', ids: list<int>}",
|
||||
$doc,
|
||||
"actionDeriveTenantScope must declare PHPStan array-shape return for GR-SEC-009."
|
||||
);
|
||||
}
|
||||
|
||||
public function testFragmentResolveReturnDocblockIsFrozen(): void
|
||||
{
|
||||
$reflection = new ReflectionFunction('actionFragmentResolveOrStatus');
|
||||
$doc = (string) $reflection->getDocComment();
|
||||
|
||||
$this->assertMatchesRegularExpression(
|
||||
"/@return array\{status:[^}]*'ok'[^}]*'forbidden'[^}]*'not_found'[^}]*'invalid_id'/",
|
||||
$doc,
|
||||
"actionFragmentResolveOrStatus must declare PHPStan array-shape return covering all 4 status values."
|
||||
);
|
||||
}
|
||||
|
||||
public function testEnforceCanViewPageDocblockHasStrictComparisonNote(): void
|
||||
{
|
||||
$reflection = new ReflectionFunction('actionEnforceCanViewPage');
|
||||
$doc = (string) $reflection->getDocComment();
|
||||
|
||||
$this->assertStringContainsString(
|
||||
'Strict comparison (=== true)',
|
||||
$doc,
|
||||
"actionEnforceCanViewPage docblock must document strict-comparison contract."
|
||||
);
|
||||
$this->assertStringContainsString(
|
||||
'No truthy coercion',
|
||||
$doc,
|
||||
"actionEnforceCanViewPage docblock must document no-truthy-coercion contract."
|
||||
);
|
||||
}
|
||||
|
||||
public function testBuildViewAuthDocblockHasEscapeWarning(): void
|
||||
{
|
||||
$reflection = new ReflectionFunction('actionBuildViewAuth');
|
||||
$doc = (string) $reflection->getDocComment();
|
||||
|
||||
$this->assertStringContainsString(
|
||||
'MUST e()-escape',
|
||||
$doc,
|
||||
"actionBuildViewAuth docblock must require e()-escaping per GR-SEC-010."
|
||||
);
|
||||
}
|
||||
|
||||
public function testAggregatorDocblocksWarnAboutCsrf(): void
|
||||
{
|
||||
$contents = $this->readProjectFile('core/Support/helpers/action_context.php');
|
||||
|
||||
$count = preg_match_all(
|
||||
'/MUST call actionRequireCsrf\(\) BEFORE this aggregator/',
|
||||
$contents
|
||||
);
|
||||
$this->assertSame(
|
||||
3,
|
||||
$count,
|
||||
'Each of the 3 aggregators (actionEditContext, actionCreateContext, actionFragmentContext) must carry the explicit CSRF-warning in its docblock (GR-SEC-001).'
|
||||
);
|
||||
}
|
||||
|
||||
public function testAggregatorsExistButAreNotFrozen(): void
|
||||
{
|
||||
// We assert existence only — signatures may evolve in Step 2.
|
||||
$this->assertTrue(function_exists('actionEditContext'));
|
||||
$this->assertTrue(function_exists('actionCreateContext'));
|
||||
$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.
|
||||
*/
|
||||
public function testResolveModelOrFailReturnsMixed(): void
|
||||
{
|
||||
$reflection = new ReflectionFunction('actionResolveModelOrFail');
|
||||
$returnType = $reflection->getReturnType();
|
||||
$this->assertInstanceOf(ReflectionNamedType::class, $returnType);
|
||||
/** @var ReflectionNamedType $returnType */
|
||||
$this->assertSame('mixed', $returnType->getName());
|
||||
}
|
||||
}
|
||||
@@ -109,6 +109,141 @@ class DetailDrawerFragmentContractTest extends TestCase
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* AuthZ-Parity: a drawer fragment must enforce the same authorization as
|
||||
* its full-page counterpart (CLAUDE.md: "Must enforce auth + scope exactly
|
||||
* like the full-page view"). This test extracts authorize / requireAbility
|
||||
* arguments from each fragment and compares them to the matched counterpart.
|
||||
*
|
||||
* Allowlist of semantic equivalences (verified manually against codebase):
|
||||
* - admin/users/view-fragment ↔ admin/users/edit
|
||||
* Reason: there is no view($id).php under pages/admin/users/, so the
|
||||
* full-page detail action is edit. The fragment uses ABILITY_VIEW;
|
||||
* edit uses ABILITY_EDIT_CONTEXT — semantically the fragment is the
|
||||
* read-only subset of the edit context. Documented anomaly.
|
||||
* - addressbook/view-fragment ↔ addressbook/view
|
||||
* Reason: both call requireAbilityOrForbidden(ABILITY_VIEW).
|
||||
* - helpdesk/ticket-fragment ↔ helpdesk/ticket
|
||||
* Reason: both call requireAbilityOrForbidden(ABILITY_ACCESS).
|
||||
*
|
||||
* The test fails explicitly if an authorize/requireAbility call is nested
|
||||
* inside an if/else block (not statically extractable) — a parity check
|
||||
* cannot be made automatically in that case.
|
||||
*/
|
||||
public function testFragmentAuthzMatchesFullPage(): void
|
||||
{
|
||||
$root = $this->projectRootPath();
|
||||
|
||||
// counterpart map: fragment-relative-path => full-page-relative-path
|
||||
$pairs = [
|
||||
'pages/admin/users/view-fragment($id).php' => 'pages/admin/users/edit($id).php',
|
||||
'modules/addressbook/pages/address-book/view-fragment($id).php' => 'modules/addressbook/pages/address-book/view($id).php',
|
||||
'modules/helpdesk/pages/helpdesk/ticket-fragment($id).php' => 'modules/helpdesk/pages/helpdesk/ticket($id).php',
|
||||
];
|
||||
|
||||
// semantic-equivalence allowlist — full-page ability => fragment ability accepted as parity
|
||||
$equivalents = [
|
||||
'UserAuthorizationPolicy::ABILITY_ADMIN_USERS_EDIT_CONTEXT' => ['UserAuthorizationPolicy::ABILITY_ADMIN_USERS_VIEW'],
|
||||
];
|
||||
|
||||
$violations = [];
|
||||
|
||||
foreach ($pairs as $fragmentRel => $fullPageRel) {
|
||||
$fragmentPath = $root . '/' . $fragmentRel;
|
||||
$fullPagePath = $root . '/' . $fullPageRel;
|
||||
|
||||
$this->assertFileExists($fragmentPath, "Fragment file missing: {$fragmentRel}");
|
||||
$this->assertFileExists($fullPagePath, "Full-page file missing: {$fullPageRel}");
|
||||
|
||||
$fragmentAbility = $this->extractTopLevelAbility((string) file_get_contents($fragmentPath), $fragmentRel, $violations);
|
||||
$fullPageAbility = $this->extractTopLevelAbility((string) file_get_contents($fullPagePath), $fullPageRel, $violations);
|
||||
|
||||
if ($fragmentAbility === null || $fullPageAbility === null) {
|
||||
continue; // already recorded as violation
|
||||
}
|
||||
|
||||
// Strip leading namespace separators for comparison resilience.
|
||||
$fragNorm = ltrim($fragmentAbility, '\\');
|
||||
$fullNorm = ltrim($fullPageAbility, '\\');
|
||||
|
||||
if ($fragNorm === $fullNorm) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$accepted = $equivalents[$fullNorm] ?? [];
|
||||
if (in_array($fragNorm, $accepted, true)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$violations[] = sprintf(
|
||||
"AuthZ parity mismatch: %s requires '%s' but full-page %s requires '%s' (no documented allowlist entry).",
|
||||
$fragmentRel,
|
||||
$fragNorm,
|
||||
$fullPageRel,
|
||||
$fullNorm
|
||||
);
|
||||
}
|
||||
|
||||
$this->assertSame([], $violations, "Drawer-fragment AuthZ-parity violations:\n" . implode("\n", $violations));
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract the first top-level (non-nested) authorize / requireAbility ability
|
||||
* argument from a PHP source string. Returns null and records a violation
|
||||
* when no statically extractable call is found.
|
||||
*/
|
||||
private function extractTopLevelAbility(string $source, string $fileRel, array &$violations): ?string
|
||||
{
|
||||
// Strip block comments and line comments to avoid false matches.
|
||||
$stripped = preg_replace('/\/\*.*?\*\//s', '', $source) ?? $source;
|
||||
$stripped = preg_replace('/\/\/[^\n]*/', '', $stripped) ?? $stripped;
|
||||
|
||||
$lines = explode("\n", $stripped);
|
||||
$depth = 0;
|
||||
$hasTopLevelMatch = false;
|
||||
$hasNestedMatch = false;
|
||||
$extracted = null;
|
||||
|
||||
foreach ($lines as $line) {
|
||||
// Update brace depth AFTER matching this line so a `{` on the same
|
||||
// line as an authorize call (rare) still counts as top-level.
|
||||
$matchPattern = '/(?:Guard::requireAbility(?:OrForbidden|DecisionOrForbidden)?|->authorize|::authorize)\s*\(\s*([A-Za-z_\\\\][A-Za-z0-9_\\\\:]*)/';
|
||||
if (preg_match($matchPattern, $line, $m)) {
|
||||
if ($depth === 0) {
|
||||
if (!$hasTopLevelMatch) {
|
||||
$extracted = $m[1];
|
||||
$hasTopLevelMatch = true;
|
||||
}
|
||||
} else {
|
||||
$hasNestedMatch = true;
|
||||
}
|
||||
}
|
||||
$opens = substr_count($line, '{');
|
||||
$closes = substr_count($line, '}');
|
||||
$depth += $opens - $closes;
|
||||
if ($depth < 0) {
|
||||
$depth = 0;
|
||||
}
|
||||
}
|
||||
|
||||
if (!$hasTopLevelMatch) {
|
||||
if ($hasNestedMatch) {
|
||||
$violations[] = sprintf(
|
||||
"Cannot verify parity when authorization is conditional — review manually: %s",
|
||||
$fileRel
|
||||
);
|
||||
} else {
|
||||
$violations[] = sprintf(
|
||||
"No authorize/requireAbility call found in %s — fragment must enforce auth.",
|
||||
$fileRel
|
||||
);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
return $extracted;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array{0: bool, 1: bool, 2: list<string>} [actionFound, viewFound, searchedDirs]
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user