fix(sharing): properly frame block list

apps/settings/src/components/AdminSettingsSharingForm.vue

@@ -115,15 +115,15 @@
 					value="yes"
 					type="radio"
 					@update:modelValue="onUpdateExcludeGroups">
-					{{ t('settings', 'Exclude some groups') }}
+					{{ t('settings', 'Exclude some groups (block list)') }}
 				</NcCheckboxRadioSwitch>
 				<NcCheckboxRadioSwitch
 					v-model="settings.excludeGroups"
 					name="excludeGroups"
 					value="allow"
 					type="radio"
 					@update:modelValue="onUpdateExcludeGroups">
-					{{ t('settings', 'Allow some groups') }}
+					{{ t('settings', 'Allow some groups only (allow list)') }}
 				</NcCheckboxRadioSwitch>
 				<div v-show="settings.excludeGroups !== 'no'" class="sharing__labeled-entry sharing__input">
 					<NcSettingsSelectGroup
@@ -133,7 +133,15 @@
 						:label="settings.excludeGroups === 'allow' ? t('settings', 'Groups allowed to share') : t('settings', 'Groups excluded from sharing')"
 						:disabled="settings.excludeGroups === 'no'"
 						style="width: 100%" />
-					<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>
+					<em id="settings-sharing-excluded-groups-desc">
+						<template v-if="settings.excludeGroups === 'allow'">
+							{{ t('settings', 'Only members of at least one of these groups will be able to initiate shares.') }}
+							{{ t('settings', 'Others will be blocked, but are still able to receive shares.') }}
+						</template>
+						<template v-else>
+							{{ 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.') }}
+						</template>
+					</em>
 				</div>
 			</div>
 

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"

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 |

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', '');

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;

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,18 +198,18 @@ 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']],
 			['yes', '["excludedGroup1", "excludedGroup2", "excludedGroup3"]', ['excludedGroup1'], []],
 			['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');