diff options
17 files changed, 515 insertions, 78 deletions
diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php index 18b54770828..8246a8eebfa 100644 --- a/apps/dav/lib/CardDAV/SystemAddressbook.php +++ b/apps/dav/lib/CardDAV/SystemAddressbook.php @@ -40,7 +40,9 @@ class SystemAddressbook extends AddressBook { } public function getChildren() { - if ($this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') !== 'yes') { + $shareEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; + $restrictShareEnumeration = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'yes') === 'yes'; + if (!$shareEnumeration || ($shareEnumeration && $restrictShareEnumeration)) { return []; } diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php index 449275e982f..41b85f162c8 100644 --- a/apps/dav/lib/Connector/Sabre/Principal.php +++ b/apps/dav/lib/Connector/Sabre/Principal.php @@ -245,7 +245,8 @@ class Principal implements BackendInterface { return []; } - $allowEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; + $allowEnumeration = $this->shareManager->allowEnumeration(); + $limitEnumeration = $this->shareManager->limitEnumerationToGroups(); // If sharing is restricted to group members only, // return only members that have groups in common @@ -259,6 +260,14 @@ class Principal implements BackendInterface { $restrictGroups = $this->groupManager->getUserGroupIds($user); } + $currentUserGroups = []; + if ($limitEnumeration) { + $currentUser = $this->userSession->getUser(); + if ($currentUser) { + $currentUserGroups = $this->groupManager->getUserGroupIds($currentUser); + } + } + foreach ($searchProperties as $prop => $value) { switch ($prop) { case '{http://sabredav.org/ns}email-address': @@ -270,6 +279,15 @@ class Principal implements BackendInterface { }); } + if ($limitEnumeration) { + $users = \array_filter($users, function (IUser $user) use ($currentUserGroups, $value) { + return !empty(array_intersect( + $this->groupManager->getUserGroupIds($user), + $currentUserGroups + )) || $user->getEMailAddress() === $value; + }); + } + $results[] = array_reduce($users, function(array $carry, IUser $user) use ($restrictGroups) { // is sharing restricted to groups only? if ($restrictGroups !== false) { @@ -293,6 +311,15 @@ class Principal implements BackendInterface { }); } + if ($limitEnumeration) { + $users = \array_filter($users, function (IUser $user) use ($currentUserGroups, $value) { + return !empty(array_intersect( + $this->groupManager->getUserGroupIds($user), + $currentUserGroups + )) || $user->getDisplayName() === $value; + }); + } + $results[] = array_reduce($users, function(array $carry, IUser $user) use ($restrictGroups) { // is sharing restricted to groups only? if ($restrictGroups !== false) { diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php index 5198b031859..99d8ea72bb0 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php @@ -431,10 +431,9 @@ class PrincipalTest extends TestCase { ->will($this->returnValue($sharingEnabled)); if ($sharingEnabled) { - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') - ->willReturn('yes'); + $this->shareManager->expects($this->once()) + ->method('allowEnumeration') + ->willReturn(true); $this->shareManager->expects($this->once()) ->method('shareWithGroupMembersOnly') @@ -526,10 +525,9 @@ class PrincipalTest extends TestCase { ->method('shareAPIEnabled') ->will($this->returnValue(true)); - $this->config->expects($this->exactly(2)) - ->method('getAppValue') - ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') - ->willReturn('yes'); + $this->shareManager->expects($this->exactly(2)) + ->method('allowEnumeration') + ->willReturn(true); $this->shareManager->expects($this->exactly(2)) ->method('shareWithGroupMembersOnly') @@ -557,10 +555,9 @@ class PrincipalTest extends TestCase { ->method('shareAPIEnabled') ->will($this->returnValue(true)); - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') - ->willReturn('no'); + $this->shareManager->expects($this->once()) + ->method('allowEnumeration') + ->willReturn(false); $this->shareManager->expects($this->once()) ->method('shareWithGroupMembersOnly') @@ -593,10 +590,9 @@ class PrincipalTest extends TestCase { ->method('shareAPIEnabled') ->will($this->returnValue(true)); - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') - ->willReturn('no'); + $this->shareManager->expects($this->once()) + ->method('allowEnumeration') + ->willReturn(false); $this->shareManager->expects($this->once()) ->method('shareWithGroupMembersOnly') @@ -624,6 +620,128 @@ class PrincipalTest extends TestCase { ['{http://sabredav.org/ns}email-address' => 'user2@foo.bar'])); } + public function testSearchPrincipalWithEnumerationLimitedDisplayname() { + $this->shareManager->expects($this->at(0)) + ->method('shareAPIEnabled') + ->will($this->returnValue(true)); + + $this->shareManager->expects($this->at(1)) + ->method('allowEnumeration') + ->willReturn(true); + + $this->shareManager->expects($this->at(2)) + ->method('limitEnumerationToGroups') + ->willReturn(true); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(false)); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user2->method('getDisplayName')->will($this->returnValue('User 2')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + $user3->method('getDisplayName')->will($this->returnValue('User 22')); + $user3->method('getEMailAddress')->will($this->returnValue('user2@foo.bar123')); + $user4 = $this->createMock(IUser::class); + $user4->method('getUID')->will($this->returnValue('user4')); + $user4->method('getDisplayName')->will($this->returnValue('User 222')); + $user4->method('getEMailAddress')->will($this->returnValue('user2@foo.bar456')); + + + $this->userSession->expects($this->at(0)) + ->method('getUser') + ->willReturn($user2); + + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(2)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(3)) + ->method('getUserGroupIds') + ->willReturn(['group2']); + + $this->userManager->expects($this->at(0)) + ->method('searchDisplayName') + ->with('User') + ->willReturn([$user2, $user3, $user4]); + + + $this->assertEquals([ + 'principals/users/user2', + 'principals/users/user3', + ], $this->connector->searchPrincipals('principals/users', + ['{DAV:}displayname' => 'User'])); + } + + public function testSearchPrincipalWithEnumerationLimitedMail() { + $this->shareManager->expects($this->at(0)) + ->method('shareAPIEnabled') + ->will($this->returnValue(true)); + + $this->shareManager->expects($this->at(1)) + ->method('allowEnumeration') + ->willReturn(true); + + $this->shareManager->expects($this->at(2)) + ->method('limitEnumerationToGroups') + ->willReturn(true); + + $this->shareManager->expects($this->once()) + ->method('shareWithGroupMembersOnly') + ->will($this->returnValue(false)); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->will($this->returnValue('user2')); + $user2->method('getDisplayName')->will($this->returnValue('User 2')); + $user2->method('getEMailAddress')->will($this->returnValue('user2@foo.bar')); + $user3 = $this->createMock(IUser::class); + $user3->method('getUID')->will($this->returnValue('user3')); + $user3->method('getDisplayName')->will($this->returnValue('User 22')); + $user3->method('getEMailAddress')->will($this->returnValue('user2@foo.bar123')); + $user4 = $this->createMock(IUser::class); + $user4->method('getUID')->will($this->returnValue('user4')); + $user4->method('getDisplayName')->will($this->returnValue('User 222')); + $user4->method('getEMailAddress')->will($this->returnValue('user2@foo.bar456')); + + + $this->userSession->expects($this->at(0)) + ->method('getUser') + ->willReturn($user2); + + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(2)) + ->method('getUserGroupIds') + ->willReturn(['group1']); + $this->groupManager->expects($this->at(3)) + ->method('getUserGroupIds') + ->willReturn(['group2']); + + $this->userManager->expects($this->at(0)) + ->method('getByEmail') + ->with('user') + ->willReturn([$user2, $user3, $user4]); + + + $this->assertEquals([ + 'principals/users/user2', + 'principals/users/user3' + ], $this->connector->searchPrincipals('principals/users', + ['{http://sabredav.org/ns}email-address' => 'user'])); + } + public function testFindByUriSharingApiDisabled() { $this->shareManager->expects($this->once()) ->method('shareApiEnabled') diff --git a/apps/files_sharing/lib/Controller/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 61678e67c17..14ed9041149 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -66,12 +66,6 @@ class ShareesAPIController extends OCSController { /** @var IManager */ protected $shareManager; - /** @var bool */ - protected $shareWithGroupOnly = false; - - /** @var bool */ - protected $shareeEnumeration = true; - /** @var int */ protected $offset = 0; @@ -205,8 +199,6 @@ class ShareesAPIController extends OCSController { } sort($shareTypes); - $this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; - $this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; $this->limit = (int) $perPage; $this->offset = $perPage * ($page - 1); diff --git a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index 1fb14ad9b8f..68c827dd922 100644 --- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php @@ -237,12 +237,10 @@ class ShareesAPIControllerTest extends TestCase { /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject $config */ $config = $this->createMock(IConfig::class); - $config->expects($this->exactly(3)) + $config->expects($this->exactly(1)) ->method('getAppValue') ->with($this->anything(), $this->anything(), $this->anything()) ->willReturnMap([ - ['core', 'shareapi_only_share_with_group_members', 'no', $apiSetting], - ['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', $enumSetting], ['files_sharing', 'lookupServerEnabled', 'yes', 'yes'], ]); @@ -302,9 +300,6 @@ class ShareesAPIControllerTest extends TestCase { })); $this->assertInstanceOf(Http\DataResponse::class, $sharees->search($search, $itemType, $page, $perPage, $shareType)); - - $this->assertSame($shareWithGroupOnly, $this->invokePrivate($sharees, 'shareWithGroupOnly')); - $this->assertSame($shareeEnumeration, $this->invokePrivate($sharees, 'shareeEnumeration')); } public function dataSearchInvalid() { diff --git a/apps/settings/js/admin.js b/apps/settings/js/admin.js index e798cd8d198..dfe9b8cabad 100644 --- a/apps/settings/js/admin.js +++ b/apps/settings/js/admin.js @@ -142,6 +142,10 @@ $(document).ready(function(){ savePublicShareDisclaimerText(this.value); }); + $('#shareapi_allow_share_dialog_user_enumeration').on('change', function() { + $('#shareapi_restrict_user_enumeration_to_group_setting').toggleClass('hidden', !this.checked); + }) + $('#allowLinks').change(function() { $("#publicLinkSettings").toggleClass('hidden', !this.checked); $('#setDefaultExpireDate').toggleClass('hidden', !(this.checked && $('#shareapiDefaultExpireDate')[0].checked)); diff --git a/apps/settings/lib/Settings/Admin/Sharing.php b/apps/settings/lib/Settings/Admin/Sharing.php index 495af9d5375..980e579d360 100644 --- a/apps/settings/lib/Settings/Admin/Sharing.php +++ b/apps/settings/lib/Settings/Admin/Sharing.php @@ -73,6 +73,7 @@ class Sharing implements ISettings { 'allowPublicUpload' => $this->config->getAppValue('core', 'shareapi_allow_public_upload', 'yes'), 'allowResharing' => $this->config->getAppValue('core', 'shareapi_allow_resharing', 'yes'), 'allowShareDialogUserEnumeration' => $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes'), + 'restrictUserEnumerationToGroup' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no'), 'enforceLinkPassword' => Util::isPublicLinkPasswordRequired(), 'onlyShareWithGroupMembers' => $this->shareManager->shareWithGroupMembersOnly(), 'shareAPIEnabled' => $this->config->getAppValue('core', 'shareapi_enabled', 'yes'), diff --git a/apps/settings/templates/settings/admin/sharing.php b/apps/settings/templates/settings/admin/sharing.php index 2bca48ce4a9..c7f3ff16b70 100644 --- a/apps/settings/templates/settings/admin/sharing.php +++ b/apps/settings/templates/settings/admin/sharing.php @@ -109,11 +109,19 @@ <br /> <em><?php p($l->t('These groups will still be able to receive shares, but not to initiate them.')); ?></em> </p> + <p class="<?php if ($_['shareAPIEnabled'] === 'no') p('hidden');?>"> <input type="checkbox" name="shareapi_allow_share_dialog_user_enumeration" value="1" id="shareapi_allow_share_dialog_user_enumeration" class="checkbox" <?php if ($_['allowShareDialogUserEnumeration'] === 'yes') print_unescaped('checked="checked"'); ?> /> <label for="shareapi_allow_share_dialog_user_enumeration"><?php p($l->t('Allow username autocompletion in share dialog. If this is disabled the full username or email address needs to be entered.'));?></label><br /> </p> + + <p id="shareapi_restrict_user_enumeration_to_group_setting" class="indent <?php if ($_['shareAPIEnabled'] === 'no' || $_['allowShareDialogUserEnumeration'] === 'no') p('hidden');?>"> + <input type="checkbox" name="shareapi_restrict_user_enumeration_to_group" value="1" id="shareapi_restrict_user_enumeration_to_group" class="checkbox" + <?php if ($_['restrictUserEnumerationToGroup'] === 'yes') print_unescaped('checked="checked"'); ?> /> + <label for="shareapi_restrict_user_enumeration_to_group"><?php p($l->t('Restrict username autocompletion to users within the same groups'));?></label><br /> + </p> + <p> <input type="checkbox" id="publicShareDisclaimer" class="checkbox noJSAutoUpdate" <?php if ($_['publicShareDisclaimerText'] !== null) print_unescaped('checked="checked"'); ?> /> diff --git a/apps/settings/tests/Settings/Admin/SharingTest.php b/apps/settings/tests/Settings/Admin/SharingTest.php index 5006f90cbd1..3afa91e0761 100644 --- a/apps/settings/tests/Settings/Admin/SharingTest.php +++ b/apps/settings/tests/Settings/Admin/SharingTest.php @@ -94,55 +94,60 @@ class SharingTest extends TestCase { $this->config ->expects($this->at(6)) ->method('getAppValue') + ->with('core', 'shareapi_restrict_user_enumeration_to_group', 'no') + ->willReturn('no'); + $this->config + ->expects($this->at(7)) + ->method('getAppValue') ->with('core', 'shareapi_enabled', 'yes') ->willReturn('yes'); $this->config - ->expects($this->at(7)) + ->expects($this->at(8)) ->method('getAppValue') ->with('core', 'shareapi_default_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(8)) + ->expects($this->at(9)) ->method('getAppValue') ->with('core', 'shareapi_expire_after_n_days', '7') ->willReturn('7'); $this->config - ->expects($this->at(9)) + ->expects($this->at(10)) ->method('getAppValue') ->with('core', 'shareapi_enforce_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(10)) + ->expects($this->at(11)) ->method('getAppValue') ->with('core', 'shareapi_exclude_groups', 'no') ->willReturn('no'); $this->config - ->expects($this->at(11)) + ->expects($this->at(12)) ->method('getAppValue') ->with('core', 'shareapi_public_link_disclaimertext', null) ->willReturn('Lorem ipsum'); $this->config - ->expects($this->at(12)) + ->expects($this->at(13)) ->method('getAppValue') ->with('core', 'shareapi_enable_link_password_by_default', 'no') ->willReturn('yes'); $this->config - ->expects($this->at(13)) + ->expects($this->at(14)) ->method('getAppValue') ->with('core', 'shareapi_default_permissions', Constants::PERMISSION_ALL) ->willReturn(Constants::PERMISSION_ALL); $this->config - ->expects($this->at(14)) + ->expects($this->at(15)) ->method('getAppValue') ->with('core', 'shareapi_default_internal_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(15)) + ->expects($this->at(16)) ->method('getAppValue') ->with('core', 'shareapi_internal_expire_after_n_days', '7') ->willReturn('7'); $this->config - ->expects($this->at(16)) + ->expects($this->at(17)) ->method('getAppValue') ->with('core', 'shareapi_enforce_internal_expire_date', 'no') ->willReturn('no'); @@ -156,6 +161,7 @@ class SharingTest extends TestCase { 'allowPublicUpload' => 'yes', 'allowResharing' => 'yes', 'allowShareDialogUserEnumeration' => 'yes', + 'restrictUserEnumerationToGroup' => 'no', 'enforceLinkPassword' => false, 'onlyShareWithGroupMembers' => false, 'shareAPIEnabled' => 'yes', @@ -212,55 +218,60 @@ class SharingTest extends TestCase { $this->config ->expects($this->at(6)) ->method('getAppValue') + ->with('core', 'shareapi_restrict_user_enumeration_to_group', 'no') + ->willReturn('no'); + $this->config + ->expects($this->at(7)) + ->method('getAppValue') ->with('core', 'shareapi_enabled', 'yes') ->willReturn('yes'); $this->config - ->expects($this->at(7)) + ->expects($this->at(8)) ->method('getAppValue') ->with('core', 'shareapi_default_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(8)) + ->expects($this->at(9)) ->method('getAppValue') ->with('core', 'shareapi_expire_after_n_days', '7') ->willReturn('7'); $this->config - ->expects($this->at(9)) + ->expects($this->at(10)) ->method('getAppValue') ->with('core', 'shareapi_enforce_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(10)) + ->expects($this->at(11)) ->method('getAppValue') ->with('core', 'shareapi_exclude_groups', 'no') ->willReturn('yes'); $this->config - ->expects($this->at(11)) + ->expects($this->at(12)) ->method('getAppValue') ->with('core', 'shareapi_public_link_disclaimertext', null) ->willReturn('Lorem ipsum'); $this->config - ->expects($this->at(12)) + ->expects($this->at(13)) ->method('getAppValue') ->with('core', 'shareapi_enable_link_password_by_default', 'no') ->willReturn('yes'); $this->config - ->expects($this->at(13)) + ->expects($this->at(14)) ->method('getAppValue') ->with('core', 'shareapi_default_permissions', Constants::PERMISSION_ALL) ->willReturn(Constants::PERMISSION_ALL); $this->config - ->expects($this->at(14)) + ->expects($this->at(15)) ->method('getAppValue') ->with('core', 'shareapi_default_internal_expire_date', 'no') ->willReturn('no'); $this->config - ->expects($this->at(15)) + ->expects($this->at(16)) ->method('getAppValue') ->with('core', 'shareapi_internal_expire_after_n_days', '7') ->willReturn('7'); $this->config - ->expects($this->at(16)) + ->expects($this->at(17)) ->method('getAppValue') ->with('core', 'shareapi_enforce_internal_expire_date', 'no') ->willReturn('no'); @@ -275,6 +286,7 @@ class SharingTest extends TestCase { 'allowPublicUpload' => 'yes', 'allowResharing' => 'yes', 'allowShareDialogUserEnumeration' => 'yes', + 'restrictUserEnumerationToGroup' => 'no', 'enforceLinkPassword' => false, 'onlyShareWithGroupMembers' => false, 'shareAPIEnabled' => 'yes', diff --git a/lib/private/Collaboration/Collaborators/GroupPlugin.php b/lib/private/Collaboration/Collaborators/GroupPlugin.php index 874c9693981..694dd161310 100644 --- a/lib/private/Collaboration/Collaborators/GroupPlugin.php +++ b/lib/private/Collaboration/Collaborators/GroupPlugin.php @@ -52,6 +52,7 @@ class GroupPlugin implements ISearchPlugin { $this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; $this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; } public function search($search, $limit, $offset, ISearchResult $searchResult) { @@ -66,7 +67,7 @@ class GroupPlugin implements ISearchPlugin { } $userGroups = []; - if (!empty($groups) && $this->shareWithGroupOnly) { + if (!empty($groups) && ($this->shareWithGroupOnly || $this->shareeEnumerationInGroupOnly)) { // Intersect all the groups that match with the groups this user is a member of $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); $userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups); @@ -93,6 +94,9 @@ class GroupPlugin implements ISearchPlugin { ], ]; } else { + if ($this->shareeEnumerationInGroupOnly && !in_array($group->getGID(), $userGroups, true)) { + continue; + } $result['wide'][] = [ 'label' => $group->getDisplayName(), 'value' => [ diff --git a/lib/private/Collaboration/Collaborators/MailPlugin.php b/lib/private/Collaboration/Collaborators/MailPlugin.php index f4af4737c1a..3a3759b5794 100644 --- a/lib/private/Collaboration/Collaborators/MailPlugin.php +++ b/lib/private/Collaboration/Collaborators/MailPlugin.php @@ -65,6 +65,8 @@ class MailPlugin implements ISearchPlugin { $this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; $this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; + } /** @@ -150,7 +152,18 @@ class MailPlugin implements ISearchPlugin { continue; } - if (!$this->isCurrentUser($cloud) && !$searchResult->hasResult($userType, $cloud->getUser())) { + $addToWide = !$this->shareeEnumerationInGroupOnly; + if ($this->shareeEnumerationInGroupOnly) { + $addToWide = false; + $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); + foreach ($userGroups as $userGroup) { + if ($this->groupManager->isInGroup($contact['UID'], $userGroup)) { + $addToWide = true; + break; + } + } + } + if ($addToWide && !$this->isCurrentUser($cloud) && !$searchResult->hasResult($userType, $cloud->getUser())) { $userResults['wide'][] = [ 'label' => $displayName, 'uuid' => $contact['UID'], @@ -160,6 +173,7 @@ class MailPlugin implements ISearchPlugin { 'shareWith' => $cloud->getUser(), ], ]; + continue; } } continue; diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index c40aaff4229..cb9d5984016 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -36,11 +36,13 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; use OCP\Share; +use OCP\Share\IShare; class UserPlugin implements ISearchPlugin { /* @var bool */ protected $shareWithGroupOnly; protected $shareeEnumeration; + protected $shareeEnumerationInGroupOnly; /** @var IConfig */ private $config; @@ -60,11 +62,13 @@ class UserPlugin implements ISearchPlugin { $this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; $this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; + $this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; } public function search($search, $limit, $offset, ISearchResult $searchResult) { $result = ['wide' => [], 'exact' => []]; $users = []; + $autoCompleteUsers = []; $hasMoreResults = false; $userGroups = []; @@ -80,10 +84,32 @@ class UserPlugin implements ISearchPlugin { } 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[(string) $user->getUID()] = $user->getDisplayName(); + + $addToWideResults = false; + if ($this->shareeEnumeration && !$this->shareeEnumerationInGroupOnly) { + $addToWideResults = true; + } + + if ($this->shareeEnumerationInGroupOnly) { + $commonGroups = array_intersect($currentUserGroups, $this->groupManager->getUserGroupIds($user)); + if (!empty($commonGroups)) { + $addToWideResults = true; + } + } + + if ($addToWideResults) { + $autoCompleteUsers[] = [ + 'label' => $user->getDisplayName(), + 'value' => [ + 'shareType' => IShare::TYPE_USER, + 'shareWith' => (string)$user->getUID(), + ], + ]; + } } } } @@ -145,8 +171,9 @@ class UserPlugin implements ISearchPlugin { } } - if (!$this->shareeEnumeration) { - $result['wide'] = []; + // overwrite wide matches if they are limited + if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly) { + $result['wide'] = $autoCompleteUsers; } $type = new SearchResultType('users'); diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index b6e401cbe71..9efa42b9a4d 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -107,12 +107,13 @@ class ContactsStore implements IContactsStore { array $entries, $filter) { $disallowEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') !== 'yes'; + $restrictEnumeration = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'yes') === 'yes'; $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes'; // whether to filter out local users $skipLocal = false; // whether to filter out all users which doesn't have the same group as the current user - $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' || $restrictEnumeration; $selfGroups = $this->groupManager->getUserGroupIds($self); diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 82e948344c1..9023e14fd2c 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1764,6 +1764,15 @@ class Manager implements IManager { return $this->config->getAppValue('core', 'shareapi_allow_group_sharing', 'yes') === 'yes'; } + public function allowEnumeration(): bool { + return $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; + } + + public function limitEnumerationToGroups(): bool { + return $this->allowEnumeration() && + $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; + } + /** * Copied from \OC_Util::isSharingDisabledForUser * diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index b6354be3978..820835365d9 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -369,6 +369,22 @@ interface IManager { public function allowGroupSharing(); /** + * Check if user enumeration is allowed + * + * @return bool + * @since 19.0.0 + */ + public function allowEnumeration(): bool; + + /** + * Check if user enumeration is limited to the users groups + * + * @return bool + * @since 19.0.0 + */ + public function limitEnumerationToGroups(): bool; + + /** * Check if sharing is disabled for the given user * * @param string $userId diff --git a/tests/lib/Collaboration/Collaborators/UserPluginTest.php b/tests/lib/Collaboration/Collaborators/UserPluginTest.php index 3aeeaa3eecb..ff916d63b38 100644 --- a/tests/lib/Collaboration/Collaborators/UserPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/UserPluginTest.php @@ -89,7 +89,27 @@ class UserPluginTest extends TestCase { ); } - public function getUserMock($uid, $displayName, $enabled = true) { + public function mockConfig($shareWithGroupOnly, $shareeEnumeration, $shareeEnumerationLimitToGroup) { + $this->config->expects($this->any()) + ->method('getAppValue') + ->willReturnCallback( + 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') { + return $shareeEnumeration ? 'yes' : 'no'; + } else if ($appName === 'core' && $key === 'shareapi_restrict_user_enumeration_to_group') { + return $shareeEnumerationLimitToGroup ? 'yes' : 'no'; + } + return $default; + } + ); + + } + + public function getUserMock($uid, $displayName, $enabled = true, $groups = []) { $user = $this->createMock(IUser::class); $user->expects($this->any()) @@ -383,21 +403,7 @@ class UserPluginTest extends TestCase { $reachedEnd, $singleUser ) { - $this->config->expects($this->any()) - ->method('getAppValue') - ->willReturnCallback( - function($appName, $key, $default) - use ($shareWithGroupOnly, $shareeEnumeration) - { - 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') { - return $shareeEnumeration ? 'yes' : 'no'; - } - return $default; - } - ); - + $this->mockConfig($shareWithGroupOnly, $shareeEnumeration, false); $this->instantiatePlugin(); $this->session->expects($this->any()) @@ -493,4 +499,110 @@ class UserPluginTest extends TestCase { $this->plugin->takeOutCurrentUser($users); $this->assertSame($expectedUIDs, array_keys($users)); } + + public function dataSearchEnumeration() { + return [ + [ + 'test', + ['groupA'], + [ + [ 'uid' => 'test1', 'groups' => ['groupA'] ], + [ 'uid' => 'test2', 'groups' => ['groupB'] ] + ], + ['test1'] + ], + [ + 'test', + ['groupA'], + [ + [ 'uid' => 'test1', 'groups' => ['groupA'] ], + [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ], + ['test1', 'test2'] + ], + [ + 'test', + ['groupA'], + [ + [ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ], + [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ], + ['test1', 'test2'] + ], + [ + 'test', + ['groupC', 'groupB'], + [ + [ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ], + [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ], + ['test1', 'test2'] + ], + [ + 'test', + [], + [ + [ 'uid' => 'test1', 'groups' => ['groupA'] ], + [ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ] + ], + [] + ], + [ + 'test', + ['groupC', 'groupB'], + [ + [ 'uid' => 'test1', 'groups' => [] ], + [ 'uid' => 'test2', 'groups' => [] ] + ], + [] + ], + ]; + } + + /** + * @dataProvider dataSearchEnumeration + */ + public function testSearchEnumerationLimit($search, $userGroups, $matchingUsers, $result) { + $this->mockConfig(false, true, true); + + $userResults = array_map(function ($user) { + return $this->getUserMock($user['uid'], $user['uid']); + }, $matchingUsers); + + $mappedResult = array_map(function ($user) { + return ['label' => $user, 'value' => [ 'shareType' => 0, 'shareWith' => $user ]]; + }, $result); + + $this->userManager->expects($this->once()) + ->method('searchDisplayName') + ->willReturn($userResults); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($this->getUserMock('test', 'foo')); + // current user + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->willReturn($userGroups); + $this->groupManager->expects($this->any()) + ->method('getUserGroupIds') + ->willReturnCallback(function ($user) use ($matchingUsers) { + $neededObject = array_filter( + $matchingUsers, + function ($e) use ($user) { + return $user->getUID() === $e['uid']; + } + ); + if (count($neededObject) > 0) { + return array_shift($neededObject)['groups']; + } + return []; + }); + + $this->instantiatePlugin(); + $this->plugin->search($search, $this->limit, $this->offset, $this->searchResult); + $result = $this->searchResult->asArray(); + + $this->assertEquals([], $result['exact']['users']); + $this->assertEquals($mappedResult, $result['users']); + } } diff --git a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php index 49dc1d9d823..31b0261cb80 100644 --- a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php +++ b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php @@ -178,15 +178,20 @@ class ContactsStoreTest extends TestCase { $this->config->expects($this->at(1)) ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_restrict_user_enumeration_to_group'), $this->equalTo('yes')) + ->willReturn('no'); + + $this->config->expects($this->at(2)) + ->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) ->willReturn('yes'); - $this->config->expects($this->at(2)) + $this->config->expects($this->at(3)) ->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $this->equalTo('no')) ->willReturn('yes'); - $this->config->expects($this->at(3)) + $this->config->expects($this->at(4)) ->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups_list'), $this->equalTo('')) ->willReturn('["group1", "group5", "group6"]'); @@ -223,16 +228,20 @@ class ContactsStoreTest extends TestCase { $this->assertCount(0, $entries); } - public function testGetContactsOnlyIfInTheSameGroup() { + public function testGetContactsOnlyShareIfInTheSameGroup() { $this->config->expects($this->at(0))->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_allow_share_dialog_user_enumeration'), $this->equalTo('yes')) ->willReturn('yes'); $this->config->expects($this->at(1)) ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_restrict_user_enumeration_to_group'), $this->equalTo('yes')) + ->willReturn('no'); + + $this->config->expects($this->at(2)) ->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) ->willReturn('no'); - $this->config->expects($this->at(2)) + $this->config->expects($this->at(3)) ->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $this->equalTo('no')) ->willReturn('yes'); @@ -305,6 +314,92 @@ class ContactsStoreTest extends TestCase { $this->assertEquals('contact', $entries[2]->getProperty('UID')); } + public function testGetContactsOnlyEnumerateIfInTheSameGroup() { + $this->config->expects($this->at(0))->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_allow_share_dialog_user_enumeration'), $this->equalTo('yes')) + ->willReturn('yes'); + + $this->config->expects($this->at(1)) ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_restrict_user_enumeration_to_group'), $this->equalTo('yes')) + ->willReturn('yes'); + + $this->config->expects($this->at(2)) ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) + ->willReturn('no'); + + $this->config->expects($this->at(3)) + ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $this->equalTo('no')) + ->willReturn('no'); + + /** @var IUser|PHPUnit_Framework_MockObject_MockObject $currentUser */ + $currentUser = $this->createMock(IUser::class); + $currentUser->expects($this->once()) + ->method('getUID') + ->willReturn('user001'); + + $this->groupManager->expects($this->at(0)) + ->method('getUserGroupIds') + ->with($this->equalTo($currentUser)) + ->willReturn(['group1', 'group2', 'group3']); + + $user1 = $this->createMock(IUser::class); + $this->userManager->expects($this->at(0)) + ->method('get') + ->with('user1') + ->willReturn($user1); + $this->groupManager->expects($this->at(1)) + ->method('getUserGroupIds') + ->with($this->equalTo($user1)) + ->willReturn(['group1']); + $user2 = $this->createMock(IUser::class); + $this->userManager->expects($this->at(1)) + ->method('get') + ->with('user2') + ->willReturn($user2); + $this->groupManager->expects($this->at(2)) + ->method('getUserGroupIds') + ->with($this->equalTo($user2)) + ->willReturn(['group2', 'group3']); + $user3 = $this->createMock(IUser::class); + $this->userManager->expects($this->at(2)) + ->method('get') + ->with('user3') + ->willReturn($user3); + $this->groupManager->expects($this->at(3)) + ->method('getUserGroupIds') + ->with($this->equalTo($user3)) + ->willReturn(['group8', 'group9']); + + $this->contactsManager->expects($this->once()) + ->method('search') + ->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL'])) + ->willReturn([ + [ + 'UID' => 'user1', + 'isLocalSystemBook' => true + ], + [ + 'UID' => 'user2', + 'isLocalSystemBook' => true + ], + [ + 'UID' => 'user3', + 'isLocalSystemBook' => true + ], + [ + 'UID' => 'contact', + ], + ]); + + $entries = $this->contactsStore->getContacts($currentUser, ''); + + $this->assertCount(3, $entries); + $this->assertEquals('user1', $entries[0]->getProperty('UID')); + $this->assertEquals('user2', $entries[1]->getProperty('UID')); + $this->assertEquals('contact', $entries[2]->getProperty('UID')); + } + public function testGetContactsWithFilter() { $this->config->expects($this->at(0))->method('getAppValue') ->with($this->equalTo('core'), $this->equalTo('shareapi_allow_share_dialog_user_enumeration'), $this->equalTo('yes')) |