Merge pull request #26050 from owncloud/stable9.1-prefilter-inaccessible-shares

[stable9.1] Prefilter inaccessible shares in DefaultShareProvider::getSharedWith()

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
@@ -582,6 +582,25 @@ class DefaultShareProvider implements IShareProvider {
 	}
 
 	/**
+	 * 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
 	 */
 	public function getSharedWith($userId, $shareType, $node, $limit, $offset) {
@@ -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')