Skip to content

Commit 4a0961d

Browse files
sorbaughsorbaugh
authored
Merge pull request #59982 from nextcloud/backport/59312/stable31
[stable31] Fix removed address book items not being synced between federated instances
2 parents 181f34c + 146ebef commit 4a0961d

6 files changed

Lines changed: 163 additions & 11 deletions

File tree

‎apps/dav/lib/CardDAV/CardDavBackend.php‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,13 @@ public function getCards($addressbookId) {
479479
->from($this->dbCardsTable)
480480
->where($query->expr()->eq('addressbookid', $query->createNamedParameter($addressbookId)));
481481

482+
return $this->getCardsFromQuery($query);
483+
}
484+
485+
/**
486+
* @return array[]
487+
*/
488+
private function getCardsFromQuery(IQueryBuilder $query): array {
482489
$cards = [];
483490

484491
$result = $query->executeQuery();
@@ -973,7 +980,8 @@ public function getChangesForAddressBook($addressBookId, $syncToken, $syncLevel,
973980
->from('cards')
974981
->where(
975982
$qb->expr()->eq('addressbookid', $qb->createNamedParameter($addressBookId))
976-
);
983+
)
984+
->orderBy('id');
977985
// No synctoken supplied, this is the initial sync.
978986
$qb->setMaxResults($limit);
979987
$stmt = $qb->executeQuery();
@@ -1532,4 +1540,32 @@ private function getUID(string $cardData): string {
15321540
// should already be handled, but just in case
15331541
throw new BadRequest('vCard can not be empty');
15341542
}
1543+
1544+
/**
1545+
* Mark all cards in an address book as needing to be validated
1546+
*
1547+
* This is done by setting the modified date to `null`, once a sync runs
1548+
* the mtime will be set to a non-null value. Leaving all deleted items with
1549+
* a null modified date.
1550+
*/
1551+
public function markCardsAsPending(int $addressBookId): void {
1552+
$query = $this->db->getQueryBuilder();
1553+
$query->update($this->dbCardsTable)
1554+
->set('lastmodified', $query->createNamedParameter(null))
1555+
->where($query->expr()->eq('addressbookid', $query->createNamedParameter($addressBookId)))
1556+
->executeStatement();
1557+
}
1558+
1559+
/**
1560+
* @return array[]
1561+
*/
1562+
public function getPendingCards(int $addressBookId): array {
1563+
$query = $this->db->getQueryBuilder();
1564+
$query->select(['id', 'addressbookid', 'uri', 'lastmodified', 'etag', 'size', 'carddata', 'uid'])
1565+
->from($this->dbCardsTable)
1566+
->where($query->expr()->eq('addressbookid', $query->createNamedParameter($addressBookId)))
1567+
->andWhere($query->expr()->isNull('lastmodified'));
1568+
1569+
return $this->getCardsFromQuery($query);
1570+
}
15351571
}

‎apps/dav/lib/CardDAV/SyncService.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,4 +355,15 @@ public function syncInstance(?\Closure $progressCallback = null) {
355355
public static function getCardUri(IUser $user): string {
356356
return $user->getBackendClassName() . ':' . $user->getUID() . '.vcf';
357357
}
358+
359+
public function markCardsAsPending(int $addressBookId): void {
360+
$this->backend->markCardsAsPending($addressBookId);
361+
}
362+
363+
public function deletePendingCards(int $addressBookId): void {
364+
$cards = $this->backend->getPendingCards($addressBookId);
365+
foreach ($cards as $card) {
366+
$this->backend->deleteCard($addressBookId, $card['uri']);
367+
}
368+
}
358369
}

‎apps/dav/lib/CardDAV/SystemAddressbook.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,8 @@ public function getChanges($syncToken, $syncLevel, $limit = null) {
232232
return $changed;
233233
}
234234

235-
$added = $modified = $deleted = [];
235+
$added = $modified = [];
236+
$deleted = array_values($changed['deleted']);
236237
foreach ($changed['added'] as $uri) {
237238
try {
238239
$this->getChild($uri);

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

Lines changed: 97 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
66
* SPDX-License-Identifier: AGPL-3.0-only
77
*/
8+
89
namespace OCA\DAV\Tests\unit\CardDAV;
910

1011
use GuzzleHttp\Exception\ClientException;
@@ -100,7 +101,7 @@ public function testEmptySync(): void {
100101
'system',
101102
'system',
102103
'1234567890',
103-
null,
104+
'1',
104105
'1',
105106
'principals/system/system',
106107
[]
@@ -171,7 +172,7 @@ public function testSyncWithNewElement(): void {
171172
'system',
172173
'system',
173174
'1234567890',
174-
null,
175+
'1',
175176
'1',
176177
'principals/system/system',
177178
[]
@@ -242,7 +243,7 @@ public function testSyncWithUpdatedElement(): void {
242243
'system',
243244
'system',
244245
'1234567890',
245-
null,
246+
'1',
246247
'1',
247248
'principals/system/system',
248249
[]
@@ -283,7 +284,7 @@ public function testSyncWithDeletedElement(): void {
283284
'system',
284285
'system',
285286
'1234567890',
286-
null,
287+
'1',
287288
'1',
288289
'principals/system/system',
289290
[]
@@ -292,6 +293,97 @@ public function testSyncWithDeletedElement(): void {
292293
$this->assertEquals('http://sabre.io/ns/sync/4', $token);
293294
}
294295

296+
public function testFullSyncWithOrphanElement(): void {
297+
$pendingCards = [];
298+
$this->backend->expects($this->exactly(0))
299+
->method('createCard');
300+
$this->backend->expects($this->exactly(1))
301+
->method('updateCard')
302+
->willReturnCallback(function ($id, $uri) use (&$pendingCards) {
303+
unset($pendingCards[$uri]);
304+
});
305+
$this->backend->expects($this->exactly(1))
306+
->method('markCardsAsPending')
307+
->willReturnCallback(function ($id) use (&$pendingCards) {
308+
$cards = array_values($this->backend->getCards($id));
309+
$uris = array_map(fn ($card) => $card['uri'], $cards);
310+
$pendingCards = array_combine($uris, $cards);
311+
});
312+
$this->backend->expects($this->exactly(1))
313+
->method('getPendingCards')
314+
->willReturnCallback(function ($id) use (&$pendingCards) {
315+
return array_values($pendingCards);
316+
});
317+
$this->backend->expects($this->exactly(1))
318+
->method('deleteCard');
319+
320+
$body = '<?xml version="1.0"?>
321+
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
322+
<d:response>
323+
<d:href>/remote.php/dav/addressbooks/system/system/system/Database:alice.vcf</d:href>
324+
<d:propstat>
325+
<d:prop>
326+
<d:getcontenttype>text/vcard; charset=utf-8</d:getcontenttype>
327+
<d:getetag>&quot;2df155fa5c2a24cd7f750353fc63f037&quot;</d:getetag>
328+
</d:prop>
329+
<d:status>HTTP/1.1 200 OK</d:status>
330+
</d:propstat>
331+
</d:response>
332+
<d:sync-token>http://sabre.io/ns/sync/3</d:sync-token>
333+
</d:multistatus>';
334+
335+
$reportResponse = new Response(new PsrResponse(
336+
207,
337+
['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)],
338+
$body
339+
));
340+
341+
$this->client
342+
->method('request')
343+
->willReturn($reportResponse);
344+
345+
$vCard = 'BEGIN:VCARD
346+
VERSION:3.0
347+
PRODID:-//Sabre//Sabre VObject 4.5.4//EN
348+
UID:alice
349+
FN;X-NC-SCOPE=v2-federated:alice
350+
N;X-NC-SCOPE=v2-federated:alice;;;;
351+
X-SOCIALPROFILE;TYPE=NEXTCLOUD;X-NC-SCOPE=v2-published:https://server2.internal/index.php/u/alice
352+
CLOUD:alice@server2.internal
353+
END:VCARD';
354+
355+
$getResponse = new Response(new PsrResponse(
356+
200,
357+
['Content-Type' => 'text/vcard; charset=utf-8', 'Content-Length' => strlen($vCard)],
358+
$vCard,
359+
));
360+
361+
$this->client
362+
->method('get')
363+
->willReturn($getResponse);
364+
365+
$this->backend->method('getCards')
366+
->willReturn([
367+
['uri' => 'Database:alice.vcf'],
368+
['uri' => 'Database:bob.vcf'],
369+
]);
370+
371+
$this->service->markCardsAsPending(1);
372+
$token = $this->service->syncRemoteAddressBook(
373+
'',
374+
'system',
375+
'system',
376+
'1234567890',
377+
null,
378+
'1',
379+
'principals/system/system',
380+
[]
381+
)[0];
382+
$this->service->deletePendingCards(1);
383+
384+
$this->assertEquals('http://sabre.io/ns/sync/3', $token);
385+
}
386+
295387
public function testEnsureSystemAddressBookExists(): void {
296388
/** @var CardDavBackend | \PHPUnit\Framework\MockObject\MockObject $backend */
297389
$backend = $this->getMockBuilder(CardDavBackend::class)->disableOriginalConstructor()->getMock();
@@ -468,7 +560,7 @@ public function testUseAbsoluteUriReport(string $host, string $expected): void {
468560
'system',
469561
'remote.php/dav/addressbooks/system/system/system',
470562
'1234567890',
471-
null,
563+
'1',
472564
'1',
473565
'principals/system/system',
474566
[]

‎apps/federation/lib/Command/SyncFederationAddressBooks.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use Symfony\Component\Console\Command\Command;
1212
use Symfony\Component\Console\Helper\ProgressBar;
1313
use Symfony\Component\Console\Input\InputInterface;
14+
use Symfony\Component\Console\Input\InputOption;
1415
use Symfony\Component\Console\Output\OutputInterface;
1516

1617
class SyncFederationAddressBooks extends Command {
@@ -23,19 +24,21 @@ public function __construct(
2324
protected function configure() {
2425
$this
2526
->setName('federation:sync-addressbooks')
26-
->setDescription('Synchronizes addressbooks of all federated clouds');
27+
->setDescription('Synchronizes addressbooks of all federated clouds')
28+
->addOption('full', null, InputOption::VALUE_NONE, 'Perform a full sync instead of a delta sync');
2729
}
2830

2931
protected function execute(InputInterface $input, OutputInterface $output): int {
3032
$progress = new ProgressBar($output);
3133
$progress->start();
34+
$full = (bool)$input->getOption('full');
3235
$this->syncService->syncThemAll(function ($url, $ex) use ($progress, $output): void {
3336
if ($ex instanceof \Exception) {
3437
$output->writeln("Error while syncing $url : " . $ex->getMessage());
3538
} else {
3639
$progress->advance();
3740
}
38-
});
41+
}, $full);
3942

4043
$progress->finish();
4144
$output->writeln('');

‎apps/federation/lib/SyncFederationAddressBooks.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public function __construct(
2828
/**
2929
* @param \Closure $callback
3030
*/
31-
public function syncThemAll(\Closure $callback) {
31+
public function syncThemAll(\Closure $callback, bool $full = false) {
3232
$trustedServers = $this->dbHandler->getAllServer();
3333
foreach ($trustedServers as $trustedServer) {
3434
$url = $trustedServer['url'];
@@ -51,7 +51,12 @@ public function syncThemAll(\Closure $callback) {
5151
];
5252

5353
try {
54-
$syncToken = $oldSyncToken;
54+
$syncToken = $full ? null : $oldSyncToken;
55+
56+
$book = $this->syncService->ensureSystemAddressBookExists($targetPrincipal, $targetBookId, $targetBookProperties);
57+
if ($full) {
58+
$this->syncService->markCardsAsPending($book['id']);
59+
}
5560

5661
do {
5762
[$syncToken, $truncated] = $this->syncService->syncRemoteAddressBook(
@@ -66,6 +71,10 @@ public function syncThemAll(\Closure $callback) {
6671
);
6772
} while ($truncated);
6873

74+
if ($full) {
75+
$this->syncService->deletePendingCards($book['id']);
76+
}
77+
6978
if ($syncToken !== $oldSyncToken) {
7079
$this->dbHandler->setServerStatus($url, TrustedServers::STATUS_OK, $syncToken);
7180
} else {

0 commit comments

Comments
 (0)