From d7f408e4553c8d0e36de519b418b15d61132b487 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sun, 14 Jan 2018 18:04:33 -0500 Subject: [PATCH] Correct saving files to DropBox/Drive/OneDrive * Replaces QSaveFile with QTemporaryFile * Added backup before save config setting * This method may cause data loss (see comments) --- src/core/Config.cpp | 1 + src/core/Database.cpp | 46 +++++++++++++++++++++++--------- src/core/Database.h | 2 +- src/gui/DatabaseTabWidget.cpp | 4 ++- src/gui/SettingsWidget.cpp | 2 ++ src/gui/SettingsWidgetGeneral.ui | 7 +++++ 6 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/core/Config.cpp b/src/core/Config.cpp index fafb3cfd4..3f49bafef 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -114,6 +114,7 @@ void Config::init(const QString& fileName) m_defaults.insert("AutoSaveAfterEveryChange", false); m_defaults.insert("AutoReloadOnChange", true); m_defaults.insert("AutoSaveOnExit", false); + m_defaults.insert("BackupBeforeSave", false); m_defaults.insert("SearchLimitGroup", false); m_defaults.insert("MinimizeOnCopy", false); m_defaults.insert("UseGroupIconOnEntryCreation", false); diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 75b91a5c5..c65c566f9 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -19,7 +19,7 @@ #include "Database.h" #include -#include +#include #include #include #include @@ -470,30 +470,52 @@ Database* Database::unlockFromStdin(QString databaseFilename, QString keyFilenam return Database::openDatabaseFile(databaseFilename, compositeKey); } -QString Database::saveToFile(QString filePath) +/** + * Save the database to a file. + * + * This function uses QTemporaryFile instead of QSaveFile due to a bug + * in Qt (https://bugreports.qt.io/browse/QTBUG-57299) that may prevent + * the QSaveFile from renaming itself when using DropBox, Drive, or OneDrive. + * + * The risk in using QTemporaryFile is that the rename function is not atomic + * and may result in loss of data if there is a crash or power loss at the + * wrong moment. + * + * @param filePath Absolute path of the file to save + * @param keepOld Rename the original database file instead of deleting + * @return error string, if any + */ +QString Database::saveToFile(QString filePath, bool keepOld) { KeePass2Writer writer; - QSaveFile saveFile(filePath); - if (saveFile.open(QIODevice::WriteOnly)) { - + QTemporaryFile saveFile; + if (saveFile.open()) { // write the database to the file setEmitModified(false); writer.writeDatabase(&saveFile, this); setEmitModified(true); if (writer.hasError()) { + // the writer failed return writer.errorString(); } - if (saveFile.commit()) { - // successfully saved database file - return QString(); - } else { - return saveFile.errorString(); + saveFile.close(); // flush to disk + + if (keepOld) { + QFile::remove(filePath + ".old"); + QFile::rename(filePath, filePath + ".old"); + } + + QFile::remove(filePath); + if (saveFile.rename(filePath)) { + // successfully saved database file + saveFile.setAutoRemove(false); + return {}; } - } else { - return saveFile.errorString(); } + + return saveFile.errorString(); } QSharedPointer Database::kdf() const diff --git a/src/core/Database.h b/src/core/Database.h index 3bf43f62d..31d190877 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -111,7 +111,7 @@ public: void emptyRecycleBin(); void setEmitModified(bool value); void merge(const Database* other); - QString saveToFile(QString filePath); + QString saveToFile(QString filePath, bool keepOld = false); /** * Returns a unique id that is only valid as long as the Database exists. diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index c00a67d07..06ff84ed3 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -28,6 +28,7 @@ #include "core/Database.h" #include "core/Group.h" #include "core/Metadata.h" +#include "core/AsyncTask.h" #include "format/CsvExporter.h" #include "gui/Clipboard.h" #include "gui/DatabaseWidget.h" @@ -322,7 +323,8 @@ bool DatabaseTabWidget::saveDatabase(Database* db, QString filePath) } dbStruct.dbWidget->blockAutoReload(true); - QString errorMessage = db->saveToFile(filePath); + // TODO: Make this async, but lock out the database widget to prevent re-entrance + QString errorMessage = db->saveToFile(filePath, config()->get("BackupBeforeSave").toBool()); dbStruct.dbWidget->blockAutoReload(false); if (errorMessage.isEmpty()) { diff --git a/src/gui/SettingsWidget.cpp b/src/gui/SettingsWidget.cpp index 7d3f65f81..12c970882 100644 --- a/src/gui/SettingsWidget.cpp +++ b/src/gui/SettingsWidget.cpp @@ -117,6 +117,7 @@ void SettingsWidget::loadSettings() config()->get("OpenPreviousDatabasesOnStartup").toBool()); m_generalUi->autoSaveAfterEveryChangeCheckBox->setChecked(config()->get("AutoSaveAfterEveryChange").toBool()); m_generalUi->autoSaveOnExitCheckBox->setChecked(config()->get("AutoSaveOnExit").toBool()); + m_generalUi->backupBeforeSaveCheckBox->setChecked(config()->get("BackupBeforeSave").toBool()); m_generalUi->autoReloadOnChangeCheckBox->setChecked(config()->get("AutoReloadOnChange").toBool()); m_generalUi->minimizeOnCopyCheckBox->setChecked(config()->get("MinimizeOnCopy").toBool()); m_generalUi->useGroupIconOnEntryCreationCheckBox->setChecked(config()->get("UseGroupIconOnEntryCreation").toBool()); @@ -193,6 +194,7 @@ void SettingsWidget::saveSettings() config()->set("AutoSaveAfterEveryChange", m_generalUi->autoSaveAfterEveryChangeCheckBox->isChecked()); config()->set("AutoSaveOnExit", m_generalUi->autoSaveOnExitCheckBox->isChecked()); + config()->set("BackupBeforeSave", m_generalUi->backupBeforeSaveCheckBox->isChecked()); config()->set("AutoReloadOnChange", m_generalUi->autoReloadOnChangeCheckBox->isChecked()); config()->set("MinimizeOnCopy", m_generalUi->minimizeOnCopyCheckBox->isChecked()); config()->set("UseGroupIconOnEntryCreation", diff --git a/src/gui/SettingsWidgetGeneral.ui b/src/gui/SettingsWidgetGeneral.ui index 87af235d4..a80c8c2f9 100644 --- a/src/gui/SettingsWidgetGeneral.ui +++ b/src/gui/SettingsWidgetGeneral.ui @@ -84,6 +84,13 @@ + + + + Backup database file before saving + + +