Skip to content

Commit a36eefa

Browse files
susnuxsusnux
authored
Merge pull request #63934 from nextcloud/backport/63828/stable34
[stable34] fix(sharing): properly frame block list
2 parents 3bec628 + 5320e68 commit a36eefa

9 files changed

Lines changed: 152 additions & 116 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 |

‎dist/settings-vue-settings-admin-sharing.js‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎dist/settings-vue-settings-admin-sharing.js.map‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎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
@@ -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;
@@ -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);
@@ -291,11 +297,15 @@ public function testGetContactsOnlyShareIfInTheSameGroupWhenUserIsInExcludeGroup
291297

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

298-
$this->groupManager->expects($this->once())
304+
$this->userManager->method('get')
305+
->with('user001')
306+
->willReturn($currentUser);
307+
308+
$this->groupManager->expects($this->exactly(2))
299309
->method('getUserGroupIds')
300310
->with($this->equalTo($currentUser))
301311
->willReturn(['group1', 'group2', 'group3']);
@@ -335,7 +345,7 @@ public function testGetContactsOnlyShareIfInTheSameGroup(): void {
335345

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

@@ -408,7 +418,7 @@ public function testGetContactsOnlyEnumerateIfInTheSameGroup(): void {
408418

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

@@ -480,7 +490,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatch(): void {
480490

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

@@ -539,7 +549,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatchWithOwnGroupsOnly():
539549

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

@@ -618,7 +628,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroup(): void {
618628

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

@@ -692,7 +702,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroupInOwnGroupsOnl
692702

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

0 commit comments

Comments
 (0)