From 19b0a75024ad95afd46d45c3d2b877052d4d87bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernard=20G=C3=BCtermann?= Date: Sun, 21 Jun 2026 11:26:46 +0200 Subject: [PATCH 1/2] fix(creds): keep the job queue alive across transient token refreshes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a transient OAuth refresh error, HttpCredentials::refreshAccessTokenInternal schedules a retry but then also emits authenticationFailed(). Account reacts to that signal with JobQueue::clear() (account.cpp), aborting every in-flight upload. The sync run then finalizes as "complete", so the aborted files are silently never retried: directories exist but files are missing, and subsequent syncs skip the already-recorded items (silent data loss). Emit authenticationFailed() only on the terminal branch (TokenRefreshMaxRetries exceeded, a real logout). On a transient error the queue stays blocked across the scheduled retry and resumes via fetched()->unblock() once the refresh succeeds. Fixes #900 Refs #948 Authored-By: Bernard Gütermann --- src/libsync/creds/httpcredentials.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp index c9a2c2f345..d8d088142a 100644 --- a/src/libsync/creds/httpcredentials.cpp +++ b/src/libsync/creds/httpcredentials.cpp @@ -233,14 +233,17 @@ bool HttpCredentials::refreshAccessTokenInternal(int tokenRefreshRetriesCount) if (nextTry >= TokenRefreshMaxRetries) { qCWarning(lcHttpCredentials) << u"Too many failed refreshes" << nextTry << u"-> log out"; forgetSensitiveData(); + // terminal failure: clears the job queue (see Account) and logs out Q_EMIT authenticationFailed(); Q_EMIT fetched(); - return; + } else { + // Transient failure: retry. Do NOT emit authenticationFailed() here -- it + // would clear the job queue and abort in-flight uploads, silently dropping + // files when the run then finalizes as complete (opencloud-eu/desktop#900, #948). + QTimer::singleShot(timeout, this, [nextTry, this] { + refreshAccessTokenInternal(nextTry); + }); } - QTimer::singleShot(timeout, this, [nextTry, this] { - refreshAccessTokenInternal(nextTry); - }); - Q_EMIT authenticationFailed(); }); connect(_oAuthJob, &AccountBasedOAuth::refreshFinished, this, [this](const QString &accessToken, const QString &refreshToken) { From f44ea9745abf18e22ad3dc186aee0d4706fb5f3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernard=20G=C3=BCtermann?= Date: Sun, 21 Jun 2026 18:33:09 +0200 Subject: [PATCH 2/2] test(oauth): regression test for transient token-refresh error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify that transient OAuth token-refresh errors do NOT emit authenticationFailed(), which would clear the job queue and abort in-flight uploads (#900, #948). Also verify that after TokenRefreshMaxRetries consecutive errors, authenticationFailed() IS emitted (terminal failure). Authored-By: Bernard Gütermann --- test/testoauth/testoauth.cpp | 124 +++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/test/testoauth/testoauth.cpp b/test/testoauth/testoauth.cpp index 6df7ae0998..a7068d3050 100644 --- a/test/testoauth/testoauth.cpp +++ b/test/testoauth/testoauth.cpp @@ -9,6 +9,7 @@ #include #include "common/asserts.h" +#include "libsync/creds/httpcredentials.h" #include "libsync/creds/oauth.h" #include "testutils/syncenginetestutils.h" #include "theme.h" @@ -308,6 +309,32 @@ class OAuthTestCase : public QObject } }; +class TestHttpCredentials : public HttpCredentials +{ +public: + TestHttpCredentials(FakeAM::Override override, const QString &refreshToken) + : HttpCredentials(QStringLiteral("initial-access-token")) + , _override(std::move(override)) + { + _refreshToken = refreshToken; + } + + OCC::AccessManager *createAM() const override + { + auto *am = new FakeAM({}, nullptr); + am->setOverride(_override); + return am; + } + + void restartOauth() override { } + void fetchFromKeychain() override { } + void persist() override { } + +private: + FakeAM::Override _override; +}; + + class TestOAuth : public QObject { Q_OBJECT @@ -757,6 +784,103 @@ private Q_SLOTS: } test; test.test(); } + + void testTransientRefreshDoesNotEmitAuthFailed() + { + QObject replyParent; + int tokenRequestCount = 0; + + // Use TimeoutError: resets nextTry to 0, so it never reaches TokenRefreshMaxRetries, + // and has TokenRefreshDefaultTimeout (30s) so no retry fires during our test window. + auto override = [&replyParent, &tokenRequestCount](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply * { + if (req.url().path().endsWith(QLatin1String("status.php"))) { + const QJsonDocument json(QJsonObject{ + {QStringLiteral("installed"), true}, + {QStringLiteral("maintenance"), false}, + {QStringLiteral("version"), QStringLiteral("10.0.0")}, + }); + return new FakePayloadReply(op, req, json.toJson(), &replyParent); + } + if (req.url().path().endsWith(QLatin1String(".well-known/openid-configuration"))) { + const QJsonDocument json(QJsonObject{ + {QStringLiteral("authorization_endpoint"), QStringLiteral("oauthtest://openidserver/authorize")}, + {QStringLiteral("token_endpoint"), QStringLiteral("oauthtest://openidserver/token_endpoint")}, + }); + return new FakePayloadReply(op, req, json.toJson(), &replyParent); + } + if (req.url().toString().contains(QLatin1String("token_endpoint"))) { + ++tokenRequestCount; + auto *reply = new FakeErrorReply(op, req, &replyParent, 408); + reply->setError(QNetworkReply::TimeoutError, QStringLiteral("Request timed out")); + return reply; + } + return new FakePayloadReply(op, req, {}, &replyParent); + }; + + auto account = Account::create(QUuid::createUuid()); + account->setUrl(sOAuthTestServer); + auto *creds = new TestHttpCredentials(override, QStringLiteral("fake-refresh-token")); + account->setCredentials(creds); + + QSignalSpy authFailedSpy(creds, &AbstractCredentials::authenticationFailed); + QSignalSpy authStartedSpy(creds, &AbstractCredentials::authenticationStarted); + + QVERIFY(creds->refreshAccessToken()); + + // Wait for the first token request to complete (queued connections fire). + // TimeoutError → nextTry resets to 0, retry scheduled in 30s — no retry in this window. + QTRY_VERIFY_WITH_TIMEOUT(tokenRequestCount >= 1, 5000); + QCOMPARE(authStartedSpy.count(), 1); + // The fix: transient errors must NOT emit authenticationFailed + QCOMPARE(authFailedSpy.count(), 0); + } + + void testTerminalRefreshEmitsAuthFailed() + { + QObject replyParent; + int tokenRequestCount = 0; + + auto override = [&replyParent, &tokenRequestCount](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *) -> QNetworkReply * { + if (req.url().path().endsWith(QLatin1String("status.php"))) { + const QJsonDocument json(QJsonObject{ + {QStringLiteral("installed"), true}, + {QStringLiteral("maintenance"), false}, + {QStringLiteral("version"), QStringLiteral("10.0.0")}, + }); + return new FakePayloadReply(op, req, json.toJson(), &replyParent); + } + if (req.url().path().endsWith(QLatin1String(".well-known/openid-configuration"))) { + const QJsonDocument json(QJsonObject{ + {QStringLiteral("authorization_endpoint"), QStringLiteral("oauthtest://openidserver/authorize")}, + {QStringLiteral("token_endpoint"), QStringLiteral("oauthtest://openidserver/token_endpoint")}, + }); + return new FakePayloadReply(op, req, json.toJson(), &replyParent); + } + if (req.url().toString().contains(QLatin1String("token_endpoint"))) { + ++tokenRequestCount; + auto *reply = new FakeErrorReply(op, req, &replyParent, 404); + reply->setError(QNetworkReply::ContentNotFoundError, QStringLiteral("Not found")); + return reply; + } + return new FakePayloadReply(op, req, {}, &replyParent); + }; + + auto account = Account::create(QUuid::createUuid()); + account->setUrl(sOAuthTestServer); + auto *creds = new TestHttpCredentials(override, QStringLiteral("fake-refresh-token")); + account->setCredentials(creds); + + QSignalSpy authFailedSpy(creds, &AbstractCredentials::authenticationFailed); + QSignalSpy fetchedSpy(creds, &AbstractCredentials::fetched); + + QVERIFY(creds->refreshAccessToken()); + + // ContentNotFoundError → timeout=0s, nextTry increments each time. + // After TokenRefreshMaxRetries (3) errors, terminal branch emits authenticationFailed. + QTRY_VERIFY_WITH_TIMEOUT(authFailedSpy.count() == 1, 10000); + QCOMPARE(fetchedSpy.count(), 1); + QVERIFY(tokenRequestCount >= 3); + } };