From cf01026fc62fa271d073ba4b8eddaa8c9030a2e8 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Fri, 31 Jul 2026 12:31:11 +0200 Subject: [PATCH] fix(imip): don't report success when a calendar cannot process iMip handleIMip() checked IHandleImipMessage only inside the match, so a writable calendar unable to process iMip returned true without doing anything. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Daniel Kesselberg --- lib/private/Calendar/Manager.php | 65 +++++++++++----------- tests/lib/Calendar/ManagerTest.php | 87 +++++++++++++++++++++++++----- 2 files changed, 104 insertions(+), 48 deletions(-) diff --git a/lib/private/Calendar/Manager.php b/lib/private/Calendar/Manager.php index 63aa4e9b691ef..ec1a5d5d4f48a 100644 --- a/lib/private/Calendar/Manager.php +++ b/lib/private/Calendar/Manager.php @@ -37,8 +37,10 @@ use Sabre\VObject\ParseException; use Sabre\VObject\Reader; use Throwable; +use function array_filter; use function array_map; use function array_merge; +use function array_values; class Manager implements IManager { /** @@ -250,9 +252,14 @@ public function handleIMip( $userUri = 'principals/users/' . $userId; - $userCalendars = $this->getCalendarsForPrincipal($userUri); - if (empty($userCalendars)) { - $this->logger->warning('iMip message could not be processed because user has no calendars', $logContext); + /** @var list $userCalendars */ + $userCalendars = array_values(array_filter( + $this->getCalendarsForPrincipal($userUri), + fn (ICalendar $calendar): bool => $this->canHandleImip($calendar), + )); + + if ($userCalendars === []) { + $this->logger->warning('iMip message could not be processed because user has no calendar that can process iMip messages', $logContext); return false; } @@ -273,7 +280,7 @@ public function handleIMip( $vEvent = $vObject->VEVENT; if (!isset($vEvent->UID)) { - $this->logger->warning('iMip message event dose not contains a UID', $logContext); + $this->logger->warning('iMip message event does not contains a UID', $logContext); return false; } @@ -290,22 +297,14 @@ public function handleIMip( } if (!isset($vEvent->ATTENDEE)) { - $this->logger->warning('iMip message event dose not contains any attendees', $logContext); + $this->logger->warning('iMip message event does not contains any attendees', $logContext); return false; } foreach ($userCalendars as $calendar) { - if (!$calendar instanceof ICalendarIsWritable) { - continue; - } - if ($calendar->isDeleted() || !$calendar->isWritable()) { - continue; - } if (!empty($calendar->search('', [], ['uid' => $vEvent->UID->getValue()]))) { try { - if ($calendar instanceof IHandleImipMessage) { - $calendar->handleIMipMessage($userId, $vObject->serialize()); - } + $calendar->handleIMipMessage($userId, $vObject->serialize()); return true; } catch (CalendarException $e) { $logContext['exception'] = $e; @@ -316,26 +315,12 @@ public function handleIMip( } if (isset($options['absent']) && $options['absent'] === 'create') { - // retrieve the primary calendar for the user - $calendar = $this->getPrimaryCalendar($userId); - if ($calendar !== null && ( - !$calendar instanceof IHandleImipMessage || !$calendar instanceof ICalendarIsWritable || $calendar->isDeleted() || !$calendar->isWritable() - )) { - $calendar = null; - } - // if no primary calendar is set, use the first writable calendar - if ($calendar === null) { - foreach ($userCalendars as $userCalendar) { - if ($userCalendar instanceof IHandleImipMessage && $userCalendar instanceof ICalendarIsWritable && !$userCalendar->isDeleted() && $userCalendar->isWritable()) { - $calendar = $userCalendar; - break; - } - } - } - if ($calendar === null) { - $this->logger->warning('iMip message could not be processed because no writable calendar was found', $logContext); - return false; - } + // use the primary calendar of the user, otherwise the first one that can process iMip messages + $primaryCalendar = $this->getPrimaryCalendar($userId); + $calendar = $primaryCalendar !== null && $this->canHandleImip($primaryCalendar) + ? $primaryCalendar + : $userCalendars[0]; + if (!empty($options['absentCreateStatus'])) { $status = strtoupper($options['absentCreateStatus']); @@ -367,6 +352,18 @@ public function handleIMip( return false; } + /** + * Determines if a calendar can be used to process an iMip message + * + * @psalm-assert-if-true ICalendarIsWritable&IHandleImipMessage $calendar + */ + private function canHandleImip(ICalendar $calendar): bool { + return $calendar instanceof ICalendarIsWritable + && $calendar instanceof IHandleImipMessage + && $calendar->isWritable() + && !$calendar->isDeleted(); + } + /** * @since 31.0.0 * diff --git a/tests/lib/Calendar/ManagerTest.php b/tests/lib/Calendar/ManagerTest.php index ce7ce113ce8bd..ba527b7209749 100644 --- a/tests/lib/Calendar/ManagerTest.php +++ b/tests/lib/Calendar/ManagerTest.php @@ -41,6 +41,13 @@ interface ITestCalendar extends ICreateFromString, IHandleImipMessage, ICalendar } +/* + * A writable calendar that is unable to process iMip messages + */ +interface ITestCalendarWithoutImip extends ICreateFromString, ICalendarIsWritable { + +} + class ManagerTest extends TestCase { /** @var Coordinator&MockObject */ private $coordinator; @@ -342,7 +349,7 @@ public function testHandleImipWithNoCalendars(): void { ->willReturn([]); // construct logger returns $this->logger->expects(self::once())->method('warning') - ->with('iMip message could not be processed because user has no calendars'); + ->with('iMip message could not be processed because user has no calendar that can process iMip messages'); // construct parameters $userId = 'attendee1'; $calendar = $this->vCalendar1a; @@ -356,6 +363,12 @@ public function testHandleImipWithNoCalendars(): void { public function testHandleImipWithNoEvent(): void { // construct mock user calendar $userCalendar = $this->createMock(ITestCalendar::class); + $userCalendar->expects(self::once()) + ->method('isDeleted') + ->willReturn(false); + $userCalendar->expects(self::once()) + ->method('isWritable') + ->willReturn(true); // construct mock calendar manager and returns /** @var Manager&MockObject $manager */ $manager = $this->getMockBuilder(Manager::class) @@ -433,6 +446,12 @@ public function testHandleImipMissingOrganizerWithRecipient(): void { public function testHandleImipMissingOrganizerNoRecipient(): void { // construct mock user calendar $userCalendar = $this->createMock(ITestCalendar::class); + $userCalendar->expects(self::once()) + ->method('isDeleted') + ->willReturn(false); + $userCalendar->expects(self::once()) + ->method('isWritable') + ->willReturn(true); // construct mock calendar manager and returns /** @var Manager&MockObject $manager */ $manager = $this->getMockBuilder(Manager::class) @@ -467,6 +486,12 @@ public function testHandleImipMissingOrganizerNoRecipient(): void { public function testHandleImipWithNoUid(): void { // construct mock user calendar $userCalendar = $this->createMock(ITestCalendar::class); + $userCalendar->expects(self::once()) + ->method('isDeleted') + ->willReturn(false); + $userCalendar->expects(self::once()) + ->method('isWritable') + ->willReturn(true); // construct mock calendar manager and returns /** @var Manager&MockObject $manager */ $manager = $this->getMockBuilder(Manager::class) @@ -487,7 +512,7 @@ public function testHandleImipWithNoUid(): void { ->willReturn([$userCalendar]); // construct logger returns $this->logger->expects(self::once())->method('warning') - ->with('iMip message event dose not contains a UID'); + ->with('iMip message event does not contains a UID'); // construct parameters $userId = 'attendee1'; $calendar = $this->vCalendar1a; @@ -542,6 +567,42 @@ public function testHandleImipWithNoMatch(): void { $this->assertFalse($result); } + public function testHandleImipWithCalendarUnableToHandleImip(): void { + // construct mock user calendar which is writable but can not process iMip messages + $userCalendar = $this->createMock(ITestCalendarWithoutImip::class); + $userCalendar->expects(self::never()) + ->method('search'); + // construct mock calendar manager and returns + /** @var Manager&MockObject $manager */ + $manager = $this->getMockBuilder(Manager::class) + ->setConstructorArgs([ + $this->coordinator, + $this->container, + $this->logger, + $this->time, + $this->secureRandom, + $this->userManager, + $this->serverFactory, + $this->propertyMapper, + ]) + ->onlyMethods(['getCalendarsForPrincipal']) + ->getMock(); + $manager->expects(self::once()) + ->method('getCalendarsForPrincipal') + ->willReturn([$userCalendar]); + // construct logger returns + $this->logger->expects(self::once())->method('warning') + ->with('iMip message could not be processed because user has no calendar that can process iMip messages'); + // construct parameters + $userId = 'attendee1'; + $calendar = $this->vCalendar1a; + $calendar->add('METHOD', 'REQUEST'); + // test method + $result = $manager->handleIMip($userId, $calendar->serialize()); + // Assert + $this->assertFalse($result); + } + public function testHandleImip(): void { // construct mock user calendar $userCalendar = $this->createMock(ITestCalendar::class); @@ -586,10 +647,10 @@ public function testHandleImip(): void { public function testHandleImipWithAbsentCreateOption(): void { // construct mock user calendar (no matching event found) $userCalendar = $this->createMock(ITestCalendar::class); - $userCalendar->expects(self::exactly(2)) + $userCalendar->expects(self::once()) ->method('isDeleted') ->willReturn(false); - $userCalendar->expects(self::exactly(2)) + $userCalendar->expects(self::once()) ->method('isWritable') ->willReturn(true); $userCalendar->expects(self::once()) @@ -683,12 +744,11 @@ public function testHandleImipWithAbsentIgnoreOption(): void { public function testHandleImipWithAbsentCreateNoWritableCalendar(): void { // construct mock user calendar (not writable) $userCalendar = $this->createMock(ITestCalendar::class); - $userCalendar->expects(self::exactly(2)) - ->method('isDeleted') - ->willReturn(false); - $userCalendar->expects(self::exactly(2)) + $userCalendar->expects(self::once()) ->method('isWritable') ->willReturn(false); + $userCalendar->expects(self::never()) + ->method('search'); // construct mock calendar manager and returns /** @var Manager&MockObject $manager */ $manager = $this->getMockBuilder(Manager::class) @@ -707,12 +767,11 @@ public function testHandleImipWithAbsentCreateNoWritableCalendar(): void { $manager->expects(self::once()) ->method('getCalendarsForPrincipal') ->willReturn([$userCalendar]); - $manager->expects(self::once()) - ->method('getPrimaryCalendar') - ->willReturn(null); + $manager->expects(self::never()) + ->method('getPrimaryCalendar'); // construct logger returns $this->logger->expects(self::once())->method('warning') - ->with('iMip message could not be processed because no writable calendar was found'); + ->with('iMip message could not be processed because user has no calendar that can process iMip messages'); // construct parameters $userId = 'attendee1'; $calendar = $this->vCalendar1a; @@ -789,10 +848,10 @@ public function testHandleImipWithAbsentCreateUsesPrimaryCalendar(): void { public function testHandleImipWithAbsentCreateOverwritesExistingStatus(): void { // construct mock user calendar (no matching event found) $userCalendar = $this->createMock(ITestCalendar::class); - $userCalendar->expects(self::exactly(2)) + $userCalendar->expects(self::once()) ->method('isDeleted') ->willReturn(false); - $userCalendar->expects(self::exactly(2)) + $userCalendar->expects(self::once()) ->method('isWritable') ->willReturn(true); $userCalendar->expects(self::once())