OAuth2: Better error logging

This does not fix a bug, just was found while spotting a bug that was no bug.
For https://github.com/owncloud/enterprise/issues/2951

diff --git a/src/gui/creds/oauth.cpp b/src/gui/creds/oauth.cpp
index 353384aa7797447acdac9c9e3184f60c65ee10f8..a22b15bdc2d7ce4cf07cab6822db3fe705c0e9c3 100644 (file)
--- a/src/gui/creds/oauth.cpp
+++ b/src/gui/creds/oauth.cpp
@@ -105,7 +105,7 @@ void OAuth::start()
                     QUrl messageUrl = json["message_url"].toString();
 
                     if (reply->error() != QNetworkReply::NoError || jsonParseError.error != QJsonParseError::NoError
-                        || json.isEmpty() || refreshToken.isEmpty() || accessToken.isEmpty()
+                        || jsonData.isEmpty() || json.isEmpty() || refreshToken.isEmpty() || accessToken.isEmpty()
                         || json["token_type"].toString() != QLatin1String("Bearer")) {
                         QString errorReason;
                         QString errorFromJson = json["error"].toString();
@@ -115,6 +115,12 @@ void OAuth::start()
                         } else if (reply->error() != QNetworkReply::NoError) {
                             errorReason = tr("There was an error accessing the 'token' endpoint: <br><em>%1</em>")
                                               .arg(reply->errorString().toHtmlEscaped());
+                        } else if (jsonData.isEmpty()) {
+                            // Can happen if a funky load balancer strips away POST data, e.g. BigIP APM my.policy
+                            errorReason = tr("Empty JSON from OAuth2 redirect");
+                            // We explicitly have this as error case since the json qcWarning output below is misleading,
+                            // it will show a fake json will null values that actually never was received like this as
+                            // soon as you access json["whatever"] the debug output json will claim to have "whatever":null
                         } else if (jsonParseError.error != QJsonParseError::NoError) {
                             errorReason = tr("Could not parse the JSON returned from the server: <br><em>%1</em>")
                                               .arg(jsonParseError.errorString());

diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp
index ba03b0057d671e0f1437af35ced09aadfbb12a1f..02d379427cce5b7c1245abe732a765355a8873ba 100644 (file)
--- a/src/libsync/abstractnetworkjob.cpp
+++ b/src/libsync/abstractnetworkjob.cpp
@@ -251,6 +251,10 @@ void AbstractNetworkJob::slotFinished()
                 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(

diff --git a/test/testoauth.cpp b/test/testoauth.cpp
index 80f48aa9e28adb504846cacd22266efaaed8ac54..506156a76c8c96199b624a59ef5db7b72f46d93a 100644 (file)
--- a/test/testoauth.cpp
+++ b/test/testoauth.cpp
@@ -33,6 +33,8 @@ class FakePostReply : public QNetworkReply
 public:
     std::unique_ptr<QIODevice> payload;
     bool aborted = false;
+    bool redirectToPolicy = false;
+    bool redirectToToken = false;
 
     FakePostReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request,
                   std::unique_ptr<QIODevice> payload_, QObject *parent)
@@ -52,6 +54,24 @@ public:
             emit metaDataChanged();
             emit finished();
             return;
+        } else if (redirectToPolicy) {
+            setHeader(QNetworkRequest::LocationHeader, "/my.policy");
+            setAttribute(QNetworkRequest::RedirectionTargetAttribute, "/my.policy");
+            setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 302); // 302 might or might not lose POST data in rfc
+            setHeader(QNetworkRequest::ContentLengthHeader, 0);
+            emit metaDataChanged();
+            emit finished();
+            return;
+        } else if (redirectToToken) {
+            // Redirect to self
+            QVariant destination = QVariant(sOAuthTestServer.toString()+QLatin1String("/index.php/apps/oauth2/api/v1/token"));
+            setHeader(QNetworkRequest::LocationHeader, destination);
+            setAttribute(QNetworkRequest::RedirectionTargetAttribute, destination);
+            setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 307); // 307 explicitly in rfc says to not lose POST data
+            setHeader(QNetworkRequest::ContentLengthHeader, 0);
+            emit metaDataChanged();
+            emit finished();
+            return;
         }
         setHeader(QNetworkRequest::ContentLengthHeader, payload->size());
         setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200);
@@ -112,7 +132,9 @@ public:
         account->setUrl(sOAuthTestServer);
         account->setCredentials(new FakeCredentials{fakeQnam});
         fakeQnam->setParent(this);
-        fakeQnam->setOverride([this](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) {
+        fakeQnam->setOverride([this](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *device) {
+            ASSERT(device);
+            ASSERT(device->bytesAvailable()>0); // OAuth2 always sends around POST data.
             return this->tokenReply(op, req);
         });
 
@@ -276,6 +298,39 @@ private slots:
         } test;
         test.test();
     }
+
+    void testTokenUrlHasRedirect()
+    {
+        struct Test : OAuthTestCase {
+            int redirectsDone = 0;
+            QNetworkReply *tokenReply(QNetworkAccessManager::Operation op, const QNetworkRequest & request) override
+            {
+                ASSERT(browserReply);
+                // Kind of reproduces what we had in https://github.com/owncloud/enterprise/issues/2951 (not 1:1)
+                if (redirectsDone == 0) {
+                    std::unique_ptr<QBuffer> payload(new QBuffer());
+                    payload->setData("");
+                    SlowFakePostReply *reply = new SlowFakePostReply(op, request, std::move(payload), this);
+                    reply->redirectToPolicy = true;
+                    redirectsDone++;
+                    return reply;
+                } else if  (redirectsDone == 1) {
+                    std::unique_ptr<QBuffer> payload(new QBuffer());
+                    payload->setData("");
+                    SlowFakePostReply *reply = new SlowFakePostReply(op, request, std::move(payload), this);
+                    reply->redirectToToken = true;
+                    redirectsDone++;
+                    return reply;
+                } else {
+                    // ^^ This is with a custom reply and not actually HTTP, so we're testing the HTTP redirect code
+                    // we have in AbstractNetworkJob::slotFinished()
+                    redirectsDone++;
+                    return OAuthTestCase::tokenReply(op, request);
+                }
+            }
+        } test;
+        test.test();
+    }
 };
 
 QTEST_GUILESS_MAIN(TestOAuth)