Add a setting to restrict returning a full match unless in phonebook or same group

Signed-off-by: Joas Schilling <coding@schilljs.com>
This commit is contained in:
Joas Schilling 2021-03-10 17:18:44 +01:00
parent 177ae33ba1
commit 5b53b6f977
No known key found for this signature in database
GPG Key ID: 7076EA9751AACDDA
13 changed files with 243 additions and 20 deletions

View File

@ -263,6 +263,7 @@ class Principal implements BackendInterface {
$allowEnumeration = $this->shareManager->allowEnumeration();
$limitEnumerationGroup = $this->shareManager->limitEnumerationToGroups();
$limitEnumerationPhone = $this->shareManager->limitEnumerationToPhone();
$allowEnumerationFullMatch = $this->shareManager->allowEnumerationFullMatch();
// If sharing is restricted to group members only,
// return only members that have groups in common
@ -290,15 +291,19 @@ class Principal implements BackendInterface {
foreach ($searchProperties as $prop => $value) {
switch ($prop) {
case '{http://sabredav.org/ns}email-address':
$users = $this->userManager->getByEmail($value);
if (!$allowEnumeration) {
$users = \array_filter($users, static function (IUser $user) use ($value) {
return $user->getEMailAddress() === $value;
});
if ($allowEnumerationFullMatch) {
$users = $this->userManager->getByEmail($value);
$users = \array_filter($users, static function (IUser $user) use ($value) {
return $user->getEMailAddress() === $value;
});
} else {
$users = [];
}
} else {
$users = \array_filter($users, function (IUser $user) use ($currentUser, $value, $limitEnumerationPhone, $limitEnumerationGroup, $currentUserGroups) {
if ($user->getEMailAddress() === $value) {
$users = $this->userManager->getByEmail($value);
$users = \array_filter($users, function (IUser $user) use ($currentUser, $value, $limitEnumerationPhone, $limitEnumerationGroup, $allowEnumerationFullMatch, $currentUserGroups) {
if ($allowEnumerationFullMatch && $user->getEMailAddress() === $value) {
return true;
}
@ -336,15 +341,20 @@ class Principal implements BackendInterface {
break;
case '{DAV:}displayname':
$users = $this->userManager->searchDisplayName($value, $searchLimit);
if (!$allowEnumeration) {
$users = \array_filter($users, static function (IUser $user) use ($value) {
return $user->getDisplayName() === $value;
});
if ($allowEnumerationFullMatch) {
$users = $this->userManager->searchDisplayName($value, $searchLimit);
$users = \array_filter($users, static function (IUser $user) use ($value) {
return $user->getDisplayName() === $value;
});
} else {
$users = [];
}
} else {
$users = \array_filter($users, function (IUser $user) use ($currentUser, $value, $limitEnumerationPhone, $limitEnumerationGroup, $currentUserGroups) {
if ($user->getDisplayName() === $value) {
$users = $this->userManager->searchDisplayName($value, $searchLimit);
$users = \array_filter($users, function (IUser $user) use ($currentUser, $value, $limitEnumerationPhone, $limitEnumerationGroup, $allowEnumerationFullMatch, $currentUserGroups) {
if ($allowEnumerationFullMatch && $user->getDisplayName() === $value) {
return true;
}

View File

@ -570,6 +570,10 @@ class PrincipalTest extends TestCase {
->method('shareWithGroupMembersOnly')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('allowEnumerationFullMatch')
->willReturn(true);
$user2 = $this->createMock(IUser::class);
$user2->method('getUID')->willReturn('user2');
$user2->method('getDisplayName')->willReturn('User 2');
@ -592,6 +596,27 @@ class PrincipalTest extends TestCase {
['{DAV:}displayname' => 'User 2']));
}
public function testSearchPrincipalWithEnumerationDisabledDisplaynameOnFullMatch() {
$this->shareManager->expects($this->once())
->method('shareAPIEnabled')
->willReturn(true);
$this->shareManager->expects($this->once())
->method('allowEnumeration')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('shareWithGroupMembersOnly')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('allowEnumerationFullMatch')
->willReturn(false);
$this->assertEquals([], $this->connector->searchPrincipals('principals/users',
['{DAV:}displayname' => 'User 2']));
}
public function testSearchPrincipalWithEnumerationDisabledEmail() {
$this->shareManager->expects($this->once())
->method('shareAPIEnabled')
@ -605,6 +630,10 @@ class PrincipalTest extends TestCase {
->method('shareWithGroupMembersOnly')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('allowEnumerationFullMatch')
->willReturn(true);
$user2 = $this->createMock(IUser::class);
$user2->method('getUID')->willReturn('user2');
$user2->method('getDisplayName')->willReturn('User 2');
@ -627,6 +656,28 @@ class PrincipalTest extends TestCase {
['{http://sabredav.org/ns}email-address' => 'user2@foo.bar']));
}
public function testSearchPrincipalWithEnumerationDisabledEmailOnFullMatch() {
$this->shareManager->expects($this->once())
->method('shareAPIEnabled')
->willReturn(true);
$this->shareManager->expects($this->once())
->method('allowEnumeration')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('shareWithGroupMembersOnly')
->willReturn(false);
$this->shareManager->expects($this->once())
->method('allowEnumerationFullMatch')
->willReturn(false);
$this->assertEquals([], $this->connector->searchPrincipals('principals/users',
['{http://sabredav.org/ns}email-address' => 'user2@foo.bar']));
}
public function testSearchPrincipalWithEnumerationLimitedDisplayname() {
$this->shareManager->expects($this->at(0))
->method('shareAPIEnabled')

View File

@ -74,6 +74,7 @@ class Sharing implements ISettings {
'allowShareDialogUserEnumeration' => $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes'),
'restrictUserEnumerationToGroup' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no'),
'restrictUserEnumerationToPhone' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no'),
'restrictUserEnumerationFullMatch' => $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes'),
'enforceLinkPassword' => Util::isPublicLinkPasswordRequired(),
'onlyShareWithGroupMembers' => $this->shareManager->shareWithGroupMembersOnly(),
'shareAPIEnabled' => $this->config->getAppValue('core', 'shareapi_enabled', 'yes'),

View File

@ -163,7 +163,7 @@
<?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 or email address needs to be entered)'));?></label><br />
<label for="shareapi_allow_share_dialog_user_enumeration"><?php p($l->t('Allow username autocompletion in share dialog'));?></label><br />
</p>
<p id="shareapi_restrict_user_enumeration_to_group_setting" class="indent <?php if ($_['shareAPIEnabled'] === 'no' || $_['allowShareDialogUserEnumeration'] === 'no') {
@ -190,6 +190,15 @@
}?>">
<em><?php p($l->t('If autocompletion "same group" and "phonebook matches" are enabled a match in either is enough to show the user.'));?></em><br />
</p>
<p id="shareapi_restrict_user_enumeration_full_match_setting" class="indent <?php if ($_['shareAPIEnabled'] === 'no') {
p('hidden');
}?>">
<input type="checkbox" name="shareapi_restrict_user_enumeration_full_match" value="1" id="shareapi_restrict_user_enumeration_full_match" class="checkbox"
<?php if ($_['restrictUserEnumerationFullMatch'] === 'yes') {
print_unescaped('checked="checked"');
} ?> />
<label for="shareapi_restrict_user_enumeration_full_match"><?php p($l->t('Allow username autocompletion when entering the full name or email address (ignoring missing phonebook match and being in the same group)'));?></label><br />
</p>
<p>
<input type="checkbox" id="publicShareDisclaimer" class="checkbox noJSAutoUpdate"

View File

@ -74,6 +74,7 @@ class SharingTest extends TestCase {
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'],
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'],
['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'],
['core', 'shareapi_restrict_user_enumeration_full_match', 'yes', 'yes'],
['core', 'shareapi_enabled', 'yes', 'yes'],
['core', 'shareapi_default_expire_date', 'no', 'no'],
['core', 'shareapi_expire_after_n_days', '7', '7'],
@ -98,6 +99,7 @@ class SharingTest extends TestCase {
'allowShareDialogUserEnumeration' => 'yes',
'restrictUserEnumerationToGroup' => 'no',
'restrictUserEnumerationToPhone' => 'no',
'restrictUserEnumerationFullMatch' => 'yes',
'enforceLinkPassword' => false,
'onlyShareWithGroupMembers' => false,
'shareAPIEnabled' => 'yes',
@ -132,6 +134,7 @@ class SharingTest extends TestCase {
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'yes'],
['core', 'shareapi_restrict_user_enumeration_to_group', 'no', 'no'],
['core', 'shareapi_restrict_user_enumeration_to_phone', 'no', 'no'],
['core', 'shareapi_restrict_user_enumeration_full_match', 'yes', 'yes'],
['core', 'shareapi_enabled', 'yes', 'yes'],
['core', 'shareapi_default_expire_date', 'no', 'no'],
['core', 'shareapi_expire_after_n_days', '7', '7'],
@ -156,6 +159,7 @@ class SharingTest extends TestCase {
'allowShareDialogUserEnumeration' => 'yes',
'restrictUserEnumerationToGroup' => 'no',
'restrictUserEnumerationToPhone' => 'no',
'restrictUserEnumerationFullMatch' => 'yes',
'enforceLinkPassword' => false,
'onlyShareWithGroupMembers' => false,
'shareAPIEnabled' => 'yes',

View File

@ -3,6 +3,7 @@ Feature: autocomplete
Given using api version "2"
And group "commongroup" exists
And user "admin" belongs to group "commongroup"
And user "auto" exists
And user "autocomplete" exists
And user "autocomplete2" exists
And user "autocomplete2" belongs to group "commongroup"
@ -20,9 +21,15 @@ Feature: autocomplete
When parameter "shareapi_allow_share_dialog_user_enumeration" of app "core" is set to "no"
Then get autocomplete for "auto"
| id | source |
| auto | users |
Then get autocomplete for "autocomplete"
| id | source |
| autocomplete | users |
When parameter "shareapi_restrict_user_enumeration_full_match" of app "core" is set to "no"
Then get autocomplete for "auto"
| id | source |
Then get autocomplete for "autocomplete"
| id | source |
Scenario: getting autocomplete with limited enumeration by group
@ -30,6 +37,7 @@ Feature: autocomplete
When parameter "shareapi_restrict_user_enumeration_to_group" of app "core" is set to "yes"
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete2 | users |
Then get autocomplete for "autocomplete"
| id | source |
@ -38,6 +46,13 @@ Feature: autocomplete
Then get autocomplete for "autocomplete2"
| id | source |
| autocomplete2 | users |
When parameter "shareapi_restrict_user_enumeration_full_match" of app "core" is set to "no"
Then get autocomplete for "autocomplete"
| id | source |
| autocomplete2 | users |
Then get autocomplete for "autocomplete2"
| id | source |
| autocomplete2 | users |
Scenario: getting autocomplete with limited enumeration by phone
@ -45,6 +60,7 @@ Feature: autocomplete
When parameter "shareapi_restrict_user_enumeration_to_phone" of app "core" is set to "yes"
Then get autocomplete for "auto"
| id | source |
| auto | users |
# autocomplete stores their phone number
Given As an "autocomplete"
@ -57,10 +73,17 @@ Feature: autocomplete
Given As an "admin"
Then get autocomplete for "auto"
| id | source |
| auto | users |
# admin populates they have the phone number
When search users by phone for region "DE" with
| random-string1 | 0711 / 252 428-90 |
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete | users |
When parameter "shareapi_restrict_user_enumeration_full_match" of app "core" is set to "no"
Then get autocomplete for "auto"
| id | source |
| autocomplete | users |
@ -83,6 +106,13 @@ Feature: autocomplete
When search users by phone for region "DE" with
| random-string1 | 0711 / 252 428-90 |
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete | users |
| autocomplete2 | users |
When parameter "shareapi_restrict_user_enumeration_full_match" of app "core" is set to "no"
Then get autocomplete for "auto"
| id | source |
| autocomplete | users |
@ -108,6 +138,7 @@ Feature: autocomplete
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete | users |
| autocomplete2 | users |
When parameter "shareapi_only_share_with_group_members" of app "core" is set to "yes"
@ -121,6 +152,7 @@ Feature: autocomplete
When parameter "shareapi_restrict_user_enumeration_to_phone" of app "core" is set to "yes"
Then get autocomplete for "auto"
| id | source |
| auto | users |
# autocomplete stores their phone number
Given As an "autocomplete"
@ -133,12 +165,14 @@ Feature: autocomplete
Given As an "admin"
Then get autocomplete for "auto"
| id | source |
| auto | users |
# admin populates they have the phone number
When search users by phone for region "DE" with
| random-string1 | 0711 / 252 428-90 |
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete | users |
# autocomplete changes their phone number
@ -152,12 +186,14 @@ Feature: autocomplete
Given As an "admin"
Then get autocomplete for "auto"
| id | source |
| auto | users |
# admin populates they have the new phone number
When search users by phone for region "DE" with
| random-string1 | 0711 / 252 428-91 |
Then get autocomplete for "auto"
| id | source |
| auto | users |
| autocomplete | users |

View File

@ -66,6 +66,7 @@ class CollaborationContext implements Context {
$this->deleteServerConfig('core', 'shareapi_allow_share_dialog_user_enumeration');
$this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_to_group');
$this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_to_phone');
$this->deleteServerConfig('core', 'shareapi_restrict_user_enumeration_full_match');
$this->deleteServerConfig('core', 'shareapi_only_share_with_group_members');
}
}

View File

@ -49,6 +49,8 @@ class MailPlugin implements ISearchPlugin {
protected $shareeEnumerationInGroupOnly;
/* @var bool */
protected $shareeEnumerationPhone;
/* @var bool */
protected $shareeEnumerationFullMatch;
/** @var IManager */
private $contactsManager;
@ -81,6 +83,7 @@ class MailPlugin implements ISearchPlugin {
$this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$this->shareeEnumerationPhone = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
$this->shareeEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
}
/**
@ -137,7 +140,7 @@ class MailPlugin implements ISearchPlugin {
continue;
}
}
if ($exactEmailMatch) {
if ($exactEmailMatch && $this->shareeEnumerationFullMatch) {
try {
$cloud = $this->cloudIdManager->resolveCloudId($contact['CLOUD'][0]);
} catch (\InvalidArgumentException $e) {

View File

@ -53,6 +53,8 @@ class UserPlugin implements ISearchPlugin {
protected $shareeEnumerationInGroupOnly;
/* @var bool */
protected $shareeEnumerationPhone;
/* @var bool */
protected $shareeEnumerationFullMatch;
/** @var IConfig */
private $config;
@ -85,6 +87,7 @@ class UserPlugin implements ISearchPlugin {
$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$this->shareeEnumerationPhone = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
$this->shareeEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
}
public function search($search, $limit, $offset, ISearchResult $searchResult) {
@ -150,6 +153,7 @@ class UserPlugin implements ISearchPlugin {
if (
$this->shareeEnumerationFullMatch &&
$lowerSearch !== '' && (strtolower($uid) === $lowerSearch ||
strtolower($userDisplayName) === $lowerSearch ||
strtolower($userEmail) === $lowerSearch)
@ -202,7 +206,7 @@ class UserPlugin implements ISearchPlugin {
}
}
if ($offset === 0 && !$foundUserById) {
if ($this->shareeEnumerationFullMatch && $offset === 0 && !$foundUserById) {
// On page one we try if the search result has a direct hit on the
// user id and if so, we add that to the exact match list
$user = $this->userManager->get($search);

View File

@ -124,6 +124,7 @@ class ContactsStore implements IContactsStore {
$disallowEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') !== 'yes';
$restrictEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
$restrictEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
$allowEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes';
// whether to filter out local users
@ -146,7 +147,7 @@ class ContactsStore implements IContactsStore {
$selfUID = $self->getUID();
return array_values(array_filter($entries, function (IEntry $entry) use ($skipLocal, $ownGroupsOnly, $selfGroups, $selfUID, $disallowEnumeration, $restrictEnumerationGroup, $restrictEnumerationPhone, $filter) {
return array_values(array_filter($entries, function (IEntry $entry) use ($skipLocal, $ownGroupsOnly, $selfGroups, $selfUID, $disallowEnumeration, $restrictEnumerationGroup, $restrictEnumerationPhone, $allowEnumerationFullMatch, $filter) {
if ($entry->getProperty('UID') === $selfUID) {
return false;
}
@ -160,6 +161,10 @@ class ContactsStore implements IContactsStore {
// Prevent enumerating local users
if ($disallowEnumeration) {
if (!$allowEnumerationFullMatch) {
return false;
}
$filterUser = true;
$mailAddresses = $entry->getEMailAddresses();

View File

@ -1834,6 +1834,10 @@ class Manager implements IManager {
$this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
}
public function allowEnumerationFullMatch(): bool {
return $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
}
/**
* Copied from \OC_Util::isSharingDisabledForUser
*

View File

@ -392,6 +392,14 @@ interface IManager {
*/
public function limitEnumerationToPhone(): bool;
/**
* Check if user enumeration is allowed to return on full match
*
* @return bool
* @since 21.0.1
*/
public function allowEnumerationFullMatch(): bool;
/**
* Check if sharing is disabled for the given user
*

View File

@ -683,9 +683,12 @@ class ContactsStoreTest extends TestCase {
}
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');
$this->config
->method('getAppValue')
->willReturnMap([
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'],
['core', 'shareapi_restrict_user_enumeration_full_match', 'yes', 'yes'],
]);
/** @var IUser|\PHPUnit\Framework\MockObject\MockObject $user */
$user = $this->createMock(IUser::class);
@ -766,6 +769,90 @@ class ContactsStoreTest extends TestCase {
], $entry[0]->getEMailAddresses());
}
public function testGetContactsWithFilterWithoutFullMatch() {
$this->config
->method('getAppValue')
->willReturnMap([
['core', 'shareapi_allow_share_dialog_user_enumeration', 'yes', 'no'],
['core', 'shareapi_restrict_user_enumeration_full_match', 'yes', '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 not match
$entry = $this->contactsStore->getContacts($user, 'a567');
$this->assertSame(1, count($entry));
$this->assertEquals([
'anne@example.com'
], $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 not match
$entry = $this->contactsStore->getContacts($user, 'john@example.com');
$this->assertSame(1, count($entry));
$this->assertEquals([
'anne@example.com'
], $entry[0]->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'))