Don't follow redirects besides in the connection validator

src/gui/accountstate.cpp

@@ -44,6 +44,8 @@ AccountState::AccountState(AccountPtr account)
         this, &AccountState::slotCredentialsFetched);
     connect(account.data(), &Account::credentialsAsked,
         this, &AccountState::slotCredentialsAsked);
+    connect(account.data(), &Account::unknownConnectionState,
+        this, &AccountState::checkConnectivity);
     _timeSinceLastETagCheck.invalidate();
 }
 
@@ -174,6 +176,7 @@ void AccountState::tagLastSuccessfullETagRequest()
 
 void AccountState::checkConnectivity()
 {
+    qCWarning(lcAccountState) << "checkConnectivity";
     if (isSignedOut() || _waitingForNewCredentials) {
         return;
     }
@@ -209,7 +212,7 @@ void AccountState::checkConnectivity()
     if (isConnected()) {
         // Use a small authed propfind as a minimal ping when we're
         // already connected.
-        conValidator->checkAuthentication();
+        conValidator->checkServerAndAuth();
     } else {
         // Check the server and then the auth.
 

src/gui/connectionvalidator.h

@@ -104,13 +104,12 @@ public slots:
     void checkServerAndAuth();
     void systemProxyLookupDone(const QNetworkProxy &proxy);
 
-    /// Checks authentication only.
-    void checkAuthentication();
-
 signals:
     void connectionResult(ConnectionValidator::Status status, const QStringList &errors);
 
 protected slots:
+    /// Checks authentication only.
+    void checkAuthentication();
     void slotCheckServerAndAuth();
 
     void slotStatusFound(const QUrl &url, const QJsonObject &info);

src/gui/owncloudsetupwizard.cpp

@@ -172,7 +172,6 @@ void OwncloudSetupWizard::slotFindServer()
 
     // Step 1: Check url/status.php
     CheckServerJob *job = new CheckServerJob(account, this);
-    job->setIgnoreCredentialFailure(true);
     connect(job, &CheckServerJob::instanceFound, this, &OwncloudSetupWizard::slotFoundServer);
     connect(job, &CheckServerJob::instanceNotFound, this, &OwncloudSetupWizard::slotFindServerBehindRedirect);
     connect(job, &CheckServerJob::timeout, this, &OwncloudSetupWizard::slotNoServerFoundTimeout);
@@ -195,15 +194,18 @@ void OwncloudSetupWizard::slotFindServerBehindRedirect()
 
     // Grab the chain of permanent redirects and adjust the account url
     // accordingly
-    auto permanentRedirects = std::make_shared<int>(0);
     connect(redirectCheckJob, &AbstractNetworkJob::redirected, this,
-        [permanentRedirects, account](QNetworkReply *reply, const QUrl &targetUrl, int count) {
-            int httpCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-            if (count == *permanentRedirects && (httpCode == 301 || httpCode == 308)) {
-                qCInfo(lcWizard) << account->url() << " was redirected to" << targetUrl;
-                account->setUrl(targetUrl);
-                *permanentRedirects += 1;
+        [account](QNetworkReply *reply, const QUrl &targetUrl) {
+            const int httpCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+            if ((httpCode == 301 || httpCode == 308)) {
+                if (targetUrl.scheme() == QLatin1String("https") && account->url().host() == targetUrl.host()) {
+                    qCInfo(lcWizard) << account->url() << " was redirected to" << targetUrl;
+                    account->setUrl(targetUrl);
+                }
+                return;
             }
+            qCWarning(lcWizard) << "Redirecting:" << reply->url() << "to" << targetUrl;
+            Q_EMIT reply->redirectAllowed();
         });
 
     // Step 3: When done, start checking status.php.
@@ -299,25 +301,31 @@ void OwncloudSetupWizard::testOwnCloudConnect()
 {
     AccountPtr account = _ocWizard->account();
 
-    auto *job = new PropfindJob(account, "/", this);
-    job->setIgnoreCredentialFailure(true);
-    // There is custom redirect handling in the error handler,
-    // so don't automatically follow redirects.
-    job->setFollowRedirects(false);
-    job->setProperties(QList<QByteArray>() << "getlastmodified");
-    connect(job, &PropfindJob::result, _ocWizard, &OwncloudWizard::successfulStep);
-    connect(job, &PropfindJob::finishedWithError, this, &OwncloudSetupWizard::slotAuthError);
-    job->start();
+    // run CheckServerJob again to follow redirects and gather cookies
+    CheckServerJob *job = new CheckServerJob(account, this);
+    connect(job, &CheckServerJob::instanceFound, this, [this, account]{
+        auto *job = new PropfindJob(account, "/", this);
+        job->setIgnoreCredentialFailure(true);
+        job->setProperties(QList<QByteArray>() << "getlastmodified");
+        connect(job, &PropfindJob::result, _ocWizard, &OwncloudWizard::successfulStep);
+        connect(job, &PropfindJob::finishedWithError, this, &OwncloudSetupWizard::slotAuthError);
+        job->start();
 
-    // get the display name so that the folder we register with the file browser has a pretty name
-    auto userJob = new JsonApiJob(account, QStringLiteral("ocs/v2.php/cloud/user"), this);
-    connect(userJob, &JsonApiJob::jsonReceived, this, [account](const QJsonDocument &json, int status) {
-        if (status == 200) {
-            const QString user = json.object().value(QStringLiteral("ocs")).toObject().value(QStringLiteral("data")).toObject().value(QStringLiteral("display-name")).toString();
-            account->setDavDisplayName(user);
-        }
+        // get the display name so that the folder we register with the file browser has a pretty name
+        auto userJob = new JsonApiJob(account, QStringLiteral("ocs/v2.php/cloud/user"), this);
+        connect(userJob, &JsonApiJob::jsonReceived, this, [account](const QJsonDocument &json, int status) {
+            if (status == 200) {
+                const QString user = json.object().value(QStringLiteral("ocs")).toObject().value(QStringLiteral("data")).toObject().value(QStringLiteral("display-name")).toString();
+                account->setDavDisplayName(user);
+            }
+        });
+        userJob->start();
     });
-    userJob->start();
+
+    connect(job, &CheckServerJob::instanceNotFound, this, &OwncloudSetupWizard::slotAuthError);
+    connect(job, &CheckServerJob::timeout, this, &OwncloudSetupWizard::slotNoServerFoundTimeout);
+    job->setTimeout((account->url().scheme() == "https") ? 30 * 1000 : 10 * 1000);
+    job->start();
 }
 
 void OwncloudSetupWizard::slotAuthError()
@@ -335,7 +343,7 @@ void OwncloudSetupWizard::slotAuthError()
     // the updated server URL, similar to redirects on status.php.
     QUrl redirectUrl = reply->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
     if (!redirectUrl.isEmpty()) {
-        qCInfo(lcWizard) << "Authed request was redirected to" << redirectUrl.toString();
+        qCInfo(lcWizard) << "Authed request to" << reply->url() << "was redirected to" << redirectUrl.toString();
 
         // strip the expected path
         QString path = redirectUrl.path();

src/libsync/abstractnetworkjob.cpp

@@ -50,7 +50,6 @@ int AbstractNetworkJob::httpTimeout = qEnvironmentVariableIntValue("OWNCLOUD_TIM
 AbstractNetworkJob::AbstractNetworkJob(AccountPtr account, const QString &path, QObject *parent)
     : QObject(parent)
     , _timedout(false)
-    , _followRedirects(true)
     , _account(account)
     , _ignoreCredentialFailure(false)
     , _reply(nullptr)
@@ -100,11 +99,6 @@ void AbstractNetworkJob::setIgnoreCredentialFailure(bool ignore)
     _ignoreCredentialFailure = ignore;
 }
 
-void AbstractNetworkJob::setFollowRedirects(bool follow)
-{
-    _followRedirects = follow;
-}
-
 void AbstractNetworkJob::setPath(const QString &path)
 {
     _path = path;
@@ -119,6 +113,9 @@ void AbstractNetworkJob::setupConnections(QNetworkReply *reply)
     connect(reply, &QNetworkReply::metaDataChanged, this, &AbstractNetworkJob::networkActivity);
     connect(reply, &QNetworkReply::downloadProgress, this, &AbstractNetworkJob::networkActivity);
     connect(reply, &QNetworkReply::uploadProgress, this, &AbstractNetworkJob::networkActivity);
+    connect(reply, &QNetworkReply::redirected, this, [this, reply](const QUrl &url) {
+        Q_EMIT redirected(reply, url);
+    });
 }
 
 QNetworkReply *AbstractNetworkJob::addTimer(QNetworkReply *reply)
@@ -224,65 +221,13 @@ void AbstractNetworkJob::slotFinished()
     // get the Date timestamp from reply
     _responseTimestamp = _reply->rawHeader("Date");
 
-    QUrl requestedUrl = reply()->request().url();
-    QUrl redirectUrl = reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
-    if (_followRedirects && !redirectUrl.isEmpty()) {
-        // Redirects may be relative
-        if (redirectUrl.isRelative())
-            redirectUrl = requestedUrl.resolved(redirectUrl);
-
-        // For POST requests where the target url has query arguments, Qt automatically
-        // moves these arguments to the body if no explicit body is specified.
-        // This can cause problems with redirected requests, because the redirect url
-        // will no longer contain these query arguments.
-        if (reply()->operation() == QNetworkAccessManager::PostOperation
-            && requestedUrl.hasQuery()
-            && !redirectUrl.hasQuery()
-            && !_requestBody) {
-            qCWarning(lcNetworkJob) << "Redirecting a POST request with an implicit body loses that body";
-        }
-
-        // ### some of the qWarnings here should be exported via displayErrors() so they
-        // ### can be presented to the user if the job executor has a GUI
-        if (requestedUrl.scheme() == QLatin1String("https") && redirectUrl.scheme() == QLatin1String("http")) {
-            qCWarning(lcNetworkJob) << this << "HTTPS->HTTP downgrade detected!";
-        } else if (requestedUrl == redirectUrl || _redirectCount + 1 >= maxRedirects()) {
-            qCWarning(lcNetworkJob) << this << "Redirect loop detected!";
-        } else if (_requestBody && _requestBody->isSequential()) {
-            qCWarning(lcNetworkJob) << this << "cannot redirect request with sequential body";
-        } else if (verb.isEmpty()) {
-            qCWarning(lcNetworkJob) << this << "cannot redirect request: could not detect original verb";
-        } else {
-            emit redirected(_reply, redirectUrl, _redirectCount);
-
-            // The signal emission may have changed this value
-            if (_followRedirects) {
-                _redirectCount++;
-
-                // Create the redirected request and send it
-                qCInfo(lcNetworkJob) << "Redirecting" << verb << requestedUrl << redirectUrl;
-                resetTimeout();
-                if (_requestBody) {
-                    if(!_requestBody->isOpen()) {
-                        // Avoid the QIODevice::seek (QBuffer): The device is not open warning message
-                       _requestBody->open(QIODevice::ReadOnly);
-                    }
-                    _requestBody->seek(0);
-                }
-                sendRequest(
-                    verb,
-                    redirectUrl,
-                    reply()->request(),
-                    _requestBody);
-                return;
-            }
-        }
-    }
-
-    AbstractCredentials *creds = _account->credentials();
-    if (!creds->stillValid(_reply) && !_ignoreCredentialFailure) {
+    if (!_account->credentials()->stillValid(_reply) && !_ignoreCredentialFailure) {
         _account->handleInvalidCredentials();
     }
+    if (!reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).isNull()) {
+        Q_EMIT _account->unknownConnectionState();
+    }
+
 
     bool discard = finished();
     if (discard) {

src/libsync/abstractnetworkjob.h

@@ -56,19 +56,6 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject
     void setIgnoreCredentialFailure(bool ignore);
     bool ignoreCredentialFailure() const { return _ignoreCredentialFailure; }
 
-    /** Whether to handle redirects transparently.
-     *
-     * If true, a follow-up request is issued automatically when
-     * a redirect is encountered. The finished() function is only
-     * called if there are no more redirects (or there are problems
-     * with the redirect).
-     *
-     * The transparent redirect following may be disabled for some
-     * requests where custom handling is necessary.
-     */
-    void setFollowRedirects(bool follow);
-    bool followRedirects() const { return _followRedirects; }
-
     QByteArray responseTimestamp();
     /* Content of the X-Request-ID header. (Only set after the request is sent) */
     QByteArray requestId();
@@ -117,9 +104,8 @@ public slots:
      *
      * \a reply The "please redirect" reply
      * \a targetUrl Where to redirect to
-     * \a redirectCount Counts redirect hops, first is 0.
      */
-    void redirected(QNetworkReply *reply, const QUrl &targetUrl, int redirectCount);
+    void redirected(QNetworkReply *reply, const QUrl &targetUrl);
 
 protected:
     /** Initiate a network request, returning a QNetworkReply.
@@ -179,10 +165,6 @@ public slots:
     QByteArray _responseTimestamp;
     bool _timedout; // set to true when the timeout slot is received
 
-    // Automatically follows redirects. Note that this only works for
-    // GET requests that don't set up any HTTP body or other flags.
-    bool _followRedirects;
-
     QString replyStatusString();
 
 private slots:

src/libsync/account.h

@@ -259,6 +259,8 @@ public slots:
     void accountChangedAvatar();
     void accountChangedDisplayName();
 
+    void unknownConnectionState();
+
 protected Q_SLOTS:
     void slotCredentialsFetched();
     void slotCredentialsAsked();

src/libsync/creds/oauth.cpp

@@ -347,7 +347,6 @@ void OAuth::fetchWellKnown()
         QUrl wellKnownUrl = Utility::concatUrlPath(_account->url(), QStringLiteral("/.well-known/openid-configuration"));
         QNetworkRequest req;
         auto job = _account->sendRequest("GET", wellKnownUrl);
-        job->setFollowRedirects(false);
         job->setAuthenticationJob(true);
         job->setTimeout(qMin(30 * 1000ll, job->timeoutMsec()));
         QObject::connect(job, &SimpleNetworkJob::finishedSignal, this, [this](QNetworkReply *reply) {

src/libsync/networkjobs.cpp

@@ -417,17 +417,19 @@ namespace {
 CheckServerJob::CheckServerJob(AccountPtr account, QObject *parent)
     : AbstractNetworkJob(account, statusphpC(), parent)
     , _subdirFallback(false)
-    , _permanentRedirects(0)
 {
     setIgnoreCredentialFailure(true);
-    connect(this, &AbstractNetworkJob::redirected,
-        this, &CheckServerJob::slotRedirected);
 }
 
 void CheckServerJob::start()
 {
     _serverUrl = account()->url();
-    sendRequest("GET", Utility::concatUrlPath(_serverUrl, path()));
+    QNetworkRequest req;
+    // don't authenticate the request to a possibly external service
+    req.setAttribute(HttpCredentials::DontAddCredentialsAttribute, true);
+    req.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy);
+    req.setRawHeader(QByteArrayLiteral("OC-Connection-Validator"), QByteArrayLiteral("desktop"));
+    sendRequest("GET", Utility::concatUrlPath(_serverUrl, path()), req);
     connect(reply(), &QNetworkReply::metaDataChanged, this, &CheckServerJob::metaDataChangedSlot);
     connect(reply(), &QNetworkReply::encrypted, this, &CheckServerJob::encryptedSlot);
     AbstractNetworkJob::start();
@@ -477,22 +479,6 @@ void CheckServerJob::encryptedSlot()
     mergeSslConfigurationForSslButton(reply()->sslConfiguration(), account());
 }
 
-void CheckServerJob::slotRedirected(QNetworkReply *reply, const QUrl &targetUrl, int redirectCount)
-{
-    const auto slashStatusPhp = QStringLiteral("/%1").arg(statusphpC());
-
-    int httpCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-    QString path = targetUrl.path();
-    if ((httpCode == 301 || httpCode == 308) // permanent redirection
-        && redirectCount == _permanentRedirects // don't apply permanent redirects after a temporary one
-        && path.endsWith(slashStatusPhp)) {
-        _serverUrl = targetUrl;
-        _serverUrl.setPath(path.left(path.size() - slashStatusPhp.size()));
-        qCInfo(lcCheckServerJob) << "status.php was permanently redirected to"
-                                 << targetUrl << "new server url is" << _serverUrl;
-        ++_permanentRedirects;
-    }
-}
 
 void CheckServerJob::metaDataChangedSlot()
 {
@@ -503,11 +489,22 @@ void CheckServerJob::metaDataChangedSlot()
 
 bool CheckServerJob::finished()
 {
-    if (reply()->request().url().scheme() == QLatin1String("https")
+    const QUrl targetUrl = [this] {
+        QUrl redirectUrl = reply()->url();
+        if (redirectUrl.isRelative()) {
+            redirectUrl = _serverUrl.resolved(redirectUrl);
+        }
+        redirectUrl.setPath(redirectUrl.path().remove(QStringLiteral("/status.php")));
+        return redirectUrl;
+    }();
+    if (targetUrl.scheme() == QLatin1String("https")
         && reply()->sslConfiguration().sessionTicket().isEmpty()
         && reply()->error() == QNetworkReply::NoError) {
         qCWarning(lcCheckServerJob) << "No SSL session identifier / session ticket is used, this might impact sync performance negatively.";
     }
+    if (_serverUrl != targetUrl) {
+        qFatal("TODO: Unhandled redirect %s != %s", qPrintable(_serverUrl.toString()), qPrintable(targetUrl.toString()));
+    }
 
     mergeSslConfigurationForSslButton(reply()->sslConfiguration(), account());
 

src/libsync/networkjobs.h

@@ -288,7 +288,6 @@ class OWNCLOUDSYNC_EXPORT CheckServerJob : public AbstractNetworkJob
 private slots:
     virtual void metaDataChangedSlot();
     virtual void encryptedSlot();
-    void slotRedirected(QNetworkReply *reply, const QUrl &targetUrl, int redirectCount);
 
 private:
     bool _subdirFallback;
@@ -299,9 +298,6 @@ private slots:
      * one do not affect this url.
      */
     QUrl _serverUrl;
-
-    /** Keep track of how many permanent redirect were applied. */
-    int _permanentRedirects;
 };