Merge pull request #37965 from nextcloud/fix/transactional-system-add…

…ressbook-sync fix(dav): Run system address book create-if-not-exists in transaction
nextcloud · May 16, 2023 · 25dd264 · 25dd264
2 parents ab84824 + 2cc672d
commit 25dd264
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 30 deletions.
diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -30,7 +31,9 @@
 namespace OCA\DAV\CardDAV;
 
 use OC\Accounts\AccountManager;
+use OCP\AppFramework\Db\TTransactional;
 use OCP\AppFramework\Http;
+use OCP\IDBConnection;
 use OCP\IUser;
 use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
@@ -39,24 +42,31 @@
 use Sabre\DAV\Xml\Service;
 use Sabre\HTTP\ClientHttpException;
 use Sabre\VObject\Reader;
+use function is_null;
 
 class SyncService {
+
+	use TTransactional;
+
 	private CardDavBackend $backend;
 	private IUserManager $userManager;
+	private IDBConnection $dbConnection;
 	private LoggerInterface $logger;
 	private ?array $localSystemAddressBook = null;
 	private Converter $converter;
 	protected string $certPath;
 
 	public function __construct(CardDavBackend $backend,
 								IUserManager $userManager,
+								IDBConnection $dbConnection,
 								LoggerInterface $logger,
 								Converter $converter) {
 		$this->backend = $backend;
 		$this->userManager = $userManager;
 		$this->logger = $logger;
 		$this->converter = $converter;
 		$this->certPath = '';
+		$this->dbConnection = $dbConnection;
 	}
 
 	/**
@@ -87,12 +97,14 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
 			$cardUri = basename($resource);
 			if (isset($status[200])) {
 				$vCard = $this->download($url, $userName, $sharedSecret, $resource);
-				$existingCard = $this->backend->getCard($addressBookId, $cardUri);
-				if ($existingCard === false) {
-					$this->backend->createCard($addressBookId, $cardUri, $vCard['body']);
-				} else {
-					$this->backend->updateCard($addressBookId, $cardUri, $vCard['body']);
-				}
+				$this->atomic(function() use ($addressBookId, $cardUri, $vCard) {
+					$existingCard = $this->backend->getCard($addressBookId, $cardUri);
+					if ($existingCard === false) {
+						$this->backend->createCard($addressBookId, $cardUri, $vCard['body']);
+					} else {
+						$this->backend->updateCard($addressBookId, $cardUri, $vCard['body']);
+					}
+				}, $this->dbConnection);
 			} else {
 				$this->backend->deleteCard($addressBookId, $cardUri);
 			}
@@ -105,14 +117,15 @@ public function syncRemoteAddressBook(string $url, string $userName, string $add
 	 * @throws \Sabre\DAV\Exception\BadRequest
 	 */
 	public function ensureSystemAddressBookExists(string $principal, string $uri, array $properties): ?array {
-		$book = $this->backend->getAddressBooksByUri($principal, $uri);
-		if (!is_null($book)) {
-			return $book;
-		}
-		// FIXME This might break in clustered DB setup
-		$this->backend->createAddressBook($principal, $uri, $properties);
+		return $this->atomic(function() use ($principal, $uri, $properties) {
+			$book = $this->backend->getAddressBooksByUri($principal, $uri);
+			if (!is_null($book)) {
+				return $book;
+			}
+			$this->backend->createAddressBook($principal, $uri, $properties);
 
-		return $this->backend->getAddressBooksByUri($principal, $uri);
+			return $this->backend->getAddressBooksByUri($principal, $uri);
+		}, $this->dbConnection);
 	}
 
 	/**
@@ -207,26 +220,28 @@ private function parseMultiStatus($body) {
 	/**
 	 * @param IUser $user
 	 */
-	public function updateUser(IUser $user) {
+	public function updateUser(IUser $user): void {
 		$systemAddressBook = $this->getLocalSystemAddressBook();
 		$addressBookId = $systemAddressBook['id'];
 
 		$cardId = self::getCardUri($user);
 		if ($user->isEnabled()) {
-			$card = $this->backend->getCard($addressBookId, $cardId);
-			if ($card === false) {
-				$vCard = $this->converter->createCardFromUser($user);
-				if ($vCard !== null) {
-					$this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false);
-				}
-			} else {
-				$vCard = $this->converter->createCardFromUser($user);
-				if (is_null($vCard)) {
-					$this->backend->deleteCard($addressBookId, $cardId);
+			$this->atomic(function() use ($addressBookId, $cardId, $user) {
+				$card = $this->backend->getCard($addressBookId, $cardId);
+				if ($card === false) {
+					$vCard = $this->converter->createCardFromUser($user);
+					if ($vCard !== null) {
+						$this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false);
+					}
 				} else {
-					$this->backend->updateCard($addressBookId, $cardId, $vCard->serialize());
+					$vCard = $this->converter->createCardFromUser($user);
+					if (is_null($vCard)) {
+						$this->backend->deleteCard($addressBookId, $cardId);
+					} else {
+						$this->backend->updateCard($addressBookId, $cardId, $vCard->serialize());
+					}
 				}
-			}
+			}, $this->dbConnection);
 		} else {
 			$this->backend->deleteCard($addressBookId, $cardId);
 		}

diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
@@ -30,6 +30,7 @@
 use OCA\DAV\CardDAV\CardDavBackend;
 use OCA\DAV\CardDAV\Converter;
 use OCA\DAV\CardDAV\SyncService;
+use OCP\IDBConnection;
 use OCP\IUser;
 use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
@@ -84,10 +85,11 @@ public function testEnsureSystemAddressBookExists(): void {
 
 		/** @var IUserManager $userManager */
 		$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
+		$dbConnection = $this->createMock(IDBConnection::class);
 		$logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock();
 		$converter = $this->createMock(Converter::class);
 
-		$ss = new SyncService($backend, $userManager, $logger, $converter);
+		$ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter);
 		$ss->ensureSystemAddressBookExists('principals/users/adam', 'contacts', []);
 	}
 
@@ -126,6 +128,7 @@ public function testUpdateAndDeleteUser($activated, $createCalls, $updateCalls,
 
 		/** @var IUserManager | \PHPUnit\Framework\MockObject\MockObject $userManager */
 		$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
+		$dbConnection = $this->createMock(IDBConnection::class);
 
 		/** @var IUser | \PHPUnit\Framework\MockObject\MockObject $user */
 		$user = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock();
@@ -139,7 +142,7 @@ public function testUpdateAndDeleteUser($activated, $createCalls, $updateCalls,
 			->method('createCardFromUser')
 			->willReturn($this->createMock(VCard::class));
 
-		$ss = new SyncService($backend, $userManager, $logger, $converter);
+		$ss = new SyncService($backend, $userManager, $dbConnection, $logger, $converter);
 		$ss->updateUser($user);
 
 		$ss->updateUser($user);
@@ -151,7 +154,7 @@ public function testUpdateAndDeleteUser($activated, $createCalls, $updateCalls,
 	 * @param int $createCount
 	 * @param int $updateCount
 	 * @param int $deleteCount
-	 * @return \PHPUnit\Framework\MockObject\MockObject
+	 * @return \PHPUnit\Framework\MockObject\MockObject|CardDavBackend
 	 */
 	private function getBackendMock($createCount, $updateCount, $deleteCount) {
 		$backend = $this->getMockBuilder(CardDavBackend::class)
@@ -170,12 +173,13 @@ private function getBackendMock($createCount, $updateCount, $deleteCount) {
 	 */
 	private function getSyncServiceMock($backend, $response) {
 		$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
+		$dbConnection = $this->createMock(IDBConnection::class);
 		$logger = $this->getMockBuilder(LoggerInterface::class)->disableOriginalConstructor()->getMock();
 		$converter = $this->createMock(Converter::class);
 		/** @var SyncService | \PHPUnit\Framework\MockObject\MockObject $ss */
 		$ss = $this->getMockBuilder(SyncService::class)
 			->setMethods(['ensureSystemAddressBookExists', 'requestSyncReport', 'download', 'getCertPath'])
-			->setConstructorArgs([$backend, $userManager, $logger, $converter])
+			->setConstructorArgs([$backend, $userManager, $dbConnection, $logger, $converter])
 			->getMock();
 		$ss->method('requestSyncReport')->withAnyParameters()->willReturn(['response' => $response, 'token' => 'sync-token-1']);
 		$ss->method('ensureSystemAddressBookExists')->willReturn(['id' => 1]);