#3604 closed defect (fixed)
[PATCH] Prohibit joins with an existing name
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 20 |
Component: | Network | Keywords: | patch |
Cc: | Patch: |
Description
#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)); ... }
Attachments (4)
Change History (13)
by , 8 years ago
Attachment: | t3604_stop_rejoin_confusion_v1.patch added |
---|
comment:1 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 19 |
Summary: | Prohibit joins with an existing name → [PATCH] Prohibit joins with an existing name |
by , 8 years ago
Attachment: | t3604_stop_rejoin_confusion_v2.patch added |
---|
comment:3 by , 8 years ago
Milestone: | Alpha 20 → Alpha 19 |
---|
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.
comment:4 by , 8 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
Pushing it due to yesterday's conversation and the fact we are in commit freeze.
comment:5 by , 8 years ago
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.
by , 8 years ago
Attachment: | t3604_stop_rejoin_confusion_v3.patch added |
---|
by , 8 years ago
Attachment: | t3604_stop_rejoin_confusion_v4.patch added |
---|
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.
comment:7 by , 8 years ago
Keywords: | review removed |
---|
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.
I fear this issue might appear daily if people have the feature enabled, so it would be nice to prevent that spam for a19.