From 6f4976fd4d5c601e60268c3650ef2aa55b5793d8 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Sun, 2 Jul 2017 11:02:18 +0200 Subject: [PATCH 1/8] Privacy enhancements for contacts menu - Groups, which are excluded from sharing should not see local users at all - If sharing is restricted to users own groups, he should only see contacts from his groups: Signed-off-by: Tobia De Koninck --- .../Contacts/ContactsMenu/ContactsStore.php | 75 ++++++++++++++++++- 1 file changed, 71 insertions(+), 4 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 40a0bf870317d..6a319d7e145f3 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -24,20 +24,40 @@ namespace OC\Contacts\ContactsMenu; +use OC\Share\Share; use OCP\Contacts\ContactsMenu\IEntry; use OCP\Contacts\IManager; +use OCP\IConfig; +use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; class ContactsStore { /** @var IManager */ private $contactsManager; + /** @var IConfig */ + private $config; + + /** @var IUserManager */ + private $userManager; + + /** @var IGroupManager */ + private $groupManager; + /** * @param IManager $contactsManager + * @param IConfig $config + * @param IUserManager $userManager + * @param IGroupManager $groupManager */ - public function __construct(IManager $contactsManager) { + public function __construct(IManager $contactsManager, IConfig $config, IUserManager $userManager, IGroupManager $groupManager) { $this->contactsManager = $contactsManager; + $this->config = $config; + $this->userManager = $userManager; + $this->groupManager = $groupManager; } /** @@ -50,13 +70,60 @@ public function getContacts(IUser $user, $filter) { 'FN', ]); - $self = $user->getUID(); $entries = array_map(function(array $contact) { return $this->contactArrayToEntry($contact); }, $allContacts); - return array_filter($entries, function(IEntry $entry) use ($self) { - return $entry->getProperty('UID') !== $self; + return $this->filterContacts($user, $entries); + } + + /** + * @brief filters the contacts. Applies 3 filters: + * 1. filter the current user + * 2. 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 + * enabled it will filter all users which doens't have a common group + * with the current user. + * @param IUser $self + * @param $entries Entry[] + * @return array the filtered contacts + */ + private function filterContacts(IUser $self, Array $entries) { + $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false; + + $skipLocal = false; // whether to filter out local users + $ownGroupsOnly = Share::shareWithGroupMembersOnly(); // whether to filter out all users which doesn't have the same group as the current user + + $selfGroups = $this->groupManager->getUserGroupIds($self); + + if ($excludedGroups) { + $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); + $excludeGroupsList = !is_null(json_decode($excludedGroups)) ? json_decode($excludedGroups, true) : []; + + if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) { + // a group of the current user is excluded -> filter all local users + $skipLocal = true; + } + } + + return array_filter($entries, function(IEntry $entry) use ($self, $skipLocal, $ownGroupsOnly, $selfGroups) { + + if ($skipLocal && $entry->getProperty('isLocalSystemBook') === true) { + 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) { + // no groups in common, so shouldn't see the contact + return false; + } + } + + return $entry->getProperty('UID') !== $self->getUID(); }); + + } /** From 88ccbef5464cdd56e221885934e96453908ab586 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Sun, 2 Jul 2017 11:33:59 +0200 Subject: [PATCH 2/8] Fix tests Signed-off-by: Tobia De Koninck --- .../Contacts/ContactsMenu/ContactsStore.php | 6 ++++-- .../ContactsMenu/ContactsStoreTest.php | 20 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 6a319d7e145f3..32bcb58ccda7b 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -106,7 +106,9 @@ private function filterContacts(IUser $self, Array $entries) { } } - return array_filter($entries, function(IEntry $entry) use ($self, $skipLocal, $ownGroupsOnly, $selfGroups) { + $selfUID = $self->getUID(); + + return array_filter($entries, function(IEntry $entry) use ($self, $skipLocal, $ownGroupsOnly, $selfGroups, $selfUID) { if ($skipLocal && $entry->getProperty('isLocalSystemBook') === true) { return false; @@ -120,7 +122,7 @@ private function filterContacts(IUser $self, Array $entries) { } } - return $entry->getProperty('UID') !== $self->getUID(); + return $entry->getProperty('UID') !== $selfUID; }); diff --git a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php index 08da360388f1c..520df2ccfae25 100644 --- a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php +++ b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php @@ -26,7 +26,10 @@ use OC\Contacts\ContactsMenu\ContactsStore; use OCP\Contacts\IManager; +use OCP\IConfig; +use OCP\IGroupManager; use OCP\IUser; +use OCP\IUserManager; use PHPUnit_Framework_MockObject_MockObject; use Test\TestCase; @@ -38,12 +41,27 @@ class ContactsStoreTest extends TestCase { /** @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; + protected function setUp() { parent::setUp(); $this->contactsManager = $this->createMock(IManager::class); - $this->contactsStore = new ContactsStore($this->contactsManager); + $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() { From d2d6ed5c97c6b0a1e962d0a9cedccbfbb693bd3f Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Sun, 2 Jul 2017 14:20:44 +0200 Subject: [PATCH 3/8] Add tests Signed-off-by: Tobia De Koninck --- .../Contacts/ContactsMenu/ContactsStore.php | 2 +- .../ContactsMenu/ContactsStoreTest.php | 129 ++++++++++++++++++ 2 files changed, 130 insertions(+), 1 deletion(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 32bcb58ccda7b..52061fbb3d405 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -92,7 +92,7 @@ private function filterContacts(IUser $self, Array $entries) { $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false; $skipLocal = false; // whether to filter out local users - $ownGroupsOnly = Share::shareWithGroupMembersOnly(); // 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' ? true : false; // whether to filter out all users which doesn't have the same group as the current user $selfGroups = $this->groupManager->getUserGroupIds($self); diff --git a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php index 520df2ccfae25..360f117ed7abe 100644 --- a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php +++ b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php @@ -175,6 +175,135 @@ public function testGetContactsWithoutAvatarURI() { $this->assertEquals('https://photo', $entries[1]->getAvatar()); } + public function testGetContactsWhenUserIsInExcludeGroups() { + $this->config->expects($this->at(0)) + ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) + ->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')) + ->willReturn('yes'); + + $this->config->expects($this->at(2)) + ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups_list'), $this->equalTo('')) + ->willReturn('["group1", "group5", "group6"]'); + + $currentUser = $this->createMock(IUser::class); + $currentUser->expects($this->once()) + ->method('getUID') + ->willReturn('user001'); + + $this->groupManager->expects($this->once()) + ->method('getUserGroupIds') + ->with($this->equalTo($currentUser)) + ->willReturn(["group1", "group2", "group3"]); + + + $this->contactsManager->expects($this->once()) + ->method('search') + ->with($this->equalTo(''), $this->equalTo(['FN'])) + ->willReturn([ + [ + 'UID' => 'user123', + 'isLocalSystemBook' => true + ], + [ + 'UID' => 'user12345', + 'isLocalSystemBook' => true + ], + ]); + + + $entries = $this->contactsStore->getContacts($currentUser, ''); + + $this->assertCount(0, $entries); + + } + + public function testGetContactsOnlyIfInTheSameGroup() { + $this->config->expects($this->at(0)) ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_exclude_groups'), $this->equalTo('no')) + ->willReturn('no'); + + $this->config->expects($this->at(1)) + ->method('getAppValue') + ->with($this->equalTo('core'), $this->equalTo('shareapi_only_share_with_group_members'), $this->equalTo('no')) + ->willReturn('yes'); + + $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'])) + ->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[3]->getProperty('UID')); + + + } + + public function testFindOneUser() { $user = $this->createMock(IUser::class); $this->contactsManager->expects($this->once()) From 3409c364fed26641573edd6c6ddbe0ec8241703b Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Fri, 7 Jul 2017 23:12:28 +0200 Subject: [PATCH 4/8] Some code improvements Signed-off-by: Tobia De Koninck --- lib/private/Contacts/ContactsMenu/ContactsStore.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 52061fbb3d405..a788ad32f6df1 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -81,7 +81,7 @@ public function getContacts(IUser $user, $filter) { * 1. filter the current user * 2. 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 + * 3. 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 @@ -89,16 +89,17 @@ public function getContacts(IUser $user, $filter) { * @return array the filtered contacts */ private function filterContacts(IUser $self, Array $entries) { - $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes' ? true : false; + $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes'; $skipLocal = false; // whether to filter out local users - $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' ? true : 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'); // whether to filter out all users which doesn't have the same group as the current user $selfGroups = $this->groupManager->getUserGroupIds($self); if ($excludedGroups) { $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); - $excludeGroupsList = !is_null(json_decode($excludedGroups)) ? json_decode($excludedGroups, true) : []; + $decodedExcludeGroups = json_decode($excludedGroups, true); + $excludeGroupsList = !is_null($decodedExcludeGroups) ? $decodedExcludeGroups : []; if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) { // a group of the current user is excluded -> filter all local users From 21e903cb310fd53c9151a1c31458ed188e964518 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Sat, 5 Aug 2017 15:21:14 +0200 Subject: [PATCH 5/8] Improve code style Signed-off-by: Tobia De Koninck --- lib/private/Contacts/ContactsMenu/ContactsStore.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index a788ad32f6df1..299e88ad01b51 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -85,14 +85,16 @@ public function getContacts(IUser $user, $filter) { * enabled it will filter all users which doens't have a common group * with the current user. * @param IUser $self - * @param $entries Entry[] - * @return array the filtered contacts + * @param Entry[] $entries + * @return Entry[] the filtered contacts */ - private function filterContacts(IUser $self, Array $entries) { + private function filterContacts(IUser $self, array $entries) { $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes'; - $skipLocal = false; // whether to filter out local users - $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no'); // whether to filter out all users which doesn't have the same group as the current user + // 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'); $selfGroups = $this->groupManager->getUserGroupIds($self); From 005cd8ae42ad6134e6e49c372e14122c01d411e9 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Mon, 14 Aug 2017 11:45:54 +0200 Subject: [PATCH 6/8] Fix issue when disabling the shareapi_only_share_with_group_members option + fix findOne Signed-off-by: Tobia De Koninck --- .../Contacts/ContactsMenu/ContactsStore.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 299e88ad01b51..87aff258aaea5 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -94,7 +94,7 @@ private function filterContacts(IUser $self, array $entries) { // 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'); + $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; $selfGroups = $this->groupManager->getUserGroupIds($self); @@ -172,7 +172,17 @@ public function findOne(IUser $user, $shareType, $shareWith) { } } - return $match ? $this->contactArrayToEntry($match) : null; + if ($match) { + $match = $this->filterContacts($user, [$this->contactArrayToEntry($match)]); + if (count($match) === 1) { + $match = $match[0]; + } else { + $match = null; + } + + } + + return $match; } /** From 96e3e0788a6e520c8fa2e11b5dcf819bc81ecb86 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 15 Sep 2017 14:32:27 +0200 Subject: [PATCH 7/8] Revert "Do not list system users in contacts menu if sharing autocompletion is disabled" This reverts commit 56a9084dd2f9a5726a74e1e789773558fade8b44. --- apps/dav/appinfo/app.php | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/apps/dav/appinfo/app.php b/apps/dav/appinfo/app.php index 963073c44139a..0d417fd3fed2d 100644 --- a/apps/dav/appinfo/app.php +++ b/apps/dav/appinfo/app.php @@ -50,13 +50,7 @@ function(GenericEvent $event) use ($app) { $cm = \OC::$server->getContactsManager(); $cm->register(function() use ($cm, $app) { $user = \OC::$server->getUserSession()->getUser(); - if (is_null($user)) { - return; + if (!is_null($user)) { + $app->setupContactsProvider($cm, $user->getUID()); } - if (\OC::$server->getConfig()->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') !== 'yes') { - // Don't include system users - // This prevents user enumeration in the contacts menu and the mail app - return; - } - $app->setupContactsProvider($cm, $user->getUID()); }); From 44ae66e7d29a7b2703760e9baa8369164084b2f9 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 15 Sep 2017 15:58:04 +0200 Subject: [PATCH 8/8] 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 --- .../Contacts/ContactsMenu/ContactsStore.php | 62 +++++-- settings/templates/admin/sharing.php | 2 +- .../ContactsMenu/ContactsStoreTest.php | 157 ++++++++++++++---- 3 files changed, 174 insertions(+), 47 deletions(-) diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 87aff258aaea5..3eda58cacfb24 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 @@ public function __construct(IManager $contactsManager, IConfig $config, IUserMan 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 @@ private function filterContacts(IUser $self, array $entries) { 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 @@ private function filterContacts(IUser $self, array $entries) { $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 @@ private function filterContacts(IUser $self, array $entries) { } return $entry->getProperty('UID') !== $selfUID; - }); - - + })); } /** @@ -173,7 +207,7 @@ public function findOne(IUser $user, $shareType, $shareWith) { } 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/admin/sharing.php b/settings/templates/admin/sharing.php index 38071a4bee9e4..9c9e8c07809b7 100644 --- a/settings/templates/admin/sharing.php +++ b/settings/templates/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 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 @@ protected function setUp() { 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 @@ public function testGetContactsWithoutFilter() { } 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 @@ public function testGetContactsHidesOwnEntry() { } 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 @@ public function testGetContactsWithoutBinaryImage() { } 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 @@ public function testGetContactsWithoutAvatarURI() { } 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 @@ public function testGetContactsWhenUserIsInExcludeGroups() { $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 @@ public function testGetContactsWhenUserIsInExcludeGroups() { $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 @@ public function testGetContactsOnlyIfInTheSameGroup() { $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 @@ public function testGetContactsOnlyIfInTheSameGroup() { $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 @@ public function testGetContactsOnlyIfInTheSameGroup() { $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 @@ public function testGetContactsOnlyIfInTheSameGroup() { $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 @@ public function testGetContactsOnlyIfInTheSameGroup() { $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 @@ public function testFindOneUser() { 'isLocalSystemBook' => true ], ]); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->willReturn('user123'); @@ -335,6 +425,7 @@ public function testFindOneUser() { } 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 @@ public function testFindOneEMail() { 'isLocalSystemBook' => false ], ]); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->willReturn('user123'); @@ -365,6 +456,7 @@ public function testFindOneEMail() { } 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 @@ public function testFindOneNotSupportedType() { } public function testFindOneNoMatches() { + /** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */ $user = $this->createMock(IUser::class); $this->contactsManager->expects($this->once()) ->method('search')