[DC-150] refactor Account shared pointer

src/gui/accountsettings.cpp

@@ -68,7 +68,7 @@ AccountSettings::AccountSettings(AccountState *accountState, QWidget *parent)
 
     // as usual we do too many things in the ctr and we need to eval all the code paths to make sure they handle
     // the QPointer properly, but as a stopgap to catch null states asap before they trickle down into other areas:
-    Q_ASSERT(_accountState);
+    Q_ASSERT(_accountState && _accountState->account());
 
 
     _model = new FolderStatusModel(this);
@@ -83,19 +83,19 @@ AccountSettings::AccountSettings(AccountState *accountState, QWidget *parent)
 
     _sortModel = weightedModel;
 
-    ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(_accountState->account()));
+    ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(_accountState->account()->spacesManager()));
     ui->quickWidget->setOCContext(QUrl(QStringLiteral("qrc:/qt/qml/org/ownCloud/gui/qml/FolderDelegate.qml")), this);
 
     connect(FolderMan::instance(), &FolderMan::folderListChanged, _model, &FolderStatusModel::resetFolders);
 
     ui->connectionStatusLabel->clear();
 
-    connect(_accountState.data(), &AccountState::stateChanged, this, &AccountSettings::slotAccountStateChanged);
-    slotAccountStateChanged();
+    connect(_accountState, &AccountState::stateChanged, this, &AccountSettings::slotAccountStateChanged);
+    slotAccountStateChanged(_accountState->state());
 
     buildManageAccountMenu();
 
-    connect(_accountState.get(), &AccountState::isSettingUpChanged, this, &AccountSettings::accountSettingUpChanged);
+    connect(_accountState, &AccountState::isSettingUpChanged, this, &AccountSettings::accountSettingUpChanged);
 
     connect(ui->stackedWidget, &QStackedWidget::currentChanged, this,
         [this] { ui->manageAccountButton->setEnabled(ui->stackedWidget->currentWidget() == ui->quickWidget); });
@@ -257,11 +257,11 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder)
 
 void AccountSettings::showSelectiveSyncDialog(Folder *folder)
 {
-    if (!_accountState) {
+    if (!_accountState || !_accountState->account()) {
         return;
     }
 
-    auto *selectiveSync = new SelectiveSyncWidget(_accountState->account(), this);
+    auto *selectiveSync = new SelectiveSyncWidget(_accountState->account().get(), this);
     selectiveSync->setDavUrl(folder->webDavUrl());
     bool ok;
     selectiveSync->setFolderInfo(
@@ -279,11 +279,11 @@ void AccountSettings::showSelectiveSyncDialog(Folder *folder)
 
 void AccountSettings::slotAddFolder()
 {
-    if (!_accountState) {
+    if (!_accountState || !_accountState->account()) {
         return;
     }
 
-    FolderWizard *folderWizard = new FolderWizard(_accountState, this);
+    FolderWizard *folderWizard = new FolderWizard(_accountState->account().get(), this);
     folderWizard->setAttribute(Qt::WA_DeleteOnClose);
 
     connect(folderWizard, &QDialog::accepted, this, &AccountSettings::slotFolderWizardAccepted);
@@ -550,13 +550,12 @@ void AccountSettings::buildManageAccountMenu()
 }
 
 // Refactoring todo: the signal sends the new account state, refactor this to use that param
-void AccountSettings::slotAccountStateChanged()
+void AccountSettings::slotAccountStateChanged(AccountState::State state)
 {
-    if (!_accountState) {
+    if (!_accountState || !_accountState->account()) {
         return;
     }
 
-    const AccountState::State state = _accountState->state();
     const AccountPtr account = _accountState->account();
     qCDebug(lcAccountSettings) << "Account state changed to" << state << "for account" << account;
 

src/gui/accountsettings.h

@@ -78,7 +78,7 @@ class OWNCLOUDGUI_EXPORT AccountSettings : public QWidget
     void syncedSpacesChanged();
 
 public Q_SLOTS:
-    void slotAccountStateChanged();
+    void slotAccountStateChanged(AccountState::State state);
     void slotSpacesUpdated();
     void slotAddFolder();
 

src/gui/accountstate.cpp

@@ -311,7 +311,7 @@ void AccountState::checkConnectivity(bool blockJobs)
 
     // =======  here we setup a new ConnectionValidator
 
-    _connectionValidator = new ConnectionValidator(_account, this);
+    _connectionValidator = new ConnectionValidator(_account.get(), this);
     connect(_connectionValidator, &ConnectionValidator::connectionResult, this, &AccountState::slotConnectionValidatorResult);
     connect(_connectionValidator, &ConnectionValidator::sslErrors, this,
         [blockJobs, this](const QList<QSslError> &errors) { handleSslConnectionErrors(errors, blockJobs); });

src/gui/activitydata.cpp

@@ -19,12 +19,12 @@
 
 namespace OCC {
 
-Activity::Activity(Activity::Type type, const QString &id, AccountPtr acc, const QString &subject, const QString &message, const QString &file,
-    const QUrl &link, const QDateTime &dateTime, const QVector<ActivityLink> &&links)
+Activity::Activity(Activity::Type type, const QString &id, const QString &accName, const QUuid &uid, const QString &subject, const QString &message,
+    const QString &file, const QUrl &link, const QDateTime &dateTime, const QVector<ActivityLink> &&links)
     : _type(type)
     , _id(id)
-    , _accName(acc->displayNameWithHost())
-    , _uuid(acc->uuid())
+    , _accName(accName)
+    , _uuid(uid)
     , _subject(subject)
     , _message(message)
     , _file(file)

src/gui/activitydata.h

@@ -52,8 +52,8 @@ class OWNCLOUDGUI_EXPORT Activity
         NotificationType
     };
     Activity() = default;
-    explicit Activity(Type type, const QString &id, AccountPtr acc, const QString &subject, const QString &message, const QString &file, const QUrl &link,
-        const QDateTime &dateTime, const QVector<ActivityLink> &&links = {});
+    explicit Activity(Type type, const QString &id, const QString &accName, const QUuid &uid, const QString &subject, const QString &message,
+        const QString &file, const QUrl &link, const QDateTime &dateTime, const QVector<ActivityLink> &&links = {});
 
     Type type() const;
 

src/gui/connectionvalidator.cpp

@@ -15,7 +15,6 @@
 #include "gui/clientproxy.h"
 #include "gui/fetchserversettings.h"
 #include "gui/networkinformation.h"
-#include "gui/tlserrordialog.h"
 #include "libsync/account.h"
 #include "libsync/creds/abstractcredentials.h"
 #include "libsync/networkjobs.h"
@@ -42,7 +41,7 @@ namespace {
     };
 }
 
-ConnectionValidator::ConnectionValidator(AccountPtr account, QObject *parent)
+ConnectionValidator::ConnectionValidator(Account *account, QObject *parent)
     : QObject(parent)
     , _account(account)
     , _checkServerJob(nullptr)
@@ -112,9 +111,14 @@ void ConnectionValidator::systemProxyLookupDone(const QNetworkProxy &proxy)
 // The actual check
 void ConnectionValidator::slotCheckServerAndAuth()
 {
-    Q_ASSERT(_checkServerJob == nullptr);
-    auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account, _clearCookies, this);
+    if (!_account) {
+        qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted";
+        reportResult(NotConfigured);
+        return;
+    }
 
+    Q_ASSERT(_checkServerJob == nullptr);
+    auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account, _clearCookies);
     _checkServerJob = checkServerFactory.startJob(_account->url(), this);
 
     connect(_checkServerJob->reply()->manager(), &AccessManager::sslErrors, this, [this](QNetworkReply *reply, const QList<QSslError> &errors) {
@@ -127,6 +131,12 @@ void ConnectionValidator::slotCheckServerAndAuth()
 
 void ConnectionValidator::checkServerJobFinished()
 {
+    if (!_account) {
+        qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted";
+        reportResult(NotConfigured);
+        return;
+    }
+
     if (_checkServerJob->success()) {
         const auto result = _checkServerJob->result().value<CheckServerJobResult>();
 
@@ -169,6 +179,12 @@ void ConnectionValidator::checkServerJobFinished()
 
 void ConnectionValidator::statusFound(const QUrl &url, const QJsonObject &info)
 {
+    if (!_account) {
+        qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted";
+        reportResult(NotConfigured);
+        return;
+    }
+
     // status.php was found.
     qCInfo(lcConnectionValidator) << "** Application: ownCloud found: "
                                   << url << " with version "
@@ -206,10 +222,16 @@ void ConnectionValidator::checkAuthentication()
 {
     // simply GET the WebDAV root, will fail if credentials are wrong.
     // continue in slotAuthCheck here :-)
+    if (!_account) {
+        _errors << tr("No account configured");
+        reportResult(NotConfigured);
+        return;
+    }
 
     // we explicitly use a legacy dav path here - remote.php/webdav is available across all former, current and future server implementations (oc10, oCIS and
     // Kiteworks PDN)
-    auto *job = new PropfindJob(_account, _account->url(), QString("remote.php/webdav/"), PropfindJob::Depth::Zero, this);
+    auto *job = new PropfindJob(_account->sharedFromThis(), _account->url(), QString("remote.php/webdav/"), PropfindJob::Depth::Zero, this);
+
     job->setAuthenticationJob(true); // don't retry
     job->setTimeout(timeoutToUse());
     job->setProperties({ QByteArrayLiteral("getlastmodified") });
@@ -221,13 +243,19 @@ void ConnectionValidator::checkAuthentication()
 // Lisa todo: I hit this once and nothing happened in the gui?!
 void ConnectionValidator::slotAuthFailed()
 {
+    if (!_account) {
+        qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted";
+        reportResult(NotConfigured);
+        return;
+    }
+
     auto job = qobject_cast<PropfindJob *>(sender());
     Status stat = Timeout;
 
     if (job->reply()->error() == QNetworkReply::SslHandshakeFailedError) {
         _errors << job->errorStringParsingBody();
         stat = NetworkInformation::instance()->isBehindCaptivePortal() ? CaptivePortal : SslError;
-
+        // dc-150 credentials::stillValid always returns true. the logic here needs fix
     } else if (job->reply()->error() == QNetworkReply::AuthenticationRequiredError || !_account->credentials()->stillValid(job->reply())) {
         qCWarning(lcConnectionValidator) << "******** Password is wrong!" << job->reply()->error() << job;
         _errors << tr("The provided credentials are not correct");
@@ -249,9 +277,15 @@ void ConnectionValidator::slotAuthFailed()
 
 void ConnectionValidator::slotAuthSuccess()
 {
+    if (!_account) {
+        qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted";
+        reportResult(NotConfigured);
+        return;
+    }
+
     _errors.clear();
     if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) {
-        auto *fetchSetting = new FetchServerSettingsJob(_account, this);
+        auto *fetchSetting = new FetchServerSettingsJob(_account->sharedFromThis(), this);
         const auto unsupportedServerError = [this] {
             _errors.append({tr("The configured server for this client is too old."), tr("Please update to the latest server and restart the client.")});
         };

src/gui/connectionvalidator.h

@@ -17,11 +17,11 @@
 #include "gui/owncloudguilib.h"
 
 #include "common/chronoelapsedtimer.h"
-#include "gui/guiutility.h"
 #include "libsync/accountfwd.h"
 
 #include <QNetworkReply>
 #include <QObject>
+#include <QPointer>
 #include <QStringList>
 #include <QVariantMap>
 
@@ -84,7 +84,7 @@ class OWNCLOUDGUI_EXPORT ConnectionValidator : public QObject
 {
     Q_OBJECT
 public:
-    explicit ConnectionValidator(AccountPtr account, QObject *parent);
+    explicit ConnectionValidator(Account *account, QObject *parent);
     ~ConnectionValidator() override;
 
     enum class ValidationMode {
@@ -151,8 +151,9 @@ protected Q_SLOTS:
     void statusFound(const QUrl &url, const QJsonObject &info);
     void checkServerJobFinished();
 
+    QPointer<Account> _account;
+
     QStringList _errors;
-    AccountPtr _account;
     bool _clearCookies = false;
 
     Utility::ChronoElapsedTimer _duration;

src/gui/creds/credentials.cpp

@@ -75,7 +75,8 @@ class CredentialsAccessManager : public AccessManager
 private:
     // The credentials object dies along with the account, while the QNAM might
     // outlive both.
-    // Lisa todo: that statement is very concerning. investigate.
+    // Lisa todo: that statement is very concerning. investigate -> In fact the nam is parented by the creds by default so no, it
+    // should not outlive the creds unless there are shenanigans wrt reparenting it.
     QPointer<const Credentials> _cred;
 };
 
@@ -168,7 +169,7 @@ bool Credentials::stillValid(QNetworkReply *reply)
     if (reply->error() == QNetworkReply::AuthenticationRequiredError) {
         slotAuthentication(reply, nullptr);
     }
-        return true;
+    return true;
 }
 
 void Credentials::slotAuthentication(QNetworkReply *reply, QAuthenticator *authenticator)
@@ -283,9 +284,11 @@ bool Credentials::networkAvailable()
 
 void Credentials::refreshAccessTokenInternal()
 {
+    if (!_account)
+        return;
     // parent with nam to ensure we reset when the nam is reset
     // todo: #22 - the parenting here is highly questionable, as is the use of the shared account ptr
-    _oAuthJob = new AccountBasedOAuth(_account->sharedFromThis(), _account->accessManager());
+    _oAuthJob = new AccountBasedOAuth(_account, this);
     connect(_oAuthJob, &AccountBasedOAuth::refreshError, this, &Credentials::handleRefreshError);
     connect(_oAuthJob, &AccountBasedOAuth::refreshFinished, this, &Credentials::handleRefreshSuccess);
 
@@ -297,7 +300,7 @@ void Credentials::askFromUser()
 {
     // I think this can happen when the re-auth process has already quasi started and is waiting for user input, but
     // we get a prompt to log in again. I am not quite sure as it's very hard to reproduce.
-    if (_requestAuth) {
+    if (_requestAuth || !_account) {
         // let the existing instance ride
         return;
     }
@@ -311,7 +314,7 @@ void Credentials::askFromUser()
     // if the auth fails the gui stays until the user gets it right or clicks the stay logged out button.
     connect(_requestAuth, &RequestAuthenticationController::authenticationSucceeded, this, &Credentials::askFromUserSucceeded);
     connect(_requestAuth, &RequestAuthenticationController::requestLogout, this, &Credentials::askFromUserLogout);
-    _requestAuth->startAuthentication(_account->sharedFromThis());
+    _requestAuth->startAuthentication(_account);
 }
 
 void Credentials::askFromUserSucceeded(const QString &token, const QString &refreshToken)

src/gui/creds/credentials.h

@@ -46,9 +46,8 @@ class OWNCLOUDGUI_EXPORT Credentials : public AbstractCredentials
     // this access manager is monitored for authenticationRequired signal
     // it also becomes the access manager member in the account. Try to invert this so the account
     // creates it and connects the creds slot to the _am.
-    // it is ALSO used by the checkServerJobFactory where it sets the parent to the parent of that job. This is so
-    // shady and needs to be fixed, but we may have to leave that to a future refactoring.
-    // the main point is I am not sure the creds should be creating the nam - I think this should be an account responsibility in the end.
+    // note this is used by the checkServerJobFactory where a new nam is "needed" for every job due to options to clear cookies
+    // and other factors that mean we don't want to use the account's nam directly.
     AccessManager *createAccessManager() const override;
 
     // todo: DC-112 I think this needs a naming update.

src/gui/creds/oauth.cpp

@@ -476,7 +476,10 @@ void OAuth::openBrowser()
     }
 }
 
-AccountBasedOAuth::AccountBasedOAuth(AccountPtr account, QObject *parent)
+// todo: I was contemplating how we can make sure the passed account isn't null before we use it
+// to seed the OAuth ctr, and really, I'm not sure this should be a subclass of oauth in the first place. Instead it could simply use an
+// oauth instance to complete the tasks it can't do itself -> this could possibly be a "has a" not an "is a" impl
+AccountBasedOAuth::AccountBasedOAuth(Account *account, QObject *parent)
     : OAuth(account->url(), account->davUser(), account->accessManager(), parent)
     , _account(account)
 {
@@ -489,9 +492,14 @@ void AccountBasedOAuth::startAuthentication()
 
 void AccountBasedOAuth::fetchWellKnown()
 {
+    if (!_account) {
+        qCWarning(lcOauth) << "Unable to fetch well known, account has been deleted";
+        return;
+    }
+
     qCDebug(lcOauth) << "starting CheckServerJob before fetching" << wellKnownPathC;
 
-    auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account, true, this).startJob(_serverUrl, this);
+    auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account, true).startJob(_serverUrl, this);
 
     connect(checkServerJob, &CoreJob::finished, this, [checkServerJob, this]() {
         if (checkServerJob->success()) {
@@ -515,7 +523,8 @@ void AccountBasedOAuth::refreshAuthentication(const QString &refreshToken)
         return;
     }
 
-    // I don't see where this ever gets set to false
+    // I don't see where this ever gets set to false - seems to rely on a one shot run before creating a new instance where the value
+    // is initialized to false. hm.
     _isRefreshingToken = true;
 
 

src/gui/creds/oauth.h

@@ -63,7 +63,7 @@ class OAuth : public QObject
     Q_ENUM(PromptValuesSupported)
     Q_DECLARE_FLAGS(PromptValuesSupportedFlags, PromptValuesSupported)
 
-    OAuth(const QUrl &serverUrl, const QString &davUser, QNetworkAccessManager *networkAccessManager,QObject *parent);
+    OAuth(const QUrl &serverUrl, const QString &davUser, QNetworkAccessManager *networkAccessManager, QObject *parent);
     ~OAuth() override;
 
     virtual void startAuthentication();
@@ -146,7 +146,7 @@ class AccountBasedOAuth : public OAuth
     Q_OBJECT
 
 public:
-    explicit AccountBasedOAuth(AccountPtr account, QObject *parent = nullptr);
+    explicit AccountBasedOAuth(Account *account, QObject *parent);
 
     void startAuthentication() override;
 
@@ -162,7 +162,7 @@ class AccountBasedOAuth : public OAuth
     void fetchWellKnown() override;
 
 private:
-    AccountPtr _account;
+    QPointer<Account> _account;
 };
 
 QString toString(OAuth::PromptValuesSupportedFlags s);