Refactor login / ban authentication steps

Relates to https://github.com/shaarli/Shaarli/issues/324

Added:
- Add the `LoginManager` class to manage logins and bans

Changed:
- Refactor IP ban management
- Simplify logic
- Avoid using globals, inject dependencies

Fixed:
- Use `ban_duration` instead of `ban_after` when setting a new ban

Signed-off-by: VirtualTam <virtualtam@flibidi.net>

6 files changed, 385 insertions, 103 deletions

diff --git a/application/LoginManager.php b/application/LoginManager.php
new file mode 100644
index 00000000..397bc6e3
--- /dev/null
+++ b/application/LoginManager.php
@@ -0,0 +1,134 @@
+<?php
+namespace Shaarli;
+
+/**
+ * User login management
+ */
+class LoginManager
+{
+    protected $globals = [];
+    protected $configManager = null;
+    protected $banFile = '';
+
+    /**
+     * Constructor
+     *
+     * @param array         $globals       The $GLOBALS array (reference)
+     * @param ConfigManager $configManager Configuration Manager instance.
+     */
+    public function __construct(& $globals, $configManager)
+    {
+        $this->globals = &$globals;
+        $this->configManager = $configManager;
+        $this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php');
+        $this->readBanFile();
+    }
+
+    /**
+     * Read a file containing banned IPs
+     */
+    protected function readBanFile()
+    {
+        if (! file_exists($this->banFile)) {
+            return;
+        }
+        include $this->banFile;
+    }
+
+    /**
+     * Write the banned IPs to a file
+     */
+    protected function writeBanFile()
+    {
+        if (! array_key_exists('IPBANS', $this->globals)) {
+            return;
+        }
+        file_put_contents(
+            $this->banFile,
+            "<?php\n\$GLOBALS['IPBANS']=" . var_export($this->globals['IPBANS'], true) . ";\n?>"
+        );
+    }
+
+    /**
+     * Handle a failed login and ban the IP after too many failed attempts
+     *
+     * @param array $server The $_SERVER array
+     */
+    public function handleFailedLogin($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+        $trusted = $this->configManager->get('security.trusted_proxies', []);
+
+        if (in_array($ip, $trusted)) {
+            $ip = getIpAddressFromProxy($server, $trusted);
+            if (! $ip) {
+                // the IP is behind a trusted forward proxy, but is not forwarded
+                // in the HTTP headers, so we do nothing
+                return;
+            }
+        }
+
+        // increment the fail count for this IP
+        if (isset($this->globals['IPBANS']['FAILURES'][$ip])) {
+            $this->globals['IPBANS']['FAILURES'][$ip]++;
+        } else {
+            $this->globals['IPBANS']['FAILURES'][$ip] = 1;
+        }
+
+        if ($this->globals['IPBANS']['FAILURES'][$ip] >= $this->configManager->get('security.ban_after')) {
+            $this->globals['IPBANS']['BANS'][$ip] = time() + $this->configManager->get('security.ban_duration', 1800);
+            logm(
+                $this->configManager->get('resource.log'),
+                $server['REMOTE_ADDR'],
+                'IP address banned from login'
+            );
+        }
+        $this->writeBanFile();
+    }
+
+    /**
+     * Handle a successful login
+     *
+     * @param array $server The $_SERVER array
+     */
+    public function handleSuccessfulLogin($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+        // FIXME unban when behind a trusted proxy?
+
+        unset($this->globals['IPBANS']['FAILURES'][$ip]);
+        unset($this->globals['IPBANS']['BANS'][$ip]);
+
+        $this->writeBanFile();
+    }
+
+    /**
+     * Check if the user can login from this IP
+     *
+     * @param array $server The $_SERVER array
+     *
+     * @return bool true if the user is allowed to login
+     */
+    public function canLogin($server)
+    {
+        $ip = $server['REMOTE_ADDR'];
+
+        if (! isset($this->globals['IPBANS']['BANS'][$ip])) {
+            // the user is not banned
+            return true;
+        }
+
+        if ($this->globals['IPBANS']['BANS'][$ip] > time()) {
+            // the user is still banned
+            return false;
+        }
+
+        // the ban has expired, the user can attempt to log in again
+        logm($this->configManager->get('resource.log'), $server['REMOTE_ADDR'], 'Ban lifted.');
+        unset($this->globals['IPBANS']['FAILURES'][$ip]);
+        unset($this->globals['IPBANS']['BANS'][$ip]);
+
+        $this->writeBanFile();
+        return true;
+    }
+}
diff --git a/index.php b/index.php
index 067b8fcb..91c3f07e 100644
--- a/index.php
+++ b/index.php
@@ -78,6 +78,7 @@ require_once 'application/Updater.php';
 use \Shaarli\Languages;
 use \Shaarli\ThemeUtils;
 use \Shaarli\Config\ConfigManager;
+use \Shaarli\LoginManager;
 use \Shaarli\SessionManager;
 
 // Ensure the PHP version is supported
@@ -122,6 +123,7 @@ if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli']))
 }
 
 $conf = new ConfigManager();
+$loginManager = new LoginManager($GLOBALS, $conf);
 $sessionManager = new SessionManager($_SESSION, $conf);
 
 // LC_MESSAGES isn't defined without php-intl, in this case use LC_COLLATE locale instead.
@@ -293,108 +295,22 @@ function logout() {
     setcookie('shaarli_staySignedIn', FALSE, 0, WEB_PATH);
 }
 
-
-// ------------------------------------------------------------------------------------------
-// Brute force protection system
-// Several consecutive failed logins will ban the IP address for 30 minutes.
-if (!is_file($conf->get('resource.ban_file', 'data/ipbans.php'))) {
-    // FIXME! globals
-    file_put_contents(
-        $conf->get('resource.ban_file', 'data/ipbans.php'),
-        "<?php\n\$GLOBALS['IPBANS']=".var_export(array('FAILURES'=>array(),'BANS'=>array()),true).";\n?>"
-    );
-}
-include $conf->get('resource.ban_file', 'data/ipbans.php');
-/**
- * Signal a failed login. Will ban the IP if too many failures:
- *
- * @param ConfigManager $conf Configuration Manager instance.
- */
-function ban_loginFailed($conf)
-{
-    $ip = $_SERVER['REMOTE_ADDR'];
-    $trusted = $conf->get('security.trusted_proxies', array());
-    if (in_array($ip, $trusted)) {
-        $ip = getIpAddressFromProxy($_SERVER, $trusted);
-        if (!$ip) {
-            return;
-        }
-    }
-    $gb = $GLOBALS['IPBANS'];
-    if (! isset($gb['FAILURES'][$ip])) {
-        $gb['FAILURES'][$ip]=0;
-    }
-    $gb['FAILURES'][$ip]++;
-    if ($gb['FAILURES'][$ip] > ($conf->get('security.ban_after') - 1))
-    {
-        $gb['BANS'][$ip] = time() + $conf->get('security.ban_after', 1800);
-        logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'IP address banned from login');
-    }
-    $GLOBALS['IPBANS'] = $gb;
-    file_put_contents(
-        $conf->get('resource.ban_file', 'data/ipbans.php'),
-        "<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
-    );
-}
-
-/**
- * Signals a successful login. Resets failed login counter.
- *
- * @param ConfigManager $conf Configuration Manager instance.
- */
-function ban_loginOk($conf)
-{
-    $ip = $_SERVER['REMOTE_ADDR'];
-    $gb = $GLOBALS['IPBANS'];
-    unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]);
-    $GLOBALS['IPBANS'] = $gb;
-    file_put_contents(
-        $conf->get('resource.ban_file', 'data/ipbans.php'),
-        "<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
-    );
-}
-
-/**
- * Checks if the user CAN login. If 'true', the user can try to login.
- *
- * @param ConfigManager $conf Configuration Manager instance.
- *
- * @return bool: true if the user is allowed to login.
- */
-function ban_canLogin($conf)
-{
-    $ip=$_SERVER["REMOTE_ADDR"]; $gb=$GLOBALS['IPBANS'];
-    if (isset($gb['BANS'][$ip]))
-    {
-        // User is banned. Check if the ban has expired:
-        if ($gb['BANS'][$ip]<=time())
-        {   // Ban expired, user can try to login again.
-            logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'Ban lifted.');
-            unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]);
-            file_put_contents(
-                $conf->get('resource.ban_file', 'data/ipbans.php'),
-                "<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
-            );
-            return true; // Ban has expired, user can login.
-        }
-        return false; // User is banned.
-    }
-    return true; // User is not banned.
-}
-
 // ------------------------------------------------------------------------------------------
 // Process login form: Check if login/password is correct.
 if (isset($_POST['login']))
 {
-    if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.'));
+    if (! $loginManager->canLogin($_SERVER)) {
+        die(t('I said: NO. You are banned for the moment. Go away.'));
+    }
     if (isset($_POST['password'])
         && $sessionManager->checkToken($_POST['token'])
         && (check_auth($_POST['login'], $_POST['password'], $conf))
-    ) {   // Login/password is OK.
-        ban_loginOk($conf);
+    ) {
+        // Login/password is OK.
+        $loginManager->handleSuccessfulLogin($_SERVER);
+
         // If user wants to keep the session cookie even after the browser closes:
-        if (!empty($_POST['longlastingsession']))
-        {
+        if (!empty($_POST['longlastingsession'])) {
             $_SESSION['longlastingsession'] = 31536000; // (31536000 seconds = 1 year)
             $expiration = time() + $_SESSION['longlastingsession']; // calculate relative cookie expiration (1 year from now)
             setcookie('shaarli_staySignedIn', STAY_SIGNED_IN_TOKEN, $expiration, WEB_PATH);
@@ -437,10 +353,8 @@ if (isset($_POST['login']))
             }
         }
         header('Location: ?'); exit;
-    }
-    else
-    {
-        ban_loginFailed($conf);
+    } else {
+        $loginManager->handleFailedLogin($_SERVER);
         $redir = '&username='. urlencode($_POST['login']);
         if (isset($_GET['post'])) {
             $redir .= '&post=' . urlencode($_GET['post']);
@@ -684,8 +598,9 @@ function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) {
  * @param LinkDB         $LINKSDB
  * @param History        $history        instance
  * @param SessionManager $sessionManager SessionManager instance
+ * @param LoginManager   $loginManager   LoginManager instance
  */
-function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager)
+function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $loginManager)
 {
     $updater = new Updater(
         read_updates_file($conf->get('resource.updates')),
@@ -761,6 +676,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager)
         $PAGE->assign('returnurl',(isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']):''));
         // add default state of the 'remember me' checkbox
         $PAGE->assign('remember_user_default', $conf->get('privacy.remember_user_default'));
+        $PAGE->assign('user_can_login', $loginManager->canLogin($_SERVER));
         $PAGE->renderPage('loginform');
         exit;
     }
@@ -2330,7 +2246,7 @@ $response = $app->run(true);
 if ($response->getStatusCode() == 404 && strpos($_SERVER['REQUEST_URI'], '/api/v1') === false) {
     // We use UTF-8 for proper international characters handling.
     header('Content-Type: text/html; charset=utf-8');
-    renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager);
+    renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager, $loginManager);
 } else {
     $app->respond($response);
 }
diff --git a/tests/LoginManagerTest.php b/tests/LoginManagerTest.php
new file mode 100644
index 00000000..4159038e
--- /dev/null
+++ b/tests/LoginManagerTest.php
@@ -0,0 +1,199 @@
+<?php
+namespace Shaarli;
+
+require_once 'tests/utils/FakeConfigManager.php';
+use \PHPUnit\Framework\TestCase;
+
+/**
+ * Test coverage for LoginManager
+ */
+class LoginManagerTest extends TestCase
+{
+    protected $configManager = null;
+    protected $loginManager = null;
+    protected $banFile = 'sandbox/ipbans.php';
+    protected $logFile = 'sandbox/shaarli.log';
+    protected $globals = [];
+    protected $ipAddr = '127.0.0.1';
+    protected $server = [];
+    protected $trustedProxy = '10.1.1.100';
+
+    /**
+     * Prepare or reset test resources
+     */
+    public function setUp()
+    {
+        if (file_exists($this->banFile)) {
+            unlink($this->banFile);
+        }
+
+        $this->configManager = new \FakeConfigManager([
+            'resource.ban_file' => $this->banFile,
+            'resource.log' => $this->logFile,
+            'security.ban_after' => 4,
+            'security.ban_duration' => 3600,
+            'security.trusted_proxies' => [$this->trustedProxy],
+        ]);
+
+        $this->globals = &$GLOBALS;
+        unset($this->globals['IPBANS']);
+
+        $this->loginManager = new LoginManager($this->globals, $this->configManager);
+        $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);
+        $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]);
+    }
+
+    /**
+     * Record a failed login attempt - IP behind a trusted proxy
+     */
+    public function testHandleFailedLoginBehindTrustedProxy()
+    {
+        $server = [
+            'REMOTE_ADDR' => $this->trustedProxy,
+            '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]);
+    }
+
+    /**
+     * Record a failed login attempt - IP behind a trusted proxy but not forwarded
+     */
+    public function testHandleFailedLoginBehindTrustedProxyNoIp()
+    {
+        $server = [
+            '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));
+    }
+
+    /**
+     * Nothing to do
+     */
+    public function testHandleSuccessfulLogin()
+    {
+        $this->assertTrue($this->loginManager->canLogin($this->server));
+
+        $this->loginManager->handleSuccessfulLogin($this->server);
+        $this->assertTrue($this->loginManager->canLogin($this->server));
+    }
+
+    /**
+     * Erase failure records after successfully logging in from this IP
+     */
+    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->assertTrue($this->loginManager->canLogin($this->server));
+        $this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
+        $this->assertFalse(isset($this->globals['IPBANS']['BANS'][$this->ipAddr]));
+    }
+
+    /**
+     * The IP is not banned
+     */
+    public function testCanLoginIpNotBanned()
+    {
+        $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));
+    }
+}
diff --git a/tests/utils/FakeConfigManager.php b/tests/utils/FakeConfigManager.php
index f29760cb..85434de7 100644
--- a/tests/utils/FakeConfigManager.php
+++ b/tests/utils/FakeConfigManager.php
@@ -5,8 +5,41 @@
  */
 class FakeConfigManager
 {
-    public static function get($key)
+    protected $values = [];
+
+    /**
+     * Initialize with test values
+     *
+     * @param array $values Initial values
+     */
+    public function __construct($values = [])
+    {
+        $this->values = $values;
+    }
+
+    /**
+     * Set a given value
+     *
+     * @param string $key   Key of the value to set
+     * @param mixed  $value Value to set
+     */
+    public function set($key, $value)
+    {
+        $this->values[$key] = $value;
+    }
+
+    /**
+     * Get a given configuration value
+     *
+     * @param string $key Index of the value to retrieve
+     *
+     * @return mixed The value if set, else the name of the key
+     */
+    public function get($key)
     {
+        if (isset($this->values[$key])) {
+            return $this->values[$key];
+        }
         return $key;
     }
 }
diff --git a/tpl/default/loginform.html b/tpl/default/loginform.html
index 5777a218..d481f452 100644
--- a/tpl/default/loginform.html
+++ b/tpl/default/loginform.html
@@ -5,7 +5,7 @@
 </head>
 <body>
 {include="page.header"}
-{if="!ban_canLogin($conf)"}
+{if="!$user_can_login"}
 <div class="pure-g pure-alert pure-alert-error pure-alert-closable center">
   <div class="pure-u-2-24"></div>
   <div class="pure-u-20-24">
diff --git a/tpl/vintage/loginform.html b/tpl/vintage/loginform.html
index 1becd44f..2c9b710e 100644
--- a/tpl/vintage/loginform.html
+++ b/tpl/vintage/loginform.html
@@ -2,7 +2,7 @@
 <html>
 <head>{include="includes"}</head>
 <body
-{if="ban_canLogin($conf)"}
+{if="$user_can_login"}
   {if="empty($username)"}
     onload="document.loginform.login.focus();"
   {else}