From dfabc76a96ba2851990950ceaf58507595a3d15a Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Mon, 2 Sep 2024 17:42:28 +0200 Subject: [PATCH] fix: respect advanced group sharing settings in frontend Signed-off-by: Richard Steinmetz --- lib/Controller/PageController.php | 14 ++- lib/Service/GroupSharingService.php | 47 ++++++++ .../Settings/SettingsAddressbookShare.vue | 21 ++-- tests/unit/Controller/PageControllerTest.php | 7 +- .../unit/Service/GroupSharingServiceTest.php | 112 ++++++++++++++++++ 5 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 lib/Service/GroupSharingService.php create mode 100644 tests/unit/Service/GroupSharingServiceTest.php diff --git a/lib/Controller/PageController.php b/lib/Controller/PageController.php index ed9c438d2..a9308e005 100644 --- a/lib/Controller/PageController.php +++ b/lib/Controller/PageController.php @@ -8,6 +8,7 @@ use OC\App\CompareVersion; use OCA\Contacts\AppInfo\Application; +use OCA\Contacts\Service\GroupSharingService; use OCA\Contacts\Service\SocialApiService; use OCP\App\IAppManager; @@ -42,6 +43,8 @@ class PageController extends Controller { /** @var CompareVersion */ private $compareVersion; + private GroupSharingService $groupSharingService; + public function __construct(IRequest $request, IConfig $config, IInitialStateService $initialStateService, @@ -49,7 +52,8 @@ public function __construct(IRequest $request, IUserSession $userSession, SocialApiService $socialApiService, IAppManager $appManager, - CompareVersion $compareVersion) { + CompareVersion $compareVersion, + GroupSharingService $groupSharingService) { parent::__construct(Application::APP_ID, $request); $this->config = $config; @@ -59,6 +63,7 @@ public function __construct(IRequest $request, $this->socialApiService = $socialApiService; $this->appManager = $appManager; $this->compareVersion = $compareVersion; + $this->groupSharingService = $groupSharingService; } /** @@ -69,10 +74,7 @@ public function __construct(IRequest $request, */ public function index(): TemplateResponse { $user = $this->userSession->getUser(); - $userId = ''; - if (!is_null($user)) { - $userId = $user->getUid(); - } + $userId = $user->getUid(); $locales = $this->languageFactory->findAvailableLocales(); $defaultProfile = $this->config->getAppValue(Application::APP_ID, 'defaultProfile', 'HOME'); @@ -89,7 +91,7 @@ public function index(): TemplateResponse { // if circles is not installed, we use 0.0.0 $isCircleVersionCompatible = $this->compareVersion->isCompatible($circleVersion ? $circleVersion : '0.0.0', 22); // Check whether group sharing is enabled or not - $isGroupSharingEnabled = $this->config->getAppValue('core', 'shareapi_allow_group_sharing', 'yes') === 'yes'; + $isGroupSharingEnabled = $this->groupSharingService->isGroupSharingAllowed($user); $talkVersion = $this->appManager->getAppVersion('spreed'); $isTalkEnabled = $this->appManager->isEnabledForUser('spreed') === true; diff --git a/lib/Service/GroupSharingService.php b/lib/Service/GroupSharingService.php new file mode 100644 index 000000000..5264bef33 --- /dev/null +++ b/lib/Service/GroupSharingService.php @@ -0,0 +1,47 @@ +shareManager->allowGroupSharing()) { + return false; + } + + $userGroups = $this->groupManager->getUserGroups($user); + $userGroupNames = array_map(static fn (IGroup $group) => $group->getGID(), $userGroups); + + $excludeGroupList = json_decode($this->config->getAppValue('core', 'shareapi_exclude_groups_list', '[]')); + $excludeGroups = $this->config->getAppValue('core', 'shareapi_exclude_groups'); + + // "no" => Allow sharing for everyone + // "yes" => Exclude listed groups from sharing + // "allow" => Limit sharing to listed groups + return match ($excludeGroups) { + 'yes' => count(array_intersect($userGroupNames, $excludeGroupList)) === 0, + 'allow' => count(array_intersect($userGroupNames, $excludeGroupList)) > 0, + default => true, + }; + } +} diff --git a/src/components/AppNavigation/Settings/SettingsAddressbookShare.vue b/src/components/AppNavigation/Settings/SettingsAddressbookShare.vue index 5e5ff24a5..ee91bfaa7 100644 --- a/src/components/AppNavigation/Settings/SettingsAddressbookShare.vue +++ b/src/components/AppNavigation/Settings/SettingsAddressbookShare.vue @@ -101,20 +101,25 @@ export default { this.usersOrGroups = [] if (query.length > 0) { const results = await client.principalPropertySearchByDisplayname(query) - this.usersOrGroups = results.reduce((list, result) => { - if (['GROUP', 'INDIVIDUAL'].indexOf(result.calendarUserType) > -1 - && !this.addressbook.shares.some((share) => share.uri === result.principalScheme)) { + this.usersOrGroups = results + .filter((result) => { + const allowedCalendarUserTypes = ['INDIVIDUAL'] + if (this.isGroupSharingEnabled) { + allowedCalendarUserTypes.push('GROUP') + } + return allowedCalendarUserTypes.includes(result.calendarUserType) + && !this.addressbook.shares.some((share) => share.uri === result.principalScheme) + }) + .map((result) => { const isGroup = result.calendarUserType === 'GROUP' - list.push({ + return { user: urldecode(result[isGroup ? 'groupId' : 'userId']), displayName: result.displayname, icon: isGroup ? 'icon-group' : 'icon-user', uri: urldecode(result.principalScheme), isGroup, - }) - } - return list - }, []) + } + }) this.isLoading = false this.inputGiven = true } else { diff --git a/tests/unit/Controller/PageControllerTest.php b/tests/unit/Controller/PageControllerTest.php index 38e47003e..20bcc3321 100644 --- a/tests/unit/Controller/PageControllerTest.php +++ b/tests/unit/Controller/PageControllerTest.php @@ -8,6 +8,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use OC\App\CompareVersion; +use OCA\Contacts\Service\GroupSharingService; use OCA\Contacts\Service\SocialApiService; use OCP\App\IAppManager; use OCP\AppFramework\Http\TemplateResponse; @@ -46,6 +47,8 @@ class PageControllerTest extends TestCase { /** @var CompareVersion|MockObject*/ private $compareVersion; + private GroupSharingService|MockObject $groupSharingService; + protected function setUp(): void { parent::setUp(); @@ -57,6 +60,7 @@ protected function setUp(): void { $this->socialApi = $this->createMock(SocialApiService::class); $this->appManager = $this->createMock(IAppManager::class); $this->compareVersion = $this->createMock(CompareVersion::class); + $this->groupSharingService = $this->createMock(GroupSharingService::class); $this->controller = new PageController( $this->request, @@ -66,7 +70,8 @@ protected function setUp(): void { $this->userSession, $this->socialApi, $this->appManager, - $this->compareVersion + $this->compareVersion, + $this->groupSharingService, ); } diff --git a/tests/unit/Service/GroupSharingServiceTest.php b/tests/unit/Service/GroupSharingServiceTest.php new file mode 100644 index 000000000..42b89fda7 --- /dev/null +++ b/tests/unit/Service/GroupSharingServiceTest.php @@ -0,0 +1,112 @@ +config = $this->createMock(IConfig::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->shareManager = $this->createMock(IShareManager::class); + + $this->service = new GroupSharingService( + $this->config, + $this->groupManager, + $this->shareManager, + ); + } + + public function provideTestIsGroupSharingAllowed(): array { + return [ + // Basic group sharing is forbidden (-> short circuit) + [false, false, '["group1"]', 'no'], + [false, false, '["group1"]', 'yes'], + [false, false, '["group1"]', 'allow'], + + // Basic sharing is allowed and allow sharing with every group + [true, true, '["group1"]', 'no'], + [true, true, '[]', 'no'], + [true, true, '["group99"]', 'no'], + + // Basic sharing is allowed and user's group is excluded + [false, true, '["group1"]', 'yes'], + [false, true, '["group2"]', 'yes'], + [false, true, '["group2", "group3"]', 'yes'], + + // Basic sharing is allowed and user's group is not excluded + [true, true, '["group3"]', 'yes'], + [true, true, '[]', 'yes'], + + // Basic sharing is allowed and user's group is included + [true, true, '["group1"]', 'allow'], + [true, true, '["group2"]', 'allow'], + [true, true, '["group2", "group3"]', 'allow'], + + // Basic sharing is allowed and user's group is not included + [false, true, '["group3"]', 'allow'], + [false, true, '[]', 'allow'], + ]; + } + + /** @dataProvider provideTestIsGroupSharingAllowed */ + public function testIsGroupSharingAllowed( + bool $expected, + bool $allowGroupSharing, + string $excludeGroupList, + string $excludeGroups, + ): void { + $user = $this->createMock(IUser::class); + $group1 = $this->createMock(IGroup::class); + $group1->method('getGID') + ->willReturn('group1'); + $group2 = $this->createMock(IGroup::class); + $group2->method('getGID') + ->willReturn('group2'); + + $this->shareManager->expects(self::once()) + ->method('allowGroupSharing') + ->willReturn($allowGroupSharing); + if ($allowGroupSharing) { + $this->groupManager->expects(self::once()) + ->method('getUserGroups') + ->with($user) + ->willReturn([$group1, $group2]); + $this->config->expects(self::exactly(2)) + ->method('getAppValue') + ->willReturnMap([ + ['core', 'shareapi_exclude_groups_list', '[]', $excludeGroupList], + ['core', 'shareapi_exclude_groups', '', $excludeGroups], + ]); + } else { + $this->groupManager->expects(self::never()) + ->method('getUserGroups'); + $this->config->expects(self::never()) + ->method('getAppValue'); + } + + $actual = $this->service->isGroupSharingAllowed($user); + $this->assertEquals($expected, $actual); + } +}