fix(carddav): return correct sync token for non-truncated requests

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

Author: kesselb

apps/dav/appinfo/v1/carddav.php

@@ -56,6 +56,8 @@
 	\OC::$server->getUserManager(),
 	\OC::$server->get(IEventDispatcher::class),
 	\OC::$server->get(\OCA\DAV\CardDAV\Sharing\Backend::class),
+	\OC::$server->get(OCP\IConfig::class),
+
 );
 
 $debugging = \OC::$server->getConfig()->getSystemValue('debug', false);

apps/dav/lib/CardDAV/CardDavBackend.php

@@ -920,18 +920,20 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel,
 
 				// 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'];
-					// get the last synctoken, needed in case a limit was set
-					$result['syncToken'] = $row['synctoken'];
+					$highestSyncToken = $row['synctoken'];
 				}
+
 				$stmt->closeCursor();
-				
+
 				// No changes found, use current token
 				if (empty($changes)) {
 					$result['syncToken'] = $currentToken;
@@ -950,6 +952,20 @@ 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('id', 'uri')

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

@@ -56,6 +56,9 @@ class CardDavBackendTest extends TestCase {
 	/** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */
 	private $groupManager;
 
+	/** @var IConfig|MockObject */
+	private $config;
+
 	/** @var IEventDispatcher|MockObject */
 	private $dispatcher;
 	private Backend $sharingBackend;
@@ -239,7 +242,7 @@ public function testAddressBookSharing(): void {
 	public function testCardOperations(): void {
 		/** @var CardDavBackend | \PHPUnit\Framework\MockObject\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();
 
 		// create a new address book
@@ -294,7 +297,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])
 			->setMethods(['updateProperties'])->getMock();
 
 		// create a new address book
@@ -347,7 +350,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();
 
 		// create 2 new address books
@@ -369,7 +372,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])
 			->setMethods(['updateProperties'])->getMock();
 
 		// create a new address book
@@ -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])
 			->setMethods(['updateProperties'])->getMock();
 
 		// create a new address book
@@ -447,7 +450,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])
 			->setMethods(['updateProperties'])->getMock();
 
 		// create a new address book

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

@@ -104,7 +104,7 @@ public function testEmptySync(): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/1', $token);
 	}
@@ -175,7 +175,7 @@ public function testSyncWithNewElement(): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/2', $token);
 	}
@@ -246,7 +246,7 @@ public function testSyncWithUpdatedElement(): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/3', $token);
 	}
@@ -287,7 +287,7 @@ public function testSyncWithDeletedElement(): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		)[0];
 
 		$this->assertEquals('http://sabre.io/ns/sync/4', $token);
 	}
@@ -430,7 +430,7 @@ public function testDeleteAddressbookWhenAccessRevoked(): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		);
 	}
 
 	/**
@@ -472,7 +472,7 @@ public function testUseAbsoluteUriReport(string $host, string $expected): void {
 			'1',
 			'principals/system/system',
 			[]
-		)['token'];
+		);
 	}
 
 	public function providerUseAbsoluteUriReport(): array {

apps/federation/tests/SyncFederationAddressbooksTest.php

@@ -55,7 +55,7 @@ public function testSync(): void {
 			->disableOriginalConstructor()
 			->getMock();
 		$syncService->expects($this->once())->method('syncRemoteAddressBook')
-			->willReturn('1');
+			->willReturn(['1', false]);
 
 		/** @var SyncService $syncService */
 		$s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger);
@@ -114,7 +114,7 @@ public function testSuccessfulSyncWithoutChangesAfterFailure(): void {
 			->disableOriginalConstructor()
 			->getMock();
 		$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

@@ -346,6 +346,12 @@
  */
 '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` enabled 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