Remove legacy ssl error handling

Author: TheOneRing

changelog/unreleased/9655

@@ -0,0 +1,5 @@
+Change: Rewrote ssl error handling
+
+We rewrote the way we handle ssl errors.
+
+https://github.com/owncloud/client/issues/9655

src/cmd/cmd.cpp

@@ -553,30 +553,44 @@ int main(int argc, char **argv)
 
     ctx.account->setUrl(ctx.credentialFreeUrl);
 
-    // Perform a call to get the capabilities.
-    auto capabilitiesJob = new JsonApiJob(ctx.account, QStringLiteral("ocs/v1.php/cloud/capabilities"), {}, {}, nullptr);
-    QObject::connect(capabilitiesJob, &JsonApiJob::finishedSignal, qApp, [capabilitiesJob, ctx] {
-        auto caps = capabilitiesJob->data().value("ocs").toObject().value("data").toObject().value("capabilities").toObject();
-        qDebug() << "Server capabilities" << caps;
-        ctx.account->setCapabilities(caps.toVariantMap());
-        ctx.account->setServerVersion(caps["core"].toObject()["status"].toObject()["version"].toString());
-
-        if (capabilitiesJob->reply()->error() != QNetworkReply::NoError) {
-            qFatal("Error connecting to server");
-        }
-
-        auto userJob = new JsonApiJob(ctx.account, QLatin1String("ocs/v1.php/cloud/user"), {}, {}, nullptr);
-        QObject::connect(userJob, &JsonApiJob::finishedSignal, qApp, [userJob, ctx] {
-            const QJsonObject data = userJob->data().value("ocs").toObject().value("data").toObject();
-            ctx.account->setDavUser(data.value("id").toString());
-            ctx.account->setDavDisplayName(data.value("display-name").toString());
+    auto *checkServer = new CheckServerJob(ctx.account, nullptr);
+    QObject::connect(checkServer, &CheckServerJob::instanceFound, [ctx] {
+        // Perform a call to get the capabilities.
+        auto *capabilitiesJob = new JsonApiJob(ctx.account, QStringLiteral("ocs/v1.php/cloud/capabilities"), {}, {}, nullptr);
+        QObject::connect(capabilitiesJob, &JsonApiJob::finishedSignal, qApp, [capabilitiesJob, ctx] {
+            auto caps = capabilitiesJob->data().value("ocs").toObject().value("data").toObject().value("capabilities").toObject();
+            qDebug() << "Server capabilities" << caps;
+            ctx.account->setCapabilities(caps.toVariantMap());
+            ctx.account->setServerVersion(caps["core"].toObject()["status"].toObject()["version"].toString());
+
+            if (capabilitiesJob->reply()->error() != QNetworkReply::NoError) {
+                qFatal("Error connecting to server");
+            }
 
-            // much lower age than the default since this utility is usually made to be run right after a change in the tests
-            SyncEngine::minimumFileAgeForUpload = std::chrono::milliseconds(0);
-            sync(ctx);
+            auto userJob = new JsonApiJob(ctx.account, QLatin1String("ocs/v1.php/cloud/user"), {}, {}, nullptr);
+            QObject::connect(userJob, &JsonApiJob::finishedSignal, qApp, [userJob, ctx] {
+                const QJsonObject data = userJob->data().value("ocs").toObject().value("data").toObject();
+                ctx.account->setDavUser(data.value("id").toString());
+                ctx.account->setDavDisplayName(data.value("display-name").toString());
+
+                // much lower age than the default since this utility is usually made to be run right after a change in the tests
+                SyncEngine::minimumFileAgeForUpload = std::chrono::milliseconds(0);
+                sync(ctx);
+            });
+            userJob->start();
         });
-        userJob->start();
+        capabilitiesJob->start();
+    });
+    QObject::connect(checkServer, &CheckServerJob::instanceNotFound, [ctx] {
+        qFatal("Failed to resolve %s.", qPrintable(ctx.account->url().toString()));
     });
-    capabilitiesJob->start();
+    QObject::connect(checkServer, &CheckServerJob::timedOut, [ctx] {
+        qFatal("Looking up %s timed out.", qPrintable(ctx.account->url().toString()));
+    });
+    QObject::connect(checkServer, &CheckServerJob::sslErrors, [ctx] {
+        qFatal(APPLICATION_EXECUTABLE "cmd currently does not support untrusted ssl certificates.");
+    });
+
+    checkServer->start();
     return app.exec();
 }

src/gui/accountsettings.cpp

@@ -220,7 +220,6 @@ Folder *AccountSettings::selectedFolder() const
 void AccountSettings::slotToggleSignInState()
 {
     if (_accountState->isSignedOut()) {
-        _accountState->account()->resetRejectedCertificates();
         _accountState->signIn();
     } else {
         _accountState->signOutByUi();

src/gui/accountstate.cpp

@@ -19,6 +19,8 @@
 #include "configfile.h"
 #include "creds/abstractcredentials.h"
 #include "creds/httpcredentials.h"
+#include "gui/settingsdialog.h"
+#include "gui/tlserrordialog.h"
 #include "logger.h"
 #include "settingsdialog.h"
 #include "theme.h"
@@ -262,6 +264,10 @@ void AccountState::checkConnectivity(bool blockJobs)
     if (isSignedOut() || _waitingForNewCredentials) {
         return;
     }
+    if (_tlsDialog) {
+        qCDebug(lcAccountState) << "Skip checkConnectivity, waiting for tls dialog";
+        return;
+    }
 
     if (_connectionValidator && blockJobs && !_queueGuard.queue()->isBlocked()) {
         // abort already running non blocking validator
@@ -278,22 +284,18 @@ void AccountState::checkConnectivity(bool blockJobs)
     if (!account()->credentials()->wasFetched()) {
         _waitingForNewCredentials = true;
         account()->credentials()->fetchFromKeychain();
-        return;
-    }
-    // we are not properly setup yet
-    if (!account()->hasCapabilities()) {
-        return;
     }
-
-    // IF the account is connected the connection check can be skipped
-    // if the last successful etag check job is not so long ago.
-    const auto pta = account()->capabilities().remotePollInterval();
-    const auto polltime = duration_cast<seconds>(ConfigFile().remotePollInterval(pta));
-    const auto elapsed = _timeOfLastETagCheck.secsTo(QDateTime::currentDateTimeUtc());
-    if (!blockJobs && isConnected() && _timeOfLastETagCheck.isValid()
-        && elapsed <= polltime.count()) {
-        qCDebug(lcAccountState) << account()->displayName() << "The last ETag check succeeded within the last " << polltime.count() << "s (" << elapsed << "s). No connection check needed!";
-        return;
+    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.
+        const auto pta = account()->capabilities().remotePollInterval();
+        const auto polltime = duration_cast<seconds>(ConfigFile().remotePollInterval(pta));
+        const auto elapsed = _timeOfLastETagCheck.secsTo(QDateTime::currentDateTimeUtc());
+        if (!blockJobs && isConnected() && _timeOfLastETagCheck.isValid()
+            && elapsed <= polltime.count()) {
+            qCDebug(lcAccountState) << account()->displayName() << "The last ETag check succeeded within the last " << polltime.count() << "s (" << elapsed << "s). No connection check needed!";
+            return;
+        }
     }
 
     if (blockJobs) {
@@ -302,6 +304,30 @@ void AccountState::checkConnectivity(bool blockJobs)
     _connectionValidator = new ConnectionValidator(account());
     connect(_connectionValidator, &ConnectionValidator::connectionResult,
         this, &AccountState::slotConnectionValidatorResult);
+
+    connect(_connectionValidator, &ConnectionValidator::sslErrors, this, [blockJobs, this](const QList<QSslError> &errors) {
+        if (!_tlsDialog) {
+            _tlsDialog = new TlsErrorDialog(errors, _account->url().host(), ocApp()->gui()->settingsDialog());
+            _tlsDialog->setAttribute(Qt::WA_DeleteOnClose);
+            QSet<QSslCertificate> certs;
+            certs.reserve(errors.size());
+            for (const auto &error : errors) {
+                certs << error.certificate();
+            }
+            connect(_tlsDialog, &TlsErrorDialog::accepted, _tlsDialog, [certs, blockJobs, this]() {
+                _account->addApprovedCerts(certs);
+                _tlsDialog.clear();
+                _waitingForNewCredentials = false;
+                checkConnectivity(blockJobs);
+            });
+            connect(_tlsDialog, &TlsErrorDialog::rejected, this, [certs, this]() {
+                setState(SignedOut);
+            });
+
+            _tlsDialog->show();
+        }
+        ocApp()->gui()->raiseDialog(_tlsDialog);
+    });
     if (isConnected()) {
         // Use a small authed propfind as a minimal ping when we're
         // already connected.
@@ -313,16 +339,12 @@ void AccountState::checkConnectivity(bool blockJobs)
         }
     } else {
         // Check the server and then the auth.
-
-        // Let's try this for all OS and see if it fixes the Qt issues we have on Linux  #4720 #3888 #4051
-        // There seems to be a bug in Qt on Windows where QNAM sometimes stops
-        // working correctly after the computer woke up from sleep. See #2895 #2899
-        // and #2973.
-        // As an attempted workaround, reset the QNAM regularly if the account is
-        // disconnected.
-        account()->clearCookieJar();
-        account()->resetAccessManager();
-        _connectionValidator->checkServerAndUpdate();
+        if (_waitingForNewCredentials) {
+            _connectionValidator->checkServer();
+        } else {
+            account()->clearCookieJar();
+            _connectionValidator->checkServerAndUpdate();
+        }
     }
 }
 
@@ -380,7 +402,7 @@ void AccountState::slotConnectionValidatorResult(ConnectionValidator::Status sta
         slotInvalidCredentials();
         break;
     case ConnectionValidator::SslError:
-        setState(SignedOut);
+        // handled with the tlsDialog
         break;
     case ConnectionValidator::ServiceUnavailable:
         _timeSinceMaintenanceOver.invalidate();

src/gui/accountstate.h

@@ -34,6 +34,7 @@ namespace OCC {
 
 class AccountState;
 class Account;
+class TlsErrorDialog;
 
 /**
  * @brief Extra info about an ownCloud server account.
@@ -169,6 +170,7 @@ protected Q_SLOTS:
     QDateTime _timeOfLastETagCheck;
     QPointer<ConnectionValidator> _connectionValidator;
     QPointer<UpdateUrlDialog> _updateUrlDialog;
+    QPointer<TlsErrorDialog> _tlsDialog;
 
     /**
      * Starts counting when the server starts being back up after 503 or

src/gui/connectionvalidator.cpp

@@ -22,10 +22,11 @@
 #include "account.h"
 #include "clientproxy.h"
 #include "connectionvalidator.h"
+#include "creds/abstractcredentials.h"
 #include "networkjobs.h"
 #include "networkjobs/jsonjob.h"
 #include "theme.h"
-#include <creds/abstractcredentials.h>
+#include "tlserrordialog.h"
 
 using namespace std::chrono_literals;
 
@@ -98,6 +99,8 @@ void ConnectionValidator::systemProxyLookupDone(const QNetworkProxy &proxy)
 // The actual check
 void ConnectionValidator::slotCheckServerAndAuth()
 {
+    // ensure we receive ssl errors
+    _account->resetAccessManager();
     CheckServerJob *checkJob = new CheckServerJob(_account, this);
     checkJob->setClearCookies(_clearCookies);
     checkJob->setTimeout(timeoutToUse);
@@ -108,6 +111,7 @@ void ConnectionValidator::slotCheckServerAndAuth()
         _errors.append(tr("timeout"));
         reportResult(Timeout);
     });
+    connect(checkJob, &CheckServerJob::sslErrors, this, &ConnectionValidator::sslErrors);
     checkJob->start();
 }
 
@@ -235,6 +239,7 @@ void ConnectionValidator::checkServerCapabilities()
 {
     // The main flow now needs the capabilities
     auto *job = new JsonApiJob(_account, QStringLiteral("ocs/v2.php/cloud/capabilities"), {}, {}, this);
+    job->setAuthenticationJob(true);
     job->setTimeout(timeoutToUse);
 
     QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, this] {
@@ -257,6 +262,7 @@ void ConnectionValidator::fetchUser()
 {
     auto *job = new JsonApiJob(_account, QLatin1String("ocs/v2.php/cloud/user"), {}, {}, this);
     job->setTimeout(timeoutToUse);
+    job->setAuthenticationJob(true);
     QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, this] {
         const QString user = job->data().value("ocs").toObject().value("data").toObject().value("id").toString();
         if (!user.isEmpty()) {
@@ -275,6 +281,7 @@ void ConnectionValidator::fetchUser()
 #ifndef TOKEN_AUTH_ONLY
         if (capabilities.isValid() && capabilities.avatarsAvailable()) {
             auto *job = new AvatarJob(_account, _account->davUser(), 128, this);
+            job->setAuthenticationJob(true);
             job->setTimeout(20s);
             QObject::connect(job, &AvatarJob::avatarPixmap, this, &ConnectionValidator::slotAvatarImage);
             job->start();

src/gui/connectionvalidator.h

@@ -112,6 +112,8 @@ public slots:
 signals:
     void connectionResult(ConnectionValidator::Status status, const QStringList &errors);
 
+    void sslErrors(const QList<QSslError> &errors);
+
 protected slots:
     /// Checks authentication only.
     void checkAuthentication();

src/gui/newwizard/jobs/resolveurljobfactory.cpp

@@ -16,6 +16,9 @@
 
 #include "accessmanager.h"
 #include "common/utility.h"
+#include "gui/application.h"
+#include "gui/owncloudgui.h"
+#include "gui/settingsdialog.h"
 #include "gui/tlserrordialog.h"
 #include "gui/updateurldialog.h"
 
@@ -100,7 +103,7 @@ CoreJob *ResolveUrlJobFactory::startJob(const QUrl &url)
     connect(reply, &QNetworkReply::finished, job, makeFinishedHandler(reply));
 
     connect(reply, &QNetworkReply::sslErrors, reply, [reply, req, job, makeFinishedHandler, nam = nam()](const QList<QSslError> &errors) mutable {
-        auto *tlsErrorDialog = new TlsErrorDialog(errors, reply->url().host());
+        auto *tlsErrorDialog = new TlsErrorDialog(errors, reply->url().host(), ocApp()->gui()->settingsDialog());
 
         reply->setProperty(abortedBySslErrorHandlerC, true);
         reply->abort();
@@ -118,6 +121,7 @@ CoreJob *ResolveUrlJobFactory::startJob(const QUrl &url)
         });
 
         tlsErrorDialog->show();
+        ocApp()->gui()->raiseDialog(tlsErrorDialog);
     });
 
     makeRequest();

src/gui/owncloudgui.cpp

@@ -960,7 +960,6 @@ void ownCloudGui::slotUpdateProgress(Folder *folder, const ProgressInfo &progres
 void ownCloudGui::slotLogin()
 {
     if (auto account = qvariant_cast<AccountStatePtr>(sender()->property(propertyAccountC))) {
-        account->account()->resetRejectedCertificates();
         account->signIn();
     } else {
         for (const auto &a : AccountManager::instance()->accounts()) {

src/gui/tlserrordialog.h

@@ -28,7 +28,7 @@ class TlsErrorDialog : public QDialog
     Q_OBJECT
 
 public:
-    explicit TlsErrorDialog(const QList<QSslError> &sslErrors, const QString &host, QWidget *parent = nullptr);
+    explicit TlsErrorDialog(const QList<QSslError> &sslErrors, const QString &host, QWidget *parent);
     ~TlsErrorDialog() override;
 
 private:

src/libsync/abstractnetworkjob.cpp

@@ -180,9 +180,6 @@ void AbstractNetworkJob::adoptRequest(QPointer<QNetworkReply> reply)
 void AbstractNetworkJob::slotFinished()
 {
     _finished = true;
-    if (_reply->error() == QNetworkReply::SslHandshakeFailedError) {
-        qCWarning(lcNetworkJob) << "SslHandshakeFailedError:" << errorString() << ": can be caused by a webserver wanting SSL client certificates";
-    }
     if (_reply->error() != QNetworkReply::NoError) {
         if (_account->jobQueue()->retry(this)) {
             qCDebug(lcNetworkJob) << "Queuing: " << _reply->url() << " for retry";

src/libsync/accessmanager.cpp

@@ -78,14 +78,14 @@ QNetworkReply *AccessManager::createRequest(QNetworkAccessManager::Operation op,
         newRequest.setAttribute(QNetworkRequest::Http2AllowedAttribute, http2EnabledEnv);
     }
 
-    // for some reason, passing an empty list causes the default chain to be removed
-    // this behavior does not match the documentation
     auto sslConfiguration = newRequest.sslConfiguration();
 
     sslConfiguration.setSslOption(QSsl::SslOptionDisableSessionTickets, false);
     sslConfiguration.setSslOption(QSsl::SslOptionDisableSessionSharing, false);
     sslConfiguration.setSslOption(QSsl::SslOptionDisableSessionPersistence, false);
     if (!_customTrustedCaCertificates.isEmpty()) {
+        // for some reason, passing an empty list causes the default chain to be removed
+        // this behavior does not match the documentation
         sslConfiguration.addCaCertificates({ _customTrustedCaCertificates.begin(), _customTrustedCaCertificates.end() });
     }
     newRequest.setSslConfiguration(sslConfiguration);

src/libsync/account.cpp

@@ -269,17 +269,12 @@ void Account::setApprovedCerts(const QList<QSslCertificate> &certs)
     _am->setCustomTrustedCaCertificates(_approvedCerts);
 }
 
-void Account::addApprovedCerts(const QList<QSslCertificate> &certs)
+void Account::addApprovedCerts(const QSet<QSslCertificate> &certs)
 {
-    _approvedCerts.unite({ certs.begin(), certs.end() });
+    _approvedCerts.unite(certs);
     _am->setCustomTrustedCaCertificates(_approvedCerts);
 }
 
-void Account::resetRejectedCertificates()
-{
-    _rejectedCertificates.clear();
-}
-
 void Account::setUrl(const QUrl &url)
 {
     _url = url;