From ee65cd0bd80d3c46d1210e9a4e8b55b0252de450 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 8 Feb 2016 10:42:15 +0100 Subject: [PATCH 1/3] Only set the default expiration date on share creation Fixes #19685 The default expiration date should only be set when we create a new share. So if a share is created and the expiration date is unset. And after that the password is updated the expiration date should remain unset. --- lib/private/share20/manager.php | 2 +- lib/private/share20/share.php | 3 +++ lib/public/share/ishare.php | 2 +- tests/lib/share20/managertest.php | 16 ++++++++++++++++ 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index 7cd44a7cb3..c00d7aa407 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -261,7 +261,7 @@ class Manager implements IManager { } // If expiredate is empty set a default one if there is a default - if ($expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { + if ($share->getFullId() === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { $expirationDate = new \DateTime(); $expirationDate->setTime(0,0,0); $expirationDate->add(new \DateInterval('P'.$this->shareApiLinkDefaultExpireDays().'D')); diff --git a/lib/private/share20/share.php b/lib/private/share20/share.php index cd30f24c42..eb112d3794 100644 --- a/lib/private/share20/share.php +++ b/lib/private/share20/share.php @@ -90,6 +90,9 @@ class Share implements \OCP\Share\IShare { * @inheritdoc */ public function getFullId() { + if ($this->providerId === null || $this->id === null) { + return null; + } return $this->providerId . ':' . $this->id; } diff --git a/lib/public/share/ishare.php b/lib/public/share/ishare.php index 5054a886af..8f5175b0db 100644 --- a/lib/public/share/ishare.php +++ b/lib/public/share/ishare.php @@ -46,7 +46,7 @@ interface IShare { * Get the full share id. This is the :. * The full id is unique in the system. * - * @return string + * @return string|null null is returned when the fullId can't be constructed * @since 9.0.0 */ public function getFullId(); diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index ea0fb1838d..73a1b0a653 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -890,6 +890,22 @@ class ManagerTest extends \Test\TestCase { $this->invokePrivate($this->manager, 'validateExpirationDate', [$share]); } + public function testValidateExpirationDateExistingShareNoDefault() { + $share = $this->manager->newShare(); + + $share->setId('42')->setProviderId('foo'); + + $this->config->method('getAppValue') + ->will($this->returnValueMap([ + ['core', 'shareapi_default_expire_date', 'no', 'yes'], + ['core', 'shareapi_expire_after_n_days', '7', '6'], + ])); + + $this->invokePrivate($this->manager, 'validateExpirationDate', [$share]); + + $this->assertEquals(null, $share->getExpirationDate()); + } + /** * @expectedException Exception * @expectedExceptionMessage Only sharing with group members is allowed From 426a12baaa6f30aee081358f61108218e11c9e07 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 8 Feb 2016 15:28:36 +0100 Subject: [PATCH 2/3] Throw exception --- lib/private/share20/manager.php | 17 ++++++++++++++--- lib/private/share20/share.php | 2 +- lib/public/share/ishare.php | 3 ++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index c00d7aa407..ddf1fa57c5 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -261,7 +261,14 @@ class Manager implements IManager { } // If expiredate is empty set a default one if there is a default - if ($share->getFullId() === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { + $fullId = null; + try { + $fullId = $share->getFullId(); + } catch (\UnexpectedValueException $e) { + // This is a new share + } + + if ($fullId === null && $expirationDate === null && $this->shareApiLinkDefaultExpireDate()) { $expirationDate = new \DateTime(); $expirationDate->setTime(0,0,0); $expirationDate->add(new \DateInterval('P'.$this->shareApiLinkDefaultExpireDays().'D')); @@ -360,8 +367,12 @@ class Manager implements IManager { $provider = $this->factory->getProviderForType(\OCP\Share::SHARE_TYPE_GROUP); $existingShares = $provider->getSharesByPath($share->getNode()); foreach($existingShares as $existingShare) { - if ($existingShare->getFullId() === $share->getFullId()) { - continue; + try { + if ($existingShare->getFullId() === $share->getFullId()) { + continue; + } + } catch (\UnexpectedValueException $e) { + //It is a new share so just continue } if ($existingShare->getSharedWith() === $share->getSharedWith()) { diff --git a/lib/private/share20/share.php b/lib/private/share20/share.php index eb112d3794..400fc1db64 100644 --- a/lib/private/share20/share.php +++ b/lib/private/share20/share.php @@ -91,7 +91,7 @@ class Share implements \OCP\Share\IShare { */ public function getFullId() { if ($this->providerId === null || $this->id === null) { - return null; + throw new \UnexpectedValueException; } return $this->providerId . ':' . $this->id; } diff --git a/lib/public/share/ishare.php b/lib/public/share/ishare.php index 8f5175b0db..fdf40f19e5 100644 --- a/lib/public/share/ishare.php +++ b/lib/public/share/ishare.php @@ -46,8 +46,9 @@ interface IShare { * Get the full share id. This is the :. * The full id is unique in the system. * - * @return string|null null is returned when the fullId can't be constructed + * @return string * @since 9.0.0 + * @throws \UnexpectedValueException If the fullId could not be constructed */ public function getFullId(); From 49726470abe14b23543dab0d5ba5af939d1f7634 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 8 Feb 2016 21:08:03 +0100 Subject: [PATCH 3/3] Catch exception --- lib/private/share20/manager.php | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index ddf1fa57c5..5d37921a1d 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -322,8 +322,12 @@ class Manager implements IManager { $existingShares = $provider->getSharesByPath($share->getNode()); foreach($existingShares as $existingShare) { // Ignore if it is the same share - if ($existingShare->getFullId() === $share->getFullId()) { - continue; + try { + if ($existingShare->getFullId() === $share->getFullId()) { + continue; + } + } catch (\UnexpectedValueException $e) { + //Shares are not identical } // Identical share already existst @@ -569,7 +573,11 @@ class Manager implements IManager { throw new \Exception('The Share API is disabled'); } - $originalShare = $this->getShareById($share->getFullId()); + try { + $originalShare = $this->getShareById($share->getFullId()); + } catch (\UnexpectedValueException $e) { + throw new \InvalidArgumentException('Share does not have a full id'); + } // We can't change the share type! if ($share->getShareType() !== $originalShare->getShareType()) { @@ -674,10 +682,15 @@ class Manager implements IManager { * * @param \OCP\Share\IShare $share * @throws ShareNotFound + * @throws \InvalidArgumentException */ public function deleteShare(\OCP\Share\IShare $share) { // Just to make sure we have all the info - $share = $this->getShareById($share->getFullId()); + try { + $share = $this->getShareById($share->getFullId()); + } catch (\UnexpectedValueException $e) { + throw new \InvalidArgumentException('Share does not have a full id'); + } $formatHookParams = function(\OCP\Share\IShare $share) { // Prepare hook