[PATCH] Prohibit joins with an existing name

[elexis]

#3242 has a side-effect. If the setting is enabled and a user has a network timeout, the client will usually disconnect and start rejoining before the server notices that the client has left.

Since the server thinks that the client is still online, the rejoining actual player will be considered an observer.

This has happened already multiple times in svn matches and since people don't know what's going on, they think it's a bug on their side.

To alleviate this problem, we should just reject joins with an existing name instead of deduplicating the playername in NetServer.cpp:

bool CNetServerWorker::OnAuthenticate(void* context, CFsmEvent* event)
{
	...
	CStrW username = server.DeduplicatePlayerName(SanitisePlayerName(message->m_Name));
	...
}

[elexis]

I fear this issue might appear daily if people have the feature enabled, so it would be nice to prevent that spam for a19.

[leper]

String freeze.

[elexis]

Didn't know we were in string freeze again. Here the same without new strings.

All previous svn games had the late-observer join enabled and the issue has occured about 5 times already.

People also don't know that they need to rejoin to solve it in that case.

[Stan]

Pushing it due to yesterday's conversation and the fact we are in commit freeze.

[elexis]

The bug described above could be fixed by not deduplicating the playername only in case of having a running game with late-observer-joins enabled.

But there is also another bug with deduplicating the playername, reproduce:

Host with player1, name Y. join with player2, name X join with player3, name X (will be mapped to X (2)) leave with player 2 and 3 rejoin with player 3, name X will be mapped to X instead of X (2)

This could be fixed by remembering the playername (which would require changing CAuthenticateResultMessage). It also means everytime a developer joins a local testgame with multiple instances of 0ad without changing the name, a "(2)" is added to the name each time. I.e. after doing that thrice, the name will be "name (2) (2) (2)". The reward of that code is not worth the effort nor code-complexity imo.

Hence I'd go with just removing the deduplication code instead, showing the user an error message in case the name is already in use and letting the developer type one key when joining the local testgame.

[elexis]

Following a discussion with Itms: doesn't remove the DeduplicatePlayer implementation but makes it optional for developers. Adds a hint to the disconnect reason that players should retry if they got disconnected.

[elexis]

In 17851:

Make the playername-deduplication ("User" -> "User (2)") optional.
Have it disabled by default to fix #3604.
Prevents players from rejoining as late-observers in case they timed-out on the client-side but not on the server-side.

[elexis]

Thanks Itms for reviewing the code!

See the irclogs of 2 days ago for further proposed optimizations. For example Philip mentioned that the server could automatically kick the timed-out client as soon as the same player attempts to rejoin. Since this can be abused (kicking players by trying to rejoin as them) one would have to do some security check, i.e. identify the player by (1) the same IP, (2) some shared secret or (3) only disconnect in case of not having received any packets of that client since few seconds. Without a shared-secret, it won't be secure. Since we have lag- and timeout-warnings now (#3264), this case is already sufficiently covered. But if we have password-protection sometime, this feature can be implemented.

[elexis]

In 17860:

Optionnames are in lower case, refs #3604.

[elexis]

(changed component to network)