From e0963ab9069c19587a4b30ff6236854f5fb81762 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 b10a495af85..6773bd2e532 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" @@ -161,6 +162,10 @@ void AbstractNetworkJob::slotFinished() } 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); @@ -408,4 +413,20 @@ QString networkReplyErrorString(const QNetworkReply &reply) return AbstractNetworkJob::tr("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 6f61d26fdf1..97eb9e7550a 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -90,6 +90,9 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject */ QString errorStringParsingBody(QByteArray *body = 0); + /** 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 41807c41922..c29ea456226 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 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject 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 f46d5dbce52..e703b98b6cc 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 @@ class HttpCredentialsAccessManager : public AccessManager 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: @@ -361,6 +369,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)); @@ -387,8 +396,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; } @@ -516,7 +532,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 2df947da0f9..38be7b9559b 100644 --- a/src/libsync/creds/httpcredentials.h +++ b/src/libsync/creds/httpcredentials.h @@ -107,6 +107,8 @@ class OWNCLOUDSYNC_EXPORT HttpCredentials : public AbstractCredentials // 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 @@ private Q_SLOTS: 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 9bc62da0f8d..c171df99282 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -159,9 +159,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 632cb0c7949..88d42c0f71c 100644 --- a/src/libsync/propagatedownload.h +++ b/src/libsync/propagatedownload.h @@ -65,7 +65,7 @@ class GETFileJob : public AbstractNetworkJob virtual void start() Q_DECL_OVERRIDE; virtual bool finished() Q_DECL_OVERRIDE { - if (reply()->bytesAvailable()) { + if (_saveBodyToFile && reply()->bytesAvailable()) { return false; } else { if (_bandwidthManager) {