From 51f0128cdba52099c40693379e72f094b42a6f80 Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Fri, 27 Apr 2018 23:17:38 +0200 Subject: Refactor session and cookie timeout control Signed-off-by: VirtualTam --- application/security/LoginManager.php | 5 +- application/security/SessionManager.php | 48 +++++++-- index.php | 47 +++++---- tests/security/SessionManagerTest.php | 181 ++++++++++++++++++++++++++++---- 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 * @@ -32,6 +38,16 @@ class SessionManager $this->conf = $conf; } + /** + * 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 * @@ -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')); + } } -- cgit v1.2.3