Code cleanup

keepassxreboot · Nov 7, 2021 · 0ca3638 · 0ca3638
1 parent be47fbb
commit 0ca3638
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 98 deletions.
diff --git a/src/core/Tools.cpp b/src/core/Tools.cpp
@@ -22,7 +22,8 @@
 #include "config-keepassx.h"
 #include "git-info.h"
 
-#include <QDateTime>
+#include "core/Clock.h"
+
 #include <QElapsedTimer>
 #include <QFileInfo>
 #include <QImageReader>
@@ -379,12 +380,12 @@ namespace Tools
         return result;
     }
 
-    QString
-    substituteBackupFilePathPattern(QString pattern, const QString& databasePath, QDateTime date, int maxSubstitutions)
+    QString substituteBackupFilePath(QString pattern, const QString& databasePath)
     {
         // Fail if substitution fails
-        if (databasePath.isEmpty())
+        if (databasePath.isEmpty()) {
             return {};
+        }
 
         // Replace backup pattern
         QFileInfo dbFileInfo(databasePath);
@@ -393,18 +394,16 @@ namespace Tools
         pattern.replace(QString("{DB_FILENAME}"), baseName);
 
         auto re = QRegularExpression(R"(\{TIME(?::([^\\]*))?\})");
-        // "Detect" loops
-        auto num_substitutions = 0;
-        for (auto matches = re.globalMatch(pattern); matches.hasNext() && num_substitutions < maxSubstitutions;
-             matches = re.globalMatch(pattern), ++num_substitutions) {
-            auto match = matches.next();
+        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 = date.toString(formatSpecifier);
+            auto replacement = Clock::currentDateTime().toString(formatSpecifier);
             pattern.replace(match.capturedStart(), match.capturedLength(), replacement);
+            match = re.match(pattern);
         }
 
         // Replace escaped braces

diff --git a/src/core/Tools.h b/src/core/Tools.h
@@ -21,7 +21,6 @@
 
 #include "core/Global.h"
 
-#include <QDateTime>
 #include <QProcessEnvironment>
 
 class QIODevice;
@@ -77,10 +76,7 @@ namespace Tools
 
     QVariantMap qo2qvm(const QObject* object, const QStringList& ignoredProperties = {"objectName"});
 
-    QString substituteBackupFilePathPattern(QString pattern,
-                                            const QString& databasePath,
-                                            QDateTime date = QDateTime::currentDateTime(),
-                                            int maxSubstitutions = 100);
+    QString substituteBackupFilePath(QString pattern, const QString& databasePath);
 } // namespace Tools
 
 #endif // KEEPASSX_TOOLS_H
diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp
@@ -1889,7 +1889,7 @@ bool DatabaseWidget::performSave(QString& errorMessage, const QString& fileName)
         }
 
         QFileInfo dbFileInfo(m_db->filePath());
-        backupFilePath = Tools::substituteBackupFilePathPattern(backupFilePath, dbFileInfo.canonicalFilePath());
+        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.

diff --git a/tests/TestTools.cpp b/tests/TestTools.cpp
@@ -17,6 +17,8 @@
 
 #include "TestTools.h"
 
+#include "core/Clock.h"
+
 #include <QTest>
 
 QTEST_GUILESS_MAIN(TestTools)
@@ -113,63 +115,50 @@ void TestTools::testBackupFilePatternSubstitution_data()
 {
     QTest::addColumn<QString>("pattern");
     QTest::addColumn<QString>("dbFilePath");
-    QTest::addColumn<QDateTime>("date");
-    QTest::addColumn<int>("maxSubstitutions");
     QTest::addColumn<QString>("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 = QDateTime::currentDateTime();
+    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 << NOW << 100 << QString();
-    QTest::newRow("Empty pattern") << QString("") << DEFAULT_DB_FILE_PATH << NOW << 100 << QString("");
-    QTest::newRow("Null database path") << "valid_pattern" << QString() << NOW << 100 << QString();
-    QTest::newRow("Empty database path") << "valid_pattern" << QString("") << NOW << 100 << QString();
-    QTest::newRow("Unclosed/invalid pattern") << "{DB_FILENAME" << DEFAULT_DB_FILE_PATH << NOW << 100 << "{DB_FILENAME";
-    QTest::newRow("Unknown pattern") << "{NO_MATCH}" << DEFAULT_DB_FILE_PATH << NOW << 100 << "{NO_MATCH}";
+    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 << NOW << 100 << "{DB_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 << NOW << 100 << "{TIME:dd.MM.yyyy}";
+        << "\\{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 << NOW << 100
+        << "{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 << NOW << 100 << DEFAULT_FORMATTED_TIME;
+    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 << NOW << 100 << DEFAULT_FORMATTED_TIME;
-    QTest::newRow("Custom time pattern") << "{TIME:dd-ss}" << DEFAULT_DB_FILE_PATH << NOW << 100
-                                         << NOW.toString("dd-ss");
-    QTest::newRow("Invalid custom time pattern")
-        << "{TIME:dd/-ss}" << DEFAULT_DB_FILE_PATH << NOW << 100 << NOW.toString("dd/-ss");
-    QTest::newRow("Recursive substitution")
-        << "{TIME:'{TIME}'}" << DEFAULT_DB_FILE_PATH << NOW << 100 << DEFAULT_FORMATTED_TIME;
-    QTest::newRow("Substitution limit is respected")
-        << "{TIME} {TIME} {TIME}" << DEFAULT_DB_FILE_PATH << NOW << 2
-        << DEFAULT_FORMATTED_TIME + QStringLiteral(" ") + DEFAULT_FORMATTED_TIME + QStringLiteral(" {TIME}");
-    QTest::newRow("Substitution limit is respected (recursive)")
-        << "{TIME} {TIME} {TIME:'{TIME}'}" << DEFAULT_DB_FILE_PATH << NOW << 3
-        << DEFAULT_FORMATTED_TIME + QStringLiteral(" ") + DEFAULT_FORMATTED_TIME + QStringLiteral(" {TIME}");
+        << "{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 << NOW << 100
+        << "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" << NOW << 100 << "some KeePassXC.kdbx 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" << NOW << 100 << "{DB_FILENAME}";
+    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(QDateTime, date);
-    QFETCH(int, maxSubstitutions);
     QFETCH(QString, expectedSubstitution);
 
-    QCOMPARE(Tools::substituteBackupFilePathPattern(pattern, dbFilePath, date, maxSubstitutions), expectedSubstitution);
+    QCOMPARE(Tools::substituteBackupFilePath(pattern, dbFilePath), expectedSubstitution);
 }
diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp
@@ -82,9 +82,6 @@ int main(int argc, char* argv[])
     return QTest::qExec(&tc, argc, argv);
 }
 
-const static QString TEST_DB_FILE_NAME = "NewDatabase.kdbx";
-static QString dbFileName = QDir(KEEPASSX_TEST_DATA_DIR).filePath(TEST_DB_FILE_NAME);
-
 void TestGui::initTestCase()
 {
     QVERIFY(Crypto::init());
@@ -97,25 +94,22 @@ void TestGui::initTestCase()
     m_mainWindow->resize(1024, 768);
 }
 
-// Every test starts with reseting config settings and 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);
-    config()->set(Config::AutoReloadOnChange, true);
     // 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);
-    // Reset backup settings
-    config()->set(Config::BackupBeforeSave, false);
-    config()->set(Config::BackupFilePathPattern, config()->getDefault(Config::BackupFilePathPattern));
 
     // 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();
@@ -1284,7 +1278,7 @@ 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();
 }
@@ -1295,8 +1289,11 @@ void TestGui::testSaveBackupPath_data()
     QTest::addColumn<QString>("expectedBackupFile");
 
     // Absolute paths should remain absolute
-    TemporaryFile safe_abs_path;
-    QTest::newRow("Absolute backup path") << safe_abs_path.fileName() << safe_abs_path.fileName();
+    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";
@@ -1320,40 +1317,31 @@ void TestGui::testSaveBackupPath()
      * performBackup() function.
      */
 
-    // Set the database name to something we control
-    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"));
-
-    // Enable automatic backups
-    config()->set(Config::BackupBeforeSave, true);
-
     // 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 dbFilePath = QFileInfo(m_dbFilePath).absolutePath();
+    auto dbFileInfo = QFileInfo(m_dbFilePath);
     if (!QDir::isAbsolutePath(expectedBackupFile)) {
-        expectedBackupFile = QDir(dbFilePath).absoluteFilePath(expectedBackupFile);
+        expectedBackupFile = QDir(dbFileInfo.absolutePath()).absoluteFilePath(expectedBackupFile);
     }
-    expectedBackupFile.replace("{DB_FILENAME}", QFileInfo(m_dbFileName).completeBaseName());
-
-    // Modify the config
-    config()->set(Config::BackupFilePathPattern, backupFilePathPattern);
+    expectedBackupFile.replace("{DB_FILENAME}", dbFileInfo.completeBaseName());
 
     // Save a modified database
     auto prevName = m_db->metadata()->name();
-    m_db->metadata()->setName("testBackupPathPattern_modified");
+    m_db->metadata()->setName("testBackupPathPattern");
 
-    // wait for the modification to occur
-    QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern_modified*"));
+    QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern*"));
     triggerAction("actionDatabaseSave");
-    QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern_modified"));
+    QTRY_COMPARE(m_tabWidget->tabText(m_tabWidget->currentIndex()), QString("testBackupPathPattern"));
 
-    // Test if file has been modified/exists
+    // Test that the backup file has the previous database name
     checkDatabase(expectedBackupFile, prevName);
 
     // Clean up
@@ -1379,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();
@@ -1443,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);
@@ -1456,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);
@@ -1778,22 +1768,18 @@ void TestGui::addCannedEntries()
     QTest::mouseClick(editEntryWidgetButtonBox->button(QDialogButtonBox::Ok), Qt::LeftButton);
 }
 
-void TestGui::checkDatabase(const QString& dbFileName, const QString& expectedDbName)
+void TestGui::checkDatabase(const QString& filePath, const QString& expectedDbName)
 {
     auto key = QSharedPointer<CompositeKey>::create();
     key->addKey(QSharedPointer<PasswordKey>::create("a"));
     auto dbSaved = QSharedPointer<Database>::create();
-    QVERIFY(dbSaved->open(dbFileName, key, nullptr, false));
+    QVERIFY(dbSaved->open(filePath, key, nullptr, false));
     QCOMPARE(dbSaved->metadata()->name(), expectedDbName);
 }
 
-void TestGui::checkDatabase(QString dbFileName)
+void TestGui::checkDatabase(const QString& filePath)
 {
-    if (dbFileName.isEmpty()) {
-        dbFileName = m_dbFilePath;
-    }
-
-    checkDatabase(dbFileName, m_db->metadata()->name());
+    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
@@ -69,8 +69,8 @@ private slots:
 
 private:
     void addCannedEntries();
-    void checkDatabase(const QString& dbFileName, const QString& expectedDbName);
-    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,