diff --git a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php index 2eafac731fdfc..161d081b39b3c 100644 --- a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php +++ b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php @@ -10,9 +10,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\IMailer; use OCP\Mail\Provider\Address; @@ -63,6 +65,7 @@ public function __construct( private IMipService $imipService, private EventComparisonService $eventComparisonService, private IMailManager $mailManager, + private IAccountManager $accountManager, ) { parent::__construct(''); } @@ -179,21 +182,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: @@ -333,6 +332,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 55e6d85396770..190ccad9ea980 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -333,7 +333,8 @@ public function __construct( $userSession, \OC::$server->get(IMipService::class), \OC::$server->get(EventComparisonService::class), - \OC::$server->get(\OCP\Mail\Provider\IManager::class) + \OC::$server->get(\OCP\Mail\Provider\IManager::class), + \OC::$server->get(IAccountManager::class), )); } $this->server->addPlugin(new \OCA\DAV\CalDAV\Search\SearchPlugin()); diff --git a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php index 9f4bd33ab425c..2d0133b80364a 100644 --- a/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php +++ b/apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php @@ -9,6 +9,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; @@ -18,6 +22,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; @@ -85,6 +90,9 @@ class IMipPluginTest extends TestCase { /** @var IMailMessageNew|MockObject */ private $mailMessageNew; + /** @var IAccountManager|MockObject */ + private $accountManager; + protected function setUp(): void { $this->mailMessage = $this->createMock(IMessage::class); $this->mailMessage->method('setFrom')->willReturn($this->mailMessage); @@ -127,6 +135,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, @@ -137,6 +147,7 @@ protected function setUp(): void { $this->service, $this->eventComparisonService, $this->mailManager, + $this->accountManager, ); } @@ -247,6 +258,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); @@ -556,6 +570,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); @@ -691,6 +708,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); @@ -923,6 +943,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); @@ -1040,6 +1063,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); @@ -1148,6 +1174,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); @@ -1307,6 +1336,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); @@ -1344,4 +1376,207 @@ 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('validateMailAddress')->with($recipient)->willReturn(true); + $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') + ->willReturnCallback(function ($app, $key, $default) use ($viaMailProvider) { + if ($app === 'core' && $key === 'mail_providers_enabled') { + return $viaMailProvider; + } + return $default; + }); + $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->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. + * + * @dataProvider 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']); + } + } + + /** + * @dataProvider 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. + * + * @dataProvider 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']); + } + } + + /** + * @dataProvider transportProvider + */ + public function testSenderNameStaysNeutralForBlankDisplayNames(bool $viaMailProvider): void { + $this->user->method('getUID')->willReturn('gandalf'); + $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], + ]; + } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index b22b7654ad51c..8c8cbb4cc60fd 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -317,7 +317,7 @@ protected function validateExpirationDateInternal(IShare $share) { if ($fullId === null && $expirationDate === null && $defaultExpireDate) { $expirationDate = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $expirationDate->setTime(23, 59, 59); - $days = (int) $this->config->getAppValue('core', $configProp, (string) $defaultExpireDays); + $days = (int)$this->config->getAppValue('core', $configProp, (string)$defaultExpireDays); if ($days > $defaultExpireDays) { $days = $defaultExpireDays; }