Skip to content

Commit

Permalink
feat(share): save date and time for expiration
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@Altahrim
Altahrim committed Feb 19, 2024
1 parent 8822b16 commit cca99e8
Show file tree
Hide file tree
Showing 7 changed files with 115 additions and 44 deletions.
5 changes: 4 additions & 1 deletion apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

Expand Down Expand Up @@ -1695,12 +1696,14 @@ protected function canDeleteShareFromSelf(\OCP\Share\IShare $share): bool {
private function parseDate(string $expireDate): \DateTime {
try {
$date = new \DateTime(trim($expireDate, "\""), $this->dateTimeZone->getTimeZone());
// Make sure it expires at midnight in user 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;
}
Expand Down
21 changes: 12 additions & 9 deletions apps/files_sharing/tests/ApiTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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('Pacific/Auckland'));

return new ShareAPIController(
self::APP_NAME,
Expand Down Expand Up @@ -1059,10 +1060,10 @@ public function testUpdateShareExpireDate() {
$config->setAppValue('core', 'shareapi_default_expire_date', 'yes');
$config->setAppValue('core', 'shareapi_enforce_expire_date', 'yes');

$dateWithinRange = new \DateTime();
$dateWithinRange = new \DateTime('now', new \DateTimeZone('Pacific/Auckland'));
$dateWithinRange->setTime(0, 0, 0);
$dateWithinRange->add(new \DateInterval('P5D'));
$dateOutOfRange = new \DateTime();
$dateOutOfRange = new \DateTime('now', new \DateTimeZone('Pacific/Auckland'));
$dateOutOfRange->setTime(0, 0, 0);
$dateOutOfRange->add(new \DateInterval('P8D'));

Expand Down Expand Up @@ -1286,13 +1287,15 @@ public function testShareStorageMountPoint() {
}

public function datesProvider() {
$date = new \DateTime();
$date = new \DateTime('now', new \DateTimeZone('Pacific/Auckland'));
$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],
];
}

Expand All @@ -1318,15 +1321,15 @@ public function testPublicLinkExpireDate($date, $valid) {

$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']);
$this->assertEquals($url, $data['url']);

$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);
}
Expand All @@ -1341,7 +1344,7 @@ public function testCreatePublicLinkExpireDateValid() {
$config->setAppValue('core', 'shareapi_default_expire_date', 'yes');
$config->setAppValue('core', 'shareapi_enforce_expire_date', 'yes');

$date = new \DateTime();
$date = new \DateTime('now', new \DateTimeZone('Pacific/Auckland'));
$date->add(new \DateInterval('P5D'));

$ocs = $this->createOCS(self::TEST_FILES_SHARING_API_USER1);
Expand All @@ -1350,7 +1353,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']);
Expand Down
4 changes: 3 additions & 1 deletion apps/files_sharing/tests/CapabilitiesTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IMountManager;
use OCP\IConfig;
use OCP\IDateTimeZone;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IURLGenerator;
Expand Down Expand Up @@ -96,7 +97,8 @@ private function getResults(array $map) {
$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());
Expand Down
2 changes: 2 additions & 0 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down Expand Up @@ -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')
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Server.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,8 @@ public function __construct($webRoot, \OC\Config $config) {
$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;
Expand Down
26 changes: 16 additions & 10 deletions lib/private/Share20/Manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use OCP\Files\Node;
use OCP\HintException;
use OCP\IConfig;
use OCP\IDateTimeZone;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IURLGenerator;
Expand Down Expand Up @@ -120,6 +121,7 @@ class Manager implements IManager {
/** @var KnownUserService */
private $knownUserService;
private ShareDisableChecker $shareDisableChecker;
private IDateTimeZone $dateTimeZone;

public function __construct(
LoggerInterface $logger,
Expand All @@ -139,7 +141,8 @@ public function __construct(
IEventDispatcher $dispatcher,
IUserSession $userSession,
KnownUserService $knownUserService,
ShareDisableChecker $shareDisableChecker
ShareDisableChecker $shareDisableChecker,
IDateTimeZone $dateTimeZone,
) {
$this->logger = $logger;
$this->config = $config;
Expand All @@ -162,6 +165,7 @@ public function __construct(
$this->userSession = $userSession;
$this->knownUserService = $knownUserService;
$this->shareDisableChecker = $shareDisableChecker;
$this->dateTimeZone = $dateTimeZone;
}

/**
Expand Down Expand Up @@ -382,10 +386,10 @@ protected function validateExpirationDateInternal(IShare $share) {
$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');
Expand Down Expand Up @@ -413,9 +417,8 @@ protected function validateExpirationDateInternal(IShare $share) {
$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;
Expand All @@ -429,7 +432,7 @@ protected function validateExpirationDateInternal(IShare $share) {
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) {
Expand Down Expand Up @@ -469,10 +472,10 @@ protected function validateExpirationDateLink(IShare $share) {
$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');
Expand All @@ -489,7 +492,7 @@ protected function validateExpirationDateLink(IShare $share) {
}

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());
Expand All @@ -505,7 +508,7 @@ protected function validateExpirationDateLink(IShare $share) {
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) {
Expand All @@ -527,6 +530,9 @@ protected function validateExpirationDateLink(IShare $share) {
throw new \Exception($message);
}

if ($expirationDate instanceof \DateTimeInterface) {
$expirationDate->setTimezone(new \DateTimeZone(date_default_timezone_get()));

Check failure on line 534 in lib/private/Share20/Manager.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedInterfaceMethod 

lib/private/Share20/Manager.php:534:21: UndefinedInterfaceMethod: Method DateTimeInterface::setTimezone does not exist (see https://psalm.dev/181)

Check failure

Code scanning / Psalm

UndefinedInterfaceMethod Error

Method DateTimeInterface::setTimezone does not exist
}
$share->setExpirationDate($expirationDate);

return $share;
Expand Down
Loading

0 comments on commit cca99e8

Please sign in to comment.