Merge pull request #5585 from nextcloud/contacts_menu_privacy

Enhance privacy of contactsmenu fixes #5107
This commit is contained in:
Morris Jobke 2017-09-18 14:25:16 +02:00 committed by GitHub
commit 8b1eaba417
4 changed files with 377 additions and 27 deletions

View File

@ -50,13 +50,7 @@ $eventDispatcher->addListener('OCP\Federation\TrustedServerEvent::remove',
$cm = \OC::$server->getContactsManager(); $cm = \OC::$server->getContactsManager();
$cm->register(function() use ($cm, $app) { $cm->register(function() use ($cm, $app) {
$user = \OC::$server->getUserSession()->getUser(); $user = \OC::$server->getUserSession()->getUser();
if (is_null($user)) { if (!is_null($user)) {
return; $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());
}); });

View File

@ -1,9 +1,10 @@
<?php <?php
/** /**
* @copyright 2017 Christoph Wurst <christoph@winzerhof-wurst.at> * @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 Christoph Wurst <christoph@winzerhof-wurst.at>
* @author 2017 Lukas Reschke <lukas@statuscode.ch>
* *
* @license GNU AGPL version 3 or any later version * @license GNU AGPL version 3 or any later version
* *
@ -24,20 +25,43 @@
namespace OC\Contacts\ContactsMenu; namespace OC\Contacts\ContactsMenu;
use OC\Share\Share;
use OCP\Contacts\ContactsMenu\IEntry; use OCP\Contacts\ContactsMenu\IEntry;
use OCP\Contacts\IManager; use OCP\Contacts\IManager;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IUser; use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
class ContactsStore { class ContactsStore {
/** @var IManager */ /** @var IManager */
private $contactsManager; private $contactsManager;
/** @var IConfig */
private $config;
/** @var IUserManager */
private $userManager;
/** @var IGroupManager */
private $groupManager;
/** /**
* @param IManager $contactsManager * @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->contactsManager = $contactsManager;
$this->config = $config;
$this->userManager = $userManager;
$this->groupManager = $groupManager;
} }
/** /**
@ -48,15 +72,97 @@ class ContactsStore {
public function getContacts(IUser $user, $filter) { public function getContacts(IUser $user, $filter) {
$allContacts = $this->contactsManager->search($filter ?: '', [ $allContacts = $this->contactsManager->search($filter ?: '', [
'FN', 'FN',
'EMAIL'
]); ]);
$self = $user->getUID();
$entries = array_map(function(array $contact) { $entries = array_map(function(array $contact) {
return $this->contactArrayToEntry($contact); return $this->contactArrayToEntry($contact);
}, $allContacts); }, $allContacts);
return array_filter($entries, function(IEntry $entry) use ($self) { return $this->filterContacts(
return $entry->getProperty('UID') !== $self; $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 @@ class ContactsStore {
} }
} }
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;
} }
/** /**

View File

@ -96,7 +96,7 @@
<p class="<?php if ($_['shareAPIEnabled'] === 'no') p('hidden');?>"> <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" <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"'); ?> /> <?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>
<p> <p>
<input type="checkbox" id="publicShareDisclaimer" class="checkbox noJSAutoUpdate" <input type="checkbox" id="publicShareDisclaimer" class="checkbox noJSAutoUpdate"

View File

@ -1,9 +1,10 @@
<?php <?php
/** /**
* @copyright 2017 Christoph Wurst <christoph@winzerhof-wurst.at> * @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 Christoph Wurst <christoph@winzerhof-wurst.at>
* @author 2017 Lukas Reschke <lukas@statuscode.ch>
* *
* @license GNU AGPL version 3 or any later version * @license GNU AGPL version 3 or any later version
* *
@ -26,31 +27,41 @@ namespace Tests\Contacts\ContactsMenu;
use OC\Contacts\ContactsMenu\ContactsStore; use OC\Contacts\ContactsMenu\ContactsStore;
use OCP\Contacts\IManager; use OCP\Contacts\IManager;
use OCP\IConfig;
use OCP\IGroupManager;
use OCP\IUser; use OCP\IUser;
use OCP\IUserManager;
use PHPUnit_Framework_MockObject_MockObject; use PHPUnit_Framework_MockObject_MockObject;
use Test\TestCase; use Test\TestCase;
class ContactsStoreTest extends TestCase { class ContactsStoreTest extends TestCase {
/** @var ContactsStore */ /** @var ContactsStore */
private $contactsStore; private $contactsStore;
/** @var IManager|PHPUnit_Framework_MockObject_MockObject */ /** @var IManager|PHPUnit_Framework_MockObject_MockObject */
private $contactsManager; 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() { protected function setUp() {
parent::setUp(); parent::setUp();
$this->contactsManager = $this->createMock(IManager::class); $this->contactsManager = $this->createMock(IManager::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->contactsStore = new ContactsStore($this->contactsManager); $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() { public function testGetContactsWithoutFilter() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
->with($this->equalTo(''), $this->equalTo(['FN'])) ->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
->willReturn([ ->willReturn([
[ [
'UID' => 123, 'UID' => 123,
@ -76,10 +87,11 @@ class ContactsStoreTest extends TestCase {
} }
public function testGetContactsHidesOwnEntry() { public function testGetContactsHidesOwnEntry() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
->with($this->equalTo(''), $this->equalTo(['FN'])) ->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
->willReturn([ ->willReturn([
[ [
'UID' => 'user123', 'UID' => 'user123',
@ -102,10 +114,11 @@ class ContactsStoreTest extends TestCase {
} }
public function testGetContactsWithoutBinaryImage() { public function testGetContactsWithoutBinaryImage() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
->with($this->equalTo(''), $this->equalTo(['FN'])) ->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
->willReturn([ ->willReturn([
[ [
'UID' => 123, 'UID' => 123,
@ -130,10 +143,11 @@ class ContactsStoreTest extends TestCase {
} }
public function testGetContactsWithoutAvatarURI() { public function testGetContactsWithoutAvatarURI() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
->with($this->equalTo(''), $this->equalTo(['FN'])) ->with($this->equalTo(''), $this->equalTo(['FN', 'EMAIL']))
->willReturn([ ->willReturn([
[ [
'UID' => 123, 'UID' => 123,
@ -157,7 +171,230 @@ class ContactsStoreTest extends TestCase {
$this->assertEquals('https://photo', $entries[1]->getAvatar()); $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() { 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); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
@ -176,7 +413,7 @@ class ContactsStoreTest extends TestCase {
'isLocalSystemBook' => true 'isLocalSystemBook' => true
], ],
]); ]);
$user->expects($this->once()) $user->expects($this->any())
->method('getUID') ->method('getUID')
->willReturn('user123'); ->willReturn('user123');
@ -188,6 +425,7 @@ class ContactsStoreTest extends TestCase {
} }
public function testFindOneEMail() { public function testFindOneEMail() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')
@ -206,7 +444,7 @@ class ContactsStoreTest extends TestCase {
'isLocalSystemBook' => false 'isLocalSystemBook' => false
], ],
]); ]);
$user->expects($this->once()) $user->expects($this->any())
->method('getUID') ->method('getUID')
->willReturn('user123'); ->willReturn('user123');
@ -218,6 +456,7 @@ class ContactsStoreTest extends TestCase {
} }
public function testFindOneNotSupportedType() { public function testFindOneNotSupportedType() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$entry = $this->contactsStore->findOne($user, 42, 'darren@roner.au'); $entry = $this->contactsStore->findOne($user, 42, 'darren@roner.au');
@ -226,6 +465,7 @@ class ContactsStoreTest extends TestCase {
} }
public function testFindOneNoMatches() { public function testFindOneNoMatches() {
/** @var IUser|PHPUnit_Framework_MockObject_MockObject $user */
$user = $this->createMock(IUser::class); $user = $this->createMock(IUser::class);
$this->contactsManager->expects($this->once()) $this->contactsManager->expects($this->once())
->method('search') ->method('search')