diff --git a/src/core/Config.cpp b/src/core/Config.cpp index fafb3cfd4f..f42bb60445 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -111,9 +111,11 @@ void Config::init(const QString& fileName) m_defaults.insert("RememberLastDatabases", true); m_defaults.insert("RememberLastKeyFiles", true); m_defaults.insert("OpenPreviousDatabasesOnStartup", true); - m_defaults.insert("AutoSaveAfterEveryChange", false); + m_defaults.insert("AutoSaveAfterEveryChange", true); 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 75b91a5c5f..70affdc58d 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -470,30 +471,101 @@ 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 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 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; - QSaveFile saveFile(filePath); - if (saveFile.open(QIODevice::WriteOnly)) { + setEmitModified(false); + writer.writeDatabase(device, this); + setEmitModified(true); - // write the database to the file - setEmitModified(false); - writer.writeDatabase(&saveFile, this); - setEmitModified(true); - - if (writer.hasError()) { - return writer.errorString(); - } - - if (saveFile.commit()) { - // successfully saved database file - return QString(); - } else { - return saveFile.errorString(); - } - } else { - return saveFile.errorString(); + if (writer.hasError()) { + // the writer failed + return writer.errorString(); } + return {}; +} + +/** + * Remove the old backup and replace it with a new one + * backups are named .old.kdbx + * + * @param filePath Path to the file to backup + * @return + */ +bool Database::backupDatabase(QString filePath) +{ + QString backupFilePath = filePath; + auto re = QRegularExpression("(?:\\.kdbx)?$", QRegularExpression::CaseInsensitiveOption); + backupFilePath.replace(re, ".old.kdbx"); + 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 3bf43f62d9..384ca814ea 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 @@ class Database : public QObject void emptyRecycleBin(); void setEmitModified(bool value); void merge(const Database* other); - QString saveToFile(QString filePath); + 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 slots: 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 c00a67d07e..47ff597b86 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" @@ -43,6 +44,7 @@ DatabaseManagerStruct::DatabaseManagerStruct() : dbWidget(nullptr) , modified(false) , readOnly(false) + , saveAttempts(0) { } @@ -322,12 +324,15 @@ 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 + bool useAtomicSaves = config()->get("UseAtomicSaves", true).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); @@ -336,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 875c3c904e..b216750ea0 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 7d3f65f813..919edf9fd4 100644 --- a/src/gui/SettingsWidget.cpp +++ b/src/gui/SettingsWidget.cpp @@ -117,6 +117,8 @@ 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->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()); @@ -193,6 +195,8 @@ 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("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 87af235d48..5ed952f1fd 100644 --- a/src/gui/SettingsWidgetGeneral.ui +++ b/src/gui/SettingsWidgetGeneral.ui @@ -34,124 +34,134 @@ - - - Start only a single instance of KeePassXC - - - 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 - - - - - - - Automatically reload the database when modified externally - - - - - - - Minimize when copying to clipboard - - - - - - - Minimize window at application startup + + + Startup + + + + + Start only a single instance of KeePassXC + + + true + + + + + + + Remember last databases + + + true + + + + + + + Remember last key files and security dongles + + + true + + + + + + + Load previous databases on startup + + + + + + + Minimize window at application startup + + + + - - - Use group icon on entry creation - - - true + + + 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 + + + + - - - Don't mark database as modified for non-data changes (e.g., expanding groups) + + + Entry Management - - - - - - - 0 - - - 0 - - - 0 - - - 0 - + - - - Qt::Vertical + + + Use group icon on entry creation - - QSizePolicy::Fixed + + true - - - 20 - 30 - + + + + + + Minimize when copying to clipboard - + @@ -160,6 +170,15 @@ + + + + + + + General + + @@ -168,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 - - - - - - - - - 0 - - - QLayout::SetMaximumSize - - - - - Qt::Horizontal - - - QSizePolicy::Fixed - - - - 40 - 20 - - - - - - - - false - - - Hide window to system tray instead of app exit + Language - - - - - - 0 - - - QLayout::SetMaximumSize - - - - Qt::Horizontal - - - QSizePolicy::Fixed - - - - 40 - 20 - - - - - - - - false - - - Dark system tray icon + + + + 0 + 0 + @@ -284,52 +351,6 @@ - - - - Qt::Vertical - - - QSizePolicy::Fixed - - - - 20 - 30 - - - - - - - - 15 - - - - - - 0 - 0 - - - - Language - - - - - - - - 0 - 0 - - - - - - diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index 44b1d06641..cf7b969e11 100644 --- a/tests/gui/TestGui.cpp +++ b/tests/gui/TestGui.cpp @@ -67,6 +67,9 @@ void TestGui::initTestCase() { QVERIFY(Crypto::init()); Config::createTempFileInstance(); + // Disable autosave so we can test the modified file indicator + Config::instance()->set("AutoSaveAfterEveryChange", false); + m_mainWindow = new MainWindow(); m_tabWidget = m_mainWindow->findChild("tabWidget"); m_mainWindow->show(); @@ -141,7 +144,6 @@ void TestGui::testCreateDatabase() DatabaseWidget* dbWidget = m_tabWidget->currentDatabaseWidget(); QWidget* databaseNewWidget = dbWidget->findChild("changeMasterKeyWidget"); - QList databaseNewWidgets = dbWidget->findChildren("changeMasterKeyWidget"); PasswordEdit* editPassword = databaseNewWidget->findChild("enterPasswordEdit"); QVERIFY(editPassword->isVisible()); @@ -154,6 +156,7 @@ void TestGui::testCreateDatabase() QTest::keyClicks(editPasswordRepeat, "test"); QTest::keyClick(editPasswordRepeat, Qt::Key_Enter); + // Auto-save after every change is enabled by default, ensure the db saves right away QTRY_VERIFY(m_tabWidget->tabText(m_tabWidget->currentIndex()).contains("*")); m_db = m_tabWidget->currentDatabaseWidget()->database();