From dae23b7fd86feb55a29ea74e3053d974b62c9d1b Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Thu, 19 Oct 2023 19:38:26 +0200 Subject: [PATCH 1/2] feat(dav): expose Nextcloud groups to Contact's system addressbook contacts And move event listeners related to SyncService to proper ones Note: Changes to user system addressbook cards are now always done in a QueuedJob Closes #38432 Signed-off-by: Thomas Citharel --- .../composer/composer/autoload_classmap.php | 3 + .../dav/composer/composer/autoload_static.php | 3 + apps/dav/lib/AppInfo/Application.php | 19 ++-- .../SyncSystemAddressBookAfterUsersChange.php | 50 ++++++++++ apps/dav/lib/CardDAV/Converter.php | 22 ++++- apps/dav/lib/Listener/GroupChangeListener.php | 56 +++++++++++ apps/dav/lib/Listener/UserChangeListener.php | 60 ++++++++++++ ...ncSystemAddressBookAfterUserChangeTest.php | 61 ++++++++++++ apps/dav/tests/unit/CardDAV/ConverterTest.php | 94 ++++++++++++++----- .../unit/Listener/GroupChangeListenerTest.php | 81 ++++++++++++++++ .../unit/Listener/UserChangeListenerTest.php | 82 ++++++++++++++++ 11 files changed, 498 insertions(+), 33 deletions(-) create mode 100644 apps/dav/lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php create mode 100644 apps/dav/lib/Listener/GroupChangeListener.php create mode 100644 apps/dav/lib/Listener/UserChangeListener.php create mode 100644 apps/dav/tests/unit/BackgroundJob/SyncSystemAddressBookAfterUserChangeTest.php create mode 100644 apps/dav/tests/unit/Listener/GroupChangeListenerTest.php create mode 100644 apps/dav/tests/unit/Listener/UserChangeListenerTest.php diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 414c5df4a4a29..2bdfea6248cd4 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -21,6 +21,7 @@ 'OCA\\DAV\\BackgroundJob\\PruneOutdatedSyncTokensJob' => $baseDir . '/../lib/BackgroundJob/PruneOutdatedSyncTokensJob.php', 'OCA\\DAV\\BackgroundJob\\RefreshWebcalJob' => $baseDir . '/../lib/BackgroundJob/RefreshWebcalJob.php', 'OCA\\DAV\\BackgroundJob\\RegisterRegenerateBirthdayCalendars' => $baseDir . '/../lib/BackgroundJob/RegisterRegenerateBirthdayCalendars.php', + 'OCA\\DAV\\BackgroundJob\\SyncSystemAddressBookAfterUsersChange' => $baseDir . '/../lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php', 'OCA\\DAV\\BackgroundJob\\UpdateCalendarResourcesRoomsBackgroundJob' => $baseDir . '/../lib/BackgroundJob/UpdateCalendarResourcesRoomsBackgroundJob.php', 'OCA\\DAV\\BackgroundJob\\UploadCleanup' => $baseDir . '/../lib/BackgroundJob/UploadCleanup.php', 'OCA\\DAV\\BackgroundJob\\UserStatusAutomation' => $baseDir . '/../lib/BackgroundJob/UserStatusAutomation.php', @@ -258,8 +259,10 @@ 'OCA\\DAV\\Listener\\CalendarShareUpdateListener' => $baseDir . '/../lib/Listener/CalendarShareUpdateListener.php', 'OCA\\DAV\\Listener\\CardListener' => $baseDir . '/../lib/Listener/CardListener.php', 'OCA\\DAV\\Listener\\ClearPhotoCacheListener' => $baseDir . '/../lib/Listener/ClearPhotoCacheListener.php', + 'OCA\\DAV\\Listener\\GroupChangeListener' => $baseDir . '/../lib/Listener/GroupChangeListener.php', 'OCA\\DAV\\Listener\\SubscriptionListener' => $baseDir . '/../lib/Listener/SubscriptionListener.php', 'OCA\\DAV\\Listener\\TrustedServerRemovedListener' => $baseDir . '/../lib/Listener/TrustedServerRemovedListener.php', + 'OCA\\DAV\\Listener\\UserChangeListener' => $baseDir . '/../lib/Listener/UserChangeListener.php', 'OCA\\DAV\\Listener\\UserPreferenceListener' => $baseDir . '/../lib/Listener/UserPreferenceListener.php', 'OCA\\DAV\\Migration\\BuildCalendarSearchIndex' => $baseDir . '/../lib/Migration/BuildCalendarSearchIndex.php', 'OCA\\DAV\\Migration\\BuildCalendarSearchIndexBackgroundJob' => $baseDir . '/../lib/Migration/BuildCalendarSearchIndexBackgroundJob.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 7b0520cc220c5..acbfdae5dc953 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -36,6 +36,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\BackgroundJob\\PruneOutdatedSyncTokensJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/PruneOutdatedSyncTokensJob.php', 'OCA\\DAV\\BackgroundJob\\RefreshWebcalJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/RefreshWebcalJob.php', 'OCA\\DAV\\BackgroundJob\\RegisterRegenerateBirthdayCalendars' => __DIR__ . '/..' . '/../lib/BackgroundJob/RegisterRegenerateBirthdayCalendars.php', + 'OCA\\DAV\\BackgroundJob\\SyncSystemAddressBookAfterUsersChange' => __DIR__ . '/..' . '/../lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php', 'OCA\\DAV\\BackgroundJob\\UpdateCalendarResourcesRoomsBackgroundJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/UpdateCalendarResourcesRoomsBackgroundJob.php', 'OCA\\DAV\\BackgroundJob\\UploadCleanup' => __DIR__ . '/..' . '/../lib/BackgroundJob/UploadCleanup.php', 'OCA\\DAV\\BackgroundJob\\UserStatusAutomation' => __DIR__ . '/..' . '/../lib/BackgroundJob/UserStatusAutomation.php', @@ -273,8 +274,10 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Listener\\CalendarShareUpdateListener' => __DIR__ . '/..' . '/../lib/Listener/CalendarShareUpdateListener.php', 'OCA\\DAV\\Listener\\CardListener' => __DIR__ . '/..' . '/../lib/Listener/CardListener.php', 'OCA\\DAV\\Listener\\ClearPhotoCacheListener' => __DIR__ . '/..' . '/../lib/Listener/ClearPhotoCacheListener.php', + 'OCA\\DAV\\Listener\\GroupChangeListener' => __DIR__ . '/..' . '/../lib/Listener/GroupChangeListener.php', 'OCA\\DAV\\Listener\\SubscriptionListener' => __DIR__ . '/..' . '/../lib/Listener/SubscriptionListener.php', 'OCA\\DAV\\Listener\\TrustedServerRemovedListener' => __DIR__ . '/..' . '/../lib/Listener/TrustedServerRemovedListener.php', + 'OCA\\DAV\\Listener\\UserChangeListener' => __DIR__ . '/..' . '/../lib/Listener/UserChangeListener.php', 'OCA\\DAV\\Listener\\UserPreferenceListener' => __DIR__ . '/..' . '/../lib/Listener/UserPreferenceListener.php', 'OCA\\DAV\\Migration\\BuildCalendarSearchIndex' => __DIR__ . '/..' . '/../lib/Migration/BuildCalendarSearchIndex.php', 'OCA\\DAV\\Migration\\BuildCalendarSearchIndexBackgroundJob' => __DIR__ . '/..' . '/../lib/Migration/BuildCalendarSearchIndexBackgroundJob.php', diff --git a/apps/dav/lib/AppInfo/Application.php b/apps/dav/lib/AppInfo/Application.php index 08529435caeb3..dd5c1b246d60b 100644 --- a/apps/dav/lib/AppInfo/Application.php +++ b/apps/dav/lib/AppInfo/Application.php @@ -69,6 +69,8 @@ use OCA\DAV\Events\CardUpdatedEvent; use OCA\DAV\Events\SubscriptionCreatedEvent; use OCA\DAV\Events\SubscriptionDeletedEvent; +use OCA\DAV\Listener\GroupChangeListener; +use OCA\DAV\Listener\UserChangeListener; use OCP\Accounts\UserUpdatedEvent; use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\Events\TrustedServerRemovedEvent; @@ -102,6 +104,10 @@ use OCP\Config\BeforePreferenceSetEvent; use OCP\Contacts\IManager as IContactsManager; use OCP\Files\AppData\IAppDataFactory; +use OCP\Group\Events\BeforeGroupChangedEvent; +use OCP\Group\Events\BeforeGroupDeletedEvent; +use OCP\Group\Events\UserAddedEvent; +use OCP\Group\Events\UserRemovedEvent; use OCP\IUser; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -195,6 +201,12 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(BeforePreferenceDeletedEvent::class, UserPreferenceListener::class); $context->registerEventListener(BeforePreferenceSetEvent::class, UserPreferenceListener::class); + $context->registerEventListener(BeforeGroupChangedEvent::class, GroupChangeListener::class); + $context->registerEventListener(BeforeGroupDeletedEvent::class, GroupChangeListener::class); + $context->registerEventListener(UserAddedEvent::class, UserChangeListener::class); + $context->registerEventListener(UserUpdatedEvent::class, UserChangeListener::class); + $context->registerEventListener(UserRemovedEvent::class, UserChangeListener::class); + $context->registerNotifierService(Notifier::class); $context->registerCalendarProvider(CalendarProvider::class); @@ -227,13 +239,6 @@ public function registerHooks(HookManager $hm, } }); - $dispatcher->addListener(UserUpdatedEvent::class, function (UserUpdatedEvent $event) use ($container) { - /** @var SyncService $syncService */ - $syncService = \OCP\Server::get(SyncService::class); - $syncService->updateUser($event->getUser()); - }); - - $dispatcher->addListener(CalendarShareUpdatedEvent::class, function (CalendarShareUpdatedEvent $event) use ($container) { /** @var Backend $backend */ $backend = $container->query(Backend::class); diff --git a/apps/dav/lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php b/apps/dav/lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php new file mode 100644 index 0000000000000..4f40469b14050 --- /dev/null +++ b/apps/dav/lib/BackgroundJob/SyncSystemAddressBookAfterUsersChange.php @@ -0,0 +1,50 @@ + + * + * @author Citharel Thomas + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\BackgroundJob; + +use OCA\DAV\CardDAV\SyncService; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\QueuedJob; +use OCP\IUser; + +class SyncSystemAddressBookAfterUsersChange extends QueuedJob { + private SyncService $syncService; + + public function __construct(SyncService $syncService, ITimeFactory $time) { + parent::__construct($time); + $this->syncService = $syncService; + } + + /** + * @param IUser[] $argument + * @return void + */ + public function run($argument): void { + foreach ($argument as $user) { + $this->syncService->updateUser($user); + } + } +} diff --git a/apps/dav/lib/CardDAV/Converter.php b/apps/dav/lib/CardDAV/Converter.php index e19b52b478362..de8496394b28b 100644 --- a/apps/dav/lib/CardDAV/Converter.php +++ b/apps/dav/lib/CardDAV/Converter.php @@ -28,7 +28,11 @@ namespace OCA\DAV\CardDAV; use Exception; +use OCA\DAV\AppInfo\Application; use OCP\Accounts\IAccountManager; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IGroupManager; use OCP\IURLGenerator; use OCP\IImage; use OCP\IUser; @@ -42,12 +46,16 @@ class Converter { /** @var IAccountManager */ private $accountManager; private IUserManager $userManager; + private IGroupManager $groupManager; + private IConfig $config; public function __construct(IAccountManager $accountManager, - IUserManager $userManager, IURLGenerator $urlGenerator) { + IUserManager $userManager, IGroupManager $groupManager, IURLGenerator $urlGenerator, IConfig $config) { $this->accountManager = $accountManager; $this->userManager = $userManager; + $this->groupManager = $groupManager; $this->urlGenerator = $urlGenerator; + $this->config = $config; } public function createCardFromUser(IUser $user): ?VCard { @@ -151,6 +159,18 @@ public function createCardFromUser(IUser $user): ?VCard { } } + if ($this->config->getAppValue(Application::APP_ID, 'system_addressbook_expose_groups', 'no') === 'yes') { + $groupsToInclude = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_include', '')); + $groupsToIgnore = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_ignore', '')); + $groupNames = array_reduce($this->groupManager->getUserGroups($user), function ($groupNames, IGroup $group) use ($groupsToInclude, $groupsToIgnore) { + if (!in_array($group->getGID(), $groupsToIgnore, true) && (empty($groupsToInclude) || in_array($group->getGID(), $groupsToInclude, true))) { + $groupNames[] = $group->getDisplayName(); + } + return $groupNames; + }, []); + $vCard->add(new Text($vCard, 'CATEGORIES', $groupNames)); + } + if ($publish && !empty($cloudId)) { $vCard->add(new Text($vCard, 'CLOUD', $cloudId)); $vCard->validate(); diff --git a/apps/dav/lib/Listener/GroupChangeListener.php b/apps/dav/lib/Listener/GroupChangeListener.php new file mode 100644 index 0000000000000..66fa44005acd2 --- /dev/null +++ b/apps/dav/lib/Listener/GroupChangeListener.php @@ -0,0 +1,56 @@ + + * + * @author Thomas Citharel + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\Listener; + +use OCA\DAV\AppInfo\Application; +use OCA\DAV\BackgroundJob\SyncSystemAddressBookAfterUsersChange; +use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\Group\Events\BeforeGroupChangedEvent; +use OCP\Group\Events\BeforeGroupDeletedEvent; +use OCP\IConfig; + +/** + * @template-implements IEventListener + */ +class GroupChangeListener implements IEventListener { + public function __construct(private IJobList $jobList, private IConfig $config) { + } + + public function handle(Event $event): void { + if (!$this->canSyncBecauseOfGroupChange($event)) { + // Not what we subscribed to + return; + } + /** @var BeforeGroupChangedEvent|BeforeGroupDeletedEvent $event */ + $this->jobList->add(SyncSystemAddressBookAfterUsersChange::class, $event->getGroup()->getUsers()); + } + + private function canSyncBecauseOfGroupChange(Event $event): bool { + return ($event instanceof BeforeGroupChangedEvent || $event instanceof BeforeGroupDeletedEvent) && $this->config->getAppValue(Application::APP_ID, 'system_addressbook_expose_groups', 'no') === 'yes'; + } +} diff --git a/apps/dav/lib/Listener/UserChangeListener.php b/apps/dav/lib/Listener/UserChangeListener.php new file mode 100644 index 0000000000000..7548ec9cfe920 --- /dev/null +++ b/apps/dav/lib/Listener/UserChangeListener.php @@ -0,0 +1,60 @@ + + * + * @author Thomas Citharel + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\Listener; + +use OCA\DAV\AppInfo\Application; +use OCA\DAV\BackgroundJob\SyncSystemAddressBookAfterUsersChange; +use OCP\Accounts\UserUpdatedEvent; +use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\Group\Events\UserAddedEvent; +use OCP\Group\Events\UserRemovedEvent; +use OCP\IConfig; + +/** + * @template-implements IEventListener + */ +class UserChangeListener implements IEventListener { + public function __construct(private IJobList $jobList, private IConfig $config) { + } + + public function handle(Event $event): void { + if (!($event instanceof UserUpdatedEvent || $event instanceof UserAddedEvent || $event instanceof UserRemovedEvent)) { + // Not what we subscribed to + return; + } + + if ($this->canSyncBecauseOfGroupMembershipChange($event)) { + /** @var UserUpdatedEvent|UserAddedEvent|UserRemovedEvent $event */ + $this->jobList->add(SyncSystemAddressBookAfterUsersChange::class, [$event->getUser()]); + } + } + + private function canSyncBecauseOfGroupMembershipChange(Event $event): bool { + return $event instanceof UserUpdatedEvent || $this->config->getAppValue(Application::APP_ID, 'system_addressbook_expose_groups', 'no') === 'yes'; + } +} diff --git a/apps/dav/tests/unit/BackgroundJob/SyncSystemAddressBookAfterUserChangeTest.php b/apps/dav/tests/unit/BackgroundJob/SyncSystemAddressBookAfterUserChangeTest.php new file mode 100644 index 0000000000000..b7e9dcc8731ff --- /dev/null +++ b/apps/dav/tests/unit/BackgroundJob/SyncSystemAddressBookAfterUserChangeTest.php @@ -0,0 +1,61 @@ + + * + * @author Christoph Wurst + * @author Georg Ehrke + * @author Joas Schilling + * @author Morris Jobke + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\Tests\unit\BackgroundJob; + +use OCA\DAV\BackgroundJob\SyncSystemAddressBookAfterUsersChange; +use OCA\DAV\CardDAV\SyncService; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IUser; +use Test\TestCase; + +class SyncSystemAddressBookAfterUserChangeTest extends TestCase { + private SyncService|MockObject $syncService; + + private SyncSystemAddressBookAfterUsersChange $backgroundJob; + + protected function setUp(): void { + parent::setUp(); + + $this->syncService = $this->createMock(SyncService::class); + $timeFactory = $this->createMock(ITimeFactory::class); + + $this->backgroundJob = new SyncSystemAddressBookAfterUsersChange( + $this->syncService, $timeFactory); + } + + public function testRun(): void { + $user1 = $this->createMock(IUser::class); + $user2 = $this->createMock(IUser::class); + + $this->syncService->expects($this->exactly(2))->method('updateUser')->withConsecutive([$user1], [$user2]); + + $this->backgroundJob->run([$user1, $user2]); + } +} diff --git a/apps/dav/tests/unit/CardDAV/ConverterTest.php b/apps/dav/tests/unit/CardDAV/ConverterTest.php index 7205cf5665444..c94c29b8206c4 100644 --- a/apps/dav/tests/unit/CardDAV/ConverterTest.php +++ b/apps/dav/tests/unit/CardDAV/ConverterTest.php @@ -30,39 +30,45 @@ */ namespace OCA\DAV\Tests\unit\CardDAV; +use OCA\DAV\AppInfo\Application; use OCA\DAV\CardDAV\Converter; use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IGroupManager; use OCP\IURLGenerator; use OCP\IImage; use OCP\IUser; use OCP\IUserManager; use PHPUnit\Framework\MockObject\MockObject; +use Sabre\VObject\Component\VCard; +use Sabre\VObject\Reader; use Test\TestCase; class ConverterTest extends TestCase { - - /** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */ - private $accountManager; - /** @var IUserManager|(IUserManager&MockObject)|MockObject */ + private IAccountManager|MockObject $accountManager; private IUserManager|MockObject $userManager; + private IGroupManager|MockObject $groupManager; + private IURLGenerator|MockObject $urlGenerator; - /** @var IURLGenerator */ - private $urlGenerator; + private IConfig|MockObject $config; protected function setUp(): void { parent::setUp(); $this->accountManager = $this->createMock(IAccountManager::class); $this->userManager = $this->createMock(IUserManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->config = $this->createMock(IConfig::class); } /** * @return IAccountProperty|MockObject */ - protected function getAccountPropertyMock(string $name, ?string $value, string $scope) { + protected function getAccountPropertyMock(string $name, ?string $value, string $scope): IAccountProperty|MockObject { $property = $this->createMock(IAccountProperty::class); $property->expects($this->any()) ->method('getName') @@ -108,10 +114,10 @@ public function testCreation($expectedVCard, $displayName = null, $eMailAddress $user = $this->getUserMock((string)$displayName, $eMailAddress, $cloudId); $accountManager = $this->getAccountManager($user); - $converter = new Converter($accountManager, $this->userManager, $this->urlGenerator); + $converter = new Converter($accountManager, $this->userManager, $this->groupManager, $this->urlGenerator, $this->config); $vCard = $converter->createCardFromUser($user); if ($expectedVCard !== null) { - $this->assertInstanceOf('Sabre\VObject\Component\VCard', $vCard); + $this->assertInstanceOf(VCard::class, $vCard); $cardData = $vCard->jsonSerialize(); $this->compareData($expectedVCard, $cardData); } else { @@ -129,7 +135,7 @@ public function testManagerProp(): void { ->willReturn('Manager'); $accountManager = $this->getAccountManager($user); - $converter = new Converter($accountManager, $this->userManager, $this->urlGenerator); + $converter = new Converter($accountManager, $this->userManager, $this->groupManager, $this->urlGenerator, $this->config); $vCard = $converter->createCardFromUser($user); $this->compareData( @@ -142,7 +148,7 @@ public function testManagerProp(): void { ); } - protected function compareData($expected, $data) { + protected function compareData($expected, $data): void { foreach ($expected as $key => $value) { $found = false; foreach ($data[1] as $d) { @@ -152,12 +158,12 @@ protected function compareData($expected, $data) { } } if (!$found) { - $this->assertTrue(false, 'Expected data: ' . $key . ' not found.'); + $this->fail('Expected data: ' . $key . ' not found.'); } } } - public function providesNewUsers() { + public function providesNewUsers(): array { return [ [ null @@ -213,17 +219,15 @@ public function providesNewUsers() { /** * @dataProvider providesNames - * @param $expected - * @param $fullName */ - public function testNameSplitter($expected, $fullName): void { - $converter = new Converter($this->accountManager, $this->userManager, $this->urlGenerator); + public function testNameSplitter(string $expected, string $fullName): void { + $converter = new Converter($this->accountManager, $this->userManager, $this->groupManager, $this->urlGenerator, $this->config); $r = $converter->splitFullName($fullName); $r = implode(';', $r); $this->assertEquals($expected, $r); } - public function providesNames() { + public function providesNames(): array { return [ ['Sauron;;;;', 'Sauron'], ['Baggins;Bilbo;;;', 'Bilbo Baggins'], @@ -231,13 +235,7 @@ public function providesNames() { ]; } - /** - * @param $displayName - * @param $eMailAddress - * @param $cloudId - * @return IUser | \PHPUnit\Framework\MockObject\MockObject - */ - protected function getUserMock(string $displayName, ?string $eMailAddress, ?string $cloudId) { + protected function getUserMock(string $displayName, ?string $eMailAddress, ?string $cloudId): IUser|MockObject { $image0 = $this->getMockBuilder(IImage::class)->disableOriginalConstructor()->getMock(); $image0->method('mimeType')->willReturn('image/jpeg'); $image0->method('data')->willReturn('123456789'); @@ -249,4 +247,50 @@ protected function getUserMock(string $displayName, ?string $eMailAddress, ?stri $user->method('getAvatarImage')->willReturn($image0); return $user; } + + /** + * @dataProvider dataForTestExposeGroups + */ + public function testExposeGroups(bool $exposeGroups, array $groups, string $groupsToInclude, string $groupsToIgnore, ?string $result): void { + $user = $this->getUserMock("user", "user@domain.tld", "user@cloud.domain.tld"); + $this->config->expects($exposeGroups ? $this->exactly(3) :$this->once())->method('getAppValue')->withConsecutive( + [Application::APP_ID, 'system_addressbook_expose_groups', 'no'], + [Application::APP_ID, 'system_addressbook_groups_to_include', ''], + [Application::APP_ID, 'system_addressbook_groups_to_ignore', ''] + )->willReturnOnConsecutiveCalls( + $exposeGroups ? 'yes' : 'no', + $groupsToInclude, + $groupsToIgnore + ); + + $ignoredGroups = explode(',', $groupsToIgnore); + $includedGroups = explode(',', $groupsToInclude); + + $this->groupManager->expects($exposeGroups ? $this->once() : $this->never())->method('getUserGroups')->with($user)->willReturn(array_map(function ($groupName) use ($groups, $exposeGroups, $includedGroups, $ignoredGroups) { + $group = $this->createMock(IGroup::class); + $group->expects($exposeGroups ? $this->once() : $this->never())->method('getGID')->willReturn($groupName); + $group->expects($exposeGroups && !in_array($groupName, $ignoredGroups, true) && (empty($includedGroups) || in_array($groupName, $includedGroups, true)) ? $this->once() : $this->never())->method('getDisplayName')->willReturn($groupName); + return $group; + }, $groups)); + $accountManager = $this->getAccountManager($user); + + $converter = new Converter($accountManager, $this->userManager, $this->groupManager, $this->urlGenerator, $this->config); + $vCard = $converter->createCardFromUser($user); + + $parsed = Reader::read($vCard->serialize(), Reader::OPTION_FORGIVING); + + $this->assertEquals((string) $parsed->CATEGORIES, $result); + } + + public function dataForTestExposeGroups(): array { + return [ + [false, ['myGroup1'], '', '', null], + [true, ['myGroup1'], '', '', 'myGroup1'], + [true, ['myGroup1', 'myGroup2'], '', '', 'myGroup1,myGroup2'], + [true, ['myGroup1', 'myGroup2'], '', 'myGroup2,myGroup3', 'myGroup1'], + [true, ['myGroup1', 'myGroup2', 'myGroup3', 'myGroup4'], '', 'myGroup2,myGroup3', 'myGroup1,myGroup4'], + [true, ['myGroup1', 'myGroup2', 'myGroup3', 'myGroup4'], 'myGroup1, myGroup5', 'myGroup2,myGroup3', 'myGroup1'], + [true, ['myGroup1', 'myGroup2'], '', 'myGroup2,myGroup3', 'myGroup1'] + ]; + } } diff --git a/apps/dav/tests/unit/Listener/GroupChangeListenerTest.php b/apps/dav/tests/unit/Listener/GroupChangeListenerTest.php new file mode 100644 index 0000000000000..678f258dd1ed0 --- /dev/null +++ b/apps/dav/tests/unit/Listener/GroupChangeListenerTest.php @@ -0,0 +1,81 @@ + + * + * @author Thomas Citharel + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\Tests\Unit\Listener; + +use OCA\DAV\AppInfo\Application; +use OCA\DAV\BackgroundJob\SyncSystemAddressBookAfterUsersChange; +use OCA\DAV\Listener\GroupChangeListener; +use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\Event; +use OCP\Group\Events\BeforeGroupChangedEvent; +use OCP\Group\Events\BeforeGroupCreatedEvent; +use OCP\Group\Events\BeforeGroupDeletedEvent; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IUser; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +class GroupChangeListenerTest extends TestCase { + private IJobList|MockObject $jobList; + private IConfig|MockObject $config; + private GroupChangeListener $listener; + + protected function setUp(): void { + parent::setUp(); + + $this->jobList = $this->createMock(IJobList::class); + $this->config = $this->createMock(IConfig::class); + + $this->listener = new GroupChangeListener( + $this->jobList, + $this->config + ); + } + + /** + * @dataProvider dataForTestHandleGroupChangeEvent + */ + public function testHandleGroupChangeEvent(Event $event, array $users, string $groupsExposed, bool $willSync): void { + $this->jobList->expects($willSync ? $this->once() : $this->never())->method('add')->with(SyncSystemAddressBookAfterUsersChange::class, $users); + $this->config->expects($event instanceof BeforeGroupCreatedEvent ? $this->never() : $this->once())->method('getAppValue')->with(Application::APP_ID, 'system_addressbook_expose_groups', 'no')->willReturn($groupsExposed); + $this->listener->handle($event); + } + + public function dataForTestHandleGroupChangeEvent(): array { + $group = $this->createMock(IGroup::class); + $users = [$this->createMock(IUser::class), $this->createMock(IUser::class)]; + $group->expects($this->any())->method('getUsers')->willReturn($users); + return [ + [new BeforeGroupChangedEvent($group, 'displayName', 'NewDisplayName', 'OldDisplayName'), $users, 'no', false], + [new BeforeGroupChangedEvent($group, 'displayName', 'NewDisplayName', 'OldDisplayName'), $users, 'yes', true], + [new BeforeGroupDeletedEvent($group), $users, 'no', false], + [new BeforeGroupDeletedEvent($group), $users, 'yes', true], + [new BeforeGroupCreatedEvent('mygroup'), $users, 'no', false], + [new BeforeGroupCreatedEvent('mygroup'), $users, 'yes', false], + ]; + } +} diff --git a/apps/dav/tests/unit/Listener/UserChangeListenerTest.php b/apps/dav/tests/unit/Listener/UserChangeListenerTest.php new file mode 100644 index 0000000000000..fbad588f2f6b1 --- /dev/null +++ b/apps/dav/tests/unit/Listener/UserChangeListenerTest.php @@ -0,0 +1,82 @@ + + * + * @author Thomas Citharel + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCA\DAV\Tests\Unit\Listener; + +use OCA\DAV\AppInfo\Application; +use OCA\DAV\BackgroundJob\SyncSystemAddressBookAfterUsersChange; +use OCA\DAV\Listener\UserChangeListener; +use OCP\Accounts\UserUpdatedEvent; +use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\Event; +use OCP\Group\Events\UserAddedEvent; +use OCP\Group\Events\UserRemovedEvent; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IUser; +use OCP\User\Events\PostLoginEvent; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +class UserChangeListenerTest extends TestCase { + private IJobList|MockObject $jobList; + private UserChangeListener $listener; + + protected function setUp(): void { + parent::setUp(); + + $this->jobList = $this->createMock(IJobList::class); + $this->config = $this->createMock(IConfig::class); + + $this->listener = new UserChangeListener( + $this->jobList, + $this->config + ); + } + + /** + * @dataProvider dataForTestHandleUserChangeEvent + */ + public function testHandleUserChangeEvent(Event $event, IUser $user, string $groupsExposed, bool $willSync): void { + $this->config->expects(($event instanceof UserAddedEvent || $event instanceof UserRemovedEvent) ? $this->once() : $this->never())->method('getAppValue')->with(Application::APP_ID, 'system_addressbook_expose_groups', 'no')->willReturn($groupsExposed); + $this->jobList->expects($willSync ? $this->once() : $this->never())->method('add')->with(SyncSystemAddressBookAfterUsersChange::class, [$user]); + $this->listener->handle($event); + } + + public function dataForTestHandleUserChangeEvent(): array { + $group = $this->createMock(IGroup::class); + $user = $this->createMock(IUser::class); + return [ + [new UserAddedEvent($group, $user), $user, 'yes', true], + [new UserAddedEvent($group, $user), $user, 'no', false], + [new UserUpdatedEvent($user, []), $user, 'yes', true], + [new UserUpdatedEvent($user, []), $user, 'no', true], + [new UserRemovedEvent($group, $user), $user, 'yes', true], + [new UserRemovedEvent($group, $user), $user, 'no', false], + [new PostLoginEvent($user, 'a', 'b', true), $user, 'yes', false], + [new PostLoginEvent($user, 'a', 'b', true), $user, 'no', false], + ]; + } +} From 7d6fddaa1e8cc02d849aa08a688c98cd53a2476b Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 20 Oct 2023 10:39:20 +0200 Subject: [PATCH 2/2] refactor(dav): various modernizations of CardDAV SyncService Signed-off-by: Thomas Citharel --- apps/dav/lib/CardDAV/SyncService.php | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/apps/dav/lib/CardDAV/SyncService.php b/apps/dav/lib/CardDAV/SyncService.php index 09c31683069fb..dd69f36df8c34 100644 --- a/apps/dav/lib/CardDAV/SyncService.php +++ b/apps/dav/lib/CardDAV/SyncService.php @@ -30,7 +30,6 @@ */ namespace OCA\DAV\CardDAV; -use OC\Accounts\AccountManager; use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Http; use OCP\IDBConnection; @@ -42,6 +41,7 @@ use Sabre\DAV\Xml\Service; use Sabre\HTTP\ClientHttpException; use Sabre\VObject\Reader; +use Sabre\Xml\ParseException; use function is_null; class SyncService { @@ -181,6 +181,9 @@ protected function download(string $url, string $userName, string $sharedSecret, return $client->request('GET', $resourcePath); } + /** + * @throws \DOMException + */ private function buildSyncCollectionRequestBody(?string $syncToken): string { $dom = new \DOMDocument('1.0', 'UTF-8'); $dom->formatOutput = true; @@ -201,9 +204,9 @@ private function buildSyncCollectionRequestBody(?string $syncToken): string { /** * @param string $body * @return array - * @throws \Sabre\Xml\ParseException + * @throws ParseException */ - private function parseMultiStatus($body) { + private function parseMultiStatus(string $body): array { $xml = new Service(); /** @var MultiStatus $multiStatus */ @@ -217,9 +220,6 @@ private function parseMultiStatus($body) { return ['response' => $result, 'token' => $multiStatus->getSyncToken()]; } - /** - * @param IUser $user - */ public function updateUser(IUser $user): void { $systemAddressBook = $this->getLocalSystemAddressBook(); $addressBookId = $systemAddressBook['id']; @@ -228,13 +228,12 @@ public function updateUser(IUser $user): void { if ($user->isEnabled()) { $this->atomic(function() use ($addressBookId, $cardId, $user) { $card = $this->backend->getCard($addressBookId, $cardId); + $vCard = $this->converter->createCardFromUser($user); if ($card === false) { - $vCard = $this->converter->createCardFromUser($user); if ($vCard !== null) { $this->backend->createCard($addressBookId, $cardId, $vCard->serialize(), false); } } else { - $vCard = $this->converter->createCardFromUser($user); if (is_null($vCard)) { $this->backend->deleteCard($addressBookId, $cardId); } else { @@ -247,10 +246,7 @@ public function updateUser(IUser $user): void { } } - /** - * @param IUser|string $userOrCardId - */ - public function deleteUser($userOrCardId) { + public function deleteUser(IUser|string $userOrCardId): void { $systemAddressBook = $this->getLocalSystemAddressBook(); if ($userOrCardId instanceof IUser) { $userOrCardId = self::getCardUri($userOrCardId); @@ -258,10 +254,7 @@ public function deleteUser($userOrCardId) { $this->backend->deleteCard($systemAddressBook['id'], $userOrCardId); } - /** - * @return array|null - */ - public function getLocalSystemAddressBook() { + public function getLocalSystemAddressBook(): ?array { if (is_null($this->localSystemAddressBook)) { $systemPrincipal = "principals/system/system"; $this->localSystemAddressBook = $this->ensureSystemAddressBookExists($systemPrincipal, 'system', [ @@ -272,7 +265,7 @@ public function getLocalSystemAddressBook() { return $this->localSystemAddressBook; } - public function syncInstance(\Closure $progressCallback = null) { + public function syncInstance(\Closure $progressCallback = null): void { $systemAddressBook = $this->getLocalSystemAddressBook(); $this->userManager->callForAllUsers(function ($user) use ($systemAddressBook, $progressCallback) { $this->updateUser($user);