From a4a7d82a0cb9e2023a0d0719e50738b6fc0a092a 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 aa239ae8bb6..77faa2ab0c9 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -240,6 +240,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'); } @@ -1695,12 +1696,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 7e916f621aa..98f87c96f31 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -124,6 +124,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, @@ -1060,10 +1061,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 @@ -1074,6 +1074,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 @@ -1287,12 +1289,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], ]; } @@ -1318,7 +1322,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']); @@ -1326,7 +1330,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); } @@ -1350,7 +1354,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 1a3c416b10c..d7c3f218d9f 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; @@ -96,7 +97,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 822212ae86f..22b1da0f285 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -845,6 +845,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]; @@ -4647,6 +4648,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 d9bbc8625e9..419d6344a30 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1265,7 +1265,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 31f3924f053..4fb606b8773 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; @@ -120,6 +121,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; @@ -162,6 +165,7 @@ class Manager implements IManager { $this->userSession = $userSession; $this->knownUserService = $knownUserService; $this->shareDisableChecker = $shareDisableChecker; + $this->dateTimeZone = $dateTimeZone; } /** @@ -382,10 +386,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'); @@ -413,9 +417,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; @@ -429,7 +432,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) { @@ -469,10 +472,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'); @@ -489,7 +492,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()); @@ -505,7 +508,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) { @@ -527,6 +530,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 c770181799f..97f22036f5f 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; @@ -114,6 +116,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); @@ -164,7 +172,8 @@ class ManagerTest extends \Test\TestCase { $this->dispatcher, $this->userSession, $this->knownUserService, - $this->shareDisabledChecker + $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -196,6 +205,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ]); } @@ -933,7 +943,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')); @@ -968,7 +978,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')); @@ -1015,7 +1025,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); @@ -1057,7 +1067,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); @@ -1105,9 +1115,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') @@ -1140,12 +1151,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); @@ -1182,7 +1193,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); @@ -1311,7 +1322,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')); @@ -1332,7 +1343,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')); @@ -1363,7 +1374,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); @@ -1392,12 +1403,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); @@ -1431,9 +1443,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([ @@ -1454,12 +1467,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); @@ -1483,11 +1528,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'); @@ -2764,6 +2810,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2812,6 +2859,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -2867,6 +2915,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $share = $this->createMock(IShare::class); @@ -4267,6 +4316,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4302,6 +4352,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4368,6 +4419,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4486,6 +4538,7 @@ class ManagerTest extends \Test\TestCase { $this->userSession, $this->knownUserService, $this->shareDisabledChecker, + $this->dateTimeZone, ); $factory->setProvider($this->defaultProvider); @@ -4612,7 +4665,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