Skip to content

Commit a7dc492

Browse files
susnuxsalmart-dev
susnux
and
salmart-dev
committed
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>
1 parent 556e9ec commit a7dc492

7 files changed

Lines changed: 149 additions & 113 deletions

File tree

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,15 @@
115115
value="yes"
116116
type="radio"
117117
@update:modelValue="onUpdateExcludeGroups">
118-
{{ t('settings', 'Exclude some groups') }}
118+
{{ t('settings', 'Exclude some groups (block list)') }}
119119
</NcCheckboxRadioSwitch>
120120
<NcCheckboxRadioSwitch
121121
v-model="settings.excludeGroups"
122122
name="excludeGroups"
123123
value="allow"
124124
type="radio"
125125
@update:modelValue="onUpdateExcludeGroups">
126-
{{ t('settings', 'Allow some groups') }}
126+
{{ t('settings', 'Allow some groups only (allow list)') }}
127127
</NcCheckboxRadioSwitch>
128128
<div v-show="settings.excludeGroups !== 'no'" class="sharing__labeled-entry sharing__input">
129129
<NcSettingsSelectGroup
@@ -133,7 +133,15 @@
133133
:label="settings.excludeGroups === 'allow' ? t('settings', 'Groups allowed to share') : t('settings', 'Groups excluded from sharing')"
134134
:disabled="settings.excludeGroups === 'no'"
135135
style="width: 100%" />
136-
<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>
136+
<em id="settings-sharing-excluded-groups-desc">
137+
<template v-if="settings.excludeGroups === 'allow'">
138+
{{ t('settings', 'Only members of at least one of these groups will be able to initiate shares.') }}
139+
{{ t('settings', 'Others will be blocked, but are still able to receive shares.') }}
140+
</template>
141+
<template v-else>
142+
{{ 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.') }}
143+
</template>
144+
</em>
137145
</div>
138146
</div>
139147

‎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 & 24 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

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

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

180181
$selfGroups = $this->groupManager->getUserGroupIds($self);
181-
182-
if ($excludeGroups && $excludeGroups !== 'no') {
183-
$excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', '');
184-
$decodedExcludeGroups = json_decode($excludedGroups, true);
185-
$excludeGroupsList = $decodedExcludeGroups ?? [];
186-
187-
if ($excludeGroups !== 'allow') {
188-
if (count($selfGroups) > 0 && count(array_diff($selfGroups, $excludeGroupsList)) === 0) {
189-
// all the groups of the current user are excluded -> filter all local users
190-
$skipLocal = true;
191-
}
192-
} else {
193-
$skipLocal = true;
194-
if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) {
195-
// a group of the current user is allowed -> do not filter all local users
196-
$skipLocal = false;
197-
}
198-
}
199-
}
200-
201182
// ownGroupsOnly : some groups may be excluded
202183
if ($ownGroupsOnly) {
203184
$excludeGroupsFromOwnGroups = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members_exclude_group_list', '');

‎lib/private/Share20/ShareDisableChecker.php‎

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

7866
$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;
@@ -67,6 +68,7 @@ protected function setUp(): void {
6768
$this->groupManager,
6869
$this->knownUserService,
6970
$this->l10nFactory,
71+
new ShareDisableChecker($this->config, $this->userManager, $this->groupManager),
7072
);
7173
}
7274

@@ -88,7 +90,7 @@ public function testGetContactsWithoutFilter(): void {
8890
],
8991
],
9092
]);
91-
$user->expects($this->exactly(2))
93+
$user->expects($this->exactly(3))
9294
->method('getUID')
9395
->willReturn('user123');
9496

@@ -118,7 +120,7 @@ public function testGetContactsHidesOwnEntry(): void {
118120
],
119121
],
120122
]);
121-
$user->expects($this->exactly(2))
123+
$user->expects($this->exactly(3))
122124
->method('getUID')
123125
->willReturn('user123');
124126

@@ -150,7 +152,7 @@ public function testGetContactsWithoutBinaryImage(): void {
150152
'PHOTO' => base64_encode('photophotophoto'),
151153
],
152154
]);
153-
$user->expects($this->exactly(2))
155+
$user->expects($this->exactly(3))
154156
->method('getUID')
155157
->willReturn('user123');
156158

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

@@ -196,18 +198,18 @@ public static function dataGetContactsWhenUserIsInExcludeGroups(): array {
196198
['yes', '["excludedGroup1"]', ['anotherGroup1'], ['user123', 'user12345']],
197199
['yes', '["excludedGroup1"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
198200
['yes', '["excludedGroup1"]', ['excludedGroup1'], []],
199-
['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']],
200-
['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
201+
['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], []],
202+
['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []],
201203
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', [], ['user123', 'user12345']],
202204
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1'], ['user123', 'user12345']],
203205
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
204206
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1'], []],
205207
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup2'], []],
206208
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3'], []],
207209
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1', 'excludedGroup2', 'excludedGroup3'], []],
208-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']],
209-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
210-
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']],
210+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], []],
211+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], []],
212+
['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []],
211213
['allow', '[]', [], []],
212214
['allow', '["allowedGroup1"]', [], []],
213215
['allow', '["allowedGroup1"]', ['anotherGroup1'], []],
@@ -244,11 +246,15 @@ public function testGetContactsWhenUserIsInExcludeGroups(string $excludeGroups,
244246

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

251-
$this->groupManager->expects($this->once())
253+
$this->userManager->method('get')
254+
->with('user001')
255+
->willReturn($currentUser);
256+
257+
$this->groupManager->expects($this->exactly(2))
252258
->method('getUserGroupIds')
253259
->with($this->equalTo($currentUser))
254260
->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']);
@@ -332,7 +342,7 @@ public function testGetContactsOnlyShareIfInTheSameGroup(): void {
332342

333343
/** @var IUser|MockObject $currentUser */
334344
$currentUser = $this->createMock(IUser::class);
335-
$currentUser->expects($this->exactly(2))
345+
$currentUser->expects($this->exactly(3))
336346
->method('getUID')
337347
->willReturn('user001');
338348

@@ -405,7 +415,7 @@ public function testGetContactsOnlyEnumerateIfInTheSameGroup(): void {
405415

406416
/** @var IUser|MockObject $currentUser */
407417
$currentUser = $this->createMock(IUser::class);
408-
$currentUser->expects($this->exactly(2))
418+
$currentUser->expects($this->exactly(3))
409419
->method('getUID')
410420
->willReturn('user001');
411421

@@ -477,7 +487,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatch(): void {
477487

478488
/** @var IUser|MockObject $currentUser */
479489
$currentUser = $this->createMock(IUser::class);
480-
$currentUser->expects($this->exactly(2))
490+
$currentUser->expects($this->exactly(3))
481491
->method('getUID')
482492
->willReturn('user001');
483493

@@ -536,7 +546,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatchWithOwnGroupsOnly():
536546

537547
/** @var IUser|MockObject $currentUser */
538548
$currentUser = $this->createMock(IUser::class);
539-
$currentUser->expects($this->exactly(2))
549+
$currentUser->expects($this->exactly(3))
540550
->method('getUID')
541551
->willReturn('user001');
542552

@@ -615,7 +625,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroup(): void {
615625

616626
/** @var IUser|MockObject $currentUser */
617627
$currentUser = $this->createMock(IUser::class);
618-
$currentUser->expects($this->exactly(2))
628+
$currentUser->expects($this->exactly(3))
619629
->method('getUID')
620630
->willReturn('user001');
621631

@@ -689,7 +699,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroupInOwnGroupsOnl
689699

690700
/** @var IUser|MockObject $currentUser */
691701
$currentUser = $this->createMock(IUser::class);
692-
$currentUser->expects($this->exactly(2))
702+
$currentUser->expects($this->exactly(3))
693703
->method('getUID')
694704
->willReturn('user001');
695705

0 commit comments

Comments
 (0)