diff options
author | Daniel Kesselberg <mail@danielkesselberg.de> | 2025-04-07 14:59:20 +0200 |
---|---|---|
committer | Daniel Kesselberg <mail@danielkesselberg.de> | 2025-05-14 09:03:32 +0200 |
commit | c05d3fdb2e54c89a08ae7085fe3b96bf7e8214d9 (patch) | |
tree | 379a1cf415d1f855f63a24f150d8d6c4f48d1f89 | |
parent | a338772ddd135d5314275fa314ba34be6aec9683 (diff) | |
download | nextcloud-server-c05d3fdb2e54c89a08ae7085fe3b96bf7e8214d9.tar.gz nextcloud-server-c05d3fdb2e54c89a08ae7085fe3b96bf7e8214d9.zip |
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 <mail@danielkesselberg.de>
18 files changed, 846 insertions, 118 deletions
diff --git a/apps/dav/appinfo/info.xml b/apps/dav/appinfo/info.xml index ac8886555f6..e0f4608e8c2 100644 --- a/apps/dav/appinfo/info.xml +++ b/apps/dav/appinfo/info.xml @@ -55,6 +55,7 @@ </repair-steps> <commands> + <command>OCA\DAV\Command\ClearCalendarUnshares</command> <command>OCA\DAV\Command\CreateAddressBook</command> <command>OCA\DAV\Command\CreateCalendar</command> <command>OCA\DAV\Command\CreateSubscription</command> @@ -63,6 +64,7 @@ <command>OCA\DAV\Command\ExportCalendar</command> <command>OCA\DAV\Command\FixCalendarSyncCommand</command> <command>OCA\DAV\Command\ListAddressbooks</command> + <command>OCA\DAV\Command\ListCalendarShares</command> <command>OCA\DAV\Command\ListCalendars</command> <command>OCA\DAV\Command\ListSubscriptions</command> <command>OCA\DAV\Command\MoveCalendar</command> diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 466ef98d433..913f5b46d98 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -155,6 +155,7 @@ return array( '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 @@ return array( '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 995d27adc2d..6369511343e 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 e69fe9ed3f0..27aaca99e2d 100644 --- a/apps/dav/lib/CalDAV/CalDavBackend.php +++ b/apps/dav/lib/CalDAV/CalDavBackend.php @@ -90,6 +90,19 @@ use function time; * 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 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription } /** - * @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 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription ->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 789fe4b55c0..1d88d04a5e3 100644 --- a/apps/dav/lib/CalDAV/Calendar.php +++ b/apps/dav/lib/CalDAV/Calendar.php @@ -214,12 +214,8 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IRestorable, IShareable } 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 00000000000..bb367a9cd0f --- /dev/null +++ b/apps/dav/lib/Command/ClearCalendarUnshares.php @@ -0,0 +1,114 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\DAV\Command; + +use OCA\DAV\CalDAV\CalDavBackend; +use OCA\DAV\CalDAV\Sharing\Backend; +use OCA\DAV\CalDAV\Sharing\Service; +use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\DAV\Sharing\Backend as BackendAlias; +use OCA\DAV\DAV\Sharing\SharingMapper; +use OCP\IAppConfig; +use OCP\IUserManager; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\QuestionHelper; +use Symfony\Component\Console\Helper\Table; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Question\ConfirmationQuestion; + +#[AsCommand( + name: 'dav:clear-calendar-unshares', + description: 'Clear calendar unshares for a user', + hidden: false, +)] +class ClearCalendarUnshares extends Command { + public function __construct( + private IUserManager $userManager, + private IAppConfig $appConfig, + private Principal $principal, + private CalDavBackend $caldav, + private Backend $sharingBackend, + private Service $sharingService, + private SharingMapper $mapper, + ) { + parent::__construct(); + } + + protected function configure(): void { + $this->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 00000000000..2729bc80530 --- /dev/null +++ b/apps/dav/lib/Command/ListCalendarShares.php @@ -0,0 +1,131 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\DAV\Command; + +use OCA\DAV\CalDAV\CalDavBackend; +use OCA\DAV\CalDAV\Sharing\Backend; +use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\DAV\Sharing\SharingMapper; +use OCP\IUserManager; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\Table; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; + +#[AsCommand( + name: 'dav:list-calendar-shares', + description: 'List all calendar shares for a user', + hidden: false, +)] +class ListCalendarShares extends Command { + public function __construct( + private IUserManager $userManager, + private Principal $principal, + private CalDavBackend $caldav, + private SharingMapper $mapper, + ) { + parent::__construct(); + } + + protected function configure(): void { + $this->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 06a082628d3..de0d6891b7c 100644 --- a/apps/dav/lib/DAV/Sharing/Backend.php +++ b/apps/dav/lib/DAV/Sharing/Backend.php @@ -90,14 +90,6 @@ abstract class Backend { // 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 @@ abstract class Backend { } 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 0aec5b7fe81..e4722208189 100644 --- a/apps/dav/lib/DAV/Sharing/SharingMapper.php +++ b/apps/dav/lib/DAV/Sharing/SharingMapper.php @@ -110,4 +110,28 @@ class SharingMapper { ->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 b9ac36ea066..11459e12d74 100644 --- a/apps/dav/lib/DAV/Sharing/SharingService.php +++ b/apps/dav/lib/DAV/Sharing/SharingService.php @@ -50,14 +50,4 @@ abstract class SharingService { 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 762307b202f..2e49df956ec 100644 --- a/apps/dav/lib/Events/CalendarShareUpdatedEvent.php +++ b/apps/dav/lib/Events/CalendarShareUpdatedEvent.php @@ -9,21 +9,22 @@ declare(strict_types=1); 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<array{href: string, commonName: string, status: int, readOnly: bool, '{http://owncloud.org/ns}principal': string, '{http://owncloud.org/ns}group-share': bool}> $oldShares * @param list<array{href: string, commonName: string, readOnly: bool}> $added * @param list<string> $removed @@ -47,7 +48,8 @@ class CalendarShareUpdatedEvent extends Event { } /** - * @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 00000000000..407d9f9ba41 --- /dev/null +++ b/apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php @@ -0,0 +1,249 @@ +<?php + +declare(strict_types=1); + +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\DAV\Tests\integration\DAV\Sharing; + +use OCA\DAV\CalDAV\Calendar; +use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\DAV\Sharing\Backend; +use OCA\DAV\DAV\Sharing\SharingMapper; +use OCA\DAV\DAV\Sharing\SharingService; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\ICacheFactory; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\Server; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * @group DB + */ +class CalDavSharingBackendTest extends TestCase { + + private IDBConnection $db; + private IUserManager $userManager; + private IGroupManager $groupManager; + private Principal $principalBackend; + private ICacheFactory $cacheFactory; + private LoggerInterface $logger; + private SharingMapper $sharingMapper; + private SharingService $sharingService; + private Backend $sharingBackend; + + private $resourceIds = [10001]; + + protected function setUp(): void { + parent::setUp(); + + $this->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 bde6b1060c8..bcf84254034 100644 --- a/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php +++ b/apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php @@ -7,6 +7,8 @@ declare(strict_types=1); * 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 61c03c9e4c1..825d798e7e1 100644 --- a/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php +++ b/apps/dav/tests/unit/CalDAV/CalDavBackendTest.php @@ -17,6 +17,7 @@ use OCA\DAV\DAV\Sharing\Plugin as SharingPlugin; 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 @@ EOD; 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 @@ EOD; $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 6433af8c340..7f2d0052162 100644 --- a/apps/dav/tests/unit/CalDAV/CalendarTest.php +++ b/apps/dav/tests/unit/CalDAV/CalendarTest.php @@ -43,12 +43,13 @@ class CalendarTest extends TestCase { } 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 @@ class CalendarTest extends TestCase { 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 00000000000..3b3f3a1519e --- /dev/null +++ b/apps/dav/tests/unit/Command/ListCalendarSharesTest.php @@ -0,0 +1,172 @@ +<?php + +declare(strict_types=1); +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace OCA\DAV\Tests\Command; + +use OCA\DAV\CalDAV\CalDavBackend; +use OCA\DAV\Command\ListCalendarShares; +use OCA\DAV\Connector\Sabre\Principal; +use OCA\DAV\DAV\Sharing\SharingMapper; +use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; +use Symfony\Component\Console\Tester\CommandTester; +use Test\TestCase; + +class ListCalendarSharesTest extends TestCase { + + private IUserManager&MockObject $userManager; + private Principal&MockObject $principal; + private CalDavBackend&MockObject $caldav; + private SharingMapper $sharingMapper; + private ListCalendarShares $command; + + protected function setUp(): void { + parent::setUp(); + + $this->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 344d57d1808..dd2681d149f 100644 --- a/apps/dav/tests/unit/DAV/Sharing/BackendTest.php +++ b/apps/dav/tests/unit/DAV/Sharing/BackendTest.php @@ -214,10 +214,7 @@ class BackendTest extends TestCase { 'getResourceId' => 42, ]); $remove = [ - [ - 'href' => 'principal:principals/users/bob', - 'readOnly' => true, - ] + 'principal:principals/users/bob', ]; $principal = 'principals/users/bob'; @@ -229,9 +226,6 @@ class BackendTest extends TestCase { $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 @@ class BackendTest extends TestCase { 'getResourceId' => 42, ]); $remove = [ - [ - 'href' => 'principal:principals/users/bob', - 'readOnly' => true, - ] + 'principal:principals/users/bob', ]; $oldShares = [ [ @@ -269,13 +260,8 @@ class BackendTest extends TestCase { $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 fad077eeda3..00000000000 --- a/apps/dav/tests/unit/DAV/Sharing/SharingServiceTest.php +++ /dev/null @@ -1,58 +0,0 @@ -<?php - -declare(strict_types=1); -/** - * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-or-later - */ -namespace OCA\DAV\Tests\unit\DAV\Sharing; - -use OCA\DAV\CalDAV\Sharing\Service; -use OCA\DAV\DAV\Sharing\SharingMapper; -use OCA\DAV\DAV\Sharing\SharingService; -use Test\TestCase; - -class SharingServiceTest extends TestCase { - - private SharingService $service; - - protected function setUp(): void { - parent::setUp(); - $this->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)); - } -} |