Merge pull request #22317 from owncloud/fix_invisible_linkshares

Do not allow invisible link shares

apps/files_sharing/lib/migration.php

@@ -55,32 +55,30 @@ class Migration {
 	 */
 	public function removeReShares() {
 
-		while(true) {
-			$reShares = $this->getReShares(1000);
+		$stmt = $this->getReShares();
 
-			if (empty($reShares)) {
-				break;
-			}
+		$owners = [];
+		while($share = $stmt->fetch()) {
 
-			// Update the cache
-			foreach($reShares as $reShare) {
-				$this->shareCache[$reShare['id']] = $reShare;
-			}
+			$this->shareCache[$share['id']] = $share;
 
-			$owners = [];
-			foreach ($reShares as $share) {
-				$owners[$share['id']] = [
+			$owners[$share['id']] = [
 					'owner' => $this->findOwner($share),
-					'initiator' => $share['uid_owner']
-				];
-			}
-			$this->updateOwners($owners);
+					'initiator' => $share['uid_owner'],
+					'type' => $share['share_type'],
+			];
 
-			//Clear the cache of the shares we just updated so we have more room
-			foreach($owners as $id => $owner) {
-				unset($this->shareCache[$id]);
+			if (count($owners) === 1000) {
+				$this->updateOwners($owners);
+				$owners = [];
 			}
 		}
+
+		$stmt->closeCursor();
+
+		if (count($owners)) {
+			$this->updateOwners($owners);
+		}
 	}
 
 	/**
@@ -99,7 +97,8 @@ class Migration {
 			foreach ($shares as $share) {
 				$owners[$share['id']] = [
 					'owner' => $share['uid_owner'],
-					'initiator' => $share['uid_owner']
+					'initiator' => $share['uid_owner'],
+					'type' => $share['share_type'],
 				];
 			}
 			$this->updateOwners($owners);
@@ -130,11 +129,11 @@ class Migration {
 	 * Get $n re-shares from the database
 	 *
 	 * @param int $n The max number of shares to fetch
-	 * @return array
+	 * @return \Doctrine\DBAL\Driver\Statement
 	 */
-	private function getReShares($n = 1000) {
+	private function getReShares() {
 		$query = $this->connection->getQueryBuilder();
-		$query->select(['id', 'parent', 'uid_owner'])
+		$query->select(['id', 'parent', 'uid_owner', 'share_type'])
 			->from($this->table)
 			->where($query->expr()->in(
 				'share_type',
@@ -156,9 +155,10 @@ class Migration {
 				)
 			))
 			->andWhere($query->expr()->isNotNull('parent'))
-			->orderBy('id', 'asc')
-			->setMaxResults($n);
-		$result = $query->execute();
+			->orderBy('id', 'asc');
+		return $query->execute();
+
+
 		$shares = $result->fetchAll();
 		$result->closeCursor();
 
@@ -178,7 +178,7 @@ class Migration {
 	 */
 	private function getMissingInitiator($n = 1000) {
 		$query = $this->connection->getQueryBuilder();
-		$query->select(['id', 'uid_owner'])
+		$query->select(['id', 'uid_owner', 'share_type'])
 			->from($this->table)
 			->where($query->expr()->in(
 				'share_type',
@@ -247,11 +247,17 @@ class Migration {
 			foreach ($owners as $id => $owner) {
 				$query = $this->connection->getQueryBuilder();
 				$query->update($this->table)
-					->set('parent', $query->createNamedParameter(null))
 					->set('uid_owner', $query->createNamedParameter($owner['owner']))
-					->set('uid_initiator', $query->createNamedParameter($owner['initiator']))
-					->where($query->expr()->eq('id', $query->createNamedParameter($id)))
-					->execute();
+					->set('uid_initiator', $query->createNamedParameter($owner['initiator']));
+
+
+				if ((int)$owner['type'] !== \OCP\Share::SHARE_TYPE_LINK) {
+					$query->set('parent', $query->createNamedParameter(null));
+				}
+
+				$query->where($query->expr()->eq('id', $query->createNamedParameter($id)));
+
+				$query->execute();
 			}
 
 			$this->connection->commit();

apps/files_sharing/tests/api.php

@@ -40,6 +40,9 @@ class Test_Files_Sharing_Api extends TestCase {
 
 	private static $tempStorage;
 
+	/** @var \OCP\Share\IManager */
+	private $shareManager;
+
 	protected function setUp() {
 		parent::setUp();
 
@@ -59,6 +62,8 @@ class Test_Files_Sharing_Api extends TestCase {
 		$this->view->mkdir($this->folder . $this->subfolder . $this->subsubfolder);
 		$this->view->file_put_contents($this->folder.$this->filename, $this->data);
 		$this->view->file_put_contents($this->folder . $this->subfolder . $this->filename, $this->data);
+
+		$this->shareManager = \OC::$server->getShareManager();
 	}
 
 	protected function tearDown() {
@@ -72,6 +77,40 @@ class Test_Files_Sharing_Api extends TestCase {
 		parent::tearDown();
 	}
 
+	/**
+	 * @param array $data
+	 * @return \OCP\IRequest
+	 */
+	private function createRequest(array $data) {
+		$request = $this->getMock('\OCP\IRequest');
+		$request->method('getParam')
+			->will($this->returnCallback(function($param, $default = null) use ($data) {
+				if (isset($data[$param])) {
+					return $data[$param];
+				}
+				return $default;
+			}));
+		return $request;
+	}
+
+	/**
+	 * @param \OCP\IRequest $request
+	 * @param string $userId The userId of the caller
+	 * @return \OCA\Files_Sharing\API\Share20OCS
+	 */
+	private function createOCS($request, $userId) {
+		$currentUser = \OC::$server->getUserManager()->get($userId);
+		return new \OCA\Files_Sharing\API\Share20OCS(
+			$this->shareManager,
+			\OC::$server->getGroupManager(),
+			\OC::$server->getUserManager(),
+			$request,
+			\OC::$server->getRootFolder(),
+			\OC::$server->getURLGenerator(),
+			$currentUser
+		);
+	}
+
 	/**
 	 * @medium
 	 */
@@ -1725,4 +1764,85 @@ class Test_Files_Sharing_Api extends TestCase {
 		$config->setAppValue('core', 'shareapi_enforce_expire_date', 'no');
 	}
 
+	/**
+	 * test for no invisible shares
+	 * See: https://github.com/owncloud/core/issues/22295
+	 */
+	public function testInvisibleSharesUser() {
+		// simulate a post request
+		$request = $this->createRequest([
+			'path' => $this->folder,
+			'shareWith' => self::TEST_FILES_SHARING_API_USER2,
+			'shareType' => \OCP\Share::SHARE_TYPE_USER
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->createShare();
+		$this->assertTrue($result->succeeded());
+		$data = $result->getData();
+
+		$topId = $data['id'];
+
+		$request = $this->createRequest([
+			'path' => $this->folder . $this->subfolder,
+			'shareType' => \OCP\Share::SHARE_TYPE_LINK,
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER2);
+		$result = $ocs->createShare();
+		$this->assertTrue($result->succeeded());
+
+		$request = $this->createRequest([]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->deleteShare($topId);
+		$this->assertTrue($result->succeeded());
+
+		$request = $this->createRequest([
+			'reshares' => 'true',
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->getShares();
+		$this->assertTrue($result->succeeded());
+
+		$this->assertEmpty($result->getData());
+	}
+
+	/**
+	 * test for no invisible shares
+	 * See: https://github.com/owncloud/core/issues/22295
+	 */
+	public function testInvisibleSharesGroup() {
+		// simulate a post request
+		$request = $this->createRequest([
+			'path' => $this->folder,
+			'shareWith' => self::TEST_FILES_SHARING_API_GROUP1,
+			'shareType' => \OCP\Share::SHARE_TYPE_GROUP
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->createShare();
+		$this->assertTrue($result->succeeded());
+		$data = $result->getData();
+
+		$topId = $data['id'];
+
+		$request = $this->createRequest([
+			'path' => $this->folder . $this->subfolder,
+			'shareType' => \OCP\Share::SHARE_TYPE_LINK,
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER2);
+		$result = $ocs->createShare();
+		$this->assertTrue($result->succeeded());
+
+		$request = $this->createRequest([]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->deleteShare($topId);
+		$this->assertTrue($result->succeeded());
+
+		$request = $this->createRequest([
+			'reshares' => 'true',
+		]);
+		$ocs = $this->createOCS($request, self::TEST_FILES_SHARING_API_USER1);
+		$result = $ocs->getShares();
+		$this->assertTrue($result->succeeded());
+
+		$this->assertEmpty($result->getData());
+	}
 }

apps/files_sharing/tests/migrationtest.php

@@ -226,6 +226,23 @@ class MigrationTest extends TestCase {
 		$this->assertSame(1,
 			$query->execute()
 		);
+
+		// Link reshare should keep its parent
+		$query->setParameter('share_type', \OCP\Share::SHARE_TYPE_LINK)
+			->setParameter('share_with', null)
+			->setParameter('uid_owner', 'user3')
+			->setParameter('uid_initiator', '')
+			->setParameter('parent', $parent)
+			->setParameter('item_type', 'file')
+			->setParameter('item_source', '2')
+			->setParameter('item_target', '/2')
+			->setParameter('file_source', 2)
+			->setParameter('file_target', '/foobar')
+			->setParameter('permissions', 31)
+			->setParameter('stime', time());
+		$this->assertSame(1,
+			$query->execute()
+		);
 	}
 
 	public function testRemoveReShares() {
@@ -238,7 +255,7 @@ class MigrationTest extends TestCase {
 		$query = $this->connection->getQueryBuilder();
 		$query->select('*')->from($this->table)->orderBy('id');
 		$result = $query->execute()->fetchAll();
-		$this->assertSame(9, count($result));
+		$this->assertSame(10, count($result));
 
 		// shares which shouldn't be modified
 		for ($i = 0; $i < 4; $i++) {
@@ -261,6 +278,14 @@ class MigrationTest extends TestCase {
 			$this->assertSame($user, $result[$i]['uid_initiator']);
 			$this->assertNull($result[$i]['parent']);
 		}
+
+		/*
+		 * The link share is flattend but has an owner to avoid invisible shares
+		 * see: https://github.com/owncloud/core/pull/22317
+		 */
+		$this->assertSame('owner2', $result[9]['uid_owner']);
+		$this->assertSame('user3', $result[9]['uid_initiator']);
+		$this->assertSame($result[7]['id'], $result[9]['parent']);
 	}
 
 	public function test1001DeepReshares() {

lib/private/share20/defaultshareprovider.php

@@ -118,6 +118,10 @@ class DefaultShareProvider implements IShareProvider {
 			if ($share->getExpirationDate() !== null) {
 				$qb->setValue('expiration', $qb->createNamedParameter($share->getExpirationDate(), 'datetime'));
 			}
+
+			if (method_exists($share, 'getParent')) {
+				$qb->setValue('parent', $qb->createNamedParameter($share->getParent()));
+			}
 		} else {
 			throw new \Exception('invalid share type!');
 		}

lib/private/share20/manager.php

@@ -414,6 +414,28 @@ class Manager implements IManager {
 		}
 	}
 
+	/**
+	 * To make sure we don't get invisible link shares we set the parent
+	 * of a link if it is a reshare. This is a quick word around
+	 * until we can properly display multiple link shares in the UI
+	 *
+	 * See: https://github.com/owncloud/core/issues/22295
+	 *
+	 * FIXME: Remove once multiple link shares can be properly displayed
+	 *
+	 * @param \OCP\Share\IShare $share
+	 */
+	protected function setLinkParent(\OCP\Share\IShare $share) {
+
+		// No sense in checking if the method is not there.
+		if (method_exists($share, 'setParent')) {
+			$storage = $share->getNode()->getStorage();
+			if ($storage->instanceOfStorage('\OCA\Files_Sharing\ISharedStorage')) {
+				$share->setParent($storage->getShareId());
+			}
+		};
+	}
+
 	/**
 	 * @param File|Folder $path
 	 */
@@ -470,6 +492,7 @@ class Manager implements IManager {
 			$this->groupCreateChecks($share);
 		} else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) {
 			$this->linkCreateChecks($share);
+			$this->setLinkParent($share);
 
 			/*
 			 * For now ignore a set token.

tests/lib/share20/managertest.php

@@ -1549,6 +1549,7 @@ class ManagerTest extends \Test\TestCase {
 				'pathCreateChecks',
 				'validateExpirationDate',
 				'verifyPassword',
+				'setLinkParent',
 			])
 			->getMock();
 
@@ -1589,6 +1590,9 @@ class ManagerTest extends \Test\TestCase {
 		$manager->expects($this->once())
 			->method('verifyPassword')
 			->with('password');
+		$manager->expects($this->once())
+			->method('setLinkParent')
+			->with($share);
 
 		$this->hasher->expects($this->once())
 			->method('hash')