From 2988efeb7fb83aa6a843647c5e2f40fcc61984d1 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Mon, 4 Jul 2022 15:43:04 +0200 Subject: [PATCH] Add logging to federation Signed-off-by: Anna Larch --- apps/dav/lib/CardDAV/SyncService.php | 4 +++- .../lib/SyncFederationAddressBooks.php | 13 ++++++++++++- apps/federation/lib/SyncJob.php | 2 +- .../tests/SyncFederationAddressbooksTest.php | 16 +++++++++++----- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 5094b7f3f5c55..da798c5768e56 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -73,9 +73,11 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add 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']); + $this->logger->error('Authorization failed, remove address book: ' . $url, ['app' => 'dav']); throw $ex; } + $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 c17cb7618bf00..401fd19bd7516 100644 --- a/apps/federation/lib/SyncFederationAddressBooks.php +++ b/apps/federation/lib/SyncFederationAddressBooks.php @@ -29,19 +29,23 @@ 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 @@ public function syncThemAll(\Closure $callback) { $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 @@ public function syncThemAll(\Closure $callback) { $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 2498f309498e5..82063311f10ae 100644 --- a/apps/federation/lib/SyncJob.php +++ b/apps/federation/lib/SyncJob.php @@ -44,7 +44,7 @@ public function __construct(SyncFederationAddressBooks $syncService, LoggerInter 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 73c44c723996a..a5da446b93168 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 @@ public function testSync() { ->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 @@ public function testSync() { } 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 @@ public function testException() { ->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]; });