From 59a5b4e0bddbc29d4e16a930972be34873a73e0c Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Wed, 20 Mar 2024 16:41:26 +0100 Subject: [PATCH 1/3] fix(shareManager): Respect empty `expireDate` in server If `expireDate` is an empty string and not `null` then the server should not set a default. Signed-off-by: fenn-cs --- .../lib/Controller/ShareAPIController.php | 23 ++- apps/files_sharing/openapi.json | 4 +- lib/private/Share20/Manager.php | 167 ++++++++++-------- lib/private/Share20/Share.php | 19 +- lib/public/Share/IShare.php | 24 ++- 5 files changed, 144 insertions(+), 93 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 1bdcee11c45ee..b9f89109304ce 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -549,7 +549,8 @@ public function deleteShare(string $id): DataResponse { * @param string $publicUpload If public uploading is allowed * @param string $password Password for the share * @param string|null $sendPasswordByTalk Send the password for the share over Talk - * @param string $expireDate Expiry date of the share using user timezone at 00:00. It means date in UTC timezone will be used. + * @param ?string $expireDate The expiry date of the share in the user's timezone (UTC) at 00:00. + * If $expireDate is not supplied or set to `null`, the system default will be used. * @param string $note Note for the share * @param string $label Label for the share (only used in link and email) * @param string|null $attributes Additional attributes for the share @@ -571,7 +572,7 @@ public function createShare( string $publicUpload = 'false', string $password = '', ?string $sendPasswordByTalk = null, - string $expireDate = '', + ?string $expireDate = null, string $note = '', string $label = '', ?string $attributes = null @@ -646,12 +647,18 @@ public function createShare( } //Expire date - if ($expireDate !== '') { - try { - $expireDateTime = $this->parseDate($expireDate); - $share->setExpirationDate($expireDateTime); - } catch (\Exception $e) { - throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD')); + if ($expireDate !== null) { + if ($expireDate !== '') { + try { + $expireDateTime = $this->parseDate($expireDate); + $share->setExpirationDate($expireDateTime); + } catch (\Exception $e) { + throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD')); + } + } else { + // Client sent empty string for expire date. + // Set noExpirationDate to true so overwrite is prevented. + $share->setNoExpirationDate(true); } } diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index 6808993af3fa6..f5a8ead3eba3b 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1781,10 +1781,10 @@ { "name": "expireDate", "in": "query", - "description": "Expiry date of the share using user timezone at 00:00. It means date in UTC timezone will be used.", + "description": "The expiry date of the share in the user's timezone (UTC) at 00:00. If $expireDate is not supplied or set to `null`, the system default will be used.", "schema": { "type": "string", - "default": "" + "nullable": true } }, { diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4f097cdc3e280..4b44edd8b0138 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -348,26 +348,6 @@ protected function validateExpirationDateInternal(IShare $share) { $expirationDate = $share->getExpirationDate(); - if ($expirationDate !== null) { - $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); - $expirationDate->setTime(0, 0, 0); - - $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'); - throw new GenericShareException($message, $message, 404); - } - } - - // If expiredate is empty set a default one if there is a default - $fullId = null; - try { - $fullId = $share->getFullId(); - } catch (\UnexpectedValueException $e) { - // This is a new share - } - if ($isRemote) { $defaultExpireDate = $this->shareApiRemoteDefaultExpireDate(); $defaultExpireDays = $this->shareApiRemoteDefaultExpireDays(); @@ -379,28 +359,53 @@ protected function validateExpirationDateInternal(IShare $share) { $configProp = 'internal_defaultExpDays'; $isEnforced = $this->shareApiInternalDefaultExpireDateEnforced(); } - if ($fullId === null && $expirationDate === null && $defaultExpireDate) { - $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; + + // If $expirationDate is falsy, noExpirationDate is true and expiration not enforced + // Then skip expiration date validation as null is accepted + if(!($share->getNoExpirationDate() && !$isEnforced)) { + if ($expirationDate != null) { + $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); + $expirationDate->setTime(0, 0, 0); + + $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'); + throw new GenericShareException($message, $message, 404); + } } - $expirationDate->add(new \DateInterval('P' . $days . 'D')); - } - // If we enforce the expiration date check that is does not exceed - if ($isEnforced) { - if ($expirationDate === null) { - throw new \InvalidArgumentException('Expiration date is enforced'); + // If expiredate is empty set a default one if there is a default + $fullId = null; + try { + $fullId = $share->getFullId(); + } catch (\UnexpectedValueException $e) { + // This is a new share } - $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); - $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); - throw new GenericShareException($message, $message, 404); + if ($fullId === null && $expirationDate === null && $defaultExpireDate) { + $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; + } + $expirationDate->add(new \DateInterval('P' . $days . 'D')); + } + + // If we enforce the expiration date check that is does not exceed + if ($isEnforced) { + if (empty($expirationDate)) { + throw new \InvalidArgumentException('Expiration date is enforced'); + } + + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); + $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); + throw new GenericShareException($message, $message, 404); + } } } @@ -433,51 +438,57 @@ protected function validateExpirationDateInternal(IShare $share) { */ protected function validateExpirationDateLink(IShare $share) { $expirationDate = $share->getExpirationDate(); - - if ($expirationDate !== null) { - $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); - $expirationDate->setTime(0, 0, 0); - - $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'); - throw new GenericShareException($message, $message, 404); + $isEnforced = $this->shareApiLinkDefaultExpireDateEnforced(); + + // If $expirationDate is falsy, noExpirationDate is true and expiration not enforced + // Then skip expiration date validation as null is accepted + if(!($share->getNoExpirationDate() && !$isEnforced)) { + if ($expirationDate !== null) { + $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); + $expirationDate->setTime(0, 0, 0); + + $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'); + throw new GenericShareException($message, $message, 404); + } } - } - - // If expiredate is empty set a default one if there is a default - $fullId = null; - try { - $fullId = $share->getFullId(); - } catch (\UnexpectedValueException $e) { - // This is a new share - } - if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { - $expirationDate = new \DateTime('now', $this->dateTimeZone->getTimeZone()); - $expirationDate->setTime(0, 0, 0); - - $days = (int)$this->config->getAppValue('core', 'link_defaultExpDays', (string)$this->shareApiLinkDefaultExpireDays()); - if ($days > $this->shareApiLinkDefaultExpireDays()) { - $days = $this->shareApiLinkDefaultExpireDays(); + // If expiredate is empty set a default one if there is a default + $fullId = null; + try { + $fullId = $share->getFullId(); + } catch (\UnexpectedValueException $e) { + // This is a new share + } + + if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { + $expirationDate = new \DateTime('now', $this->dateTimeZone->getTimeZone()); + $expirationDate->setTime(0, 0, 0); + + $days = (int)$this->config->getAppValue('core', 'link_defaultExpDays', (string)$this->shareApiLinkDefaultExpireDays()); + if ($days > $this->shareApiLinkDefaultExpireDays()) { + $days = $this->shareApiLinkDefaultExpireDays(); + } + $expirationDate->add(new \DateInterval('P' . $days . 'D')); } - $expirationDate->add(new \DateInterval('P' . $days . 'D')); - } - - // If we enforce the expiration date check that is does not exceed - if ($this->shareApiLinkDefaultExpireDateEnforced()) { - if ($expirationDate === null) { - throw new \InvalidArgumentException('Expiration date is enforced'); + + // If we enforce the expiration date check that is does not exceed + if ($isEnforced) { + if (empty($expirationDate)) { + throw new \InvalidArgumentException('Expiration date is enforced'); + } + + $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); + $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()); + throw new GenericShareException($message, $message, 404); + } } - $date = new \DateTime('now', $this->dateTimeZone->getTimeZone()); - $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()); - throw new GenericShareException($message, $message, 404); - } } $accepted = true; diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index e1d8818216b7a..0bc85712647c7 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -90,13 +90,13 @@ class Share implements IShare { private $mailSend; /** @var string */ private $label = ''; - /** @var ICacheEntry|null */ private $nodeCacheEntry; - /** @var bool */ private $hideDownload = false; + private bool $noExpirationDate = false; + public function __construct( private IRootFolder $rootFolder, private IUserManager $userManager, @@ -421,6 +421,21 @@ public function getExpirationDate() { return $this->expireDate; } + /** + * @inheritdoc + */ + public function setNoExpirationDate(bool $noExpirationDate) { + $this->noExpirationDate = $noExpirationDate; + return $this; + } + + /** + * @inheritdoc + */ + public function getNoExpirationDate(): bool { + return $this->noExpirationDate; + } + /** * @inheritdoc */ diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index 0961631ea92f5..0f4f9561eeb65 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -385,20 +385,38 @@ public function getNote(); /** * Set the expiration date * - * @param null|\DateTime $expireDate + * @param \DateTime|null $expireDate * @return \OCP\Share\IShare The modified object * @since 9.0.0 */ - public function setExpirationDate($expireDate); + public function setExpirationDate(\DateTime|null $expireDate); /** * Get the expiration date * - * @return null|\DateTime + * @return \DateTime|null * @since 9.0.0 */ public function getExpirationDate(); + /** + * Set overwrite flag for falsy expiry date vavlues + * + * @param bool $noExpirationDate + * @return \OCP\Share\IShare The modified object + * @since 30.0.0 + */ + public function setNoExpirationDate(bool $noExpirationDate); + + + /** + * Get value of overwrite falsy expiry date flag + * + * @return bool + * @since 30.0.0 + */ + public function getNoExpirationDate(); + /** * Is the share expired ? * From f5a433f946e14eece904ea935cdc7ce29af0b9b7 Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Tue, 30 Apr 2024 12:19:22 +0100 Subject: [PATCH 2/3] refactor(shareManager): Use constructor property promotion Signed-off-by: fenn-cs --- lib/private/Share20/Manager.php | 98 ++++++++------------------------- 1 file changed, 23 insertions(+), 75 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4b44edd8b0138..11e246e6d2100 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -84,86 +84,34 @@ * This class is the communication hub for all sharing related operations. */ class Manager implements IManager { - /** @var IProviderFactory */ - private $factory; - private LoggerInterface $logger; - /** @var IConfig */ - private $config; - /** @var ISecureRandom */ - private $secureRandom; - /** @var IHasher */ - private $hasher; - /** @var IMountManager */ - private $mountManager; - /** @var IGroupManager */ - private $groupManager; - /** @var IL10N */ - private $l; - /** @var IFactory */ - private $l10nFactory; - /** @var IUserManager */ - private $userManager; - /** @var IRootFolder */ - private $rootFolder; - /** @var LegacyHooks */ - private $legacyHooks; - /** @var IMailer */ - private $mailer; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var \OC_Defaults */ - private $defaults; - /** @var IEventDispatcher */ - private $dispatcher; - /** @var IUserSession */ - private $userSession; - /** @var KnownUserService */ - private $knownUserService; - private ShareDisableChecker $shareDisableChecker; - private IDateTimeZone $dateTimeZone; + + private IL10N|null $l; + private LegacyHooks $legacyHooks; public function __construct( - LoggerInterface $logger, - IConfig $config, - ISecureRandom $secureRandom, - IHasher $hasher, - IMountManager $mountManager, - IGroupManager $groupManager, - IFactory $l10nFactory, - IProviderFactory $factory, - IUserManager $userManager, - IRootFolder $rootFolder, - IMailer $mailer, - IURLGenerator $urlGenerator, - \OC_Defaults $defaults, - IEventDispatcher $dispatcher, - IUserSession $userSession, - KnownUserService $knownUserService, - ShareDisableChecker $shareDisableChecker, - IDateTimeZone $dateTimeZone, + private LoggerInterface $logger, + private IConfig $config, + private ISecureRandom $secureRandom, + private IHasher $hasher, + private IMountManager $mountManager, + private IGroupManager $groupManager, + private IFactory $l10nFactory, + private IProviderFactory $factory, + private IUserManager $userManager, + private IRootFolder $rootFolder, + private IMailer $mailer, + private IURLGenerator $urlGenerator, + private \OC_Defaults $defaults, + private IEventDispatcher $dispatcher, + private IUserSession $userSession, + private KnownUserService $knownUserService, + private ShareDisableChecker $shareDisableChecker, + private IDateTimeZone $dateTimeZone ) { - $this->logger = $logger; - $this->config = $config; - $this->secureRandom = $secureRandom; - $this->hasher = $hasher; - $this->mountManager = $mountManager; - $this->groupManager = $groupManager; - $this->l = $l10nFactory->get('lib'); - $this->l10nFactory = $l10nFactory; - $this->factory = $factory; - $this->userManager = $userManager; - $this->rootFolder = $rootFolder; + $this->l = $this->l10nFactory->get('lib'); // The constructor of LegacyHooks registers the listeners of share events // do not remove if those are not properly migrated - $this->legacyHooks = new LegacyHooks($dispatcher); - $this->mailer = $mailer; - $this->urlGenerator = $urlGenerator; - $this->defaults = $defaults; - $this->dispatcher = $dispatcher; - $this->userSession = $userSession; - $this->knownUserService = $knownUserService; - $this->shareDisableChecker = $shareDisableChecker; - $this->dateTimeZone = $dateTimeZone; + $this->legacyHooks = new LegacyHooks($this->dispatcher); } /** From d41d885713909360287a4e165ac3603bf6bf7460 Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Wed, 22 May 2024 14:17:24 +0100 Subject: [PATCH 3/3] test(Sharing): Integration test for no expiration set date for share - Verify that explicitly sending empty `expireDate` param to server overwrite default and sets not expiry date, if non is enforced. - Update tests to avoid converting empty string to date. Signed-off-by: fenn-cs --- .../integration/features/bootstrap/Sharing.php | 8 ++++++-- .../sharing_features/sharing-v1.feature | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index 5677dd7285723..c0affcb9cdad5 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -54,7 +54,9 @@ public function asCreatingAShareWith($user, $body) { $fd = $body->getRowsHash(); if (array_key_exists('expireDate', $fd)) { $dateModification = $fd['expireDate']; - $fd['expireDate'] = date('Y-m-d', strtotime($dateModification)); + if (!empty($dateModification)) { + $fd['expireDate'] = date('Y-m-d', strtotime($dateModification)); + } } $options['form_params'] = $fd; } @@ -301,7 +303,9 @@ public function createShare($user, public function isFieldInResponse($field, $contentExpected) { $data = simplexml_load_string($this->response->getBody())->data[0]; if ((string)$field == 'expiration') { - $contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00"; + if(!empty($contentExpected)) { + $contentExpected = date('Y-m-d', strtotime($contentExpected)) . " 00:00:00"; + } } if (count($data->element) > 0) { foreach ($data as $element) { diff --git a/build/integration/sharing_features/sharing-v1.feature b/build/integration/sharing_features/sharing-v1.feature index ca030bd3a31fa..2b5d4e8933184 100644 --- a/build/integration/sharing_features/sharing-v1.feature +++ b/build/integration/sharing_features/sharing-v1.feature @@ -229,6 +229,24 @@ Feature: sharing | url | AN_URL | | mimetype | httpd/unix-directory | + Scenario: Creating a new share with expiration date removed, when default expiration is set + Given user "user0" exists + And user "user1" exists + And parameter "shareapi_default_expire_date" of app "core" is set to "yes" + And As an "user0" + When creating a share with + | path | welcome.txt | + | shareWith | user1 | + | shareType | 0 | + | expireDate | | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And Getting info of last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And Share fields of last share match with + | expiration || + Scenario: Creating a new public share, updating its password and getting its info Given user "user0" exists And As an "user0"