Skip to content

Commit 5e9da2a

Browse files
susnuxbackportbot[bot]
susnux
authored and
backportbot[bot]
committed
fix(sharing): properly frame block list
fix(sharing): properly frame block list Co-authored-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de> [skip ci]
1 parent 568b4ef commit 5e9da2a

7 files changed

Lines changed: 146 additions & 90 deletions

File tree

‎apps/settings/src/components/AdminSettingsSharingForm.vue‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,15 @@
120120
:label="settings.excludeGroups === 'allow' ? t('settings', 'Groups allowed to share') : t('settings', 'Groups excluded from sharing')"
121121
:disabled="settings.excludeGroups === 'no'"
122122
style="width: 100%" />
123-
<em id="settings-sharing-excluded-groups-desc">{{ t('settings', 'Not allowed groups will still be able to receive shares, but not to initiate them.') }}</em>
123+
<em id="settings-sharing-excluded-groups-desc">
124+
<template v-if="settings.excludeGroups === 'allow'">
125+
{{ t('settings', 'Only members of at least one of these groups will be able to initiate shares.') }}
126+
{{ t('settings', 'Others will be blocked, but are still able to receive shares.') }}
127+
</template>
128+
<template v-else>
129+
{{ t('settings', 'A member of at least one of these groups will not be able to initiate new shares, but will still be able to receive shares.') }}
130+
</template>
131+
</em>
124132
</div>
125133
</div>
126134

‎build/integration/features/contacts-menu.feature‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ Feature: contacts-menu
103103
And searching for contacts matching with "test"
104104
Then the list of searched contacts has "0" contacts
105105

106-
Scenario: users can be searched by display name when searcher belongs to both a group excluded from sharing and another group
106+
Scenario: users can not be searched by display name when searcher belongs to both a group excluded from sharing and another group
107107
Given user "user0" exists
108108
And group "ExcludedGroup" exists
109109
And user "user0" belongs to group "ExcludedGroup"
@@ -118,13 +118,45 @@ Feature: contacts-menu
118118
| value | Test name |
119119
When Logging in using web as "user0"
120120
And searching for contacts matching with "test"
121+
Then the list of searched contacts has "0" contacts
122+
123+
Scenario: users can not be searched by email when searcher belongs to both a group excluded from sharing and another group
124+
Given user "user0" exists
125+
And group "ExcludedGroup" exists
126+
And user "user0" belongs to group "ExcludedGroup"
127+
And group "AnotherGroup" exists
128+
And user "user0" belongs to group "AnotherGroup"
129+
And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
130+
And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup"
131+
And user "user1" exists
132+
And As an "admin"
133+
And sending "PUT" to "/cloud/users/user1" with
134+
| key | email |
135+
| value | test@example.com |
136+
When Logging in using web as "user0"
137+
And searching for contacts matching with "test"
138+
Then the list of searched contacts has "0" contacts
139+
140+
Scenario: users can be searched by display name when searcher does not belong to a group excluded from sharing
141+
Given user "user0" exists
142+
And group "ExcludedGroup" exists
143+
And group "AnotherGroup" exists
144+
And user "user0" belongs to group "AnotherGroup"
145+
And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
146+
And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup"
147+
And user "user1" exists
148+
And As an "admin"
149+
And sending "PUT" to "/cloud/users/user1" with
150+
| key | displayname |
151+
| value | Test name |
152+
When Logging in using web as "user0"
153+
And searching for contacts matching with "test"
121154
Then the list of searched contacts has "1" contacts
122155
And searched contact "0" is named "Test name"
123156

124-
Scenario: users can be searched by email when searcher belongs to both a group excluded from sharing and another group
157+
Scenario: users can be searched by email when searcher does not belong to a group excluded from sharing
125158
Given user "user0" exists
126159
And group "ExcludedGroup" exists
127-
And user "user0" belongs to group "ExcludedGroup"
128160
And group "AnotherGroup" exists
129161
And user "user0" belongs to group "AnotherGroup"
130162
And parameter "shareapi_exclude_groups" of app "core" is set to "yes"

‎build/integration/sharees_features/sharees.feature‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,24 @@ Feature: sharees
163163
Then the OCS status code should be "100"
164164
And the HTTP status code should be "200"
165165
And "exact users" sharees returned is empty
166+
And "users" sharees returned is empty
167+
And "exact groups" sharees returned is empty
168+
And "groups" sharees returned is empty
169+
And "exact remotes" sharees returned is empty
170+
And "remotes" sharees returned is empty
171+
172+
Scenario: Search when belonging to a group not excluded from sharing
173+
Given As an "test"
174+
And group "AnotherGroup" exists
175+
And user "test" belongs to group "AnotherGroup"
176+
And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
177+
And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup"
178+
When getting sharees for
179+
| search | sharee |
180+
| itemType | file |
181+
Then the OCS status code should be "100"
182+
And the HTTP status code should be "200"
183+
And "exact users" sharees returned is empty
166184
And "users" sharees returned are
167185
| Sharee1 | 0 | Sharee1 | Sharee1 |
168186
| Sharee2 | 0 | Sharee2 | sharee2@system.com |

‎lib/private/Contacts/ContactsMenu/ContactsStore.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
use OC\KnownUser\KnownUserService;
1111
use OC\Profile\ProfileManager;
12+
use OC\Share20\ShareDisableChecker;
1213
use OCA\UserStatus\Db\UserStatus;
1314
use OCA\UserStatus\Service\StatusService;
1415
use OCP\Contacts\ContactsMenu\IContactsStore;
@@ -38,6 +39,7 @@ public function __construct(
3839
private IGroupManager $groupManager,
3940
private KnownUserService $knownUserService,
4041
private IL10NFactory $l10nFactory,
42+
private ShareDisableChecker $shareDisableChecker,
4143
) {
4244
}
4345

@@ -148,8 +150,8 @@ public function getContacts(IUser $user, ?string $filter, ?int $limit = null, ?i
148150
* Filters the contacts. Applied filters:
149151
* 1. if the `shareapi_allow_share_dialog_user_enumeration` config option is
150152
* enabled it will filter all local users
151-
* 2. if the `shareapi_exclude_groups` config option is enabled and the
152-
* current user is only in excluded groups it will filter all local users.
153+
* 2. if sharing is disabled for the current user by the `shareapi_exclude_groups`
154+
* config option it will filter all local users.
153155
* 3. if the `shareapi_only_share_with_group_members` config option is
154156
* enabled it will filter all users which doesn't have a common group
155157
* with the current user.
@@ -169,10 +171,9 @@ private function filterContacts(
169171
$restrictEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
170172
$restrictEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
171173
$allowEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes';
172-
$excludeGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no');
173174

174175
// whether to filter out local users
175-
$skipLocal = false;
176+
$skipLocal = $this->shareDisableChecker->sharingDisabledForUser($self->getUID());
176177
// whether to filter out all users which don't have a common group as the current user
177178
$ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
178179

‎lib/private/Share20/ShareDisableChecker.php‎

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -49,29 +49,17 @@ public function sharingDisabledForUser(?string $userId): bool {
4949
return false;
5050
}
5151
$usersGroups = $this->groupManager->getUserGroupIds($user);
52-
if ($excludeGroups !== 'allow') {
53-
if (!empty($usersGroups)) {
54-
$remainingGroups = array_diff($usersGroups, $excludedGroups);
55-
// if the user is only in groups which are disabled for sharing then
56-
// sharing is also disabled for the user
57-
if (empty($remainingGroups)) {
58-
$this->sharingDisabledForUsersCache[$userId] = true;
59-
return true;
60-
}
61-
}
62-
} else {
63-
if (!empty($usersGroups)) {
64-
$remainingGroups = array_intersect($usersGroups, $excludedGroups);
65-
// if the user is in any group which is allowed for sharing then
66-
// sharing is also allowed for the user
67-
if (!empty($remainingGroups)) {
68-
$this->sharingDisabledForUsersCache[$userId] = false;
69-
return false;
70-
}
71-
}
72-
$this->sharingDisabledForUsersCache[$userId] = true;
73-
return true;
74-
}
52+
$intersectingGroups = array_intersect($usersGroups, $excludedGroups);
53+
54+
// 1. If the user is in a group which is disabled for sharing then
55+
// sharing is also disabled for the user.
56+
// 2. If the user is in a group which is allowed for sharing then
57+
// sharing is also allowed for the user.
58+
$isInList = $intersectingGroups !== [];
59+
$isBlockList = $excludeGroups !== 'allow';
60+
$sharingDisabled = $isBlockList ? $isInList : !$isInList;
61+
$this->sharingDisabledForUsersCache[$userId] = $sharingDisabled;
62+
return $sharingDisabled;
7563
}
7664

7765
$this->sharingDisabledForUsersCache[$userId] = false;

‎tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OC\Contacts\ContactsMenu\ContactsStore;
1313
use OC\KnownUser\KnownUserService;
1414
use OC\Profile\ProfileManager;
15+
use OC\Share20\ShareDisableChecker;
1516
use OCA\UserStatus\Db\UserStatus;
1617
use OCA\UserStatus\Service\StatusService;
1718
use OCP\Contacts\IManager;
@@ -66,6 +67,7 @@ protected function setUp(): void {
6667
$this->groupManager,
6768
$this->knownUserService,
6869
$this->l10nFactory,
70+
new ShareDisableChecker($this->config, $this->userManager, $this->groupManager),
6971
);
7072
}
7173

@@ -87,7 +89,7 @@ public function testGetContactsWithoutFilter(): void {
8789
],
8890
],
8991
]);
90-
$user->expects($this->exactly(2))
92+
$user->expects($this->exactly(3))
9193
->method('getUID')
9294
->willReturn('user123');
9395

@@ -117,7 +119,7 @@ public function testGetContactsHidesOwnEntry(): void {
117119
],
118120
],
119121
]);
120-
$user->expects($this->exactly(2))
122+
$user->expects($this->exactly(3))
121123
->method('getUID')
122124
->willReturn('user123');
123125

@@ -149,7 +151,7 @@ public function testGetContactsWithoutBinaryImage(): void {
149151
'PHOTO' => base64_encode('photophotophoto'),
150152
],
151153
]);
152-
$user->expects($this->exactly(2))
154+
$user->expects($this->exactly(3))
153155
->method('getUID')
154156
->willReturn('user123');
155157

@@ -178,7 +180,7 @@ public function testGetContactsWithoutAvatarURI(): void {
178180
'PHOTO' => 'VALUE=uri:https://photo',
179181
],
180182
]);
181-
$user->expects($this->exactly(2))
183+
$user->expects($this->exactly(3))
182184
->method('getUID')
183185
->willReturn('user123');
184186

@@ -195,18 +197,18 @@ public static function dataGetContactsWhenUserIsInExcludeGroups(): array {
195197
['yes', '["excludedGroup1"]', ['anotherGroup1'], ['user123', 'user12345']],
196198
['yes', '["excludedGroup1"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
197199
['yes', '["excludedGroup1"]', ['excludedGroup1'], []],
198-
['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']],
199-
['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
200+
['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], []],
201+
['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []],
200202
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', [], ['user123', 'user12345']],
201203
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1'], ['user123', 'user12345']],
202204
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
203205
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1'], []],
204206
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup2'], []],
205207
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3'], []],
206208
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1', 'excludedGroup2', 'excludedGroup3'], []],
207-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']],
208-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
209-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
209+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], []],
210+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], []],
211+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []],
210212
['allow', '[]', [], []],
211213
['allow', '["allowedGroup1"]', [], []],
212214
['allow', '["allowedGroup1"]', ['anotherGroup1'], []],
@@ -243,11 +245,15 @@ public function testGetContactsWhenUserIsInExcludeGroups(string $excludeGroups,
243245

244246
/** @var IUser|MockObject $currentUser */
245247
$currentUser = $this->createMock(IUser::class);
246-
$currentUser->expects($this->exactly(2))
248+
$currentUser->expects($this->exactly(3))
247249
->method('getUID')
248250
->willReturn('user001');
249251

250-
$this->groupManager->expects($this->once())
252+
$this->userManager->method('get')
253+
->with('user001')
254+
->willReturn($currentUser);
255+
256+
$this->groupManager->expects($this->exactly(2))
251257
->method('getUserGroupIds')
252258
->with($this->equalTo($currentUser))
253259
->willReturn($currentUserGroupIds);
@@ -290,11 +296,15 @@ public function testGetContactsOnlyShareIfInTheSameGroupWhenUserIsInExcludeGroup
290296

291297
/** @var IUser|MockObject $currentUser */
292298
$currentUser = $this->createMock(IUser::class);
293-
$currentUser->expects($this->exactly(2))
299+
$currentUser->expects($this->exactly(3))
294300
->method('getUID')
295301
->willReturn('user001');
296302

297-
$this->groupManager->expects($this->once())
303+
$this->userManager->method('get')
304+
->with('user001')
305+
->willReturn($currentUser);
306+
307+
$this->groupManager->expects($this->exactly(2))
298308
->method('getUserGroupIds')
299309
->with($this->equalTo($currentUser))
300310
->willReturn(['group1', 'group2', 'group3']);
@@ -334,7 +344,7 @@ public function testGetContactsOnlyShareIfInTheSameGroup(): void {
334344

335345
/** @var IUser|MockObject $currentUser */
336346
$currentUser = $this->createMock(IUser::class);
337-
$currentUser->expects($this->exactly(2))
347+
$currentUser->expects($this->exactly(3))
338348
->method('getUID')
339349
->willReturn('user001');
340350

@@ -407,7 +417,7 @@ public function testGetContactsOnlyEnumerateIfInTheSameGroup(): void {
407417

408418
/** @var IUser|MockObject $currentUser */
409419
$currentUser = $this->createMock(IUser::class);
410-
$currentUser->expects($this->exactly(2))
420+
$currentUser->expects($this->exactly(3))
411421
->method('getUID')
412422
->willReturn('user001');
413423

@@ -479,7 +489,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatch(): void {
479489

480490
/** @var IUser|MockObject $currentUser */
481491
$currentUser = $this->createMock(IUser::class);
482-
$currentUser->expects($this->exactly(2))
492+
$currentUser->expects($this->exactly(3))
483493
->method('getUID')
484494
->willReturn('user001');
485495

@@ -538,7 +548,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatchWithOwnGroupsOnly():
538548

539549
/** @var IUser|MockObject $currentUser */
540550
$currentUser = $this->createMock(IUser::class);
541-
$currentUser->expects($this->exactly(2))
551+
$currentUser->expects($this->exactly(3))
542552
->method('getUID')
543553
->willReturn('user001');
544554

@@ -617,7 +627,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroup(): void {
617627

618628
/** @var IUser|MockObject $currentUser */
619629
$currentUser = $this->createMock(IUser::class);
620-
$currentUser->expects($this->exactly(2))
630+
$currentUser->expects($this->exactly(3))
621631
->method('getUID')
622632
->willReturn('user001');
623633

@@ -691,7 +701,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroupInOwnGroupsOnl
691701

692702
/** @var IUser|MockObject $currentUser */
693703
$currentUser = $this->createMock(IUser::class);
694-
$currentUser->expects($this->exactly(2))
704+
$currentUser->expects($this->exactly(3))
695705
->method('getUID')
696706
->willReturn('user001');
697707

0 commit comments

Comments
 (0)