fix(firewall): account for authentication failures exactly once

Signed-off-by: Sebastian Krupinski <krupinski01@gmail.com>

core/lib/Models/Firewall/FirewallLogObject.php

@@ -26,6 +26,7 @@ class FirewallLogObject implements \JsonSerializable, JsonDeserializable
     public const EVENT_RULE_REMOVED = 'rule_removed';
 
     private ?string $id = null;
+    private ?string $eventId = null;
     private ?string $tenantId = null;
     private ?string $ipAddress = null;
     private ?string $deviceFingerprint = null;
@@ -55,6 +56,9 @@ class FirewallLogObject implements \JsonSerializable, JsonDeserializable
         if (array_key_exists('tenantId', $data)) {
             $this->tenantId = $data['tenantId'] !== null ? (string)$data['tenantId'] : null;
         }
+        if (array_key_exists('eventId', $data)) {
+            $this->eventId = $data['eventId'] !== null ? (string)$data['eventId'] : null;
+        }
         if (array_key_exists('ipAddress', $data)) {
             $this->ipAddress = $data['ipAddress'] !== null ? (string)$data['ipAddress'] : null;
         }
@@ -101,6 +105,7 @@ class FirewallLogObject implements \JsonSerializable, JsonDeserializable
     {
         return [
             'id' => $this->id,
+            'eventId' => $this->eventId,
             'tenantId' => $this->tenantId,
             'ipAddress' => $this->ipAddress,
             'deviceFingerprint' => $this->deviceFingerprint,
@@ -130,6 +135,17 @@ class FirewallLogObject implements \JsonSerializable, JsonDeserializable
         return $this;
     }
 
+    public function getEventId(): ?string
+    {
+        return $this->eventId;
+    }
+
+    public function setEventId(?string $eventId): self
+    {
+        $this->eventId = $eventId;
+        return $this;
+    }
+
     public function getTenantId(): ?string
     {
         return $this->tenantId;

core/lib/Module/Module.php

@@ -35,7 +35,6 @@ class Module extends ModuleInstanceAbstract implements ModuleConsoleInterface, M
         );
 
         foreach ([
-            SecurityEvent::AUTH_FAILURE,
             SecurityEvent::AUTH_SUCCESS,
             SecurityEvent::ACCESS_DENIED,
             SecurityEvent::BRUTE_FORCE_DETECTED,

core/lib/Service/FirewallService.php

@@ -139,6 +139,12 @@ class FirewallService
             return;
         }
 
+        $event->setTenantId($tenantId);
+        $log = $this->securityLog($event);
+        if ($log === null || !$this->store->createLogOnce($log)) {
+            return;
+        }
+
         // Check for brute force
         $windowSeconds = $this->getBoundedIntegerConfig(
             self::CONFIG_FAILURE_WINDOW,
@@ -157,9 +163,6 @@ class FirewallService
             $windowSeconds
         );
 
-        // Include current failure in count
-        $failureCount++;
-
         if ($failureCount >= $maxFailures) {
             $this->handleBruteForce($tenantId, $ipAddress, $failureCount, $windowSeconds);
         }
@@ -200,15 +203,24 @@ class FirewallService
      * Log security event to firewall logs
      */
     public function logSecurityEvent(SecurityEvent $event): void
+    {
+        $log = $this->securityLog($event);
+        if ($log !== null) {
+            $this->store->createLog($log);
+        }
+    }
+
+    private function securityLog(SecurityEvent $event): ?FirewallLogObject
     {
         $tenantId = $event->getTenantId() ?? $this->tenantContext->identifier();
         $ruleScope = $event->get('ruleScope');
         if (!$tenantId && $ruleScope !== FirewallRuleObject::SCOPE_SYSTEM) {
-            return;
+            return null;
         }
 
         $log = new FirewallLogObject();
-        $log->setTenantId($tenantId)
+        return $log->setEventId($event->getEventId())
+            ->setTenantId($tenantId)
             ->setIpAddress($event->getIpAddress())
             ->setDeviceFingerprint($event->getDeviceFingerprint())
             ->setUserAgent($event->getUserAgent())
@@ -221,8 +233,6 @@ class FirewallService
             ->setIdentityId($event->getUserId() ?? $event->getIdentityId())
             ->setTimestamp(new \DateTimeImmutable())
             ->setMetadata($event->getData());
-
-        $this->store->createLog($log);
     }
 
     /**

core/lib/Stores/FirewallStore.php

@@ -260,6 +260,30 @@ class FirewallStore
         return $log;
     }
 
+    /**
+     * Insert an event-backed log once, using the event ID as MongoDB's unique key.
+     */
+    public function createLogOnce(FirewallLogObject $log): bool
+    {
+        $eventId = $log->getEventId();
+        if ($eventId === null || $eventId === '') {
+            throw new \InvalidArgumentException('Idempotent firewall logs require an event ID.');
+        }
+
+        $data = $log->jsonSerialize();
+        unset($data['id']);
+        $data['_id'] = $eventId;
+
+        $result = $this->dataStore->selectCollection(self::LOGS_COLLECTION)->updateOne(
+            ['_id' => $eventId],
+            ['$setOnInsert' => $data],
+            ['upsert' => true]
+        );
+        $log->setId($eventId);
+
+        return $result->getUpsertedCount() === 1;
+    }
+
     /**
      * Get logs for a tenant with optional filters
      */

shared/lib/Event/Event.php

@@ -12,6 +12,7 @@ class Event
     private bool $propagationStopped = false;
     private array $data = [];
     private float $timestamp;
+    private string $eventId;
     private ?string $tenantId = null;
     private ?string $identityId = null;
 
@@ -21,6 +22,7 @@ class Event
     ) {
         $this->data = $data;
         $this->timestamp = microtime(true);
+        $this->eventId = bin2hex(random_bytes(16));
     }
 
     /**
@@ -80,6 +82,11 @@ class Event
         return $this->timestamp;
     }
 
+    public function getEventId(): string
+    {
+        return $this->eventId;
+    }
+
     /**
      * Stop event propagation to subsequent listeners
      */
@@ -143,4 +150,4 @@ class Event
             'identityId' => $this->identityId,
         ];
     }
-}
+}

tests/php/Integration/Stores/FirewallStoreTest.php

@@ -219,6 +219,25 @@ class FirewallStoreTest extends TestCase
         self::assertSame(FirewallRuleManager::ORIGIN_MANUAL, $logs[0]->getMetadata()['origin']);
     }
 
+    #[TestDox('Event-backed firewall logs are inserted exactly once')]
+    public function testIdempotentLogPersistence(): void
+    {
+        $log = (new FirewallLogObject())
+            ->setEventId('event-123')
+            ->setTenantId('tenant-a')
+            ->setIpAddress('203.0.113.10')
+            ->setEventType(FirewallLogObject::EVENT_AUTH_FAILURE)
+            ->setResult(FirewallLogObject::RESULT_BLOCKED)
+            ->setTimestamp(new \DateTimeImmutable());
+
+        self::assertTrue($this->store->createLogOnce($log));
+        self::assertFalse($this->store->createLogOnce($log));
+
+        $logs = $this->store->listLogs('tenant-a');
+        self::assertCount(1, $logs);
+        self::assertSame('event-123', $logs[0]->getEventId());
+    }
+
     private function rule(
         string $reason,
         string $scope,

tests/php/Unit/Models/Firewall/FirewallLogObjectTest.php

@@ -15,12 +15,14 @@ class FirewallLogObjectTest extends TestCase
     public function testRuleContextSerialization(): void
     {
         $log = (new FirewallLogObject())
+            ->setEventId('event-123')
             ->setRuleId('rule-123')
             ->setRuleScope(FirewallRuleObject::SCOPE_SYSTEM);
 
         $restored = (new FirewallLogObject())->jsonDeserialize($log->jsonSerialize());
 
         self::assertSame('rule-123', $restored->getRuleId());
+        self::assertSame('event-123', $restored->getEventId());
         self::assertSame(FirewallRuleObject::SCOPE_SYSTEM, $restored->getRuleScope());
     }
 

tests/php/Unit/Module/CoreModuleTest.php

@@ -28,12 +28,13 @@ final class CoreModuleTest extends TestCase
         $module->boot();
         $definitions = $registry->definitions();
 
-        self::assertCount(10, $definitions);
+        self::assertCount(9, $definitions);
         self::assertSame(['core'], array_values(array_unique(array_column($definitions, 'module'))));
         self::assertSame(
             FirewallService::class,
             $registry->listeners(SecurityEvent::AUTH_FAILURE, DeliveryMode::Immediate)[0]->service,
         );
+        self::assertSame([], $registry->listeners(SecurityEvent::AUTH_FAILURE, DeliveryMode::Deferred));
         foreach ([
             SecurityEvent::RATE_LIMIT_EXCEEDED,
             SecurityEvent::SUSPICIOUS_ACTIVITY,

tests/php/Unit/Service/FirewallServiceTest.php

@@ -339,6 +339,7 @@ class FirewallServiceTest extends TestCase
     #[TestDox('Typed tenant firewall settings drive brute-force thresholds')]
     public function testFirewallConfiguration(): void
     {
+        $this->store->method('createLogOnce')->willReturn(true);
         $this->currentConfiguration = (new TenantConfiguration())->jsonDeserialize([
             'firewall' => [
                 'enabled' => true,
@@ -364,6 +365,7 @@ class FirewallServiceTest extends TestCase
     #[TestDox('Unsafe numeric firewall settings fall back to safe defaults')]
     public function testConfigurationBounds(): void
     {
+        $this->store->method('createLogOnce')->willReturn(true);
         $this->currentConfiguration = (new TenantConfiguration())->jsonDeserialize([
             'firewall' => [
                 'maxAuthFailures' => 0,
@@ -384,11 +386,12 @@ class FirewallServiceTest extends TestCase
     #[TestDox('Automatic blocks retain the tenant carried by the authentication event')]
     public function testAutomaticBlockTenant(): void
     {
+        $this->store->method('createLogOnce')->willReturn(true);
         $this->currentTenant = 'tenant-context';
         $this->store->expects($this->once())
             ->method('countRecentFailures')
             ->with('tenant-event', '203.0.113.10', 300)
-            ->willReturn(4);
+            ->willReturn(5);
         $this->store->expects($this->once())
             ->method('findExactIpRule')
             ->with(
@@ -431,6 +434,7 @@ class FirewallServiceTest extends TestCase
     #[TestDox('Authentication events without a tenant use the current tenant')]
     public function testAutomaticBlockTenantFallback(): void
     {
+        $this->store->method('createLogOnce')->willReturn(true);
         $this->store->expects($this->once())
             ->method('countRecentFailures')
             ->with('tenant-a', '203.0.113.10', 300)
@@ -453,6 +457,25 @@ class FirewallServiceTest extends TestCase
         );
     }
 
+    #[TestDox('Repeated delivery of one authentication event is counted once')]
+    public function testAuthenticationFailureIdempotency(): void
+    {
+        $this->store->expects($this->exactly(2))
+            ->method('createLogOnce')
+            ->willReturnOnConsecutiveCalls(true, false);
+        $this->store->expects($this->once())
+            ->method('countRecentFailures')
+            ->with('tenant-a', '203.0.113.10', 300)
+            ->willReturn(1);
+        $event = \KTXF\Event\SecurityEvent::authFailure('203.0.113.10');
+        $eventId = $event->getEventId();
+
+        $this->service->handleAuthFailure($event);
+        $this->service->handleAuthFailure($event);
+
+        self::assertSame($eventId, $event->getEventId());
+    }
+
     private function rule(
         string $id,
         string $scope,