[stable31] fix(federation): Don't load the addressbook when resolving a cloud ID

lib/private/Federation/CloudId.php

@@ -9,29 +9,15 @@
 namespace OC\Federation;
 
 use OCP\Federation\ICloudId;
+use OCP\Federation\ICloudIdManager;
 
 class CloudId implements ICloudId {
-	/** @var string */
-	private $id;
-	/** @var string */
-	private $user;
-	/** @var string */
-	private $remote;
-	/** @var string|null */
-	private $displayName;
-
-	/**
-	 * CloudId constructor.
-	 *
-	 * @param string $id
-	 * @param string $user
-	 * @param string $remote
-	 */
-	public function __construct(string $id, string $user, string $remote, ?string $displayName = null) {
-		$this->id = $id;
-		$this->user = $user;
-		$this->remote = $remote;
-		$this->displayName = $displayName;
+	public function __construct(
+		protected string $id,
+		protected string $user,
+		protected string $remote,
+		protected ?string $displayName = null,
+	) {
 	}
 
 	/**
@@ -44,12 +30,18 @@ public function getId(): string {
 	}
 
 	public function getDisplayId(): string {
+		if ($this->displayName === null) {
+			/** @var CloudIdManager $cloudIdManager */
+			$cloudIdManager = \OCP\Server::get(ICloudIdManager::class);
+			$this->displayName = $cloudIdManager->getDisplayNameFromContact($this->getId());
+		}
+
+		$atHost = str_replace(['http://', 'https://'], '', $this->getRemote());
+
 		if ($this->displayName) {
-			$atPos = strrpos($this->getId(), '@');
-			$atHost = substr($this->getId(), $atPos);
-			return $this->displayName . $atHost;
+			return $this->displayName . '@' . $atHost;
 		}
-		return str_replace('https://', '', str_replace('http://', '', $this->getId()));
+		return $this->getUser() . '@' . $atHost;
 	}
 
 	/**

lib/private/Federation/CloudIdManager.php

@@ -28,6 +28,7 @@ class CloudIdManager implements ICloudIdManager {
 	/** @var IUserManager */
 	private $userManager;
 	private ICache $memCache;
+	private ICache $displayNameCache;
 	/** @var array[] */
 	private array $cache = [];
 
@@ -42,6 +43,7 @@ public function __construct(
 		$this->urlGenerator = $urlGenerator;
 		$this->userManager = $userManager;
 		$this->memCache = $cacheFactory->createDistributed('cloud_id_');
+		$this->displayNameCache = $cacheFactory->createDistributed('cloudid_name_');
 		$eventDispatcher->addListener(UserChangedEvent::class, [$this, 'handleUserEvent']);
 		$eventDispatcher->addListener(CardUpdatedEvent::class, [$this, 'handleCardEvent']);
 	}
@@ -108,13 +110,21 @@ public function resolveCloudId(string $cloudId): ICloudId {
 
 			if (!empty($user) && !empty($remote)) {
 				$remote = $this->ensureDefaultProtocol($remote);
-				return new CloudId($id, $user, $remote, $this->getDisplayNameFromContact($id));
+				return new CloudId($id, $user, $remote, null);
 			}
 		}
 		throw new \InvalidArgumentException('Invalid cloud id');
 	}
 
-	protected function getDisplayNameFromContact(string $cloudId): ?string {
+	public function getDisplayNameFromContact(string $cloudId): ?string {
+		$cachedName = $this->displayNameCache->get($cloudId);
+		if ($cachedName !== null) {
+			if ($cachedName === $cloudId) {
+				return null;
+			}
+			return $cachedName;
+		}
+
 		$addressBookEntries = $this->contactsManager->search($cloudId, ['CLOUD'], [
 			'limit' => 1,
 			'enumeration' => false,
@@ -128,14 +138,17 @@ protected function getDisplayNameFromContact(string $cloudId): ?string {
 						// Warning, if user decides to make their full name local only,
 						// no FN is found on federated servers
 						if (isset($entry['FN'])) {
+							$this->displayNameCache->set($cloudId, $entry['FN'], 15 * 60);
 							return $entry['FN'];
 						} else {
-							return $cloudID;
+							$this->displayNameCache->set($cloudId, $cloudId, 15 * 60);
+							return null;
 						}
 					}
 				}
 			}
 		}
+		$this->displayNameCache->set($cloudId, $cloudId, 15 * 60);
 		return null;
 	}
 
@@ -168,7 +181,7 @@ public function getCloudId(string $user, ?string $remote): ICloudId {
 			$localUser = $this->userManager->get($user);
 			$displayName = $localUser ? $localUser->getDisplayName() : '';
 		} else {
-			$displayName = $this->getDisplayNameFromContact($user . '@' . $host);
+			$displayName = null;
 		}
 
 		// For the visible cloudID we only strip away https

tests/lib/Federation/CloudIdManagerTest.php

@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -10,11 +13,15 @@
 use OC\Memcache\ArrayCache;
 use OCP\Contacts\IManager;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Federation\ICloudIdManager;
 use OCP\ICacheFactory;
 use OCP\IURLGenerator;
 use OCP\IUserManager;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class CloudIdManagerTest extends TestCase {
 	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
 	protected $contactsManager;
@@ -36,7 +43,7 @@ protected function setUp(): void {
 		$this->userManager = $this->createMock(IUserManager::class);
 
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
-		$this->cacheFactory->method('createLocal')
+		$this->cacheFactory->method('createDistributed')
 			->willReturn(new ArrayCache(''));
 
 		$this->cloudIdManager = new CloudIdManager(
@@ -46,6 +53,35 @@ protected function setUp(): void {
 			$this->cacheFactory,
 			$this->createMock(IEventDispatcher::class)
 		);
+		$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
+	}
+
+	public function dataGetDisplayNameFromContact(): array {
+		return [
+			['test1@example.tld', 'test', 'test'],
+			['test2@example.tld', null, null],
+			['test3@example.tld', 'test3@example', 'test3@example'],
+			['test4@example.tld', 'test4@example.tld', null],
+		];
+	}
+
+	/**
+	 * @dataProvider dataGetDisplayNameFromContact
+	 */
+	public function testGetDisplayNameFromContact(string $cloudId, ?string $displayName, ?string $expected): void {
+		$returnedContact = [
+			'CLOUD' => [$cloudId],
+			'FN' => $expected,
+		];
+		if ($displayName === null) {
+			unset($returnedContact['FN']);
+		}
+		$this->contactsManager->method('search')
+			->with($cloudId, ['CLOUD'])
+			->willReturn([$returnedContact]);
+
+		$this->assertEquals($expected, $this->cloudIdManager->getDisplayNameFromContact($cloudId));
+		$this->assertEquals($expected, $this->cloudIdManager->getDisplayNameFromContact($cloudId));
 	}
 
 	public function cloudIdProvider(): array {
@@ -70,7 +106,7 @@ public function testResolveCloudId(string $cloudId, string $user, string $noProt
 			->willReturn([
 				[
 					'CLOUD' => [$cleanId],
-					'FN' => 'Ample Ex',
+					'FN' => $displayName,
 				]
 			]);
 
@@ -92,9 +128,6 @@ public function invalidCloudIdProvider(): array {
 
 	/**
 	 * @dataProvider invalidCloudIdProvider
-	 *
-	 * @param string $cloudId
-	 *
 	 */
 	public function testInvalidCloudId(string $cloudId): void {
 		$this->expectException(\InvalidArgumentException::class);

tests/lib/Federation/CloudIdTest.php

@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,25 +10,42 @@
 namespace Test\Federation;
 
 use OC\Federation\CloudId;
+use OC\Federation\CloudIdManager;
+use OCP\Federation\ICloudIdManager;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class CloudIdTest extends TestCase {
-	public function dataGetDisplayCloudId() {
+	protected CloudIdManager&MockObject $cloudIdManager;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->cloudIdManager = $this->createMock(CloudIdManager::class);
+		$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
+	}
+
+	public function dataGetDisplayCloudId(): array {
 		return [
-			['test@example.com', 'test@example.com'],
-			['test@http://example.com', 'test@example.com'],
-			['test@https://example.com', 'test@example.com'],
+			['test@example.com', 'test', 'example.com', 'test@example.com'],
+			['test@http://example.com', 'test', 'http://example.com', 'test@example.com'],
+			['test@https://example.com', 'test', 'https://example.com', 'test@example.com'],
+			['test@https://example.com', 'test', 'https://example.com', 'Beloved Amy@example.com', 'Beloved Amy'],
 		];
 	}
 
 	/**
 	 * @dataProvider dataGetDisplayCloudId
-	 *
-	 * @param string $id
-	 * @param string $display
 	 */
-	public function testGetDisplayCloudId($id, $display): void {
-		$cloudId = new CloudId($id, '', '');
+	public function testGetDisplayCloudId(string $id, string $user, string $remote, string $display, ?string $addressbookName = null): void {
+		$this->cloudIdManager->expects($this->once())
+			->method('getDisplayNameFromContact')
+			->willReturn($addressbookName);
+
+		$cloudId = new CloudId($id, $user, $remote);
 		$this->assertEquals($display, $cloudId->getDisplayId());
 	}
 }