From c0ea6f65f934858944a9eae5b584dc8c5ae9471a Mon Sep 17 00:00:00 2001 From: Tamino Bauknecht <28907748+taminob@users.noreply.github.com> Date: Sun, 14 Sep 2025 18:02:22 +0200 Subject: [PATCH] Database merge confirmation dialog (#10173) * Add Entry::calculateDifference() This new function contains the logic that was previously in EntryHistoryModel::calculateHistoryModifications(). It allows the re-use to display the differences in case of a merge. * Introduce Database Merge Confirmation Dialog Adds a dialog allowing a user to review the changes of a merge operation. This dialog displays the changes and allows the user to abort the merge without modifying the database. Fixes #1152 * Added dry run option to Merger * Changed behavior when actual merge differs from dry run to just output a warning to console * Fixed KeeShare conflicting with merge operations in the middle of a merge --------- Co-authored-by: Jonathan White --- share/translations/keepassxc_en.ts | 303 +++++++++++++++--------- src/CMakeLists.txt | 1 + src/cli/Merge.cpp | 6 +- src/core/Entry.cpp | 62 +++++ src/core/Entry.h | 7 + src/core/Group.cpp | 1 + src/core/Merger.cpp | 353 ++++++++++++++++++++++------ src/core/Merger.h | 49 +++- src/gui/DatabaseWidget.cpp | 35 ++- src/gui/MergeDialog.cpp | 199 ++++++++++++++++ src/gui/MergeDialog.h | 83 +++++++ src/gui/MergeDialog.ui | 31 +++ src/gui/entry/EntryHistoryModel.cpp | 59 +---- src/keeshare/KeeShare.cpp | 11 + src/keeshare/KeeShare.h | 1 + src/keeshare/ShareObserver.cpp | 7 +- src/keeshare/ShareObserver.h | 2 + tests/TestCli.cpp | 10 +- tests/TestMerge.cpp | 4 +- tests/gui/TestGui.cpp | 4 + 20 files changed, 952 insertions(+), 276 deletions(-) create mode 100644 src/gui/MergeDialog.cpp create mode 100644 src/gui/MergeDialog.h create mode 100644 src/gui/MergeDialog.ui diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 80c019ee2..2b97b6dc5 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -2670,14 +2670,6 @@ This is definitely a bug, please report it to the developers. No source database, nothing to do. - - Successfully merged the database files. - - - - Database was not modified by merge operation. - - Search Results (%1) @@ -2774,14 +2766,6 @@ Disable safe saves and try again? Remote sync '%1' completed successfully! - - Remote sync '%1' failed: %2 - - - - Error while saving database %1: %2 - - Downloading... @@ -2790,10 +2774,18 @@ Disable safe saves and try again? Uploading... + + Remote sync '%1' failed: %2 + + Syncing... + + Error while saving database %1: %2 + + Remove passkey from entry @@ -2810,10 +2802,6 @@ Disable safe saves and try again? Do you want to load the changes? - - Reload database - - Reloading database… @@ -2838,6 +2826,10 @@ Disable safe saves and try again? The database file "%1" was modified externally.<br>How would you like to proceed?<br><br>Merge all changes then save<br>Overwrite the changes on disk<br>Discard unsaved changes + + Reload database + + Database file overwritten. @@ -2850,14 +2842,6 @@ Disable safe saves and try again? Failed to save backup database: %1 - - Save - - - - Save Database Backup - - Confirm Delete Group @@ -2870,6 +2854,26 @@ Disable safe saves and try again? Confirm Recycle Group + + Save + + + + Save Database Backup + + + + Successfully merged the selected database. + + + + No changes were made by the merge operation. + + + + Merge canceled, no changes were made. + + EditEntryAttachmentsDialog @@ -3940,6 +3944,62 @@ This may cause the affected plugins to malfunction. %2 + + Title + + + + Username + + + + Password + + + + URL + + + + Notes + + + + Custom Attributes + + + + Icon + + + + Color + + + + Expiration + + + + TOTP + + + + Custom Data + + + + Attachments + + + + Auto-Type + + + + Tags + + EntryAttachments @@ -4144,62 +4204,6 @@ Would you like to overwrite the existing attachment? Size - - Title - - - - Username - - - - Password - - - - URL - - - - Notes - - - - Custom Attributes - - - - Icon - - - - Color - - - - Expiration - - - - TOTP - - - - Custom Data - - - - Attachments - - - - Auto-Type - - - - Tags - - EntryModel @@ -6429,44 +6433,43 @@ Expect some bugs and minor issues, this version is meant for testing purposes. + + MergeDialog + + Database Merge Confirmation + + + + Merge + + + + Group + + + + Title + + + + UUID + + + + Details + + + + Change + + + Merger - - Creating missing %1 [%2] - - - - Relocating %1 [%2] - - - - Overwriting %1 [%2] - - - - Synchronizing from newer source %1 [%2] - - - - Synchronizing from older source %1 [%2] - - - - Deleting child %1 [%2] - - - - Deleting orphan %1 [%2] - - Changed deleted objects - - Adding missing icon %1 - - Removed custom data %1 [%2] @@ -6475,6 +6478,74 @@ Expect some bugs and minor issues, this version is meant for testing purposes.Adding custom data %1 [%2] + + Added + + + + Modified + + + + Moved + + + + Deleted + + + + Previous location: %1 + + + + Number of entries in group: %1 + + + + Group name + + + + Notes + + + + Icon (UUID) + + + + Icon (Number) + + + + Expiry time + + + + Modification time + + + + %1 (Add local modifications to new entry) + + + + %1 (Add new modifications to existing entry) + + + + Explicit deletion + + + + Implicit deletion (e.g. removal of parent group) + + + + Adding new icon %1 + + NewDatabaseWizard diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 9ddc46cf9..11de7e4ba 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -134,6 +134,7 @@ set(gui_SOURCES gui/IconModels.cpp gui/KMessageWidget.cpp gui/MainWindow.cpp + gui/MergeDialog.cpp gui/MessageBox.cpp gui/MessageWidget.cpp gui/PasswordWidget.cpp diff --git a/src/cli/Merge.cpp b/src/cli/Merge.cpp index 6cf351c96..312edf8c3 100644 --- a/src/cli/Merge.cpp +++ b/src/cli/Merge.cpp @@ -87,10 +87,10 @@ int Merge::executeWithDatabase(QSharedPointer database, QSharedPointer } Merger merger(db2.data(), database.data()); - QStringList changeList = merger.merge(); + auto changeList = merger.merge(); - for (auto& mergeChange : changeList) { - out << "\t" << mergeChange << Qt::endl; + for (const auto& mergeChange : changeList) { + out << "\t" << mergeChange.toString() << Qt::endl; } if (!changeList.isEmpty() && !parser->isSet(Merge::DryRunOption)) { diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 1ef96aabe..844b3c463 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -959,6 +959,68 @@ bool Entry::equals(const Entry* other, CompareItemOptions options) const return true; } +QStringList Entry::calculateDifference(const Entry* other) +{ + QStringList modifiedFields; + + if (*attributes() != *other->attributes()) { + bool foundAttribute = false; + + if (title() != other->title()) { + modifiedFields << tr("Title"); + foundAttribute = true; + } + if (username() != other->username()) { + modifiedFields << tr("Username"); + foundAttribute = true; + } + if (password() != other->password()) { + modifiedFields << tr("Password"); + foundAttribute = true; + } + if (url() != other->url()) { + modifiedFields << tr("URL"); + foundAttribute = true; + } + if (notes() != other->notes()) { + modifiedFields << tr("Notes"); + foundAttribute = true; + } + + if (!foundAttribute) { + modifiedFields << tr("Custom Attributes"); + } + } + if (iconNumber() != other->iconNumber() || iconUuid() != other->iconUuid()) { + modifiedFields << tr("Icon"); + } + if (foregroundColor() != other->foregroundColor() || backgroundColor() != other->backgroundColor()) { + modifiedFields << tr("Color"); + } + if (timeInfo().expires() != other->timeInfo().expires() + || timeInfo().expiryTime() != other->timeInfo().expiryTime()) { + modifiedFields << tr("Expiration"); + } + if (totp() != other->totp()) { + modifiedFields << tr("TOTP"); + } + if (*customData() != *other->customData()) { + modifiedFields << tr("Custom Data"); + } + if (*attachments() != *other->attachments()) { + modifiedFields << tr("Attachments"); + } + if (*autoTypeAssociations() != *other->autoTypeAssociations() || autoTypeEnabled() != other->autoTypeEnabled() + || defaultAutoTypeSequence() != other->defaultAutoTypeSequence()) { + modifiedFields << tr("Auto-Type"); + } + if (tags() != other->tags()) { + modifiedFields << tr("Tags"); + } + + return modifiedFields; +} + Entry* Entry::clone(CloneFlags flags) const { auto entry = new Entry(); diff --git a/src/core/Entry.h b/src/core/Entry.h index ddf6b21a6..69c38ac46 100644 --- a/src/core/Entry.h +++ b/src/core/Entry.h @@ -179,6 +179,13 @@ public: bool equals(const Entry* other, CompareItemOptions options = CompareItemDefault) const; + /** + * Determine differences between attributes of this and another entry. + * + * @return The list of attribute names that are different between the two entries + */ + QStringList calculateDifference(const Entry* other); + enum CloneFlag { CloneNoFlags = 0, diff --git a/src/core/Group.cpp b/src/core/Group.cpp index b06ac7727..8b6cd75c5 100644 --- a/src/core/Group.cpp +++ b/src/core/Group.cpp @@ -457,6 +457,7 @@ const Group* Group::parentGroup() const void Group::setParent(Group* parent, int index, bool trackPrevious) { Q_ASSERT(parent); + Q_ASSERT(this != parent); Q_ASSERT(index >= -1 && index <= parent->children().size()); // setting a new parent for root groups is not allowed Q_ASSERT(!m_db || (m_db->rootGroup() != this)); diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp index 31da6e106..f4828f8e6 100644 --- a/src/core/Merger.cpp +++ b/src/core/Merger.cpp @@ -21,6 +21,107 @@ #include "core/Metadata.h" #include "core/Tools.h" +Merger::Change::Change(Type type, QString details) + : m_type{type} + , m_details{std::move(details)} +{ +} + +Merger::Change::Change(Type type, const Group& group, QString details) + : m_type{type} + , m_group{group.fullPath()} + , m_uuid{group.uuid()} + , m_details{std::move(details)} +{ +} +Merger::Change::Change(Type type, const Entry& entry, QString details) + : m_type{type} + , m_title{entry.title()} + , m_uuid{entry.uuid()} + , m_details{std::move(details)} +{ + if (const auto* group = entry.group()) { + m_group = group->fullPath(); + } +} +Merger::Change::Change(QString details) + : m_details{std::move(details)} +{ +} + +bool Merger::Change::operator==(const Merger::Change& other) const +{ + return m_type == other.m_type && m_group == other.m_group && m_title == other.m_title && m_uuid == other.m_uuid + && m_details == other.m_details; +} + +bool Merger::Change::operator!=(const Merger::Change& other) const +{ + return !(*this == other); +} + +Merger::Change::Type Merger::Change::type() const +{ + return m_type; +} +const QString& Merger::Change::title() const +{ + return m_title; +} +const QString& Merger::Change::group() const +{ + return m_group; +} +const QUuid& Merger::Change::uuid() const +{ + return m_uuid; +} +const QString& Merger::Change::details() const +{ + return m_details; +} + +QString Merger::Change::typeString() const +{ + switch (m_type) { + case Type::Added: + return tr("Added"); + case Type::Modified: + return tr("Modified"); + case Type::Moved: + return tr("Moved"); + case Type::Deleted: + return tr("Deleted"); + case Type::Metadata: + return "Metadata"; + case Type::Unspecified: + return ""; + default: + return "?"; + } +} + +QString Merger::Change::toString() const +{ + QString result; + if (m_type != Type::Unspecified) { + result += QString("%1: ").arg(typeString()); + } + if (!m_group.isEmpty()) { + result += QString("'%1'").arg(m_group); + } + if (!m_title.isEmpty()) { + result += QString("/'%1'").arg(m_title); + } + if (!m_uuid.isNull()) { + result += QString(" [%1]").arg(m_uuid.toString()); + } + if (!m_details.isEmpty()) { + result += QString(" (%1)").arg(m_details); + } + return result; +} + Merger::Merger(const Database* sourceDb, Database* targetDb) : m_mode(Group::Default) { @@ -64,8 +165,9 @@ void Merger::setSkipDatabaseCustomData(bool state) m_skipCustomData = state; } -QStringList Merger::merge() +Merger::ChangeList Merger::merge(bool dryRun) { + m_dryRun = dryRun; // Order of merge steps is important - it is possible that we // create some items before deleting them afterwards ChangeList changes; @@ -74,9 +176,10 @@ QStringList Merger::merge() changes << mergeMetadata(m_context); // At this point we have a list of changes we may want to show the user - if (!changes.isEmpty()) { + if (!changes.isEmpty() && !dryRun) { m_context.m_targetDb->markAsModified(); } + m_dryRun = false; return changes; } @@ -88,43 +191,59 @@ Merger::ChangeList Merger::mergeGroup(const MergeContext& context) for (Entry* sourceEntry : sourceEntries) { Entry* targetEntry = context.m_targetRootGroup->findEntryByUuid(sourceEntry->uuid()); if (!targetEntry) { - changes << tr("Creating missing %1 [%2]").arg(sourceEntry->title(), sourceEntry->uuidToHex()); // This entry does not exist at all. Create it. - targetEntry = sourceEntry->clone(Entry::CloneIncludeHistory); - moveEntry(targetEntry, context.m_targetGroup); + changes << Change(Change::Type::Added, *sourceEntry); + if (!m_dryRun) { + targetEntry = sourceEntry->clone(Entry::CloneIncludeHistory); + moveEntry(targetEntry, context.m_targetGroup); + } } else { // Entry is already present in the database. Update it. const bool locationChanged = targetEntry->timeInfo().locationChanged() < sourceEntry->timeInfo().locationChanged(); if (locationChanged && targetEntry->group() != context.m_targetGroup) { - changes << tr("Relocating %1 [%2]").arg(sourceEntry->title(), sourceEntry->uuidToHex()); - moveEntry(targetEntry, context.m_targetGroup); + changes << Change( + Change::Type::Moved, *sourceEntry, tr("Previous location: %1").arg(targetEntry->group()->name())); + if (!m_dryRun) { + moveEntry(targetEntry, context.m_targetGroup); + } } changes << resolveEntryConflict(context, sourceEntry, targetEntry); } } - // merge groups recursively + // merge child groups recursively const QList sourceChildGroups = context.m_sourceGroup->children(); for (Group* sourceChildGroup : sourceChildGroups) { + bool groupCreated = false; Group* targetChildGroup = context.m_targetRootGroup->findGroupByUuid(sourceChildGroup->uuid()); if (!targetChildGroup) { - changes << tr("Creating missing %1 [%2]").arg(sourceChildGroup->name(), sourceChildGroup->uuidToHex()); + changes << Change( + Change::Type::Added, + *sourceChildGroup, + tr("Number of entries in group: %1").arg(QString::number(sourceChildGroup->entries().size()))); + // Create the target group, it will be cleaned up later if in dry run mode targetChildGroup = sourceChildGroup->clone(Entry::CloneNoFlags, Group::CloneNoFlags); - moveGroup(targetChildGroup, context.m_targetGroup); - TimeInfo timeinfo = targetChildGroup->timeInfo(); - timeinfo.setLocationChanged(sourceChildGroup->timeInfo().locationChanged()); - targetChildGroup->setTimeInfo(timeinfo); - } else { - bool locationChanged = - targetChildGroup->timeInfo().locationChanged() < sourceChildGroup->timeInfo().locationChanged(); - if (locationChanged && targetChildGroup->parent() != context.m_targetGroup) { - changes << tr("Relocating %1 [%2]").arg(sourceChildGroup->name(), sourceChildGroup->uuidToHex()); + groupCreated = true; + if (!m_dryRun) { moveGroup(targetChildGroup, context.m_targetGroup); TimeInfo timeinfo = targetChildGroup->timeInfo(); timeinfo.setLocationChanged(sourceChildGroup->timeInfo().locationChanged()); targetChildGroup->setTimeInfo(timeinfo); } + } else { + bool locationChanged = + targetChildGroup->timeInfo().locationChanged() < sourceChildGroup->timeInfo().locationChanged(); + if (locationChanged && targetChildGroup->parent() != context.m_targetGroup) { + changes << Change( + Change::Type::Moved, *sourceChildGroup, tr("Previous location: %1").arg(targetChildGroup->name())); + if (!m_dryRun) { + moveGroup(targetChildGroup, context.m_targetGroup); + TimeInfo timeinfo = targetChildGroup->timeInfo(); + timeinfo.setLocationChanged(sourceChildGroup->timeInfo().locationChanged()); + targetChildGroup->setTimeInfo(timeinfo); + } + } changes << resolveGroupConflict(context, sourceChildGroup, targetChildGroup); } MergeContext subcontext{context.m_sourceDb, @@ -134,6 +253,10 @@ Merger::ChangeList Merger::mergeGroup(const MergeContext& context) sourceChildGroup, targetChildGroup}; changes << mergeGroup(subcontext); + // Cleanup the temporary target group structure + if (m_dryRun && groupCreated) { + delete subcontext.m_targetGroup; + } } return changes; } @@ -149,24 +272,68 @@ Merger::resolveGroupConflict(const MergeContext& context, const Group* sourceChi // only if the other group is newer, update the existing one. if (timeExisting < timeOther) { - changes << tr("Overwriting %1 [%2]").arg(sourceChildGroup->name(), sourceChildGroup->uuidToHex()); - targetChildGroup->setName(sourceChildGroup->name()); - targetChildGroup->setNotes(sourceChildGroup->notes()); + QStringList modifications; + auto updateIfNecessary = [&modifications, this](const auto& targetValue, + const auto& sourceValue, + auto&& updateFunction, + const QString& modification) { + if (targetValue != sourceValue) { + modifications << modification; + if (!m_dryRun) { + updateFunction(sourceValue); + } + return true; + } + return false; + }; + updateIfNecessary( + targetChildGroup->name(), + sourceChildGroup->name(), + [&](auto&& newValue) { targetChildGroup->setName(newValue); }, + tr("Group name")); + updateIfNecessary( + targetChildGroup->notes(), + sourceChildGroup->notes(), + [&](auto&& newValue) { targetChildGroup->setNotes(newValue); }, + tr("Notes")); if (sourceChildGroup->iconNumber() == 0) { - targetChildGroup->setIcon(sourceChildGroup->iconUuid()); + updateIfNecessary( + targetChildGroup->iconUuid(), + sourceChildGroup->iconUuid(), + [&](auto&& newValue) { targetChildGroup->setIcon(newValue); }, + tr("Icon (UUID)")); } else { - targetChildGroup->setIcon(sourceChildGroup->iconNumber()); + updateIfNecessary( + targetChildGroup->iconNumber(), + sourceChildGroup->iconNumber(), + [&](auto&& newValue) { targetChildGroup->setIcon(newValue); }, + tr("Icon (Number)")); } - targetChildGroup->setExpiryTime(sourceChildGroup->timeInfo().expiryTime()); - TimeInfo timeInfo = targetChildGroup->timeInfo(); - timeInfo.setLastModificationTime(timeOther); - targetChildGroup->setTimeInfo(timeInfo); + updateIfNecessary( + targetChildGroup->timeInfo().expiryTime(), + sourceChildGroup->timeInfo().expiryTime(), + [&](auto&& newValue) { targetChildGroup->setExpiryTime(newValue); }, + tr("Expiry time")); + updateIfNecessary( + timeExisting, + timeOther, + [&](auto&& newValue) { + TimeInfo timeInfo = targetChildGroup->timeInfo(); + timeInfo.setLastModificationTime(newValue); + targetChildGroup->setTimeInfo(timeInfo); + }, + tr("Modification time")); + changes << Change(Change::Type::Modified, *sourceChildGroup, modifications.join(", ")); } return changes; } void Merger::moveEntry(Entry* entry, Group* targetGroup) { + if (m_dryRun) { + return; + } + Q_ASSERT(entry); Group* sourceGroup = entry->group(); if (sourceGroup == targetGroup) { @@ -196,6 +363,10 @@ void Merger::moveEntry(Entry* entry, Group* targetGroup) void Merger::moveGroup(Group* group, Group* targetGroup) { + if (m_dryRun) { + return; + } + Q_ASSERT(group); Group* sourceGroup = group->parentGroup(); if (sourceGroup == targetGroup) { @@ -225,6 +396,10 @@ void Merger::moveGroup(Group* group, Group* targetGroup) void Merger::eraseEntry(Entry* entry) { + if (m_dryRun) { + return; + } + Database* database = entry->database(); // most simple method to remove an item from DeletedObjects :( const QList deletions = database->deletedObjects(); @@ -242,6 +417,10 @@ void Merger::eraseEntry(Entry* entry) void Merger::eraseGroup(Group* group) { + if (m_dryRun) { + return; + } + Database* database = group->database(); // most simple method to remove an item from DeletedObjects :( const QList deletions = database->deletedObjects(); @@ -268,6 +447,8 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex const int comparison = compare(targetEntry->timeInfo().lastModificationTime(), sourceEntry->timeInfo().lastModificationTime(), CompareItemIgnoreMilliseconds); + auto differences = targetEntry->calculateDifference(sourceEntry); + differences += "History"; const int maxItems = targetEntry->database()->metadata()->historyMaxItems(); if (comparison < 0) { Group* currentGroup = targetEntry->group(); @@ -276,7 +457,9 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex qPrintable(targetEntry->title()), qPrintable(sourceEntry->title()), qPrintable(currentGroup->name())); - changes << tr("Synchronizing from newer source %1 [%2]").arg(targetEntry->title(), targetEntry->uuidToHex()); + changes << Change(Change::Type::Modified, + *targetEntry, + tr("%1 (Add local modifications to new entry)").arg(differences.join(", "))); mergeHistory(targetEntry, clonedEntry, mergeMethod, maxItems); eraseEntry(targetEntry); moveEntry(clonedEntry, currentGroup); @@ -287,8 +470,9 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex qPrintable(targetEntry->group()->name())); const bool changed = mergeHistory(sourceEntry, targetEntry, mergeMethod, maxItems); if (changed) { - changes - << tr("Synchronizing from older source %1 [%2]").arg(targetEntry->title(), targetEntry->uuidToHex()); + changes << Change(Change::Type::Modified, + *targetEntry, + tr("%1 (Add new modifications to existing entry)").arg(differences.join(", "))); } } return changes; @@ -300,8 +484,10 @@ Merger::resolveEntryConflict(const MergeContext& context, const Entry* sourceEnt // We need to cut off the milliseconds since the persistent format only supports times down to seconds // so when we import data from a remote source, it may represent the (or even some msec newer) data // which may be discarded due to higher runtime precision - - Group::MergeMode mergeMode = m_mode == Group::Default ? context.m_targetGroup->mergeMode() : m_mode; + Group::MergeMode mergeMode = m_mode; + if (mergeMode == Group::Default && context.m_targetGroup) { + mergeMode = context.m_targetGroup->mergeMode(); + } return resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode); } @@ -324,11 +510,11 @@ bool Merger::mergeHistory(const Entry* sourceEntry, const QDateTime modificationTime = Clock::serialized(historyItem->timeInfo().lastModificationTime()); if (merged.contains(modificationTime) && !merged[modificationTime]->equals(historyItem, CompareItemIgnoreMilliseconds)) { - ::qWarning("Inconsistent history entry of %s[%s] at %s contains conflicting changes - conflict resolution " - "may lose data!", - qPrintable(sourceEntry->title()), - qPrintable(sourceEntry->uuidToHex()), - qPrintable(modificationTime.toString("yyyy-MM-dd HH-mm-ss-zzz"))); + qWarning("Inconsistent history entry of %s[%s] at %s contains conflicting changes - conflict resolution " + "may lose data!", + qPrintable(sourceEntry->title()), + qPrintable(sourceEntry->uuidToHex()), + qPrintable(modificationTime.toString("yyyy-MM-dd HH-mm-ss-zzz"))); } merged[modificationTime] = historyItem->clone(Entry::CloneNoFlags); } @@ -337,11 +523,10 @@ bool Merger::mergeHistory(const Entry* sourceEntry, const QDateTime modificationTime = Clock::serialized(historyItem->timeInfo().lastModificationTime()); if (merged.contains(modificationTime) && !merged[modificationTime]->equals(historyItem, CompareItemIgnoreMilliseconds)) { - ::qWarning( - "History entry of %s[%s] at %s contains conflicting changes - conflict resolution may lose data!", - qPrintable(sourceEntry->title()), - qPrintable(sourceEntry->uuidToHex()), - qPrintable(modificationTime.toString("yyyy-MM-dd HH-mm-ss-zzz"))); + qWarning("History entry of %s[%s] at %s contains conflicting changes - conflict resolution may lose data!", + qPrintable(sourceEntry->title()), + qPrintable(sourceEntry->uuidToHex()), + qPrintable(modificationTime.toString("yyyy-MM-dd HH-mm-ss-zzz"))); } if (preferRemote && merged.contains(modificationTime)) { // forcefully apply the remote history item @@ -357,9 +542,9 @@ bool Merger::mergeHistory(const Entry* sourceEntry, if (targetModificationTime == sourceModificationTime && !targetEntry->equals(sourceEntry, CompareItemIgnoreMilliseconds | CompareItemIgnoreHistory | CompareItemIgnoreLocation)) { - ::qWarning("Entry of %s[%s] contains conflicting changes - conflict resolution may lose data!", - qPrintable(sourceEntry->title()), - qPrintable(sourceEntry->uuidToHex())); + qWarning("Entry of %s[%s] contains conflicting changes - conflict resolution may lose data!", + qPrintable(sourceEntry->title()), + qPrintable(sourceEntry->uuidToHex())); } if (targetModificationTime < sourceModificationTime) { @@ -398,22 +583,24 @@ bool Merger::mergeHistory(const Entry* sourceEntry, qDeleteAll(updatedHistoryItems); return false; } - // We need to prevent any modification to the database since every change should be tracked either - // in a clone history item or in the Entry itself - const TimeInfo timeInfo = targetEntry->timeInfo(); - const bool blockedSignals = targetEntry->blockSignals(true); - bool updateTimeInfo = targetEntry->canUpdateTimeinfo(); - targetEntry->setUpdateTimeinfo(false); - targetEntry->removeHistoryItems(targetHistoryItems); - for (Entry* historyItem : merged) { - Q_ASSERT(!historyItem->parent()); - targetEntry->addHistoryItem(historyItem); + if (!m_dryRun) { + // We need to prevent any modification to the database since every change should be tracked either + // in a clone history item or in the Entry itself + const TimeInfo timeInfo = targetEntry->timeInfo(); + const bool blockedSignals = targetEntry->blockSignals(true); + bool updateTimeInfo = targetEntry->canUpdateTimeinfo(); + targetEntry->setUpdateTimeinfo(false); + targetEntry->removeHistoryItems(targetHistoryItems); + for (Entry* historyItem : merged) { + Q_ASSERT(!historyItem->parent()); + targetEntry->addHistoryItem(historyItem); + } + targetEntry->truncateHistory(); + targetEntry->blockSignals(blockedSignals); + targetEntry->setUpdateTimeinfo(updateTimeInfo); + Q_ASSERT(timeInfo == targetEntry->timeInfo()); + Q_UNUSED(timeInfo); } - targetEntry->truncateHistory(); - targetEntry->blockSignals(blockedSignals); - targetEntry->setUpdateTimeinfo(updateTimeInfo); - Q_ASSERT(timeInfo == targetEntry->timeInfo()); - Q_UNUSED(timeInfo); return true; } @@ -437,7 +624,6 @@ Merger::ChangeList Merger::mergeDeletions(const MergeContext& context) for (const auto& object : (targetDeletions + sourceDeletions)) { if (!mergedDeletions.contains(object.uuid)) { mergedDeletions[object.uuid] = object; - auto* entry = context.m_targetRootGroup->findEntryByUuid(object.uuid); if (entry) { entries << entry; @@ -465,17 +651,18 @@ Merger::ChangeList Merger::mergeDeletions(const MergeContext& context) } deletions << object; if (entry->group()) { - changes << tr("Deleting child %1 [%2]").arg(entry->title(), entry->uuidToHex()); + changes << Change(Change::Type::Deleted, *entry, tr("Explicit deletion")); } else { - changes << tr("Deleting orphan %1 [%2]").arg(entry->title(), entry->uuidToHex()); + changes << Change(Change::Type::Deleted, *entry, tr("Implicit deletion (e.g. removal of parent group)")); + } + if (!m_dryRun) { + eraseEntry(entry); } - // Entry is inserted into deletedObjects after deletions are processed - eraseEntry(entry); } while (!groups.isEmpty()) { auto* group = groups.takeFirst(); - if (Tools::asSet(group->children()).intersects(Tools::asSet(groups))) { + if (!(group->children().toSet() & groups.toSet()).isEmpty()) { // we need to finish all children before we are able to determine if the group can be removed groups << group; continue; @@ -491,17 +678,22 @@ Merger::ChangeList Merger::mergeDeletions(const MergeContext& context) } deletions << object; if (group->parentGroup()) { - changes << tr("Deleting child %1 [%2]").arg(group->name(), group->uuidToHex()); + changes << Change(Change::Type::Deleted, *group, tr("Explicit deletion")); } else { - changes << tr("Deleting orphan %1 [%2]").arg(group->name(), group->uuidToHex()); + changes << Change(Change::Type::Deleted, *group, tr("Implicit deletion (e.g. removal of parent group)")); + } + if (!m_dryRun) { + eraseGroup(group); } - eraseGroup(group); } // Put every deletion to the earliest date of deletion if (deletions != context.m_targetDb->deletedObjects()) { - changes << tr("Changed deleted objects"); + changes << Change(Change::Type::Metadata, tr("Changed deleted objects")); + if (!m_dryRun) { + context.m_targetDb->setDeletedObjects(deletions); + } } - context.m_targetDb->setDeletedObjects(deletions); + return changes; } @@ -516,8 +708,11 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) for (const auto& iconUuid : sourceMetadata->customIconsOrder()) { if (!targetMetadata->hasCustomIcon(iconUuid)) { - targetMetadata->addCustomIcon(iconUuid, sourceMetadata->customIcon(iconUuid)); - changes << tr("Adding missing icon %1").arg(QString::fromLatin1(iconUuid.toRfc4122().toHex())); + changes << Change(Change::Type::Metadata, + tr("Adding new icon %1").arg(QString::fromLatin1(iconUuid.toRfc4122().toHex()))); + if (!m_dryRun) { + targetMetadata->addCustomIcon(iconUuid, sourceMetadata->customIcon(iconUuid)); + } } } @@ -540,8 +735,10 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) // Do not remove protected custom data if (!sourceMetadata->customData()->contains(key) && !sourceMetadata->customData()->isProtected(key)) { auto value = targetMetadata->customData()->value(key); - targetMetadata->customData()->remove(key); - changes << tr("Removed custom data %1 [%2]").arg(key, value); + changes << Change(Change::Type::Metadata, tr("Removed custom data %1 [%2]").arg(key, value)); + if (!m_dryRun) { + targetMetadata->customData()->remove(key); + } } } @@ -556,8 +753,10 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) auto targetValue = targetMetadata->customData()->value(key); // Merge only if the values are not the same. if (sourceValue != targetValue) { - targetMetadata->customData()->set(key, sourceValue); - changes << tr("Adding custom data %1 [%2]").arg(key, sourceValue); + changes << Change(Change::Type::Metadata, tr("Adding custom data %1 [%2]").arg(key, sourceValue)); + if (!m_dryRun) { + targetMetadata->customData()->set(key, sourceValue); + } } } } diff --git a/src/core/Merger.h b/src/core/Merger.h index 9669e89e3..4eed4ba09 100644 --- a/src/core/Merger.h +++ b/src/core/Merger.h @@ -32,12 +32,51 @@ public: void setForcedMergeMode(Group::MergeMode mode); void resetForcedMergeMode(); void setSkipDatabaseCustomData(bool state); - QStringList merge(); + + class Change + { + public: + enum class Type + { + Unspecified, + Added, + Modified, + Moved, + Deleted, + Metadata, + }; + + Change(Type type, QString details); + Change(Type type, const Group& group, QString details = ""); + Change(Type type, const Entry& entry, QString details = ""); + explicit Change(QString details = ""); + + [[nodiscard]] Type type() const; + [[nodiscard]] QString typeString() const; + [[nodiscard]] const QString& title() const; + [[nodiscard]] const QString& group() const; + [[nodiscard]] const QUuid& uuid() const; + [[nodiscard]] const QString& details() const; + + [[nodiscard]] QString toString() const; + void merge(); + + bool operator==(const Change& other) const; + bool operator!=(const Change& other) const; + + private: + Type m_type{Type::Unspecified}; + QString m_title; + QString m_group; + QUuid m_uuid; + QString m_details; + }; + + using ChangeList = QList; + + ChangeList merge(bool dryRun = false); private: - typedef QString Change; - typedef QStringList ChangeList; - struct MergeContext { QPointer m_sourceDb; @@ -47,6 +86,7 @@ private: QPointer m_sourceGroup; QPointer m_targetGroup; }; + ChangeList mergeGroup(const MergeContext& context); ChangeList mergeDeletions(const MergeContext& context); ChangeList mergeMetadata(const MergeContext& context); @@ -68,6 +108,7 @@ private: MergeContext m_context; Group::MergeMode m_mode; bool m_skipCustomData = false; + bool m_dryRun = false; }; #endif // KEEPASSXC_MERGER_H diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 1f203c2b4..e5cc2a9e4 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -44,6 +44,7 @@ #include "gui/FileDialog.h" #include "gui/GuiTools.h" #include "gui/MainWindow.h" +#include "gui/MergeDialog.h" #include "gui/MessageBox.h" #include "gui/TotpDialog.h" #include "gui/TotpExportSettingsDialog.h" @@ -1387,18 +1388,30 @@ void DatabaseWidget::mergeDatabase(bool accepted) return; } - Merger merger(srcDb.data(), m_db.data()); - QStringList changeList = merger.merge(); +#ifdef WITH_XC_KEESHARE + // Disable KeeShare while merging to avoid conflicts with incoming changes + KeeShare::instance()->setSharingEnabled(m_db, false); +#endif - if (!changeList.isEmpty()) { - showMessage(tr("Successfully merged the database files."), MessageWidget::Information); - } else { - showMessage(tr("Database was not modified by merge operation."), MessageWidget::Information); - } + auto* mergeDialog = new MergeDialog(srcDb, m_db, this); + connect(mergeDialog, &MergeDialog::databaseMerged, [this](bool changed) { + if (changed) { + showMessage(tr("Successfully merged the selected database."), MessageWidget::Positive); + emit databaseMerged(m_db); + } else { + showMessage(tr("No changes were made by the merge operation."), MessageWidget::Information); + } + }); + connect(mergeDialog, &MergeDialog::finished, [this](int result) { + if (result == QDialog::Rejected) { + showMessage(tr("Merge canceled, no changes were made."), MessageWidget::Information); + } +#ifdef WITH_XC_KEESHARE + KeeShare::instance()->setSharingEnabled(m_db, true); +#endif + }); + mergeDialog->open(); } - - switchToMainView(); - emit databaseMerged(m_db); } void DatabaseWidget::syncUnlockedDatabase(bool accepted) @@ -1438,7 +1451,7 @@ bool DatabaseWidget::syncWithDatabase(const QSharedPointer& otherDb, Q emit updateSyncProgress(50, tr("Syncing...")); Merger firstMerge(m_db.data(), otherDb.data()); Merger secondMerge(otherDb.data(), m_db.data()); - QStringList changeList = firstMerge.merge() + secondMerge.merge(); + auto changeList = firstMerge.merge() + secondMerge.merge(); if (!changeList.isEmpty()) { // Save synced databases diff --git a/src/gui/MergeDialog.cpp b/src/gui/MergeDialog.cpp new file mode 100644 index 000000000..ed8ab3c16 --- /dev/null +++ b/src/gui/MergeDialog.cpp @@ -0,0 +1,199 @@ +/* + * Copyright (C) 2025 KeePassXC Team + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 or (at your option) + * version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "MergeDialog.h" +#include "ui_MergeDialog.h" + +#include "core/Database.h" + +#include +#include + +MergeDialog::MergeDialog(QSharedPointer source, QSharedPointer target, QWidget* parent) + : QDialog(parent) + , m_ui(new Ui::MergeDialog()) + , m_headerContextMenu(new QMenu()) + , m_sourceDatabase(std::move(source)) + , m_targetDatabase(std::move(target)) +{ + setAttribute(Qt::WA_DeleteOnClose); + // block input to other windows since other interactions can lead to unexpected merge results + setWindowModality(Qt::WindowModality::ApplicationModal); + + m_ui->setupUi(this); + + m_ui->buttonBox->button(QDialogButtonBox::Ok)->setText(tr("Merge")); + m_ui->buttonBox->button(QDialogButtonBox::Ok)->setFocus(); + + connect(m_ui->buttonBox, &QDialogButtonBox::rejected, this, &MergeDialog::cancelMerge); + connect(m_ui->buttonBox, &QDialogButtonBox::accepted, this, &MergeDialog::performMerge); + + setupChangeTable(); + updateChangeTable(); +} + +MergeDialog::MergeDialog(const Merger::ChangeList& changes, QWidget* parent) + : QDialog(parent) + , m_ui(new Ui::MergeDialog()) + , m_headerContextMenu(new QMenu()) + , m_changes(changes) +{ + setAttribute(Qt::WA_DeleteOnClose); + + m_ui->setupUi(this); + + m_ui->buttonBox->button(QDialogButtonBox::Ok)->setFocus(); + m_ui->buttonBox->button(QDialogButtonBox::Abort)->hide(); + + connect(m_ui->buttonBox, &QDialogButtonBox::accepted, this, &MergeDialog::close); + + setupChangeTable(); +} + +MergeDialog::~MergeDialog() = default; + +QVector MergeDialog::columns() +{ + return {MergeDialogColumns::Group, + MergeDialogColumns::Title, + MergeDialogColumns::Uuid, + MergeDialogColumns::Type, + MergeDialogColumns::Details}; +} + +int MergeDialog::columnIndex(MergeDialogColumns column) +{ + return columns().indexOf(column); +} + +QString MergeDialog::columnName(MergeDialogColumns column) +{ + switch (column) { + case MergeDialogColumns::Group: + return tr("Group"); + case MergeDialogColumns::Title: + return tr("Title"); + case MergeDialogColumns::Uuid: + return tr("UUID"); + case MergeDialogColumns::Type: + return tr("Change"); + case MergeDialogColumns::Details: + return tr("Details"); + } + return {}; +} + +QString MergeDialog::cellValue(const Merger::Change& change, MergeDialogColumns column) +{ + switch (column) { + case MergeDialogColumns::Group: + return change.group(); + case MergeDialogColumns::Title: + return change.title(); + case MergeDialogColumns::Uuid: + if (!change.uuid().isNull()) { + return change.uuid().toString(); + } + break; + case MergeDialogColumns::Type: + return change.typeString(); + case MergeDialogColumns::Details: + return change.details(); + } + return {}; +} + +bool MergeDialog::isColumnHiddenByDefault(MergeDialogColumns column) +{ + return column == MergeDialogColumns::Uuid; +} + +void MergeDialog::setupChangeTable() +{ + Q_ASSERT(m_ui); + Q_ASSERT(m_ui->changeTable); + + m_ui->changeTable->verticalHeader()->setVisible(false); + m_ui->changeTable->horizontalHeader()->setSectionResizeMode(QHeaderView::ResizeMode::Interactive); + m_ui->changeTable->horizontalHeader()->setContextMenuPolicy(Qt::ActionsContextMenu); + + m_ui->changeTable->setShowGrid(false); + m_ui->changeTable->setEditTriggers(QAbstractItemView::NoEditTriggers); + m_ui->changeTable->setSelectionBehavior(QAbstractItemView::SelectRows); + m_ui->changeTable->setSelectionMode(QAbstractItemView::SingleSelection); + + // Create the header context menu + for (auto column : columns()) { + auto* action = new QAction(columnName(column), this); + action->setCheckable(true); + action->setChecked(!isColumnHiddenByDefault(column)); + connect(action, &QAction::toggled, [this, column](bool checked) { + m_ui->changeTable->setColumnHidden(columnIndex(column), !checked); + m_ui->changeTable->horizontalHeader()->resizeSections(QHeaderView::ResizeMode::ResizeToContents); + }); + m_ui->changeTable->horizontalHeader()->addAction(action); + } +} + +void MergeDialog::updateChangeTable() +{ + Q_ASSERT(m_ui); + Q_ASSERT(m_ui->changeTable); + Q_ASSERT(m_sourceDatabase.get()); + Q_ASSERT(m_targetDatabase.get()); + + m_changes = Merger(m_sourceDatabase.data(), m_targetDatabase.get()).merge(true); + + m_ui->changeTable->clear(); + + auto allColumns = columns(); + m_ui->changeTable->setColumnCount(allColumns.size()); + m_ui->changeTable->setRowCount(m_changes.size()); + for (auto column : allColumns) { + auto name = columnName(column); + auto index = columnIndex(column); + + m_ui->changeTable->setHorizontalHeaderItem(index, new QTableWidgetItem(name)); + m_ui->changeTable->setColumnHidden(index, isColumnHiddenByDefault(column)); + } + for (int row = 0; row < m_changes.size(); ++row) { + const auto& change = m_changes[row]; + for (auto column : allColumns) { + m_ui->changeTable->setItem(row, columnIndex(column), new QTableWidgetItem(cellValue(change, column))); + } + } + + m_ui->changeTable->horizontalHeader()->resizeSections(QHeaderView::ResizeMode::ResizeToContents); +} + +void MergeDialog::performMerge() +{ + auto changes = Merger(m_sourceDatabase.data(), m_targetDatabase.data()).merge(); + if (changes != m_changes) { + qWarning("Merge results differed from the expected changes. Expected: %d, Actual: %d", + m_changes.size(), + changes.size()); + } + + emit databaseMerged(!changes.isEmpty()); + done(QDialog::Accepted); +} + +void MergeDialog::cancelMerge() +{ + done(QDialog::Rejected); +} diff --git a/src/gui/MergeDialog.h b/src/gui/MergeDialog.h new file mode 100644 index 000000000..45d880dc3 --- /dev/null +++ b/src/gui/MergeDialog.h @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2025 KeePassXC Team + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 or (at your option) + * version 3 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifndef KEEPASSX_MERGEDIALOG_H +#define KEEPASSX_MERGEDIALOG_H + +#include "core/Merger.h" + +#include +#include + +namespace Ui +{ + class MergeDialog; +} + +class Database; + +class MergeDialog : public QDialog +{ + Q_OBJECT + +public: + /** + * Merge source into copy of target and display changes. + * On user confirmation, merge source into target. + */ + explicit MergeDialog(QSharedPointer source, QSharedPointer target, QWidget* parent = nullptr); + /** + * Display given changes. + */ + explicit MergeDialog(const Merger::ChangeList& changes, QWidget* parent = nullptr); + ~MergeDialog() override; + +signals: + // Signal will be emitted when a normal merge operation has been performed. + void databaseMerged(bool databaseChanged); + +private slots: + void performMerge(); + void cancelMerge(); + +private: + enum class MergeDialogColumns + { + Group, + Title, + Uuid, + Type, + Details + }; + static QVector columns(); + static int columnIndex(MergeDialogColumns column); + static QString columnName(MergeDialogColumns column); + static QString cellValue(const Merger::Change& change, MergeDialogColumns column); + static bool isColumnHiddenByDefault(MergeDialogColumns column); + + void setupChangeTable(); + void updateChangeTable(); + + QScopedPointer m_ui; + QScopedPointer m_headerContextMenu; + + Merger::ChangeList m_changes; + QSharedPointer m_sourceDatabase; + QSharedPointer m_targetDatabase; +}; + +#endif // KEEPASSX_MERGEDIALOG_H diff --git a/src/gui/MergeDialog.ui b/src/gui/MergeDialog.ui new file mode 100644 index 000000000..b85bb20d6 --- /dev/null +++ b/src/gui/MergeDialog.ui @@ -0,0 +1,31 @@ + + + MergeDialog + + + + 0 + 0 + 800 + 450 + + + + Database Merge Confirmation + + + + + + + + + QDialogButtonBox::Abort|QDialogButtonBox::Ok + + + + + + + + diff --git a/src/gui/entry/EntryHistoryModel.cpp b/src/gui/entry/EntryHistoryModel.cpp index 57cac8d9f..f0be46e91 100644 --- a/src/gui/entry/EntryHistoryModel.cpp +++ b/src/gui/entry/EntryHistoryModel.cpp @@ -193,64 +193,7 @@ void EntryHistoryModel::calculateHistoryModifications() continue; } - QStringList modifiedFields; - - if (*curr->attributes() != *compare->attributes()) { - bool foundAttribute = false; - - if (curr->title() != compare->title()) { - modifiedFields << tr("Title"); - foundAttribute = true; - } - if (curr->username() != compare->username()) { - modifiedFields << tr("Username"); - foundAttribute = true; - } - if (curr->password() != compare->password()) { - modifiedFields << tr("Password"); - foundAttribute = true; - } - if (curr->url() != compare->url()) { - modifiedFields << tr("URL"); - foundAttribute = true; - } - if (curr->notes() != compare->notes()) { - modifiedFields << tr("Notes"); - foundAttribute = true; - } - - if (!foundAttribute) { - modifiedFields << tr("Custom Attributes"); - } - } - if (curr->iconNumber() != compare->iconNumber() || curr->iconUuid() != compare->iconUuid()) { - modifiedFields << tr("Icon"); - } - if (curr->foregroundColor() != compare->foregroundColor() - || curr->backgroundColor() != compare->backgroundColor()) { - modifiedFields << tr("Color"); - } - if (curr->timeInfo().expires() != compare->timeInfo().expires() - || curr->timeInfo().expiryTime() != compare->timeInfo().expiryTime()) { - modifiedFields << tr("Expiration"); - } - if (curr->totp() != compare->totp()) { - modifiedFields << tr("TOTP"); - } - if (*curr->customData() != *compare->customData()) { - modifiedFields << tr("Custom Data"); - } - if (*curr->attachments() != *compare->attachments()) { - modifiedFields << tr("Attachments"); - } - if (*curr->autoTypeAssociations() != *compare->autoTypeAssociations() - || curr->autoTypeEnabled() != compare->autoTypeEnabled() - || curr->defaultAutoTypeSequence() != compare->defaultAutoTypeSequence()) { - modifiedFields << tr("Auto-Type"); - } - if (curr->tags() != compare->tags()) { - modifiedFields << tr("Tags"); - } + auto modifiedFields = curr->calculateDifference(compare); m_historyModifications << modifiedFields.join(", "); diff --git a/src/keeshare/KeeShare.cpp b/src/keeshare/KeeShare.cpp index 2dec75502..f14b0d5ae 100644 --- a/src/keeshare/KeeShare.cpp +++ b/src/keeshare/KeeShare.cpp @@ -217,6 +217,17 @@ void KeeShare::connectDatabase(QSharedPointer newDb, QSharedPointer db, bool enabled) +{ + if (!db || !m_observersByDatabase.contains(db->uuid())) { + return false; + } + + auto observer = m_observersByDatabase.value(db->uuid()); + observer->setEnabled(enabled); + return true; +} + const QString KeeShare::signedContainerFileType() { static const QString filetype("kdbx.share"); diff --git a/src/keeshare/KeeShare.h b/src/keeshare/KeeShare.h index 606947da3..17052a9c6 100644 --- a/src/keeshare/KeeShare.h +++ b/src/keeshare/KeeShare.h @@ -65,6 +65,7 @@ public: static QString referenceTypeLabel(const KeeShareSettings::Reference& reference); void connectDatabase(QSharedPointer newDb, QSharedPointer oldDb); + bool setSharingEnabled(QSharedPointer db, bool enabled); static const QString signedContainerFileType(); static const QString unsignedContainerFileType(); diff --git a/src/keeshare/ShareObserver.cpp b/src/keeshare/ShareObserver.cpp index 5a0da292d..812dbc0f0 100644 --- a/src/keeshare/ShareObserver.cpp +++ b/src/keeshare/ShareObserver.cpp @@ -57,6 +57,11 @@ ShareObserver::~ShareObserver() m_db->disconnect(this); } +void ShareObserver::setEnabled(bool enabled) +{ + m_enabled = enabled; +} + void ShareObserver::deinitialize() { m_groupToReference.clear(); @@ -177,7 +182,7 @@ void ShareObserver::handleDatabaseChanged() return; } const auto active = KeeShare::active(); - if (!active.out && !active.in) { + if (!m_enabled || (!active.out && !active.in)) { deinitialize(); } else { reinitialize(); diff --git a/src/keeshare/ShareObserver.h b/src/keeshare/ShareObserver.h index 7126946a9..0694aee50 100644 --- a/src/keeshare/ShareObserver.h +++ b/src/keeshare/ShareObserver.h @@ -37,6 +37,7 @@ public: ~ShareObserver(); QSharedPointer database(); + void setEnabled(bool enabled); struct Result { @@ -82,6 +83,7 @@ private: QMap> m_shareToGroup; QMap> m_fileWatchers; bool m_inFileUpdate = false; + bool m_enabled = true; }; #endif // KEEPASSXC_SHAREOBSERVER_H diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index c23b62cbd..9e3c28b19 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -1682,8 +1682,9 @@ void TestCli::testMerge() m_stderr->readLine(); // Skip password prompt QCOMPARE(m_stderr->readAll(), QByteArray()); QList outLines1 = m_stdout->readAll().split('\n'); - QVERIFY(outLines1.at(0).contains("Overwriting Internet")); - QVERIFY(outLines1.at(1).contains("Creating missing Some Website")); + QVERIFY(outLines1.at(0).contains("Modified")); + QVERIFY(outLines1.at(0).contains("Modification time")); + QVERIFY(outLines1.at(1).contains("Added")); QCOMPARE(outLines1.at(2), QString("Successfully merged %1 into %2.").arg(sourceFile.fileName(), targetFile1.fileName()).toUtf8()); @@ -1699,8 +1700,9 @@ void TestCli::testMerge() setInput("a"); execCmd(mergeCmd, {"merge", "--dry-run", "-s", targetFile2.fileName(), sourceFile.fileName()}); QList outLines2 = m_stdout->readAll().split('\n'); - QVERIFY(outLines2.at(0).contains("Overwriting Internet")); - QVERIFY(outLines2.at(1).contains("Creating missing Some Website")); + QVERIFY(outLines1.at(0).contains("Modified")); + QVERIFY(outLines1.at(0).contains("Modification time")); + QVERIFY(outLines2.at(1).contains("Added")); QCOMPARE(outLines2.at(2), QByteArray("Database was not modified by merge operation.")); mergedDb = QSharedPointer::create(); diff --git a/tests/TestMerge.cpp b/tests/TestMerge.cpp index 3dde3a063..1465e57db 100644 --- a/tests/TestMerge.cpp +++ b/tests/TestMerge.cpp @@ -1146,7 +1146,7 @@ void TestMerge::testCustomData() m_clock->advanceSecond(1); Merger merger(dbSource.data(), dbDestination.data()); - QStringList changes = merger.merge(); + auto changes = merger.merge(); QVERIFY(!changes.isEmpty()); @@ -1167,7 +1167,7 @@ void TestMerge::testCustomData() dbSource->metadata()->customData()->set("key3", "oldValue"); dbSource->metadata()->customData()->set("key3", "newValue"); Merger merger2(dbSource.data(), dbDestination.data()); - QStringList changes2 = merger2.merge(); + auto changes2 = merger2.merge(); QVERIFY(changes2.isEmpty()); Merger merger3(dbSource2.data(), dbDestination2.data()); diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index 9011d445c..96c8b0703 100644 --- a/tests/gui/TestGui.cpp +++ b/tests/gui/TestGui.cpp @@ -365,6 +365,10 @@ void TestGui::testMergeDatabase() QTest::keyClicks(editPasswordMerge, "a"); QTest::keyClick(editPasswordMerge, Qt::Key_Enter); + // confirm merge in confirmation dialog + QTRY_VERIFY(QApplication::focusWindow()->title().contains("Merge")); + QTest::keyClick(QApplication::focusWidget(), Qt::Key_Enter); + QTRY_COMPARE(dbMergeSpy.count(), 1); QTRY_VERIFY(m_tabWidget->tabText(m_tabWidget->currentIndex()).contains("*"));