Merge pull request #27869 from owncloud/restrict-autocomplete-groups

Add option to restrict share autocomplete to group members
owncloud · May 15, 2017 · 347264c · 347264c
2 parents 9c54ae8 + 1a49ff6
commit 347264c
Show file tree

Hide file tree

Showing 6 changed files with 452 additions and 254 deletions.
diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php
@@ -76,6 +76,9 @@ class ShareesController extends OCSController  {
 	/** @var bool */
 	protected $shareeEnumeration = true;
 
+	/** @var bool */
+	protected $shareeEnumerationGroupMembers = false;
+
 	/** @var int */
 	protected $offset = 0;
 
@@ -137,7 +140,7 @@ protected function getUsers($search) {
 		$this->result['users'] = $this->result['exact']['users'] = $users = [];
 
 		$userGroups = [];
-		if ($this->shareWithGroupOnly) {
+		if ($this->shareWithGroupOnly || $this->shareeEnumerationGroupMembers) {
 			// Search in all the groups this user is part of
 			$userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
 			foreach ($userGroups as $userGroup) {
@@ -228,7 +231,7 @@ protected function getGroups($search) {
 		}
 
 		$userGroups =  [];
-		if (!empty($groups) && $this->shareWithGroupOnly) {
+		if (!empty($groups) && ($this->shareWithGroupOnly || $this->shareeEnumerationGroupMembers)) {
 			// Intersect all the groups that match with the groups this user is a member of
 			$userGroups = $this->groupManager->getUserGroups($this->userSession->getUser(), 'sharing');
 			$userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups);
@@ -469,6 +472,11 @@ public function search($search = '', $itemType = null, $page = 1, $perPage = 200
 
 		$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';
+		if ($this->shareeEnumeration) {
+			$this->shareeEnumerationGroupMembers = $this->config->getAppValue('core', 'shareapi_share_dialog_user_enumeration_group_members', 'no') === 'yes';
+		} else {
+			$this->shareeEnumerationGroupMembers = false;
+		}
 		$this->limit = (int) $perPage;
 		$this->offset = $perPage * ($page - 1);
 

diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php
@@ -413,6 +413,55 @@ public function dataGetUsers() {
 				true,
 				false,
 			],
+			// share enumeration limited to group memberships
+			[
+				// search for user in same group
+				'ano',
+				false,
+				true,
+				// memberships
+				['group1', 'group2'],
+				// args and user response for "displayNamesInGroup" call
+				[
+					['group1', 'ano', 2, 0, [
+						'another1' => 'Another One',
+					]],
+					['group2', 'ano', 2, 0, [
+					]],
+				],
+				// exact expected
+				[],
+				// fuzzy match expected
+				[
+					['label' => 'Another One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'another1']],
+				],
+				true,
+				false,
+				true,
+			],
+			[
+				// pick user directly by name
+				'another1',
+				false,
+				true,
+				// memberships
+				['group1', 'group2'],
+				// args and user response for "displayNamesInGroup" call
+				[
+					// no such user in member groups
+					['group1', 'another1', 2, 0, []],
+					['group2', 'another1', 2, 0, []],
+				],
+				// exact expected
+				[
+					['label' => 'Another One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'another1']],
+				],
+				// fuzzy match expected
+				[],
+				true,
+				$this->getUserMock('another1', 'Another One'),
+				true,
+			],
 		];
 	}
 
@@ -422,31 +471,47 @@ public function dataGetUsers() {
 	 * @param string $searchTerm
 	 * @param bool $shareWithGroupOnly
 	 * @param bool $shareeEnumeration
-	 * @param array $groupResponse
-	 * @param array $userResponse
-	 * @param array $exactExpected
-	 * @param array $expected
+	 * @param array $groupResponse user's group memberships
+	 * @param array $userResponse user manager's search response
+	 * @param array $exactExpected exact expected result
+	 * @param array $expected non-exact expected result
 	 * @param bool $reachedEnd
-	 * @param mixed $singleUser
+	 * @param mixed $singleUser false for testing search or user mock when we are testing a direct match
+	 * @param mixed $shareeEnumerationGroupMembers restrict enumeration to group members
 	 */
-	public function testGetUsers($searchTerm, $shareWithGroupOnly, $shareeEnumeration, $groupResponse, $userResponse, $exactExpected, $expected, $reachedEnd, $singleUser) {
+	public function testGetUsers(
+		$searchTerm,
+		$shareWithGroupOnly,
+		$shareeEnumeration,
+		$groupResponse,
+		$userResponse,
+		$exactExpected,
+		$expected,
+		$reachedEnd,
+		$singleUser,
+		$shareeEnumerationGroupMembers = false
+	) {
 		$this->invokePrivate($this->sharees, 'limit', [2]);
 		$this->invokePrivate($this->sharees, 'offset', [0]);
 		$this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]);
 		$this->invokePrivate($this->sharees, 'shareeEnumeration', [$shareeEnumeration]);
+		$this->invokePrivate($this->sharees, 'shareeEnumerationGroupMembers', [$shareeEnumerationGroupMembers]);
 
 		$user = $this->getUserMock('admin', 'Administrator');
 		$this->session->expects($this->any())
 			->method('getUser')
 			->willReturn($user);
 
-		if (!$shareWithGroupOnly) {
+		if (!$shareWithGroupOnly && !$shareeEnumerationGroupMembers) {
 			$this->userManager->expects($this->once())
 				->method('searchDisplayName')
 				->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset'))
 				->willReturn($userResponse);
 		} else {
-			if ($singleUser !== false) {
+			if ($singleUser !== false && !$shareeEnumerationGroupMembers) {
+				// first call is for the current user's group memberships
+				// second call happens later for an exact match to check whether
+				// that match also is member of the same groups
 				$this->groupManager->expects($this->exactly(2))
 					->method('getUserGroupIds')
 					->withConsecutive(
@@ -773,6 +838,44 @@ public function dataGetGroups() {
 				true,
 				$this->getGroupMock('test'),
 			],
+			// group enumeration restricted to group memberships
+			[
+				// partial search
+				'test', false, true,
+				// group results
+				[
+					$this->getGroupMock('test0'),
+				],
+				// user group memberships
+				[$this->getGroupMock('test0'), $this->getGroupMock('anothergroup')],
+				// exact expected
+				[],
+				// non-exact expected
+				[
+					['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']],
+				],
+				true,
+				false,
+				true
+			],
+			[
+				// exact match
+				'test0', false, true,
+				// group results
+				[],
+				// user group memberships
+				[$this->getGroupMock('test')],
+				// exact expected
+				[
+					['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']],
+				],
+				// non-exact expected
+				[],
+				true,
+				// exact match to test for
+				$this->getGroupMock('test0'),
+				true
+			],
 		];
 	}
 
@@ -782,18 +885,30 @@ public function dataGetGroups() {
 	 * @param string $searchTerm
 	 * @param bool $shareWithGroupOnly
 	 * @param bool $shareeEnumeration
-	 * @param array $groupResponse
-	 * @param array $userGroupsResponse
+	 * @param array $groupResponse group manager search response
+	 * @param array $userGroupsResponse user's group memberships
 	 * @param array $exactExpected
 	 * @param array $expected
 	 * @param bool $reachedEnd
-	 * @param mixed $singleGroup
+	 * @param mixed $singleGroup false when testing a search or group mock when testing direct match
 	 */
-	public function testGetGroups($searchTerm, $shareWithGroupOnly, $shareeEnumeration, $groupResponse, $userGroupsResponse, $exactExpected, $expected, $reachedEnd, $singleGroup) {
+	public function testGetGroups(
+		$searchTerm,
+		$shareWithGroupOnly,
+		$shareeEnumeration,
+		$groupResponse,
+		$userGroupsResponse,
+		$exactExpected,
+		$expected,
+		$reachedEnd,
+		$singleGroup,
+		$shareeEnumerationGroupMembers = false
+	) {
 		$this->invokePrivate($this->sharees, 'limit', [2]);
 		$this->invokePrivate($this->sharees, 'offset', [0]);
 		$this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]);
 		$this->invokePrivate($this->sharees, 'shareeEnumeration', [$shareeEnumeration]);
+		$this->invokePrivate($this->sharees, 'shareeEnumerationGroupMembers', [$shareeEnumerationGroupMembers]);
 
 		$this->groupManager->expects($this->once())
 			->method('search')
@@ -807,7 +922,7 @@ public function testGetGroups($searchTerm, $shareWithGroupOnly, $shareeEnumerati
 				->willReturn($singleGroup);
 		}
 
-		if ($shareWithGroupOnly) {
+		if ($shareWithGroupOnly || $shareeEnumerationGroupMembers) {
 			$user = $this->getUserMock('admin', 'Administrator');
 			$this->session->expects($this->any())
 				->method('getUser')

diff --git a/settings/Panels/Admin/FileSharing.php b/settings/Panels/Admin/FileSharing.php
@@ -56,6 +56,7 @@ public function getPanel() {
 		$template->assign('onlyShareWithGroupMembers', $this->helper->shareWithGroupMembersOnly());
 		$template->assign('allowMailNotification', $this->config->getAppValue('core', 'shareapi_allow_mail_notification', 'no'));
 		$template->assign('allowShareDialogUserEnumeration', $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes'));
+		$template->assign('shareDialogUserEnumerationGroupMembers', $this->config->getAppValue('core', 'shareapi_share_dialog_user_enumeration_group_members', 'no'));
 		$excludeGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false;
 		$template->assign('shareExcludeGroups', $excludeGroups);
 		$excludedGroupsList = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', '');

diff --git a/settings/templates/panels/admin/filesharing.php b/settings/templates/panels/admin/filesharing.php
@@ -84,4 +84,9 @@
 			<?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 needs to be entered.'));?></label><br />
 	</p>
+	<p class="indent <?php if ($_['shareAPIEnabled'] === 'no') p('hidden');?>">
+		<input type="checkbox" name="shareapi_share_dialog_user_enumeration_group_members" value="1" id="shareapi_share_dialog_user_enumeration_group_members" class="checkbox"
+			<?php if ($_['shareDialogUserEnumerationGroupMembers'] === 'yes') print_unescaped('checked="checked"'); ?> />
+		<label for="shareapi_share_dialog_user_enumeration_group_members"><?php p($l->t('Restrict enumeration to group members'));?></label><br />
+	</p>
 </div>
diff --git a/tests/integration/features/bootstrap/ShareesContext.php b/tests/integration/features/bootstrap/ShareesContext.php
@@ -68,6 +68,7 @@ public function getArrayOfShareesResponded(ResponseInterface $response, $shareeT
 	protected function resetAppConfigs() {
 		$this->modifyServerConfig('core', 'shareapi_only_share_with_group_members', 'no');
 		$this->modifyServerConfig('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes');
+		$this->modifyServerConfig('core', 'shareapi_share_dialog_user_enumeration_group_members', 'no');
 		$this->modifyServerConfig('core', 'shareapi_allow_group_sharing', 'yes');
 	}
 }