diff --git a/apps/dav/lib/CardDAV/SystemAddressbook.php b/apps/dav/lib/CardDAV/SystemAddressbook.php
index 18b5477082..8246a8eebf 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 449275e982..41b85f162c 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 5198b03185..99d8ea72bb 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 61678e67c1..14ed904114 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 1fb14ad9b8..68c827dd92 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 e798cd8d19..dfe9b8caba 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 495af9d537..980e579d36 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 2bca48ce4a..c7f3ff16b7 100644
--- a/apps/settings/templates/settings/admin/sharing.php
+++ b/apps/settings/templates/settings/admin/sharing.php
@@ -109,11 +109,19 @@
t('These groups will still be able to receive shares, but not to initiate them.')); ?>
/>
/> diff --git a/apps/settings/tests/Settings/Admin/SharingTest.php b/apps/settings/tests/Settings/Admin/SharingTest.php index 5006f90cbd..3afa91e076 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 874c969398..694dd16131 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 f4af4737c1..3a3759b579 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 c40aaff422..cb9d598401 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 b6e401cbe7..9efa42b9a4 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 82e948344c..9023e14fd2 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 b6354be397..820835365d 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -368,6 +368,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 * diff --git a/tests/lib/Collaboration/Collaborators/UserPluginTest.php b/tests/lib/Collaboration/Collaborators/UserPluginTest.php index 3aeeaa3eec..ff916d63b3 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 49dc1d9d82..31b0261cb8 100644 --- a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php +++ b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php @@ -177,16 +177,21 @@ class ContactsStoreTest extends TestCase { ->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('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'))