From 259370224360fa8d6ba306d142f2e0352f97e4f7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 9 Apr 2025 10:22:40 +0200 Subject: [PATCH 1/2] fix(federation): Don't load the addressbook when resolving a cloud ID Instead we delay the lookup of the display name until it is actually used Signed-off-by: Joas Schilling --- lib/private/Federation/CloudId.php | 42 +++++++++------------ lib/private/Federation/CloudIdManager.php | 16 ++++++-- tests/lib/Federation/CloudIdManagerTest.php | 15 +++++--- tests/lib/Federation/CloudIdTest.php | 38 ++++++++++++++----- 4 files changed, 69 insertions(+), 42 deletions(-) diff --git a/lib/private/Federation/CloudId.php b/lib/private/Federation/CloudId.php index c20dbfc6418ab..b807c29d812e7 100644 --- a/lib/private/Federation/CloudId.php +++ b/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; } /** diff --git a/lib/private/Federation/CloudIdManager.php b/lib/private/Federation/CloudIdManager.php index 3528d06a16755..25ce42aadcfb5 100644 --- a/lib/private/Federation/CloudIdManager.php +++ b/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,18 @@ 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) { + return $cachedName; + } + $addressBookEntries = $this->contactsManager->search($cloudId, ['CLOUD'], [ 'limit' => 1, 'enumeration' => false, @@ -128,14 +135,17 @@ protected function getDisplayNameFromContact(string $cloudId): ?string { // Warning, if user decides to make his 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 { + $this->displayNameCache->set($cloudId, $cloudID, 15 * 60); return $cloudID; } } } } } + $this->displayNameCache->set($cloudId, $cloudId, 15 * 60); return null; } @@ -168,7 +178,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 diff --git a/tests/lib/Federation/CloudIdManagerTest.php b/tests/lib/Federation/CloudIdManagerTest.php index 13e6b1118648a..14daef2714227 100644 --- a/tests/lib/Federation/CloudIdManagerTest.php +++ b/tests/lib/Federation/CloudIdManagerTest.php @@ -1,4 +1,7 @@ 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,7 @@ protected function setUp(): void { $this->cacheFactory, $this->createMock(IEventDispatcher::class) ); + $this->overwriteService(ICloudIdManager::class, $this->cloudIdManager); } public function cloudIdProvider(): array { @@ -70,7 +78,7 @@ public function testResolveCloudId(string $cloudId, string $user, string $noProt ->willReturn([ [ 'CLOUD' => [$cleanId], - 'FN' => 'Ample Ex', + 'FN' => $displayName, ] ]); @@ -92,9 +100,6 @@ public function invalidCloudIdProvider(): array { /** * @dataProvider invalidCloudIdProvider - * - * @param string $cloudId - * */ public function testInvalidCloudId(string $cloudId): void { $this->expectException(\InvalidArgumentException::class); diff --git a/tests/lib/Federation/CloudIdTest.php b/tests/lib/Federation/CloudIdTest.php index 4ac9bd92d64ab..ca949d163c732 100644 --- a/tests/lib/Federation/CloudIdTest.php +++ b/tests/lib/Federation/CloudIdTest.php @@ -1,4 +1,7 @@ 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) { - $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()); } } From 32d30f8f674f0216382d1ee2b551842b7575d542 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 10 Apr 2025 09:20:27 +0200 Subject: [PATCH 2/2] fix(federation): Fix returning "no display name" after cache result Signed-off-by: Joas Schilling --- lib/private/Federation/CloudIdManager.php | 7 ++++-- tests/lib/Federation/CloudIdManagerTest.php | 28 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/private/Federation/CloudIdManager.php b/lib/private/Federation/CloudIdManager.php index 25ce42aadcfb5..884bbbaa9c6af 100644 --- a/lib/private/Federation/CloudIdManager.php +++ b/lib/private/Federation/CloudIdManager.php @@ -119,6 +119,9 @@ public function resolveCloudId(string $cloudId): ICloudId { public function getDisplayNameFromContact(string $cloudId): ?string { $cachedName = $this->displayNameCache->get($cloudId); if ($cachedName !== null) { + if ($cachedName === $cloudId) { + return null; + } return $cachedName; } @@ -138,8 +141,8 @@ public function getDisplayNameFromContact(string $cloudId): ?string { $this->displayNameCache->set($cloudId, $entry['FN'], 15 * 60); return $entry['FN']; } else { - $this->displayNameCache->set($cloudId, $cloudID, 15 * 60); - return $cloudID; + $this->displayNameCache->set($cloudId, $cloudId, 15 * 60); + return null; } } } diff --git a/tests/lib/Federation/CloudIdManagerTest.php b/tests/lib/Federation/CloudIdManagerTest.php index 14daef2714227..1775c18a5e912 100644 --- a/tests/lib/Federation/CloudIdManagerTest.php +++ b/tests/lib/Federation/CloudIdManagerTest.php @@ -56,6 +56,34 @@ protected function setUp(): void { $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 { return [ ['test@example.com', 'test', 'example.com', 'test@example.com'],