From d21df33a50c502432124d2f8840382e43163d4a8 Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Apr 2021 13:29:11 +0200 Subject: [PATCH 1/3] Add missing copyright headers for push notifications Signed-off-by: Felix Weilbach --- src/libsync/pushnotifications.cpp | 14 ++++++++++++++ test/testpushnotifications.cpp | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/libsync/pushnotifications.cpp b/src/libsync/pushnotifications.cpp index 71bc391bc..377973481 100644 --- a/src/libsync/pushnotifications.cpp +++ b/src/libsync/pushnotifications.cpp @@ -1,3 +1,17 @@ +/* + * Copyright (C) by Felix Weilbach + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + #include "pushnotifications.h" #include "creds/abstractcredentials.h" #include "account.h" diff --git a/test/testpushnotifications.cpp b/test/testpushnotifications.cpp index 969f0da8f..6e4d047dd 100644 --- a/test/testpushnotifications.cpp +++ b/test/testpushnotifications.cpp @@ -1,3 +1,17 @@ +/* + * Copyright (C) by Felix Weilbach + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + #include #include #include From aea867c44569ddebfb8b1e5c6391a28ce05e36f1 Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Apr 2021 13:31:04 +0200 Subject: [PATCH 2/3] Refactor push notification tests Signed-off-by: Felix Weilbach --- test/testpushnotifications.cpp | 87 ++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/test/testpushnotifications.cpp b/test/testpushnotifications.cpp index 6e4d047dd..c760161e5 100644 --- a/test/testpushnotifications.cpp +++ b/test/testpushnotifications.cpp @@ -21,17 +21,42 @@ #include "pushnotifications.h" #include "pushnotificationstestutils.h" -bool verifyCalledOnceWithAccount(QSignalSpy &spy, OCC::AccountPtr account) -{ - if (spy.count() != 1) { - return false; +#define RETURN_FALSE_ON_FAIL(expr) \ + if (!(expr)) { \ + return false; \ } +bool verifyCalledOnceWithAccount(QSignalSpy &spy, OCC::AccountPtr account) +{ + RETURN_FALSE_ON_FAIL(spy.count() == 1); auto accountFromSpy = spy.at(0).at(0).value(); - if (accountFromSpy != account.data()) { - return false; + RETURN_FALSE_ON_FAIL(accountFromSpy == account.data()); + + return true; +} + +bool failThreeAuthenticationAttempts(FakeWebSocketServer &fakeServer, OCC::AccountPtr account) +{ + RETURN_FALSE_ON_FAIL(account); + RETURN_FALSE_ON_FAIL(account->pushNotifications()); + + account->pushNotifications()->setReconnectTimerInterval(0); + + QSignalSpy authenticationFailedSpy(account->pushNotifications(), &OCC::PushNotifications::authenticationFailed); + + // Let three authentication attempts fail + for (uint8_t i = 0; i < 3; ++i) { + RETURN_FALSE_ON_FAIL(fakeServer.waitForTextMessages()); + RETURN_FALSE_ON_FAIL(fakeServer.textMessagesCount() == 2); + auto socket = fakeServer.socketForTextMessage(0); + fakeServer.clearTextMessages(); + socket->sendTextMessage("err: Invalid credentials"); } + // Now the authenticationFailed Signal should be emitted + RETURN_FALSE_ON_FAIL(authenticationFailedSpy.wait()); + RETURN_FALSE_ON_FAIL(authenticationFailedSpy.count() == 1); + return true; } @@ -138,6 +163,7 @@ private slots: FakeWebSocketServer fakeServer; auto account = FakeWebSocketServer::createAccount(); QSignalSpy connectionLostSpy(account->pushNotifications(), &OCC::PushNotifications::connectionLost); + QSignalSpy pushNotificationsDisabledSpy(account.data(), &OCC::Account::pushNotificationsDisabled); QVERIFY(connectionLostSpy.isValid()); // Wait for authentication and then sent a network error @@ -147,38 +173,26 @@ private slots: socket->abort(); QVERIFY(connectionLostSpy.wait()); - // Account handled connectionLost signal and deleted PushNotifications - QCOMPARE(account->pushNotifications(), nullptr); + // Account handled connectionLost signal and disabled push notifications + QCOMPARE(pushNotificationsDisabledSpy.count(), 1); } - void testSetup_maxConnectionAttemptsReached_deletePushNotifications() + void testSetup_maxConnectionAttemptsReached_disablePushNotifications() { FakeWebSocketServer fakeServer; auto account = FakeWebSocketServer::createAccount(); - account->pushNotifications()->setReconnectTimerInterval(0); - QSignalSpy authenticationFailedSpy(account->pushNotifications(), &OCC::PushNotifications::authenticationFailed); - QVERIFY(authenticationFailedSpy.isValid()); + QSignalSpy pushNotificationsDisabledSpy(account.data(), &OCC::Account::pushNotificationsDisabled); - // Let three authentication attempts fail - for (uint8_t i = 0; i < 3; ++i) { - QVERIFY(fakeServer.waitForTextMessages()); - QCOMPARE(fakeServer.textMessagesCount(), 2); - auto socket = fakeServer.socketForTextMessage(0); - fakeServer.clearTextMessages(); - socket->sendTextMessage("err: Invalid credentials"); - } - - // Now the authenticationFailed Signal should be emitted - QVERIFY(authenticationFailedSpy.wait()); - QCOMPARE(authenticationFailedSpy.count(), 1); - // Account deleted the push notifications - QCOMPARE(account->pushNotifications(), nullptr); + QVERIFY(failThreeAuthenticationAttempts(fakeServer, account)); + // Account disabled the push notifications + QCOMPARE(pushNotificationsDisabledSpy.count(), 1); } void testOnWebSocketSslError_sslError_deletePushNotifications() { FakeWebSocketServer fakeServer; auto account = FakeWebSocketServer::createAccount(); + QSignalSpy pushNotificationsDisabledSpy(account.data(), &OCC::Account::pushNotificationsDisabled); QVERIFY(fakeServer.waitForTextMessages()); // FIXME: This a little bit ugly but I had no better idea how to trigger a error on the websocket client. @@ -187,8 +201,8 @@ private slots: QVERIFY(pushNotificationsWebSocketChildren.size() == 1); emit pushNotificationsWebSocketChildren[0]->sslErrors(QList()); - // Account handled connectionLost signal and deleted PushNotifications - QCOMPARE(account->pushNotifications(), nullptr); + // Account handled connectionLost signal and the authenticationFailed Signal should be emitted + QCOMPARE(pushNotificationsDisabledSpy.count(), 1); } void testAccount_web_socket_connectionLost_emitNotificationsDisabled() @@ -222,25 +236,14 @@ private slots: { FakeWebSocketServer fakeServer; auto account = FakeWebSocketServer::createAccount(); - account->pushNotifications()->setReconnectTimerInterval(0); - QSignalSpy authenticationFailedSpy(account->pushNotifications(), &OCC::PushNotifications::authenticationFailed); - QVERIFY(authenticationFailedSpy.isValid()); + account->setPushNotificationsReconnectInterval(0); QSignalSpy pushNotificationsDisabledSpy(account.data(), &OCC::Account::pushNotificationsDisabled); QVERIFY(pushNotificationsDisabledSpy.isValid()); - // Let three authentication attempts fail - for (uint8_t i = 0; i < 3; ++i) { - QVERIFY(fakeServer.waitForTextMessages()); - QCOMPARE(fakeServer.textMessagesCount(), 2); - auto socket = fakeServer.socketForTextMessage(0); - fakeServer.clearTextMessages(); - socket->sendTextMessage("err: Invalid credentials"); - } + QVERIFY(failThreeAuthenticationAttempts(fakeServer, account)); - // Now the authenticationFailed and pushNotificationsDisabled Signals should be emitted - QVERIFY(pushNotificationsDisabledSpy.wait()); + // Now the pushNotificationsDisabled Signal should be emitted QCOMPARE(pushNotificationsDisabledSpy.count(), 1); - QCOMPARE(authenticationFailedSpy.count(), 1); auto accountSent = pushNotificationsDisabledSpy.at(0).at(0).value(); QCOMPARE(accountSent, account.data()); } From 7112d2aa782df73673155f0f3fe980700d50aa58 Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Apr 2021 13:31:58 +0200 Subject: [PATCH 3/3] Push Notifications: Reconnect forever if capabilities allow it Fixes #3115 Signed-off-by: Felix Weilbach --- src/libsync/account.cpp | 38 ++++++++++++++++++++------- src/libsync/account.h | 2 ++ src/libsync/pushnotifications.cpp | 43 ++++++++++++------------------- src/libsync/pushnotifications.h | 2 +- test/testpushnotifications.cpp | 16 +++++++++++- 5 files changed, 64 insertions(+), 37 deletions(-) diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index ac89dc754..42b605300 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -47,6 +47,10 @@ using namespace QKeychain; +namespace { +constexpr int pushNotificationsReconnectInterval = 1000 * 60 * 2; +} + namespace OCC { Q_LOGGING_CATEGORY(lcAccount, "nextcloud.sync.account", QtInfoMsg) @@ -59,6 +63,9 @@ Account::Account(QObject *parent) { qRegisterMetaType("AccountPtr"); qRegisterMetaType("Account*"); + + _pushNotificationsReconnectTimer.setInterval(pushNotificationsReconnectInterval); + connect(&_pushNotificationsReconnectTimer, &QTimer::timeout, this, &Account::trySetupPushNotifications); } AccountPtr Account::create() @@ -203,29 +210,42 @@ void Account::setCredentials(AbstractCredentials *cred) trySetupPushNotifications(); } +void Account::setPushNotificationsReconnectInterval(int interval) +{ + _pushNotificationsReconnectTimer.setInterval(interval); +} + void Account::trySetupPushNotifications() { + // Stop the timer to prevent parallel setup attempts + _pushNotificationsReconnectTimer.stop(); + if (_capabilities.availablePushNotifications() != PushNotificationType::None) { qCInfo(lcAccount) << "Try to setup push notifications"; if (!_pushNotifications) { _pushNotifications = new PushNotifications(this, this); - connect(_pushNotifications, &PushNotifications::ready, this, [this]() { emit pushNotificationsReady(this); }); + connect(_pushNotifications, &PushNotifications::ready, this, [this]() { + _pushNotificationsReconnectTimer.stop(); + emit pushNotificationsReady(this); + }); - const auto deletePushNotifications = [this]() { - qCInfo(lcAccount) << "Delete push notifications object because authentication failed or connection lost"; + const auto disablePushNotifications = [this]() { + qCInfo(lcAccount) << "Disable push notifications object because authentication failed or connection lost"; if (!_pushNotifications) { return; } - Q_ASSERT(!_pushNotifications->isReady()); - _pushNotifications->deleteLater(); - _pushNotifications = nullptr; - emit pushNotificationsDisabled(this); + if (!_pushNotifications->isReady()) { + emit pushNotificationsDisabled(this); + } + if (!_pushNotificationsReconnectTimer.isActive()) { + _pushNotificationsReconnectTimer.start(); + } }; - connect(_pushNotifications, &PushNotifications::connectionLost, this, deletePushNotifications); - connect(_pushNotifications, &PushNotifications::authenticationFailed, this, deletePushNotifications); + connect(_pushNotifications, &PushNotifications::connectionLost, this, disablePushNotifications); + connect(_pushNotifications, &PushNotifications::authenticationFailed, this, disablePushNotifications); } // If push notifications already running it is no problem to call setup again _pushNotifications->setup(); diff --git a/src/libsync/account.h b/src/libsync/account.h index e20b8c3a6..3c9533346 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -253,6 +253,7 @@ public: void trySetupPushNotifications(); PushNotifications *pushNotifications() const; + void setPushNotificationsReconnectInterval(int interval); public slots: /// Used when forgetting credentials @@ -299,6 +300,7 @@ private: QString _id; QString _davUser; QString _displayName; + QTimer _pushNotificationsReconnectTimer; #ifndef TOKEN_AUTH_ONLY QImage _avatarImg; #endif diff --git a/src/libsync/pushnotifications.cpp b/src/libsync/pushnotifications.cpp index 377973481..fcae8e985 100644 --- a/src/libsync/pushnotifications.cpp +++ b/src/libsync/pushnotifications.cpp @@ -28,7 +28,14 @@ Q_LOGGING_CATEGORY(lcPushNotifications, "nextcloud.sync.pushnotifications", QtIn PushNotifications::PushNotifications(Account *account, QObject *parent) : QObject(parent) , _account(account) + , _webSocket(new QWebSocket(QString(), QWebSocketProtocol::VersionLatest, this)) { + connect(_webSocket, QOverload::of(&QWebSocket::error), this, &PushNotifications::onWebSocketError); + connect(_webSocket, &QWebSocket::sslErrors, this, &PushNotifications::onWebSocketSslErrors); + connect(_webSocket, &QWebSocket::connected, this, &PushNotifications::onWebSocketConnected); + connect(_webSocket, &QWebSocket::disconnected, this, &PushNotifications::onWebSocketDisconnected); + connect(_webSocket, &QWebSocket::pong, this, &PushNotifications::onWebSocketPongReceived); + connect(&_pingTimer, &QTimer::timeout, this, &PushNotifications::pingWebSocketServer); _pingTimer.setSingleShot(true); _pingTimer.setInterval(PING_INTERVAL); @@ -58,7 +65,7 @@ void PushNotifications::reconnectToWebSocket() void PushNotifications::closeWebSocket() { - qCInfo(lcPushNotifications) << "Close websocket" << _webSocket << "for account" << _account->url(); + qCInfo(lcPushNotifications) << "Close websocket for account" << _account->url(); _pingTimer.stop(); _pingTimedOutTimer.stop(); @@ -69,14 +76,12 @@ void PushNotifications::closeWebSocket() _reconnectTimer->stop(); } - if (_webSocket) { - _webSocket->close(); - } + _webSocket->close(); } void PushNotifications::onWebSocketConnected() { - qCInfo(lcPushNotifications) << "Connected to websocket" << _webSocket << "for account" << _account->url(); + qCInfo(lcPushNotifications) << "Connected to websocket for account" << _account->url(); connect(_webSocket, &QWebSocket::textMessageReceived, this, &PushNotifications::onWebSocketTextMessageReceived, Qt::UniqueConnection); @@ -96,7 +101,7 @@ void PushNotifications::authenticateOnWebSocket() void PushNotifications::onWebSocketDisconnected() { - qCInfo(lcPushNotifications) << "Disconnected from websocket" << _webSocket << "for account" << _account->url(); + qCInfo(lcPushNotifications) << "Disconnected from websocket for account" << _account->url(); } void PushNotifications::onWebSocketTextMessageReceived(const QString &message) @@ -125,8 +130,8 @@ void PushNotifications::onWebSocketError(QAbstractSocket::SocketError error) return; } - qCWarning(lcPushNotifications) << "Websocket error on" << _webSocket << "with account" << _account->url() << error; - _isReady = false; + qCWarning(lcPushNotifications) << "Websocket error on with account" << _account->url() << error; + closeWebSocket(); emit connectionLost(); } @@ -154,8 +159,8 @@ bool PushNotifications::tryReconnectToWebSocket() void PushNotifications::onWebSocketSslErrors(const QList &errors) { - qCWarning(lcPushNotifications) << "Websocket ssl errors on" << _webSocket << "with account" << _account->url() << errors; - _isReady = false; + qCWarning(lcPushNotifications) << "Websocket ssl errors on with account" << _account->url() << errors; + closeWebSocket(); emit authenticationFailed(); } @@ -165,21 +170,8 @@ void PushNotifications::openWebSocket() const auto capabilities = _account->capabilities(); const auto webSocketUrl = capabilities.pushNotificationsWebSocketUrl(); - if (!_webSocket) { - _webSocket = new QWebSocket(QString(), QWebSocketProtocol::VersionLatest, this); - qCInfo(lcPushNotifications) << "Created websocket" << _webSocket << "for account" << _account->url(); - } - - if (_webSocket) { - connect(_webSocket, QOverload::of(&QWebSocket::error), this, &PushNotifications::onWebSocketError, Qt::UniqueConnection); - connect(_webSocket, &QWebSocket::sslErrors, this, &PushNotifications::onWebSocketSslErrors, Qt::UniqueConnection); - connect(_webSocket, &QWebSocket::connected, this, &PushNotifications::onWebSocketConnected, Qt::UniqueConnection); - connect(_webSocket, &QWebSocket::disconnected, this, &PushNotifications::onWebSocketDisconnected, Qt::UniqueConnection); - connect(_webSocket, &QWebSocket::pong, this, &PushNotifications::onWebSocketPongReceived, Qt::UniqueConnection); - - qCInfo(lcPushNotifications) << "Open connection to websocket on:" << webSocketUrl; - _webSocket->open(webSocketUrl); - } + qCInfo(lcPushNotifications) << "Open connection to websocket on" << webSocketUrl << "for account" << _account->url(); + _webSocket->open(webSocketUrl); } void PushNotifications::setReconnectTimerInterval(uint32_t interval) @@ -257,7 +249,6 @@ void PushNotifications::startPingTimedOutTimer() void PushNotifications::pingWebSocketServer() { - Q_ASSERT(_webSocket); qCDebug(lcPushNotifications, "Ping websocket server"); _pongReceivedFromWebSocketServer = false; diff --git a/src/libsync/pushnotifications.h b/src/libsync/pushnotifications.h index ebfc9ef09..9d6b8445d 100644 --- a/src/libsync/pushnotifications.h +++ b/src/libsync/pushnotifications.h @@ -129,7 +129,7 @@ private: void emitActivitiesChanged(); Account *_account = nullptr; - QWebSocket *_webSocket = nullptr; + QWebSocket *_webSocket; uint8_t _failedAuthenticationAttemptsCount = 0; QTimer *_reconnectTimer = nullptr; uint32_t _reconnectTimerInterval = 20 * 1000; diff --git a/test/testpushnotifications.cpp b/test/testpushnotifications.cpp index c760161e5..b7e00583f 100644 --- a/test/testpushnotifications.cpp +++ b/test/testpushnotifications.cpp @@ -188,7 +188,7 @@ private slots: QCOMPARE(pushNotificationsDisabledSpy.count(), 1); } - void testOnWebSocketSslError_sslError_deletePushNotifications() + void testOnWebSocketSslError_sslError_disablePushNotifications() { FakeWebSocketServer fakeServer; auto account = FakeWebSocketServer::createAccount(); @@ -272,6 +272,20 @@ private slots: QVERIFY(verifyCalledOnceWithAccount(*activitiesChangedSpy, account)); })); } + + void testTryReconnect_capabilitesReportPushNotificationsAvailable_reconnectForEver() + { + FakeWebSocketServer fakeServer; + auto account = FakeWebSocketServer::createAccount(); + account->setPushNotificationsReconnectInterval(0); + + // Let if fail a few times + QVERIFY(failThreeAuthenticationAttempts(fakeServer, account)); + QVERIFY(failThreeAuthenticationAttempts(fakeServer, account)); + + // Push notifications should try to reconnect + QVERIFY(fakeServer.authenticateAccount(account)); + } }; QTEST_GUILESS_MAIN(TestPushNotifications)