From bc639175d5f69ffe34543e456c4976f36320a794 Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Tue, 1 Nov 2022 15:49:54 +0100 Subject: [PATCH] Fix duplicate event email notifications Signed-off-by: Richard Steinmetz --- .../CalDAV/Reminder/INotificationProvider.php | 3 + .../NotificationProvider/AbstractProvider.php | 3 + .../NotificationProvider/EmailProvider.php | 14 ++- .../NotificationProvider/PushProvider.php | 5 +- .../lib/CalDAV/Reminder/ReminderService.php | 18 +++- apps/dav/lib/CalDAV/Schedule/Plugin.php | 20 +++-- apps/dav/lib/Connector/Sabre/Principal.php | 40 +++++++++ .../EmailProviderTest.php | 90 ++++++++++++++++++- .../NotificationProvider/PushProviderTest.php | 5 +- .../CalDAV/Reminder/ReminderServiceTest.php | 9 +- .../unit/Connector/Sabre/PrincipalTest.php | 31 +++++++ 11 files changed, 219 insertions(+), 19 deletions(-) diff --git a/apps/dav/lib/CalDAV/Reminder/INotificationProvider.php b/apps/dav/lib/CalDAV/Reminder/INotificationProvider.php index a6b439c0b4fe6..505960ed66208 100644 --- a/apps/dav/lib/CalDAV/Reminder/INotificationProvider.php +++ b/apps/dav/lib/CalDAV/Reminder/INotificationProvider.php @@ -8,6 +8,7 @@ * @author Christoph Wurst * @author Georg Ehrke * @author Roeland Jago Douma + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -42,10 +43,12 @@ interface INotificationProvider { * * @param VEvent $vevent * @param string $calendarDisplayName + * @param string[] $principalEmailAddresses All email addresses associated to the principal owning the calendar object * @param IUser[] $users * @return void */ public function send(VEvent $vevent, string $calendarDisplayName, + array $principalEmailAddresses, array $users = []): void; } diff --git a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/AbstractProvider.php b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/AbstractProvider.php index 044e5fac4e237..68cdc245a481b 100644 --- a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/AbstractProvider.php +++ b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/AbstractProvider.php @@ -10,6 +10,7 @@ * @author Georg Ehrke * @author Joas Schilling * @author Roeland Jago Douma + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -89,11 +90,13 @@ public function __construct(ILogger $logger, * * @param VEvent $vevent * @param string $calendarDisplayName + * @param string[] $principalEmailAddresses * @param IUser[] $users * @return void */ abstract public function send(VEvent $vevent, string $calendarDisplayName, + array $principalEmailAddresses, array $users = []): void; /** diff --git a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php index 85590d506cf15..fdd16640d19f4 100644 --- a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php +++ b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php @@ -78,16 +78,28 @@ public function __construct(IConfig $config, * * @param VEvent $vevent * @param string $calendarDisplayName + * @param string[] $principalEmailAddresses * @param array $users * @throws \Exception */ public function send(VEvent $vevent, string $calendarDisplayName, + array $principalEmailAddresses, array $users = []):void { $fallbackLanguage = $this->getFallbackLanguage(); + $organizerEmailAddress = null; + if (isset($vevent->ORGANIZER)) { + $organizerEmailAddress = $this->getEMailAddressOfAttendee($vevent->ORGANIZER); + } + $emailAddressesOfSharees = $this->getEMailAddressesOfAllUsersWithWriteAccessToCalendar($users); - $emailAddressesOfAttendees = $this->getAllEMailAddressesFromEvent($vevent); + $emailAddressesOfAttendees = []; + if (count($principalEmailAddresses) === 0 + || ($organizerEmailAddress && in_array($organizerEmailAddress, $principalEmailAddresses, true)) + ) { + $emailAddressesOfAttendees = $this->getAllEMailAddressesFromEvent($vevent); + } // Quote from php.net: // If the input arrays have the same string keys, then the later value for that key will overwrite the previous one. diff --git a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/PushProvider.php b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/PushProvider.php index fb123960df825..a1336ae96d31d 100644 --- a/apps/dav/lib/CalDAV/Reminder/NotificationProvider/PushProvider.php +++ b/apps/dav/lib/CalDAV/Reminder/NotificationProvider/PushProvider.php @@ -10,6 +10,7 @@ * @author Georg Ehrke * @author Roeland Jago Douma * @author Thomas Citharel + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -81,11 +82,13 @@ public function __construct(IConfig $config, * * @param VEvent $vevent * @param string $calendarDisplayName + * @param string[] $principalEmailAddresses * @param IUser[] $users * @throws \Exception */ public function send(VEvent $vevent, - string $calendarDisplayName = null, + string $calendarDisplayName, + array $principalEmailAddresses, array $users = []):void { if ($this->config->getAppValue('dav', 'sendEventRemindersPush', 'no') !== 'yes') { return; diff --git a/apps/dav/lib/CalDAV/Reminder/ReminderService.php b/apps/dav/lib/CalDAV/Reminder/ReminderService.php index 109fc95e9bef7..366bec62d442b 100644 --- a/apps/dav/lib/CalDAV/Reminder/ReminderService.php +++ b/apps/dav/lib/CalDAV/Reminder/ReminderService.php @@ -11,6 +11,7 @@ * @author Joas Schilling * @author Roeland Jago Douma * @author Thomas Citharel + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -32,6 +33,7 @@ use DateTimeImmutable; use OCA\DAV\CalDAV\CalDavBackend; +use OCA\DAV\Connector\Sabre\Principal; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IGroup; use OCP\IGroupManager; @@ -66,6 +68,9 @@ class ReminderService { /** @var ITimeFactory */ private $timeFactory; + /** @var Principal */ + private $principalConnector; + public const REMINDER_TYPE_EMAIL = 'EMAIL'; public const REMINDER_TYPE_DISPLAY = 'DISPLAY'; public const REMINDER_TYPE_AUDIO = 'AUDIO'; @@ -90,19 +95,22 @@ class ReminderService { * @param IGroupManager $groupManager * @param CalDavBackend $caldavBackend * @param ITimeFactory $timeFactory + * @param Principal $principalConnector */ public function __construct(Backend $backend, NotificationProviderManager $notificationProviderManager, IUserManager $userManager, IGroupManager $groupManager, CalDavBackend $caldavBackend, - ITimeFactory $timeFactory) { + ITimeFactory $timeFactory, + Principal $principalConnector) { $this->backend = $backend; $this->notificationProviderManager = $notificationProviderManager; $this->userManager = $userManager; $this->groupManager = $groupManager; $this->caldavBackend = $caldavBackend; $this->timeFactory = $timeFactory; + $this->principalConnector = $principalConnector; } /** @@ -151,8 +159,14 @@ public function processReminders():void { $users[] = $user; } + $userPrincipalEmailAddresses = []; + $userPrincipal = $this->principalConnector->getPrincipalByPath($reminder['principaluri']); + if ($userPrincipal) { + $userPrincipalEmailAddresses = $this->principalConnector->getEmailAddressesOfPrincipal($userPrincipal); + } + $notificationProvider = $this->notificationProviderManager->getProvider($reminder['type']); - $notificationProvider->send($vevent, $reminder['displayname'], $users); + $notificationProvider->send($vevent, $reminder['displayname'], $userPrincipalEmailAddresses, $users); $this->deleteOrProcessNext($reminder, $vevent); } diff --git a/apps/dav/lib/CalDAV/Schedule/Plugin.php b/apps/dav/lib/CalDAV/Schedule/Plugin.php index 96bacce4454b6..21334724b6620 100644 --- a/apps/dav/lib/CalDAV/Schedule/Plugin.php +++ b/apps/dav/lib/CalDAV/Schedule/Plugin.php @@ -9,6 +9,7 @@ * @author Joas Schilling * @author Roeland Jago Douma * @author Thomas Citharel + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -45,6 +46,7 @@ use Sabre\VObject\Component\VCalendar; use Sabre\VObject\Component\VEvent; use Sabre\VObject\DateTimeParser; +use Sabre\VObject\Document; use Sabre\VObject\FreeBusyGenerator; use Sabre\VObject\ITip; use Sabre\VObject\Parameter; @@ -163,6 +165,14 @@ public function calendarObjectChange(RequestInterface $request, ResponseInterfac * @inheritDoc */ public function scheduleLocalDelivery(ITip\Message $iTipMessage):void { + /** @var Component|null $vevent */ + $vevent = $iTipMessage->message->VEVENT ?? null; + + // Strip VALARMs from incoming VEVENT + if ($vevent && isset($vevent->VALARM)) { + $vevent->remove('VALARM'); + } + parent::scheduleLocalDelivery($iTipMessage); // We only care when the message was successfully delivered locally @@ -199,18 +209,10 @@ public function scheduleLocalDelivery(ITip\Message $iTipMessage):void { return; } - if (!isset($iTipMessage->message)) { + if (!$vevent) { return; } - $vcalendar = $iTipMessage->message; - if (!isset($vcalendar->VEVENT)) { - return; - } - - /** @var Component $vevent */ - $vevent = $vcalendar->VEVENT; - // We don't support autoresponses for recurrencing events for now if (isset($vevent->RRULE) || isset($vevent->RDATE)) { return; diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php index 94e3978e67d1a..c1ad253593625 100644 --- a/apps/dav/lib/Connector/Sabre/Principal.php +++ b/apps/dav/lib/Connector/Sabre/Principal.php @@ -591,4 +591,44 @@ public function getCircleMembership($principal):array { return []; } + + /** + * Get all email addresses associated to a principal. + * + * @param array $principal Data from getPrincipal*() + * @return string[] All email addresses without the mailto: prefix + */ + public function getEmailAddressesOfPrincipal(array $principal): array { + $emailAddresses = []; + + if (($primaryAddress = $principal['{http://sabredav.org/ns}email-address'])) { + $emailAddresses[] = $primaryAddress; + } + + if (isset($principal['{DAV:}alternate-URI-set'])) { + foreach ($principal['{DAV:}alternate-URI-set'] as $address) { + if (str_starts_with($address, 'mailto:')) { + $emailAddresses[] = substr($address, 7); + } + } + } + + if (isset($principal['{urn:ietf:params:xml:ns:caldav}calendar-user-address-set'])) { + foreach ($principal['{urn:ietf:params:xml:ns:caldav}calendar-user-address-set'] as $address) { + if (str_starts_with($address, 'mailto:')) { + $emailAddresses[] = substr($address, 7); + } + } + } + + if (isset($principal['{http://calendarserver.org/ns/}email-address-set'])) { + foreach ($principal['{http://calendarserver.org/ns/}email-address-set'] as $address) { + if (str_starts_with($address, 'mailto:')) { + $emailAddresses[] = substr($address, 7); + } + } + } + + return array_values(array_unique($emailAddresses)); + } } diff --git a/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/EmailProviderTest.php b/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/EmailProviderTest.php index b5cbf94016ccc..67d93d94103a1 100644 --- a/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/EmailProviderTest.php +++ b/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/EmailProviderTest.php @@ -81,6 +81,7 @@ protected function setUp(): void { public function testSendWithoutAttendees():void { [$user1, $user2, $user3, , $user5] = $users = $this->getUsers(); + $principalEmailAddresses = [$user1->getEmailAddress()]; $enL10N = $this->createMock(IL10N::class); $enL10N->method('t') @@ -186,11 +187,12 @@ public function testSendWithoutAttendees():void { $this->setupURLGeneratorMock(2); $vcalendar = $this->getNoAttendeeVCalendar(); - $this->provider->send($vcalendar->VEVENT, $this->calendarDisplayName, $users); + $this->provider->send($vcalendar->VEVENT, $this->calendarDisplayName, $principalEmailAddresses, $users); } - public function testSendWithAttendees(): void { + public function testSendWithAttendeesWhenOwnerIsOrganizer(): void { [$user1, $user2, $user3, , $user5] = $users = $this->getUsers(); + $principalEmailAddresses = [$user1->getEmailAddress()]; $enL10N = $this->createMock(IL10N::class); $enL10N->method('t') @@ -282,7 +284,81 @@ public function testSendWithAttendees(): void { $this->setupURLGeneratorMock(2); $vcalendar = $this->getAttendeeVCalendar(); - $this->provider->send($vcalendar->VEVENT, $this->calendarDisplayName, $users); + $this->provider->send($vcalendar->VEVENT, $this->calendarDisplayName, $principalEmailAddresses, $users); + } + + public function testSendWithAttendeesWhenOwnerIsAttendee(): void { + [$user1, $user2, $user3] = $this->getUsers(); + $users = [$user2, $user3]; + $principalEmailAddresses = [$user2->getEmailAddress()]; + + $deL10N = $this->createMock(IL10N::class); + $deL10N->method('t') + ->willReturnArgument(0); + $deL10N->method('l') + ->willReturnArgument(0); + + $this->l10nFactory + ->method('getUserLanguage') + ->willReturnMap([ + [$user2, 'de'], + [$user3, 'de'], + ]); + + $this->l10nFactory + ->method('findGenericLanguage') + ->willReturn('en'); + + $this->l10nFactory + ->method('languageExists') + ->willReturnMap([ + ['dav', 'de', true], + ]); + + $this->l10nFactory + ->method('get') + ->willReturnMap([ + ['dav', 'de', null, $deL10N], + ]); + + $template1 = $this->getTemplateMock(); + $message12 = $this->getMessageMock('uid2@example.com', $template1); + $message13 = $this->getMessageMock('uid3@example.com', $template1); + + $this->mailer->expects(self::once()) + ->method('createEMailTemplate') + ->with('dav.calendarReminder') + ->willReturnOnConsecutiveCalls( + $template1, + ); + $this->mailer->expects($this->atLeastOnce()) + ->method('validateMailAddress') + ->willReturnMap([ + ['foo1@example.org', true], + ['foo3@example.org', true], + ['foo4@example.org', true], + ['uid1@example.com', true], + ['uid2@example.com', true], + ['uid3@example.com', true], + ['invalid', false], + ]); + $this->mailer->expects($this->exactly(2)) + ->method('createMessage') + ->with() + ->willReturnOnConsecutiveCalls( + $message12, + $message13, + ); + $this->mailer->expects($this->exactly(2)) + ->method('send') + ->withConsecutive( + [$message12], + [$message13], + )->willReturn([]); + $this->setupURLGeneratorMock(1); + + $vcalendar = $this->getAttendeeVCalendar(); + $this->provider->send($vcalendar->VEVENT, $this->calendarDisplayName, $principalEmailAddresses, $users); } /** @@ -392,6 +468,14 @@ private function getAttendeeVCalendar():VCalendar { 'DESCRIPTION' => 'DESCRIPTION 456', ]); + $vcalendar->VEVENT->add( + 'ORGANIZER', + 'mailto:uid1@example.com', + [ + 'LANG' => 'en' + ] + ); + $vcalendar->VEVENT->add( 'ATTENDEE', 'mailto:foo1@example.org', diff --git a/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/PushProviderTest.php b/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/PushProviderTest.php index 9e9759f5eb884..62571e855ca8b 100644 --- a/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/PushProviderTest.php +++ b/apps/dav/tests/unit/CalDAV/Reminder/NotificationProvider/PushProviderTest.php @@ -10,6 +10,7 @@ * @author Georg Ehrke * @author Roeland Jago Douma * @author Thomas Citharel + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -107,7 +108,7 @@ public function testNotSend(): void { $users = [$user1, $user2, $user3]; - $this->provider->send($this->vcalendar->VEVENT, $this->calendarDisplayName, $users); + $this->provider->send($this->vcalendar->VEVENT, $this->calendarDisplayName, [], $users); } public function testSend(): void { @@ -160,7 +161,7 @@ public function testSend(): void { ->method('notify') ->with($notification3); - $this->provider->send($this->vcalendar->VEVENT, $this->calendarDisplayName, $users); + $this->provider->send($this->vcalendar->VEVENT, $this->calendarDisplayName, [], $users); } /** diff --git a/apps/dav/tests/unit/CalDAV/Reminder/ReminderServiceTest.php b/apps/dav/tests/unit/CalDAV/Reminder/ReminderServiceTest.php index 660b1a48fd939..7edd80a0e3773 100644 --- a/apps/dav/tests/unit/CalDAV/Reminder/ReminderServiceTest.php +++ b/apps/dav/tests/unit/CalDAV/Reminder/ReminderServiceTest.php @@ -9,6 +9,7 @@ * @author Georg Ehrke * @author Roeland Jago Douma * @author Thomas Citharel + * @author Richard Steinmetz * * @license GNU AGPL version 3 or any later version * @@ -33,6 +34,7 @@ use OCA\DAV\CalDAV\Reminder\INotificationProvider; use OCA\DAV\CalDAV\Reminder\NotificationProviderManager; use OCA\DAV\CalDAV\Reminder\ReminderService; +use OCA\DAV\Connector\Sabre\Principal; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IGroupManager; use OCP\IUser; @@ -66,6 +68,9 @@ class ReminderServiceTest extends TestCase { /** @var ReminderService */ private $reminderService; + /** @var Principal|\PHPUnit\Framework\MockObject\MockObject */ + private $principalConnector; + public const CALENDAR_DATA = <<groupManager = $this->createMock(IGroupManager::class); $this->caldavBackend = $this->createMock(CalDavBackend::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->principalConnector = $this->createMock(Principal::class); $this->caldavBackend->method('getShares')->willReturn([]); @@ -202,7 +208,8 @@ protected function setUp(): void { $this->userManager, $this->groupManager, $this->caldavBackend, - $this->timeFactory); + $this->timeFactory, + $this->principalConnector); } public function testOnCalendarObjectDelete():void { diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php index 86413e4a366a5..3ffbe7c405821 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php @@ -11,6 +11,7 @@ * @author Morris Jobke * @author Roeland Jago Douma * @author Thomas Müller + * @author Richard Steinmetz * * @license AGPL-3.0 * @@ -925,4 +926,34 @@ public function findByUriWithoutGroupRestrictionDataProvider(): array { ['mailto:user3@foo.bar', 'user3@foo.bar', 'principals/users/user3'], ]; } + + public function testGetEmailAddressesOfPrincipal(): void { + $principal = [ + '{http://sabredav.org/ns}email-address' => 'bar@company.org', + '{DAV:}alternate-URI-set' => [ + '/some/url', + 'mailto:foo@bar.com', + 'mailto:duplicate@example.com', + ], + '{urn:ietf:params:xml:ns:caldav}calendar-user-address-set' => [ + 'mailto:bernard@example.com', + 'mailto:bernard.desruisseaux@example.com', + ], + '{http://calendarserver.org/ns/}email-address-set' => [ + 'mailto:duplicate@example.com', + 'mailto:user@some.org', + ], + ]; + + $expected = [ + 'bar@company.org', + 'foo@bar.com', + 'duplicate@example.com', + 'bernard@example.com', + 'bernard.desruisseaux@example.com', + 'user@some.org', + ]; + $actual = $this->connector->getEmailAddressesOfPrincipal($principal); + $this->assertEquals($expected, $actual); + } }