Merge pull request #1514 from nextcloud/integration-tests-orphaned-shares

Integration tests orphaned shares + Prefilter inaccessible shares

build/integration/features/bootstrap/BasicStructure.php

@@ -179,6 +179,27 @@ trait BasicStructure {
 		}
 	}
 
+	public function sendingToWithDirectUrl($verb, $url, $body) {
+		$fullUrl = substr($this->baseUrl, 0, -5) . $url;
+		$client = new Client();
+		$options = [];
+		if ($this->currentUser === 'admin') {
+			$options['auth'] = $this->adminUser;
+		} else {
+			$options['auth'] = [$this->currentUser, $this->regularUser];
+		}
+		if ($body instanceof \Behat\Gherkin\Node\TableNode) {
+			$fd = $body->getRowsHash();
+			$options['body'] = $fd;
+		}
+
+		try {
+			$this->response = $client->send($client->createRequest($verb, $fullUrl, $options));
+		} catch (\GuzzleHttp\Exception\ClientException $ex) {
+			$this->response = $ex->getResponse();
+		}
+	}
+
 	public function isExpectedUrl($possibleUrl, $finalPart){
 		$baseUrlChopped = substr($this->baseUrl, 0, -4);
 		$endCharacter = strlen($baseUrlChopped) + strlen($finalPart);
@@ -320,6 +341,16 @@ trait BasicStructure {
 		fclose($file);
 	}
 
+	/**
+	 * @When User :user empties trashbin
+	 * @param string $user
+	 */
+	public function emptyTrashbin($user) {
+		$body = new \Behat\Gherkin\Node\TableNode([['allfiles', 'true'], ['dir', '%2F']]);
+		$this->sendingToWithDirectUrl('POST', "/index.php/apps/files_trashbin/ajax/delete.php", $body);
+		$this->theHTTPStatusCodeShouldBe('200');
+	}
+
 	/**
 	 * @BeforeSuite
 	 */

build/integration/features/bootstrap/WebDav.php

@@ -455,11 +455,12 @@ trait WebDav {
 	}
 
 	/**
-	 * @When User :user deletes file :file
+	 * @When /^User "([^"]*)" deletes (file|folder) "([^"]*)"$/
 	 * @param string $user
+	 * @param string $type
 	 * @param string $file
 	 */
-	public function userDeletesFile($user, $file)  {
+	public function userDeletesFile($user, $type, $file)  {
 		try {
 			$this->response = $this->makeDavRequest($user, 'DELETE', $file, []);
 		} catch (\GuzzleHttp\Exception\ServerException $e) {

build/integration/features/sharing-v1.feature

@@ -920,3 +920,21 @@ Feature: sharing
 #        |{http://owncloud.org/ns}permissions|
 #    And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK"
 #    And as "user1" the folder "merge-test-outside-groups-renamebeforesecondshare" does not exist
+
+  Scenario: Empting trashbin
+    Given As an "admin"
+    And user "user0" exists
+    And User "user0" deletes file "/textfile0.txt"
+    When User "user0" empties trashbin
+    Then the HTTP status code should be "200"
+
+  Scenario: orphaned shares
+    Given As an "admin"
+    And user "user0" exists
+    And user "user1" exists
+    And user "user0" created a folder "/common"
+    And user "user0" created a folder "/common/sub"
+    And file "/common/sub" of user "user0" is shared with user "user1"
+    And User "user0" deletes folder "/common"
+    When User "user0" empties trashbin
+    Then as "user1" the folder "sub" does not exist

lib/private/Share20/DefaultShareProvider.php

@@ -582,6 +582,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
 	 */
@@ -592,11 +611,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) {
@@ -619,7 +641,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();
 
@@ -640,9 +664,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) {
@@ -672,7 +699,10 @@ class DefaultShareProvider implements IShareProvider {
 						$offset--;
 						continue;
 					}
-					$shares2[] = $this->createShare($data);
+
+					if ($this->isAccessibleResult($data)) {
+						$shares2[] = $this->createShare($data);
+					}
 				}
 				$cursor->closeCursor();
 			}

tests/lib/Share20/DefaultShareProviderTest.php

@@ -77,6 +77,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();
 	}
 
 	/**
@@ -781,7 +783,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([
@@ -790,7 +832,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),
 			]);
@@ -805,7 +847,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),
 			]);
@@ -813,7 +855,7 @@ class DefaultShareProviderTest extends \Test\TestCase {
 
 		$file = $this->createMock(File::class);
 		$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);
@@ -826,7 +868,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([
@@ -835,7 +883,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),
 			]);
@@ -849,7 +897,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),
 			]);
@@ -884,7 +932,7 @@ class DefaultShareProviderTest extends \Test\TestCase {
 
 		$file = $this->createMock(File::class);
 		$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);
@@ -897,7 +945,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([
@@ -906,7 +959,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),
 			]);
@@ -924,7 +977,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),
@@ -942,7 +995,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),
@@ -970,7 +1023,7 @@ class DefaultShareProviderTest extends \Test\TestCase {
 
 		$file = $this->createMock(File::class);
 		$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);
@@ -985,11 +1038,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->createMock(IUser::class);
 		$user0->method('getUID')->willReturn('user0');
@@ -1002,9 +1061,10 @@ class DefaultShareProviderTest extends \Test\TestCase {
 		]);
 
 		$file = $this->createMock(File::class);
-		$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);
@@ -1018,11 +1078,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->createMock(IUser::class);
 		$user0->method('getUID')->willReturn('user0');
@@ -1041,9 +1107,9 @@ class DefaultShareProviderTest extends \Test\TestCase {
 		$this->groupManager->method('getUserGroups')->with($user0)->willReturn([$group0]);
 
 		$node = $this->createMock(Folder::class);
-		$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);
@@ -1057,6 +1123,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')