[Server] Harden private chat history identity matching

Match both participants by account id and name so a federated user's remote id cannot collide with a local account, and stop storing foreign ids in the local chat log. Extract the paging assembly into a pure helper and cover the whole-second boundary rules with gtests.

libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp

@@ -804,10 +804,13 @@ Response::ResponseCode Server_ProtocolHandler::cmdMessage(const Command_Message
     userInterface->sendProtocolItem(*se);
     rc.enqueuePreResponseItem(ServerMessage::SESSION_EVENT, se);
 
-    databaseInterface->logMessage(userInfo->id(), QString::fromStdString(userInfo->name()),
+    databaseInterface->logMessage(
-                                  QString::fromStdString(userInfo->address()), QString::fromStdString(cmd.message()),
+        userInfo->id(), QString::fromStdString(userInfo->name()), QString::fromStdString(userInfo->address()),
-                                  Server_DatabaseInterface::MessageTargetChat, userInterface->getUserInfo()->id(),
+        QString::fromStdString(cmd.message()), Server_DatabaseInterface::MessageTargetChat,
-                                  receiver);
+        // A federated user's id belongs to the remote server's
+        // database, not ours. Logging it would collide with a
+        // local id and corrupt local history lookups.
+        userInterface->getUserInfo()->server_id() == -1 ? userInterface->getUserInfo()->id() : 0, receiver);
     resetIdleTimer();
     return Response::RespOk;
 }

servatrice/CMakeLists.txt

@@ -10,6 +10,7 @@ set(servatrice_SOURCES
     src/forwarded_address.cpp
     src/main.cpp
     src/metrics_registry.cpp
+    src/private_chat_paging.cpp
     src/ratelimiter.cpp
     src/servatrice.cpp
     src/servatrice_connection_pool.cpp

servatrice/src/private_chat_paging.cpp

@@ -0,0 +1,31 @@
+#include "private_chat_paging.h"
+
+#include <algorithm>
+
+PrivateChatPage assemblePrivateChatPage(const QList<ServerInfo_PrivateChatMessage> &newestFirst, int limit)
+{
+    PrivateChatPage page;
+    page.messages = newestFirst;
+    page.hasMore = newestFirst.size() > limit;
+    if (!page.hasMore) {
+        std::reverse(page.messages.begin(), page.messages.end());
+        return page;
+    }
+
+    // The (limit+1)th message is the first one that does not fit. If its second
+    // appears anywhere in the page, that whole second would be split, so drop
+    // every message in that second and leave it for the next page.
+    const quint64 boundary = newestFirst.at(limit).time();
+    while (!page.messages.isEmpty() && page.messages.last().time() == boundary) {
+        page.messages.removeLast();
+    }
+
+    if (page.messages.isEmpty()) {
+        page.needsWholeSecond = true;
+        page.boundarySecond = boundary;
+        return page;
+    }
+
+    std::reverse(page.messages.begin(), page.messages.end());
+    return page;
+}

servatrice/src/private_chat_paging.h

@@ -0,0 +1,30 @@
+#ifndef PRIVATE_CHAT_PAGING_H
+#define PRIVATE_CHAT_PAGING_H
+
+#include <QList>
+#include <libcockatrice/protocol/pb/serverinfo_private_chat_message.pb.h>
+
+/** One page of private chat history, ready for the response. */
+struct PrivateChatPage
+{
+    /** Messages for the response, oldest first. */
+    QList<ServerInfo_PrivateChatMessage> messages;
+    /** Whether messages older than this page may still exist. */
+    bool hasMore = false;
+    /**
+     * Set when every fetched message belongs to the same second, so the caller
+     * has to fetch that whole second before it can answer. messages is empty in
+     * that case and boundarySecond names the second to fetch.
+     */
+    bool needsWholeSecond = false;
+    quint64 boundarySecond = 0;
+};
+
+/**
+ * Assembles one newest-first page from up to limit + 1 messages ordered newest
+ * first. A second is never split across pages: when the limit would cut into a
+ * second, that whole second is deferred to the next page instead.
+ */
+PrivateChatPage assemblePrivateChatPage(const QList<ServerInfo_PrivateChatMessage> &newestFirst, int limit);
+
+#endif // PRIVATE_CHAT_PAGING_H

servatrice/src/servatrice_database_interface.cpp

@@ -8,6 +8,7 @@
 #include "libcockatrice/protocol/pb/serverinfo_user_alt.pb.h"
 #include "libcockatrice/protocol/pb/serverinfo_user_session.pb.h"
 #include "libcockatrice/protocol/pb/serverinfo_warning.pb.h"
+#include "private_chat_paging.h"
 #include "servatrice.h"
 #include "server.h"
 #include "server_protocolhandler.h"
@@ -2639,7 +2640,9 @@ int Servatrice_DatabaseInterface::clearBlacklistedBanners()
 }
 
 bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId,
+                                                         const QString &callerName,
                                                          int peerId,
+                                                         const QString &peerName,
                                                          quint64 before,
                                                          int limit,
                                                          QList<ServerInfo_PrivateChatMessage> &messages,
@@ -2647,30 +2650,35 @@ bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId,
 {
     messages.clear();
     hasMore = false;
-    if (callerId < 1 || peerId < 1 || !checkSql()) {
+    if (callerId < 1 || peerId < 1 || callerName.isEmpty() || peerName.isEmpty() || !checkSql()) {
         return false;
     }
 
-    // Both participants are matched by account id, never by name: names can be
+    // Match both participants by account id, and verify their names as well.
-    // reused by guests or re-registered after deletion, and matching by name
+    // Names can be reused (guests, re-registration), so ids are what separate
-    // would merge conversations from different people.
+    // earlier holders; names are what reject a federated user's remote id that
+    // happens to collide with a local one in the shared log table.
     const QString conversationFilter = " FROM {prefix}_log WHERE target_type = 'chat'"
-                                       " AND ((sender_id = :caller_sender_id AND target_id = :peer_target_id)"
+                                       " AND ((sender_id = :caller_sender_id AND sender_name = :caller_name"
-                                       " OR (target_id = :caller_target_id AND sender_id = :peer_sender_id))";
+                                       " AND target_id = :peer_target_id AND target_name = :peer_name)"
+                                       " OR (target_id = :caller_target_id AND target_name = :caller_name"
+                                       " AND sender_id = :peer_sender_id AND sender_name = :peer_name))";
     const QString selectMessages = "SELECT UNIX_TIMESTAMP(log_time), sender_name, log_message";
     const auto bindParticipants = [&](QSqlQuery *query) {
         query->bindValue(":caller_sender_id", callerId);
+        query->bindValue(":caller_name", callerName);
         query->bindValue(":peer_target_id", peerId);
+        query->bindValue(":peer_name", peerName);
         query->bindValue(":peer_sender_id", peerId);
         query->bindValue(":caller_target_id", callerId);
     };
-    const auto readMessages = [&](QSqlQuery *query) {
+    const auto readMessages = [&](QSqlQuery *query, QList<ServerInfo_PrivateChatMessage> &out) {
         while (query->next()) {
             ServerInfo_PrivateChatMessage message;
             message.set_time(query->value(0).toULongLong());
             message.set_sender_name(query->value(1).toString().toStdString());
             message.set_message(query->value(2).toString().toStdString());
-            messages.append(message);
+            out.append(message);
         }
     };
 
@@ -2688,34 +2696,32 @@ bool Servatrice_DatabaseInterface::getPrivateChatHistory(int callerId,
     if (!execSqlQuery(query)) {
         return false;
     }
-    readMessages(query);
 
-    hasMore = messages.size() > limit;
+    QList<ServerInfo_PrivateChatMessage> newestFirst;
-    if (hasMore) {
+    readMessages(query, newestFirst);
-        const quint64 boundary = messages.at(limit).time();
+    PrivateChatPage page = assemblePrivateChatPage(newestFirst, limit);
-        while (!messages.isEmpty() && messages.last().time() == boundary) {
-            messages.removeLast();
-        }
-        if (messages.isEmpty()) {
-            query = prepareQuery(selectMessages + conversationFilter + " AND log_time = FROM_UNIXTIME(:second)");
-            bindParticipants(query);
-            query->bindValue(":second", boundary);
-            if (!execSqlQuery(query)) {
-                return false;
-            }
-            readMessages(query);
 
-            query = prepareQuery("SELECT 1" + conversationFilter + " AND log_time < FROM_UNIXTIME(:before) LIMIT 1");
+    if (page.needsWholeSecond) {
-            bindParticipants(query);
+        query = prepareQuery(selectMessages + conversationFilter + " AND log_time = FROM_UNIXTIME(:second)");
-            query->bindValue(":before", boundary);
+        bindParticipants(query);
-            if (!execSqlQuery(query)) {
+        query->bindValue(":second", page.boundarySecond);
-                return false;
+        if (!execSqlQuery(query)) {
-            }
+            return false;
-            hasMore = query->next();
         }
+        readMessages(query, page.messages);
+        std::reverse(page.messages.begin(), page.messages.end());
+
+        query = prepareQuery("SELECT 1" + conversationFilter + " AND log_time < FROM_UNIXTIME(:before) LIMIT 1");
+        bindParticipants(query);
+        query->bindValue(":before", page.boundarySecond);
+        if (!execSqlQuery(query)) {
+            return false;
+        }
+        page.hasMore = query->next();
     }
 
-    std::reverse(messages.begin(), messages.end());
+    messages = page.messages;
+    hasMore = page.hasMore;
     return true;
 }
 

servatrice/src/servatrice_database_interface.h

@@ -226,7 +226,9 @@ public:
                                                        bool *hasMore = nullptr);
     QList<ServerInfo_UserSession> getUserSessions(const QString &userName, int limit);
     bool getPrivateChatHistory(int callerId,
+                               const QString &callerName,
                                int peerId,
+                               const QString &peerName,
                                quint64 before,
                                int limit,
                                QList<ServerInfo_PrivateChatMessage> &messages,

servatrice/src/serversocketinterface.cpp

@@ -1735,7 +1735,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdGetPrivateChatHistory(c
     const int limit = qBound<quint32>(1, cmd.limit(), 200);
     QList<ServerInfo_PrivateChatMessage> messages;
     bool hasMore = false;
-    if (!sqlInterface->getPrivateChatHistory(userInfo->id(), peerId, cmd.before(), limit, messages, hasMore)) {
+    if (!sqlInterface->getPrivateChatHistory(userInfo->id(), QString::fromStdString(userInfo->name()), peerId, peer,
+                                             cmd.before(), limit, messages, hasMore)) {
         return Response::RespInternalError;
     }
 

tests/CMakeLists.txt

@@ -426,6 +426,19 @@ cockatrice_add_test(
   LABELS
   server
 )
+cockatrice_add_test(
+  NAME
+  private_chat_paging_test
+  SOURCES
+  ${CMAKE_SOURCE_DIR}/servatrice/src/private_chat_paging.cpp
+  private_chat_paging_test.cpp
+  LIBS
+  libcockatrice_protocol
+  INCLUDE_DIRS
+  ${CMAKE_SOURCE_DIR}/servatrice/src
+  LABELS
+  server
+)
 cockatrice_add_test(
   NAME
   latency_tracker_test

tests/private_chat_paging_test.cpp

@@ -0,0 +1,115 @@
+#include "private_chat_paging.h"
+
+#include <gtest/gtest.h>
+
+namespace
+{
+ServerInfo_PrivateChatMessage messageAt(quint64 time, const QString &text)
+{
+    ServerInfo_PrivateChatMessage message;
+    message.set_time(time);
+    message.set_sender_name("sender");
+    message.set_message(text.toStdString());
+    return message;
+}
+
+QList<ServerInfo_PrivateChatMessage> newestFirst(std::initializer_list<quint64> times)
+{
+    QList<ServerInfo_PrivateChatMessage> messages;
+    for (const quint64 time : times) {
+        messages.append(messageAt(time, QString::number(time)));
+    }
+    return messages;
+}
+
+QList<quint64> timesOf(const QList<ServerInfo_PrivateChatMessage> &messages)
+{
+    QList<quint64> times;
+    for (const auto &message : messages) {
+        times.append(message.time());
+    }
+    return times;
+}
+} // namespace
+
+TEST(PrivateChatPaging, EmptyInputProducesEmptyPage)
+{
+    const PrivateChatPage page = assemblePrivateChatPage({}, 50);
+
+    EXPECT_TRUE(page.messages.isEmpty());
+    EXPECT_FALSE(page.hasMore);
+    EXPECT_FALSE(page.needsWholeSecond);
+}
+
+TEST(PrivateChatPaging, PageShorterThanLimitIsReturnedOldestFirst)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 50);
+
+    EXPECT_EQ(timesOf(page.messages), QList<quint64>({100, 200, 300}));
+    EXPECT_FALSE(page.hasMore);
+    EXPECT_FALSE(page.needsWholeSecond);
+}
+
+TEST(PrivateChatPaging, PageExactlyAtLimitIsReturnedWithoutMore)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 3);
+
+    EXPECT_EQ(timesOf(page.messages), QList<quint64>({100, 200, 300}));
+    EXPECT_FALSE(page.hasMore);
+}
+
+TEST(PrivateChatPaging, DistinctSecondsKeepWholePage)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 100}), 2);
+
+    EXPECT_EQ(timesOf(page.messages), QList<quint64>({200, 300}));
+    EXPECT_TRUE(page.hasMore);
+    EXPECT_FALSE(page.needsWholeSecond);
+}
+
+TEST(PrivateChatPaging, BoundarySecondIsDeferredToNextPage)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({300, 200, 200}), 2);
+
+    EXPECT_EQ(timesOf(page.messages), QList<quint64>({300}));
+    EXPECT_TRUE(page.hasMore);
+    EXPECT_FALSE(page.needsWholeSecond);
+}
+
+TEST(PrivateChatPaging, WholeSecondIsRequestedWhenNothingElseFits)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({200, 200, 200}), 2);
+
+    EXPECT_TRUE(page.messages.isEmpty());
+    EXPECT_TRUE(page.hasMore);
+    EXPECT_TRUE(page.needsWholeSecond);
+    EXPECT_EQ(page.boundarySecond, 200);
+}
+
+TEST(PrivateChatPaging, WholeSecondIsRequestedForSingleMessageLimit)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({200, 200}), 1);
+
+    EXPECT_TRUE(page.messages.isEmpty());
+    EXPECT_TRUE(page.needsWholeSecond);
+    EXPECT_EQ(page.boundarySecond, 200);
+}
+
+TEST(PrivateChatPaging, BoundarySecondAtPageEdgeIsDropped)
+{
+    const PrivateChatPage page = assemblePrivateChatPage(newestFirst({400, 400, 300, 300}), 3);
+
+    EXPECT_EQ(timesOf(page.messages), QList<quint64>({400, 400}));
+    EXPECT_TRUE(page.hasMore);
+    EXPECT_FALSE(page.needsWholeSecond);
+}
+
+TEST(PrivateChatPaging, ReturnedMessagesKeepSenderAndText)
+{
+    const PrivateChatPage page = assemblePrivateChatPage({messageAt(200, "hello"), messageAt(100, "world")}, 50);
+
+    ASSERT_EQ(page.messages.size(), 2);
+    EXPECT_EQ(page.messages.at(0).message(), "world");
+    EXPECT_EQ(page.messages.at(1).message(), "hello");
+    EXPECT_EQ(page.messages.at(0).sender_name(), "sender");
+}