Merge pull request #19569 from nextcloud/enh/noid/restrict-user-enumeration-to-groups

Restrict sharing user enumeration to groups

9 files changed, 211 insertions, 52 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',