conversation_module: avoid two one to one requests

This allow to avoid to get a duplicate if a contact is added,
removed and re-added (as it will generate two conversations).
The first one is automatically declined and replaced by the new one.

GitLab: #855
Change-Id: I67d51d2286aaee3b29c9e9bdbcb10aa45f40aa26

src/jamidht/conversation.h

@@ -87,6 +87,13 @@ struct ConversationRequest
         return m.size() == om.size() && std::equal(m.begin(), m.end(), om.begin());
     }
 
+    bool isOneToOne() const {
+        try {
+            return metadatas.at("mode") == "0";
+        } catch (...) {}
+        return true;
+    }
+
     MSGPACK_DEFINE_MAP(from, conversationId, metadatas, received, declined)
 };
 

src/jamidht/conversation_module.cpp

@@ -269,15 +269,21 @@ public:
     {
         ConversationModule::saveConvRequests(accountId_, conversationsRequests_);
     }
+    void declineOtherConversationWith(const std::string& uri) noexcept;
     bool addConversationRequest(const std::string& id, const ConversationRequest& req)
     {
         std::lock_guard<std::mutex> lk(conversationsRequestsMtx_);
+        // Check that we're not adding a second one to one trust request
+        // NOTE: If a new one to one request is received, we can decline the previous one.
+        if (req.isOneToOne())
+            declineOtherConversationWith(req.from);
         auto it = conversationsRequests_.find(id);
         if (it != conversationsRequests_.end()) {
             // Check if updated
             if (req == it->second)
                 return false;
         }
+        JAMI_DEBUG("Adding conversation request from {} ({})", req.from, id);
         conversationsRequests_[id] = req;
         saveConvRequests();
         return true;
@@ -710,6 +716,22 @@ ConversationModule::Impl::getOneToOneConversation(const std::string& uri) const
     return {};
 }
 
+void
+ConversationModule::Impl::declineOtherConversationWith(const std::string& uri) noexcept
+{
+    // conversationsRequestsMtx_ MUST BE LOCKED
+    for (auto& [id, request] : conversationsRequests_) {
+        if (request.declined)
+            continue; // Ignore already declined requests
+        if (request.isOneToOne() && request.from == uri) {
+            JAMI_WARNING("Decline conversation request ({}) from {}", id, uri);
+            request.declined = std::time(nullptr);
+            emitSignal<libjami::ConversationSignal::ConversationRequestDeclined>(accountId_,
+                                                                                id);
+        }
+    }
+}
+
 std::vector<std::map<std::string, std::string>>
 ConversationModule::Impl::getConversationMembers(const std::string& conversationId,
                                                  bool includeBanned) const
@@ -1384,11 +1406,6 @@ ConversationModule::onTrustRequest(const std::string& uri,
                   pimpl_->accountId_.c_str());
         return;
     }
-    emitSignal<libjami::ConfigurationSignal::IncomingTrustRequest>(pimpl_->accountId_,
-                                                                   conversationId,
-                                                                   uri,
-                                                                   payload,
-                                                                   received);
     ConversationRequest req;
     req.from = uri;
     req.conversationId = conversationId;
@@ -1396,11 +1413,17 @@ ConversationModule::onTrustRequest(const std::string& uri,
     req.metadatas = ConversationRepository::infosFromVCard(vCard::utils::toMap(
         std::string_view(reinterpret_cast<const char*>(payload.data()), payload.size())));
     auto reqMap = req.toMap();
-    pimpl_->addConversationRequest(conversationId, std::move(req));
+    if (pimpl_->addConversationRequest(conversationId, std::move(req))) {
+        emitSignal<libjami::ConfigurationSignal::IncomingTrustRequest>(pimpl_->accountId_,
+                                                                        conversationId,
+                                                                        uri,
+                                                                        payload,
+                                                                        received);
         emitSignal<libjami::ConversationSignal::ConversationRequestReceived>(pimpl_->accountId_,
                                                                             conversationId,
                                                                             reqMap);
     }
+}
 
 void
 ConversationModule::onConversationRequest(const std::string& from, const Json::Value& value)
@@ -1424,7 +1447,7 @@ ConversationModule::onConversationRequest(const std::string& from, const Json::V
     }
     req.received = std::time(nullptr);
     auto reqMap = req.toMap();
-    pimpl_->addConversationRequest(convId, std::move(req));
+    if (pimpl_->addConversationRequest(convId, std::move(req))) {
         // Note: no need to sync here because other connected devices should receive
         // the same conversation request. Will sync when the conversation will be added
 
@@ -1432,6 +1455,7 @@ ConversationModule::onConversationRequest(const std::string& from, const Json::V
                                                                             convId,
                                                                             reqMap);
     }
+}
 
 std::string
 ConversationModule::peerFromConversationRequest(const std::string& convId) const

test/unitTest/conversation/conversationMembersEvent.cpp

@@ -88,6 +88,7 @@ public:
     void testBanUnbanGotFirstConv();
     void testBanHostWhileHosting();
     void testRemoveContactTwice();
+    void testAddContactTwice();
 
     std::string aliceId;
     std::string bobId;
@@ -132,6 +133,7 @@ private:
     CPPUNIT_TEST(testBanUnbanGotFirstConv);
     CPPUNIT_TEST(testBanHostWhileHosting);
     CPPUNIT_TEST(testRemoveContactTwice);
+    CPPUNIT_TEST(testAddContactTwice);
     CPPUNIT_TEST_SUITE_END();
 };
 
@@ -2666,6 +2668,63 @@ ConversationMembersEventTest::testRemoveContactTwice()
     CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return contactRemoved; }));
 }
 
+void
+ConversationMembersEventTest::testAddContactTwice()
+{
+    std::cout << "\nRunning test: " << __func__ << std::endl;
+
+    auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+    auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+    auto bobUri = bobAccount->getUsername();
+    auto aliceUri = aliceAccount->getUsername();
+    std::map<std::string, std::shared_ptr<libjami::CallbackWrapperBase>> confHandlers;
+    bool requestReceived = false;
+    confHandlers.insert(
+        libjami::exportable_callback<libjami::ConversationSignal::ConversationRequestReceived>(
+            [&](const std::string& accountId,
+                const std::string&,
+                std::map<std::string, std::string> /*metadatas*/) {
+                if (accountId == bobId)
+                    requestReceived = true;
+                cv.notify_one();
+            }));
+    std::string convId = "";
+    confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
+        [&](const std::string& accountId, const std::string& conversationId) {
+            if (accountId == aliceId)
+                convId = conversationId;
+            cv.notify_one();
+        }));
+    auto requestDeclined = false;
+    confHandlers.insert(
+        libjami::exportable_callback<libjami::ConversationSignal::ConversationRequestDeclined>(
+            [&](const std::string& accountId, const std::string&) {
+                if (accountId == bobId)
+                    requestDeclined = true;
+                cv.notify_one();
+            }));
+    auto contactRemoved = false;
+    confHandlers.insert(libjami::exportable_callback<libjami::ConfigurationSignal::ContactRemoved>(
+        [&](const std::string& accountId, const std::string& uri, bool) {
+            if (accountId == aliceId && uri == bobUri)
+                contactRemoved = true;
+            cv.notify_one();
+        }));
+    libjami::registerSignalHandlers(confHandlers);
+    requestReceived = false;
+    aliceAccount->addContact(bobUri);
+    aliceAccount->sendTrustRequest(bobUri, {});
+    CPPUNIT_ASSERT(cv.wait_for(lk, 10s, [&]() { return requestReceived; }));
+    requestReceived = false;
+    aliceAccount->removeContact(bobUri, false);
+    CPPUNIT_ASSERT(cv.wait_for(lk, 10s, [&]() { return contactRemoved; }));
+    // wait that connections are closed.
+    std::this_thread::sleep_for(10s);
+    aliceAccount->addContact(bobUri);
+    aliceAccount->sendTrustRequest(bobUri, {});
+    CPPUNIT_ASSERT(cv.wait_for(lk, 10s, [&]() { return requestDeclined && requestReceived; }));
+}
+
 } // namespace test
 } // namespace jami
 

test/unitTest/syncHistory/syncHistory.cpp

@@ -729,9 +729,10 @@ SyncHistoryTest::testSyncCreateAccountExportDeleteReimportWithConvReq()
         }));
     confHandlers.insert(
         libjami::exportable_callback<libjami::ConversationSignal::ConversationRequestReceived>(
-            [&](const std::string& /*accountId*/,
+            [&](const std::string& accountId,
                 const std::string& /* conversationId */,
                 std::map<std::string, std::string> /*metadatas*/) {
+                if (accountId == aliceId)
                     requestReceived = true;
                 cv.notify_one();
             }));
@@ -783,6 +784,7 @@ SyncHistoryTest::testSyncCreateAccountExportDeleteReimportWithConvReq()
     CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&] { return alice2Ready; }));
 
     // Should get the same request as before.
+    messageBobReceived = 0;
     libjami::acceptConversationRequest(alice2Id, convId);
     CPPUNIT_ASSERT(
         cv.wait_for(lk, 30s, [&]() { return conversationReady && messageBobReceived == 1; }));