From: Markus Goetz Date: Tue, 14 May 2019 12:00:14 +0000 (+0200) Subject: OAuth2: Better error logging X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~229 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=62d876b09a925a919d56a37c76fe6b5e1233e539;p=nextcloud-desktop.git 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 353384aa7..a22b15bdc 100644 --- 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:
%1") .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:
%1") .arg(jsonParseError.errorString()); diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index ba03b0057..02d379427 100644 --- 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 80f48aa9e..506156a76 100644 --- a/test/testoauth.cpp +++ b/test/testoauth.cpp @@ -33,6 +33,8 @@ class FakePostReply : public QNetworkReply public: std::unique_ptr payload; bool aborted = false; + bool redirectToPolicy = false; + bool redirectToToken = false; FakePostReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request, std::unique_ptr 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 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 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)