Skip to content

Commit a6a2060

Browse files
provokateurinprovokateurin
committed
perf(MailPlugin): Optimize checking group memberships
Signed-off-by: provokateurin <kate@provokateurin.de>
1 parent 89f8383 commit a6a2060

4 files changed

Lines changed: 37 additions & 43 deletions

File tree

‎lib/private/Collaboration/Collaborators/MailByMailPlugin.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCP\Federation\ICloudIdManager;
1414
use OCP\IConfig;
1515
use OCP\IGroupManager;
16+
use OCP\IUserManager;
1617
use OCP\IUserSession;
1718
use OCP\Mail\IEmailValidator;
1819
use OCP\Share\IShare;
@@ -30,6 +31,7 @@ public function __construct(
3031
KnownUserService $knownUserService,
3132
IUserSession $userSession,
3233
IEmailValidator $emailValidator,
34+
IUserManager $userManager,
3335
mixed $shareWithGroupOnlyExcludeGroupsList = [],
3436
) {
3537
parent::__construct(
@@ -40,6 +42,7 @@ public function __construct(
4042
$knownUserService,
4143
$userSession,
4244
$emailValidator,
45+
$userManager,
4346
$shareWithGroupOnlyExcludeGroupsList,
4447
IShare::TYPE_EMAIL,
4548
);

‎lib/private/Collaboration/Collaborators/MailPlugin.php‎

Lines changed: 15 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
55
* SPDX-License-Identifier: AGPL-3.0-or-later
66
*/
7+
78
namespace OC\Collaboration\Collaborators;
89

910
use OC\KnownUser\KnownUserService;
@@ -16,6 +17,7 @@
1617
use OCP\IConfig;
1718
use OCP\IGroupManager;
1819
use OCP\IUser;
20+
use OCP\IUserManager;
1921
use OCP\IUserSession;
2022
use OCP\Mail\IEmailValidator;
2123
use OCP\Share\IShare;
@@ -41,6 +43,7 @@ public function __construct(
4143
private KnownUserService $knownUserService,
4244
private IUserSession $userSession,
4345
private IEmailValidator $emailValidator,
46+
private IUserManager $userManager,
4447
private mixed $shareWithGroupOnlyExcludeGroupsList,
4548
private int $shareType,
4649
) {
@@ -71,6 +74,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
7174
}
7275

7376
$currentUserId = $this->userSession->getUser()->getUID();
77+
$userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
7478

7579
$result = $userResults = ['wide' => [], 'exact' => []];
7680
$userType = new SearchResultType('users');
@@ -113,26 +117,16 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
113117
$exactEmailMatch = strtolower($emailAddress) === $lowerSearch;
114118

115119
if (isset($contact['isLocalSystemBook'])) {
116-
if ($this->shareWithGroupOnly) {
117-
/*
118-
* Check if the user may share with the user associated with the e-mail of the just found contact
119-
*/
120-
$userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
121-
122-
// ShareWithGroupOnly filtering
123-
$userGroups = array_diff($userGroups, $this->shareWithGroupOnlyExcludeGroupsList);
124-
125-
$found = false;
126-
foreach ($userGroups as $userGroup) {
127-
if ($this->groupManager->isInGroup($contact['UID'], $userGroup)) {
128-
$found = true;
129-
break;
130-
}
131-
}
132-
if (!$found) {
133-
continue;
134-
}
120+
$contactUser = $this->userManager->get($contact['UID']);
121+
if ($contactUser === null) {
122+
continue;
135123
}
124+
$contactGroups = $this->groupManager->getUserGroupIds($contactUser);
125+
126+
if ($this->shareWithGroupOnly && array_intersect($contactGroups, array_diff($userGroups, $this->shareWithGroupOnlyExcludeGroupsList)) === []) {
127+
continue;
128+
}
129+
136130
if ($exactEmailMatch && $this->shareeEnumerationFullMatch) {
137131
try {
138132
$cloud = $this->cloudIdManager->resolveCloudId($contact['CLOUD'][0] ?? '');
@@ -174,14 +168,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
174168
}
175169

176170
if (!$addToWide && $this->shareeEnumerationInGroupOnly) {
177-
$addToWide = false;
178-
$userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
179-
foreach ($userGroups as $userGroup) {
180-
if ($this->groupManager->isInGroup($contact['UID'], $userGroup)) {
181-
$addToWide = true;
182-
break;
183-
}
184-
}
171+
$addToWide = array_intersect($contactGroups, $userGroups) !== [];
185172
}
186173
if ($addToWide && !$this->isCurrentUser($cloud) && !$searchResult->hasResult($userType, $cloud->getUser())) {
187174
if ($this->shareType === IShare::TYPE_USER) {
@@ -247,7 +234,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
247234
}
248235

249236
if ($this->shareType === IShare::TYPE_EMAIL
250-
&& !$searchResult->hasExactIdMatch($emailType) && $this->emailValidator->isValid($search)) {
237+
&& !$searchResult->hasExactIdMatch($emailType) && $this->emailValidator->isValid($search)) {
251238
$result['exact'][] = [
252239
'label' => $search,
253240
'uuid' => $search,

‎lib/private/Collaboration/Collaborators/UserByMailPlugin.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCP\Federation\ICloudIdManager;
1414
use OCP\IConfig;
1515
use OCP\IGroupManager;
16+
use OCP\IUserManager;
1617
use OCP\IUserSession;
1718
use OCP\Mail\IEmailValidator;
1819
use OCP\Share\IShare;
@@ -30,6 +31,7 @@ public function __construct(
3031
KnownUserService $knownUserService,
3132
IUserSession $userSession,
3233
IEmailValidator $emailValidator,
34+
IUserManager $userManager,
3335
mixed $shareWithGroupOnlyExcludeGroupsList = [],
3436
) {
3537
parent::__construct(
@@ -40,6 +42,7 @@ public function __construct(
4042
$knownUserService,
4143
$userSession,
4244
$emailValidator,
45+
$userManager,
4346
$shareWithGroupOnlyExcludeGroupsList,
4447
IShare::TYPE_USER,
4548
);

‎tests/lib/Collaboration/Collaborators/MailPluginTest.php‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class MailPluginTest extends TestCase {
3939
protected IGroupManager&MockObject $groupManager;
4040
protected KnownUserService&MockObject $knownUserService;
4141
protected IUserSession&MockObject $userSession;
42+
protected IUserManager&MockObject $userManager;
4243

4344
#[\Override]
4445
protected function setUp(): void {
@@ -49,6 +50,17 @@ protected function setUp(): void {
4950
$this->groupManager = $this->createMock(IGroupManager::class);
5051
$this->knownUserService = $this->createMock(KnownUserService::class);
5152
$this->userSession = $this->createMock(IUserSession::class);
53+
$this->userManager = $this->createMock(IUserManager::class);
54+
$this->userManager
55+
->method('get')
56+
->willReturnCallback(function (string $uid): IUser {
57+
$user = $this->createMock(IUser::class);
58+
$user
59+
->method('getUID')
60+
->willReturn($uid);
61+
62+
return $user;
63+
});
5264
$this->cloudIdManager = new CloudIdManager(
5365
$this->createMock(ICacheFactory::class),
5466
$this->createMock(IEventDispatcher::class),
@@ -69,6 +81,7 @@ public function instantiatePlugin(int $shareType) {
6981
$this->knownUserService,
7082
$this->userSession,
7183
$this->getEmailValidatorWithStrictEmailCheck(),
84+
$this->userManager,
7285
[],
7386
$shareType,
7487
);
@@ -727,7 +740,7 @@ public static function dataSearchUser(): array {
727740
]
728741
],
729742
false,
730-
['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)','value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
743+
['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
731744
true,
732745
false,
733746
],
@@ -872,12 +885,6 @@ function ($appName, $key, $default) {
872885
return $userToGroupMapping[$user->getUID()];
873886
});
874887

875-
$this->groupManager->expects($this->any())
876-
->method('isInGroup')
877-
->willReturnCallback(function ($userId, $group) use ($userToGroupMapping) {
878-
return in_array($group, $userToGroupMapping[$userId]);
879-
});
880-
881888
$moreResults = $this->plugin->search($searchTerm, 2, 0, $this->searchResult);
882889
$result = $this->searchResult->asArray();
883890

@@ -942,7 +949,7 @@ public static function dataSearchEmailGroupsOnly(): array {
942949
'UID' => 'User',
943950
]
944951
],
945-
['emails' => [], 'exact' => ['emails' => [['label' => 'test@example.com', 'uuid' => 'test@example.com', 'value' => ['shareType' => IShare::TYPE_EMAIL,'shareWith' => 'test@example.com']]]]],
952+
['emails' => [], 'exact' => ['emails' => [['label' => 'test@example.com', 'uuid' => 'test@example.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@example.com']]]]],
946953
false,
947954
false,
948955
[
@@ -996,12 +1003,6 @@ function ($appName, $key, $default) {
9961003
return $userToGroupMapping[$user->getUID()];
9971004
});
9981005

999-
$this->groupManager->expects($this->any())
1000-
->method('isInGroup')
1001-
->willReturnCallback(function ($userId, $group) use ($userToGroupMapping) {
1002-
return in_array($group, $userToGroupMapping[$userId]);
1003-
});
1004-
10051006
$moreResults = $this->plugin->search($searchTerm, 2, 0, $this->searchResult);
10061007
$result = $this->searchResult->asArray();
10071008

@@ -1024,7 +1025,7 @@ public static function dataSearchUserGroupsOnly(): array {
10241025
'UID' => 'User',
10251026
]
10261027
],
1027-
['users' => [['label' => 'User (test@example.com)', 'uuid' => 'User', 'name' => 'User', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'],'shareWithDisplayNameUnique' => 'test@example.com',]], 'exact' => ['users' => []]],
1028+
['users' => [['label' => 'User (test@example.com)', 'uuid' => 'User', 'name' => 'User', 'value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com',]], 'exact' => ['users' => []]],
10281029
false,
10291030
false,
10301031
[

0 commit comments

Comments
 (0)