From ccbf4072c717f11a8aba1b1d4b17334a460a9e7c Mon Sep 17 00:00:00 2001
From: Hartmnt <hartmunt@protonmail.com>
Date: Thu, 5 May 2022 13:55:57 +0000
Subject: [PATCH] FIX(server): Improve rememberchannelduration compare logic

The previous implementation of rememberchannelduration (#4147)
acknowledged the unreliability of the last_disconnect database
field. It used a workaround to set the last_disconnect field
to NULL for every user on every start of the mumble server.
However, with a default behavior for NULL, this created the
unintended side-effect that rememberchannel was erroneously
used for every user on their first connection after a server
restart.

This new implementation reverts the workaround of #4147 and
uses the last_active database field to check if last_disconnect
is reliable. If last_disconnect is unreliable, it will now use
the server uptime and compare it to rememberchanneduration.
We expect last_disconnect to be unreliable when the server
was shut down while clients are still connected.

Fixes #5639
---
 src/murmur/ServerDB.cpp | 55 +++++++++++++++++++++++++++--------------
 src/murmur/ServerDB.h   |  4 ---
 src/murmur/main.cpp     |  2 --
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/src/murmur/ServerDB.cpp b/src/murmur/ServerDB.cpp
index 8ac992b45..95bfbda1e 100644
--- a/src/murmur/ServerDB.cpp
+++ b/src/murmur/ServerDB.cpp
@@ -27,6 +27,8 @@
 #include <QtSql/QSqlError>
 #include <QtSql/QSqlQuery>
 
+#include <cstdint>
+
 #ifdef Q_OS_WIN
 #	include <winsock2.h>
 #else
@@ -2156,25 +2158,49 @@ int Server::readLastChannel(int id) {
 	TransactionHolder th;
 	QSqlQuery &query = *th.qsqQuery;
 
-	SQLPREP("SELECT `lastchannel`,`last_disconnect` FROM `%1users` WHERE `server_id` = ? AND `user_id` = ?");
+	SQLPREP(
+		"SELECT `lastchannel`,`last_active`,`last_disconnect` FROM `%1users` WHERE `server_id` = ? AND `user_id` = ?");
 	query.addBindValue(iServerNum);
 	query.addBindValue(id);
 	SQLEXEC();
 
 	if (query.next()) {
 		int cid = query.value(0).toInt();
-		if (query.value(1).isNull()) {
-			return qhChannels.contains(cid) ? cid : -1;
+
+		if (!qhChannels.contains(cid)) {
+			return -1;
 		}
-		QDateTime last_disconnect = QDateTime::fromString(query.value(1).toString(), Qt::ISODate);
-		last_disconnect.setTimeSpec(Qt::UTC);
-		QDateTime now = QDateTime::currentDateTime();
-		now           = now.toTimeSpec(Qt::UTC);
 
 		int duration = Meta::mp.iRememberChanDuration;
-		if (duration <= 0 || last_disconnect.secsTo(now) <= duration) {
-			if (qhChannels.contains(cid))
-				return cid;
+
+		if (duration <= 0) {
+			return cid;
+		}
+
+		if (query.value(1).isNull()) {
+			return -1;
+		}
+
+		QDateTime last_active = QDateTime::fromString(query.value(1).toString(), Qt::ISODate);
+		QDateTime last_disconnect;
+
+		// NULL column for last_disconnect will yield an empty invalid QDateTime object.
+		// Using that object with QDateTime::secsTo() will return 0 as per Qt specification.
+		if (!query.value(2).isNull()) {
+			last_disconnect = QDateTime::fromString(query.value(2).toString(), Qt::ISODate);
+		}
+
+		if (last_active.secsTo(last_disconnect) <= 0) {
+			// last_disconnect < last_active (or NULL). This can happen, if last_disconnect is invalid, because it has
+			// not been updated properly (e.g. because the entire server was shut down while the user was still
+			// connected). Use the server uptime as a reference point instead.
+			last_disconnect          = QDateTime::currentDateTime();
+			std::int64_t uptimeMSecs = static_cast< std::int64_t >(tUptime.elapsed() / 1000ULL);
+			last_disconnect          = last_disconnect.addMSecs(-uptimeMSecs);
+		}
+
+		if (last_disconnect.secsTo(QDateTime::currentDateTime()) <= duration) {
+			return cid;
 		}
 	}
 	return -1;
@@ -2487,12 +2513,3 @@ void ServerDB::deleteServer(int server_id) {
 	query.addBindValue(server_id);
 	SQLEXEC();
 }
-
-void ServerDB::clearLastDisconnect(Server *server) {
-	TransactionHolder th;
-	QSqlQuery &query = *th.qsqQuery;
-
-	SQLPREP("UPDATE `%1users` SET `last_disconnect` = NULL WHERE `server_id` = ?");
-	query.addBindValue(server->iServerNum);
-	SQLEXEC();
-}
diff --git a/src/murmur/ServerDB.h b/src/murmur/ServerDB.h
index cea39f280..2815ede5d 100644
--- a/src/murmur/ServerDB.h
+++ b/src/murmur/ServerDB.h
@@ -68,10 +68,6 @@ public:
 private:
 	static void loadOrSetupMetaPBKDF2IterationCount(QSqlQuery &query);
 	static void writeSUPW(int srvnum, const QString &pwHash, const QString &saltHash, const QVariant &kdfIterations);
-
-public slots:
-	/// Clear last_disconnect date of every user of the server
-	void clearLastDisconnect(Server *);
 };
 
 #endif
diff --git a/src/murmur/main.cpp b/src/murmur/main.cpp
index 212e657b1..dc7ffb36d 100644
--- a/src/murmur/main.cpp
+++ b/src/murmur/main.cpp
@@ -553,8 +553,6 @@ int main(int argc, char **argv) {
 
 	meta = new Meta();
 
-	QObject::connect(meta, SIGNAL(started(Server *)), &db, SLOT(clearLastDisconnect(Server *)));
-
 #ifdef Q_OS_UNIX
 	// It doesn't matter that this code comes after the forking because detach is
 	// set to false when readPw is set to true.