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

diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp
index f5695fec856ba84fcd042c9a4085e2a8349001ef..7365c97a382ef3cb282e9b2818c0a2022ca71780 100644 (file)
--- a/src/libsync/abstractnetworkjob.cpp
+++ b/src/libsync/abstractnetworkjob.cpp
@@ -29,6 +29,7 @@
 #include <QAuthenticator>
 #include <QMetaEnum>
 
+#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 c169e5451616cb267afc70ae8a7448e7e43ca946..b414b55bc84ac846517907671482ecad4d08875c 100644 (file)
--- 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 896626c12eb86127b715ff1e0be9978a0c1e5ea0..d679ac6be2b278ea9104abb745e0f29262b053bf 100644 (file)
--- 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 551d2f3cd767123ac2ec8e6c0b8aefd163cc404c..4fb16792d7a94e8882f376c80c3b379112f63f94 100644 (file)
--- 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 45c350fa2a01472d487bf4cbb5549f0b96ae5bfd..be74f9e516f8dde9fb8941c64324ec71f3208708 100644 (file)
--- 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<QPointer<AbstractNetworkJob>> _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 b276a8928c5c9728342a0d0fc832b94d98c384c0..05a4c89bfd39fd98fa94969bed0a09aff411c8ab 100644 (file)
--- 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 86635d08758a2ccd2664806b2ad2c6d0d5a09a4f..856656ddd6a3096dc06412cd1e29d450f76ffe82 100644 (file)
--- 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) {