Refactor session and cookie timeout control

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

4 files changed, 224 insertions, 57 deletions

diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php
index e7b9b21e..27247f3f 100644
--- a/application/security/LoginManager.php
+++ b/application/security/LoginManager.php
@@ -49,13 +49,12 @@ class LoginManager
      * Check user session state and validity (expiration)
      *
      * @param array  $cookie     The $_COOKIE array
-     * @param string $webPath    Path on the server in which the cookie will be available on
      * @param string $clientIpId Client IP address identifier
      * @param string $token      Session token
      *
      * @return bool true if the user session is valid, false otherwise
      */
-    public function checkLoginState($cookie, $webPath, $clientIpId, $token)
+    public function checkLoginState($cookie, $clientIpId, $token)
     {
         if (! $this->configManager->exists('credentials.login')) {
             // Shaarli is not configured yet
@@ -73,7 +72,7 @@ class LoginManager
         if ($this->sessionManager->hasSessionExpired()
             || $this->sessionManager->hasClientIpChanged($clientIpId)
         ) {
-            $this->sessionManager->logout($webPath);
+            $this->sessionManager->logout();
             $this->isLoggedIn = false;
             return;
         }
diff --git a/application/security/SessionManager.php b/application/security/SessionManager.php
index 6f004b24..0dcd7f90 100644
--- a/application/security/SessionManager.php
+++ b/application/security/SessionManager.php
@@ -9,7 +9,10 @@ use Shaarli\Config\ConfigManager;
 class SessionManager
 {
     /** @var int Session expiration timeout, in seconds */
-    public static $INACTIVITY_TIMEOUT = 3600;
+    public static $SHORT_TIMEOUT = 3600;    // 1 hour
+
+    /** @var int Session expiration timeout, in seconds */
+    public static $LONG_TIMEOUT = 31536000; // 1 year
 
     /** @var string Name of the cookie set after logging in **/
     public static $LOGGED_IN_COOKIE = 'shaarli_staySignedIn';
@@ -20,6 +23,9 @@ class SessionManager
     /** @var ConfigManager Configuration Manager instance **/
     protected $conf = null;
 
+    /** @var bool Whether the user should stay signed in (LONG_TIMEOUT) */
+    protected $staySignedIn = false;
+
     /**
      * Constructor
      *
@@ -33,6 +39,16 @@ class SessionManager
     }
 
     /**
+     * Define whether the user should stay signed in across browser sessions
+     *
+     * @param bool $staySignedIn Keep the user signed in
+     */
+    public function setStaySignedIn($staySignedIn)
+    {
+        $this->staySignedIn = $staySignedIn;
+    }
+
+    /**
      * Generates a session token
      *
      * @return string token
@@ -104,7 +120,7 @@ class SessionManager
         $this->session['uid'] = sha1(uniqid('', true) . '_' . mt_rand());
         $this->session['ip'] = $clientIpId;
         $this->session['username'] = $this->conf->get('credentials.login');
-        $this->session['expires_on'] = time() + self::$INACTIVITY_TIMEOUT;
+        $this->extendTimeValidityBy(self::$SHORT_TIMEOUT);
     }
 
     /**
@@ -112,12 +128,24 @@ class SessionManager
      */
     public function extendSession()
     {
-        if (! empty($this->session['longlastingsession'])) {
-            // "Stay signed in" is enabled
-            $this->session['expires_on'] = time() + $this->session['longlastingsession'];
-            return;
+        if ($this->staySignedIn) {
+            return $this->extendTimeValidityBy(self::$LONG_TIMEOUT);
         }
-        $this->session['expires_on'] = time() + self::$INACTIVITY_TIMEOUT;
+        return $this->extendTimeValidityBy(self::$SHORT_TIMEOUT);
+    }
+
+    /**
+     * Extend expiration time
+     *
+     * @param int $duration Expiration time extension (seconds)
+     *
+     * @return int New session expiration time
+     */
+    protected function extendTimeValidityBy($duration)
+    {
+        $expirationTime = time() + $duration;
+        $this->session['expires_on'] = $expirationTime;
+        return $expirationTime;
     }
 
     /**
@@ -125,19 +153,17 @@ class SessionManager
      *
      * See:
      * - https://secure.php.net/manual/en/function.setcookie.php
-     *
-     * @param string $webPath path on the server in which the cookie will be available on
      */
-    public function logout($webPath)
+    public function logout()
     {
         if (isset($this->session)) {
             unset($this->session['uid']);
             unset($this->session['ip']);
+            unset($this->session['expires_on']);
             unset($this->session['username']);
             unset($this->session['visibility']);
             unset($this->session['untaggedonly']);
         }
-        setcookie(self::$LOGGED_IN_COOKIE, 'false', 0, $webPath);
     }
 
     /**
diff --git a/index.php b/index.php
index 139812d7..8e3bade0 100644
--- a/index.php
+++ b/index.php
@@ -179,7 +179,7 @@ if (! is_file($conf->getConfigFileExt())) {
 // a token depending of deployment salt, user password, and the current ip
 define('STAY_SIGNED_IN_TOKEN', sha1($conf->get('credentials.hash') . $_SERVER['REMOTE_ADDR'] . $conf->get('credentials.salt')));
 
-$loginManager->checkLoginState($_COOKIE, WEB_PATH, $clientIpId, STAY_SIGNED_IN_TOKEN);
+$loginManager->checkLoginState($_COOKIE, $clientIpId, STAY_SIGNED_IN_TOKEN);
 
 /**
  * Adapter function to ensure compatibility with third-party templates
@@ -205,31 +205,35 @@ if (isset($_POST['login'])) {
         && $sessionManager->checkToken($_POST['token'])
         && $loginManager->checkCredentials($_SERVER['REMOTE_ADDR'], $clientIpId, $_POST['login'], $_POST['password'])
     ) {
-        // Login/password is OK.
         $loginManager->handleSuccessfulLogin($_SERVER);
 
-        // If user wants to keep the session cookie even after the browser closes:
-        if (!empty($_POST['longlastingsession'])) {
-            $_SESSION['longlastingsession'] = 31536000; // (31536000 seconds = 1 year)
-            $expiration = time() + $_SESSION['longlastingsession']; // calculate relative cookie expiration (1 year from now)
-            setcookie($sessionManager::$LOGGED_IN_COOKIE, STAY_SIGNED_IN_TOKEN, $expiration, WEB_PATH);
-            $_SESSION['expires_on'] = $expiration;  // Set session expiration on server-side.
-
-            $cookiedir = '';
-            if (dirname($_SERVER['SCRIPT_NAME']) != '/') {
-                $cookiedir = dirname($_SERVER["SCRIPT_NAME"]) . '/';
-            }
-            session_set_cookie_params($_SESSION['longlastingsession'],$cookiedir,$_SERVER['SERVER_NAME']); // Set session cookie expiration on client side
+        $cookiedir = '';
+        if (dirname($_SERVER['SCRIPT_NAME']) != '/') {
             // Note: Never forget the trailing slash on the cookie path!
-            session_regenerate_id(true);  // Send cookie with new expiration date to browser.
+            $cookiedir = dirname($_SERVER["SCRIPT_NAME"]) . '/';
         }
-        else // Standard session expiration (=when browser closes)
-        {
-            $cookiedir = ''; if(dirname($_SERVER['SCRIPT_NAME'])!='/') $cookiedir=dirname($_SERVER["SCRIPT_NAME"]).'/';
-            session_set_cookie_params(0,$cookiedir,$_SERVER['SERVER_NAME']); // 0 means "When browser closes"
-            session_regenerate_id(true);
+
+        if (!empty($_POST['longlastingsession'])) {
+            // Keep the session cookie even after the browser closes
+            $sessionManager->setStaySignedIn(true);
+            $expirationTime = $sessionManager->extendSession();
+
+            setcookie(
+                $sessionManager::$LOGGED_IN_COOKIE,
+                STAY_SIGNED_IN_TOKEN,
+                $expirationTime,
+                WEB_PATH
+            );
+
+        } else {
+            // Standard session expiration (=when browser closes)
+            $expirationTime = 0;
         }
 
+        // Send cookie with the new expiration date to the browser
+        session_set_cookie_params($expirationTime, $cookiedir, $_SERVER['SERVER_NAME']);
+        session_regenerate_id(true);
+
         // Optional redirect after login:
         if (isset($_GET['post'])) {
             $uri = '?post='. urlencode($_GET['post']);
@@ -590,7 +594,8 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager,
     if (isset($_SERVER['QUERY_STRING']) && startsWith($_SERVER['QUERY_STRING'], 'do=logout'))
     {
         invalidateCaches($conf->get('resource.page_cache'));
-        $sessionManager->logout(WEB_PATH);
+        $sessionManager->logout();
+        setcookie(SessionManager::$LOGGED_IN_COOKIE, 'false', 0, WEB_PATH);
         header('Location: ?');
         exit;
     }
diff --git a/tests/security/SessionManagerTest.php b/tests/security/SessionManagerTest.php
index e4e1cfbc..e1c72707 100644
--- a/tests/security/SessionManagerTest.php
+++ b/tests/security/SessionManagerTest.php
@@ -14,11 +14,17 @@ use \PHPUnit\Framework\TestCase;
  */
 class SessionManagerTest extends TestCase
 {
-    // Session ID hashes
+    /** @var array Session ID hashes */
     protected static $sidHashes = null;
 
-    // Fake ConfigManager
-    protected static $conf = null;
+    /** @var FakeConfigManager ConfigManager substitute for testing */
+    protected $conf = null;
+
+    /** @var array $_SESSION array for testing */
+    protected $session = [];
+
+    /** @var SessionManager Server-side session management abstraction */
+    protected $sessionManager = null;
 
     /**
      * Assign reference data
@@ -26,7 +32,20 @@ class SessionManagerTest extends TestCase
     public static function setUpBeforeClass()
     {
         self::$sidHashes = ReferenceSessionIdHashes::getHashes();
-        self::$conf = new FakeConfigManager();
+    }
+
+    /**
+     * Initialize or reset test resources
+     */
+    public function setUp()
+    {
+        $this->conf = new FakeConfigManager([
+            'credentials.login' => 'johndoe',
+            'credentials.salt' => 'salt',
+            'security.session_protection_disabled' => false,
+        ]);
+        $this->session = [];
+        $this->sessionManager = new SessionManager($this->session, $this->conf);
     }
 
     /**
@@ -34,12 +53,9 @@ class SessionManagerTest extends TestCase
      */
     public function testGenerateToken()
     {
-        $session = [];
-        $sessionManager = new SessionManager($session, self::$conf);
-
-        $token = $sessionManager->generateToken();
+        $token = $this->sessionManager->generateToken();
 
-        $this->assertEquals(1, $session['tokens'][$token]);
+        $this->assertEquals(1, $this->session['tokens'][$token]);
         $this->assertEquals(40, strlen($token));
     }
 
@@ -54,7 +70,7 @@ class SessionManagerTest extends TestCase
                 $token => 1,
             ],
         ];
-        $sessionManager = new SessionManager($session, self::$conf);
+        $sessionManager = new SessionManager($session, $this->conf);
 
         // check and destroy the token
         $this->assertTrue($sessionManager->checkToken($token));
@@ -69,21 +85,18 @@ class SessionManagerTest extends TestCase
      */
     public function testGenerateAndCheckToken()
     {
-        $session = [];
-        $sessionManager = new SessionManager($session, self::$conf);
-
-        $token = $sessionManager->generateToken();
+        $token = $this->sessionManager->generateToken();
 
         // ensure a token has been generated
-        $this->assertEquals(1, $session['tokens'][$token]);
+        $this->assertEquals(1, $this->session['tokens'][$token]);
         $this->assertEquals(40, strlen($token));
 
         // check and destroy the token
-        $this->assertTrue($sessionManager->checkToken($token));
-        $this->assertFalse(isset($session['tokens'][$token]));
+        $this->assertTrue($this->sessionManager->checkToken($token));
+        $this->assertFalse(isset($this->session['tokens'][$token]));
 
         // ensure the token has been destroyed
-        $this->assertFalse($sessionManager->checkToken($token));
+        $this->assertFalse($this->sessionManager->checkToken($token));
     }
 
     /**
@@ -91,10 +104,7 @@ class SessionManagerTest extends TestCase
      */
     public function testCheckInvalidToken()
     {
-        $session = [];
-        $sessionManager = new SessionManager($session, self::$conf);
-
-        $this->assertFalse($sessionManager->checkToken('4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b'));
+        $this->assertFalse($this->sessionManager->checkToken('4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b'));
     }
 
     /**
@@ -146,4 +156,131 @@ class SessionManagerTest extends TestCase
             SessionManager::checkId('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=')
         );
     }
+
+    /**
+     * Store login information after a successful login
+     */
+    public function testStoreLoginInfo()
+    {
+        $this->sessionManager->storeLoginInfo('ip_id');
+
+        $this->assertTrue(isset($this->session['uid']));
+        $this->assertGreaterThan(time(), $this->session['expires_on']);
+        $this->assertEquals('ip_id', $this->session['ip']);
+        $this->assertEquals('johndoe', $this->session['username']);
+    }
+
+    /**
+     * Extend a server-side session by SessionManager::$SHORT_TIMEOUT
+     */
+    public function testExtendSession()
+    {
+        $this->sessionManager->extendSession();
+
+        $this->assertGreaterThan(time(), $this->session['expires_on']);
+        $this->assertLessThanOrEqual(
+            time() + SessionManager::$SHORT_TIMEOUT,
+            $this->session['expires_on']
+        );
+    }
+
+    /**
+     * Extend a server-side session by SessionManager::$LONG_TIMEOUT
+     */
+    public function testExtendSessionStaySignedIn()
+    {
+        $this->sessionManager->setStaySignedIn(true);
+        $this->sessionManager->extendSession();
+
+        $this->assertGreaterThan(time(), $this->session['expires_on']);
+        $this->assertGreaterThan(
+            time() + SessionManager::$LONG_TIMEOUT - 10,
+            $this->session['expires_on']
+        );
+        $this->assertLessThanOrEqual(
+            time() + SessionManager::$LONG_TIMEOUT,
+            $this->session['expires_on']
+        );
+    }
+
+    /**
+     * Unset session variables after logging out
+     */
+    public function testLogout()
+    {
+        $this->session = [
+            'uid' => 'some-uid',
+            'ip' => 'ip_id',
+            'expires_on' => time() + 1000,
+            'username' => 'johndoe',
+            'visibility' => 'public',
+            'untaggedonly' => false,
+        ];
+        $this->sessionManager->logout();
+
+        $this->assertFalse(isset($this->session['uid']));
+        $this->assertFalse(isset($this->session['ip']));
+        $this->assertFalse(isset($this->session['expires_on']));
+        $this->assertFalse(isset($this->session['username']));
+        $this->assertFalse(isset($this->session['visibility']));
+        $this->assertFalse(isset($this->session['untaggedonly']));
+    }
+
+    /**
+     * The session is considered as expired because the UID is missing
+     */
+    public function testHasExpiredNoUid()
+    {
+        $this->assertTrue($this->sessionManager->hasSessionExpired());
+    }
+
+    /**
+     * The session is active and expiration time has been reached
+     */
+    public function testHasExpiredTimeElapsed()
+    {
+        $this->session['uid'] = 'some-uid';
+        $this->session['expires_on'] = time() - 10;
+
+        $this->assertTrue($this->sessionManager->hasSessionExpired());
+    }
+
+    /**
+     * The session is active and expiration time has not been reached
+     */
+    public function testHasNotExpired()
+    {
+        $this->session['uid'] = 'some-uid';
+        $this->session['expires_on'] = time() + 1000;
+
+        $this->assertFalse($this->sessionManager->hasSessionExpired());
+    }
+
+    /**
+     * Session hijacking protection is disabled, we assume the IP has not changed
+     */
+    public function testHasClientIpChangedNoSessionProtection()
+    {
+        $this->conf->set('security.session_protection_disabled', true);
+
+        $this->assertFalse($this->sessionManager->hasClientIpChanged(''));
+    }
+
+    /**
+     * The client IP identifier has not changed
+     */
+    public function testHasClientIpChangedNope()
+    {
+        $this->session['ip'] = 'ip_id';
+        $this->assertFalse($this->sessionManager->hasClientIpChanged('ip_id'));
+    }
+
+    /**
+     * The client IP identifier has changed
+     */
+    public function testHasClientIpChanged()
+    {
+        $this->session['ip'] = 'ip_id_one';
+        $this->assertTrue($this->sessionManager->hasClientIpChanged('ip_id_two'));
+    }
 }