From 5d6035ed85e8ec1f2fc4a9542e3efcc1fac8a721 Mon Sep 17 00:00:00 2001 From: Andres Contreras Date: Wed, 9 Sep 2026 19:02:25 -0700 Subject: [PATCH] fix: five defects found by building a real application on 26.09.1 Every one reproduced by a failing test first. - cqrs: CommandProcessingException/QueryProcessingException dropped the cause's errorCode while copying its httpStatus, category and severity, so every domain fault reached the client as COMMAND_PROCESSING_ERROR and callers had nothing to branch on. BREAKING. - container: ContainerRegistrar rebound every #[Component] class unconditionally during boot(), silently discarding a binding the application had registered in register(). The failure surfaced later as an unrelated-looking autowiring error. Explicit bindings now win, matching the bean sweep's own precedence rule. BREAKING. - security/context: strict method security refused the boot of `firefly:cache` itself, so the command its own error message told you to run could never run. AppScan::regenerating() now reports that boot and the gate stands down for it. - context: a stale compiled manifest killed EagerSingletonsPass before `firefly:cache` could replace it, leaving `rm -rf bootstrap/cache/firefly` as the only recovery. Compiled artefacts are ignored while regenerating. - skeleton: `/tests export-ignore` deleted the test scaffold from every scaffolded project, so a fresh `composer create-project` fatalled on a missing Tests\CreatesApplication trait before running one assertion. Release 26.09.2. --- CHANGELOG.md | 61 ++++++++++++++++++ README.md | 6 +- docs/versioning.md | 4 +- .../src/Registrar/ContainerRegistrar.php | 19 ++++++ .../Registrar/ContainerRegistrarTest.php | 47 ++++++++++++++ packages/context/src/Scan/AppScan.php | 37 +++++++++++ packages/context/tests/Scan/AppScanTest.php | 46 ++++++++++++++ .../Exception/CommandProcessingException.php | 14 ++++- .../Exception/QueryProcessingException.php | 10 ++- .../Exception/CqrsExceptionTaxonomyTest.php | 33 +++++++++- packages/kernel/src/Version.php | 2 +- .../security/src/SecurityWiringProvider.php | 56 +++++++++++------ .../tests/Boot/UncachedMethodSecurityTest.php | 61 ++++++++++++++++++ samples/lumen/tests/Web/WalletRestTest.php | 7 ++- skeleton/.gitattributes | 19 +++++- tests/SkeletonScaffoldTest.php | 63 +++++++++++++++++++ 16 files changed, 449 insertions(+), 36 deletions(-) create mode 100644 tests/SkeletonScaffoldTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e0a1aa..d0a3eb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,67 @@ All notable changes to LaraFly are documented here. This project uses CalVer (`YY.MM.Patch`). +## [26.09.2] - 2026-09-09 + +A correctness release found by building a real application on `26.09.1`. Five defects, every one of them +reproduced by a failing test first, and every one of them a case where the framework's behaviour contradicted +what its own documentation and error messages said it did. Two are BREAKING in the sense that an application +can observe the change; both changes are the behaviour that was always intended. + +### BREAKING + +- **`packages/cqrs` — `CommandProcessingException` / `QueryProcessingException` now carry the CAUSE's error + code.** Both wrappers copied a `FireflyException` cause's `httpStatus`, `category` and `severity` — and + then overwrote its `errorCode` with their own `COMMAND_PROCESSING_ERROR` / `QUERY_PROCESSING_ERROR`. Three + quarters of a fault's identity survived the bus and the quarter a client actually branches on did not: a + duplicate came back as `409 COMMAND_PROCESSING_ERROR`, a missing row as `404 COMMAND_PROCESSING_ERROR`, an + authorization denial as `403 COMMAND_PROCESSING_ERROR`. Applications worked around it by catching the + wrapper and re-throwing `getPrevious()` in every controller that dispatched a command. The cause's code is + now copied alongside the other three. **Migration:** if you assert on `COMMAND_PROCESSING_ERROR` for a + fault that has its own code, assert on that code instead — it is the one the cause always declared. A + cause that is not a `FireflyException` still yields the generic code, so genuine internal failures do not + start leaking codes. The Lumen capstone's own security assertion moved from `COMMAND_PROCESSING_ERROR` to + `ACCESS_DENIED` in this release for exactly this reason. + +- **`packages/container` — a component's class key is no longer rebound when the application has already + bound it.** The scan runs in `boot()`, after every provider's `register()`, and `ContainerRegistrar` bound + each `#[Component]` class to an autowiring closure unconditionally — so an application that had + deliberately bound a component, which is the normal way to hand one a value the container cannot autowire + (a string from config, a client built from credentials), silently lost that binding. The loss surfaced + nowhere near its cause: boot succeeded, and the first consumer died with `Unresolvable dependency + resolving [Parameter #0 [ string $x ]]`, which reads like a defect in the component. Explicit + bindings now win, which is the precedence rule the bean sweep already applied where a `#[Bean]` name and a + component name collide. **Migration:** none for the common case. If you relied on the scan replacing a + binding you made yourself, remove the binding. + +### Fixed + +- **`packages/security` + `packages/context` — `php artisan firefly:cache` can now run on an application + that has no manifests yet.** With `firefly.security.method.strict` enabled and no compiled + `security-methods.php`, `SecurityWiringProvider` refused to boot — including for `firefly:cache`, the only + command that writes that file. Its own error message said "Run `php artisan firefly:cache`", and that + command hit the same error: a fresh clone, a cleared cache directory and the first layer of an image build + were all unrecoverable without turning strict mode off by hand. `AppScan::regenerating()` now reports when + `firefly:cache` is the running command, and the strict gate stands down for that boot in favour of the + in-process scan — the same code path that produces the manifest it is about to write. + +- **`packages/context` — a stale compiled manifest no longer bricks the command that would replace it.** + Every artefact under `bootstrap/cache/firefly` is treated as absent while `firefly:cache` is running, so a + `component.php` naming a class that has since stopped being autowirable is ignored rather than eagerly + resolved by `EagerSingletonsPass` before the writer is reached. `EagerSingletonsPass` already tolerated an + entry whose class no longer *exists*; this closes the neighbouring case, where the class exists and the + manifest is simply out of date. The recovery for both is now `firefly:cache` rather than + `rm -rf bootstrap/cache/firefly`. + +- **`skeleton` — `composer create-project firefly/skeleton` ships its test scaffold again.** The skeleton's + `.gitattributes` carried `/tests export-ignore`, which is right for a library and wrong for a project + template: Composer honours it when exporting the package into the new project, so every scaffolded + application arrived with a `phpunit.xml` pointing at `tests`, an `autoload-dev` mapping `Tests\` to + `tests/`, and no `tests/` directory at all. The first `vendor/bin/phpunit` fatalled with + `Trait "Tests\CreatesApplication" not found` before running a single assertion. Nothing in the template is + export-ignored now, and `tests/SkeletonScaffoldTest.php` asserts that every `autoload-dev` path the + skeleton declares is a directory it actually ships. + ## [26.09.1] - 2026-09-03 A correctness release that also grew two surfaces. Several headline features were found not to work at all diff --git a/README.md b/README.md index 3de9c33..068e38b 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ PHP 8.3+ Laravel 13 License: Apache 2.0 - Version: 26.09.1 + Version: 26.09.2 PHPStan: max Code Style: Pint

@@ -65,7 +65,7 @@ [*PyFly by Example*](https://github.com/fireflyframework/fireflyframework-pyfly). It builds **Lumen**, the wallet-and-ledger service in [`samples/lumen/`](samples/lumen/), from an empty directory into a secured, event-driven, actuator-observed microservice, chapter by chapter — every listing drawn from that real project -(it boots and its tests pass against this framework version, `26.09.1`). +(it boots and its tests pass against this framework version, `26.09.2`). The book is **complete and bilingual (English + Spanish)**: a quick start, **fourteen chapters** across four parts — Foundations (DI, config, HTTP), Modelling & Persisting the Domain (repositories, DDD), Coordinating & @@ -332,7 +332,7 @@ and the seam that makes this possible. Nine showcases below, each an accurate snippet lifted straight from `samples/lumen/` (the wallet-and-ledger sample) or the framework itself — no invented API. Every attribute and class shown here compiles against the -shipped `26.09.1` release. +shipped `26.09.2` release. ### Attribute DI — `#[Service]` diff --git a/docs/versioning.md b/docs/versioning.md index 5218685..9c6cae2 100644 --- a/docs/versioning.md +++ b/docs/versioning.md @@ -20,7 +20,7 @@ The single place the current version *is* asserted in code is: // packages/kernel/src/Version.php final class Version { - public const string VERSION = '26.09.1'; + public const string VERSION = '26.09.2'; } ``` @@ -38,7 +38,7 @@ actually cut. ```php use Firefly\Kernel\Version; -echo Version::VERSION; // "26.09.1" +echo Version::VERSION; // "26.09.2" ``` This is the only version string LaraFly itself exposes; there is no runtime version-detection mechanism diff --git a/packages/container/src/Registrar/ContainerRegistrar.php b/packages/container/src/Registrar/ContainerRegistrar.php index 904d275..9db563b 100644 --- a/packages/container/src/Registrar/ContainerRegistrar.php +++ b/packages/container/src/Registrar/ContainerRegistrar.php @@ -62,6 +62,25 @@ private function bindClass(ComponentDescriptor $component): void { $class = $component->class; + /* + | An explicit binding the application made itself WINS over the scan. + | + | The scan runs during boot(), after every provider's register(), and this method used to rebind + | the class key unconditionally to an autowiring closure — so an application that had deliberately + | bound a #[Component], which is the normal way to hand one a value the container cannot autowire + | (a string read from config, a client built from credentials), silently lost that binding. The + | loss surfaced nowhere near its cause: boot succeeded and the first consumer died with + | `Unresolvable dependency resolving [Parameter #0 [ string $x ]]`, which reads like a + | defect in the component rather than a binding that was discarded. + | + | This is the precedence rule registerBeans() already applies where a #[Bean] name and a component + | name collide: what was declared explicitly wins. Re-registering the same manifest stays + | idempotent, because the second pass now finds the first pass's own binding and leaves it alone. + */ + if ($this->container->bound($class)) { + return; + } + match ($component->scope) { Scope::Singleton => $this->container->singleton($class, $class), Scope::Transient => $this->container->bind($class, $class), diff --git a/packages/container/tests/Registrar/ContainerRegistrarTest.php b/packages/container/tests/Registrar/ContainerRegistrarTest.php index 97eb960..97b2187 100644 --- a/packages/container/tests/Registrar/ContainerRegistrarTest.php +++ b/packages/container/tests/Registrar/ContainerRegistrarTest.php @@ -198,3 +198,50 @@ interfaces: [Greeter::class], expect($tagged)->toHaveCount(3); }); + +/* + | An explicit binding the application made itself WINS over the scan. + | + | The scan runs during boot(), after every provider's register(), and used to rebind each component's + | class key unconditionally to an autowiring closure. So an application that had deliberately bound a + | #[Component] — the normal way to hand a component a value the container cannot autowire, such as a + | string read from config — silently lost that binding. Worse, the loss surfaced nowhere near its cause: + | boot succeeded, and the first thing to ask for the class died with `Unresolvable dependency resolving + | [Parameter #0 [ string $producer ]]`, which reads like a defect in the component rather than + | a binding that was thrown away. + | + | This is the same precedence rule the bean sweep already applies where a #[Bean] name and a component + | name collide: what was declared explicitly wins. + */ +it('does not overwrite a binding the application registered for a component class', function () { + $illuminate = new IlluminateContainer; + $configured = new WithRequiredScalar('dw-control-plane@26.09.2'); + $illuminate->instance(WithRequiredScalar::class, $configured); + + (new ContainerRegistrar($illuminate))->register(new ComponentManifest([ + new ComponentDescriptor(WithRequiredScalar::class, 'component', null, Scope::Singleton, false, 0, null, [], []), + ])); + + expect($illuminate->make(WithRequiredScalar::class))->toBe($configured) + ->and($illuminate->make(WithRequiredScalar::class)->producer)->toBe('dw-control-plane@26.09.2'); +}); + +it('still binds a component class the application has not bound itself', function () { + $illuminate = new IlluminateContainer; + + (new ContainerRegistrar($illuminate))->register(new ComponentManifest([ + new ComponentDescriptor(SoloBeeper::class, 'component', null, Scope::Singleton, false, 0, null, [], []), + ])); + + expect($illuminate->bound(SoloBeeper::class))->toBeTrue() + ->and($illuminate->make(SoloBeeper::class))->toBeInstanceOf(SoloBeeper::class); +}); + +/** + * A component the container cannot autowire on its own: `string $producer` has no type to resolve. + * Declared here rather than under tests/Fixtures/ so the shared ComponentScanner never picks it up. + */ +final class WithRequiredScalar +{ + public function __construct(public readonly string $producer) {} +} diff --git a/packages/context/src/Scan/AppScan.php b/packages/context/src/Scan/AppScan.php index 156f971..cf32cc7 100644 --- a/packages/context/src/Scan/AppScan.php +++ b/packages/context/src/Scan/AppScan.php @@ -36,6 +36,9 @@ */ final class AppScan { + /** The console command that regenerates every artefact below; see self::regenerating(). */ + public const string REGENERATE_COMMAND = 'firefly:cache'; + public const string COMPONENT = 'component.php'; public const string CONTEXT = 'context.php'; @@ -95,11 +98,45 @@ public static function paths(Container $app): array */ public static function cachedFile(Container $app, string $basename): ?string { + if (self::regenerating()) { + return null; + } + $path = self::dir($app).'/'.$basename; return is_file($path) ? $path : null; } + /** + * True while `php artisan firefly:cache` is the command being run. + * + * The command that WRITES the manifests can only run by booting the very application whose manifests + * it is about to replace, so a boot that trusts what is already on disk makes the command unable to + * repair it: a `component.php` naming a class that no longer autowires kills EagerSingletonsPass + * before the writer is reached, and the only recovery is deleting the cache directory by hand. The + * same applies to strict method security, which refuses to boot without the manifest that this + * command is on its way to produce. + * + * While regenerating, every compiled artefact is therefore treated as absent and each capability + * falls back to its in-process scan — which is exactly the code path that produces the new manifest. + * + * `$_SERVER['argv']` is read rather than `Application::runningConsoleCommand()` because that helper + * inspects `argv[1]` only, so a global option before the command name (`artisan --no-ansi + * firefly:cache`) would defeat it, and because AppScan is handed a bare Container in tests and in + * Lumen, where the helper does not exist at all. + */ + public static function regenerating(): bool + { + if (PHP_SAPI !== 'cli' && PHP_SAPI !== 'phpdbg') { + return false; + } + + /** @var mixed $argv */ + $argv = $_SERVER['argv'] ?? null; + + return is_array($argv) && in_array(self::REGENERATE_COMMAND, $argv, true); + } + public static function dir(Container $app): string { $configured = self::config($app)->get('firefly.cache.path'); diff --git a/packages/context/tests/Scan/AppScanTest.php b/packages/context/tests/Scan/AppScanTest.php index c0ddcff..31f3351 100644 --- a/packages/context/tests/Scan/AppScanTest.php +++ b/packages/context/tests/Scan/AppScanTest.php @@ -80,3 +80,49 @@ function appScanContainer(array $firefly = []): Container expect($source)->toContain("'{$basename}'"); } }); + +/* + | `firefly:cache` is the command that WRITES the manifests, and it can only run by booting the very + | application whose manifests it is about to replace. Trusting the manifests already on disk during that + | boot makes the command unable to fix the thing it exists to fix: a component.php naming a class that + | can no longer be autowired kills EagerSingletonsPass before the writer runs, and the only recovery is + | deleting the cache directory by hand. While regenerating, the compiled artefacts are therefore ignored + | and every capability falls back to its in-process scan — which is what produces the correct manifest. + */ +it('reports that it is regenerating only while firefly:cache is the running command', function (array $argv, bool $expected) { + $original = $_SERVER['argv'] ?? null; + $_SERVER['argv'] = $argv; + + try { + expect(AppScan::regenerating())->toBe($expected); + } finally { + $original === null ? array_key_exists('argv', $_SERVER) && ($_SERVER['argv'] = []) : $_SERVER['argv'] = $original; + } +})->with([ + 'firefly:cache' => [['artisan', 'firefly:cache'], true], + 'with options first' => [['artisan', '--no-ansi', 'firefly:cache'], true], + 'another command' => [['artisan', 'migrate'], false], + 'a lookalike' => [['artisan', 'firefly:cache-clear'], false], + 'no command' => [['artisan'], false], +]); + +it('ignores a compiled artefact that exists while firefly:cache is regenerating', function () { + $dir = sys_get_temp_dir().'/firefly-appscan-'.bin2hex(random_bytes(4)); + mkdir($dir, 0777, true); + file_put_contents($dir.'/'.AppScan::COMPONENT, ' ['path' => $dir]]); + $original = $_SERVER['argv'] ?? []; + + try { + $_SERVER['argv'] = ['artisan', 'migrate']; + expect(AppScan::cachedFile($container, AppScan::COMPONENT))->toBe($dir.'/'.AppScan::COMPONENT); + + $_SERVER['argv'] = ['artisan', 'firefly:cache']; + expect(AppScan::cachedFile($container, AppScan::COMPONENT))->toBeNull(); + } finally { + $_SERVER['argv'] = $original; + @unlink($dir.'/'.AppScan::COMPONENT); + @rmdir($dir); + } +}); diff --git a/packages/cqrs/src/Exception/CommandProcessingException.php b/packages/cqrs/src/Exception/CommandProcessingException.php index 7d82a6c..9e30f54 100644 --- a/packages/cqrs/src/Exception/CommandProcessingException.php +++ b/packages/cqrs/src/Exception/CommandProcessingException.php @@ -11,8 +11,8 @@ /** * The wrapper DefaultCommandBus re-throws any handler/stage throwable in (unless it is already a - * CommandProcessingException, in which case the bus re-throws as-is). CATEGORY-PRESERVING: when the cause is a - * FireflyException, its httpStatus/category/severity are copied so an expected client/domain fault (validation, + * CommandProcessingException, in which case the bus re-throws as-is). IDENTITY-PRESERVING: when the cause is a + * FireflyException, its errorCode/httpStatus/category/severity are copied so an expected client/domain fault (validation, * not-found) keeps its own kernel category and is NOT masked into a generic 500; a plain Throwable becomes * internal/500. Carries the command class + the cause as `previous` (design §2.3 / pyfly command/bus.py:153-164). */ @@ -22,7 +22,15 @@ public function __construct(string $commandClass, Throwable $cause) { parent::__construct( "Processing command [{$commandClass}] failed: {$cause->getMessage()}", - 'COMMAND_PROCESSING_ERROR', + /* + | The cause's OWN code, not a generic one. The error code is part of a fault's identity in + | exactly the way its status, category and severity are, and copying three of the four left + | every domain failure indistinguishable on the wire — a duplicate as `409 COMMAND_PROCESSING_ERROR`, + | a missing row as `404 COMMAND_PROCESSING_ERROR` — so callers had nothing to branch on and + | worked around it by rethrowing getPrevious() in every controller. A cause that is not a + | FireflyException has no code of its own and still yields the generic one below. + */ + $cause instanceof FireflyException ? $cause->errorCode() : 'COMMAND_PROCESSING_ERROR', $cause instanceof FireflyException ? $cause->httpStatus() : 500, $cause instanceof FireflyException ? $cause->category() : ErrorCategory::Internal, $cause instanceof FireflyException ? $cause->severity() : ErrorSeverity::Error, diff --git a/packages/cqrs/src/Exception/QueryProcessingException.php b/packages/cqrs/src/Exception/QueryProcessingException.php index ae0b932..6e930c4 100644 --- a/packages/cqrs/src/Exception/QueryProcessingException.php +++ b/packages/cqrs/src/Exception/QueryProcessingException.php @@ -19,7 +19,15 @@ public function __construct(string $queryClass, Throwable $cause) { parent::__construct( "Processing query [{$queryClass}] failed: {$cause->getMessage()}", - 'QUERY_PROCESSING_ERROR', + /* + | The cause's OWN code, not a generic one. The error code is part of a fault's identity in + | exactly the way its status, category and severity are, and copying three of the four left + | every domain failure indistinguishable on the wire — a duplicate as `409 QUERY_PROCESSING_ERROR`, + | a missing row as `404 QUERY_PROCESSING_ERROR` — so callers had nothing to branch on and + | worked around it by rethrowing getPrevious() in every controller. A cause that is not a + | FireflyException has no code of its own and still yields the generic one below. + */ + $cause instanceof FireflyException ? $cause->errorCode() : 'QUERY_PROCESSING_ERROR', $cause instanceof FireflyException ? $cause->httpStatus() : 500, $cause instanceof FireflyException ? $cause->category() : ErrorCategory::Internal, $cause instanceof FireflyException ? $cause->severity() : ErrorSeverity::Error, diff --git a/packages/cqrs/tests/Exception/CqrsExceptionTaxonomyTest.php b/packages/cqrs/tests/Exception/CqrsExceptionTaxonomyTest.php index ef94adf..ab7c724 100644 --- a/packages/cqrs/tests/Exception/CqrsExceptionTaxonomyTest.php +++ b/packages/cqrs/tests/Exception/CqrsExceptionTaxonomyTest.php @@ -9,6 +9,7 @@ use Firefly\Cqrs\Exception\QueryProcessingException; use Firefly\Kernel\Error\ErrorCategory; use Firefly\Kernel\Error\ErrorSeverity; +use Firefly\Kernel\Exception\Business\ResourceNotFoundException; use Firefly\Kernel\Exception\Business\ValidationException; use Firefly\Kernel\Exception\FireflyException; @@ -30,12 +31,12 @@ ->and($config->severity())->toBe(ErrorSeverity::Critical); }); -it('preserves the cause category/severity/httpStatus when wrapping (does not mask to 500)', function () { +it('preserves the cause code/category/severity/httpStatus when wrapping (does not mask to 500)', function () { $cause = new ValidationException('bad input'); $wrapped = new CommandProcessingException('App\CreateOrder', $cause); expect($wrapped)->toBeInstanceOf(CqrsException::class) - ->and($wrapped->errorCode())->toBe('COMMAND_PROCESSING_ERROR') + ->and($wrapped->errorCode())->toBe($cause->errorCode()) // VALIDATION_ERROR, NOT COMMAND_PROCESSING_ERROR ->and($wrapped->httpStatus())->toBe($cause->httpStatus()) // 422, NOT 500 ->and($wrapped->category())->toBe($cause->category()) ->and($wrapped->severity())->toBe($cause->severity()) @@ -53,3 +54,31 @@ ->and($wrapped->errorCode())->toBe('QUERY_PROCESSING_ERROR') ->and($wrapped->getPrevious())->toBe($cause); // the cause chain is preserved }); + +/* + | The error CODE is part of the fault's identity, exactly as its status, category and severity are. + | + | Copying three of the four left every domain failure indistinguishable on the wire: a duplicate came + | back as `409 COMMAND_PROCESSING_ERROR`, a missing row as `404 COMMAND_PROCESSING_ERROR`, and a caller + | had no way to branch on which had happened. Applications worked around it by catching the wrapper and + | rethrowing `getPrevious()` in every controller that dispatched a command. + */ +it('carries the cause error code through both wrappers', function (string $class, string $subject) { + $cause = new ResourceNotFoundException('no such room', 'ROOM_NOT_FOUND'); + /** @var CqrsException $wrapped */ + $wrapped = new $class($subject, $cause); + + expect($wrapped->errorCode())->toBe('ROOM_NOT_FOUND') + ->and($wrapped->httpStatus())->toBe(404) + ->and($wrapped->getPrevious())->toBe($cause); +})->with([ + 'command' => [CommandProcessingException::class, 'App\IngestMessage'], + 'query' => [QueryProcessingException::class, 'App\FindRoom'], +]); + +it('keeps its own generic code when the cause is not a FireflyException', function () { + expect((new CommandProcessingException('App\CreateOrder', new RuntimeException('boom')))->errorCode()) + ->toBe('COMMAND_PROCESSING_ERROR') + ->and((new QueryProcessingException('App\FindOrder', new RuntimeException('boom')))->errorCode()) + ->toBe('QUERY_PROCESSING_ERROR'); +}); diff --git a/packages/kernel/src/Version.php b/packages/kernel/src/Version.php index f4fe33b..062121f 100644 --- a/packages/kernel/src/Version.php +++ b/packages/kernel/src/Version.php @@ -15,5 +15,5 @@ */ final class Version { - public const string VERSION = '26.09.1'; + public const string VERSION = '26.09.2'; } diff --git a/packages/security/src/SecurityWiringProvider.php b/packages/security/src/SecurityWiringProvider.php index 4650aef..d1585e4 100644 --- a/packages/security/src/SecurityWiringProvider.php +++ b/packages/security/src/SecurityWiringProvider.php @@ -39,32 +39,50 @@ final class SecurityWiringProvider extends FireflyServiceProvider public function register(): void { if (! $this->app->bound(SecurityMethodManifest::class)) { - $this->app->singleton(SecurityMethodManifest::class, static function (Container $app): SecurityMethodManifest { - $file = AppScan::cachedFile($app, AppScan::SECURITY_METHODS); + $this->app->singleton( + SecurityMethodManifest::class, + static fn (Container $app): SecurityMethodManifest => self::methodManifest($app), + ); + } - /** @var Repository $repository */ - $repository = $app->get('config'); - $strict = (new Config($repository))->bool('firefly.security.method.strict', false); + parent::register(); + } - if ($file !== null) { - return SecurityMethodManifest::load($file); - } + /** + * The method-security manifest for this boot: the compiled artefact when there is one, an in-process + * scan when there is not, and a refusal when strict mode says a missing artefact is a build error. + * + * The one boot strict mode must NOT refuse is `php artisan firefly:cache` itself. That command is what + * writes security-methods.php, and it can only run by booting the application — so with strict mode on + * and no manifest yet (a fresh clone, a cleared cache directory, the first layer of an image build) the + * refusal made its own remedy unrunnable: the error said "Run `php artisan firefly:cache`" and that + * command hit the same error. While regenerating, the in-process scan is taken instead. That is not a + * hole in the guard: the process is a developer's or a build's own invocation, it serves no request, + * and the manifest it goes on to write is exactly what every later boot enforces strictly. + */ + public static function methodManifest(Container $app): SecurityMethodManifest + { + $file = AppScan::cachedFile($app, AppScan::SECURITY_METHODS); - if ($strict) { - throw new ConfigurationException( - 'Refusing to boot: firefly.security.method.strict is enabled but no compiled method-security ' - .'manifest was found at '.AppScan::dir($app).'/'.AppScan::SECURITY_METHODS.'. Run `php artisan ' - .'firefly:cache`, or disable strict mode to allow the in-process scan fallback.' - ); - } + /** @var Repository $repository */ + $repository = $app->get('config'); + $strict = (new Config($repository))->bool('firefly.security.method.strict', false); - $paths = AppScan::paths($app); + if ($file !== null) { + return SecurityMethodManifest::load($file); + } - return new SecurityMethodManifest($paths === [] ? [] : (new MethodSecurityScanner)->scan($paths)); - }); + if ($strict && ! AppScan::regenerating()) { + throw new ConfigurationException( + 'Refusing to boot: firefly.security.method.strict is enabled but no compiled method-security ' + .'manifest was found at '.AppScan::dir($app).'/'.AppScan::SECURITY_METHODS.'. Run `php artisan ' + .'firefly:cache`, or disable strict mode to allow the in-process scan fallback.' + ); } - parent::register(); + $paths = AppScan::paths($app); + + return new SecurityMethodManifest($paths === [] ? [] : (new MethodSecurityScanner)->scan($paths)); } /** diff --git a/packages/security/tests/Boot/UncachedMethodSecurityTest.php b/packages/security/tests/Boot/UncachedMethodSecurityTest.php index 7f90a61..57951ce 100644 --- a/packages/security/tests/Boot/UncachedMethodSecurityTest.php +++ b/packages/security/tests/Boot/UncachedMethodSecurityTest.php @@ -7,6 +7,7 @@ use Firefly\Kernel\Exception\Framework\ConfigurationException; use Firefly\Security\Access\Method\SecurityMethodManifest; use Firefly\Security\Scanner\MethodSecurityScanner; +use Firefly\Security\SecurityWiringProvider; use Illuminate\Config\Repository; use Illuminate\Container\Container; @@ -71,3 +72,63 @@ function securityScanPaths(): array expect(static fn () => $resolve($app))->toThrow(ConfigurationException::class); }); + +/* + | …but it must not fail closed against the command that produces the manifest. + | + | `firefly:cache` is the only thing that writes security-methods.php, and it can only run by booting the + | application. With strict mode on and no manifest yet — a fresh clone, a cleared cache directory, or the + | very first build of an image — the boot refused, so the command could never write the file it was being + | told to run. The instruction in the error message ("Run `php artisan firefly:cache`") was unfollowable. + | + | While regenerating, the provider therefore takes the same in-process scan the non-strict path takes. It + | is not a hole: the process is a developer's or an image build's own `firefly:cache` invocation, it + | serves no request, and the manifest it writes is what every later boot enforces strictly. + */ +it('lets the boot that regenerates the manifest through, even under strict mode', function () { + $app = new Container; + $app->instance('config', new Repository([ + 'firefly' => [ + 'cache' => ['path' => sys_get_temp_dir().'/firefly-definitely-not-here-'.bin2hex(random_bytes(6))], + 'security' => ['method' => ['strict' => true]], + 'scan' => ['paths' => securityScanPaths()], + ], + ])); + + $original = $_SERVER['argv'] ?? []; + $_SERVER['argv'] = ['artisan', 'firefly:cache']; + + try { + $manifest = SecurityWiringProvider::methodManifest($app); + + expect($manifest)->toBeInstanceOf(SecurityMethodManifest::class); + + // and it is the REAL scan, not an empty (permissive) stand-in + $rules = (new MethodSecurityScanner)->scan(securityScanPaths()); + $first = $rules[0]; + expect($manifest->ruleFor($first->class, $first->method))->not->toBeNull(); + } finally { + $_SERVER['argv'] = $original; + } +}); + +it('still refuses a normal boot under strict mode with no manifest', function () { + $app = new Container; + $app->instance('config', new Repository([ + 'firefly' => [ + 'cache' => ['path' => sys_get_temp_dir().'/firefly-definitely-not-here-'.bin2hex(random_bytes(6))], + 'security' => ['method' => ['strict' => true]], + 'scan' => ['paths' => securityScanPaths()], + ], + ])); + + $original = $_SERVER['argv'] ?? []; + $_SERVER['argv'] = ['artisan', 'serve']; + + try { + expect(fn () => SecurityWiringProvider::methodManifest($app)) + ->toThrow(ConfigurationException::class); + } finally { + $_SERVER['argv'] = $original; + } +}); diff --git a/samples/lumen/tests/Web/WalletRestTest.php b/samples/lumen/tests/Web/WalletRestTest.php index c9ff794..7f0e13c 100644 --- a/samples/lumen/tests/Web/WalletRestTest.php +++ b/samples/lumen/tests/Web/WalletRestTest.php @@ -84,8 +84,9 @@ // SecurityCommandAuthorizer at the bus. LumenTestCase's HTTP security filters (jwt/http/csrf) are OFF, so a // plain HTTP POST carries NO principal -> SecurityContextHolder::getContext() is anonymous -> the bus denies // the command -> AuthorizationException, wrapped as CommandProcessingException (which copies the cause's - // httpStatus/category but keeps its own COMMAND_PROCESSING_ERROR code) -> problem-details renders 403. This - // is a genuine teaching point, not a workaround: the endpoint really is guarded. + // errorCode/httpStatus/category, so the wire says ACCESS_DENIED rather than the bus's own generic code) + // -> problem-details renders 403. This is a genuine teaching point, not a workaround: the endpoint really + // is guarded, and a client can tell WHY it was refused. /** @var string $id */ $id = $this->postJson('/api/v1/wallets', ['owner_id' => 'owner-3', 'currency' => 'EUR'])->json('wallet_id'); $this->postJson("/api/v1/wallets/{$id}/deposit", ['amount_minor' => 5000]); @@ -94,7 +95,7 @@ ->assertStatus(403) ->assertHeader('Content-Type', 'application/problem+json') ->assertJsonPath('status', 403) - ->assertJsonPath('code', 'COMMAND_PROCESSING_ERROR') + ->assertJsonPath('code', 'ACCESS_DENIED') ->assertJsonPath('category', 'security'); // Proof the denial happened BEFORE the handler touched the balance. diff --git a/skeleton/.gitattributes b/skeleton/.gitattributes index 538b69a..a53565f 100644 --- a/skeleton/.gitattributes +++ b/skeleton/.gitattributes @@ -1,2 +1,17 @@ -/tests export-ignore -/.gitattributes export-ignore +# The skeleton is a PROJECT TEMPLATE, not a library. +# +# `export-ignore` is how a library keeps its own tests out of a consumer's vendor directory, and Composer +# honours it when exporting this package into a newly created project. Applied here it deleted the very +# files the template exists to hand over: `/tests export-ignore` produced projects with a phpunit.xml +# pointing at `tests`, an autoload-dev mapping `Tests\` to `tests/`, and no `tests/` directory — so the +# first `vendor/bin/phpunit` fatalled on the missing `Tests\CreatesApplication` trait. Nothing in this +# template is export-ignored; everything here is meant to arrive in the new project. + +* text=auto eol=lf + +*.php text diff=php +*.md text diff=markdown +*.json text +*.yml text +*.yaml text +*.sh text eol=lf diff --git a/tests/SkeletonScaffoldTest.php b/tests/SkeletonScaffoldTest.php new file mode 100644 index 0000000..6223f61 --- /dev/null +++ b/tests/SkeletonScaffoldTest.php @@ -0,0 +1,63 @@ +toBeTrue(); // nothing is excluded at all, which is also correct + + return; + } + + $ignored = []; + + foreach (file($gitattributes, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES) ?: [] as $line) { + if (str_starts_with(trim($line), '#') || ! str_contains($line, 'export-ignore')) { + continue; + } + + $fields = preg_split('/\s+/', trim($line)); + $ignored[] = is_array($fields) && $fields !== [] ? trim((string) $fields[0]) : ''; + } + + expect($ignored)->not->toContain('/tests') + ->and($ignored)->not->toContain('tests') + ->and($ignored)->not->toContain('/tests/'); +}); + +it('ships the test scaffold its own phpunit.xml and autoload-dev depend on', function () use ($skeleton) { + $decoded = json_decode((string) file_get_contents($skeleton.'/composer.json'), true, 512, JSON_THROW_ON_ERROR); + + /** @var array $composer */ + $composer = is_array($decoded) ? $decoded : []; + + /** @var array $autoloadDev */ + $autoloadDev = is_array($composer['autoload-dev'] ?? null) ? $composer['autoload-dev'] : []; + + /** @var array $psr4 */ + $psr4 = is_array($autoloadDev['psr-4'] ?? null) ? $autoloadDev['psr-4'] : []; + + foreach ($psr4 as $prefix => $dir) { + expect(is_dir($skeleton.'/'.rtrim($dir, '/'))) + ->toBeTrue("autoload-dev maps {$prefix} to {$dir}, which the skeleton does not ship"); + } + + // The two files Laravel's own base test case needs; without either, every test in a fresh project fatals. + expect(is_file($skeleton.'/tests/TestCase.php'))->toBeTrue() + ->and(is_file($skeleton.'/tests/CreatesApplication.php'))->toBeTrue(); +});