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

[Stable23] Logging, updating status for general error in federation

4 files changed, 39 insertions, 24 deletions

diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
index 73bfaf01b60..021d6abea8b 100644
--- a/apps/dav/lib/CardDAV/SyncService.php
+++ b/apps/dav/lib/CardDAV/SyncService.php
@@ -99,9 +99,11 @@ class SyncService {
 			if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) {
 				// remote server revoked access to the address book, remove it
 				$this->backend->deleteAddressBook($addressBookId);
-				$this->logger->info('Authorization failed, remove address book: ' . $url, ['app' => 'dav']);
-				throw $ex;
+				$this->logger->error('Authorization failed, remove address book: ' . $url, ['app' => 'dav']);
+			} else {
+				$this->logger->error('Client exception:', ['app' => 'dav', 'exception' => $ex]);
 			}
+			throw $ex;
 		}
 
 		// 3. apply changes
diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php
index ace5c07065a..f1803ad7864 100644
--- a/apps/federation/lib/SyncFederationAddressBooks.php
+++ b/apps/federation/lib/SyncFederationAddressBooks.php
@@ -29,6 +29,7 @@ use OC\OCS\DiscoveryService;
 use OCA\DAV\CardDAV\SyncService;
 use OCP\AppFramework\Http;
 use OCP\OCS\IDiscoveryService;
+use Psr\Log\LoggerInterface;
 
 class SyncFederationAddressBooks {
 
@@ -41,18 +42,18 @@ class SyncFederationAddressBooks {
 	/** @var  DiscoveryService */
 	private $ocsDiscoveryService;
 
-	/**
-	 * @param DbHandler $dbHandler
-	 * @param SyncService $syncService
-	 * @param IDiscoveryService $ocsDiscoveryService
-	 */
+	/** @var LoggerInterface */
+	private $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;
 	}
 
 	/**
@@ -71,6 +72,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->error('Shared secret is null');
 				continue;
 			}
 			$targetBookId = $trustedServer['url_hash'];
@@ -82,10 +84,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->info('Token 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 failed because of revoked access.');
+				} else {
+					$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_FAILURE);
+					$this->logger->error('Server sync failed.');
 				}
 				$callback($url, $ex);
 			}
diff --git a/apps/federation/lib/SyncJob.php b/apps/federation/lib/SyncJob.php
index f16d08a80d8..bbf346f3192 100644
--- a/apps/federation/lib/SyncJob.php
+++ b/apps/federation/lib/SyncJob.php
@@ -26,21 +26,20 @@
 namespace OCA\Federation;
 
 use OC\BackgroundJob\TimedJob;
-use OCP\ILogger;
+use Psr\Log\LoggerInterface;
 
 class SyncJob extends TimedJob {
 
 	/** @var SyncFederationAddressBooks */
 	protected $syncService;
 
-	/** @var ILogger */
+	/** @var LoggerInterface */
 	protected $logger;
 
 	/**
 	 * @param SyncFederationAddressBooks $syncService
-	 * @param ILogger $logger
 	 */
-	public function __construct(SyncFederationAddressBooks $syncService, ILogger $logger) {
+	public function __construct(SyncFederationAddressBooks $syncService, LoggerInterface $logger) {
 		// Run once a day
 		$this->setInterval(24 * 60 * 60);
 		$this->syncService = $syncService;
@@ -50,11 +49,11 @@ class SyncJob extends TimedJob {
 	protected function run($argument) {
 		$this->syncService->syncThemAll(function ($url, $ex) {
 			if ($ex instanceof \Exception) {
-				$this->logger->logException($ex, [
-					'message' => "Error while syncing $url.",
-					'level' => ILogger::INFO,
-					'app' => 'fed-sync',
-				]);
+				$this->logger->error("Error while syncing $url.",
+					[
+						'exception' => $ex
+					]
+				);
 			}
 		});
 	}
diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php
index 36dd43e7cd2..b39a80d95e4 100644
--- a/apps/federation/tests/SyncFederationAddressbooksTest.php
+++ b/apps/federation/tests/SyncFederationAddressbooksTest.php
@@ -31,25 +31,31 @@ namespace OCA\Federation\Tests;
 use OC\OCS\DiscoveryService;
 use OCA\Federation\DbHandler;
 use OCA\Federation\SyncFederationAddressBooks;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 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();
@@ -62,8 +68,8 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 					'sync_token' => '0'
 				]
 			]);
-		$dbHandler->expects($this->once())->method('setServerStatus')->
-			with('https://cloud.drop.box', 1, '1');
+		$dbHandler->expects($this->once())->method('setServerStatus')
+			->with('https://cloud.drop.box', 1, '1');
 		$syncService = $this->getMockBuilder('OCA\DAV\CardDAV\SyncService')
 			->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];
 		});