diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 87aff258aa..3eda58cacf 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -1,9 +1,10 @@ + * @copyright 2017 Lukas Reschke * * @author 2017 Christoph Wurst + * @author 2017 Lukas Reschke * * @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 { diff --git a/settings/templates/settings/admin/sharing.php b/settings/templates/settings/admin/sharing.php index 38071a4bee..9c9e8c0780 100644 --- a/settings/templates/settings/admin/sharing.php +++ b/settings/templates/settings/admin/sharing.php @@ -96,7 +96,7 @@

/> -
+

+ * @copyright 2017 Lukas Reschke * * @author 2017 Christoph Wurst + * @author 2017 Lukas Reschke * * @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)) - ->method('getAppValue') - ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) + $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_only_share_with_group_members'), $this->equalTo('no')) + ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) ->willReturn('yes'); $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(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')