fix: keep the log processors working on a stateless firewall

src/Logging/InjectUserProcessor.php

@@ -8,8 +8,15 @@ use Symfony\Bundle\SecurityBundle\Security;
 
 class InjectUserProcessor
 {
-    public function __construct(private readonly Security $security)
-    {
+    /**
+     * The SAPI is injected rather than read from PHP_SAPI inside the processor so a test can
+     * exercise the web path: under PHPUnit php_sapi_name() is always "cli", which made the
+     * branch below unreachable from every functional test.
+     */
+    public function __construct(
+        private readonly Security $security,
+        private readonly string $sapi = \PHP_SAPI,
+    ) {
     }
 
     public function __invoke(LogRecord $record): LogRecord
@@ -18,10 +25,9 @@ class InjectUserProcessor
             return $record;
         }
 
-        /** @var User $user */
         $user = $this->security->getUser();
 
-        if (null === $user || 'cli' === php_sapi_name()) {
+        if (null === $user || 'cli' === $this->sapi) {
             $record->extra['user'] = [
                 'id' => null,
                 'username' => 'system',
@@ -31,6 +37,20 @@ class InjectUserProcessor
             return $record;
         }
 
+        // Not every firewall authenticates a row in the user table: the stateless "api" firewall
+        // resolves its shared key to an in-memory user, which has an identifier and roles and
+        // nothing else. Reaching for getId() or getEmail() there is a 500 on a request that had
+        // already stored its data.
+        if (false === $user instanceof User) {
+            $record->extra['user'] = [
+                'id' => null,
+                'username' => $user->getUserIdentifier(),
+                'roles' => $user->getRoles(),
+            ];
+
+            return $record;
+        }
+
         $record->extra['user'] = [
             'id' => $user->getId(),
             'email' => $user->getEmail(),

src/Logging/UserDataProcessor.php

@@ -2,7 +2,7 @@
 
 namespace App\Logging;
 
-use App\Entity\Core\User;
+use App\Entity\User;
 use Monolog\Attribute\AsMonologProcessor;
 use Monolog\LogRecord;
 use Symfony\Bundle\SecurityBundle\Security;
@@ -11,8 +11,15 @@ use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
 #[AsMonologProcessor]
 class UserDataProcessor
 {
-    public function __construct(private readonly Security $security)
-    {
+    /**
+     * The SAPI is injected rather than read from PHP_SAPI inside the processor so a test can
+     * exercise the web path: under PHPUnit php_sapi_name() is always "cli", which made
+     * everything below the check unreachable from every functional test.
+     */
+    public function __construct(
+        private readonly Security $security,
+        private readonly string $sapi = \PHP_SAPI,
+    ) {
     }
 
     public function __invoke(LogRecord $record): LogRecord
@@ -21,7 +28,7 @@ class UserDataProcessor
             return $record;
         }
 
-        if ('cli' === php_sapi_name()) {
+        if ('cli' === $this->sapi) {
             $record->extra['user'] = [
                 'id' => null,
                 'username' => 'SYSTEM',
@@ -31,7 +38,6 @@ class UserDataProcessor
             return $record;
         }
 
-        /** @var User $user */
         $user = $this->security->getUser();
 
         if (null === $user) {
@@ -57,7 +63,10 @@ class UserDataProcessor
             $username = $user->getUserIdentifier();
         }
         $record->extra['user'] = [
-            'id' => $user->getId(),
+            // Only a user table row has an id: the stateless "api" firewall resolves its shared
+            // key to an in-memory user, and calling getId() on that is a 500 on a request that
+            // had already stored its data.
+            'id' => $user instanceof User ? $user->getId() : null,
             'username' => $username,
             'roles' => $user->getRoles(),
         ];

tests/Logging/InjectUserProcessorTest.php

@@ -0,0 +1,83 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Logging;
+
+use App\Logging\InjectUserProcessor;
+use Monolog\Level;
+use Monolog\LogRecord;
+use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\Security\Core\User\InMemoryUser;
+
+/**
+ * The processor runs on every channel, so it sees whatever user the firewall of the request
+ * authenticated - including the in-memory user the stateless "api" firewall resolves its shared
+ * key to, which has no getId() and no getEmail(). That cost a stored submission a 500 once.
+ *
+ * Every case here passes a web SAPI: with "cli" the processor returns the system record before
+ * reaching any of this, which is exactly why no functional test caught the bug.
+ */
+class InjectUserProcessorTest extends TestCase
+{
+    public function testAnApiFirewallUserIsLoggedByItsIdentifier(): void
+    {
+        $record = $this->process(new InMemoryUser('api', null, ['ROLE_API']));
+
+        $this->assertSame(
+            ['id' => null, 'username' => 'api', 'roles' => ['ROLE_API']],
+            $record->extra['user']
+        );
+    }
+
+    public function testAnAnonymousRequestIsLoggedAsSystem(): void
+    {
+        $record = $this->process(null);
+
+        $this->assertSame('system', $record->extra['user']['username']);
+    }
+
+    public function testTheConsoleIsLoggedAsSystemEvenWithAUser(): void
+    {
+        $record = $this->process(new InMemoryUser('api', null, ['ROLE_API']), sapi: 'cli');
+
+        $this->assertSame('system', $record->extra['user']['username']);
+    }
+
+    public function testAnAlreadyResolvedUserIsLeftAlone(): void
+    {
+        $security = $this->createMock(Security::class);
+        $security->expects($this->never())->method('getUser');
+
+        $processor = new InjectUserProcessor($security, 'fpm-fcgi');
+        $record = $processor(self::record(['user' => ['username' => 'kept']]));
+
+        $this->assertSame(['username' => 'kept'], $record->extra['user']);
+    }
+
+    private function process(?InMemoryUser $user, string $sapi = 'fpm-fcgi'): LogRecord
+    {
+        $security = $this->createMock(Security::class);
+        $security->method('getUser')->willReturn($user);
+
+        $processor = new InjectUserProcessor($security, $sapi);
+
+        return $processor(self::record());
+    }
+
+    /**
+     * @param array<string, mixed> $extra
+     */
+    private static function record(array $extra = []): LogRecord
+    {
+        return new LogRecord(
+            new \DateTimeImmutable(),
+            'api',
+            Level::Info,
+            'Application form submission received',
+            [],
+            $extra
+        );
+    }
+}

tests/Logging/UserDataProcessorTest.php

@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Logging;
+
+use App\Logging\UserDataProcessor;
+use Monolog\Level;
+use Monolog\LogRecord;
+use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\Security\Core\User\InMemoryUser;
+
+/**
+ * Same hazard as InjectUserProcessorTest: this one feeds the log table through
+ * App\Logging\DatabaseHandler, and it is registered globally with #[AsMonologProcessor], so a
+ * request on the stateless "api" firewall reaches it with an in-memory user that has no getId().
+ */
+class UserDataProcessorTest extends TestCase
+{
+    public function testAnApiFirewallUserGetsNoIdInsteadOfAnError(): void
+    {
+        $security = $this->createMock(Security::class);
+        $security->method('getUser')->willReturn(new InMemoryUser('api', null, ['ROLE_API']));
+        $security->method('getToken')->willReturn(null);
+
+        $processor = new UserDataProcessor($security, 'fpm-fcgi');
+
+        $record = $processor(new LogRecord(
+            new \DateTimeImmutable(),
+            'api',
+            Level::Info,
+            'Application form submission received'
+        ));
+
+        $this->assertSame(
+            ['id' => null, 'username' => 'api', 'roles' => ['ROLE_API']],
+            $record->extra['user']
+        );
+    }
+}