diff --git a/src/Infrastructure/Log/Providers/LoggerBase.php b/src/Infrastructure/Log/Providers/LoggerBase.php index d410f9967..ff76d4a6c 100644 --- a/src/Infrastructure/Log/Providers/LoggerBase.php +++ b/src/Infrastructure/Log/Providers/LoggerBase.php @@ -27,6 +27,7 @@ namespace SP\Infrastructure\Log\Providers; use Exception; +use Throwable; use Psr\Log\LoggerInterface; use SP\Application\Application; use SP\Domain\Core\Events\Event; @@ -37,6 +38,7 @@ use SP\Domain\Core\LanguageInterface; use SP\Domain\Http\Ports\RequestService; +use function SP\processException; use function SP\__; use function SP\getLastCaller; @@ -78,6 +80,37 @@ public function update(Event $event): void { $this->language->setAppLocales(); + try { + $this->writeEvent($event); + } catch (Throwable $e) { + // A log that cannot be written must not fail the thing it was reporting on. + // + // This receiver is attached on every request, before the install check and regardless + // of any config flag, and it was the only one of the four with no guard — + // `DatabaseHandler`, `MailEvent` and `NotificationEvent` all catch and hand to + // `processException()`. Monolog's `StreamHandler` throws when `var/syspass.log` cannot + // be opened or appended to, and `notify()` is always called *after* the work it + // describes, so a full disk turned a completed operation into an error response: the + // administrator is told a master-password rotation failed when it had already + // finished, which is the one thing that must never be ambiguous. + // + // `processException()` is safe to call from here: `logger()` writes with a suppressed + // `file_put_contents()` and falls back to `error_log()`, so it does not come back + // through Monolog. + // + // `Throwable` rather than the siblings' `Exception`, because a stream failure can + // surface as an `Error`; `processException()` accepts either. + processException($e); + } finally { + $this->language->unsetAppLocales(); + } + } + + /** + * @throws InvalidClassException + */ + private function writeEvent(Event $event): void + { $eventName = $event->getName(); $userLogin = 'N/A'; @@ -115,8 +148,6 @@ public function update(Event $event): void ) ); } - - $this->language->unsetAppLocales(); } /** diff --git a/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php b/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php index e362829cd..d0198b5c4 100644 --- a/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php +++ b/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php @@ -131,6 +131,33 @@ public function testUpdateWithNoMessage() $this->logHandler->update($event); } + /** + * A log that cannot be written does not fail the thing it was reporting on. + * + * This receiver is attached on every request — before the install check, and regardless of any + * config flag — and it was the only one of the four with no guard around `update()`: + * `DatabaseHandler`, `MailEvent` and `NotificationEvent` all catch and hand to + * `processException()`. Monolog's `StreamHandler` throws when `var/syspass.log` cannot be + * opened or appended to, and `notify()` is always called *after* the work it describes, so a + * full disk turned a completed operation into an error response — the administrator is told a + * master-password rotation failed when it had already finished. + */ + public function testAFailingLoggerDoesNotFailTheRequest() + { + $this->logger + ->expects($this->once()) + ->method('debug') + ->willThrowException(new RuntimeException('could not write to var/syspass.log')); + + // And the locales are still put back, which a plain try/catch around the call would miss. + $this->language->expects($this->once())->method('setAppLocales'); + $this->language->expects($this->once())->method('unsetAppLocales'); + + $this->logHandler->update(new Event('test.event', $this)); + + self::assertTrue(true, 'update() returned rather than propagating'); + } + /** * @throws InvalidClassException */