[PATCH] Kick / ban players from a match

[elexis]

Sometimes people can be annoying and deserve to be kicked from the gamesetup and session.

If the host enters "/kick <playername>" into the gamesetup or session, then the player should be disconnected. Maybe a message should be sent to all players, so that this information can be displayed in the chat area and on a popup box on the kicked client. The kicking logic could be done on the client then as well, if that is easier to implement.

If the host enters "/ban <playername>" , the player should be unassigned or otherwise remembered, so that he/she cannot connect again (in that match).

Refs #700.

[leper]

Something like /list would probably be helpful.

That logic should be done server-side.

/ban should just work per IP and use some list on the server to check if a connecting player is in there and if yes don't do the handshake.

/unban seems unnecessary as you can just host a new game.

[elexis]

If its easy to implement, then one could go for the /mute /unmute commands as well. If not, then it will be done in another ticket...

[elexis]

• Allows the host to kick and ban players using the "/kick name" and "/ban name" chat command.
• The server bans players by username, so that they can't connect with different IPs (restarting the router to get a new dynamic IP)
• The server bans players by IP, so that they can't rejoin the game by IP (without lobby, main menu -> join game) with a changed username (#3305, #3242 or just joining with the name of another disconnected player).
• Side effect: If two players use the same IP address and one of them is banned, then the other can't rejoin in case of losing the connection.
• The host can't kick himself/herself.
• Implements the "/list" chat command, that displays all names of all connected players and observers.
• Adds GetPlayerGUID() function to accommodate kicking as observer and kicking other observers. This also fixes the private messages for observers error #3270 (allows sending PMs from player and observer to observer, but not from observer to player)
• Doesn't add arbitrary whitespace.

[elexis]

Next version, soon to be uploaded will be more transparent, so that people see the reason for server disconnects. Maybe this can help network analysis later too.

[elexis]

Should be finished by now. Most importantly fixes that everyone had the ability to kick.

[elexis]

Notice that I get an error if I use let instead of var here:

	case "/kick":
		let trimmed = msg.text.substr(split[0].length + 1);
	case "/msg":
		let trimmed = msg.text.substr(split[0].length + 1);

TypeError: redeclaration of variable trimmed

[Itms]

It's because trimmed is defined as var somewhere before (for instance in gamesetup.js, which is probably included before messages.js in the XML page).

You should just replace some vars by lets in other places (keep it minimal to avoid megapatches though).

[elexis]

Replying to Itms:

It's because trimmed is defined as var somewhere before

Nope, if you add let askdjflksjdflksjdfsd = ""; below the two definitions of trimmed then you get the same error.

[elexis]

See Temporal dead zone and errors with let on ​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

[elexis]

Changes var to let for all local variables in messages.js and in the touched gamesetup functions. Fixes whitespace in two empty lines.

[Itms]

Replying to Itms:

replace some vars by lets in other places (keep it minimal to avoid megapatches though).

I'm not sure why I wrote that, but maybe it was to avoid what you did?

Actually, most of your changes for var/let are OK, but you're making mistakes by merging variables that were separated before. For instance you define "message" l447 of messages.js. It is used in the switch(msg.type) block AND in the if ("translate" in msg) block, but both are not the same! So variables must be created inside both structures, with let. tl;dr it was nice before, except it used var, so don't change too many things.

• still whitespace!
• line 1706 of gamesetup, it's better to initialize the value of message because the following code doesn't guarantee it is set before being used.
• messages.js
  • l246, you should include the "no reason" situation in getDisconnectReason (and have "The game has ended" in that case). Also missing semicolon!
  • l638 missing semicolon
  • l685 : add a comment to tell it's a fall-through
  • remove the comments l702-703, both are useless
• CNetSession::GetIPAddress should be consistent with CNetServerWorker and send "error" instead of "(error)" by default.
• GameSetup.cpp, I don't understand what you're suggesting here (maybe create a ticket instead of adding that cryptic comment)

[elexis]

Minipatch instead of megapatch, whitespace, missing semicolon, always displaying 'The game has ended", added fall-through comment, removed cryptic comment. CNetSession::GetIPAddress is now consistent with CNetServerWorker::GetPlayerIPAddress, but both return "(error)" instead of "error", so that they are consistent with CNetServerWorker::RunStep.

[elexis]

Btw this tool is very useful if you want to rehost a match with the same players. You can kick everyone thats taking a slot and prevents the other players from joining.

[elexis]

Changed std::string("") to std::string(). Removed dots from non-sentences in network.js. The removal of trailing whitespace in two exisiting empty lines is intentional.

[elexis]

Surprise, replacing auto with CNetClientSession* works.

[elexis]

Removed for-each-loop. Removed optional braces. Fixed "if(". CStr instead of std::string. Initialized string in ctor. Fat arrow for sorting. Removed local variable ipAddress. Some empty lines. empty() instead of == "".

[elexis]

Introduces the net message CKickedMessage to correctly translate the messages on every client. Notice that it correctly works for non-ascii playernames too. Sanitizes usernames in non-lobby games to remove trailing whitespace. Moves more logic to C++ so that the JS code becomes more cleaner. This way the functionsGetHostGUID and GetPlayerGUID (which were introduced in the previous patch) are not needed anymore (They will have to be introduced in #3270 though). Removes partial username matching. Introduces the dot at the end of the getDisconnectReason string at the right place. Uses emplace_back instead of push_back and few whitespace fixes.

[elexis]

Edit:

Introduces the net message CKickedMessage to correctly translate the messages on every client. Notice that it correctly works for non-ascii playernames too

(That is if we wouldn't sanitize the playernames in gamesetup_mp.xml)

[elexis]

Uses an iterator, find_if and a lambda function to reduce CNetServerWorker::KickPlayer by 6 lines. Back to usingpush_back instead of emplace_back, because no elements on that vector are moved.

[elexis]

Increments PS_PROTOCOL_VERSION.

[elexis]

The previous patch broke /msg commands. Refactored the cheat and chat code to cleanly and uniformly implement the feature. Fixed sorting in that username-list.

[elexis]

Some more cleanup.

[Itms]

I'm waiting for #3242 to be committed: after that you'll have to fix some conflicts. When you do so, remember you can remove the protocol change thingy for the same reason I told you in #3242 :)

[elexis]

Rebased and added JSDoc comments.

[elexis]

Rebased after leper commited some parts of this patch in r16953. Also eliminated trim() call and reduced comments.

[elexis]

... and some parts have been commited with r16958.

[elexis]

Rebased & minimized by removing cleanup. Doesn't sanitize playernames anymore since they can contain whitespace due to the rating anyway. In case autocomplete fails, /list will show the full name of everybody.

[elexis]

We have continous attacks on many lobby games since some weeks. Since we still need some time to fix the pathfinder, maybe someone finds time to commit this.

Having this feature without translations would still be better than having it not at all. Thus we can avoid breaking the string freeze.

See also lepers quick review on IRC on october 23rd:

(03:44:05) leper: looks sane (then again I'm not sure if I should be reviewing patches currently, but if you can get hold of someone with commit privs and convince them to break string freeze (tell them that it is for a greater good and that I totally didn't say so) that they might want to take a look at it and commit it (03:45:25) leper: (in case they do they should also post an announcement on transifex explaining that we encountered some release blocking bugs and the release is delayed until we can fix those; still translators should update/complete their translations to get above the 90% threshold)

[Josh]

A few comments:

• On NetSession.cpp line 181, you use ARRAY_SIZE which is effectively strlen. enet_address_get_host ​takes the maximum length, not the actual length. The maximum length of an IP Address is fixed. This, including the null terminator, is only 40 characters for IPv6. You give 256. You also don't handle the case in that function where enet returns an error in an ideal fashion. It would be best to log a warning message and maybe even return an error code.
• NetServer.cpp lines 729 and 730 are unreadable. Less lines do not imply better code.
• I would strongly recommend having KickPlayer in NetServer.cpp return a success/failure code. That code should be returned all the way back into JS where a message is printed to the host relaying the results of his/her command.
• The implementation of getUsernameList() in network.js does not strike me as particularly readable.

[leper]

Why /clientlist instead of just /list?

The std::find() plus lambda is fine, the indentation could use some work, but dunno how to make that nicer.

getDisconnectReason() should keep that comment about keeping it in sync.

[elexis]

Replying to Josh:

• On NetSession.cpp line 181, you use ARRAY_SIZE which is effectively strlen.

Unfortunately enet_address_get_host returns an error code when using strlen, but not when using ARRAY_SIZE (the way it is done in NetServer.cpp too). I changed it to 40 characters and also updated NetServer to use the new function.

It would be best to log a warning message and maybe even return an error code.

Roger. Now the host gets that displayed when trying to ban him/herself.

• NetServer.cpp lines 729 and 730 are unreadable. Less lines do not imply better code.
• The implementation of getUsernameList() in network.js does not strike me as particularly readable.

We had many discussions about the use of less known functions like lambda functions in c++ or array functions in JS. I will continue to use them as I think they are better once one gets used to the syntax. See also lepers comment. I added comments and newlines to the JS function.

• I would strongly recommend having KickPlayer in NetServer.cpp return a success/failure code. That code should be returned all the way back into JS where a message is printed to the host relaying the results of his/her command.

Implemented that.

---

Replying to leper:

Why /clientlist instead of just /list?

It is

getDisconnectReason() should keep that comment about keeping it in sync.

Done.

Also added the translation comment to "Reason: %(reason)s." as you mentioned on IRC 24th october.

Thanks for the reviews!

[elexis]

Uses MAX_IP_LEN instead of 256.

[leper]

Replying to Josh:

• On NetSession.cpp line 181, you use ARRAY_SIZE which is effectively strlen.

No, it isn't. Check the definition of ARRAY_SIZE, understand it, then use it again. Using 40 there seems strange, 256 isn't that wasteful. Also that LEN define seems useless.

[elexis]

In that case use attachment:t3241_kick_v6.5.patch​. It uses ARRAY_SIZE (though 40 characters, which indeed works for IPv4 and IPv6 addresses)

[elexis]

Same with ARRAY_SIZE and 256

[Josh]

Replying to leper:

Replying to Josh:

• On NetSession.cpp line 181, you use ARRAY_SIZE which is effectively strlen.

No, it isn't. Check the definition of ARRAY_SIZE, understand it, then use it again. Using 40 there seems strange, 256 isn't that wasteful. Also that LEN define seems useless.

Whoops, you are right. It's a compile-time computation. Apologies. (I've been writing too much embedded C recently)

I guess that means that stylistically, the patch looks good. We'll still need to do some functionality testing before committing it.

[elexis]

Changed some more things after some remarks by leper on irc:

• doesn't show a messageBox but a chat message on failed kick
• introduces a "system" message to prevent introducing very similar message types
• rearranged reportDisconnect() to add a translation comment and uses this translation comment in messages.js
• replaced "(error)" with empty string when failing to get the IP address
• removed whitespace in getUsernameList()

[leper]

In 17217:

Allow kicking/banning players from gamesetup and session. Patch by elexis. Fixes #3241.

[leper]

Thanks.

[elexis]

In 18140:

Network cleanup, fixes #3953.

Don't compare for "127.0.0.1" to identify the host, but check for a new boolean flag that is set by the client, refs #2854.
Remove an unneeded IP address conversion from u32 to string, refs #3241.

[elexis]

(set component to network)

[elexis]

In 18391:

Colorize playernames in the kick/ban chat notification, refs #3241.

[elexis]

In 18859:

Don't throw FSM update errors (type=16 state=6) when kicking clients while others haven't finished the authentication, loading screen or synchronization yet, refs #3241.

[elexis]

In 19007:

Add chat command to kick or ban all observers, refs #3241.

This is in particular useful as many observers can join in the gamesetup phase
(since the server doesn't restrict connects then due to not knowing in advance which client will become an assigned player.)

[elexis]

In 21918:

Use a banmask for multiplayer matches that have lobby-authentication enabled.
This prevents a lobby player banned by the host from rejoining after getting a new IP address and changing the rating part of the nickname, refs #5320, #3241 / rP17217, #3549 / rP21520 / D897.

Differential Revision: ​https://code.wildfiregames.com/D1655
Reproduced By: Hannibal_Barca