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 <benjamin.gaussorgues@nextcloud.com>
nextcloud · Feb 13, 2024 · ceebc38 · ceebc38
1 parent 8822b16
commit ceebc38
Show file tree

Hide file tree

Showing 5 changed files with 15 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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);