From 7a36a4b99c632f72b1e9ec267bc3d5208ca76d9f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Jun 2020 11:59:56 +0200 Subject: [PATCH 1/4] Move back to IGroupManager::displayNamesInGroup() The problem is that despite it's name IGroup::searchDisplayName() only searches by userid and this is less fixable than changing back to this method here Signed-off-by: Joas Schilling --- .../Collaboration/Collaborators/UserPlugin.php | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index 7c02e3c79a..4e293dd28b 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -73,20 +73,19 @@ class UserPlugin implements ISearchPlugin { $users = []; $hasMoreResults = false; - $userGroups = []; + $currentUserGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); if ($this->shareWithGroupOnly) { // Search in all the groups this user is part of - $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); - foreach ($userGroups as $userGroup) { - $usersInGroup = $userGroup->searchDisplayName($search, $limit, $offset); - foreach ($usersInGroup as $user) { - $users[$user->getUID()] = $user; + foreach ($currentUserGroups as $userGroupId) { + $usersInGroup = $this->groupManager->displayNamesInGroup($userGroupId, $search, $limit, $offset); + foreach ($usersInGroup as $userId => $displayName) { + $userId = (string) $userId; + $users[$userId] = $this->userManager->get($userId); } } } else { // Search in all users $usersTmp = $this->userManager->searchDisplayName($search, $limit, $offset); - $currentUserGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($usersTmp as $user) { if ($user->isEnabled()) { // Don't keep deactivated users $users[$user->getUID()] = $user; @@ -155,9 +154,6 @@ class UserPlugin implements ISearchPlugin { if ($this->shareWithGroupOnly) { // Only add, if we have a common group - $userGroupIds = array_map(function (IGroup $group) { - return $group->getGID(); - }, $userGroups); $commonGroups = array_intersect($userGroupIds, $this->groupManager->getUserGroupIds($user)); $addUser = !empty($commonGroups); } From 83b0d37093af208415f0d6a219e06eb1493dacf7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Jun 2020 12:06:50 +0200 Subject: [PATCH 2/4] Add another test for exact matching of a user in group restriction Signed-off-by: Joas Schilling --- .../sharees_features/sharees.feature | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/build/integration/sharees_features/sharees.feature b/build/integration/sharees_features/sharees.feature index 5a6291d1e2..1d770b96b7 100644 --- a/build/integration/sharees_features/sharees.feature +++ b/build/integration/sharees_features/sharees.feature @@ -72,6 +72,23 @@ Feature: sharees And "exact remotes" sharees returned is empty And "remotes" sharees returned is empty + Scenario: Search only with group members - allowed with exact match + Given As an "test" + And parameter "shareapi_only_share_with_group_members" of app "core" is set to "yes" + And user "Sharee1" belongs to group "ShareeGroup" + When getting sharees for + | search | Sharee1 | + | itemType | file | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And "exact users" sharees returned are + | Sharee1 | 0 | Sharee1 | + And "users" sharees returned is empty + And "exact groups" sharees returned is empty + And "groups" sharees returned is empty + And "exact remotes" sharees returned is empty + And "remotes" sharees returned is empty + Scenario: Search only with group members - no group as non-member Given As an "Sharee1" And parameter "shareapi_only_share_with_group_members" of app "core" is set to "yes" From 9ce288163cb12398f1871cfd67b57c7ac5f95e58 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Jun 2020 12:08:17 +0200 Subject: [PATCH 3/4] Fix PHP CS Signed-off-by: Joas Schilling --- lib/private/Collaboration/Collaborators/UserPlugin.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index 4e293dd28b..946baf687c 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -33,7 +33,6 @@ 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; From d1df66f7af7c46e12ccb78a60ec0e85ea0678ede Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 18 Jun 2020 11:33:43 +0200 Subject: [PATCH 4/4] Fix unit tests Signed-off-by: Joas Schilling --- .../Collaborators/UserPlugin.php | 2 +- .../Collaborators/UserPluginTest.php | 84 +++++++++++-------- 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index 946baf687c..123fa73144 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -153,7 +153,7 @@ class UserPlugin implements ISearchPlugin { if ($this->shareWithGroupOnly) { // Only add, if we have a common group - $commonGroups = array_intersect($userGroupIds, $this->groupManager->getUserGroupIds($user)); + $commonGroups = array_intersect($currentUserGroups, $this->groupManager->getUserGroupIds($user)); $addUser = !empty($commonGroups); } diff --git a/tests/lib/Collaboration/Collaborators/UserPluginTest.php b/tests/lib/Collaboration/Collaborators/UserPluginTest.php index 0256c1eb15..40813c8285 100644 --- a/tests/lib/Collaboration/Collaborators/UserPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/UserPluginTest.php @@ -275,7 +275,7 @@ class UserPluginTest extends TestCase { true, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], + ['abc', 'test', 2, 0, ['test1' => 'Test One']], ['xyz', 'test', 2, 0, []], ], [], @@ -284,6 +284,7 @@ class UserPluginTest extends TestCase { ], true, false, + [['test1', $this->getUserMock('test1', 'Test One')]], ], [ 'test', @@ -291,13 +292,14 @@ class UserPluginTest extends TestCase { false, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], + ['abc', 'test', 2, 0, ['test1' => 'Test One']], ['xyz', 'test', 2, 0, []], ], [], [], true, false, + [['test1', $this->getUserMock('test1', 'Test One')]], ], [ 'test', @@ -306,12 +308,12 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - $this->getUserMock('test1', 'Test One'), - $this->getUserMock('test2', 'Test Two'), + 'test1' => 'Test One', + 'test2' => 'Test Two', ]], ['xyz', 'test', 2, 0, [ - $this->getUserMock('test1', 'Test One'), - $this->getUserMock('test2', 'Test Two'), + 'test1' => 'Test One', + 'test2' => 'Test Two', ]], ], [], @@ -321,6 +323,10 @@ class UserPluginTest extends TestCase { ], false, false, + [ + ['test1', $this->getUserMock('test1', 'Test One')], + ['test2', $this->getUserMock('test2', 'Test Two')], + ], ], [ 'test', @@ -329,18 +335,22 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - $this->getUserMock('test1', 'Test One'), - $this->getUserMock('test2', 'Test Two'), + 'test1' => 'Test One', + 'test2' => 'Test Two', ]], ['xyz', 'test', 2, 0, [ - $this->getUserMock('test1', 'Test One'), - $this->getUserMock('test2', 'Test Two'), + 'test1' => 'Test One', + 'test2' => 'Test Two', ]], ], [], [], true, false, + [ + ['test1', $this->getUserMock('test1', 'Test One')], + ['test2', $this->getUserMock('test2', 'Test Two')], + ], ], [ 'test', @@ -349,10 +359,10 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - $this->getUserMock('test', 'Test One'), + 'test' => 'Test One', ]], ['xyz', 'test', 2, 0, [ - $this->getUserMock('test2', 'Test Two'), + 'test2' => 'Test Two', ]], ], [ @@ -363,6 +373,10 @@ class UserPluginTest extends TestCase { ], false, false, + [ + ['test', $this->getUserMock('test', 'Test One')], + ['test2', $this->getUserMock('test2', 'Test Two')], + ], ], [ 'test', @@ -371,10 +385,10 @@ class UserPluginTest extends TestCase { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - $this->getUserMock('test', 'Test One'), + 'test' => 'Test One', ]], ['xyz', 'test', 2, 0, [ - $this->getUserMock('test2', 'Test Two'), + 'test2' => 'Test Two', ]], ], [ @@ -383,6 +397,10 @@ class UserPluginTest extends TestCase { [], true, false, + [ + ['test', $this->getUserMock('test', 'Test One')], + ['test2', $this->getUserMock('test2', 'Test Two')], + ], ], ]; } @@ -399,6 +417,7 @@ class UserPluginTest extends TestCase { * @param array $expected * @param bool $reachedEnd * @param bool|IUser $singleUser + * @param array $users */ public function testSearch( $searchTerm, @@ -409,7 +428,8 @@ class UserPluginTest extends TestCase { array $exactExpected, array $expected, $reachedEnd, - $singleUser + $singleUser, + array $users = [] ) { $this->mockConfig($shareWithGroupOnly, $shareeEnumeration, false); $this->instantiatePlugin(); @@ -424,39 +444,29 @@ class UserPluginTest extends TestCase { ->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->method('getUserGroups') - ->with($this->user) - ->willReturn($groups); + $this->groupManager->method('getUserGroupIds') + ->with($this->user) + ->willReturn($groupResponse); + if ($singleUser !== false) { $this->groupManager->method('getUserGroupIds') ->with($singleUser) ->willReturn($groupResponse); - } else { - $this->groupManager->expects($this->once()) - ->method('getUserGroups') - ->with($this->user) - ->willReturn($groups); } - foreach ($userResponse as $groupDefinition) { - [$gid, $search, $limit, $offset, $users] = $groupDefinition; - $groups[$gid]->method('searchDisplayName') - ->with($search, $limit, $offset) - ->willReturn($users); - } + $this->groupManager->method('displayNamesInGroup') + ->willReturnMap($userResponse); } if ($singleUser !== false) { - $this->userManager->expects($this->once()) - ->method('get') - ->with($searchTerm) - ->willReturn($singleUser); + $users[] = [$searchTerm, $singleUser]; } + if (!empty($users)) { + $this->userManager->expects($this->atLeastOnce()) + ->method('get') + ->willReturnMap($users); + } $moreResults = $this->plugin->search($searchTerm, $this->limit, $this->offset, $this->searchResult); $result = $this->searchResult->asArray();