From 589d1bc2de3adf1fd57ad083ccc26ccc9c2098fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Thu, 15 Jun 2023 12:53:27 -0400
Subject: [PATCH] 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 | 7 +++
src/jamidht/conversation_module.cpp | 54 ++++++++++++-----
.../conversation/conversationMembersEvent.cpp | 59 +++++++++++++++++++
test/unitTest/syncHistory/syncHistory.cpp | 6 +-
4 files changed, 109 insertions(+), 17 deletions(-)
diff --git a/src/jamidht/conversation.h b/src/jamidht/conversation.h
index 46812fcb7a..96d778ea88 100644
--- a/src/jamidht/conversation.h
+++ b/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)
};
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index bc1887ba49..59175593d9 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/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,10 +1413,16 @@ 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));
- emitSignal<libjami::ConversationSignal::ConversationRequestReceived>(pimpl_->accountId_,
- conversationId,
- reqMap);
+ 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
@@ -1424,13 +1447,14 @@ ConversationModule::onConversationRequest(const std::string& from, const Json::V
}
req.received = std::time(nullptr);
auto reqMap = req.toMap();
- 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
+ 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
- emitSignal<libjami::ConversationSignal::ConversationRequestReceived>(pimpl_->accountId_,
- convId,
- reqMap);
+ emitSignal<libjami::ConversationSignal::ConversationRequestReceived>(pimpl_->accountId_,
+ convId,
+ reqMap);
+ }
}
std::string
diff --git a/test/unitTest/conversation/conversationMembersEvent.cpp b/test/unitTest/conversation/conversationMembersEvent.cpp
index 3ddeafa18e..ca63628081 100644
--- a/test/unitTest/conversation/conversationMembersEvent.cpp
+++ b/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
diff --git a/test/unitTest/syncHistory/syncHistory.cpp b/test/unitTest/syncHistory/syncHistory.cpp
index 53309d85d3..f40b8190de 100644
--- a/test/unitTest/syncHistory/syncHistory.cpp
+++ b/test/unitTest/syncHistory/syncHistory.cpp
@@ -729,10 +729,11 @@ 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*/) {
- requestReceived = true;
+ if (accountId == aliceId)
+ requestReceived = true;
cv.notify_one();
}));
confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
@@ -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; }));
--
GitLab