Skip to content

Fix removed address book items not being synced between federated instances #59312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
icewind1991 wants to merge 3 commits into
base: master
Choose a base branch
from
+98 −10
Open

Fix removed address book items not being synced between federated instances #59312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa57c67
fix: send deleted addressbook items in caldav sync
icewind1991 Mar 30, 2026
117233f
feat: add option to perform a full addressbook sync instead of a delt…
icewind1991 Mar 30, 2026
e8b61c7
test: adjust tests to full address book sync
icewind1991 Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions apps/dav/lib/CardDAV/SyncService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
throw $ex;
}

$received = [];
// 3. apply changes
// TODO: use multi-get for download
foreach ($response['response'] as $resource => $status) {
$cardUri = basename($resource);
if (isset($status[200])) {
$received[] = $cardUri;
$absoluteUrl = $this->prepareUri($url, $resource);
$vCard = $this->download($absoluteUrl, $userName, $sharedSecret);
$this->atomic(function () use ($addressBookId, $cardUri, $vCard): void {
Expand All @@ -86,6 +88,15 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
}
}

// when doing a full sync, remove any items in the local address book that aren't in the remote one
if (!$syncToken) {
$existingCards = $this->backend->getCards($addressBookId);
$removedCards = array_filter($existingCards, fn (array $card) => !in_array($card['uri'], $received));
Copy link
Copy Markdown
Contributor

@hamza221 hamza221 Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Premature approval,
This logic doesn't work in case of truncation, not all the cards are received at each step

foreach ($removedCards as $removedCard) {
$this->backend->deleteCard($addressBookId, $removedCard['uri']);
}
}

return [
$response['token'],
$response['truncated'],
Expand Down
3 changes: 2 additions & 1 deletion apps/dav/lib/CardDAV/SystemAddressbook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ public function getChanges($syncToken, $syncLevel, $limit = null) {
return $changed;
}

$added = $modified = $deleted = [];
$added = $modified = [];
$deleted = array_values($changed['deleted']);
foreach ($changed['added'] as $uri) {
try {
$this->getChild($uri);
Expand Down
83 changes: 78 additions & 5 deletions apps/dav/tests/unit/CardDAV/SyncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public function testEmptySync(): void {
'system',
'system',
'1234567890',
null,
'1',
'1',
'principals/system/system',
[]
Expand Down Expand Up @@ -175,7 +175,7 @@ public function testSyncWithNewElement(): void {
'system',
'system',
'1234567890',
null,
'1',
'1',
'principals/system/system',
[]
Expand Down Expand Up @@ -246,7 +246,7 @@ public function testSyncWithUpdatedElement(): void {
'system',
'system',
'1234567890',
null,
'1',
'1',
'principals/system/system',
[]
Expand Down Expand Up @@ -287,7 +287,7 @@ public function testSyncWithDeletedElement(): void {
'system',
'system',
'1234567890',
null,
'1',
'1',
'principals/system/system',
[]
Expand All @@ -296,6 +296,79 @@ public function testSyncWithDeletedElement(): void {
$this->assertEquals('http://sabre.io/ns/sync/4', $token);
}

public function testFullSyncWithOrphanElement(): void {
$this->backend->expects($this->exactly(0))
->method('createCard');
$this->backend->expects($this->exactly(1))
->method('updateCard');
$this->backend->expects($this->exactly(1))
->method('deleteCard');

$body = '<?xml version="1.0"?>
<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">
<d:response>
<d:href>/remote.php/dav/addressbooks/system/system/system/Database:alice.vcf</d:href>
<d:propstat>
<d:prop>
<d:getcontenttype>text/vcard; charset=utf-8</d:getcontenttype>
<d:getetag>&quot;2df155fa5c2a24cd7f750353fc63f037&quot;</d:getetag>
</d:prop>
<d:status>HTTP/1.1 200 OK</d:status>
</d:propstat>
</d:response>
<d:sync-token>http://sabre.io/ns/sync/3</d:sync-token>
</d:multistatus>';

$reportResponse = new Response(new PsrResponse(
207,
['Content-Type' => 'application/xml; charset=utf-8', 'Content-Length' => strlen($body)],
$body
));

$this->client
->method('request')
->willReturn($reportResponse);

$vCard = 'BEGIN:VCARD
VERSION:3.0
PRODID:-//Sabre//Sabre VObject 4.5.4//EN
UID:alice
FN;X-NC-SCOPE=v2-federated:alice
N;X-NC-SCOPE=v2-federated:alice;;;;
X-SOCIALPROFILE;TYPE=NEXTCLOUD;X-NC-SCOPE=v2-published:https://server2.internal/index.php/u/alice
CLOUD:alice@server2.internal
END:VCARD';

$getResponse = new Response(new PsrResponse(
200,
['Content-Type' => 'text/vcard; charset=utf-8', 'Content-Length' => strlen($vCard)],
$vCard,
));

$this->client
->method('get')
->willReturn($getResponse);

$this->backend->method('getCards')
->willReturn([
['uri' => 'Database:alice.vcf'],
['uri' => 'Database:bob.vcf'],
]);

$token = $this->service->syncRemoteAddressBook(
'',
'system',
'system',
'1234567890',
null,
'1',
'principals/system/system',
[]
)[0];

$this->assertEquals('http://sabre.io/ns/sync/3', $token);
}

public function testEnsureSystemAddressBookExists(): void {
/** @var CardDavBackend&MockObject $backend */
$backend = $this->createMock(CardDavBackend::class);
Expand Down Expand Up @@ -458,7 +531,7 @@ public function testUseAbsoluteUriReport(string $host, string $expected): void {
'system',
'remote.php/dav/addressbooks/system/system/system',
'1234567890',
null,
'1',
'1',
'principals/system/system',
[]
Expand Down
7 changes: 5 additions & 2 deletions apps/federation/lib/Command/SyncFederationAddressBooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\ProgressBar;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;

class SyncFederationAddressBooks extends Command {
Expand All @@ -25,19 +26,21 @@ public function __construct(
protected function configure() {
$this
->setName('federation:sync-addressbooks')
->setDescription('Synchronizes addressbooks of all federated clouds');
->setDescription('Synchronizes addressbooks of all federated clouds')
->addOption('full', null, InputOption::VALUE_NONE, 'Perform a full sync instead of a delta sync');
}

protected function execute(InputInterface $input, OutputInterface $output): int {
$progress = new ProgressBar($output);
$progress->start();
$full = (bool)$input->getOption('full');
$this->syncService->syncThemAll(function ($url, $ex) use ($progress, $output): void {
if ($ex instanceof \Exception) {
$output->writeln("Error while syncing $url : " . $ex->getMessage());
} else {
$progress->advance();
}
});
}, $full);

$progress->finish();
$output->writeln('');
Expand Down
4 changes: 2 additions & 2 deletions apps/federation/lib/SyncFederationAddressBooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function __construct(
/**
* @param \Closure $callback
*/
public function syncThemAll(\Closure $callback) {
public function syncThemAll(\Closure $callback, bool $full = false) {
$trustedServers = $this->dbHandler->getAllServer();
foreach ($trustedServers as $trustedServer) {
$url = $trustedServer['url'];
Expand Down Expand Up @@ -55,7 +55,7 @@ public function syncThemAll(\Closure $callback) {
$cardDavUser,
$addressBookUrl,
$sharedSecret,
$syncToken,
$full ? null : $syncToken,
$targetBookId,
$targetPrincipal,
$targetBookProperties
Expand Down
Loading