diff --git a/src/jamidht/account_manager.cpp b/src/jamidht/account_manager.cpp index 3535defc1333ed8a2baddc04c453dfcb257086aa..9d0a4dd6b99af04d69dacb80708456742a85b906 100644 --- a/src/jamidht/account_manager.cpp +++ b/src/jamidht/account_manager.cpp @@ -341,6 +341,8 @@ AccountManager::startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dc v.confirm, v.conversationId, std::move(v.payload))) { + if (v.confirm) // No need to send a confirmation as already accepted here + return; auto conversationId = v.conversationId; // Check if there was an old active conversation. auto details = info_->contacts->getContactDetails(peer_account); diff --git a/src/jamidht/conversation.cpp b/src/jamidht/conversation.cpp index 392994c2f316ce4c154a1ccf1d9f5c4b75bbcf76..0a1f4126c4c9a97fcfc7c52c3e0d34a74d6b7cb4 100644 --- a/src/jamidht/conversation.cpp +++ b/src/jamidht/conversation.cpp @@ -1584,10 +1584,15 @@ Conversation::lastCommitId() const LogOptions options; options.nbOfCommits = 1; options.skipMerge = true; - auto messages = pimpl_->loadMessages(options); - if (messages.empty()) + History optHistory; + std::lock_guard lk(pimpl_->historyMtx_); + if (!pimpl_->loadedHistory_.messageList.empty()) { + return (*pimpl_->loadedHistory_.messageList.begin())->id; + } + auto res = pimpl_->loadMessages2(options, &optHistory); + if (res.empty()) return {}; - return messages.front().at(ConversationMapKeys::ID); + return (*optHistory.messageList.begin())->id; } std::vector<std::map<std::string, std::string>> @@ -2302,7 +2307,10 @@ Conversation::bootstrap(std::function<void()> onBootstraped, }); }); pimpl_->checkedMembers_.clear(); - if (!pimpl_->swarmManager_->setKnownNodes(devices)) { + // If is shutdown, the conversation was re-added, causing no new nodes to be connected, but just a classic connectivity change + if (pimpl_->swarmManager_->isShutdown()) { + pimpl_->swarmManager_->maintainBuckets(); + } else if (!pimpl_->swarmManager_->setKnownNodes(devices)) { fallback(this, true); } } diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp index ad01a002222f27f241f732dfaf3f1df2c7ef62d5..bed03e6779095eed068ca222c97c46e9d97260dc 100644 --- a/src/jamidht/conversation_module.cpp +++ b/src/jamidht/conversation_module.cpp @@ -536,6 +536,10 @@ ConversationModule::Impl::fetchNewCommits(const std::string& peer, if (not commitId.empty() && conv->conversation->getCommit(commitId) != std::nullopt) { return; } + if (conv->conversation->isRemoving()) { + JAMI_WARNING("[Account {}] Conversation {} is being removed", accountId_, conversationId); + return; + } if (!conv->conversation->isMember(peer, true)) { JAMI_WARNING("[Account {}] {} is not a member of {}", accountId_, peer, conversationId); return; @@ -807,8 +811,16 @@ ConversationModule::Impl::getOneToOneConversation(const std::string& uri) const auto details = acc->getContactDetails(uri); auto itRemoved = details.find("removed"); // If contact is removed there is no conversation - if (itRemoved != details.end() && itRemoved->second != "0") - return {}; + if (itRemoved != details.end() && itRemoved->second != "0") { + auto itBanned = details.find("banned"); + // If banned, conversation is still on disk + if (itBanned == details.end() || itBanned->second == "0") { + // Check if contact is removed + auto itAdded = details.find("added"); + if (std::stoi(itRemoved->second) > std::stoi(itAdded->second)) + return {}; + } + } auto it = details.find(libjami::Account::TrustRequest::CONVERSATIONID); if (it != details.end()) return it->second; @@ -852,6 +864,9 @@ void ConversationModule::Impl::removeRepositoryImpl(SyncedConversation& conv, bool sync, bool force) { if (conv.conversation && (force || conv.conversation->isRemoving())) { + // Stop fetch! + conv.pending.reset(); + JAMI_LOG("Remove conversation: {}", conv.info.id); try { if (conv.conversation->mode() == ConversationMode::ONE_TO_ONE) { @@ -2641,8 +2656,13 @@ ConversationModule::removeContact(const std::string& uri, bool banned) pimpl_->needsSyncingCb_({}); } } - if (banned) - return; // Keep the conversation in banned model + if (banned) { + auto conversationId = getOneToOneConversation(uri); + pimpl_->withConversation(conversationId, + [&](auto& conv) { + conv.shutdownConnections(); }); + return; // Keep the conversation in banned model but stop connections + } // Remove related conversation auto isSelf = uri == pimpl_->username_; std::vector<std::string> toRm; diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp index c675641d488960abef51ac2ac9a68cf7ee570981..cbf51c237d1e1aa791423d0d87948e3cc15a57d1 100644 --- a/src/jamidht/conversationrepository.cpp +++ b/src/jamidht/conversationrepository.cpp @@ -1771,7 +1771,7 @@ ConversationRepository::Impl::validateDevice() relativeParentPath = memberPath; parentPath /= relativeParentPath; if (relativeParentPath.empty()) { - JAMI_ERROR("Invalid parent path (not in members or admins"); + JAMI_ERROR("Invalid parent path (not in members or admins)"); return false; } wrongDeviceFile = false; diff --git a/src/jamidht/jami_contact.h b/src/jamidht/jami_contact.h index 84166098e7d8d7f75259ffea41317ba347ed7753..feea24de32d6c176218ffbdd3b5ca89f76825d6a 100644 --- a/src/jamidht/jami_contact.h +++ b/src/jamidht/jami_contact.h @@ -82,6 +82,10 @@ struct Contact if (c.confirmed != confirmed) { confirmed = c.confirmed or confirmed; } + if (isActive()) { + removed = 0; + banned = 0; + } if (c.isActive() and conversationId.empty() and not c.conversationId.empty()) { conversationId = c.conversationId; } diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp index b297614a18ef49373ec53a4cb3c1049ac3181b5f..2192ac1468a0a70e7c0cddc2afcf86f1a779fcdf 100644 --- a/src/jamidht/jamiaccount.cpp +++ b/src/jamidht/jamiaccount.cpp @@ -2872,14 +2872,13 @@ JamiAccount::updateConvForContact(const std::string& uri, const std::string& oldConv, const std::string& newConv) { - if (newConv != oldConv) { - std::lock_guard lock(configurationMutex_); - auto details = getContactDetails(uri); - auto itDetails = details.find(libjami::Account::TrustRequest::CONVERSATIONID); - if (itDetails != details.end() && itDetails->second != oldConv) { + auto cm = convModule(false); + if (newConv != oldConv && cm) { + auto conversation = cm->getOneToOneConversation(uri); + if (conversation != oldConv) { JAMI_DEBUG("Old conversation is not found in details {} - found: {}", oldConv, - itDetails->second); + conversation); return false; } accountManager_->updateContactConversation(uri, newConv); diff --git a/test/unitTest/conversation/conversationMembersEvent.cpp b/test/unitTest/conversation/conversationMembersEvent.cpp index 332a96939ae0fa01f8424ea5e3b1104e25afc693..328a4ce507c94f491728795aa5bfed03647e3175 100644 --- a/test/unitTest/conversation/conversationMembersEvent.cpp +++ b/test/unitTest/conversation/conversationMembersEvent.cpp @@ -54,6 +54,8 @@ struct UserData { bool stopped {false}; bool deviceAnnounced {false}; bool contactRemoved {false}; + bool contactAdded {false}; + Conversation::BootstrapStatus bootstrap {Conversation::BootstrapStatus::FAILED}; std::string payloadTrustRequest; std::vector<libjami::SwarmMessage> messages; std::vector<libjami::SwarmMessage> messagesUpdated; @@ -385,6 +387,15 @@ ConversationMembersEventTest::connectSignals() } cv.notify_one(); })); + confHandlers.insert(libjami::exportable_callback<libjami::ConfigurationSignal::ContactAdded>( + [&](const std::string& accountId, const std::string&, bool) { + if (accountId == bobId) { + bobData.contactAdded = true; + } else if (accountId == bob2Id) { + bob2Data.contactAdded = true; + } + cv.notify_one(); + })); libjami::registerSignalHandlers(confHandlers); } @@ -594,7 +605,7 @@ ConversationMembersEventTest::testAddAcceptOfflineThenConnects() // Accepts libjami::acceptConversationRequest(bobId, aliceData.conversationId); - CPPUNIT_ASSERT(!cv.wait_for(lk, 60s, [&]() { return !bobData.conversationId.empty(); })); + CPPUNIT_ASSERT(!cv.wait_for(lk, 30s, [&]() { return !bobData.conversationId.empty(); })); Manager::instance().sendRegister(aliceId, true); // This avoid to sync immediately CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&]() { return !bobData.conversationId.empty(); })); @@ -917,7 +928,8 @@ ConversationMembersEventTest::testMemberCannotBanOther() // Now Carla remove Bob as a member // remove from member & add into banned without voting for the ban simulateRemoval(carlaAccount, convId, bobUri); - CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return aliceData.errorDetected; })); + // Note: it may be possible that alice doesn't get the error if they got messages from bob (and bob rejects due to an error) + CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return aliceData.errorDetected || bobData.errorDetected; })); auto bobMsgSize = bobData.messages.size(); libjami::sendMessage(aliceId, convId, "hi"s, ""); @@ -1557,8 +1569,6 @@ ConversationMembersEventTest::testBanUnbanGotFirstConv() CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return !bobData.conversationId.empty() && !bob2Data.conversationId.empty() && aliceMsgSize + 1 == aliceData.messages.size(); })); - - // Bob bans alice, should update bob2 bobAccount->removeContact(aliceUri, true); CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return bobData.contactRemoved && bob2Data.contactRemoved; })); @@ -1570,26 +1580,16 @@ ConversationMembersEventTest::testBanUnbanGotFirstConv() auto getMessage = [](const auto& data, const auto& mid) -> bool { return std::find_if(data.messages.begin(), data.messages.end(), [&](auto& msg) { return msg.id == mid; }) != data.messages.end(); }; - CPPUNIT_ASSERT(!cv.wait_for(lk, 30s, [&]() { return getMessage(bobData, msgId) && getMessage(bob2Data, msgId); })); + // Connection MUST fail + CPPUNIT_ASSERT(!cv.wait_for(lk, 20s, [&]() { return getMessage(bobData, msgId) && getMessage(bob2Data, msgId); })); // Bobs re-add Alice + bobData.contactAdded = false; bob2Data.contactAdded = false; bobAccount->addContact(aliceUri); - bobAccount->sendTrustRequest(aliceUri, {}); - CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return bobData.conversationId == aliceData.conversationId && bob2Data.conversationId == aliceData.conversationId; })); - - // Alice can sends some messages now - aliceMsgSize = aliceData.messages.size(); - auto getMessageFromBody = [](const auto& data, const auto& body) -> std::string { - auto it = std::find_if(data.messages.begin(), data.messages.end(), [&](auto& msg) { return msg.body.find("body") != msg.body.end() && msg.body.at("body") == body; }); - if (it != data.messages.end()) { - return it->id; - } - return {}; - }; - libjami::sendMessage(aliceId, aliceData.conversationId, "hi"s, ""); - CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return !getMessageFromBody(aliceData, "hi").empty(); })); - msgId = getMessageFromBody(aliceData, "hi"); - CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return getMessage(bobData, msgId) && getMessage(bob2Data, msgId); })); + CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return bobData.contactAdded + && bob2Data.contactAdded + && bobData.conversationId == bob2Data.conversationId + && bobData.conversationId == aliceData.conversationId; })); } void diff --git a/test/unitTest/syncHistory/syncHistory.cpp b/test/unitTest/syncHistory/syncHistory.cpp index c3bbf7122fa5e14bcb669b64d3c97b52f7106fb5..f3ba61e55dec10e5ac52a39382e36e1594b7d576 100644 --- a/test/unitTest/syncHistory/syncHistory.cpp +++ b/test/unitTest/syncHistory/syncHistory.cpp @@ -890,7 +890,8 @@ SyncHistoryTest::testLastInteractionAfterSomeMessages() alice2Id = Manager::instance().addAccount(details); CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() {return alice2Data.deviceAnnounced; })); auto getMessageFromBody = [](const auto& data, const auto& body) -> std::string { - auto it = std::find_if(data.messages.begin(), data.messages.end(), [&](auto& msg) { return msg.body.find("body") != msg.body.end() && msg.body.at("body") == body; }); + auto it = std::find_if(data.messages.begin(), data.messages.end(), [&](auto& msg) { + return msg.body.find("body") != msg.body.end() && msg.body.at("body") == body; }); if (it != data.messages.end()) { return it->id; }