diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php index 6450d06ae..73b84d3c4 100644 --- a/application/transaction.class.inc.php +++ b/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); - } } } } diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php index d2ffff0a1..4597d77fb 100644 --- a/core/userrights.class.inc.php +++ b/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']; } diff --git a/test/application/privUITransactionFileTest.php b/test/application/privUITransactionFileTest.php index 1b27ebe86..b983436a8 100644 --- a/test/application/privUITransactionFileTest.php +++ b/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'); + } }