From ceebc38f5fb30ceaefcedbb78f77c7def7f5ef4f 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 | 2 +- apps/files_sharing/tests/ApiTest.php | 3 ++- .../Controller/ShareAPIControllerTest.php | 2 ++ lib/private/Share20/Manager.php | 13 ---------- tests/lib/Share20/ManagerTest.php | 26 +++++++------------ 5 files changed, 15 insertions(+), 31 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index aa239ae8bb6b3..7f8f110183241 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -240,6 +240,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'); } @@ -1700,7 +1701,6 @@ private function parseDate(string $expireDate): \DateTime { } $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 7e916f621aa27..e4a89d9a39e80 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('UTC')); return new ShareAPIController( self::APP_NAME, @@ -1350,7 +1351,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/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 822212ae86f1c..22b1da0f285ab 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -845,6 +845,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]; @@ -4647,6 +4648,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/Share20/Manager.php b/lib/private/Share20/Manager.php index 3af74789602c3..5a9d4e0550757 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -382,11 +382,7 @@ protected function validateExpirationDateInternal(IShare $share) { $expirationDate = $share->getExpirationDate(); if ($expirationDate !== null) { - //Make sure the expiration date is a date - $expirationDate->setTime(0, 0, 0); - $date = new \DateTime(); - $date->setTime(0, 0, 0); if ($date >= $expirationDate) { $message = $this->l->t('Expiration date is in the past'); throw new GenericShareException($message, $message, 404); @@ -414,8 +410,6 @@ protected function validateExpirationDateInternal(IShare $share) { } if ($fullId === null && $expirationDate === null && $defaultExpireDate) { $expirationDate = new \DateTime(); - $expirationDate->setTime(0, 0, 0); - $days = (int)$this->config->getAppValue('core', $configProp, (string)$defaultExpireDays); if ($days > $defaultExpireDays) { $days = $defaultExpireDays; @@ -430,7 +424,6 @@ protected function validateExpirationDateInternal(IShare $share) { } $date = new \DateTime(); - $date->setTime(0, 0, 0); $date->add(new \DateInterval('P' . $defaultExpireDays . 'D')); if ($date < $expirationDate) { $message = $this->l->n('Cannot set expiration date more than %n day in the future', 'Cannot set expiration date more than %n days in the future', $defaultExpireDays); @@ -469,11 +462,7 @@ protected function validateExpirationDateLink(IShare $share) { $expirationDate = $share->getExpirationDate(); if ($expirationDate !== null) { - //Make sure the expiration date is a date - $expirationDate->setTime(0, 0, 0); - $date = new \DateTime(); - $date->setTime(0, 0, 0); if ($date >= $expirationDate) { $message = $this->l->t('Expiration date is in the past'); throw new GenericShareException($message, $message, 404); @@ -490,7 +479,6 @@ protected function validateExpirationDateLink(IShare $share) { if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { $expirationDate = new \DateTime(); - $expirationDate->setTime(0, 0, 0); $days = (int)$this->config->getAppValue('core', 'link_defaultExpDays', (string)$this->shareApiLinkDefaultExpireDays()); if ($days > $this->shareApiLinkDefaultExpireDays()) { @@ -506,7 +494,6 @@ protected function validateExpirationDateLink(IShare $share) { } $date = new \DateTime(); - $date->setTime(0, 0, 0); $date->add(new \DateInterval('P' . $this->shareApiLinkDefaultExpireDays() . 'D')); if ($date < $expirationDate) { $message = $this->l->n('Cannot set expiration date more than %n day in the future', 'Cannot set expiration date more than %n days in the future', $this->shareApiLinkDefaultExpireDays()); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index eaa26ef7e856e..334a513fc1995 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -940,7 +940,7 @@ public function testValidateExpirationDateInternalEnforceButNotSetNewShare($shar self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]); $this->assertNotNull($share->getExpirationDate()); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } /** @@ -975,7 +975,7 @@ public function testValidateExpirationDateInternalEnforceRelaxedDefaultButNotSet self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]); $this->assertNotNull($share->getExpirationDate()); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } /** @@ -1050,7 +1050,7 @@ public function testValidateExpirationDateInternalEnforceValid($shareType) { self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } /** @@ -1062,7 +1062,6 @@ public function testValidateExpirationDateInternalNoDefault($shareType) { $date->setTime(1, 2, 3); $expected = clone $date; - $expected->setTime(0, 0, 0); $share = $this->manager->newShare(); $share->setShareType($shareType); @@ -1107,7 +1106,6 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) { $expected = new \DateTime(); $expected->add(new \DateInterval('P3D')); - $expected->setTime(0, 0, 0); if ($shareType === IShare::TYPE_USER) { $this->config->method('getAppValue') @@ -1128,12 +1126,12 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) { $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; + return $data['expirationDate']->format('YmdH') == $expected->format('YmdH'); })); self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected->format('YmdH'), $share->getExpirationDate()->format('YmdH')); } /** @@ -1145,7 +1143,6 @@ public function testValidateExpirationDateInternalDefault($shareType) { $future->setTime(1, 2, 3); $expected = clone $future; - $expected->setTime(0, 0, 0); $share = $this->manager->newShare(); $share->setShareType($shareType); @@ -1318,7 +1315,7 @@ public function testValidateExpirationDateEnforceButNotSetNewShare() { self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]); $this->assertNotNull($share->getExpirationDate()); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } public function testValidateExpirationDateEnforceRelaxedDefaultButNotSetNewShare() { @@ -1339,7 +1336,7 @@ public function testValidateExpirationDateEnforceRelaxedDefaultButNotSetNewShare self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]); $this->assertNotNull($share->getExpirationDate()); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } public function testValidateExpirationDateEnforceTooFarIntoFuture() { @@ -1388,7 +1385,7 @@ public function testValidateExpirationDateEnforceValid() { self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0)); } public function testValidateExpirationDateNoDefault() { @@ -1397,7 +1394,6 @@ public function testValidateExpirationDateNoDefault() { $date->setTime(1, 2, 3); $expected = clone $date; - $expected->setTime(0, 0, 0); $share = $this->manager->newShare(); $share->setExpirationDate($date); @@ -1433,7 +1429,6 @@ public function testValidateExpirationDateNoDateDefault() { $expected = new \DateTime(); $expected->add(new \DateInterval('P3D')); - $expected->setTime(0, 0, 0); $this->config->method('getAppValue') ->willReturnMap([ @@ -1445,12 +1440,12 @@ public function testValidateExpirationDateNoDateDefault() { $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; + return $data['expirationDate']->format('YmdH') == $expected->format('YmdH'); })); self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]); - $this->assertEquals($expected, $share->getExpirationDate()); + $this->assertEquals($expected->format('YmdH'), $share->getExpirationDate()->format('YmdH')); } public function testValidateExpirationDateDefault() { @@ -1459,7 +1454,6 @@ public function testValidateExpirationDateDefault() { $future->setTime(1, 2, 3); $expected = clone $future; - $expected->setTime(0, 0, 0); $share = $this->manager->newShare(); $share->setExpirationDate($future);