Refactoring: Cleanup creation of the credentials classes

src/cmd/cmd.cpp

@@ -279,7 +279,7 @@ void setupCredentials(SyncCTX &ctx)
         f.close();
     }
 
-    ctx.account->setCredentials(HttpCredentialsText::create(ctx.options.interactive, ctx.user, password));
+    ctx.account->setCredentials(HttpCredentialsText::create(ctx.account.get(), ctx.options.interactive, ctx.user, password));
     if (ctx.options.trustSSL) {
         QObject::connect(ctx.account->accessManager(), &QNetworkAccessManager::sslErrors, [](QNetworkReply *reply, const QList<QSslError> &errors) {
             reply->ignoreSslErrors(errors);

src/cmd/httpcredentialstext.cpp

@@ -74,8 +74,8 @@ QString queryPassword(const QString &user)
 }
 }
 
-HttpCredentialsText::HttpCredentialsText(const QString &user, const QString &password)
-    : OCC::HttpCredentials(OCC::DetermineAuthTypeJob::AuthType::Basic, user, password)
+HttpCredentialsText::HttpCredentialsText(OCC::Account *account, const QString &user, const QString &password)
+    : OCC::HttpCredentials(account, OCC::DetermineAuthTypeJob::AuthType::Basic, user, password)
 {
     if (user.isEmpty()) {
         qFatal("Invalid credentials: Username is empty");
@@ -85,7 +85,7 @@ HttpCredentialsText::HttpCredentialsText(const QString &user, const QString &pas
     }
 }
 
-HttpCredentialsText *HttpCredentialsText::create(bool interactive, const QString &_user, const QString &_password)
+HttpCredentialsText *HttpCredentialsText::create(OCC::Account *account, bool interactive, const QString &_user, const QString &_password)
 {
     QString user = _user;
     QString password = _password;
@@ -101,7 +101,7 @@ HttpCredentialsText *HttpCredentialsText::create(bool interactive, const QString
             password = queryPassword(user);
         }
     }
-    return new HttpCredentialsText(user, password);
+    return new HttpCredentialsText(account, user, password);
 }
 
 void HttpCredentialsText::askFromUser()

src/cmd/httpcredentialstext.h

@@ -17,16 +17,17 @@
  */
 #pragma once
 
+#include "accountfwd.h"
 #include "creds/httpcredentials.h"
 
 class HttpCredentialsText : public OCC::HttpCredentials
 {
     Q_OBJECT
 public:
-    static HttpCredentialsText *create(bool interactive, const QString &user, const QString &password);
+    static HttpCredentialsText *create(OCC::Account *account, bool interactive, const QString &user, const QString &password);
 
     void askFromUser() override;
 
 private:
-    HttpCredentialsText(const QString &user, const QString &password);
+    HttpCredentialsText(OCC::Account *account, const QString &user, const QString &password);
 };

src/gui/accountmanager.cpp

@@ -119,6 +119,11 @@ bool AccountManager::restore()
         if (auto acc = loadAccountHelper(*settings)) {
             acc->_id = accountId;
             if (auto accState = AccountState::loadFromSettings(acc, *settings)) {
+                // If we never fetched credentials, do that now - otherwise connection attempts
+                // make little sense.
+                if (!accState->account()->credentials()) {
+                    accState->account()->setCredentials(HttpCredentialsGui::fromSettings(accState.get()));
+                }
                 addAccountState(std::move(accState));
             }
         }
@@ -297,8 +302,6 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings)
             continue;
         acc->_settingsMap.insert(key, settings.value(key));
     }
-    acc->setCredentials(new HttpCredentialsGui);
-
     // now the server cert, it is in the general group
     settings.beginGroup(QStringLiteral("General"));
     const auto certs = QSslCertificate::fromData(settings.value(caCertsKeyC()).toByteArray());

src/gui/accountstate.cpp

@@ -27,6 +27,7 @@
 #include "gui/spacemigration.h"
 #include "gui/tlserrordialog.h"
 
+#include "creds/httpcredentialsgui.h"
 #include "logger.h"
 #include "settingsdialog.h"
 #include "socketapi/socketapi.h"
@@ -88,7 +89,6 @@ AccountState::AccountState(AccountPtr account)
     , _queueGuard(_account->jobQueue())
     , _state(AccountState::Disconnected)
     , _connectionStatus(ConnectionValidator::Undefined)
-    , _waitingForNewCredentials(false)
     , _maintenanceToConnectedDelay(1min + minutes(QRandomGenerator::global()->generate() % 4)) // 1-5min delay
 {
     qRegisterMetaType<AccountState *>("AccountState*");
@@ -166,6 +166,7 @@ std::unique_ptr<AccountState> AccountState::loadFromSettings(AccountPtr account,
         accountState->setState(SignedOut);
     }
     accountState->_supportsSpaces = settings.value(supportsSpacesC(), false).toBool();
+    // accountState->account()->setCredentials(new HttpCredentialsGui(accountState.get(), l));
     return accountState;
 }
 
@@ -273,7 +274,6 @@ void AccountState::freshConnectionAttempt()
 void AccountState::signIn()
 {
     if (_state == SignedOut) {
-        _waitingForNewCredentials = false;
         setState(Disconnected);
         // persist that we are no longer signed out
         Q_EMIT account()->wantsAccountSaved(account().data());
@@ -292,12 +292,12 @@ void AccountState::tagLastSuccessfullETagRequest(const QDateTime &tp)
 
 void AccountState::checkConnectivity(bool blockJobs)
 {
-    if (_state != Connected) {
-        setState(Connecting);
+    if (isSignedOut()) {
+        return;
     }
     qCWarning(lcAccountState) << "checkConnectivity blocking:" << blockJobs;
-    if (isSignedOut() || _waitingForNewCredentials) {
-        return;
+    if (_state != Connected) {
+        setState(Connecting);
     }
     if (_tlsDialog) {
         qCDebug(lcAccountState) << "Skip checkConnectivity, waiting for tls dialog";
@@ -314,12 +314,6 @@ void AccountState::checkConnectivity(bool blockJobs)
         return;
     }
 
-    // If we never fetched credentials, do that now - otherwise connection attempts
-    // make little sense.
-    if (!account()->credentials()->wasFetched()) {
-        _waitingForNewCredentials = true;
-        account()->credentials()->fetchFromKeychain();
-    }
     if (account()->hasCapabilities()) {
         // IF the account is connected the connection check can be skipped
         // if the last successful etag check job is not so long ago.
@@ -356,7 +350,6 @@ void AccountState::checkConnectivity(bool blockJobs)
                 connect(_tlsDialog, &TlsErrorDialog::accepted, _tlsDialog, [certs, blockJobs, this]() {
                     _account->addApprovedCerts(certs);
                     _tlsDialog.clear();
-                    _waitingForNewCredentials = false;
                     checkConnectivity(blockJobs);
                 });
                 connect(_tlsDialog, &TlsErrorDialog::rejected, this, [certs, this]() {
@@ -382,7 +375,8 @@ void AccountState::checkConnectivity(bool blockJobs)
         }
     } else {
         // Check the server and then the auth.
-        if (_waitingForNewCredentials) {
+        // TODO
+        if (false) {
             mode = ConnectionValidator::ValidationMode::ValidateServer;
         } else {
             _connectionValidator->setClearCookies(true);
@@ -518,7 +512,9 @@ void AccountState::slotCredentialsFetched()
     qCInfo(lcAccountState) << "Fetched credentials for" << _account->url().toString()
                            << "attempting to connect";
     _waitingForNewCredentials = false;
-    checkConnectivity();
+    if (!isSignedOut()) {
+        checkConnectivity();
+    }
 }
 
 void AccountState::slotCredentialsAsked()

src/gui/accountstate.h

@@ -181,7 +181,7 @@ protected Q_SLOTS:
     State _state;
     ConnectionStatus _connectionStatus;
     QStringList _connectionErrors;
-    bool _waitingForNewCredentials;
+    bool _waitingForNewCredentials = false;
     QDateTime _timeOfLastETagCheck;
     QPointer<ConnectionValidator> _connectionValidator;
     QPointer<UpdateUrlDialog> _updateUrlDialog;

src/gui/application.cpp

@@ -213,11 +213,8 @@ void Application::slotAccountStateAdded(AccountStatePtr accountState) const
     // Hook up the folder manager slots to the account state's signals:
     connect(accountState.data(), &AccountState::stateChanged,
         _folderManager.data(), &FolderMan::slotAccountStateChanged);
-    connect(accountState->account().data(), &Account::serverVersionChanged,
-        _folderManager.data(), [account = accountState->account().data()] {
-            FolderMan::instance()->slotServerVersionChanged(account);
-        });
-    accountState->checkConnectivity();
+    connect(accountState->account().data(), &Account::serverVersionChanged, _folderManager.data(),
+        [account = accountState->account().data()] { FolderMan::instance()->slotServerVersionChanged(account); });
 }
 
 void Application::slotCleanup()

src/gui/creds/httpcredentialsgui.cpp

@@ -14,6 +14,7 @@
  */
 
 #include "creds/httpcredentialsgui.h"
+
 #include "account.h"
 #include "application.h"
 #include "basicloginwidget.h"
@@ -29,11 +30,28 @@
 #include <QNetworkReply>
 #include <QTimer>
 
+namespace {
+auto isOAuthC()
+{
+    return QStringLiteral("oauth");
+}
+
+const QString userC()
+{
+    return QStringLiteral("user");
+}
+}
+
 
 namespace OCC {
 
 Q_LOGGING_CATEGORY(lcHttpCredentialsGui, "sync.credentials.http.gui", QtInfoMsg)
 
+HttpCredentialsGui::HttpCredentialsGui(Account *account)
+    : HttpCredentials(account)
+{
+}
+
 void HttpCredentialsGui::openBrowser()
 {
     OC_ASSERT(isUsingOAuth());
@@ -58,11 +76,13 @@ void HttpCredentialsGui::askFromUser()
 
 void HttpCredentialsGui::askFromUserAsync()
 {
+    // TODO are we logged out
+    //_account
     if (isUsingOAuth()) {
         restartOAuth();
     } else {
         // First, we will check what kind of auth we need.
-        auto job = new DetermineAuthTypeJob(_account->sharedFromThis(), this);
+        auto job = new DetermineAuthTypeJob(account()->sharedFromThis(), this);
         QObject::connect(job, &DetermineAuthTypeJob::authType, this, [this](DetermineAuthTypeJob::AuthType type) {
             _authType = type;
             if (type == DetermineAuthTypeJob::AuthType::OAuth) {
@@ -114,7 +134,7 @@ void HttpCredentialsGui::showDialog()
     // make sure it's cleaned up since it's not owned by the account settings (also prevents memory leaks)
     dialog->setAttribute(Qt::WA_DeleteOnClose);
 
-    dialog->setTopLabelText(tr("Please enter your password to log in to the account %1.").arg(_account->displayName()));
+    dialog->setTopLabelText(tr("Please enter your password to log in to the account %1.").arg(account()->displayName()));
 
     auto *contentWidget = qobject_cast<BasicLoginWidget *>(dialog->contentWidget());
     contentWidget->forceUsername(user());
@@ -160,7 +180,7 @@ QUrl HttpCredentialsGui::authorisationLink() const
 
 void HttpCredentialsGui::restartOAuth()
 {
-    _asyncAuth.reset(new AccountBasedOAuth(_account->sharedFromThis(), this));
+    _asyncAuth.reset(new AccountBasedOAuth(account()->sharedFromThis(), this));
     connect(_asyncAuth.data(), &OAuth::result,
         this, &HttpCredentialsGui::asyncAuthResult);
     connect(_asyncAuth.data(), &OAuth::destroyed,
@@ -169,4 +189,33 @@ void HttpCredentialsGui::restartOAuth()
     emit authorisationLinkChanged();
 }
 
+HttpCredentialsGui::HttpCredentialsGui(AccountState *accountState, const QString &loginUser, const QString &password)
+    : HttpCredentials(accountState->account().get(), DetermineAuthTypeJob::AuthType::Basic, loginUser, password)
+{
+}
+
+HttpCredentialsGui::HttpCredentialsGui(AccountState *accountState, const QString &davUser, const QString &password, const QString &refreshToken)
+    : HttpCredentials(accountState->account().get(), DetermineAuthTypeJob::AuthType::OAuth, davUser, password)
+{
+    _refreshToken = refreshToken;
+}
+
+HttpCredentialsGui *HttpCredentialsGui::fromSettings(AccountState *accountState)
+{
+    auto *out = new HttpCredentialsGui(accountState->account().get());
+    out->_user = accountState->account()->credentialSetting(out, userC()).toString();
+    Q_ASSERT(!out->_user.isEmpty());
+
+    const auto isOauth = accountState->account()->credentialSetting(out, isOAuthC()).toBool();
+    out->_authType = isOauth ? DetermineAuthTypeJob::AuthType::OAuth : DetermineAuthTypeJob::AuthType::Basic;
+
+    out->fetchFromKeychain();
+    return out;
+}
+void HttpCredentialsGui::persist()
+{
+    account()->setCredentialSetting(this, userC(), _user);
+    account()->setCredentialSetting(this, isOAuthC(), isUsingOAuth());
+    HttpCredentials::persist();
+}
 } // namespace OCC

src/gui/creds/httpcredentialsgui.h

@@ -29,23 +29,17 @@ class HttpCredentialsGui : public HttpCredentials
 {
     Q_OBJECT
 public:
-    HttpCredentialsGui()
-        : HttpCredentials()
-    {
-    }
-    HttpCredentialsGui(const QString &loginUser, const QString &password)
-        : HttpCredentials(DetermineAuthTypeJob::AuthType::Basic, loginUser, password)
-    {
-    }
-
-    HttpCredentialsGui(const QString &davUser, const QString &password, const QString &refreshToken)
-        : HttpCredentials(DetermineAuthTypeJob::AuthType::OAuth, davUser, password)
-    {
-        _refreshToken = refreshToken;
-    }
+    static HttpCredentialsGui *fromSettings(AccountState *accountState);
+
+    HttpCredentialsGui(AccountState *accountState, const QString &loginUser, const QString &password);
+
+    HttpCredentialsGui(AccountState *accountState, const QString &davUser, const QString &password, const QString &refreshToken);
 
     void openBrowser();
 
+
+    void persist() override;
+
     QUrl authorisationLink() const;
 
     /**
@@ -69,6 +63,10 @@ private slots:
 
     void oAuthErrorOccurred();
 
+protected:
+    HttpCredentialsGui(Account *account);
+    ;
+
 private:
     QScopedPointer<AccountBasedOAuth, QScopedPointerObjectDeleteLater<AccountBasedOAuth>> _asyncAuth;
     QPointer<QWidget> _loginRequiredDialog;

src/gui/newwizard/setupwizardaccountbuilder.cpp

@@ -37,9 +37,9 @@ HttpBasicAuthenticationStrategy::HttpBasicAuthenticationStrategy(const QString &
 {
 }
 
-HttpCredentialsGui *HttpBasicAuthenticationStrategy::makeCreds()
+HttpCredentialsGui *HttpBasicAuthenticationStrategy::makeCreds(AccountState *accountState)
 {
-    return new HttpCredentialsGui(loginUser(), _password);
+    return new HttpCredentialsGui(accountState, loginUser(), _password);
 }
 
 bool HttpBasicAuthenticationStrategy::isValid()
@@ -68,10 +68,10 @@ OAuth2AuthenticationStrategy::OAuth2AuthenticationStrategy(const QString &token,
 {
 }
 
-HttpCredentialsGui *OAuth2AuthenticationStrategy::makeCreds()
+HttpCredentialsGui *OAuth2AuthenticationStrategy::makeCreds(AccountState *accountState)
 {
     Q_ASSERT(isValid());
-    return new HttpCredentialsGui(davUser(), _token, _refreshToken);
+    return new HttpCredentialsGui(accountState, davUser(), _token, _refreshToken);
 }
 
 bool OAuth2AuthenticationStrategy::isValid()
@@ -111,7 +111,7 @@ void SetupWizardAccountBuilder::setLegacyWebFingerUsername(const QString &userna
     _legacyWebFingerUsername = username;
 }
 
-AccountPtr SetupWizardAccountBuilder::build()
+AccountStatePtr SetupWizardAccountBuilder::build()
 {
     auto newAccountPtr = Account::create(QUuid::createUuid());
 
@@ -127,8 +127,6 @@ AccountPtr SetupWizardAccountBuilder::build()
 
     // TODO: perhaps _authenticationStrategy->setUpAccountPtr(...) would be more elegant? no need for getters then
     newAccountPtr->setDavUser(_authenticationStrategy->davUser());
-    newAccountPtr->setCredentials(_authenticationStrategy->makeCreds());
-
     newAccountPtr->setDavDisplayName(_displayName);
 
     newAccountPtr->addApprovedCerts({ _customTrustedCaCertificates.begin(), _customTrustedCaCertificates.end() });
@@ -137,7 +135,10 @@ AccountPtr SetupWizardAccountBuilder::build()
         newAccountPtr->setDefaultSyncRoot(_defaultSyncTargetDir);
     }
 
-    return newAccountPtr;
+    auto accountState = AccountManager::instance()->addAccount(newAccountPtr);
+    newAccountPtr->setCredentials(_authenticationStrategy->makeCreds(accountState));
+
+    return accountState;
 }
 
 bool SetupWizardAccountBuilder::hasValidCredentials() const