Merge pull request #50092 from nextcloud/feat/sync-truncation

feat(cardav): support result truncation for addressbook federation

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);

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);
 	}

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,22 +915,31 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 						)
 					)->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 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 							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;

apps/dav/lib/CardDAV/SyncService.php

@@ -22,6 +22,7 @@ use Psr\Log\LoggerInterface;
 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 @@ class SyncService {
 	}
 
 	/**
+	 * @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 @@ class SyncService {
 			}
 		}
 
-		return $response['token'];
+		return [
+			$response['token'],
+			$response['truncated'],
+		];
 	}
 
 	/**
@@ -127,7 +132,7 @@ class SyncService {
 
 	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 @@ class SyncService {
 	}
 
 	/**
+	 * @return array{response: array<string, array<array-key, mixed>>, 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 @@ class SyncService {
 		$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 @@ class SyncService {
 	}
 
 	/**
-	 * @param string $body
-	 * @return array
-	 * @throws \Sabre\Xml\ParseException
+	 * @return array{response: array<string, array<array-key, mixed>>, 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, '/')
+		);
 	}
 
 	/**

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;

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;

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);

apps/dav/tests/unit/CardDAV/SyncServiceTest.php

@@ -108,7 +108,7 @@ class SyncServiceTest extends TestCase {
 			'1',
 			'principals/system/system',
 			[]
-		);
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/1', $token);
 	}
@@ -179,7 +179,7 @@ END:VCARD';
 			'1',
 			'principals/system/system',
 			[]
-		);
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/2', $token);
 	}
@@ -250,7 +250,7 @@ END:VCARD';
 			'1',
 			'principals/system/system',
 			[]
-		);
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/3', $token);
 	}
@@ -291,7 +291,7 @@ END:VCARD';
 			'1',
 			'principals/system/system',
 			[]
-		);
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/4', $token);
 	}

apps/federation/lib/SyncFederationAddressBooks.php

@@ -34,7 +34,7 @@ class SyncFederationAddressBooks {
 			$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 @@ class SyncFederationAddressBooks {
 			$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.

apps/federation/tests/SyncFederationAddressbooksTest.php

@@ -45,7 +45,7 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 			->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 @@ class SyncFederationAddressbooksTest extends \Test\TestCase {
 			->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);

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

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();