From 13688f39a734e3c94888666d2c1b62e992a6f477 Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Wed, 20 Mar 2024 16:41:26 +0100 Subject: 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 | 44 +++--- apps/files_sharing/openapi.json | 4 +- lib/private/Share20/Manager.php | 167 +++++++++++---------- lib/private/Share20/Share.php | 16 +- lib/public/Share/IShare.php | 24 ++- 5 files changed, 144 insertions(+), 111 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index eb60a3a85f1..28cf46a0328 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -579,7 +579,7 @@ class ShareAPIController extends OCSController { string $publicUpload = 'false', string $password = '', string $sendPasswordByTalk = null, - string $expireDate = '', + string $expireDate = null, string $note = '', string $label = '', string $attributes = null @@ -653,6 +653,21 @@ class ShareAPIController extends OCSController { $share = $this->setShareAttributes($share, $attributes); } + if ($expireDate !== null) { + if ($expireDate !== '') { + try { + $expireDate = $this->parseDate($expireDate); + $share->setExpirationDate($expireDate); + } 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); + } + } + $share->setSharedBy($this->currentUser); $this->checkInheritedAttributes($share); @@ -739,14 +754,6 @@ class ShareAPIController extends OCSController { $share->setSharedWith($shareWith); $share->setPermissions($permissions); - if ($expireDate !== '') { - try { - $expireDate = $this->parseDate($expireDate); - $share->setExpirationDate($expireDate); - } catch (\Exception $e) { - throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD')); - } - } $share->setSharedWithDisplayName($this->getCachedFederatedDisplayName($shareWith, false)); } elseif ($shareType === IShare::TYPE_REMOTE_GROUP) { @@ -760,14 +767,7 @@ class ShareAPIController extends OCSController { $share->setSharedWith($shareWith); $share->setPermissions($permissions); - if ($expireDate !== '') { - try { - $expireDate = $this->parseDate($expireDate); - $share->setExpirationDate($expireDate); - } catch (\Exception $e) { - throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD')); - } - } + } elseif ($shareType === IShare::TYPE_CIRCLE) { if (!\OC::$server->getAppManager()->isEnabledForUser('circles') || !class_exists('\OCA\Circles\ShareByCircleProvider')) { throw new OCSNotFoundException($this->l->t('You cannot share to a Circle if the app is not enabled')); @@ -803,16 +803,6 @@ class ShareAPIController extends OCSController { throw new OCSBadRequestException($this->l->t('Unknown share type')); } - //Expire date - if ($expireDate !== '') { - try { - $expireDate = $this->parseDate($expireDate); - $share->setExpirationDate($expireDate); - } catch (\Exception $e) { - throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD')); - } - } - $share->setShareType($shareType); if ($note !== '') { diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index cf81e358b5d..ea43488ec49 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1563,10 +1563,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 62dfeafe26a..d3ba1549c61 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -392,26 +392,6 @@ class Manager implements IManager { $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(); @@ -423,28 +403,53 @@ class Manager implements IManager { $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); + } } } @@ -477,51 +482,57 @@ class Manager implements IManager { */ 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 0a50fa0ccfb..8523557a5b4 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -99,7 +99,6 @@ class Share implements IShare { /** @var ICacheEntry|null */ private $nodeCacheEntry; - /** @var bool */ private $hideDownload = false; @@ -421,6 +420,21 @@ class Share implements IShare { 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 40548c6c73d..84388c00bbe 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -385,20 +385,38 @@ interface IShare { /** * 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 \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 27.1.10 + */ + public function setNoExpirationDate(bool $noExpirationDate); + + + /** + * Get value of overwrite falsy expiry date flag + * + * @return bool + * @since 27.1.10 + */ + public function getNoExpirationDate(); + /** * Is the share expired ? * -- cgit v1.2.3 -- cgit v1.2.3 From bdeba6590db488da2b204b839ef3df9ef43ae7e4 Mon Sep 17 00:00:00 2001 From: fenn-cs Date: Wed, 22 May 2024 14:17:24 +0100 Subject: 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 --- .../lib/Controller/ShareAPIController.php | 8 +++--- build/integration/features/bootstrap/Sharing.php | 22 +++++++++------- .../sharing_features/sharing-v1.feature | 18 ++++++++++++++ lib/private/Share20/Manager.php | 29 +++++++++++----------- lib/private/Share20/Share.php | 4 ++- 5 files changed, 52 insertions(+), 29 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 28cf46a0328..23a35a0550d 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -233,7 +233,7 @@ class ShareAPIController extends OCSController { $result['share_with'] = $share->getSharedWith(); $result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith(); $result['share_with_displayname_unique'] = $sharedWith !== null ? ( - !empty($sharedWith->getSystemEMailAddress()) ? $sharedWith->getSystemEMailAddress() : $sharedWith->getUID() + !empty($sharedWith->getSystemEMailAddress()) ? $sharedWith->getSystemEMailAddress() : $sharedWith->getUID() ) : $share->getSharedWith(); $result['status'] = []; @@ -332,7 +332,7 @@ class ShareAPIController extends OCSController { $result['attributes'] = null; if ($attributes = $share->getAttributes()) { - $result['attributes'] = \json_encode($attributes->toArray()); + $result['attributes'] = \json_encode($attributes->toArray()); } return $result; @@ -1275,8 +1275,8 @@ class ShareAPIController extends OCSController { } if (!$this->hasPermission($newPermissions, Constants::PERMISSION_READ) && ( - $this->hasPermission($newPermissions, Constants::PERMISSION_UPDATE) || $this->hasPermission($newPermissions, Constants::PERMISSION_DELETE) - )) { + $this->hasPermission($newPermissions, Constants::PERMISSION_UPDATE) || $this->hasPermission($newPermissions, Constants::PERMISSION_DELETE) + )) { throw new OCSBadRequestException($this->l->t('Share must have READ permission if UPDATE or DELETE permission is set')); } } diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index 2a6a509d65f..0f1b2215639 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -81,7 +81,9 @@ trait Sharing { $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; } @@ -270,13 +272,13 @@ trait Sharing { } public function createShare($user, - $path = null, - $shareType = null, - $shareWith = null, - $publicUpload = null, - $password = null, - $permissions = null, - $viewOnly = false) { + $path = null, + $shareType = null, + $shareWith = null, + $publicUpload = null, + $password = null, + $permissions = null, + $viewOnly = false) { $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/apps/files_sharing/api/v{$this->sharingApiVersion}/shares"; $client = new Client(); $options = [ @@ -328,7 +330,9 @@ trait Sharing { 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 ca030bd3a31..2b5d4e89331 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" diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index d3ba1549c61..1bb4657ef79 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -298,7 +298,7 @@ class Manager implements IManager { $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); $permissions = 0; - + $isReshare = $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy(); if (!$isReshare && $isUpdate) { // in case of update on owner-less filesystem, we use share owner to improve reshare detection @@ -406,7 +406,7 @@ class Manager implements IManager { // 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 (!($share->getNoExpirationDate() && !$isEnforced)) { if ($expirationDate != null) { $expirationDate->setTimezone($this->dateTimeZone->getTimeZone()); $expirationDate->setTime(0, 0, 0); @@ -486,11 +486,11 @@ class Manager implements IManager { // 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 (!($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) { @@ -506,24 +506,24 @@ class Manager implements IManager { } 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')); } - + // 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')); @@ -532,7 +532,6 @@ class Manager implements IManager { throw new GenericShareException($message, $message, 404); } } - } $accepted = true; @@ -907,12 +906,12 @@ class Manager implements IManager { * @param \DateTime|null $expiration */ protected function sendMailNotification(IL10N $l, - $filename, - $link, - $initiator, - $shareWith, - \DateTime $expiration = null, - $note = '') { + $filename, + $link, + $initiator, + $shareWith, + \DateTime $expiration = null, + $note = '') { $initiatorUser = $this->userManager->get($initiator); $initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator; diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index 8523557a5b4..b911a3b4623 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -29,8 +29,8 @@ */ namespace OC\Share20; -use OCP\Files\File; use OCP\Files\Cache\ICacheEntry; +use OCP\Files\File; use OCP\Files\FileInfo; use OCP\Files\IRootFolder; use OCP\Files\Node; @@ -102,6 +102,8 @@ class Share implements IShare { /** @var bool */ private $hideDownload = false; + private bool $noExpirationDate = false; + public function __construct(IRootFolder $rootFolder, IUserManager $userManager) { $this->rootFolder = $rootFolder; $this->userManager = $userManager; -- cgit v1.2.3