From 35967fc2d6cfe3fda3429b50b60296cfa3d663ec Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Wed, 17 Oct 2018 14:49:00 +0200 Subject: [PATCH] OAuth2: Refresh the token without aborting the sync OAuth2 access token typically only has a token valid for 1 hour. Before this patch, when the token was timing out during the sync, the sync was aborted, and the ConnectionValidator was then requesting a new token, so the sync can be started over. If the discovery takes longer than the oauth2 validity, this means that the sync can never proceed, as it would be always restarted from scratch. With this patch, we try to transparently renew the OAuth2 token and restart the jobs that failed because the access token was invalid. Note that some changes were required in the GETFile job because it handled the error itself and so it was erroring the jobs before its too late. Issue #6814 --- src/libsync/abstractnetworkjob.cpp | 21 +++++++++++++ src/libsync/abstractnetworkjob.h | 3 ++ src/libsync/creds/abstractcredentials.h | 5 ++++ src/libsync/creds/httpcredentials.cpp | 40 +++++++++++++++++++++++-- src/libsync/creds/httpcredentials.h | 5 ++++ src/libsync/propagatedownload.cpp | 11 +++++-- src/libsync/propagatedownload.h | 2 +- 7 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index f5695fec8..7365c97a3 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -29,6 +29,7 @@ #include #include +#include "common/asserts.h" #include "networkjobs.h" #include "account.h" #include "owncloudpropagator.h" @@ -193,6 +194,10 @@ void AbstractNetworkJob::slotFinished() #endif if (_reply->error() != QNetworkReply::NoError) { + + if (_account->credentials()->retryIfNeeded(this)) + return; + if (!_ignoreCredentialFailure || _reply->error() != QNetworkReply::AuthenticationRequiredError) { qCWarning(lcNetworkJob) << _reply->error() << errorString() << _reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); @@ -440,4 +445,20 @@ QString networkReplyErrorString(const QNetworkReply &reply) return AbstractNetworkJob::tr(R"(Server replied "%1 %2" to "%3 %4")").arg(QString::number(httpStatus), httpReason, requestVerb(reply), reply.request().url().toDisplayString()); } +void AbstractNetworkJob::retry() +{ + ENFORCE(_reply); + auto req = _reply->request(); + QUrl requestedUrl = req.url(); + QByteArray verb = requestVerb(*_reply); + qCInfo(lcNetworkJob) << "Restarting" << verb << requestedUrl; + resetTimeout(); + if (_requestBody) { + _requestBody->seek(0); + } + // The cookie will be added automatically, we don't want AccessManager::createRequest to duplicate them + req.setRawHeader("cookie", QByteArray()); + sendRequest(verb, requestedUrl, req, _requestBody); +} + } // namespace OCC diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index c169e5451..b414b55bc 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -91,6 +91,9 @@ public: */ QString errorStringParsingBody(QByteArray *body = nullptr); + /** Make a new request */ + void retry(); + /** static variable the HTTP timeout (in seconds). If set to 0, the default will be used */ static int httpTimeout; diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h index 896626c12..d679ac6be 100644 --- a/src/libsync/creds/abstractcredentials.h +++ b/src/libsync/creds/abstractcredentials.h @@ -25,6 +25,8 @@ class QNetworkAccessManager; class QNetworkReply; namespace OCC { +class AbstractNetworkJob; + class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject { Q_OBJECT @@ -87,6 +89,9 @@ public: static QString keychainKey(const QString &url, const QString &user, const QString &accountId); + /** If the job need to be restarted or queue, this does it and returns true. */ + virtual bool retryIfNeeded(AbstractNetworkJob *) { return false; } + Q_SIGNALS: /** Emitted when fetchFromKeychain() is done. * diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp index 551d2f3cd..4fb16792d 100644 --- a/src/libsync/creds/httpcredentials.cpp +++ b/src/libsync/creds/httpcredentials.cpp @@ -45,6 +45,7 @@ namespace { const char clientCertificatePEMC[] = "_clientCertificatePEM"; const char clientKeyPEMC[] = "_clientKeyPEM"; const char authenticationFailedC[] = "owncloud-authentication-failed"; + const char needRetryC[] = "owncloud-need-retry"; } // ns class HttpCredentialsAccessManager : public AccessManager @@ -84,8 +85,15 @@ protected: req.setSslConfiguration(sslConfiguration); } + auto *reply = AccessManager::createRequest(op, req, outgoingData); - return AccessManager::createRequest(op, req, outgoingData); + if (_cred->_isRenewingOAuthToken) { + // We know this is going to fail, but we have no way to queue it there, so we will + // simply restart the job after the failure. + reply->setProperty(needRetryC, true); + } + + return reply; } private: @@ -362,6 +370,7 @@ bool HttpCredentials::refreshAccessToken() QString basicAuth = QString("%1:%2").arg( Theme::instance()->oauthClientId(), Theme::instance()->oauthClientSecret()); req.setRawHeader("Authorization", "Basic " + basicAuth.toUtf8().toBase64()); + req.setAttribute(HttpCredentials::DontAddCredentialsAttribute, true); auto requestBody = new QBuffer; QUrlQuery arguments(QString("grant_type=refresh_token&refresh_token=%1").arg(_refreshToken)); @@ -388,8 +397,15 @@ bool HttpCredentials::refreshAccessToken() _refreshToken = json["refresh_token"].toString(); persist(); } + _isRenewingOAuthToken = false; + for (const auto &job : _retryQueue) { + if (job) + job->retry(); + } + _retryQueue.clear(); emit fetched(); }); + _isRenewingOAuthToken = true; return true; } @@ -517,7 +533,27 @@ void HttpCredentials::slotAuthentication(QNetworkReply *reply, QAuthenticator *a // Thus, if we reach this signal, those credentials were invalid and we terminate. qCWarning(lcHttpCredentials) << "Stop request: Authentication failed for " << reply->url().toString(); reply->setProperty(authenticationFailedC, true); - reply->close(); + + if (_isRenewingOAuthToken) { + reply->setProperty(needRetryC, true); + } else if (isUsingOAuth() && !reply->property(needRetryC).toBool()) { + reply->setProperty(needRetryC, true); + qCInfo(lcHttpCredentials) << "Refreshing token"; + refreshAccessToken(); + } +} + +bool HttpCredentials::retryIfNeeded(AbstractNetworkJob *job) +{ + auto *reply = job->reply(); + if (!reply || !reply->property(needRetryC).toBool()) + return false; + if (_isRenewingOAuthToken) { + _retryQueue.append(job); + } else { + job->retry(); + } + return true; } } // namespace OCC diff --git a/src/libsync/creds/httpcredentials.h b/src/libsync/creds/httpcredentials.h index 45c350fa2..be74f9e51 100644 --- a/src/libsync/creds/httpcredentials.h +++ b/src/libsync/creds/httpcredentials.h @@ -107,6 +107,8 @@ public: // Whether we are using OAuth bool isUsingOAuth() const { return !_refreshToken.isNull(); } + bool retryIfNeeded(AbstractNetworkJob *) override; + private Q_SLOTS: void slotAuthentication(QNetworkReply *, QAuthenticator *); @@ -138,10 +140,13 @@ protected: QString _fetchErrorString; bool _ready = false; + bool _isRenewingOAuthToken = false; QSslKey _clientSslKey; QSslCertificate _clientSslCertificate; bool _keychainMigration = false; bool _retryOnKeyChainError = true; // true if we haven't done yet any reading from keychain + + QVector> _retryQueue; // Jobs we need to retry once the auth token is fetched }; diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index b276a8928..05a4c89bf 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -157,9 +157,16 @@ void GETFileJob::slotMetaDataChanged() int httpStatus = reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); - // Ignore redirects - if (httpStatus == 301 || httpStatus == 302 || httpStatus == 303 || httpStatus == 307 || httpStatus == 308) + if (httpStatus == 301 || httpStatus == 302 || httpStatus == 303 || httpStatus == 307 + || httpStatus == 308 || httpStatus == 401) { + // Redirects and auth failures (oauth token renew) are handled by AbstractNetworkJob and + // will end up restarting the job. We do not want to process further data from the initial + // request. newReplyHook() will reestablish signal connections for the follow-up request. + bool ok = disconnect(reply(), &QNetworkReply::finished, this, &GETFileJob::slotReadyRead) + && disconnect(reply(), &QNetworkReply::readyRead, this, &GETFileJob::slotReadyRead); + ASSERT(ok); return; + } // If the status code isn't 2xx, don't write the reply body to the file. // For any error: handle it when the job is finished, not here. diff --git a/src/libsync/propagatedownload.h b/src/libsync/propagatedownload.h index 86635d087..856656ddd 100644 --- a/src/libsync/propagatedownload.h +++ b/src/libsync/propagatedownload.h @@ -67,7 +67,7 @@ public: void start() override; bool finished() override { - if (reply()->bytesAvailable()) { + if (_saveBodyToFile && reply()->bytesAvailable()) { return false; } else { if (_bandwidthManager) { -- 2.30.2