From 5ed56d9edb54bf3f977ea12f44fca9e4b650c72b Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 5 Feb 2016 12:59:46 +0100 Subject: [PATCH 1/4] Delete a link share if it is expired on access If we access a link share we should check if it has expired already. If so we should remove it and throw a ShareNotFound exception --- lib/private/share20/manager.php | 6 ++++- tests/lib/share20/managertest.php | 43 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index d65fb927f9..a393cdeb89 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -831,7 +831,11 @@ class Manager implements IManager { $share = $provider->getShareByToken($token); - //TODO check if share expired + if ($share->getExpirationDate() !== null && + $share->getExpirationDate() <= new \DateTime()) { + $this->deleteShare($share); + throw new ShareNotFound(); + } return $share; } diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index 131bc7fbfd..d93a881a44 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -22,6 +22,7 @@ namespace Test\Share20; use OCP\Files\IRootFolder; use OCP\IUserManager; +use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IProviderFactory; use OCP\Share\IShare; use OC\Share20\Manager; @@ -1704,6 +1705,48 @@ class ManagerTest extends \Test\TestCase { $this->assertSame($share, $ret); } + /** + * @expectedException \OCP\Share\Exceptions\ShareNotFound + */ + public function testGetShareByTokenExpired() { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare']) + ->getMock(); + + $date = new \DateTime(); + $date->setTime(0,0,0); + $share = $this->manager->newShare(); + $share->setExpirationDate($date); + + $this->defaultProvider->expects($this->once()) + ->method('getShareByToken') + ->with('expiredToken') + ->willReturn($share); + + $manager->expects($this->once()) + ->method('deleteShare') + ->with($this->equalTo($share)); + + $manager->getShareByToken('expiredToken'); + } + + public function testGetShareByTokenNotExpired() { + $date = new \DateTime(); + $date->setTime(0,0,0); + $date->add(new \DateInterval('P2D')); + $share = $this->manager->newShare(); + $share->setExpirationDate($date); + + $this->defaultProvider->expects($this->once()) + ->method('getShareByToken') + ->with('expiredToken') + ->willReturn($share); + + $res = $this->manager->getShareByToken('expiredToken'); + + $this->assertSame($share, $res); + } + public function testCheckPasswordNoLinkShare() { $share = $this->getMock('\OCP\Share\IShare'); $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER); From 3028ec5440a3c4d448bbaf8a6b246391bea22317 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 5 Feb 2016 15:34:30 +0100 Subject: [PATCH 2/4] Delete expired share when fetched by id --- lib/private/share20/manager.php | 8 ++++++++ tests/lib/share20/managertest.php | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index a393cdeb89..c4fec31fb8 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -803,6 +803,14 @@ class Manager implements IManager { $share = $provider->getShareById($id, $recipient); + // Validate link shares expiration date + if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK && + $share->getExpirationDate() !== null && + $share->getExpirationDate() <= new \DateTime()) { + $this->deleteShare($share); + throw new ShareNotFound(); + } + return $share; } diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index d93a881a44..55c13f03f8 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -500,6 +500,33 @@ class ManagerTest extends \Test\TestCase { $this->assertEquals($share, $this->manager->getShareById('default:42')); } + /** + * @expectedException \OCP\Share\Exceptions\ShareNotFound + */ + public function testGetExpiredShareById() { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare']) + ->getMock(); + + $date = new \DateTime(); + $date->setTime(0,0,0); + + $share = $this->manager->newShare(); + $share->setExpirationDate($date) + ->setShareType(\OCP\Share::SHARE_TYPE_LINK); + + $this->defaultProvider->expects($this->once()) + ->method('getShareById') + ->with('42') + ->willReturn($share); + + $manager->expects($this->once()) + ->method('deleteShare') + ->with($share); + + $manager->getShareById('default:42'); + } + /** * @expectedException InvalidArgumentException * @expectedExceptionMessage Passwords are enforced for link shares From 8486d617645a5ef75da34df10ed663ef77bed257 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 5 Feb 2016 20:00:07 +0100 Subject: [PATCH 3/4] getSharesBy should also expire link shares --- lib/private/share20/manager.php | 48 ++++++++++++- lib/public/share/ishareprovider.php | 2 +- tests/lib/share20/managertest.php | 103 ++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index c4fec31fb8..7d0017bf6f 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -778,7 +778,53 @@ class Manager implements IManager { $provider = $this->factory->getProviderForType($shareType); - return $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + + /* + * Work around so we don't return expired shares but still follow + * proper pagination. + */ + if ($shareType === \OCP\Share::SHARE_TYPE_LINK) { + $shares2 = []; + $today = new \DateTime(); + + while(true) { + $added = 0; + foreach ($shares as $share) { + // Check if the share is expired and if so delete it + if ($share->getExpirationDate() !== null && + $share->getExpirationDate() <= $today + ) { + $this->deleteShare($share); + continue; + } + $added++; + $shares2[] = $share; + + if (count($shares2) === $limit) { + break; + } + } + + if (count($shares2) === $limit) { + break; + } + + $offset += $added; + + // Fetch again $limit shares + $shares = $provider->getSharesBy($userId, $shareType, $path, $reshares, $limit, $offset); + + // No more shares means we are done + if (empty($shares)) { + break; + } + } + + $shares = $shares2; + } + + return $shares; } /** diff --git a/lib/public/share/ishareprovider.php b/lib/public/share/ishareprovider.php index 25fa76369a..b339ce63d3 100644 --- a/lib/public/share/ishareprovider.php +++ b/lib/public/share/ishareprovider.php @@ -101,7 +101,7 @@ interface IShareProvider { * @param bool $reshares Also get the shares where $user is the owner instead of just the shares where $user is the initiator * @param int $limit The maximum number of shares to be returned, -1 for all shares * @param int $offset - * @return \OCP\Share\IShare Share[] + * @return \OCP\Share\IShare[] * @since 9.0.0 */ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offset); diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index 55c13f03f8..ea0fb1838d 100644 --- a/tests/lib/share20/managertest.php +++ b/tests/lib/share20/managertest.php @@ -1700,6 +1700,109 @@ class ManagerTest extends \Test\TestCase { $manager->createShare($share); } + public function testGetSharesBy() { + $share = $this->manager->newShare(); + + $node = $this->getMock('OCP\Files\Folder'); + + $this->defaultProvider->expects($this->once()) + ->method('getSharesBy') + ->with( + $this->equalTo('user'), + $this->equalTo(\OCP\Share::SHARE_TYPE_USER), + $this->equalTo($node), + $this->equalTo(true), + $this->equalTo(1), + $this->equalTo(1) + )->willReturn([$share]); + + $shares = $this->manager->getSharesBy('user', \OCP\Share::SHARE_TYPE_USER, $node, true, 1, 1); + + $this->assertCount(1, $shares); + $this->assertSame($share, $shares[0]); + } + + /** + * Test to ensure we correctly remove expired link shares + * + * We have 8 Shares and we want the 3 first valid shares. + * share 3-6 and 8 are expired. Thus at the end of this test we should + * have received share 1,2 and 7. And from the manager. Share 3-6 should be + * deleted (as they are evaluated). but share 8 should still be there. + */ + public function testGetSharesByExpiredLinkShares() { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare']) + ->getMock(); + + /** @var \OCP\Share\IShare[] $shares */ + $shares = []; + + /* + * This results in an array of 8 IShare elements + */ + for ($i = 0; $i < 8; $i++) { + $share = $this->manager->newShare(); + $share->setId($i); + $shares[] = $share; + } + + $today = new \DateTime(); + $today->setTime(0,0,0); + + /* + * Set the expiration date to today for some shares + */ + $shares[2]->setExpirationDate($today); + $shares[3]->setExpirationDate($today); + $shares[4]->setExpirationDate($today); + $shares[5]->setExpirationDate($today); + + /** @var \OCP\Share\IShare[] $i */ + $shares2 = []; + for ($i = 0; $i < 8; $i++) { + $shares2[] = clone $shares[$i]; + } + + $node = $this->getMock('OCP\Files\File'); + + /* + * Simulate the getSharesBy call. + */ + $this->defaultProvider + ->method('getSharesBy') + ->will($this->returnCallback(function($uid, $type, $node, $reshares, $limit, $offset) use (&$shares2) { + return array_slice($shares2, $offset, $limit); + })); + + /* + * Simulate the deleteShare call. + */ + $manager->method('deleteShare') + ->will($this->returnCallback(function($share) use (&$shares2) { + for($i = 0; $i < count($shares2); $i++) { + if ($shares2[$i]->getId() === $share->getId()) { + array_splice($shares2, $i, 1); + break; + } + } + })); + + $res = $manager->getSharesBy('user', \OCP\Share::SHARE_TYPE_LINK, $node, true, 3, 0); + + $this->assertCount(3, $res); + $this->assertEquals($shares[0]->getId(), $res[0]->getId()); + $this->assertEquals($shares[1]->getId(), $res[1]->getId()); + $this->assertEquals($shares[6]->getId(), $res[2]->getId()); + + $this->assertCount(4, $shares2); + $this->assertEquals(0, $shares2[0]->getId()); + $this->assertEquals(1, $shares2[1]->getId()); + $this->assertEquals(6, $shares2[2]->getId()); + $this->assertEquals(7, $shares2[3]->getId()); + $this->assertSame($today, $shares[3]->getExpirationDate()); + } + public function testGetShareByToken() { $factory = $this->getMock('\OCP\Share\IProviderFactory'); From 46faf6d3ca437c8795af79f1851b4bd6b4a92280 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sat, 6 Feb 2016 13:31:31 +0100 Subject: [PATCH 4/4] Fix exception on delete --- lib/private/share20/manager.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index 7d0017bf6f..33085410e1 100644 --- a/lib/private/share20/manager.php +++ b/lib/private/share20/manager.php @@ -22,6 +22,7 @@ namespace OC\Share20; use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; use OCP\IUserManager; use OCP\Share\IManager; use OCP\Share\IProviderFactory; @@ -795,7 +796,11 @@ class Manager implements IManager { if ($share->getExpirationDate() !== null && $share->getExpirationDate() <= $today ) { - $this->deleteShare($share); + try { + $this->deleteShare($share); + } catch (NotFoundException $e) { + //Ignore since this basically means the share is deleted + } continue; } $added++;