From 01c147a4a5016f0672d3ad9069df66421a6a6067 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 21 Jun 2019 15:38:19 +0200 Subject: [PATCH] dont show remote and email options if we have an exact match for local user email Signed-off-by: Robin Appelman --- .../Collaboration/Collaborators/Search.php | 7 + .../Collaborators/UserPlugin.php | 29 +++- .../Collaborators/UserPluginTest.php | 139 ++++++++++-------- 3 files changed, 105 insertions(+), 70 deletions(-) diff --git a/lib/private/Collaboration/Collaborators/Search.php b/lib/private/Collaboration/Collaborators/Search.php index 7f5c5a1a81..9b9decfecb 100644 --- a/lib/private/Collaboration/Collaborators/Search.php +++ b/lib/private/Collaboration/Collaborators/Search.php @@ -85,6 +85,13 @@ class Search implements ISearch { $searchResult->unsetResult($emailType); } + // if we have an exact local user match, there is no need to show the remote and email matches + $userType = new SearchResultType('users'); + if($searchResult->hasExactIdMatch($userType)) { + $searchResult->unsetResult($remoteType); + $searchResult->unsetResult($emailType); + } + return [$searchResult->asArray(), (bool)$hasMoreResults]; } diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index bf2c1cfeda..a344e557ac 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -32,6 +32,7 @@ use OCP\Collaboration\Collaborators\ISearchPlugin; use OCP\Collaboration\Collaborators\ISearchResult; use OCP\Collaboration\Collaborators\SearchResultType; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -75,11 +76,11 @@ class UserPlugin implements ISearchPlugin { $userGroups = []; if ($this->shareWithGroupOnly) { // Search in all the groups this user is part of - $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); + $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); foreach ($userGroups as $userGroup) { - $usersTmp = $this->groupManager->displayNamesInGroup($userGroup, $search, $limit, $offset); - foreach ($usersTmp as $uid => $userDisplayName) { - $users[(string) $uid] = $userDisplayName; + $usersInGroup = $userGroup->searchDisplayName($search, $limit, $offset); + foreach ($usersInGroup as $user) { + $users[$user->getUID()] = $user; } } } else { @@ -88,7 +89,7 @@ class UserPlugin implements ISearchPlugin { $currentUserGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($usersTmp as $user) { if ($user->isEnabled()) { // Don't keep deactivated users - $users[(string) $user->getUID()] = $user->getDisplayName(); + $users[$user->getUID()] = $user; $addToWideResults = false; if ($this->shareeEnumeration && !$this->shareeEnumerationInGroupOnly) { @@ -123,9 +124,15 @@ class UserPlugin implements ISearchPlugin { $foundUserById = false; $lowerSearch = strtolower($search); - foreach ($users as $uid => $userDisplayName) { + foreach ($users as $uid => $user) { + $userDisplayName = $user->getDisplayName(); + $userEmail = $user->getEMailAddress(); $uid = (string) $uid; - if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) { + if ( + strtolower($uid) === $lowerSearch || + strtolower($userDisplayName) === $lowerSearch || + strtolower($userEmail) === $lowerSearch + ) { if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } @@ -156,7 +163,10 @@ class UserPlugin implements ISearchPlugin { if ($this->shareWithGroupOnly) { // Only add, if we have a common group - $commonGroups = array_intersect($userGroups, $this->groupManager->getUserGroupIds($user)); + $userGroupIds = array_map(function(IGroup $group) { + return $group->getGID(); + }, $userGroups); + $commonGroups = array_intersect($userGroupIds, $this->groupManager->getUserGroupIds($user)); $addUser = !empty($commonGroups); } @@ -179,6 +189,9 @@ class UserPlugin implements ISearchPlugin { $type = new SearchResultType('users'); $searchResult->addResultSet($type, $result['wide'], $result['exact']); + if (count($result['exact'])) { + $searchResult->markExactIdMatch($type); + } return $hasMoreResults; } diff --git a/tests/lib/Collaboration/Collaborators/UserPluginTest.php b/tests/lib/Collaboration/Collaborators/UserPluginTest.php index ff916d63b3..f279ada254 100644 --- a/tests/lib/Collaboration/Collaborators/UserPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/UserPluginTest.php @@ -27,6 +27,7 @@ use OC\Collaboration\Collaborators\SearchResult; use OC\Collaboration\Collaborators\UserPlugin; use OCP\Collaboration\Collaborators\ISearchResult; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -93,9 +94,8 @@ class UserPluginTest extends TestCase { $this->config->expects($this->any()) ->method('getAppValue') ->willReturnCallback( - function($appName, $key, $default) - use ($shareWithGroupOnly, $shareeEnumeration, $shareeEnumerationLimitToGroup) - { + function ($appName, $key, $default) + use ($shareWithGroupOnly, $shareeEnumeration, $shareeEnumerationLimitToGroup) { if ($appName === 'core' && $key === 'shareapi_only_share_with_group_members') { return $shareWithGroupOnly ? 'yes' : 'no'; } else if ($appName === 'core' && $key === 'shareapi_allow_share_dialog_user_enumeration') { @@ -127,6 +127,16 @@ class UserPluginTest extends TestCase { return $user; } + public function getGroupMock($gid) { + $group = $this->createMock(IGroup::class); + + $group->expects($this->any()) + ->method('getGID') + ->willReturn($gid); + + return $group; + } + public function dataGetUsers() { return [ ['test', false, true, [], [], [], [], true, false], @@ -137,33 +147,33 @@ class UserPluginTest extends TestCase { 'test', false, true, [], [], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', false, false, [], [], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, true, [], [], - [], [], true, $this->getUserMock('test', 'Test') + [], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, false, [], [], - [], [], true, $this->getUserMock('test', 'Test') + [], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, true, ['test-group'], [['test-group', 'test', 2, 0, []]], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, false, ['test-group'], [['test-group', 'test', 2, 0, []]], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', @@ -267,7 +277,7 @@ class UserPluginTest extends TestCase { true, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -283,7 +293,7 @@ class UserPluginTest extends TestCase { false, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -298,12 +308,12 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -321,12 +331,12 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -341,10 +351,10 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -363,10 +373,10 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -410,31 +420,36 @@ class UserPluginTest extends TestCase { ->method('getUser') ->willReturn($this->user); - if(!$shareWithGroupOnly) { + if (!$shareWithGroupOnly) { $this->userManager->expects($this->once()) ->method('searchDisplayName') ->with($searchTerm, $this->limit, $this->offset) ->willReturn($userResponse); } else { + $groups = array_combine($groupResponse, array_map(function ($gid) { + return $this->getGroupMock($gid); + }, $groupResponse)); if ($singleUser !== false) { - $this->groupManager->expects($this->exactly(2)) - ->method('getUserGroupIds') - ->withConsecutive( - [$this->user], - [$singleUser] - ) + $this->groupManager->method('getUserGroups') + ->with($this->user) + ->willReturn($groups); + + $this->groupManager->method('getUserGroupIds') + ->with($singleUser) ->willReturn($groupResponse); } else { $this->groupManager->expects($this->once()) - ->method('getUserGroupIds') + ->method('getUserGroups') ->with($this->user) - ->willReturn($groupResponse); + ->willReturn($groups); } - $this->groupManager->expects($this->exactly(sizeof($groupResponse))) - ->method('displayNamesInGroup') - ->with($this->anything(), $searchTerm, $this->limit, $this->offset) - ->willReturnMap($userResponse); + foreach ($userResponse as $groupDefinition) { + [$gid, $search, $limit, $offset, $users] = $groupDefinition; + $groups[$gid]->method('searchDisplayName') + ->with($search, $limit, $offset) + ->willReturn($users); + } } if ($singleUser !== false) { @@ -457,24 +472,24 @@ class UserPluginTest extends TestCase { $inputUsers = [ 'alice' => 'Alice', 'bob' => 'Bob', - 'carol' => 'Carol' + 'carol' => 'Carol', ]; return [ [ $inputUsers, ['alice', 'carol'], - 'bob' + 'bob', ], [ $inputUsers, ['alice', 'bob', 'carol'], - 'dave' + 'dave', ], [ $inputUsers, ['alice', 'bob', 'carol'], - null - ] + null, + ], ]; } @@ -489,8 +504,8 @@ class UserPluginTest extends TestCase { $this->session->expects($this->once()) ->method('getUser') - ->willReturnCallback(function() use ($currentUserId) { - if($currentUserId !== null) { + ->willReturnCallback(function () use ($currentUserId) { + if ($currentUserId !== null) { return $this->getUserMock($currentUserId, $currentUserId); } return null; @@ -506,55 +521,55 @@ class UserPluginTest extends TestCase { 'test', ['groupA'], [ - [ 'uid' => 'test1', 'groups' => ['groupA'] ], - [ 'uid' => 'test2', 'groups' => ['groupB'] ] + ['uid' => 'test1', 'groups' => ['groupA']], + ['uid' => 'test2', 'groups' => ['groupB']], ], - ['test1'] + ['test1'], ], [ 'test', ['groupA'], [ - [ 'uid' => 'test1', 'groups' => ['groupA'] ], - [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ['uid' => 'test1', 'groups' => ['groupA']], + ['uid' => 'test2', 'groups' => ['groupB', 'groupA']], ], - ['test1', 'test2'] + ['test1', 'test2'], ], [ 'test', ['groupA'], [ - [ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ], - [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ['uid' => 'test1', 'groups' => ['groupA', 'groupC']], + ['uid' => 'test2', 'groups' => ['groupB', 'groupA']], ], - ['test1', 'test2'] + ['test1', 'test2'], ], [ 'test', ['groupC', 'groupB'], [ - [ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ], - [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ['uid' => 'test1', 'groups' => ['groupA', 'groupC']], + ['uid' => 'test2', 'groups' => ['groupB', 'groupA']], ], - ['test1', 'test2'] + ['test1', 'test2'], ], [ 'test', [], [ - [ 'uid' => 'test1', 'groups' => ['groupA'] ], - [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ['uid' => 'test1', 'groups' => ['groupA']], + ['uid' => 'test2', 'groups' => ['groupB', 'groupA']], ], - [] + [], ], [ 'test', ['groupC', 'groupB'], [ - [ 'uid' => 'test1', 'groups' => [] ], - [ 'uid' => 'test2', 'groups' => [] ] + ['uid' => 'test1', 'groups' => []], + ['uid' => 'test2', 'groups' => []], ], - [] + [], ], ]; } @@ -570,7 +585,7 @@ class UserPluginTest extends TestCase { }, $matchingUsers); $mappedResult = array_map(function ($user) { - return ['label' => $user, 'value' => [ 'shareType' => 0, 'shareWith' => $user ]]; + return ['label' => $user, 'value' => ['shareType' => 0, 'shareWith' => $user]]; }, $result); $this->userManager->expects($this->once())