From: VirtualTam Date: Sat, 17 Feb 2018 00:46:27 +0000 (+0100) Subject: Refactor user credential validation at login time X-Git-Tag: v0.10.0~23^2~11 X-Git-Url: https://git.immae.eu/?p=github%2Fshaarli%2FShaarli.git;a=commitdiff_plain;h=63ea23c2a67d2a1cf6cda79fa2fe49a143571cde Refactor user credential validation at login time Changed: - move login/password verification to LoginManager - code cleanup Signed-off-by: VirtualTam --- diff --git a/application/LoginManager.php b/application/LoginManager.php index 397bc6e3..8f6bf0da 100644 --- a/application/LoginManager.php +++ b/application/LoginManager.php @@ -8,20 +8,123 @@ class LoginManager { protected $globals = []; protected $configManager = null; + protected $sessionManager = null; protected $banFile = ''; + protected $isLoggedIn = false; + protected $openShaarli = false; /** * Constructor * - * @param array $globals The $GLOBALS array (reference) - * @param ConfigManager $configManager Configuration Manager instance. + * @param array $globals The $GLOBALS array (reference) + * @param ConfigManager $configManager Configuration Manager instance + * @param SessionManager $sessionManager SessionManager instance */ - public function __construct(& $globals, $configManager) + public function __construct(& $globals, $configManager, $sessionManager) { $this->globals = &$globals; $this->configManager = $configManager; + $this->sessionManager = $sessionManager; $this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php'); $this->readBanFile(); + if ($this->configManager->get('security.open_shaarli')) { + $this->openShaarli = true; + } + } + + /** + * Check user session state and validity (expiration) + * + * @param array $server The $_SERVER array + * @param array $session The $_SESSION array (reference) + * @param array $cookie The $_COOKIE array + * @param string $webPath Path on the server in which the cookie will be available on + * @param string $token Session token + * + * @return bool true if the user session is valid, false otherwise + */ + public function checkLoginState($server, & $session, $cookie, $webPath, $token) + { + if (! $this->configManager->exists('credentials.login')) { + // Shaarli is not configured yet + $this->isLoggedIn = false; + return; + } + + if (isset($cookie[SessionManager::$LOGGED_IN_COOKIE]) + && $cookie[SessionManager::$LOGGED_IN_COOKIE] === $token + ) { + $this->sessionManager->storeLoginInfo($server); + $this->isLoggedIn = true; + } + + // Logout when: + // - the session does not exist on the server side + // - the session has expired + // - the client IP address has changed + if (empty($session['uid']) + || ($this->configManager->get('security.session_protection_disabled') === false + && $session['ip'] != client_ip_id($server)) + || time() >= $session['expires_on'] + ) { + $this->sessionManager->logout($webPath); + $this->isLoggedIn = false; + return; + } + + // Extend session validity + if (! empty($session['longlastingsession'])) { + // "Stay signed in" is enabled + $session['expires_on'] = time() + $session['longlastingsession']; + } else { + $session['expires_on'] = time() + SessionManager::$INACTIVITY_TIMEOUT; + } + } + + /** + * Return whether the user is currently logged in + * + * @return true when the user is logged in, false otherwise + */ + public function isLoggedIn() + { + if ($this->openShaarli) { + return true; + } + return $this->isLoggedIn; + } + + /** + * Check user credentials are valid + * + * @param array $server The $_SERVER array + * @param string $login Username + * @param string $password Password + * + * @return bool true if the provided credentials are valid, false otherwise + */ + public function checkCredentials($server, $login, $password) + { + $hash = sha1($password . $login . $this->configManager->get('credentials.salt')); + + if ($login != $this->configManager->get('credentials.login') + || $hash != $this->configManager->get('credentials.hash') + ) { + logm( + $this->configManager->get('resource.log'), + $server['REMOTE_ADDR'], + 'Login failed for user ' . $login + ); + return false; + } + + $this->sessionManager->storeLoginInfo($server); + logm( + $this->configManager->get('resource.log'), + $server['REMOTE_ADDR'], + 'Login successful' + ); + return true; } /** diff --git a/index.php b/index.php index 34785209..5e15b9c2 100644 --- a/index.php +++ b/index.php @@ -121,8 +121,8 @@ if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli'])) } $conf = new ConfigManager(); -$loginManager = new LoginManager($GLOBALS, $conf); $sessionManager = new SessionManager($_SESSION, $conf); +$loginManager = new LoginManager($GLOBALS, $conf, $sessionManager); // LC_MESSAGES isn't defined without php-intl, in this case use LC_COLLATE locale instead. if (! defined('LC_MESSAGES')) { @@ -178,88 +178,20 @@ 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'))); -/** - * Checking session state (i.e. is the user still logged in) - * - * @param ConfigManager $conf Configuration Manager instance. - * @param SessionManager $sessionManager SessionManager instance - * - * @return bool true if the user is logged in, false otherwise. - */ -function setup_login_state($conf, $sessionManager) -{ - if ($conf->get('security.open_shaarli')) { - return true; - } - $userIsLoggedIn = false; // By default, we do not consider the user as logged in; - $loginFailure = false; // If set to true, every attempt to authenticate the user will fail. This indicates that an important condition isn't met. - if (! $conf->exists('credentials.login')) { - $userIsLoggedIn = false; // Shaarli is not configured yet. - $loginFailure = true; - } - if (isset($_COOKIE[SessionManager::$LOGGED_IN_COOKIE]) - && $_COOKIE[SessionManager::$LOGGED_IN_COOKIE] === STAY_SIGNED_IN_TOKEN - && !$loginFailure - ) { - $sessionManager->storeLoginInfo($_SERVER); - $userIsLoggedIn = true; - } - // If session does not exist on server side, or IP address has changed, or session has expired, logout. - if (empty($_SESSION['uid']) - || ($conf->get('security.session_protection_disabled') === false && $_SESSION['ip'] != client_ip_id($_SERVER)) - || time() >= $_SESSION['expires_on']) - { - $sessionManager->logout(WEB_PATH); - $userIsLoggedIn = false; - $loginFailure = true; - } - if (!empty($_SESSION['longlastingsession'])) { - $_SESSION['expires_on']=time()+$_SESSION['longlastingsession']; // In case of "Stay signed in" checked. - } else { - $_SESSION['expires_on'] = time() + $sessionManager::$INACTIVITY_TIMEOUT; - } - if (!$loginFailure) { - $userIsLoggedIn = true; - } - - return $userIsLoggedIn; -} - -$userIsLoggedIn = setup_login_state($conf, $sessionManager); - -// ------------------------------------------------------------------------------------------ -// Session management +$loginManager->checkLoginState($_SERVER, $_SESSION, $_COOKIE, WEB_PATH, STAY_SIGNED_IN_TOKEN); /** - * Check that user/password is correct. - * - * @param string $login Username - * @param string $password User password - * @param ConfigManager $conf Configuration Manager instance. - * @param SessionManager $sessionManager SessionManager instance + * Adapter function for PageBuilder * - * @return bool: authentication successful or not. + * TODO: update PageBuilder and tests */ -function check_auth($login, $password, $conf, $sessionManager) -{ - $hash = sha1($password . $login . $conf->get('credentials.salt')); - if ($login == $conf->get('credentials.login') && $hash == $conf->get('credentials.hash')) { - // Login/password is correct. - $sessionManager->storeLoginInfo($_SERVER); - logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'Login successful'); - return true; - } - logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'Login failed for user '.$login); - return false; -} - -// Returns true if the user is logged in. function isLoggedIn() { - global $userIsLoggedIn; - return $userIsLoggedIn; + global $loginManager; + return $loginManager->isLoggedIn(); } + // ------------------------------------------------------------------------------------------ // Process login form: Check if login/password is correct. if (isset($_POST['login'])) { @@ -268,7 +200,7 @@ if (isset($_POST['login'])) { } if (isset($_POST['password']) && $sessionManager->checkToken($_POST['token']) - && (check_auth($_POST['login'], $_POST['password'], $conf, $sessionManager)) + && $loginManager->checkCredentials($_SERVER, $_POST['login'], $_POST['password']) ) { // Login/password is OK. $loginManager->handleSuccessfulLogin($_SERVER); @@ -347,15 +279,16 @@ if (!isset($_SESSION['tokens'])) $_SESSION['tokens']=array(); // Token are atta * Gives the last 7 days (which have links). * This RSS feed cannot be filtered. * - * @param ConfigManager $conf Configuration Manager instance. + * @param ConfigManager $conf Configuration Manager instance + * @param LoginManager $loginManager LoginManager instance */ -function showDailyRSS($conf) { +function showDailyRSS($conf, $loginManager) { // Cache system $query = $_SERVER['QUERY_STRING']; $cache = new CachedPage( $conf->get('config.PAGE_CACHE'), page_url($_SERVER), - startsWith($query,'do=dailyrss') && !isLoggedIn() + startsWith($query,'do=dailyrss') && !$loginManager->isLoggedIn() ); $cached = $cache->cachedVersion(); if (!empty($cached)) { @@ -367,7 +300,7 @@ function showDailyRSS($conf) { // Read links from database (and filter private links if used it not logged in). $LINKSDB = new LinkDB( $conf->get('resource.datastore'), - isLoggedIn(), + $loginManager->isLoggedIn(), $conf->get('privacy.hide_public_links'), $conf->get('redirector.url'), $conf->get('redirector.encode_url') @@ -509,7 +442,7 @@ function showDaily($pageBuilder, $LINKSDB, $conf, $pluginManager) /* Hook is called before column construction so that plugins don't have to deal with columns. */ - $pluginManager->executeHooks('render_daily', $data, array('loggedin' => isLoggedIn())); + $pluginManager->executeHooks('render_daily', $data, array('loggedin' => $loginManager->isLoggedIn())); /* We need to spread the articles on 3 columns. I did not want to use a JavaScript lib like http://masonry.desandro.com/ @@ -553,8 +486,8 @@ function showDaily($pageBuilder, $LINKSDB, $conf, $pluginManager) * @param ConfigManager $conf Configuration Manager instance. * @param PluginManager $pluginManager Plugin Manager instance. */ -function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) { - buildLinkList($PAGE,$LINKSDB, $conf, $pluginManager); // Compute list of links to display +function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager, $loginManager) { + buildLinkList($PAGE,$LINKSDB, $conf, $pluginManager, $loginManager); $PAGE->renderPage('linklist'); } @@ -574,7 +507,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, read_updates_file($conf->get('resource.updates')), $LINKSDB, $conf, - isLoggedIn() + $loginManager->isLoggedIn() ); try { $newUpdates = $updater->update(); @@ -596,11 +529,11 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, // Determine which page will be rendered. $query = (isset($_SERVER['QUERY_STRING'])) ? $_SERVER['QUERY_STRING'] : ''; - $targetPage = Router::findPage($query, $_GET, isLoggedIn()); + $targetPage = Router::findPage($query, $_GET, $loginManager->isLoggedIn()); if ( // if the user isn't logged in - !isLoggedIn() && + !$loginManager->isLoggedIn() && // and Shaarli doesn't have public content... $conf->get('privacy.hide_public_links') && // and is configured to enforce the login @@ -628,7 +561,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $pluginManager->executeHooks('render_' . $name, $plugin_data, array( 'target' => $targetPage, - 'loggedin' => isLoggedIn() + 'loggedin' => $loginManager->isLoggedIn() ) ); $PAGE->assign('plugins_' . $name, $plugin_data); @@ -680,7 +613,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $data = array( 'linksToDisplay' => $linksToDisplay, ); - $pluginManager->executeHooks('render_picwall', $data, array('loggedin' => isLoggedIn())); + $pluginManager->executeHooks('render_picwall', $data, array('loggedin' => $loginManager->isLoggedIn())); foreach ($data as $key => $value) { $PAGE->assign($key, $value); @@ -727,7 +660,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, 'search_tags' => $searchTags, 'tags' => $tagList, ); - $pluginManager->executeHooks('render_tagcloud', $data, array('loggedin' => isLoggedIn())); + $pluginManager->executeHooks('render_tagcloud', $data, array('loggedin' => $loginManager->isLoggedIn())); foreach ($data as $key => $value) { $PAGE->assign($key, $value); @@ -760,7 +693,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, 'search_tags' => $searchTags, 'tags' => $tags, ]; - $pluginManager->executeHooks('render_taglist', $data, ['loggedin' => isLoggedIn()]); + $pluginManager->executeHooks('render_taglist', $data, ['loggedin' => $loginManager->isLoggedIn()]); foreach ($data as $key => $value) { $PAGE->assign($key, $value); @@ -787,7 +720,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $cache = new CachedPage( $conf->get('resource.page_cache'), page_url($_SERVER), - startsWith($query,'do='. $targetPage) && !isLoggedIn() + startsWith($query,'do='. $targetPage) && !$loginManager->isLoggedIn() ); $cached = $cache->cachedVersion(); if (!empty($cached)) { @@ -796,15 +729,15 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, } // Generate data. - $feedGenerator = new FeedBuilder($LINKSDB, $feedType, $_SERVER, $_GET, isLoggedIn()); + $feedGenerator = new FeedBuilder($LINKSDB, $feedType, $_SERVER, $_GET, $loginManager->isLoggedIn()); $feedGenerator->setLocale(strtolower(setlocale(LC_COLLATE, 0))); - $feedGenerator->setHideDates($conf->get('privacy.hide_timestamps') && !isLoggedIn()); + $feedGenerator->setHideDates($conf->get('privacy.hide_timestamps') && !$loginManager->isLoggedIn()); $feedGenerator->setUsePermalinks(isset($_GET['permalinks']) || !$conf->get('feed.rss_permalinks')); $data = $feedGenerator->buildData(); // Process plugin hook. $pluginManager->executeHooks('render_feed', $data, array( - 'loggedin' => isLoggedIn(), + 'loggedin' => $loginManager->isLoggedIn(), 'target' => $targetPage, )); @@ -952,7 +885,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, } // -------- Handle other actions allowed for non-logged in users: - if (!isLoggedIn()) + if (!$loginManager->isLoggedIn()) { // User tries to post new link but is not logged in: // Show login screen, then redirect to ?post=... @@ -968,7 +901,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, exit; } - showLinkList($PAGE, $LINKSDB, $conf, $pluginManager); + showLinkList($PAGE, $LINKSDB, $conf, $pluginManager, $loginManager); if (isset($_GET['edit_link'])) { header('Location: ?do=login&edit_link='. escape($_GET['edit_link'])); exit; @@ -1019,7 +952,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $conf->set('credentials.salt', sha1(uniqid('', true) .'_'. mt_rand())); $conf->set('credentials.hash', sha1($_POST['setpassword'] . $conf->get('credentials.login') . $conf->get('credentials.salt'))); try { - $conf->write(isLoggedIn()); + $conf->write($loginManager->isLoggedIn()); } catch(Exception $e) { error_log( @@ -1070,7 +1003,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $conf->set('translation.language', escape($_POST['language'])); try { - $conf->write(isLoggedIn()); + $conf->write($loginManager->isLoggedIn()); $history->updateSettings(); invalidateCaches($conf->get('resource.page_cache')); } @@ -1522,7 +1455,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, else { $conf->set('general.enabled_plugins', save_plugin_config($_POST)); } - $conf->write(isLoggedIn()); + $conf->write($loginManager->isLoggedIn()); $history->updateSettings(); } catch (Exception $e) { @@ -1547,7 +1480,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, } // -------- Otherwise, simply display search form and links: - showLinkList($PAGE, $LINKSDB, $conf, $pluginManager); + showLinkList($PAGE, $LINKSDB, $conf, $pluginManager, $loginManager); exit; } @@ -1559,8 +1492,9 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, * @param LinkDB $LINKSDB LinkDB instance. * @param ConfigManager $conf Configuration Manager instance. * @param PluginManager $pluginManager Plugin Manager instance. + * @param LoginManager $loginManager LoginManager instance */ -function buildLinkList($PAGE,$LINKSDB, $conf, $pluginManager) +function buildLinkList($PAGE, $LINKSDB, $conf, $pluginManager, $loginManager) { // Used in templates if (isset($_GET['searchtags'])) { @@ -1599,8 +1533,6 @@ function buildLinkList($PAGE,$LINKSDB, $conf, $pluginManager) $keys[] = $key; } - - // Select articles according to paging. $pagecount = ceil(count($keys) / $_SESSION['LINKS_PER_PAGE']); $pagecount = $pagecount == 0 ? 1 : $pagecount; @@ -1681,7 +1613,7 @@ function buildLinkList($PAGE,$LINKSDB, $conf, $pluginManager) $data['pagetitle'] .= '- '. $conf->get('general.title'); } - $pluginManager->executeHooks('render_linklist', $data, array('loggedin' => isLoggedIn())); + $pluginManager->executeHooks('render_linklist', $data, array('loggedin' => $loginManager->isLoggedIn())); foreach ($data as $key => $value) { $PAGE->assign($key, $value); @@ -1952,7 +1884,7 @@ function install($conf, $sessionManager) { ); try { // Everything is ok, let's create config file. - $conf->write(isLoggedIn()); + $conf->write($loginManager->isLoggedIn()); } catch(Exception $e) { error_log( @@ -2216,7 +2148,7 @@ try { $linkDb = new LinkDB( $conf->get('resource.datastore'), - isLoggedIn(), + $loginManager->isLoggedIn(), $conf->get('privacy.hide_public_links'), $conf->get('redirector.url'), $conf->get('redirector.encode_url') diff --git a/tests/LoginManagerTest.php b/tests/LoginManagerTest.php index 4159038e..27ca0db5 100644 --- a/tests/LoginManagerTest.php +++ b/tests/LoginManagerTest.php @@ -38,7 +38,7 @@ class LoginManagerTest extends TestCase $this->globals = &$GLOBALS; unset($this->globals['IPBANS']); - $this->loginManager = new LoginManager($this->globals, $this->configManager); + $this->loginManager = new LoginManager($this->globals, $this->configManager, null); $this->server['REMOTE_ADDR'] = $this->ipAddr; } @@ -59,7 +59,7 @@ class LoginManagerTest extends TestCase $this->banFile, " array('127.0.0.1' => 99));\n?>" ); - new LoginManager($this->globals, $this->configManager); + new LoginManager($this->globals, $this->configManager, null); $this->assertEquals(99, $this->globals['IPBANS']['FAILURES']['127.0.0.1']); }