From 5d1ca3193b7ca3d6038662b7090d124ac69fcf61 Mon Sep 17 00:00:00 2001 From: Benjamin Gaussorgues Date: Wed, 7 Feb 2024 12:06:12 +0100 Subject: [PATCH] feat(share): save date and time for expiration Because of timezones, not saving time can lead to unexpected behaviour when sharing an item sooner than timezone offset Example: sharing a file before 9am when in UTC+9 Signed-off-by: Benjamin Gaussorgues --- .../lib/Controller/ShareAPIController.php | 5 +- apps/files_sharing/tests/ApiTest.php | 20 ++-- apps/files_sharing/tests/CapabilitiesTest.php | 4 +- .../Controller/ShareAPIControllerTest.php | 2 + lib/private/Server.php | 3 +- lib/private/Share20/Manager.php | 26 +++-- tests/lib/Share20/ManagerTest.php | 98 ++++++++++++++----- 7 files changed, 115 insertions(+), 43 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 245c8ad0b67b7..f069efc06f81e 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -223,6 +223,7 @@ protected function formatShare(IShare $share, Node $recipientNode = null): array $expiration = $share->getExpirationDate(); if ($expiration !== null) { + $expiration->setTimezone($this->dateTimeZone->getTimeZone()); $result['expiration'] = $expiration->format('Y-m-d 00:00:00'); } @@ -1662,12 +1663,14 @@ protected function canDeleteShareFromSelf(\OCP\Share\IShare $share): bool { private function parseDate(string $expireDate): \DateTime { try { $date = new \DateTime(trim($expireDate, "\""), $this->dateTimeZone->getTimeZone()); + // Make sure it expires at midnight in owner timezone + $date->setTime(0, 0, 0); } catch (\Exception $e) { throw new \Exception('Invalid date. Format must be YYYY-MM-DD'); } + // Use server timezone to store the date $date->setTimezone(new \DateTimeZone(date_default_timezone_get())); - $date->setTime(0, 0, 0); return $date; } diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index 1500eb237f068..afdb280b95bfe 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -124,6 +124,7 @@ private function createOCS($userId) { $userStatusManager = $this->createMock(IUserStatusManager::class); $previewManager = $this->createMock(IPreview::class); $dateTimeZone = $this->createMock(IDateTimeZone::class); + $dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone(date_default_timezone_get())); return new ShareAPIController( self::APP_NAME, @@ -1063,10 +1064,9 @@ public function testUpdateShareExpireDate() { $config->setAppValue('core', 'shareapi_enforce_expire_date', 'yes'); $dateWithinRange = new \DateTime(); - $dateWithinRange->setTime(0, 0, 0); - $dateWithinRange->add(new \DateInterval('P5D')); + $dateWithinRange->add(new \DateInterval('P6D')); + $dateOutOfRange = new \DateTime(); - $dateOutOfRange->setTime(0, 0, 0); $dateOutOfRange->add(new \DateInterval('P8D')); // update expire date to a valid value @@ -1077,6 +1077,8 @@ public function testUpdateShareExpireDate() { $share1 = $this->shareManager->getShareById($share1->getFullId()); // date should be changed + $dateWithinRange->setTime(0, 0, 0); + $dateWithinRange->setTimezone(new \DateTimeZone(date_default_timezone_get())); $this->assertEquals($dateWithinRange, $share1->getExpirationDate()); // update expire date to a value out of range @@ -1290,12 +1292,14 @@ public function testShareStorageMountPoint() { public function datesProvider() { $date = new \DateTime(); + $date->setTime(0, 0); $date->add(new \DateInterval('P5D')); + $date->setTimezone(new \DateTimeZone(date_default_timezone_get())); return [ - [$date->format('Y-m-d'), true], + [$date->format('Y-m-d H:i:s'), true], ['abc', false], - [$date->format('Y-m-d') . 'xyz', false], + [$date->format('Y-m-d H:i:s') . 'xyz', false], ]; } @@ -1321,7 +1325,7 @@ public function testPublicLinkExpireDate($date, $valid) { $data = $result->getData(); $this->assertTrue(is_string($data['token'])); - $this->assertEquals($date, substr($data['expiration'], 0, 10)); + $this->assertEquals(substr($date, 0, 10), substr($data['expiration'], 0, 10)); // check for correct link $url = \OC::$server->getURLGenerator()->getAbsoluteURL('/index.php/s/' . $data['token']); @@ -1329,7 +1333,7 @@ public function testPublicLinkExpireDate($date, $valid) { $share = $this->shareManager->getShareById('ocinternal:'.$data['id']); - $this->assertEquals($date, $share->getExpirationDate()->format('Y-m-d')); + $this->assertEquals($date, $share->getExpirationDate()->format('Y-m-d H:i:s')); $this->shareManager->deleteShare($share); } @@ -1353,7 +1357,7 @@ public function testCreatePublicLinkExpireDateValid() { $data = $result->getData(); $this->assertTrue(is_string($data['token'])); - $this->assertEquals($date->format('Y-m-d') . ' 00:00:00', $data['expiration']); + $this->assertEquals($date->format('Y-m-d 00:00:00'), $data['expiration']); // check for correct link $url = \OC::$server->getURLGenerator()->getAbsoluteURL('/index.php/s/' . $data['token']); diff --git a/apps/files_sharing/tests/CapabilitiesTest.php b/apps/files_sharing/tests/CapabilitiesTest.php index 294bbba7d9043..aa375818bd62e 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/apps/files_sharing/tests/CapabilitiesTest.php @@ -36,6 +36,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; use OCP\IConfig; +use OCP\IDateTimeZone; use OCP\IGroupManager; use OCP\IL10N; use OCP\IURLGenerator; @@ -98,7 +99,8 @@ private function getResults(array $map) { $this->createMock(IEventDispatcher::class), $this->createMock(IUserSession::class), $this->createMock(KnownUserService::class), - $this->createMock(ShareDisableChecker::class) + $this->createMock(ShareDisableChecker::class), + $this->createMock(IDateTimeZone::class), ); $cap = new Capabilities($config, $shareManager); $result = $this->getFilesSharingPart($cap->getCapabilities()); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 4f4577f96e198..164fd51c1fe1b 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -835,6 +835,7 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) { $this->groupManager->method('get')->willReturnMap([ ['group', $group], ]); + $this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC')); $d = $ocs->getShare($share->getId())->getData()[0]; @@ -4603,6 +4604,7 @@ public function testFormatShare(array $expects, \OCP\Share\IShare $share, array $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturnSelf(); + $this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC')); if (!$exception) { $this->rootFolder->method('getById') diff --git a/lib/private/Server.php b/lib/private/Server.php index 008019db97631..613739b9a4e81 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1314,7 +1314,8 @@ public function __construct($webRoot, \OC\Config $config) { $c->get(IEventDispatcher::class), $c->get(IUserSession::class), $c->get(KnownUserService::class), - $c->get(ShareDisableChecker::class) + $c->get(ShareDisableChecker::class), + $c->get(IDateTimeZone::class), ); return $manager; diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 0b2fe03b5b8fd..66c3781da644d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -54,6 +54,7 @@ use OCP\Files\Node; use OCP\HintException; use OCP\IConfig; +use OCP\IDateTimeZone; use OCP\IGroupManager; use OCP\IL10N; use OCP\IURLGenerator; @@ -119,6 +120,7 @@ class Manager implements IManager { /** @var KnownUserService */ private $knownUserService; private ShareDisableChecker $shareDisableChecker; + private IDateTimeZone $dateTimeZone; public function __construct( LoggerInterface $logger, @@ -139,7 +141,8 @@ public function __construct( IEventDispatcher $dispatcher, IUserSession $userSession, KnownUserService $knownUserService, - ShareDisableChecker $shareDisableChecker + ShareDisableChecker $shareDisableChecker, + IDateTimeZone $dateTimeZone, ) { $this->logger = $logger; $this->config = $config; @@ -163,6 +166,7 @@ public function __construct( $this->userSession = $userSession; $this->knownUserService = $knownUserService; $this->shareDisableChecker = $shareDisableChecker; + $this->dateTimeZone = $dateTimeZone; } /** @@ -383,10 +387,10 @@ protected function validateExpirationDateInternal(IShare $share) { $expirationDate = $share->getExpirationDate(); if ($expirationDate !== null) { - //Make sure the expiration date is a date + $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); $expirationDate->setTime(0, 0, 0); - $date = new \DateTime(); + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $date->setTime(0, 0, 0); if ($date >= $expirationDate) { $message = $this->l->t('Expiration date is in the past'); @@ -414,9 +418,8 @@ protected function validateExpirationDateInternal(IShare $share) { $isEnforced = $this->shareApiInternalDefaultExpireDateEnforced(); } if ($fullId === null && $expirationDate === null && $defaultExpireDate) { - $expirationDate = new \DateTime(); + $expirationDate = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $expirationDate->setTime(0, 0, 0); - $days = (int)$this->config->getAppValue('core', $configProp, (string)$defaultExpireDays); if ($days > $defaultExpireDays) { $days = $defaultExpireDays; @@ -430,7 +433,7 @@ protected function validateExpirationDateInternal(IShare $share) { throw new \InvalidArgumentException('Expiration date is enforced'); } - $date = new \DateTime(); + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $date->setTime(0, 0, 0); $date->add(new \DateInterval('P' . $defaultExpireDays . 'D')); if ($date < $expirationDate) { @@ -470,10 +473,10 @@ protected function validateExpirationDateLink(IShare $share) { $expirationDate = $share->getExpirationDate(); if ($expirationDate !== null) { - //Make sure the expiration date is a date + $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); $expirationDate->setTime(0, 0, 0); - $date = new \DateTime(); + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $date->setTime(0, 0, 0); if ($date >= $expirationDate) { $message = $this->l->t('Expiration date is in the past'); @@ -490,7 +493,7 @@ protected function validateExpirationDateLink(IShare $share) { } if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { - $expirationDate = new \DateTime(); + $expirationDate = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $expirationDate->setTime(0, 0, 0); $days = (int)$this->config->getAppValue('core', 'link_defaultExpDays', $this->shareApiLinkDefaultExpireDays()); @@ -506,7 +509,7 @@ protected function validateExpirationDateLink(IShare $share) { throw new \InvalidArgumentException('Expiration date is enforced'); } - $date = new \DateTime(); + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $date->setTime(0, 0, 0); $date->add(new \DateInterval('P' . $this->shareApiLinkDefaultExpireDays() . 'D')); if ($date < $expirationDate) { @@ -528,6 +531,9 @@ protected function validateExpirationDateLink(IShare $share) { throw new \Exception($message); } + if ($expirationDate instanceof \DateTime) { + $expirationDate->setTimezone(new \DateTimeZone(date_default_timezone_get())); + } $share->setExpirationDate($expirationDate); return $share; diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 221a783ff11ef..9a729fa321e82 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -21,6 +21,7 @@ namespace Test\Share20; +use DateTimeZone; use OC\Files\Mount\MoveableMount; use OC\KnownUser\KnownUserService; use OC\Share20\DefaultShareProvider; @@ -39,6 +40,7 @@ use OCP\Files\Storage; use OCP\HintException; use OCP\IConfig; +use OCP\IDateTimeZone; use OCP\IGroup; use OCP\IGroupManager; use OCP\IL10N; @@ -113,6 +115,9 @@ class ManagerTest extends \Test\TestCase { protected $knownUserService; /** @var ShareDisableChecker|MockObject */ protected $shareDisabledChecker; + private DateTimeZone $timezone; + /** @var IDateTimeZone|MockObject */ + protected $dateTimeZone; protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); @@ -132,6 +137,9 @@ protected function setUp(): void { $this->knownUserService = $this->createMock(KnownUserService::class); $this->shareDisabledChecker = new ShareDisableChecker($this->config, $this->userManager, $this->groupManager); + $this->dateTimeZone = $this->createMock(IDateTimeZone::class); + $this->timezone = new \DateTimeZone('Pacific/Auckland'); + $this->dateTimeZone->method('getTimeZone')->willReturnCallback(fn () => $this->timezone); $this->l10nFactory = $this->createMock(IFactory::class); $this->l = $this->createMock(IL10N::class); @@ -165,7 +173,8 @@ protected function setUp(): void { $this->dispatcher, $this->userSession, $this->knownUserService, - $this->shareDisabledChecker + $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -198,6 +207,7 @@ private function createManagerMock() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ]); } @@ -917,7 +927,7 @@ public function testValidateExpirationDateInternalEnforceButNotSetNewShare($shar ]); } - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P3D')); @@ -952,7 +962,7 @@ public function testValidateExpirationDateInternalEnforceRelaxedDefaultButNotSet ]); } - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P1D')); @@ -999,7 +1009,7 @@ public function testValidateExpirationDateInternalEnforceTooFarIntoFuture($share * @dataProvider validateExpirationDateInternalProvider */ public function testValidateExpirationDateInternalEnforceValid($shareType) { - $future = new \DateTime(); + $future = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $future->add(new \DateInterval('P2D')); $future->setTime(1, 2, 3); @@ -1041,7 +1051,7 @@ public function testValidateExpirationDateInternalEnforceValid($shareType) { * @dataProvider validateExpirationDateInternalProvider */ public function testValidateExpirationDateInternalNoDefault($shareType) { - $date = new \DateTime(); + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); $date->add(new \DateInterval('P5D')); $date->setTime(1, 2, 3); @@ -1089,9 +1099,10 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) { $share = $this->manager->newShare(); $share->setShareType($shareType); - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); + $expected->setTime(0, 0); $expected->add(new \DateInterval('P3D')); - $expected->setTime(0, 0, 0); + $expected->setTimezone(new \DateTimeZone(date_default_timezone_get())); if ($shareType === IShare::TYPE_USER) { $this->config->method('getAppValue') @@ -1124,12 +1135,12 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) { * @dataProvider validateExpirationDateInternalProvider */ public function testValidateExpirationDateInternalDefault($shareType) { - $future = new \DateTime(); + $future = new \DateTime('now', $this->timezone); $future->add(new \DateInterval('P5D')); $future->setTime(1, 2, 3); $expected = clone $future; - $expected->setTime(0, 0, 0); + $expected->setTime(0, 0); $share = $this->manager->newShare(); $share->setShareType($shareType); @@ -1166,7 +1177,7 @@ public function testValidateExpirationDateInternalDefault($shareType) { * @dataProvider validateExpirationDateInternalProvider */ public function testValidateExpirationDateInternalHookModification($shareType) { - $nextWeek = new \DateTime(); + $nextWeek = new \DateTime('now', $this->timezone); $nextWeek->add(new \DateInterval('P7D')); $nextWeek->setTime(0, 0, 0); @@ -1295,7 +1306,7 @@ public function testValidateExpirationDateEnforceButNotSetNewShare() { ['core', 'link_defaultExpDays', 3, '3'], ]); - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P3D')); @@ -1316,7 +1327,7 @@ public function testValidateExpirationDateEnforceRelaxedDefaultButNotSetNewShare ['core', 'link_defaultExpDays', 3, '1'], ]); - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P1D')); @@ -1347,7 +1358,7 @@ public function testValidateExpirationDateEnforceTooFarIntoFuture() { } public function testValidateExpirationDateEnforceValid() { - $future = new \DateTime(); + $future = new \DateTime('now', $this->timezone); $future->add(new \DateInterval('P2D')); $future->setTime(1, 2, 3); @@ -1376,12 +1387,13 @@ public function testValidateExpirationDateEnforceValid() { } public function testValidateExpirationDateNoDefault() { - $date = new \DateTime(); + $date = new \DateTime('now', $this->timezone); $date->add(new \DateInterval('P5D')); $date->setTime(1, 2, 3); $expected = clone $date; - $expected->setTime(0, 0, 0); + $expected->setTime(0, 0); + $expected->setTimezone(new \DateTimeZone(date_default_timezone_get())); $share = $this->manager->newShare(); $share->setExpirationDate($date); @@ -1415,9 +1427,10 @@ public function testValidateExpirationDateNoDateNoDefault() { public function testValidateExpirationDateNoDateDefault() { $share = $this->manager->newShare(); - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->add(new \DateInterval('P3D')); - $expected->setTime(0, 0, 0); + $expected->setTime(0, 0); + $expected->setTimezone(new \DateTimeZone(date_default_timezone_get())); $this->config->method('getAppValue') ->willReturnMap([ @@ -1438,12 +1451,44 @@ public function testValidateExpirationDateNoDateDefault() { } public function testValidateExpirationDateDefault() { - $future = new \DateTime(); + $future = new \DateTime('now', $this->timezone); $future->add(new \DateInterval('P5D')); $future->setTime(1, 2, 3); $expected = clone $future; - $expected->setTime(0, 0, 0); + $expected->setTime(0, 0); + $expected->setTimezone(new \DateTimeZone(date_default_timezone_get())); + + $share = $this->manager->newShare(); + $share->setExpirationDate($future); + + $this->config->method('getAppValue') + ->willReturnMap([ + ['core', 'shareapi_default_expire_date', 'no', 'yes'], + ['core', 'shareapi_expire_after_n_days', '7', '3'], + ['core', 'link_defaultExpDays', '3', '1'], + ]); + + $hookListener = $this->getMockBuilder('Dummy')->setMethods(['listener'])->getMock(); + \OCP\Util::connectHook('\OC\Share', 'verifyExpirationDate', $hookListener, 'listener'); + $hookListener->expects($this->once())->method('listener')->with($this->callback(function ($data) use ($expected) { + return $data['expirationDate'] == $expected; + })); + + self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]); + + $this->assertEquals($expected, $share->getExpirationDate()); + } + + public function testValidateExpirationNegativeOffsetTimezone() { + $this->timezone = new \DateTimeZone('Pacific/Tahiti'); + $future = new \DateTime(); + $future->add(new \DateInterval('P5D')); + + $expected = clone $future; + $expected->setTimezone($this->timezone); + $expected->setTime(0, 0); + $expected->setTimezone(new \DateTimeZone(date_default_timezone_get())); $share = $this->manager->newShare(); $share->setExpirationDate($future); @@ -1467,11 +1512,12 @@ public function testValidateExpirationDateDefault() { } public function testValidateExpirationDateHookModification() { - $nextWeek = new \DateTime(); + $nextWeek = new \DateTime('now', $this->timezone); $nextWeek->add(new \DateInterval('P7D')); - $nextWeek->setTime(0, 0, 0); $save = clone $nextWeek; + $save->setTime(0, 0); + $save->setTimezone(new \DateTimeZone(date_default_timezone_get())); $hookListener = $this->getMockBuilder('Dummy')->setMethods(['listener'])->getMock(); \OCP\Util::connectHook('\OC\Share', 'verifyExpirationDate', $hookListener, 'listener'); @@ -2750,6 +2796,7 @@ public function testGetShareByToken() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2799,6 +2846,7 @@ public function testGetShareByTokenRoom() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2855,6 +2903,7 @@ public function testGetShareByTokenWithException() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -4256,6 +4305,7 @@ public function testShareProviderExists($shareType, $expected) { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4292,6 +4342,7 @@ public function testGetSharesInFolder() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4359,6 +4410,7 @@ public function testGetAccessList() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4478,6 +4530,7 @@ public function testGetAccessListWithCurrentAccess() { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4605,7 +4658,8 @@ public function testGetAllShares() { $this->dispatcher, $this->userSession, $this->knownUserService, - $this->shareDisabledChecker + $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider);