From 3db9a86a4c89151dd0eb0f8929e0a0a8b9ff6af8 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Wed, 17 Jan 2018 20:13:13 -0500 Subject: [PATCH] After 3 failed saves, offer to disable safe saves * User is prompted to disable safe saves after three failed attempts * Completely retooled basic settings to group settings logically * Added setting for "atomic saves" --- src/core/Config.cpp | 1 + src/core/Database.cpp | 107 ++++-- src/core/Database.h | 5 +- src/gui/DatabaseTabWidget.cpp | 21 +- src/gui/DatabaseTabWidget.h | 1 + src/gui/SettingsWidget.cpp | 2 + src/gui/SettingsWidgetGeneral.ui | 538 ++++++++++++++++--------------- 7 files changed, 384 insertions(+), 291 deletions(-) diff --git a/src/core/Config.cpp b/src/core/Config.cpp index 3f49bafef..328dfaed8 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -115,6 +115,7 @@ void Config::init(const QString& fileName) m_defaults.insert("AutoReloadOnChange", true); m_defaults.insert("AutoSaveOnExit", false); m_defaults.insert("BackupBeforeSave", false); + m_defaults.insert("UseAtomicSaves", true); 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 c65c566f9..9a1fe0b3d 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -19,6 +19,7 @@ #include "Database.h" #include +#include #include #include #include @@ -482,40 +483,92 @@ Database* Database::unlockFromStdin(QString databaseFilename, QString keyFilenam * wrong moment. * * @param filePath Absolute path of the file to save - * @param keepOld Rename the original database file instead of deleting + * @param atomic Use atomic file transactions + * @param backup Backup the existing database file, if exists * @return error string, if any */ -QString Database::saveToFile(QString filePath, bool keepOld) +QString Database::saveToFile(QString filePath, bool atomic, bool backup) +{ + QString error; + if (atomic) { + QSaveFile saveFile(filePath); + if (saveFile.open(QIODevice::WriteOnly)) { + // write the database to the file + error = writeDatabase(&saveFile); + if (!error.isEmpty()) { + return error; + } + + if (backup) { + backupDatabase(filePath); + } + + if (saveFile.commit()) { + // successfully saved database file + return {}; + } + } + error = saveFile.errorString(); + } else { + QTemporaryFile tempFile; + if (tempFile.open()) { + // write the database to the file + error = writeDatabase(&tempFile); + if (!error.isEmpty()) { + return error; + } + + tempFile.close(); // flush to disk + + if (backup) { + backupDatabase(filePath); + } + + // Delete the original db and move the temp file in place + QFile::remove(filePath); + if (tempFile.rename(filePath)) { + // successfully saved database file + tempFile.setAutoRemove(false); + return {}; + } + } + error = tempFile.errorString(); + } + // Saving failed + return error; +} + +QString Database::writeDatabase(QIODevice* device) { KeePass2Writer writer; - QTemporaryFile saveFile; - if (saveFile.open()) { - // write the database to the file - setEmitModified(false); - writer.writeDatabase(&saveFile, this); - setEmitModified(true); + setEmitModified(false); + writer.writeDatabase(device, this); + setEmitModified(true); - if (writer.hasError()) { - // the writer failed - return writer.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 {}; - } + if (writer.hasError()) { + // the writer failed + return writer.errorString(); } + return {}; +} - return saveFile.errorString(); +/** + * Remove the old backup and replace it with a new one + * backups are named .old.kdbx4 + * + * @param filePath Path to the file to backup + * @return + */ +bool Database::backupDatabase(QString filePath) +{ + QString backupFilePath = filePath; + backupFilePath.replace(".kdbx", ".old.kdbx", Qt::CaseInsensitive); + if (!backupFilePath.endsWith(".old.kdbx")) { + // Fallback in case of poorly named file + backupFilePath = filePath + ".old"; + } + QFile::remove(backupFilePath); + return QFile::copy(filePath, backupFilePath); } QSharedPointer Database::kdf() const diff --git a/src/core/Database.h b/src/core/Database.h index 31d190877..384ca814e 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -32,6 +32,7 @@ enum class EntryReferenceType; class Group; class Metadata; class QTimer; +class QIODevice; struct DeletedObject { @@ -111,7 +112,7 @@ public: void emptyRecycleBin(); void setEmitModified(bool value); void merge(const Database* other); - QString saveToFile(QString filePath, bool keepOld = false); + QString saveToFile(QString filePath, bool atomic = true, bool backup = false); /** * Returns a unique id that is only valid as long as the Database exists. @@ -144,6 +145,8 @@ private: Group* findGroupRecursive(const Uuid& uuid, Group* group); void createRecycleBin(); + QString writeDatabase(QIODevice* device); + bool backupDatabase(QString filePath); Metadata* const m_metadata; Group* m_rootGroup; diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index 06ff84ed3..1765a9cdf 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -44,6 +44,7 @@ DatabaseManagerStruct::DatabaseManagerStruct() : dbWidget(nullptr) , modified(false) , readOnly(false) + , saveAttempts(0) { } @@ -324,12 +325,14 @@ bool DatabaseTabWidget::saveDatabase(Database* db, QString filePath) dbStruct.dbWidget->blockAutoReload(true); // TODO: Make this async, but lock out the database widget to prevent re-entrance - QString errorMessage = db->saveToFile(filePath, config()->get("BackupBeforeSave").toBool()); + bool useAtomicSaves = config()->get("UseAtomicSaves").toBool(); + QString errorMessage = db->saveToFile(filePath, useAtomicSaves, config()->get("BackupBeforeSave").toBool()); dbStruct.dbWidget->blockAutoReload(false); if (errorMessage.isEmpty()) { // successfully saved database file dbStruct.modified = false; + dbStruct.saveAttempts = 0; dbStruct.fileInfo = QFileInfo(filePath); dbStruct.dbWidget->databaseSaved(); updateTabName(db); @@ -338,6 +341,22 @@ bool DatabaseTabWidget::saveDatabase(Database* db, QString filePath) } else { dbStruct.modified = true; updateTabName(db); + + if (++dbStruct.saveAttempts > 2 && useAtomicSaves) { + // Saving failed 3 times, issue a warning and attempt to resolve + auto choice = MessageBox::question(this, tr("Disable safe saves?"), + tr("KeePassXC has failed to save the database multiple times. " + "This is likely caused by file sync services holding a lock on " + "the save file.\nDisable safe saves and try again?"), + QMessageBox::Yes | QMessageBox::No, QMessageBox::Yes); + if (choice == QMessageBox::Yes) { + config()->set("UseAtomicSaves", false); + return saveDatabase(db, filePath); + } + // Reset save attempts without changing anything + dbStruct.saveAttempts = 0; + } + emit messageTab(tr("Writing the database failed.").append("\n").append(errorMessage), MessageWidget::Error); return false; diff --git a/src/gui/DatabaseTabWidget.h b/src/gui/DatabaseTabWidget.h index 875c3c904..b216750ea 100644 --- a/src/gui/DatabaseTabWidget.h +++ b/src/gui/DatabaseTabWidget.h @@ -40,6 +40,7 @@ struct DatabaseManagerStruct QFileInfo fileInfo; bool modified; bool readOnly; + int saveAttempts; }; Q_DECLARE_TYPEINFO(DatabaseManagerStruct, Q_MOVABLE_TYPE); diff --git a/src/gui/SettingsWidget.cpp b/src/gui/SettingsWidget.cpp index 12c970882..919edf9fd 100644 --- a/src/gui/SettingsWidget.cpp +++ b/src/gui/SettingsWidget.cpp @@ -118,6 +118,7 @@ void SettingsWidget::loadSettings() 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->useAtomicSavesCheckBox->setChecked(config()->get("UseAtomicSaves").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()); @@ -195,6 +196,7 @@ void SettingsWidget::saveSettings() m_generalUi->autoSaveAfterEveryChangeCheckBox->isChecked()); config()->set("AutoSaveOnExit", m_generalUi->autoSaveOnExitCheckBox->isChecked()); config()->set("BackupBeforeSave", m_generalUi->backupBeforeSaveCheckBox->isChecked()); + config()->set("UseAtomicSaves", m_generalUi->useAtomicSavesCheckBox->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 a80c8c2f9..f8414f07c 100644 --- a/src/gui/SettingsWidgetGeneral.ui +++ b/src/gui/SettingsWidgetGeneral.ui @@ -34,131 +34,134 @@ - - - Start only a single instance of KeePassXC + + + Startup - - true - - - - - - - Remember last databases - - - true - - - - - - - Remember last key files and security dongles - - - true - - - - - - - Load previous databases on startup - - - - - - - Automatically save on exit - - - - - - - Automatically save after every change - - - - - - - Backup database file before saving - - - - - - - Automatically reload the database when modified externally - - - - - - - Minimize when copying to clipboard - - - - - - - Minimize window at application startup - - - - - - - Use group icon on entry creation - - - true - - - - - - - Don't mark database as modified for non-data changes (e.g., expanding groups) - - - - - - - - 0 - - - 0 - - - 0 - - - 0 - + - - - Qt::Vertical + + + Start only a single instance of KeePassXC - - QSizePolicy::Fixed + + true - - - 20 - 30 - + + + + + + Remember last databases - + + true + + + + + + + Remember last key files and security dongles + + + true + + + + + + + Load previous databases on startup + + + + + + + Minimize window at application startup + + + + + + + + + + File Management + + + + + + Safely save database files (may be incompatible with DropBox, etc) + + + true + + + + + + + Backup database file before saving + + + + + + + Automatically save after every change + + + + + + + Automatically save on exit + + + + + + + Don't mark database as modified for non-data changes (e.g., expanding groups) + + + + + + + Automatically reload the database when modified externally + + + + + + + + + + Entry Management + + + + + + Use group icon on entry creation + + + true + + + + + + + Minimize when copying to clipboard + + @@ -167,6 +170,15 @@ + + + + + + + General + + @@ -175,114 +187,162 @@ - + + + + 0 + + + 0 + + + 0 + + + 0 + + + + + 0 + + + QLayout::SetMaximumSize + + + + + Qt::Horizontal + + + QSizePolicy::Fixed + + + + 40 + 20 + + + + + + + + false + + + + 0 + 0 + + + + Hide window to system tray when minimized + + + + + + + + + 0 + + + QLayout::SetMaximumSize + + + + + Qt::Horizontal + + + QSizePolicy::Fixed + + + + 40 + 20 + + + + + + + + false + + + Hide window to system tray instead of app exit + + + + + + + + + 0 + + + QLayout::SetMaximumSize + + + + + Qt::Horizontal + + + QSizePolicy::Fixed + + + + 40 + 20 + + + + + + + + false + + + Dark system tray icon + + + + + + + + + + - 0 + 15 - - QLayout::SetMaximumSize - - - - - Qt::Horizontal - - - QSizePolicy::Fixed - - - - 40 - 20 - - - - - - - - false - + + - + 0 0 - Hide window to system tray when minimized + Language - - - - - - 0 - - - QLayout::SetMaximumSize - - - - Qt::Horizontal - - - QSizePolicy::Fixed - - - - 40 - 20 - - - - - - - - false - - - Hide window to system tray instead of app exit - - - - - - - - - 0 - - - QLayout::SetMaximumSize - - - - - Qt::Horizontal - - - QSizePolicy::Fixed - - - - 40 - 20 - - - - - - - - false - - - Dark system tray icon + + + + 0 + 0 + @@ -291,52 +351,6 @@ - - - - Qt::Vertical - - - QSizePolicy::Fixed - - - - 20 - 30 - - - - - - - - 15 - - - - - - 0 - 0 - - - - Language - - - - - - - - 0 - 0 - - - - - -