Merge pull request #5416 from nextcloud/bugfix/remove-account-crash

Clean up account creation and deletion code

src/gui/accountmanager.cpp

@@ -249,7 +249,6 @@ void AccountManager::save(bool saveCredentials)
     for (const auto &acc : qAsConst(_accounts)) {
         settings->beginGroup(acc->account()->id());
         saveAccountHelper(acc->account().data(), *settings, saveCredentials);
-        acc->writeToSettings(*settings);
         settings->endGroup();
     }
 
@@ -274,7 +273,6 @@ void AccountManager::saveAccountState(AccountState *a)
     qCDebug(lcAccountManager) << "Saving account state" << a->account()->url().toString();
     const auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));
     settings->beginGroup(a->account()->id());
-    a->writeToSettings(*settings);
     settings->endGroup();
 
     settings->sync();

src/gui/accountsettings.cpp

@@ -1521,33 +1521,6 @@ void AccountSettings::refreshSelectiveSyncStatus()
     }
 }
 
-void AccountSettings::slotDeleteAccount()
-{
-    // Deleting the account potentially deletes 'this', so
-    // the QMessageBox should be destroyed before that happens.
-    const auto messageBox = new QMessageBox(QMessageBox::Question,
-                                            tr("Confirm Account Removal"),
-                                            tr("<p>Do you really want to remove the connection to the account <i>%1</i>?</p>"
-                                               "<p><b>Note:</b> This will <b>not</b> delete any files.</p>")
-                                            .arg(_accountState->account()->displayName()),
-                                            QMessageBox::NoButton,
-                                            this);
-    const auto yesButton = messageBox->addButton(tr("Remove connection"), QMessageBox::YesRole);
-    messageBox->addButton(tr("Cancel"), QMessageBox::NoRole);
-    messageBox->setAttribute(Qt::WA_DeleteOnClose);
-    connect(messageBox, &QMessageBox::finished, this, [this, messageBox, yesButton]{
-        if (messageBox->clickedButton() == yesButton) {
-            // Else it might access during destruction. This should be better handled by it having a QSharedPointer
-            _model->setAccountState(nullptr);
-
-            const auto manager = AccountManager::instance();
-            manager->deleteAccount(_accountState);
-            manager->save();
-        }
-    });
-    messageBox->open();
-}
-
 bool AccountSettings::event(QEvent *e)
 {
     if (e->type() == QEvent::Hide || e->type() == QEvent::Show) {

src/gui/accountsettings.h

@@ -93,7 +93,6 @@ protected slots:
     void slotSetSubFolderAvailability(OCC::Folder *folder, const QString &path, OCC::PinState state);
     void slotFolderWizardAccepted();
     void slotFolderWizardRejected();
-    void slotDeleteAccount();
     void slotToggleSignInState();
     void refreshSelectiveSyncStatus();
     void slotMarkSubfolderEncrypted(OCC::FolderStatusModel::SubFolderInfo *folderInfo);

src/gui/accountstate.cpp

@@ -83,10 +83,6 @@ AccountState *AccountState::loadFromSettings(AccountPtr account, QSettings & /*s
     return accountState;
 }
 
-void AccountState::writeToSettings(QSettings & /*settings*/)
-{
-}
-
 AccountPtr AccountState::account() const
 {
     return _account;

src/gui/accountstate.h

@@ -93,12 +93,6 @@ public:
      */
     static AccountState *loadFromSettings(AccountPtr account, QSettings &settings);
 
-    /** Writes account state information to settings.
-     *
-     * It does not write the Account data.
-     */
-    void writeToSettings(QSettings &settings);
-
     AccountPtr account() const;
 
     ConnectionStatus connectionStatus() const;

src/libsync/account.cpp

@@ -807,10 +807,14 @@ void Account::deleteAppPassword()
     job->setKey(kck);
     connect(job, &DeletePasswordJob::finished, [this](Job *incoming) {
         auto *deleteJob = dynamic_cast<DeletePasswordJob *>(incoming);
-        if (deleteJob->error() == NoError)
+        const auto jobError = deleteJob->error();
+        if (jobError == NoError) {
             qCInfo(lcAccount) << "appPassword deleted from keychain";
-        else
+        } else if (jobError == EntryNotFound) {
+            qCInfo(lcAccount) << "no appPassword entry found";
+        } else {
             qCWarning(lcAccount) << "Unable to delete appPassword from keychain" << deleteJob->errorString();
+        }
 
         // Allow storing a new app password on re-login
         _wroteAppPassword = false;

src/libsync/clientsideencryption.cpp

@@ -1102,6 +1102,11 @@ void ClientSideEncryption::forgetSensitiveData(const AccountPtr &account)
 {
     _publicKey = QSslKey();
 
+    if (!sensitiveDataRemaining()) {
+        checkAllSensitiveDataDeleted();
+        return;
+    }
+
     const auto createDeleteJob = [account](const QString user) {
         auto *job = new DeletePasswordJob(Theme::instance()->appName());
         job->setInsecureFallback(false);
@@ -1124,7 +1129,8 @@ void ClientSideEncryption::forgetSensitiveData(const AccountPtr &account)
 
 void ClientSideEncryption::handlePrivateKeyDeleted(const QKeychain::Job* const incoming)
 {
-    if (incoming->error() != QKeychain::NoError) {
+    const auto error = incoming->error();
+    if (error != QKeychain::NoError && error != QKeychain::EntryNotFound) {
         qCWarning(lcCse) << "Private key could not be deleted:" << incoming->errorString();
         return;
     }
@@ -1137,7 +1143,8 @@ void ClientSideEncryption::handlePrivateKeyDeleted(const QKeychain::Job* const i
 
 void ClientSideEncryption::handleCertificateDeleted(const QKeychain::Job* const incoming)
 {
-    if (incoming->error() != QKeychain::NoError) {
+    const auto error = incoming->error();
+    if (error != QKeychain::NoError && error != QKeychain::EntryNotFound) {
         qCWarning(lcCse) << "Certificate could not be deleted:" << incoming->errorString();
         return;
     }
@@ -1150,7 +1157,8 @@ void ClientSideEncryption::handleCertificateDeleted(const QKeychain::Job* const
 
 void ClientSideEncryption::handleMnemonicDeleted(const QKeychain::Job* const incoming)
 {
-    if (incoming->error() != QKeychain::NoError) {
+    const auto error = incoming->error();
+    if (error != QKeychain::NoError && error != QKeychain::EntryNotFound) {
         qCWarning(lcCse) << "Mnemonic could not be deleted:" << incoming->errorString();
         return;
     }
@@ -1161,17 +1169,23 @@ void ClientSideEncryption::handleMnemonicDeleted(const QKeychain::Job* const inc
     checkAllSensitiveDataDeleted();
 }
 
+bool ClientSideEncryption::sensitiveDataRemaining() const
+{
+    return !_privateKey.isEmpty() || !_certificate.isNull() || !_mnemonic.isEmpty();
+}
+
 void ClientSideEncryption::checkAllSensitiveDataDeleted()
 {
-    if (_privateKey.isEmpty() && _certificate.isNull() && _mnemonic.isEmpty()) {
-        qCDebug(lcCse) << "All sensitive encryption data has been deleted.";
-        Q_EMIT sensitiveDataForgotten();
-    }
-
+    if (sensitiveDataRemaining()) {
         qCDebug(lcCse) << "Some sensitive data emaining:"
                        << "Private key:" << _privateKey
                        << "Certificate is null:" << _certificate.isNull()
                        << "Mnemonic:" << _mnemonic;
+        return;
+    }
+
+    qCDebug(lcCse) << "All sensitive encryption data has been deleted.";
+    Q_EMIT sensitiveDataForgotten();
 }
 
 void ClientSideEncryption::generateKeyPair(const AccountPtr &account)

src/libsync/clientsideencryption.h

@@ -168,6 +168,7 @@ private:
 
     [[nodiscard]] bool checkPublicKeyValidity(const AccountPtr &account) const;
     [[nodiscard]] bool checkServerPublicKeyValidity(const QByteArray &serverPublicKeyString) const;
+    [[nodiscard]] bool sensitiveDataRemaining() const;
 
     bool isInitialized = false;
 };