Merge pull request #1005 from virtualtam/refactor/authentication

Refactor session management utilities

6 files changed, 272 insertions, 138 deletions

diff --git a/application/PageBuilder.php b/application/PageBuilder.php
index af290671..468f144b 100644
--- a/application/PageBuilder.php
+++ b/application/PageBuilder.php
@@ -32,12 +32,14 @@ class PageBuilder
      *
      * @param ConfigManager $conf   Configuration Manager instance (reference).
      * @param LinkDB        $linkDB instance.
+     * @param string        $token  Session token
      */
-    public function __construct(&$conf, $linkDB = null)
+    public function __construct(&$conf, $linkDB = null, $token = null)
     {
         $this->tpl = false;
         $this->conf = $conf;
         $this->linkDB = $linkDB;
+        $this->token = $token;
     }
 
     /**
@@ -92,7 +94,7 @@ class PageBuilder
         $this->tpl->assign('showatom', $this->conf->get('feed.show_atom', true));
         $this->tpl->assign('feed_type', $this->conf->get('feed.show_atom', true) !== false ? 'atom' : 'rss');
         $this->tpl->assign('hide_timestamps', $this->conf->get('privacy.hide_timestamps', false));
-        $this->tpl->assign('token', getToken($this->conf));
+        $this->tpl->assign('token', $this->token);
 
         if ($this->linkDB !== null) {
             $this->tpl->assign('tags', $this->linkDB->linksCountPerTag());
diff --git a/application/SessionManager.php b/application/SessionManager.php
new file mode 100644
index 00000000..3aa4ddfc
--- /dev/null
+++ b/application/SessionManager.php
@@ -0,0 +1,83 @@
+<?php
+namespace Shaarli;
+
+/**
+ * Manages the server-side session
+ */
+class SessionManager
+{
+    protected $session = [];
+
+    /**
+     * Constructor
+     *
+     * @param array         $session The $_SESSION array (reference)
+     * @param ConfigManager $conf    ConfigManager instance (reference)
+     */
+    public function __construct(& $session, & $conf)
+    {
+        $this->session = &$session;
+        $this->conf = &$conf;
+    }
+
+    /**
+     * Generates a session token
+     *
+     * @return string token
+     */
+    public function generateToken()
+    {
+        $token = sha1(uniqid('', true) .'_'. mt_rand() . $this->conf->get('credentials.salt'));
+        $this->session['tokens'][$token] = 1;
+        return $token;
+    }
+
+    /**
+     * Checks the validity of a session token, and destroys it afterwards
+     *
+     * @param string $token The token to check
+     *
+     * @return bool true if the token is valid, else false
+     */
+    public function checkToken($token)
+    {
+        if (! isset($this->session['tokens'][$token])) {
+            // the token is wrong, or has already been used
+            return false;
+        }
+
+        // destroy the token to prevent future use
+        unset($this->session['tokens'][$token]);
+        return true;
+    }
+
+    /**
+     * Validate session ID to prevent Full Path Disclosure.
+     *
+     * See #298.
+     * The session ID's format depends on the hash algorithm set in PHP settings
+     *
+     * @param string $sessionId Session ID
+     *
+     * @return true if valid, false otherwise.
+     *
+     * @see http://php.net/manual/en/function.hash-algos.php
+     * @see http://php.net/manual/en/session.configuration.php
+     */
+    public static function checkId($sessionId)
+    {
+        if (empty($sessionId)) {
+            return false;
+        }
+
+        if (!$sessionId) {
+            return false;
+        }
+
+        if (!preg_match('/^[a-zA-Z0-9,-]{2,128}$/', $sessionId)) {
+            return false;
+        }
+
+        return true;
+    }
+}
diff --git a/application/Utils.php b/application/Utils.php
index 2f38a8de..97b12fcf 100644
--- a/application/Utils.php
+++ b/application/Utils.php
@@ -182,36 +182,6 @@ function generateLocation($referer, $host, $loopTerms = array())
 }
 
 /**
- * Validate session ID to prevent Full Path Disclosure.
- *
- * See #298.
- * The session ID's format depends on the hash algorithm set in PHP settings
- *
- * @param string $sessionId Session ID
- *
- * @return true if valid, false otherwise.
- *
- * @see http://php.net/manual/en/function.hash-algos.php
- * @see http://php.net/manual/en/session.configuration.php
- */
-function is_session_id_valid($sessionId)
-{
-    if (empty($sessionId)) {
-        return false;
-    }
-
-    if (!$sessionId) {
-        return false;
-    }
-
-    if (!preg_match('/^[a-zA-Z0-9,-]{2,128}$/', $sessionId)) {
-        return false;
-    }
-
-    return true;
-}
-
-/**
  * Sniff browser language to set the locale automatically.
  * Note that is may not work on your server if the corresponding locale is not installed.
  *
diff --git a/index.php b/index.php
index 1dc81843..e1516d37 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\SessionManager;
 
 // Ensure the PHP version is supported
 try {
@@ -115,12 +116,13 @@ if (session_id() == '') {
 }
 
 // Regenerate session ID if invalid or not defined in cookie.
-if (isset($_COOKIE['shaarli']) && !is_session_id_valid($_COOKIE['shaarli'])) {
+if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli'])) {
     session_regenerate_id(true);
     $_COOKIE['shaarli'] = session_id();
 }
 
 $conf = new ConfigManager();
+$sessionManager = new SessionManager($_SESSION, $conf);
 
 // Sniff browser language and set date format accordingly.
 if (isset($_SERVER['HTTP_ACCEPT_LANGUAGE'])) {
@@ -165,7 +167,7 @@ if (! is_file($conf->getConfigFileExt())) {
     }
 
     // Display the installation form if no existing config is found
-    install($conf);
+    install($conf, $sessionManager);
 }
 
 // a token depending of deployment salt, user password, and the current ip
@@ -381,7 +383,7 @@ if (isset($_POST['login']))
 {
     if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.'));
     if (isset($_POST['password'])
-        && tokenOk($_POST['token'])
+        && $sessionManager->checkToken($_POST['token'])
         && (check_auth($_POST['login'], $_POST['password'], $conf))
     ) {   // Login/password is OK.
         ban_loginOk($conf);
@@ -455,32 +457,6 @@ if (isset($_POST['login']))
 if (!isset($_SESSION['tokens'])) $_SESSION['tokens']=array();  // Token are attached to the session.
 
 /**
- * Returns a token.
- *
- * @param ConfigManager $conf Configuration Manager instance.
- *
- * @return string token.
- */
-function getToken($conf)
-{
-    $rnd = sha1(uniqid('', true) .'_'. mt_rand() . $conf->get('credentials.salt'));  // We generate a random string.
-    $_SESSION['tokens'][$rnd]=1;  // Store it on the server side.
-    return $rnd;
-}
-
-// Tells if a token is OK. Using this function will destroy the token.
-// true=token is OK.
-function tokenOk($token)
-{
-    if (isset($_SESSION['tokens'][$token]))
-    {
-        unset($_SESSION['tokens'][$token]); // Token is used: destroy it.
-        return true; // Token is OK.
-    }
-    return false; // Wrong token, or already used.
-}
-
-/**
  * Daily RSS feed: 1 RSS entry per day giving all the links on that day.
  * Gives the last 7 days (which have links).
  * This RSS feed cannot be filtered.
@@ -687,12 +663,13 @@ function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) {
 /**
  * Render HTML page (according to URL parameters and user rights)
  *
- * @param ConfigManager $conf          Configuration Manager instance.
- * @param PluginManager $pluginManager Plugin Manager instance,
- * @param LinkDB        $LINKSDB
- * @param History       $history       instance
+ * @param ConfigManager  $conf           Configuration Manager instance.
+ * @param PluginManager  $pluginManager  Plugin Manager instance,
+ * @param LinkDB         $LINKSDB
+ * @param History        $history        instance
+ * @param SessionManager $sessionManager SessionManager instance
  */
-function renderPage($conf, $pluginManager, $LINKSDB, $history)
+function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager)
 {
     $updater = new Updater(
         read_updates_file($conf->get('resource.updates')),
@@ -713,7 +690,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
         die($e->getMessage());
     }
 
-    $PAGE = new PageBuilder($conf, $LINKSDB);
+    $PAGE = new PageBuilder($conf, $LINKSDB, $sessionManager->generateToken());
     $PAGE->assign('linkcount', count($LINKSDB));
     $PAGE->assign('privateLinkcount', count_private($LINKSDB));
     $PAGE->assign('plugin_errors', $pluginManager->getErrors());
@@ -1109,13 +1086,13 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
 
         if (!empty($_POST['setpassword']) && !empty($_POST['oldpassword']))
         {
-            if (!tokenOk($_POST['token'])) die(t('Wrong token.')); // Go away!
+            if (!$sessionManager->checkToken($_POST['token'])) die(t('Wrong token.')); // Go away!
 
             // Make sure old password is correct.
             $oldhash = sha1($_POST['oldpassword'].$conf->get('credentials.login').$conf->get('credentials.salt'));
             if ($oldhash!= $conf->get('credentials.hash')) {
                 echo '<script>alert("'. t('The old password is not correct.') .'");document.location=\'?do=changepasswd\';</script>';
-                exit; 
+                exit;
             }
             // Save new password
             // Salt renders rainbow-tables attacks useless.
@@ -1149,7 +1126,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
     {
         if (!empty($_POST['title']) )
         {
-            if (!tokenOk($_POST['token'])) {
+            if (!$sessionManager->checkToken($_POST['token'])) {
                 die(t('Wrong token.')); // Go away!
             }
             $tz = 'UTC';
@@ -1225,7 +1202,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
             exit;
         }
 
-        if (!tokenOk($_POST['token'])) {
+        if (!$sessionManager->checkToken($_POST['token'])) {
             die(t('Wrong token.'));
         }
 
@@ -1255,7 +1232,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
     if (isset($_POST['save_edit']))
     {
         // Go away!
-        if (! tokenOk($_POST['token'])) {
+        if (! $sessionManager->checkToken($_POST['token'])) {
             die(t('Wrong token.'));
         }
 
@@ -1355,7 +1332,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
     // -------- User clicked the "Delete" button when editing a link: Delete link from database.
     if ($targetPage == Router::$PAGE_DELETELINK)
     {
-        if (! tokenOk($_GET['token'])) {
+        if (! $sessionManager->checkToken($_GET['token'])) {
             die(t('Wrong token.'));
         }
 
@@ -1572,7 +1549,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
             echo '<script>alert("'. $msg .'");document.location=\'?do='.Router::$PAGE_IMPORT .'\';</script>';
             exit;
         }
-        if (! tokenOk($_POST['token'])) {
+        if (! $sessionManager->checkToken($_POST['token'])) {
             die('Wrong token.');
         }
         $status = NetscapeBookmarkUtils::import(
@@ -1639,7 +1616,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history)
     // Get a fresh token
     if ($targetPage == Router::$GET_TOKEN) {
         header('Content-Type:text/plain');
-        echo getToken($conf);
+        echo $sessionManager->generateToken($conf);
         exit;
     }
 
@@ -1965,10 +1942,10 @@ function lazyThumbnail($conf, $url,$href=false)
  * Installation
  * This function should NEVER be called if the file data/config.php exists.
  *
- * @param ConfigManager $conf Configuration Manager instance.
+ * @param ConfigManager  $conf           Configuration Manager instance.
+ * @param SessionManager $sessionManager SessionManager instance
  */
-function install($conf)
-{
+function install($conf, $sessionManager) {
     // On free.fr host, make sure the /sessions directory exists, otherwise login will not work.
     if (endsWith($_SERVER['HTTP_HOST'],'.free.fr') && !is_dir($_SERVER['DOCUMENT_ROOT'].'/sessions')) mkdir($_SERVER['DOCUMENT_ROOT'].'/sessions',0705);
 
@@ -2051,7 +2028,7 @@ function install($conf)
         exit;
     }
 
-    $PAGE = new PageBuilder($conf);
+    $PAGE = new PageBuilder($conf, null, $sessionManager->generateToken());
     list($continents, $cities) = generateTimeZoneData(timezone_identifiers_list(), date_default_timezone_get());
     $PAGE->assign('continents', $continents);
     $PAGE->assign('cities', $cities);
@@ -2328,7 +2305,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);
+    renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager);
 } else {
     $app->respond($response);
 }
diff --git a/tests/SessionManagerTest.php b/tests/SessionManagerTest.php
new file mode 100644
index 00000000..a92c3ccc
--- /dev/null
+++ b/tests/SessionManagerTest.php
@@ -0,0 +1,160 @@
+<?php
+// Initialize reference data _before_ PHPUnit starts a session
+require_once 'tests/utils/ReferenceSessionIdHashes.php';
+ReferenceSessionIdHashes::genAllHashes();
+
+use \Shaarli\SessionManager;
+use \PHPUnit\Framework\TestCase;
+
+
+/**
+ * Fake ConfigManager
+ */
+class FakeConfigManager
+{
+    public static function get($key)
+    {
+        return $key;
+    }
+}
+
+
+/**
+ * Test coverage for SessionManager
+ */
+class SessionManagerTest extends TestCase
+{
+    // Session ID hashes
+    protected static $sidHashes = null;
+
+    /**
+     * Assign reference data
+     */
+    public static function setUpBeforeClass()
+    {
+        self::$sidHashes = ReferenceSessionIdHashes::getHashes();
+    }
+
+    /**
+     * Generate a session token
+     */
+    public function testGenerateToken()
+    {
+        $session = [];
+        $conf = new FakeConfigManager();
+        $sessionManager = new SessionManager($session, $conf);
+
+        $token = $sessionManager->generateToken();
+
+        $this->assertEquals(1, $session['tokens'][$token]);
+        $this->assertEquals(40, strlen($token));
+    }
+
+    /**
+     * Check a session token
+     */
+    public function testCheckToken()
+    {
+        $token = '4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b';
+        $session = [
+            'tokens' => [
+                $token => 1,
+            ],
+        ];
+        $conf = new FakeConfigManager();
+        $sessionManager = new SessionManager($session, $conf);
+
+
+        // check and destroy the token
+        $this->assertTrue($sessionManager->checkToken($token));
+        $this->assertFalse(isset($session['tokens'][$token]));
+
+        // ensure the token has been destroyed
+        $this->assertFalse($sessionManager->checkToken($token));
+    }
+
+    /**
+     * Generate and check a session token
+     */
+    public function testGenerateAndCheckToken()
+    {
+        $session = [];
+        $conf = new FakeConfigManager();
+        $sessionManager = new SessionManager($session, $conf);
+
+        $token = $sessionManager->generateToken();
+
+        // ensure a token has been generated
+        $this->assertEquals(1, $session['tokens'][$token]);
+        $this->assertEquals(40, strlen($token));
+
+        // check and destroy the token
+        $this->assertTrue($sessionManager->checkToken($token));
+        $this->assertFalse(isset($session['tokens'][$token]));
+
+        // ensure the token has been destroyed
+        $this->assertFalse($sessionManager->checkToken($token));
+    }
+
+    /**
+     * Check an invalid session token
+     */
+    public function testCheckInvalidToken()
+    {
+        $session = [];
+        $conf = new FakeConfigManager();
+        $sessionManager = new SessionManager($session, $conf);
+
+        $this->assertFalse($sessionManager->checkToken('4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b'));
+    }
+
+    /**
+     * Test SessionManager::checkId with a valid ID - TEST ALL THE HASHES!
+     *
+     * This tests extensively covers all hash algorithms / bit representations
+     */
+    public function testIsAnyHashSessionIdValid()
+    {
+        foreach (self::$sidHashes as $algo => $bpcs) {
+            foreach ($bpcs as $bpc => $hash) {
+                $this->assertTrue(SessionManager::checkId($hash));
+            }
+        }
+    }
+
+    /**
+     * Test checkId with a valid ID - SHA-1 hashes
+     */
+    public function testIsSha1SessionIdValid()
+    {
+        $this->assertTrue(SessionManager::checkId(sha1('shaarli')));
+    }
+
+    /**
+     * Test checkId with a valid ID - SHA-256 hashes
+     */
+    public function testIsSha256SessionIdValid()
+    {
+        $this->assertTrue(SessionManager::checkId(hash('sha256', 'shaarli')));
+    }
+
+    /**
+     * Test checkId with a valid ID - SHA-512 hashes
+     */
+    public function testIsSha512SessionIdValid()
+    {
+        $this->assertTrue(SessionManager::checkId(hash('sha512', 'shaarli')));
+    }
+
+    /**
+     * Test checkId with invalid IDs.
+     */
+    public function testIsSessionIdInvalid()
+    {
+        $this->assertFalse(SessionManager::checkId(''));
+        $this->assertFalse(SessionManager::checkId([]));
+        $this->assertFalse(
+            SessionManager::checkId('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=')
+        );
+    }
+}
diff --git a/tests/UtilsTest.php b/tests/UtilsTest.php
index 840eaf21..6cd37a7a 100644
--- a/tests/UtilsTest.php
+++ b/tests/UtilsTest.php
@@ -5,10 +5,6 @@
 
 require_once 'application/Utils.php';
 require_once 'application/Languages.php';
-require_once 'tests/utils/ReferenceSessionIdHashes.php';
-
-// Initialize reference data before PHPUnit starts a session
-ReferenceSessionIdHashes::genAllHashes();
 
 
 /**
@@ -16,9 +12,6 @@ ReferenceSessionIdHashes::genAllHashes();
  */
 class UtilsTest extends PHPUnit_Framework_TestCase
 {
-    // Session ID hashes
-    protected static $sidHashes = null;
-
     // Log file
     protected static $testLogFile = 'tests.log';
 
@@ -30,13 +23,11 @@ class UtilsTest extends PHPUnit_Framework_TestCase
      */
     protected static $defaultTimeZone;
 
-
     /**
      * Assign reference data
      */
     public static function setUpBeforeClass()
     {
-        self::$sidHashes = ReferenceSessionIdHashes::getHashes();
         self::$defaultTimeZone = date_default_timezone_get();
         // Timezone without DST for test consistency
         date_default_timezone_set('Africa/Nairobi');
@@ -221,57 +212,8 @@ class UtilsTest extends PHPUnit_Framework_TestCase
         $this->assertEquals('?', generateLocation($ref, 'localhost'));
     }
 
-    /**
-     * Test is_session_id_valid with a valid ID - TEST ALL THE HASHES!
-     *
-     * This tests extensively covers all hash algorithms / bit representations
-     */
-    public function testIsAnyHashSessionIdValid()
-    {
-        foreach (self::$sidHashes as $algo => $bpcs) {
-            foreach ($bpcs as $bpc => $hash) {
-                $this->assertTrue(is_session_id_valid($hash));
-            }
-        }
-    }
 
     /**
-     * Test is_session_id_valid with a valid ID - SHA-1 hashes
-     */
-    public function testIsSha1SessionIdValid()
-    {
-        $this->assertTrue(is_session_id_valid(sha1('shaarli')));
-    }
-
-    /**
-     * Test is_session_id_valid with a valid ID - SHA-256 hashes
-     */
-    public function testIsSha256SessionIdValid()
-    {
-        $this->assertTrue(is_session_id_valid(hash('sha256', 'shaarli')));
-    }
-
-    /**
-     * Test is_session_id_valid with a valid ID - SHA-512 hashes
-     */
-    public function testIsSha512SessionIdValid()
-    {
-        $this->assertTrue(is_session_id_valid(hash('sha512', 'shaarli')));
-    }
-
-    /**
-     * Test is_session_id_valid with invalid IDs.
-     */
-    public function testIsSessionIdInvalid()
-    {
-        $this->assertFalse(is_session_id_valid(''));
-        $this->assertFalse(is_session_id_valid(array()));
-        $this->assertFalse(
-            is_session_id_valid('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=')
-        );
-    }
-    
-    /**
      * Test generateSecretApi.
      */
     public function testGenerateSecretApi()