diff --git a/apps/settings/src/components/AdminSettingsSharingForm.vue b/apps/settings/src/components/AdminSettingsSharingForm.vue index f0b4672e88125..89fa03525adfa 100644 --- a/apps/settings/src/components/AdminSettingsSharingForm.vue +++ b/apps/settings/src/components/AdminSettingsSharingForm.vue @@ -115,7 +115,7 @@ value="yes" type="radio" @update:modelValue="onUpdateExcludeGroups"> - {{ t('settings', 'Exclude some groups') }} + {{ t('settings', 'Exclude some groups (block list)') }} - {{ t('settings', 'Allow some groups') }} + {{ t('settings', 'Allow some groups only (allow list)') }}
- {{ t('settings', 'Not allowed groups will still be able to receive shares, but not to initiate them.') }} + + + +
diff --git a/build/integration/features/contacts-menu.feature b/build/integration/features/contacts-menu.feature index c771a3fe2f1bf..077a33d14a13a 100644 --- a/build/integration/features/contacts-menu.feature +++ b/build/integration/features/contacts-menu.feature @@ -103,7 +103,7 @@ Feature: contacts-menu And searching for contacts matching with "test" Then the list of searched contacts has "0" contacts - Scenario: users can be searched by display name when searcher belongs to both a group excluded from sharing and another group + Scenario: users can not be searched by display name when searcher belongs to both a group excluded from sharing and another group Given user "user0" exists And group "ExcludedGroup" exists And user "user0" belongs to group "ExcludedGroup" @@ -118,13 +118,45 @@ Feature: contacts-menu | value | Test name | When Logging in using web as "user0" And searching for contacts matching with "test" + Then the list of searched contacts has "0" contacts + + Scenario: users can not be searched by email when searcher belongs to both a group excluded from sharing and another group + Given user "user0" exists + And group "ExcludedGroup" exists + And user "user0" belongs to group "ExcludedGroup" + And group "AnotherGroup" exists + And user "user0" belongs to group "AnotherGroup" + And parameter "shareapi_exclude_groups" of app "core" is set to "yes" + And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup" + And user "user1" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | email | + | value | test@example.com | + When Logging in using web as "user0" + And searching for contacts matching with "test" + Then the list of searched contacts has "0" contacts + + Scenario: users can be searched by display name when searcher does not belong to a group excluded from sharing + Given user "user0" exists + And group "ExcludedGroup" exists + And group "AnotherGroup" exists + And user "user0" belongs to group "AnotherGroup" + And parameter "shareapi_exclude_groups" of app "core" is set to "yes" + And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup" + And user "user1" exists + And As an "admin" + And sending "PUT" to "/cloud/users/user1" with + | key | displayname | + | value | Test name | + When Logging in using web as "user0" + And searching for contacts matching with "test" Then the list of searched contacts has "1" contacts And searched contact "0" is named "Test name" - Scenario: users can be searched by email when searcher belongs to both a group excluded from sharing and another group + Scenario: users can be searched by email when searcher does not belong to a group excluded from sharing Given user "user0" exists And group "ExcludedGroup" exists - And user "user0" belongs to group "ExcludedGroup" And group "AnotherGroup" exists And user "user0" belongs to group "AnotherGroup" And parameter "shareapi_exclude_groups" of app "core" is set to "yes" diff --git a/build/integration/sharees_features/sharees.feature b/build/integration/sharees_features/sharees.feature index 97994472c0cbe..1150ccb016893 100644 --- a/build/integration/sharees_features/sharees.feature +++ b/build/integration/sharees_features/sharees.feature @@ -163,6 +163,24 @@ Feature: sharees Then the OCS status code should be "100" And the HTTP status code should be "200" And "exact users" sharees returned is empty + And "users" sharees returned is empty + And "exact groups" sharees returned is empty + And "groups" sharees returned is empty + And "exact remotes" sharees returned is empty + And "remotes" sharees returned is empty + + Scenario: Search when belonging to a group not excluded from sharing + Given As an "test" + And group "AnotherGroup" exists + And user "test" belongs to group "AnotherGroup" + And parameter "shareapi_exclude_groups" of app "core" is set to "yes" + And parameter "shareapi_exclude_groups_list" of app "core" is set to "ExcludedGroup" + When getting sharees for + | search | sharee | + | itemType | file | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And "exact users" sharees returned is empty And "users" sharees returned are | Sharee1 | 0 | Sharee1 | Sharee1 | | Sharee2 | 0 | Sharee2 | sharee2@system.com | diff --git a/lib/private/Contacts/ContactsMenu/ContactsStore.php b/lib/private/Contacts/ContactsMenu/ContactsStore.php index 0a5cef3cf5151..59fa0ed2b8cdf 100644 --- a/lib/private/Contacts/ContactsMenu/ContactsStore.php +++ b/lib/private/Contacts/ContactsMenu/ContactsStore.php @@ -9,6 +9,7 @@ use OC\KnownUser\KnownUserService; use OC\Profile\ProfileManager; +use OC\Share20\ShareDisableChecker; use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\Service\StatusService; use OCP\Contacts\ContactsMenu\IContactsStore; @@ -38,6 +39,7 @@ public function __construct( private IGroupManager $groupManager, private KnownUserService $knownUserService, private IL10NFactory $l10nFactory, + private ShareDisableChecker $shareDisableChecker, ) { } @@ -149,8 +151,8 @@ public function getContacts(IUser $user, ?string $filter, ?int $limit = null, ?i * Filters the contacts. Applied filters: * 1. if the `shareapi_allow_share_dialog_user_enumeration` config option is * enabled it will filter all local users - * 2. if the `shareapi_exclude_groups` config option is enabled and the - * current user is only in excluded groups it will filter all local users. + * 2. if sharing is disabled for the current user by the `shareapi_exclude_groups` + * config option it will filter all local users. * 3. if the `shareapi_only_share_with_group_members` config option is * enabled it will filter all users which doesn't have a common group * with the current user. @@ -170,34 +172,13 @@ private function filterContacts( $restrictEnumerationGroup = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; $restrictEnumerationPhone = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes'; $allowEnumerationFullMatch = $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_full_match', 'yes') === 'yes'; - $excludeGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no'); // whether to filter out local users - $skipLocal = false; + $skipLocal = $this->shareDisableChecker->sharingDisabledForUser($self->getUID()); // whether to filter out all users which don't have a common group as the current user $ownGroupsOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; $selfGroups = $this->groupManager->getUserGroupIds($self); - - if ($excludeGroups && $excludeGroups !== 'no') { - $excludedGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); - $decodedExcludeGroups = json_decode($excludedGroups, true); - $excludeGroupsList = $decodedExcludeGroups ?? []; - - if ($excludeGroups !== 'allow') { - if (count($selfGroups) > 0 && count(array_diff($selfGroups, $excludeGroupsList)) === 0) { - // all the groups of the current user are excluded -> filter all local users - $skipLocal = true; - } - } else { - $skipLocal = true; - if (count(array_intersect($excludeGroupsList, $selfGroups)) !== 0) { - // a group of the current user is allowed -> do not filter all local users - $skipLocal = false; - } - } - } - // ownGroupsOnly : some groups may be excluded if ($ownGroupsOnly) { $excludeGroupsFromOwnGroups = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members_exclude_group_list', ''); diff --git a/lib/private/Share20/ShareDisableChecker.php b/lib/private/Share20/ShareDisableChecker.php index 1a549989d350d..1129cfeb1d3d5 100644 --- a/lib/private/Share20/ShareDisableChecker.php +++ b/lib/private/Share20/ShareDisableChecker.php @@ -50,29 +50,17 @@ public function sharingDisabledForUser(?string $userId): bool { return false; } $usersGroups = $this->groupManager->getUserGroupIds($user); - if ($excludeGroups !== 'allow') { - if (!empty($usersGroups)) { - $remainingGroups = array_diff($usersGroups, $excludedGroups); - // if the user is only in groups which are disabled for sharing then - // sharing is also disabled for the user - if (empty($remainingGroups)) { - $this->sharingDisabledForUsersCache[$userId] = true; - return true; - } - } - } else { - if (!empty($usersGroups)) { - $remainingGroups = array_intersect($usersGroups, $excludedGroups); - // if the user is in any group which is allowed for sharing then - // sharing is also allowed for the user - if (!empty($remainingGroups)) { - $this->sharingDisabledForUsersCache[$userId] = false; - return false; - } - } - $this->sharingDisabledForUsersCache[$userId] = true; - return true; - } + $intersectingGroups = array_intersect($usersGroups, $excludedGroups); + + // 1. If the user is in a group which is disabled for sharing then + // sharing is also disabled for the user. + // 2. If the user is in a group which is allowed for sharing then + // sharing is also allowed for the user. + $isInList = $intersectingGroups !== []; + $isBlockList = $excludeGroups !== 'allow'; + $sharingDisabled = $isBlockList ? $isInList : !$isInList; + $this->sharingDisabledForUsersCache[$userId] = $sharingDisabled; + return $sharingDisabled; } $this->sharingDisabledForUsersCache[$userId] = false; diff --git a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php index 645167c28d293..b1c553d9e9668 100644 --- a/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php +++ b/tests/lib/Contacts/ContactsMenu/ContactsStoreTest.php @@ -12,6 +12,7 @@ use OC\Contacts\ContactsMenu\ContactsStore; use OC\KnownUser\KnownUserService; use OC\Profile\ProfileManager; +use OC\Share20\ShareDisableChecker; use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\Service\StatusService; use OCP\Contacts\IManager; @@ -67,6 +68,7 @@ protected function setUp(): void { $this->groupManager, $this->knownUserService, $this->l10nFactory, + new ShareDisableChecker($this->config, $this->userManager, $this->groupManager), ); } @@ -88,7 +90,7 @@ public function testGetContactsWithoutFilter(): void { ], ], ]); - $user->expects($this->exactly(2)) + $user->expects($this->exactly(3)) ->method('getUID') ->willReturn('user123'); @@ -118,7 +120,7 @@ public function testGetContactsHidesOwnEntry(): void { ], ], ]); - $user->expects($this->exactly(2)) + $user->expects($this->exactly(3)) ->method('getUID') ->willReturn('user123'); @@ -150,7 +152,7 @@ public function testGetContactsWithoutBinaryImage(): void { 'PHOTO' => base64_encode('photophotophoto'), ], ]); - $user->expects($this->exactly(2)) + $user->expects($this->exactly(3)) ->method('getUID') ->willReturn('user123'); @@ -179,7 +181,7 @@ public function testGetContactsWithoutAvatarURI(): void { 'PHOTO' => 'VALUE=uri:https://photo', ], ]); - $user->expects($this->exactly(2)) + $user->expects($this->exactly(3)) ->method('getUID') ->willReturn('user123'); @@ -196,8 +198,8 @@ public static function dataGetContactsWhenUserIsInExcludeGroups(): array { ['yes', '["excludedGroup1"]', ['anotherGroup1'], ['user123', 'user12345']], ['yes', '["excludedGroup1"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']], ['yes', '["excludedGroup1"]', ['excludedGroup1'], []], - ['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']], - ['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']], + ['yes', '["excludedGroup1"]', ['anotherGroup1', 'excludedGroup1'], []], + ['yes', '["excludedGroup1"]', ['excludedGroup1', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []], ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', [], ['user123', 'user12345']], ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1'], ['user123', 'user12345']], ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']], @@ -205,9 +207,9 @@ public static function dataGetContactsWhenUserIsInExcludeGroups(): array { ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup2'], []], ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3'], []], ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1', 'excludedGroup2', 'excludedGroup3'], []], - ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], ['user123', 'user12345']], - ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']], - ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], ['user123', 'user12345']], + ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup1'], []], + ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['anotherGroup1', 'excludedGroup2', 'anotherGroup2', 'anotherGroup3'], []], + ['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup3', 'anotherGroup1', 'anotherGroup2', 'anotherGroup3'], []], ['allow', '[]', [], []], ['allow', '["allowedGroup1"]', [], []], ['allow', '["allowedGroup1"]', ['anotherGroup1'], []], @@ -244,11 +246,15 @@ public function testGetContactsWhenUserIsInExcludeGroups(string $excludeGroups, /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); - $this->groupManager->expects($this->once()) + $this->userManager->method('get') + ->with('user001') + ->willReturn($currentUser); + + $this->groupManager->expects($this->exactly(2)) ->method('getUserGroupIds') ->with($this->equalTo($currentUser)) ->willReturn($currentUserGroupIds); @@ -290,11 +296,15 @@ public function testGetContactsOnlyShareIfInTheSameGroupWhenUserIsInExcludeGroup /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); - $this->groupManager->expects($this->once()) + $this->userManager->method('get') + ->with('user001') + ->willReturn($currentUser); + + $this->groupManager->expects($this->exactly(2)) ->method('getUserGroupIds') ->with($this->equalTo($currentUser)) ->willReturn(['group1', 'group2', 'group3']); @@ -332,7 +342,7 @@ public function testGetContactsOnlyShareIfInTheSameGroup(): void { /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); @@ -405,7 +415,7 @@ public function testGetContactsOnlyEnumerateIfInTheSameGroup(): void { /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); @@ -477,7 +487,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatch(): void { /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); @@ -536,7 +546,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookMatchWithOwnGroupsOnly(): /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); @@ -615,7 +625,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroup(): void { /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); @@ -689,7 +699,7 @@ public function testGetContactsOnlyEnumerateIfPhoneBookOrSameGroupInOwnGroupsOnl /** @var IUser|MockObject $currentUser */ $currentUser = $this->createMock(IUser::class); - $currentUser->expects($this->exactly(2)) + $currentUser->expects($this->exactly(3)) ->method('getUID') ->willReturn('user001'); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 68bfffe94220c..8dd1af514cf42 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -3032,53 +3032,52 @@ public function testPathCreateChecksContainsNoFolder(): void { self::invokePrivate($this->manager, 'pathCreateChecks', [$path]); } - public static function dataIsSharingDisabledForUser() { - $data = []; - - // No exclude groups - $data[] = ['no', null, null, [], false]; - - // empty exclude / allow list, user no groups - $data[] = ['yes', '', json_encode(['']), [], false]; - $data[] = ['allow', '', json_encode(['']), [], true]; - - // empty exclude / allow list, user groups - $data[] = ['yes', '', json_encode(['']), ['group1', 'group2'], false]; - $data[] = ['allow', '', json_encode(['']), ['group1', 'group2'], true]; - - // Convert old list to json - $data[] = ['yes', 'group1,group2', json_encode(['group1', 'group2']), [], false]; - $data[] = ['allow', 'group1,group2', json_encode(['group1', 'group2']), [], true]; - - // Old list partly groups in common - $data[] = ['yes', 'group1,group2', json_encode(['group1', 'group2']), ['group1', 'group3'], false]; - $data[] = ['allow', 'group1,group2', json_encode(['group1', 'group2']), ['group1', 'group3'], false]; - - // Old list only groups in common - $data[] = ['yes', 'group1,group2', json_encode(['group1', 'group2']), ['group1'], true]; - $data[] = ['allow', 'group1,group2', json_encode(['group1', 'group2']), ['group1'], false]; + public static function dataIsSharingDisabledForUser(): array { + $legacyList = 'group1,group2'; + $jsonList = json_encode(['group1', 'group2']); - // New list partly in common - $data[] = ['yes', json_encode(['group1', 'group2']), null, ['group1', 'group3'], false]; - $data[] = ['allow', json_encode(['group1', 'group2']), null, ['group1', 'group3'], false]; - - // New list only groups in common - $data[] = ['yes', json_encode(['group1', 'group2']), null, ['group2'], true]; - $data[] = ['allow', json_encode(['group1', 'group2']), null, ['group2'], false]; - - return $data; + return [ + // No group restrictions configured + 'no restrictions, user without groups' => ['no', null, null, [], false], + 'no restrictions, user with groups' => ['no', null, null, ['group1'], false], + + // Empty legacy list, converted to json + 'block list, empty list, user without groups' => ['yes', '', json_encode(['']), [], false], + 'allow list, empty list, user without groups' => ['allow', '', json_encode(['']), [], true], + 'block list, empty list, user with groups' => ['yes', '', json_encode(['']), ['group1', 'group2'], false], + 'allow list, empty list, user with groups' => ['allow', '', json_encode(['']), ['group1', 'group2'], true], + + // Legacy comma separated list, converted to json + 'block list, legacy list, user without groups' => ['yes', $legacyList, $jsonList, [], false], + 'allow list, legacy list, user without groups' => ['allow', $legacyList, $jsonList, [], true], + 'block list, legacy list, user not in list' => ['yes', $legacyList, $jsonList, ['group3'], false], + 'allow list, legacy list, user not in list' => ['allow', $legacyList, $jsonList, ['group3'], true], + 'block list, legacy list, user partly in list' => ['yes', $legacyList, $jsonList, ['group1', 'group3'], true], + 'allow list, legacy list, user partly in list' => ['allow', $legacyList, $jsonList, ['group1', 'group3'], false], + 'block list, legacy list, user only in list' => ['yes', $legacyList, $jsonList, ['group1'], true], + 'allow list, legacy list, user only in list' => ['allow', $legacyList, $jsonList, ['group1'], false], + + // Json encoded list + 'block list, user without groups' => ['yes', $jsonList, null, [], false], + 'allow list, user without groups' => ['allow', $jsonList, null, [], true], + 'block list, user not in list' => ['yes', $jsonList, null, ['group3'], false], + 'allow list, user not in list' => ['allow', $jsonList, null, ['group3'], true], + 'block list, user partly in list' => ['yes', $jsonList, null, ['group1', 'group3'], true], + 'allow list, user partly in list' => ['allow', $jsonList, null, ['group1', 'group3'], false], + 'block list, user only in list' => ['yes', $jsonList, null, ['group2'], true], + 'allow list, user only in list' => ['allow', $jsonList, null, ['group2'], false], + ]; } /** - * - * @param string $excludeGroups - * @param string $groupList - * @param string $setList - * @param string[] $groupIds - * @param bool $expected + * @param string $excludeGroups Value of the `shareapi_exclude_groups` setting + * @param ?string $groupList Value of the `shareapi_exclude_groups_list` setting + * @param ?string $setList Expected value the group list is migrated to, or null if no migration is expected + * @param string[] $groupIds Groups of the user + * @param bool $expected Whether sharing is expected to be disabled for the user */ #[DataProvider('dataIsSharingDisabledForUser')] - public function testIsSharingDisabledForUser($excludeGroups, $groupList, $setList, $groupIds, $expected): void { + public function testIsSharingDisabledForUser(string $excludeGroups, ?string $groupList, ?string $setList, array $groupIds, bool $expected): void { $user = $this->createMock(IUser::class); $this->config->method('getAppValue')