Merge pull request #26922 from nextcloud/techdebt/noid/dav-no-private-class

dav: Converter & SyncService shall not use private AccountManager

4 files changed, 84 insertions, 137 deletions

diff --git a/apps/dav/lib/CardDAV/Converter.php b/apps/dav/lib/CardDAV/Converter.php
index 95ac43aba36..f9ef0bb8cde 100644
--- a/apps/dav/lib/CardDAV/Converter.php
+++ b/apps/dav/lib/CardDAV/Converter.php
@@ -27,7 +27,7 @@
 
 namespace OCA\DAV\CardDAV;
 
-use OC\Accounts\AccountManager;
+use Exception;
 use OCP\Accounts\IAccountManager;
 use OCP\IImage;
 use OCP\IUser;
@@ -36,24 +36,15 @@ use Sabre\VObject\Property\Text;
 
 class Converter {
 
-	/** @var AccountManager */
+	/** @var IAccountManager */
 	private $accountManager;
 
-	/**
-	 * Converter constructor.
-	 *
-	 * @param AccountManager $accountManager
-	 */
-	public function __construct(AccountManager $accountManager) {
+	public function __construct(IAccountManager $accountManager) {
 		$this->accountManager = $accountManager;
 	}
 
-	/**
-	 * @param IUser $user
-	 * @return VCard|null
-	 */
-	public function createCardFromUser(IUser $user) {
-		$userData = $this->accountManager->getUser($user);
+	public function createCardFromUser(IUser $user): ?VCard {
+		$userProperties = $this->accountManager->getAccount($user)->getProperties();
 
 		$uid = $user->getUID();
 		$cloudId = $user->getCloudId();
@@ -65,23 +56,19 @@ class Converter {
 
 		$publish = false;
 
-		if ($image !== null && isset($userData[IAccountManager::PROPERTY_AVATAR])) {
-			$userData[IAccountManager::PROPERTY_AVATAR]['value'] = true;
-		}
-
-		foreach ($userData as $property => $value) {
+		foreach ($userProperties as $property) {
 			$shareWithTrustedServers =
-				$value['scope'] === AccountManager::SCOPE_FEDERATED ||
-				$value['scope'] === AccountManager::SCOPE_PUBLISHED;
+				$property->getScope() === IAccountManager::SCOPE_FEDERATED ||
+				$property->getScope() === IAccountManager::SCOPE_PUBLISHED;
 
-			$emptyValue = !isset($value['value']) || $value['value'] === '';
+			$emptyValue = $property->getValue() === '';
 
 			if ($shareWithTrustedServers && !$emptyValue) {
 				$publish = true;
-				switch ($property) {
+				switch ($property->getName()) {
 					case IAccountManager::PROPERTY_DISPLAYNAME:
-						$vCard->add(new Text($vCard, 'FN', $value['value']));
-						$vCard->add(new Text($vCard, 'N', $this->splitFullName($value['value'])));
+						$vCard->add(new Text($vCard, 'FN', $property->getValue()));
+						$vCard->add(new Text($vCard, 'N', $this->splitFullName($property->getValue())));
 						break;
 					case IAccountManager::PROPERTY_AVATAR:
 						if ($image !== null) {
@@ -89,19 +76,19 @@ class Converter {
 						}
 						break;
 					case IAccountManager::PROPERTY_EMAIL:
-						$vCard->add(new Text($vCard, 'EMAIL', $value['value'], ['TYPE' => 'OTHER']));
+						$vCard->add(new Text($vCard, 'EMAIL', $property->getValue(), ['TYPE' => 'OTHER']));
 						break;
 					case IAccountManager::PROPERTY_WEBSITE:
-						$vCard->add(new Text($vCard, 'URL', $value['value']));
+						$vCard->add(new Text($vCard, 'URL', $property->getValue()));
 						break;
 					case IAccountManager::PROPERTY_PHONE:
-						$vCard->add(new Text($vCard, 'TEL', $value['value'], ['TYPE' => 'OTHER']));
+						$vCard->add(new Text($vCard, 'TEL', $property->getValue(), ['TYPE' => 'OTHER']));
 						break;
 					case IAccountManager::PROPERTY_ADDRESS:
-						$vCard->add(new Text($vCard, 'ADR', $value['value'], ['TYPE' => 'OTHER']));
+						$vCard->add(new Text($vCard, 'ADR', $property->getValue(), ['TYPE' => 'OTHER']));
 						break;
 					case IAccountManager::PROPERTY_TWITTER:
-						$vCard->add(new Text($vCard, 'X-SOCIALPROFILE', $value['value'], ['TYPE' => 'TWITTER']));
+						$vCard->add(new Text($vCard, 'X-SOCIALPROFILE', $property->getValue(), ['TYPE' => 'TWITTER']));
 						break;
 				}
 			}
@@ -116,11 +103,7 @@ class Converter {
 		return null;
 	}
 
-	/**
-	 * @param string $fullName
-	 * @return string[]
-	 */
-	public function splitFullName($fullName) {
+	public function splitFullName(string $fullName): array {
 		// Very basic western style parsing. I'm not gonna implement
 		// https://github.com/android/platform_packages_providers_contactsprovider/blob/master/src/com/android/providers/contacts/NameSplitter.java ;)
 
@@ -140,14 +123,10 @@ class Converter {
 		return $result;
 	}
 
-	/**
-	 * @param IUser $user
-	 * @return null|IImage
-	 */
-	private function getAvatarImage(IUser $user) {
+	private function getAvatarImage(IUser $user): ?IImage {
 		try {
 			return $user->getAvatarImage(-1);
-		} catch (\Exception $ex) {
+		} catch (Exception $ex) {
 			return null;
 		}
 	}
diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php
index bcb20409524..2cbdba2b119 100644
--- a/apps/dav/lib/CardDAV/SyncService.php
+++ b/apps/dav/lib/CardDAV/SyncService.php
@@ -54,8 +54,8 @@ class SyncService {
 	/** @var array */
 	private $localSystemAddressBook;
 
-	/** @var AccountManager */
-	private $accountManager;
+	/** @var Converter */
+	private $converter;
 
 	/** @var string */
 	protected $certPath;
@@ -68,11 +68,11 @@ class SyncService {
 	 * @param ILogger $logger
 	 * @param AccountManager $accountManager
 	 */
-	public function __construct(CardDavBackend $backend, IUserManager $userManager, ILogger $logger, AccountManager $accountManager) {
+	public function __construct(CardDavBackend $backend, IUserManager $userManager, ILogger $logger, Converter $converter) {
 		$this->backend = $backend;
 		$this->userManager = $userManager;
 		$this->logger = $logger;
-		$this->accountManager = $accountManager;
+		$this->converter = $converter;
 		$this->certPath = '';
 	}
 
@@ -265,7 +265,6 @@ class SyncService {
 	public function updateUser(IUser $user) {
 		$systemAddressBook = $this->getLocalSystemAddressBook();
 		$addressBookId = $systemAddressBook['id'];
-		$converter = new Converter($this->accountManager);
 		$name = $user->getBackendClassName();
 		$userId = $user->getUID();
 
@@ -273,12 +272,12 @@ class SyncService {
 		$card = $this->backend->getCard($addressBookId, $cardId);
 		if ($user->isEnabled()) {
 			if ($card === false) {
-				$vCard = $converter->createCardFromUser($user);
+				$vCard = $this->converter->createCardFromUser($user);
 				if ($vCard !== null) {
 					$this->backend->createCard($addressBookId, $cardId, $vCard->serialize());
 				}
 			} else {
-				$vCard = $converter->createCardFromUser($user);
+				$vCard = $this->converter->createCardFromUser($user);
 				if (is_null($vCard)) {
 					$this->backend->deleteCard($addressBookId, $cardId);
 				} else {
diff --git a/apps/dav/tests/unit/CardDAV/ConverterTest.php b/apps/dav/tests/unit/CardDAV/ConverterTest.php
index 43344451437..ef46845a5ae 100644
--- a/apps/dav/tests/unit/CardDAV/ConverterTest.php
+++ b/apps/dav/tests/unit/CardDAV/ConverterTest.php
@@ -27,65 +27,66 @@
 
 namespace OCA\DAV\Tests\unit\CardDAV;
 
-use OC\Accounts\AccountManager;
 use OCA\DAV\CardDAV\Converter;
+use OCP\Accounts\IAccount;
 use OCP\Accounts\IAccountManager;
+use OCP\Accounts\IAccountProperty;
 use OCP\IImage;
 use OCP\IUser;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class ConverterTest extends TestCase {
 
-	/** @var AccountManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */
 	private $accountManager;
 
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->accountManager = $this->createMock(AccountManager::class);
+		$this->accountManager = $this->createMock(IAccountManager::class);
+	}
+
+	/**
+	 * @return IAccountProperty|MockObject
+	 */
+	protected function getAccountPropertyMock(string $name, ?string $value, string $scope) {
+		$property = $this->createMock(IAccountProperty::class);
+		$property->expects($this->any())
+			->method('getName')
+			->willReturn($name);
+		$property->expects($this->any())
+			->method('getValue')
+			->willReturn((string)$value);
+		$property->expects($this->any())
+			->method('getScope')
+			->willReturn($scope);
+		$property->expects($this->any())
+			->method('getVerified')
+			->willReturn(IAccountManager::NOT_VERIFIED);
+		return $property;
 	}
 
 	public function getAccountManager(IUser $user) {
-		$accountManager = $this->getMockBuilder(AccountManager::class)
+		$account = $this->createMock(IAccount::class);
+		$account->expects($this->any())
+			->method('getProperties')
+			->willReturnCallback(function () use ($user) {
+				return [
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_DISPLAYNAME, $user->getDisplayName(), IAccountManager::SCOPE_FEDERATED),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_ADDRESS, '', IAccountManager::SCOPE_LOCAL),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_WEBSITE, '', IAccountManager::SCOPE_LOCAL),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_EMAIL, $user->getEMailAddress(), IAccountManager::SCOPE_FEDERATED),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_AVATAR, $user->getAvatarImage(-1)->data(), IAccountManager::SCOPE_FEDERATED),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_PHONE, '', IAccountManager::SCOPE_LOCAL),
+					$this->getAccountPropertyMock(IAccountManager::PROPERTY_TWITTER, '', IAccountManager::SCOPE_LOCAL),
+				];
+			});
+
+		$accountManager = $this->getMockBuilder(IAccountManager::class)
 			->disableOriginalConstructor()->getMock();
-		$accountManager->expects($this->any())->method('getUser')->willReturn(
-			[
-				IAccountManager::PROPERTY_DISPLAYNAME =>
-					[
-						'value' => $user->getDisplayName(),
-						'scope' => AccountManager::SCOPE_FEDERATED,
-					],
-				IAccountManager::PROPERTY_ADDRESS =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_WEBSITE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_EMAIL =>
-					[
-						'value' => $user->getEMailAddress(),
-						'scope' => AccountManager::SCOPE_FEDERATED,
-					],
-				IAccountManager::PROPERTY_AVATAR =>
-					[
-						'scope' => AccountManager::SCOPE_FEDERATED
-					],
-				IAccountManager::PROPERTY_PHONE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_TWITTER =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-			]
-		);
+
+		$accountManager->expects($this->any())->method('getAccount')->willReturn($account);
 
 		return $accountManager;
 	}
@@ -94,7 +95,7 @@ class ConverterTest extends TestCase {
 	 * @dataProvider providesNewUsers
 	 */
 	public function testCreation($expectedVCard, $displayName = null, $eMailAddress = null, $cloudId = null) {
-		$user = $this->getUserMock($displayName, $eMailAddress, $cloudId);
+		$user = $this->getUserMock((string)$displayName, $eMailAddress, $cloudId);
 		$accountManager = $this->getAccountManager($user);
 
 		$converter = new Converter($accountManager);
@@ -203,7 +204,7 @@ class ConverterTest extends TestCase {
 	 * @param $cloudId
 	 * @return IUser | \PHPUnit\Framework\MockObject\MockObject
 	 */
-	protected function getUserMock($displayName, $eMailAddress, $cloudId) {
+	protected function getUserMock(string $displayName, ?string $eMailAddress, ?string $cloudId) {
 		$image0 = $this->getMockBuilder(IImage::class)->disableOriginalConstructor()->getMock();
 		$image0->method('mimeType')->willReturn('image/jpeg');
 		$image0->method('data')->willReturn('123456789');
diff --git a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
index 724670bc986..fbec28ca7fd 100644
--- a/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
+++ b/apps/dav/tests/unit/CardDAV/SyncServiceTest.php
@@ -30,11 +30,13 @@ namespace OCA\DAV\Tests\unit\CardDAV;
 
 use OC\Accounts\AccountManager;
 use OCA\DAV\CardDAV\CardDavBackend;
+use OCA\DAV\CardDAV\Converter;
 use OCA\DAV\CardDAV\SyncService;
 use OCP\Accounts\IAccountManager;
 use OCP\ILogger;
 use OCP\IUser;
 use OCP\IUserManager;
+use Sabre\VObject\Component\VCard;
 use Test\TestCase;
 
 class SyncServiceTest extends TestCase {
@@ -82,8 +84,9 @@ class SyncServiceTest extends TestCase {
 		/** @var IUserManager $userManager */
 		$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
 		$logger = $this->getMockBuilder(ILogger::class)->disableOriginalConstructor()->getMock();
-		$accountManager = $this->getMockBuilder(AccountManager::class)->disableOriginalConstructor()->getMock();
-		$ss = new SyncService($backend, $userManager, $logger, $accountManager);
+		$converter = $this->createMock(Converter::class);
+
+		$ss = new SyncService($backend, $userManager, $logger, $converter);
 		$ss->ensureSystemAddressBookExists('principals/users/adam', 'contacts', []);
 	}
 
@@ -130,47 +133,12 @@ class SyncServiceTest extends TestCase {
 		$user->method('getCloudId')->willReturn('cloudId');
 		$user->method('getDisplayName')->willReturn('test-user');
 		$user->method('isEnabled')->willReturn($activated);
-		$accountManager = $this->getMockBuilder(AccountManager::class)->disableOriginalConstructor()->getMock();
-		$accountManager->expects($this->any())->method('getUser')
-			->willReturn([
-				IAccountManager::PROPERTY_DISPLAYNAME =>
-					[
-						'value' => $user->getDisplayName(),
-						'scope' => AccountManager::SCOPE_FEDERATED,
-					],
-				IAccountManager::PROPERTY_ADDRESS =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_WEBSITE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_EMAIL =>
-					[
-						'value' => $user->getEMailAddress(),
-						'scope' => AccountManager::SCOPE_FEDERATED,
-					],
-				IAccountManager::PROPERTY_AVATAR =>
-					[
-						'scope' => AccountManager::SCOPE_FEDERATED
-					],
-				IAccountManager::PROPERTY_PHONE =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-				IAccountManager::PROPERTY_TWITTER =>
-					[
-						'value' => '',
-						'scope' => AccountManager::SCOPE_LOCAL,
-					],
-			]
-			);
-
-		$ss = new SyncService($backend, $userManager, $logger, $accountManager);
+		$converter = $this->createMock(Converter::class);
+		$converter->expects($this->any())
+			->method('createCardFromUser')
+			->willReturn($this->createMock(VCard::class));
+
+		$ss = new SyncService($backend, $userManager, $logger, $converter);
 		$ss->updateUser($user);
 
 		$ss->updateUser($user);
@@ -202,11 +170,11 @@ class SyncServiceTest extends TestCase {
 	private function getSyncServiceMock($backend, $response) {
 		$userManager = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock();
 		$logger = $this->getMockBuilder(ILogger::class)->disableOriginalConstructor()->getMock();
-		$accountManager = $this->getMockBuilder(AccountManager::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, $accountManager])
+			->setConstructorArgs([$backend, $userManager, $logger, $converter])
 			->getMock();
 		$ss->method('requestSyncReport')->withAnyParameters()->willReturn(['response' => $response, 'token' => 'sync-token-1']);
 		$ss->method('ensureSystemAddressBookExists')->willReturn(['id' => 1]);