Add filter for `shareapi_allow_share_dialog_user_enumeration`

This adjusts the contacts menu to also support searching by email address which is relevant in scenarios where no UID is known such as LDAP, etc.

Furthermore, if `shareapi_allow_share_dialog_user_enumeration` is disabled only results are shown that match the full user ID or email address.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

lib/private/Contacts/ContactsMenu/ContactsStore.php

@@ -1,9 +1,10 @@
 <?php
-
 /**
  * @copyright 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ * @copyright 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @author 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ * @author 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -53,7 +54,10 @@ class ContactsStore {
 	 * @param IUserManager $userManager
 	 * @param IGroupManager $groupManager
 	 */
-	public function __construct(IManager $contactsManager, IConfig $config, IUserManager $userManager, IGroupManager $groupManager) {
+	public function __construct(IManager $contactsManager,
+								IConfig $config,
+								IUserManager $userManager,
+								IGroupManager $groupManager) {
 		$this->contactsManager = $contactsManager;
 		$this->config = $config;
 		$this->userManager = $userManager;
@@ -68,27 +72,39 @@ class ContactsStore {
 	public function getContacts(IUser $user, $filter) {
 		$allContacts = $this->contactsManager->search($filter ?: '', [
 			'FN',
+			'EMAIL'
 		]);
 
 		$entries = array_map(function(array $contact) {
 			return $this->contactArrayToEntry($contact);
 		}, $allContacts);
-		return $this->filterContacts($user, $entries);
+		return $this->filterContacts(
+			$user,
+			$entries,
+			$filter
+		);
 	}
 
 	/**
-	 * @brief filters the contacts. Applies 3 filters:
+	 * Filters the contacts. Applies 3 filters:
 	 *  1. filter the current user
-	 *  2. if the `shareapi_exclude_groups` config option is enabled and the
+	 *  2. if the `shareapi_allow_share_dialog_user_enumeration` config option is
+	 * enabled it will filter all local users
+	 *  3. if the `shareapi_exclude_groups` config option is enabled and the
 	 * current user is in an excluded group it will filter all local users.
-	 *  3. if the `shareapi_only_share_with_group_members` config option is
+	 *  4. if the `shareapi_only_share_with_group_members` config option is
 	 * enabled it will filter all users which doens't have a common group
 	 * with the current user.
+	 *
 	 * @param IUser $self
 	 * @param Entry[] $entries
+	 * @param string $filter
 	 * @return Entry[] the filtered contacts
 	 */
-	private function filterContacts(IUser $self, array $entries) {
+	private function filterContacts(IUser $self,
+									array $entries,
+									$filter) {
+		$disallowEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') !== 'yes';
 		$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes';
 
 		// whether to filter out local users
@@ -101,7 +117,7 @@ class ContactsStore {
 		if ($excludedGroups) {
 			$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', '');
 			$decodedExcludeGroups = json_decode($excludedGroups, true);
-			$excludeGroupsList = !is_null($decodedExcludeGroups) ? $decodedExcludeGroups :  [];
+			$excludeGroupsList = ($decodedExcludeGroups !== null) ? $decodedExcludeGroups :  [];
 
 			if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) {
 				// a group of the current user is excluded -> filter all local users
@@ -111,12 +127,32 @@ class ContactsStore {
 
 		$selfUID = $self->getUID();
 
-		return array_filter($entries, function(IEntry $entry) use ($self, $skipLocal, $ownGroupsOnly, $selfGroups, $selfUID) {
-
+		return array_values(array_filter($entries, function(IEntry $entry) use ($self, $skipLocal, $ownGroupsOnly, $selfGroups, $selfUID, $disallowEnumeration, $filter) {
 			if ($skipLocal && $entry->getProperty('isLocalSystemBook') === true) {
 				return false;
 			}
 
+			// Prevent enumerating local users
+			if($disallowEnumeration && $entry->getProperty('isLocalSystemBook')) {
+				$filterUser = true;
+
+				$mailAddresses = $entry->getEMailAddresses();
+				foreach($mailAddresses as $mailAddress) {
+					if($mailAddress === $filter) {
+						$filterUser = false;
+						break;
+					}
+				}
+
+				if($entry->getProperty('UID') && $entry->getProperty('UID') === $filter) {
+					$filterUser = false;
+				}
+
+				if($filterUser) {
+					return false;
+				}
+			}
+
 			if ($ownGroupsOnly && $entry->getProperty('isLocalSystemBook') === true) {
 				$contactGroups = $this->groupManager->getUserGroupIds($this->userManager->get($entry->getProperty('UID')));
 				if (count(array_intersect($contactGroups, $selfGroups)) === 0) {
@@ -126,9 +162,7 @@ class ContactsStore {
 			}
 
 			return $entry->getProperty('UID') !== $selfUID;
-		});
-
-
+		}));
 	}
 
 	/**
@@ -173,7 +207,7 @@ class ContactsStore {
 		}
 
 		if ($match) {
-			$match = $this->filterContacts($user, [$this->contactArrayToEntry($match)]);
+			$match = $this->filterContacts($user, [$this->contactArrayToEntry($match)], $shareWith);
 			if (count($match) === 1) {
 				$match = $match[0];
 			} else {

settings/templates/admin/sharing.php

@@ -96,7 +96,7 @@
 	<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 needs to be entered.'));?></label><br />
+		<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>
 		<input type="checkbox" id="publicShareDisclaimer" class="checkbox noJSAutoUpdate"

tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php

@@ -1,9 +1,10 @@
 <?php
-
 /**
  * @copyright 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ * @copyright 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @author 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ * @author 2017 Lukas Reschke <lukas@statuscode.ch>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -34,19 +35,14 @@ use PHPUnit_Framework_MockObject_MockObject;
 use Test\TestCase;
 
 class ContactsStoreTest extends TestCase {
-
 	/** @var ContactsStore */
 	private $contactsStore;
-
 	/** @var IManager|PHPUnit_Framework_MockObject_MockObject */
 	private $contactsManager;
-
 	/** @var IUserManager|PHPUnit_Framework_MockObject_MockObject */
 	private $userManager;
-
 	/** @var IGroupManager|PHPUnit_Framework_MockObject_MockObject */
 	private $groupManager;
-
 	/** @var IConfig|PHPUnit_Framework_MockObject_MockObject */
 	private $config;
 
@@ -54,21 +50,18 @@ class ContactsStoreTest extends TestCase {
 		parent::setUp();
 
 		$this->contactsManager = $this->createMock(IManager::class);
-
 		$this->userManager = $this->createMock(IUserManager::class);
-
 		$this->groupManager = $this->createMock(IGroupManager::class);
-
 		$this->config = $this->createMock(IConfig::class);
-
 		$this->contactsStore = new ContactsStore($this->contactsManager, $this->config, $this->userManager, $this->groupManager);
 	}
 
 	public function testGetContactsWithoutFilter() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 123,
@@ -94,10 +87,11 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testGetContactsHidesOwnEntry() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 'user123',
@@ -120,10 +114,11 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testGetContactsWithoutBinaryImage() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 123,
@@ -148,10 +143,11 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testGetContactsWithoutAvatarURI() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 123,
@@ -176,21 +172,26 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testGetContactsWhenUserIsInExcludeGroups() {
-		$this->config->expects($this->at(0))
+		$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_exclude_groups'), $this->equalTo('no'))
 			->willReturn('yes');
 
-		$this->config->expects($this->at(1))
+		$this->config->expects($this->at(2))
 			->method('getAppValue')
 			->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $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_exclude_groups_list'), $this->equalTo(''))
 			->willReturn('["group1", "group5", "group6"]');
 
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $currentUser */
 		$currentUser = $this->createMock(IUser::class);
 		$currentUser->expects($this->once())
 			->method('getUID')
@@ -199,12 +200,12 @@ class ContactsStoreTest extends TestCase {
 		$this->groupManager->expects($this->once())
 			->method('getUserGroupIds')
 			->with($this->equalTo($currentUser))
-			->willReturn(["group1", "group2", "group3"]);
+			->willReturn(['group1', 'group2', 'group3']);
 
 
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 'user123',
@@ -220,19 +221,23 @@ class ContactsStoreTest extends TestCase {
 		$entries = $this->contactsStore->getContacts($currentUser, '');
 
 		$this->assertCount(0, $entries);
-
 	}
 
 	public function testGetContactsOnlyIfInTheSameGroup() {
-		$this->config->expects($this->at(0)) ->method('getAppValue')
+		$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_exclude_groups'), $this->equalTo('no'))
 			->willReturn('no');
 
-		$this->config->expects($this->at(1))
+		$this->config->expects($this->at(2))
 			->method('getAppValue')
 			->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $this->equalTo('no'))
 			->willReturn('yes');
 
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $currentUser */
 		$currentUser = $this->createMock(IUser::class);
 		$currentUser->expects($this->once())
 			->method('getUID')
@@ -241,8 +246,7 @@ class ContactsStoreTest extends TestCase {
 		$this->groupManager->expects($this->at(0))
 			->method('getUserGroupIds')
 			->with($this->equalTo($currentUser))
-			->willReturn(["group1", "group2", "group3"]);
-
+			->willReturn(['group1', 'group2', 'group3']);
 
 		$user1 = $this->createMock(IUser::class);
 		$this->userManager->expects($this->at(0))
@@ -252,7 +256,7 @@ class ContactsStoreTest extends TestCase {
 		$this->groupManager->expects($this->at(1))
 			->method('getUserGroupIds')
 			->with($this->equalTo($user1))
-			->willReturn(["group1"]);
+			->willReturn(['group1']);
 		$user2 = $this->createMock(IUser::class);
 		$this->userManager->expects($this->at(1))
 			->method('get')
@@ -261,7 +265,7 @@ class ContactsStoreTest extends TestCase {
 		$this->groupManager->expects($this->at(2))
 			->method('getUserGroupIds')
 			->with($this->equalTo($user2))
-			->willReturn(["group2", "group3"]);
+			->willReturn(['group2', 'group3']);
 		$user3 = $this->createMock(IUser::class);
 		$this->userManager->expects($this->at(2))
 			->method('get')
@@ -270,11 +274,11 @@ class ContactsStoreTest extends TestCase {
 		$this->groupManager->expects($this->at(3))
 			->method('getUserGroupIds')
 			->with($this->equalTo($user3))
-			->willReturn(["group8", "group9"]);
+			->willReturn(['group8', 'group9']);
 
 		$this->contactsManager->expects($this->once())
 			->method('search')
-			->with($this->equalTo(''), $this->equalTo(['FN']))
+			->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
 			->willReturn([
 				[
 					'UID' => 'user1',
@@ -298,13 +302,99 @@ class ContactsStoreTest extends TestCase {
 		$this->assertCount(3, $entries);
 		$this->assertEquals('user1', $entries[0]->getProperty('UID'));
 		$this->assertEquals('user2', $entries[1]->getProperty('UID'));
-		$this->assertEquals('contact', $entries[3]->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'))
+			->willReturn('no');
 
-	}
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
+		$user = $this->createMock(IUser::class);
+		$this->contactsManager->expects($this->any())
+			->method('search')
+			->willReturn([
+				[
+					'UID' => 'a567',
+					'FN' => 'Darren Roner',
+					'EMAIL' => [
+						'darren@roner.au',
+					],
+					'isLocalSystemBook' => true,
+				],
+				[
+					'UID' => 'john',
+					'FN' => 'John Doe',
+					'EMAIL' => [
+						'john@example.com',
+					],
+					'isLocalSystemBook' => true,
+				],
+				[
+					'FN' => 'Anne D',
+					'EMAIL' => [
+						'anne@example.com',
+					],
+					'isLocalSystemBook' => false,
+				],
+			]);
+		$user->expects($this->any())
+			->method('getUID')
+			->willReturn('user123');
 
+		// Complete match on UID should match
+		$entry = $this->contactsStore->getContacts($user, 'a567');
+		$this->assertSame(2, count($entry));
+		$this->assertEquals([
+			'darren@roner.au'
+		], $entry[0]->getEMailAddresses());
+
+		// Partial match on UID should not match
+		$entry = $this->contactsStore->getContacts($user, 'a56');
+		$this->assertSame(1, count($entry));
+		$this->assertEquals([
+			'anne@example.com'
+		], $entry[0]->getEMailAddresses());
+
+		// Complete match on email should match
+		$entry = $this->contactsStore->getContacts($user, 'john@example.com');
+		$this->assertSame(2, count($entry));
+		$this->assertEquals([
+			'john@example.com'
+		], $entry[0]->getEMailAddresses());
+		$this->assertEquals([
+			'anne@example.com'
+		], $entry[1]->getEMailAddresses());
+
+		// Partial match on email should not match
+		$entry = $this->contactsStore->getContacts($user, 'john@example.co');
+		$this->assertSame(1, count($entry));
+		$this->assertEquals([
+			'anne@example.com'
+		], $entry[0]->getEMailAddresses());
+
+		// Match on FN should not match
+		$entry = $this->contactsStore->getContacts($user, 'Darren Roner');
+		$this->assertSame(1, count($entry));
+		$this->assertEquals([
+			'anne@example.com'
+		], $entry[0]->getEMailAddresses());
+
+		// Don't filter users in local addressbook
+		$entry = $this->contactsStore->getContacts($user, 'Anne D');
+		$this->assertSame(1, count($entry));
+		$this->assertEquals([
+			'anne@example.com'
+		], $entry[0]->getEMailAddresses());
+	}
 
 	public function testFindOneUser() {
+		$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');
+
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
@@ -323,7 +413,7 @@ class ContactsStoreTest extends TestCase {
 					'isLocalSystemBook' => true
 				],
 			]);
-		$user->expects($this->once())
+		$user->expects($this->any())
 			->method('getUID')
 			->willReturn('user123');
 
@@ -335,6 +425,7 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testFindOneEMail() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')
@@ -353,7 +444,7 @@ class ContactsStoreTest extends TestCase {
 					'isLocalSystemBook' => false
 				],
 			]);
-		$user->expects($this->once())
+		$user->expects($this->any())
 			->method('getUID')
 			->willReturn('user123');
 
@@ -365,6 +456,7 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testFindOneNotSupportedType() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 
 		$entry = $this->contactsStore->findOne($user, 42, 'darren@roner.au');
@@ -373,6 +465,7 @@ class ContactsStoreTest extends TestCase {
 	}
 
 	public function testFindOneNoMatches() {
+		/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->createMock(IUser::class);
 		$this->contactsManager->expects($this->once())
 			->method('search')