Merge pull request #33104 from nextcloud/enhancement/add-logging-federation

Federation - add logging, update the server status if sync fails

3 files changed, 24 insertions, 7 deletions

diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php
index c17cb7618bf..401fd19bd75 100644
--- a/apps/federation/lib/SyncFederationAddressBooks.php
+++ b/apps/federation/lib/SyncFederationAddressBooks.php
@@ -29,19 +29,23 @@ use OC\OCS\DiscoveryService;
 use OCA\DAV\CardDAV\SyncService;
 use OCP\AppFramework\Http;
 use OCP\OCS\IDiscoveryService;
+use Psr\Log\LoggerInterface;
 
 class SyncFederationAddressBooks {
 	protected DbHandler $dbHandler;
 	private SyncService $syncService;
 	private DiscoveryService $ocsDiscoveryService;
+	private LoggerInterface $logger;
 
 	public function __construct(DbHandler $dbHandler,
 								SyncService $syncService,
-								IDiscoveryService $ocsDiscoveryService
+								IDiscoveryService $ocsDiscoveryService,
+								LoggerInterface $logger
 	) {
 		$this->syncService = $syncService;
 		$this->dbHandler = $dbHandler;
 		$this->ocsDiscoveryService = $ocsDiscoveryService;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -60,6 +64,7 @@ class SyncFederationAddressBooks {
 			$addressBookUrl = isset($endPoints['system-address-book']) ? trim($endPoints['system-address-book'], '/') : 'remote.php/dav/addressbooks/system/system/system';
 
 			if (is_null($sharedSecret)) {
+				$this->logger->debug("Shared secret for $url is null");
 				continue;
 			}
 			$targetBookId = $trustedServer['url_hash'];
@@ -71,10 +76,16 @@ class SyncFederationAddressBooks {
 				$newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties);
 				if ($newToken !== $syncToken) {
 					$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken);
+				} else {
+					$this->logger->debug("Sync Token for $url unchanged from previous sync");
 				}
 			} catch (\Exception $ex) {
 				if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) {
 					$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_ACCESS_REVOKED);
+					$this->logger->error("Server sync for $url failed because of revoked access.");
+				} else {
+					$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_FAILURE);
+					$this->logger->error("Server sync for $url failed.");
 				}
 				$callback($url, $ex);
 			}
diff --git a/apps/federation/lib/SyncJob.php b/apps/federation/lib/SyncJob.php
index 2498f309498..82063311f10 100644
--- a/apps/federation/lib/SyncJob.php
+++ b/apps/federation/lib/SyncJob.php
@@ -44,7 +44,7 @@ class SyncJob extends TimedJob {
 	protected function run($argument) {
 		$this->syncService->syncThemAll(function ($url, $ex) {
 			if ($ex instanceof \Exception) {
-				$this->logger->info("Error while syncing $url.", [
+				$this->logger->error("Error while syncing $url.", [
 					'app' => 'fed-sync',
 					'exception' => $ex,
 				]);
diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php
index 73c44c72399..a5da446b931 100644
--- a/apps/federation/tests/SyncFederationAddressbooksTest.php
+++ b/apps/federation/tests/SyncFederationAddressbooksTest.php
@@ -28,28 +28,34 @@
  */
 namespace OCA\Federation\Tests;
 
+use Psr\Log\LoggerInterface;
 use OC\OCS\DiscoveryService;
 use OCA\Federation\DbHandler;
 use OCA\Federation\SyncFederationAddressBooks;
+use PHPUnit\Framework\MockObject\MockObject;
 
 class SyncFederationAddressbooksTest extends \Test\TestCase {
 
 	/** @var array */
 	private $callBacks = [];
 
-	/** @var  \PHPUnit\Framework\MockObject\MockObject | DiscoveryService */
+	/** @var  MockObject | DiscoveryService */
 	private $discoveryService;
 
+	/** @var MockObject|LoggerInterface  */
+	private $logger;
+
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->discoveryService = $this->getMockBuilder(DiscoveryService::class)
 			->disableOriginalConstructor()->getMock();
 		$this->discoveryService->expects($this->any())->method('discover')->willReturn([]);
+		$this->logger = $this->createMock(LoggerInterface::class);
 	}
 
 	public function testSync() {
-		/** @var DbHandler | \PHPUnit\Framework\MockObject\MockObject $dbHandler */
+		/** @var DbHandler | MockObject $dbHandler */
 		$dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')
 			->disableOriginalConstructor()
 			->getMock();
@@ -71,7 +77,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 			->willReturn('1');
 
 		/** @var \OCA\DAV\CardDAV\SyncService $syncService */
-		$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService);
+		$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
 		$s->syncThemAll(function ($url, $ex) {
 			$this->callBacks[] = [$url, $ex];
 		});
@@ -79,7 +85,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 	}
 
 	public function testException() {
-		/** @var DbHandler | \PHPUnit\Framework\MockObject\MockObject $dbHandler */
+		/** @var DbHandler | MockObject $dbHandler */
 		$dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')->
 		disableOriginalConstructor()->
 		getMock();
@@ -99,7 +105,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 			->willThrowException(new \Exception('something did not work out'));
 
 		/** @var \OCA\DAV\CardDAV\SyncService $syncService */
-		$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService);
+		$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
 		$s->syncThemAll(function ($url, $ex) {
 			$this->callBacks[] = [$url, $ex];
 		});