Improve Database and CLI tests

src/core/Database.cpp

@@ -54,7 +54,7 @@ Database::Database()
 
     s_uuidMap.insert(m_uuid, this);
 
-    connect(m_metadata, SIGNAL(metadataModified()), this, SLOT(markAsModified()));
+    connect(m_metadata, SIGNAL(metadataModified()), SLOT(markAsModified()));
     connect(m_timer, SIGNAL(timeout()), SIGNAL(databaseModified()));
     connect(this, SIGNAL(databaseOpened()), SLOT(updateCommonUsernames()));
     connect(this, SIGNAL(databaseSaved()), SLOT(updateCommonUsernames()));
@@ -189,7 +189,7 @@ bool Database::save(QString* error, bool atomic, bool backup)
 /**
  * Save the database to a specific file.
  *
- * If atmoic is false, this function uses QTemporaryFile instead of QSaveFile
+ * If atomic is false, 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, Google Drive,
  * or OneDrive.

tests/TestCli.cpp

@@ -144,36 +144,32 @@ void TestCli::init()
 
     m_stdinFile.reset(new TemporaryFile());
     m_stdinFile->open();
-    m_stdinHandle = fdopen(m_stdinFile->handle(), "r+");
+    Utils::STDIN = fdopen(m_stdinFile->handle(), "r+");
-    Utils::STDIN = m_stdinHandle;
 
     m_stdoutFile.reset(new TemporaryFile());
     m_stdoutFile->open();
-    m_stdoutHandle = fdopen(m_stdoutFile->handle(), "r+");
+    Utils::STDOUT = fdopen(m_stdoutFile->handle(), "r+");
-    Utils::STDOUT = m_stdoutHandle;
 
     m_stderrFile.reset(new TemporaryFile());
     m_stderrFile->open();
-    m_stderrHandle = fdopen(m_stderrFile->handle(), "r+");
+    Utils::STDERR = fdopen(m_stderrFile->handle(), "r+");
-    Utils::STDERR = m_stderrHandle;
 }
 
 void TestCli::cleanup()
 {
     m_dbFile.reset();
-
     m_dbFile2.reset();
+    m_keyFileProtectedDbFile.reset();
+    m_keyFileProtectedNoPasswordDbFile.reset();
+    m_yubiKeyProtectedDbFile.reset();
 
     m_stdinFile.reset();
-    m_stdinHandle = stdin;
     Utils::STDIN = stdin;
 
     m_stdoutFile.reset();
     Utils::STDOUT = stdout;
-    m_stdoutHandle = stdout;
 
     m_stderrFile.reset();
-    m_stderrHandle = stderr;
     Utils::STDERR = stderr;
 }
 
@@ -184,8 +180,8 @@ void TestCli::cleanupTestCase()
 QSharedPointer<Database> TestCli::readTestDatabase() const
 {
     Utils::Test::setNextPassword("a");
-    auto db = QSharedPointer<Database>(Utils::unlockDatabase(m_dbFile->fileName(), true, "", "", m_stdoutHandle));
+    auto db = QSharedPointer<Database>(Utils::unlockDatabase(m_dbFile->fileName(), true, "", "", Utils::STDOUT));
-    m_stdoutFile->seek(ftell(m_stdoutHandle)); // re-synchronize handles
+    m_stdoutFile->seek(ftell(Utils::STDOUT)); // re-synchronize handles
     return db;
 }
 
@@ -282,17 +278,25 @@ void TestCli::testAdd()
     addCmd.execute({"add", "-q", "-u", "newuser", "-g", "-L", "20", m_dbFile->fileName(), "/newentry-quiet"});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(posErr);
-    QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
     QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
     db = readTestDatabase();
     entry = db->rootGroup()->findEntryByPath("/newentry-quiet");
     QVERIFY(entry);
     QCOMPARE(entry->password().size(), 20);
 
+    pos = m_stdoutFile->pos();
+    posErr = m_stderrFile->pos();
     Utils::Test::setNextPassword("a");
     Utils::Test::setNextPassword("newpassword");
     addCmd.execute(
         {"add", "-u", "newuser2", "--url", "https://example.net/", "-p", m_dbFile->fileName(), "/newuser-entry2"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(posErr);
+    m_stdoutFile->readLine(); // skip password prompt
+    m_stdoutFile->readLine(); // skip password input
+    QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Successfully added entry newuser-entry2.\n"));
 
     db = readTestDatabase();
     entry = db->rootGroup()->findEntryByPath("/newuser-entry2");
@@ -309,8 +313,8 @@ void TestCli::testAdd()
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(posErr);
     m_stdoutFile->readLine(); // skip password prompt
-    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Successfully added entry newuser-entry3.\n"));
     QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Successfully added entry newuser-entry3.\n"));
 
     db = readTestDatabase();
     entry = db->rootGroup()->findEntryByPath("/newuser-entry3");
@@ -339,8 +343,8 @@ void TestCli::testAdd()
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(posErr);
     m_stdoutFile->readLine(); // skip password prompt
-    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Successfully added entry newuser-entry4.\n"));
     QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Successfully added entry newuser-entry4.\n"));
 
     db = readTestDatabase();
     entry = db->rootGroup()->findEntryByPath("/newuser-entry4");
@@ -541,7 +545,7 @@ void TestCli::testCreate()
 
     QScopedPointer<QTemporaryDir> testDir(new QTemporaryDir());
 
-    QString databaseFilename = testDir->path() + "testCreate1.kdbx";
+    QString databaseFilename = testDir->path() + "/testCreate1.kdbx";
     // Password
     Utils::Test::setNextPassword("a");
     createCmd.execute({"create", databaseFilename});
@@ -568,8 +572,8 @@ void TestCli::testCreate()
     QCOMPARE(m_stderrFile->readAll(), errorMessage.toUtf8());
 
     // Testing with keyfile creation
-    QString databaseFilename2 = testDir->path() + "testCreate2.kdbx";
+    QString databaseFilename2 = testDir->path() + "/testCreate2.kdbx";
-    QString keyfilePath = testDir->path() + "keyfile.txt";
+    QString keyfilePath = testDir->path() + "/keyfile.txt";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
     Utils::Test::setNextPassword("a");
@@ -586,7 +590,7 @@ void TestCli::testCreate()
     QVERIFY(db2);
 
     // Testing with existing keyfile
-    QString databaseFilename3 = testDir->path() + "testCreate3.kdbx";
+    QString databaseFilename3 = testDir->path() + "/testCreate3.kdbx";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
     Utils::Test::setNextPassword("a");

tests/TestCli.h

@@ -91,9 +91,6 @@ private:
     QScopedPointer<TemporaryFile> m_stdoutFile;
     QScopedPointer<TemporaryFile> m_stderrFile;
     QScopedPointer<TemporaryFile> m_stdinFile;
-    FILE* m_stdoutHandle = stdout;
-    FILE* m_stderrHandle = stderr;
-    FILE* m_stdinHandle = stdin;
 };
 
 #endif // KEEPASSXC_TESTCLI_H

tests/TestDatabase.cpp

@@ -23,7 +23,6 @@
 
 #include "config-keepassx-tests.h"
 #include "core/Metadata.h"
-#include "core/Tools.h"
 #include "crypto/Crypto.h"
 #include "format/KeePass2Writer.h"
 #include "keys/PasswordKey.h"
@@ -31,6 +30,8 @@
 
 QTEST_GUILESS_MAIN(TestDatabase)
 
+static QString dbFileName = QStringLiteral(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx");
+
 void TestDatabase::initTestCase()
 {
     QVERIFY(Crypto::init());
@@ -45,7 +46,7 @@ void TestDatabase::testOpen()
     auto key = QSharedPointer<CompositeKey>::create();
     key->addKey(QSharedPointer<PasswordKey>::create("a"));
 
-    bool ok = db->open(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"), key);
+    bool ok = db->open(dbFileName, key);
     QVERIFY(ok);
 
     QVERIFY(db->isInitialized());
@@ -57,16 +58,8 @@ void TestDatabase::testOpen()
 
 void TestDatabase::testSave()
 {
-    QByteArray data;
-    QFile sourceDbFile(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"));
-    QVERIFY(sourceDbFile.open(QIODevice::ReadOnly));
-    QVERIFY(Tools::readAllFromDevice(&sourceDbFile, data));
-    sourceDbFile.close();
-
     TemporaryFile tempFile;
-    QVERIFY(tempFile.open());
+    QVERIFY(tempFile.copyFromFile(dbFileName));
-    QCOMPARE(tempFile.write(data), static_cast<qint64>((data.size())));
-    tempFile.close();
 
     auto db = QSharedPointer<Database>::create();
     auto key = QSharedPointer<CompositeKey>::create();
@@ -79,15 +72,63 @@ void TestDatabase::testSave()
     // Test safe saves
     db->metadata()->setName("test");
     QVERIFY(db->isModified());
-
-    // Test unsafe saves
-    QVERIFY2(db->save(&error, false, false), error.toLatin1());
-
     QVERIFY2(db->save(&error), error.toLatin1());
     QVERIFY(!db->isModified());
 
+    // Test unsafe saves
+    db->metadata()->setName("test2");
+    QVERIFY2(db->save(&error, false, false), error.toLatin1());
+    QVERIFY(!db->isModified());
+
     // Test save backups
+    db->metadata()->setName("test3");
     QVERIFY2(db->save(&error, true, true), 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));
+}
+
+void TestDatabase::testSignals()
+{
+    TemporaryFile tempFile;
+    QVERIFY(tempFile.copyFromFile(dbFileName));
+
+    auto db = QSharedPointer<Database>::create();
+    auto key = QSharedPointer<CompositeKey>::create();
+    key->addKey(QSharedPointer<PasswordKey>::create("a"));
+
+    QSignalSpy spyFilePathChanged(db.data(), SIGNAL(filePathChanged(const QString&, const QString&)));
+    QString error;
+    bool ok = db->open(tempFile.fileName(), key, &error);
+    QVERIFY(ok);
+    QCOMPARE(spyFilePathChanged.count(), 1);
+
+    QSignalSpy spyModified(db.data(), SIGNAL(databaseModified()));
+    db->metadata()->setName("test1");
+    QTRY_COMPARE(spyModified.count(), 1);
+
+    QSignalSpy spySaved(db.data(), SIGNAL(databaseSaved()));
+    QVERIFY(db->save(&error));
+    QCOMPARE(spySaved.count(), 1);
+
+    QSignalSpy spyFileChanged(db.data(), SIGNAL(databaseFileChanged()));
+    QVERIFY(tempFile.copyFromFile(dbFileName));
+    QTRY_COMPARE(spyFileChanged.count(), 1);
+    QTRY_VERIFY(!db->isModified());
+
+    db->metadata()->setName("test2");
+    QTRY_VERIFY(db->isModified());
+
+    QSignalSpy spyDiscarded(db.data(), SIGNAL(databaseDiscarded()));
+    QVERIFY(db->open(tempFile.fileName(), key, &error));
+    QCOMPARE(spyDiscarded.count(), 1);
 }
 
 void TestDatabase::testEmptyRecycleBinOnDisabled()

tests/TestDatabase.h

@@ -29,6 +29,7 @@ private slots:
     void initTestCase();
     void testOpen();
     void testSave();
+    void testSignals();
     void testEmptyRecycleBinOnDisabled();
     void testEmptyRecycleBinOnNotCreated();
     void testEmptyRecycleBinOnEmpty();

tests/gui/TestGui.cpp

@@ -75,6 +75,8 @@
 
 QTEST_MAIN(TestGui)
 
+static QString dbFileName = QStringLiteral(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx");
+
 void TestGui::initTestCase()
 {
     QVERIFY(Crypto::init());
@@ -91,26 +93,19 @@ void TestGui::initTestCase()
 
     m_mainWindow.reset(new MainWindow());
     Bootstrap::restoreMainWindowState(*m_mainWindow);
+    Bootstrap::bootstrapApplication();
     m_tabWidget = m_mainWindow->findChild<DatabaseTabWidget*>("tabWidget");
     m_mainWindow->show();
-
-    // Load the NewDatabase.kdbx file into temporary storage
-    QFile sourceDbFile(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"));
-    QVERIFY(sourceDbFile.open(QIODevice::ReadOnly));
-    QVERIFY(Tools::readAllFromDevice(&sourceDbFile, m_dbData));
-    sourceDbFile.close();
 }
 
 // Every test starts with opening the temp database
 void TestGui::init()
 {
-    m_dbFile.reset(new TemporaryFile());
+    // Copy the test database file to the temporary file
-    // Write the temp storage to a temp database file for use in our tests
+    QVERIFY(m_dbFile.copyFromFile(dbFileName));
-    QVERIFY(m_dbFile->open());
+
-    QCOMPARE(m_dbFile->write(m_dbData), static_cast<qint64>((m_dbData.size())));
+    m_dbFileName = QFileInfo(m_dbFile.fileName()).fileName();
-    m_dbFileName = QFileInfo(m_dbFile->fileName()).fileName();
+    m_dbFilePath = m_dbFile.fileName();
-    m_dbFilePath = m_dbFile->fileName();
-    m_dbFile->close();
 
     // make sure window is activated or focus tests may fail
     m_mainWindow->activateWindow();
@@ -145,13 +140,11 @@ void TestGui::cleanup()
     if (m_dbWidget) {
         delete m_dbWidget;
     }
-
-    m_dbFile->remove();
 }
 
 void TestGui::cleanupTestCase()
 {
-    m_dbFile->remove();
+    m_dbFile.remove();
 }
 
 void TestGui::testSettingsDefaultTabOrder()
@@ -333,19 +326,10 @@ void TestGui::testAutoreloadDatabase()
 {
     config()->set("AutoReloadOnChange", false);
 
-    // Load the MergeDatabase.kdbx file into temporary storage
-    QByteArray unmodifiedMergeDatabase;
-    QFile mergeDbFile(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx"));
-    QVERIFY(mergeDbFile.open(QIODevice::ReadOnly));
-    QVERIFY(Tools::readAllFromDevice(&mergeDbFile, unmodifiedMergeDatabase));
-    mergeDbFile.close();
-
     // Test accepting new file in autoreload
     MessageBox::setNextAnswer(MessageBox::Yes);
     // Overwrite the current database with the temp data
-    QVERIFY(m_dbFile->open());
+    QVERIFY(m_dbFile.copyFromFile(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx")));
-    QVERIFY(m_dbFile->write(unmodifiedMergeDatabase, static_cast<qint64>(unmodifiedMergeDatabase.size())));
-    m_dbFile->close();
 
     QTRY_VERIFY(m_db != m_dbWidget->database());
     m_db = m_dbWidget->database();
@@ -360,10 +344,8 @@ void TestGui::testAutoreloadDatabase()
 
     // Test rejecting new file in autoreload
     MessageBox::setNextAnswer(MessageBox::No);
-    // Overwrite the current temp database with a new file
+    // Overwrite the current database with the temp data
-    QVERIFY(m_dbFile->open());
+    QVERIFY(m_dbFile.copyFromFile(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx")));
-    QVERIFY(m_dbFile->write(unmodifiedMergeDatabase, static_cast<qint64>(unmodifiedMergeDatabase.size())));
-    m_dbFile->close();
 
     // Ensure the merge did not take place
     QCOMPARE(m_db->rootGroup()->findChildByName("General")->entries().size(), 0);
@@ -382,9 +364,7 @@ void TestGui::testAutoreloadDatabase()
     // This is saying yes to merging the entries
     MessageBox::setNextAnswer(MessageBox::Merge);
     // Overwrite the current database with the temp data
-    QVERIFY(m_dbFile->open());
+    QVERIFY(m_dbFile.copyFromFile(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx")));
-    QVERIFY(m_dbFile->write(unmodifiedMergeDatabase, static_cast<qint64>(unmodifiedMergeDatabase.size())));
-    m_dbFile->close();
 
     QTRY_VERIFY(m_db != m_dbWidget->database());
     m_db = m_dbWidget->database();
@@ -1279,9 +1259,8 @@ void TestGui::testDragAndDropKdbxFiles()
 
     QCOMPARE(m_tabWidget->count(), openedDatabasesCount);
 
-    const QString goodDatabaseFilePath(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"));
     QMimeData goodMimeData;
-    goodMimeData.setUrls({QUrl::fromLocalFile(goodDatabaseFilePath)});
+    goodMimeData.setUrls({QUrl::fromLocalFile(dbFileName)});
     QDragEnterEvent goodDragEvent(QPoint(1, 1), Qt::LinkAction, &goodMimeData, Qt::LeftButton, Qt::NoModifier);
     qApp->notify(m_mainWindow.data(), &goodDragEvent);
     QCOMPARE(goodDragEvent.isAccepted(), true);

tests/gui/TestGui.h

@@ -91,8 +91,7 @@ private:
     QPointer<DatabaseTabWidget> m_tabWidget;
     QPointer<DatabaseWidget> m_dbWidget;
     QSharedPointer<Database> m_db;
-    QByteArray m_dbData;
+    TemporaryFile m_dbFile;
-    QScopedPointer<TemporaryFile> m_dbFile;
     QString m_dbFileName;
     QString m_dbFilePath;
 };

tests/util/TemporaryFile.cpp

@@ -17,7 +17,7 @@
 
 #include "TemporaryFile.h"
 
-#ifdef Q_OS_WIN
+#include <QTextStream>
 
 TemporaryFile::TemporaryFile()
     : TemporaryFile(nullptr)
@@ -47,9 +47,35 @@ TemporaryFile::TemporaryFile(const QString& templateName, QObject* parent)
     tmp.close();
 }
 
+TemporaryFile::~TemporaryFile()
+{
+    remove();
+}
+
 bool TemporaryFile::open()
 {
     return QFile::open(QIODevice::ReadWrite);
 }
 
-#endif
+bool TemporaryFile::copyFromFile(const QString& otherFileName)
+{
+    close();
+    if (!open(QFile::WriteOnly | QFile::Truncate)) {
+        return false;
+    }
+
+    QFile otherFile(otherFileName);
+    if (!otherFile.open(QFile::ReadOnly)) {
+        close();
+        return false;
+    }
+
+    QByteArray data;
+    while(!(data = otherFile.read(1024)).isEmpty()) {
+        write(data);
+    }
+
+    otherFile.close();
+    close();
+    return true;
+}

tests/util/TemporaryFile.h

@@ -20,31 +20,20 @@
 
 #include <QTemporaryFile>
 
-#ifdef Q_OS_WIN
-/**
- * QTemporaryFile does not actually close a file when close() is
- * called, which causes the file to be locked on Windows.
- * This class extends a QFile with the extra functionality
- * of a QTemporaryFile to circumvent this problem.
- */
 class TemporaryFile : public QFile
-#else
-class TemporaryFile : public QTemporaryFile
-#endif
 {
     Q_OBJECT
 
-#ifdef Q_OS_WIN
 public:
     TemporaryFile();
     explicit TemporaryFile(const QString& templateName);
     explicit TemporaryFile(QObject* parent);
     TemporaryFile(const QString& templateName, QObject* parent);
-    ~TemporaryFile() override = default;
+    ~TemporaryFile() override;
 
     using QFile::open;
     bool open();
-#endif
+    bool copyFromFile(const QString& otherFileName);
 };
 
 #endif // KEEPASSXC_TEMPORARYFILE_H