Opened 4 years ago

Closed 3 years ago

Last modified 8 weeks ago

#3241 closed enhancement (fixed)

[PATCH] Kick / ban players from a match

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 19
Component: Network Keywords: patch
Cc: Patch:

Description

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.

Attachments (20)

t3241_kick_wip_1.patch (22.7 KB) - added by elexis 3 years ago.
kick_v2.png (146.3 KB) - added by elexis 3 years ago.
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.
t3241_kick_v2.patch (25.1 KB) - added by elexis 3 years ago.
Should be finished by now. Most importantly fixes that everyone had the ability to kick.
t3241_kick_v3.patch (39.1 KB) - added by elexis 3 years ago.
Changes var to let for all local variables in messages.js and in the touched gamesetup functions. Fixes whitespace in two empty lines.
t3241_kick_v4.patch (25.9 KB) - added by elexis 3 years ago.
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?.
t3241_kick_v4.1.patch (25.6 KB) - added by elexis 3 years ago.
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.
t3241_kick_v4.2.patch (25.6 KB) - added by elexis 3 years ago.
Surprise, replacing auto with CNetClientSession* works.
t3241_kick_v4.3.patch (25.6 KB) - added by elexis 3 years ago.
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 == "".
t3241_kick_v5.patch (27.9 KB) - added by elexis 3 years ago.
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.
t3241_kick_v5.1.patch (27.8 KB) - added by elexis 3 years ago.
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.
t3241_kick_v5.2.patch (28.4 KB) - added by elexis 3 years ago.
Increments PS_PROTOCOL_VERSION.
t3241_kick_v6.patch (36.7 KB) - added by elexis 3 years ago.
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.
t3241_kick_v6.1.patch (39.7 KB) - added by elexis 3 years ago.
Some more cleanup.
t3241_kick_v6.2.patch (40.1 KB) - added by elexis 3 years ago.
Rebased and added JSDoc comments.
t3241_kick_v6.3.patch (28.2 KB) - added by elexis 3 years ago.
Rebased after leper commited some parts of this patch in r16953. Also eliminated trim() call and reduced comments.
t3241_kick_v6.4.patch (25.6 KB) - added by elexis 3 years ago.
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.
t3241_kick_v6.5.patch (26.8 KB) - added by elexis 3 years ago.
t3241_kick_v6.6.patch (27.7 KB) - added by elexis 3 years ago.
Uses MAX_IP_LEN instead of 256.
t3241_kick_v6.5.2.patch (26.8 KB) - added by elexis 3 years ago.
Same with ARRAY_SIZE and 256
t3241_kick_v6.6.2.patch (27.5 KB) - added by elexis 3 years ago.

Download all attachments as: .zip

Change History (50)

comment:1 Changed 4 years ago by 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.

comment:2 Changed 4 years ago by 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...

Changed 3 years ago by elexis

Attachment: t3241_kick_wip_1.patch added

comment:3 Changed 3 years ago by elexis

Keywords: patch review added
Milestone: BacklogAlpha 19
Summary: Kick / ban players from a match[PATCH] Kick / ban players from a match
  • 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.

Changed 3 years ago by elexis

Attachment: kick_v2.png added

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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v2.patch added

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

comment:4 Changed 3 years ago by 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

comment:5 Changed 3 years ago by 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).

comment:6 in reply to:  5 Changed 3 years ago by 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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v3.patch added

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

comment:8 in reply to:  5 Changed 3 years ago by 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)

comment:9 Changed 3 years ago by leper

Keywords: review removed

Changed 3 years ago by elexis

Attachment: t3241_kick_v4.patch added

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?.

comment:10 Changed 3 years ago by elexis

Keywords: review added

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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v4.1.patch added

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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v4.2.patch added

Surprise, replacing auto with CNetClientSession* works.

Changed 3 years ago by elexis

Attachment: t3241_kick_v4.3.patch added

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 == "".

Changed 3 years ago by elexis

Attachment: t3241_kick_v5.patch added

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.

comment:11 Changed 3 years ago by 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)

Changed 3 years ago by elexis

Attachment: t3241_kick_v5.1.patch added

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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v5.2.patch added

Increments PS_PROTOCOL_VERSION.

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.patch added

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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.1.patch added

Some more cleanup.

comment:12 Changed 3 years ago by 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 :)

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.2.patch added

Rebased and added JSDoc comments.

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.3.patch added

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

comment:13 Changed 3 years ago by elexis

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

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.4.patch added

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.

comment:14 Changed 3 years ago by Itms

Milestone: Alpha 19Alpha 20

comment:15 Changed 3 years ago by elexis

Milestone: Alpha 20Alpha 19
Priority: Should HaveMust Have

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)

comment:16 Changed 3 years ago by Josh

Keywords: review removed
Owner: set to elexis

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.

comment:17 Changed 3 years ago by 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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.5.patch added

comment:18 in reply to:  17 Changed 3 years ago by elexis

Keywords: reviewed added

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!

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.6.patch added

Uses MAX_IP_LEN instead of 256.

comment:19 in reply to:  16 ; Changed 3 years ago by 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.

comment:20 Changed 3 years ago by 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)

Last edited 3 years ago by elexis (previous) (diff)

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.5.2.patch added

Same with ARRAY_SIZE and 256

comment:21 in reply to:  19 Changed 3 years ago by 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.

Changed 3 years ago by elexis

Attachment: t3241_kick_v6.6.2.patch added

comment:22 Changed 3 years ago by 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()

comment:23 Changed 3 years ago by leper

Resolution: fixed
Status: newclosed

In 17217:

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

comment:24 Changed 3 years ago by leper

Keywords: reviewed removed

Thanks.

comment:25 Changed 3 years ago by 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.

comment:26 Changed 3 years ago by elexis

Component: UI & SimulationNetwork

(set component to network)

comment:27 Changed 3 years ago by elexis

In 18391:

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

comment:28 Changed 2 years ago by 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.

comment:29 Changed 2 years ago by 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.)

comment:30 Changed 8 weeks ago by 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

Note: See TracTickets for help on using tickets.