Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dav): Prevent out-of-office event time drifts #42142

Merged
merged 1 commit into from
Dec 11, 2023
+92 −29
Merged

fix(dav): Prevent out-of-office event time drifts #42142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 10 additions & 23 deletions apps/dav/lib/Listener/OutOfOfficeListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@
namespace OCA\DAV\Listener;

use DateTimeImmutable;
use DateTimeZone;
use OCA\DAV\CalDAV\CalDavBackend;
use OCA\DAV\CalDAV\Calendar;
use OCA\DAV\CalDAV\CalendarHome;
use OCA\DAV\CalDAV\TimezoneService;
use OCA\DAV\ServerFactory;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventListener;
Expand All @@ -40,9 +42,6 @@
use Psr\Log\LoggerInterface;
use Sabre\DAV\Exception\NotFound;
use Sabre\VObject\Component\VCalendar;
use Sabre\VObject\Component\VEvent;
use Sabre\VObject\Component\VTimeZone;
use Sabre\VObject\Reader;
use function fclose;
use function fopen;
use function fwrite;
Expand All @@ -55,6 +54,7 @@ class OutOfOfficeListener implements IEventListener {
public function __construct(
private ServerFactory $serverFactory,
private IConfig $appConfig,
private TimezoneService $timezoneService,
private LoggerInterface $logger
) {
}
Expand All @@ -67,8 +67,8 @@ public function handle(Event $event): void {
if ($calendarNode === null) {
return;
}
$tz = $calendarNode->getProperties([])['{urn:ietf:params:xml:ns:caldav}calendar-timezone'] ?? null;
$vCalendarEvent = $this->createVCalendarEvent($event->getData(), $tz);
$tzId = $this->timezoneService->getUserTimezone($userId) ?? $this->timezoneService->getDefaultTimezone();
$vCalendarEvent = $this->createVCalendarEvent($event->getData(), $tzId);
$stream = fopen('php://memory', 'rb+');
try {
fwrite($stream, $vCalendarEvent->serialize());
Expand All @@ -87,8 +87,8 @@ public function handle(Event $event): void {
if ($calendarNode === null) {
return;
}
$tz = $calendarNode->getProperties([])['{urn:ietf:params:xml:ns:caldav}calendar-timezone'] ?? null;
$vCalendarEvent = $this->createVCalendarEvent($event->getData(), $tz);
$tzId = $this->timezoneService->getUserTimezone($userId) ?? $this->timezoneService->getDefaultTimezone();
$vCalendarEvent = $this->createVCalendarEvent($event->getData(), $tzId);
try {
$oldEvent = $calendarNode->getChild($this->getEventFileName($event->getData()->getId()));
$oldEvent->put($vCalendarEvent->serialize());
Expand Down Expand Up @@ -169,13 +169,15 @@ private function getEventFileName(string $id): string {
return "out_of_office_$id.ics";
}

private function createVCalendarEvent(IOutOfOfficeData $data, ?string $timeZoneData): VCalendar {
private function createVCalendarEvent(IOutOfOfficeData $data, string $tzId): VCalendar {
$shortMessage = $data->getShortMessage();
$longMessage = $data->getMessage();
$start = (new DateTimeImmutable)
->setTimezone(new DateTimeZone($tzId))
->setTimestamp($data->getStartDate())
->setTime(0, 0);
$end = (new DateTimeImmutable())
->setTimezone(new DateTimeZone($tzId))
->setTimestamp($data->getEndDate())
->modify('+ 1 days')
->setTime(0, 0);
Expand All @@ -188,21 +190,6 @@ private function createVCalendarEvent(IOutOfOfficeData $data, ?string $timeZoneD
'DTEND' => $end,
'X-NEXTCLOUD-OUT-OF-OFFICE' => $data->getId(),
]);
/** @var VEvent $vEvent */
$vEvent = $vCalendar->VEVENT;
if ($timeZoneData !== null) {
/** @var VCalendar $vtimezoneObj */
$vtimezoneObj = Reader::read($timeZoneData);
/** @var VTimeZone $vtimezone */
$vtimezone = $vtimezoneObj->VTIMEZONE;
$calendarTimeZone = $vtimezone->getTimeZone();
$vCalendar->add($vtimezone);

/** @psalm-suppress UndefinedMethod */
$vEvent->DTSTART->setDateTime($start->setTimezone($calendarTimeZone)->setTime(0, 0));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the day drift happened here because we set the time above without a timezone. if the actual time is on UTC+1 and you set to 00:00 the date moves by one day. So even if we set the time again here, it's one day too early.

setting timezone first, then the time ensures that the day does not change unexpectedly

/** @psalm-suppress UndefinedMethod */
$vEvent->DTEND->setDateTime($end->setTimezone($calendarTimeZone)->setTime(0, 0));
}
return $vCalendar;
}
}
88 changes: 82 additions & 6 deletions apps/dav/tests/unit/Listener/OutOfOfficeListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,14 @@

namespace OCA\DAV\Tests\Unit\Listener;

use DateTimeImmutable;
use InvalidArgumentException;
use OCA\DAV\CalDAV\Calendar;
use OCA\DAV\CalDAV\CalendarHome;
use OCA\DAV\CalDAV\CalendarObject;
use OCA\DAV\CalDAV\InvitationResponse\InvitationResponseServer;
use OCA\DAV\CalDAV\Plugin;
use OCA\DAV\CalDAV\TimezoneService;
use OCA\DAV\Connector\Sabre\Server;
use OCA\DAV\Listener\OutOfOfficeListener;
use OCA\DAV\ServerFactory;
Expand All @@ -45,6 +48,9 @@
use Psr\Log\LoggerInterface;
use Sabre\DAV\Exception\NotFound;
use Sabre\DAV\Tree;
use Sabre\VObject\Component\VCalendar;
use Sabre\VObject\Component\VEvent;
use Sabre\VObject\Reader;
use Test\TestCase;

/**
Expand All @@ -55,20 +61,23 @@ class OutOfOfficeListenerTest extends TestCase {
private ServerFactory|MockObject $serverFactory;
private IConfig|MockObject $appConfig;
private LoggerInterface|MockObject $loggerInterface;
private OutOfOfficeListener $listener;
private MockObject|TimezoneService $timezoneService;
private IManager|MockObject $manager;
private OutOfOfficeListener $listener;

protected function setUp(): void {
parent::setUp();

$this->serverFactory = $this->createMock(ServerFactory::class);
$this->appConfig = $this->createMock(IConfig::class);
$this->timezoneService = $this->createMock(TimezoneService::class);
$this->loggerInterface = $this->createMock(LoggerInterface::class);
$this->manager = $this->createMock(IManager::class);

$this->listener = new OutOfOfficeListener(
$this->serverFactory,
$this->appConfig,
$this->timezoneService,
$this->loggerInterface,
$this->manager
);
Expand Down Expand Up @@ -166,11 +175,15 @@ public function testHandleSchedulingPersonalCalendarNotFound(): void {
$this->listener->handle($event);
}

public function testHandleScheduling(): void {
public function testHandleSchedulingWithDefaultTimezone(): void {
$user = $this->createMock(IUser::class);
$user->method('getUID')->willReturn('user123');
$data = $this->createMock(IOutOfOfficeData::class);
$data->method('getUser')->willReturn($user);
$data->method('getStartDate')
->willReturn((new DateTimeImmutable('2023-12-12T00:00:00Z'))->getTimestamp());
$data->method('getEndDate')
->willReturn((new DateTimeImmutable('2023-12-13T00:00:00Z'))->getTimestamp());
$davServer = $this->createMock(Server::class);
$invitationServer = $this->createMock(InvitationResponseServer::class);
$invitationServer->method('getServer')->willReturn($davServer);
Expand All @@ -195,12 +208,28 @@ public function testHandleScheduling(): void {
->with('user123', 'dav', 'defaultCalendar', 'personal')
->willReturn('personal-1');
$calendar = $this->createMock(Calendar::class);
$this->timezoneService->expects(self::once())
->method('getUserTimezone')
->with('user123')
->willReturn('Europe/Prague');
$calendarHome->expects(self::once())
->method('getChild')
->with('personal-1')
->willReturn($calendar);
$calendar->expects(self::once())
->method('createFile');
->method('createFile')
->willReturnCallback(function ($name, $data) {
$vcalendar = Reader::read($data);
if (!($vcalendar instanceof VCalendar)) {
throw new InvalidArgumentException('Calendar data should be a VCALENDAR');
}
$vevent = $vcalendar->VEVENT;
if ($vevent === null || !($vevent instanceof VEvent)) {
throw new InvalidArgumentException('Calendar data should contain a VEVENT');
}
self::assertSame('Europe/Prague', $vevent->DTSTART['TZID']?->getValue());
self::assertSame('Europe/Prague', $vevent->DTEND['TZID']?->getValue());
});
$event = new OutOfOfficeScheduledEvent($data);

$this->listener->handle($event);
Expand Down Expand Up @@ -295,6 +324,10 @@ public function testHandleChangeRecreate(): void {
$user->method('getUID')->willReturn('user123');
$data = $this->createMock(IOutOfOfficeData::class);
$data->method('getUser')->willReturn($user);
$data->method('getStartDate')
->willReturn((new DateTimeImmutable('2023-12-12T00:00:00Z'))->getTimestamp());
$data->method('getEndDate')
->willReturn((new DateTimeImmutable('2023-12-14T00:00:00Z'))->getTimestamp());
$davServer = $this->createMock(Server::class);
$invitationServer = $this->createMock(InvitationResponseServer::class);
$invitationServer->method('getServer')->willReturn($davServer);
Expand All @@ -319,6 +352,13 @@ public function testHandleChangeRecreate(): void {
->with('user123', 'dav', 'defaultCalendar', 'personal')
->willReturn('personal-1');
$calendar = $this->createMock(Calendar::class);
$this->timezoneService->expects(self::once())
->method('getUserTimezone')
->with('user123')
->willReturn(null);
$this->timezoneService->expects(self::once())
->method('getDefaultTimezone')
->willReturn('Europe/Berlin');
$calendarHome->expects(self::once())
->method('getChild')
->with('personal-1')
Expand All @@ -327,17 +367,33 @@ public function testHandleChangeRecreate(): void {
->method('getChild')
->willThrowException(new NotFound());
$calendar->expects(self::once())
->method('createFile');
->method('createFile')
->willReturnCallback(function ($name, $data) {
$vcalendar = Reader::read($data);
if (!($vcalendar instanceof VCalendar)) {
throw new InvalidArgumentException('Calendar data should be a VCALENDAR');
}
$vevent = $vcalendar->VEVENT;
if ($vevent === null || !($vevent instanceof VEvent)) {
throw new InvalidArgumentException('Calendar data should contain a VEVENT');
}
self::assertSame('Europe/Berlin', $vevent->DTSTART['TZID']?->getValue());
self::assertSame('Europe/Berlin', $vevent->DTEND['TZID']?->getValue());
});
$event = new OutOfOfficeChangedEvent($data);

$this->listener->handle($event);
}

public function testHandleChange(): void {
public function testHandleChangeWithoutTimezone(): void {
$user = $this->createMock(IUser::class);
$user->method('getUID')->willReturn('user123');
$data = $this->createMock(IOutOfOfficeData::class);
$data->method('getUser')->willReturn($user);
$data->method('getStartDate')
->willReturn((new DateTimeImmutable('2023-01-12T00:00:00Z'))->getTimestamp());
$data->method('getEndDate')
->willReturn((new DateTimeImmutable('2023-12-14T00:00:00Z'))->getTimestamp());
$davServer = $this->createMock(Server::class);
$invitationServer = $this->createMock(InvitationResponseServer::class);
$invitationServer->method('getServer')->willReturn($davServer);
Expand Down Expand Up @@ -367,11 +423,31 @@ public function testHandleChange(): void {
->with('personal-1')
->willReturn($calendar);
$eventNode = $this->createMock(CalendarObject::class);
$this->timezoneService->expects(self::once())
->method('getUserTimezone')
->with('user123')
->willReturn(null);
$this->timezoneService->expects(self::once())
->method('getDefaultTimezone')
->willReturn('UTC');
$calendar->expects(self::once())
->method('getChild')
->willReturn($eventNode);
$eventNode->expects(self::once())
->method('put');
->method('put')
->willReturnCallback(function ($data) {
$vcalendar = Reader::read($data);
if (!($vcalendar instanceof VCalendar)) {
throw new InvalidArgumentException('Calendar data should be a VCALENDAR');
}
$vevent = $vcalendar->VEVENT;
if ($vevent === null || !($vevent instanceof VEvent)) {
throw new InvalidArgumentException('Calendar data should contain a VEVENT');
}
// UTC datetimes are stored without a TZID
self::assertSame(null, $vevent->DTSTART['TZID']?->getValue());
self::assertSame(null, $vevent->DTEND['TZID']?->getValue());
});
$calendar->expects(self::never())
->method('createFile');
$event = new OutOfOfficeChangedEvent($data);
Expand Down
Loading