Skip to content

Commit 719eed6

Browse files
AltahrimAltahrim
committed
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>
1 parent 7ff6cbc commit 719eed6

File tree

6 files changed

+23
-39
lines changed

6 files changed

+23
-39
lines changed

‎apps/files_sharing/lib/Controller/ShareAPIController.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,8 @@ protected function formatShare(IShare $share, Node $recipientNode = null): array
240240

241241
$expiration = $share->getExpirationDate();
242242
if ($expiration !== null) {
243-
$result['expiration'] = $expiration->format('Y-m-d 00:00:00');
243+
$expiration->setTimezone($this->dateTimeZone->getTimeZone());
244+
$result['expiration'] = $expiration->format('Y-m-d');
244245
}
245246

246247
if ($share->getShareType() === IShare::TYPE_USER) {
@@ -1700,7 +1701,6 @@ private function parseDate(string $expireDate): \DateTime {
17001701
}
17011702

17021703
$date->setTimezone(new \DateTimeZone(date_default_timezone_get()));
1703-
$date->setTime(0, 0, 0);
17041704

17051705
return $date;
17061706
}

‎apps/files_sharing/tests/ApiTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ private function createOCS($userId) {
124124
$userStatusManager = $this->createMock(IUserStatusManager::class);
125125
$previewManager = $this->createMock(IPreview::class);
126126
$dateTimeZone = $this->createMock(IDateTimeZone::class);
127+
$dateTimeZone->method('getTimeZone')->willReturn(new \DateTimeZone('UTC'));
127128

128129
return new ShareAPIController(
129130
self::APP_NAME,
@@ -1350,7 +1351,7 @@ public function testCreatePublicLinkExpireDateValid() {
13501351

13511352
$data = $result->getData();
13521353
$this->assertTrue(is_string($data['token']));
1353-
$this->assertEquals($date->format('Y-m-d') . ' 00:00:00', $data['expiration']);
1354+
$this->assertEquals($date->format('Y-m-d'), $data['expiration']);
13541355

13551356
// check for correct link
13561357
$url = \OC::$server->getURLGenerator()->getAbsoluteURL('/index.php/s/' . $data['token']);

‎apps/files_sharing/tests/Controller/ShareAPIControllerTest.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,7 @@ public function dataGetShare() {
741741
'file_target' => 'target',
742742
'file_parent' => 3,
743743
'token' => 'token',
744-
'expiration' => '2000-01-02 00:00:00',
744+
'expiration' => '2000-01-02',
745745
'permissions' => 4,
746746
'attributes' => null,
747747
'stime' => 5,
@@ -845,6 +845,7 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) {
845845
$this->groupManager->method('get')->willReturnMap([
846846
['group', $group],
847847
]);
848+
$this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC'));
848849

849850
$d = $ocs->getShare($share->getId())->getData()[0];
850851

@@ -4109,7 +4110,7 @@ public function dataFormatShare() {
41094110
'attributes' => null,
41104111
'stime' => 946684862,
41114112
'parent' => null,
4112-
'expiration' => '2001-01-02 00:00:00',
4113+
'expiration' => '2001-01-02',
41134114
'token' => 'myToken',
41144115
'uid_file_owner' => 'owner',
41154116
'displayname_file_owner' => 'owner',
@@ -4165,7 +4166,7 @@ public function dataFormatShare() {
41654166
'permissions' => 1,
41664167
'stime' => 946684862,
41674168
'parent' => null,
4168-
'expiration' => '2001-01-02 00:00:00',
4169+
'expiration' => '2001-01-02',
41694170
'token' => 'myToken',
41704171
'uid_file_owner' => 'owner',
41714172
'displayname_file_owner' => 'owner',
@@ -4218,7 +4219,7 @@ public function dataFormatShare() {
42184219
'permissions' => 1,
42194220
'stime' => 946684862,
42204221
'parent' => null,
4221-
'expiration' => '2001-02-03 00:00:00',
4222+
'expiration' => '2001-02-03',
42224223
'token' => null,
42234224
'uid_file_owner' => 'owner',
42244225
'displayname_file_owner' => 'owner',
@@ -4268,7 +4269,7 @@ public function dataFormatShare() {
42684269
'permissions' => 1,
42694270
'stime' => 946684862,
42704271
'parent' => null,
4271-
'expiration' => '2001-02-03 00:00:00',
4272+
'expiration' => '2001-02-03',
42724273
'token' => null,
42734274
'uid_file_owner' => 'owner',
42744275
'displayname_file_owner' => 'owner',
@@ -4647,6 +4648,7 @@ public function testFormatShare(array $expects, \OCP\Share\IShare $share, array
46474648
$this->rootFolder->method('getUserFolder')
46484649
->with($this->currentUser)
46494650
->willReturnSelf();
4651+
$this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC'));
46504652

46514653
if (!$exception) {
46524654
$this->rootFolder->method('getById')

‎build/integration/features/bootstrap/Sharing.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ public function createShare($user,
328328
public function isFieldInResponse($field, $contentExpected) {
329329
$data = simplexml_load_string($this->response->getBody())->data[0];
330330
if ((string)$field == 'expiration') {
331-
$contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00";
331+
$contentExpected = date('Y-m-d', strtotime($contentExpected));
332332
}
333333
if (count($data->element) > 0) {
334334
foreach ($data as $element) {
@@ -626,7 +626,7 @@ private function assertFieldIsInReturnedShare(string $field, string $contentExpe
626626
}
627627

628628
if ($field === 'expiration' && !empty($contentExpected)) {
629-
$contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00";
629+
$contentExpected = date('Y-m-d', strtotime($contentExpected));
630630
}
631631

632632
if ($contentExpected === 'A_NUMBER') {

‎lib/private/Share20/Manager.php‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,7 @@ protected function validateExpirationDateInternal(IShare $share) {
382382
$expirationDate = $share->getExpirationDate();
383383

384384
if ($expirationDate !== null) {
385-
//Make sure the expiration date is a date
386-
$expirationDate->setTime(0, 0, 0);
387-
388385
$date = new \DateTime();
389-
$date->setTime(0, 0, 0);
390386
if ($date >= $expirationDate) {
391387
$message = $this->l->t('Expiration date is in the past');
392388
throw new GenericShareException($message, $message, 404);
@@ -414,8 +410,6 @@ protected function validateExpirationDateInternal(IShare $share) {
414410
}
415411
if ($fullId === null && $expirationDate === null && $defaultExpireDate) {
416412
$expirationDate = new \DateTime();
417-
$expirationDate->setTime(0, 0, 0);
418-
419413
$days = (int)$this->config->getAppValue('core', $configProp, (string)$defaultExpireDays);
420414
if ($days > $defaultExpireDays) {
421415
$days = $defaultExpireDays;
@@ -430,7 +424,6 @@ protected function validateExpirationDateInternal(IShare $share) {
430424
}
431425

432426
$date = new \DateTime();
433-
$date->setTime(0, 0, 0);
434427
$date->add(new \DateInterval('P' . $defaultExpireDays . 'D'));
435428
if ($date < $expirationDate) {
436429
$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) {
469462
$expirationDate = $share->getExpirationDate();
470463

471464
if ($expirationDate !== null) {
472-
//Make sure the expiration date is a date
473-
$expirationDate->setTime(0, 0, 0);
474-
475465
$date = new \DateTime();
476-
$date->setTime(0, 0, 0);
477466
if ($date >= $expirationDate) {
478467
$message = $this->l->t('Expiration date is in the past');
479468
throw new GenericShareException($message, $message, 404);
@@ -490,7 +479,6 @@ protected function validateExpirationDateLink(IShare $share) {
490479

491480
if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) {
492481
$expirationDate = new \DateTime();
493-
$expirationDate->setTime(0, 0, 0);
494482

495483
$days = (int)$this->config->getAppValue('core', 'link_defaultExpDays', (string)$this->shareApiLinkDefaultExpireDays());
496484
if ($days > $this->shareApiLinkDefaultExpireDays()) {
@@ -506,7 +494,6 @@ protected function validateExpirationDateLink(IShare $share) {
506494
}
507495

508496
$date = new \DateTime();
509-
$date->setTime(0, 0, 0);
510497
$date->add(new \DateInterval('P' . $this->shareApiLinkDefaultExpireDays() . 'D'));
511498
if ($date < $expirationDate) {
512499
$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());

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -940,7 +940,7 @@ public function testValidateExpirationDateInternalEnforceButNotSetNewShare($shar
940940
self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]);
941941

942942
$this->assertNotNull($share->getExpirationDate());
943-
$this->assertEquals($expected, $share->getExpirationDate());
943+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
944944
}
945945

946946
/**
@@ -975,7 +975,7 @@ public function testValidateExpirationDateInternalEnforceRelaxedDefaultButNotSet
975975
self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]);
976976

977977
$this->assertNotNull($share->getExpirationDate());
978-
$this->assertEquals($expected, $share->getExpirationDate());
978+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
979979
}
980980

981981
/**
@@ -1050,7 +1050,7 @@ public function testValidateExpirationDateInternalEnforceValid($shareType) {
10501050

10511051
self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]);
10521052

1053-
$this->assertEquals($expected, $share->getExpirationDate());
1053+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
10541054
}
10551055

10561056
/**
@@ -1062,7 +1062,6 @@ public function testValidateExpirationDateInternalNoDefault($shareType) {
10621062
$date->setTime(1, 2, 3);
10631063

10641064
$expected = clone $date;
1065-
$expected->setTime(0, 0, 0);
10661065

10671066
$share = $this->manager->newShare();
10681067
$share->setShareType($shareType);
@@ -1107,7 +1106,6 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) {
11071106

11081107
$expected = new \DateTime();
11091108
$expected->add(new \DateInterval('P3D'));
1110-
$expected->setTime(0, 0, 0);
11111109

11121110
if ($shareType === IShare::TYPE_USER) {
11131111
$this->config->method('getAppValue')
@@ -1128,12 +1126,12 @@ public function testValidateExpirationDateInternalNoDateDefault($shareType) {
11281126
$hookListener = $this->getMockBuilder('Dummy')->setMethods(['listener'])->getMock();
11291127
\OCP\Util::connectHook('\OC\Share', 'verifyExpirationDate', $hookListener, 'listener');
11301128
$hookListener->expects($this->once())->method('listener')->with($this->callback(function ($data) use ($expected) {
1131-
return $data['expirationDate'] == $expected;
1129+
return $data['expirationDate']->format('YmdH') == $expected->format('YmdH');
11321130
}));
11331131

11341132
self::invokePrivate($this->manager, 'validateExpirationDateInternal', [$share]);
11351133

1136-
$this->assertEquals($expected, $share->getExpirationDate());
1134+
$this->assertEquals($expected->format('YmdH'), $share->getExpirationDate()->format('YmdH'));
11371135
}
11381136

11391137
/**
@@ -1145,7 +1143,6 @@ public function testValidateExpirationDateInternalDefault($shareType) {
11451143
$future->setTime(1, 2, 3);
11461144

11471145
$expected = clone $future;
1148-
$expected->setTime(0, 0, 0);
11491146

11501147
$share = $this->manager->newShare();
11511148
$share->setShareType($shareType);
@@ -1318,7 +1315,7 @@ public function testValidateExpirationDateEnforceButNotSetNewShare() {
13181315
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
13191316

13201317
$this->assertNotNull($share->getExpirationDate());
1321-
$this->assertEquals($expected, $share->getExpirationDate());
1318+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
13221319
}
13231320

13241321
public function testValidateExpirationDateEnforceRelaxedDefaultButNotSetNewShare() {
@@ -1339,7 +1336,7 @@ public function testValidateExpirationDateEnforceRelaxedDefaultButNotSetNewShare
13391336
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
13401337

13411338
$this->assertNotNull($share->getExpirationDate());
1342-
$this->assertEquals($expected, $share->getExpirationDate());
1339+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
13431340
}
13441341

13451342
public function testValidateExpirationDateEnforceTooFarIntoFuture() {
@@ -1388,7 +1385,7 @@ public function testValidateExpirationDateEnforceValid() {
13881385

13891386
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
13901387

1391-
$this->assertEquals($expected, $share->getExpirationDate());
1388+
$this->assertEquals($expected, $share->getExpirationDate()->setTime(0, 0, 0));
13921389
}
13931390

13941391
public function testValidateExpirationDateNoDefault() {
@@ -1397,7 +1394,6 @@ public function testValidateExpirationDateNoDefault() {
13971394
$date->setTime(1, 2, 3);
13981395

13991396
$expected = clone $date;
1400-
$expected->setTime(0, 0, 0);
14011397

14021398
$share = $this->manager->newShare();
14031399
$share->setExpirationDate($date);
@@ -1433,7 +1429,6 @@ public function testValidateExpirationDateNoDateDefault() {
14331429

14341430
$expected = new \DateTime();
14351431
$expected->add(new \DateInterval('P3D'));
1436-
$expected->setTime(0, 0, 0);
14371432

14381433
$this->config->method('getAppValue')
14391434
->willReturnMap([
@@ -1445,12 +1440,12 @@ public function testValidateExpirationDateNoDateDefault() {
14451440
$hookListener = $this->getMockBuilder('Dummy')->setMethods(['listener'])->getMock();
14461441
\OCP\Util::connectHook('\OC\Share', 'verifyExpirationDate', $hookListener, 'listener');
14471442
$hookListener->expects($this->once())->method('listener')->with($this->callback(function ($data) use ($expected) {
1448-
return $data['expirationDate'] == $expected;
1443+
return $data['expirationDate']->format('YmdH') == $expected->format('YmdH');
14491444
}));
14501445

14511446
self::invokePrivate($this->manager, 'validateExpirationDateLink', [$share]);
14521447

1453-
$this->assertEquals($expected, $share->getExpirationDate());
1448+
$this->assertEquals($expected->format('YmdH'), $share->getExpirationDate()->format('YmdH'));
14541449
}
14551450

14561451
public function testValidateExpirationDateDefault() {
@@ -1459,7 +1454,6 @@ public function testValidateExpirationDateDefault() {
14591454
$future->setTime(1, 2, 3);
14601455

14611456
$expected = clone $future;
1462-
$expected->setTime(0, 0, 0);
14631457

14641458
$share = $this->manager->newShare();
14651459
$share->setExpirationDate($future);

0 commit comments

Comments
 (0)