N°7001 - Being admin to generate oAuth token is too restrictive

Was caused by 239c51bb, which adds 65bb76b9 tests improvements but only partly : we were missing in TransactionsTest::tearDown the mysqli reset from mock to original mysqli cnx

Without this reset the rollback made in ItopTestCase::tearDown is throwing an exception (query() method on the DbConnectionWrapper cnx returns false in \CMDBSource::DBQuery) on PHP 8.1 and 8.2

Co-authored-by: Romain Quetiez <romain.queriez@combodo.com>

core/DbConnectionWrapper.php

@@ -59,9 +59,17 @@ class DbConnectionWrapper
 	 * Use this to register a mock that will handle {@see mysqli::query()}
 	 *
 	 * @param \mysqli|null $oMysqli
+	 * @since 3.0.4 3.1.1 3.2.0 Param $oMysqli becomes nullable
+	 * @since 3.1.0-4 N°6848 backport of restoring cnx on null parameter value
 	 */
-	public static function SetDbConnectionMockForQuery(?mysqli $oMysqli): void
+	public static function SetDbConnectionMockForQuery(?mysqli $oMysqli = null): void
 	{
-		static::$oDbCnxMockableForQuery = $oMysqli;
+		if (is_null($oMysqli)) {
+			// Reset to standard connection
+			static::$oDbCnxMockableForQuery = static::$oDbCnxStandard;
+		}
+		else {
+			static::$oDbCnxMockableForQuery = $oMysqli;
+		}
 	}
 }

core/cmdbsource.class.inc.php

@@ -3,7 +3,7 @@
 //
 //   This file is part of iTop.
 //
-//   iTop is free software; you can redistribute it and/or modify
+//   iTop is free software; you can redistribute it and/or modify	
 //   it under the terms of the GNU Affero General Public License as published by
 //   the Free Software Foundation, either version 3 of the License, or
 //   (at your option) any later version.
@@ -380,7 +380,7 @@ class CMDBSource
 	public static function GetDBVendor()
 	{
 		$sDBVendor = static::ENUM_DB_VENDOR_MYSQL;
-
+		
 		$sVersionComment = static::GetServerVariable('version') .  ' - ' . static::GetServerVariable('version_comment');
 		if(preg_match('/mariadb/i', $sVersionComment) === 1)
 		{
@@ -390,7 +390,7 @@ class CMDBSource
 		{
 			$sDBVendor = static::ENUM_DB_VENDOR_PERCONA;
 		}
-
+		
 		return $sDBVendor;
 	}
 
@@ -664,7 +664,7 @@ class CMDBSource
 		);
 		DeadLockLog::Info($sMessage, $iMySqlErrorNo, $aLogContext);
 
-		IssueLog::Error($sMessage, LogChannels::DEADLOCK, [$e->getMessage()]);
+		IssueLog::Error($sMessage, LogChannels::DEADLOCK, $e->getMessage());
 	}
 
 	/**
@@ -923,7 +923,7 @@ class CMDBSource
 		{
 			throw new MySQLException('Failed to issue SQL query', array('query' => $sSql));
 		}
-
+				
 		while ($aRow = $oResult->fetch_array($iMode))
 		{
 			$aData[] = $aRow;
@@ -1077,7 +1077,7 @@ class CMDBSource
 		if (!array_key_exists($iKey, $aTableInfo["Fields"])) return false;
 		$aFieldData = $aTableInfo["Fields"][$iKey];
 		if (!array_key_exists("Key", $aFieldData)) return false;
-		return ($aFieldData["Key"] == "PRI");
+		return ($aFieldData["Key"] == "PRI"); 
 	}
 
 	public static function IsAutoIncrement($sTable, $sField)
@@ -1088,7 +1088,7 @@ class CMDBSource
 		$aFieldData = $aTableInfo["Fields"][$sField];
 		if (!array_key_exists("Extra", $aFieldData)) return false;
 		//MyHelpers::debug_breakpoint($aFieldData);
-		return (strstr($aFieldData["Extra"], "auto_increment"));
+		return (strstr($aFieldData["Extra"], "auto_increment")); 
 	}
 
 	public static function IsField($sTable, $sField)
@@ -1355,13 +1355,13 @@ class CMDBSource
 	public static function GetTableFieldsList($sTable)
 	{
 		assert(!empty($sTable));
-
+		
 		$aTableInfo = self::GetTableInfo($sTable);
 		if (empty($aTableInfo)) return array(); // #@# or an error ?
 
 		return array_keys($aTableInfo["Fields"]);
 	}
-
+	
 	// Cache the information about existing tables, and their fields
 	private static $m_aTablesInfo = array();
 	private static function _TablesInfoCacheReset($sTableName = null)
@@ -1494,7 +1494,7 @@ class CMDBSource
 		{
 			throw new MySQLException('Failed to issue SQL query', array('query' => $sSql));
 		}
-
+		
 		$aRows = array();
 		while ($aRow = $oResult->fetch_array(MYSQLI_ASSOC))
 		{
@@ -1503,7 +1503,7 @@ class CMDBSource
 		$oResult->free();
 		return $aRows;
 	}
-
+	
 	/**
 	 * Returns the value of the specified server variable
 	 * @param string $sVarName Name of the server variable
@@ -1519,7 +1519,7 @@ class CMDBSource
 	/**
 	 * Returns the privileges of the current user
 	 * @return string privileges in a raw format
-	 */
+	 */	   	
 	public static function GetRawPrivileges()
 	{
 		try
@@ -1545,8 +1545,8 @@ class CMDBSource
 
 	/**
 	 * Determine the slave status of the server
-	 * @return bool true if the server is slave
-	 */
+	 * @return bool true if the server is slave 
+	 */	   	
 	public static function IsSlaveServer()
 	{
 		try

pages/oauth.landing.php

@@ -10,6 +10,5 @@ IssueLog::Trace('----- Request: '.utils::GetRequestUri(), LogChannels::WEB_REQUE
 $sTemplates = APPROOT.'templates/pages/backoffice/oauth';
 
 $oUpdateController = new OAuthLandingController($sTemplates, 'core');
-$oUpdateController->AllowOnlyAdmin();
 $oUpdateController->SetDefaultOperation('Landing');
 $oUpdateController->HandleOperation();

setup/backup.class.inc.php

@@ -3,7 +3,7 @@
 //
 //   This file is part of iTop.
 //
-//   iTop is free software; you can redistribute it and/or modify
+//   iTop is free software; you can redistribute it and/or modify	
 //   it under the terms of the GNU Affero General Public License as published by
 //   the Free Software Foundation, either version 3 of the License, or
 //   (at your option) any later version.
@@ -146,23 +146,18 @@ class DBBackup
 	/**
 	 * Create a normalized backup name, depending on the current date/time and Database
 	 *
-	 * @param string|null $sNameSpec Name and path, eventually containing itop placeholders + time formatting following the strftime() format {@link https://www.php.net/manual/fr/function.strftime.php}
+	 * @param string $sNameSpec Name and path, eventually containing itop placeholders + time formatting following the strftime() format {@link https://www.php.net/manual/fr/function.strftime.php}
 	 * @param \DateTime|null $oDateTime Date time to use for the name
 	 *
 	 * @return string Name of the backup file WITHOUT the file extension (eg. `.tar.gz`)
 	 * @since 3.1.0 N°5279 Add $oDateTime parameter
 	 */
-	public function MakeName(?string $sNameSpec = null, ?DateTime $oDateTime = null)
+	public function MakeName(string $sNameSpec = "__DB__-%Y-%m-%d", DateTime $oDateTime = null)
 	{
 		if ($oDateTime === null) {
 			$oDateTime = new DateTime();
 		}
 
-		//N°6640
-		if ($sNameSpec === null) {
-			$sNameSpec = "__DB__-%Y-%m-%d";
-		}
-
 		$sFileName = $sNameSpec;
 		$sFileName = str_replace('__HOST__', $this->sDBHost, $sFileName);
 		$sFileName = str_replace('__DB__', $this->sDBName, $sFileName);
@@ -227,7 +222,7 @@ class DBBackup
 	 *
 	 * @param string $sSourceConfigFile
 	 * @param string $sTmpFolder
-	 * @param bool $bSkipSQLDumpForTesting
+	 * @param bool $bSkipSQLDumpForTesting 
 	 *
 	 * @return array list of files to archive
 	 * @throws \Exception
@@ -278,7 +273,7 @@ class DBBackup
 				if(!file_exists(APPROOT.'/'.$sExtraFileOrDir)) {
 					continue; // Ignore non-existing files
 				}
-
+	
 				$sExtraFullPath = utils::RealPath(APPROOT.'/'.$sExtraFileOrDir, APPROOT);
 				if ($sExtraFullPath === false)
 				{

tests/php-unit-tests/unitary-tests/core/CMDBSource/TransactionsTest.php

@@ -276,6 +276,7 @@ class TransactionsTest extends ItopTestCase
 	protected function tearDown(): void
 	{
 		try {
+			DbConnectionWrapper::SetDbConnectionMockForQuery(); // Else will throw error on PHP 8.1+ (see N°6848)
 			parent::tearDown();
 		}
 		catch (MySQLTransactionNotClosedException $e) {

tests/php-unit-tests/unitary-tests/setup/DBBackupTest.php

@@ -118,7 +118,7 @@ class DBBackupTest extends ItopTestCase
 	 *
 	 * @return void
 	 */
-	public function testMakeName(?string $sInputFormat, ?DateTime $oBackupDateTime, string $sExpectedFilename): void
+	public function testMakeName(string $sInputFormat, DateTime $oBackupDateTime, string $sExpectedFilename): void
 	{
 		$oConfig = utils::GetConfig();
 
@@ -138,11 +138,6 @@ class DBBackupTest extends ItopTestCase
 		$oBackupDateTime = DateTime::createFromFormat('Y-m-d H:i:s', '1985-07-30 15:30:59');
 
 		return [
-			'Default format - no params' => [
-				'__DB__-%Y-%m-%d',
-				$oBackupDateTime,
-				static::DUMMY_DB_NAME.'-1985-07-30',
-			],
 			'Default format' => [
 				'__DB__-%Y-%m-%d',
 				$oBackupDateTime,
@@ -165,28 +160,4 @@ class DBBackupTest extends ItopTestCase
 			],
 		];
 	}
-
-	/**
-	 * N°6640 - Broken unattended setup with XML backup configuration
-	 */
-	public function testMakeNameWithoutParams(): void
-	{
-		$oConfig = utils::GetConfig();
-
-		// See https://github.com/Combodo/iTop/commit/f7ee21f1d7d1c23910506e9e31b57f33311bd5e0#diff-d693fb790e3463d1aa960c2b8b293532b1bbd12c3b8f885d568d315c404f926aR131
-		$oConfig->Set('db_host', static::DUMMY_DB_HOST);
-		$oConfig->Set('db_name', static::DUMMY_DB_NAME);
-		$oConfig->Set('db_subname', static::DUMMY_DB_SUBNAME);
-
-		$oDateTime = new DateTime();
-		$sExpectedFilename = static::DUMMY_DB_NAME . '-' . $oDateTime->format("Y-m-d");
-
-		$oBackup = new DBBackup($oConfig);
-
-		$sTestedFilename = $oBackup->MakeName(null);
-		$this->assertEquals($sExpectedFilename, $sTestedFilename, "Backup filename format doesn't match. Got '$sTestedFilename', expected '$sExpectedFilename'.");
-
-		$sTestedFilename = $oBackup->MakeName();
-		$this->assertEquals($sExpectedFilename, $sTestedFilename, "Backup filename format doesn't match. Got '$sTestedFilename', expected '$sExpectedFilename'.");
-	}
 }