diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 310f1ec83be3d..cf172be79e323 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -333,6 +333,7 @@ 'OCA\\DAV\\Events\\SubscriptionDeletedEvent' => $baseDir . '/../lib/Events/SubscriptionDeletedEvent.php', 'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => $baseDir . '/../lib/Events/SubscriptionUpdatedEvent.php', 'OCA\\DAV\\Exception\\ExampleEventException' => $baseDir . '/../lib/Exception/ExampleEventException.php', + 'OCA\\DAV\\Exception\\InvalidSyncTokenException' => $baseDir . '/../lib/Exception/InvalidSyncTokenException.php', 'OCA\\DAV\\Exception\\ServerMaintenanceMode' => $baseDir . '/../lib/Exception/ServerMaintenanceMode.php', 'OCA\\DAV\\Exception\\UidConflict' => $baseDir . '/../lib/Exception/UidConflict.php', 'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => $baseDir . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 98a49d46284dd..a399d6581fd35 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -348,6 +348,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Events\\SubscriptionDeletedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionDeletedEvent.php', 'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionUpdatedEvent.php', 'OCA\\DAV\\Exception\\ExampleEventException' => __DIR__ . '/..' . '/../lib/Exception/ExampleEventException.php', + 'OCA\\DAV\\Exception\\InvalidSyncTokenException' => __DIR__ . '/..' . '/../lib/Exception/InvalidSyncTokenException.php', 'OCA\\DAV\\Exception\\ServerMaintenanceMode' => __DIR__ . '/..' . '/../lib/Exception/ServerMaintenanceMode.php', 'OCA\\DAV\\Exception\\UidConflict' => __DIR__ . '/..' . '/../lib/Exception/UidConflict.php', 'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => __DIR__ . '/..' . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php', diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index d181d25f7644c..ce8f3d18278ce 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -2886,6 +2886,10 @@ public function getChangesForCalendar($calendarId, $syncToken, $syncLevel, $limi return null; } + if (!$initialSync && $this->isSyncTokenOutdated((int)$calendarId, $calendarType, (int)$syncToken, (int)$currentToken)) { + return null; + } + // evaluate if this is a initial sync and construct appropriate command if ($initialSync) { $qb = $this->db->getQueryBuilder(); @@ -3312,6 +3316,12 @@ public function createSchedulingObject($principalUri, $objectUri, $objectData) { * @return void */ protected function addChanges(int $calendarId, array $objectUris, int $operation, int $calendarType = self::CALENDAR_TYPE_CALENDAR): void { + if ($objectUris === []) { + // Bumping the sync token without a change record would leave a gap that + // looks like pruned history to isSyncTokenOutdated() + return; + } + $this->cachedObjects = []; $table = $calendarType === self::CALENDAR_TYPE_CALENDAR ? 'calendars': 'calendarsubscriptions'; @@ -3347,6 +3357,32 @@ protected function addChanges(int $calendarId, array $objectUris, int $operation }, $this->db); } + /** + * Checks whether changes after the given sync token were already removed by + * pruneOutdatedSyncTokens(). A delta sync from such a token would silently + * miss those changes, so the client has to do a full sync instead. + */ + private function isSyncTokenOutdated(int $calendarId, int $calendarType, int $syncToken, int $currentToken): bool { + // Sync tokens start at 1, a token of 0 describes the same (empty) state + $syncToken = max($syncToken, 1); + if ($syncToken >= $currentToken) { + return false; + } + + $qb = $this->db->getQueryBuilder(); + $qb->select($qb->func()->min('synctoken')) + ->from('calendarchanges') + ->where($qb->expr()->eq('calendarid', $qb->createNamedParameter($calendarId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('calendartype', $qb->createNamedParameter($calendarType, IQueryBuilder::PARAM_INT))); + $result = $qb->executeQuery(); + $oldestSyncToken = $result->fetchOne(); + $result->closeCursor(); + + // Every change bumps the sync token by one (see addChanges), so the history + // is only complete if the oldest change at most is the client's sync token + return $oldestSyncToken === null || $oldestSyncToken === false || (int)$oldestSyncToken > $syncToken; + } + public function restoreChanges(int $calendarId, int $calendarType = self::CALENDAR_TYPE_CALENDAR): void { $this->cachedObjects = []; diff --git a/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php b/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php index 45ea9be17be7f..58eebf8be78d1 100644 --- a/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php +++ b/apps/dav/lib/CalDAV/Federation/FederatedCalendarSyncService.php @@ -10,6 +10,7 @@ namespace OCA\DAV\CalDAV\Federation; use OCA\DAV\CalDAV\CalDavBackend; +use OCA\DAV\Exception\InvalidSyncTokenException; use OCA\DAV\Service\ASyncService; use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Http; @@ -64,15 +65,33 @@ private function getCredentials(FederatedCalendarEntity $calendar): array { */ public function syncOne(FederatedCalendarEntity $calendar): int { $credentials = $this->getCredentials($calendar); - $syncToken = $calendar->getSyncTokenForSabre(); + // Never synced before: request an initial sync instead of replaying the remote's + // change history from the start, which might have been pruned already + $syncToken = $calendar->getSyncToken() === 0 ? null : $calendar->getSyncTokenForSabre(); + $fullSync = $syncToken === null; try { - $response = $this->requestSyncReport( - $credentials['remoteUrl'], - $credentials['username'], - $credentials['token'], - $syncToken, - ); + try { + $response = $this->requestSyncReport( + $credentials['remoteUrl'], + $credentials['username'], + $credentials['token'], + $syncToken, + ); + } catch (InvalidSyncTokenException $e) { + // The remote no longer has the changes since our sync token, e.g. because it pruned them. + // A delta sync would leave us with an incomplete calendar, so start over. + $this->logger->warning("Sync token for {$credentials['remoteUrl']} was rejected by the remote server, performing a full sync", [ + 'exception' => $e, + ]); + $fullSync = true; + $response = $this->requestSyncReport( + $credentials['remoteUrl'], + $credentials['username'], + $credentials['token'], + null, + ); + } } catch (ClientExceptionInterface $ex) { if ($ex->getCode() === Http::STATUS_UNAUTHORIZED) { // Remote server revoked access to the calendar => remove it @@ -88,8 +107,10 @@ public function syncOne(FederatedCalendarEntity $calendar): int { // Process changes from remote $downloadedEvents = 0; + $remoteObjectUris = []; foreach ($response['response'] as $resource => $status) { $objectUri = basename($resource); + $remoteObjectUris[$objectUri] = true; if (isset($status[200])) { // Object created or updated $absoluteUrl = $this->prepareUri($credentials['remoteUrl'], $resource); @@ -128,6 +149,21 @@ public function syncOne(FederatedCalendarEntity $calendar): int { } } + if ($fullSync) { + // A full sync only lists existing objects, so drop everything the remote does not have anymore + $localObjects = $this->backend->getCalendarObjects($calendar->getId(), CalDavBackend::CALENDAR_TYPE_FEDERATED); + foreach ($localObjects as $localObject) { + if (!isset($remoteObjectUris[$localObject['uri']])) { + $this->backend->deleteCalendarObject( + $calendar->getId(), + $localObject['uri'], + CalDavBackend::CALENDAR_TYPE_FEDERATED, + true + ); + } + } + } + $newSyncToken = $response['token']; // Check sync token format and extract the actual sync token integer diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index 51357b704cf51..8f7e108b106d0 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -897,7 +897,7 @@ public function deleteCard($addressBookId, $cardUri) { * @param string $syncToken * @param int $syncLevel * @param int|null $limit - * @return array + * @return array|null */ #[\Override] public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $limit = null) { @@ -929,6 +929,9 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $syncValues = explode('_', $syncToken); $lastID = $syncValues[1]; $initialSyncToken = $syncValues[2]; + if ($this->isSyncTokenOutdated((int)$addressBookId, (int)$initialSyncToken, (int)$currentToken)) { + return null; + } $qb = $this->db->getQueryBuilder(); $qb->select('id', 'uri') ->from('cards') @@ -952,6 +955,10 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, $result['result_truncated'] = count($result['added']) >= $limit; } } elseif ($syncToken) { + if ($this->isSyncTokenOutdated((int)$addressBookId, (int)$syncToken, (int)$currentToken)) { + return null; + } + $qb = $this->db->getQueryBuilder(); $qb->select('uri', 'operation', 'synctoken') ->from('addressbookchanges') @@ -969,7 +976,8 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, // Fetching all changes $stmt = $qb->executeQuery(); - $rowCount = $stmt->rowCount(); + // Count the fetched rows ourselves, rowCount() is not reliable for SELECT statements + $rowCount = 0; $changes = []; $highestSyncToken = 0; @@ -979,6 +987,7 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, while ($row = $stmt->fetchAssociative()) { $changes[$row['uri']] = $row['operation']; $highestSyncToken = $row['synctoken']; + $rowCount++; } $stmt->closeCursor(); @@ -1011,7 +1020,7 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, * * Therefore, we differentiate between truncated and non-truncated results when returning the synctoken. */ - if ($rowCount === $limit && $highestSyncToken < $currentToken) { + if ($limit > 0 && $rowCount === $limit && $highestSyncToken < $currentToken) { $result['syncToken'] = $highestSyncToken; $result['result_truncated'] = true; } @@ -1045,6 +1054,31 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel, }, $this->db); } + /** + * Checks whether changes after the given sync token were already removed by + * pruneOutdatedSyncTokens(). A delta sync from such a token would silently + * miss those changes, so the client has to do a full sync instead. + */ + private function isSyncTokenOutdated(int $addressBookId, int $syncToken, int $currentToken): bool { + // Sync tokens start at 1, a token of 0 describes the same (empty) state + $syncToken = max($syncToken, 1); + if ($syncToken >= $currentToken) { + return false; + } + + $qb = $this->db->getQueryBuilder(); + $qb->select($qb->func()->min('synctoken')) + ->from('addressbookchanges') + ->where($qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId, IQueryBuilder::PARAM_INT))); + $result = $qb->executeQuery(); + $oldestSyncToken = $result->fetchOne(); + $result->closeCursor(); + + // Every change bumps the sync token by one (see addChange), so the history + // is only complete if the oldest change at most is the client's sync token + return $oldestSyncToken === null || $oldestSyncToken === false || (int)$oldestSyncToken > $syncToken; + } + /** * Adds a change record to the addressbookchanges table. * diff --git a/apps/dav/lib/Exception/InvalidSyncTokenException.php b/apps/dav/lib/Exception/InvalidSyncTokenException.php new file mode 100644 index 0000000000000..c8d4405489b52 --- /dev/null +++ b/apps/dav/lib/Exception/InvalidSyncTokenException.php @@ -0,0 +1,18 @@ +>, token: ?string, truncated: bool} + * @throws InvalidSyncTokenException If the remote server rejected the sync token */ protected function requestSyncReport( string $absoluteUrl, @@ -93,11 +96,18 @@ protected function requestSyncReport( ), ]; - $response = $client->request( - 'REPORT', - $absoluteUrl, - $options, - ); + try { + $response = $client->request( + 'REPORT', + $absoluteUrl, + $options, + ); + } catch (BadResponseException $e) { + if ($syncToken !== null && $this->isInvalidSyncTokenResponse($e)) { + throw new InvalidSyncTokenException("Sync token $syncToken was rejected by $absoluteUrl", $e->getCode(), $e); + } + throw $e; + } $body = $response->getBody(); assert(is_string($body)); @@ -105,6 +115,16 @@ protected function requestSyncReport( return $this->parseMultiStatus($body, $absoluteUrl); } + /** + * RFC 6578 3.2: An expired or unknown sync token is answered with + * 403 Forbidden and the DAV:valid-sync-token precondition. + */ + private function isInvalidSyncTokenResponse(BadResponseException $e): bool { + $response = $e->getResponse(); + return $response->getStatusCode() === 403 + && str_contains((string)$response->getBody(), 'valid-sync-token'); + } + protected function download( string $absoluteUrl, string $userName, diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php index 3abc8084b4f88..efb265e3edd9a 100644 --- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php +++ b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php @@ -1565,12 +1565,12 @@ public function testPruneOutdatedSyncTokens(): void { $deleted = $this->backend->pruneOutdatedSyncTokens(0, 0); // At least one from the object creation and one from the object update $this->assertGreaterThanOrEqual(2, $deleted); - $changes = $this->backend->getChangesForCalendar($calendarId, $syncToken, 1); - $this->assertEmpty($changes['added']); - $this->assertEmpty($changes['modified']); - $this->assertEmpty($changes['deleted']); + // The changes since the sync token are gone, so it must be rejected instead of returning an incomplete delta + $this->assertNull($this->backend->getChangesForCalendar($calendarId, $syncToken, 1)); - // Test that objects remain + // Start over with an initial sync + $changes = $this->backend->getChangesForCalendar($calendarId, '', 1); + $syncToken = $changes['syncToken']; // Currently changes are empty $changes = $this->backend->getChangesForCalendar($calendarId, $syncToken, 100); @@ -1601,6 +1601,7 @@ public function testPruneOutdatedSyncTokens(): void { $this->assertEquals(1, count($changes['added'])); $this->assertEmpty($changes['modified']); $this->assertEmpty($changes['deleted']); + $syncTokenAfterAdd = $changes['syncToken']; // update the card $calData = <<<'EOD' @@ -1631,8 +1632,11 @@ public function testPruneOutdatedSyncTokens(): void { $deleted = $this->backend->pruneOutdatedSyncTokens(1, 0); $this->assertEquals(1, $deleted); // We had two changes before, now one + // The add is gone, so a delta from before it must be rejected + $this->assertNull($this->backend->getChangesForCalendar($calendarId, $syncToken, 100)); + // Only update should remain - $changes = $this->backend->getChangesForCalendar($calendarId, $syncToken, 100); + $changes = $this->backend->getChangesForCalendar($calendarId, $syncTokenAfterAdd, 100); $this->assertEmpty($changes['added']); $this->assertEquals(1, count($changes['modified'])); $this->assertEmpty($changes['deleted']); @@ -1792,10 +1796,20 @@ public function testRestoreChanges(): void { $this->backend->restoreChanges($calendarId); - $changesAfter = $this->backend->getChangesForCalendar($calendarId, $changesBefore['syncToken'], 1); - self::assertEquals([], $changesAfter['added']); - self::assertEqualsCanonicalizing([$uri1, $uri3], $changesAfter['modified']); - self::assertEquals([$uri2], $changesAfter['deleted']); + // The original changes are gone, so a client with an old sync token has to do a full sync + self::assertNull($this->backend->getChangesForCalendar($calendarId, $changesBefore['syncToken'], 1)); + + $query = $this->db->getQueryBuilder(); + $query->select('uri', 'operation') + ->from('calendarchanges') + ->where($query->expr()->eq('calendarid', $query->createNamedParameter($calendarId))); + $restored = []; + foreach ($query->executeQuery()->fetchAllAssociative() as $row) { + $restored[(int)$row['operation']][] = $row['uri']; + } + self::assertArrayNotHasKey(1, $restored); + self::assertEqualsCanonicalizing([$uri1, $uri3], $restored[2]); + self::assertEquals([$uri2], $restored[3]); } public function testSearchWithLimitAndTimeRange(): void { diff --git a/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php b/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php index 21c410a879e1a..2c75864d1a1fc 100644 --- a/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php +++ b/apps/dav/tests/unit/CalDAV/Federation/FederatedCalendarSyncServiceTest.php @@ -9,6 +9,9 @@ namespace OCA\DAV\Tests\unit\CalDAV\Federation; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Request as PsrRequest; +use GuzzleHttp\Psr7\Response as PsrResponse; use OCA\DAV\CalDAV\CalDavBackend; use OCA\DAV\CalDAV\Federation\FederatedCalendarEntity; use OCA\DAV\CalDAV\Federation\FederatedCalendarMapper; @@ -146,6 +149,121 @@ public function testSyncOneUnchanged(): void { $this->assertEquals(0, $this->federatedCalendarSyncService->syncOne($calendar)); } + private function mockCloudIdAndConfig(): void { + $cloudId = $this->createMock(ICloudId::class); + $cloudId->method('getId') + ->willReturn('user1@nextcloud.testing'); + $this->cloudIdManager->method('getCloudId') + ->with('user1') + ->willReturn($cloudId); + + $this->config->method('getSystemValueInt') + ->willReturn(30); + $this->config->method('getSystemValue') + ->willReturn(false); + } + + private function createFullSyncResponse(): IResponse { + $response = $this->createMock(IResponse::class); + $response->method('getBody') + ->willReturn(' + + + /remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2/event1.ics + "1"HTTP/1.1 200 OK + + http://sabre.io/ns/sync/120 +'); + return $response; + } + + public function testSyncOneFullSyncWhenSyncTokenIsRejected(): void { + $calendar = new FederatedCalendarEntity(); + $calendar->setId(1); + $calendar->setPrincipaluri('principals/users/user1'); + $calendar->setRemoteUrl('https://remote.tld/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2'); + $calendar->setSyncToken(100); + $calendar->setToken('token'); + $this->mockCloudIdAndConfig(); + + $invalidSyncToken = new ClientException( + 'Client error: `REPORT` resulted in a `403 Forbidden` response', + new PsrRequest('REPORT', $calendar->getRemoteUrl()), + new PsrResponse(403, [], ''), + ); + + $requestBodies = []; + $client = $this->createMock(IClient::class); + $client->expects(self::exactly(2)) + ->method('request') + ->willReturnCallback(function (string $method, string $url, array $options) use (&$requestBodies, $invalidSyncToken) { + $requestBodies[] = $options['body']; + if (count($requestBodies) === 1) { + throw $invalidSyncToken; + } + return $this->createFullSyncResponse(); + }); + $downloadResponse = $this->createMock(IResponse::class); + $downloadResponse->method('getBody') + ->willReturn('BEGIN:VCALENDAR' . "\r\n" . 'END:VCALENDAR'); + $client->method('get') + ->willReturn($downloadResponse); + $this->clientService->method('newClient') + ->willReturn($client); + + $this->backend->method('getCalendarObject') + ->willReturn(['uri' => 'event1.ics']); + $this->backend->expects(self::once()) + ->method('updateCalendarObject') + ->with(1, 'event1.ics', self::anything(), CalDavBackend::CALENDAR_TYPE_FEDERATED); + $this->backend->method('getCalendarObjects') + ->with(1, CalDavBackend::CALENDAR_TYPE_FEDERATED) + ->willReturn([['uri' => 'event1.ics'], ['uri' => 'stale.ics']]); + // Only the object that is gone on the remote is removed + $this->backend->expects(self::once()) + ->method('deleteCalendarObject') + ->with(1, 'stale.ics', CalDavBackend::CALENDAR_TYPE_FEDERATED, true); + + $this->federatedCalendarMapper->expects(self::once()) + ->method('updateSyncTokenAndTime') + ->with(1, 120); + + $this->assertEquals(1, $this->federatedCalendarSyncService->syncOne($calendar)); + $this->assertStringContainsString('http://sabre.io/ns/sync/100', $requestBodies[0]); + $this->assertStringNotContainsString('http://sabre.io/ns/sync/', $requestBodies[1]); + } + + public function testSyncOneNeverSyncedDoesInitialSync(): void { + $calendar = new FederatedCalendarEntity(); + $calendar->setId(1); + $calendar->setPrincipaluri('principals/users/user1'); + $calendar->setRemoteUrl('https://remote.tld/remote.php/dav/remote-calendars/abcdef123/cal1_shared_by_user2'); + $calendar->setSyncToken(0); + $calendar->setToken('token'); + $this->mockCloudIdAndConfig(); + + $response = $this->createMock(IResponse::class); + $response->method('getBody') + ->willReturn('http://sabre.io/ns/sync/5'); + $client = $this->createMock(IClient::class); + $client->expects(self::once()) + ->method('request') + ->with('REPORT', $calendar->getRemoteUrl(), self::callback( + fn (array $options): bool => !str_contains($options['body'], 'http://sabre.io/ns/sync/') + )) + ->willReturn($response); + $this->clientService->method('newClient') + ->willReturn($client); + + $this->backend->method('getCalendarObjects') + ->willReturn([]); + $this->federatedCalendarMapper->expects(self::once()) + ->method('updateSyncTokenAndTime') + ->with(1, 5); + + $this->assertEquals(0, $this->federatedCalendarSyncService->syncOne($calendar)); + } + public static function provideUnexpectedSyncTokenData(): array { return [ ['http://sabre.io/ns/sync/'], diff --git a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php index 03d049b34da83..8f649573cfe9c 100644 --- a/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php +++ b/apps/dav/tests/unit/CardDAV/CardDavBackendTest.php @@ -512,6 +512,38 @@ public function testSyncSupport(): void { $this->assertEquals($uri0, $changes['added'][0]); } + public function testGetChangesForAddressBookTruncatesDeltaSync(): void { + $this->config->method('getSystemValueInt') + ->with('carddav_sync_request_truncation', 2500) + ->willReturn(2); + + $bookId = $this->backend->createAddressBook(self::UNIT_TEST_USER, 'Example', []); + $syncToken = $this->backend->getChangesForAddressBook($bookId, '', 1)['syncToken']; + + $uris = []; + foreach ([$this->vcardTest0, $this->vcardTest1, $this->vcardTest2] as $vcard) { + $uri = $this->getUniqueID('card'); + $this->backend->createCard($bookId, $uri, $vcard); + $uris[] = $uri; + } + + $changes = $this->backend->getChangesForAddressBook($bookId, $syncToken, 1); + $this->assertSame([$uris[0], $uris[1]], $changes['added']); + $this->assertTrue($changes['result_truncated']); + + // Follow the truncated pages until the end + $added = $changes['added']; + for ($i = 0; $i < 5 && ($changes['result_truncated'] ?? false); $i++) { + $changes = $this->backend->getChangesForAddressBook($bookId, $changes['syncToken'], 1); + $added = array_merge($added, $changes['added']); + } + $this->assertFalse($changes['result_truncated'] ?? false); + $this->assertEqualsCanonicalizing($uris, array_unique($added)); + + $currentToken = $this->backend->getAddressBookById($bookId)['{http://sabredav.org/ns}sync-token']; + $this->assertEquals($currentToken, $changes['syncToken']); + } + public function testSharing(): void { $this->userManager->expects($this->any()) ->method('userExists') @@ -930,12 +962,12 @@ public function testPruneOutdatedSyncTokens(): void { $deleted = $this->backend->pruneOutdatedSyncTokens(0, 0); // At least one from the object creation and one from the object update $this->assertGreaterThanOrEqual(2, $deleted); - $changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 1); - $this->assertEmpty($changes['added']); - $this->assertEmpty($changes['modified']); - $this->assertEmpty($changes['deleted']); + // The changes since the sync token are gone, so it must be rejected instead of returning an incomplete delta + $this->assertNull($this->backend->getChangesForAddressBook($addressBookId, $syncToken, 1)); - // Test that objects remain + // Start over with an initial sync + $changes = $this->backend->getChangesForAddressBook($addressBookId, '', 1); + $syncToken = $changes['syncToken']; // Currently changes are empty $changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 100); @@ -949,6 +981,7 @@ public function testPruneOutdatedSyncTokens(): void { $this->assertEquals(1, count($changes['added'])); $this->assertEmpty($changes['modified']); $this->assertEmpty($changes['deleted']); + $syncTokenAfterAdd = $changes['syncToken']; // Update card $this->backend->updateCard($addressBookId, $uri, $this->vcardTest1); @@ -962,8 +995,11 @@ public function testPruneOutdatedSyncTokens(): void { $deleted = $this->backend->pruneOutdatedSyncTokens(1, 0); $this->assertEquals(1, $deleted); // We had two changes before, now one + // The add is gone, so a delta from before it must be rejected + $this->assertNull($this->backend->getChangesForAddressBook($addressBookId, $syncToken, 100)); + // Only update should remain - $changes = $this->backend->getChangesForAddressBook($addressBookId, $syncToken, 100); + $changes = $this->backend->getChangesForAddressBook($addressBookId, $syncTokenAfterAdd, 100); $this->assertEmpty($changes['added']); $this->assertEquals(1, count($changes['modified'])); $this->assertEmpty($changes['deleted']); diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php index 232c5c392abae..43052fb8ea71f 100644 --- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php +++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php @@ -15,6 +15,7 @@ use OCA\DAV\CardDAV\CardDavBackend; use OCA\DAV\CardDAV\Converter; use OCA\DAV\CardDAV\SyncService; +use OCA\DAV\Exception\InvalidSyncTokenException; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\IConfig; @@ -592,6 +593,69 @@ public function testDeleteAddressbookWhenAccessRevoked(): void { ); } + private function createForbiddenReportException(string $body): ClientException { + $request = new PsrRequest( + 'REPORT', + 'https://server2.internal/remote.php/dav/addressbooks/system/system/system', + ['Content-Type' => 'application/xml'], + ); + $response = new PsrResponse( + 403, + ['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)], + $body + ); + return new ClientException('Client error: `REPORT` resulted in a `403 Forbidden` response', $request, $response); + } + + public function testInvalidSyncToken(): void { + $body = ' + + Sabre\DAV\Exception\InvalidSyncToken + Invalid or unknown sync token + +'; + $this->client + ->method('request') + ->willThrowException($this->createForbiddenReportException($body)); + $this->backend->expects($this->never()) + ->method('deleteAddressBook'); + + $this->expectException(InvalidSyncTokenException::class); + $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + 'http://sabre.io/ns/sync/42', + '1', + 'principals/system/system', + [] + ); + } + + public function testOtherForbiddenIsNotInvalidSyncToken(): void { + $body = ' + + Sabre\DAV\Exception\Forbidden + Access denied +'; + $this->client + ->method('request') + ->willThrowException($this->createForbiddenReportException($body)); + + $this->expectException(ClientException::class); + $this->service->syncRemoteAddressBook( + '', + 'system', + 'system', + '1234567890', + 'http://sabre.io/ns/sync/42', + '1', + 'principals/system/system', + [] + ); + } + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'providerUseAbsoluteUriReport')] public function testUseAbsoluteUriReport(string $host, string $expected): void { $body = ' diff --git a/apps/federation/lib/SyncFederationAddressBooks.php b/apps/federation/lib/SyncFederationAddressBooks.php index f61cde8e10d3a..49d3b914add15 100644 --- a/apps/federation/lib/SyncFederationAddressBooks.php +++ b/apps/federation/lib/SyncFederationAddressBooks.php @@ -9,6 +9,7 @@ namespace OCA\Federation; use OCA\DAV\CardDAV\SyncService; +use OCA\DAV\Exception\InvalidSyncTokenException; use OCP\AppFramework\Http; use OCP\OCS\IDiscoveryService; use Psr\Log\LoggerInterface; @@ -48,28 +49,27 @@ public function syncThemAll(\Closure $callback, bool $full = false) { ]; try { - $syncToken = $full ? null : $oldSyncToken; + $sync = fn (bool $fullSync): ?string => $this->syncAddressBook( + $fullSync, + $fullSync ? null : $oldSyncToken, + $url, + $cardDavUser, + $addressBookUrl, + $sharedSecret, + $targetBookId, + $targetPrincipal, + $targetBookProperties + ); - $book = $this->syncService->ensureSystemAddressBookExists($targetPrincipal, $targetBookId, $targetBookProperties); - if ($full) { - $this->syncService->markCardsAsPending($book['id']); - } - - do { - [$syncToken, $truncated] = $this->syncService->syncRemoteAddressBook( - $url, - $cardDavUser, - $addressBookUrl, - $sharedSecret, - $syncToken, - $targetBookId, - $targetPrincipal, - $targetBookProperties - ); - } while ($truncated); - - if ($full) { - $this->syncService->deletePendingCards($book['id']); + try { + $syncToken = $sync($full); + } catch (InvalidSyncTokenException $e) { + // The remote no longer has the changes since our sync token, e.g. because it pruned them. + // A delta sync would leave us with an incomplete address book, so start over. + $this->logger->warning("Sync token for $url was rejected by the remote server, performing a full sync", [ + 'exception' => $e, + ]); + $syncToken = $sync(true); } if ($syncToken !== $oldSyncToken) { @@ -97,4 +97,44 @@ public function syncThemAll(\Closure $callback, bool $full = false) { } } } + + /** + * @return ?string The new sync token + * @throws \Exception + */ + private function syncAddressBook( + bool $full, + ?string $syncToken, + string $url, + string $cardDavUser, + string $addressBookUrl, + string $sharedSecret, + string $targetBookId, + string $targetPrincipal, + array $targetBookProperties, + ): ?string { + $book = $this->syncService->ensureSystemAddressBookExists($targetPrincipal, $targetBookId, $targetBookProperties); + if ($full) { + $this->syncService->markCardsAsPending($book['id']); + } + + do { + [$syncToken, $truncated] = $this->syncService->syncRemoteAddressBook( + $url, + $cardDavUser, + $addressBookUrl, + $sharedSecret, + $syncToken, + $targetBookId, + $targetPrincipal, + $targetBookProperties + ); + } while ($truncated); + + if ($full) { + $this->syncService->deletePendingCards($book['id']); + } + + return $syncToken; + } } diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php index 1dacbb288ba8e..e9b45e8024aab 100644 --- a/apps/federation/tests/SyncFederationAddressbooksTest.php +++ b/apps/federation/tests/SyncFederationAddressbooksTest.php @@ -11,6 +11,7 @@ use OC\OCS\DiscoveryService; use OCA\DAV\CardDAV\SyncService; +use OCA\DAV\Exception\InvalidSyncTokenException; use OCA\Federation\DbHandler; use OCA\Federation\SyncFederationAddressBooks; use OCA\Federation\TrustedServers; @@ -56,6 +57,49 @@ public function testSync(): void { $this->assertCount(1, $this->callBacks); } + public function testFullSyncWhenSyncTokenIsRejected(): void { + /** @var DbHandler&MockObject $dbHandler */ + $dbHandler = $this->createMock(DbHandler::class); + $dbHandler->method('getAllServer') + ->willReturn([ + [ + 'url' => 'https://cloud.example.org', + 'url_hash' => 'sha1', + 'shared_secret' => 'ilovenextcloud', + 'sync_token' => 'http://sabre.io/ns/sync/30996077' + ] + ]); + $dbHandler->expects($this->once())->method('setServerStatus') + ->with('https://cloud.example.org', TrustedServers::STATUS_OK, 'http://sabre.io/ns/sync/31000000'); + $syncService = $this->createMock(SyncService::class); + $syncService->method('ensureSystemAddressBookExists') + ->willReturn(['id' => 42]); + $calls = []; + $syncService->expects($this->exactly(3))->method('syncRemoteAddressBook') + ->willReturnCallback(function (string $url, string $userName, string $addressBookUrl, string $sharedSecret, ?string $syncToken) use (&$calls) { + $calls[] = $syncToken; + return match (count($calls)) { + 1 => throw new InvalidSyncTokenException(), + 2 => ['http://sabre.io/ns/sync/init_100_31000000', true], + 3 => ['http://sabre.io/ns/sync/31000000', false], + }; + }); + $syncService->expects($this->once())->method('markCardsAsPending')->with(42); + $syncService->expects($this->once())->method('deletePendingCards')->with(42); + + /** @var SyncService $syncService */ + $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService, $this->logger); + $s->syncThemAll(function ($url, $ex): void { + $this->callBacks[] = [$url, $ex]; + }); + $this->assertSame([ + 'http://sabre.io/ns/sync/30996077', + null, + 'http://sabre.io/ns/sync/init_100_31000000', + ], $calls); + $this->assertCount(1, $this->callBacks); + } + public function testException(): void { /** @var DbHandler&MockObject $dbHandler */ $dbHandler = $this->createMock(DbHandler::class);