diff --git a/apps/dav/appinfo/v1/carddav.php b/apps/dav/appinfo/v1/carddav.php index bcd66e4709068..415a5c9634a89 100644 --- a/apps/dav/appinfo/v1/carddav.php +++ b/apps/dav/appinfo/v1/carddav.php @@ -63,6 +63,7 @@ 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 d239188058567..4d30d507a7d16 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 @@ private function canWrite(): bool { } 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 06bd8d8ee2c44..a78686eb61d72 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -23,6 +23,7 @@ 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 @@ public function __construct( private IUserManager $userManager, private IEventDispatcher $dispatcher, private Sharing\Backend $sharingBackend, + private IConfig $config, ) { } @@ -851,6 +853,8 @@ public function deleteCard($addressBookId, $cardUri) { * @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 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, '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,22 +915,31 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, ) )->orderBy('synctoken'); - if (is_int($limit) && $limit > 0) { + if ($limit > 0) { $qb->setMaxResults($limit); } // Fetching all changes $stmt = $qb->executeQuery(); + $rowCount = $stmt->rowCount(); $changes = []; + $highestSyncToken = 0; // This loop ensures that any duplicates are overwritten, only the // last change on a node is relevant. while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) { $changes[$row['uri']] = $row['operation']; + $highestSyncToken = $row['synctoken']; } + $stmt->closeCursor(); + // No changes found, use current token + if (empty($changes)) { + $result['syncToken'] = $currentToken; + } + foreach ($changes as $uri => $operation) { switch ($operation) { case 1: @@ -915,16 +953,43 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, break; } } + + /* + * The synctoken in oc_addressbooks is always the highest synctoken in oc_addressbookchanges for a given addressbook plus one (see addChange). + * + * For truncated results, it is expected that we return the highest token from the response, so the client can continue from the latest change. + * + * For non-truncated results, it is expected to return the currentToken. If we return the highest token, as with truncated results, the client will always think it is one change behind. + * + * Therefore, we differentiate between truncated and non-truncated results when returning the synctoken. + */ + if ($rowCount === $limit && $highestSyncToken < $currentToken) { + $result['syncToken'] = $highestSyncToken; + $result['result_truncated'] = true; + } } 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/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 4a75f8ced6cc8..e6da3ed5923d1 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -22,6 +22,7 @@ use Sabre\DAV\Xml\Response\MultiStatus; use Sabre\DAV\Xml\Service; use Sabre\VObject\Reader; +use Sabre\Xml\ParseException; use function is_null; class SyncService { @@ -43,9 +44,10 @@ public function __construct( } /** + * @psalm-return list{0: ?string, 1: boolean} * @throws \Exception */ - public function syncRemoteAddressBook(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken, string $targetBookHash, string $targetPrincipal, array $targetProperties): string { + public function syncRemoteAddressBook(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken, string $targetBookHash, string $targetPrincipal, array $targetProperties): array { // 1. create addressbook $book = $this->ensureSystemAddressBookExists($targetPrincipal, $targetBookHash, $targetProperties); $addressBookId = $book['id']; @@ -83,7 +85,10 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add } } - return $response['token']; + return [ + $response['token'], + $response['truncated'], + ]; } /** @@ -127,7 +132,7 @@ public function ensureLocalSystemAddressBookExists(): ?array { private function prepareUri(string $host, string $path): string { /* - * The trailing slash is important for merging the uris together. + * The trailing slash is important for merging the uris. * * $host is stored in oc_trusted_servers.url and usually without a trailing slash. * @@ -158,7 +163,9 @@ private function prepareUri(string $host, string $path): string { } /** + * @return array{response: array>, token: ?string, truncated: bool} * @throws ClientExceptionInterface + * @throws ParseException */ protected function requestSyncReport(string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken): array { $client = $this->clientService->newClient(); @@ -181,7 +188,7 @@ protected function requestSyncReport(string $url, string $userName, string $addr $body = $response->getBody(); assert(is_string($body)); - return $this->parseMultiStatus($body); + return $this->parseMultiStatus($body, $addressBookUrl); } protected function download(string $url, string $userName, string $sharedSecret, string $resourcePath): string { @@ -219,22 +226,50 @@ private function buildSyncCollectionRequestBody(?string $syncToken): string { } /** - * @param string $body - * @return array - * @throws \Sabre\Xml\ParseException + * @return array{response: array>, token: ?string, truncated: bool} + * @throws ParseException */ - private function parseMultiStatus($body) { - $xml = new Service(); - + private function parseMultiStatus(string $body, string $addressBookUrl): array { /** @var MultiStatus $multiStatus */ - $multiStatus = $xml->expect('{DAV:}multistatus', $body); + $multiStatus = (new Service())->expect('{DAV:}multistatus', $body); $result = []; + $truncated = false; + foreach ($multiStatus->getResponses() as $response) { - $result[$response->getHref()] = $response->getResponseProperties(); + $href = $response->getHref(); + if ($response->getHttpStatus() === '507' && $this->isResponseForRequestUri($href, $addressBookUrl)) { + $truncated = true; + } else { + $result[$response->getHref()] = $response->getResponseProperties(); + } } - return ['response' => $result, 'token' => $multiStatus->getSyncToken()]; + return ['response' => $result, 'token' => $multiStatus->getSyncToken(), 'truncated' => $truncated]; + } + + /** + * Determines whether the provided response URI corresponds to the given request URI. + */ + private function isResponseForRequestUri(string $responseUri, string $requestUri): bool { + /* + * Example response uri: + * + * /remote.php/dav/addressbooks/system/system/system/ + * /cloud/remote.php/dav/addressbooks/system/system/system/ (when installed in a subdirectory) + * + * Example request uri: + * + * remote.php/dav/addressbooks/system/system/system + * + * References: + * https://github.com/nextcloud/3rdparty/blob/e0a509739b13820f0a62ff9cad5d0fede00e76ee/sabre/dav/lib/DAV/Sync/Plugin.php#L172-L174 + * https://github.com/nextcloud/server/blob/b40acb34a39592070d8455eb91c5364c07928c50/apps/federation/lib/SyncFederationAddressBooks.php#L41 + */ + return str_ends_with( + rtrim($responseUri, '/'), + rtrim($requestUri, '/') + ); } /** diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index e0032044e701d..912a2f1dcee05 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -8,7 +8,6 @@ */ namespace OCA\DAV\CardDAV; -use OCA\DAV\Exception\UnsupportedLimitOnInitialSyncException; use OCA\Federation\TrustedServers; use OCP\Accounts\IAccountManager; use OCP\IConfig; @@ -212,14 +211,7 @@ public function getChild($name): Card { } 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 f1963c0ef016e..870aa0d4540ba 100644 --- a/apps/dav/lib/RootCollection.php +++ b/apps/dav/lib/RootCollection.php @@ -132,6 +132,7 @@ public function __construct() { ); $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 @@ public function __construct() { $userManager, $dispatcher, $contactsSharingBackend, + $config ); $usersAddressBookRoot = new AddressBookRoot($userPrincipalBackend, $usersCardDavBackend, $pluginManager, $userSession->getUser(), $groupManager, 'principals/users'); $usersAddressBookRoot->disableListing = $disableListing; @@ -150,6 +152,7 @@ public function __construct() { $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 1966a8d8c9a96..c5eafa0764a42 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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $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 @@ public function testAddressBookSharing(): void { 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 @@ public function testCardOperations(): void { 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 @@ public function testMultiCard(): void { 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 @@ public function testMultipleUIDOnDifferentAddressbooks(): void { 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 @@ public function testMultipleUIDDenied(): void { 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 @@ public function testNoValidUID(): void { 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 @@ public function testDeleteWithoutCard(): void { 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 @@ public function testUpdateProperties(): void { $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 ea4886a67e6de..77caed336f49d 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -108,7 +108,7 @@ public function testEmptySync(): void { '1', 'principals/system/system', [] - ); + )[0]; $this->assertEquals('http://sabre.io/ns/sync/1', $token); } @@ -179,7 +179,7 @@ public function testSyncWithNewElement(): void { '1', 'principals/system/system', [] - ); + )[0]; $this->assertEquals('http://sabre.io/ns/sync/2', $token); } @@ -250,7 +250,7 @@ public function testSyncWithUpdatedElement(): void { '1', 'principals/system/system', [] - ); + )[0]; $this->assertEquals('http://sabre.io/ns/sync/3', $token); } @@ -291,7 +291,7 @@ public function testSyncWithDeletedElement(): void { '1', 'principals/system/system', [] - ); + )[0]; $this->assertEquals('http://sabre.io/ns/sync/4', $token); } diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php index 05144b40879bf..d11f92b76efa8 100644 --- a/apps/federation/lib/SyncFederationAddressBooks.php +++ b/apps/federation/lib/SyncFederationAddressBooks.php @@ -34,7 +34,7 @@ public function syncThemAll(\Closure $callback) { $url = $trustedServer['url']; $callback($url, null); $sharedSecret = $trustedServer['shared_secret']; - $syncToken = $trustedServer['sync_token']; + $oldSyncToken = $trustedServer['sync_token']; $endPoints = $this->ocsDiscoveryService->discover($url, 'FEDERATED_SHARING'); $cardDavUser = $endPoints['carddav-user'] ?? 'system'; @@ -49,10 +49,25 @@ public function syncThemAll(\Closure $callback) { $targetBookProperties = [ '{DAV:}displayname' => $url ]; + try { - $newToken = $this->syncService->syncRemoteAddressBook($url, $cardDavUser, $addressBookUrl, $sharedSecret, $syncToken, $targetBookId, $targetPrincipal, $targetBookProperties); - if ($newToken !== $syncToken) { - $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $newToken); + $syncToken = $oldSyncToken; + + do { + [$syncToken, $truncated] = $this->syncService->syncRemoteAddressBook( + $url, + $cardDavUser, + $addressBookUrl, + $sharedSecret, + $syncToken, + $targetBookId, + $targetPrincipal, + $targetBookProperties + ); + } while ($truncated); + + if ($syncToken !== $oldSyncToken) { + $this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $syncToken); } else { $this->logger->debug("Sync Token for $url unchanged from previous sync"); // The server status might have been changed to a failure status in previous runs. diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php index 8b07520485969..ff03f5cf44232 100644 --- a/apps/federation/tests/SyncFederationAddressbooksTest.php +++ b/apps/federation/tests/SyncFederationAddressbooksTest.php @@ -45,7 +45,7 @@ public function testSync(): void { ->with('https://cloud.example.org', 1, '1'); $syncService = $this->createMock(SyncService::class); $syncService->expects($this->once())->method('syncRemoteAddressBook') - ->willReturn('1'); + ->willReturn(['1', false]); /** @var SyncService $syncService */ $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger); @@ -96,7 +96,7 @@ public function testSuccessfulSyncWithoutChangesAfterFailure(): void { ->with('https://cloud.example.org', 1); $syncService = $this->createMock(SyncService::class); $syncService->expects($this->once())->method('syncRemoteAddressBook') - ->willReturn('0'); + ->willReturn(['0', false]); /** @var SyncService $syncService */ $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger); diff --git a/config/config.sample.php b/config/config.sample.php index 4494cd8c4815e..b5fe5efdbc9ee 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -354,6 +354,11 @@ */ '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 diff --git a/tests/lib/Avatar/UserAvatarTest.php b/tests/lib/Avatar/UserAvatarTest.php index 940a15dd1625a..1ca3b8135cc32 100644 --- a/tests/lib/Avatar/UserAvatarTest.php +++ b/tests/lib/Avatar/UserAvatarTest.php @@ -27,7 +27,7 @@ class UserAvatarTest extends \Test\TestCase { private SimpleFolder&MockObject $folder; private IConfig&MockObject $config; private User&MockObject $user; - + protected function setUp(): void { parent::setUp();