From 39182a49abc7624153417410d40e1aba57f6e726 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 2 Sep 2016 11:23:55 +0200 Subject: Prefilter inaccessible shares in DefaultShareProvider::getSharedWith() The DefaultShareProvider now does a DB-level check to find out whether file_source is accessible at all (deleted file) or whether it's in the trashbin of a home storage. One small corner case where the home storage id is in md5 form cannot be covered properly with this approach. --- lib/private/Share20/DefaultShareProvider.php | 46 +++++-- tests/lib/Share20/DefaultShareProviderTest.php | 180 +++++++++++++++++++++---- 2 files changed, 195 insertions(+), 31 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index aa8f1729a9d..a3d7a5bb83a 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -581,6 +581,25 @@ class DefaultShareProvider implements IShareProvider { return $shares; } + /** + * Returns whether the given database result can be interpreted as + * a share with accessible file (not trashed, not deleted) + */ + private function isAccessibleResult($data) { + // exclude shares leading to deleted file entries + if ($data['fileid'] === null) { + return false; + } + + // exclude shares leading to trashbin on home storages + $pathSections = explode('/', $data['path'], 2); + // FIXME: would not detect rare md5'd home storage case properly + if ($pathSections[0] !== 'files' && explode(':', $data['storage_string_id'], 2)[0] === 'home') { + return false; + } + return true; + } + /** * @inheritdoc */ @@ -591,11 +610,14 @@ class DefaultShareProvider implements IShareProvider { if ($shareType === \OCP\Share::SHARE_TYPE_USER) { //Get shares directly with this user $qb = $this->dbConn->getQueryBuilder(); - $qb->select('*') - ->from('share'); + $qb->select('s.*', 'f.fileid', 'f.path') + ->selectAlias('st.id', 'storage_string_id') + ->from('share', 's') + ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) + ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')); // Order by id - $qb->orderBy('id'); + $qb->orderBy('s.id'); // Set limit and offset if ($limit !== -1) { @@ -618,7 +640,9 @@ class DefaultShareProvider implements IShareProvider { $cursor = $qb->execute(); while($data = $cursor->fetch()) { - $shares[] = $this->createShare($data); + if ($this->isAccessibleResult($data)) { + $shares[] = $this->createShare($data); + } } $cursor->closeCursor(); @@ -639,9 +663,12 @@ class DefaultShareProvider implements IShareProvider { } $qb = $this->dbConn->getQueryBuilder(); - $qb->select('*') - ->from('share') - ->orderBy('id') + $qb->select('s.*', 'f.fileid', 'f.path') + ->selectAlias('st.id', 'storage_string_id') + ->from('share', 's') + ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) + ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) + ->orderBy('s.id') ->setFirstResult(0); if ($limit !== -1) { @@ -671,7 +698,10 @@ class DefaultShareProvider implements IShareProvider { $offset--; continue; } - $shares2[] = $this->createShare($data); + + if ($this->isAccessibleResult($data)) { + $shares2[] = $this->createShare($data); + } } $cursor->closeCursor(); } diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 6ef00721a70..3d3c80fb780 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -72,6 +72,8 @@ class DefaultShareProviderTest extends \Test\TestCase { public function tearDown() { $this->dbConn->getQueryBuilder()->delete('share')->execute(); + $this->dbConn->getQueryBuilder()->delete('filecache')->execute(); + $this->dbConn->getQueryBuilder()->delete('storages')->execute(); } /** @@ -776,7 +778,47 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->provider->getShareByToken('invalidtoken'); } - public function testGetSharedWithUser() { + private function createTestStorageEntry($storageStringId) { + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('storages') + ->values([ + 'id' => $qb->expr()->literal($storageStringId), + ]); + $this->assertEquals(1, $qb->execute()); + return $qb->getLastInsertId(); + } + + private function createTestFileEntry($path, $storage = 1) { + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('filecache') + ->values([ + 'storage' => $qb->expr()->literal($storage), + 'path' => $qb->expr()->literal($path), + 'path_hash' => $qb->expr()->literal(md5($path)), + 'name' => $qb->expr()->literal(basename($path)), + ]); + $this->assertEquals(1, $qb->execute()); + return $qb->getLastInsertId(); + } + + public function storageAndFileNameProvider() { + return [ + // regular file on regular storage + ['home::shareOwner', 'files/test.txt', 'files/test2.txt'], + // regular file on external storage + ['smb::whatever', 'files/test.txt', 'files/test2.txt'], + // regular file on external storage in trashbin-like folder, + ['smb::whatever', 'files_trashbin/files/test.txt', 'files_trashbin/files/test2.txt'], + ]; + } + + /** + * @dataProvider storageAndFileNameProvider + */ + public function testGetSharedWithUser($storageStringId, $fileName1, $fileName2) { + $storageId = $this->createTestStorageEntry($storageStringId); + $fileId = $this->createTestFileEntry($fileName1, $storageId); + $fileId2 = $this->createTestFileEntry($fileName2, $storageId); $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ @@ -785,7 +827,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(42), + 'file_source' => $qb->expr()->literal($fileId), 'file_target' => $qb->expr()->literal('myTarget'), 'permissions' => $qb->expr()->literal(13), ]); @@ -800,7 +842,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner2'), 'uid_initiator' => $qb->expr()->literal('sharedBy2'), 'item_type' => $qb->expr()->literal('file2'), - 'file_source' => $qb->expr()->literal(43), + 'file_source' => $qb->expr()->literal($fileId2), 'file_target' => $qb->expr()->literal('myTarget2'), 'permissions' => $qb->expr()->literal(14), ]); @@ -808,7 +850,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $file = $this->getMock('\OCP\Files\File'); $this->rootFolder->method('getUserFolder')->with('shareOwner')->will($this->returnSelf()); - $this->rootFolder->method('getById')->with(42)->willReturn([$file]); + $this->rootFolder->method('getById')->with($fileId)->willReturn([$file]); $share = $this->provider->getSharedWith('sharedWith', \OCP\Share::SHARE_TYPE_USER, null, 1 , 0); $this->assertCount(1, $share); @@ -821,7 +863,13 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals(\OCP\Share::SHARE_TYPE_USER, $share->getShareType()); } - public function testGetSharedWithGroup() { + /** + * @dataProvider storageAndFileNameProvider + */ + public function testGetSharedWithGroup($storageStringId, $fileName1, $fileName2) { + $storageId = $this->createTestStorageEntry($storageStringId); + $fileId = $this->createTestFileEntry($fileName1, $storageId); + $fileId2 = $this->createTestFileEntry($fileName2, $storageId); $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ @@ -830,7 +878,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner2'), 'uid_initiator' => $qb->expr()->literal('sharedBy2'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(43), + 'file_source' => $qb->expr()->literal($fileId2), 'file_target' => $qb->expr()->literal('myTarget2'), 'permissions' => $qb->expr()->literal(14), ]); @@ -844,7 +892,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(42), + 'file_source' => $qb->expr()->literal($fileId), 'file_target' => $qb->expr()->literal('myTarget'), 'permissions' => $qb->expr()->literal(13), ]); @@ -879,7 +927,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $file = $this->getMock('\OCP\Files\File'); $this->rootFolder->method('getUserFolder')->with('shareOwner')->will($this->returnSelf()); - $this->rootFolder->method('getById')->with(42)->willReturn([$file]); + $this->rootFolder->method('getById')->with($fileId)->willReturn([$file]); $share = $this->provider->getSharedWith('sharedWith', \OCP\Share::SHARE_TYPE_GROUP, null, 20 , 1); $this->assertCount(1, $share); @@ -892,7 +940,12 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals(\OCP\Share::SHARE_TYPE_GROUP, $share->getShareType()); } - public function testGetSharedWithGroupUserModified() { + /** + * @dataProvider storageAndFileNameProvider + */ + public function testGetSharedWithGroupUserModified($storageStringId, $fileName1, $fileName2) { + $storageId = $this->createTestStorageEntry($storageStringId); + $fileId = $this->createTestFileEntry($fileName1, $storageId); $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') ->values([ @@ -901,7 +954,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(42), + 'file_source' => $qb->expr()->literal($fileId), 'file_target' => $qb->expr()->literal('myTarget'), 'permissions' => $qb->expr()->literal(13), ]); @@ -919,7 +972,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(42), + 'file_source' => $qb->expr()->literal($fileId), 'file_target' => $qb->expr()->literal('wrongTarget'), 'permissions' => $qb->expr()->literal(31), 'parent' => $qb->expr()->literal($id), @@ -937,7 +990,7 @@ class DefaultShareProviderTest extends \Test\TestCase { 'uid_owner' => $qb->expr()->literal('shareOwner'), 'uid_initiator' => $qb->expr()->literal('sharedBy'), 'item_type' => $qb->expr()->literal('file'), - 'file_source' => $qb->expr()->literal(42), + 'file_source' => $qb->expr()->literal($fileId), 'file_target' => $qb->expr()->literal('userTarget'), 'permissions' => $qb->expr()->literal(0), 'parent' => $qb->expr()->literal($id), @@ -965,7 +1018,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $file = $this->getMock('\OCP\Files\File'); $this->rootFolder->method('getUserFolder')->with('shareOwner')->will($this->returnSelf()); - $this->rootFolder->method('getById')->with(42)->willReturn([$file]); + $this->rootFolder->method('getById')->with($fileId)->willReturn([$file]); $share = $this->provider->getSharedWith('user', \OCP\Share::SHARE_TYPE_GROUP, null, -1, 0); $this->assertCount(1, $share); @@ -980,11 +1033,17 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertSame('userTarget', $share->getTarget()); } - public function testGetSharedWithUserWithNode() { + /** + * @dataProvider storageAndFileNameProvider + */ + public function testGetSharedWithUserWithNode($storageStringId, $fileName1, $fileName2) { + $storageId = $this->createTestStorageEntry($storageStringId); + $fileId = $this->createTestFileEntry($fileName1, $storageId); + $fileId2 = $this->createTestFileEntry($fileName2, $storageId); $this->addShareToDB(\OCP\Share::SHARE_TYPE_USER, 'user0', 'user1', 'user1', - 'file', 42, 'myTarget', 31, null, null, null); + 'file', $fileId, 'myTarget', 31, null, null, null); $id = $this->addShareToDB(\OCP\Share::SHARE_TYPE_USER, 'user0', 'user1', 'user1', - 'file', 43, 'myTarget', 31, null, null, null); + 'file', $fileId2, 'myTarget', 31, null, null, null); $user0 = $this->getMock('\OCP\IUser'); $user0->method('getUID')->willReturn('user0'); @@ -997,9 +1056,9 @@ class DefaultShareProviderTest extends \Test\TestCase { ]); $file = $this->getMock('\OCP\Files\File'); - $file->method('getId')->willReturn(43); + $file->method('getId')->willReturn($fileId2); $this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf()); - $this->rootFolder->method('getById')->with(43)->willReturn([$file]); + $this->rootFolder->method('getById')->with($fileId2)->willReturn([$file]); $share = $this->provider->getSharedWith('user0', \OCP\Share::SHARE_TYPE_USER, $file, -1, 0); $this->assertCount(1, $share); @@ -1013,11 +1072,17 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals(\OCP\Share::SHARE_TYPE_USER, $share->getShareType()); } - public function testGetSharedWithGroupWithNode() { + /** + * @dataProvider storageAndFileNameProvider + */ + public function testGetSharedWithGroupWithNode($storageStringId, $fileName1, $fileName2) { + $storageId = $this->createTestStorageEntry($storageStringId); + $fileId = $this->createTestFileEntry($fileName1, $storageId); + $fileId2 = $this->createTestFileEntry($fileName2, $storageId); $this->addShareToDB(\OCP\Share::SHARE_TYPE_GROUP, 'group0', 'user1', 'user1', - 'file', 42, 'myTarget', 31, null, null, null); + 'file', $fileId, 'myTarget', 31, null, null, null); $id = $this->addShareToDB(\OCP\Share::SHARE_TYPE_GROUP, 'group0', 'user1', 'user1', - 'file', 43, 'myTarget', 31, null, null, null); + 'file', $fileId2, 'myTarget', 31, null, null, null); $user0 = $this->getMock('\OCP\IUser'); $user0->method('getUID')->willReturn('user0'); @@ -1036,9 +1101,9 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->groupManager->method('getUserGroups')->with($user0)->willReturn([$group0]); $node = $this->getMock('\OCP\Files\Folder'); - $node->method('getId')->willReturn(43); + $node->method('getId')->willReturn($fileId2); $this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf()); - $this->rootFolder->method('getById')->with(43)->willReturn([$node]); + $this->rootFolder->method('getById')->with($fileId2)->willReturn([$node]); $share = $this->provider->getSharedWith('user0', \OCP\Share::SHARE_TYPE_GROUP, $node, -1, 0); $this->assertCount(1, $share); @@ -1052,6 +1117,75 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals(\OCP\Share::SHARE_TYPE_GROUP, $share->getShareType()); } + public function shareTypesProvider() { + return [ + [\OCP\Share::SHARE_TYPE_USER, false], + [\OCP\Share::SHARE_TYPE_GROUP, false], + [\OCP\Share::SHARE_TYPE_USER, true], + [\OCP\Share::SHARE_TYPE_GROUP, true], + ]; + } + + /** + * @dataProvider shareTypesProvider + */ + public function testGetSharedWithWithDeletedFile($shareType, $trashed) { + if ($trashed) { + // exists in database but is in trash + $storageId = $this->createTestStorageEntry('home::shareOwner'); + $deletedFileId = $this->createTestFileEntry('files_trashbin/files/test.txt.d1465553223', $storageId); + } else { + // fileid that doesn't exist in the database + $deletedFileId = 123; + } + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal($shareType), + 'share_with' => $qb->expr()->literal('sharedWith'), + 'uid_owner' => $qb->expr()->literal('shareOwner'), + 'uid_initiator' => $qb->expr()->literal('sharedBy'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal($deletedFileId), + 'file_target' => $qb->expr()->literal('myTarget'), + 'permissions' => $qb->expr()->literal(13), + ]); + $this->assertEquals(1, $qb->execute()); + + $file = $this->getMock('\OCP\Files\File'); + $this->rootFolder->method('getUserFolder')->with('shareOwner')->will($this->returnSelf()); + $this->rootFolder->method('getById')->with($deletedFileId)->willReturn([$file]); + + $groups = []; + foreach(range(0, 100) as $i) { + $group = $this->getMock('\OCP\IGroup'); + $group->method('getGID')->willReturn('group'.$i); + $groups[] = $group; + } + + $group = $this->getMock('\OCP\IGroup'); + $group->method('getGID')->willReturn('sharedWith'); + $groups[] = $group; + + $user = $this->getMock('\OCP\IUser'); + $user->method('getUID')->willReturn('sharedWith'); + $owner = $this->getMock('\OCP\IUser'); + $owner->method('getUID')->willReturn('shareOwner'); + $initiator = $this->getMock('\OCP\IUser'); + $initiator->method('getUID')->willReturn('sharedBy'); + + $this->userManager->method('get')->willReturnMap([ + ['sharedWith', $user], + ['shareOwner', $owner], + ['sharedBy', $initiator], + ]); + $this->groupManager->method('getUserGroups')->with($user)->willReturn($groups); + $this->groupManager->method('get')->with('sharedWith')->willReturn($group); + + $share = $this->provider->getSharedWith('sharedWith', $shareType, null, 1 , 0); + $this->assertCount(0, $share); + } + public function testGetSharesBy() { $qb = $this->dbConn->getQueryBuilder(); $qb->insert('share') -- cgit v1.2.3