From a5bc2f77cb33d33e65156275b487852b8426824c Mon Sep 17 00:00:00 2001
From: Kawe Mazidjatari <48657826+Mauler125@users.noreply.github.com>
Date: Sat, 29 Apr 2023 20:36:29 +0200
Subject: [PATCH] Fix broken 'SV_IsClientBanned' implementation

SV_IsClientBanned is now checked after the creating of CClient, and thus 'CServer::AuthClient' has been inlined into 'CServer::ConnectClient'. Since we have to request the master server to check whether or not this particular client is globally banned, we have to detach it into a separate thread, and process the results back into the main thread once finished. This takes too much time, so we connect the client anyways and if the master server marked it as 'banned', we disconnect it right away.
---
 r5dev/engine/server/server.cpp  | 43 +++++++++++++--------------------
 r5dev/engine/server/server.h    |  1 -
 r5dev/engine/server/sv_main.cpp | 25 +++++++++++++------
 r5dev/engine/server/sv_main.h   |  3 ++-
 4 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/r5dev/engine/server/server.cpp b/r5dev/engine/server/server.cpp
index 2bb47c4e..d77021ea 100644
--- a/r5dev/engine/server/server.cpp
+++ b/r5dev/engine/server/server.cpp
@@ -79,12 +79,17 @@ int CServer::GetNumClients(void) const
 }
 
 //---------------------------------------------------------------------------------
-// Purpose: client to server authentication
-// Input  : *pChallenge - 
-// Output : true if user isn't banned, false otherwise
+// Purpose: Initializes a CSVClient for a new net connection. This will only be called
+//			once for a player each game, not once for each level change.
+// Input  : *pServer - 
+//			*pInpacket - 
+// Output : pointer to client instance on success, nullptr on failure
 //---------------------------------------------------------------------------------
-bool CServer::AuthClient(user_creds_s* pChallenge)
+CClient* CServer::ConnectClient(CServer* pServer, user_creds_s* pChallenge)
 {
+	if (pServer->m_State < server_state_t::ss_active)
+		return nullptr;
+
 	char* pszPersonaName = pChallenge->personaName;
 	uint64_t nNucleusID = pChallenge->personaId;
 
@@ -98,47 +103,33 @@ bool CServer::AuthClient(user_creds_s* pChallenge)
 	// Only proceed connection if the client's name is valid and UTF-8 encoded.
 	if (!VALID_CHARSTAR(pszPersonaName) || !IsValidUTF8(pszPersonaName) || !IsValidPersonaName(pszPersonaName))
 	{
-		RejectConnection(m_Socket, &pChallenge->netAdr, "#Valve_Reject_Invalid_Name");
+		pServer->RejectConnection(pServer->m_Socket, &pChallenge->netAdr, "#Valve_Reject_Invalid_Name");
 		if (bEnableLogging)
 			Warning(eDLL_T::SERVER, "Connection rejected for '%s' ('%llu' has an invalid name!)\n", pszAddresBuffer, nNucleusID);
 
-		return false;
+		return nullptr;
 	}
 
 	if (g_pBanSystem->IsBanListValid())
 	{
 		if (g_pBanSystem->IsBanned(pszAddresBuffer, nNucleusID))
 		{
-			RejectConnection(m_Socket, &pChallenge->netAdr, "#Valve_Reject_Banned");
+			pServer->RejectConnection(pServer->m_Socket, &pChallenge->netAdr, "#Valve_Reject_Banned");
 			if (bEnableLogging)
 				Warning(eDLL_T::SERVER, "Connection rejected for '%s' ('%llu' is banned from this server!)\n", pszAddresBuffer, nNucleusID);
 
-			return false;
+			return nullptr;
 		}
 	}
 
-	if (sv_globalBanlist->GetBool())
+	CClient* pClient = v_CServer_ConnectClient(pServer, pChallenge);
+
+	if (pClient && sv_globalBanlist->GetBool())
 	{
-		std::thread th(SV_IsClientBanned, string(pszAddresBuffer), nNucleusID, string(pszPersonaName));
+		std::thread th(SV_IsClientBanned, pClient, string(pszAddresBuffer), nNucleusID, string(pszPersonaName));
 		th.detach();
 	}
 
-	return true;
-}
-
-//---------------------------------------------------------------------------------
-// Purpose: Initializes a CSVClient for a new net connection. This will only be called
-//			once for a player each game, not once for each level change.
-// Input  : *pServer - 
-//			*pInpacket - 
-// Output : pointer to client instance on success, nullptr on failure
-//---------------------------------------------------------------------------------
-CClient* CServer::ConnectClient(CServer* pServer, user_creds_s* pChallenge)
-{
-	if (pServer->m_State < server_state_t::ss_active || !pServer->AuthClient(pChallenge))
-		return nullptr;
-
-	CClient* pClient = v_CServer_ConnectClient(pServer, pChallenge);
 	return pClient;
 }
 
diff --git a/r5dev/engine/server/server.h b/r5dev/engine/server/server.h
index a2681214..5483128c 100644
--- a/r5dev/engine/server/server.h
+++ b/r5dev/engine/server/server.h
@@ -42,7 +42,6 @@ public:
 	bool IsActive(void) const { return m_State >= server_state_t::ss_active; }
 	bool IsLoading(void) const { return m_State == server_state_t::ss_loading; }
 	bool IsDedicated(void) const { return m_bIsDedicated; }
-	bool AuthClient(user_creds_s* pChallenge);
 	void RejectConnection(int iSocket, netadr_t* pNetAdr, const char* szMessage);
 	static CClient* ConnectClient(CServer* pServer, user_creds_s* pChallenge);
 	static void RunFrame(CServer* pServer);
diff --git a/r5dev/engine/server/sv_main.cpp b/r5dev/engine/server/sv_main.cpp
index 685a8378..d538eda8 100644
--- a/r5dev/engine/server/sv_main.cpp
+++ b/r5dev/engine/server/sv_main.cpp
@@ -15,8 +15,10 @@
 //-----------------------------------------------------------------------------
 // Purpose: checks if particular client is banned on the comp server
 //-----------------------------------------------------------------------------
-void SV_IsClientBanned(const string& svIPAddr, const uint64_t nNucleusID, const string& svPersonaName)
+void SV_IsClientBanned(CClient* pClient, const string& svIPAddr, const uint64_t nNucleusID, const string& svPersonaName)
 {
+	Assert(pClient != nullptr);
+
 	string svError;
 	bool bCompBanned = g_pMasterServer->CheckForBan(svIPAddr, nNucleusID, svPersonaName, svError);
 
@@ -24,11 +26,19 @@ void SV_IsClientBanned(const string& svIPAddr, const uint64_t nNucleusID, const
 	{
 		if (!ThreadInMainThread())
 		{
-			g_TaskScheduler->Dispatch([svError, svIPAddr, nNucleusID]
+			g_TaskScheduler->Dispatch([pClient, svError, svIPAddr, nNucleusID]
 				{
-					g_pBanSystem->KickPlayerById(svIPAddr.c_str(), svError.c_str());
-					Warning(eDLL_T::SERVER, "Removed client '%s' ('%llu' is banned globally!)\n",
-						svIPAddr.c_str(), nNucleusID);
+					// Make sure client isn't already disconnected,
+					// and that if there is a valid netchannel, that
+					// it hasn't been taken by a different client by
+					// the time this task is getting executed.
+					CNetChan* pChan = pClient->GetNetChan();
+					if (pChan && pClient->GetNucleusID() == nNucleusID)
+					{
+						pClient->Disconnect(Reputation_t::REP_MARK_BAD, svError.c_str());
+						Warning(eDLL_T::SERVER, "Removed client '%s' ('%llu' is banned globally!)\n",
+							svIPAddr.c_str(), nNucleusID);
+					}
 				}, 0);
 		}
 	}
@@ -77,15 +87,16 @@ void SV_CheckForBan(const BannedVec_t* pBannedVec /*= nullptr*/)
 		const uint64_t nNucleusID = pClient->GetNucleusID();
 
 		if (!pBannedVec)
-			bannedVec.push_back(std::make_pair(szIPAddr, nNucleusID));
+			bannedVec.push_back(std::make_pair(string(szIPAddr), nNucleusID));
 		else
 		{
 			for (auto& it : *pBannedVec)
 			{
 				if (it.second == pClient->GetNucleusID())
 				{
-					Warning(eDLL_T::SERVER, "Removing client '%s' from slot '%i' ('%llu' is banned globally!)\n", szIPAddr, c, nNucleusID);
 					pClient->Disconnect(Reputation_t::REP_MARK_BAD, "%s", it.first.c_str());
+					Warning(eDLL_T::SERVER, "Removed client '%s' from slot '%i' ('%llu' is banned globally!)\n",
+						szIPAddr, c, nNucleusID);
 				}
 			}
 		}
diff --git a/r5dev/engine/server/sv_main.h b/r5dev/engine/server/sv_main.h
index 941f8eb8..4f31c323 100644
--- a/r5dev/engine/server/sv_main.h
+++ b/r5dev/engine/server/sv_main.h
@@ -3,6 +3,7 @@
 #include "networksystem/bansystem.h"
 
 ///////////////////////////////////////////////////////////////////////////////
+class CClient;
 
 /* ==== SV_MAIN ======================================================================================================================================================= */
 inline CMemory p_SV_InitGameDLL;
@@ -27,7 +28,7 @@ inline bool* s_bIsDedicated = nullptr;
 void SV_InitGameDLL();
 void SV_ShutdownGameDLL();
 bool SV_ActivateServer();
-void SV_IsClientBanned(const string& svIPAddr, const uint64_t nNucleusID, const string& svPersonaName);
+void SV_IsClientBanned(CClient* pClient, const string& svIPAddr, const uint64_t nNucleusID, const string& svPersonaName);
 void SV_CheckForBan(const BannedVec_t* pBannedVec = nullptr);
 ///////////////////////////////////////////////////////////////////////////////