AbstractNetworkJob: Improve redirect handling #5555

* For requests:
  - reuse the original QNetworkRequest, so headers and attributes
    are the same as in the original request
  - determine the original http method from the reply and the request
    attributes
  - keep the original request body around such that it can be sent
    again in case the request is redirected

* Simplify the interface that is used for creating new requests in
  AbstractNetworkJob.

src/gui/notificationconfirmjob.cpp

@@ -54,9 +54,7 @@ void NotificationConfirmJob::start()
     req.setRawHeader("Ocs-APIREQUEST", "true");
     req.setRawHeader("Content-Type", "application/x-www-form-urlencoded");
 
-    QIODevice *iodevice = 0;
-    setReply(davRequest(_verb, _link, req, iodevice));
-    setupConnections(reply());
+    sendRequest(_verb, _link, req);
 
     AbstractNetworkJob::start();
 }

src/gui/ocsjob.cpp

@@ -93,9 +93,7 @@ void OcsJob::start()
     queryItems.append(qMakePair(QByteArray("format"), QByteArray("json")));
     url.setEncodedQueryItems(queryItems);
 
-    setReply(davRequest(_verb, url, req, buffer));
-    setupConnections(reply());
-    buffer->setParent(reply());
+    sendRequest(_verb, url, req, buffer);
     AbstractNetworkJob::start();
 }
 

src/gui/owncloudsetupwizard.cpp

@@ -594,9 +594,7 @@ DetermineAuthTypeJob::DetermineAuthTypeJob(AccountPtr account, QObject *parent)
 
 void DetermineAuthTypeJob::start()
 {
-    QNetworkReply *reply = getRequest(account()->davPath());
-    setReply(reply);
-    setupConnections(reply);
+    sendRequest("GET", account()->davUrl());
     AbstractNetworkJob::start();
 }
 
@@ -613,8 +611,7 @@ bool DetermineAuthTypeJob::finished()
         // do a new run
         _redirects++;
         resetTimeout();
-        setReply(getRequest(redirection));
-        setupConnections(reply());
+        sendRequest("GET", redirection);
         return false; // don't discard
     } else {
 #ifndef NO_SHIBBOLETH

src/gui/thumbnailjob.cpp

@@ -27,8 +27,7 @@ ThumbnailJob::ThumbnailJob(const QString &path, AccountPtr account, QObject* par
 
 void ThumbnailJob::start()
 {
-    setReply(getRequest(path()));
-    setupConnections(reply());
+    sendRequest("GET", makeAccountUrl(path()));
     AbstractNetworkJob::start();
 }
 

src/libsync/abstractnetworkjob.cpp

@@ -122,40 +122,49 @@ QNetworkReply* AbstractNetworkJob::addTimer(QNetworkReply *reply)
     return reply;
 }
 
-QNetworkReply* AbstractNetworkJob::davRequest(const QByteArray &verb, const QString &relPath,
-                                              QNetworkRequest req, QIODevice *data)
+QNetworkReply *AbstractNetworkJob::sendRequest(const QByteArray &verb, const QUrl &url,
+                                               QNetworkRequest req, QIODevice *requestBody)
 {
-    return addTimer(_account->davRequest(verb, relPath, req, data));
-}
-
-QNetworkReply *AbstractNetworkJob::davRequest(const QByteArray &verb, const QUrl &url, QNetworkRequest req, QIODevice *data)
-{
-    return addTimer(_account->davRequest(verb, url, req, data));
-}
-
-QNetworkReply* AbstractNetworkJob::getRequest(const QString &relPath)
-{
-    return addTimer(_account->getRequest(relPath));
-}
-
-QNetworkReply *AbstractNetworkJob::getRequest(const QUrl &url)
-{
-    return addTimer(_account->getRequest(url));
+    auto reply = _account->sendRequest(verb, url, req, requestBody);
+    _requestBody = requestBody;
+    if (_requestBody) {
+        _requestBody->setParent(reply);
+    }
+    addTimer(reply);
+    setReply(reply);
+    setupConnections(reply);
+    return reply;
 }
 
-QNetworkReply *AbstractNetworkJob::headRequest(const QString &relPath)
+QUrl AbstractNetworkJob::makeAccountUrl(const QString& relativePath) const
 {
-    return addTimer(_account->headRequest(relPath));
+    return Utility::concatUrlPath(_account->url(), relativePath);
 }
 
-QNetworkReply *AbstractNetworkJob::headRequest(const QUrl &url)
+QUrl AbstractNetworkJob::makeDavUrl(const QString& relativePath) const
 {
-    return addTimer(_account->headRequest(url));
+    return Utility::concatUrlPath(_account->davUrl(), relativePath);
 }
 
-QNetworkReply *AbstractNetworkJob::deleteRequest(const QUrl &url)
+QByteArray AbstractNetworkJob::requestVerb(QNetworkReply* reply)
 {
-    return addTimer(_account->deleteRequest(url));
+    switch (reply->operation()) {
+    case QNetworkAccessManager::HeadOperation:
+        return "HEAD";
+    case QNetworkAccessManager::GetOperation:
+        return "GET";
+    case QNetworkAccessManager::PutOperation:
+        return "PUT";
+    case QNetworkAccessManager::PostOperation:
+        return "POST";
+    case QNetworkAccessManager::DeleteOperation:
+        return "DELETE";
+    case QNetworkAccessManager::CustomOperation:
+        return reply->request().attribute(QNetworkRequest::CustomVerbAttribute).toByteArray();
+    case QNetworkAccessManager::UnknownOperation:
+        break;
+    }
+    return QByteArray();
 }
 
 void AbstractNetworkJob::slotFinished()
@@ -178,24 +187,36 @@ void AbstractNetworkJob::slotFinished()
     // get the Date timestamp from reply
     _responseTimestamp = _reply->rawHeader("Date");
 
-    if (_followRedirects) {
-        // ### the qWarnings here should be exported via displayErrors() so they
+    QUrl requestedUrl = reply()->request().url();
+    QUrl redirectUrl = reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
+    if (_followRedirects && !redirectUrl.isEmpty()) {
+        _redirectCount++;
+
+        // ### 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
-        QUrl requestedUrl = reply()->request().url();
-        QUrl redirectUrl = reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).toUrl();
-        if (!redirectUrl.isEmpty()) {
-            _redirectCount++;
-            if (requestedUrl.scheme() == QLatin1String("https") &&
-                    redirectUrl.scheme() == QLatin1String("http")) {
-                qWarning() << this << "HTTPS->HTTP downgrade detected!";
-            } else if (requestedUrl == redirectUrl || _redirectCount >= maxRedirects()) {
-                qWarning() << this << "Redirect loop detected!";
-            } else {
-                resetTimeout();
-                setReply(getRequest(redirectUrl));
-                setupConnections(reply());
-                return;
+        QByteArray verb = requestVerb(reply());
+        if (requestedUrl.scheme() == QLatin1String("https") &&
+                redirectUrl.scheme() == QLatin1String("http")) {
+            qWarning() << this << "HTTPS->HTTP downgrade detected!";
+        } else if (requestedUrl == redirectUrl || _redirectCount >= maxRedirects()) {
+            qWarning() << this << "Redirect loop detected!";
+        } else if (_requestBody && _requestBody->isSequential()) {
+            qWarning() << this << "cannot redirect request with sequential body";
+        } else if (verb.isEmpty()) {
+            qWarning() << this << "cannot redirect request: could not detect original verb";
+        } else {
+            // Create the redirected request and send it
+            qDebug() << "Redirecting" << verb << requestedUrl << redirectUrl;
+            resetTimeout();
+            if (_requestBody) {
+                _requestBody->seek(0);
             }
+            sendRequest(
+                    verb,
+                    redirectUrl,
+                    reply()->request(),
+                    _requestBody);
+            return;
         }
     }
 

src/libsync/abstractnetworkjob.h

@@ -78,17 +78,29 @@ public slots:
     void networkActivity();
 protected:
     void setupConnections(QNetworkReply *reply);
-    QNetworkReply* davRequest(const QByteArray& verb, const QString &relPath,
-                              QNetworkRequest req = QNetworkRequest(),
-                              QIODevice *data = 0);
-    QNetworkReply* davRequest(const QByteArray& verb, const QUrl &url,
-                              QNetworkRequest req = QNetworkRequest(),
-                              QIODevice *data = 0);
-    QNetworkReply* getRequest(const QString &relPath);
-    QNetworkReply* getRequest(const QUrl &url);
-    QNetworkReply* headRequest(const QString &relPath);
-    QNetworkReply* headRequest(const QUrl &url);
-    QNetworkReply* deleteRequest(const QUrl &url);
+
+    /** Initiate a network request, returning a QNetworkReply.
+     *
+     * Calls setReply() and setupConnections() on it.
+     *
+     * Takes ownership of the requestBody (to allow redirects).
+     */
+    QNetworkReply* sendRequest(const QByteArray& verb, const QUrl &url,
+                               QNetworkRequest req = QNetworkRequest(),
+                               QIODevice *requestBody = 0);
+
+    // sendRequest does not take a relative path instead of an url,
+    // but the old API allowed that. We have this undefined overload
+    // to help catch usage errors
+    QNetworkReply* sendRequest(const QByteArray& verb, const QString &relativePath,
+                               QNetworkRequest req = QNetworkRequest(),
+                               QIODevice *requestBody = 0);
+
+    /// Creates a url for the account from a relative path
+    QUrl makeAccountUrl(const QString& relativePath) const;
+
+    /// Like makeAccountUrl() but uses the account's dav base path
+    QUrl makeDavUrl(const QString& relativePath) const;
 
     int maxRedirects() const { return 10; }
     virtual bool finished() = 0;
@@ -99,19 +111,32 @@ public slots:
     // GET requests that don't set up any HTTP body or other flags.
     bool          _followRedirects;
 
+    /** Helper to construct the HTTP verb used in the request
+     *
+     * Returns an empty QByteArray for UnknownOperation.
+     */
+    static QByteArray requestVerb(QNetworkReply* reply);
+
 private slots:
     void slotFinished();
     virtual void slotTimeout();
 
 protected:
     AccountPtr _account;
+
 private:
     QNetworkReply* addTimer(QNetworkReply *reply);
     bool _ignoreCredentialFailure;
     QPointer<QNetworkReply> _reply; // (QPointer because the NetworkManager may be destroyed before the jobs at exit)
     QString _path;
     QTimer _timer;
     int _redirectCount;
+
+    // Set by the xyzRequest() functions and needed to be able to redirect
+    // requests, should it be required.
+    //
+    // Reparented to the currently running QNetworkReply.
+    QPointer<QIODevice> _requestBody;
 };
 
 /**

src/libsync/account.cpp

@@ -202,54 +202,23 @@ QNetworkAccessManager *Account::networkAccessManager()
     return _am.data();
 }
 
-QNetworkReply *Account::headRequest(const QString &relPath)
-{
-    return headRequest(Utility::concatUrlPath(url(), relPath));
-}
-
-QNetworkReply *Account::headRequest(const QUrl &url)
-{
-    QNetworkRequest request(url);
-#if QT_VERSION > QT_VERSION_CHECK(4, 8, 4)
-    request.setSslConfiguration(this->getOrCreateSslConfig());
-#endif
-    return _am->head(request);
-}
-
-QNetworkReply *Account::getRequest(const QString &relPath)
-{
-    return getRequest(Utility::concatUrlPath(url(), relPath));
-}
-
-QNetworkReply *Account::getRequest(const QUrl &url)
-{
-    QNetworkRequest request(url);
-#if QT_VERSION > QT_VERSION_CHECK(4, 8, 4)
-    request.setSslConfiguration(this->getOrCreateSslConfig());
-#endif
-    return _am->get(request);
-}
-
-QNetworkReply *Account::deleteRequest( const QUrl &url)
-{
-    QNetworkRequest request(url);
-#if QT_VERSION > QT_VERSION_CHECK(4, 8, 4)
-    request.setSslConfiguration(this->getOrCreateSslConfig());
-#endif
-    return _am->deleteResource(request);
-}
-
-QNetworkReply *Account::davRequest(const QByteArray &verb, const QString &relPath, QNetworkRequest req, QIODevice *data)
-{
-    return davRequest(verb, Utility::concatUrlPath(davUrl(), relPath), req, data);
-}
-
-QNetworkReply *Account::davRequest(const QByteArray &verb, const QUrl &url, QNetworkRequest req, QIODevice *data)
+QNetworkReply *Account::sendRequest(const QByteArray &verb, const QUrl &url, QNetworkRequest req, QIODevice *data)
 {
     req.setUrl(url);
 #if QT_VERSION > QT_VERSION_CHECK(4, 8, 4)
     req.setSslConfiguration(this->getOrCreateSslConfig());
 #endif
+    if (verb == "HEAD" && !data) {
+        return _am->head(req);
+    } else if (verb == "GET" && !data) {
+        return _am->get(req);
+    } else if (verb == "POST") {
+        return _am->post(req, data);
+    } else if (verb == "PUT") {
+        return _am->put(req, data);
+    } else if (verb == "DELETE" && !data) {
+        return _am->deleteResource(req);
+    }
     return _am->sendCustomRequest(req, verb, data);
 }
 

src/libsync/account.h

@@ -106,14 +106,10 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject {
 
 
     // For creating various network requests
-    QNetworkReply* headRequest(const QString &relPath);
-    QNetworkReply* headRequest(const QUrl &url);
-    QNetworkReply* getRequest(const QString &relPath);
-    QNetworkReply* getRequest(const QUrl &url);
-    QNetworkReply* deleteRequest( const QUrl &url);
-    QNetworkReply* davRequest(const QByteArray &verb, const QString &relPath, QNetworkRequest req, QIODevice *data = 0);
-    QNetworkReply* davRequest(const QByteArray &verb, const QUrl &url, QNetworkRequest req, QIODevice *data = 0);
-
+    QNetworkReply* sendRequest(const QByteArray &verb,
+                               const QUrl &url,
+                               QNetworkRequest req = QNetworkRequest(),
+                               QIODevice *data = 0);
 
     /** The ssl configuration during the first connection */
     QSslConfiguration getOrCreateSslConfig();