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()); }); diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 40a0bf870317d..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 * @@ -24,20 +25,43 @@ 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; } /** @@ -48,15 +72,97 @@ public function __construct(IManager $contactsManager) { public function getContacts(IUser $user, $filter) { $allContacts = $this->contactsManager->search($filter ?: '', [ 'FN', + 'EMAIL' ]); - $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, + $filter + ); + } + + /** + * Filters the contacts. Applies 3 filters: + * 1. filter the current user + * 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. + * 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, + $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 + $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') === 'yes'; + + $selfGroups = $this->groupManager->getUserGroupIds($self); + + if ($excludedGroups) { + $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); + $decodedExcludeGroups = json_decode($excludedGroups, true); + $excludeGroupsList = ($decodedExcludeGroups !== null) ? $decodedExcludeGroups : []; + + if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) { + // a group of the current user is excluded -> filter all local users + $skipLocal = true; + } + } + + $selfUID = $self->getUID(); + + 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) { + // no groups in common, so shouldn't see the contact + return false; + } + } + + return $entry->getProperty('UID') !== $selfUID; + })); } /** @@ -100,7 +206,17 @@ public function findOne(IUser $user, $shareType, $shareWith) { } } - return $match ? $this->contactArrayToEntry($match) : null; + if ($match) { + $match = $this->filterContacts($user, [$this->contactArrayToEntry($match)], $shareWith); + if (count($match) === 1) { + $match = $match[0]; + } else { + $match = null; + } + + } + + return $match; } /** diff --git a/settings/templates/settings/admin/sharing.php b/settings/templates/settings/admin/sharing.php index 38071a4bee9e4..9c9e8c07809b7 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 * @@ -26,31 +27,41 @@ 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; 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; 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() { + /** @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, @@ -76,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', @@ -102,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, @@ -130,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, @@ -157,7 +171,230 @@ 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_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(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') + ->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', 'EMAIL'])) + ->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_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(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') + ->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', 'EMAIL'])) + ->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[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') @@ -176,7 +413,7 @@ public function testFindOneUser() { 'isLocalSystemBook' => true ], ]); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->willReturn('user123'); @@ -188,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') @@ -206,7 +444,7 @@ public function testFindOneEMail() { 'isLocalSystemBook' => false ], ]); - $user->expects($this->once()) + $user->expects($this->any()) ->method('getUID') ->willReturn('user123'); @@ -218,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'); @@ -226,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')