From c05d3fdb2e54c89a08ae7085fe3b96bf7e8214d9 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 7 Apr 2025 14:59:20 +0200 Subject: [PATCH 1/2] fix(caldav): prevent unshare entry creation for owner unsharing - Introduces a `unshare` method in `CalDavBackend` to handle user unshares. - Implements check to determine if unshare entry is needed based on group/circle membership. - Ensures `updateShares` is only used when the calendar owner manages shares. - Resolves issue where unsharing a calendar as owner created an unshare entry in `oc_dav_shares`. Related PRs: - https://github.com/nextcloud/server/pull/43117 - https://github.com/nextcloud/server/pull/47737 Signed-off-by: Daniel Kesselberg --- apps/dav/appinfo/info.xml | 2 + .../composer/composer/autoload_classmap.php | 2 + .../dav/composer/composer/autoload_static.php | 2 + apps/dav/lib/CalDAV/CalDavBackend.php | 38 ++- apps/dav/lib/CalDAV/Calendar.php | 8 +- .../dav/lib/Command/ClearCalendarUnshares.php | 114 ++++++++ apps/dav/lib/Command/ListCalendarShares.php | 131 +++++++++ apps/dav/lib/DAV/Sharing/Backend.php | 49 +++- apps/dav/lib/DAV/Sharing/SharingMapper.php | 24 ++ apps/dav/lib/DAV/Sharing/SharingService.php | 10 - .../lib/Events/CalendarShareUpdatedEvent.php | 10 +- .../DAV/Sharing/CalDavSharingBackendTest.php | 249 ++++++++++++++++++ .../DAV/Sharing/SharingMapperTest.php | 2 + .../tests/unit/CalDAV/CalDavBackendTest.php | 45 +++- apps/dav/tests/unit/CalDAV/CalendarTest.php | 26 +- .../unit/Command/ListCalendarSharesTest.php | 172 ++++++++++++ .../tests/unit/DAV/Sharing/BackendTest.php | 22 +- .../unit/DAV/Sharing/SharingServiceTest.php | 58 ---- 18 files changed, 846 insertions(+), 118 deletions(-) create mode 100644 apps/dav/lib/Command/ClearCalendarUnshares.php create mode 100644 apps/dav/lib/Command/ListCalendarShares.php create mode 100644 apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php create mode 100644 apps/dav/tests/unit/Command/ListCalendarSharesTest.php delete mode 100644 apps/dav/tests/unit/DAV/Sharing/SharingServiceTest.php diff --git a/apps/dav/appinfo/info.xml b/apps/dav/appinfo/info.xml index ac8886555f6d1..e0f4608e8c2b5 100644 --- a/apps/dav/appinfo/info.xml +++ b/apps/dav/appinfo/info.xml @@ -55,6 +55,7 @@ + OCA\DAV\Command\ClearCalendarUnshares OCA\DAV\Command\CreateAddressBook OCA\DAV\Command\CreateCalendar OCA\DAV\Command\CreateSubscription @@ -63,6 +64,7 @@ OCA\DAV\Command\ExportCalendar OCA\DAV\Command\FixCalendarSyncCommand OCA\DAV\Command\ListAddressbooks + OCA\DAV\Command\ListCalendarShares OCA\DAV\Command\ListCalendars OCA\DAV\Command\ListSubscriptions OCA\DAV\Command\MoveCalendar diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 466ef98d433df..913f5b46d9886 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -155,6 +155,7 @@ 'OCA\\DAV\\CardDAV\\UserAddressBooks' => $baseDir . '/../lib/CardDAV/UserAddressBooks.php', 'OCA\\DAV\\CardDAV\\Validation\\CardDavValidatePlugin' => $baseDir . '/../lib/CardDAV/Validation/CardDavValidatePlugin.php', 'OCA\\DAV\\CardDAV\\Xml\\Groups' => $baseDir . '/../lib/CardDAV/Xml/Groups.php', + 'OCA\\DAV\\Command\\ClearCalendarUnshares' => $baseDir . '/../lib/Command/ClearCalendarUnshares.php', 'OCA\\DAV\\Command\\CreateAddressBook' => $baseDir . '/../lib/Command/CreateAddressBook.php', 'OCA\\DAV\\Command\\CreateCalendar' => $baseDir . '/../lib/Command/CreateCalendar.php', 'OCA\\DAV\\Command\\CreateSubscription' => $baseDir . '/../lib/Command/CreateSubscription.php', @@ -163,6 +164,7 @@ 'OCA\\DAV\\Command\\ExportCalendar' => $baseDir . '/../lib/Command/ExportCalendar.php', 'OCA\\DAV\\Command\\FixCalendarSyncCommand' => $baseDir . '/../lib/Command/FixCalendarSyncCommand.php', 'OCA\\DAV\\Command\\ListAddressbooks' => $baseDir . '/../lib/Command/ListAddressbooks.php', + 'OCA\\DAV\\Command\\ListCalendarShares' => $baseDir . '/../lib/Command/ListCalendarShares.php', 'OCA\\DAV\\Command\\ListCalendars' => $baseDir . '/../lib/Command/ListCalendars.php', 'OCA\\DAV\\Command\\ListSubscriptions' => $baseDir . '/../lib/Command/ListSubscriptions.php', 'OCA\\DAV\\Command\\MoveCalendar' => $baseDir . '/../lib/Command/MoveCalendar.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 995d27adc2d9c..6369511343e0a 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -170,6 +170,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\CardDAV\\UserAddressBooks' => __DIR__ . '/..' . '/../lib/CardDAV/UserAddressBooks.php', 'OCA\\DAV\\CardDAV\\Validation\\CardDavValidatePlugin' => __DIR__ . '/..' . '/../lib/CardDAV/Validation/CardDavValidatePlugin.php', 'OCA\\DAV\\CardDAV\\Xml\\Groups' => __DIR__ . '/..' . '/../lib/CardDAV/Xml/Groups.php', + 'OCA\\DAV\\Command\\ClearCalendarUnshares' => __DIR__ . '/..' . '/../lib/Command/ClearCalendarUnshares.php', 'OCA\\DAV\\Command\\CreateAddressBook' => __DIR__ . '/..' . '/../lib/Command/CreateAddressBook.php', 'OCA\\DAV\\Command\\CreateCalendar' => __DIR__ . '/..' . '/../lib/Command/CreateCalendar.php', 'OCA\\DAV\\Command\\CreateSubscription' => __DIR__ . '/..' . '/../lib/Command/CreateSubscription.php', @@ -178,6 +179,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Command\\ExportCalendar' => __DIR__ . '/..' . '/../lib/Command/ExportCalendar.php', 'OCA\\DAV\\Command\\FixCalendarSyncCommand' => __DIR__ . '/..' . '/../lib/Command/FixCalendarSyncCommand.php', 'OCA\\DAV\\Command\\ListAddressbooks' => __DIR__ . '/..' . '/../lib/Command/ListAddressbooks.php', + 'OCA\\DAV\\Command\\ListCalendarShares' => __DIR__ . '/..' . '/../lib/Command/ListCalendarShares.php', 'OCA\\DAV\\Command\\ListCalendars' => __DIR__ . '/..' . '/../lib/Command/ListCalendars.php', 'OCA\\DAV\\Command\\ListSubscriptions' => __DIR__ . '/..' . '/../lib/Command/ListSubscriptions.php', 'OCA\\DAV\\Command\\MoveCalendar' => __DIR__ . '/..' . '/../lib/Command/MoveCalendar.php', diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index e69fe9ed3f04e..27aaca99e2dc3 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -90,6 +90,19 @@ * Code is heavily inspired by https://github.com/fruux/sabre-dav/blob/master/lib/CalDAV/Backend/PDO.php * * @package OCA\DAV\CalDAV + * + * @psalm-type CalendarInfo = array{ + * id: int, + * uri: string, + * principaluri: string, + * '{http://calendarserver.org/ns/}getctag': string, + * '{http://sabredav.org/ns}sync-token': int, + * '{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set': \Sabre\CalDAV\Xml\Property\SupportedCalendarComponentSet, + * '{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp': \Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp, + * '{DAV:}displayname': string, + * '{urn:ietf:params:xml:ns:caldav}calendar-timezone': ?string, + * '{http://nextcloud.com/ns}owner-displayname': string, + * } */ class CalDavBackend extends AbstractBackend implements SyncSupport, SubscriptionSupport, SchedulingSupport { use TTransactional; @@ -651,7 +664,8 @@ public function getCalendarByUri($principal, $uri) { } /** - * @return array{id: int, uri: string, '{http://calendarserver.org/ns/}getctag': string, '{http://sabredav.org/ns}sync-token': int, '{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set': SupportedCalendarComponentSet, '{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp': ScheduleCalendarTransp, '{urn:ietf:params:xml:ns:caldav}calendar-timezone': ?string }|null + * @psalm-return CalendarInfo|null + * @return array|null */ public function getCalendarById(int $calendarId): ?array { $fields = array_column($this->propertyMap, 0); @@ -3671,4 +3685,26 @@ protected function purgeObjectInvitations(string $eventId): void { ->where($cmd->expr()->eq('uid', $cmd->createNamedParameter($eventId, IQueryBuilder::PARAM_STR), IQueryBuilder::PARAM_STR)); $cmd->executeStatement(); } + + public function unshare(IShareable $shareable, string $principal): void { + $this->atomic(function () use ($shareable, $principal): void { + $calendarData = $this->getCalendarById($shareable->getResourceId()); + if ($calendarData === null) { + throw new \RuntimeException('Trying to update shares for non-existing calendar: ' . $shareable->getResourceId()); + } + + $oldShares = $this->getShares($shareable->getResourceId()); + $unshare = $this->calendarSharingBackend->unshare($shareable, $principal); + + if ($unshare) { + $this->dispatcher->dispatchTyped(new CalendarShareUpdatedEvent( + $shareable->getResourceId(), + $calendarData, + $oldShares, + [], + [$principal] + )); + } + }, $this->db); + } } diff --git a/apps/dav/lib/CalDAV/Calendar.php b/apps/dav/lib/CalDAV/Calendar.php index 789fe4b55c0b4..1d88d04a5e3ff 100644 --- a/apps/dav/lib/CalDAV/Calendar.php +++ b/apps/dav/lib/CalDAV/Calendar.php @@ -214,12 +214,8 @@ public function getOwner(): ?string { } public function delete() { - if (isset($this->calendarInfo['{http://owncloud.org/ns}owner-principal']) && - $this->calendarInfo['{http://owncloud.org/ns}owner-principal'] !== $this->calendarInfo['principaluri']) { - $principal = 'principal:' . parent::getOwner(); - $this->caldavBackend->updateShares($this, [], [ - $principal - ]); + if ($this->isShared()) { + $this->caldavBackend->unshare($this, 'principal:' . $this->getPrincipalURI()); return; } diff --git a/apps/dav/lib/Command/ClearCalendarUnshares.php b/apps/dav/lib/Command/ClearCalendarUnshares.php new file mode 100644 index 0000000000000..bb367a9cd0fa4 --- /dev/null +++ b/apps/dav/lib/Command/ClearCalendarUnshares.php @@ -0,0 +1,114 @@ +addArgument( + 'uid', + InputArgument::REQUIRED, + 'User whose unshares to clear' + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $user = (string)$input->getArgument('uid'); + if (!$this->userManager->userExists($user)) { + throw new \InvalidArgumentException("User $user is unknown"); + } + + $principal = $this->principal->getPrincipalByPath('principals/users/' . $user); + if ($principal === null) { + throw new \InvalidArgumentException("Unable to fetch principal for user $user "); + } + + $shares = $this->mapper->getSharesByPrincipals([$principal['uri']], 'calendar'); + $unshares = array_filter($shares, static fn ($share) => $share['access'] === BackendAlias::ACCESS_UNSHARED); + + if (count($unshares) === 0) { + $output->writeln("User $user has no calendar unshares"); + return self::SUCCESS; + } + + $rows = array_map(fn ($share) => $this->formatCalendarUnshare($share), $shares); + + $table = new Table($output); + $table + ->setHeaders(['Share Id', 'Calendar Id', 'Calendar URI', 'Calendar Name']) + ->setRows($rows) + ->render(); + + $output->writeln(''); + + /** @var QuestionHelper $helper */ + $helper = $this->getHelper('question'); + $question = new ConfirmationQuestion('Please confirm to delete the above calendar unshare entries [y/n]', false); + + if ($helper->ask($input, $output, $question)) { + $this->mapper->deleteUnsharesByPrincipal($principal['uri'], 'calendar'); + $output->writeln("Calendar unshares for user $user deleted"); + } + + return self::SUCCESS; + } + + private function formatCalendarUnshare(array $share): array { + $calendarInfo = $this->caldav->getCalendarById($share['resourceid']); + + $resourceUri = 'Resource not found'; + $resourceName = ''; + + if ($calendarInfo !== null) { + $resourceUri = $calendarInfo['uri']; + $resourceName = $calendarInfo['{DAV:}displayname']; + } + + return [ + $share['id'], + $share['resourceid'], + $resourceUri, + $resourceName, + ]; + } +} diff --git a/apps/dav/lib/Command/ListCalendarShares.php b/apps/dav/lib/Command/ListCalendarShares.php new file mode 100644 index 0000000000000..2729bc8053012 --- /dev/null +++ b/apps/dav/lib/Command/ListCalendarShares.php @@ -0,0 +1,131 @@ +addArgument( + 'uid', + InputArgument::REQUIRED, + 'User whose calendar shares will be listed' + ); + $this->addOption( + 'calendar-id', + '', + InputOption::VALUE_REQUIRED, + 'List only shares for the given calendar id id', + null, + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $user = (string)$input->getArgument('uid'); + if (!$this->userManager->userExists($user)) { + throw new \InvalidArgumentException("User $user is unknown"); + } + + $principal = $this->principal->getPrincipalByPath('principals/users/' . $user); + if ($principal === null) { + throw new \InvalidArgumentException("Unable to fetch principal for user $user"); + } + + $memberships = array_merge( + [$principal['uri']], + $this->principal->getGroupMembership($principal['uri']), + $this->principal->getCircleMembership($principal['uri']), + ); + + $shares = $this->mapper->getSharesByPrincipals($memberships, 'calendar'); + + $calendarId = $input->getOption('calendar-id'); + if ($calendarId !== null) { + $shares = array_filter($shares, fn ($share) => $share['resourceid'] === (int)$calendarId); + } + + $rows = array_map(fn ($share) => $this->formatCalendarShare($share), $shares); + + if (count($rows) > 0) { + $table = new Table($output); + $table + ->setHeaders(['Share Id', 'Calendar Id', 'Calendar URI', 'Calendar Name', 'Calendar Owner', 'Access By', 'Permissions']) + ->setRows($rows) + ->render(); + } else { + $output->writeln("User $user has no calendar shares"); + } + + return self::SUCCESS; + } + + private function formatCalendarShare(array $share): array { + $calendarInfo = $this->caldav->getCalendarById($share['resourceid']); + + $calendarUri = 'Resource not found'; + $calendarName = ''; + $calendarOwner = ''; + + if ($calendarInfo !== null) { + $calendarUri = $calendarInfo['uri']; + $calendarName = $calendarInfo['{DAV:}displayname']; + $calendarOwner = $calendarInfo['{http://nextcloud.com/ns}owner-displayname'] . ' (' . $calendarInfo['principaluri'] . ')'; + } + + $accessBy = match (true) { + str_starts_with($share['principaluri'], 'principals/users/') => 'Individual', + str_starts_with($share['principaluri'], 'principals/groups/') => 'Group (' . $share['principaluri'] . ')', + str_starts_with($share['principaluri'], 'principals/circles/') => 'Team (' . $share['principaluri'] . ')', + default => $share['principaluri'], + }; + + $permissions = match ($share['access']) { + Backend::ACCESS_READ => 'Read', + Backend::ACCESS_READ_WRITE => 'Read/Write', + Backend::ACCESS_UNSHARED => 'Unshare', + default => $share['access'], + }; + + return [ + $share['id'], + $share['resourceid'], + $calendarUri, + $calendarName, + $calendarOwner, + $accessBy, + $permissions, + ]; + } +} diff --git a/apps/dav/lib/DAV/Sharing/Backend.php b/apps/dav/lib/DAV/Sharing/Backend.php index 06a082628d3eb..de0d6891b7cf4 100644 --- a/apps/dav/lib/DAV/Sharing/Backend.php +++ b/apps/dav/lib/DAV/Sharing/Backend.php @@ -90,14 +90,6 @@ public function updateShares(IShareable $shareable, array $add, array $remove, a // Delete any possible direct shares (since the frontend does not separate between them) $this->service->deleteShare($shareable->getResourceId(), $principal); - - // Check if a user has a groupshare that they're trying to free themselves from - // If so we need to add a self::ACCESS_UNSHARED row - if (!str_contains($principal, 'group') - && $this->service->hasGroupShare($oldShares) - ) { - $this->service->unshare($shareable->getResourceId(), $principal); - } } } @@ -204,4 +196,45 @@ public function applyShareAcl(array $shares, array $acl): array { } return $acl; } + + public function unshare(IShareable $shareable, string $principalUri): bool { + $this->shareCache->clear(); + + $principal = $this->principalBackend->findByUri($principalUri, ''); + if (empty($principal)) { + return false; + } + + if ($shareable->getOwner() === $principal) { + return false; + } + + // Delete any possible direct shares (since the frontend does not separate between them) + $this->service->deleteShare($shareable->getResourceId(), $principal); + + $needsUnshare = $this->hasAccessByGroupOrCirclesMembership( + $shareable->getResourceId(), + $principal + ); + + if ($needsUnshare) { + $this->service->unshare($shareable->getResourceId(), $principal); + } + + return true; + } + + private function hasAccessByGroupOrCirclesMembership(int $resourceId, string $principal) { + $memberships = array_merge( + $this->principalBackend->getGroupMembership($principal, true), + $this->principalBackend->getCircleMembership($principal) + ); + + $shares = array_column( + $this->service->getShares($resourceId), + 'principaluri' + ); + + return count(array_intersect($memberships, $shares)) > 0; + } } diff --git a/apps/dav/lib/DAV/Sharing/SharingMapper.php b/apps/dav/lib/DAV/Sharing/SharingMapper.php index 0aec5b7fe81d1..e47222081896c 100644 --- a/apps/dav/lib/DAV/Sharing/SharingMapper.php +++ b/apps/dav/lib/DAV/Sharing/SharingMapper.php @@ -110,4 +110,28 @@ public function deleteAllSharesByUser(string $principaluri, string $resourceType ->andWhere($query->expr()->eq('type', $query->createNamedParameter($resourceType))) ->executeStatement(); } + + public function getSharesByPrincipals(array $principals, string $resourceType): array { + $query = $this->db->getQueryBuilder(); + $result = $query->select(['id', 'principaluri', 'type', 'access', 'resourceid']) + ->from('dav_shares') + ->where($query->expr()->in('principaluri', $query->createNamedParameter($principals, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY)) + ->andWhere($query->expr()->eq('type', $query->createNamedParameter($resourceType))) + ->orderBy('id') + ->executeQuery(); + + $rows = $result->fetchAll(); + $result->closeCursor(); + + return $rows; + } + + public function deleteUnsharesByPrincipal(string $principal, string $resourceType): void { + $query = $this->db->getQueryBuilder(); + $query->delete('dav_shares') + ->where($query->expr()->eq('principaluri', $query->createNamedParameter($principal))) + ->andWhere($query->expr()->eq('type', $query->createNamedParameter($resourceType))) + ->andWhere($query->expr()->eq('access', $query->createNamedParameter(Backend::ACCESS_UNSHARED, IQueryBuilder::PARAM_INT))) + ->executeStatement(); + } } diff --git a/apps/dav/lib/DAV/Sharing/SharingService.php b/apps/dav/lib/DAV/Sharing/SharingService.php index b9ac36ea066fa..11459e12d74c8 100644 --- a/apps/dav/lib/DAV/Sharing/SharingService.php +++ b/apps/dav/lib/DAV/Sharing/SharingService.php @@ -50,14 +50,4 @@ public function getUnshares(int $resourceId): array { public function getSharesForIds(array $resourceIds): array { return $this->mapper->getSharesForIds($resourceIds, $this->getResourceType()); } - - /** - * @param array $oldShares - * @return bool - */ - public function hasGroupShare(array $oldShares): bool { - return !empty(array_filter($oldShares, function (array $share) { - return $share['{http://owncloud.org/ns}group-share'] === true; - })); - } } diff --git a/apps/dav/lib/Events/CalendarShareUpdatedEvent.php b/apps/dav/lib/Events/CalendarShareUpdatedEvent.php index 762307b202f98..2e49df956ec9c 100644 --- a/apps/dav/lib/Events/CalendarShareUpdatedEvent.php +++ b/apps/dav/lib/Events/CalendarShareUpdatedEvent.php @@ -9,21 +9,22 @@ namespace OCA\DAV\Events; use OCP\EventDispatcher\Event; -use Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp; -use Sabre\CalDAV\Xml\Property\SupportedCalendarComponentSet; /** * Class CalendarShareUpdatedEvent * * @package OCA\DAV\Events * @since 20.0.0 + * + * @psalm-import-type CalendarInfo from \OCA\DAV\CalDAV\CalDavBackend */ class CalendarShareUpdatedEvent extends Event { /** * CalendarShareUpdatedEvent constructor. * * @param int $calendarId - * @param array{id: int, uri: string, '{http://calendarserver.org/ns/}getctag': string, '{http://sabredav.org/ns}sync-token': int, '{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set': SupportedCalendarComponentSet, '{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp': ScheduleCalendarTransp, '{urn:ietf:params:xml:ns:caldav}calendar-timezone': ?string } $calendarData + * @psalm-param CalendarInfo $calendarData + * @param array $calendarData * @param list $oldShares * @param list $added * @param list $removed @@ -47,7 +48,8 @@ public function getCalendarId(): int { } /** - * @return array{id: int, uri: string, '{http://calendarserver.org/ns/}getctag': string, '{http://sabredav.org/ns}sync-token': int, '{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set': SupportedCalendarComponentSet, '{urn:ietf:params:xml:ns:caldav}schedule-calendar-transp': ScheduleCalendarTransp, '{urn:ietf:params:xml:ns:caldav}calendar-timezone': ?string } + * @psalm-return CalendarInfo + * @return array * @since 20.0.0 */ public function getCalendarData(): array { diff --git a/apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php b/apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php new file mode 100644 index 0000000000000..407d9f9ba41e1 --- /dev/null +++ b/apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php @@ -0,0 +1,249 @@ +db = Server::get(IDBConnection::class); + + $this->userManager = $this->createMock(IUserManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->principalBackend = $this->createMock(Principal::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory->method('createInMemory') + ->willReturn(new \OC\Memcache\NullCache()); + $this->logger = new \Psr\Log\NullLogger(); + + $this->sharingMapper = new SharingMapper($this->db); + $this->sharingService = new \OCA\DAV\CalDAV\Sharing\Service($this->sharingMapper); + + $this->sharingBackend = new \OCA\DAV\CalDAV\Sharing\Backend( + $this->userManager, + $this->groupManager, + $this->principalBackend, + $this->cacheFactory, + $this->sharingService, + $this->logger + ); + + $this->removeFixtures(); + } + + protected function tearDown(): void { + $this->removeFixtures(); + } + + protected function removeFixtures(): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete('dav_shares') + ->where($qb->expr()->in('resourceid', $qb->createNamedParameter($this->resourceIds, IQueryBuilder::PARAM_INT_ARRAY))); + $qb->executeStatement(); + } + + public function testShareCalendarWithGroup(): void { + $calendar = $this->createMock(Calendar::class); + $calendar->method('getResourceId') + ->willReturn(10001); + $calendar->method('getOwner') + ->willReturn('principals/users/admin'); + + $this->principalBackend->method('findByUri') + ->willReturn('principals/groups/alice_bob'); + + $this->groupManager->method('groupExists') + ->willReturn(true); + + $this->sharingBackend->updateShares( + $calendar, + [['href' => 'principals/groups/alice_bob']], + [], + [] + ); + + $this->assertCount(1, $this->sharingService->getShares(10001)); + } + + public function testUnshareCalendarFromGroup(): void { + $calendar = $this->createMock(Calendar::class); + $calendar->method('getResourceId') + ->willReturn(10001); + $calendar->method('getOwner') + ->willReturn('principals/users/admin'); + + $this->principalBackend->method('findByUri') + ->willReturn('principals/groups/alice_bob'); + + $this->groupManager->method('groupExists') + ->willReturn(true); + + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [['href' => 'principals/groups/alice_bob']], + remove: [], + ); + + $this->assertCount(1, $this->sharingService->getShares(10001)); + + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [], + remove: ['principals/groups/alice_bob'], + ); + + $this->assertCount(0, $this->sharingService->getShares(10001)); + } + + public function testShareCalendarWithGroupAndUnshareAsUser(): void { + $calendar = $this->createMock(Calendar::class); + $calendar->method('getResourceId') + ->willReturn(10001); + $calendar->method('getOwner') + ->willReturn('principals/users/admin'); + + $this->principalBackend->method('findByUri') + ->willReturnMap([ + ['principals/groups/alice_bob', '', 'principals/groups/alice_bob'], + ['principals/users/bob', '', 'principals/users/bob'], + ]); + $this->principalBackend->method('getGroupMembership') + ->willReturn([ + 'principals/groups/alice_bob', + ]); + $this->principalBackend->method('getCircleMembership') + ->willReturn([]); + + $this->groupManager->method('groupExists') + ->willReturn(true); + + /* + * Owner is sharing the calendar with a group. + */ + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [['href' => 'principals/groups/alice_bob']], + remove: [], + ); + + $this->assertCount(1, $this->sharingService->getShares(10001)); + + /* + * Member of the group unshares the calendar. + */ + $this->sharingBackend->unshare( + shareable: $calendar, + principalUri: 'principals/users/bob' + ); + + $this->assertCount(1, $this->sharingService->getShares(10001)); + $this->assertCount(1, $this->sharingService->getUnshares(10001)); + } + + /** + * Tests the functionality of sharing a calendar with a user, then with a group (that includes the shared user), + * and subsequently unsharing it from the individual user. Verifies that the unshare operation correctly removes the specific user share + * without creating an additional unshare entry. + */ + public function testShareCalendarWithUserThenGroupThenUnshareUser(): void { + $calendar = $this->createMock(Calendar::class); + $calendar->method('getResourceId') + ->willReturn(10001); + $calendar->method('getOwner') + ->willReturn('principals/users/admin'); + + $this->principalBackend->method('findByUri') + ->willReturnMap([ + ['principals/groups/alice_bob', '', 'principals/groups/alice_bob'], + ['principals/users/bob', '', 'principals/users/bob'], + ]); + $this->principalBackend->method('getGroupMembership') + ->willReturn([ + 'principals/groups/alice_bob', + ]); + $this->principalBackend->method('getCircleMembership') + ->willReturn([]); + + $this->userManager->method('userExists') + ->willReturn(true); + $this->groupManager->method('groupExists') + ->willReturn(true); + + /* + * Step 1) The owner shares the calendar with a user. + */ + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [['href' => 'principals/users/bob']], + remove: [], + ); + + $this->assertCount(1, $this->sharingService->getShares(10001)); + + /* + * Step 2) The owner shares the calendar with a group that includes the + * user from step 1 as a member. + */ + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [['href' => 'principals/groups/alice_bob']], + remove: [], + ); + + $this->assertCount(2, $this->sharingService->getShares(10001)); + + /* + * Step 3) Unshare the calendar from user as owner. + */ + $this->sharingBackend->updateShares( + shareable: $calendar, + add: [], + remove: ['principals/users/bob'], + ); + + /* + * The purpose of this test is to ensure that removing a user from a share, as the owner, does not result in an "unshare" row being added. + * Instead, the actual user share should be removed. + */ + $this->assertCount(1, $this->sharingService->getShares(10001)); + $this->assertCount(0, $this->sharingService->getUnshares(10001)); + } + +} diff --git a/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php b/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php index bde6b1060c8ad..bcf8425403407 100644 --- a/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php +++ b/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php @@ -7,6 +7,8 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ +namespace OCA\DAV\Tests\integration\DAV\Sharing; + use OCA\DAV\DAV\Sharing\SharingMapper; use OCP\IDBConnection; use OCP\Server; diff --git a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php index 61c03c9e4c1bd..825d798e7e1b0 100644 --- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php +++ b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php @@ -17,6 +17,7 @@ use OCA\DAV\Events\CalendarDeletedEvent; use OCP\IConfig; use OCP\IL10N; +use Psr\Log\NullLogger; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\PropPatch; use Sabre\DAV\Xml\Property\Href; @@ -519,7 +520,7 @@ public function testCalendarSynchronization(): void { sort($stateLive['deleted']); // test live state $this->assertEquals($stateTest, $stateLive, 'Failed test delta sync state with events in calendar'); - + /** modify/delete events in calendar */ $this->deleteEvent($calendarId, $event1); $this->modifyEvent($calendarId, $event2, '20250701T140000Z', '20250701T150000Z'); @@ -1847,4 +1848,46 @@ public function testSearchShouldReturnObjectsInTheSameOrderMissingDate(): void { $this->assertEquals('Missing DTSTART 1', $results[2]['objects'][0]['SUMMARY'][0]); $this->assertEquals('Missing DTSTART 2', $results[3]['objects'][0]['SUMMARY'][0]); } + + public function testUnshare(): void { + $principalGroup = 'principal:' . self::UNIT_TEST_GROUP; + $principalUser = 'principal:' . self::UNIT_TEST_USER; + + $l10n = $this->createMock(IL10N::class); + $l10n->method('t') + ->willReturnCallback(fn ($text, $parameters = []) => vsprintf($text, $parameters)); + $config = $this->createMock(IConfig::class); + $logger = new NullLogger(); + + $this->principal->expects($this->exactly(2)) + ->method('findByUri') + ->willReturnMap([ + [$principalGroup, '', self::UNIT_TEST_GROUP], + [$principalUser, '', self::UNIT_TEST_USER], + ]); + $this->groupManager->expects($this->once()) + ->method('groupExists') + ->willReturn(true); + $this->dispatcher->expects($this->exactly(2)) + ->method('dispatchTyped'); + + $calendarId = $this->createTestCalendar(); + $calendarInfo = $this->backend->getCalendarById($calendarId); + + $calendar = new Calendar($this->backend, $calendarInfo, $l10n, $config, $logger); + + $this->backend->updateShares( + shareable: $calendar, + add: [ + ['href' => $principalGroup, 'readOnly' => false] + ], + remove: [] + ); + + $this->backend->unshare( + shareable: $calendar, + principal: $principalUser + ); + + } } diff --git a/apps/dav/tests/unit/CalDAV/CalendarTest.php b/apps/dav/tests/unit/CalDAV/CalendarTest.php index 6433af8c3402a..7f2d0052162ac 100644 --- a/apps/dav/tests/unit/CalDAV/CalendarTest.php +++ b/apps/dav/tests/unit/CalDAV/CalendarTest.php @@ -43,12 +43,13 @@ protected function setUp(): void { } public function testDelete(): void { - /** @var MockObject | CalDavBackend $backend */ - $backend = $this->getMockBuilder(CalDavBackend::class)->disableOriginalConstructor()->getMock(); - $backend->expects($this->once())->method('updateShares'); - $backend->expects($this->any())->method('getShares')->willReturn([ - ['href' => 'principal:user2'] - ]); + /** @var CalDavBackend&MockObject $backend */ + $backend = $this->createMock(CalDavBackend::class); + $backend->expects($this->never()) + ->method('updateShares'); + $backend->expects($this->once()) + ->method('unshare'); + $calendarInfo = [ '{http://owncloud.org/ns}owner-principal' => 'user1', 'principaluri' => 'user2', @@ -61,12 +62,13 @@ public function testDelete(): void { public function testDeleteFromGroup(): void { - /** @var MockObject | CalDavBackend $backend */ - $backend = $this->getMockBuilder(CalDavBackend::class)->disableOriginalConstructor()->getMock(); - $backend->expects($this->once())->method('updateShares'); - $backend->expects($this->any())->method('getShares')->willReturn([ - ['href' => 'principal:group2'] - ]); + /** @var CalDavBackend&MockObject $backend */ + $backend = $this->createMock(CalDavBackend::class); + $backend->expects($this->never()) + ->method('updateShares'); + $backend->expects($this->once()) + ->method('unshare'); + $calendarInfo = [ '{http://owncloud.org/ns}owner-principal' => 'user1', 'principaluri' => 'user2', diff --git a/apps/dav/tests/unit/Command/ListCalendarSharesTest.php b/apps/dav/tests/unit/Command/ListCalendarSharesTest.php new file mode 100644 index 0000000000000..3b3f3a1519ea8 --- /dev/null +++ b/apps/dav/tests/unit/Command/ListCalendarSharesTest.php @@ -0,0 +1,172 @@ +userManager = $this->createMock(IUserManager::class); + $this->principal = $this->createMock(Principal::class); + $this->caldav = $this->createMock(CalDavBackend::class); + $this->sharingMapper = $this->createMock(SharingMapper::class); + + $this->command = new ListCalendarShares( + $this->userManager, + $this->principal, + $this->caldav, + $this->sharingMapper, + ); + } + + public function testUserUnknown(): void { + $user = 'bob'; + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("User $user is unknown"); + + $this->userManager->expects($this->once()) + ->method('userExists') + ->with($user) + ->willReturn(false); + + $commandTester = new CommandTester($this->command); + $commandTester->execute([ + 'uid' => $user, + ]); + } + + public function testPrincipalNotFound(): void { + $user = 'bob'; + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Unable to fetch principal for user $user"); + + $this->userManager->expects($this->once()) + ->method('userExists') + ->with($user) + ->willReturn(true); + + $this->principal->expects($this->once()) + ->method('getPrincipalByPath') + ->with('principals/users/' . $user) + ->willReturn(null); + + $commandTester = new CommandTester($this->command); + $commandTester->execute([ + 'uid' => $user, + ]); + } + + public function testNoCalendarShares(): void { + $user = 'bob'; + + $this->userManager->expects($this->once()) + ->method('userExists') + ->with($user) + ->willReturn(true); + + $this->principal->expects($this->once()) + ->method('getPrincipalByPath') + ->with('principals/users/' . $user) + ->willReturn([ + 'uri' => 'principals/users/' . $user, + ]); + + $this->principal->expects($this->once()) + ->method('getGroupMembership') + ->willReturn([]); + $this->principal->expects($this->once()) + ->method('getCircleMembership') + ->willReturn([]); + + $this->sharingMapper->expects($this->once()) + ->method('getSharesByPrincipals') + ->willReturn([]); + + $commandTester = new CommandTester($this->command); + $commandTester->execute([ + 'uid' => $user, + ]); + + $this->assertStringContainsString( + "User $user has no calendar shares", + $commandTester->getDisplay() + ); + } + + public function testFilterByCalendarId(): void { + $user = 'bob'; + + $this->userManager->expects($this->once()) + ->method('userExists') + ->with($user) + ->willReturn(true); + + $this->principal->expects($this->once()) + ->method('getPrincipalByPath') + ->with('principals/users/' . $user) + ->willReturn([ + 'uri' => 'principals/users/' . $user, + ]); + + $this->principal->expects($this->once()) + ->method('getGroupMembership') + ->willReturn([]); + $this->principal->expects($this->once()) + ->method('getCircleMembership') + ->willReturn([]); + + $this->sharingMapper->expects($this->once()) + ->method('getSharesByPrincipals') + ->willReturn([ + [ + 'id' => 1000, + 'principaluri' => 'principals/users/bob', + 'type' => 'calendar', + 'access' => 2, + 'resourceid' => 10 + ], + [ + 'id' => 1001, + 'principaluri' => 'principals/users/bob', + 'type' => 'calendar', + 'access' => 3, + 'resourceid' => 11 + ], + ]); + + $commandTester = new CommandTester($this->command); + $commandTester->execute([ + 'uid' => $user, + '--calendar-id' => 10, + ]); + + $this->assertStringNotContainsString( + '1001', + $commandTester->getDisplay() + ); + } +} diff --git a/apps/dav/tests/unit/DAV/Sharing/BackendTest.php b/apps/dav/tests/unit/DAV/Sharing/BackendTest.php index 344d57d1808ae..dd2681d149f7f 100644 --- a/apps/dav/tests/unit/DAV/Sharing/BackendTest.php +++ b/apps/dav/tests/unit/DAV/Sharing/BackendTest.php @@ -214,10 +214,7 @@ public function testUnshareBob(): void { 'getResourceId' => 42, ]); $remove = [ - [ - 'href' => 'principal:principals/users/bob', - 'readOnly' => true, - ] + 'principal:principals/users/bob', ]; $principal = 'principals/users/bob'; @@ -229,9 +226,6 @@ public function testUnshareBob(): void { $this->calendarService->expects(self::once()) ->method('deleteShare') ->with($shareable->getResourceId(), $principal); - $this->calendarService->expects(self::once()) - ->method('hasGroupShare') - ->willReturn(false); $this->calendarService->expects(self::never()) ->method('unshare'); @@ -244,10 +238,7 @@ public function testUnshareWithBobGroup(): void { 'getResourceId' => 42, ]); $remove = [ - [ - 'href' => 'principal:principals/users/bob', - 'readOnly' => true, - ] + 'principal:principals/users/bob', ]; $oldShares = [ [ @@ -269,13 +260,8 @@ public function testUnshareWithBobGroup(): void { $this->calendarService->expects(self::once()) ->method('deleteShare') ->with($shareable->getResourceId(), 'principals/users/bob'); - $this->calendarService->expects(self::once()) - ->method('hasGroupShare') - ->with($oldShares) - ->willReturn(true); - $this->calendarService->expects(self::once()) - ->method('unshare') - ->with($shareable->getResourceId(), 'principals/users/bob'); + $this->calendarService->expects(self::never()) + ->method('unshare'); $this->backend->updateShares($shareable, [], $remove, $oldShares); } diff --git a/apps/dav/tests/unit/DAV/Sharing/SharingServiceTest.php b/apps/dav/tests/unit/DAV/Sharing/SharingServiceTest.php deleted file mode 100644 index fad077eeda36b..0000000000000 --- a/apps/dav/tests/unit/DAV/Sharing/SharingServiceTest.php +++ /dev/null @@ -1,58 +0,0 @@ -service = new Service($this->createMock(SharingMapper::class)); - } - - public function testHasGroupShare(): void { - $oldShares = [ - [ - 'href' => 'principal:principals/groups/bob', - 'commonName' => 'bob', - 'status' => 1, - 'readOnly' => true, - '{http://owncloud.org/ns}principal' => 'principals/groups/bob', - '{http://owncloud.org/ns}group-share' => true, - ], - [ - 'href' => 'principal:principals/users/bob', - 'commonName' => 'bob', - 'status' => 1, - 'readOnly' => true, - '{http://owncloud.org/ns}principal' => 'principals/users/bob', - '{http://owncloud.org/ns}group-share' => false, - ] - ]; - - $this->assertTrue($this->service->hasGroupShare($oldShares)); - - $oldShares = [ - [ - 'href' => 'principal:principals/users/bob', - 'commonName' => 'bob', - 'status' => 1, - 'readOnly' => true, - '{http://owncloud.org/ns}principal' => 'principals/users/bob', - '{http://owncloud.org/ns}group-share' => false, - ] - ]; - $this->assertFalse($this->service->hasGroupShare($oldShares)); - } -} From 023b98c44bd69e6029aeafcd4a059c6bd7926079 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Tue, 8 Apr 2025 17:16:59 +0200 Subject: [PATCH 2/2] fix(dav): only consider user's principal for unsharing entries Before: Find all entries in `dav_shares` with `access = 5` for the user's principal, as well as group and circle memberships. After: Find all entries in `dav_shares` with `access = 5` solely for the user's principal. Future support for unsharing group or circle principals could be considered as a feature enhancement. Signed-off-by: Daniel Kesselberg --- apps/dav/lib/CalDAV/CalDavBackend.php | 2 +- apps/dav/lib/CardDAV/CardDavBackend.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/CalDAV/CalDavBackend.php b/apps/dav/lib/CalDAV/CalDavBackend.php index 27aaca99e2dc3..5643e89d797b3 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -387,7 +387,7 @@ public function getCalendarsForUser($principalUri) { $subSelect->select('resourceid') ->from('dav_shares', 'd') ->where($subSelect->expr()->eq('d.access', $select->createNamedParameter(Backend::ACCESS_UNSHARED, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)) - ->andWhere($subSelect->expr()->in('d.principaluri', $select->createNamedParameter($principals, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY)); + ->andWhere($subSelect->expr()->eq('d.principaluri', $select->createNamedParameter($principalUri, IQueryBuilder::PARAM_STR), IQueryBuilder::PARAM_STR)); $select->select($fields) ->from('dav_shares', 's') diff --git a/apps/dav/lib/CardDAV/CardDavBackend.php b/apps/dav/lib/CardDAV/CardDavBackend.php index b15ed60707685..de209754ae437 100644 --- a/apps/dav/lib/CardDAV/CardDavBackend.php +++ b/apps/dav/lib/CardDAV/CardDavBackend.php @@ -137,7 +137,7 @@ public function getAddressBooksForUser($principalUri) { $subSelect->select('id') ->from('dav_shares', 'd') ->where($subSelect->expr()->eq('d.access', $select->createNamedParameter(\OCA\DAV\CardDAV\Sharing\Backend::ACCESS_UNSHARED, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)) - ->andWhere($subSelect->expr()->in('d.principaluri', $select->createNamedParameter($principals, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY)); + ->andWhere($subSelect->expr()->eq('d.principaluri', $select->createNamedParameter($principalUri, IQueryBuilder::PARAM_STR), IQueryBuilder::PARAM_STR)); $select->select(['a.id', 'a.uri', 'a.displayname', 'a.principaluri', 'a.description', 'a.synctoken', 's.access'])