From 84ff6a13f9cd11214396deb31d54955abbcd7b0b Mon Sep 17 00:00:00 2001 From: Patrick Klein <42714034+libklein@users.noreply.github.com> Date: Sun, 7 Nov 2021 23:41:17 +0100 Subject: [PATCH] Allow specifing database backup paths. (#7035) - Default backupFilePath is '{DB_FILENAME}.old.kdbx' to conform to existing standards - Implement backupPathPattern tests. - Show tooltip on how to format database backup location text field. --- share/translations/keepassxc_en.ts | 20 ++++ src/cli/Add.cpp | 2 +- src/cli/AddGroup.cpp | 2 +- src/cli/Create.cpp | 2 +- src/cli/Edit.cpp | 2 +- src/cli/Import.cpp | 2 +- src/cli/Merge.cpp | 2 +- src/cli/Move.cpp | 2 +- src/cli/Remove.cpp | 2 +- src/cli/RemoveGroup.cpp | 2 +- src/core/Config.cpp | 6 + src/core/Config.h | 2 + src/core/Database.cpp | 56 +++++----- src/core/Database.h | 13 ++- src/core/Tools.cpp | 36 ++++++ src/core/Tools.h | 2 + src/gui/ApplicationSettingsWidget.cpp | 25 +++++ src/gui/ApplicationSettingsWidget.h | 1 + src/gui/ApplicationSettingsWidgetGeneral.ui | 50 ++++++++- src/gui/DatabaseWidget.cpp | 25 ++++- tests/TestDatabase.cpp | 17 ++- tests/TestTools.cpp | 54 +++++++++ tests/TestTools.h | 2 + tests/gui/TestGui.cpp | 117 ++++++++++++++++---- tests/gui/TestGui.h | 5 +- 25 files changed, 368 insertions(+), 81 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index dd87e1920..e671d762f 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -215,6 +215,10 @@ Monochrome + + Select backup storage directory + + ApplicationSettingsWidgetGeneral @@ -440,6 +444,22 @@ Directly write to database file (dangerous) + + Choose... + + + + Backup destination + + + + Specifies the database backup file location. Occurences of "{DB_FILENAME}" are replaced with the filename of the saved database without extension. {TIME:<format>} is replaced with the backup time, see https://doc.qt.io/qt-5/qdatetime.html#toString. <format> defaults to format string "dd_MM_yyyy_hh-mm-ss". + + + + {DB_FILENAME}.old.kdbx + + ApplicationSettingsWidgetSecurity diff --git a/src/cli/Add.cpp b/src/cli/Add.cpp index e4bde05f3..409d2a0aa 100644 --- a/src/cli/Add.cpp +++ b/src/cli/Add.cpp @@ -121,7 +121,7 @@ int Add::executeWithDatabase(QSharedPointer database, QSharedPointersave(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 46837c77b..8fc9ceb39 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 867471697..4f820d000 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, Database::Atomic, false, &errorMessage)) { + if (!db->saveAs(databaseFilename, Database::Atomic, QString(), &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 8941df5c5..ad1d701f7 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 4c1e99437..432507d20 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, Database::Atomic, false, &errorMessage)) { + if (!db->saveAs(dbPath, Database::Atomic, QString(), &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 41e18ca0b..a80f454bb 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 350cc4b82..990f45691 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 6ed667fa1..ed6a27502 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 e1d1d7e69..df6bf4cfa 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(Database::Atomic, false, &errorMessage)) { + if (!database->save(Database::Atomic, QString(), &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 54fc59c81..a6d179504 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -61,6 +61,7 @@ static const QHash configStrings = { {Config::AutoSaveOnExit,{QS("AutoSaveOnExit"), Roaming, true}}, {Config::AutoSaveNonDataChanges,{QS("AutoSaveNonDataChanges"), Roaming, true}}, {Config::BackupBeforeSave,{QS("BackupBeforeSave"), Roaming, false}}, + {Config::BackupFilePathPattern,{QS("BackupFilePathPattern"), Roaming, QString("{DB_FILENAME}.old.kdbx")}}, {Config::UseAtomicSaves,{QS("UseAtomicSaves"), Roaming, true}}, {Config::UseDirectWriteSaves,{QS("UseDirectWriteSaves"), Local, false}}, {Config::SearchLimitGroup,{QS("SearchLimitGroup"), Roaming, false}}, @@ -229,6 +230,11 @@ QVariant Config::get(ConfigKey key) return m_settings->value(cfg.name, defaultValue); } +QVariant Config::getDefault(Config::ConfigKey key) +{ + return configStrings[key].defaultValue; +} + bool Config::hasAccessError() { return m_settings->status() & QSettings::AccessError; diff --git a/src/core/Config.h b/src/core/Config.h index 8e5c14e35..1be8699ca 100644 --- a/src/core/Config.h +++ b/src/core/Config.h @@ -43,6 +43,7 @@ public: AutoSaveOnExit, AutoSaveNonDataChanges, BackupBeforeSave, + BackupFilePathPattern, UseAtomicSaves, UseDirectWriteSaves, SearchLimitGroup, @@ -195,6 +196,7 @@ public: ~Config() override; QVariant get(ConfigKey key); + QVariant getDefault(ConfigKey key); QString getFileName(); void set(ConfigKey key, const QVariant& value); void remove(ConfigKey key); diff --git a/src/core/Database.cpp b/src/core/Database.cpp index b30c3623a..5738a3b0a 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -178,10 +178,10 @@ bool Database::isSaving() * * @param error error message in case of failure * @param atomic Use atomic file transactions - * @param backup Backup the existing database file, if exists + * @param backupFilePath Absolute file path to write the backup file to. Pass an empty QString to disable backup. * @return true on success */ -bool Database::save(SaveAction action, bool backup, QString* error) +bool Database::save(SaveAction action, const QString& backupFilePath, QString* error) { Q_ASSERT(!m_data.filePath.isEmpty()); if (m_data.filePath.isEmpty()) { @@ -191,7 +191,7 @@ bool Database::save(SaveAction action, bool backup, QString* error) return false; } - return saveAs(m_data.filePath, action, backup, error); + return saveAs(m_data.filePath, action, backupFilePath, error); } /** @@ -209,10 +209,11 @@ bool Database::save(SaveAction action, bool backup, QString* error) * @param filePath Absolute path of the file to save * @param error error message in case of failure * @param atomic Use atomic file transactions - * @param backup Backup the existing database file, if exists + * @param backupFilePath Absolute path to the location where the backup should be stored. Passing an empty string + * disables backup. * @return true on success */ -bool Database::saveAs(const QString& filePath, SaveAction action, bool backup, QString* error) +bool Database::saveAs(const QString& filePath, SaveAction action, const QString& backupFilePath, QString* error) { // Disallow overlapping save operations if (isSaving()) { @@ -260,7 +261,7 @@ bool Database::saveAs(const QString& filePath, SaveAction action, bool backup, Q QFileInfo fileInfo(filePath); auto realFilePath = fileInfo.exists() ? fileInfo.canonicalFilePath() : fileInfo.absoluteFilePath(); bool isNewFile = !QFile::exists(realFilePath); - bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(realFilePath, action, backup, error); }); + bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(realFilePath, action, backupFilePath, error); }); if (ok) { markAsClean(); setFilePath(filePath); @@ -276,10 +277,10 @@ bool Database::saveAs(const QString& filePath, SaveAction action, bool backup, Q return ok; } -bool Database::performSave(const QString& filePath, SaveAction action, bool backup, QString* error) +bool Database::performSave(const QString& filePath, SaveAction action, const QString& backupFilePath, QString* error) { - if (backup) { - backupDatabase(filePath); + if (!backupFilePath.isNull()) { + backupDatabase(filePath, backupFilePath); } #if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0) @@ -337,7 +338,7 @@ bool Database::performSave(const QString& filePath, SaveAction action, bool back tempFile.setFileTime(createTime, QFile::FileBirthTime); #endif return true; - } else if (!backup || !restoreDatabase(filePath)) { + } else if (backupFilePath.isEmpty() || !restoreDatabase(filePath, backupFilePath)) { // Failed to copy new database in place, and // failed to restore from backup or backups disabled tempFile.setAutoRemove(false); @@ -485,23 +486,26 @@ void Database::releaseData() } /** - * Remove the old backup and replace it with a new one - * backups are named .old. + * Remove the old backup and replace it with a new one. Backup name is taken from destinationFilePath. + * Non-existing parent directories will be created automatically. * * @param filePath Path to the file to backup + * @param destinationFilePath Path to the backup destination file * @return true on success */ -bool Database::backupDatabase(const QString& filePath) +bool Database::backupDatabase(const QString& filePath, const QString& destinationFilePath) { - static auto re = QRegularExpression("(\\.[^.]+)$"); - - auto match = re.match(filePath); - auto backupFilePath = filePath; + // Ensure that the path to write to actually exists + auto parentDirectory = QFileInfo(destinationFilePath).absoluteDir(); + if (!parentDirectory.exists()) { + if (!QDir().mkpath(parentDirectory.absolutePath())) { + return false; + } + } auto perms = QFile::permissions(filePath); - backupFilePath = backupFilePath.replace(re, "") + ".old" + match.captured(1); - QFile::remove(backupFilePath); - bool res = QFile::copy(filePath, backupFilePath); - QFile::setPermissions(backupFilePath, perms); + QFile::remove(destinationFilePath); + bool res = QFile::copy(filePath, destinationFilePath); + QFile::setPermissions(destinationFilePath, perms); return res; } @@ -513,17 +517,13 @@ bool Database::backupDatabase(const QString& filePath) * @param filePath Path to the file to restore * @return true on success */ -bool Database::restoreDatabase(const QString& filePath) +bool Database::restoreDatabase(const QString& filePath, const QString& fromBackupFilePath) { - static auto re = QRegularExpression("^(.*?)(\\.[^.]+)?$"); - - auto match = re.match(filePath); auto perms = QFile::permissions(filePath); - auto backupFilePath = match.captured(1) + ".old" + match.captured(2); // Only try to restore if the backup file actually exists - if (QFile::exists(backupFilePath)) { + if (QFile::exists(fromBackupFilePath)) { QFile::remove(filePath); - if (QFile::copy(backupFilePath, filePath)) { + if (QFile::copy(fromBackupFilePath, filePath)) { return QFile::setPermissions(filePath, perms); } } diff --git a/src/core/Database.h b/src/core/Database.h index 24bb79db2..c42025f85 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -79,8 +79,11 @@ public: QSharedPointer key, QString* error = nullptr, bool readOnly = 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 save(SaveAction action = Atomic, const QString& backupFilePath = QString(), QString* error = nullptr); + bool saveAs(const QString& filePath, + SaveAction action = Atomic, + const QString& backupFilePath = QString(), + QString* error = nullptr); bool extract(QByteArray&, QString* error = nullptr); bool import(const QString& xmlExportPath, QString* error = nullptr); @@ -203,9 +206,9 @@ private: void createRecycleBin(); bool writeDatabase(QIODevice* device, QString* error = nullptr); - bool backupDatabase(const QString& filePath); - bool restoreDatabase(const QString& filePath); - bool performSave(const QString& filePath, SaveAction flags, bool backup, QString* error); + bool backupDatabase(const QString& filePath, const QString& destinationFilePath); + bool restoreDatabase(const QString& filePath, const QString& fromBackupFilePath); + bool performSave(const QString& filePath, SaveAction flags, const QString& backupFilePath, QString* error); void startModifiedTimer(); void stopModifiedTimer(); diff --git a/src/core/Tools.cpp b/src/core/Tools.cpp index 58a38f433..7c6d52a59 100644 --- a/src/core/Tools.cpp +++ b/src/core/Tools.cpp @@ -22,7 +22,10 @@ #include "config-keepassx.h" #include "git-info.h" +#include "core/Clock.h" + #include +#include #include #include #include @@ -376,4 +379,37 @@ namespace Tools } return result; } + + QString substituteBackupFilePath(QString pattern, const QString& databasePath) + { + // Fail if substitution fails + if (databasePath.isEmpty()) { + return {}; + } + + // Replace backup pattern + QFileInfo dbFileInfo(databasePath); + QString baseName = dbFileInfo.completeBaseName(); + + pattern.replace(QString("{DB_FILENAME}"), baseName); + + auto re = QRegularExpression(R"(\{TIME(?::([^\\]*))?\})"); + auto match = re.match(pattern); + while (match.hasMatch()) { + // Extract time format specifier + auto formatSpecifier = QString("dd_MM_yyyy_hh-mm-ss"); + if (!match.captured(1).isEmpty()) { + formatSpecifier = match.captured(1); + } + auto replacement = Clock::currentDateTime().toString(formatSpecifier); + pattern.replace(match.capturedStart(), match.capturedLength(), replacement); + match = re.match(pattern); + } + + // Replace escaped braces + pattern.replace("\\{", "{"); + pattern.replace("\\}", "}"); + + return pattern; + } } // namespace Tools diff --git a/src/core/Tools.h b/src/core/Tools.h index 8ebcef1c5..cf3b3593d 100644 --- a/src/core/Tools.h +++ b/src/core/Tools.h @@ -75,6 +75,8 @@ namespace Tools } QVariantMap qo2qvm(const QObject* object, const QStringList& ignoredProperties = {"objectName"}); + + QString substituteBackupFilePath(QString pattern, const QString& databasePath); } // namespace Tools #endif // KEEPASSX_TOOLS_H diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp index 100258120..ac96d23b4 100644 --- a/src/gui/ApplicationSettingsWidget.cpp +++ b/src/gui/ApplicationSettingsWidget.cpp @@ -19,6 +19,8 @@ #include "ApplicationSettingsWidget.h" #include "ui_ApplicationSettingsWidgetGeneral.h" #include "ui_ApplicationSettingsWidgetSecurity.h" +#include +#include #include "config-keepassx.h" @@ -28,6 +30,7 @@ #include "gui/MainWindow.h" #include "gui/osutils/OSUtils.h" +#include "FileDialog.h" #include "MessageBox.h" #ifdef Q_OS_MACOS #include "touchid/TouchID.h" @@ -112,6 +115,12 @@ ApplicationSettingsWidget::ApplicationSettingsWidget(QWidget* parent) connect(m_generalUi->useAlternativeSaveCheckBox, SIGNAL(toggled(bool)), m_generalUi->alternativeSaveComboBox, SLOT(setEnabled(bool))); + connect(m_generalUi->backupBeforeSaveCheckBox, SIGNAL(toggled(bool)), + m_generalUi->backupFilePath, SLOT(setEnabled(bool))); + connect(m_generalUi->backupBeforeSaveCheckBox, SIGNAL(toggled(bool)), + m_generalUi->backupFilePathPicker, SLOT(setEnabled(bool))); + connect(m_generalUi->backupFilePathPicker, SIGNAL(pressed()), SLOT(selectBackupDirectory())); + connect(m_secUi->clearClipboardCheckBox, SIGNAL(toggled(bool)), m_secUi->clearClipboardSpinBox, SLOT(setEnabled(bool))); connect(m_secUi->clearSearchCheckBox, SIGNAL(toggled(bool)), @@ -188,6 +197,9 @@ 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->backupFilePath->setText(config()->get(Config::BackupFilePathPattern).toString()); + 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()); @@ -326,6 +338,9 @@ 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::BackupFilePathPattern, m_generalUi->backupFilePath->text()); + 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()); @@ -504,3 +519,13 @@ void ApplicationSettingsWidget::checkUpdatesToggled(bool checked) { m_generalUi->checkForUpdatesIncludeBetasCheckBox->setEnabled(checked); } + +void ApplicationSettingsWidget::selectBackupDirectory() +{ + auto backupDirectory = + FileDialog::instance()->getExistingDirectory(this, tr("Select backup storage directory"), QDir::homePath()); + if (!backupDirectory.isEmpty()) { + m_generalUi->backupFilePath->setText( + QDir(backupDirectory).filePath(config()->getDefault(Config::BackupFilePathPattern).toString())); + } +} \ No newline at end of file diff --git a/src/gui/ApplicationSettingsWidget.h b/src/gui/ApplicationSettingsWidget.h index f36e5ef12..c5f2ed7e3 100644 --- a/src/gui/ApplicationSettingsWidget.h +++ b/src/gui/ApplicationSettingsWidget.h @@ -62,6 +62,7 @@ private slots: void systrayToggled(bool checked); void rememberDatabasesToggled(bool checked); void checkUpdatesToggled(bool checked); + void selectBackupDirectory(); private: QWidget* const m_secWidget; diff --git a/src/gui/ApplicationSettingsWidgetGeneral.ui b/src/gui/ApplicationSettingsWidgetGeneral.ui index 0e18dfde6..7729e7c8f 100644 --- a/src/gui/ApplicationSettingsWidgetGeneral.ui +++ b/src/gui/ApplicationSettingsWidgetGeneral.ui @@ -58,8 +58,8 @@ 0 0 - 581 - 924 + 664 + 1215 @@ -278,6 +278,52 @@ + + + + + + + 0 + 0 + + + + Backup destination + + + backupFilePath + + + + + + + false + + + Specifies the database backup file location. Occurences of "{DB_FILENAME}" are replaced with the filename of the saved database without extension. {TIME:<format>} is replaced with the backup time, see https://doc.qt.io/qt-5/qdatetime.html#toString. <format> defaults to format string "dd_MM_yyyy_hh-mm-ss". + + + {DB_FILENAME}.old.kdbx + + + + + + + false + + + Choose... + + + + + + + + diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 7a7d961ae..6d88a6613 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include "autotype/AutoType.h" #include "core/EntrySearcher.h" @@ -1879,11 +1880,31 @@ bool DatabaseWidget::performSave(QString& errorMessage, const QString& fileName) } } + QString backupFilePath; + if (config()->get(Config::BackupBeforeSave).toBool()) { + backupFilePath = config()->get(Config::BackupFilePathPattern).toString(); + // Fall back to default + if (backupFilePath.isEmpty()) { + backupFilePath = config()->getDefault(Config::BackupFilePathPattern).toString(); + } + + QFileInfo dbFileInfo(m_db->filePath()); + backupFilePath = Tools::substituteBackupFilePath(backupFilePath, dbFileInfo.canonicalFilePath()); + if (!backupFilePath.isNull()) { + // Note that we cannot guarantee that backupFilePath is actually a valid filename. QT currently provides + // no function for this. Moreover, we don't check if backupFilePath is a file and not a directory. + // If this isn't the case, just let the backup fail. + if (QDir::isRelativePath(backupFilePath)) { + backupFilePath = QDir::cleanPath(dbFileInfo.absolutePath() + QDir::separator() + backupFilePath); + } + } + } + bool ok; if (fileName.isEmpty()) { - ok = m_db->save(saveAction, config()->get(Config::BackupBeforeSave).toBool(), &errorMessage); + ok = m_db->save(saveAction, backupFilePath, &errorMessage); } else { - ok = m_db->saveAs(fileName, saveAction, config()->get(Config::BackupBeforeSave).toBool(), &errorMessage); + ok = m_db->saveAs(fileName, saveAction, backupFilePath, &errorMessage); } // Return control diff --git a/tests/TestDatabase.cpp b/tests/TestDatabase.cpp index 989d4957c..9fa09d315 100644 --- a/tests/TestDatabase.cpp +++ b/tests/TestDatabase.cpp @@ -75,29 +75,26 @@ void TestDatabase::testSave() // Test safe saves db->metadata()->setName("test"); QVERIFY(db->isModified()); - QVERIFY2(db->save(Database::Atomic, false, &error), error.toLatin1()); + QVERIFY2(db->save(Database::Atomic, QString(), &error), error.toLatin1()); QVERIFY(!db->isModified()); // Test temp-file saves db->metadata()->setName("test2"); - QVERIFY2(db->save(Database::TempFile, false, &error), error.toLatin1()); + QVERIFY2(db->save(Database::TempFile, QString(), &error), error.toLatin1()); QVERIFY(!db->isModified()); // Test direct-write saves db->metadata()->setName("test3"); - QVERIFY2(db->save(Database::DirectWrite, false, &error), error.toLatin1()); + QVERIFY2(db->save(Database::DirectWrite, QString(), &error), error.toLatin1()); QVERIFY(!db->isModified()); // Test save backups + TemporaryFile backupFile; + auto backupFilePath = backupFile.fileName(); db->metadata()->setName("test4"); - QVERIFY2(db->save(Database::Atomic, true, &error), error.toLatin1()); + QVERIFY2(db->save(Database::Atomic, backupFilePath, &error), error.toLatin1()); QVERIFY(!db->isModified()); - // Confirm backup exists and then delete it - auto re = QRegularExpression("(\\.[^.]+)$"); - auto match = re.match(tempFile.fileName()); - auto backupFilePath = tempFile.fileName(); - backupFilePath = backupFilePath.replace(re, "") + ".old" + match.captured(1); QVERIFY(QFile::exists(backupFilePath)); QFile::remove(backupFilePath); QVERIFY(!QFile::exists(backupFilePath)); @@ -123,7 +120,7 @@ void TestDatabase::testSignals() QTRY_COMPARE(spyModified.count(), 1); QSignalSpy spySaved(db.data(), SIGNAL(databaseSaved())); - QVERIFY(db->save(Database::Atomic, false, &error)); + QVERIFY(db->save(Database::Atomic, QString(), &error)); QCOMPARE(spySaved.count(), 1); // Short delay to allow file system settling to reduce test failures diff --git a/tests/TestTools.cpp b/tests/TestTools.cpp index 881434c19..edfeb5034 100644 --- a/tests/TestTools.cpp +++ b/tests/TestTools.cpp @@ -17,6 +17,8 @@ #include "TestTools.h" +#include "core/Clock.h" + #include QTEST_GUILESS_MAIN(TestTools) @@ -108,3 +110,55 @@ void TestTools::testValidUuid() QVERIFY(!Tools::isValidUuid(longUuid)); QVERIFY(!Tools::isValidUuid(nonHexUuid)); } + +void TestTools::testBackupFilePatternSubstitution_data() +{ + QTest::addColumn("pattern"); + QTest::addColumn("dbFilePath"); + QTest::addColumn("expectedSubstitution"); + + static const auto DEFAULT_DB_FILE_NAME = QStringLiteral("KeePassXC"); + static const auto DEFAULT_DB_FILE_PATH = QStringLiteral("/tmp/") + DEFAULT_DB_FILE_NAME + QStringLiteral(".kdbx"); + static const auto NOW = Clock::currentDateTime(); + auto DEFAULT_FORMATTED_TIME = NOW.toString("dd_MM_yyyy_hh-mm-ss"); + + QTest::newRow("Null pattern") << QString() << DEFAULT_DB_FILE_PATH << QString(); + QTest::newRow("Empty pattern") << QString("") << DEFAULT_DB_FILE_PATH << QString(""); + QTest::newRow("Null database path") << "valid_pattern" << QString() << QString(); + QTest::newRow("Empty database path") << "valid_pattern" << QString("") << QString(); + QTest::newRow("Unclosed/invalid pattern") << "{DB_FILENAME" << DEFAULT_DB_FILE_PATH << "{DB_FILENAME"; + QTest::newRow("Unknown pattern") << "{NO_MATCH}" << DEFAULT_DB_FILE_PATH << "{NO_MATCH}"; + QTest::newRow("Do not replace escaped patterns (filename)") + << "\\{DB_FILENAME\\}" << DEFAULT_DB_FILE_PATH << "{DB_FILENAME}"; + QTest::newRow("Do not replace escaped patterns (time)") + << "\\{TIME:dd.MM.yyyy\\}" << DEFAULT_DB_FILE_PATH << "{TIME:dd.MM.yyyy}"; + QTest::newRow("Multiple patterns should be replaced") + << "{DB_FILENAME} {TIME} {DB_FILENAME}" << DEFAULT_DB_FILE_PATH + << DEFAULT_DB_FILE_NAME + QStringLiteral(" ") + DEFAULT_FORMATTED_TIME + QStringLiteral(" ") + + DEFAULT_DB_FILE_NAME; + QTest::newRow("Default time pattern") << "{TIME}" << DEFAULT_DB_FILE_PATH << DEFAULT_FORMATTED_TIME; + QTest::newRow("Default time pattern (empty formatter)") + << "{TIME:}" << DEFAULT_DB_FILE_PATH << DEFAULT_FORMATTED_TIME; + QTest::newRow("Custom time pattern") << "{TIME:dd-ss}" << DEFAULT_DB_FILE_PATH << NOW.toString("dd-ss"); + QTest::newRow("Invalid custom time pattern") << "{TIME:dd/-ss}" << DEFAULT_DB_FILE_PATH << NOW.toString("dd/-ss"); + QTest::newRow("Recursive substitution") << "{TIME:'{TIME}'}" << DEFAULT_DB_FILE_PATH << DEFAULT_FORMATTED_TIME; + QTest::newRow("{DB_FILENAME} substitution") + << "some {DB_FILENAME} thing" << DEFAULT_DB_FILE_PATH + << QStringLiteral("some ") + DEFAULT_DB_FILE_NAME + QStringLiteral(" thing"); + QTest::newRow("{DB_FILENAME} substitution with multiple extensions") << "some {DB_FILENAME} thing" + << "/tmp/KeePassXC.kdbx.ext" + << "some KeePassXC.kdbx thing"; + // Not relevant right now, added test anyway + QTest::newRow("There should be no substitution loops") << "{DB_FILENAME}" + << "{TIME:'{DB_FILENAME}'}.ext" + << "{DB_FILENAME}"; +} + +void TestTools::testBackupFilePatternSubstitution() +{ + QFETCH(QString, pattern); + QFETCH(QString, dbFilePath); + QFETCH(QString, expectedSubstitution); + + QCOMPARE(Tools::substituteBackupFilePath(pattern, dbFilePath), expectedSubstitution); +} diff --git a/tests/TestTools.h b/tests/TestTools.h index 24554b6fd..29f2bf0f5 100644 --- a/tests/TestTools.h +++ b/tests/TestTools.h @@ -29,6 +29,8 @@ private slots: void testIsBase64(); void testEnvSubstitute(); void testValidUuid(); + void testBackupFilePatternSubstitution_data(); + void testBackupFilePatternSubstitution(); }; #endif // KEEPASSX_TESTTOOLS_H diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index 2a669c41d..e6c0c3e12 100644 --- a/tests/gui/TestGui.cpp +++ b/tests/gui/TestGui.cpp @@ -82,22 +82,10 @@ int main(int argc, char* argv[]) return QTest::qExec(&tc, argc, argv); } -static QString dbFileName = QStringLiteral(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"); - void TestGui::initTestCase() { QVERIFY(Crypto::init()); Config::createTempFileInstance(); - // Disable autosave so we can test the modified file indicator - config()->set(Config::AutoSaveAfterEveryChange, false); - config()->set(Config::AutoSaveOnExit, false); - // Enable the tray icon so we can test hiding/restoring the windowQByteArray - config()->set(Config::GUI_ShowTrayIcon, true); - // Disable advanced settings mode (activate within individual tests to test advanced settings) - config()->set(Config::GUI_AdvancedSettings, false); - // Disable the update check first time alert - config()->set(Config::UpdateCheckMessageShown, true); - Application::bootstrap(); m_mainWindow.reset(new MainWindow()); @@ -106,11 +94,22 @@ void TestGui::initTestCase() m_mainWindow->resize(1024, 768); } -// Every test starts with opening the temp database +// Every test starts with resetting config settings and opening the temp database void TestGui::init() { + // Reset config to defaults + config()->resetToDefaults(); + // Disable autosave so we can test the modified file indicator + config()->set(Config::AutoSaveAfterEveryChange, false); + config()->set(Config::AutoSaveOnExit, false); + // Enable the tray icon so we can test hiding/restoring the windowQByteArray + config()->set(Config::GUI_ShowTrayIcon, true); + // Disable the update check first time alert + config()->set(Config::UpdateCheckMessageShown, true); + // Copy the test database file to the temporary file - QVERIFY(m_dbFile.copyFromFile(dbFileName)); + auto origFilePath = QDir(KEEPASSX_TEST_DATA_DIR).absoluteFilePath("NewDatabase.kdbx"); + QVERIFY(m_dbFile.copyFromFile(origFilePath)); m_dbFileName = QFileInfo(m_dbFile.fileName()).fileName(); m_dbFilePath = m_dbFile.fileName(); @@ -354,6 +353,8 @@ void TestGui::testAutoreloadDatabase() cleanup(); init(); + config()->set(Config::AutoReloadOnChange, false); + // Test rejecting new file in autoreload MessageBox::setNextAnswer(MessageBox::No); // Overwrite the current database with the temp data @@ -1277,11 +1278,76 @@ void TestGui::testSave() QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testSave*")); triggerAction("actionDatabaseSave"); - QCOMPARE(m_tabWidget->tabName(m_tabWidget->currentIndex()), QString("testSave")); + QTRY_COMPARE(m_tabWidget->tabName(m_tabWidget->currentIndex()), QString("testSave")); checkDatabase(); } +void TestGui::testSaveBackupPath_data() +{ + QTest::addColumn("backupFilePathPattern"); + QTest::addColumn("expectedBackupFile"); + + // Absolute paths should remain absolute + TemporaryFile tmpFile; + QVERIFY(tmpFile.open()); + tmpFile.remove(); + + QTest::newRow("Absolute backup path") << tmpFile.fileName() << tmpFile.fileName(); + // relative paths should be resolved to database parent directory + QTest::newRow("Relative backup path (implicit)") << "other_dir/test.old.kdbx" + << "other_dir/test.old.kdbx"; + QTest::newRow("Relative backup path (explicit)") << "./other_dir2/test2.old.kdbx" + << "other_dir2/test2.old.kdbx"; + + QTest::newRow("Path with placeholders") << "{DB_FILENAME}.old.kdbx" + << "KeePassXC.old.kdbx"; + // empty path should be replaced with default pattern + QTest::newRow("Empty path") << QString("") << config()->getDefault(Config::BackupFilePathPattern).toString(); + // {DB_FILENAME} should be replaced with database filename + QTest::newRow("") << "{DB_FILENAME}_.old.kdbx" + << "{DB_FILENAME}_.old.kdbx"; +} + +void TestGui::testSaveBackupPath() +{ + /** + * Tests that the backupFilePathPattern config entry is respected. We do not test patterns like {TIME} etc here + * as this is done in a separate test case. We do however check {DB_FILENAME} as this is a feature of the + * performBackup() function. + */ + + // Get test data + QFETCH(QString, backupFilePathPattern); + QFETCH(QString, expectedBackupFile); + + // Enable automatic backups + config()->set(Config::BackupBeforeSave, true); + config()->set(Config::BackupFilePathPattern, backupFilePathPattern); + + // Replace placeholders and resolve relative paths. This cannot be done in the _data() function as the + // db path/filename is not known yet + auto dbFileInfo = QFileInfo(m_dbFilePath); + if (!QDir::isAbsolutePath(expectedBackupFile)) { + expectedBackupFile = QDir(dbFileInfo.absolutePath()).absoluteFilePath(expectedBackupFile); + } + expectedBackupFile.replace("{DB_FILENAME}", dbFileInfo.completeBaseName()); + + // Save a modified database + auto prevName = m_db->metadata()->name(); + m_db->metadata()->setName("testBackupPathPattern"); + + QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern*")); + triggerAction("actionDatabaseSave"); + QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern")); + + // Test that the backup file has the previous database name + checkDatabase(expectedBackupFile, prevName); + + // Clean up + QFile(expectedBackupFile).remove(); +} + void TestGui::testDatabaseSettings() { m_db->metadata()->setName("testDatabaseSettings"); @@ -1301,7 +1367,7 @@ void TestGui::testDatabaseSettings() QCOMPARE(m_db->kdf()->rounds(), 123456); triggerAction("actionDatabaseSave"); - QCOMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testDatabaseSettings")); + QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testDatabaseSettings")); advancedToggle->setChecked(false); QApplication::processEvents(); @@ -1365,6 +1431,8 @@ void TestGui::testDragAndDropKdbxFiles() const int openedDatabasesCount = m_tabWidget->count(); const QString badDatabaseFilePath(QString(KEEPASSX_TEST_DATA_DIR).append("/NotDatabase.notkdbx")); + const QString goodDatabaseFilePath(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx")); + QMimeData badMimeData; badMimeData.setUrls({QUrl::fromLocalFile(badDatabaseFilePath)}); QDragEnterEvent badDragEvent(QPoint(1, 1), Qt::LinkAction, &badMimeData, Qt::LeftButton, Qt::NoModifier); @@ -1378,7 +1446,7 @@ void TestGui::testDragAndDropKdbxFiles() QCOMPARE(m_tabWidget->count(), openedDatabasesCount); QMimeData goodMimeData; - goodMimeData.setUrls({QUrl::fromLocalFile(dbFileName)}); + goodMimeData.setUrls({QUrl::fromLocalFile(goodDatabaseFilePath)}); QDragEnterEvent goodDragEvent(QPoint(1, 1), Qt::LinkAction, &goodMimeData, Qt::LeftButton, Qt::NoModifier); qApp->notify(m_mainWindow.data(), &goodDragEvent); QCOMPARE(goodDragEvent.isAccepted(), true); @@ -1700,17 +1768,18 @@ void TestGui::addCannedEntries() QTest::mouseClick(editEntryWidgetButtonBox->button(QDialogButtonBox::Ok), Qt::LeftButton); } -void TestGui::checkDatabase(QString dbFileName) +void TestGui::checkDatabase(const QString& filePath, const QString& expectedDbName) { - if (dbFileName.isEmpty()) { - dbFileName = m_dbFilePath; - } - auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create("a")); auto dbSaved = QSharedPointer::create(); - QVERIFY(dbSaved->open(dbFileName, key, nullptr, false)); - QCOMPARE(dbSaved->metadata()->name(), m_db->metadata()->name()); + QVERIFY(dbSaved->open(filePath, key, nullptr, false)); + QCOMPARE(dbSaved->metadata()->name(), expectedDbName); +} + +void TestGui::checkDatabase(const QString& filePath) +{ + checkDatabase(filePath.isEmpty() ? m_dbFilePath : filePath, m_db->metadata()->name()); } void TestGui::triggerAction(const QString& name) diff --git a/tests/gui/TestGui.h b/tests/gui/TestGui.h index 0185c544d..f9dcf8867 100644 --- a/tests/gui/TestGui.h +++ b/tests/gui/TestGui.h @@ -57,6 +57,8 @@ private slots: void testSaveAs(); void testSaveBackup(); void testSave(); + void testSaveBackupPath(); + void testSaveBackupPath_data(); void testDatabaseSettings(); void testKeePass1Import(); void testDatabaseLocking(); @@ -67,7 +69,8 @@ private slots: private: void addCannedEntries(); - void checkDatabase(QString dbFileName = ""); + void checkDatabase(const QString& filePath, const QString& expectedDbName); + void checkDatabase(const QString& filePath = {}); void triggerAction(const QString& name); void dragAndDropGroup(const QModelIndex& sourceIndex, const QModelIndex& targetIndex,