fix(federation): always set server status to OK after successful runs

Previously if a server status got set to failure, it stayed that way
until an addressbook-sync found changes. Now the server status is set to
OK after each successful sync check (if that's not the case already),
regardless of addressbook changes.

This change also includes two new logging statements, which could help
next time someone debugs this.

Signed-off-by: Pablo Zimdahl <pablo@nextcloud.com>

diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php
index f07e0d8c2a703357b76f1b4b69230893d25ae54f..1a23d58a7d100fabe8b400e1fcfd50786d63f06b 100644 (file)
--- a/apps/federation/lib/BackgroundJob/GetSharedSecret.php
+++ b/apps/federation/lib/BackgroundJob/GetSharedSecret.php
@@ -90,6 +90,7 @@ class GetSharedSecret extends Job {
                // kill job after 30 days of trying
                $deadline = $currentTime - $this->maxLifespan;
                if ($created < $deadline) {
+                       $this->logger->warning("The job to get the shared secret job is too old and gets stopped now without retention. Setting server status of '{$target}' to failure.");
                        $this->retainJob = false;
                        $this->trustedServers->setServerStatus($target, TrustedServers::STATUS_FAILURE);
                        return;

diff --git a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php
index 07243f5c94d51ffa47a3a9d760ebbb4c59fa17fd..a1d0d2b0df06ac017dfd8a2cd1e3da5c0defde39 100644 (file)
--- a/apps/federation/lib/BackgroundJob/RequestSharedSecret.php
+++ b/apps/federation/lib/BackgroundJob/RequestSharedSecret.php
@@ -93,6 +93,7 @@ class RequestSharedSecret extends Job {
                // kill job after 30 days of trying
                $deadline = $currentTime - $this->maxLifespan;
                if ($created < $deadline) {
+                       $this->logger->warning("The job to request the shared secret job is too old and gets stopped now without retention. Setting server status of '{$target}' to failure.");
                        $this->retainJob = false;
                        $this->trustedServers->setServerStatus($target, TrustedServers::STATUS_FAILURE);
                        return;

diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php
index 46a6a8c40a512be7852c8a3e56228afa82729c04..1b47c92db1a4ccac7e5baa1722cc6e55f21e6d58 100644 (file)
--- a/apps/federation/lib/SyncFederationAddressBooks.php
+++ b/apps/federation/lib/SyncFederationAddressBooks.php
@@ -60,6 +60,10 @@ class SyncFederationAddressBooks {
                                        $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken);
                                } else {
                                        $this->logger->debug("Sync Token for $url unchanged from previous sync");
+                                       // The server status might have been changed to a failure status in previous runs.
+                                       if ($this->dbHandler->getServerStatus($url) !== TrustedServers::STATUS_OK) {
+                                               $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK);
+                                       }
                                }
                        } catch (\Exception $ex) {
                                if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) {

diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php
index 1ebca2f6a0f40c7ce974f96d3b1ed027ccdf2c20..1b53f238bfaa6389e0be9face65fb22a63c93c91 100644 (file)
--- a/apps/federation/tests/SyncFederationAddressbooksTest.php
+++ b/apps/federation/tests/SyncFederationAddressbooksTest.php
@@ -90,4 +90,35 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
                });
                $this->assertEquals(2, count($this->callBacks));
        }
+
+       public function testSuccessfulSyncWithoutChangesAfterFailure() {
+               /** @var DbHandler | MockObject $dbHandler */
+               $dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')
+                       ->disableOriginalConstructor()
+                       ->getMock();
+               $dbHandler->method('getAllServer')
+                       ->willReturn([
+                               [
+                                       'url' => 'https://cloud.drop.box',
+                                       'url_hash' => 'sha1',
+                                       'shared_secret' => 'ilovenextcloud',
+                                       'sync_token' => '0'
+                               ]
+                       ]);
+               $dbHandler->method('getServerStatus')->willReturn(\OCA\Federation\TrustedServers::STATUS_FAILURE);
+               $dbHandler->expects($this->once())->method('setServerStatus')->
+                       with('https://cloud.drop.box', 1);
+               $syncService = $this->getMockBuilder('OCA\DAV\CardDAV\SyncService')
+                       ->disableOriginalConstructor()
+                       ->getMock();
+               $syncService->expects($this->once())->method('syncRemoteAddressBook')
+                       ->willReturn('0');
+
+               /** @var \OCA\DAV\CardDAV\SyncService $syncService */
+               $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
+               $s->syncThemAll(function ($url, $ex) {
+                       $this->callBacks[] = [$url, $ex];
+               });
+               $this->assertEquals('1', count($this->callBacks));
+       }
 }