diff --git a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php index 18d7559f1a702..1df4bc01992a1 100644 --- a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php +++ b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php @@ -11,9 +11,11 @@ use OCA\DAV\CalDAV\CalendarObject; use OCA\DAV\CalDAV\EventComparisonService; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\IAppConfig; +use OCP\IUser; use OCP\IUserSession; use OCP\Mail\IEmailValidator; use OCP\Mail\IMailer; @@ -66,6 +68,7 @@ public function __construct( private EventComparisonService $eventComparisonService, private IMailManager $mailManager, private IEmailValidator $emailValidator, + private IAccountManager $accountManager, ) { parent::__construct(''); } @@ -184,21 +187,17 @@ public function schedule(Message $iTipMessage) { } $this->imipService->setL10nFromAttendee($attendee); - // Build the sender name. + $sender = substr($iTipMessage->sender, 7); + // Due to a bug in sabre, the senderName property for an iTIP message can actually also be a VObject Property - // If the iTIP message senderName is null or empty use the user session name as the senderName if (($iTipMessage->senderName instanceof Parameter) && !empty(trim($iTipMessage->senderName->getValue()))) { $senderName = trim($iTipMessage->senderName->getValue()); } elseif (is_string($iTipMessage->senderName) && !empty(trim($iTipMessage->senderName))) { $senderName = trim($iTipMessage->senderName); - } elseif ($this->userSession->getUser() !== null) { - $senderName = trim($this->userSession->getUser()->getDisplayName()); } else { - $senderName = ''; + $senderName = $this->getSenderNameFor($sender); } - $sender = substr($iTipMessage->sender, 7); - $replyingAttendee = null; switch (strtolower($iTipMessage->method)) { case self::METHOD_REPLY: @@ -338,6 +337,40 @@ public function schedule(Message $iTipMessage) { } } + /** + * Messages are regularly brokered on behalf of somebody else, so the + * session user's name is only used when the sender address is one of + * theirs. + */ + private function getSenderNameFor(string $sender): ?string { + $user = $this->userSession->getUser(); + if ($user !== null && $this->isAddressOfUser($sender, $user)) { + return trim($user->getDisplayName()) ?: null; + } + + return null; + } + + /** + * Profile email addresses are part of the user's calendar-user-address-set + * and therefore valid sender addresses next to the system email address. + */ + private function isAddressOfUser(string $address, IUser $user): bool { + if (strcasecmp((string)$user->getEMailAddress(), $address) === 0) { + return true; + } + + $emailCollection = $this->accountManager->getAccount($user) + ->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + foreach ($emailCollection->getProperties() as $property) { + if (strcasecmp($property->getValue(), $address) === 0) { + return true; + } + } + + return false; + } + /** * @return ?VCalendar */ diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index fdaa35e4a4d16..e5479702bc99d 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -360,6 +360,7 @@ public function __construct( \OCP\Server::get(EventComparisonService::class), \OCP\Server::get(\OCP\Mail\Provider\IManager::class), \OCP\Server::get(IEmailValidator::class), + \OCP\Server::get(IAccountManager::class), )); } $this->server->addPlugin(new \OCA\DAV\CalDAV\Search\SearchPlugin()); diff --git a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php index ec6a4567e9c92..4763f675ddff7 100644 --- a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php +++ b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginCharsetTest.php @@ -13,6 +13,7 @@ use OCA\DAV\CalDAV\EventComparisonService; use OCA\DAV\CalDAV\Schedule\IMipPlugin; use OCA\DAV\CalDAV\Schedule\IMipService; +use OCP\Accounts\IAccountManager; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Config\IUserConfig; use OCP\Defaults; @@ -132,6 +133,7 @@ protected function setUp(): void { $this->eventComparisonService, $this->mailManager, $this->getEmailValidatorWithStrictEmailCheck(), + $this->createMock(IAccountManager::class), ); // ITipMessage diff --git a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php index 048dcd13b67e3..b7cbcad149ef6 100644 --- a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php +++ b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php @@ -12,6 +12,10 @@ use OCA\DAV\CalDAV\EventComparisonService; use OCA\DAV\CalDAV\Schedule\IMipPlugin; use OCA\DAV\CalDAV\Schedule\IMipService; +use OCP\Accounts\IAccount; +use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\IAppConfig; @@ -21,6 +25,7 @@ use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; use OCP\Mail\IMessage; +use OCP\Mail\Provider\Address; use OCP\Mail\Provider\IManager as IMailManager; use OCP\Mail\Provider\IMessage as IMailMessageNew; use OCP\Mail\Provider\IMessageSend as IMailMessageSend; @@ -58,6 +63,7 @@ class IMipPluginTest extends TestCase { private IMailManager&MockObject $mailManager; private IMailServiceMock&MockObject $mailService; private IMailMessageNew&MockObject $mailMessageNew; + private IAccountManager&MockObject $accountManager; protected function setUp(): void { $this->mailMessage = $this->createMock(IMessage::class); @@ -101,6 +107,8 @@ protected function setUp(): void { $this->mailMessageNew = $this->createMock(IMailMessageNew::class); + $this->accountManager = $this->createMock(IAccountManager::class); + $this->plugin = new IMipPlugin( $this->config, $this->mailer, @@ -112,6 +120,7 @@ protected function setUp(): void { $this->eventComparisonService, $this->mailManager, $this->getEmailValidatorWithStrictEmailCheck(), + $this->accountManager, ); } @@ -213,6 +222,9 @@ public function testParsingSingle(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -505,6 +517,9 @@ public function testParsingRecurrence(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -632,6 +647,9 @@ public function testFailedDelivery(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -850,6 +868,9 @@ public function testMailProviderDisabled(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -953,6 +974,9 @@ public function testNoOldEvent(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1052,6 +1076,9 @@ public function testNoButtons(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1203,6 +1230,9 @@ public function testExternalAttendeesDisabledForSystemUser(): void { $this->user->expects(self::any()) ->method('getDisplayName') ->willReturn('Mr. Wizard'); + $this->user->expects(self::any()) + ->method('getEMailAddress') + ->willReturn('gandalf@wiz.ard'); $this->userSession->expects(self::any()) ->method('getUser') ->willReturn($this->user); @@ -1237,4 +1267,197 @@ public function testExternalAttendeesDisabledForSystemUser(): void { $this->plugin->schedule($message); $this->assertEquals('1.1', $message->getScheduleStatus()); } + + /** + * Runs schedule() for an organizer sourced REQUEST whose iTip message + * carries no sender name and captures the resulting From and Reply-To + * headers, sent either via the system or the user's mail account. + * + * @return array{from: ?array, replyTo: ?array} + */ + private function scheduleWithoutSenderName(string $organizer, string $recipient, bool $viaMailProvider = false): array { + $vCalendar = new VCalendar(); + $vEvent = new VEvent($vCalendar, 'VEVENT', [ + 'UID' => 'uid-1234', + 'SEQUENCE' => 1, + 'SUMMARY' => 'Meeting', + 'DTSTART' => new \DateTime('2017-01-01 00:00:00'), + ]); + $vEvent->add('ORGANIZER', 'mailto:' . $organizer); + $vEvent->add('ATTENDEE', 'mailto:' . $recipient, ['RSVP' => 'TRUE', 'CN' => 'Frodo']); + + $message = new Message(); + $message->method = 'REQUEST'; + $message->message = $vCalendar; + $message->sender = 'mailto:' . $organizer; + $message->senderName = null; + $message->recipient = 'mailto:' . $recipient; + + $capturedFrom = null; + $capturedReplyTo = null; + $mailMessage = $this->createMock(IMessage::class); + $mailMessage->method('setTo')->willReturn($mailMessage); + $mailMessage->method('setFrom') + ->willReturnCallback(function (array $from) use (&$capturedFrom, $mailMessage) { + $capturedFrom = $from; + return $mailMessage; + }); + $mailMessage->method('setReplyTo') + ->willReturnCallback(function (array $replyTo) use (&$capturedReplyTo, $mailMessage) { + $capturedReplyTo = $replyTo; + return $mailMessage; + }); + + $mailer = $this->createMock(IMailer::class); + $mailer->method('createMessage')->willReturn($mailMessage); + $mailer->method('createEMailTemplate')->willReturn($this->emailTemplate); + $mailer->method('send')->willReturn([]); + + if ($viaMailProvider) { + $this->mailMessageNew->method('setFrom') + ->willReturnCallback(function (Address $from) use (&$capturedFrom) { + $capturedFrom = [$from->getAddress() => (string)$from->getLabel()]; + return $this->mailMessageNew; + }); + $this->mailService->method('initiateMessage')->willReturn($this->mailMessageNew); + $this->mailService->expects(self::once()) + ->method('sendMessage') + ->with($this->mailMessageNew); + $this->mailManager->method('findServiceByAddress')->willReturn($this->mailService); + } + + $this->service->method('getLastOccurrence')->willReturn(1496912700); + $this->service->method('getCurrentAttendee')->willReturn($vEvent->select('ATTENDEE')[0]); + $this->service->method('isRoomOrResource')->willReturn(false); + $this->service->method('isCircle')->willReturn(false); + $this->service->method('getAttendeeRsvpOrReqForParticipant')->willReturn(false); + $this->service->method('buildBodyData')->willReturn([ + 'meeting_title' => 'Meeting', + 'invitee_name' => '', + 'attendee_name' => $recipient, + ]); + // Mirrors the real IMipService::getFrom() so assertions read like the + // actual mail header. + $this->service->method('getFrom') + ->willReturnCallback(static fn (?string $senderName, string $default): string + => ($senderName === null || $senderName === '') ? $default : $senderName . ' via ' . $default); + + $this->config->method('getValueBool')->willReturnMap([ + ['dav', 'caldav_external_attendees_disabled', false, false], + ['core', 'mail_providers_enabled', true, $viaMailProvider], + ]); + $this->eventComparisonService->method('findModified') + ->willReturn(['old' => [], 'new' => [$vEvent]]); + + $plugin = new IMipPlugin( + $this->config, + $mailer, + $this->logger, + $this->timeFactory, + $this->defaults, + $this->userSession, + $this->service, + $this->eventComparisonService, + $this->mailManager, + $this->getEmailValidatorWithStrictEmailCheck(), + $this->accountManager, + ); + $plugin->schedule($message); + self::assertSame('1.1', $message->getScheduleStatus()); + + return ['from' => $capturedFrom, 'replyTo' => $capturedReplyTo]; + } + + /** + * Messages are regularly brokered on behalf of somebody else, so headers + * must not fall back to the session user's name when the sender address + * is not theirs. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameIsNotTakenFromAnUnrelatedSessionUser(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('bilbo'); + $this->user->method('getDisplayName')->willReturn('Bilbo Baggins'); + $this->user->method('getEMailAddress')->willReturn('bilbo@hobb.it'); + + $result = $this->scheduleWithoutSenderName('a@example.com', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['a@example.com'], $result['replyTo']); + } + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameFallsBackToSessionUserWhenTheyAreTheSender(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('gandalf'); + $this->user->method('getDisplayName')->willReturn('Mr. Wizard'); + $this->user->method('getEMailAddress')->willReturn('gandalf@wiz.ard'); + + $result = $this->scheduleWithoutSenderName('gandalf@wiz.ard', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Mr. Wizard via Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['gandalf@wiz.ard' => 'Mr. Wizard'], $result['replyTo']); + } + } + + /** + * The session user must also be recognized as the sender when sending + * under one of their profile email aliases. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameUsesSessionUserForTheirAliasAddress(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('carl'); + $this->user->method('getDisplayName')->willReturn('Carl Session'); + $this->user->method('getEMailAddress')->willReturn('carl@example.com'); + + $aliasProperty = $this->createMock(IAccountProperty::class); + $aliasProperty->method('getValue')->willReturn('Shared@Corp.example'); + $emailCollection = $this->createMock(IAccountPropertyCollection::class); + $emailCollection->method('getProperties')->willReturn([$aliasProperty]); + $account = $this->createMock(IAccount::class); + $account->method('getPropertyCollection') + ->with(IAccountManager::COLLECTION_EMAIL) + ->willReturn($emailCollection); + $this->accountManager->method('getAccount')->with($this->user)->willReturn($account); + + $result = $this->scheduleWithoutSenderName('shared@corp.example', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Carl Session via Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['shared@corp.example' => 'Carl Session'], $result['replyTo']); + } + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'transportProvider')] + public function testSenderNameStaysNeutralForBlankDisplayNames(bool $viaMailProvider): void { + $this->user->method('getDisplayName')->willReturn(' '); + $this->user->method('getEMailAddress')->willReturn('gandalf@wiz.ard'); + + $result = $this->scheduleWithoutSenderName('gandalf@wiz.ard', 'frodo@hobb.it', $viaMailProvider); + + self::assertSame(['Instance Name 123'], array_values($result['from'])); + if (!$viaMailProvider) { + self::assertSame(['gandalf@wiz.ard'], $result['replyTo']); + } + } + + /** + * Sessionless contexts: invitation link responses and background jobs. + */ + public function testSenderNameStaysNeutralWithoutSessionUser(): void { + $this->userSession = $this->createMock(IUserSession::class); + + $result = $this->scheduleWithoutSenderName('a@example.com', 'frodo@hobb.it'); + + self::assertSame(['Instance Name 123'], array_values($result['from'])); + self::assertSame(['a@example.com'], $result['replyTo']); + } + + public static function transportProvider(): array { + return [ + 'system email account' => [false], + 'user email account' => [true], + ]; + } }