From b499034ba4c0e1f1cb6a06e5fa3d497df7a5c716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Blin?= Date: Fri, 25 Feb 2022 17:35:05 -0500 Subject: [PATCH] swarm: improve vote system and supports unban This patch fixes several behaviours: + Banning a member now stores the previous state of the member + This allow to store empty .crt files for invited + Unbanning a member is now possible for an admin + This re-add the member in its previous state https://git.jami.net/savoirfairelinux/jami-product-backlog/-/issues/51 Change-Id: I34d5913c023043e07544f1b8bb6211aea5db0065 --- src/jamidht/conversation.cpp | 126 +++++- src/jamidht/conversation.h | 2 +- src/jamidht/conversationrepository.cpp | 392 +++++++++++++----- src/jamidht/conversationrepository.h | 34 +- .../conversation/conversationMembersEvent.cpp | 129 +++++- 5 files changed, 544 insertions(+), 139 deletions(-) diff --git a/src/jamidht/conversation.cpp b/src/jamidht/conversation.cpp index 56312296d..62d8bf283 100644 --- a/src/jamidht/conversation.cpp +++ b/src/jamidht/conversation.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014-2019 Savoir-faire Linux Inc. + * Copyright (C) 2014-2022 Savoir-faire Linux Inc. * * Author: Adrien Béraud * Author: Sébastien Blin @@ -222,6 +222,8 @@ public: action = 2; else if (actionStr == "ban") action = 3; + else if (actionStr == "unban") + action = 4; if (action != -1) { announceMember = true; emitSignal( @@ -320,6 +322,28 @@ public: msgpack::pack(file, lastDisplayed_); } + void voteUnban(const std::string& contactUri, const std::string& type, const OnDoneCb& cb); + + std::string bannedType(const std::string& uri) const + { + auto shared = account_.lock(); + if (!shared) + return {}; + auto bannedMember = fmt::format("{}/banned/members/{}.crt", repoPath(), uri); + if (fileutils::isFile(bannedMember)) + return "members"; + auto bannedAdmin = fmt::format("{}/banned/admins/{}.crt", repoPath(), uri); + if (fileutils::isFile(bannedAdmin)) + return "admins"; + auto bannedInvited = fmt::format("{}/banned/invited/{}", repoPath(), uri); + if (fileutils::isFile(bannedInvited)) + return "invited"; + auto bannedDevice = fmt::format("{}/banned/devices/{}.crt", repoPath(), uri); + if (fileutils::isFile(bannedDevice)) + return "devices"; + return {}; + } + std::unique_ptr repository_; std::weak_ptr account_; std::atomic_bool isRemoving_ {false}; @@ -523,8 +547,23 @@ Conversation::addMember(const std::string& contactUri, const OnDoneCb& cb) return; } if (isBanned(contactUri)) { - JAMI_WARN("Could not add member %s because this member is banned", contactUri.c_str()); - cb(false, ""); + if (pimpl_->isAdmin()) { + dht::ThreadPool::io().run( + [w = weak(), contactUri = std::move(contactUri), cb = std::move(cb)] { + if (auto sthis = w.lock()) { + auto members = sthis->pimpl_->repository_->members(); + std::string type = sthis->pimpl_->bannedType(contactUri); + if (type.empty()) { + cb(false, {}); + return; + } + sthis->pimpl_->voteUnban(contactUri, type, cb); + } + }); + } else { + JAMI_WARN("Could not add member %s because this member is banned", contactUri.c_str()); + cb(false, ""); + } return; } @@ -541,6 +580,44 @@ Conversation::addMember(const std::string& contactUri, const OnDoneCb& cb) }); } +void +Conversation::Impl::voteUnban(const std::string& contactUri, + const std::string& type, + const OnDoneCb& cb) +{ + // Check if admin + if (!isAdmin()) { + JAMI_WARN("You're not an admin of this repo. Cannot unban %s", contactUri.c_str()); + cb(false, {}); + return; + } + + // Vote for removal + std::unique_lock lk(writeMtx_); + auto voteCommit = repository_->voteUnban(contactUri, type); + if (voteCommit.empty()) { + JAMI_WARN("Unbanning %s failed", contactUri.c_str()); + cb(false, ""); + return; + } + + auto lastId = voteCommit; + std::vector commits; + commits.emplace_back(voteCommit); + + // If admin, check vote + auto resolveCommit = repository_->resolveVote(contactUri, type, "unban"); + if (!resolveCommit.empty()) { + commits.emplace_back(resolveCommit); + lastId = resolveCommit; + JAMI_WARN("Vote solved for unbanning %s.", contactUri.c_str()); + } + announce(commits); + lk.unlock(); + if (cb) + cb(!lastId.empty(), lastId); +} + void Conversation::removeMember(const std::string& contactUri, bool isDevice, const OnDoneCb& cb) { @@ -555,9 +632,34 @@ Conversation::removeMember(const std::string& contactUri, bool isDevice, const O cb(false, {}); return; } + + // Get current user type + std::string type; + if (isDevice) { + type = "devices"; + } else { + auto members = sthis->pimpl_->repository_->members(); + for (const auto& member : members) { + if (member.uri == contactUri) { + if (member.role == MemberRole::INVITED) { + type = "invited"; + } else if (member.role == MemberRole::ADMIN) { + type = "admins"; + } else if (member.role == MemberRole::MEMBER) { + type = "members"; + } + break; + } + } + if (type.empty()) { + cb(false, {}); + return; + } + } + // Vote for removal std::unique_lock lk(sthis->pimpl_->writeMtx_); - auto voteCommit = sthis->pimpl_->repository_->voteKick(contactUri, isDevice); + auto voteCommit = sthis->pimpl_->repository_->voteKick(contactUri, type); if (voteCommit.empty()) { JAMI_WARN("Kicking %s failed", contactUri.c_str()); cb(false, ""); @@ -569,7 +671,7 @@ Conversation::removeMember(const std::string& contactUri, bool isDevice, const O commits.emplace_back(voteCommit); // If admin, check vote - auto resolveCommit = sthis->pimpl_->repository_->resolveVote(contactUri, isDevice); + auto resolveCommit = sthis->pimpl_->repository_->resolveVote(contactUri, type, "ban"); if (!resolveCommit.empty()) { commits.emplace_back(resolveCommit); lastId = resolveCommit; @@ -579,8 +681,7 @@ Conversation::removeMember(const std::string& contactUri, bool isDevice, const O } sthis->pimpl_->announce(commits); lk.unlock(); - if (cb) - cb(!lastId.empty(), lastId); + cb(!lastId.empty(), lastId); } }); } @@ -663,16 +764,9 @@ Conversation::isMember(const std::string& uri, bool includeInvited) const } bool -Conversation::isBanned(const std::string& uri, bool isDevice) const +Conversation::isBanned(const std::string& uri) const { - auto shared = pimpl_->account_.lock(); - if (!shared) - return true; - - auto type = isDevice ? "devices" : "members"; - auto bannedPath = pimpl_->repoPath() + DIR_SEPARATOR_STR + "banned" + DIR_SEPARATOR_STR + type - + DIR_SEPARATOR_STR + uri + ".crt"; - return fileutils::isFile(bannedPath); + return !pimpl_->bannedType(uri).empty(); } void diff --git a/src/jamidht/conversation.h b/src/jamidht/conversation.h index aa06928b2..5d180ec92 100644 --- a/src/jamidht/conversation.h +++ b/src/jamidht/conversation.h @@ -174,7 +174,7 @@ public: * @return true if uri is a member */ bool isMember(const std::string& uri, bool includeInvited = false) const; - bool isBanned(const std::string& uri, bool isDevice = false) const; + bool isBanned(const std::string& uri) const; // Message send void sendMessage(std::string&& message, diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp index 1ed4b93ea..b9e897b12 100644 --- a/src/jamidht/conversationrepository.cpp +++ b/src/jamidht/conversationrepository.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2019 Savoir-faire Linux Inc. + * Copyright (C) 2019-2022 Savoir-faire Linux Inc. * Author: Sébastien Blin * * This program is free software; you can redistribute it and/or modify @@ -99,6 +99,11 @@ public: const std::string& uriMember, const std::string& commitid, const std::string& parentId) const; + bool checkValidVoteResolution(const std::string& userDevice, + const std::string& uriMember, + const std::string& commitId, + const std::string& parentId, + const std::string& voteType) const; bool checkValidProfileUpdate(const std::string& userDevice, const std::string& commitid, const std::string& parentId) const; @@ -131,6 +136,9 @@ public: std::vector getInitialMembers() const; + bool resolveBan(const std::string& type, const std::string& uri); + bool resolveUnban(const std::string& type, const std::string& uri); + std::weak_ptr account_; const std::string id_; mutable std::optional mode_ {}; @@ -892,20 +900,25 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice, } // Check votedFile path - const std::regex regex_votes("votes.(members|devices).(\\w+).(\\w+)"); + const std::regex regex_votes("votes.(\\w+).(members|devices|admins|invited).(\\w+).(\\w+)"); std::smatch base_match; - if (!std::regex_match(votedFile, base_match, regex_votes) or base_match.size() != 4) { + if (!std::regex_match(votedFile, base_match, regex_votes) or base_match.size() != 5) { JAMI_WARN("Invalid votes path: %s", votedFile.c_str()); return false; } - std::string matchedUri = base_match[3]; + std::string matchedUri = base_match[4]; if (matchedUri != userUri) { JAMI_ERR("Admin voted for other user: %s vs %s", userUri.c_str(), matchedUri.c_str()); return false; } - std::string votedUri = base_match[2]; - std::string type = base_match[1]; + std::string votedUri = base_match[3]; + std::string type = base_match[2]; + std::string voteType = base_match[1]; + if (voteType != "ban" && voteType != "unban") { + JAMI_ERR("Unrecognized vote %s", voteType.c_str()); + return false; + } // Check that vote file is empty and wasn't modified if (fileAtTree(votedFile, treeOld)) { @@ -926,7 +939,7 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice, } // Check that peer voted is only other device or other member - if (type == "members") { + if (type != "devices") { if (votedUri == userUri) { JAMI_ERR("Detected vote for self: %s", votedUri.c_str()); return false; @@ -937,7 +950,7 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice, JAMI_ERR("No member file found for vote: %s", votedUri.c_str()); return false; } - } else if (type == "devices") { + } else { // Check not current device if (votedUri == userDevice) { JAMI_ERR("Detected vote for self: %s", votedUri.c_str()); @@ -949,9 +962,6 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice, JAMI_ERR("No device file found for vote: %s", votedUri.c_str()); return false; } - } else { - JAMI_ERR("Unknown vote type: %s", type.c_str()); - return false; } return true; @@ -1109,15 +1119,13 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice, const std::string& parentId) const { auto userUri = uriFromDevice(userDevice); - auto removeSelf = userUri == uriMember; // 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) + if (not treeOld) return false; auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId)); @@ -1127,39 +1135,19 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice, std::string memberFile = fmt::format("members/{}.crt", uriMember); std::string crlFile = fmt::format("CRLs/{}", uriMember); std::string invitedFile = fmt::format("invited/{}", uriMember); - std::vector voters; std::vector devicesRemoved; - std::vector bannedFiles; - // Check that no weird file is added nor removed - const std::regex regex_votes("votes.(members|devices).(\\w+).(\\w+)"); + // Check that no weird file is added nor removed const std::regex regex_devices("devices.(\\w+)\\.crt"); - const std::regex regex_banned("banned.(members|devices).(\\w+)\\.crt"); std::smatch base_match; for (const auto& f : changedFiles) { - if (f == deviceFile || f == adminFile || f == memberFile || f == crlFile || f == invitedFile) { + if (f == deviceFile || f == adminFile || f == memberFile || f == crlFile + || f == invitedFile) { // Ignore continue; - } else if (std::regex_match(f, base_match, regex_votes)) { - if (base_match.size() != 4 or base_match[2] != uriMember) { - JAMI_ERR("Invalid vote file detected: %s", f.c_str()); - return false; - } - voters.emplace_back(base_match[3]); - // Check that votes were not added here - if (!fileAtTree(f, treeOld)) { - JAMI_ERR("invalid vote added (%s)", f.c_str()); - return false; - } } else if (std::regex_match(f, base_match, regex_devices)) { if (base_match.size() == 2) devicesRemoved.emplace_back(base_match[1]); - } else if (std::regex_match(f, base_match, regex_banned)) { - bannedFiles.emplace_back(f); - if (base_match.size() != 3 or base_match[2] != uriMember) { - JAMI_ERR("Invalid banned file detected :%s", f.c_str()); - return false; - } } else { JAMI_ERR("Unwanted changed file detected: %s", f.c_str()); return false; @@ -1180,11 +1168,100 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice, } } - if (removeSelf) { - return bannedFiles.empty() && voters.empty(); + return true; +} + +bool +ConversationRepository::Impl::checkValidVoteResolution(const std::string& userDevice, + const std::string& uriMember, + const std::string& commitId, + const std::string& parentId, + const std::string& voteType) const +{ + auto userUri = uriFromDevice(userDevice); + + // Retrieve tree for recent commit + auto repo = repository(); + if (!repo) + return false; + auto treeOld = treeAtCommit(repo.get(), parentId); + if (not treeOld) + return false; + + auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId)); + // NOTE: libgit2 return a diff with /, not DIR_SEPARATOR_DIR + std::string deviceFile = fmt::format("devices/{}.crt", userDevice); + std::string adminFile = fmt::format("admins/{}.crt", uriMember); + std::string memberFile = fmt::format("members/{}.crt", uriMember); + std::string crlFile = fmt::format("CRLs/{}", uriMember); + std::string invitedFile = fmt::format("invited/{}", uriMember); + std::vector voters; + std::vector devicesRemoved; + std::vector bannedFiles; + // Check that no weird file is added nor removed + + const std::regex regex_votes("votes." + voteType + + ".(members|devices|admins|invited).(\\w+).(\\w+)"); + const std::regex regex_devices("devices.(\\w+)\\.crt"); + const std::regex regex_banned("banned.(members|devices|admins).(\\w+)\\.crt"); + const std::regex regex_banned_invited("banned.(invited).(\\w+)"); + std::smatch base_match; + for (const auto& f : changedFiles) { + if (f == deviceFile || f == adminFile || f == memberFile || f == crlFile + || f == invitedFile) { + // Ignore + continue; + } else if (std::regex_match(f, base_match, regex_votes)) { + if (base_match.size() != 4 or base_match[2] != uriMember) { + JAMI_ERR("Invalid vote file detected: %s", f.c_str()); + return false; + } + voters.emplace_back(base_match[3]); + // Check that votes were not added here + if (!fileAtTree(f, treeOld)) { + JAMI_ERR("invalid vote added (%s)", f.c_str()); + return false; + } + } else if (std::regex_match(f, base_match, regex_devices)) { + if (base_match.size() == 2) + devicesRemoved.emplace_back(base_match[1]); + } else if (std::regex_match(f, base_match, regex_banned) + || std::regex_match(f, base_match, regex_banned_invited)) { + bannedFiles.emplace_back(f); + if (base_match.size() != 3 or base_match[2] != uriMember) { + JAMI_ERR("Invalid banned file detected : %s", f.c_str()); + return false; + } + } else { + JAMI_ERR("Unwanted changed file detected: %s", f.c_str()); + return false; + } } - // If not for self check that user device is admin + // Check that removed devices are for removed member (or directly uriMember) + for (const auto& deviceUri : devicesRemoved) { + deviceFile = fmt::format("devices/{}.crt", deviceUri); + if (voteType == "ban") { + // If we ban a device, it should be there before + if (!fileAtTree(deviceFile, treeOld)) { + JAMI_ERR("device not found added (%s)", deviceFile.c_str()); + return false; + } + } else if (voteType == "unban") { + // If we unban a device, it should not be there before + if (fileAtTree(deviceFile, treeOld)) { + JAMI_ERR("device not found added (%s)", deviceFile.c_str()); + return false; + } + } + if (uriMember != uriFromDevice(deviceUri) + and uriMember != deviceUri /* If device is removed */) { + JAMI_ERR("device removed but not for removed user (%s)", deviceFile.c_str()); + return false; + } + } + + // Check that voters are admins adminFile = fmt::format("admins/{}.crt", userUri); if (!fileAtTree(adminFile, treeOld)) { JAMI_ERR("admin file (%s) not found", adminFile.c_str()); @@ -2344,9 +2421,13 @@ ConversationRepository::Impl::validCommits( } return false; } - } else if (action == "ban") { + } else if (action == "ban" || action == "unban") { // Note device.size() == "member".size() - if (!checkValidRemove(userDevice, uriMember, commit.id, commit.parents[0])) { + if (!checkValidVoteResolution(userDevice, + uriMember, + commit.id, + commit.parents[0], + action)) { JAMI_WARN( "Malformed removes commit %s. Please check you use the latest version " "of Jami, or that your contact is not doing unwanted stuff.", @@ -3012,10 +3093,8 @@ ConversationRepository::mode() const } std::string -ConversationRepository::voteKick(const std::string& uri, bool isDevice) +ConversationRepository::voteKick(const std::string& uri, const std::string& type) { - // Add vote + commit - // TODO simplify auto repo = pimpl_->repository(); auto account = pimpl_->account_.lock(); if (!account || !repo) @@ -3030,9 +3109,13 @@ ConversationRepository::voteKick(const std::string& uri, bool isDevice) return {}; } - // TODO avoid duplicate - auto relativeVotePath = std::string("votes") + DIR_SEPARATOR_STR - + (isDevice ? "devices" : "members") + DIR_SEPARATOR_STR + uri; + if (!fileutils::isFile( + fmt::format("{}/{}/{}{}", repoPath, type, uri, (type != "invited" ? ".crt" : "")))) { + JAMI_WARN("Didn't found file for %s with type %s", uri.c_str(), type.c_str()); + return {}; + } + + auto relativeVotePath = fmt::format("votes/ban/{}/{}", type, uri); auto voteDirectory = repoPath + DIR_SEPARATOR_STR + relativeVotePath; if (!fileutils::recursive_mkdir(voteDirectory, 0700)) { JAMI_ERR("Error when creating %s. Abort vote", voteDirectory.c_str()); @@ -3060,7 +3143,146 @@ ConversationRepository::voteKick(const std::string& uri, bool isDevice) } std::string -ConversationRepository::resolveVote(const std::string& uri, bool isDevice) +ConversationRepository::voteUnban(const std::string& uri, const std::string& type) +{ + auto repo = pimpl_->repository(); + auto account = pimpl_->account_.lock(); + if (!account || !repo) + return {}; + std::string repoPath = git_repository_workdir(repo.get()); + auto cert = account->identity().second; + if (!cert || !cert->issuer) + return {}; + auto adminUri = cert->issuer->getId().toString(); + + auto relativeVotePath = fmt::format("votes/unban/{}/{}", type, uri); + auto voteDirectory = repoPath + DIR_SEPARATOR_STR + relativeVotePath; + if (!fileutils::recursive_mkdir(voteDirectory, 0700)) { + JAMI_ERR("Error when creating %s. Abort vote", voteDirectory.c_str()); + return {}; + } + auto votePath = fileutils::getFullPath(voteDirectory, adminUri); + auto voteFile = fileutils::ofstream(votePath, std::ios::trunc | std::ios::binary); + if (!voteFile.is_open()) { + JAMI_ERR("Could not write data to %s", votePath.c_str()); + return {}; + } + voteFile.close(); + + auto toAdd = fileutils::getFullPath(relativeVotePath, adminUri); + if (!pimpl_->add(toAdd.c_str())) + return {}; + + Json::Value json; + json["uri"] = uri; + json["type"] = "vote"; + Json::StreamWriterBuilder wbuilder; + wbuilder["commentStyle"] = "None"; + wbuilder["indentation"] = ""; + return commitMessage(Json::writeString(wbuilder, json)); +} + +bool +ConversationRepository::Impl::resolveBan(const std::string& type, const std::string& uri) +{ + auto repo = repository(); + std::string repoPath = git_repository_workdir(repo.get()); + std::string bannedPath = repoPath + "banned"; + std::string devicesPath = repoPath + "devices"; + // Move from device or members file into banned + auto crtStr = (type != "invited" ? ".crt" : ""); + std::string originFilePath = fmt::format("{}/{}/{}{}", repoPath, type, uri, crtStr); + + auto destPath = bannedPath + DIR_SEPARATOR_STR + type; + auto destFilePath = fmt::format("{}/{}{}", destPath, uri, crtStr); + if (!fileutils::recursive_mkdir(destPath, 0700)) { + JAMI_ERR("Error when creating %s. Abort resolving vote", destPath.c_str()); + return false; + } + + if (std::rename(originFilePath.c_str(), destFilePath.c_str())) { + JAMI_ERR("Error when moving %s to %s. Abort resolving vote", + originFilePath.c_str(), + destFilePath.c_str()); + return false; + } + + // If members, remove related devices and mark as banned + if (type != "devices") { + for (const auto& certificate : fileutils::readDirectory(devicesPath)) { + auto certPath = fileutils::getFullPath(devicesPath, certificate); + auto deviceCert = fileutils::loadTextFile(certPath); + try { + crypto::Certificate cert(deviceCert); + if (auto issuer = cert.issuer) + if (issuer->toString() == uri) + fileutils::remove(certPath, true); + } catch (...) { + continue; + } + } + std::lock_guard lk(membersMtx_); + auto updated = false; + + for (auto& member : members_) { + if (member.uri == uri) { + updated = true; + member.role = MemberRole::BANNED; + break; + } + } + if (!updated) + members_.emplace_back(ConversationMember {uri, MemberRole::BANNED}); + } + return true; +} + +bool +ConversationRepository::Impl::resolveUnban(const std::string& type, const std::string& uri) +{ + auto repo = repository(); + std::string repoPath = git_repository_workdir(repo.get()); + std::string bannedPath = repoPath + "banned"; + auto crtStr = (type != "invited" ? ".crt" : ""); + auto originFilePath = fmt::format("{}/{}/{}{}", bannedPath, type, uri, crtStr); + auto destPath = repoPath + DIR_SEPARATOR_STR + type; + auto destFilePath = fmt::format("{}/{}{}", destPath, uri, crtStr); + if (!fileutils::recursive_mkdir(destPath, 0700)) { + JAMI_ERR("Error when creating %s. Abort resolving vote", destPath.c_str()); + return false; + } + if (std::rename(originFilePath.c_str(), destFilePath.c_str())) { + JAMI_ERR("Error when moving %s to %s. Abort resolving vote", + originFilePath.c_str(), + destFilePath.c_str()); + return false; + } + + std::lock_guard lk(membersMtx_); + auto updated = false; + + auto role = MemberRole::MEMBER; + if (type == "invited") + role = MemberRole::INVITED; + else if (type == "admins") + role = MemberRole::ADMIN; + + for (auto& member : members_) { + if (member.uri == uri) { + updated = true; + member.role = role; + break; + } + } + if (!updated) + members_.emplace_back(ConversationMember {uri, role}); + return true; +} + +std::string +ConversationRepository::resolveVote(const std::string& uri, + const std::string& type, + const std::string& voteType) { // Count ratio admin/votes auto nbAdmins = 0, nbVotes = 0; @@ -3070,17 +3292,9 @@ ConversationRepository::resolveVote(const std::string& uri, bool isDevice) return {}; std::string repoPath = git_repository_workdir(repo.get()); std::string adminsPath = repoPath + "admins"; - std::string invitedPath = repoPath + "invited"; - std::string membersPath = repoPath + "members"; - std::string devicesPath = repoPath + "devices"; - std::string bannedPath = repoPath + "banned"; - auto isAdmin = fileutils::isFile(fileutils::getFullPath(adminsPath, uri + ".crt")); - auto isInvited = fileutils::isFile(fileutils::getFullPath(invitedPath, uri)); + auto votePath = fmt::format("{}/{}", voteType, type); - auto voteDirectory = fmt::format("{}/votes/{}/{}", - repoPath, - (isDevice ? "devices" : "members"), - uri); + auto voteDirectory = fmt::format("{}/votes/{}/{}", repoPath, votePath, uri); for (const auto& certificate : fileutils::readDirectory(adminsPath)) { if (certificate.find(".crt") == std::string::npos) { JAMI_WARN("Incorrect file found: %s/%s", adminsPath.c_str(), certificate.c_str()); @@ -3098,43 +3312,12 @@ ConversationRepository::resolveVote(const std::string& uri, bool isDevice) // Remove vote directory fileutils::removeAll(voteDirectory, true); - // Move from device or members file into banned - std::string originFilePath = fmt::format("{}/{}.crt", membersPath, uri); - if (isDevice) - originFilePath = fmt::format("{}/{}.crt", devicesPath, uri); - else if (isAdmin) - originFilePath = fmt::format("{}/{}.crt", adminsPath, uri); - else if (isInvited) - originFilePath = fmt::format("{}/{}", invitedPath, uri); - - auto destPath = bannedPath + DIR_SEPARATOR_STR + (isDevice ? "devices" : "members"); - auto destFilePath = fmt::format("{}/{}.crt", destPath, uri); - if (!fileutils::recursive_mkdir(destPath, 0700)) { - JAMI_ERR("Error when creating %s. Abort resolving vote", destPath.c_str()); - return {}; - } - - if (std::rename(originFilePath.c_str(), destFilePath.c_str())) { - JAMI_ERR("Error when moving %s to %s. Abort resolving vote", - originFilePath.c_str(), - destFilePath.c_str()); - return {}; - } - - // If members, remove related devices - if (!isDevice) { - for (const auto& certificate : fileutils::readDirectory(devicesPath)) { - auto certPath = fileutils::getFullPath(devicesPath, certificate); - auto deviceCert = fileutils::loadTextFile(certPath); - try { - crypto::Certificate cert(deviceCert); - if (auto issuer = cert.issuer) - if (issuer->toString() == uri) - fileutils::remove(certPath, true); - } catch (...) { - continue; - } - } + if (voteType == "ban") { + if (!pimpl_->resolveBan(type, uri)) + return {}; + } else if (voteType == "unban") { + if (!pimpl_->resolveUnban(type, uri)) + return {}; } // Commit @@ -3142,27 +3325,12 @@ ConversationRepository::resolveVote(const std::string& uri, bool isDevice) return {}; Json::Value json; - json["action"] = "ban"; + json["action"] = voteType; json["uri"] = uri; json["type"] = "member"; Json::StreamWriterBuilder wbuilder; wbuilder["commentStyle"] = "None"; wbuilder["indentation"] = ""; - - if (!isDevice) { - std::lock_guard lk(pimpl_->membersMtx_); - auto updated = false; - - for (auto& member : pimpl_->members_) { - if (member.uri == uri) { - updated = true; - member.role = MemberRole::BANNED; - break; - } - } - if (!updated) - pimpl_->members_.emplace_back(ConversationMember {uri, MemberRole::BANNED}); - } return commitMessage(Json::writeString(wbuilder, json)); } diff --git a/src/jamidht/conversationrepository.h b/src/jamidht/conversationrepository.h index 141511d56..358f28d67 100644 --- a/src/jamidht/conversationrepository.h +++ b/src/jamidht/conversationrepository.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2019 Savoir-faire Linux Inc. + * Copyright (C) 2019-2022 Savoir-faire Linux Inc. * Author: Sébastien Blin * * This program is free software; you can redistribute it and/or modify @@ -258,8 +258,36 @@ public: */ ConversationMode mode() const; - std::string voteKick(const std::string& uri, bool isDevice); - std::string resolveVote(const std::string& uri, bool isDevice); + /** + * The voting system is divided in two parts. The voting phase where + * admins can decide an action (such as kicking someone) + * and the resolving phase, when > 50% of the admins voted, we can + * considered the vote as finished + */ + /** + * Add a vote to kick a device or a user + * @param uri identified of the user/device + * @param type device, members, admins or invited + * @return the commit id or empty if failed + */ + std::string voteKick(const std::string& uri, const std::string& type); + /** + * Add a vote to re-add a user + * @param uri identified of the user + * @param type device, members, admins or invited + * @return the commit id or empty if failed + */ + std::string voteUnban(const std::string& uri, const std::string& type); + /** + * Validate if a vote is finished + * @param uri identified of the user/device + * @param type device, members, admins or invited + * @param voteType "ban" or "unban" + * @return the commit id or empty if failed + */ + std::string resolveVote(const std::string& uri, + const std::string& type, + const std::string& voteType); /** * Validate a fetch with remote device diff --git a/test/unitTest/conversation/conversationMembersEvent.cpp b/test/unitTest/conversation/conversationMembersEvent.cpp index 0b1d6a58e..bedfd0dfe 100644 --- a/test/unitTest/conversation/conversationMembersEvent.cpp +++ b/test/unitTest/conversation/conversationMembersEvent.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2017-2019 Savoir-faire Linux Inc. + * Copyright (C) 2017-2022 Savoir-faire Linux Inc. * Author: Sébastien Blin * * This program is free software; you can redistribute it and/or modify @@ -67,8 +67,9 @@ public: void testMemberBanNoBadFile(); void testMemberTryToRemoveAdmin(); void testBannedMemberCannotSendMessage(); - void testAddBannedMember(); + void testAdminCanReAddMember(); void testMemberCannotBanOther(); + void testMemberCannotUnBanOther(); void testCheckAdminFakeAVoteIsDetected(); void testAdminCannotKickTheirself(); void testCommitUnauthorizedUser(); @@ -100,8 +101,9 @@ private: CPPUNIT_TEST(testMemberBanNoBadFile); CPPUNIT_TEST(testMemberTryToRemoveAdmin); CPPUNIT_TEST(testBannedMemberCannotSendMessage); - CPPUNIT_TEST(testAddBannedMember); + CPPUNIT_TEST(testAdminCanReAddMember); CPPUNIT_TEST(testMemberCannotBanOther); + CPPUNIT_TEST(testMemberCannotUnBanOther); CPPUNIT_TEST(testCheckAdminFakeAVoteIsDetected); CPPUNIT_TEST(testAdminCannotKickTheirself); CPPUNIT_TEST(testCommitUnauthorizedUser); @@ -968,7 +970,7 @@ ConversationMembersEventTest::testBannedMemberCannotSendMessage() } void -ConversationMembersEventTest::testAddBannedMember() +ConversationMembersEventTest::testAdminCanReAddMember() { auto aliceAccount = Manager::instance().getAccount(aliceId); auto bobAccount = Manager::instance().getAccount(bobId); @@ -1024,10 +1026,17 @@ ConversationMembersEventTest::testAddBannedMember() CPPUNIT_ASSERT( cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated && voteMessageGenerated; })); - // Then check that bobUri cannot be re-added - memberMessageGenerated = false; + auto members = DRing::getConversationMembers(aliceId, convId); + CPPUNIT_ASSERT(members.size() == 1); + + // Then check that bobUri can be re-added + memberMessageGenerated = false, voteMessageGenerated = false; DRing::addConversationMember(aliceId, convId, bobUri); - CPPUNIT_ASSERT(!cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated; })); + CPPUNIT_ASSERT( + cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated && voteMessageGenerated; })); + + members = DRing::getConversationMembers(aliceId, convId); + CPPUNIT_ASSERT(members.size() == 2); DRing::unregisterSignalHandlers(); } @@ -1127,6 +1136,112 @@ ConversationMembersEventTest::testMemberCannotBanOther() CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return messageBobReceived; })); } +void +ConversationMembersEventTest::testMemberCannotUnBanOther() +{ + auto aliceAccount = Manager::instance().getAccount(aliceId); + auto bobAccount = Manager::instance().getAccount(bobId); + auto aliceUri = aliceAccount->getUsername(); + auto bobUri = bobAccount->getUsername(); + auto convId = DRing::startConversation(aliceId); + auto carlaAccount = Manager::instance().getAccount(carlaId); + auto carlaUri = carlaAccount->getUsername(); + aliceAccount->trackBuddyPresence(carlaUri, true); + + std::mutex mtx; + std::unique_lock lk {mtx}; + std::condition_variable cv; + std::map> confHandlers; + bool conversationReady = false, requestReceived = false, memberMessageGenerated = false, + voteMessageGenerated = false, messageBobReceived = false, errorDetected = false, + carlaConnected = false, messageCarlaReceived = false; + confHandlers.insert( + DRing::exportable_callback( + [&](const std::string&, const std::string&, std::map) { + requestReceived = true; + cv.notify_one(); + })); + confHandlers.insert( + DRing::exportable_callback( + [&](const std::string&, const std::map&) { + auto details = carlaAccount->getVolatileAccountDetails(); + auto deviceAnnounced = details[DRing::Account::VolatileProperties::DEVICE_ANNOUNCED]; + if (deviceAnnounced == "true") { + carlaConnected = true; + cv.notify_one(); + } + })); + confHandlers.insert(DRing::exportable_callback( + [&](const std::string& accountId, const std::string& conversationId) { + if (accountId == bobId && conversationId == convId) { + conversationReady = true; + cv.notify_one(); + } + })); + confHandlers.insert(DRing::exportable_callback( + [&](const std::string& accountId, + const std::string& conversationId, + std::map message) { + if (accountId == aliceId && conversationId == convId && message["type"] == "vote") { + voteMessageGenerated = true; + } else if (accountId == aliceId && conversationId == convId + && message["type"] == "member") { + memberMessageGenerated = true; + } else if (accountId == bobId && conversationId == convId) { + messageBobReceived = true; + } else if (accountId == carlaId && conversationId == convId) { + messageCarlaReceived = true; + } + cv.notify_one(); + })); + confHandlers.insert(DRing::exportable_callback( + [&](const std::string& /* accountId */, + const std::string& /* conversationId */, + int code, + const std::string& /* what */) { + if (code == 3) + errorDetected = true; + cv.notify_one(); + })); + DRing::registerSignalHandlers(confHandlers); + Manager::instance().sendRegister(carlaId, true); + CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return carlaConnected; })); + DRing::addConversationMember(aliceId, convId, bobUri); + CPPUNIT_ASSERT( + cv.wait_for(lk, 30s, [&]() { return requestReceived && memberMessageGenerated; })); + memberMessageGenerated = false; + DRing::acceptConversationRequest(bobId, convId); + CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated; })); + requestReceived = false; + memberMessageGenerated = false; + DRing::addConversationMember(aliceId, convId, carlaUri); + CPPUNIT_ASSERT( + cv.wait_for(lk, 30s, [&]() { return requestReceived && memberMessageGenerated; })); + memberMessageGenerated = false; + messageBobReceived = false; + DRing::acceptConversationRequest(carlaId, convId); + CPPUNIT_ASSERT( + cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated && messageBobReceived; })); + + // Now check that alice, has the only admin, can remove bob + memberMessageGenerated = false; + messageCarlaReceived = false; + voteMessageGenerated = false; + DRing::removeConversationMember(aliceId, convId, bobUri); + CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { + return memberMessageGenerated && voteMessageGenerated && messageCarlaReceived; + })); + + memberMessageGenerated = false; + voteMessageGenerated = false; + DRing::addConversationMember(carlaId, convId, bobUri); + CPPUNIT_ASSERT(!cv.wait_for(lk, 30s, [&]() { + return memberMessageGenerated && voteMessageGenerated; + })); + auto members = DRing::getConversationMembers(aliceId, convId); + CPPUNIT_ASSERT(members.size() == 2); +} + void ConversationMembersEventTest::testCheckAdminFakeAVoteIsDetected() {