From 36d9fcbb4d45977a6dabd90f2ee8d10d74b1c61e Mon Sep 17 00:00:00 2001 From: Hamza Mahjoubi Date: Wed, 8 Jan 2025 20:39:18 +0700 Subject: [PATCH] feat(cardav): support result truncation for addressbook federation Signed-off-by: Hamza Mahjoubi --- apps/dav/appinfo/v1/carddav.php | 1 + apps/dav/lib/CardDAV/AddressBook.php | 4 -- apps/dav/lib/CardDAV/CardDavBackend.php | 61 +++++++++++++++++-- apps/dav/lib/CardDAV/SystemAddressbook.php | 8 --- apps/dav/lib/RootCollection.php | 3 + .../tests/unit/CardDAV/CardDavBackendTest.php | 21 ++++--- .../tests/unit/CardDAV/SyncServiceTest.php | 12 ++-- .../lib/SyncFederationAddressBooks.php | 6 ++ config/config.sample.php | 5 ++ 9 files changed, 88 insertions(+), 33 deletions(-) diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index bcd66e47090..415a5c9634a 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -63,6 +63,7 @@ $cardDavBackend = new CardDavBackend( Server::get(IUserManager::class), Server::get(IEventDispatcher::class), Server::get(\OCA\DAV\CardDAV\Sharing\Backend::class), + Server::get(IConfig::class), ); $debugging = Server::get(IConfig::class)->getSystemValue('debug', false); diff --git a/apps/dav/lib/CardDAV/AddressBook.php b/apps/dav/lib/CardDAV/AddressBook.php index d2391880585..4d30d507a7d 100644 --- a/apps/dav/lib/CardDAV/AddressBook.php +++ b/apps/dav/lib/CardDAV/AddressBook.php @@ -8,7 +8,6 @@ namespace OCA\DAV\CardDAV; use OCA\DAV\DAV\Sharing\IShareable; -use OCA\DAV\Exception\UnsupportedLimitOnInitialSyncException; use OCP\DB\Exception; use OCP\IL10N; use OCP\Server; @@ -234,9 +233,6 @@ class AddressBook extends \Sabre\CardDAV\AddressBook implements IShareable, IMov } public function getChanges($syncToken, $syncLevel, $limit = null) { - if (!$syncToken && $limit) { - throw new UnsupportedLimitOnInitialSyncException(); - } return parent::getChanges($syncToken, $syncLevel, $limit); } diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index 06bd8d8ee2c..3006e54e7b7 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -23,6 +23,7 @@ use OCP\AppFramework\Db\TTransactional; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use OCP\IDBConnection; use OCP\IUserManager; use PDO; @@ -59,6 +60,7 @@ class CardDavBackend implements BackendInterface, SyncSupport { private IUserManager $userManager, private IEventDispatcher $dispatcher, private Sharing\Backend $sharingBackend, + private IConfig $config, ) { } @@ -851,6 +853,8 @@ class CardDavBackend implements BackendInterface, SyncSupport { * @return array */ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $limit = null) { + $maxLimit = $this->config->getSystemValueInt('carddav_sync_request_truncation', 2500); + $limit = ($limit === null) ? $maxLimit : min($limit, $maxLimit); // Current synctoken return $this->atomic(function () use ($addressBookId, $syncToken, $syncLevel, $limit) { $qb = $this->db->getQueryBuilder(); @@ -873,10 +877,35 @@ class CardDavBackend implements BackendInterface, SyncSupport { 'modified' => [], 'deleted' => [], ]; - - if ($syncToken) { + if (str_starts_with($syncToken, 'init_')) { + $syncValues = explode('_', $syncToken); + $lastID = $syncValues[1]; + $initialSyncToken = $syncValues[2]; $qb = $this->db->getQueryBuilder(); - $qb->select('uri', 'operation') + $qb->select('id', 'uri') + ->from('cards') + ->where( + $qb->expr()->andX( + $qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId)), + $qb->expr()->gt('id', $qb->createNamedParameter($lastID))) + )->orderBy('id') + ->setMaxResults($limit); + $stmt = $qb->executeQuery(); + $values = $stmt->fetchAll(\PDO::FETCH_ASSOC); + $stmt->closeCursor(); + if (count($values) === 0) { + $result['syncToken'] = $initialSyncToken; + $result['result_truncated'] = false; + $result['added'] = []; + } else { + $lastID = $values[array_key_last($values)]['id']; + $result['added'] = array_column($values, 'uri'); + $result['syncToken'] = count($result['added']) >= $limit ? "init_{$lastID}_$initialSyncToken" : $initialSyncToken ; + $result['result_truncated'] = count($result['added']) >= $limit; + } + } elseif ($syncToken) { + $qb = $this->db->getQueryBuilder(); + $qb->select('uri', 'operation', 'synctoken') ->from('addressbookchanges') ->where( $qb->expr()->andX( @@ -886,7 +915,7 @@ class CardDavBackend implements BackendInterface, SyncSupport { ) )->orderBy('synctoken'); - if (is_int($limit) && $limit > 0) { + if ($limit > 0) { $qb->setMaxResults($limit); } @@ -899,8 +928,15 @@ class CardDavBackend implements BackendInterface, SyncSupport { // last change on a node is relevant. while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) { $changes[$row['uri']] = $row['operation']; + // get the last synctoken, needed in case a limit was set + $result['syncToken'] = $row['synctoken']; } $stmt->closeCursor(); + + // No changes found, use current token + if (empty($changes)) { + $result['syncToken'] = $currentToken; + } foreach ($changes as $uri => $operation) { switch ($operation) { @@ -917,14 +953,27 @@ class CardDavBackend implements BackendInterface, SyncSupport { } } else { $qb = $this->db->getQueryBuilder(); - $qb->select('uri') + $qb->select('id', 'uri') ->from('cards') ->where( $qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId)) ); // No synctoken supplied, this is the initial sync. + $qb->setMaxResults($limit); $stmt = $qb->executeQuery(); - $result['added'] = $stmt->fetchAll(\PDO::FETCH_COLUMN); + $values = $stmt->fetchAll(\PDO::FETCH_ASSOC); + if (empty($values)) { + $result['added'] = []; + return $result; + } + $lastID = $values[array_key_last($values)]['id']; + if (count($values) >= $limit) { + $result['syncToken'] = 'init_' . $lastID . '_' . $currentToken; + $result['result_truncated'] = true; + } + + $result['added'] = array_column($values, 'uri'); + $stmt->closeCursor(); } return $result; diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index e0032044e70..912a2f1dcee 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -8,7 +8,6 @@ declare(strict_types=1); */ namespace OCA\DAV\CardDAV; -use OCA\DAV\Exception\UnsupportedLimitOnInitialSyncException; use OCA\Federation\TrustedServers; use OCP\Accounts\IAccountManager; use OCP\IConfig; @@ -212,14 +211,7 @@ class SystemAddressbook extends AddressBook { } return new Card($this->carddavBackend, $this->addressBookInfo, $obj); } - - /** - * @throws UnsupportedLimitOnInitialSyncException - */ public function getChanges($syncToken, $syncLevel, $limit = null) { - if (!$syncToken && $limit) { - throw new UnsupportedLimitOnInitialSyncException(); - } if (!$this->carddavBackend instanceof SyncSupport) { return null; diff --git a/apps/dav/lib/RootCollection.php b/apps/dav/lib/RootCollection.php index f1963c0ef01..870aa0d4540 100644 --- a/apps/dav/lib/RootCollection.php +++ b/apps/dav/lib/RootCollection.php @@ -132,6 +132,7 @@ class RootCollection extends SimpleCollection { ); $contactsSharingBackend = Server::get(\OCA\DAV\CardDAV\Sharing\Backend::class); + $config = Server::get(IConfig::class); $pluginManager = new PluginManager(\OC::$server, Server::get(IAppManager::class)); $usersCardDavBackend = new CardDavBackend( @@ -140,6 +141,7 @@ class RootCollection extends SimpleCollection { $userManager, $dispatcher, $contactsSharingBackend, + $config ); $usersAddressBookRoot = new AddressBookRoot($userPrincipalBackend, $usersCardDavBackend, $pluginManager, $userSession->getUser(), $groupManager, 'principals/users'); $usersAddressBookRoot->disableListing = $disableListing; @@ -150,6 +152,7 @@ class RootCollection extends SimpleCollection { $userManager, $dispatcher, $contactsSharingBackend, + $config ); $systemAddressBookRoot = new AddressBookRoot(new SystemPrincipalBackend(), $systemCardDavBackend, $pluginManager, $userSession->getUser(), $groupManager, 'principals/system'); $systemAddressBookRoot->disableListing = $disableListing; diff --git a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php index 1966a8d8c9a..c5eafa0764a 100644 --- a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php +++ b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php @@ -50,6 +50,7 @@ class CardDavBackendTest extends TestCase { private IUserManager&MockObject $userManager; private IGroupManager&MockObject $groupManager; private IEventDispatcher&MockObject $dispatcher; + private IConfig&MockObject $config; private Backend $sharingBackend; private IDBConnection $db; private CardDavBackend $backend; @@ -96,6 +97,7 @@ class CardDavBackendTest extends TestCase { $this->userManager = $this->createMock(IUserManager::class); $this->groupManager = $this->createMock(IGroupManager::class); + $this->config = $this->createMock(IConfig::class); $this->principal = $this->getMockBuilder(Principal::class) ->setConstructorArgs([ $this->userManager, @@ -106,7 +108,7 @@ class CardDavBackendTest extends TestCase { $this->createMock(IAppManager::class), $this->createMock(ProxyMapper::class), $this->createMock(KnownUserService::class), - $this->createMock(IConfig::class), + $this->config, $this->createMock(IFactory::class) ]) ->onlyMethods(['getPrincipalByPath', 'getGroupMembership', 'findByUri']) @@ -135,6 +137,7 @@ class CardDavBackendTest extends TestCase { $this->userManager, $this->dispatcher, $this->sharingBackend, + $this->config, ); // start every test with a empty cards_properties and cards table $query = $this->db->getQueryBuilder(); @@ -231,7 +234,7 @@ class CardDavBackendTest extends TestCase { public function testCardOperations(): void { /** @var CardDavBackend&MockObject $backend */ $backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend,$this->config]) ->onlyMethods(['updateProperties', 'purgeProperties']) ->getMock(); @@ -291,7 +294,7 @@ class CardDavBackendTest extends TestCase { public function testMultiCard(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend,$this->config]) ->onlyMethods(['updateProperties']) ->getMock(); @@ -345,7 +348,7 @@ class CardDavBackendTest extends TestCase { public function testMultipleUIDOnDifferentAddressbooks(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend,$this->config]) ->onlyMethods(['updateProperties']) ->getMock(); @@ -368,7 +371,7 @@ class CardDavBackendTest extends TestCase { public function testMultipleUIDDenied(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config]) ->onlyMethods(['updateProperties']) ->getMock(); @@ -390,7 +393,7 @@ class CardDavBackendTest extends TestCase { public function testNoValidUID(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config]) ->onlyMethods(['updateProperties']) ->getMock(); @@ -408,7 +411,7 @@ class CardDavBackendTest extends TestCase { public function testDeleteWithoutCard(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config]) ->onlyMethods([ 'getCardId', 'addChange', @@ -453,7 +456,7 @@ class CardDavBackendTest extends TestCase { public function testSyncSupport(): void { $this->backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config]) ->onlyMethods(['updateProperties']) ->getMock(); @@ -522,7 +525,7 @@ class CardDavBackendTest extends TestCase { $cardId = 2; $backend = $this->getMockBuilder(CardDavBackend::class) - ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend]) + ->setConstructorArgs([$this->db, $this->principal, $this->userManager, $this->dispatcher, $this->sharingBackend, $this->config]) ->onlyMethods(['getCardId'])->getMock(); $backend->expects($this->any())->method('getCardId')->willReturn($cardId); diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index ea4886a67e6..9bcabf0c7c4 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -108,7 +108,7 @@ class SyncServiceTest extends TestCase { '1', 'principals/system/system', [] - ); + )['token']; $this->assertEquals('http://sabre.io/ns/sync/1', $token); } @@ -179,7 +179,7 @@ END:VCARD'; '1', 'principals/system/system', [] - ); + )['token']; $this->assertEquals('http://sabre.io/ns/sync/2', $token); } @@ -250,7 +250,7 @@ END:VCARD'; '1', 'principals/system/system', [] - ); + )['token']; $this->assertEquals('http://sabre.io/ns/sync/3', $token); } @@ -291,7 +291,7 @@ END:VCARD'; '1', 'principals/system/system', [] - ); + )['token']; $this->assertEquals('http://sabre.io/ns/sync/4', $token); } @@ -422,7 +422,7 @@ END:VCARD'; '1', 'principals/system/system', [] - ); + )['token']; } #[\PHPUnit\Framework\Attributes\DataProvider('providerUseAbsoluteUriReport')] @@ -462,7 +462,7 @@ END:VCARD'; '1', 'principals/system/system', [] - ); + )['token']; } public static function providerUseAbsoluteUriReport(): array { diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php index 05144b40879..7f700cde21b 100644 --- a/apps/federation/lib/SyncFederationAddressBooks.php +++ b/apps/federation/lib/SyncFederationAddressBooks.php @@ -52,6 +52,12 @@ class SyncFederationAddressBooks { try { $newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties); if ($newToken !== $syncToken) { + // Finish truncated initial sync. + if (strpos($newToken, 'init') !== false) { + do { + $newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties); + } while (str_contains($newToken, 'init_')); + } $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken); } else { $this->logger->debug("Sync Token for $url unchanged from previous sync"); diff --git a/config/config.sample.php b/config/config.sample.php index 4494cd8c481..b5fe5efdbc9 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -354,6 +354,11 @@ $CONFIG = [ */ 'carddav_sync_request_timeout' => 30, +/** + * The limit applied to the synchronization report request, e.g. federated system address books (as run by `occ federation:sync-addressbooks`). + */ +'carddav_sync_request_truncation' => 2500, + /** * `true` enables a relaxed session timeout, where the session timeout would no longer be * handled by Nextcloud but by either the PHP garbage collection or the expiration of