Merge pull request #24162 from nextcloud/fix/noid/fedshares-displaynamez

set the display name of federated sharees from addressbook

apps/federatedfilesharing/tests/AddressHandlerTest.php

@@ -29,10 +29,13 @@ namespace OCA\FederatedFileSharing\Tests;
 
 use OC\Federation\CloudIdManager;
 use OCA\FederatedFileSharing\AddressHandler;
+use OCP\Contacts\IManager;
 use OCP\IL10N;
 use OCP\IURLGenerator;
 
 class AddressHandlerTest extends \Test\TestCase {
+	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
 
 	/** @var  AddressHandler */
 	private $addressHandler;
@@ -54,7 +57,9 @@ class AddressHandlerTest extends \Test\TestCase {
 		$this->il10n = $this->getMockBuilder(IL10N::class)
 			->getMock();
 
-		$this->cloudIdManager = new CloudIdManager();
+		$this->contactsManager = $this->createMock(IManager::class);
+
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 
 		$this->addressHandler = new AddressHandler($this->urlGenerator, $this->il10n, $this->cloudIdManager);
 	}
@@ -98,6 +103,10 @@ class AddressHandlerTest extends \Test\TestCase {
 	 * @param string $expectedUrl
 	 */
 	public function testSplitUserRemote($remote, $expectedUser, $expectedUrl) {
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		list($remoteUser, $remoteUrl) = $this->addressHandler->splitUserRemote($remote);
 		$this->assertSame($expectedUser, $remoteUser);
 		$this->assertSame($expectedUrl, $remoteUrl);

apps/federatedfilesharing/tests/Controller/MountPublicLinkControllerTest.php

@@ -34,6 +34,7 @@ use OCA\FederatedFileSharing\AddressHandler;
 use OCA\FederatedFileSharing\Controller\MountPublicLinkController;
 use OCA\FederatedFileSharing\FederatedShareProvider;
 use OCP\AppFramework\Http;
+use OCP\Contacts\IManager as IContactsManager;
 use OCP\Federation\ICloudIdManager;
 use OCP\Files\IRootFolder;
 use OCP\Http\Client\IClientService;
@@ -46,6 +47,8 @@ use OCP\Share\IManager;
 use OCP\Share\IShare;
 
 class MountPublicLinkControllerTest extends \Test\TestCase {
+	/** @var IContactsManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
 
 	/** @var  MountPublicLinkController */
 	private $controller;
@@ -102,7 +105,8 @@ class MountPublicLinkControllerTest extends \Test\TestCase {
 		$this->l10n = $this->getMockBuilder(IL10N::class)->disableOriginalConstructor()->getMock();
 		$this->userSession = $this->getMockBuilder(IUserSession::class)->disableOriginalConstructor()->getMock();
 		$this->clientService = $this->getMockBuilder('OCP\Http\Client\IClientService')->disableOriginalConstructor()->getMock();
-		$this->cloudIdManager = new CloudIdManager();
+		$this->contactsManager = $this->createMock(IContactsManager::class);
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 
 		$this->controller = new MountPublicLinkController(
 			'federatedfilesharing', $this->request,

apps/federatedfilesharing/tests/FederatedShareProviderTest.php

@@ -35,6 +35,7 @@ use OCA\FederatedFileSharing\AddressHandler;
 use OCA\FederatedFileSharing\FederatedShareProvider;
 use OCA\FederatedFileSharing\Notifications;
 use OCA\FederatedFileSharing\TokenHandler;
+use OCP\Contacts\IManager as IContactsManager;
 use OCP\Federation\ICloudFederationProviderManager;
 use OCP\Federation\ICloudIdManager;
 use OCP\Files\File;
@@ -46,6 +47,7 @@ use OCP\ILogger;
 use OCP\IUserManager;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use PHPUnit\Framework\MockObject\MockObject;
 
 /**
  * Class FederatedShareProviderTest
@@ -80,6 +82,8 @@ class FederatedShareProviderTest extends \Test\TestCase {
 	protected $shareManager;
 	/** @var FederatedShareProvider */
 	protected $provider;
+	/** @var IContactsManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
 
 	/** @var  ICloudIdManager */
 	private $cloudIdManager;
@@ -108,7 +112,8 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->userManager = $this->getMockBuilder(IUserManager::class)->getMock();
 		//$this->addressHandler = new AddressHandler(\OC::$server->getURLGenerator(), $this->l);
 		$this->addressHandler = $this->getMockBuilder('OCA\FederatedFileSharing\AddressHandler')->disableOriginalConstructor()->getMock();
-		$this->cloudIdManager = new CloudIdManager();
+		$this->contactsManager = $this->createMock(IContactsManager::class);
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 		$this->gsConfig = $this->createMock(\OCP\GlobalScale\IConfig::class);
 
 		$this->userManager->expects($this->any())->method('userExists')->willReturn(true);
@@ -142,6 +147,7 @@ class FederatedShareProviderTest extends \Test\TestCase {
 	public function testCreate() {
 		$share = $this->shareManager->newShare();
 
+		/** @var File|MockObject $node */
 		$node = $this->getMockBuilder(File::class)->getMock();
 		$node->method('getId')->willReturn(42);
 		$node->method('getName')->willReturn('myFile');
@@ -171,10 +177,15 @@ class FederatedShareProviderTest extends \Test\TestCase {
 				'shareOwner@http://localhost/',
 				'sharedBy',
 				'sharedBy@http://localhost/'
-			)->willReturn(true);
+			)
+			->willReturn(true);
 
 		$this->rootFolder->expects($this->never())->method($this->anything());
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->provider->create($share);
 
 		$qb = $this->connection->getQueryBuilder();
@@ -251,6 +262,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 			->with('42')
 			->willReturn([$node]);
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		try {
 			$share = $this->provider->create($share);
 			$this->fail();
@@ -308,6 +323,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 			->with('42')
 			->willReturn([$node]);
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		try {
 			$share = $this->provider->create($share);
 			$this->fail();
@@ -345,6 +364,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 			->setPermissions(19)
 			->setNode($node);
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$this->rootFolder->expects($this->never())->method($this->anything());
 
 		try {
@@ -404,6 +427,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 
 		$this->rootFolder->expects($this->never())->method($this->anything());
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$this->provider->create($share);
 
 		try {
@@ -442,7 +469,6 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$node->method('getId')->willReturn(42);
 		$node->method('getName')->willReturn('myFile');
 
-
 		$this->addressHandler->expects($this->any())->method('splitUserRemote')
 			->willReturn(['user', 'server.com']);
 
@@ -478,6 +504,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 
 		$this->rootFolder->expects($this->never())->method($this->anything());
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->provider->create($share);
 
 		$share->setPermissions(1);
@@ -516,6 +546,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 
 		$this->rootFolder->expects($this->never())->method($this->anything());
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->shareManager->newShare();
 		$share->setSharedWith('user@server.com')
 			->setSharedBy('sharedBy')
@@ -556,6 +590,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->addressHandler->method('generateRemoteURL')
 			->willReturn('remoteurl.com');
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->shareManager->newShare();
 		$share->setSharedWith('user@server.com')
 			->setSharedBy('sharedBy')
@@ -599,6 +637,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->addressHandler->method('generateRemoteURL')
 			->willReturn('remoteurl.com');
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->shareManager->newShare();
 		$share->setSharedWith('user@server.com')
 			->setSharedBy('shareOwner')
@@ -645,6 +687,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->addressHandler->method('generateRemoteURL')
 			->willReturn('remoteurl.com');
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share = $this->shareManager->newShare();
 		$share->setSharedWith('user@server.com')
 			->setSharedBy('sharedBy')
@@ -845,6 +891,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 		$this->addressHandler->method('generateRemoteURL')
 			->willReturn('remoteurl.com');
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$share1 = $this->shareManager->newShare();
 		$share1->setSharedWith('user@server.com')
 			->setSharedBy($u1->getUID())
@@ -892,6 +942,10 @@ class FederatedShareProviderTest extends \Test\TestCase {
 			->method('sendRemoteShare')
 			->willReturn(true);
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$result = $this->provider->getAccessList([$file1], true);
 		$this->assertEquals(['remote' => []], $result);
 

apps/files_sharing/tests/External/CacheTest.php

@@ -29,6 +29,7 @@ namespace OCA\Files_Sharing\Tests\External;
 
 use OC\Federation\CloudIdManager;
 use OCA\Files_Sharing\Tests\TestCase;
+use OCP\Contacts\IManager;
 use OCP\Federation\ICloudIdManager;
 
 /**
@@ -39,6 +40,8 @@ use OCP\Federation\ICloudIdManager;
  * @package OCA\Files_Sharing\Tests\External
  */
 class CacheTest extends TestCase {
+	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
 
 	/**
 	 * @var \OC\Files\Storage\Storage
@@ -61,7 +64,9 @@ class CacheTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->cloudIdManager = new CloudIdManager();
+		$this->contactsManager = $this->createMock(IManager::class);
+
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 		$this->remoteUser = $this->getUniqueID('remoteuser');
 
 		$this->storage = $this->getMockBuilder('\OCA\Files_Sharing\External\Storage')
@@ -71,6 +76,11 @@ class CacheTest extends TestCase {
 			->expects($this->any())
 			->method('getId')
 			->willReturn('dummystorage::');
+
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$this->cache = new \OCA\Files_Sharing\External\Cache(
 			$this->storage,
 			$this->cloudIdManager->getCloudId($this->remoteUser, 'http://example.com/owncloud')

apps/files_sharing/tests/External/ManagerTest.php

@@ -33,6 +33,7 @@ use OC\Files\Storage\StorageFactory;
 use OCA\Files_Sharing\External\Manager;
 use OCA\Files_Sharing\External\MountProvider;
 use OCA\Files_Sharing\Tests\TestCase;
+use OCP\Contacts\IManager;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Federation\ICloudFederationFactory;
 use OCP\Federation\ICloudFederationProviderManager;
@@ -53,6 +54,9 @@ use Test\Traits\UserTrait;
 class ManagerTest extends TestCase {
 	use UserTrait;
 
+	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
+
 	/** @var Manager|\PHPUnit\Framework\MockObject\MockObject **/
 	private $manager;
 
@@ -99,6 +103,12 @@ class ManagerTest extends TestCase {
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 
+		$this->contactsManager = $this->createMock(IManager::class);
+		// needed for MountProvider() initialization
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		$this->manager = $this->getMockBuilder(Manager::class)
 			->setConstructorArgs(
 				[
@@ -119,7 +129,7 @@ class ManagerTest extends TestCase {
 
 		$this->testMountProvider = new MountProvider(\OC::$server->getDatabaseConnection(), function () {
 			return $this->manager;
-		}, new CloudIdManager());
+		}, new CloudIdManager($this->contactsManager));
 	}
 
 	private function setupMounts() {

lib/private/Federation/CloudId.php

@@ -36,6 +36,8 @@ class CloudId implements ICloudId {
 	private $user;
 	/** @var string */
 	private $remote;
+	/** @var string|null */
+	private $displayName;
 
 	/**
 	 * CloudId constructor.
@@ -44,10 +46,11 @@ class CloudId implements ICloudId {
 	 * @param string $user
 	 * @param string $remote
 	 */
-	public function __construct(string $id, string $user, 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;
 	}
 
 	/**
@@ -60,6 +63,11 @@ class CloudId implements ICloudId {
 	}
 
 	public function getDisplayId(): string {
+		if ($this->displayName) {
+			$atPos = strrpos($this->getId(), '@');
+			$atHost = substr($this->getId(), $atPos);
+			return $this->displayName . $atHost;
+		}
 		return str_replace('https://', '', str_replace('http://', '', $this->getId()));
 	}
 

lib/private/Federation/CloudIdManager.php

@@ -29,10 +29,18 @@ declare(strict_types=1);
 
 namespace OC\Federation;
 
+use OCP\Contacts\IManager;
 use OCP\Federation\ICloudId;
 use OCP\Federation\ICloudIdManager;
 
 class CloudIdManager implements ICloudIdManager {
+	/** @var IManager */
+	private $contactsManager;
+
+	public function __construct(IManager $contactsManager) {
+		$this->contactsManager = $contactsManager;
+	}
+
 	/**
 	 * @param string $cloudId
 	 * @return ICloudId
@@ -60,23 +68,32 @@ class CloudIdManager implements ICloudIdManager {
 			$invalidPos = min($posSlash, $posColon);
 		}
 
-		// Find the last @ before $invalidPos
-		$pos = $lastAtPos = 0;
-		while ($lastAtPos !== false && $lastAtPos <= $invalidPos) {
-			$pos = $lastAtPos;
-			$lastAtPos = strpos($id, '@', $pos + 1);
-		}
+		$lastValidAtPos = strrpos($id, '@', $invalidPos - strlen($id));
 
-		if ($pos !== false) {
-			$user = substr($id, 0, $pos);
-			$remote = substr($id, $pos + 1);
+		if ($lastValidAtPos !== false) {
+			$user = substr($id, 0, $lastValidAtPos);
+			$remote = substr($id, $lastValidAtPos + 1);
 			if (!empty($user) && !empty($remote)) {
-				return new CloudId($id, $user, $remote);
+				return new CloudId($id, $user, $remote, $this->getDisplayNameFromContact($id));
 			}
 		}
 		throw new \InvalidArgumentException('Invalid cloud id');
 	}
 
+	protected function getDisplayNameFromContact(string $cloudId): ?string {
+		$addressBookEntries = $this->contactsManager->search($cloudId, ['CLOUD']);
+		foreach ($addressBookEntries as $entry) {
+			if (isset($entry['CLOUD'])) {
+				foreach ($entry['CLOUD'] as $cloudID) {
+					if ($cloudID === $cloudId) {
+						return $entry['FN'];
+					}
+				}
+			}
+		}
+		return null;
+	}
+
 	/**
 	 * @param string $user
 	 * @param string $remote
@@ -84,7 +101,16 @@ class CloudIdManager implements ICloudIdManager {
 	 */
 	public function getCloudId(string $user, string $remote): ICloudId {
 		// TODO check what the correct url is for remote (asking the remote)
-		return new CloudId($user. '@' . $remote, $user, $remote);
+		$fixedRemote = $this->fixRemoteURL($remote);
+		if (strpos($fixedRemote, 'http://') === 0) {
+			$host = substr($fixedRemote, strlen('http://'));
+		} elseif (strpos($fixedRemote, 'https://') === 0) {
+			$host = substr($fixedRemote, strlen('https://'));
+		} else {
+			$host = $fixedRemote;
+		}
+		$id = $user . '@' . $remote;
+		return new CloudId($id, $user, $fixedRemote, $this->getDisplayNameFromContact($id));
 	}
 
 	/**

lib/private/Server.php

@@ -1257,7 +1257,7 @@ class Server extends ServerContainer implements IServerContainer {
 		});
 
 		$this->registerService(ICloudIdManager::class, function (ContainerInterface $c) {
-			return new CloudIdManager();
+			return new CloudIdManager($c->get(\OCP\Contacts\IManager::class));
 		});
 
 		$this->registerAlias(\OCP\GlobalScale\IConfig::class, \OC\GlobalScale\Config::class);

tests/lib/Collaboration/Collaborators/MailPluginTest.php

@@ -65,7 +65,8 @@ class MailPluginTest extends TestCase {
 		$this->contactsManager = $this->createMock(IManager::class);
 		$this->groupManager = $this->createMock(IGroupManager::class);
 		$this->userSession = $this->createMock(IUserSession::class);
-		$this->cloudIdManager = new CloudIdManager();
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
+
 		$this->searchResult = new SearchResult();
 	}
 
@@ -104,8 +105,12 @@ class MailPluginTest extends TestCase {
 
 		$this->contactsManager->expects($this->any())
 			->method('search')
-			->with($searchTerm, ['EMAIL', 'FN'])
-			->willReturn($contacts);
+			->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
+				if ($search === $searchTerm) {
+					return $contacts;
+				}
+				return [];
+			});
 
 		$moreResults = $this->plugin->search($searchTerm, 2, 0, $this->searchResult);
 		$result = $this->searchResult->asArray();
@@ -553,8 +558,12 @@ class MailPluginTest extends TestCase {
 
 		$this->contactsManager->expects($this->any())
 			->method('search')
-			->with($searchTerm, ['EMAIL', 'FN'])
-			->willReturn($contacts);
+			->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
+				if ($search === $searchTerm) {
+					return $contacts;
+				}
+				return [];
+			});
 
 		$this->userSession->expects($this->any())
 			->method('getUser')

tests/lib/Collaboration/Collaborators/RemotePluginTest.php

@@ -62,7 +62,7 @@ class RemotePluginTest extends TestCase {
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->config = $this->createMock(IConfig::class);
 		$this->contactsManager = $this->createMock(IManager::class);
-		$this->cloudIdManager = new CloudIdManager();
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 		$this->searchResult = new SearchResult();
 	}
 
@@ -104,8 +104,12 @@ class RemotePluginTest extends TestCase {
 
 		$this->contactsManager->expects($this->any())
 			->method('search')
-			->with($searchTerm, ['CLOUD', 'FN'])
-			->willReturn($contacts);
+			->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
+				if ($search === $searchTerm) {
+					return $contacts;
+				}
+				return [];
+			});
 
 		$moreResults = $this->plugin->search($searchTerm, 2, 0, $this->searchResult);
 		$result = $this->searchResult->asArray();
@@ -125,6 +129,10 @@ class RemotePluginTest extends TestCase {
 	public function testSplitUserRemote($remote, $expectedUser, $expectedUrl) {
 		$this->instantiatePlugin();
 
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([]);
+
 		list($remoteUser, $remoteUrl) = $this->plugin->splitUserRemote($remote);
 		$this->assertSame($expectedUser, $remoteUser);
 		$this->assertSame($expectedUrl, $remoteUrl);

tests/lib/Federation/CloudIdManagerTest.php

@@ -22,15 +22,21 @@
 namespace Test\Federation;
 
 use OC\Federation\CloudIdManager;
+use OCP\Contacts\IManager;
 use Test\TestCase;
 
 class CloudIdManagerTest extends TestCase {
+	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
+	protected $contactsManager;
 	/** @var CloudIdManager */
 	private $cloudIdManager;
 
 	protected function setUp(): void {
 		parent::setUp();
-		$this->cloudIdManager = new CloudIdManager();
+
+		$this->contactsManager = $this->createMock(IManager::class);
+
+		$this->cloudIdManager = new CloudIdManager($this->contactsManager);
 	}
 
 	public function cloudIdProvider() {
@@ -51,11 +57,24 @@ class CloudIdManagerTest extends TestCase {
 	 * @param string $remote
 	 */
 	public function testResolveCloudId($cloudId, $user, $remote, $cleanId) {
+		$displayName = 'Ample Ex';
+
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->with($cleanId, ['CLOUD'])
+			->willReturn([
+				[
+					'CLOUD' => [$cleanId],
+					'FN' => 'Ample Ex',
+				]
+			]);
+
 		$cloudId = $this->cloudIdManager->resolveCloudId($cloudId);
 
 		$this->assertEquals($user, $cloudId->getUser());
 		$this->assertEquals($remote, $cloudId->getRemote());
 		$this->assertEquals($cleanId, $cloudId->getId());
+		$this->assertEquals($displayName . '@' . $remote, $cloudId->getDisplayId());
 	}
 
 	public function invalidCloudIdProvider() {
@@ -75,6 +94,9 @@ class CloudIdManagerTest extends TestCase {
 	public function testInvalidCloudId($cloudId) {
 		$this->expectException(\InvalidArgumentException::class);
 
+		$this->contactsManager->expects($this->never())
+			->method('search');
+
 		$this->cloudIdManager->resolveCloudId($cloudId);
 	}
 
@@ -93,6 +115,16 @@ class CloudIdManagerTest extends TestCase {
 	 * @param string $id
 	 */
 	public function testGetCloudId($user, $remote, $id) {
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->with($id, ['CLOUD'])
+			->willReturn([
+				[
+					'CLOUD' => [$id],
+					'FN' => 'Ample Ex',
+				]
+			]);
+
 		$cloudId = $this->cloudIdManager->getCloudId($user, $remote);
 
 		$this->assertEquals($id, $cloudId->getId());