Merge lp:~widelands-dev/widelands/net-uuid into lp:widelands

[Notabilis]

Improved the relogin code by adding semi-constant - 12 hour valid - user ids.
Also improved doubled logins with registered accounts.

Improved the relogin code by adding semi-constant user ids.

Reconnecting to the same name on the metaserver should now work for most cases, even when the complete client computer crashes. The id is used to recognize the user and assign his old name to him instead of giving him the requested name. When the user does a clean logout the reservation is removed.

If logging in with a registered account a second time, a new name will be assigned to the new client automatically instead of displaying an error message.

This branch should be merged at the same time as deploying the related branch[1] of the metaserver, since compatibility with the current metaserver version breaks.

[1] https://github.com/widelands/widelands_metaserver/pull/13

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2704. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/287726199.
Appveyor build 2519. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_uuid-2519.

[GunChleoc (gunchleoc)]

Looks mostly OK - I have added some nits.

We should test to make sure that 2 Widelands instances can be run from the same computer and join the same game. I need to figure out how to run a metaserver.

[Notabilis (notabilis27)]

Thanks for the review! And for the grammar errors... ehem.

After some testing I am no longer so sure about the trimming of chat messages. Currently it is possible to send space-only messages to everyone or as a whisper message to a single player. So we should probably remove trailing (and leading?) space from all messages. Shall I do so? (A slight disadvantage would be that I can no longer explain the @whisper syntax by prepending it with a space.)

The first NOCOM is related to the fact that the SendMessage m is written to but the SendMessage s is send. It looks pretty broken to me, but has no-one ever tried to send an announcement and discovered this bug? I would just like to have a short confirmation that this is a bug (looking at the code is good enough for me).

As far as I understand it (language-wise), sent_uuid is correct. The variable is a boolean specifying whether the uuid should be sent, not the uuid itself. (The checkbox in the login dialog.)

I can't reproduce the bug of the second NOCOM, probably it was something unrelated to the code. Maybe on layer 8.

All on my local computer: Starting a metaserver and three clients. All Clients try to login with the name "Notabilis", second and third clients receive the names "Notabilis1" and "Notabilis2" respectively. First client opens a game, other two join it. As far as I can tell, everything worked fine.
If this answers your test, fine. But feel free to do some tests on your own. If you want me to setup a metaserver for it, just say so. (Hint: There is a --metaserver=IP command line switch in the game. Took me much too long to notice it.)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2747. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/296944647.
Appveyor build 2559. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_uuid-2559.

[GunChleoc (gunchleoc)]

I opened a new issue over at the metaserver, because I don't know how to run one. So, I'll just rely on your testing here.

As to the 2 NOCOMs, best open new bugs for them and note that they need investigating whether they are bugs.

As to the trimming, you could use it for an .empty() check only to prevent sending empty messages. The font renderer does some trimming on its own, so it shouldn't be an issue when the messages are displayed. For explaining the @whisper command, you could also rephrase it to have the explanation start with "Use @whisper at the beginning of the line to... "

Feel free to merge and deploy this once your'e ready.

review: Approve

[Notabilis (notabilis27)]

The NOCOM-bug is really a bug and has been fixed. I fear I introduced it myself a few month ago.
Trimming is also added now. Space is removed in front and at the end of messages. When this results in an empty message, it is dropped. This is also done with the message-part of the @whisper, /motd and /announcement commands.

As far as I am concerned, this can be merged as soon as the metaserver is ready.

[GunChleoc (gunchleoc)]

Changes LGTM :)

[SirVer (sirver)]

Reviewing now. Sorry that this waited so long for me.

1) third_party/crypto is under "public domain" which is not a license in most parts of the world. I expect issues with debian about this. Can we ask the author to relicense under Apache 2 or MIT? Thes licenses are mostly what the author intended with public domain, but more formal. We can merge this before that happens of course.

2) I am not a fan of 'send_uuid' as a variable name - its semantics are unclear. I do not understand if it means: 'client expects server to send a uuid' or 'server expects client to send a uuid'. Maybe that becomes clearer while I read on, but a better name would be appreciated.

3) internet_gaming_protocol.h: remove all mentions of the nounce implementation? e.g. you write that the UUID replaces the nounce. But the nounce was only an interim version of the metaserver that we no longer support - so why keep this historic backage in the documentation?

I added some consts, and a few clarifying comments. After addressing these changes this lgtm.

Will review the go code next.

review: Needs Fixing

[Notabilis (notabilis27)]

Thanks for the review!

1) I replaced the third party crypto code with a call to a boost function. This means that instead of SHA-3 it uses SHA-1 now. This is less secure (not that it matters in this case) but SHA-1 will most likely be needed in a future branch of mine anyway. A slight problem with the boost function: It is part of a ::detail:: namespace so it might become removed at some time.

2) I renamed send_uuid to use_permanent_uuid. Not sure if this is better. What is an improvement is that I removed the variable from the InternetGaming class. It does not need to now whether it is a temporary or permanent ID and calling classes can handle this themselves.

3) Done.

Another point: I replaced my primitive "random string" code with boost::uuid. And then I realized that a simple random number would be just as good. So we can remove the random_string() function if we want to.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2836. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/306952777.
Appveyor build 2646. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_uuid-2646.

[SirVer (sirver)]

1) SHA1 is sufficient for our crypto needs for sure. And using a boost function is preferable for now too. We deal with it going away once it happens. Thanks!

2) I was thinking about this a bit and I suggest removing the option and always use the "permanent", i.e. 24h valid UUID. We could reduce the 24 hours to 2h or 5h or so to make sure the UUID shuffles daily and does not become personally identifiable information.

Its only use is to make reconnecting possible with the same nickname, for this we do not need the uuid to stay alive long - it must basically survive the crash and restart of Widelands.

The option is very confusing for users - they need to know what a UUID is and understand how it is used inside of the metaserver. That is an impossible ask. Also, relogin into the same ID is expected behavior for users, so even those that would opt out would still not be happy that they get a new ID assigned on relogin.

So: Get rid of the option and make the behavior default?

3) Yes please! I like uuid() much better and it conveys the semantic better than random_string(). I also like that we use a true uuid in the places where we call our variables uuid. Much better!

[Notabilis (notabilis27)]

2) Would be fine by me. The "temporary"-solution got in before the 24h lifetime was suggested by GunChleoc and I never reconsidered it. So it can go now. Reducing the lifetime can be done, too, but 2 hours is probably not enough. The timestamp is stored when starting the game so when I have the game running for over 2 hours and it crashes then a new UUID will be generated. So maybe something like 12 hours?

3) I don't really understand what you mean here. I am now using boost::uuid() which seems to be fine. I am not sure whether something else should be changed. Do you want me to drop the string/uuid-string and use a simple integer? Or rename random_string() to generate_uuid()? Or... ?

[SirVer (sirver)]

2) 12h sounds good to me. We could go more fancy and rewrite the last_started_timestamp every few real world seconds, but just doing 12h is fine IMHO.

3) Sorry, that was unclear from me. I suggest renaming the function. I did this real quick while thinking how it should be named and investigating if the boost generator could be static in the function. I missed that during review - c++ global variables should be avoided like the plague. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[Notabilis (notabilis27)]

The option to use a temporary UUID has been removed and the interval for the "permanent" ones reduced to 12 hours. So I guess this branch is ready for merge now?

Thanks for the link, will be an interesting read. Is that the style guide Widelands (or you?) are trying to follow?

[SirVer (sirver)]

> The option to use a temporary UUID has been removed and the interval for the "permanent" ones reduced to 12 hours. So I guess this branch is ready for merge now?

Will have another quick read through in a minute, but yes, I think so. I will merge this and also merge the metaserver branch as soon as this is in. Thanks for working on this! Now on to the even more important relay stuff :).

> Thanks for the link, will be an interesting read. Is that the style guide Widelands (or you?) are trying to follow?

Unfortunately, Widelands predates the Google style guide. Therefore it is not really enforcable in our code base anymore. Also, the style guide mixes style (indent, whitespace, curly braces positions) with good practices (no globals, no non-const references as arguments). Generally it is the best guideline for writing good c++ that I know and I really like the thorough explanations that come with each point.

Having worked at Google for 5 years, I also came to appreciate how many bugs following the style guide avoids. So I usually adhere to it whenever I write new code.

So I'd say, having a read through will give you a lot of food for thought.

[SirVer (sirver)]

Looks great! Thanks again.

@bunnybot merge

review: Approve

[SirVer (sirver)]

r8472 has messed up the export to GitHub and the git repository state in the related git branch. Travis will not run again for this branch and merging this into trunk will likely mess up the trunk states in git as well. I will do some surgery on this branch and only retain the diff, throwing away all revisions.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 2841. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/307141215.

[SirVer (sirver)]

For the record, this was the surgery I did:

In the net-uuid branch:
$ bzr diff --diff-options="-u" -r ancestor:lp:widelands > /tmp/diff.patch

Then get a fresh trunk branched:

$ bzr branch lp:widelands
$ patch -p0 < /tmp/diff.patch
$ bzr commit -m "All changes in one clean diff on master." --author "Notabilis <email address hidden>"
$ bzr push --overwrite lp:~widelands-dev/widelands/net-uuid

Unfortunately that made bunnybot unhappy and I had to manually intervene and fix some state. Next time, we should just close the merge request and reopen a new one with the clean patch.

@Notabilis: sorry for loosing your history, but at least the work is still credited to you in full.

@bunnybot merge

[Notabilis (notabilis27)]

No problem. As long as we have one commit for the merge the single steps aren't that important.

Seems like I am quite talented with breaking bunnybot though, sorry. So the problem was merging with the same branch? I would have thought that this is a quite normal operation for bzr and git, isn't it? What would have been the bunnybot-friendly way to merge(?) the diverged branch heads?

[SirVer (sirver)]

> Seems like I am quite talented with breaking bunnybot though, sorry.

Hardly your fault :/. The whole setup with launchpad -> bzr -> git -> github is kinda fragile by design.

> So the problem was merging with the same branch?

Yes, that was fine for bzr. But in the git import it deleted all the files except for the ones changed in this branch.

> What would have been the bunnybot-friendly way to merge(?) the diverged branch heads?

I think a rebase would have been better. Unfortunately rebase is not part of the core bzr tools, but requires a bzr plugin to be installed: https://launchpad.net/bzr-rewrite. Then a bzr rebase does the job. You are right that merging the diverged branch is the bzr way to do it - it is a bummer that this apparently does not work in our setup.

[SirVer (sirver)]

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2851. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/307441242.
Appveyor build 2661. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_uuid-2661.

[Notabilis (notabilis27)]

This branch has been merged and the metaserver has been updated, too. This means that previous versions of trunk can no longer connect to the metaserver. Branches based on older trunk are also affected, build 19 should not be affected.

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2017-11-05 20:06:36 +0000
+++ src/CMakeLists.txt	2017-11-26 10:34:25 +0000
@@ -121,6 +121,7 @@
     map_io_map_loader
     network
     profile
+    random
     sound
     ui_basic
     ui_fsmenu

=== modified file 'src/network/CMakeLists.txt'
--- src/network/CMakeLists.txt	2017-11-05 20:06:36 +0000
+++ src/network/CMakeLists.txt	2017-11-26 10:34:25 +0000
@@ -1,6 +1,8 @@
 wl_library(network
   SRCS
     constants.h
+    crypto.h
+    crypto.cc
     internet_gaming.cc
     internet_gaming.h
     internet_gaming_messages.cc
@@ -46,6 +48,7 @@
     logic_game_settings
     map_io_map_loader
     profile
+    random
     scripting_lua_interface
     scripting_lua_table
     ui_basic

=== added file 'src/network/crypto.cc'
--- src/network/crypto.cc	1970-01-01 00:00:00 +0000
+++ src/network/crypto.cc	2017-11-26 10:34:25 +0000
@@ -0,0 +1,25 @@
+#include "network/crypto.h"
+
+#include <boost/uuid/sha1.hpp>
+
+namespace crypto {
+
+std::string sha1(const std::string& input) {
+
+	// Hash the input
+	boost::uuids::detail::sha1 sha;
+	sha.process_bytes(input.data(), input.size());
+	uint32_t digest[5];
+	sha.get_digest(digest);
+
+    // Back to string
+    char result[41] = {0};
+
+    for (int i = 0; i < 5; i++) {
+        std::sprintf(result + (i << 3), "%08x", digest[i]);
+    }
+
+    return std::string(result);
+}
+
+}

=== added file 'src/network/crypto.h'
--- src/network/crypto.h	1970-01-01 00:00:00 +0000
+++ src/network/crypto.h	2017-11-26 10:34:25 +0000
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2006-2017 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#ifndef WL_NETWORK_CRYPTO_H
+#define WL_NETWORK_CRYPTO_H
+
+#include <string>
+
+namespace crypto {
+
+/**
+ Hashes the given input string with SHA-1 and returns the hash.
+ @param input A string to calculate the hash of.
+ @return The hash as hex-string.
+ */
+std::string sha1(const std::string& input);
+
+}
+
+#endif  // end of include guard: WL_NETWORK_CRYPTO_H

=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2017-11-11 14:27:22 +0000
+++ src/network/internet_gaming.cc	2017-11-26 10:34:25 +0000
@@ -20,8 +20,8 @@
 #include "network/internet_gaming.h"
 
 #include <memory>
-#include <random>
 
+#include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
 #include <boost/lexical_cast.hpp>
 
@@ -32,7 +32,9 @@
 #include "io/fileread.h"
 #include "io/filesystem/layered_filesystem.h"
 #include "network/constants.h"
+#include "network/crypto.h"
 #include "network/internet_gaming_messages.h"
+#include "random/random.h"
 
 /// Private constructor by purpose: NEVER call directly. Always call InternetGaming::ref(), this
 /// will ensure
@@ -63,7 +65,7 @@
 void InternetGaming::reset() {
 	net.reset();
 	state_ = OFFLINE;
-	pwd_ = "";
+	authenticator_ = "";
 	reg_ = false;
 	meta_ = INTERNET_GAMING_METASERVER;
 	port_ = INTERNET_GAMING_PORT;
@@ -120,32 +122,29 @@
 
 /// Login to metaserver
 bool InternetGaming::login(const std::string& nick,
-                           const std::string& pwd,
-                           bool reg,
+                           const std::string& authenticator,
+                           bool registered,
                            const std::string& meta,
                            uint32_t port) {
-	assert(state_ == OFFLINE);
 
-	pwd_ = pwd;
-	reg_ = reg;
+	clientname_ = nick;
+	reg_ = registered;
 	meta_ = meta;
 	port_ = port;
 
-	// If we are not connecting to a registered account, create a random value
-	// to send as password. Used so the metaserver can match our IPv4 and IPv6 connections.
-	// See internet_gaming_protocol.h for more information
-	if (!reg_) {
-		// Admittedly this is a pretty stupid generator. But it should be fine for us
-		static const std::string random_chars = "0123456789ABCDEF";
-		pwd_ = "";
-		std::random_device rd;
-		std::mt19937 gen(rd());
-		std::uniform_int_distribution<> dist(0, random_chars.length() - 1);
-		while (pwd_.length() < 8) {
-			pwd_.push_back(random_chars[dist(gen)]);
-		}
+	if (registered) {
+		authenticator_ = authenticator;
+	} else {
+		authenticator_ = crypto::sha1(nick + authenticator);
 	}
 
+	assert(!authenticator_.empty());
+
+	return do_login();
+}
+
+bool InternetGaming::do_login(bool should_relogin) {
+
 	initialize_connection();
 
 	// If we are here, a connection was established and we can send our login package through the
@@ -154,10 +153,10 @@
 	SendPacket s;
 	s.string(IGPCMD_LOGIN);
 	s.string(boost::lexical_cast<std::string>(INTERNET_GAMING_PROTOCOL_VERSION));
-	s.string(nick);
+	s.string(clientname_);
 	s.string(build_id());
 	s.string(bool2str(reg_));
-	s.string(pwd_);
+	s.string(authenticator_);
 	net->send(s);
 
 	// Now let's see, whether the metaserver is answering
@@ -169,9 +168,11 @@
 		// paperwork, so we put our feet up and just return. ;)
 		if (state_ != CONNECTING) {
 			if (state_ == LOBBY) {
-				format_and_add_chat(
-				   "", "", true, _("For hosting a game, please take a look at the notes at:"));
-				format_and_add_chat("", "", true, "http://wl.widelands.org/wiki/InternetGaming");
+				if (!should_relogin) {
+					format_and_add_chat(
+						"", "", true, _("For hosting a game, please take a look at the notes at:"));
+					format_and_add_chat("", "", true, "http://wl.widelands.org/wiki/InternetGaming");
+				}
 
 				// Try to establish a second connection to tell the metaserver about our IPv4 address
 				create_second_connection();
@@ -191,42 +192,11 @@
 		throw wexception("InternetGaming::relogin: This only makes sense if there was an error.");
 	}
 
-	initialize_connection();
-
-	// If we are here, a connection was established and we can send our login package through the
-	// socket.
-	log("InternetGaming: Sending relogin request.\n");
-	SendPacket s;
-	s.string(IGPCMD_RELOGIN);
-	s.string(boost::lexical_cast<std::string>(INTERNET_GAMING_PROTOCOL_VERSION));
-	s.string(clientname_);
-	s.string(build_id());
-	s.string(bool2str(reg_));
-	s.string(pwd_);
-	net->send(s);
-
-	// Now let's see, whether the metaserver is answering
-	uint32_t const secs = time(nullptr);
-	state_ = CONNECTING;
-	while (INTERNET_GAMING_TIMEOUT > time(nullptr) - secs) {
-		handle_metaserver_communication();
-		// Check if we are a step further... if yes handle_packet has taken care about all the
-		// paperwork, so we put our feet up and just return. ;)
-		if (state_ != CONNECTING) {
-			if (state_ == LOBBY) {
-				break;
-			} else if (error())
-				return false;
-		}
-	}
-
-	if (INTERNET_GAMING_TIMEOUT <= time(nullptr) - secs) {
-		log("InternetGaming: No answer from metaserver!\n");
+	if (!do_login(true)) {
 		return false;
 	}
 
-	create_second_connection();
-
+	state_ = LOBBY;
 	// Client is reconnected, so let's try resend the timeouted command.
 	if (waitcmd_ == IGPCMD_GAME_CONNECT)
 		join_game(gamename_);
@@ -238,6 +208,9 @@
 		set_game_playing();
 	}
 
+	log("InternetGaming: Reconnected to metaserver\n");
+	format_and_add_chat("", "", true, _("Successfully reconnected to the metaserver!"));
+
 	return true;
 }
 
@@ -385,7 +358,7 @@
 	s.string(IGPCMD_TELL_IP);
 	s.string(boost::lexical_cast<std::string>(INTERNET_GAMING_PROTOCOL_VERSION));
 	s.string(clientname_);
-	s.string(pwd_);
+	s.string(authenticator_);
 	tmpNet->send(s);
 
 	// Close the connection
@@ -416,19 +389,24 @@
 	if (state_ == CONNECTING) {
 		if (cmd == IGPCMD_LOGIN) {
 			// Clients request to login was granted
-			clientname_ = packet.string();
+			const std::string assigned_name = packet.string();
+			if (clientname_ != assigned_name) {
+				format_and_add_chat("", "", true,
+					(boost::format(
+						_("You have been logged in as '%s' since your requested name is already in use or reserved."))
+							% assigned_name).str());
+			}
+			clientname_ = assigned_name;
 			clientrights_ = packet.string();
+			if (clientrights_ == INTERNET_CLIENT_UNREGISTERED) {
+				// Might happen that we log in with less rights than we wanted to.
+				// Happens when we are already logged in with another client.
+				reg_ = false;
+			}
 			state_ = LOBBY;
 			log("InternetGaming: Client %s logged in.\n", clientname_.c_str());
 			return;
 
-		} else if (cmd == IGPCMD_RELOGIN) {
-			// Clients request to relogin was granted
-			state_ = LOBBY;
-			log("InternetGaming: Client %s relogged in.\n", clientname_.c_str());
-			format_and_add_chat("", "", true, _("Successfully reconnected to the metaserver!"));
-			return;
-
 		} else if (cmd == IGPCMD_ERROR) {
 			std::string errortype = packet.string();
 			if (errortype != "LOGIN" && errortype != "RELOGIN") {
@@ -451,9 +429,8 @@
 			   cmd.c_str());
 		}
 	}
-
 	try {
-		if (cmd == IGPCMD_LOGIN || cmd == IGPCMD_RELOGIN) {
+		if (cmd == IGPCMD_LOGIN) {
 			// Login specific commands but not in CONNECTING state...
 			log("InternetGaming: Received %s cmd although client is not in CONNECTING state.\n",
 			    cmd.c_str());
@@ -766,16 +743,28 @@
 
 /// ChatProvider: sends a message via the metaserver.
 void InternetGaming::send(const std::string& msg) {
+	// TODO(Notabilis): Messages can get lost when we are temporarily disconnected from the metaserver,
+	// even when we reconnect again. "Answered" messages like IGPCMD_GAME_CONNECT are resent but chat
+	// messages are not. Resend them after some time when we did not receive the matching IGPCMD_CHAT
+	// command from the server? For global/public messages we could wait for the returned IGPCMD_CHAT
+	// from the metaserver, similar to other commands. What about private messages? Maybe modify the
+	// metaserver to send them back, too?
 	if (!logged_in()) {
 		format_and_add_chat(
 		   "", "", true, _("Message could not be sent: You are not connected to the metaserver!"));
 		return;
 	}
 
+	std::string trimmed = boost::algorithm::trim_copy(msg);
+	if (trimmed.empty()) {
+		// Message is empty or only space characters. We don't want it either way
+		return;
+	}
+
 	SendPacket s;
 	s.string(IGPCMD_CHAT);
 
-	if (msg.size() && *msg.begin() == '@') {
+	if (*msg.begin() == '@') {
 		// Format a personal message
 		std::string::size_type const space = msg.find(' ');
 		if (space >= msg.size() - 1) {
@@ -784,12 +773,19 @@
 			   _("Message could not be sent: Was this supposed to be a private message?"));
 			return;
 		}
-		s.string(msg.substr(space + 1));     // message
+		trimmed = boost::algorithm::trim_copy(msg.substr(space + 1));
+		if (trimmed.empty()) {
+			format_and_add_chat(
+			   "", "", true,
+			   _("Message could not be sent: Was this supposed to be a private message?"));
+			return;
+		}
+		s.string(trimmed);     // message
 		s.string(msg.substr(1, space - 1));  // recipient
 
 		format_and_add_chat(clientname_, msg.substr(1, space - 1), false, msg.substr(space + 1));
 
-	} else if (clientrights_ == INTERNET_CLIENT_SUPERUSER && msg.size() && *msg.begin() == '/') {
+	} else if (clientrights_ == INTERNET_CLIENT_SUPERUSER && *msg.begin() == '/') {
 		// This is either a /me command, a super user command, or well... just a chat message
 		// beginning
 		// with a "/" - let's see...
@@ -798,15 +794,16 @@
 		std::string cmd, arg;
 		std::string temp = msg.substr(1);  // cut off '/'
 		std::string::size_type const space = temp.find(' ');
-		if (space > temp.size())
+		if (space > temp.size()) {
 			// no argument
 			goto normal;
+		}
 
 		// get the cmd and the arg
 		cmd = temp.substr(0, space);
-		arg = temp.substr(space + 1);
+		arg = boost::algorithm::trim_copy(temp.substr(space + 1));
 
-		if (cmd == "motd") {
+		if (!arg.empty() && cmd == "motd") {
 			SendPacket m;
 			m.string(IGPCMD_MOTD);
 			// Check whether motd is attached or should be loaded from a file
@@ -827,19 +824,19 @@
 			}
 			// send the request to change the motd
 			m.string(arg);
-			net->send(s);
+			net->send(m);
 			return;
-		} else if (cmd == "announcement") {
+		} else if (!arg.empty() && cmd == "announcement") {
 			// send the request to change the motd
 			SendPacket m;
 			m.string(IGPCMD_ANNOUNCEMENT);
 			m.string(arg);
-			net->send(s);
+			net->send(m);
 			return;
-		} else
+		} else {
 			// let everything else pass
 			goto normal;
-
+		}
 	} else {
 	normal:
 		s.string(msg);

=== modified file 'src/network/internet_gaming.h'
--- src/network/internet_gaming.h	2017-07-01 08:22:54 +0000
+++ src/network/internet_gaming.h	2017-11-26 10:34:25 +0000
@@ -51,15 +51,28 @@
  * The InternetGaming struct.
  */
 struct InternetGaming : public ChatProvider {
+
 	/// The only instance of InternetGaming -> the constructor is private by purpose!
 	static InternetGaming& ref();
 
 	void reset();
 
 	// Login and logout
+
 	void initialize_connection();
+
+	/**
+	 * Try to login on the metaserver.
+	 * @param nick The preferred username. Another username might be chosen by the metaserver if
+	 *             the requested one is already in use.
+	 * @param authenticator If \c registered is \c true, this is the password. Otherwise, it is some unique
+	 *             id the server can use to identify the user.
+	 * @param metaserver The hostname of the metaserver.
+	 * @param port The port number of the metaserver.
+	 * @return Whether the login was successful.
+	 */
 	bool login(const std::string& nick,
-	           const std::string& pwd,
+	           const std::string& authenticator,
 	           bool registered,
 	           const std::string& metaserver,
 	           uint32_t port);
@@ -180,6 +193,13 @@
 	                         bool system,
 	                         const std::string& msg);
 
+	/**
+	 * Does the real work of the login.
+	 * \param relogin Whether this is a relogin. Only difference is that
+	 *                on first login a greeting is shown.
+	 */
+	bool do_login(bool relogin = false);
+
 	/// The connection to the metaserver
 	std::unique_ptr<NetClient> net;
 
@@ -187,8 +207,9 @@
 	enum { OFFLINE, CONNECTING, LOBBY, IN_GAME, COMMUNICATION_ERROR } state_;
 
 	/// data saved for possible relogin
-	std::string pwd_;
+	std::string authenticator_;
 	bool reg_;
+
 	std::string meta_;
 	uint16_t port_;
 

=== modified file 'src/network/internet_gaming_protocol.h'
--- src/network/internet_gaming_protocol.h	2017-07-01 08:22:54 +0000
+++ src/network/internet_gaming_protocol.h	2017-11-26 10:34:25 +0000
@@ -28,12 +28,16 @@
 
 /**
  * The current version of the in-game network protocol. Client and metaserver
- * protocol versions must match.
+ * protocol versions must match. The metaserver supports the protocol version of latest stable build
+ * and the newest version in trunk.
  * Used Versions:
- * 0: Build 19 and before
+ * 0: Build 19 and before [stable, supported]
  * 1: Between build 19 and build 20 - IPv6 support added
+ * 2: Between build 19 and build 20 - Added UUID to allow reconnect with same username after crashes.
+ *                                    When logging twice with a registered account, the second connection
+ *                                    gets a free username assigned. Dropping RELOGIN command. [supported]
  */
-#define INTERNET_GAMING_PROTOCOL_VERSION 1
+#define INTERNET_GAMING_PROTOCOL_VERSION 2
 
 /**
  * The default timeout time after which the client tries to resend a package or even finally closes
@@ -91,35 +95,43 @@
  * Every packet starts with a single-byte command code.
  *
  * \note ALL PAYLOADS SHALL BE STRINGS - this is for easier handling and debugging of the
- * communication
- *       between metaserver and client. If an unsigned or signed value has to be sent, convert it
- * with
- *       boost::lexical_cast<std::string>. Boolean values should be sent in form of "true" or
- * "false".
+ *       communication between metaserver and client. If an unsigned or signed value has
+ *       to be sent, convert it with boost::lexical_cast<std::string>. Boolean values should
+         be sent in form of "true" or "false".
  */
 
 /**
- * The nonce:
- *
- * The nonce is used on the metaserver to link multiple connections by the same client. This
- * normally
- * happens when the client supports IPv4 and IPv6 and connects with both protocol versions. This
- * way,
- * the metaserver knows that the clients supports both versions and can show games / offer his game
+ * The UUID:
+ *
+ * The UUID is a semi-permanent ID stored in the configuration file of Widelands.
+ * It has to be stored in the file since it should survive crashes of the game or computer.
+ * If the game is not started for 24 hours, a new one is created to increase privacy.
+ * Basically it allows the metaserver to identify the user even when multiple users try to join with
+ * the same username. Note that the sent UUID differs from the stored one: The sent UUID is
+ * hash(username | stored-id).
+ * In the case of registered players, the password can be used instead of a UUID. The username
+ * alone can not be used for this, especially not for unregistered users: The metaserver can not
+ * differentiate between a second connection by the user and an initial login of another user
+ * (with the same name).
+ *
+ * Use-cases of the UUID:
+ *
+ * 1) Linking connections with IPv4 and IPv6
+ * The UUID is used on the metaserver to link multiple connections by the same client. This
+ * normally happens when the client supports IPv4 and IPv6 and connects with both protocol versions. This
+ * way, the metaserver knows that the client supports both versions and can show games / offer its game
  * of/for clients with both protocol versions.
  *
- * When a network client connects to the metaserver with (RE)LOGIN he also sends a nonce.
- * When "another" netclient connects to the metaserver and sends TELL_IP containing the same nonce,
+ * When a network client connects to the metaserver with (RE)LOGIN it also sends the UUID.
+ * When "another" netclient connects to the metaserver and sends TELL_IP containing the same UUID,
  * it is considered the same game client connecting with another IP. This way, two connections by
- * IPv4 and
- * IPv6 can be matched so the server learns both addresses of the client.
+ * IPv4 and IPv6 can be matched so the server learns both addresses of the client.
  *
- * In the case of registered players, the password can be used instead of a random nonce. The
- * username alone
- * can not be used for this, especially not for unregistered users: The metaserver can not
- * differentiate
- * between a second connection by the user and an initial login of another user (with the same
- * name).
+ * 2) Reconnect after crash / network problems.
+ * When Widelands breaks the connection without logging out, the server still assumes that the old
+ * connection is active. So when the player reconnects, another name is chosen. Sending the UUID allows
+ * to reclaim the old name, since the server recognizes that there isn't a second player trying to use
+ * the same name.
  */
 
 /**
@@ -132,15 +144,12 @@
  * closing the connection. The receiver of this command should just close the connection.
  *
  * \note that either party is allowed to close the connection without sending a \ref
- * IGPCMD_DISCONNECT
- *       command first (in any case, this can happen when the program crashes or network connection
- * is lost).
+ *       IGPCMD_DISCONNECT command first (in any case, this can happen when the program crashes
+ *       or network connection is lost).
  *
  * \note If you want to change the payload of this command, change it only by appending new items.
- * The
- *       reason is that this is the only command that can be sent by the metaserver even when the
- * protocol
- *       versions differ.
+ *       The reason is that this is the only command that can be sent by the metaserver even when the
+ *       protocol versions differ.
  *
  */
 static const std::string IGPCMD_DISCONNECT = "DISCONNECT";
@@ -153,62 +162,28 @@
  * \li string:    protocol version
  * \li string:    client name
  * \li string:    build_id of the client
- * \li string:    whether the client wants to login in to a registered account ("true" or "false" as
- * string)
+ * \li string:    whether the client wants to login in to a registered account
+ *                ("true" or "false" as string)
  * \li string:    for registered accounts: password in clear text
- *                for unregistered users a random nonce to recognized the matching IPv4 and IPv6
- * connections.
- *                for an explanation of the nonce, see above.
+ *                for unregistered users the UUID to recognize the matching IPv4 and IPv6
+ *                connections or to reclaim the username after a unintended disconnect.
+ *                For an explanation of the UUID, see above.
  *
  * If the metaserver accepts, it replies with a LOGIN command with the following payload:
  * \li string:    client name (might be different to the previously chosen one, if the client did
  *                NOT login to a registered account and either the chosen is registered or already
- * used.)
+ *                used.)
  * \li string:    clients rights  (see client rights section above)
  *
  * If no answer is received in \ref INTERNET_GAMING_TIMEOUT s the client will again try to login
  * \ref INTERNET_GAMING_RETRIES times until it finally bails out something like "server does not
  * answer"
  *
- * For the case, that the metaserver does not accept the login, take a look at \ref IGPCMD_REJECTED
+ * For the case, that the metaserver does not accept the login, take a look at \ref IGPCMD_ERROR
  */
 static const std::string IGPCMD_LOGIN = "LOGIN";
 
 /**
- * Reinitiate a connection.
- *
- * Basically the difference to \ref IGPCMD_LOGIN is, that the metaserver allows the user to go on at
- * the
- * place it was before - so if the user was in a running game and only lost connection to the
- * metaserver,
- * but not to the game itself, statistics for that game can still be sent and saved.
- * To ensure everything is fine, the login information will still be checked + if a client with the
- * name
- * is still listed as online, the metaserver will ping the client and if the client does not answer
- * intime
- * it will be replaced by the user requesting the relogin
- *
- * sent by the client, with the following payload:
- * \li string:    protocol version
- * \li string:    client name - the one the metaserver replied at the first login
- * \li string:    build_id of the client
- * \li string:    whether the client wants to login in to a registered account ("false", "true")
- * \li string:    for registered accounts: password in clear text
- *                for unregistered users a random nonce to recognized the matching IPv4 and IPv6
- * connections.
- *                for an explanation of the nonce, see above.
- *
- * If the metaserver accepts, it replies with a RELOGIN command without any payload.
- *
- * If no answer is received in \ref INTERNET_GAMING_TIMEOUT s the client will try to relogin
- * \ref INTERNET_GAMING_RETRIES times until it finally bails out something like "server does not
- * answer"
- *
- * For the case, that the metaserver does not accept the login, it sends a \ref IGPCMD_ERROR "LOGIN"
- */
-static const std::string IGPCMD_RELOGIN = "RELOGIN";
-
-/**
  * Tells the metaserver about a secondary IP address.
  *
  * Assuming the client already has a connection over IPv6 and tries to establish a secondary
@@ -220,8 +195,8 @@
  * \li string:    protocol version
  * \li string:    client name - the one the metaserver replied at the first login
  * \li string:    for registered accounts: password in clear text
- *                for unregistered users the random nonce used on login
- *                for an explanation of the nonce, see above.
+ *                for unregistered users the UUID used on login
+ *                for an explanation of the UUID, see above.
  */
 static const std::string IGPCMD_TELL_IP = "TELL_IP";
 
@@ -291,15 +266,13 @@
  * The metaserver will echo the message if the client is allowed to send chat messages.
  *
  * The metaserver either broadcasts a chat message to all clients or sends it to the pm recipient
- * with the
- * following payload:
+ * with the following payload:
  * \li string:    sender (may be empty if it is a system message)
  * \li string:    the message
  * \li string:    type ("public", "private", "system")
  *
- * \note system messages are the motd (Sent by the metaserver to the client, after login (but not
- * relogin)
- *       and after the motd got changed) and announcements by superusers.
+ * \note system messages are the motd (Sent by the metaserver to the client, after login
+ * (but not relogin) and after the motd got changed) and announcements by superusers.
  */
 static const std::string IGPCMD_CHAT = "CHAT";
 

=== modified file 'src/random/random.cc'
--- src/random/random.cc	2017-01-25 18:55:59 +0000
+++ src/random/random.cc	2017-11-26 10:34:25 +0000
@@ -20,6 +20,11 @@
 #include "random/random.h"
 
 #include <cstdio>
+#include <random>
+
+#include <boost/uuid/uuid.hpp>
+#include <boost/uuid/uuid_generators.hpp>
+#include <boost/uuid/uuid_io.hpp>
 
 #include "base/wexception.h"
 #include "io/streamread.h"
@@ -98,3 +103,8 @@
 	sw.unsigned_32(state0);
 	sw.unsigned_32(state1);
 }
+
+std::string generate_random_uuid() {
+	static boost::uuids::random_generator gen = boost::uuids::random_generator();
+	return boost::uuids::to_string(gen());
+}

=== modified file 'src/random/random.h'
--- src/random/random.h	2017-01-25 18:55:59 +0000
+++ src/random/random.h	2017-11-26 10:34:25 +0000
@@ -21,6 +21,7 @@
 #define WL_RANDOM_RANDOM_H
 
 #include <stdint.h>
+#include <string>
 
 extern const uint32_t rng_sbox[256];
 
@@ -44,4 +45,9 @@
 
 #define SIMPLE_RAND(x) (((x) >> 8) ^ rng_sbox[(x)&0xff])
 
+/// Generates a random UUID, looking like "550e8400-e29b-11d4-a716-446655440000".
+/// This does not use logic_rand(), but instead a thread local random number
+/// generator, so do not use in logic code - it will desync.
+std::string generate_random_uuid();
+
 #endif  // end of include guard: WL_RANDOM_RANDOM_H

=== modified file 'src/ui_fsmenu/CMakeLists.txt'
--- src/ui_fsmenu/CMakeLists.txt	2017-11-05 20:09:44 +0000
+++ src/ui_fsmenu/CMakeLists.txt	2017-11-26 10:34:25 +0000
@@ -79,6 +79,7 @@
     map_io_map_loader
     network
     profile
+    random
     scripting_coroutine
     scripting_lua_interface
     scripting_lua_table

=== modified file 'src/ui_fsmenu/internet_lobby.cc'
--- src/ui_fsmenu/internet_lobby.cc	2017-06-20 19:55:32 +0000
+++ src/ui_fsmenu/internet_lobby.cc	2017-11-26 10:34:25 +0000
@@ -27,10 +27,12 @@
 #include "base/macros.h"
 #include "graphic/graphic.h"
 #include "graphic/text_constants.h"
+#include "network/crypto.h"
 #include "network/gameclient.h"
 #include "network/gamehost.h"
 #include "network/internet_gaming.h"
 #include "profile/profile.h"
+#include "random/random.h"
 #include "ui_basic/messagebox.h"
 
 FullscreenMenuInternetLobby::FullscreenMenuInternetLobby(char const* const nick,
@@ -192,8 +194,9 @@
 	Section& s = g_options.pull_section("global");
 	const std::string& metaserver = s.get_string("metaserver", INTERNET_GAMING_METASERVER.c_str());
 	uint32_t port = s.get_natural("metaserverport", INTERNET_GAMING_PORT);
-
-	InternetGaming::ref().login(nickname_, password_, is_registered_, metaserver, port);
+	std::string auth = is_registered_ ? password_ : s.get_string("uuid");
+	assert(!auth.empty());
+	InternetGaming::ref().login(nickname_, auth, is_registered_, metaserver, port);
 }
 
 /// fills the server list

=== modified file 'src/ui_fsmenu/multiplayer.cc'
--- src/ui_fsmenu/multiplayer.cc	2017-02-25 13:27:40 +0000
+++ src/ui_fsmenu/multiplayer.cc	2017-11-26 10:34:25 +0000
@@ -21,8 +21,10 @@
 
 #include "base/i18n.h"
 #include "graphic/text_constants.h"
+#include "network/crypto.h"
 #include "network/internet_gaming.h"
 #include "profile/profile.h"
+#include "random/random.h"
 #include "ui_basic/messagebox.h"
 #include "wui/login_box.h"
 
@@ -122,7 +124,9 @@
 	// Try to connect to the metaserver
 	const std::string& meta = s.get_string("metaserver", INTERNET_GAMING_METASERVER.c_str());
 	uint32_t port = s.get_natural("metaserverport", INTERNET_GAMING_PORT);
-	InternetGaming::ref().login(nickname_, password_, register_, meta, port);
+	std::string auth = register_ ? password_ : s.get_string("uuid");
+	assert(!auth.empty());
+	InternetGaming::ref().login(nickname_, auth, register_, meta, port);
 
 	// Check whether metaserver send some data
 	if (InternetGaming::ref().logged_in())

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2017-11-24 09:19:52 +0000
+++ src/wlapplication.cc	2017-11-26 10:34:25 +0000
@@ -75,6 +75,7 @@
 #include "network/gamehost.h"
 #include "network/internet_gaming.h"
 #include "profile/profile.h"
+#include "random/random.h"
 #include "sound/sound_handler.h"
 #include "ui_basic/messagebox.h"
 #include "ui_basic/progresswindow.h"
@@ -743,6 +744,7 @@
 	s.get_bool("write_syncstreams");
 	s.get_bool("sound_at_message");
 	s.get_bool("transparent_chat");
+	s.get_string("uuid");
 	s.get_string("registered");
 	s.get_string("nickname");
 	s.get_string("password");
@@ -755,6 +757,24 @@
 	s.get_natural("metaserverport");
 	// KLUDGE!
 
+	long int last_start = s.get_int("last_start", 0);
+	if (last_start + 12 * 60 * 60 < time(nullptr)) {
+		// First start of the game or not started for 12 hours. Create a (new) UUID.
+		// For the use of the UUID, see network/internet_gaming_protocol.h
+		s.set_string("uuid", generate_random_uuid());
+	}
+	s.set_int("last_start", time(nullptr));
+
+	// Save configuration now. Otherwise, the UUID is not saved
+	// when the game crashes, loosing part of its advantage
+	try {
+		g_options.write("config", false);
+	} catch (const std::exception& e) {
+		log("WARNING: could not save configuration: %s\n", e.what());
+	} catch (...) {
+		log("WARNING: could not save configuration");
+	}
+
 	return true;
 }