From 0ca7fd369aaa364ad437de694714b9488e5e5636 Mon Sep 17 00:00:00 2001 From: Janek Bevendorff Date: Fri, 19 Oct 2018 21:41:42 +0200 Subject: [PATCH] Implement review feedback --- src/browser/BrowserAction.cpp | 2 +- src/cli/Remove.cpp | 5 +-- src/cli/Utils.cpp | 7 ++-- src/cli/Utils.h | 6 +++- src/cli/keepassxc-cli.cpp | 4 +-- src/config-keepassx.h.cmake | 2 +- src/core/Tools.cpp | 15 --------- src/core/Tools.h | 1 - src/gui/AboutDialog.cpp | 4 +-- src/gui/DatabaseWidget.cpp | 8 +++-- src/gui/WelcomeWidget.cpp | 2 +- src/gui/group/GroupModel.cpp | 6 ++-- src/main.cpp | 2 +- tests/TestCli.cpp | 60 +++++++++++++++++------------------ 14 files changed, 59 insertions(+), 65 deletions(-) diff --git a/src/browser/BrowserAction.cpp b/src/browser/BrowserAction.cpp index 6001acb18..e0c3a8501 100644 --- a/src/browser/BrowserAction.cpp +++ b/src/browser/BrowserAction.cpp @@ -376,7 +376,7 @@ QJsonObject BrowserAction::getErrorReply(const QString& action, const int errorC QJsonObject BrowserAction::buildMessage(const QString& nonce) const { QJsonObject message; - message["version"] = KEEPASSX_VERSION; + message["version"] = KEEPASSXC_VERSION; message["success"] = "true"; message["nonce"] = nonce; return message; diff --git a/src/cli/Remove.cpp b/src/cli/Remove.cpp index 5bbfd67e4..6523cff97 100644 --- a/src/cli/Remove.cpp +++ b/src/cli/Remove.cpp @@ -76,7 +76,7 @@ int Remove::removeEntry(Database* database, const QString& databasePath, const Q QTextStream out(Utils::STDOUT, QIODevice::WriteOnly); QTextStream err(Utils::STDERR, QIODevice::WriteOnly); - Entry* entry = database->rootGroup()->findEntryByPath(entryPath); + QPointer entry = database->rootGroup()->findEntryByPath(entryPath); if (!entry) { err << QObject::tr("Entry %1 not found.").arg(entryPath) << endl; return EXIT_FAILURE; @@ -84,7 +84,8 @@ int Remove::removeEntry(Database* database, const QString& databasePath, const Q QString entryTitle = entry->title(); bool recycled = true; - if (Tools::hasChild(database->metadata()->recycleBin(), entry) || !database->metadata()->recycleBinEnabled()) { + auto* recycleBin = database->metadata()->recycleBin(); + if (!database->metadata()->recycleBinEnabled() || (recycleBin && recycleBin->findEntryByUuid(entry->uuid()))) { delete entry; recycled = false; } else { diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp index 8a0f5abe3..a0f75bc8e 100644 --- a/src/cli/Utils.cpp +++ b/src/cli/Utils.cpp @@ -72,6 +72,8 @@ void setStdinEcho(bool enable = true) #endif } +namespace Test +{ QStringList nextPasswords = {}; /** @@ -85,6 +87,7 @@ void setNextPassword(const QString& password) { nextPasswords.append(password); } +} // namespace Test /** * Read a user password from STDIN or return a password previously @@ -97,8 +100,8 @@ QString getPassword() QTextStream out(STDOUT, QIODevice::WriteOnly); // return preset password if one is set - if (!nextPasswords.isEmpty()) { - auto password = nextPasswords.takeFirst(); + if (!Test::nextPasswords.isEmpty()) { + auto password = Test::nextPasswords.takeFirst(); // simulate user entering newline out << endl; return password; diff --git a/src/cli/Utils.h b/src/cli/Utils.h index 868ccdef5..1d5e0f356 100644 --- a/src/cli/Utils.h +++ b/src/cli/Utils.h @@ -29,8 +29,12 @@ extern FILE* STDIN; void setStdinEcho(bool enable); QString getPassword(); -void setNextPassword(const QString& password); int clipText(const QString& text); + +namespace Test +{ +void setNextPassword(const QString& password); +} }; #endif // KEEPASSXC_UTILS_H diff --git a/src/cli/keepassxc-cli.cpp b/src/cli/keepassxc-cli.cpp index 97efd8c08..041908663 100644 --- a/src/cli/keepassxc-cli.cpp +++ b/src/cli/keepassxc-cli.cpp @@ -40,7 +40,7 @@ int main(int argc, char** argv) } QCoreApplication app(argc, argv); - QCoreApplication::setApplicationVersion(KEEPASSX_VERSION); + QCoreApplication::setApplicationVersion(KEEPASSXC_VERSION); #ifdef QT_NO_DEBUG Bootstrap::bootstrapApplication(); @@ -72,7 +72,7 @@ int main(int argc, char** argv) if (parser.positionalArguments().empty()) { if (parser.isSet("version")) { // Switch to parser.showVersion() when available (QT 5.4). - out << KEEPASSX_VERSION << endl; + out << KEEPASSXC_VERSION << endl; return EXIT_SUCCESS; } parser.showHelp(); diff --git a/src/config-keepassx.h.cmake b/src/config-keepassx.h.cmake index d1f0723b4..52d14ce94 100644 --- a/src/config-keepassx.h.cmake +++ b/src/config-keepassx.h.cmake @@ -3,7 +3,7 @@ #ifndef KEEPASSX_CONFIG_KEEPASSX_H #define KEEPASSX_CONFIG_KEEPASSX_H -#define KEEPASSX_VERSION "@KEEPASSXC_VERSION@" +#define KEEPASSXC_VERSION "@KEEPASSXC_VERSION@" #define KEEPASSX_SOURCE_DIR "@CMAKE_SOURCE_DIR@" #define KEEPASSX_BINARY_DIR "@CMAKE_BINARY_DIR@" diff --git a/src/core/Tools.cpp b/src/core/Tools.cpp index 60c50b451..8467fb416 100644 --- a/src/core/Tools.cpp +++ b/src/core/Tools.cpp @@ -77,21 +77,6 @@ QString humanReadableFileSize(qint64 bytes, quint32 precision) return QString("%1 %2").arg(QLocale().toString(size, 'f', precision), units.at(i)); } -bool hasChild(const QObject* parent, const QObject* child) -{ - if (!parent || !child) { - return false; - } - - const QObjectList children = parent->children(); - for (QObject* c : children) { - if (child == c || hasChild(c, child)) { - return true; - } - } - return false; -} - bool readFromDevice(QIODevice* device, QByteArray& data, int size) { QByteArray buffer; diff --git a/src/core/Tools.h b/src/core/Tools.h index aec937304..13d9869f7 100644 --- a/src/core/Tools.h +++ b/src/core/Tools.h @@ -31,7 +31,6 @@ class QIODevice; namespace Tools { QString humanReadableFileSize(qint64 bytes, quint32 precision = 2); -bool hasChild(const QObject* parent, const QObject* child); bool readFromDevice(QIODevice* device, QByteArray& data, int size = 16384); bool readAllFromDevice(QIODevice* device, QByteArray& data); QString imageReaderFilter(); diff --git a/src/gui/AboutDialog.cpp b/src/gui/AboutDialog.cpp index c7c75a11e..f6a9d15f9 100644 --- a/src/gui/AboutDialog.cpp +++ b/src/gui/AboutDialog.cpp @@ -37,7 +37,7 @@ AboutDialog::AboutDialog(QWidget* parent) setWindowFlags(Qt::Sheet); setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint); - m_ui->nameLabel->setText(m_ui->nameLabel->text().replace("${VERSION}", KEEPASSX_VERSION)); + m_ui->nameLabel->setText(m_ui->nameLabel->text().replace("${VERSION}", KEEPASSXC_VERSION)); QFont nameLabelFont = m_ui->nameLabel->font(); nameLabelFont.setPointSize(nameLabelFont.pointSize() + 4); m_ui->nameLabel->setFont(nameLabelFont); @@ -52,7 +52,7 @@ AboutDialog::AboutDialog(QWidget* parent) } QString debugInfo = "KeePassXC - "; - debugInfo.append(tr("Version %1").arg(KEEPASSX_VERSION).append("\n")); + debugInfo.append(tr("Version %1").arg(KEEPASSXC_VERSION).append("\n")); #ifndef KEEPASSXC_BUILD_TYPE_RELEASE debugInfo.append(tr("Build Type: %1").arg(KEEPASSXC_BUILD_TYPE).append("\n")); #endif diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index c90fc52f7..effae45c1 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -460,7 +460,8 @@ void DatabaseWidget::deleteEntries() selectedEntries.append(m_entryView->entryFromIndex(index)); } - bool inRecycleBin = Tools::hasChild(m_db->metadata()->recycleBin(), selectedEntries.first()); + auto* recycleBin = m_db->metadata()->recycleBin(); + bool inRecycleBin = recycleBin && recycleBin->findEntryByUuid(selectedEntries.first()->uuid()); if (inRecycleBin || !m_db->metadata()->recycleBinEnabled()) { QString prompt; if (selected.size() == 1) { @@ -688,9 +689,10 @@ void DatabaseWidget::deleteGroup() return; } - bool inRecycleBin = Tools::hasChild(m_db->metadata()->recycleBin(), currentGroup); + auto* recycleBin = m_db->metadata()->recycleBin(); + bool inRecycleBin = recycleBin && recycleBin->findGroupByUuid(currentGroup->uuid()); bool isRecycleBin = (currentGroup == m_db->metadata()->recycleBin()); - bool isRecycleBinSubgroup = Tools::hasChild(currentGroup, m_db->metadata()->recycleBin()); + bool isRecycleBinSubgroup = currentGroup->findGroupByUuid(m_db->metadata()->recycleBin()->uuid()); if (inRecycleBin || isRecycleBin || isRecycleBinSubgroup || !m_db->metadata()->recycleBinEnabled()) { QMessageBox::StandardButton result = MessageBox::question( this, diff --git a/src/gui/WelcomeWidget.cpp b/src/gui/WelcomeWidget.cpp index 7bb4484c7..ed0ca2936 100644 --- a/src/gui/WelcomeWidget.cpp +++ b/src/gui/WelcomeWidget.cpp @@ -29,7 +29,7 @@ WelcomeWidget::WelcomeWidget(QWidget* parent) { m_ui->setupUi(this); - m_ui->welcomeLabel->setText(tr("Welcome to KeePassXC %1").arg(KEEPASSX_VERSION)); + m_ui->welcomeLabel->setText(tr("Welcome to KeePassXC %1").arg(KEEPASSXC_VERSION)); QFont welcomeLabelFont = m_ui->welcomeLabel->font(); welcomeLabelFont.setBold(true); welcomeLabelFont.setPointSize(welcomeLabelFont.pointSize() + 4); diff --git a/src/gui/group/GroupModel.cpp b/src/gui/group/GroupModel.cpp index e8f51909f..a3c72b792 100644 --- a/src/gui/group/GroupModel.cpp +++ b/src/gui/group/GroupModel.cpp @@ -234,11 +234,11 @@ bool GroupModel::dropMimeData(const QMimeData* data, } Group* dragGroup = db->resolveGroup(groupUuid); - if (!dragGroup || !Tools::hasChild(db, dragGroup) || dragGroup == db->rootGroup()) { + if (!dragGroup || !db->rootGroup()->findGroupByUuid(dragGroup->uuid()) || dragGroup == db->rootGroup()) { return false; } - if (dragGroup == parentGroup || Tools::hasChild(dragGroup, parentGroup)) { + if (dragGroup == parentGroup || dragGroup->findGroupByUuid(parentGroup->uuid())) { return false; } @@ -278,7 +278,7 @@ bool GroupModel::dropMimeData(const QMimeData* data, } Entry* dragEntry = db->resolveEntry(entryUuid); - if (!dragEntry || !Tools::hasChild(db, dragEntry)) { + if (!dragEntry || !db->rootGroup()->findEntryByUuid(dragEntry->uuid())) { continue; } diff --git a/src/main.cpp b/src/main.cpp index 74af06953..a37648ec1 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -48,7 +48,7 @@ int main(int argc, char** argv) { Application app(argc, argv); Application::setApplicationName("keepassxc"); - Application::setApplicationVersion(KEEPASSX_VERSION); + Application::setApplicationVersion(KEEPASSXC_VERSION); // don't set organizationName as that changes the return value of // QStandardPaths::writableLocation(QDesktopServices::DataLocation) Bootstrap::bootstrapApplication(); diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index fd51aa2e4..e10a651d7 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -115,7 +115,7 @@ void TestCli::cleanupTestCase() QSharedPointer TestCli::readTestDatabase() const { - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); auto db = QSharedPointer(Database::unlockFromStdin(m_dbFile->fileName(), "", m_stdoutHandle)); m_stdoutFile->seek(ftell(m_stdoutHandle)); // re-synchronize handles return db; @@ -145,7 +145,7 @@ void TestCli::testAdd() QVERIFY(!addCmd.name.isEmpty()); QVERIFY(addCmd.getDescriptionLine().contains(addCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); addCmd.execute({"add", "-u", "newuser", "--url", "https://example.com/", "-g", "-l", "20", m_dbFile->fileName(), "/newuser-entry"}); m_stderrFile->reset(); @@ -156,8 +156,8 @@ void TestCli::testAdd() QCOMPARE(entry->url(), QString("https://example.com/")); QCOMPARE(entry->password().size(), 20); - Utils::setNextPassword("a"); - Utils::setNextPassword("newpassword"); + Utils::Test::setNextPassword("a"); + Utils::Test::setNextPassword("newpassword"); addCmd.execute({"add", "-u", "newuser2", "--url", "https://example.net/", "-g", "-l", "20", "-p", m_dbFile->fileName(), "/newuser-entry2"}); db = readTestDatabase(); @@ -177,7 +177,7 @@ void TestCli::testClip() QVERIFY(!clipCmd.name.isEmpty()); QVERIFY(clipCmd.getDescriptionLine().contains(clipCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); clipCmd.execute({"clip", m_dbFile->fileName(), "/Sample Entry"}); m_stderrFile->reset(); @@ -190,7 +190,7 @@ void TestCli::testClip() QCOMPARE(clipboard->text(), QString("Password")); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); QFuture future = QtConcurrent::run(&clipCmd, &Clip::execute, QStringList{"clip", m_dbFile->fileName(), "/Sample Entry", "1"}); QTRY_COMPARE_WITH_TIMEOUT(clipboard->text(), QString("Password"), 500); @@ -246,7 +246,7 @@ void TestCli::testEdit() QVERIFY(!editCmd.name.isEmpty()); QVERIFY(editCmd.getDescriptionLine().contains(editCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); editCmd.execute({"edit", "-u", "newuser", "--url", "https://otherurl.example.com/", "-t", "newtitle", m_dbFile->fileName(), "/Sample Entry"}); auto db = readTestDatabase(); @@ -256,7 +256,7 @@ void TestCli::testEdit() QCOMPARE(entry->url(), QString("https://otherurl.example.com/")); QCOMPARE(entry->password(), QString("Password")); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); editCmd.execute({"edit", "-g", m_dbFile->fileName(), "/newtitle"}); db = readTestDatabase(); entry = db->rootGroup()->findEntryByPath("/newtitle"); @@ -266,7 +266,7 @@ void TestCli::testEdit() QVERIFY(!entry->password().isEmpty()); QVERIFY(entry->password() != QString("Password")); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); editCmd.execute({"edit", "-g", "-l", "34", "-t", "yet another title", m_dbFile->fileName(), "/newtitle"}); db = readTestDatabase(); entry = db->rootGroup()->findEntryByPath("/yet another title"); @@ -276,8 +276,8 @@ void TestCli::testEdit() QVERIFY(entry->password() != QString("Password")); QCOMPARE(entry->password().size(), 34); - Utils::setNextPassword("a"); - Utils::setNextPassword("newpassword"); + Utils::Test::setNextPassword("a"); + Utils::Test::setNextPassword("newpassword"); editCmd.execute({"edit", "-p", m_dbFile->fileName(), "/yet another title"}); db = readTestDatabase(); entry = db->rootGroup()->findEntryByPath("/yet another title"); @@ -392,7 +392,7 @@ void TestCli::testExtract() QVERIFY(!extractCmd.name.isEmpty()); QVERIFY(extractCmd.getDescriptionLine().contains(extractCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); extractCmd.execute({"extract", m_dbFile->fileName()}); m_stdoutFile->seek(0); @@ -471,7 +471,7 @@ void TestCli::testList() QVERIFY(!listCmd.name.isEmpty()); QVERIFY(listCmd.getDescriptionLine().contains(listCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); listCmd.execute({"ls", m_dbFile->fileName()}); m_stdoutFile->reset(); m_stdoutFile->readLine(); // skip password prompt @@ -484,7 +484,7 @@ void TestCli::testList() "Homebanking/\n")); qint64 pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); listCmd.execute({"ls", "-R", m_dbFile->fileName()}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -503,14 +503,14 @@ void TestCli::testList() " [empty]\n")); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); listCmd.execute({"ls", m_dbFile->fileName(), "/General/"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); QCOMPARE(m_stdoutFile->readAll(), QByteArray("[empty]\n")); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); listCmd.execute({"ls", m_dbFile->fileName(), "/DoesNotExist/"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -525,14 +525,14 @@ void TestCli::testLocate() QVERIFY(!locateCmd.name.isEmpty()); QVERIFY(locateCmd.getDescriptionLine().contains(locateCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); locateCmd.execute({"locate", m_dbFile->fileName(), "Sample"}); m_stdoutFile->reset(); m_stdoutFile->readLine(); // skip password prompt QCOMPARE(m_stdoutFile->readAll(), QByteArray("/Sample Entry\n")); qint64 pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); locateCmd.execute({"locate", m_dbFile->fileName(), "Does Not Exist"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -556,14 +556,14 @@ void TestCli::testLocate() tmpFile.close(); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); locateCmd.execute({"locate", tmpFile.fileName(), "New"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt QCOMPARE(m_stdoutFile->readAll(), QByteArray("/General/New Entry\n")); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); locateCmd.execute({"locate", tmpFile.fileName(), "Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -612,7 +612,7 @@ void TestCli::testMerge() sourceFile.close(); qint64 pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); mergeCmd.execute({"merge", "-s", targetFile1.fileName(), sourceFile.fileName()}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); @@ -631,8 +631,8 @@ void TestCli::testMerge() // try again with different passwords for both files pos = m_stdoutFile->pos(); - Utils::setNextPassword("b"); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("b"); + Utils::Test::setNextPassword("a"); mergeCmd.execute({"merge", targetFile2.fileName(), sourceFile.fileName()}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); @@ -671,7 +671,7 @@ void TestCli::testRemove() qint64 pos = m_stdoutFile->pos(); // delete entry and verify - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); removeCmd.execute({"rm", m_dbFile->fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -690,7 +690,7 @@ void TestCli::testRemove() pos = m_stdoutFile->pos(); // try again, this time without recycle bin - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); removeCmd.execute({"rm", fileCopy.fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -707,7 +707,7 @@ void TestCli::testRemove() pos = m_stdoutFile->pos(); // finally, try deleting a non-existent entry - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); removeCmd.execute({"rm", fileCopy.fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -722,7 +722,7 @@ void TestCli::testShow() QVERIFY(!showCmd.name.isEmpty()); QVERIFY(showCmd.getDescriptionLine().contains(showCmd.name)); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); showCmd.execute({"show", m_dbFile->fileName(), "/Sample Entry"}); m_stdoutFile->reset(); m_stdoutFile->readLine(); // skip password prompt @@ -733,14 +733,14 @@ void TestCli::testShow() "Notes: Notes\n")); qint64 pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); showCmd.execute({"show", "-a", "Title", m_dbFile->fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt QCOMPARE(m_stdoutFile->readAll(), QByteArray("Sample Entry\n")); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); showCmd.execute({"show", "-a", "Title", "-a", "URL", m_dbFile->fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt @@ -748,7 +748,7 @@ void TestCli::testShow() "http://www.somesite.com/\n")); pos = m_stdoutFile->pos(); - Utils::setNextPassword("a"); + Utils::Test::setNextPassword("a"); showCmd.execute({"show", "-a", "DoesNotExist", m_dbFile->fileName(), "/Sample Entry"}); m_stdoutFile->seek(pos); m_stdoutFile->readLine(); // skip password prompt