fix(firewall): enforce system rules independently of tenant context

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

core/lib/Service/FirewallRuleCache.php

@@ -10,20 +10,30 @@ use KTXC\Stores\FirewallStore;
 final class FirewallRuleCache
 {
     /** @var array<string, FirewallRuleObject[]> */
-    private array $rules = [];
+    private array $tenantRules = [];
+
+    /** @var FirewallRuleObject[]|null */
+    private ?array $systemRules = null;
 
     public function __construct(private readonly FirewallStore $store)
     {
     }
 
     /** @return FirewallRuleObject[] */
-    public function applicable(string $tenantId): array
+    public function tenant(string $tenantId): array
     {
-        return $this->rules[$tenantId] ??= $this->store->listApplicableRules($tenantId);
+        return $this->tenantRules[$tenantId] ??= $this->store->listRules($tenantId);
+    }
+
+    /** @return FirewallRuleObject[] */
+    public function system(): array
+    {
+        return $this->systemRules ??= $this->store->listSystemRules();
     }
 
     public function invalidate(): void
     {
-        $this->rules = [];
+        $this->tenantRules = [];
+        $this->systemRules = null;
     }
 }

core/lib/Service/FirewallService.php

@@ -73,26 +73,22 @@ class FirewallService
         string $ipAddress,
         ?string $deviceFingerprint = null
     ): FirewallAnalyzeResult {
-        // Check if firewall is enabled for this tenant
-        if (!$this->isEnabled()) {
-            return new FirewallAnalyzeResult(true);
-        }
-
         $tenantId = $this->tenantContext->identifier();
-        if (!$tenantId) {
+        $ruleGroups = [
-            return new FirewallAnalyzeResult(true);
+            [$this->ruleCache->system(), FirewallRuleObject::ACTION_BLOCK],
+        ];
+
+        if ($tenantId !== null && $this->isEnabled()) {
+            $tenantRules = $this->ruleCache->tenant($tenantId);
+            $ruleGroups[] = [$tenantRules, FirewallRuleObject::ACTION_ALLOW];
+            $ruleGroups[] = [$tenantRules, FirewallRuleObject::ACTION_BLOCK];
         }
 
-        $rules = $this->getActiveRules();
+        $ruleGroups[] = [$this->ruleCache->system(), FirewallRuleObject::ACTION_ALLOW];
 
-        foreach ([
+        foreach ($ruleGroups as [$rules, $action]) {
-            [FirewallRuleObject::SCOPE_SYSTEM, FirewallRuleObject::ACTION_BLOCK],
-            [FirewallRuleObject::SCOPE_TENANT, FirewallRuleObject::ACTION_ALLOW],
-            [FirewallRuleObject::SCOPE_TENANT, FirewallRuleObject::ACTION_BLOCK],
-            [FirewallRuleObject::SCOPE_SYSTEM, FirewallRuleObject::ACTION_ALLOW],
-        ] as [$scope, $action]) {
             foreach ($rules as $rule) {
-                if ($rule->getScope() !== $scope || $rule->getAction() !== $action) {
+                if ($rule->getAction() !== $action) {
                     continue;
                 }
 
@@ -310,20 +306,6 @@ class FirewallService
         return $value;
     }
 
-    /**
-     * Get active rules (cached)
-     * @return FirewallRuleObject[]
-     */
-    private function getActiveRules(): array
-    {
-        $tenantId = $this->tenantContext->identifier();
-        if (!$tenantId) {
-            return [];
-        }
-
-        return $this->ruleCache->applicable($tenantId);
-    }
-
     /**
      * Cleanup maintenance tasks
      */

core/lib/Stores/FirewallStore.php

@@ -55,43 +55,6 @@ class FirewallStore
         return $list;
     }
 
-    /**
-     * List active system-wide rules and active rules owned by a tenant.
-     */
-    public function listApplicableRules(string $tenantId): array
-    {
-        $now = (new \DateTimeImmutable())->format(\DateTimeInterface::ATOM);
-        $filter = [
-            'enabled' => true,
-            '$and' => [
-                [
-                    '$or' => [
-                        ['scope' => FirewallRuleObject::SCOPE_SYSTEM],
-                        [
-                            'tenantId' => $tenantId,
-                            'scope' => FirewallRuleObject::SCOPE_TENANT,
-                        ],
-                    ],
-                ],
-                [
-                    '$or' => [
-                        ['expiresAt' => null],
-                        ['expiresAt' => ['$gt' => $now]],
-                    ],
-                ],
-            ],
-        ];
-
-        $cursor = $this->dataStore->selectCollection(self::RULES_COLLECTION)->find($filter);
-        $list = [];
-
-        foreach ($cursor as $entry) {
-            $list[] = (new FirewallRuleObject())->jsonDeserialize((array)$entry);
-        }
-
-        return $list;
-    }
-
     public function listSystemRules(bool $activeOnly = true): array
     {
         $filter = ['scope' => FirewallRuleObject::SCOPE_SYSTEM, 'tenantId' => null];

tests/php/Integration/Stores/FirewallStoreTest.php

@@ -45,7 +45,7 @@ class FirewallStoreTest extends TestCase
         }
     }
 
-    #[TestDox('Applicable rules contain system rules and only the requested tenant rules')]
+    #[TestDox('System and tenant rule sets remain independently scoped')]
     public function testApplicableScopes(): void
     {
         $tenantA = $this->rule('tenant-a', FirewallRuleObject::SCOPE_TENANT, 'tenant-a');
@@ -60,7 +60,10 @@ class FirewallStoreTest extends TestCase
             $this->store->depositRule($rule);
         }
 
-        $rules = $this->store->listApplicableRules('tenant-a');
+        $rules = array_merge(
+            $this->store->listSystemRules(),
+            $this->store->listRules('tenant-a')
+        );
         $reasons = array_map(static fn(FirewallRuleObject $rule): ?string => $rule->getReason(), $rules);
         sort($reasons);
 

tests/php/Unit/Service/FirewallRuleManagerTest.php

@@ -94,7 +94,7 @@ class FirewallRuleManagerTest extends TestCase
     public function testCacheInvalidation(): void
     {
         $this->store->expects($this->exactly(2))
-            ->method('listApplicableRules')
+            ->method('listRules')
             ->with('tenant-a')
             ->willReturnOnConsecutiveCalls([], []);
         $cache = new FirewallRuleCache($this->store);
@@ -102,8 +102,8 @@ class FirewallRuleManagerTest extends TestCase
         $this->store->method('findExactIpRule')->willReturn(null);
         $this->store->method('depositRule')->willReturnArgument(0);
 
-        self::assertSame([], $cache->applicable('tenant-a'));
+        self::assertSame([], $cache->tenant('tenant-a'));
         $manager->blockIp(FirewallRuleScope::tenant('tenant-a'), '203.0.113.10', null, 'admin');
-        self::assertSame([], $cache->applicable('tenant-a'));
+        self::assertSame([], $cache->tenant('tenant-a'));
     }
 }

tests/php/Unit/Service/FirewallServiceTest.php

@@ -67,10 +67,8 @@ class FirewallServiceTest extends TestCase
             'tenant-a'
         );
 
-        $this->store->expects($this->once())
+        $this->store->expects($this->once())->method('listSystemRules')->willReturn([$systemBlock]);
-            ->method('listApplicableRules')
+        $this->store->expects($this->once())->method('listRules')->with('tenant-a')->willReturn([$tenantAllow]);
-            ->with('tenant-a')
-            ->willReturn([$tenantAllow, $systemBlock]);
         $this->events->expects($this->once())->method('dispatch');
 
         $result = $this->service->analyze('203.0.113.10');
@@ -95,7 +93,8 @@ class FirewallServiceTest extends TestCase
             'tenant-a'
         );
 
-        $this->store->method('listApplicableRules')->willReturn([$systemAllow, $tenantBlock]);
+        $this->store->method('listSystemRules')->willReturn([$systemAllow]);
+        $this->store->method('listRules')->with('tenant-a')->willReturn([$tenantBlock]);
         $this->events->expects($this->once())->method('dispatch');
 
         $result = $this->service->analyze('203.0.113.10');
@@ -107,8 +106,9 @@ class FirewallServiceTest extends TestCase
     #[TestDox('Rule caches are isolated by tenant')]
     public function testTenantCacheIsolation(): void
     {
+        $this->store->expects($this->once())->method('listSystemRules')->willReturn([]);
         $this->store->expects($this->exactly(2))
-            ->method('listApplicableRules')
+            ->method('listRules')
             ->willReturnCallback(static fn(string $tenantId): array => [
                 (new FirewallRuleObject())
                     ->setId($tenantId)
@@ -124,6 +124,79 @@ class FirewallServiceTest extends TestCase
         self::assertSame('tenant-b', $this->service->analyze('203.0.113.10')->ruleId);
     }
 
+    #[TestDox('System blocks apply when no tenant is resolved')]
+    public function testSystemBlockWithoutTenant(): void
+    {
+        $this->currentTenant = null;
+        $this->store->method('listSystemRules')->willReturn([
+            $this->rule(
+                'system-block',
+                FirewallRuleObject::SCOPE_SYSTEM,
+                FirewallRuleObject::ACTION_BLOCK,
+                null
+            ),
+        ]);
+        $this->store->expects($this->never())->method('listRules');
+
+        self::assertSame('system-block', $this->service->analyze('203.0.113.10')->ruleId);
+    }
+
+    #[TestDox('System allows can match when no tenant is resolved')]
+    public function testSystemAllowWithoutTenant(): void
+    {
+        $this->currentTenant = null;
+        $this->store->method('listSystemRules')->willReturn([
+            $this->rule(
+                'system-allow',
+                FirewallRuleObject::SCOPE_SYSTEM,
+                FirewallRuleObject::ACTION_ALLOW,
+                null
+            ),
+        ]);
+
+        $result = $this->service->analyze('203.0.113.10');
+
+        self::assertTrue($result->isAllowed());
+        self::assertSame('system-allow', $result->ruleId);
+    }
+
+    #[TestDox('System blocks remain active when the tenant firewall is disabled')]
+    public function testSystemBlockWithDisabledTenant(): void
+    {
+        $this->disableTenantFirewall();
+        $this->store->method('listSystemRules')->willReturn([
+            $this->rule(
+                'system-block',
+                FirewallRuleObject::SCOPE_SYSTEM,
+                FirewallRuleObject::ACTION_BLOCK,
+                null
+            ),
+        ]);
+        $this->store->expects($this->never())->method('listRules');
+
+        self::assertSame('system-block', $this->service->analyze('203.0.113.10')->ruleId);
+    }
+
+    #[TestDox('Tenant rules are ignored when the tenant firewall is disabled')]
+    public function testTenantRulesDisabled(): void
+    {
+        $this->disableTenantFirewall();
+        $this->store->method('listSystemRules')->willReturn([]);
+        $this->store->expects($this->never())->method('listRules');
+
+        self::assertTrue($this->service->analyze('203.0.113.10')->isAllowed());
+    }
+
+    #[TestDox('Requests without a tenant and without a system match are allowed')]
+    public function testNoTenantDefault(): void
+    {
+        $this->currentTenant = null;
+        $this->store->method('listSystemRules')->willReturn([]);
+        $this->store->expects($this->never())->method('listRules');
+
+        self::assertTrue($this->service->analyze('203.0.113.10')->isAllowed());
+    }
+
     #[TestDox('Typed tenant firewall settings drive brute-force thresholds')]
     public function testFirewallConfiguration(): void
     {
@@ -248,4 +321,11 @@ class FirewallServiceTest extends TestCase
             ->setValue('203.0.113.10')
             ->setReason($id);
     }
+
+    private function disableTenantFirewall(): void
+    {
+        $this->currentConfiguration = (new TenantConfiguration())->jsonDeserialize([
+            'firewall' => ['enabled' => false],
+        ]);
+    }
 }