diff --git a/apps/files_sharing/lib/migration.php b/apps/files_sharing/lib/migration.php index e734638551..31a76687d4 100644 --- a/apps/files_sharing/lib/migration.php +++ b/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(); diff --git a/apps/files_sharing/tests/api.php b/apps/files_sharing/tests/api.php index 49a08d3d0c..3a2ae1eef3 100644 --- a/apps/files_sharing/tests/api.php +++ b/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()); + } } diff --git a/apps/files_sharing/tests/migrationtest.php b/apps/files_sharing/tests/migrationtest.php index 8a40b76a64..1bca4a419f 100644 --- a/apps/files_sharing/tests/migrationtest.php +++ b/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() { diff --git a/lib/private/share20/defaultshareprovider.php b/lib/private/share20/defaultshareprovider.php index 0ab0dc81fa..e18e306d7f 100644 --- a/lib/private/share20/defaultshareprovider.php +++ b/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!'); } diff --git a/lib/private/share20/manager.php b/lib/private/share20/manager.php index 4345784d2e..12893c2696 100644 --- a/lib/private/share20/manager.php +++ b/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. diff --git a/tests/lib/share20/managertest.php b/tests/lib/share20/managertest.php index 73a1b0a653..1230c191b6 100644 --- a/tests/lib/share20/managertest.php +++ b/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')