From c140ebcb6b218ada3670ba39478e3084b37565f2 Mon Sep 17 00:00:00 2001 From: odain-cbd <56586767+odain-cbd@users.noreply.github.com> Date: Fri, 12 Jan 2024 08:13:40 +0100 Subject: [PATCH 1/2] =?UTF-8?q?N=C2=B07085=20-=20Fix=20infinite=20loop=20i?= =?UTF-8?q?n=20login=20page=20until=20fatal=20error=20occurs=20(#592)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * N°7085 - login page infinite loop until fatal error- add Config->AddAllowedLoginTypes * N°7085 - reproduce issue via a test * N°7085-fix infinite loop * N°7085 - ci: fix config file rights in tearDown * N°7085 - ci: fix config file rights in tearDown (again) * N°7085 - ci: fix config file content * N°7085 - ci : add runTestsInSeparateProcesses * Update core/config.class.inc.php Co-authored-by: Molkobain * N°7085 - exit -1 + enhance log message * PR feedbacks from Romain regarding LoginTest annotations --------- Co-authored-by: Molkobain --- application/logindefault.class.inc.php | 7 +- core/config.class.inc.php | 24 ++++++- .../unitary-tests/application/LoginTest.php | 67 +++++++++++++++++++ 3 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/application/LoginTest.php diff --git a/application/logindefault.class.inc.php b/application/logindefault.class.inc.php index fb82eb6e0..c046ed5a0 100644 --- a/application/logindefault.class.inc.php +++ b/application/logindefault.class.inc.php @@ -117,6 +117,11 @@ class LoginDefaultAfter extends AbstractLoginFSMExtension implements iLogoutExte protected function OnConnected(&$iErrorCode) { unset($_SESSION['login_temp_auth_user']); + if (is_null(UserRights::GetUserObject())){ + //N°7085 avoid infinite loop + IssueLog::Error("No user logged in. exit"); + exit(-1); + } return LoginWebPage::LOGIN_FSM_CONTINUE; } @@ -132,4 +137,4 @@ class LoginDefaultAfter extends AbstractLoginFSMExtension implements iLogoutExte } } } -} \ No newline at end of file +} diff --git a/core/config.class.inc.php b/core/config.class.inc.php index e9d61596d..cb50820bd 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -1634,7 +1634,7 @@ class Config } if (strlen($sNoise) > 0) { - // Note: sNoise is an html output, but so far it was ok for me (e.g. showing the entire call stack) + // Note: sNoise is an html output, but so far it was ok for me (e.g. showing the entire call stack) throw new ConfigException('Syntax error in configuration file', array('file' => $sConfigFile, 'error' => ''.htmlentities($sNoise, ENT_QUOTES, 'UTF-8').'')); } @@ -1909,6 +1909,24 @@ class Config $this->m_sAllowedLoginTypes = implode('|', $aAllowedLoginTypes); } + /** + * @since 2.7.11 N°7085 + * Add login mode if not configured already + * @param string $sLoginMode + * + * @return void + */ + public function AddAllowedLoginTypes($sLoginMode) + { + $aAllowedLoginTypes = $this->GetAllowedLoginTypes(); + if (in_array($sLoginMode, $aAllowedLoginTypes)){ + return; + } + + $aAllowedLoginTypes[] = $sLoginMode; + $this->SetAllowedLoginTypes($aAllowedLoginTypes); + } + public function SetExternalAuthenticationVariable($sExtAuthVariable) { $this->m_sExtAuthVariable = $sExtAuthVariable; @@ -2428,7 +2446,7 @@ class ConfigPlaceholdersResolver } $sPattern = '/\%(env|server)\((\w+)\)(?:\?:(\w*))?\%/'; //3 capturing groups, ie `%env(HTTP_PORT)?:8080%` produce: `env` `HTTP_PORT` and `8080`. - + if (! preg_match_all($sPattern, $rawValue, $aMatchesCollection, PREG_SET_ORDER)) { return $rawValue; @@ -2481,4 +2499,4 @@ class ConfigPlaceholdersResolver IssueLog::Error($sErrorMessage, self::class, array($sSourceName, $sKey, $sDefault, $sWholeMask)); throw new ConfigException($sErrorMessage); } -} \ No newline at end of file +} diff --git a/tests/php-unit-tests/unitary-tests/application/LoginTest.php b/tests/php-unit-tests/unitary-tests/application/LoginTest.php new file mode 100644 index 000000000..52b4b70c9 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/application/LoginTest.php @@ -0,0 +1,67 @@ +sConfigPath = MetaModel::GetConfig()->GetLoadedFile(); + $this->sConfigTmpBackupFile = tempnam(sys_get_temp_dir(), "config_"); + file_put_contents($this->sConfigTmpBackupFile, file_get_contents($this->sConfigPath)); + + $oConfig = new \Config($this->sConfigPath); + $this->sLoginMode = "unimplemented_loginmode"; + $oConfig->AddAllowedLoginTypes($this->sLoginMode); + + @chmod($this->sConfigPath, 0770); + $oConfig->WriteToFile(); + @chmod($this->sConfigPath, 0440); + } + + protected function tearDown(): void { + if (! is_null($this->sConfigTmpBackupFile) && is_file($this->sConfigTmpBackupFile)){ + //put config back + @chmod($this->sConfigPath, 0770); + file_put_contents($this->sConfigPath, file_get_contents($this->sConfigTmpBackupFile)); + @chmod($this->sConfigPath, 0440); + @unlink($this->sConfigTmpBackupFile); + } + parent::tearDown(); + } + + public function testLoginInfiniteLoopFix() { + $iTimeStamp = microtime(true); + $sOutput = $this->CallItopUrlByCurl(sprintf("/pages/UI.php?login_mode=%s", $this->sLoginMode)); + $iElapsedInMs = (microtime(true) - $iTimeStamp) * 1000; + $sMaxExecutionInS = 1; + $this->assertTrue($iElapsedInMs < $sMaxExecutionInS * 1000, "iTop answered in $iElapsedInMs ms. it should do it in less than $sMaxExecutionInS seconds (max_execution_time)"); + $this->assertFalse(strpos($sOutput, "Fatal error"), "no fatal error due to max execution time should be returned" . $sOutput); + } + + protected function CallItopUrlByCurl($sUri, ?array $aPostFields=[]){ + $ch = curl_init(); + + $sUrl = MetaModel::GetConfig()->Get('app_root_url') . "/$sUri"; + curl_setopt($ch, CURLOPT_URL, $sUrl); + if (0 !== sizeof($aPostFields)){ + curl_setopt($ch, CURLOPT_POST, 1);// set post data to true + curl_setopt($ch, CURLOPT_POSTFIELDS, $aPostFields); + } + curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); + $sOutput = curl_exec($ch); + curl_close ($ch); + + return $sOutput; + } +} From 9830178a479b5e77eb59b4aba08acd09ad5c8565 Mon Sep 17 00:00:00 2001 From: odain Date: Fri, 12 Jan 2024 08:36:15 +0100 Subject: [PATCH 2/2] =?UTF-8?q?N=C2=B07085=20-=20ci=20restore=20runTestsIn?= =?UTF-8?q?SeparateProcesses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/php-unit-tests/unitary-tests/application/LoginTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/php-unit-tests/unitary-tests/application/LoginTest.php b/tests/php-unit-tests/unitary-tests/application/LoginTest.php index 52b4b70c9..0935f7aac 100644 --- a/tests/php-unit-tests/unitary-tests/application/LoginTest.php +++ b/tests/php-unit-tests/unitary-tests/application/LoginTest.php @@ -4,6 +4,9 @@ namespace Combodo\iTop\Test\UnitTest\Application; use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use MetaModel; +/** + * @runTestsInSeparateProcesses + */ class LoginTest extends ItopDataTestCase { protected $sConfigTmpBackupFile; protected $sConfigPath;