diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index d6380bab2974433020aeaf4375416564dbf17d41..40c7dd080e5aafdac60c3983d55785000927e524 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -302,6 +302,22 @@ public:
mutable std::mutex deviceToUriMtx_;
mutable std::map<std::string, std::string> deviceToUri_;
+ std::string uriFromDevice(const std::string& deviceId, const std::string& commitId) const
+ {
+ auto repo = repository();
+ if (!repo)
+ return {};
+ auto tree = treeAtCommit(repo.get(), commitId);
+ auto deviceFile = fmt::format("devices/{}.crt", deviceId);
+ auto blob_device = fileAtTree(deviceFile, tree);
+ if (!blob_device) {
+ JAMI_ERROR("{} announced but not found", deviceId);
+ return {};
+ }
+ auto deviceCert = dht::crypto::Certificate(as_view(blob_device));
+ return deviceCert.getIssuerUID();
+ }
+
/**
* Verify that a certificate modification is correct
* @param certPath Where the certificate is saved (relative path)
@@ -885,15 +901,6 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
auto repo = repository();
if (!repo)
return false;
- auto treeNew = treeAtCommit(repo.get(), commitId);
- if (not treeNew)
- return false;
- std::string deviceFile = fmt::format("devices/{}.crt", userDevice);
- if (!fileAtTree(deviceFile, treeNew)) {
- JAMI_ERROR("{} announced but not found", deviceFile.c_str());
- return false;
- }
-
// Here, we check that a file device is modified or not.
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
if (changedFiles.size() == 0)
@@ -901,10 +908,12 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
// If a certificate is modified (in the changedFiles), it MUST be a certificate from the user
// Retrieve userUri
- auto userUri = uriFromDevice(userDevice);
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto userUri = uriFromDevice(userDevice, commitId);
if (userUri.empty())
return false;
+ std::string userDeviceFile = fmt::format("devices/{}.crt", userDevice);
std::string adminsFile = fmt::format("admins/{}.crt", userUri);
std::string membersFile = fmt::format("members/{}.crt", userUri);
auto treeOld = treeAtCommit(repo.get(), parentId);
@@ -924,7 +933,7 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
JAMI_ERROR("Invalid certificate {}", changedFile);
return false;
}
- } else if (changedFile == deviceFile) {
+ } else if (changedFile == userDeviceFile) {
// In this case, device is added or modified (certificate expiration)
auto oldFile = fileAtTree(changedFile, treeOld);
std::string_view oldCert;
@@ -949,7 +958,12 @@ bool
ConversationRepository::Impl::checkEdit(const std::string& userDevice,
const ConversationCommit& commit) const
{
- auto userUri = uriFromDevice(userDevice);
+ auto repo = repository();
+ if (!repo)
+ return false;
+ auto userUri = uriFromDevice(userDevice, commit.id);
+ if (userUri.empty())
+ return false;
// Check that edited commit is found, for the same author, and editable (plain/text)
auto commitMap = convCommitToMap(commit);
if (commitMap == std::nullopt) {
@@ -1017,14 +1031,9 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice,
if (not treeNew or not treeOld)
return false;
- if (not deviceFile.empty()) {
- if (fileAtTree(deviceFile, treeOld)) {
- JAMI_ERROR("Invalid device file modified: {}", deviceFile);
- return false;
- }
- }
-
- auto userUri = uriFromDevice(userDevice);
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
// Check that voter is admin
auto adminFile = fmt::format("admins/{}.crt", userUri);
@@ -1108,9 +1117,8 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
auto repo = repository();
- if (userUri.empty() or not repo)
+ if (not repo)
return false;
std::string repoPath = git_repository_workdir(repo.get());
@@ -1123,6 +1131,10 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
}
}
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
// Check that only /invited/uri.crt is added & deviceFile & CRLs
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
if (changedFiles.size() == 0) {
@@ -1161,8 +1173,6 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
if (not treeOld)
return false;
auto treeNew = treeAtCommit(repo.get(), commitId);
- if (not treeNew)
- return false;
auto blob_invite = fileAtTree(invitedFile, treeNew);
if (!blob_invite) {
JAMI_ERROR("Invitation not found for commit {}", commitId);
@@ -1265,8 +1275,6 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice,
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
-
// Retrieve tree for recent commit
auto repo = repository();
if (!repo)
@@ -1304,11 +1312,15 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice,
// Check that removed devices are for removed member (or directly uriMember)
for (const auto& deviceUri : devicesRemoved) {
deviceFile = fmt::format("devices/{}.crt", deviceUri);
- if (!fileAtTree(deviceFile, treeOld)) {
+ auto blob_device = fileAtTree(deviceFile, treeOld);
+ if (!blob_device) {
JAMI_ERROR("device not found added ({})", deviceFile);
return false;
}
- if (uriMember != uriFromDevice(deviceUri)
+ auto deviceCert = dht::crypto::Certificate(as_view(blob_device));
+ auto userUri = deviceCert.getIssuerUID();
+
+ if (uriMember != userUri
and uriMember != deviceUri /* If device is removed */) {
JAMI_ERROR("device removed but not for removed user ({})", deviceFile);
return false;
@@ -1325,8 +1337,6 @@ ConversationRepository::Impl::checkValidVoteResolution(const std::string& userDe
const std::string& parentId,
const std::string& voteType) const
{
- auto userUri = uriFromDevice(userDevice);
-
// Retrieve tree for recent commit
auto repo = repository();
if (!repo)
@@ -1408,6 +1418,10 @@ ConversationRepository::Impl::checkValidVoteResolution(const std::string& userDe
}
}
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
// Check that voters are admins
adminFile = fmt::format("admins/{}.crt", userUri);
if (!fileAtTree(adminFile, treeOld)) {
@@ -1445,7 +1459,19 @@ ConversationRepository::Impl::checkValidProfileUpdate(const std::string& userDev
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
+ // Retrieve tree for recent commit
+ auto repo = repository();
+ if (!repo)
+ return false;
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto treeOld = treeAtCommit(repo.get(), parentId);
+ if (not treeNew or not treeOld)
+ return false;
+
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
auto valid = false;
{
std::lock_guard<std::mutex> lk(membersMtx_);
@@ -1461,15 +1487,6 @@ ConversationRepository::Impl::checkValidProfileUpdate(const std::string& userDev
return false;
}
- // Retrieve tree for recent commit
- auto repo = repository();
- if (!repo)
- return false;
- auto treeNew = treeAtCommit(repo.get(), commitId);
- auto treeOld = treeAtCommit(repo.get(), parentId);
- if (not treeNew or not treeOld)
- return false;
-
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
// Check that no weird file is added nor removed
for (const auto& f : changedFiles) {
@@ -1577,7 +1594,12 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
JAMI_WARNING("Invalid repository detected");
return false;
}
- auto userUri = uriFromDevice(userDevice);
+
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, ""));
// NOTE: libgit2 return a diff with /, not DIR_SEPARATOR_DIR
@@ -1607,21 +1629,6 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
std::string crlFile = fmt::format("CRLs/{}", userUri);
std::string invitedFile = fmt::format("invited/{}", invited);
- git_oid oid;
- git_commit* commit = nullptr;
- if (git_oid_fromstr(&oid, commitId.c_str()) < 0
- || git_commit_lookup(&commit, repo.get(), &oid) < 0) {
- JAMI_WARNING("Failed to look up commit {}", commitId);
- return false;
- }
- GitCommit gc = {commit, git_commit_free};
- git_tree* tNew = nullptr;
- if (git_commit_tree(&tNew, gc.get()) < 0) {
- JAMI_ERROR("Could not look up initial tree");
- return false;
- }
- GitTree treeNew = {tNew, git_tree_free};
-
// Check that admin cert is added
// Check that device cert is added
// Check CRLs added
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index 410a276bbae54643c5de60d3b36f978fade41b88..280b1218e58d1e9415b99f81b8c143465e539e27 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -87,7 +87,6 @@ private:
void testReplaceWithBadCertificate();
void testSendMessageTriggerMessageReceived();
void testMergeTwoDifferentHeads();
- void testMergeAfterMigration();
void testSendMessageToMultipleParticipants();
void testPingPongMessages();
void testIsComposing();
@@ -111,7 +110,6 @@ private:
void testMemberCannotUpdateProfile();
void testUpdateProfileWithBadFile();
void testFetchProfileUnauthorized();
- void testDoNotLoadIncorrectConversation();
void testSyncingWhileAccepting();
void testCountInteractions();
void testReplayConversation();
@@ -141,7 +139,6 @@ private:
CPPUNIT_TEST(testReplaceWithBadCertificate);
CPPUNIT_TEST(testSendMessageTriggerMessageReceived);
CPPUNIT_TEST(testMergeTwoDifferentHeads);
- CPPUNIT_TEST(testMergeAfterMigration);
CPPUNIT_TEST(testSendMessageToMultipleParticipants);
CPPUNIT_TEST(testPingPongMessages);
CPPUNIT_TEST(testIsComposing);
@@ -165,7 +162,6 @@ private:
CPPUNIT_TEST(testMemberCannotUpdateProfile);
CPPUNIT_TEST(testUpdateProfileWithBadFile);
CPPUNIT_TEST(testFetchProfileUnauthorized);
- CPPUNIT_TEST(testDoNotLoadIncorrectConversation);
CPPUNIT_TEST(testSyncingWhileAccepting);
CPPUNIT_TEST(testCountInteractions);
CPPUNIT_TEST(testReplayConversation);
@@ -725,122 +721,6 @@ ConversationTest::testMergeTwoDifferentHeads()
libjami::unregisterSignalHandlers();
}
-void
-ConversationTest::testMergeAfterMigration()
-{
- std::cout << "\nRunning test: " << __func__ << std::endl;
-
- auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
- auto carlaAccount = Manager::instance().getAccount<JamiAccount>(carlaId);
- auto aliceUri = aliceAccount->getUsername();
- auto carlaUri = carlaAccount->getUsername();
- aliceAccount->trackBuddyPresence(carlaUri, true);
- carlaAccount->trackBuddyPresence(aliceUri, true);
-
- std::map<std::string, std::shared_ptr<libjami::CallbackWrapperBase>> confHandlers;
- bool conversationReady = false, conversationAliceReady = false;
- confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
- [&](const std::string& accountId, const std::string& /* conversationId */) {
- if (accountId == aliceId)
- conversationAliceReady = true;
- else if (accountId == carlaId)
- conversationReady = true;
- cv.notify_one();
- }));
- std::string carlaGotMessage, aliceGotMessage;
- confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::MessageReceived>(
- [&](const std::string& accountId,
- const std::string& conversationId,
- std::map<std::string, std::string> message) {
- if (accountId == carlaId) {
- carlaGotMessage = message["id"];
- } else if (accountId == aliceId) {
- aliceGotMessage = message["id"];
- }
- cv.notify_one();
- }));
- bool carlaConnected = false;
- confHandlers.insert(
- libjami::exportable_callback<libjami::ConfigurationSignal::VolatileDetailsChanged>(
- [&](const std::string&, const std::map<std::string, std::string>&) {
- auto details = carlaAccount->getVolatileAccountDetails();
- auto deviceAnnounced
- = details[libjami::Account::VolatileProperties::DEVICE_ANNOUNCED];
- if (deviceAnnounced == "true") {
- carlaConnected = true;
- cv.notify_one();
- }
- }));
- libjami::registerSignalHandlers(confHandlers);
-
- auto convId = libjami::startConversation(aliceId);
- CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&] { return conversationAliceReady; }));
-
- aliceGotMessage = "";
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !aliceGotMessage.empty(); }));
-
- // Cp conversations & convInfo
- auto repoPathAlice = fileutils::get_data_dir() + DIR_SEPARATOR_STR
- + aliceAccount->getAccountID() + DIR_SEPARATOR_STR + "conversations";
- auto repoPathCarla = fileutils::get_data_dir() + DIR_SEPARATOR_STR
- + carlaAccount->getAccountID() + DIR_SEPARATOR_STR + "conversations";
- auto p = std::filesystem::path(repoPathCarla);
- dhtnet::fileutils::recursive_mkdir(p.parent_path());
- std::filesystem::copy(repoPathAlice, repoPathCarla, std::filesystem::copy_options::recursive);
- auto ciPathAlice = fileutils::get_data_dir() + DIR_SEPARATOR_STR + aliceAccount->getAccountID()
- + DIR_SEPARATOR_STR + "convInfo";
- auto ciPathCarla = fileutils::get_data_dir() + DIR_SEPARATOR_STR + carlaAccount->getAccountID()
- + DIR_SEPARATOR_STR + "convInfo";
- std::filesystem::remove_all(ciPathCarla);
- std::filesystem::copy(ciPathAlice, ciPathCarla);
-
- // Accept for carla
- ConversationRepository repo(carlaAccount, convId);
- repo.join();
-
- std::filesystem::remove_all(repoPathAlice);
- p = std::filesystem::path(repoPathAlice);
- dhtnet::fileutils::recursive_mkdir(p.parent_path());
- std::filesystem::copy(repoPathCarla, repoPathAlice, std::filesystem::copy_options::recursive);
-
- CPPUNIT_ASSERT(std::filesystem::is_directory(repoPathAlice));
- CPPUNIT_ASSERT(std::filesystem::is_directory(repoPathAlice + "/" + convId));
-
- // Makes different heads
- carlaAccount->convModule()->loadConversations(); // necessary to load conversation
- carlaGotMessage = "";
- libjami::sendMessage(carlaId, convId, "hi"s, "");
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !carlaGotMessage.empty(); }));
- aliceGotMessage = "";
- libjami::sendMessage(aliceId, convId, "hi"s, "");
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !aliceGotMessage.empty(); }));
-
- // Connect only carla to migrate it.
- Manager::instance().sendRegister(aliceId, false);
- Manager::instance().sendRegister(carlaId, true);
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return carlaConnected; }));
-
- // Migrate carla
- CPPUNIT_ASSERT(carlaAccount->setValidity("", {}, 10));
- auto now = std::chrono::system_clock::now();
- std::this_thread::sleep_until(now + 20s);
- carlaConnected = false;
- carlaAccount->forceReloadAccount();
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return carlaConnected; }));
- // Force move certificate in alice's certStore (because forceReloadAccount is different than a real migration)
- // and DHT state is still the old one
- aliceAccount->certStore().pinCertificate(carlaAccount->identity().second);
-
- // Start Carla, should merge and all messages should be there
- Manager::instance().sendRegister(aliceId, true);
- carlaGotMessage = "";
- aliceGotMessage = "";
- CPPUNIT_ASSERT(
- cv.wait_for(lk, 60s, [&] { return !carlaGotMessage.empty() && !aliceGotMessage.empty(); }));
- libjami::unregisterSignalHandlers();
-}
-
void
ConversationTest::testSendMessageToMultipleParticipants()
{
@@ -2811,33 +2691,6 @@ END:VCARD";
libjami::unregisterSignalHandlers();
}
-void
-ConversationTest::testDoNotLoadIncorrectConversation()
-{
- std::cout << "\nRunning test: " << __func__ << std::endl;
-
- auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
- auto uri = aliceAccount->getUsername();
- auto convId = libjami::startConversation(aliceId);
-
- auto convInfos = libjami::conversationInfos(aliceId, convId);
- CPPUNIT_ASSERT(convInfos.find("mode") != convInfos.end());
- CPPUNIT_ASSERT(convInfos.find("syncing") == convInfos.end());
-
- Manager::instance().sendRegister(aliceId, false);
- auto repoGitPath = fileutils::get_data_dir() + DIR_SEPARATOR_STR + aliceAccount->getAccountID()
- + DIR_SEPARATOR_STR + "conversations" + DIR_SEPARATOR_STR + convId
- + DIR_SEPARATOR_STR + ".git";
- dhtnet::fileutils::removeAll(repoGitPath, true); // This make the repository not usable
-
- aliceAccount->convModule()
- ->loadConversations(); // Refresh. This should detect the incorrect conversations.
-
- // the conv should be detected as non existing
- convInfos = libjami::conversationInfos(aliceId, convId);
- CPPUNIT_ASSERT(convInfos.empty());
-}
-
void
ConversationTest::testSyncingWhileAccepting()
{