From e6c2c7ed93ee019f4113217bbe6e3b8fb2b3f208 Mon Sep 17 00:00:00 2001
From: louib <L0U13@protonmail.com>
Date: Wed, 18 Mar 2020 21:51:36 -0400
Subject: [PATCH] CLI: Cleanup create options (#4313)

* Add ability to create database with an empty password
* Add password repeat check
* Standardize process between `db-create` and `import` commands
* Improve db-create tests with new password repeat

Co-authored-by: Jonathan White <support@dmapps.us>
---
 CHANGELOG.md                   |   2 +
 share/docs/man/keepassxc-cli.1 |   8 +-
 src/cli/Create.cpp             |  37 ++++++---
 src/cli/Create.h               |   2 +
 src/cli/Import.cpp             |   8 +-
 src/cli/Utils.cpp              |  32 +++++++-
 src/cli/Utils.h                |   2 +-
 tests/TestCli.cpp              | 134 ++++++++++++++++++++++++---------
 8 files changed, 171 insertions(+), 54 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 76bb5c38b..6bdcb5bf0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,8 @@
 
 ### Changed
 - Renamed CLI create command to db-create [#4231]
+- Added --set-password option for CLI db-create command
+- Added --set-key-file option for CLI db-create command (replacing --key-file option)
 
 ## 2.5.3 (2020-01-19)
 
diff --git a/share/docs/man/keepassxc-cli.1 b/share/docs/man/keepassxc-cli.1
index d1360cd65..bac9a8d37 100644
--- a/share/docs/man/keepassxc-cli.1
+++ b/share/docs/man/keepassxc-cli.1
@@ -29,7 +29,7 @@ Copies an attribute or the current TOTP (if the \fI-t\fP option is specified) of
 In interactive mode, closes the currently opened database (see \fIopen\fP).
 
 .IP "\fBdb-create\fP [options] <database>"
-Creates a new database with a key file and/or password. The key file will be created if the file that is referred to does not exist. If both the key file and password are empty, no database will be created.
+Creates a new database with a password and/or a key file. The key file will be created if the file that is referred to does not exist. If both the key file and password are empty, no database will be created.
 
 .IP "\fBdb-info\fP [options] <database>"
 Show a database's information.
@@ -185,6 +185,12 @@ Will report an error if no TOTP is configured for the entry.
 
 .SS "Create options"
 
+.IP "\fB-k\fP, \fB--set-key-file\fP <path>"
+Set the key file for the database.
+
+.IP "\fB-p\fP, \fB--set-password\fP"
+Set a password for the database.
+
 .IP "\fB-t\fP, \fB--decryption-time\fP <time>"
 Target decryption time in MS for the database.
 
diff --git a/src/cli/Create.cpp b/src/cli/Create.cpp
index c8e3b771f..2a01097f3 100644
--- a/src/cli/Create.cpp
+++ b/src/cli/Create.cpp
@@ -36,12 +36,24 @@ const QCommandLineOption Create::DecryptionTimeOption =
                        QObject::tr("Target decryption time in MS for the database."),
                        QObject::tr("time"));
 
+const QCommandLineOption Create::SetKeyFileOption =
+    QCommandLineOption(QStringList() << "k"
+                                     << "set-key-file",
+                       QObject::tr("Set the key file for the database."),
+                       QObject::tr("path"));
+
+const QCommandLineOption Create::SetPasswordOption =
+    QCommandLineOption(QStringList() << "p"
+                                     << "set-password",
+                       QObject::tr("Set a password for the database."));
+
 Create::Create()
 {
     name = QString("db-create");
     description = QObject::tr("Create a new database.");
     positionalArguments.append({QString("database"), QObject::tr("Path of the database."), QString("")});
-    options.append(Command::KeyFileOption);
+    options.append(Create::SetKeyFileOption);
+    options.append(Create::SetPasswordOption);
     options.append(Create::DecryptionTimeOption);
 }
 
@@ -97,21 +109,26 @@ int Create::execute(const QStringList& arguments)
 
     auto key = QSharedPointer<CompositeKey>::create();
 
-    auto password = Utils::getPasswordFromStdin();
-    if (!password.isNull()) {
-        key->addKey(password);
+    if (parser->isSet(Create::SetPasswordOption)) {
+        auto passwordKey = Utils::getPasswordFromStdin();
+        if (passwordKey.isNull()) {
+            err << QObject::tr("Failed to set database password.") << endl;
+            return EXIT_FAILURE;
+        }
+        key->addKey(passwordKey);
     }
 
-    QSharedPointer<FileKey> fileKey;
-    if (parser->isSet(Command::KeyFileOption)) {
-        if (!loadFileKey(parser->value(Command::KeyFileOption), fileKey)) {
+    if (parser->isSet(Create::SetKeyFileOption)) {
+        QSharedPointer<FileKey> fileKey;
+
+        if (!loadFileKey(parser->value(Create::SetKeyFileOption), fileKey)) {
             err << QObject::tr("Loading the key file failed") << endl;
             return EXIT_FAILURE;
         }
-    }
 
-    if (!fileKey.isNull()) {
-        key->addKey(fileKey);
+        if (!fileKey.isNull()) {
+            key->addKey(fileKey);
+        }
     }
 
     if (key->isEmpty()) {
diff --git a/src/cli/Create.h b/src/cli/Create.h
index 47e2e34ab..0eb36fd55 100644
--- a/src/cli/Create.h
+++ b/src/cli/Create.h
@@ -28,6 +28,8 @@ public:
     Create();
     int execute(const QStringList& arguments) override;
 
+    static const QCommandLineOption SetKeyFileOption;
+    static const QCommandLineOption SetPasswordOption;
     static const QCommandLineOption DecryptionTimeOption;
 
 private:
diff --git a/src/cli/Import.cpp b/src/cli/Import.cpp
index dd7b12c64..9331cd078 100644
--- a/src/cli/Import.cpp
+++ b/src/cli/Import.cpp
@@ -70,10 +70,12 @@ int Import::execute(const QStringList& arguments)
 
     auto key = QSharedPointer<CompositeKey>::create();
 
-    auto password = Utils::getPasswordFromStdin();
-    if (!password.isNull()) {
-        key->addKey(password);
+    auto passwordKey = Utils::getPasswordFromStdin();
+    if (passwordKey.isNull()) {
+        errorTextStream << QObject::tr("Failed to set database password.") << endl;
+        return EXIT_FAILURE;
     }
+    key->addKey(passwordKey);
 
     if (key->isEmpty()) {
         errorTextStream << QObject::tr("No key is set. Aborting database creation.") << endl;
diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp
index 88d70466b..c20f30ee1 100644
--- a/src/cli/Utils.cpp
+++ b/src/cli/Utils.cpp
@@ -93,9 +93,12 @@ namespace Utils
          *
          * @param password password to return next
          */
-        void setNextPassword(const QString& password)
+        void setNextPassword(const QString& password, bool repeat)
         {
             nextPasswords.append(password);
+            if (repeat) {
+                nextPasswords.append(password);
+            }
         }
     } // namespace Test
 
@@ -232,10 +235,31 @@ namespace Utils
 
         out << QObject::tr("Enter password to encrypt database (optional): ");
         out.flush();
-        QString password = Utils::getPassword();
+        auto password = Utils::getPassword();
 
-        if (!password.isEmpty()) {
-            passwordKey = QSharedPointer<PasswordKey>(new PasswordKey(password));
+        if (password.isEmpty()) {
+            out << QObject::tr("Do you want to create a database with an empty password? [y/N]: ");
+            out.flush();
+            TextStream ts(STDIN, QIODevice::ReadOnly);
+            if (!ts.device()->isSequential()) {
+                // This is required for testing on macOS
+                ts.seek(0);
+            }
+            auto ans = ts.readLine();
+            if (ans.toLower().startsWith("y")) {
+                passwordKey = QSharedPointer<PasswordKey>::create("");
+            }
+            out << endl;
+        } else {
+            out << QObject::tr("Repeat password: ");
+            out.flush();
+            auto repeat = Utils::getPassword();
+
+            if (password == repeat) {
+                passwordKey = QSharedPointer<PasswordKey>::create(password);
+            } else {
+                out << QObject::tr("Error: Passwords do not match.") << endl;
+            }
         }
 
         return passwordKey;
diff --git a/src/cli/Utils.h b/src/cli/Utils.h
index d96e260c4..512b209d6 100644
--- a/src/cli/Utils.h
+++ b/src/cli/Utils.h
@@ -62,7 +62,7 @@ namespace Utils
 
     namespace Test
     {
-        void setNextPassword(const QString& password);
+        void setNextPassword(const QString& password, bool repeat = false);
     }
 }; // namespace Utils
 
diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp
index efaff1831..31e090bee 100644
--- a/tests/TestCli.cpp
+++ b/tests/TestCli.cpp
@@ -566,73 +566,133 @@ void TestCli::testCreate()
     QVERIFY(createCmd.getDescriptionLine().contains(createCmd.name));
 
     QScopedPointer<QTemporaryDir> testDir(new QTemporaryDir());
+    QString dbFilename;
 
-    QString databaseFilename = testDir->path() + "/testCreate1.kdbx";
-    // Password
+    // Testing password option, password mismatch
+    dbFilename = testDir->path() + "/testCreate_pw.kdbx";
     Utils::Test::setNextPassword("a");
-    createCmd.execute({"db-create", databaseFilename});
-
+    Utils::Test::setNextPassword("b");
+    createCmd.execute({"db-create", dbFilename, "-p"});
     m_stderrFile->reset();
     m_stdoutFile->reset();
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Error: Passwords do not match.\n"));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("Failed to set database password.\n"));
+
+    // Testing password option
+    Utils::Test::setNextPassword("a", true);
+    qint64 pos = m_stdoutFile->pos();
+    qint64 errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename, true, "", "", Utils::DEVNULL));
+    auto db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
+    QVERIFY(db);
+
+    // Testing with empty password (deny it)
+    dbFilename = testDir->path() + "/testCreate_blankpw.kdbx";
+    Utils::Test::setNextPassword("");
+    m_stdinFile->reset();
+    m_stdinFile->write("n\n");
+    m_stdinFile->reset();
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QVERIFY(m_stdoutFile->readLine().contains("empty password"));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("Failed to set database password.\n"));
+
+    // Testing with empty password (accept it)
+    Utils::Test::setNextPassword("");
+    m_stdinFile->reset();
+    m_stdinFile->write("y\n");
+    m_stdinFile->reset();
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename, "-p"});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QVERIFY(m_stdoutFile->readLine().contains("empty password"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
+
+    Utils::Test::setNextPassword("");
+    db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
     QVERIFY(db);
 
     // Should refuse to create the database if it already exists.
-    qint64 pos = m_stdoutFile->pos();
-    qint64 errPos = m_stderrFile->pos();
-    createCmd.execute({"db-create", databaseFilename});
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", "-p", dbFilename});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
     // Output should be empty when there is an error.
     QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
-    QString errorMessage = QString("File " + databaseFilename + " already exists.\n");
+    QString errorMessage = QString("File " + dbFilename + " already exists.\n");
     QCOMPARE(m_stderrFile->readAll(), errorMessage.toUtf8());
 
+    // Should refuse to create without any key provided.
+    dbFilename = testDir->path() + "/testCreate_key.kdbx";
+    pos = m_stdoutFile->pos();
+    errPos = m_stderrFile->pos();
+    createCmd.execute({"db-create", dbFilename});
+    m_stdoutFile->seek(pos);
+    m_stderrFile->seek(errPos);
+    QCOMPARE(m_stdoutFile->readAll(), QByteArray(""));
+    QCOMPARE(m_stderrFile->readLine(), QByteArray("No key is set. Aborting database creation.\n"));
+
     // Testing with keyfile creation
-    QString databaseFilename2 = testDir->path() + "/testCreate2.kdbx";
+    dbFilename = testDir->path() + "/testCreate_key2.kdbx";
     QString keyfilePath = testDir->path() + "/keyfile.txt";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
-    createCmd.execute({"db-create", databaseFilename2, "-k", keyfilePath});
+    Utils::Test::setNextPassword("a", true);
+    createCmd.execute({"db-create", dbFilename, "-p", "-k", keyfilePath});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db2 =
-        QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename2, true, keyfilePath, "", Utils::DEVNULL));
-    QVERIFY(db2);
+    db = Utils::unlockDatabase(dbFilename, true, keyfilePath, "", Utils::DEVNULL);
+    QVERIFY(db);
 
     // Testing with existing keyfile
-    QString databaseFilename3 = testDir->path() + "/testCreate3.kdbx";
+    dbFilename = testDir->path() + "/testCreate_key3.kdbx";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
-    createCmd.execute({"db-create", databaseFilename3, "-k", keyfilePath});
+    Utils::Test::setNextPassword("a", true);
+    createCmd.execute({"db-create", dbFilename, "-p", "-k", keyfilePath});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully created new database.\n"));
 
     Utils::Test::setNextPassword("a");
-    auto db3 =
-        QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename3, true, keyfilePath, "", Utils::DEVNULL));
-    QVERIFY(db3);
+    db = Utils::unlockDatabase(dbFilename, true, keyfilePath, "", Utils::DEVNULL);
+    QVERIFY(db);
 
     // Invalid decryption time (format).
-    QString databaseFilename4 = testDir->path() + "/testCreate4.kdbx";
+    dbFilename = testDir->path() + "/testCreate_time.kdbx";
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    createCmd.execute({"create", databaseFilename4, "-t", "NAN"});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", "NAN"});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
@@ -642,7 +702,7 @@ void TestCli::testCreate()
     // Invalid decryption time (range).
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    createCmd.execute({"create", databaseFilename4, "-t", "10"});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", "10"});
     m_stdoutFile->seek(pos);
     m_stderrFile->seek(errPos);
 
@@ -653,9 +713,9 @@ void TestCli::testCreate()
     // Custom encryption time
     pos = m_stdoutFile->pos();
     errPos = m_stderrFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     int epochBefore = QDateTime::currentMSecsSinceEpoch();
-    createCmd.execute({"create", databaseFilename4, "-t", QString::number(encryptionTime)});
+    createCmd.execute({"db-create", dbFilename, "-p", "-t", QString::number(encryptionTime)});
     // Removing 100ms to make sure we account for changes in computation time.
     QVERIFY(QDateTime::currentMSecsSinceEpoch() > (epochBefore + encryptionTime - 100));
     m_stdoutFile->seek(pos);
@@ -663,12 +723,13 @@ void TestCli::testCreate()
 
     QCOMPARE(m_stderrFile->readAll(), QByteArray(""));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Benchmarking key derivation function for 500ms delay.\n"));
     QVERIFY(m_stdoutFile->readLine().contains(QByteArray("rounds for key derivation function.\n")));
 
     Utils::Test::setNextPassword("a");
-    auto db4 = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilename4, true, "", "", Utils::DEVNULL));
-    QVERIFY(db4);
+    db = Utils::unlockDatabase(dbFilename, true, "", "", Utils::DEVNULL);
+    QVERIFY(db);
 }
 
 void TestCli::testInfo()
@@ -1105,13 +1166,14 @@ void TestCli::testImport()
     QScopedPointer<QTemporaryDir> testDir(new QTemporaryDir());
     QString databaseFilename = testDir->path() + "testImport1.kdbx";
 
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     importCmd.execute({"import", m_xmlFile->fileName(), databaseFilename});
 
     m_stderrFile->reset();
     m_stdoutFile->reset();
 
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
     QCOMPARE(m_stdoutFile->readLine(), QByteArray("Successfully imported database.\n"));
 
     Utils::Test::setNextPassword("a");
@@ -1137,11 +1199,13 @@ void TestCli::testImport()
     QString databaseFilenameQuiet = testDirQuiet->path() + "testImport2.kdbx";
 
     pos = m_stdoutFile->pos();
-    Utils::Test::setNextPassword("a");
+    Utils::Test::setNextPassword("a", true);
     importCmd.execute({"import", "-q", m_xmlFile->fileName(), databaseFilenameQuiet});
     m_stdoutFile->seek(pos);
 
-    QCOMPARE(m_stdoutFile->readAll(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Enter password to encrypt database (optional): \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray("Repeat password: \n"));
+    QCOMPARE(m_stdoutFile->readLine(), QByteArray());
 
     Utils::Test::setNextPassword("a");
     auto dbQuiet = QSharedPointer<Database>(Utils::unlockDatabase(databaseFilenameQuiet, true, "", "", Utils::DEVNULL));
@@ -1544,11 +1608,11 @@ void TestCli::testMergeWithKeys()
 
     qint64 pos = m_stdoutFile->pos();
 
-    Utils::Test::setNextPassword("a");
-    createCmd.execute({"db-create", sourceDatabaseFilename, "-k", sourceKeyfilePath});
+    Utils::Test::setNextPassword("a", true);
+    createCmd.execute({"db-create", sourceDatabaseFilename, "-p", "-k", sourceKeyfilePath});
 
-    Utils::Test::setNextPassword("b");
-    createCmd.execute({"db-create", targetDatabaseFilename, "-k", targetKeyfilePath});
+    Utils::Test::setNextPassword("b", true);
+    createCmd.execute({"db-create", targetDatabaseFilename, "-p", "-k", targetKeyfilePath});
 
     Utils::Test::setNextPassword("a");
     auto sourceDatabase = QSharedPointer<Database>(