Minz allow parallel sessions (FreshRSS#3096)

* Minz allow parallel sessions #fix FreshRSS#3093 * Array optimisation * Array optimisation missing * Reduce direct access to $_SESSION except in install process * Fix session start headers warning * Use cookie only the first time the session is started: `PHP Warning: session_start(): Cannot start session when headers already sent in /var/www/FreshRSS/lib/Minz/Session.php on line 39` * New concept of volatile session for API calls Optimisation: do not use cookies or local storage at all for API calls without a Web session Fix warning: ``` PHP Warning: session_destroy(): Trying to destroy uninitialized session in Unknown on line 0 ``` * Only call Minz_Session::init once in our index It was called twice (once indirectly via FreshRSS->init()) * Whitespace * Mutex for notifications Implement mutex for notifications FreshRSS#3208 (comment) * Typo * Install script is not ready for using Minz_Session
tonitonae · Oct 6, 2020 · 0319cc9 · 0319cc9
1 parent 3aed0b9
commit 0319cc9
Show file tree

Hide file tree

Showing 15 changed files with 136 additions and 51 deletions.
diff --git a/app/Controllers/authController.php b/app/Controllers/authController.php
@@ -141,9 +141,11 @@ public function formLoginAction() {
 			);
 			if ($ok) {
 				// Set session parameter to give access to the user.
-				Minz_Session::_param('currentUser', $username);
-				Minz_Session::_param('passwordHash', $conf->passwordHash);
-				Minz_Session::_param('csrf');
+				Minz_Session::_params([
+					'currentUser' => $username,
+					'passwordHash' => $conf->passwordHash,
+					'csrf' => false,
+				]);
 				FreshRSS_Auth::giveAccess();
 
 				// Set cookie parameter if nedded.
@@ -190,9 +192,11 @@ public function formLoginAction() {
 			$ok = password_verify($password, $s);
 			unset($password);
 			if ($ok) {
-				Minz_Session::_param('currentUser', $username);
-				Minz_Session::_param('passwordHash', $s);
-				Minz_Session::_param('csrf');
+				Minz_Session::_params([
+					'currentUser' => $username,
+					'passwordHash' => $s,
+					'csrf' => false,
+				]);
 				FreshRSS_Auth::giveAccess();
 
 				Minz_Translate::init($conf->language);

diff --git a/app/Controllers/errorController.php b/app/Controllers/errorController.php
@@ -16,8 +16,10 @@ class FreshRSS_error_Controller extends Minz_ActionController {
 	public function indexAction() {
 		$code_int = Minz_Session::param('error_code', 404);
 		$error_logs = Minz_Session::param('error_logs', array());
-		Minz_Session::_param('error_code');
-		Minz_Session::_param('error_logs');
+		Minz_Session::_params([
+			'error_code' => false,
+			'error_logs' => false,
+		]);
 
 		switch ($code_int) {
 		case 200 :

diff --git a/app/Controllers/userController.php b/app/Controllers/userController.php
@@ -350,9 +350,11 @@ public function createAction() {
 			// get started immediately.
 			if ($ok && !FreshRSS_Auth::hasAccess('admin')) {
 				$user_conf = get_user_configuration($new_user_name);
-				Minz_Session::_param('currentUser', $new_user_name);
-				Minz_Session::_param('passwordHash', $user_conf->passwordHash);
-				Minz_Session::_param('csrf');
+				Minz_Session::_params([
+					'currentUser' => $new_user_name,
+					'passwordHash' => $user_conf->passwordHash,
+					'csrf' => false,
+				]);
 				FreshRSS_Auth::giveAccess();
 			}
 

diff --git a/app/Models/Auth.php b/app/Models/Auth.php
@@ -23,8 +23,10 @@ public static function init() {
 		if ($current_user === '') {
 			$conf = Minz_Configuration::get('system');
 			$current_user = $conf->default_user;
-			Minz_Session::_param('currentUser', $current_user);
-			Minz_Session::_param('csrf');
+			Minz_Session::_params([
+				'currentUser' => $current_user,
+				'csrf' => false,
+			]);
 		}
 
 		if (self::$login_ok) {
@@ -55,9 +57,11 @@ private static function accessControl() {
 			$current_user = '';
 			if (isset($credentials[1])) {
 				$current_user = trim($credentials[0]);
-				Minz_Session::_param('currentUser', $current_user);
-				Minz_Session::_param('passwordHash', trim($credentials[1]));
-				Minz_Session::_param('csrf');
+				Minz_Session::_params([
+					'currentUser' => $current_user,
+					'passwordHash' => trim($credentials[1]),
+					'csrf' => false,
+				]);
 			}
 			return $current_user != '';
 		case 'http_auth':
@@ -79,8 +83,10 @@ private static function accessControl() {
 				]);
 			}
 			if ($login_ok) {
-				Minz_Session::_param('currentUser', $current_user);
-				Minz_Session::_param('csrf');
+				Minz_Session::_params([
+					'currentUser' => $current_user,
+					'csrf' => false,
+				]);
 			}
 			return $login_ok;
 		case 'none':
@@ -118,8 +124,10 @@ public static function giveAccess() {
 			self::$login_ok = false;
 		}
 
-		Minz_Session::_param('loginOk', self::$login_ok);
-		Minz_Session::_param('REMOTE_USER', httpAuthUser());
+		Minz_Session::_params([
+			'loginOk' => self::$login_ok,
+			'REMOTE_USER' => httpAuthUser(),
+		]);
 		return self::$login_ok;
 	}
 
@@ -153,9 +161,11 @@ public static function hasAccess($scope = 'general') {
 	 */
 	public static function removeAccess() {
 		self::$login_ok = false;
-		Minz_Session::_param('loginOk');
-		Minz_Session::_param('csrf');
-		Minz_Session::_param('REMOTE_USER');
+		Minz_Session::_params([
+			'loginOk' => false,
+			'csrf' => false,
+			'REMOTE_USER' => false,
+		]);
 		$system_conf = Minz_Configuration::get('system');
 
 		$username = '';

diff --git a/app/Models/DatabaseDAO.php b/app/Models/DatabaseDAO.php
@@ -20,11 +20,10 @@ public function create() {
 
 		try {
 			$sql = sprintf($SQL_CREATE_DB, empty($db['base']) ? '' : $db['base']);
-			return $this->pdo->exec($sql) !== false;
+			return $this->pdo->exec($sql) === false ? 'Error during CREATE DATABASE' : '';
 		} catch (Exception $e) {
-			$_SESSION['bd_error'] = $e->getMessage();
-			syslog(LOG_DEBUG, __method__ . ' warning: ' . $e->getMessage());
-			return false;
+			syslog(LOG_DEBUG, __method__ . ' notice: ' . $e->getMessage());
+			return $e->getMessage();
 		}
 	}
 
@@ -33,11 +32,10 @@ public function testConnection() {
 			$sql = 'SELECT 1';
 			$stm = $this->pdo->query($sql);
 			$res = $stm->fetchAll(PDO::FETCH_COLUMN, 0);
-			return $res != false;
+			return $res == false ? 'Error during SQL connection test!' : '';
 		} catch (Exception $e) {
-			$_SESSION['bd_error'] = $e->getMessage();
 			syslog(LOG_DEBUG, __method__ . ' warning: ' . $e->getMessage());
-			return false;
+			return $e->getMessage();
 		}
 	}
 

diff --git a/app/actualize_script.php b/app/actualize_script.php
@@ -78,8 +78,10 @@ function notice($message) {
 		}
 	}
 
-	Minz_Session::_param('currentUser', '_');
-	Minz_Session::_param('loginOk');
+	Minz_Session::_params([
+		'currentUser' => '_',
+		'loginOk' => false,
+	]);
 	gc_collect_cycles();
 }
 

diff --git a/app/install.php b/app/install.php
@@ -163,9 +163,14 @@ function saveStep2() {
 
 		$ok = false;
 		try {
-			Minz_Session::_param('currentUser', $config_array['default_user']);
-			$ok = initDb();
-			Minz_Session::_param('currentUser');
+			$_SESSION['currentUser'] = $config_array['default_user'];
+			$error = initDb();
+			unset($_SESSION['currentUser']);
+			if ($error != '') {
+				$_SESSION['bd_error'] = $error;
+			} else {
+				$ok = true;
+			}
 		} catch (Exception $ex) {
 			$_SESSION['bd_error'] = $ex->getMessage();
 			$ok = false;

diff --git a/cli/_cli.php b/cli/_cli.php
@@ -10,6 +10,7 @@
 require(LIB_PATH . '/lib_rss.php');	//Includes class autoloader
 require(LIB_PATH . '/lib_install.php');
 
+Minz_Session::init('FreshRSS', true);
 Minz_Configuration::register('system',
 	DATA_PATH . '/config.php',
 	FRESHRSS_PATH . '/config.default.php');

diff --git a/cli/do-install.php b/cli/do-install.php
@@ -93,10 +93,14 @@
 
 $ok = false;
 try {
-	$ok = initDb();
+	$error = initDb();
+	if ($error != '') {
+		$_SESSION['bd_error'] = $error;
+	} else {
+		$ok = true;
+	}
 } catch (Exception $ex) {
 	$_SESSION['bd_error'] = $ex->getMessage();
-	$ok = false;
 }
 
 if (!$ok) {

diff --git a/lib/Minz/Error.php b/lib/Minz/Error.php
@@ -24,8 +24,10 @@ public static function error ($code = 404, $logs = array (), $redirect = true) {
 		$error_filename = APP_PATH . '/Controllers/errorController.php';
 
 		if (file_exists ($error_filename)) {
-			Minz_Session::_param('error_code', $code);
-			Minz_Session::_param('error_logs', $logs);
+			Minz_Session::_params([
+				'error_code' => $code,
+				'error_logs' => $logs,
+			]);
 
 			Minz_Request::forward (array (
 				'c' => 'error'

diff --git a/lib/Minz/Request.php b/lib/Minz/Request.php
@@ -269,13 +269,14 @@ private static function requestId() {
 	}
 
 	private static function setNotification($type, $content) {
-		//TODO: Will need to ensure non-concurrency when landing https://github.com/FreshRSS/FreshRSS/pull/3096
+		Minz_Session::lock();
 		$requests = Minz_Session::param('requests', []);
 		$requests[self::requestId()] = [
 				'time' => time(),
 				'notification' => [ 'type' => $type, 'content' => $content ],
 			];
 		Minz_Session::_param('requests', $requests);
+		Minz_Session::unlock();
 	}
 
 	public static function setGoodNotification($content) {
@@ -288,7 +289,7 @@ public static function setBadNotification($content) {
 
 	public static function getNotification() {
 		$notif = null;
-		//TODO: Will need to ensure non-concurrency when landing https://github.com/FreshRSS/FreshRSS/pull/3096
+		Minz_Session::lock();
 		$requests = Minz_Session::param('requests');
 		if ($requests) {
 			//Delete abandonned notifications
@@ -301,6 +302,7 @@ public static function getNotification() {
 			}
 			Minz_Session::_param('requests', $requests);
 		}
+		Minz_Session::unlock();
 		return $notif;
 	}
 

diff --git a/lib/Minz/Session.php b/lib/Minz/Session.php
@@ -4,18 +4,51 @@
  * La classe Session gère la session utilisateur
  */
 class Minz_Session {
+	private static $volatile = false;
+
+	/**
+	 * For mutual exclusion.
+	 */
+	private static $locked = false;
+
+	public static function lock() {
+		if (!self::$volatile && !self::$locked && session_start()) {
+			self::$locked = true;
+		}
+		return self::$locked;
+	}
+
+	public static function unlock() {
+		if (!self::$volatile && session_write_close()) {
+			self::$locked = false;
+		}
+		return self::$locked;
+	}
+
 	/**
 	 * Initialise la session, avec un nom
 	 * Le nom de session est utilisé comme nom pour les cookies et les URLs(i.e. PHPSESSID).
 	 * Il ne doit contenir que des caractères alphanumériques ; il doit être court et descriptif
+	 * If the volatile parameter is true, then no cookie and not session storage are used.
+	 * Volatile is especially useful for API calls without cookie / Web session.
 	 */
-	public static function init($name) {
+	public static function init($name, $volatile = false) {
+		self::$volatile = $volatile;
+		if (self::$volatile) {
+			$_SESSION = [];
+			return;
+		}
+
 		$cookie = session_get_cookie_params();
 		self::keepCookie($cookie['lifetime']);
 
 		// démarre la session
 		session_name($name);
+		//When using cookies (default value), session_stars() sends HTTP headers
 		session_start();
+		session_write_close();
+		//Use cookie only the first time the session is started to avoid resending HTTP headers
+		ini_set('session.use_cookies', '0');
 	}
 
 
@@ -35,13 +68,34 @@ public static function param($p, $default = false) {
 	 * @param $v la valeur à attribuer, false pour supprimer
 	 */
 	public static function _param($p, $v = false) {
+		if (!self::$volatile && !self::$locked) {
+			session_start();
+		}
 		if ($v === false) {
 			unset($_SESSION[$p]);
 		} else {
 			$_SESSION[$p] = $v;
 		}
+		if (!self::$volatile && !self::$locked) {
+			session_write_close();
+		}
 	}
 
+	public static function _params($keyValues) {
+		if (!self::$volatile && !self::$locked) {
+			session_start();
+		}
+		foreach ($keyValues as $k => $v) {
+			if ($v === false) {
+				unset($_SESSION[$k]);
+			} else {
+				$_SESSION[$k] = $v;
+			}
+		}
+		if (!self::$volatile && !self::$locked) {
+			session_write_close();
+		}
+	}
 
 	/**
 	 * Permet d'effacer une session
@@ -50,7 +104,9 @@ public static function _param($p, $v = false) {
 	public static function unset_session($force = false) {
 		$language = self::param('language');
 
-		session_destroy();
+		if (!self::$volatile) {
+			session_destroy();
+		}
 		$_SESSION = array();
 
 		if (!$force) {

diff --git a/p/api/fever.php b/p/api/fever.php
@@ -25,9 +25,7 @@
 	die('Service Unavailable!');
 }
 
-ini_set('session.use_cookies', '0');
-register_shutdown_function('session_destroy');
-Minz_Session::init('FreshRSS');
+Minz_Session::init('FreshRSS', true);
 // ================================================================================================
 
 // <Debug>

diff --git a/p/api/greader.php b/p/api/greader.php
@@ -935,9 +935,7 @@ function markAllAsRead($streamId, $olderThanId) {
 	checkCompatibility();
 }
 
-ini_set('session.use_cookies', '0');
-register_shutdown_function('session_destroy');
-Minz_Session::init('FreshRSS');
+Minz_Session::init('FreshRSS', true);
 
 $user = $pathInfos[1] === 'accounts' ? '' : authorizationToUser();
 FreshRSS_Context::$user_conf = null;