Merge pull request #26550 from nextcloud/fix/noid/prefer-cloudshare-over-mail-take2

hide mail share if a cloud share is possible – backend only solution

4 files changed, 137 insertions, 24 deletions

diff --git a/lib/private/Collaboration/Collaborators/Search.php b/lib/private/Collaboration/Collaborators/Search.php
index 79dfd5e1d82..87463572944 100644
--- a/lib/private/Collaboration/Collaborators/Search.php
+++ b/lib/private/Collaboration/Collaborators/Search.php
@@ -81,8 +81,8 @@ class Search implements ISearch {
 
 		// sanitizing, could go into the plugins as well
 
-		// if we have a exact match, either for the federated cloud id or for the
-		// email address we only return the exact match. It is highly unlikely
+		// if we have an exact match, either for the federated cloud id or for the
+		// email address, we only return the exact match. It is highly unlikely
 		// that the exact same email address and federated cloud id exists
 		$emailType = new SearchResultType('emails');
 		$remoteType = new SearchResultType('remotes');
@@ -92,6 +92,8 @@ class Search implements ISearch {
 			$searchResult->unsetResult($emailType);
 		}
 
+		$this->dropMailSharesWhereRemoteShareIsPossible($searchResult);
+
 		// if we have an exact local user match with an email-a-like query,
 		// there is no need to show the remote and email matches.
 		$userType = new SearchResultType('users');
@@ -110,4 +112,34 @@ class Search implements ISearch {
 		}
 		$this->pluginList[$shareType][] = $pluginInfo['class'];
 	}
+
+	protected function dropMailSharesWhereRemoteShareIsPossible(ISearchResult $searchResult): void {
+		$allResults = $searchResult->asArray();
+
+		$emailType = new SearchResultType('emails');
+		$remoteType = new SearchResultType('remotes');
+
+		if (!isset($allResults[$remoteType->getLabel()])
+			|| !isset($allResults[$emailType->getLabel()])) {
+			return;
+		}
+
+		$mailIdMap = [];
+		foreach ($allResults[$emailType->getLabel()] as $mailRow) {
+			// sure, array_reduce looks nicer, but foreach needs less resources and is faster
+			if (!isset($mailRow['uuid'])) {
+				continue;
+			}
+			$mailIdMap[$mailRow['uuid']] = $mailRow['value']['shareWith'];
+		}
+
+		foreach ($allResults[$remoteType->getLabel()] as $resultRow) {
+			if (!isset($resultRow['uuid'])) {
+				continue;
+			}
+			if (isset($mailIdMap[$resultRow['uuid']])) {
+				$searchResult->removeCollaboratorResult($emailType, $mailIdMap[$resultRow['uuid']]);
+			}
+		}
+	}
 }
diff --git a/lib/private/Collaboration/Collaborators/SearchResult.php b/lib/private/Collaboration/Collaborators/SearchResult.php
index 8e2c5a1ff39..e9511f869fa 100644
--- a/lib/private/Collaboration/Collaborators/SearchResult.php
+++ b/lib/private/Collaboration/Collaborators/SearchResult.php
@@ -85,4 +85,24 @@ class SearchResult implements ISearchResult {
 			$this->result['exact'][$type] = [];
 		}
 	}
+
+	public function removeCollaboratorResult(SearchResultType $type, string $collaboratorId): bool {
+		$type = $type->getLabel();
+		if (!isset($this->result[$type])) {
+			return false;
+		}
+
+		$actionDone = false;
+		$resultArrays = [&$this->result['exact'][$type], &$this->result[$type]];
+		foreach ($resultArrays as &$resultArray) {
+			foreach ($resultArray as $k => $result) {
+				if ($result['value']['shareWith'] === $collaboratorId) {
+					unset($resultArray[$k]);
+					$actionDone = true;
+				}
+			}
+		}
+
+		return $actionDone;
+	}
 }
diff --git a/lib/public/Collaboration/Collaborators/ISearchResult.php b/lib/public/Collaboration/Collaborators/ISearchResult.php
index d0d61ccfb28..a559892bb47 100644
--- a/lib/public/Collaboration/Collaborators/ISearchResult.php
+++ b/lib/public/Collaboration/Collaborators/ISearchResult.php
@@ -46,6 +46,14 @@ interface ISearchResult {
 	public function hasResult(SearchResultType $type, $collaboratorId);
 
 	/**
+	 * Removes all result where $collaborationId exactly matches shareWith of
+	 * any of wide and exact result matches of the given type.
+	 *
+	 * @since 22.0.0
+	 */
+	public function removeCollaboratorResult(SearchResultType $type, string $collaboratorId): bool;
+
+	/**
 	 * @param SearchResultType $type
 	 * @since 13.0.0
 	 */
diff --git a/tests/lib/Collaboration/Collaborators/SearchTest.php b/tests/lib/Collaboration/Collaborators/SearchTest.php
index 17f5c39aca7..1d55ee6dd22 100644
--- a/tests/lib/Collaboration/Collaborators/SearchTest.php
+++ b/tests/lib/Collaboration/Collaborators/SearchTest.php
@@ -48,27 +48,18 @@ class SearchTest extends TestCase {
 
 	/**
 	 * @dataProvider dataSearchSharees
-	 *
-	 * @param string $searchTerm
-	 * @param array $shareTypes
-	 * @param int $page
-	 * @param int $perPage
-	 * @param array $mockedUserResult
-	 * @param array $mockedGroupsResult
-	 * @param array $mockedRemotesResult
-	 * @param array $expected
-	 * @param bool $expectedMoreResults
 	 */
 	public function testSearch(
-		$searchTerm,
+		string $searchTerm,
 		array $shareTypes,
-		$page,
-		$perPage,
+		int $page,
+		int $perPage,
 		array $mockedUserResult,
 		array $mockedGroupsResult,
 		array $mockedRemotesResult,
+		array $mockedMailResult,
 		array $expected,
-		$expectedMoreResults
+		bool $expectedMoreResults
 	) {
 		$searchResult = new SearchResult();
 
@@ -104,9 +95,18 @@ class SearchTest extends TestCase {
 				return $expectedMoreResults;
 			});
 
+		$mailPlugin = $this->createMock(ISearchPlugin::class);
+		$mailPlugin->expects($this->any())
+			->method('search')
+			->willReturnCallback(function () use ($searchResult, $mockedMailResult, $expectedMoreResults) {
+				$type = new SearchResultType('emails');
+				$searchResult->addResultSet($type, $mockedMailResult);
+				return $expectedMoreResults;
+			});
+
 		$this->container->expects($this->any())
 			->method('resolve')
-			->willReturnCallback(function ($class) use ($searchResult, $userPlugin, $groupPlugin, $remotePlugin) {
+			->willReturnCallback(function ($class) use ($searchResult, $userPlugin, $groupPlugin, $remotePlugin, $mailPlugin) {
 				if ($class === SearchResult::class) {
 					return $searchResult;
 				} elseif ($class === $userPlugin) {
@@ -115,6 +115,8 @@ class SearchTest extends TestCase {
 					return $groupPlugin;
 				} elseif ($class === $remotePlugin) {
 					return $remotePlugin;
+				} elseif ($class === $mailPlugin) {
+					return $mailPlugin;
 				}
 				return null;
 			});
@@ -122,6 +124,7 @@ class SearchTest extends TestCase {
 		$this->search->registerPlugin(['shareType' => 'SHARE_TYPE_USER', 'class' => $userPlugin]);
 		$this->search->registerPlugin(['shareType' => 'SHARE_TYPE_GROUP', 'class' => $groupPlugin]);
 		$this->search->registerPlugin(['shareType' => 'SHARE_TYPE_REMOTE', 'class' => $remotePlugin]);
+		$this->search->registerPlugin(['shareType' => 'SHARE_TYPE_EMAIL', 'class' => $mailPlugin]);
 
 		[$results, $moreResults] = $this->search->search($searchTerm, $shareTypes, false, $perPage, $perPage * ($page - 1));
 
@@ -131,24 +134,31 @@ class SearchTest extends TestCase {
 
 	public function dataSearchSharees() {
 		return [
+			// #0
 			[
 				'test', [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE], 1, 2, [], [], ['results' => [], 'exact' => [], 'exactIdMatch' => false],
+				[],
 				[
 					'exact' => ['users' => [], 'groups' => [], 'remotes' => []],
 					'users' => [],
 					'groups' => [],
 					'remotes' => [],
-				], false
+				],
+				false
 			],
+			// #1
 			[
 				'test', [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE], 1, 2, [], [], ['results' => [], 'exact' => [], 'exactIdMatch' => false],
+				[],
 				[
 					'exact' => ['users' => [], 'groups' => [], 'remotes' => []],
 					'users' => [],
 					'groups' => [],
 					'remotes' => [],
-				], false
+				],
+				false
 			],
+			// #2
 			[
 				'test', [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE], 1, 2, [
 					['label' => 'test One', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
@@ -157,6 +167,7 @@ class SearchTest extends TestCase {
 				], [
 					'results' => [['label' => 'testz@remote', 'value' => ['shareType' => IShare::TYPE_REMOTE, 'shareWith' => 'testz@remote']]], 'exact' => [], 'exactIdMatch' => false,
 				],
+				[],
 				[
 					'exact' => ['users' => [], 'groups' => [], 'remotes' => []],
 					'users' => [
@@ -170,13 +181,14 @@ class SearchTest extends TestCase {
 					],
 				], true,
 			],
-			// No groups requested
+			// #3 No groups requested
 			[
 				'test', [IShare::TYPE_USER, IShare::TYPE_REMOTE], 1, 2, [
 					['label' => 'test One', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
 				], [], [
 					'results' => [['label' => 'testz@remote', 'value' => ['shareType' => IShare::TYPE_REMOTE, 'shareWith' => 'testz@remote']]], 'exact' => [], 'exactIdMatch' => false
 				],
+				[],
 				[
 					'exact' => ['users' => [], 'remotes' => []],
 					'users' => [
@@ -187,11 +199,11 @@ class SearchTest extends TestCase {
 					],
 				], false,
 			],
-			// Share type restricted to user - Only one user
+			// #4 Share type restricted to user - Only one user
 			[
 				'test', [IShare::TYPE_USER], 1, 2, [
 					['label' => 'test One', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
-				], [], [],
+				], [], [], [],
 				[
 					'exact' => ['users' => []],
 					'users' => [
@@ -199,12 +211,12 @@ class SearchTest extends TestCase {
 					],
 				], false,
 			],
-			// Share type restricted to user - Multipage result
+			// #5 Share type restricted to user - Multipage result
 			[
 				'test', [IShare::TYPE_USER], 1, 2, [
 					['label' => 'test 1', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
 					['label' => 'test 2', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test2']],
-				], [], [],
+				], [], [], [],
 				[
 					'exact' => ['users' => []],
 					'users' => [
@@ -213,6 +225,47 @@ class SearchTest extends TestCase {
 					],
 				], true,
 			],
+			// #6 Mail shares filtered out in favor of remote shares
+			[
+				'test', // search term
+				[IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_REMOTE, IShare::TYPE_EMAIL], // plugins
+				1, // page
+				10, // per page
+				[  // user result
+					['label' => 'test One', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
+				],
+				[  // group result
+					['label' => 'testgroup1', 'value' => ['shareType' => IShare::TYPE_GROUP, 'shareWith' => 'testgroup1']],
+				],
+				[  // remote result
+					'results' => [
+						['label' => 'testz@remote.tld', 'uuid' => 'f3d78140-abcc-46df-b58d-c7cc1176aadf','value' => ['shareType' => IShare::TYPE_REMOTE, 'shareWith' => 'testz@remote.tld']]
+					],
+					'exact' => [],
+					'exactIdMatch' => false,
+				],
+				[  //  mail result
+					['label' => 'test Two', 'uuid' => 'b2321e9e-31af-43ac-a406-583fb26d1964', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test2@remote.tld']],
+					['label' => 'test One', 'uuid' => 'f3d78140-abcc-46df-b58d-c7cc1176aadf', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'testz@remote.tld']],
+				],
+				[  // expected result
+					'exact' => ['users' => [], 'groups' => [], 'remotes' => [], 'emails' => []],
+					'users' => [
+						['label' => 'test One', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test1']],
+					],
+					'groups' => [
+						['label' => 'testgroup1', 'value' => ['shareType' => IShare::TYPE_GROUP, 'shareWith' => 'testgroup1']],
+					],
+					'remotes' => [
+						['label' => 'testz@remote.tld', 'uuid' => 'f3d78140-abcc-46df-b58d-c7cc1176aadf', 'value' => ['shareType' => IShare::TYPE_REMOTE, 'shareWith' => 'testz@remote.tld']],
+					],
+					'emails' => [
+						//  one passes, another is filtered out
+						['label' => 'test Two', 'uuid' => 'b2321e9e-31af-43ac-a406-583fb26d1964', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test2@remote.tld']]
+					]
+				],
+				false, // expected more results indicator
+			],
 		];
 	}
 }