N°7633 - Reloads the same user multiple times if it no longer exists

Combodo · Dec 17, 2024 · 33ef112 · 33ef112
1 parent 0a3b02b
commit 33ef112
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 32 deletions.
diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php
@@ -1921,50 +1921,51 @@ public static function FlushPrivileges($bResetAdminCache = false)
 	 */
 	protected static function FindUser($sLogin, $sAuthentication = 'any', $bAllowDisabledUsers = false)
 	{
-		if ($sAuthentication == 'any')
+		if ($sAuthentication === 'any')
 		{
 			$oUser = self::FindUser($sLogin, 'internal');
-			if ($oUser == null)
+			if ($oUser !== null)
 			{
-				$oUser = self::FindUser($sLogin, 'external');
+				return $oUser;
 			}
+
+			return self::FindUser($sLogin, 'external');
 		}
-		else
+
+		if (!isset(self::$m_aCacheUsers))
 		{
-			if (!isset(self::$m_aCacheUsers))
-			{
-				self::$m_aCacheUsers = array('internal' => array(), 'external' => array());
-			}
+			self::$m_aCacheUsers = [ 'internal' => array(), 'external' => array() ];
+		}
 
-			if (!isset(self::$m_aCacheUsers[$sAuthentication][$sLogin]))
+		if (! isset(self::$m_aCacheUsers[$sAuthentication]) || ! array_key_exists($sLogin, self::$m_aCacheUsers[$sAuthentication]))
+		{
+			switch($sAuthentication)
 			{
-				switch($sAuthentication)
-				{
-					case 'external':
-					$sBaseClass = 'UserExternal';
-					break;
+				case 'external':
+				$sBaseClass = 'UserExternal';
+				break;
 
-					case 'internal':
-					$sBaseClass = 'UserInternal';
-					break;
+				case 'internal':
+				$sBaseClass = 'UserInternal';
+				break;
 
-					default:
-					echo "<p>sAuthentication = $sAuthentication</p>\n";
-					assert(false); // should never happen
-				}
-				$oSearch = DBObjectSearch::FromOQL("SELECT $sBaseClass WHERE login = :login");
-				$oSearch->AllowAllData();
-				if (!$bAllowDisabledUsers)
-				{
-					$oSearch->AddCondition('status', 'enabled');
-				}
-				$oSet = new DBObjectSet($oSearch, array(), array('login' => $sLogin));
-				$oUser = $oSet->fetch();
-				self::$m_aCacheUsers[$sAuthentication][$sLogin] = $oUser;
+				default:
+				echo "<p>sAuthentication = $sAuthentication</p>\n";
+				assert(false); // should never happen
 			}
-			$oUser = self::$m_aCacheUsers[$sAuthentication][$sLogin];
+			$oSearch = DBObjectSearch::FromOQL("SELECT $sBaseClass WHERE login = :login");
+			$oSearch->AllowAllData();
+			if (!$bAllowDisabledUsers)
+			{
+				$oSearch->AddCondition('status', 'enabled');
+			}
+			$oSet = new DBObjectSet($oSearch, array(), array('login' => $sLogin));
+			$oUser = $oSet->fetch();
+
+			self::$m_aCacheUsers[$sAuthentication][$sLogin] = $oUser;
 		}
-		return $oUser;
+
+		return self::$m_aCacheUsers[$sAuthentication][$sLogin];
 	}
 
 	/**

diff --git a/tests/php-unit-tests/unitary-tests/core/UserRightsTest.php b/tests/php-unit-tests/unitary-tests/core/UserRightsTest.php
@@ -488,4 +488,63 @@ public function NonAdminCannotListAdminProfilesProvider(): array
 			'with Admins hidden' => [true, 0],
 		];
 	}
+
+	public function testFindUser_internaluser()
+	{
+		$sLogin = 'admin'.uniqid();
+		$iKey = $this->CreateUser($sLogin, self::$aURP_Profiles['Administrator'])->GetKey();
+		$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+
+		$this->assertNotNull($oUser);
+		$this->assertEquals($iKey, $oUser->GetKey());
+		$this->assertEquals(\UserLocal::class, get_class($oUser));
+
+		$this->assertDBQueryCount(0, function() use ($sLogin, $iKey){
+			$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+			static::assertEquals($iKey, $oUser->GetKey());
+			static::assertEquals(\UserLocal::class, get_class($oUser));
+		});
+	}
+
+	public function testFindUserUnknownLogin_AvoidSameSearchAgain()
+	{
+		$sLogin = 'admin'.uniqid();
+		$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+		$this->assertNull($oUser);
+
+		$this->assertDBQueryCount(0, function() use ($sLogin){
+			$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+			$this->assertNull($oUser);
+		});
+	}
+
+	public function testFindUser_externaluser()
+	{
+		$sLogin = 'admin'.uniqid();
+
+		$oUserProfile = new URP_UserProfile();
+		$oUserProfile->Set('profileid', self::$aURP_Profiles['Administrator']);
+		$oUserProfile->Set('reason', 'UNIT Tests');
+		$oSet = DBObjectSet::FromObject($oUserProfile);
+		/** @var \UserLocal $oUser */
+		$oUser = $this->createObject(\UserExternal::class, array(
+			'login' => $sLogin,
+			'language' => 'EN US',
+			'profile_list' => $oSet,
+		));
+		$iKey = $oUser->GetKey();
+
+		$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+
+		$this->assertNotNull($oUser);
+		$this->assertEquals($iKey, $oUser->GetKey());
+		$this->assertEquals(\UserExternal::class, get_class($oUser));
+
+		$this->assertDBQueryCount(0, function() use ($sLogin, $iKey){
+			$oUser = $this->InvokeNonPublicStaticMethod(UserRights::class, "FindUser", [$sLogin]);
+			static::assertEquals($iKey, $oUser->GetKey());
+			static::assertEquals(\UserExternal::class, get_class($oUser));
+		});
+	}
+
 }