From 09f96f860c91058009fbe64519dffaa8ab686982 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 00:53:07 +0200 Subject: [PATCH] fix: the CLI must attach the event bus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ModuleBase::initEventHandlers()` is what attaches the four receivers that make the event bus do anything — the log handler, the database handler that writes the Eventlog table the security log reads, the mail alerts and the in-app notifications. Web/Init and Api/Init each call it from their own initialize(). Cli/Init never did. EventDispatcher is a container singleton and notify() is a plain loop over the attached receivers, so with none attached it is a silent no-op for the whole process. Nothing throws, nothing logs that an event was dropped. A master password rotation or a backup run from bin/cli.php therefore left no trace of having happened, while the identical operation through the web or the API was fully recorded — including run.backup.start, the rotation's per-stage events, and the exception events fired when one of them fails. An operator reading the security log to find out what had been done to the instance was reading a log with the CLI's work missing from it. That it is an oversight rather than a decision is visible in the constructor: Cli\Init already takes ProvidersHelper and hands it to ModuleBase, and nothing in the CLI ever used it — initEventHandlers() is its only consumer. It goes before initCli(), which is what runs the command. initEventHandlers() returns early after attaching the log handler when the instance is not installed, so sp:install is unaffected: it still gets file logging and nothing that would need a database. The test builds each module's real container and asserts the dispatcher has the log handler afterwards. For the CLI it calls the real initialize() (with an inert console application, so no command is dispatched); for web and api it invokes the shared initEventHandlers() directly, because reaching it through their initialize() means a real PHP session for one and a fully installed config with a live database for the other. Checked by removing the line: only the CLI test fails, and it fails because the handler is absent. --- src/Infrastructure/Adapter/In/Cli/Init.php | 9 + .../In/EventHandlersAreAttachedTest.php | 242 ++++++++++++++++++ .../Support/Stubs/InertConsoleApplication.php | 49 ++++ 3 files changed, 300 insertions(+) create mode 100644 tests/Integration/Infrastructure/Adapter/In/EventHandlersAreAttachedTest.php create mode 100644 tests/Support/Stubs/InertConsoleApplication.php diff --git a/src/Infrastructure/Adapter/In/Cli/Init.php b/src/Infrastructure/Adapter/In/Cli/Init.php index a2afb55e7..151ad79a9 100644 --- a/src/Infrastructure/Adapter/In/Cli/Init.php +++ b/src/Infrastructure/Adapter/In/Cli/Init.php @@ -86,6 +86,15 @@ public function initialize(string $controller): void // Load language $this->language->setLanguage(); + // Before initCli(), which is what runs the command. Without this the dispatcher has no + // receivers at all, and notify() is a loop over receivers — so every event a CLI command + // fired was silently discarded, including the ones that write the Eventlog table the + // security log reads. A master password rotation or a backup run from bin/cli.php left no + // trace of having happened, while the same operation through the web or the API was fully + // recorded. Web/Init and Api/Init both call this from their own initialize(); this door + // was the one that never had it. + $this->initEventHandlers(); + $this->initCli(); } diff --git a/tests/Integration/Infrastructure/Adapter/In/EventHandlersAreAttachedTest.php b/tests/Integration/Infrastructure/Adapter/In/EventHandlersAreAttachedTest.php new file mode 100644 index 000000000..64ac11ecb --- /dev/null +++ b/tests/Integration/Infrastructure/Adapter/In/EventHandlersAreAttachedTest.php @@ -0,0 +1,242 @@ +. + */ + +namespace SP\Tests\Integration\Infrastructure\Adapter\In; + +use DI\ContainerBuilder; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use ReflectionMethod; +use SP\Domain\Core\Bootstrap\ModuleInterface; +use SP\Domain\Core\Bootstrap\Path; +use SP\Domain\Core\Events\EventDispatcherInterface; +use SP\Domain\File\FileSystem; +use SP\Infrastructure\Definitions\CoreDefinitions; +use SP\Infrastructure\Definitions\DomainDefinitions; +use SP\Infrastructure\ModuleBase; +use SP\Infrastructure\ProvidersHelper; +use SP\Tests\Support\Stubs\InertConsoleApplication; +use Symfony\Component\Console\Application as ConsoleApplication; + +use function DI\create; +use function SP\Tests\getResource; + +/** + * ModuleBase::initEventHandlers() is what attaches the four EventReceivers that make the event + * bus do anything at all: LogHandler always, and -- once the application is installed -- + * DatabaseHandler (the Eventlog table the web UI's security log reads), MailEvent and + * NotificationEvent. Nothing fires unless something is attached to receive it. + * + * Web\Init::initialize() calls it at line 223 and Api\Init::initialize() calls it at line 122. + * Cli\Init::initialize() never called it at all: sp:backup and sp:crypt:update-master-password + * left nothing in the Eventlog table, while the same operations through the web or API were fully + * recorded. tests/Integration/Infrastructure/Adapter/In/Cli/CliTestCase.php -- the harness the + * real end-to-end CLI command tests use -- resolves a Command class straight out of the container + * and runs it with CommandTester, which is exactly what let this slip through: none of those tests + * go anywhere near Cli\Init::initialize(), so nothing was ever in a position to notice the missing + * call. + * + * This builds each module's real container -- DomainDefinitions + CoreDefinitions($module) + that + * module's own module.php, the same way CliTestCase and AccountAccessTest do -- and checks the + * same thing for all three doors, so the invariant is stated once rather than only for the one + * that broke. + * + * The CLI door is exercised through its actual, unmodified initialize(): the only thing swapped + * out is the Console\Application binding, so initCli() does not go on to actually run a command. + * Web and Api are not run through their own initialize() here. Doing that for real would need + * materially more than the CLI does: Web binds Context to a real PHP session, which this project + * only exercises under #[RunClassInSeparateProcess] (see SessionTest) precisely because a real + * session started in the shared integration process is not safe for the tests around it. Api's + * checkInstalled() runs before initEventHandlers(), so reaching it for real needs a config that + * claims to be installed, an app/database version that matches exactly (or checkUpgradeNeeded() + * intervenes first) and live checkDatabaseConnection()/checkDatabaseTables() checks against the + * real schema. Instead, the shared, protected method is invoked directly, by reflection, on a + * real container-built Init -- proving the ProvidersHelper/EventDispatcher wiring those two doors + * already depend on is sound, without paying for either door's full request lifecycle. + */ +#[Group('integration')] +final class EventHandlersAreAttachedTest extends TestCase +{ + private string $root; + private string $configPath; + + protected function setUp(): void + { + parent::setUp(); + + // A real (non-vfs), uniquely-named directory -- vfs is shared process-wide, and every + // test here builds its own container against its own config. + $this->root = FileSystem::buildPath( + sys_get_temp_dir(), + 'syspass-event-handlers-' . bin2hex(random_bytes(6)) + ); + $this->configPath = FileSystem::buildPath($this->root, 'config'); + + foreach ([$this->configPath, $this->cachePath(), $this->tmpPath(), $this->backupPath()] as $dir) { + if (!mkdir($dir, 0777, true) && !is_dir($dir)) { + self::fail(sprintf('Directory "%s" was not created', $dir)); + } + } + + // The same not-installed fixture config CliTestCase/AccountAccessTest use. + // ModuleBase::initEventHandlers() attaches only the LogHandler while the application is + // not installed, which is exactly what makes the LogHandler the one thing safe to assert + // on every door regardless of whether it also reaches DatabaseHandler/MailEvent/ + // NotificationEvent. + file_put_contents( + FileSystem::buildPath($this->configPath, 'config.xml'), + getResource('config', 'config.xml') + ); + } + + protected function tearDown(): void + { + FileSystem::rmdirRecursive($this->root); + + parent::tearDown(); + } + + /** + * The fix: Cli\Init::initialize() attaches the event handlers before initCli() hands control + * to the console application, exactly like the web and API doors already do. + */ + #[Test] + public function theCliEntryPointAttachesTheLogHandlerBeforeRunningAnyCommand(): void + { + $dic = $this->buildContainer('cli'); + + $dic->get(ModuleInterface::class)->initialize(''); + + self::assertHandlerAttached($dic, 'cli'); + } + + /** + * @return array + */ + public static function httpModuleProvider(): array + { + return ['web' => ['web'], 'api' => ['api']]; + } + + /** + * The invariant Cli\Init was missing, stated for the two doors that already had it -- see the + * class docblock for why these two go through initEventHandlers() directly by reflection + * rather than through their own initialize(). + */ + #[Test] + #[DataProvider('httpModuleProvider')] + public function theHttpEntryPointsAlreadyAttachTheLogHandler(string $module): void + { + $dic = $this->buildContainer($module); + $init = $dic->get(ModuleInterface::class); + + (new ReflectionMethod(ModuleBase::class, 'initEventHandlers'))->invoke($init); + + self::assertHandlerAttached($dic, $module); + } + + private static function assertHandlerAttached(ContainerInterface $dic, string $module): void + { + $dispatcher = $dic->get(EventDispatcherInterface::class); + $logHandler = $dic->get(ProvidersHelper::class)->getLogHandler(); + + self::assertTrue( + $dispatcher->has($logHandler), + sprintf( + 'The "%s" module\'s EventDispatcher has no LogHandler attached after ' + . 'initEventHandlers() ran, so nothing that module fires would ever reach the log ' + . 'or, once installed, the Eventlog table.', + $module + ) + ); + } + + private function buildContainer(string $module): ContainerInterface + { + // CoreDefinitions computes the paths when called: point the config (and with it the log + // file) at this test's own directory rather than the working copy's var/ state. + $_ENV['CONFIG_PATH'] = $this->configPath; + + try { + $coreDefinitions = CoreDefinitions::getDefinitions(REAL_APP_ROOT, $module); + } finally { + unset($_ENV['CONFIG_PATH']); + } + + $coreDefinitions['paths'] = array_map( + fn(array $path) => match ($path[0]) { + Path::CACHE => [Path::CACHE, $this->cachePath()], + Path::TMP => [Path::TMP, $this->tmpPath()], + Path::BACKUP => [Path::BACKUP, $this->backupPath()], + default => $path, + }, + $coreDefinitions['paths'] + ); + + $moduleDefinitions = FileSystem::require( + FileSystem::buildPath( + REAL_APP_ROOT, + 'src', + 'Infrastructure', + 'Adapter', + 'In', + ucfirst($module), + 'module.php' + ) + ); + + $builder = new ContainerBuilder(); + $definitions = [DomainDefinitions::getDefinitions(), $coreDefinitions, $moduleDefinitions]; + + if ($module === 'cli') { + // initCli() would otherwise hand off to the real console application and run whatever + // command was on the (empty) command line -- swap in a double whose run() does nothing. + $definitions[] = [ConsoleApplication::class => create(InertConsoleApplication::class)]; + } + + $builder->addDefinitions(...$definitions); + + return $builder->build(); + } + + private function cachePath(): string + { + return FileSystem::buildPath($this->root, 'cache'); + } + + private function tmpPath(): string + { + return FileSystem::buildPath($this->root, 'tmp'); + } + + private function backupPath(): string + { + return FileSystem::buildPath($this->root, 'backup'); + } +} diff --git a/tests/Support/Stubs/InertConsoleApplication.php b/tests/Support/Stubs/InertConsoleApplication.php new file mode 100644 index 000000000..0a8ceff1c --- /dev/null +++ b/tests/Support/Stubs/InertConsoleApplication.php @@ -0,0 +1,49 @@ +. + */ + +declare(strict_types=1); + +namespace SP\Tests\Support\Stubs; + +use Symfony\Component\Console\Application; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +/** + * A Console\Application whose run() does nothing. + * + * SP\Infrastructure\Adapter\In\Cli\Init::initCli() -- called from initialize(), after event + * handlers are attached -- ends by dispatching whatever command the CLI was invoked with. A test + * that wants to exercise initialize() itself, without actually running a command (and whatever + * that command does to the database or filesystem), swaps this in for the real Application via + * the container. run() is not final on the real class, so overriding it here is enough to make + * the rest of Init::initialize() safe to call directly. + */ +final class InertConsoleApplication extends Application +{ + public function run(?InputInterface $input = null, ?OutputInterface $output = null): int + { + return 0; + } +}