Merge remote-tracking branch 'origin/support/2.6' into support/2.7

# Conflicts:
#	application/transaction.class.inc.php
#	test/application/privUITransactionFileTest.php

application/transaction.class.inc.php

@@ -97,8 +97,10 @@ class privUITransaction
 }
 
 /**
- * The original (and by default) mechanism for storing transaction information
- * as an array in the $_SESSION variable
+ * The original mechanism for storing transaction information as an array in the $_SESSION variable
+ *
+ * Warning, since 2.6.0 the session is regenerated on each login (see PR #20) !
+ * Also, we saw some problems when using memcached as the PHP session implementation (see N°1835)
  */
 class privUITransactionSession
 {
@@ -192,8 +194,31 @@ class privUITransactionSession
 class privUITransactionFile
 {
 	/**
+	 * @return int
+	 * @throws \SecurityException if no connected user
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
+	 */
+	private static function GetCurrentUserId() {
+		$iCurrentUserId = UserRights::GetConnectedUserId();
+		if ('' === $iCurrentUserId) {
+			throw new SecurityException('Cannot creation transaction_id when no user logged');
+		}
+
+		return $iCurrentUserId;
+	}
+
+	/**
+	 * Create a new transaction id, store it in the session and return its id
+	 *
+	 * @param void
+	 *
 	 * @return int The new transaction identifier
+	 *
+	 * @throws \SecurityException
 	 * @throws \Exception
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 security hardening + throws SecurityException if no user logged
 	 */
 	public static function GetNewTransactionId()
 	{
@@ -210,24 +235,36 @@ class privUITransactionFile
 				throw new Exception('Failed to create the directory "'.APPROOT.'data/transactions". Ajust the rights on the parent directory or let an administrator create the transactions directory and give the web sever enough rights to write into it.');
 			}
 		}
+
 		if (!is_writable(APPROOT.'data/transactions'))
 		{
 			throw new Exception('The directory "'.APPROOT.'data/transactions" must be writable to the application.');
 		}
-		self::CleanupOldTransactions();
-		$id = basename(tempnam(APPROOT.'data/transactions', static::GetUserPrefix()));
-		self::Info('GetNewTransactionId: Created transaction: '.$id);
 
-		return (string)$id;
+		$iCurrentUserId = static::GetCurrentUserId();
+
+		self::CleanupOldTransactions();
+
+		$sTransactionIdFullPath = tempnam(APPROOT.'data/transactions', static::GetUserPrefix());
+		file_put_contents($sTransactionIdFullPath, $iCurrentUserId, LOCK_EX);
+
+		$sTransactionIdFileName = basename($sTransactionIdFullPath);
+		self::Info('GetNewTransactionId: Created transaction: '.$sTransactionIdFileName);
+
+		return $sTransactionIdFileName;
 	}
 
 	/**
 	 * Check whether a transaction is valid or not and (optionally) remove the valid transaction from
 	 * the session so that another call to IsTransactionValid for the same transaction id
 	 * will return false
+	 *
 	 * @param int $id Identifier of the transaction, as returned by GetNewTransactionId
 	 * @param bool $bRemoveTransaction True if the transaction must be removed
+	 *
 	 * @return bool True if the transaction is valid, false otherwise
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening
 	 */
 	public static function IsTransactionValid($id, $bRemoveTransaction = true)
 	{
@@ -241,25 +278,32 @@ class privUITransactionFile
 
 		clearstatcache(true, $sFilepath);
 		$bResult = file_exists($sFilepath);
-		if ($bResult)
+
+		if (false === $bResult) {
+			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
+			return false;
+		}
+
+		$iCurrentUserId = static::GetCurrentUserId();
+		$sTransactionIdUserId = file_get_contents($sFilepath);
+		if ($iCurrentUserId != $sTransactionIdUserId) {
+			self::Info("IsTransactionValid: Transaction '$id' not existing for current user. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
+			return false;
+		}
+
+		if ($bRemoveTransaction)
 		{
-			if ($bRemoveTransaction)
+			$bResult = @unlink($sFilepath);
+			if (!$bResult)
 			{
-				$bResult = @unlink($sFilepath);
-				if (!$bResult)
-				{
-					self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
-				}
-				else
-				{
-					self::Info('IsTransactionValid: OK. Removed transaction: '.$id);
-				}
+				self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
+			}
+			else
+			{
+				self::Info('IsTransactionValid: OK. Removed transaction: '.$id);
 			}
 		}
-		else
-		{
-			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions for this user:\n".implode("\n", self::GetPendingTransactions()));
-		}
+
 		return $bResult;
 	}
 
@@ -267,26 +311,20 @@ class privUITransactionFile
 	 * Removes the transaction specified by its id
 	 * @param int $id The Identifier (as returned by GetNewTransactionId) of the transaction to be removed.
 	 * @return bool true if the token can be removed
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening
 	 */
 	public static function RemoveTransaction($id)
 	{
-		$sFilepath = APPROOT.'data/transactions/'.$id;
-		clearstatcache(true, $sFilepath);
-		if (!file_exists($sFilepath)) {
-			$bSuccess = false;
-			self::Error("RemoveTransaction: Transaction '$id' not found. Pending transactions for this user:\n"
+		/** @noinspection PhpRedundantOptionalArgumentInspection */
+		$bResult = static::IsTransactionValid($id, true);
+		if (false === $bResult) {
+			self::Error("RemoveTransaction: Transaction '$id' is invalid. Pending transactions:\n"
 				.implode("\n", self::GetPendingTransactions()));
-		} else {
-			$bSuccess = @unlink($sFilepath);
+			return false;
 		}
 
-		if (!$bSuccess) {
-			self::Error('RemoveTransaction: FAILED to remove transaction '.$id);
-		} else {
-			self::Info('RemoveTransaction: OK '.$id);
-		}
-
-		return $bSuccess;
+		return true;
 	}
 
 	/**
@@ -359,22 +397,35 @@ class privUITransactionFile
 	{
 		self::Write('Error | '.$sText);
 	}
-	
+
+	protected static function IsLogEnabled() {
+		$oConfig = MetaModel::GetConfig();
+		if (is_null($oConfig)) {
+			return false;
+		}
+
+		$bLogTransactions = $oConfig->Get('log_transactions');
+		if (true === $bLogTransactions) {
+			return true;
+		}
+
+		return false;
+	}
+
 	protected static function Write($sText)
 	{
-		$bLogEnabled = MetaModel::GetConfig()->Get('log_transactions');
-		if ($bLogEnabled)
-		{
+		if (false === static::IsLogEnabled()) {
+			return;
+		}
+
 		$hLogFile = @fopen(APPROOT.'log/transactions.log', 'a');
-		if ($hLogFile !== false)
-		{
+		if ($hLogFile !== false) {
 			flock($hLogFile, LOCK_EX);
 			$sDate = date('Y-m-d H:i:s');
 			fwrite($hLogFile, "$sDate | $sText\n");
 			fflush($hLogFile);
 			flock($hLogFile, LOCK_UN);
 			fclose($hLogFile);
-			}
 		}
 	}
 }

core/userrights.class.inc.php

@@ -952,6 +952,21 @@ class UserRights
 		return self::$m_oRealUser;
 	}
 
+	/**
+	 * @return int|string ID of the connected user : if impersonate then use {@see m_oRealUser}, else {@see m_oUser}. If no user set then return ''
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
+	 */
+	public static function GetConnectedUserId() {
+		if (false === is_null(static::$m_oRealUser)) {
+			return static::$m_oRealUser->GetKey();
+		}
+		if (false === is_null(static::$m_oUser)) {
+			return static::$m_oUser->GetKey();
+		}
+
+		return '';
+	}
+
 	public static function GetRealUserId()
 	{
 		if (is_null(self::$m_oRealUser))
@@ -1212,7 +1227,7 @@ class UserRights
 		elseif ((self::$m_oUser !== null) && ($oUser->GetKey() == self::$m_oUser->GetKey()))
 		{
 			// Data about the current user can be found into the session data
-			if (array_key_exists('profile_list', $_SESSION))
+			if ((false === utils::IsModeCLI()) && array_key_exists('profile_list', $_SESSION))
 			{
 				$aProfiles = $_SESSION['profile_list'];
 			}

test/application/privUITransactionFileTest.php

@@ -17,7 +17,7 @@
  * You should have received a copy of the GNU Affero General Public License
  */
 
-use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
 
 /**
  * @runTestsInSeparateProcesses
@@ -25,7 +25,7 @@ use Combodo\iTop\Test\UnitTest\ItopTestCase;
  * @backupGlobals disabled
  * @covers utils
  */
-class privUITransactionFileTest extends ItopTestCase
+class privUITransactionFileTest extends ItopDataTestCase
 {
 	public function setUp()
 	{
@@ -132,4 +132,33 @@ class privUITransactionFileTest extends ItopTestCase
 		}
 		return rmdir($sDir);
 	}
+
+	const USER_TEST_LOGIN = 'support_test_privUITransaction';
+
+	/**
+	 * @throws \SecurityException
+	 */
+	public function testIsTransactionValid() {
+		$this->markTestSkipped('Still need some work for Jenkins (Token created by support user must be invalid in the admin user context)');
+
+		$this->CreateUser(static::USER_TEST_LOGIN, 5); // profile:5 is "Support agent"
+
+		// create token in the support user context
+		UserRights::Login(self::USER_TEST_LOGIN);
+		$sTransactionIdUserSupport = privUITransactionFile::GetNewTransactionId();
+		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
+		$this->assertTrue($bResult, 'Token created by support user must be valid in the support user context');
+
+		// test token in the admin user context
+		UserRights::Login('admin');
+		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
+		$this->assertFalse($bResult, 'Token created by support user must be invalid in the admin user context');
+		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
+		$this->assertFalse($bResult, 'Token created by support user cannot be removed in the admin user context');
+
+		// test other methods in the support user context
+		UserRights::Login(self::USER_TEST_LOGIN);
+		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
+		$this->assertTrue($bResult, 'Token created by support user must be removed in the support user context');
+	}
 }