Skip to content

Commit e7772cf

Browse files
hamza221hamza221
authored
Merge pull request #12796 from nextcloud/backport/12795/stable5.6
[stable5.6] fix(autocomplete): respect privacy settings for sharing
2 parents 005bf83 + 9cebfc7 commit e7772cf

10 files changed

Lines changed: 298 additions & 108 deletions

File tree

‎lib/Controller/ContactIntegrationController.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public function __construct(string $appName,
4646
*/
4747
#[TrapError]
4848
public function match(string $mail): JSONResponse {
49-
return (new JSONResponse($this->service->findMatches($mail)))->cacheFor(60 * 60, false, true);
49+
return (new JSONResponse($this->service->findMatches($this->uid, $mail)))->cacheFor(60 * 60, false, true);
5050
}
5151

5252
/**
@@ -92,7 +92,7 @@ public function autoComplete(string $term): JSONResponse {
9292
return new JSONResponse($decoded);
9393
}
9494
}
95-
$res = $this->service->autoComplete($term);
95+
$res = $this->service->autoComplete($this->uid, $term);
9696
$this->cache->set("{$this->uid}:$term", json_encode($res), 24 * 3600);
9797
return new JSONResponse($res);
9898
}

‎lib/IMAP/ImapMessageFetcher.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ private function parseHeaders(Horde_Imap_Client_Data_Fetch $fetch): void {
532532
$this->hasDkimSignature = $dkimSignatureHeader !== null;
533533

534534
if ($this->runPhishingCheck) {
535-
$this->phishingDetails = $this->phishingDetectionService->checkHeadersForPhishing($parsedHeaders, $this->hasHtmlMessage, $this->htmlMessage);
535+
$this->phishingDetails = $this->phishingDetectionService->checkHeadersForPhishing($this->userId, $parsedHeaders, $this->hasHtmlMessage, $this->htmlMessage);
536536
}
537537

538538
$listUnsubscribeHeader = $parsedHeaders->getHeader('list-unsubscribe');

‎lib/Service/ContactIntegration/ContactIntegrationService.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ public function __construct(ContactsIntegration $ci) {
1919
$this->contactsIntegration = $ci;
2020
}
2121

22-
public function findMatches(string $mail): array {
23-
$matches = $this->contactsIntegration->getContactsWithMail($mail);
22+
public function findMatches(string $uid, string $mail): array {
23+
$matches = $this->contactsIntegration->getContactsWithMail($uid, $mail);
2424
return $matches;
2525
}
2626

@@ -32,7 +32,7 @@ public function newContact(string $name, string $mail): ?array {
3232
return $this->contactsIntegration->newContact($name, $mail);
3333
}
3434

35-
public function autoComplete(string $term): array {
36-
return $this->contactsIntegration->getContactsWithName($term);
35+
public function autoComplete(string $uid, string $term): array {
36+
return $this->contactsIntegration->getContactsWithName($uid, $term);
3737
}
3838
}

‎lib/Service/ContactsIntegration.php‎

Lines changed: 94 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OCP\IConfig;
1515
use OCP\IGroupManager;
1616
use OCP\IUserManager;
17+
use function is_array;
1718

1819
class ContactsIntegration {
1920
/** @var IManager */
@@ -46,55 +47,9 @@ public function __construct(IManager $contactsManager,
4647
* @return array
4748
*/
4849
public function getMatchingRecipient(string $userId, string $term): array {
49-
if (!$this->contactsManager->isEnabled()) {
50-
return [];
51-
}
52-
53-
// If 'Allow username autocompletion in share dialog' is disabled in the admin sharing settings, then we must not
54-
// auto-complete system users
55-
$shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
56-
$shareeEnumerationInGroupOnly = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
57-
$shareeEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
58-
$shareeEnumerationFullMatchUserId = $shareeEnumerationFullMatch && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_userid', 'yes') === 'yes';
59-
$shareeEnumerationFullMatchEmail = $shareeEnumerationFullMatch && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_email', 'yes') === 'yes';
60-
61-
$result = $this->contactsManager->search(
62-
$term,
63-
['UID', 'FN', 'EMAIL'],
64-
[
65-
'enumeration' => $shareeEnumeration,
66-
'fullmatch' => $shareeEnumerationFullMatch,
67-
'limit' => 20,
68-
],
69-
);
70-
if (empty($result)) {
71-
return [];
72-
}
50+
$result = $this->search($userId, $term, ['UID', 'FN', 'EMAIL']);
7351
$receivers = [];
74-
75-
if ($shareeEnumeration && $shareeEnumerationInGroupOnly) {
76-
$user = $this->userManager->get($userId);
77-
if ($user === null) {
78-
return [];
79-
}
80-
$userGroups = $this->groupManager->getUserGroupIds($user);
81-
}
82-
8352
foreach ($result as $r) {
84-
$isSystemUser = isset($r['isLocalSystemBook']) && $r['isLocalSystemBook'];
85-
$isInSameGroup = false;
86-
if ($isSystemUser && $shareeEnumerationInGroupOnly) {
87-
foreach ($userGroups as $userGroup) {
88-
if ($this->groupManager->isInGroup($r['UID'], $userGroup)) {
89-
$isInSameGroup = true;
90-
break;
91-
}
92-
}
93-
if (!$shareeEnumerationFullMatch && !$isInSameGroup) {
94-
continue;
95-
}
96-
}
97-
9853
$id = $r['UID'];
9954
$fn = $r['FN'] ?? null;
10055
if (!isset($r['EMAIL'])) {
@@ -111,23 +66,10 @@ public function getMatchingRecipient(string $userId, string $term): array {
11166
if ($e === '') {
11267
continue;
11368
}
114-
$lowerTerm = strtolower($term);
115-
116-
if ($isSystemUser && $shareeEnumerationInGroupOnly && !$isInSameGroup) {
117-
// Check for full match. If full match is disabled, matching results already filtered out
118-
if (!($lowerTerm !== '' && (
119-
($shareeEnumerationFullMatch && !empty($fn) && $lowerTerm === strtolower($fn))
120-
|| ($shareeEnumerationFullMatchUserId && $lowerTerm === strtolower($id))
121-
|| ($shareeEnumerationFullMatchEmail && $lowerTerm === strtolower($e))))) {
122-
// Not a full Match
123-
continue;
124-
}
125-
}
126-
12769
$receivers[] = [
12870
'id' => $id,
12971
// Show full name if possible or fall back to email
130-
'label' => $fn,
72+
'label' => $fn ?? $e,
13173
'email' => $e,
13274
'photo' => $photo,
13375
'source' => 'contacts',
@@ -229,21 +171,98 @@ public function newContact(string $name, string $mailAddr, string $type = 'HOME'
229171
return $createdContact;
230172
}
231173

174+
private function search(string $userId, string $term, array $fields, ?bool $strictSearch = null): array {
175+
if (!$this->contactsManager->isEnabled()) {
176+
return [];
177+
}
178+
179+
// If 'Allow username autocompletion in share dialog' is disabled in the admin sharing settings, then we must not
180+
// auto-complete system users
181+
$shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
182+
$shareeEnumerationInGroupOnly = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
183+
$shareeEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
184+
$shareeEnumerationFullMatchDisplayName = $shareeEnumerationFullMatch && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_displayname', 'yes') === 'yes';
185+
$shareeEnumerationFullMatchUserId = $shareeEnumerationFullMatch && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_userid', 'yes') === 'yes';
186+
$shareeEnumerationFullMatchEmail = $shareeEnumerationFullMatch && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match_email', 'yes') === 'yes';
187+
188+
$options = [
189+
'enumeration' => $shareeEnumeration,
190+
'fullmatch' => $shareeEnumerationFullMatch,
191+
'limit' => 20,
192+
];
193+
if ($strictSearch !== null) {
194+
$options['strict_search'] = $strictSearch;
195+
}
196+
197+
$result = $this->contactsManager->search(
198+
$term,
199+
$fields,
200+
$options,
201+
);
202+
203+
$userGroups = [];
204+
if ($shareeEnumeration && $shareeEnumerationInGroupOnly) {
205+
$user = $this->userManager->get($userId);
206+
if ($user === null) {
207+
return [];
208+
}
209+
$userGroups = $this->groupManager->getUserGroupIds($user);
210+
}
211+
212+
$filteredResults = [];
213+
foreach ($result as $r) {
214+
$isSystemUser = isset($r['isLocalSystemBook']) && $r['isLocalSystemBook'];
215+
$isInSameGroup = false;
216+
if ($isSystemUser && $shareeEnumerationInGroupOnly) {
217+
foreach ($userGroups as $userGroup) {
218+
if ($this->groupManager->isInGroup($r['UID'], $userGroup)) {
219+
$isInSameGroup = true;
220+
break;
221+
}
222+
}
223+
if (!$shareeEnumerationFullMatch && !$isInSameGroup) {
224+
continue;
225+
}
226+
}
227+
228+
if ($isSystemUser && $shareeEnumerationInGroupOnly && !$isInSameGroup && $shareeEnumerationFullMatch) {
229+
// Check for full match. If full match is disabled, non-matching results already filtered out above.
230+
$id = $r['UID'];
231+
$fn = $r['FN'] ?? null;
232+
$lowerTerm = strtolower($term);
233+
$isMatch = ($lowerTerm !== '' && (
234+
($shareeEnumerationFullMatchDisplayName && !empty($fn) && $lowerTerm === strtolower($fn))
235+
|| ($shareeEnumerationFullMatchUserId && $lowerTerm === strtolower($id)))) ;
236+
if ($shareeEnumerationFullMatchEmail && !$isMatch) {
237+
$email = $r['EMAIL'] ?? null;
238+
if ($email === null) {
239+
continue;
240+
}
241+
$emails = is_array($email) ? $email : [$email];
242+
foreach ($emails as $e) {
243+
if ($lowerTerm === strtolower($e)) {
244+
$isMatch = true;
245+
break;
246+
}
247+
}
248+
}
249+
if (!$isMatch) {
250+
continue;
251+
}
252+
}
253+
254+
$filteredResults[] = $r;
255+
}
256+
return $filteredResults;
257+
}
258+
232259
/**
233260
* @param string[] $fields
234261
*/
235-
private function doSearch(string $term, array $fields, bool $strictSearch) : array {
236-
$allowSystemUsers = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
237-
238-
$result = $this->contactsManager->search($term, $fields, [
239-
'strict_search' => $strictSearch,
240-
'limit' => 20,
241-
]);
262+
private function doSearch(string $userId, string $term, array $fields, bool $strictSearch) : array {
263+
$result = $this->search($userId, $term, $fields, $strictSearch);
242264
$matches = [];
243265
foreach ($result as $r) {
244-
if (!$allowSystemUsers && isset($r['isLocalSystemBook']) && $r['isLocalSystemBook']) {
245-
continue;
246-
}
247266
$id = $r['UID'];
248267
$fn = $r['FN'];
249268
$email = $r['EMAIL'] ?? null;
@@ -258,18 +277,15 @@ private function doSearch(string $term, array $fields, bool $strictSearch) : arr
258277

259278
/**
260279
* Extracts all Contacts with the specified mail address
261-
*
262-
* @param string $mailAddr
263-
* @return array
264280
*/
265-
public function getContactsWithMail(string $mailAddr) {
266-
return $this->doSearch($mailAddr, ['EMAIL'], true);
281+
public function getContactsWithMail(string $userId, string $mailAddr): array {
282+
return $this->doSearch($userId, $mailAddr, ['EMAIL'], true);
267283
}
268284

269285
/**
270286
* Extracts all Contacts with the specified name
271287
*/
272-
public function getContactsWithName(string $name): array {
273-
return $this->doSearch($name, ['FN'], false);
288+
public function getContactsWithName(string $userId, string $name): array {
289+
return $this->doSearch($userId, $name, ['FN'], false);
274290
}
275291
}

‎lib/Service/PhishingDetection/ContactCheck.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@ public function __construct(
2222
$this->contactIntegration = $contactIntegration;
2323
}
2424

25-
public function run(string $fn, string $email): PhishingDetectionResult {
25+
public function run(string $userId, string $fn, string $email): PhishingDetectionResult {
2626
$emails = [];
27-
$contacts = $this->contactIntegration->getContactsWithName($fn);
27+
$contacts = $this->contactIntegration->getContactsWithName($userId, $fn);
2828
foreach ($contacts as $contact) {
2929
if (!isset($contact['email'])) {
3030
continue;

‎lib/Service/PhishingDetection/PhishingDetectionService.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ public function __construct(
2424
) {
2525
}
2626

27-
public function checkHeadersForPhishing(Horde_Mime_Headers $headers, bool $hasHtmlMessage, string $htmlMessage = ''): array {
27+
public function checkHeadersForPhishing(string $uid, Horde_Mime_Headers $headers, bool $hasHtmlMessage, string $htmlMessage = ''): array {
2828
/** @var string|null $fromFN */
2929
$fromFN = null;
3030
/** @var string|null $fromEmail */
@@ -57,7 +57,7 @@ public function checkHeadersForPhishing(Horde_Mime_Headers $headers, bool $hasHt
5757
$list->addCheck($this->replyToCheck->run($fromEmail, $replyToEmail));
5858
}
5959
if ($fromFN !== null) {
60-
$list->addCheck($this->contactCheck->run($fromFN, $fromEmail));
60+
$list->addCheck($this->contactCheck->run($uid, $fromFN, $fromEmail));
6161
}
6262
if ($customEmail !== null) {
6363
$list->addCheck($this->customEmailCheck->run($fromEmail, $customEmail));

‎tests/Integration/Service/Phishing/PhishingDetectionServiceIntegrationTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,10 @@ protected function setUp(): void {
5252
public function testContactCheck(): void {
5353
$this->contactsIntegration->expects(self::once())
5454
->method('getContactsWithName')
55-
->with('John Doe')
55+
->with('batman', 'John Doe')
5656
->willReturn([['id' => 1, 'fn' => 'John Doe', 'email' => ['[email protected]','[email protected]']]]);
5757

58-
$result = $this->contactCheck->run('John Doe', '[email protected]');
58+
$result = $this->contactCheck->run('batman', 'John Doe', '[email protected]');
5959

6060
$this->assertTrue($result->isPhishing());
6161
}
@@ -73,7 +73,7 @@ public function testCheckHeadersForPhishing(): void {
7373
$headerStream = fopen(__DIR__ . '/../../../data/phishing-mail-headers.txt', 'r');
7474
$parsedHeaders = Horde_Mime_Headers::parseHeaders($headerStream);
7575
fclose($headerStream);
76-
$result = $this->service->checkHeadersForPhishing($parsedHeaders, false);
76+
$result = $this->service->checkHeadersForPhishing('batman', $parsedHeaders, false);
7777
$this->assertTrue($result['warning']);
7878
}
7979

0 commit comments

Comments
 (0)