fix(account/setup): repair account creation via CLI

Setting up an account with --userid/--apppassword/--serverurl never
succeeded on macOS. Four independent defects were involved:

- Options were only accepted as "--option value". Argument lists are now
  normalised with Utility::expandCommandLineOptionValues(), so the
  "--option=value" spelling works for every option in both the client and
  nextcloudcmd. The latter has to normalise before it checks for --userid,
  which is what selects provisioning mode.

- --localdirpath is documented as optional but an empty one was rejected
  outright, although the setup already had a branch for accounts without a
  folder. It now falls back to the folder the account wizard would suggest.

- FolderMan::addFolder() refuses to create a classic sync folder while the
  app-level File Provider mode is enabled, yet the setup always tried to
  create one, so it always failed. localSyncFolderRequired() now mirrors
  that guard and the account is set up without a folder in that mode.

- 09f4692 added the account and its sync folder before the credentials
  were checked. Setting up the folder ends by scheduling qApp->quit(), so
  the credential check, the account save and the keychain write never ran.
  The account is now only added once the credentials validate, which also
  means a failed setup no longer leaves a half-written account behind.

Two related fixes: a failed folder setup called deleteAccount(), revoking
the app password that was passed on the command line, and the client
overwrote the configured VFS setting on every start because the account
setup parser instance is never null.

nextcloudcmd now runs the event loop so the asynchronous setup can finish.
Its keychain jobs never signal completion, which the previous immediate
return hid; the wait for the credentials is bounded so provisioning ends
with a warning instead of hanging.

Signed-off-by: Iva Horn <iva.horn@nextcloud.com>

Author: i2h3

src/cmd/cmd.cpp

@@ -228,7 +228,8 @@ CommandMode parseOptions(const QStringList &app_args, CmdOptions *options)
 {
     auto result = CommandMode::UnknownMode;
 
-    auto args(app_args);
+    // Accept both "--option value" and "--option=value" for every option below.
+    auto args = Utility::expandCommandLineOptionValues(app_args);
 
     const auto argCount = args.count();
 
@@ -446,19 +447,22 @@ int main(int argc, char **argv)
             return -1;
         }
 
-        if (AccountSetupCommandLineManager::instance()->isCommandLineParsed()) {
-            if (AccountSetupCommandLineManager::instance()->setupAccountFromCommandLine()) {
-                return 0;
-            } else {
-                qWarning() << "Creation of the account failed. See prior messages for a detailed error.";
-                return -1;
-            }
-        } else {
+        if (!AccountSetupCommandLineManager::instance()->isCommandLineParsed()) {
             AccountSetupCommandLineManager::destroy();
             qWarning() << "Missing mandatory command line options for provisioning mode";
             help();
             return -1;
         }
+
+        if (!AccountSetupCommandLineManager::instance()->setupAccountFromCommandLine()) {
+            qWarning() << "Creation of the account failed. See prior messages for a detailed error.";
+            return -1;
+        }
+
+        // Setting up the account validates the credentials against the server and stores
+        // them in the keychain, both of which are asynchronous. The setup job ends the
+        // event loop with the exit code once it has finished.
+        return app.exec();
     }
 
     AccountPtr account = Account::create();

src/common/utility.cpp

@@ -681,5 +681,28 @@ QString Utility::fullRemotePathToRemoteSyncRootRelative(const QString &fullRemot
     return noLeadingSlashPath(noTrailingSlashPath(relativePathToRemoteSyncRoot));
 }
 
+QStringList Utility::expandCommandLineOptionValues(const QStringList &arguments)
+{
+    QStringList expandedArguments;
+    expandedArguments.reserve(arguments.size());
+
+    for (const auto &argument : arguments) {
+        // Anything that is not a long option is passed through untouched: paths and custom
+        // URI scheme arguments may legitimately contain a '='.
+        const auto separator = argument.startsWith(QLatin1String("--")) ? argument.indexOf(QLatin1Char('=')) : -1;
+        if (separator < 3) {
+            expandedArguments.append(argument);
+            continue;
+        }
+
+        expandedArguments.append(argument.left(separator));
+
+        if (const auto value = argument.mid(separator + 1); !value.isEmpty()) {
+            expandedArguments.append(value);
+        }
+    }
+
+    return expandedArguments;
+}
 
 } // namespace OCC

src/common/utility.h

@@ -336,6 +336,19 @@ namespace Utility {
     OCSYNC_EXPORT QString noTrailingSlashPath(const QString &path);
     OCSYNC_EXPORT QString fullRemotePathToRemoteSyncRootRelative(const QString &fullRemotePath, const QString &remoteSyncRoot);
 
+    /**
+     * @brief Splits "--option=value" arguments into a separate "--option" and "value" entry
+     *
+     * The option parsers of the client and of nextcloudcmd walk the argument list with an
+     * iterator and expect the value of an option in the entry that follows it. Normalising
+     * the list before parsing makes both spellings work without teaching every single
+     * option about the inline form.
+     *
+     * Only entries starting with "--" are split, and only at their first '='. "--option="
+     * keeps no value so that the parsers report their usual "not specified" error.
+     */
+    OCSYNC_EXPORT QStringList expandCommandLineOptionValues(const QStringList &arguments);
+
 #ifdef Q_OS_WIN
     OCSYNC_EXPORT bool registryKeyExists(HKEY hRootKey, const QString &subKey);
     OCSYNC_EXPORT QVariant registryGetKeyValue(HKEY hRootKey, const QString &subKey, const QString &valueName);

src/gui/accountsetupfromcommandlinejob.cpp

@@ -7,24 +7,34 @@
 
 #include "accountmanager.h"
 #include "accountstate.h"
+#include "common/filesystembase.h"
+#include "configfile.h"
 #include "creds/abstractcredentials.h"
 #include "creds/webflowcredentials.h"
-#include "common/filesystembase.h"
 #include "folder.h"
 #include "folderman.h"
 #include "networkjobs.h"
+#include "theme.h"
+
+#include <chrono>
+#include <iostream>
 
 #include <QDir>
 #include <QGuiApplication>
 #include <QJsonObject>
 #include <QLoggingCategory>
+#include <QTimer>
 
 using namespace Qt::StringLiterals;
 
 namespace OCC
 {
 Q_LOGGING_CATEGORY(lcAccountSetupCommandLineJob, "nextcloud.gui.accountsetupcommandlinejob", QtInfoMsg)
 
+// How long to wait for the keychain to confirm that the credentials were written before
+// finishing the setup without them.
+constexpr auto credentialsPersistTimeout = std::chrono::seconds(30);
+
 AccountSetupFromCommandLineJob::AccountSetupFromCommandLineJob(QString appPassword,
                                                                QString userId,
                                                                QUrl serverUrl,
@@ -42,37 +52,77 @@ AccountSetupFromCommandLineJob::AccountSetupFromCommandLineJob(QString appPasswo
 {
 }
 
+bool AccountSetupFromCommandLineJob::localSyncFolderRequired() const
+{
+#ifdef BUILD_FILE_PROVIDER_MODULE
+    // Mirrors the guard in FolderMan::addFolder(): while the app-level File Provider mode
+    // is enabled, a classic sync folder cannot be created at all. The account is synced
+    // through the File Provider domain that is set up for it instead.
+    return !ConfigFile().macFileProviderModeEnabled();
+#else
+    return true;
+#endif
+}
+
+QString AccountSetupFromCommandLineJob::defaultLocalDirPath() const
+{
+    const auto overrideLocalDir = ConfigFile().overrideLocalDir();
+    auto localDirPath = overrideLocalDir;
+
+    if (localDirPath.isEmpty()) {
+        localDirPath = Theme::instance()->defaultClientFolder();
+
+        if (localDirPath.isEmpty()) {
+            return {};
+        }
+
+        if (!QDir(localDirPath).isAbsolute()) {
+            localDirPath = QDir::homePath() + QLatin1Char('/') + localDirPath;
+        }
+    }
+
+    auto serverUrlForFolder = _serverUrl;
+    serverUrlForFolder.setUserName(_userId);
+
+    return FolderMan::instance()->findGoodPathForNewSyncFolder(localDirPath,
+                                                               serverUrlForFolder,
+                                                               overrideLocalDir.isEmpty() ? FolderMan::GoodPathStrategy::AllowOnlyNewPath
+                                                                                          : FolderMan::GoodPathStrategy::AllowOverrideExistingPath);
+}
+
 bool AccountSetupFromCommandLineJob::handleAccountSetupFromCommandLine()
 {
     if (AccountManager::instance()->accountFromUserId(QStringLiteral("%1@%2").arg(_userId, _serverUrl.host()))) {
         printAccountSetupFromCommandLineStatusAndExit(QStringLiteral("Account %1 already exists!").arg(QDir::toNativeSeparators(_userId)), true);
         return false;
     }
 
-    if (_localDirPath.isEmpty()) {
-        printAccountSetupFromCommandLineStatusAndExit(
-            QStringLiteral("Folder creation failed. Could not create local folder because the name is empty"),
-            true);
-        return false;
+    if (!localSyncFolderRequired()) {
+        if (!_localDirPath.isEmpty()) {
+            qCInfo(lcAccountSetupCommandLineJob) << "Ignoring the given local folder, File Provider mode is enabled";
+            _localDirPath.clear();
+        }
     } else {
-        QDir dir(_localDirPath);
-        if (dir.exists() && !dir.isEmpty()) {
+        if (_localDirPath.isEmpty()) {
+            // The local folder is documented as optional, so fall back to the folder the
+            // account wizard would suggest rather than refusing to set the account up.
+            _localDirPath = defaultLocalDirPath();
+        }
+
+        if (_localDirPath.isEmpty()) {
             printAccountSetupFromCommandLineStatusAndExit(
-                QStringLiteral("Local folder %1 already exists and is non-empty!").arg(QDir::toNativeSeparators(_localDirPath)),
+                QStringLiteral("Could not determine a local folder to sync into. Please pass one with --localdirpath."),
                 true);
             return false;
         }
 
-        qCInfo(lcAccountSetupCommandLineJob) << "Creating folder" << _localDirPath;
-        if (!dir.exists() && !dir.mkpath(".")) {
+        const QDir localDir(_localDirPath);
+        if (localDir.exists() && !localDir.isEmpty()) {
             printAccountSetupFromCommandLineStatusAndExit(
-                QStringLiteral("Folder creation failed. Could not create local folder %1").arg(QDir::toNativeSeparators(_localDirPath)),
+                QStringLiteral("Local folder %1 already exists and is non-empty!").arg(QDir::toNativeSeparators(_localDirPath)),
                 true);
             return false;
         }
-
-        FileSystem::setFolderMinimumPermissions(_localDirPath);
-        Utility::setupFavLink(_localDirPath);
     }
 
     const auto credentials = new WebFlowCredentials(_userId, _appPassword);
@@ -81,12 +131,16 @@ bool AccountSetupFromCommandLineJob::handleAccountSetupFromCommandLine()
     _account->setCredentials(credentials);
     _account->setCredentialSetting(u"user"_s, _userId);
     _account->setUrl(_serverUrl);
-    auto accountState = AccountManager::instance()->addAccount(_account);
-    setupLocalSyncFolder(accountState);
-
-    Q_EMIT _account->wantsAccountSaved(_account);
 
+    // The account is only added, saved and given a sync folder once the credentials have
+    // been checked against the server, so that a failed setup leaves nothing behind. Both
+    // that check and the keychain write are asynchronous: the job keeps running until
+    // printAccountSetupFromCommandLineStatusAndExit() ends the event loop.
     if (_appPassword.isEmpty()) {
+        // Nothing to authenticate with, so the server cannot be asked for the dav user
+        // either. Store what was given and let the user log in from the client later on.
+        _account->setDavUser(_userId);
+        accountSetupFromCommandLinePropfindHandleSuccess();
         return true;
     }
 
@@ -112,19 +166,40 @@ void AccountSetupFromCommandLineJob::accountSetupFromCommandLinePropfindHandleSu
     const auto accountManager = AccountManager::instance();
     const auto accountState = accountManager->addAccount(_account);
 
-    // credentials->persist() (called by save()) is asynchronous — it chains
-    // multiple keychain write jobs before the password actually lands in the
-    // keychain.  Wait for the final write to complete before exiting so that
-    // the credentials are not lost when the process quits.
-    connect(_account->credentials(), &AbstractCredentials::credentialsPersisted, this, [this, accountState]() {
+    const auto finishAccountSetup = [this, accountState]() {
         if (!_localDirPath.isEmpty()) {
             setupLocalSyncFolder(accountState);
         } else {
             qCInfo(lcAccountSetupCommandLineJob) << QStringLiteral("Set up a new account without a folder.");
             printAccountSetupFromCommandLineStatusAndExit(QStringLiteral("Account %1 setup from command line success.").arg(_account->displayName()), false);
         }
+    };
+
+    // credentials->persist() (called by save()) is asynchronous — it chains
+    // multiple keychain write jobs before the password actually lands in the
+    // keychain.  Wait for the final write to complete before exiting so that
+    // the credentials are not lost when the process quits, but give up eventually:
+    // a keychain that never answers must not turn the setup into a hang.
+    const auto credentialsPersistTimer = new QTimer(this);
+    credentialsPersistTimer->setSingleShot(true);
+
+    connect(_account->credentials(), &AbstractCredentials::credentialsPersisted, this, [credentialsPersistTimer, finishAccountSetup]() {
+        if (!credentialsPersistTimer->isActive()) {
+            return;
+        }
+
+        credentialsPersistTimer->stop();
+        finishAccountSetup();
+    });
+
+    connect(credentialsPersistTimer, &QTimer::timeout, this, [finishAccountSetup]() {
+        qCWarning(lcAccountSetupCommandLineJob) << "Timed out waiting for the credentials to be written to the keychain,"
+                                                << "the account may have to be authenticated again";
+        finishAccountSetup();
     });
 
+    credentialsPersistTimer->start(credentialsPersistTimeout);
+
     accountManager->save();
 }
 
@@ -183,6 +258,22 @@ void AccountSetupFromCommandLineJob::accountSetupFromCommandLinePropfindHandleFa
 
 void AccountSetupFromCommandLineJob::setupLocalSyncFolder(AccountState *accountState)
 {
+    QDir localDir(_localDirPath);
+    if (!localDir.exists()) {
+        qCInfo(lcAccountSetupCommandLineJob) << "Creating folder" << _localDirPath;
+
+        if (!localDir.mkpath(QStringLiteral("."))) {
+            AccountManager::instance()->removeAccountState(accountState);
+            printAccountSetupFromCommandLineStatusAndExit(
+                QStringLiteral("Folder creation failed. Could not create local folder %1").arg(QDir::toNativeSeparators(_localDirPath)),
+                true);
+            return;
+        }
+    }
+
+    FileSystem::setFolderMinimumPermissions(_localDirPath);
+    Utility::setupFavLink(_localDirPath);
+
     FolderDefinition definition;
     definition.localPath = _localDirPath;
     definition.targetPath = FolderDefinition::prepareTargetPath(!_remoteDirPath.isEmpty() ? _remoteDirPath : QStringLiteral("/"));
@@ -209,7 +300,9 @@ void AccountSetupFromCommandLineJob::setupLocalSyncFolder(AccountState *accountS
         qCInfo(lcAccountSetupCommandLineJob) << QStringLiteral("Folder %1 setup from command line success.").arg(definition.localPath);
         printAccountSetupFromCommandLineStatusAndExit(QStringLiteral("Account %1 setup from command line success.").arg(_account->displayName()), false);
     } else {
-        AccountManager::instance()->deleteAccount(accountState);
+        // Drop the account again, but keep the app password valid: it was passed in on the
+        // command line and revoking it would make a retry impossible.
+        AccountManager::instance()->removeAccountState(accountState);
         printAccountSetupFromCommandLineStatusAndExit(
             QStringLiteral("Account %1 setup from command line failed, due to folder creation failure.").arg(_account->displayName()),
             true);
@@ -220,8 +313,10 @@ void AccountSetupFromCommandLineJob::printAccountSetupFromCommandLineStatusAndEx
 {
     if (isFailure) {
         qCWarning(lcAccountSetupCommandLineJob) << status;
+        std::cerr << qUtf8Printable(status) << std::endl;
     } else {
         qCInfo(lcAccountSetupCommandLineJob) << status;
+        std::cout << qUtf8Printable(status) << std::endl;
     }
     QTimer::singleShot(0, this, [this, isFailure]() {
         this->deleteLater();

src/gui/accountsetupfromcommandlinejob.h

@@ -24,7 +24,7 @@ class AccountSetupFromCommandLineJob : public QObject
                                    QString userId,
                                    QUrl serverUrl,
                                    QString localDirPath = {},
-                                   bool nonVfsMode = false,
+                                   bool isVfsEnabled = false,
                                    QString remoteDirPath = QStringLiteral("/"),
                                    QObject *parent = nullptr);
 
@@ -45,6 +45,20 @@ private Q_SLOTS:
     void fetchUserName();
 
 private:
+    /** Whether a classic sync folder has to be set up for the new account.
+     *
+     * With the app-level File Provider mode enabled the account is synced through its
+     * File Provider domain and FolderMan refuses to add a classic sync folder.
+     */
+    [[nodiscard]] bool localSyncFolderRequired() const;
+
+    /** The local folder to use when none was given on the command line.
+     *
+     * Follows what the account wizard suggests: the configured override, otherwise the
+     * theme's default client folder, made unique against the existing sync folders.
+     */
+    [[nodiscard]] QString defaultLocalDirPath() const;
+
     QString _appPassword;
     QString _userId;
     QUrl _serverUrl;

src/gui/application.cpp

@@ -27,10 +27,11 @@
 #include "updater/ocupdater.h"
 #endif
 
+#include "common/utility.h"
+#include "common/vfs.h"
+#include "csync_exclude.h"
 #include "owncloudsetupwizard.h"
 #include "version.h"
-#include "csync_exclude.h"
-#include "common/vfs.h"
 
 #include "config.h"
 
@@ -389,7 +390,10 @@ Application::Application(int &argc, char **argv)
             shouldExit = true;
         }
 
-        if (AccountSetupCommandLineManager::instance()) {
+        // Only a command line that actually provisions an account carries a meaningful
+        // --isvfsenabled value; on a normal start this would overwrite the user's setting
+        // with the default of an unused parser.
+        if (AccountSetupCommandLineManager::instance()->isCommandLineParsed()) {
             cfg.setVfsEnabled(AccountSetupCommandLineManager::instance()->isVfsEnabled());
         }
 
@@ -1006,7 +1010,10 @@ void Application::slotActivateRequestedMessage(const QStringList &arguments, con
 
 void Application::parseOptions(const QStringList &options)
 {
-    QStringListIterator it(options);
+    // Accept both "--option value" and "--option=value" for every option below.
+    const auto expandedOptions = Utility::expandCommandLineOptionValues(options);
+
+    QStringListIterator it(expandedOptions);
     // skip file name;
     if (it.hasNext()) {
         it.next();