diff --git a/src/cmd/cmd.cpp b/src/cmd/cmd.cpp index 28184c0ed69..25e5356bcf9 100644 --- a/src/cmd/cmd.cpp +++ b/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 &errors) { reply->ignoreSslErrors(errors); diff --git a/src/cmd/httpcredentialstext.cpp b/src/cmd/httpcredentialstext.cpp index 4c6a80482d9..6621d138827 100644 --- a/src/cmd/httpcredentialstext.cpp +++ b/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() diff --git a/src/cmd/httpcredentialstext.h b/src/cmd/httpcredentialstext.h index 7f4dc3de494..98e4ce83292 100644 --- a/src/cmd/httpcredentialstext.h +++ b/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); }; diff --git a/src/gui/accountmanager.cpp b/src/gui/accountmanager.cpp index 8422e850707..3b8a43c9ab1 100644 --- a/src/gui/accountmanager.cpp +++ b/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()); diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index 392e3d7f448..9b710344856 100644 --- a/src/gui/accountstate.cpp +++ b/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*"); @@ -166,6 +166,7 @@ std::unique_ptr 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() diff --git a/src/gui/accountstate.h b/src/gui/accountstate.h index abe5d81936e..618eb612a6e 100644 --- a/src/gui/accountstate.h +++ b/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; QPointer _updateUrlDialog; diff --git a/src/gui/application.cpp b/src/gui/application.cpp index 34646b5cf87..076960bddd0 100644 --- a/src/gui/application.cpp +++ b/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() diff --git a/src/gui/creds/httpcredentialsgui.cpp b/src/gui/creds/httpcredentialsgui.cpp index f3d0c466423..7c90d624f58 100644 --- a/src/gui/creds/httpcredentialsgui.cpp +++ b/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 #include +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(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 diff --git a/src/gui/creds/httpcredentialsgui.h b/src/gui/creds/httpcredentialsgui.h index 1c09872e180..ff9167862b0 100644 --- a/src/gui/creds/httpcredentialsgui.h +++ b/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> _asyncAuth; QPointer _loginRequiredDialog; diff --git a/src/gui/newwizard/setupwizardaccountbuilder.cpp b/src/gui/newwizard/setupwizardaccountbuilder.cpp index fb3821e17ba..beff6d38018 100644 --- a/src/gui/newwizard/setupwizardaccountbuilder.cpp +++ b/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 diff --git a/src/gui/newwizard/setupwizardaccountbuilder.h b/src/gui/newwizard/setupwizardaccountbuilder.h index e9ba094970e..4aad548c7e0 100644 --- a/src/gui/newwizard/setupwizardaccountbuilder.h +++ b/src/gui/newwizard/setupwizardaccountbuilder.h @@ -35,7 +35,7 @@ class AbstractAuthenticationStrategy * Create credentials object for use in the account. * @return credentials */ - virtual HttpCredentialsGui *makeCreds() = 0; + virtual HttpCredentialsGui *makeCreds(AccountState *accountState) = 0; /** * Checks whether the passed credentials are valid. @@ -60,7 +60,7 @@ class HttpBasicAuthenticationStrategy : public AbstractAuthenticationStrategy public: explicit HttpBasicAuthenticationStrategy(const QString &username, const QString &password); - HttpCredentialsGui *makeCreds() override; + HttpCredentialsGui *makeCreds(AccountState *accountState) override; bool isValid() override; @@ -84,7 +84,7 @@ class OAuth2AuthenticationStrategy : public AbstractAuthenticationStrategy public: explicit OAuth2AuthenticationStrategy(const QString &token, const QString &refreshToken); - HttpCredentialsGui *makeCreds() override; + HttpCredentialsGui *makeCreds(AccountState *accountState) override; bool isValid() override; @@ -168,7 +168,7 @@ class SetupWizardAccountBuilder * Attempt to build an account from the previously entered information. * @return built account or null if information is still missing */ - AccountPtr build(); + AccountStatePtr build(); void setWebFingerAuthenticationServerUrl(const QUrl &url); QUrl webFingerAuthenticationServerUrl() const; diff --git a/src/gui/newwizard/setupwizardcontroller.h b/src/gui/newwizard/setupwizardcontroller.h index 413939e84e3..96869a869a5 100644 --- a/src/gui/newwizard/setupwizardcontroller.h +++ b/src/gui/newwizard/setupwizardcontroller.h @@ -51,7 +51,7 @@ class SetupWizardController : public QObject /** * Emitted when the wizard has finished. It passes the built account object. */ - void finished(AccountPtr newAccount, SyncMode syncMode, const QVariantMap &dynamicRegistrationData); + void finished(AccountStatePtr newAccountState, SyncMode syncMode, const QVariantMap &dynamicRegistrationData); private: void changeStateTo(SetupWizardState nextState, SetupWizardControllerPrivate::ChangeReason reason = SetupWizardControllerPrivate::ChangeReason::Default); diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp index e3fec643488..e5184449c36 100644 --- a/src/gui/owncloudgui.cpp +++ b/src/gui/owncloudgui.cpp @@ -839,16 +839,15 @@ void ownCloudGui::runNewAccountWizard() FolderMan::instance()->setSyncEnabled(false); connect(_wizardController, &Wizard::SetupWizardController::finished, ocApp(), - [this](AccountPtr newAccount, Wizard::SyncMode syncMode, const QVariantMap &dynamicRegistrationData) { + [this](AccountStatePtr accountStatePtr, Wizard::SyncMode syncMode, const QVariantMap &dynamicRegistrationData) { // note: while the wizard is shown, we disable the folder synchronization // previously we could perform this just here, but now we have to postpone this depending on whether selective sync was chosen // see also #9497 // when the dialog is closed before it has finished, there won't be a new account to set up // the wizard controller signalizes this by passing a null pointer - if (!newAccount.isNull()) { + if (!accountStatePtr.isNull()) { // finally, call the slot that finalizes the setup - auto accountStatePtr = ocApp()->addNewAccount(newAccount); accountStatePtr->setSettingUp(true); // ensure we are connected and fetch the capabilities diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 384dd1ec275..5864c263c76 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -183,7 +183,6 @@ void Account::setCredentials(AbstractCredentials *cred) // The order for these two is important! Reading the credential's // settings accesses the account as well as account->_credentials, _credentials.reset(cred); - cred->setAccount(this); _am = _credentials->createAM(); @@ -248,13 +247,17 @@ QNetworkReply *Account::sendRawRequest(const QByteArray &verb, const QUrl &url, void Account::setApprovedCerts(const QList &certs) { _approvedCerts = { certs.begin(), certs.end() }; - _am->setCustomTrustedCaCertificates(_approvedCerts); + if (_am) { + _am->setCustomTrustedCaCertificates(_approvedCerts); + } } void Account::addApprovedCerts(const QSet &certs) { _approvedCerts.unite(certs); - _am->setCustomTrustedCaCertificates(_approvedCerts); + if (_am) { + _am->setCustomTrustedCaCertificates(_approvedCerts); + } Q_EMIT wantsAccountSaved(this); } @@ -263,25 +266,19 @@ void Account::setUrl(const QUrl &url) _url = url; } -QVariant Account::credentialSetting(const QString &key) const +QVariant Account::credentialSetting(AbstractCredentials *credentials, const QString &key) const { - if (_credentials) { - QString prefix = _credentials->authType(); - QVariant value = _settingsMap.value(prefix + QLatin1Char('_') + key); - if (value.isNull()) { - value = _settingsMap.value(key); - } - return value; + QVariant value = _settingsMap.value(credentials->authType() + QLatin1Char('_') + key); + qDebug() << value << credentials->authType() + QLatin1Char('_') + key; + if (value.isNull()) { + value = _settingsMap.value(key); } - return QVariant(); + return value; } -void Account::setCredentialSetting(const QString &key, const QVariant &value) +void Account::setCredentialSetting(AbstractCredentials *credentials, const QString &key, const QVariant &value) { - if (_credentials) { - QString prefix = _credentials->authType(); - _settingsMap.insert(prefix + QLatin1Char('_') + key, value); - } + _settingsMap.insert(credentials->authType() + QLatin1Char('_') + key, value); } JobQueue *Account::jobQueue() diff --git a/src/libsync/account.h b/src/libsync/account.h index 8bcc3e39b1e..eb17981de53 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -168,8 +168,9 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject void addApprovedCerts(const QSet &certs); // To be called by credentials only, for storing username and the like - QVariant credentialSetting(const QString &key) const; - void setCredentialSetting(const QString &key, const QVariant &value); + // TODO: hndle this sanely + QVariant credentialSetting(AbstractCredentials *credentials, const QString &key) const; + void setCredentialSetting(AbstractCredentials *credentials, const QString &key, const QVariant &value); /** Access the server capabilities */ const Capabilities &capabilities() const; diff --git a/src/libsync/creds/abstractcredentials.cpp b/src/libsync/creds/abstractcredentials.cpp index cc82beb9698..e42a3056a5f 100644 --- a/src/libsync/creds/abstractcredentials.cpp +++ b/src/libsync/creds/abstractcredentials.cpp @@ -24,16 +24,11 @@ namespace OCC { Q_LOGGING_CATEGORY(lcCredentials, "sync.credentials", QtInfoMsg) -AbstractCredentials::AbstractCredentials() - : _account(nullptr) +AbstractCredentials::AbstractCredentials(Account *account) + : QObject(account) + , _account(account) , _wasFetched(false) { } -void AbstractCredentials::setAccount(Account *account) -{ - OC_ENFORCE_X(!_account, "should only setAccount once"); - _account = account; -} - } // namespace OCC diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h index 023a1a1e917..2b59027a3c9 100644 --- a/src/libsync/creds/abstractcredentials.h +++ b/src/libsync/creds/abstractcredentials.h @@ -33,17 +33,9 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject Q_OBJECT public: - AbstractCredentials(); + AbstractCredentials(Account *account); // No need for virtual destructor - QObject already has one. - /** The bound account for the credentials instance. - * - * Credentials are always used in conjunction with an account. - * Calling Account::setCredentials() will call this function. - * Credentials only live as long as the underlying account object. - */ - virtual void setAccount(Account *account); - virtual QString authType() const = 0; virtual QString user() const = 0; virtual AccessManager *createAM() const = 0; @@ -54,12 +46,6 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject /** Whether fetchFromKeychain() was called before. */ bool wasFetched() const { return _wasFetched; } - /** Trigger (async) fetching of credential information - * - * Should set _wasFetched = true, and later emit fetched() when done. - */ - virtual void fetchFromKeychain() = 0; - /** Ask credentials from the user (typically async) * * Should emit asked() when done. @@ -88,6 +74,8 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject */ virtual void forgetSensitiveData() = 0; + Account *account() const { return _account; } + Q_SIGNALS: /** Emitted when fetchFromKeychain() is done. * @@ -108,8 +96,16 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject void requestLogout(); protected: - Account *_account; + /** Trigger (async) fetching of credential information + * + * Should set _wasFetched = true, and later emit fetched() when done. + */ + virtual void fetchFromKeychain() = 0; + bool _wasFetched; + +private: + Account *_account = nullptr; }; } // namespace OCC diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp index f04374c3343..db57a12f8b0 100644 --- a/src/libsync/creds/httpcredentials.cpp +++ b/src/libsync/creds/httpcredentials.cpp @@ -40,11 +40,6 @@ constexpr int TokenRefreshMaxRetries = 3; constexpr int CredentialVersion = 1; const char authenticationFailedC[] = "owncloud-authentication-failed"; -auto isOAuthC() -{ - return QStringLiteral("oauth"); -} - auto passwordKeyC() { return QStringLiteral("http/password"); @@ -59,11 +54,6 @@ auto CredentialVersionKey() { return QStringLiteral("CredentialVersion"); } - -const QString userC() -{ - return QStringLiteral("user"); -} } namespace OCC { @@ -101,8 +91,14 @@ class HttpCredentialsAccessManager : public AccessManager QPointer _cred; }; -HttpCredentials::HttpCredentials(DetermineAuthTypeJob::AuthType authType, const QString &user, const QString &password) - : _user(user) +HttpCredentials::HttpCredentials(Account *account) + : AbstractCredentials(account) +{ +} + +HttpCredentials::HttpCredentials(Account *account, DetermineAuthTypeJob::AuthType authType, const QString &user, const QString &password) + : AbstractCredentials(account) + , _user(user) , _password(password) , _ready(true) , _authType(authType) @@ -119,18 +115,6 @@ QString HttpCredentials::user() const return _user; } -void HttpCredentials::setAccount(Account *account) -{ - AbstractCredentials::setAccount(account); - if (_user.isEmpty()) { - fetchUser(); - } - const auto isOauth = account->credentialSetting(isOAuthC()); - if (isOauth.isValid()) { - _authType = isOauth.toBool() ? DetermineAuthTypeJob::AuthType::OAuth : DetermineAuthTypeJob::AuthType::Basic; - } -} - AccessManager *HttpCredentials::createAM() const { AccessManager *am = new HttpCredentialsAccessManager(this); @@ -146,25 +130,10 @@ bool HttpCredentials::ready() const return _ready; } -QString HttpCredentials::fetchUser() -{ - // it makes no sense to overwrite an existing username with a config file value - if (_user.isEmpty()) { - qCDebug(lcHttpCredentials) << "user not set, populating from settings"; - _user = _account->credentialSetting(userC()).toString(); - } else { - qCDebug(lcHttpCredentials) << "user already set, no need to fetch from settings"; - } - return _user; -} - void HttpCredentials::fetchFromKeychain() { _wasFetched = true; - // User must be fetched from config file - fetchUser(); - if (!_ready && !_refreshToken.isEmpty()) { // This happens if the credentials are still loaded from the keychain, bur we are called // here because the auth is invalid, so this means we simply need to refresh the credentials @@ -182,7 +151,7 @@ void HttpCredentials::fetchFromKeychain() void HttpCredentials::fetchFromKeychainHelper() { Q_ASSERT(!_user.isEmpty()); - auto job = _account->credentialManager()->get(isUsingOAuth() ? refreshTokenKeyC() : passwordKeyC()); + auto job = account()->credentialManager()->get(isUsingOAuth() ? refreshTokenKeyC() : passwordKeyC()); connect(job, &CredentialJob::finished, this, [job, this] { auto handleError = [job, this] { qCWarning(lcHttpCredentials) << "Could not retrieve client password from keychain" << job->errorString(); @@ -267,7 +236,7 @@ bool HttpCredentials::refreshAccessTokenInternal(int tokenRefreshRetriesCount) // _ready = false; // parent with nam to ensure we reset when the nam is reset - _oAuthJob = new AccountBasedOAuth(_account->sharedFromThis(), _account->accessManager()); + _oAuthJob = new AccountBasedOAuth(account()->sharedFromThis(), account()->accessManager()); connect(_oAuthJob, &AccountBasedOAuth::refreshError, this, [tokenRefreshRetriesCount, this](QNetworkReply::NetworkError error, const QString &) { _oAuthJob->deleteLater(); int nextTry = tokenRefreshRetriesCount + 1; @@ -337,11 +306,8 @@ void HttpCredentials::invalidateToken() _password = QString(); _ready = false; - // User must be fetched from config file to generate a valid key - fetchUser(); - // clear the session cookie. - _account->clearCookieJar(); + account()->clearCookieJar(); if (!_refreshToken.isEmpty()) { // Only invalidate the access_token (_password) but keep the _refreshToken in the keychain @@ -349,13 +315,13 @@ void HttpCredentials::invalidateToken() return; } - _account->credentialManager()->clear(QStringLiteral("http")); + account()->credentialManager()->clear(QStringLiteral("http")); // let QNAM forget about the password // This needs to be done later in the event loop because we might be called (directly or // indirectly) from QNetworkAccessManagerPrivate::authenticationRequired, which itself // is a called from a BlockingQueuedConnection from the Qt HTTP thread. And clearing the // cache needs to synchronize again with the HTTP thread. - QTimer::singleShot(0, _account, &Account::clearAMCache); + QTimer::singleShot(0, account(), &Account::clearAMCache); } void HttpCredentials::forgetSensitiveData() @@ -373,20 +339,18 @@ void HttpCredentials::persist() // We never connected or fetched the user, there is nothing to save. return; } - _account->setCredentialSetting(CredentialVersionKey(), CredentialVersion); - _account->setCredentialSetting(userC(), _user); - _account->setCredentialSetting(isOAuthC(), isUsingOAuth()); - Q_EMIT _account->wantsAccountSaved(_account); + account()->setCredentialSetting(this, CredentialVersionKey(), CredentialVersion); + Q_EMIT account()->wantsAccountSaved(account()); // write secrets to the keychain if (isUsingOAuth()) { // _refreshToken should only be empty when we are logged out... if (!_refreshToken.isEmpty()) { - _account->credentialManager()->set(refreshTokenKeyC(), _refreshToken); + account()->credentialManager()->set(refreshTokenKeyC(), _refreshToken); } } else { if (!_password.isEmpty()) { - _account->credentialManager()->set(passwordKeyC(), _password); + account()->credentialManager()->set(passwordKeyC(), _password); } } } diff --git a/src/libsync/creds/httpcredentials.h b/src/libsync/creds/httpcredentials.h index 8fe60e71586..37fbe66f540 100644 --- a/src/libsync/creds/httpcredentials.h +++ b/src/libsync/creds/httpcredentials.h @@ -51,7 +51,7 @@ class OWNCLOUDSYNC_EXPORT HttpCredentials : public AbstractCredentials /// Don't add credentials if this is set on a QNetworkRequest static constexpr QNetworkRequest::Attribute DontAddCredentialsAttribute = QNetworkRequest::User; - explicit HttpCredentials(DetermineAuthTypeJob::AuthType authType, const QString &user, const QString &password); + explicit HttpCredentials(Account *account, DetermineAuthTypeJob::AuthType authType, const QString &user, const QString &password); QString authType() const override; AccessManager *createAM() const override; @@ -62,20 +62,17 @@ class OWNCLOUDSYNC_EXPORT HttpCredentials : public AbstractCredentials QString user() const override; void invalidateToken() override; void forgetSensitiveData() override; - QString fetchUser(); /* If we still have a valid refresh token, try to refresh it assynchronously and emit fetched() * otherwise return false */ bool refreshAccessToken(); - // To fetch the user name as early as possible - void setAccount(Account *account) override; - // Whether we are using OAuth bool isUsingOAuth() const { return _authType == DetermineAuthTypeJob::AuthType::OAuth; } + protected: - HttpCredentials() = default; + HttpCredentials(Account *account); void slotAuthentication(QNetworkReply *reply, QAuthenticator *authenticator); void fetchFromKeychainHelper(); diff --git a/test/testoauth.cpp b/test/testoauth.cpp index 5b9de1c2d0e..9cd590a9169 100644 --- a/test/testoauth.cpp +++ b/test/testoauth.cpp @@ -127,7 +127,7 @@ class OAuthTestCase : public QObject account->setUrl(sOAuthTestServer); // the account seizes ownership over the qnam in account->setCredentials(...) by keeping a shared pointer on it // therefore, we should never call fakeAm->setThis(...) - account->setCredentials(new FakeCredentials { fakeAm }); + account->setCredentials(new FakeCredentials{account.get(), fakeAm}); fakeAm->setOverride([this](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *device) { if (req.url().path().endsWith(QLatin1String(".well-known/openid-configuration"))) { return this->wellKnownReply(op, req); diff --git a/test/testutils/syncenginetestutils.cpp b/test/testutils/syncenginetestutils.cpp index 3e92ae9a300..ed947edb7c4 100644 --- a/test/testutils/syncenginetestutils.cpp +++ b/test/testutils/syncenginetestutils.cpp @@ -941,7 +941,7 @@ FakeFolder::FakeFolder(const FileInfo &fileTemplate, OCC::Vfs::Mode vfsMode, boo _fakeAm = new FakeAM(fileTemplate, this); _accountState = std::move(OCC::TestUtils::createDummyAccount()); - account()->setCredentials(new FakeCredentials{_fakeAm}); + account()->setCredentials(new FakeCredentials{_accountState->account().get(), _fakeAm}); _journalDb.reset(new OCC::SyncJournalDb(localPath() + QStringLiteral(".sync_test.db"))); // TODO: davUrl diff --git a/test/testutils/syncenginetestutils.h b/test/testutils/syncenginetestutils.h index bba3439ea5c..cbb65f98f9d 100644 --- a/test/testutils/syncenginetestutils.h +++ b/test/testutils/syncenginetestutils.h @@ -541,8 +541,9 @@ class FakeAM : public OCC::AccessManager class FakeCredentials : public OCC::AbstractCredentials { public: - FakeCredentials(OCC::AccessManager *am) - : _am { am } + FakeCredentials(OCC::Account *account, OCC::AccessManager *am) + : OCC::AbstractCredentials(account) + , _am{am} { } diff --git a/test/testutils/testutils.cpp b/test/testutils/testutils.cpp index 62fbe862334..33d1079bf3d 100644 --- a/test/testutils/testutils.cpp +++ b/test/testutils/testutils.cpp @@ -11,8 +11,8 @@ namespace { class HttpCredentialsTest : public OCC::HttpCredentials { public: - HttpCredentialsTest(const QString &user, const QString &password) - : HttpCredentials(OCC::DetermineAuthTypeJob::AuthType::Basic, user, password) + HttpCredentialsTest(OCC::Account *account, const QString &user, const QString &password) + : HttpCredentials(account, OCC::DetermineAuthTypeJob::AuthType::Basic, user, password) { } @@ -29,7 +29,7 @@ namespace TestUtils { { // don't use the account manager to create the account, it would try to use widgets auto acc = Account::create(QUuid::createUuid()); - HttpCredentialsTest *cred = new HttpCredentialsTest(QStringLiteral("testuser"), QStringLiteral("secret")); + HttpCredentialsTest *cred = new HttpCredentialsTest(acc.get(), QStringLiteral("testuser"), QStringLiteral("secret")); acc->setCredentials(cred); acc->setUrl(QUrl(QStringLiteral("http://localhost/owncloud"))); acc->setDavDisplayName(QStringLiteral("fakename") + acc->uuid().toString(QUuid::WithoutBraces));