From 1b30da153ce6e7968ab016dba27746bb495ac367 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 81a6646ade3..eb60a3a85f1 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -224,6 +224,7 @@ class ShareAPIController extends OCSController { $expiration = $share->getExpirationDate(); if ($expiration !== null) { + $expiration->setTimezone($this->dateTimeZone->getTimeZone()); $result['expiration'] = $expiration->format('Y-m-d 00:00:00'); } @@ -1668,12 +1669,14 @@ class ShareAPIController extends OCSController { 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 25e32f67b72..66c8432c1c9 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -125,6 +125,7 @@ class ApiTest extends TestCase { $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, @@ -1064,10 +1065,9 @@ class ApiTest extends TestCase { $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 @@ -1078,6 +1078,8 @@ class ApiTest extends TestCase { $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 @@ -1291,12 +1293,14 @@ class ApiTest extends TestCase { 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], ]; } @@ -1322,7 +1326,7 @@ class ApiTest extends TestCase { $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']); @@ -1330,7 +1334,7 @@ class ApiTest extends TestCase { $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); } @@ -1354,7 +1358,7 @@ class ApiTest extends TestCase { $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 294bbba7d90..aa375818bd6 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/apps/files_sharing/tests/CapabilitiesTest.php @@ -36,6 +36,7 @@ use OCP\EventDispatcher\IEventDispatcher; 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 @@ class CapabilitiesTest extends \Test\TestCase { $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 5ecb5185512..0f62c625c62 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -837,6 +837,7 @@ class ShareAPIControllerTest extends TestCase { $this->groupManager->method('get')->willReturnMap([ ['group', $group], ]); + $this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC')); $d = $ocs->getShare($share->getId())->getData()[0]; @@ -4605,6 +4606,7 @@ class ShareAPIControllerTest extends TestCase { $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 dca2ebb50ea..c66ddfd2259 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1323,7 +1323,8 @@ class Server extends ServerContainer implements IServerContainer { $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 74bb505c036..acb104a1ca7 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -54,6 +54,7 @@ use OCP\Files\Mount\IMountManager; 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 @@ class Manager implements IManager { IEventDispatcher $dispatcher, IUserSession $userSession, KnownUserService $knownUserService, - ShareDisableChecker $shareDisableChecker + ShareDisableChecker $shareDisableChecker, + IDateTimeZone $dateTimeZone, ) { $this->logger = $logger; $this->config = $config; @@ -163,6 +166,7 @@ class Manager implements IManager { $this->userSession = $userSession; $this->knownUserService = $knownUserService; $this->shareDisableChecker = $shareDisableChecker; + $this->dateTimeZone = $dateTimeZone; } /** @@ -383,10 +387,10 @@ class Manager implements IManager { $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 @@ class Manager implements IManager { $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 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { $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 @@ class Manager implements IManager { } 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', (string)$this->shareApiLinkDefaultExpireDays()); @@ -506,7 +509,7 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { 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 18a55ca996c..ab180d0dc4e 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\Node; 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 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $this->dispatcher, $this->userSession, $this->knownUserService, - $this->shareDisabledChecker + $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -198,6 +207,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ]); } @@ -917,7 +927,7 @@ class ManagerTest extends \Test\TestCase { ]); } - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P3D')); @@ -952,7 +962,7 @@ class ManagerTest extends \Test\TestCase { ]); } - $expected = new \DateTime(); + $expected = new \DateTime('now', $this->timezone); $expected->setTime(0, 0, 0); $expected->add(new \DateInterval('P1D')); @@ -999,7 +1009,7 @@ class ManagerTest extends \Test\TestCase { * @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 @@ class ManagerTest extends \Test\TestCase { * @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 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { * @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 @@ class ManagerTest extends \Test\TestCase { * @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 @@ class ManagerTest extends \Test\TestCase { ['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 @@ class ManagerTest extends \Test\TestCase { ['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 @@ class ManagerTest extends \Test\TestCase { } 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 @@ class ManagerTest extends \Test\TestCase { } 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 @@ class ManagerTest extends \Test\TestCase { 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 @@ class ManagerTest extends \Test\TestCase { } 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 @@ class ManagerTest extends \Test\TestCase { } 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 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2799,6 +2846,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2855,6 +2903,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -4256,6 +4305,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4292,6 +4342,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4359,6 +4410,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4478,6 +4530,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4605,7 +4658,8 @@ class ManagerTest extends \Test\TestCase { $this->dispatcher, $this->userSession, $this->knownUserService, - $this->shareDisabledChecker + $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); -- 2.39.5