diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 3604750bc..e68e21754 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -380,10 +380,6 @@ Automatically launch KeePassXC at system startup - - Safely save database files (disable if experiencing problems with Dropbox, etc.) - - User Interface @@ -432,6 +428,18 @@ Hide expired entries from Auto-Type + + Use alternative saving method (may solve problems with Dropbox, Google Drive, GVFS, etc.) + + + + Temporary file moved into place + + + + Directly write to database file (dangerous) + + ApplicationSettingsWidgetSecurity diff --git a/src/cli/Add.cpp b/src/cli/Add.cpp index fe016160a..e4bde05f3 100644 --- a/src/cli/Add.cpp +++ b/src/cli/Add.cpp @@ -121,7 +121,7 @@ int Add::executeWithDatabase(QSharedPointer database, QSharedPointersave(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Writing the database failed %1.").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/AddGroup.cpp b/src/cli/AddGroup.cpp index 64f5d8150..46837c77b 100644 --- a/src/cli/AddGroup.cpp +++ b/src/cli/AddGroup.cpp @@ -63,7 +63,7 @@ int AddGroup::executeWithDatabase(QSharedPointer database, QSharedPoin newGroup->setParent(parentGroup); QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Writing the database failed %1.").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Create.cpp b/src/cli/Create.cpp index 59084c4e6..867471697 100644 --- a/src/cli/Create.cpp +++ b/src/cli/Create.cpp @@ -165,7 +165,7 @@ int Create::execute(const QStringList& arguments) } QString errorMessage; - if (!db->saveAs(databaseFilename, &errorMessage, true, false)) { + if (!db->saveAs(databaseFilename, Database::Atomic, false, &errorMessage)) { err << QObject::tr("Failed to save the database: %1.").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Edit.cpp b/src/cli/Edit.cpp index 82352b0cd..8941df5c5 100644 --- a/src/cli/Edit.cpp +++ b/src/cli/Edit.cpp @@ -126,7 +126,7 @@ int Edit::executeWithDatabase(QSharedPointer database, QSharedPointer< entry->endUpdate(); QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Writing the database failed: %1").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Import.cpp b/src/cli/Import.cpp index cc231738c..4c1e99437 100644 --- a/src/cli/Import.cpp +++ b/src/cli/Import.cpp @@ -75,7 +75,7 @@ int Import::execute(const QStringList& arguments) return EXIT_FAILURE; } - if (!db->saveAs(dbPath, &errorMessage, true, false)) { + if (!db->saveAs(dbPath, Database::Atomic, false, &errorMessage)) { err << QObject::tr("Failed to save the database: %1.").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Merge.cpp b/src/cli/Merge.cpp index 9d8711ed7..41e18ca0b 100644 --- a/src/cli/Merge.cpp +++ b/src/cli/Merge.cpp @@ -95,7 +95,7 @@ int Merge::executeWithDatabase(QSharedPointer database, QSharedPointer if (!changeList.isEmpty() && !parser->isSet(Merge::DryRunOption)) { QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Unable to save database to file : %1").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Move.cpp b/src/cli/Move.cpp index a42bb4760..350cc4b82 100644 --- a/src/cli/Move.cpp +++ b/src/cli/Move.cpp @@ -65,7 +65,7 @@ int Move::executeWithDatabase(QSharedPointer database, QSharedPointer< entry->endUpdate(); QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Writing the database failed %1.").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/Remove.cpp b/src/cli/Remove.cpp index c9381fdbe..6ed667fa1 100644 --- a/src/cli/Remove.cpp +++ b/src/cli/Remove.cpp @@ -53,7 +53,7 @@ int Remove::executeWithDatabase(QSharedPointer database, QSharedPointe }; QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Unable to save database to file: %1").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/cli/RemoveGroup.cpp b/src/cli/RemoveGroup.cpp index c495e8337..e1d1d7e69 100644 --- a/src/cli/RemoveGroup.cpp +++ b/src/cli/RemoveGroup.cpp @@ -63,7 +63,7 @@ int RemoveGroup::executeWithDatabase(QSharedPointer database, QSharedP }; QString errorMessage; - if (!database->save(&errorMessage, true, false)) { + if (!database->save(Database::Atomic, false, &errorMessage)) { err << QObject::tr("Unable to save database to file: %1").arg(errorMessage) << endl; return EXIT_FAILURE; } diff --git a/src/core/Config.cpp b/src/core/Config.cpp index 0da3191ef..b586cee05 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -62,6 +62,7 @@ static const QHash configStrings = { {Config::AutoSaveNonDataChanges,{QS("AutoSaveNonDataChanges"), Roaming, true}}, {Config::BackupBeforeSave,{QS("BackupBeforeSave"), Roaming, false}}, {Config::UseAtomicSaves,{QS("UseAtomicSaves"), Roaming, true}}, + {Config::UseDirectWriteSaves,{QS("UseDirectWriteSaves"), Local, false}}, {Config::SearchLimitGroup,{QS("SearchLimitGroup"), Roaming, false}}, {Config::MinimizeOnOpenUrl,{QS("MinimizeOnOpenUrl"), Roaming, false}}, {Config::HideWindowOnCopy,{QS("HideWindowOnCopy"), Roaming, false}}, diff --git a/src/core/Config.h b/src/core/Config.h index 7fa4e006c..c0e3d142b 100644 --- a/src/core/Config.h +++ b/src/core/Config.h @@ -44,6 +44,7 @@ public: AutoSaveNonDataChanges, BackupBeforeSave, UseAtomicSaves, + UseDirectWriteSaves, SearchLimitGroup, MinimizeOnOpenUrl, HideWindowOnCopy, diff --git a/src/core/Database.cpp b/src/core/Database.cpp index e29403fcf..b30c3623a 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -181,7 +181,7 @@ bool Database::isSaving() * @param backup Backup the existing database file, if exists * @return true on success */ -bool Database::save(QString* error, bool atomic, bool backup) +bool Database::save(SaveAction action, bool backup, QString* error) { Q_ASSERT(!m_data.filePath.isEmpty()); if (m_data.filePath.isEmpty()) { @@ -191,7 +191,7 @@ bool Database::save(QString* error, bool atomic, bool backup) return false; } - return saveAs(m_data.filePath, error, atomic, backup); + return saveAs(m_data.filePath, action, backup, error); } /** @@ -212,7 +212,7 @@ bool Database::save(QString* error, bool atomic, bool backup) * @param backup Backup the existing database file, if exists * @return true on success */ -bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool backup) +bool Database::saveAs(const QString& filePath, SaveAction action, bool backup, QString* error) { // Disallow overlapping save operations if (isSaving()) { @@ -260,7 +260,7 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool QFileInfo fileInfo(filePath); auto realFilePath = fileInfo.exists() ? fileInfo.canonicalFilePath() : fileInfo.absoluteFilePath(); bool isNewFile = !QFile::exists(realFilePath); - bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(realFilePath, error, atomic, backup); }); + bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(realFilePath, action, backup, error); }); if (ok) { markAsClean(); setFilePath(filePath); @@ -276,14 +276,19 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool return ok; } -bool Database::performSave(const QString& filePath, QString* error, bool atomic, bool backup) +bool Database::performSave(const QString& filePath, SaveAction action, bool backup, QString* error) { + if (backup) { + backupDatabase(filePath); + } + #if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0) QFileInfo info(filePath); auto createTime = info.exists() ? info.birthTime() : QDateTime::currentDateTime(); #endif - if (atomic) { + switch (action) { + case Atomic: { QSaveFile saveFile(filePath); if (saveFile.open(QIODevice::WriteOnly)) { // write the database to the file @@ -291,12 +296,8 @@ bool Database::performSave(const QString& filePath, QString* error, bool atomic, return false; } - if (backup) { - backupDatabase(filePath); - } - #if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0) - // Retain orginal creation time + // Retain original creation time saveFile.setFileTime(createTime, QFile::FileBirthTime); #endif @@ -309,20 +310,17 @@ bool Database::performSave(const QString& filePath, QString* error, bool atomic, if (error) { *error = saveFile.errorString(); } - } else { + break; + } + case TempFile: { QTemporaryFile tempFile; if (tempFile.open()) { // write the database to the file if (!writeDatabase(&tempFile, error)) { return false; } - tempFile.close(); // flush to disk - if (backup) { - backupDatabase(filePath); - } - // Delete the original db and move the temp file in place auto perms = QFile::permissions(filePath); QFile::remove(filePath); @@ -353,6 +351,23 @@ bool Database::performSave(const QString& filePath, QString* error, bool atomic, if (error) { *error = tempFile.errorString(); } + break; + } + case DirectWrite: { + // Open the original database file for direct-write + QFile dbFile(filePath); + if (dbFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + if (!writeDatabase(&dbFile, error)) { + return false; + } + dbFile.close(); + return true; + } + if (error) { + *error = dbFile.errorString(); + } + break; + } } // Saving failed diff --git a/src/core/Database.h b/src/core/Database.h index d5e5b312e..24bb79db2 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -63,6 +63,13 @@ public: }; static const quint32 CompressionAlgorithmMax = CompressionGZip; + enum SaveAction + { + Atomic, // Saves are transactional and atomic + TempFile, // Write to a temporary location then move into place, may be non-atomic + DirectWrite, // Directly write to the destination file (dangerous) + }; + Database(); explicit Database(const QString& filePath); ~Database() override; @@ -72,8 +79,8 @@ public: QSharedPointer key, QString* error = nullptr, bool readOnly = false); - bool save(QString* error = nullptr, bool atomic = true, bool backup = false); - bool saveAs(const QString& filePath, QString* error = nullptr, bool atomic = true, bool backup = false); + bool save(SaveAction action = Atomic, bool backup = false, QString* error = nullptr); + bool saveAs(const QString& filePath, SaveAction action = Atomic, bool backup = false, QString* error = nullptr); bool extract(QByteArray&, QString* error = nullptr); bool import(const QString& xmlExportPath, QString* error = nullptr); @@ -198,7 +205,7 @@ private: bool writeDatabase(QIODevice* device, QString* error = nullptr); bool backupDatabase(const QString& filePath); bool restoreDatabase(const QString& filePath); - bool performSave(const QString& filePath, QString* error, bool atomic, bool backup); + bool performSave(const QString& filePath, SaveAction flags, bool backup, QString* error); void startModifiedTimer(); void stopModifiedTimer(); diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp index e26163f9f..100258120 100644 --- a/src/gui/ApplicationSettingsWidget.cpp +++ b/src/gui/ApplicationSettingsWidget.cpp @@ -109,6 +109,8 @@ ApplicationSettingsWidget::ApplicationSettingsWidget(QWidget* parent) connect(m_generalUi->systrayShowCheckBox, SIGNAL(toggled(bool)), SLOT(systrayToggled(bool))); connect(m_generalUi->rememberLastDatabasesCheckBox, SIGNAL(toggled(bool)), SLOT(rememberDatabasesToggled(bool))); connect(m_generalUi->resetSettingsButton, SIGNAL(clicked()), SLOT(resetSettings())); + connect(m_generalUi->useAlternativeSaveCheckBox, SIGNAL(toggled(bool)), + m_generalUi->alternativeSaveComboBox, SLOT(setEnabled(bool))); connect(m_secUi->clearClipboardCheckBox, SIGNAL(toggled(bool)), m_secUi->clearClipboardSpinBox, SLOT(setEnabled(bool))); @@ -186,7 +188,8 @@ void ApplicationSettingsWidget::loadSettings() m_generalUi->autoSaveOnExitCheckBox->setChecked(config()->get(Config::AutoSaveOnExit).toBool()); m_generalUi->autoSaveNonDataChangesCheckBox->setChecked(config()->get(Config::AutoSaveNonDataChanges).toBool()); m_generalUi->backupBeforeSaveCheckBox->setChecked(config()->get(Config::BackupBeforeSave).toBool()); - m_generalUi->useAtomicSavesCheckBox->setChecked(config()->get(Config::UseAtomicSaves).toBool()); + m_generalUi->useAlternativeSaveCheckBox->setChecked(!config()->get(Config::UseAtomicSaves).toBool()); + m_generalUi->alternativeSaveComboBox->setCurrentIndex(config()->get(Config::UseDirectWriteSaves).toBool() ? 1 : 0); m_generalUi->autoReloadOnChangeCheckBox->setChecked(config()->get(Config::AutoReloadOnChange).toBool()); m_generalUi->minimizeAfterUnlockCheckBox->setChecked(config()->get(Config::MinimizeAfterUnlock).toBool()); m_generalUi->minimizeOnOpenUrlCheckBox->setChecked(config()->get(Config::MinimizeOnOpenUrl).toBool()); @@ -323,7 +326,8 @@ void ApplicationSettingsWidget::saveSettings() config()->set(Config::AutoSaveOnExit, m_generalUi->autoSaveOnExitCheckBox->isChecked()); config()->set(Config::AutoSaveNonDataChanges, m_generalUi->autoSaveNonDataChangesCheckBox->isChecked()); config()->set(Config::BackupBeforeSave, m_generalUi->backupBeforeSaveCheckBox->isChecked()); - config()->set(Config::UseAtomicSaves, m_generalUi->useAtomicSavesCheckBox->isChecked()); + config()->set(Config::UseAtomicSaves, !m_generalUi->useAlternativeSaveCheckBox->isChecked()); + config()->set(Config::UseDirectWriteSaves, m_generalUi->alternativeSaveComboBox->currentIndex() == 1); config()->set(Config::AutoReloadOnChange, m_generalUi->autoReloadOnChangeCheckBox->isChecked()); config()->set(Config::MinimizeAfterUnlock, m_generalUi->minimizeAfterUnlockCheckBox->isChecked()); config()->set(Config::MinimizeOnOpenUrl, m_generalUi->minimizeOnOpenUrlCheckBox->isChecked()); diff --git a/src/gui/ApplicationSettingsWidgetGeneral.ui b/src/gui/ApplicationSettingsWidgetGeneral.ui index dc55d30cb..0e18dfde6 100644 --- a/src/gui/ApplicationSettingsWidgetGeneral.ui +++ b/src/gui/ApplicationSettingsWidgetGeneral.ui @@ -7,7 +7,7 @@ 0 0 605 - 1279 + 968 @@ -59,7 +59,7 @@ 0 0 581 - 1235 + 924 @@ -264,13 +264,6 @@ - - - - Backup database file before saving - - - @@ -279,15 +272,72 @@ - + - Safely save database files (disable if experiencing problems with Dropbox, etc.) - - - true + Backup database file before saving + + + + Use alternative saving method (may solve problems with Dropbox, Google Drive, GVFS, etc.) + + + + + + + 0 + + + + + Qt::Horizontal + + + QSizePolicy::Fixed + + + + 30 + 20 + + + + + + + + false + + + + Temporary file moved into place + + + + + Directly write to database file (dangerous) + + + + + + + + Qt::Horizontal + + + + 40 + 20 + + + + + + @@ -1046,8 +1096,7 @@ autoSaveOnExitCheckBox autoSaveNonDataChangesCheckBox backupBeforeSaveCheckBox - autoReloadOnChangeCheckBox - useAtomicSavesCheckBox + useAlternativeSaveCheckBox useGroupIconOnEntryCreationCheckBox minimizeOnOpenUrlCheckBox hideWindowOnCopyCheckBox diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 5e7044c78..50a379fb3 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1867,16 +1867,20 @@ bool DatabaseWidget::performSave(QString& errorMessage, const QString& fileName) m_groupView->setDisabled(true); QApplication::processEvents(); + Database::SaveAction saveAction = Database::Atomic; + if (!config()->get(Config::UseAtomicSaves).toBool()) { + if (config()->get(Config::UseDirectWriteSaves).toBool()) { + saveAction = Database::DirectWrite; + } else { + saveAction = Database::TempFile; + } + } + bool ok; if (fileName.isEmpty()) { - ok = m_db->save(&errorMessage, - config()->get(Config::UseAtomicSaves).toBool(), - config()->get(Config::BackupBeforeSave).toBool()); + ok = m_db->save(saveAction, config()->get(Config::BackupBeforeSave).toBool(), &errorMessage); } else { - ok = m_db->saveAs(fileName, - &errorMessage, - config()->get(Config::UseAtomicSaves).toBool(), - config()->get(Config::BackupBeforeSave).toBool()); + ok = m_db->saveAs(fileName, saveAction, config()->get(Config::BackupBeforeSave).toBool(), &errorMessage); } // Return control diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index ed5846b7b..989d4957c 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -75,17 +75,22 @@ void TestDatabase::testSave() // Test safe saves db->metadata()->setName("test"); QVERIFY(db->isModified()); - QVERIFY2(db->save(&error), error.toLatin1()); + QVERIFY2(db->save(Database::Atomic, false, &error), error.toLatin1()); QVERIFY(!db->isModified()); - // Test unsafe saves + // Test temp-file saves db->metadata()->setName("test2"); - QVERIFY2(db->save(&error, false, false), error.toLatin1()); + QVERIFY2(db->save(Database::TempFile, false, &error), error.toLatin1()); + QVERIFY(!db->isModified()); + + // Test direct-write saves + db->metadata()->setName("test3"); + QVERIFY2(db->save(Database::DirectWrite, false, &error), error.toLatin1()); QVERIFY(!db->isModified()); // Test save backups - db->metadata()->setName("test3"); - QVERIFY2(db->save(&error, true, true), error.toLatin1()); + db->metadata()->setName("test4"); + QVERIFY2(db->save(Database::Atomic, true, &error), error.toLatin1()); QVERIFY(!db->isModified()); // Confirm backup exists and then delete it @@ -118,7 +123,7 @@ void TestDatabase::testSignals() QTRY_COMPARE(spyModified.count(), 1); QSignalSpy spySaved(db.data(), SIGNAL(databaseSaved())); - QVERIFY(db->save(&error)); + QVERIFY(db->save(Database::Atomic, false, &error)); QCOMPARE(spySaved.count(), 1); // Short delay to allow file system settling to reduce test failures