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 @backportbot
Altahrim authored and backportbot[bot] committed Feb 26, 2024
1 parent 269bd24 commit 5d1ca31
Show file tree
Hide file tree
Showing 7 changed files with 115 additions and 43 deletions.
5 changes: 4 additions & 1 deletion apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Expand Up @@ -223,6 +223,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 @@ -1662,12 +1663,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 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;
}
Expand Down
20 changes: 12 additions & 8 deletions apps/files_sharing/tests/ApiTest.php
View file
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(date_default_timezone_get()));

return new ShareAPIController(
self::APP_NAME,
Expand Down Expand Up @@ -1063,10 +1064,9 @@ public function testUpdateShareExpireDate() {
$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
Expand All @@ -1077,6 +1077,8 @@ public function testUpdateShareExpireDate() {
$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
Expand Down Expand Up @@ -1290,12 +1292,14 @@ public function testShareStorageMountPoint() {

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],
];
}

Expand All @@ -1321,15 +1325,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 @@ -1353,7 +1357,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
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 @@ -98,7 +99,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
Expand Up @@ -835,6 +835,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 @@ -4603,6 +4604,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
Expand Up @@ -1314,7 +1314,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
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 @@ -119,6 +120,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 @@ -163,6 +166,7 @@ public function __construct(
$this->userSession = $userSession;
$this->knownUserService = $knownUserService;
$this->shareDisableChecker = $shareDisableChecker;
$this->dateTimeZone = $dateTimeZone;
}

/**
Expand Down Expand Up @@ -383,10 +387,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 @@ -414,9 +418,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 @@ -430,7 +433,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 @@ -470,10 +473,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 @@ -490,7 +493,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', $this->shareApiLinkDefaultExpireDays());
Expand All @@ -506,7 +509,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 @@ -528,6 +531,9 @@ protected function validateExpirationDateLink(IShare $share) {
throw new \Exception($message);
}

if ($expirationDate instanceof \DateTime) {
$expirationDate->setTimezone(new \DateTimeZone(date_default_timezone_get()));
}
$share->setExpirationDate($expirationDate);

return $share;
Expand Down

0 comments on commit 5d1ca31

Please sign in to comment.