From 11089f7d3341afd05048ad4899919f15155a3a35 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Mon, 4 Jul 2022 15:24:10 +0200 Subject: [PATCH] Add logging to federation sync Signed-off-by: Anna Larch --- apps/dav/lib/CardDAV/SyncService.php | 6 ++++-- .../lib/SyncFederationAddressBooks.php | 20 +++++++++++++------ apps/federation/lib/SyncJob.php | 17 ++++++++-------- .../tests/SyncFederationAddressbooksTest.php | 20 ++++++++++++------- 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]; }); -- 2.39.5