From 11ecaf4fa4b360dc2effd00e2c2b92418f0f42e0 Mon Sep 17 00:00:00 2001 From: ckieschnick Date: Sat, 16 Mar 2019 03:39:46 +0100 Subject: [PATCH] Hotfix/2657 prevent share overwrite (#2746) * Fix problem with export from newly saved database Newly created/saved databases (or used with DatabaseWidget::saveAs) were not exported/imported correctly. Fixed the problem by reinitializing the ShareObserver on DatabaseWidget::saveAs. * Introduce warnings and prevent conflicting shares Introduced several warnings and errors to indicate improper settings. Prevent export when a path is used multiple times (only the file path is checked - may ignore multiple similar ways to reference a share). * Improve KeeShare integration in DatabaseWidget Moved initial KeeShare association to constructor. Introduced Q_UNUSED to indicate need for assignment statement. --- src/gui/DatabaseWidget.cpp | 9 +++ src/gui/group/EditGroupWidget.cpp | 6 +- src/gui/group/EditGroupWidget.h | 2 +- src/keeshare/ShareObserver.cpp | 61 ++++++++++++++++--- src/keeshare/group/EditGroupPageKeeShare.cpp | 4 +- src/keeshare/group/EditGroupPageKeeShare.h | 2 +- .../group/EditGroupWidgetKeeShare.cpp | 41 ++++++++++++- src/keeshare/group/EditGroupWidgetKeeShare.h | 3 +- 8 files changed, 109 insertions(+), 19 deletions(-) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 08f7db9c..8728c331 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -208,6 +208,12 @@ DatabaseWidget::DatabaseWidget(QSharedPointer db, QWidget* parent) } #endif +#ifdef WITH_XC_KEESHARE + // We need to reregister the database to allow exports + // from a newly created database + KeeShare::instance()->connectDatabase(m_db, {}); +#endif + switchToMainView(); } @@ -391,6 +397,9 @@ void DatabaseWidget::replaceDatabase(QSharedPointer db) processAutoOpen(); #if defined(WITH_XC_KEESHARE) KeeShare::instance()->connectDatabase(m_db, oldDb); +#else + // Keep the instance active till the end of this function + Q_UNUSED(oldDb); #endif } diff --git a/src/gui/group/EditGroupWidget.cpp b/src/gui/group/EditGroupWidget.cpp index 41351d3d..6c869cf2 100644 --- a/src/gui/group/EditGroupWidget.cpp +++ b/src/gui/group/EditGroupWidget.cpp @@ -36,9 +36,9 @@ public: { } - void set(Group* temporaryGroup) const + void set(Group* temporaryGroup, QSharedPointer database) const { - editPage->set(widget, temporaryGroup); + editPage->set(widget, temporaryGroup, database); } void assign() const @@ -133,7 +133,7 @@ void EditGroupWidget::loadGroup(Group* group, bool create, const QSharedPointer< m_editWidgetProperties->setCustomData(m_temporaryGroup->customData()); for (const ExtraPage& page : asConst(m_extraPages)) { - page.set(m_temporaryGroup.data()); + page.set(m_temporaryGroup.data(), m_db); } setCurrentPage(0); diff --git a/src/gui/group/EditGroupWidget.h b/src/gui/group/EditGroupWidget.h index 4de11772..fd744503 100644 --- a/src/gui/group/EditGroupWidget.h +++ b/src/gui/group/EditGroupWidget.h @@ -43,7 +43,7 @@ public: virtual QString name() = 0; virtual QIcon icon() = 0; virtual QWidget* createWidget() = 0; - virtual void set(QWidget* widget, Group* tempoaryGroup) = 0; + virtual void set(QWidget* widget, Group* tempoaryGroup, QSharedPointer database) = 0; virtual void assign(QWidget* widget) = 0; }; diff --git a/src/keeshare/ShareObserver.cpp b/src/keeshare/ShareObserver.cpp index aea0b0df..63d8358c 100644 --- a/src/keeshare/ShareObserver.cpp +++ b/src/keeshare/ShareObserver.cpp @@ -25,6 +25,7 @@ #include "core/Entry.h" #include "core/FilePath.h" #include "core/FileWatcher.h" +#include "core/Global.h" #include "core/Group.h" #include "core/Merger.h" #include "core/Metadata.h" @@ -191,7 +192,7 @@ void ShareObserver::reinitialize() const auto active = KeeShare::active(); QList updated; - QList groups = m_db->rootGroup()->groupsRecursive(true); + const QList groups = m_db->rootGroup()->groupsRecursive(true); for (Group* group : groups) { Update couple{group, m_groupToReference.value(group), KeeShare::referenceOf(group)}; if (couple.oldReference == couple.newReference) { @@ -214,7 +215,9 @@ void ShareObserver::reinitialize() QStringList success; QStringList warning; QStringList error; - for (const auto& update : updated) { + QMap imported; + QMap exported; + for (const auto& update : asConst(updated)) { if (!update.oldReference.path.isEmpty()) { m_fileWatcher->removePath(update.oldReference.path); } @@ -222,8 +225,12 @@ void ShareObserver::reinitialize() if (!update.newReference.path.isEmpty() && update.newReference.type != KeeShareSettings::Inactive) { m_fileWatcher->addPath(update.newReference.path); } + if (update.newReference.isExporting()) { + exported[update.newReference.path] << update.group->name(); + } if (update.newReference.isImporting()) { + imported[update.newReference.path] << update.group->name(); const auto result = this->importFromReferenceContainer(update.newReference.path); if (!result.isValid()) { // tolerable result - blocked import or missing source @@ -241,6 +248,16 @@ void ShareObserver::reinitialize() } } } + for (auto it = imported.cbegin(); it != imported.cend(); ++it) { + if (it.value().count() > 1) { + warning << tr("Multiple import source path to %1 in %2").arg(it.key(), it.value().join(", ")); + } + } + for (auto it = exported.cbegin(); it != exported.cend(); ++it) { + if (it.value().count() > 1) { + error << tr("Conflicting export target path %1 in %2").arg(it.key(), it.value().join(", ")); + } + } notifyAbout(success, warning, error); } @@ -733,28 +750,55 @@ ShareObserver::Result ShareObserver::exportIntoReferenceUnsignedContainer(const QList ShareObserver::exportIntoReferenceContainers() { QList results; + struct Reference + { + KeeShareSettings::Reference config; + const Group* group; + }; + + QMap> references; const auto groups = m_db->rootGroup()->groupsRecursive(true); for (const auto* group : groups) { const auto reference = KeeShare::referenceOf(group); if (!reference.isExporting()) { continue; } + references[reference.path] << Reference{reference, group}; + } - m_fileWatcher->ignoreFileChanges(reference.path); - QScopedPointer targetDb(exportIntoContainer(reference, group)); - QFileInfo info(reference.path); + for (auto it = references.cbegin(); it != references.cend(); ++it) { + if (it.value().count() != 1) { + const auto path = it.value().first().config.path; + QStringList groups; + for (const auto& reference : it.value()) { + groups << reference.group->name(); + } + results << Result{ + path, Result::Error, tr("Conflicting export target path %1 in %2").arg(path, groups.join(", "))}; + } + } + if (!results.isEmpty()) { + // We need to block export due to config + return results; + } + + for (auto it = references.cbegin(); it != references.cend(); ++it) { + const auto& reference = it.value().first(); + m_fileWatcher->ignoreFileChanges(reference.config.path); + QScopedPointer targetDb(exportIntoContainer(reference.config, reference.group)); + QFileInfo info(reference.config.path); if (isOfExportType(info, KeeShare::signedContainerFileType())) { - results << exportIntoReferenceSignedContainer(reference, targetDb.data()); + results << exportIntoReferenceSignedContainer(reference.config, targetDb.data()); m_fileWatcher->observeFileChanges(true); continue; } if (isOfExportType(info, KeeShare::unsignedContainerFileType())) { - results << exportIntoReferenceUnsignedContainer(reference, targetDb.data()); + results << exportIntoReferenceUnsignedContainer(reference.config, targetDb.data()); m_fileWatcher->observeFileChanges(true); continue; } Q_ASSERT(false); - results << Result{reference.path, Result::Error, tr("Unexpected export error occurred")}; + results << Result{reference.config.path, Result::Error, tr("Unexpected export error occurred")}; } return results; } @@ -767,6 +811,7 @@ void ShareObserver::handleDatabaseSaved() QStringList error; QStringList warning; QStringList success; + const auto results = exportIntoReferenceContainers(); for (const Result& result : results) { if (!result.isValid()) { diff --git a/src/keeshare/group/EditGroupPageKeeShare.cpp b/src/keeshare/group/EditGroupPageKeeShare.cpp index 6d2eabb9..dbc3e118 100644 --- a/src/keeshare/group/EditGroupPageKeeShare.cpp +++ b/src/keeshare/group/EditGroupPageKeeShare.cpp @@ -42,10 +42,10 @@ QWidget* EditGroupPageKeeShare::createWidget() return new EditGroupWidgetKeeShare(); } -void EditGroupPageKeeShare::set(QWidget* widget, Group* temporaryGroup) +void EditGroupPageKeeShare::set(QWidget* widget, Group* temporaryGroup, QSharedPointer database) { EditGroupWidgetKeeShare* settingsWidget = reinterpret_cast(widget); - settingsWidget->setGroup(temporaryGroup); + settingsWidget->setGroup(temporaryGroup, database); } void EditGroupPageKeeShare::assign(QWidget* widget) diff --git a/src/keeshare/group/EditGroupPageKeeShare.h b/src/keeshare/group/EditGroupPageKeeShare.h index 786c4343..a712d93d 100644 --- a/src/keeshare/group/EditGroupPageKeeShare.h +++ b/src/keeshare/group/EditGroupPageKeeShare.h @@ -30,7 +30,7 @@ public: QString name() override; QIcon icon() override; QWidget* createWidget() override; - void set(QWidget* widget, Group* temporaryGroup) override; + void set(QWidget* widget, Group* temporaryGroup, QSharedPointer database) override; void assign(QWidget* widget) override; }; diff --git a/src/keeshare/group/EditGroupWidgetKeeShare.cpp b/src/keeshare/group/EditGroupWidgetKeeShare.cpp index e0827a36..49e64063 100644 --- a/src/keeshare/group/EditGroupWidgetKeeShare.cpp +++ b/src/keeshare/group/EditGroupWidgetKeeShare.cpp @@ -85,12 +85,13 @@ EditGroupWidgetKeeShare::~EditGroupWidgetKeeShare() { } -void EditGroupWidgetKeeShare::setGroup(Group* temporaryGroup) +void EditGroupWidgetKeeShare::setGroup(Group* temporaryGroup, QSharedPointer database) { if (m_temporaryGroup) { m_temporaryGroup->disconnect(this); } + m_database = database; m_temporaryGroup = temporaryGroup; if (m_temporaryGroup) { @@ -128,9 +129,43 @@ void EditGroupWidgetKeeShare::showSharingState() .arg(supportedExtensions.join(", ")), MessageWidget::Warning); return; - } else { - m_ui->messageWidget->hide(); } + + const auto groups = m_database->rootGroup()->groupsRecursive(true); + bool conflictExport = false; + bool multipleImport = false; + bool cycleImportExport = false; + for (const auto* group : groups) { + if (group->uuid() == m_temporaryGroup->uuid()) { + continue; + } + const auto other = KeeShare::referenceOf(group); + if (other.path != reference.path) { + continue; + } + multipleImport |= other.isImporting() && reference.isImporting(); + conflictExport |= other.isExporting() && reference.isExporting(); + cycleImportExport |= + (other.isImporting() && reference.isExporting()) || (other.isExporting() && reference.isImporting()); + } + if (conflictExport) { + m_ui->messageWidget->showMessage(tr("The export container %1 is already referenced.").arg(reference.path), + MessageWidget::Error); + return; + } + if (multipleImport) { + m_ui->messageWidget->showMessage(tr("The import container %1 is already imported.").arg(reference.path), + MessageWidget::Warning); + return; + } + if (cycleImportExport) { + m_ui->messageWidget->showMessage( + tr("The container %1 imported and export by different groups.").arg(reference.path), + MessageWidget::Warning); + return; + } + + m_ui->messageWidget->hide(); } const auto active = KeeShare::active(); if (!active.in && !active.out) { diff --git a/src/keeshare/group/EditGroupWidgetKeeShare.h b/src/keeshare/group/EditGroupWidgetKeeShare.h index 8ecfc2bc..b4e169b5 100644 --- a/src/keeshare/group/EditGroupWidgetKeeShare.h +++ b/src/keeshare/group/EditGroupWidgetKeeShare.h @@ -37,7 +37,7 @@ public: explicit EditGroupWidgetKeeShare(QWidget* parent = nullptr); ~EditGroupWidgetKeeShare(); - void setGroup(Group* temporaryGroup); + void setGroup(Group* temporaryGroup, QSharedPointer database); private slots: void showSharingState(); @@ -55,6 +55,7 @@ private slots: private: QScopedPointer m_ui; QPointer m_temporaryGroup; + QSharedPointer m_database; }; #endif // KEEPASSXC_EDITGROUPWIDGETKEESHARE_H