Add CLI tests and improve coding style and i18n

The CLI module was lacking unit test coverage and showed some severe coding style violations, which this patch addresses. In addition, all uses of qCritical() with untranslatble raw char* sequences were removed in favor of proper locale strings. These are written to STDERR through QTextStreams and support output redirection for testing purposes. With this change, error messages don't depend on the global Qt logging settings and targets anymore and go directly to the terminal or into a file if needed. This patch also fixes a bug discovered during unit test development, where the extract command would just dump the raw XML contents without decrypting embedded Salsa20-protected values first, making the XML export mostly useless, since passwords are scrambled. Lastly, all CLI commands received a dedicated -h/--help option.
keepassxreboot · Oct 4, 2018 · b82ea87 · b82ea87
1 parent 9e9680e
commit b82ea87
Show file tree

Hide file tree

Showing 67 changed files with 2,250 additions and 1,239 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -303,16 +303,16 @@ endif()
 include(CLangFormat)
 
 if(UNIX AND NOT APPLE)
-  find_package(Qt5 COMPONENTS Core Network Concurrent Widgets Test LinguistTools DBus REQUIRED)
+  find_package(Qt5 COMPONENTS Core Network Concurrent Gui Widgets Test LinguistTools DBus REQUIRED)
 elseif(APPLE)
-  find_package(Qt5 COMPONENTS Core Network Concurrent Widgets Test LinguistTools REQUIRED
+  find_package(Qt5 COMPONENTS Core Network Concurrent Gui Widgets Test LinguistTools REQUIRED
     HINTS /usr/local/Cellar/qt/*/lib/cmake ENV PATH
   )
   find_package(Qt5 COMPONENTS MacExtras
     HINTS /usr/local/Cellar/qt/*/lib/cmake ENV PATH
   )
 else()
-  find_package(Qt5 COMPONENTS Core Network Concurrent Widgets Test LinguistTools REQUIRED)
+  find_package(Qt5 COMPONENTS Core Network Concurrent Gui Widgets Test LinguistTools REQUIRED)
 endif()
 
 if(Qt5Core_VERSION VERSION_LESS "5.2.0")

diff --git a/Dockerfile b/Dockerfile
@@ -16,7 +16,7 @@
 
 FROM ubuntu:14.04
 
-ENV REBUILD_COUNTER=8
+ENV REBUILD_COUNTER=9
 
 ENV QT5_VERSION=qt510
 ENV QT5_PPA_VERSION=qt-5.10.1
@@ -55,7 +55,9 @@ RUN set -x \
         libxtst-dev \
         mesa-common-dev \
         libyubikey-dev \
-        libykpers-1-dev
+        libykpers-1-dev \
+        xclip \
+        xvfb
 
 ENV PATH="/opt/${QT5_VERSION}/bin:${PATH}"
 ENV CMAKE_PREFIX_PATH="/opt/${QT5_VERSION}/lib/cmake"

diff --git a/ci/trusty/Dockerfile b/ci/trusty/Dockerfile
@@ -39,6 +39,7 @@ RUN set -x \
         clang-3.6 \
         libclang-common-3.6-dev \
         clang-format-3.6 \
+        llvm-3.6 \
         cmake3 \
         make \
         libgcrypt20-18-dev \
@@ -54,6 +55,7 @@ RUN set -x \
         libykpers-1-dev \
         libxi-dev \
         libxtst-dev \
+        xclip \
         xvfb
 
 ENV PATH="/opt/${QT5_VERSION}/bin:${PATH}"

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -51,6 +51,7 @@ set(keepassx_SOURCES
     core/EntryAttributes.cpp
     core/EntrySearcher.cpp
     core/FilePath.cpp
+    core/Bootstrap.cpp
     core/Global.h
     core/Group.cpp
     core/InactivityTimer.cpp

diff --git a/src/browser/BrowserAction.cpp b/src/browser/BrowserAction.cpp
@@ -271,7 +271,6 @@ QJsonObject BrowserAction::handleGeneratePassword(const QJsonObject& json, const
 {
     const QString nonce = json.value("nonce").toString();
     const QString password = browserSettings()->generatePassword();
-    const QString bits = QString::number(browserSettings()->getbits()); // For some reason this always returns 1140 bits?
 
     if (nonce.isEmpty() || password.isEmpty()) {
         return QJsonObject();

diff --git a/src/browser/BrowserSettings.cpp b/src/browser/BrowserSettings.cpp
@@ -485,11 +485,6 @@ QString BrowserSettings::generatePassword()
     }
 }
 
-int BrowserSettings::getbits()
-{
-    return m_passwordGenerator.getbits();
-}
-
 void BrowserSettings::updateBinaryPaths(QString customProxyLocation)
 {
     bool isProxy = supportBrowserProxy();

diff --git a/src/browser/BrowserSettings.h b/src/browser/BrowserSettings.h
@@ -112,7 +112,6 @@ class BrowserSettings
     PasswordGenerator::CharClasses passwordCharClasses();
     PasswordGenerator::GeneratorFlags passwordGeneratorFlags();
     QString generatePassword();
-    int getbits();
     void updateBinaryPaths(QString customProxyLocation = QString());
 
 private:

diff --git a/src/cli/Add.cpp b/src/cli/Add.cpp
@@ -41,46 +41,43 @@ Add::~Add()
 
 int Add::execute(const QStringList& arguments)
 {
-
-    QTextStream inputTextStream(stdin, QIODevice::ReadOnly);
-    QTextStream outputTextStream(stdout, QIODevice::WriteOnly);
+    QTextStream inputTextStream(Utils::STDIN, QIODevice::ReadOnly);
+    QTextStream outputTextStream(Utils::STDOUT, QIODevice::WriteOnly);
+    QTextStream errorTextStream(Utils::STDERR, QIODevice::WriteOnly);
 
     QCommandLineParser parser;
-    parser.setApplicationDescription(this->description);
+    parser.setApplicationDescription(description);
     parser.addPositionalArgument("database", QObject::tr("Path of the database."));
 
-    QCommandLineOption keyFile(QStringList() << "k"
-                                             << "key-file",
+    QCommandLineOption keyFile(QStringList() << "k" << "key-file",
                                QObject::tr("Key file of the database."),
                                QObject::tr("path"));
     parser.addOption(keyFile);
 
-    QCommandLineOption username(QStringList() << "u"
-                                              << "username",
+    QCommandLineOption username(QStringList() << "u"  << "username",
                                 QObject::tr("Username for the entry."),
                                 QObject::tr("username"));
     parser.addOption(username);
 
     QCommandLineOption url(QStringList() << "url", QObject::tr("URL for the entry."), QObject::tr("URL"));
     parser.addOption(url);
 
-    QCommandLineOption prompt(QStringList() << "p"
-                                            << "password-prompt",
+    QCommandLineOption prompt(QStringList() << "p" << "password-prompt",
                               QObject::tr("Prompt for the entry's password."));
     parser.addOption(prompt);
 
-    QCommandLineOption generate(QStringList() << "g"
-                                              << "generate",
+    QCommandLineOption generate(QStringList() << "g"  << "generate",
                                 QObject::tr("Generate a password for the entry."));
     parser.addOption(generate);
 
-    QCommandLineOption length(QStringList() << "l"
-                                            << "password-length",
+    QCommandLineOption length(QStringList() << "l" << "password-length",
                               QObject::tr("Length for the generated password."),
                               QObject::tr("length"));
     parser.addOption(length);
 
     parser.addPositionalArgument("entry", QObject::tr("Path of the entry to add."));
+
+    parser.addHelpOption();
     parser.process(arguments);
 
     const QStringList args = parser.positionalArguments();
@@ -89,25 +86,25 @@ int Add::execute(const QStringList& arguments)
         return EXIT_FAILURE;
     }
 
-    QString databasePath = args.at(0);
-    QString entryPath = args.at(1);
+    const QString& databasePath = args.at(0);
+    const QString& entryPath = args.at(1);
 
-    Database* db = Database::unlockFromStdin(databasePath, parser.value(keyFile));
-    if (db == nullptr) {
+    Database* db = Database::unlockFromStdin(databasePath, parser.value(keyFile), Utils::STDOUT, Utils::STDERR);
+    if (!db) {
         return EXIT_FAILURE;
     }
 
     // Validating the password length here, before we actually create
     // the entry.
     QString passwordLength = parser.value(length);
     if (!passwordLength.isEmpty() && !passwordLength.toInt()) {
-        qCritical("Invalid value for password length %s.", qPrintable(passwordLength));
+        errorTextStream << QObject::tr("Invalid value for password length %1.").arg(passwordLength) << endl;
         return EXIT_FAILURE;
     }
 
     Entry* entry = db->rootGroup()->addEntryWithPath(entryPath);
     if (!entry) {
-        qCritical("Could not create entry with path %s.", qPrintable(entryPath));
+        errorTextStream << QObject::tr("Could not create entry with path %1.").arg(entryPath) << endl;
         return EXIT_FAILURE;
     }
 
@@ -120,8 +117,7 @@ int Add::execute(const QStringList& arguments)
     }
 
     if (parser.isSet(prompt)) {
-        outputTextStream << "Enter password for new entry: ";
-        outputTextStream.flush();
+        outputTextStream << QObject::tr("Enter password for new entry: ") << flush;
         QString password = Utils::getPassword();
         entry->setPassword(password);
     } else if (parser.isSet(generate)) {
@@ -130,7 +126,7 @@ int Add::execute(const QStringList& arguments)
         if (passwordLength.isEmpty()) {
             passwordGenerator.setLength(PasswordGenerator::DefaultLength);
         } else {
-            passwordGenerator.setLength(passwordLength.toInt());
+            passwordGenerator.setLength(static_cast<size_t>(passwordLength.toInt()));
         }
 
         passwordGenerator.setCharClasses(PasswordGenerator::DefaultCharset);
@@ -141,10 +137,10 @@ int Add::execute(const QStringList& arguments)
 
     QString errorMessage = db->saveToFile(databasePath);
     if (!errorMessage.isEmpty()) {
-        qCritical("Writing the database failed %s.", qPrintable(errorMessage));
+        errorTextStream << QObject::tr("Writing the database failed %1.").arg(errorMessage) << endl;
         return EXIT_FAILURE;
     }
 
-    outputTextStream << "Successfully added entry " << entry->title() << "." << endl;
+    outputTextStream << QObject::tr("Successfully added entry %1.").arg(entry->title()) << endl;
     return EXIT_SUCCESS;
 }
diff --git a/src/cli/Clip.cpp b/src/cli/Clip.cpp
@@ -42,20 +42,19 @@ Clip::~Clip()
 
 int Clip::execute(const QStringList& arguments)
 {
-
-    QTextStream out(stdout);
+    QTextStream out(Utils::STDOUT);
 
     QCommandLineParser parser;
-    parser.setApplicationDescription(this->description);
+    parser.setApplicationDescription(description);
     parser.addPositionalArgument("database", QObject::tr("Path of the database."));
-    QCommandLineOption keyFile(QStringList() << "k"
-                                             << "key-file",
+    QCommandLineOption keyFile(QStringList() << "k" << "key-file",
                                QObject::tr("Key file of the database."),
                                QObject::tr("path"));
     parser.addOption(keyFile);
     parser.addPositionalArgument("entry", QObject::tr("Path of the entry to clip.", "clip = copy to clipboard"));
-    parser.addPositionalArgument(
-        "timeout", QObject::tr("Timeout in seconds before clearing the clipboard."), QString("[timeout]"));
+    parser.addPositionalArgument("timeout",
+                                 QObject::tr("Timeout in seconds before clearing the clipboard."), "[timeout]");
+    parser.addHelpOption();
     parser.process(arguments);
 
     const QStringList args = parser.positionalArguments();
@@ -64,29 +63,30 @@ int Clip::execute(const QStringList& arguments)
         return EXIT_FAILURE;
     }
 
-    Database* db = Database::unlockFromStdin(args.at(0), parser.value(keyFile));
+    Database* db = Database::unlockFromStdin(args.at(0), parser.value(keyFile), Utils::STDOUT, Utils::STDERR);
     if (!db) {
         return EXIT_FAILURE;
     }
 
-    return this->clipEntry(db, args.at(1), args.value(2));
+    return clipEntry(db, args.at(1), args.value(2));
 }
 
 int Clip::clipEntry(Database* database, QString entryPath, QString timeout)
 {
+    QTextStream err(Utils::STDERR);
 
     int timeoutSeconds = 0;
     if (!timeout.isEmpty() && !timeout.toInt()) {
-        qCritical("Invalid timeout value %s.", qPrintable(timeout));
+        err << QObject::tr("Invalid timeout value %1.").arg(timeout) << endl;
         return EXIT_FAILURE;
     } else if (!timeout.isEmpty()) {
         timeoutSeconds = timeout.toInt();
     }
 
-    QTextStream outputTextStream(stdout, QIODevice::WriteOnly);
+    QTextStream outputTextStream(Utils::STDOUT, QIODevice::WriteOnly);
     Entry* entry = database->rootGroup()->findEntry(entryPath);
     if (!entry) {
-        qCritical("Entry %s not found.", qPrintable(entryPath));
+        err << QObject::tr("Entry %1 not found.").arg(entryPath) << endl;
         return EXIT_FAILURE;
     }
 
@@ -95,20 +95,23 @@ int Clip::clipEntry(Database* database, QString entryPath, QString timeout)
         return exitCode;
     }
 
-    outputTextStream << "Entry's password copied to the clipboard!" << endl;
+    outputTextStream << QObject::tr("Entry's password copied to the clipboard!") << endl;
 
     if (!timeoutSeconds) {
         return exitCode;
     }
 
+    QString lastLine = "";
     while (timeoutSeconds > 0) {
-        outputTextStream << "\rClearing the clipboard in " << timeoutSeconds << " seconds...";
-        outputTextStream.flush();
+        outputTextStream << '\r' << QString(lastLine.size(), ' ') << '\r';
+        lastLine = QObject::tr("Clearing the clipboard in %1 second(s)...", "", timeoutSeconds).arg(timeoutSeconds);
+        outputTextStream << lastLine << flush;
         std::this_thread::sleep_for(std::chrono::milliseconds(1000));
-        timeoutSeconds--;
+        --timeoutSeconds;
     }
     Utils::clipText("");
-    outputTextStream << "\nClipboard cleared!" << endl;
+    outputTextStream << '\r' << QString(lastLine.size(), ' ') << '\r';
+    outputTextStream << QObject::tr("Clipboard cleared!") << endl;
 
     return EXIT_SUCCESS;
 }
diff --git a/src/cli/Command.cpp b/src/cli/Command.cpp
@@ -41,19 +41,14 @@ Command::~Command()
 {
 }
 
-int Command::execute(const QStringList&)
-{
-    return EXIT_FAILURE;
-}
-
 QString Command::getDescriptionLine()
 {
 
-    QString response = this->name;
+    QString response = name;
     QString space(" ");
-    QString spaces = space.repeated(15 - this->name.length());
+    QString spaces = space.repeated(15 - name.length());
     response = response.append(spaces);
-    response = response.append(this->description);
+    response = response.append(description);
     response = response.append("\n");
     return response;
 }

diff --git a/src/cli/Command.h b/src/cli/Command.h
@@ -29,7 +29,7 @@ class Command
 {
 public:
     virtual ~Command();
-    virtual int execute(const QStringList& arguments);
+    virtual int execute(const QStringList& arguments) = 0;
     QString name;
     QString description;
     QString getDescriptionLine();

diff --git a/src/cli/Diceware.cpp b/src/cli/Diceware.cpp
@@ -24,6 +24,7 @@
 #include <QTextStream>
 
 #include "core/PassphraseGenerator.h"
+#include "Utils.h"
 
 Diceware::Diceware()
 {
@@ -37,26 +38,25 @@ Diceware::~Diceware()
 
 int Diceware::execute(const QStringList& arguments)
 {
-    QTextStream inputTextStream(stdin, QIODevice::ReadOnly);
-    QTextStream outputTextStream(stdout, QIODevice::WriteOnly);
+    QTextStream in(Utils::STDIN, QIODevice::ReadOnly);
+    QTextStream out(Utils::STDOUT, QIODevice::WriteOnly);
 
     QCommandLineParser parser;
-    parser.setApplicationDescription(this->description);
-    QCommandLineOption words(QStringList() << "W"
-                                           << "words",
+    parser.setApplicationDescription(description);
+    QCommandLineOption words(QStringList() << "W" << "words",
                              QObject::tr("Word count for the diceware passphrase."),
-                             QObject::tr("count"));
+                             QObject::tr("count", "CLI parameter"));
     parser.addOption(words);
-    QCommandLineOption wordlistFile(QStringList() << "w"
-                                                  << "word-list",
+    QCommandLineOption wordlistFile(QStringList() << "w" << "word-list",
                                     QObject::tr("Wordlist for the diceware generator.\n[Default: EFF English]"),
                                     QObject::tr("path"));
     parser.addOption(wordlistFile);
+    parser.addHelpOption();
     parser.process(arguments);
 
     const QStringList args = parser.positionalArguments();
     if (!args.isEmpty()) {
-        outputTextStream << parser.helpText().replace("keepassxc-cli", "keepassxc-cli diceware");
+        out << parser.helpText().replace("keepassxc-cli", "keepassxc-cli diceware");
         return EXIT_FAILURE;
     }
 
@@ -76,12 +76,12 @@ int Diceware::execute(const QStringList& arguments)
     }
 
     if (!dicewareGenerator.isValid()) {
-        outputTextStream << parser.helpText().replace("keepassxc-cli", "keepassxc-cli diceware");
+        out << parser.helpText().replace("keepassxc-cli", "keepassxc-cli diceware");
         return EXIT_FAILURE;
     }
 
     QString password = dicewareGenerator.generatePassphrase();
-    outputTextStream << password << endl;
+    out << password << endl;
 
     return EXIT_SUCCESS;
 }