Refactor name bans, move code to separate class CNameBans

Unify all code for name bans in new class `CNameBans` in the existing `name_ban.cpp/h` files. The previously global function `IsNameBanned` is now the member function `CNameBans::IsBanned`. The existing name ban tests are extended for the `CNameBans` class.

Move `CNameBan` constructor definition to source file to avoid including `system.h` in the header file. Use `bool` instead of `int` for `m_IsSubstring`. Reorder `CNameBan` constructor arguments and remove unnecessary default value.

src/engine/server/name_ban.cpp

@@ -1,6 +1,92 @@
 #include "name_ban.h"
 
-CNameBan *IsNameBanned(const char *pName, std::vector<CNameBan> &vNameBans)
+#include <base/system.h>
+
+#include <engine/shared/config.h>
+
+CNameBan::CNameBan(const char *pName, const char *pReason, int Distance, bool IsSubstring) :
+	m_Distance(Distance), m_IsSubstring(IsSubstring)
+{
+	str_copy(m_aName, pName);
+	str_copy(m_aReason, pReason);
+	m_SkeletonLength = str_utf8_to_skeleton(m_aName, m_aSkeleton, std::size(m_aSkeleton));
+}
+
+void CNameBans::InitConsole(IConsole *pConsole)
+{
+	m_pConsole = pConsole;
+
+	m_pConsole->Register("name_ban", "s[name] ?i[distance] ?i[is_substring] ?r[reason]", CFGFLAG_SERVER, ConNameBan, this, "Ban a certain nickname");
+	m_pConsole->Register("name_unban", "s[name]", CFGFLAG_SERVER, ConNameUnban, this, "Unban a certain nickname");
+	m_pConsole->Register("name_bans", "", CFGFLAG_SERVER, ConNameBans, this, "List all name bans");
+}
+
+void CNameBans::Ban(const char *pName, const char *pReason, const int Distance, const bool IsSubstring)
+{
+	for(auto &Ban : m_vNameBans)
+	{
+		if(str_comp(Ban.m_aName, pName) == 0)
+		{
+			if(m_pConsole)
+			{
+				char aBuf[256];
+				str_format(aBuf, sizeof(aBuf), "changed name='%s' distance=%d old_distance=%d is_substring=%d old_is_substring=%d reason='%s' old_reason='%s'", pName, Distance, Ban.m_Distance, IsSubstring, Ban.m_IsSubstring, pReason, Ban.m_aReason);
+				m_pConsole->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
+			}
+			str_copy(Ban.m_aReason, pReason);
+			Ban.m_Distance = Distance;
+			Ban.m_IsSubstring = IsSubstring;
+			return;
+		}
+	}
+
+	m_vNameBans.emplace_back(pName, pReason, Distance, IsSubstring);
+	if(m_pConsole)
+	{
+		char aBuf[256];
+		str_format(aBuf, sizeof(aBuf), "added name='%s' distance=%d is_substring=%d reason='%s'", pName, Distance, IsSubstring, pReason);
+		m_pConsole->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
+	}
+}
+
+void CNameBans::Unban(const char *pName)
+{
+	auto ToRemove = std::remove_if(m_vNameBans.begin(), m_vNameBans.end(), [pName](const CNameBan &Ban) { return str_comp(Ban.m_aName, pName) == 0; });
+	if(ToRemove == m_vNameBans.end())
+	{
+		if(m_pConsole)
+		{
+			char aBuf[256];
+			str_format(aBuf, sizeof(aBuf), "name ban '%s' not found", pName);
+			m_pConsole->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
+		}
+	}
+	else
+	{
+		if(m_pConsole)
+		{
+			char aBuf[256];
+			str_format(aBuf, sizeof(aBuf), "removed name='%s' distance=%d is_substring=%d reason='%s'", (*ToRemove).m_aName, (*ToRemove).m_Distance, (*ToRemove).m_IsSubstring, (*ToRemove).m_aReason);
+			m_pConsole->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
+		}
+		m_vNameBans.erase(ToRemove, m_vNameBans.end());
+	}
+}
+
+void CNameBans::Dump() const
+{
+	if(!m_pConsole)
+		return;
+
+	char aBuf[256];
+	for(const auto &Ban : m_vNameBans)
+	{
+		str_format(aBuf, sizeof(aBuf), "name='%s' distance=%d is_substring=%d reason='%s'", Ban.m_aName, Ban.m_Distance, Ban.m_IsSubstring, Ban.m_aReason);
+		m_pConsole->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
+	}
+}
+
+const CNameBan *CNameBans::IsBanned(const char *pName) const
 {
 	char aTrimmed[MAX_NAME_LENGTH];
 	str_copy(aTrimmed, str_utf8_skip_whitespaces(pName));
@@ -10,12 +96,32 @@ CNameBan *IsNameBanned(const char *pName, std::vector<CNameBan> &vNameBans)
 	int SkeletonLength = str_utf8_to_skeleton(aTrimmed, aSkeleton, std::size(aSkeleton));
 	int aBuffer[MAX_NAME_SKELETON_LENGTH * 2 + 2];
 
-	CNameBan *pResult = nullptr;
-	for(CNameBan &Ban : vNameBans)
+	const CNameBan *pResult = nullptr;
+	for(const CNameBan &Ban : m_vNameBans)
 	{
 		int Distance = str_utf32_dist_buffer(aSkeleton, SkeletonLength, Ban.m_aSkeleton, Ban.m_SkeletonLength, aBuffer, std::size(aBuffer));
-		if(Distance <= Ban.m_Distance || (Ban.m_IsSubstring == 1 && str_utf8_find_nocase(pName, Ban.m_aName)))
+		if(Distance <= Ban.m_Distance || (Ban.m_IsSubstring && str_utf8_find_nocase(pName, Ban.m_aName)))
 			pResult = &Ban;
 	}
 	return pResult;
 }
+
+void CNameBans::ConNameBan(IConsole::IResult *pResult, void *pUser)
+{
+	const char *pName = pResult->GetString(0);
+	const char *pReason = pResult->NumArguments() > 3 ? pResult->GetString(3) : "";
+	const int Distance = pResult->NumArguments() > 1 ? pResult->GetInteger(1) : str_length(pName) / 3;
+	const bool IsSubstring = pResult->NumArguments() > 2 ? pResult->GetInteger(2) != 0 : false;
+	static_cast<CNameBans *>(pUser)->Ban(pName, pReason, Distance, IsSubstring);
+}
+
+void CNameBans::ConNameUnban(IConsole::IResult *pResult, void *pUser)
+{
+	const char *pName = pResult->GetString(0);
+	static_cast<CNameBans *>(pUser)->Unban(pName);
+}
+
+void CNameBans::ConNameBans(IConsole::IResult *pResult, void *pUser)
+{
+	static_cast<CNameBans *>(pUser)->Dump();
+}

src/engine/server/name_ban.h

@@ -1,7 +1,7 @@
 #ifndef ENGINE_SERVER_NAME_BAN_H
 #define ENGINE_SERVER_NAME_BAN_H
 
-#include <base/system.h>
+#include <engine/console.h>
 #include <engine/shared/protocol.h>
 
 #include <vector>
@@ -15,22 +15,31 @@ enum
 class CNameBan
 {
 public:
-	CNameBan() {}
-	CNameBan(const char *pName, int Distance, int IsSubstring, const char *pReason = "") :
-		m_Distance(Distance), m_IsSubstring(IsSubstring)
-	{
-		str_copy(m_aName, pName);
-		m_SkeletonLength = str_utf8_to_skeleton(m_aName, m_aSkeleton, std::size(m_aSkeleton));
-		str_copy(m_aReason, pReason);
-	}
+	CNameBan(const char *pName, const char *pReason, int Distance, bool IsSubstring);
+
 	char m_aName[MAX_NAME_LENGTH];
 	char m_aReason[MAX_NAMEBAN_REASON_LENGTH];
 	int m_aSkeleton[MAX_NAME_SKELETON_LENGTH];
 	int m_SkeletonLength;
 	int m_Distance;
-	int m_IsSubstring;
+	bool m_IsSubstring;
 };
 
-CNameBan *IsNameBanned(const char *pName, std::vector<CNameBan> &vNameBans);
+class CNameBans
+{
+	IConsole *m_pConsole = nullptr;
+	std::vector<CNameBan> m_vNameBans;
+
+	static void ConNameBan(IConsole::IResult *pResult, void *pUser);
+	static void ConNameUnban(IConsole::IResult *pResult, void *pUser);
+	static void ConNameBans(IConsole::IResult *pResult, void *pUser);
+
+public:
+	void InitConsole(IConsole *pConsole);
+	void Ban(const char *pName, const char *pReason, const int Distance, const bool IsSubstring);
+	void Unban(const char *pName);
+	void Dump() const;
+	const CNameBan *IsBanned(const char *pName) const;
+};
 
 #endif // ENGINE_SERVER_NAME_BAN_H

src/engine/server/server.cpp

@@ -403,7 +403,7 @@ bool CServer::SetClientNameImpl(int ClientID, const char *pNameRequest, bool Set
 	if(m_aClients[ClientID].m_State < CClient::STATE_READY)
 		return false;
 
-	CNameBan *pBanned = IsNameBanned(pNameRequest, m_vNameBans);
+	const CNameBan *pBanned = m_NameBans.IsBanned(pNameRequest);
 	if(pBanned)
 	{
 		if(m_aClients[ClientID].m_State == CClient::STATE_READY && Set)
@@ -3352,63 +3352,6 @@ void CServer::ConAuthList(IConsole::IResult *pResult, void *pUser)
 	pManager->ListKeys(ListKeysCallback, pThis);
 }
 
-void CServer::ConNameBan(IConsole::IResult *pResult, void *pUser)
-{
-	CServer *pThis = (CServer *)pUser;
-	char aBuf[256];
-	const char *pName = pResult->GetString(0);
-	const char *pReason = pResult->NumArguments() > 3 ? pResult->GetString(3) : "";
-	int Distance = pResult->NumArguments() > 1 ? pResult->GetInteger(1) : str_length(pName) / 3;
-	int IsSubstring = pResult->NumArguments() > 2 ? pResult->GetInteger(2) : 0;
-
-	for(auto &Ban : pThis->m_vNameBans)
-	{
-		if(str_comp(Ban.m_aName, pName) == 0)
-		{
-			str_format(aBuf, sizeof(aBuf), "changed name='%s' distance=%d old_distance=%d is_substring=%d old_is_substring=%d reason='%s' old_reason='%s'", pName, Distance, Ban.m_Distance, IsSubstring, Ban.m_IsSubstring, pReason, Ban.m_aReason);
-			pThis->Console()->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
-			Ban.m_Distance = Distance;
-			Ban.m_IsSubstring = IsSubstring;
-			str_copy(Ban.m_aReason, pReason);
-			return;
-		}
-	}
-
-	pThis->m_vNameBans.emplace_back(pName, Distance, IsSubstring, pReason);
-	str_format(aBuf, sizeof(aBuf), "added name='%s' distance=%d is_substring=%d reason='%s'", pName, Distance, IsSubstring, pReason);
-	pThis->Console()->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
-}
-
-void CServer::ConNameUnban(IConsole::IResult *pResult, void *pUser)
-{
-	CServer *pThis = (CServer *)pUser;
-	const char *pName = pResult->GetString(0);
-
-	for(size_t i = 0; i < pThis->m_vNameBans.size(); i++)
-	{
-		CNameBan *pBan = &pThis->m_vNameBans[i];
-		if(str_comp(pBan->m_aName, pName) == 0)
-		{
-			char aBuf[128];
-			str_format(aBuf, sizeof(aBuf), "removed name='%s' distance=%d is_substring=%d reason='%s'", pBan->m_aName, pBan->m_Distance, pBan->m_IsSubstring, pBan->m_aReason);
-			pThis->Console()->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
-			pThis->m_vNameBans.erase(pThis->m_vNameBans.begin() + i);
-		}
-	}
-}
-
-void CServer::ConNameBans(IConsole::IResult *pResult, void *pUser)
-{
-	CServer *pThis = (CServer *)pUser;
-
-	for(auto &Ban : pThis->m_vNameBans)
-	{
-		char aBuf[128];
-		str_format(aBuf, sizeof(aBuf), "name='%s' distance=%d is_substring=%d reason='%s'", Ban.m_aName, Ban.m_Distance, Ban.m_IsSubstring, Ban.m_aReason);
-		pThis->Console()->Print(IConsole::OUTPUT_LEVEL_STANDARD, "name_ban", aBuf);
-	}
-}
-
 void CServer::ConShutdown(IConsole::IResult *pResult, void *pUser)
 {
 	CServer *pThis = static_cast<CServer *>(pUser);
@@ -3852,10 +3795,6 @@ void CServer::RegisterCommands()
 	Console()->Register("auth_remove", "s[ident]", CFGFLAG_SERVER | CFGFLAG_NONTEEHISTORIC, ConAuthRemove, this, "Remove a rcon key");
 	Console()->Register("auth_list", "", CFGFLAG_SERVER, ConAuthList, this, "List all rcon keys");
 
-	Console()->Register("name_ban", "s[name] ?i[distance] ?i[is_substring] ?r[reason]", CFGFLAG_SERVER, ConNameBan, this, "Ban a certain nickname");
-	Console()->Register("name_unban", "s[name]", CFGFLAG_SERVER, ConNameUnban, this, "Unban a certain nickname");
-	Console()->Register("name_bans", "", CFGFLAG_SERVER, ConNameBans, this, "List all name bans");
-
 	RustVersionRegister(*Console());
 
 	Console()->Chain("sv_name", ConchainSpecialInfoupdate, this);
@@ -3880,6 +3819,7 @@ void CServer::RegisterCommands()
 
 	// register console commands in sub parts
 	m_ServerBan.InitServerBan(Console(), Storage(), this);
+	m_NameBans.InitConsole(Console());
 	m_pGameServer->OnConsoleInit();
 }
 

src/engine/server/server.h

@@ -282,7 +282,7 @@ public:
 
 	char m_aErrorShutdownReason[128];
 
-	std::vector<CNameBan> m_vNameBans;
+	CNameBans m_NameBans;
 
 	size_t m_AnnouncementLastLine;
 	std::vector<std::string> m_vAnnouncements;
@@ -429,10 +429,6 @@ public:
 	static void ConAuthRemove(IConsole::IResult *pResult, void *pUser);
 	static void ConAuthList(IConsole::IResult *pResult, void *pUser);
 
-	static void ConNameBan(IConsole::IResult *pResult, void *pUser);
-	static void ConNameUnban(IConsole::IResult *pResult, void *pUser);
-	static void ConNameBans(IConsole::IResult *pResult, void *pUser);
-
 	// console commands for sqlmasters
 	static void ConAddSqlServer(IConsole::IResult *pResult, void *pUserData);
 	static void ConDumpSqlServers(IConsole::IResult *pResult, void *pUserData);

src/test/name_ban.cpp

@@ -4,29 +4,77 @@
 
 TEST(NameBan, Empty)
 {
-	std::vector<CNameBan> vBans;
-	EXPECT_FALSE(IsNameBanned("", vBans));
-	EXPECT_FALSE(IsNameBanned("abc", vBans));
+	CNameBans Bans;
+	EXPECT_FALSE(Bans.IsBanned(""));
+	EXPECT_FALSE(Bans.IsBanned("abc"));
+}
+
+TEST(NameBan, BanInfo)
+{
+	CNameBans Bans;
+
+	Bans.Ban("abc", "old reason", 1, false);
+	{
+		const CNameBan *pOld = Bans.IsBanned("abc");
+		ASSERT_TRUE(pOld);
+		EXPECT_STREQ(pOld->m_aName, "abc");
+		EXPECT_STREQ(pOld->m_aReason, "old reason");
+		EXPECT_EQ(pOld->m_Distance, 1);
+		EXPECT_EQ(pOld->m_IsSubstring, false);
+	}
+
+	// Update existing name ban
+	Bans.Ban("abc", "new reason", 2, true);
+	{
+		const CNameBan *pNew = Bans.IsBanned("abc");
+		ASSERT_TRUE(pNew);
+		EXPECT_STREQ(pNew->m_aName, "abc");
+		EXPECT_STREQ(pNew->m_aReason, "new reason");
+		EXPECT_EQ(pNew->m_Distance, 2);
+		EXPECT_EQ(pNew->m_IsSubstring, true);
+	}
 }
 
 TEST(NameBan, Equality)
 {
-	std::vector<CNameBan> vBans;
-	vBans.emplace_back("abc", 0, 0);
-	EXPECT_TRUE(IsNameBanned("abc", vBans));
-	EXPECT_TRUE(IsNameBanned("   abc", vBans));
-	EXPECT_TRUE(IsNameBanned("abc   ", vBans));
-	EXPECT_TRUE(IsNameBanned("abc                   foo", vBans)); // Maximum name length.
-	EXPECT_TRUE(IsNameBanned("äbc", vBans)); // Confusables
-	EXPECT_FALSE(IsNameBanned("def", vBans));
-	EXPECT_FALSE(IsNameBanned("abcdef", vBans));
+	CNameBans Bans;
+	Bans.Ban("abc", "", 0, false);
+	EXPECT_TRUE(Bans.IsBanned("abc"));
+	EXPECT_TRUE(Bans.IsBanned("   abc"));
+	EXPECT_TRUE(Bans.IsBanned("abc   "));
+	EXPECT_TRUE(Bans.IsBanned("abc                   foo")); // Maximum name length.
+	EXPECT_TRUE(Bans.IsBanned("äbc")); // Confusables
+	EXPECT_FALSE(Bans.IsBanned("def"));
+	EXPECT_FALSE(Bans.IsBanned("abcdef"));
 }
 
 TEST(NameBan, Substring)
 {
-	std::vector<CNameBan> vBans;
-	vBans.emplace_back("xyz", 0, 1);
-	EXPECT_TRUE(IsNameBanned("abcxyz", vBans));
-	EXPECT_TRUE(IsNameBanned("abcxyzdef", vBans));
-	EXPECT_FALSE(IsNameBanned("abcdef", vBans));
+	CNameBans Bans;
+	Bans.Ban("xyz", "", 0, true);
+	EXPECT_TRUE(Bans.IsBanned("abcxyz"));
+	EXPECT_TRUE(Bans.IsBanned("abcxyzdef"));
+	EXPECT_FALSE(Bans.IsBanned("abcdef"));
+}
+
+TEST(NameBan, Unban)
+{
+	CNameBans Bans;
+	Bans.Ban("abc", "", 0, false);
+	Bans.Ban("xyz", "", 0, false);
+	EXPECT_TRUE(Bans.IsBanned("abc"));
+	EXPECT_TRUE(Bans.IsBanned("xyz"));
+	Bans.Unban("abc");
+	EXPECT_FALSE(Bans.IsBanned("abc"));
+	EXPECT_TRUE(Bans.IsBanned("xyz"));
+	Bans.Unban("xyz");
+	EXPECT_FALSE(Bans.IsBanned("abc"));
+	EXPECT_FALSE(Bans.IsBanned("xyz"));
+}
+
+TEST(NameBan, UnbanNotFound)
+{
+	// Try to remove a name ban that does not exist
+	CNameBans Bans;
+	Bans.Unban("abc");
 }