Don't rely on legacy url to validate credentials

Author: TheOneRing

src/gui/accountstate.cpp

@@ -422,10 +422,8 @@ void AccountState::checkConnectivity(bool blockJobs)
         // already connected.
         if (blockJobs) {
             _connectionValidator->setClearCookies(true);
-            mode = ConnectionValidator::ValidationMode::ValidateAuth;
-        } else {
-            mode = ConnectionValidator::ValidationMode::ValidateAuthAndUpdate;
         }
+        mode = ConnectionValidator::ValidationMode::ValidateAuthAndUpdate;
     } else {
         // Check the server and then the auth.
         if (_waitingForNewCredentials) {

src/gui/connectionvalidator.cpp

@@ -33,14 +33,6 @@ namespace OCC {
 
 Q_LOGGING_CATEGORY(lcConnectionValidator, "sync.connectionvalidator", QtInfoMsg)
 
-// Make sure the timeout for this job is less than how often we get called
-// This makes sure we get tried often enough without "ConnectionValidator already running"
-namespace {
-    auto timeoutToUse()
-    {
-        return std::min(ConnectionValidator::DefaultCallingInterval - 5s, AbstractNetworkJob::httpTimeout);
-    };
-}
 
 ConnectionValidator::ConnectionValidator(AccountPtr account, QObject *parent)
     : QObject(parent)
@@ -169,92 +161,36 @@ void ConnectionValidator::slotStatusFound(const QUrl &url, const QJsonObject &in
     }
     // now check the authentication
     if (_mode != ConnectionValidator::ValidationMode::ValidateServer) {
-        checkAuthentication();
-    } else {
-        reportResult(Connected);
-    }
-}
-
-void ConnectionValidator::checkAuthentication()
-{
-    // simply GET the WebDAV root, will fail if credentials are wrong.
-    // continue in slotAuthCheck here :-)
-    qCDebug(lcConnectionValidator) << "# Check whether authenticated propfind works.";
-
-    // we explicitly use a legacy dav path here
-    auto *job = new PropfindJob(_account, _account->url(), QStringLiteral("/remote.php/webdav/"), PropfindJob::Depth::Zero, this);
-    job->setAuthenticationJob(true); // don't retry
-    job->setTimeout(timeoutToUse());
-    job->setProperties({ QByteArrayLiteral("getlastmodified") });
-    connect(job, &PropfindJob::finishedWithoutError, this, &ConnectionValidator::slotAuthSuccess);
-    connect(job, &PropfindJob::finishedWithError, this, &ConnectionValidator::slotAuthFailed);
-    job->start();
-}
-
-void ConnectionValidator::slotAuthFailed()
-{
-    auto job = qobject_cast<PropfindJob *>(sender());
-    Status stat = Timeout;
-
-    if (job->reply()->error() == QNetworkReply::SslHandshakeFailedError) {
-        _errors << job->errorStringParsingBody();
-        stat = NetworkInformation::instance()->isBehindCaptivePortal() ? CaptivePortal : SslError;
-
-    } else if (job->reply()->error() == QNetworkReply::AuthenticationRequiredError) {
-        qCWarning(lcConnectionValidator) << "******** Authentication failed!" << job->reply()->error() << job;
-        _errors << tr("Authentication error");
-        stat = CredentialsWrong;
-    } else if (job->reply()->error() == QNetworkReply::ContentAccessDenied) {
-        stat = ClientUnsupported;
-        _errors << extractErrorMessage(job->reply()->readAll());
-    } else if (job->reply()->error() != QNetworkReply::NoError) {
-        _errors << job->errorStringParsingBody();
-
-        if (job->httpStatusCode() == 503) {
-            _errors.clear();
-            stat = ServiceUnavailable;
-        }
-    }
-
-    reportResult(stat);
-}
-
-void ConnectionValidator::slotAuthSuccess()
-{
-    _errors.clear();
-    if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) {
         auto *fetchSetting = new FetchServerSettingsJob(_account, 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.")});
         };
-        connect(fetchSetting, &FetchServerSettingsJob::finishedSignal, this, [unsupportedServerError, this](FetchServerSettingsJob::Result result) {
-            switch (result) {
-            case FetchServerSettingsJob::Result::UnsupportedServer:
-                unsupportedServerError();
-                reportResult(ServerVersionMismatch);
-                break;
-            case FetchServerSettingsJob::Result::InvalidCredentials:
-                reportResult(CredentialsWrong);
-                break;
-            case FetchServerSettingsJob::Result::TimeOut:
-                reportResult(Timeout);
-                break;
-            case FetchServerSettingsJob::Result::Success:
-                if (_account->serverSupportLevel() == Account::ServerSupportLevel::Unknown) {
+        connect(fetchSetting, &FetchServerSettingsJob::finishedSignal, this,
+            [unsupportedServerError, this](ConnectionValidator::Status result, const QString &error) {
+                if (!error.isEmpty()) {
+                    _errors.append(error);
+                }
+                switch (result) {
+                case ServerVersionMismatch:
                     unsupportedServerError();
+                    reportResult(ServerVersionMismatch);
+                    break;
+                case Connected:
+                    if (_account->serverSupportLevel() == Account::ServerSupportLevel::Unknown) {
+                        unsupportedServerError();
+                    }
+                    [[fallthrough]];
+                default:
+                    reportResult(result);
+                    break;
                 }
-                reportResult(Connected);
-                break;
-            case FetchServerSettingsJob::Result::Undefined:
-                reportResult(Undefined);
-                break;
-            }
-        });
+            });
 
         fetchSetting->start();
         return;
+    } else {
+        reportResult(Connected);
     }
-    reportResult(Connected);
 }
 
 void ConnectionValidator::reportResult(Status status)

src/gui/connectionvalidator.h

@@ -29,64 +29,13 @@
 
 namespace OCC {
 
-/**
- * This is a job-like class to check that the server is up and that we are connected.
- * There are two entry points: checkServerAndAuth and checkAuthentication
- * checkAuthentication is the quick version that only does the propfind
- * while checkServerAndAuth is doing the 4 calls.
- *
- * We cannot use the capabilites call to test the login and the password because of
- * https://github.com/owncloud/core/issues/12930
- *
- * Here follows the state machine
-
-\code{.unparsed}
-*---> checkServerAndAuth  (check status.php)
-        Will asynchronously check for system proxy (if using system proxy)
-        And then invoke slotCheckServerAndAuth
-        CheckServerJob
-        |
-        +-> slotNoStatusFound --> X
-        |
-        +-> slotJobTimeout --> X
-        |
-        +-> slotStatusFound --+--> X (if credentials are still missing)
-                              |
-  +---------------------------+
-  |
-*-+-> checkAuthentication (PROPFIND on root)
-        PropfindJob
-        |
-        +-> slotAuthFailed --> X
-        |
-        +-> slotAuthSuccess --+--> X (depending if coming from checkServerAndAuth or not)
-                              |
-  +---------------------------+
-  |
-  +-> checkServerCapabilities
-        JsonApiJob (cloud/capabilities) -> slotCapabilitiesRecieved -+
-                                                                     |
-  +------------------------------------------------------------------+
-  |
-  +-> fetchUser -+
-                 |
-                 +-> AvatarJob
-                            |
-                            +-> slotAvatarImage --> reportResult()
-
-    \endcode
- */
 class OPENCLOUD_GUI_EXPORT ConnectionValidator : public QObject
 {
     Q_OBJECT
 public:
     explicit ConnectionValidator(AccountPtr account, QObject *parent = nullptr);
 
-    enum class ValidationMode {
-        ValidateServer,
-        ValidateAuth,
-        ValidateAuthAndUpdate
-    };
+    enum class ValidationMode { ValidateServer, ValidateAuthAndUpdate };
     Q_ENUM(ValidationMode)
 
     enum Status {
@@ -127,15 +76,10 @@ public Q_SLOTS:
     void sslErrors(const QList<QSslError> &errors);
 
 protected Q_SLOTS:
-    /// Checks authentication only.
-    void checkAuthentication();
     void slotCheckServerAndAuth();
 
     void slotStatusFound(const QUrl &url, const QJsonObject &info);
 
-    void slotAuthFailed();
-    void slotAuthSuccess();
-
 private:
     void reportResult(Status status);
 

src/gui/fetchserversettings.cpp

@@ -16,9 +16,11 @@
 
 #include "gui/accountstate.h"
 #include "gui/connectionvalidator.h"
+#include "gui/networkinformation.h"
 
 #include "libsync/networkjobs/jsonjob.h"
 
+
 using namespace std::chrono_literals;
 
 using namespace OCC;
@@ -67,37 +69,44 @@ void FetchServerSettingsJob::start()
             case Account::ServerSupportLevel::Supported:
                 break;
             case Account::ServerSupportLevel::Unsupported:
-                Q_EMIT finishedSignal(Result::UnsupportedServer);
+                Q_EMIT finishedSignal(ConnectionValidator::ServerVersionMismatch);
                 return;
             }
             auto *userJob = new JsonApiJob(_account, QStringLiteral("ocs/v2.php/cloud/user"), SimpleNetworkJob::UrlQuery{}, QNetworkRequest{}, this);
             userJob->setAuthenticationJob(isAuthJob());
             userJob->setTimeout(fetchSettingsTimeout());
             connect(userJob, &JsonApiJob::finishedSignal, this, [userJob, this] {
                 if (userJob->timedOut()) {
-                    Q_EMIT finishedSignal(Result::TimeOut);
+                    Q_EMIT finishedSignal(ConnectionValidator::Timeout);
                 } else if (userJob->httpStatusCode() == 401) {
-                    Q_EMIT finishedSignal(Result::InvalidCredentials);
+                    Q_EMIT finishedSignal(ConnectionValidator::CredentialsWrong);
                 } else if (userJob->ocsSuccess()) {
                     const auto userData = userJob->data().value(QStringLiteral("ocs")).toObject().value(QStringLiteral("data")).toObject();
                     const QString displayName = userData.value(QStringLiteral("display-name")).toString();
                     if (!displayName.isEmpty()) {
                         _account->setDavDisplayName(displayName);
                     }
                     runAsyncUpdates();
-                    Q_EMIT finishedSignal(Result::Success);
+                    Q_EMIT finishedSignal(ConnectionValidator::Connected);
                 } else {
-                    Q_EMIT finishedSignal(Result::Undefined);
+                    Q_EMIT finishedSignal(ConnectionValidator::Undefined);
                 }
             });
             userJob->start();
         } else {
-            if (job->timedOut()) {
-                Q_EMIT finishedSignal(Result::TimeOut);
+            if (job->reply()->error() == QNetworkReply::ContentAccessDenied) {
+                Q_EMIT finishedSignal(ConnectionValidator::ClientUnsupported, extractErrorMessage(job->reply()->readAll()));
+            } else if (job->reply()->error() == QNetworkReply::SslHandshakeFailedError) {
+                Q_EMIT finishedSignal(
+                    NetworkInformation::instance()->isBehindCaptivePortal() ? ConnectionValidator::CaptivePortal : ConnectionValidator::SslError);
+            } else if (job->timedOut()) {
+                Q_EMIT finishedSignal(ConnectionValidator::Timeout);
             } else if (job->httpStatusCode() == 401) {
-                Q_EMIT finishedSignal(Result::InvalidCredentials);
+                Q_EMIT finishedSignal(ConnectionValidator::CredentialsWrong);
+            } else if (job->httpStatusCode() == 503) {
+                Q_EMIT finishedSignal(ConnectionValidator::ServiceUnavailable);
             } else {
-                Q_EMIT finishedSignal(Result::Undefined);
+                Q_EMIT finishedSignal(ConnectionValidator::Undefined);
             }
         }
     });

src/gui/fetchserversettings.h

@@ -14,6 +14,7 @@
 
 #pragma once
 
+#include "gui/connectionvalidator.h"
 #include "libsync/accountfwd.h"
 
 #include <QObject>
@@ -25,14 +26,12 @@ class FetchServerSettingsJob : public QObject
 {
     Q_OBJECT
 public:
-    enum class Result { Success, TimeOut, InvalidCredentials, UnsupportedServer, Undefined };
-    Q_ENUM(Result);
     FetchServerSettingsJob(const AccountPtr &account, QObject *parent);
 
     void start();
 
 Q_SIGNALS:
-    void finishedSignal(Result);
+    void finishedSignal(ConnectionValidator::Status, QString errorMessage = {});
 
 private:
     void runAsyncUpdates();

test/testconnectionvalidator/testconnectionvalidator.cpp

@@ -30,7 +30,7 @@ class TestConnectionValidator : public QObject
 {
     Q_OBJECT
 
-    enum class FailStage { Invalid, StatusPhp, AuthValidation, Capabilities, UserInfo };
+    enum class FailStage { Invalid, StatusPhp, Capabilities, UserInfo };
 
     // we can't use QMap direclty with QFETCH
     using Values = QMap<QString, QString>;
@@ -60,20 +60,17 @@ private Q_SLOTS:
         const auto defaultValue = Values{{QStringLiteral("maintenance"), QStringLiteral("false")}, {QStringLiteral("version"), QStringLiteral("10.11.0.0")},
             {QStringLiteral("productversion"), QStringLiteral("4.0.5")}};
 
-        QTest::newRow("stauts.php maintenance") << FailStage::StatusPhp << [value = defaultValue]() mutable {
+        QTest::newRow("status.php maintenance") << FailStage::StatusPhp << [value = defaultValue]() mutable {
             value[QStringLiteral("maintenance")] = QStringLiteral("true");
             return value;
         }() << ConnectionValidator::MaintenanceMode;
-        QTest::newRow("stauts.php ServiceUnavailable") << FailStage::StatusPhp << defaultValue << ConnectionValidator::StatusNotFound;
-        QTest::newRow("stauts.php UnsupportedClient") << FailStage::StatusPhp << defaultValue << ConnectionValidator::ClientUnsupported;
-
-        QTest::newRow("auth 401") << FailStage::AuthValidation << defaultValue << ConnectionValidator::CredentialsWrong;
-        QTest::newRow("auth timeout") << FailStage::AuthValidation << defaultValue << ConnectionValidator::Timeout;
-        QTest::newRow("auth ServiceUnavailable") << FailStage::AuthValidation << defaultValue << ConnectionValidator::ServiceUnavailable;
-        QTest::newRow("auth UnsupportedClient") << FailStage::AuthValidation << defaultValue << ConnectionValidator::ClientUnsupported;
+        QTest::newRow("status.php ServiceUnavailable") << FailStage::StatusPhp << defaultValue << ConnectionValidator::StatusNotFound;
+        QTest::newRow("status.php UnsupportedClient") << FailStage::StatusPhp << defaultValue << ConnectionValidator::ClientUnsupported;
 
         QTest::newRow("capabilites timeout") << FailStage::Capabilities << defaultValue << ConnectionValidator::CredentialsWrong;
         QTest::newRow("capabilites 401") << FailStage::Capabilities << defaultValue << ConnectionValidator::Timeout;
+        QTest::newRow("capabilites ServiceUnavailable") << FailStage::Capabilities << defaultValue << ConnectionValidator::ServiceUnavailable;
+        QTest::newRow("capabilites UnsupportedClient") << FailStage::Capabilities << defaultValue << ConnectionValidator::ClientUnsupported;
         QTest::newRow("capabilites unsupported server") << FailStage::Capabilities << [value = defaultValue]() mutable {
             value[QStringLiteral("version")] = QStringLiteral("7.0");
             value[QStringLiteral("productversion")] = QString();
@@ -116,6 +113,10 @@ private Q_SLOTS:
                     if (failStage == FailStage::Capabilities) {
                         if (status == ConnectionValidator::CredentialsWrong) {
                             return new FakeErrorReply(op, request, this, 401);
+                        } else if (status == ConnectionValidator::ClientUnsupported) {
+                            return new FakeErrorReply(op, request, this, 403);
+                        } else if (status == ConnectionValidator::ServiceUnavailable) {
+                            return new FakeErrorReply(op, request, this, 503);
                         } else if (status == ConnectionValidator::Timeout) {
                             return new FakeHangingReply(op, request, this);
                         }
@@ -132,17 +133,6 @@ private Q_SLOTS:
                     }
                     return new FakePayloadReply(op, request, getPayload(QStringLiteral("user.json")), this);
                 }
-            } else if (failStage == FailStage::AuthValidation && verb == "PROPFIND") {
-                reachedStage = FailStage::AuthValidation;
-                if (status == ConnectionValidator::CredentialsWrong) {
-                    return new FakeErrorReply(op, request, this, 401);
-                } else if (status == ConnectionValidator::Timeout) {
-                    return new FakeHangingReply(op, request, this);
-                } else if (status == ConnectionValidator::ServiceUnavailable) {
-                    return new FakeErrorReply(op, request, this, 503);
-                } else if (status == ConnectionValidator::ClientUnsupported) {
-                    return new FakeErrorReply(op, request, this, 403);
-                }
             }
             return nullptr;
         });

test/testutils/syncenginetestutils.cpp

@@ -1133,14 +1133,8 @@ QString getFilePathFromUrl(const QUrl &url)
     const QString path = url.path();
     const QString sRootUrl = OCC::TestUtils::dummyDavUrl().path();
 
-    const QString legacyConnectionValidator = QStringLiteral("/remote.php/webdav/");
-
-
-    QString out;
     if (path.startsWith(sRootUrl)) {
-        out = path.mid(sRootUrl.length());
-    } else if (path.startsWith(legacyConnectionValidator)) {
-        out = path.mid(legacyConnectionValidator.length());
+        return OCC::Utility::stripLeadingSlash(path.mid(sRootUrl.length()));
     }
-    return OCC::Utility::stripLeadingSlash(out);
+    return {};
 }