diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php index b83230a61c..053c718915 100644 --- a/application/transaction.class.inc.php +++ b/application/transaction.class.inc.php @@ -193,10 +193,32 @@ protected static function GetUserPrefix() */ 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 identifier of the new transaction + * + * @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() { @@ -212,24 +234,36 @@ public static function GetNewTransactionId() 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.'); } + + $iCurrentUserId = static::GetCurrentUserId(); + self::CleanupOldTransactions(); - $id = basename(tempnam(APPROOT.'data/transactions', static::GetUserPrefix())); - self::Info('GetNewTransactionId: Created transaction: '.$id); - return (string)$id; + $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) { @@ -243,25 +277,32 @@ public static function IsTransactionValid($id, $bRemoveTransaction = true) 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; } @@ -269,26 +310,20 @@ public static function IsTransactionValid($id, $bRemoveTransaction = true) * 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); - } - - if (!$bSuccess) { - self::Error('RemoveTransaction: FAILED to remove transaction '.$id); - } else { - self::Info('RemoveTransaction: OK '.$id); + return false; } - return $bSuccess; + return true; } /** diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php index 12a936787a..1819c0a9e9 100644 --- a/core/userrights.class.inc.php +++ b/core/userrights.class.inc.php @@ -951,6 +951,21 @@ public static function GetRealUserObject() 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)) @@ -1211,7 +1226,7 @@ public static function ListProfiles($oUser = null) 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 new file mode 100644 index 0000000000..c3df443cc5 --- /dev/null +++ b/test/application/privUITransactionFileTest.php @@ -0,0 +1,27 @@ +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'); + } +} \ No newline at end of file