From b49a04f796b9ad8533c53fee541132a4c2214909 Mon Sep 17 00:00:00 2001
From: ArthurHoaro <arthur@hoa.ro>
Date: Sat, 9 Feb 2019 16:44:48 +0100
Subject: Rewrite IP ban management

This adds a dedicated manager class to handle all ban interactions, which is instantiated and handled by LoginManager.
IPs are now stored in the same format as the datastore, through FileUtils.

Fixes #1032 #587
---
 tests/security/BanManagerTest.php   | 393 ++++++++++++++++++++++++++++++++++++
 tests/security/LoginManagerTest.php | 104 +---------
 2 files changed, 400 insertions(+), 97 deletions(-)
 create mode 100644 tests/security/BanManagerTest.php

(limited to 'tests/security')

diff --git a/tests/security/BanManagerTest.php b/tests/security/BanManagerTest.php
new file mode 100644
index 00000000..bba7c8ad
--- /dev/null
+++ b/tests/security/BanManagerTest.php
@@ -0,0 +1,393 @@
+<?php
+
+
+namespace Shaarli\Security;
+
+use PHPUnit\Framework\TestCase;
+use Shaarli\FileUtils;
+
+/**
+ * Test coverage for BanManager
+ */
+class BanManagerTest extends TestCase
+{
+    /** @var BanManager Ban Manager instance */
+    protected $banManager;
+
+    /** @var string Banned IP filename */
+    protected $banFile = 'sandbox/ipbans.php';
+
+    /** @var string Log filename */
+    protected $logFile = 'sandbox/shaarli.log';
+
+    /** @var string Local client IP address */
+    protected $ipAddr = '127.0.0.1';
+
+    /** @var string Trusted proxy IP address */
+    protected $trustedProxy = '10.1.1.100';
+
+    /** @var array Simulates the $_SERVER array */
+    protected $server = [];
+
+    /**
+     * Prepare or reset test resources
+     */
+    public function setUp()
+    {
+        if (file_exists($this->banFile)) {
+            unlink($this->banFile);
+        }
+
+        $this->banManager = $this->getNewBanManagerInstance();
+        $this->server['REMOTE_ADDR'] = $this->ipAddr;
+    }
+
+    /**
+     * Test constructor with initial file.
+     */
+    public function testInstantiateFromFile()
+    {
+        $time = time() + 10;
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'failures' => [
+                    $this->ipAddr => 2,
+                    $ip = '1.2.3.4' => 1,
+                ],
+                'bans' => [
+                    $ip2 = '8.8.8.8' => $time,
+                    $ip3 = '1.1.1.1' => $time + 1,
+                ],
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(2, $this->banManager->getFailures());
+        $this->assertEquals(2, $this->banManager->getFailures()[$this->ipAddr]);
+        $this->assertEquals(1, $this->banManager->getFailures()[$ip]);
+        $this->assertCount(2, $this->banManager->getBans());
+        $this->assertEquals($time, $this->banManager->getBans()[$ip2]);
+        $this->assertEquals($time + 1, $this->banManager->getBans()[$ip3]);
+    }
+
+    /**
+     * Test constructor with initial file with invalid values
+     */
+    public function testInstantiateFromCrappyFile()
+    {
+        FileUtils::writeFlatDB($this->banFile, 'plop');
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertEquals([], $this->banManager->getFailures());
+        $this->assertEquals([], $this->banManager->getBans());
+    }
+
+    /**
+     * Test failed attempt with a direct IP.
+     */
+    public function testHandleFailedAttempt()
+    {
+        $this->assertCount(0, $this->banManager->getFailures());
+
+        $this->banManager->handleFailedAttempt($this->server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(1, $this->banManager->getFailures()[$this->ipAddr]);
+
+        $this->banManager->handleFailedAttempt($this->server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(2, $this->banManager->getFailures()[$this->ipAddr]);
+    }
+
+    /**
+     * Test failed attempt behind a trusted proxy IP (with proper IP forwarding).
+     */
+    public function testHandleFailedAttemptBehingProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+            'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
+        ];
+        $this->assertCount(0, $this->banManager->getFailures());
+
+        $this->banManager->handleFailedAttempt($server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(1, $this->banManager->getFailures()[$this->ipAddr]);
+
+        $this->banManager->handleFailedAttempt($server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(2, $this->banManager->getFailures()[$this->ipAddr]);
+    }
+
+    /**
+     * Test failed attempt behind a trusted proxy IP but without IP forwarding.
+     */
+    public function testHandleFailedAttemptBehindNotConfiguredProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+        ];
+        $this->assertCount(0, $this->banManager->getFailures());
+
+        $this->banManager->handleFailedAttempt($server);
+        $this->assertCount(0, $this->banManager->getFailures());
+
+        $this->banManager->handleFailedAttempt($server);
+        $this->assertCount(0, $this->banManager->getFailures());
+    }
+
+    /**
+     * Test failed attempts with multiple direct IP.
+     */
+    public function testHandleFailedAttemptMultipleIp()
+    {
+        $this->assertCount(0, $this->banManager->getFailures());
+        $this->banManager->handleFailedAttempt($this->server);
+        $this->server['REMOTE_ADDR'] = '1.2.3.4';
+        $this->banManager->handleFailedAttempt($this->server);
+        $this->banManager->handleFailedAttempt($this->server);
+        $this->assertCount(2, $this->banManager->getFailures());
+        $this->assertEquals(1, $this->banManager->getFailures()[$this->ipAddr]);
+        $this->assertEquals(2, $this->banManager->getFailures()[$this->server['REMOTE_ADDR']]);
+    }
+
+    /**
+     * Test clear failure for provided IP without any additional data.
+     */
+    public function testClearFailuresEmpty()
+    {
+        $this->assertCount(0, $this->banManager->getFailures());
+        $this->banManager->clearFailures($this->server);
+        $this->assertCount(0, $this->banManager->getFailures());
+    }
+
+    /**
+     * Test clear failure for provided IP with failed attempts.
+     */
+    public function testClearFailuresFromFile()
+    {
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'failures' => [
+                    $this->ipAddr => 2,
+                    $ip = '1.2.3.4' => 1,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(2, $this->banManager->getFailures());
+        $this->banManager->clearFailures($this->server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(1, $this->banManager->getFailures()[$ip]);
+    }
+
+    /**
+     * Test clear failure for provided IP with failed attempts, behind a reverse proxy.
+     */
+    public function testClearFailuresFromFileBehindProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+            'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
+        ];
+
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'failures' => [
+                    $this->ipAddr => 2,
+                    $ip = '1.2.3.4' => 1,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(2, $this->banManager->getFailures());
+        $this->banManager->clearFailures($server);
+        $this->assertCount(1, $this->banManager->getFailures());
+        $this->assertEquals(1, $this->banManager->getFailures()[$ip]);
+    }
+
+    /**
+     * Test clear failure for provided IP with failed attempts,
+     * behind a reverse proxy without forwarding.
+     */
+    public function testClearFailuresFromFileBehindNotConfiguredProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+        ];
+
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'failures' => [
+                    $this->ipAddr => 2,
+                    $ip = '1.2.3.4' => 1,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(2, $this->banManager->getFailures());
+        $this->banManager->clearFailures($server);
+        $this->assertCount(2, $this->banManager->getFailures());
+    }
+
+    /**
+     * Test isBanned without any data
+     */
+    public function testIsBannedEmpty()
+    {
+        $this->assertFalse($this->banManager->isBanned($this->server));
+    }
+
+    /**
+     * Test isBanned with banned IP from file data
+     */
+    public function testBannedFromFile()
+    {
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() + 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertTrue($this->banManager->isBanned($this->server));
+    }
+
+    /**
+     * Test isBanned with banned IP from file data behind a reverse proxy
+     */
+    public function testBannedFromFileBehindProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+            'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
+        ];
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() + 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertTrue($this->banManager->isBanned($server));
+    }
+
+    /**
+     * Test isBanned with banned IP from file data behind a reverse proxy,
+     * without IP forwarding
+     */
+    public function testBannedFromFileBehindNotConfiguredProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+        ];
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() + 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertFalse($this->banManager->isBanned($server));
+    }
+
+    /**
+     * Test isBanned with an expired ban
+     */
+    public function testLiftBan()
+    {
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() - 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertFalse($this->banManager->isBanned($this->server));
+    }
+
+    /**
+     * Test isBanned with an expired ban behind a reverse proxy
+     */
+    public function testLiftBanBehindProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+            'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
+        ];
+
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() - 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertFalse($this->banManager->isBanned($server));
+    }
+
+    /**
+     * Test isBanned with an expired ban behind a reverse proxy
+     */
+    public function testLiftBanBehindNotConfiguredProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+        ];
+
+        FileUtils::writeFlatDB(
+            $this->banFile,
+            [
+                'bans' => [
+                    $this->ipAddr => time() - 10,
+                ]
+            ]
+        );
+        $this->banManager = $this->getNewBanManagerInstance();
+
+        $this->assertCount(1, $this->banManager->getBans());
+        $this->assertFalse($this->banManager->isBanned($server));
+    }
+
+    /**
+     * Build a new instance of BanManager, which will reread the ban file.
+     *
+     * @return BanManager instance
+     */
+    protected function getNewBanManagerInstance()
+    {
+        return new BanManager(
+            [$this->trustedProxy],
+            3,
+            1800,
+            $this->banFile,
+            $this->logFile
+        );
+    }
+}
diff --git a/tests/security/LoginManagerTest.php b/tests/security/LoginManagerTest.php
index 7b0262b3..eef0f22a 100644
--- a/tests/security/LoginManagerTest.php
+++ b/tests/security/LoginManagerTest.php
@@ -75,54 +75,27 @@ class LoginManagerTest extends TestCase
             'credentials.salt' => $this->salt,
             'resource.ban_file' => $this->banFile,
             'resource.log' => $this->logFile,
-            'security.ban_after' => 4,
+            'security.ban_after' => 2,
             'security.ban_duration' => 3600,
             'security.trusted_proxies' => [$this->trustedProxy],
         ]);
 
         $this->cookie = [];
-
-        $this->globals = &$GLOBALS;
-        unset($this->globals['IPBANS']);
-
         $this->session = [];
 
         $this->sessionManager = new SessionManager($this->session, $this->configManager);
-        $this->loginManager = new LoginManager($this->globals, $this->configManager, $this->sessionManager);
+        $this->loginManager = new LoginManager($this->configManager, $this->sessionManager);
         $this->server['REMOTE_ADDR'] = $this->ipAddr;
     }
 
-    /**
-     * Wipe test resources
-     */
-    public function tearDown()
-    {
-        unset($this->globals['IPBANS']);
-    }
-
-    /**
-     * Instantiate a LoginManager and load ban records
-     */
-    public function testReadBanFile()
-    {
-        file_put_contents(
-            $this->banFile,
-            "<?php\n\$GLOBALS['IPBANS']=array('FAILURES' => array('127.0.0.1' => 99));\n?>"
-        );
-        new LoginManager($this->globals, $this->configManager, null);
-        $this->assertEquals(99, $this->globals['IPBANS']['FAILURES']['127.0.0.1']);
-    }
-
     /**
      * Record a failed login attempt
      */
     public function testHandleFailedLogin()
     {
         $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-
         $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
+        $this->assertFalse($this->loginManager->canLogin($this->server));
     }
 
     /**
@@ -135,10 +108,8 @@ class LoginManagerTest extends TestCase
             'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
         ];
         $this->loginManager->handleFailedLogin($server);
-        $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-
         $this->loginManager->handleFailedLogin($server);
-        $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
+        $this->assertFalse($this->loginManager->canLogin($server));
     }
 
     /**
@@ -150,39 +121,8 @@ class LoginManagerTest extends TestCase
             'REMOTE_ADDR' => $this->trustedProxy,
         ];
         $this->loginManager->handleFailedLogin($server);
-        $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
-
         $this->loginManager->handleFailedLogin($server);
-        $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
-    }
-
-    /**
-     * Record a failed login attempt and ban the IP after too many failures
-     */
-    public function testHandleFailedLoginBanIp()
-    {
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-        $this->assertTrue($this->loginManager->canLogin($this->server));
-
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-        $this->assertTrue($this->loginManager->canLogin($this->server));
-
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(3, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-        $this->assertTrue($this->loginManager->canLogin($this->server));
-
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(4, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-        $this->assertFalse($this->loginManager->canLogin($this->server));
-
-        // handleFailedLogin is not supposed to be called at this point:
-        // - no login form should be displayed once an IP has been banned
-        // - yet this could happen when using custom templates / scripts
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(5, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
-        $this->assertFalse($this->loginManager->canLogin($this->server));
+        $this->assertTrue($this->loginManager->canLogin($server));
     }
 
     /**
@@ -202,14 +142,11 @@ class LoginManagerTest extends TestCase
     public function testHandleSuccessfulLoginAfterFailure()
     {
         $this->loginManager->handleFailedLogin($this->server);
-        $this->loginManager->handleFailedLogin($this->server);
-        $this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
         $this->assertTrue($this->loginManager->canLogin($this->server));
 
         $this->loginManager->handleSuccessfulLogin($this->server);
+        $this->loginManager->handleFailedLogin($this->server);
         $this->assertTrue($this->loginManager->canLogin($this->server));
-        $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
-        $this->assertFalse(isset($this->globals['IPBANS']['BANS'][$this->ipAddr]));
     }
 
     /**
@@ -220,33 +157,6 @@ class LoginManagerTest extends TestCase
         $this->assertTrue($this->loginManager->canLogin($this->server));
     }
 
-    /**
-     * The IP is banned
-     */
-    public function testCanLoginIpBanned()
-    {
-        // ban the IP for an hour
-        $this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10;
-        $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600;
-
-        $this->assertFalse($this->loginManager->canLogin($this->server));
-    }
-
-    /**
-     * The IP is banned, and the ban duration is over
-     */
-    public function testCanLoginIpBanExpired()
-    {
-        // ban the IP for an hour
-        $this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10;
-        $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600;
-        $this->assertFalse($this->loginManager->canLogin($this->server));
-
-        // lift the ban
-        $this->globals['IPBANS']['BANS'][$this->ipAddr] = time() - 3600;
-        $this->assertTrue($this->loginManager->canLogin($this->server));
-    }
-
     /**
      * Generate a token depending on the user credentials and client IP
      */
@@ -282,7 +192,7 @@ class LoginManagerTest extends TestCase
         $configManager = new \FakeConfigManager([
             'resource.ban_file' => $this->banFile,
         ]);
-        $loginManager = new LoginManager($this->globals, $configManager, null);
+        $loginManager = new LoginManager($configManager, null);
         $loginManager->checkLoginState([], '');
 
         $this->assertFalse($loginManager->isLoggedIn());
-- 
cgit v1.2.3