Make kick and ban messages and logs clearer

They now tell you if the person you're banning is actually already banned, if there was an error banning them etc.
drawpile · Dec 3, 2024 · 15c6bc6 · 15c6bc6
1 parent 80734ce
commit 15c6bc6
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 39 deletions.
diff --git a/src/libserver/opcommands.cpp b/src/libserver/opcommands.cpp
@@ -228,67 +228,95 @@ getClient(Session *session, const QJsonValue &idOrName, Client *&outClient)
 CmdResult
 kickUser(Client *client, const QJsonArray &args, const QJsonObject &kwargs)
 {
-	if(args.size() != 1)
-		return CmdResult::err("Expected one argument: user ID or name");
+	if(args.size() != 1) {
+		return CmdResult::err(
+			QStringLiteral("Expected one argument: user ID or name"));
+	}
 
-	const bool ban = kwargs["ban"].toBool();
+	bool ban = kwargs[QStringLiteral("ban")].toBool();
+	Session *session = client->session();
 
-	if(ban && client->session()->hasPastClientWithId(args.at(0).toInt())) {
+	if(ban && session->hasPastClientWithId(args.at(0).toInt())) {
 		// Retroactive ban
-		const auto target =
-			client->session()->getPastClientById(args.at(0).toInt());
-		if(target.isBannable) {
-			client->session()->addPastBan(target, client->username(), client);
-			client->session()->keyMessageAll(
-				target.username + " banned by " + client->username(), false,
-				net::ServerReply::KEY_BAN,
+		const Session::PastClient &target =
+			session->getPastClientById(args.at(0).toInt());
+		if(session->isPastBanned(target)) {
+			return CmdResult::err(
+				QStringLiteral("%1 is already banned.").arg(target.username));
+		} else if(!target.isBannable) {
+			return CmdResult::err(
+				QStringLiteral("%1 cannot be banned.").arg(target.username));
+		} else if(!session->addPastBan(target, client->username(), client)) {
+			return CmdResult::err(
+				QStringLiteral("Error banning %1.").arg(target.username));
+		} else {
+			session->keyMessageAll(
+				QStringLiteral("%1 banned by %2")
+					.arg(target.username, client->username()),
+				false, net::ServerReply::KEY_BAN,
 				{{QStringLiteral("target"), target.username},
 				 {QStringLiteral("by"), client->username()}});
 			return CmdResult::ok();
-		} else {
-			return CmdResult::err(target.username + " cannot be banned.");
 		}
 	}
 
 	Client *target;
-	CmdResult result = getClient(client->session(), args.at(0), target);
+	CmdResult result = getClient(session, args.at(0), target);
 	if(!result.success()) {
 		return result;
 	}
 
-	if(target == client)
-		return CmdResult::err("cannot kick self");
+	if(target == client) {
+		return CmdResult::err(QStringLiteral("cannot kick self"));
+	}
 
-	if(target->isModerator())
-		return CmdResult::err("cannot kick moderators");
+	if(target->isModerator()) {
+		return CmdResult::err(QStringLiteral("cannot kick moderators"));
+	}
 
 	if(client->isDeputy()) {
-		if(target->isOperator() || target->isTrusted())
-			return CmdResult::err("cannot kick trusted users");
+		if(target->isOperator()) {
+			return CmdResult::err(QStringLiteral("cannot kick operators"));
+		} else if(target->isTrusted()) {
+			return CmdResult::err(QStringLiteral("cannot kick trusted users"));
+		}
 	}
 
-	if(ban) {
-		client->session()->keyMessageAll(
-			target->username() + " banned by " + client->username(), false,
-			net::ServerReply::KEY_BAN,
-			{{QStringLiteral("target"), target->username()},
-			 {QStringLiteral("by"), client->username()}});
+	bool alreadyBanned = ban && session->isBanned(target);
+	bool banFailed = false;
+	if(ban && !alreadyBanned) {
+		if(session->addBan(target, client->username(), client)) {
+			session->keyMessageAll(
+				QStringLiteral("%1 banned by %2")
+					.arg(target->username(), client->username()),
+				false, net::ServerReply::KEY_BAN,
+				{{QStringLiteral("target"), target->username()},
+				 {QStringLiteral("by"), client->username()}});
+		} else {
+			banFailed = true;
+		}
 		target->disconnectClient(
-			Client::DisconnectionReason::Kick, client->username(),
-			QStringLiteral("via op command"));
-		client->session()->addBan(target, client->username(), client);
+			Client::DisconnectionReason::Kick, client->username(), QString());
 	} else {
 		target->disconnectClient(
-			Client::DisconnectionReason::Kick, client->username(),
-			QStringLiteral("via op command"));
-		client->session()->keyMessageAll(
-			target->username() + " kicked by " + client->username(), false,
-			net::ServerReply::KEY_KICK,
+			Client::DisconnectionReason::Kick, client->username(), QString());
+		session->keyMessageAll(
+			QStringLiteral("%1 kicked by %2")
+				.arg(target->username(), client->username()),
+			false, net::ServerReply::KEY_KICK,
 			{{QStringLiteral("target"), target->username()},
 			 {QStringLiteral("by"), client->username()}});
 	}
 
-	return CmdResult::ok();
+	if(alreadyBanned) {
+		return CmdResult::err(
+			QStringLiteral("%1 is already banned.").arg(target->username()));
+	} else if(banFailed) {
+		return CmdResult::err(
+			QStringLiteral("Error banning %1.").arg(target->username()));
+	} else {
+		return CmdResult::ok();
+	}
 }
 
 CmdResult

diff --git a/src/libserver/session.cpp b/src/libserver/session.cpp
@@ -477,7 +477,7 @@ Client *Session::getClientByUsername(const QString &username)
 	return nullptr;
 }
 
-void Session::addBan(
+bool Session::addBan(
 	const Client *target, const QString &bannedBy, const Client *client)
 {
 	Q_ASSERT(target);
@@ -488,10 +488,13 @@ void Session::addBan(
 						.about(Log::Level::Info, Log::Topic::Ban)
 						.message("Banned by " + bannedBy));
 		sendUpdatedBanlist();
+		return true;
+	} else {
+		return false;
 	}
 }
 
-void Session::addPastBan(
+bool Session::addPastBan(
 	const PastClient &target, const QString &bannedBy, const Client *client)
 {
 	Q_ASSERT(target.id > 0);
@@ -503,9 +506,25 @@ void Session::addPastBan(
 				.about(Log::Level::Info, Log::Topic::Ban)
 				.message("Banned by " + bannedBy));
 		sendUpdatedBanlist();
+		return true;
+	} else {
+		return false;
 	}
 }
 
+bool Session::isBanned(const Client *target) const
+{
+	return m_history->banlist().isBanned(
+		target->username(), target->peerAddress(), target->authId(),
+		target->sid());
+}
+
+bool Session::isPastBanned(const PastClient &target) const
+{
+	return m_history->banlist().isBanned(
+		target.username, target.peerAddress, target.authId, target.sid);
+}
+
 void Session::removeBan(int entryId, const QString &removedBy)
 {
 	QString unbanned = m_history->removeBan(entryId);

diff --git a/src/libserver/session.h b/src/libserver/session.h
@@ -165,14 +165,17 @@ class Session : public QObject, public sessionlisting::Announcable {
 	/**
 	 * @brief Add an in-session IP ban for the given client
 	 */
-	void addBan(
+	bool addBan(
 		const Client *target, const QString &bannedBy,
 		const Client *client = nullptr);
 
-	void addPastBan(
+	bool addPastBan(
 		const PastClient &target, const QString &bannedBy,
 		const Client *client = nullptr);
 
+	bool isBanned(const Client *target) const;
+	bool isPastBanned(const PastClient &target) const;
+
 	/**
 	 * @brief Remove a session specific IP ban
 	 * @param entryId ban entry ID