From d84df155900bfdb58a8826802cde5a096065a078 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 9 Sep 2016 12:53:17 +0200 Subject: [PATCH 01/29] Add getAccessList to ShareManager Signed-off-by: Roeland Jago Douma --- lib/private/Share20/Manager.php | 67 +++++++++++++++++++++++-- tests/lib/Share20/ManagerTest.php | 83 ++++++++++++++++++++++++++++++- 2 files changed, 144 insertions(+), 6 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 292b07d28d..127eef423e 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -48,6 +48,7 @@ use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IProviderFactory; +use OCP\Share\IShare; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\EventDispatcher\GenericEvent; use OCP\Share\IShareProvider; @@ -1176,7 +1177,7 @@ class Manager implements IManager { /** * Get access list to a path. This means - * all the users and groups that can access a given path. + * all the users that can access a given path. * * Consider: * -root @@ -1185,20 +1186,76 @@ class Manager implements IManager { * |-fileA * * fileA is shared with user1 - * folder2 is shared with group2 + * folder2 is shared with group2 (user4 is a member of group2) * folder1 is shared with user2 * * Then the access list will to '/folder1/folder2/fileA' is: * [ - * 'users' => ['user1', 'user2'], - * 'groups' => ['group2'] + * users => ['user1', 'user2', 'user4'], + * public => bool + * remote => bool * ] * - * This is required for encryption + * This is required for encryption/activities * * @param \OCP\Files\Node $path + * @return array */ public function getAccessList(\OCP\Files\Node $path) { + $owner = $path->getOwner()->getUID(); + + //Get node for the owner + $userFolder = $this->rootFolder->getUserFolder($owner); + $path = $userFolder->getById($path->getId())[0]; + + $providers = $this->factory->getAllProviders(); + + /** @var IShare[] $shares */ + $shares = []; + + // Collect all the shares + while ($path !== $userFolder) { + foreach ($providers as $provider) { + $shares = array_merge($shares, $provider->getSharesByPath($path)); + } + $path = $path->getParent(); + } + + $users = []; + $public = false; + $remote = false; + foreach ($shares as $share) { + if ($share->getShareType() === \OCP\Share::SHARE_TYPE_USER) { + $uid = $share->getSharedWith(); + + // Skip if user does not exist + if (!$this->userManager->userExists($uid)) { + continue; + } + + $users[$uid] = null; + } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { + $group = $this->groupManager->get($share->getSharedWith()); + + // If group does not exist skip + if ($group === null) { + continue; + } + + $groupUsers = $group->getUsers(); + foreach ($groupUsers as $groupUser) { + $users[$groupUser->getUID()] = null; + } + } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) { + $public = true; + } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_REMOTE) { + $remote = true; + } + } + + $users = array_keys($users); + + return ['users' => $users, 'public' => $public, 'remote' => $remote]; } /** diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 5436b18835..20ba62fa22 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2639,7 +2639,6 @@ class ManagerTest extends \Test\TestCase { $this->manager->moveShare($share, 'recipient'); } - /** * @dataProvider dataTestShareProviderExists */ @@ -2737,6 +2736,88 @@ class ManagerTest extends \Test\TestCase { $this->assertSame($expects, $result); } + + public function testGetAccessList() { + $owner = $this->createMock(IUser::class); + $owner->expects($this->once()) + ->method('getUID') + ->willReturn('owner'); + + $node = $this->createMock(Node::class); + $node->expects($this->once()) + ->method('getOwner') + ->willReturn($owner); + $node->expects($this->once()) + ->method('getId') + ->willReturn(42); + + $userFolder = $this->createMock(Folder::class); + $file = $this->createMock(File::class); + $folder = $this->createMock(Folder::class); + + $file->method('getParent') + ->willReturn($folder); + $folder->method('getParent') + ->willReturn($userFolder); + $userFolder->method('getById') + ->with($this->equalTo(42)) + ->willReturn([$file]); + + $userShare = $this->createMock(IShare::class); + $userShare->method('getShareType') + ->willReturn(\OCP\Share::SHARE_TYPE_USER); + $userShare->method('getSharedWith') + ->willReturn('user1'); + $groupShare = $this->createMock(IShare::class); + $groupShare->method('getShareType') + ->willReturn(\OCP\Share::SHARE_TYPE_GROUP); + $groupShare->method('getSharedWith') + ->willReturn('group1'); + $publicShare = $this->createMock(IShare::class); + $publicShare->method('getShareType') + ->willReturn(\OCP\Share::SHARE_TYPE_LINK); + $remoteShare = $this->createMock(IShare::class); + $remoteShare->method('getShareType') + ->willReturn(\OCP\Share::SHARE_TYPE_REMOTE); + + $this->userManager->method('userExists') + ->with($this->equalTo('user1')) + ->willReturn(true); + + $user2 = $this->createMock(IUser::class); + $user2->method('getUID') + ->willReturn('user2'); + $group1 = $this->createMock(IGroup::class); + $this->groupManager->method('get') + ->with($this->equalTo('group1')) + ->willReturn($group1); + $group1->method('getUsers') + ->willReturn([$user2]); + + $this->defaultProvider->expects($this->any()) + ->method('getSharesByPath') + ->will($this->returnCallback(function(Node $path) use ($file, $folder, $userShare, $groupShare, $publicShare, $remoteShare) { + if ($path === $file) { + return [$userShare, $publicShare]; + } else if ($path === $folder) { + return [$groupShare, $remoteShare]; + } else { + return []; + } + })); + + $this->rootFolder->method('getUserFolder') + ->with($this->equalTo('owner')) + ->willReturn($userFolder); + + $expected = [ + 'users' => ['user1', 'user2'], + 'public' => true, + 'remote' => true, + ]; + + $this->assertEquals($expected, $this->manager->getAccessList($node)); + } } class DummyFactory implements IProviderFactory { From 7dcc98eb202c089a2966004ffb988166f3c4e3d7 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 9 Sep 2016 12:57:01 +0200 Subject: [PATCH 02/29] Add owner to access list Signed-off-by: Roeland Jago Douma --- .htaccess | 4 ++++ lib/private/Share20/Manager.php | 2 +- tests/lib/Share20/ManagerTest.php | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.htaccess b/.htaccess index 7bf8759e38..26be470fd1 100644 --- a/.htaccess +++ b/.htaccess @@ -78,3 +78,7 @@ Options -Indexes ModPagespeed Off +#### DO NOT CHANGE ANYTHING ABOVE THIS LINE #### + +ErrorDocument 403 /core/templates/403.php +ErrorDocument 404 /core/templates/404.php diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 127eef423e..ce0444a76b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1221,7 +1221,7 @@ class Manager implements IManager { $path = $path->getParent(); } - $users = []; + $users = [$owner => 'null']; $public = false; $remote = false; foreach ($shares as $share) { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 20ba62fa22..bbdef2130f 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2811,7 +2811,7 @@ class ManagerTest extends \Test\TestCase { ->willReturn($userFolder); $expected = [ - 'users' => ['user1', 'user2'], + 'users' => ['owner', 'user1', 'user2'], 'public' => true, 'remote' => true, ]; From 88299ec27c209a432f925f19970e581f46fc54cd Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 19 Oct 2016 21:27:07 +0200 Subject: [PATCH 03/29] Added to public interface Signed-off-by: Roeland Jago Douma --- .htaccess | 4 ---- lib/private/Share20/Manager.php | 15 +++++++++++---- lib/public/Share/IManager.php | 30 ++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/.htaccess b/.htaccess index 26be470fd1..7bf8759e38 100644 --- a/.htaccess +++ b/.htaccess @@ -78,7 +78,3 @@ Options -Indexes ModPagespeed Off -#### DO NOT CHANGE ANYTHING ABOVE THIS LINE #### - -ErrorDocument 403 /core/templates/403.php -ErrorDocument 404 /core/templates/404.php diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index ce0444a76b..d93883e95d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1196,17 +1196,21 @@ class Manager implements IManager { * remote => bool * ] * - * This is required for encryption/activities + * This is required for encryption/activity * * @param \OCP\Files\Node $path + * @param bool $recursive Should we check all parent folders as well * @return array */ - public function getAccessList(\OCP\Files\Node $path) { + public function getAccessList(\OCP\Files\Node $path, $recursive = true) { $owner = $path->getOwner()->getUID(); //Get node for the owner $userFolder = $this->rootFolder->getUserFolder($owner); - $path = $userFolder->getById($path->getId())[0]; + + if (!$userFolder->isSubNode($path)) { + $path = $userFolder->getById($path->getId())[0]; + } $providers = $this->factory->getAllProviders(); @@ -1214,10 +1218,13 @@ class Manager implements IManager { $shares = []; // Collect all the shares - while ($path !== $userFolder) { + while ($path->getPath() !== $userFolder->getPath()) { foreach ($providers as $provider) { $shares = array_merge($shares, $provider->getSharesByPath($path)); } + if (!$recursive) { + break; + } $path = $path->getParent(); } diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index a15020bbd6..45e0e9d80c 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -191,6 +191,36 @@ interface IManager { */ public function userDeletedFromGroup($uid, $gid); + /** + * Get access list to a path. This means + * all the users that can access a given path. + * + * Consider: + * -root + * |-folder1 + * |-folder2 + * |-fileA + * + * fileA is shared with user1 + * folder2 is shared with group2 (user4 is a member of group2) + * folder1 is shared with user2 + * + * Then the access list will to '/folder1/folder2/fileA' is: + * [ + * users => ['user1', 'user2', 'user4'], + * public => bool + * remote => bool + * ] + * + * This is required for encryption/activity + * + * @param \OCP\Files\Node $path + * @param bool $recursive Should we check all parent folders as well + * @return array + * @since 9.2.0 + */ + public function getAccessList(\OCP\Files\Node $path, $recursive = true); + /** * Instantiates a new share object. This is to be passed to * createShare. From a1edcc8ecf7b81e19c772fbe8586acc38e1eadad Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 19 Oct 2016 21:27:34 +0200 Subject: [PATCH 04/29] Port Encryption/file to new getAccessList Signed-off-by: Roeland Jago Douma --- lib/private/Encryption/File.php | 33 +++++++++++++++++++++++++++------ lib/private/Server.php | 6 +++++- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index 240a8f1cca..329fb12b83 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -25,12 +25,21 @@ namespace OC\Encryption; use OC\Cache\CappedMemoryCache; +use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; +use OCP\Share\IManager; class File implements \OCP\Encryption\IFile { /** @var Util */ protected $util; + /** @var IRootFolder */ + private $rootFolder; + + /** @var IManager */ + private $shareManager; + /** * cache results of already checked folders * @@ -38,9 +47,13 @@ class File implements \OCP\Encryption\IFile { */ protected $cache; - public function __construct(Util $util) { + public function __construct(Util $util, + IRootFolder $rootFolder, + IManager $shareManager) { $this->util = $util; $this->cache = new CappedMemoryCache(); + $this->rootFolder = $rootFolder; + $this->shareManager = $shareManager; } @@ -63,16 +76,22 @@ class File implements \OCP\Encryption\IFile { } $ownerPath = substr($ownerPath, strlen('/files')); + $userFolder = $this->rootFolder->getUserFolder($owner); + try { + $file = $userFolder->get($ownerPath); + } catch (NotFoundException $e) { + $file = null; + } $ownerPath = $this->util->stripPartialFileExtension($ownerPath); - // first get the shares for the parent and cache the result so that we don't // need to check all parents for every file $parent = dirname($ownerPath); + $parentNode = $userFolder->get($parent); if (isset($this->cache[$parent])) { $resultForParents = $this->cache[$parent]; } else { - $resultForParents = \OCP\Share::getUsersSharingFile($parent, $owner); + $resultForParents = $this->shareManager->getAccessList($parentNode); $this->cache[$parent] = $resultForParents; } $userIds = \array_merge($userIds, $resultForParents['users']); @@ -80,9 +99,11 @@ class File implements \OCP\Encryption\IFile { // Find out who, if anyone, is sharing the file - $resultForFile = \OCP\Share::getUsersSharingFile($ownerPath, $owner, false, false, false); - $userIds = \array_merge($userIds, $resultForFile['users']); - $public = $resultForFile['public'] || $resultForFile['remote'] || $public; + if ($file !== null) { + $resultForFile = $this->shareManager->getAccessList($file, false); + $userIds = \array_merge($userIds, $resultForFile['users']); + $public = $resultForFile['public'] || $resultForFile['remote'] || $public; + } // check if it is a group mount if (\OCP\App::isEnabled("files_external")) { diff --git a/lib/private/Server.php b/lib/private/Server.php index 00698a04f8..c38675934c 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -173,7 +173,11 @@ class Server extends ServerContainer implements IServerContainer { $c->getGroupManager(), $c->getConfig() ); - return new Encryption\File($util); + return new Encryption\File( + $util, + $c->getRootFolder(), + $c->getShareManager() + ); }); $this->registerService('EncryptionKeyStorage', function (Server $c) { From 553b3b29280dde318c4987dfd42436f72eff0df4 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 27 Oct 2016 15:16:53 +0200 Subject: [PATCH 05/29] Fix tests Signed-off-by: Roeland Jago Douma --- tests/lib/Share20/ManagerTest.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index bbdef2130f..abfe2ca9c1 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2757,11 +2757,17 @@ class ManagerTest extends \Test\TestCase { $file->method('getParent') ->willReturn($folder); + $file->method('getPath') + ->willReturn('/owner/files/folder/file'); $folder->method('getParent') ->willReturn($userFolder); + $folder->method('getPath') + ->willReturn('/owner/files/folder'); $userFolder->method('getById') ->with($this->equalTo(42)) ->willReturn([$file]); + $userFolder->method('getPath') + ->willReturn('/owner/files'); $userShare = $this->createMock(IShare::class); $userShare->method('getShareType') From 97f8ca6595caf051eae9c9261bdbd481c9dd4be1 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 27 Oct 2016 15:32:23 +0200 Subject: [PATCH 06/29] Added ShareHelper Signed-off-by: Roeland Jago Douma --- lib/private/Share20/ShareHelper.php | 64 +++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 lib/private/Share20/ShareHelper.php diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php new file mode 100644 index 0000000000..a7d46457cd --- /dev/null +++ b/lib/private/Share20/ShareHelper.php @@ -0,0 +1,64 @@ + + * + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OC\Share20; + +use OCP\Files\IRootFolder; +use OCP\Files\Node; +use OCP\Files\NotFoundException; + +class ShareHelper { + /** @var IRootFolder */ + private $rootFolder; + + public function __construct(IRootFolder $rootFolder) { + $this->rootFolder = $rootFolder; + } + + /** + * If a user has access to a file + * + * @param Node $node + * @param array $users Array of userIds + * @return array Mapping $uid to an array of nodes + */ + public function getPathsForAccessList(Node $node, $users) { + $result = []; + + foreach ($users as $user) { + try { + $userFolder = $this->rootFolder->getUserFolder($user); + } catch (NotFoundException $e) { + continue; + } + + $nodes = $userFolder->getById($node->getId()); + if ($nodes === []) { + continue; + } + + $result[$user] = $nodes; + } + + return $result; + } +} From 2cbac3357ba445a3a4cd073e119efb871ea0f719 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 22 Dec 2016 21:44:21 +0100 Subject: [PATCH 07/29] Offload acceslist creation to providers * This allows for effective queries. * Introduce currentAccess parameter to speciy if the users needs to have currently acces (deleted incomming group share). (For notifications) Signed-off-by: Roeland Jago Douma --- .../lib/FederatedShareProvider.php | 25 +++ apps/sharebymail/lib/ShareByMailProvider.php | 4 + lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Share20/DefaultShareProvider.php | 76 +++++++- lib/private/Share20/Manager.php | 57 ++---- lib/public/Share/IShareProvider.php | 17 ++ .../lib/Share20/DefaultShareProviderTest.php | 175 ++++++++++++++++++ tests/lib/Share20/ManagerTest.php | 96 ++++++---- 9 files changed, 371 insertions(+), 81 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 120365263a..b54f8a69fd 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -28,6 +28,7 @@ namespace OCA\FederatedFileSharing; use OC\Share20\Share; use OCP\Federation\ICloudIdManager; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\IConfig; @@ -974,4 +975,28 @@ class FederatedShareProvider implements IShareProvider { $result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'yes'); return ($result === 'yes'); } + + public function getAccessList($nodes, $currentAccess) { + $ids = []; + foreach ($nodes as $node) { + $ids[] = $node->getId(); + } + + $qb = $this->dbConnection->getQueryBuilder(); + $qb->select('share_with') + ->from('share') + ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_REMOTE))) + ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) + )) + ->setMaxResults(1); + $cursor = $qb->execute(); + + $remote = $cursor->fetch() !== false; + $cursor->closeCursor(); + + return ['users' => [], 'remote' => $remote, 'public' => false]; + } } diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 0b959ce426..c367130487 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -834,4 +834,8 @@ class ShareByMailProvider implements IShareProvider { return $shares; } + public function getAccessList($nodes, $currentAccess) { + return ['users' => [], 'remote' => false, 'public' => false]; + } + } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index ab6a378114..3cf8733e41 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -794,6 +794,7 @@ return array( 'OC\\Share20\\Manager' => $baseDir . '/lib/private/Share20/Manager.php', 'OC\\Share20\\ProviderFactory' => $baseDir . '/lib/private/Share20/ProviderFactory.php', 'OC\\Share20\\Share' => $baseDir . '/lib/private/Share20/Share.php', + 'OC\\Share20\\ShareHelper' => $baseDir . '/lib/private/Share20/ShareHelper.php', 'OC\\Share\\Constants' => $baseDir . '/lib/private/Share/Constants.php', 'OC\\Share\\Helper' => $baseDir . '/lib/private/Share/Helper.php', 'OC\\Share\\MailNotifications' => $baseDir . '/lib/private/Share/MailNotifications.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 1b2c9f84df..4aecf62329 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -824,6 +824,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Share20\\Manager' => __DIR__ . '/../../..' . '/lib/private/Share20/Manager.php', 'OC\\Share20\\ProviderFactory' => __DIR__ . '/../../..' . '/lib/private/Share20/ProviderFactory.php', 'OC\\Share20\\Share' => __DIR__ . '/../../..' . '/lib/private/Share20/Share.php', + 'OC\\Share20\\ShareHelper' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareHelper.php', 'OC\\Share\\Constants' => __DIR__ . '/../../..' . '/lib/private/Share/Constants.php', 'OC\\Share\\Helper' => __DIR__ . '/../../..' . '/lib/private/Share/Helper.php', 'OC\\Share\\MailNotifications' => __DIR__ . '/../../..' . '/lib/private/Share/MailNotifications.php', diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index feae147066..93e8cfc9d7 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -362,7 +362,7 @@ class DefaultShareProvider implements IShareProvider { if ($data === false) { $qb = $this->dbConn->getQueryBuilder(); - $type = $share->getNode() instanceof \OCP\Files\File ? 'file' : 'folder'; + $type = $share->getNodeType(); //Insert new share $qb->insert('share') @@ -373,8 +373,8 @@ class DefaultShareProvider implements IShareProvider { 'uid_initiator' => $qb->createNamedParameter($share->getSharedBy()), 'parent' => $qb->createNamedParameter($share->getId()), 'item_type' => $qb->createNamedParameter($type), - 'item_source' => $qb->createNamedParameter($share->getNode()->getId()), - 'file_source' => $qb->createNamedParameter($share->getNode()->getId()), + 'item_source' => $qb->createNamedParameter($share->getNodeId()), + 'file_source' => $qb->createNamedParameter($share->getNodeId()), 'file_target' => $qb->createNamedParameter($share->getTarget()), 'permissions' => $qb->createNamedParameter(0), 'stime' => $qb->createNamedParameter($share->getShareTime()->getTimestamp()), @@ -1070,4 +1070,74 @@ class DefaultShareProvider implements IShareProvider { } } } + + /** + * @inheritdoc + */ + public function getAccessList($nodes, $currentAccess) { + $ids = []; + foreach ($nodes as $node) { + $ids[] = $node->getId(); + } + + $qb = $this->dbConn->getQueryBuilder(); + + $or = $qb->expr()->orX( + $qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_USER)), + $qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_GROUP)), + $qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_LINK)) + ); + + if ($currentAccess) { + $or->add($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USERGROUP))); + } + + $qb->select('share_type', 'share_with') + ->from('share') + ->where( + $or + ) + ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) + )); + $cursor = $qb->execute(); + + $users = []; + $link = false; + while($row = $cursor->fetch()) { + $type = (int)$row['share_type']; + if ($type === \OCP\Share::SHARE_TYPE_USER) { + $uid = $row['share_with']; + $users[$uid] = isset($users[$uid]) ? $users[$uid] + 1 : 1; + } else if ($type === \OCP\Share::SHARE_TYPE_GROUP) { + $gid = $row['share_with']; + $group = $this->groupManager->get($gid); + + if ($gid === null) { + continue; + } + + $userList = $group->getUsers(); + foreach ($userList as $user) { + $users[$user->getUID()] = isset($users[$user->getUID()]) ? $users[$user->getUID()] + 1 : 1; + } + } else if ($type === \OCP\Share::SHARE_TYPE_LINK) { + $link = true; + } else if ($type === self::SHARE_TYPE_USERGROUP) { + if ($currentAccess === true) { + $uid = $row['share_with']; + $users[$uid] = isset($users[$uid]) ? $users[$uid] - 1 : -1; + } + } + } + $cursor->closeCursor(); + + $users = array_filter($users, function($count) { + return $count > 0; + }); + + return ['users' => array_keys($users), 'public' => $link, 'remote' => false]; + } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index d93883e95d..096d05fbec 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1200,69 +1200,50 @@ class Manager implements IManager { * * @param \OCP\Files\Node $path * @param bool $recursive Should we check all parent folders as well + * @param bool $currentAccess Should the user have currently access to the file * @return array */ - public function getAccessList(\OCP\Files\Node $path, $recursive = true) { + public function getAccessList(\OCP\Files\Node $path, $recursive = true, $currentAccess = false) { $owner = $path->getOwner()->getUID(); + $al = ['users' => [], 'public' => false, 'remote' => false]; + if (!$this->userManager->userExists($owner)) { + return $al; + } + //Get node for the owner $userFolder = $this->rootFolder->getUserFolder($owner); - if (!$userFolder->isSubNode($path)) { $path = $userFolder->getById($path->getId())[0]; } $providers = $this->factory->getAllProviders(); - /** @var IShare[] $shares */ - $shares = []; + /** @var Node[] $nodes */ + $nodes = []; + + $al['users'][] = $owner; // Collect all the shares while ($path->getPath() !== $userFolder->getPath()) { - foreach ($providers as $provider) { - $shares = array_merge($shares, $provider->getSharesByPath($path)); - } + $nodes[] = $path; if (!$recursive) { break; } $path = $path->getParent(); } - $users = [$owner => 'null']; - $public = false; - $remote = false; - foreach ($shares as $share) { - if ($share->getShareType() === \OCP\Share::SHARE_TYPE_USER) { - $uid = $share->getSharedWith(); + foreach ($providers as $provider) { + $tmp = $provider->getAccessList($nodes, $currentAccess); - // Skip if user does not exist - if (!$this->userManager->userExists($uid)) { - continue; - } - - $users[$uid] = null; - } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { - $group = $this->groupManager->get($share->getSharedWith()); - - // If group does not exist skip - if ($group === null) { - continue; - } - - $groupUsers = $group->getUsers(); - foreach ($groupUsers as $groupUser) { - $users[$groupUser->getUID()] = null; - } - } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) { - $public = true; - } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_REMOTE) { - $remote = true; - } + $al['users'] = array_merge($al['users'], $tmp['users']); + $al['public'] = $al['public'] || $tmp['public']; + $al['remote'] = $al['remote'] || $tmp['remote']; } - $users = array_keys($users); + $al['users'] = array_unique($al['users']); - return ['users' => $users, 'public' => $public, 'remote' => $remote]; + return $al; } /** diff --git a/lib/public/Share/IShareProvider.php b/lib/public/Share/IShareProvider.php index 6257c97eb7..2533e81abf 100644 --- a/lib/public/Share/IShareProvider.php +++ b/lib/public/Share/IShareProvider.php @@ -190,4 +190,21 @@ interface IShareProvider { * @since 9.1.0 */ public function userDeletedFromGroup($uid, $gid); + + /** + * Get the access list to the array of provided nodes. + * Return will look like: + * + * [ + * users => ['user1', 'user2', 'user4'], + * public => bool + * remote => bool + * ] + * + * @param Node[] $nodes The list of nodes to get access for + * @param bool $currentAccess If current access is required (like for removed shares that might get revived later) + * @return string[] + * @since 12 + */ + public function getAccessList($nodes, $currentAccess); } diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index fce5668440..ccc9f0f511 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -2434,4 +2434,179 @@ class DefaultShareProviderTest extends \Test\TestCase { $u3->delete(); $g1->delete(); } + + public function testGetAccessListNoCurrentAccessRequired() { + $userManager = \OC::$server->getUserManager(); + $groupManager = \OC::$server->getGroupManager(); + $rootFolder = \OC::$server->getRootFolder(); + + $provider = new DefaultShareProvider( + $this->dbConn, + $userManager, + $groupManager, + $rootFolder + ); + + $u1 = $userManager->createUser('testShare1', 'test'); + $u2 = $userManager->createUser('testShare2', 'test'); + $u3 = $userManager->createUser('testShare3', 'test'); + $u4 = $userManager->createUser('testShare4', 'test'); + $u5 = $userManager->createUser('testShare5', 'test'); + + $g1 = $groupManager->createGroup('group1'); + $g1->addUser($u3); + $g1->addUser($u4); + + $u1Folder = $rootFolder->getUserFolder($u1->getUID()); + $folder1 = $u1Folder->newFolder('foo'); + $folder2 = $folder1->newFolder('baz'); + $file1 = $folder2->newFile('bar'); + + $shareManager = \OC::$server->getShareManager(); + $share1 = $shareManager->newShare(); + $share1->setNode($folder1) + ->setSharedBy($u1->getUID()) + ->setSharedWith($u2->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_USER) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + $share1 = $this->provider->create($share1); + + $share2 = $shareManager->newShare(); + $share2->setNode($folder2) + ->setSharedBy($u2->getUID()) + ->setSharedWith($g1->getGID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_GROUP) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + $share2 = $this->provider->create($share2); + + $shareManager->deleteFromSelf($share2, $u4->getUID()); + + $share3 = $shareManager->newShare(); + $share3->setNode($file1) + ->setSharedBy($u3->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_LINK) + ->setPermissions(\OCP\Constants::PERMISSION_READ); + $share3 = $this->provider->create($share3); + + $share4 = $shareManager->newShare(); + $share4->setNode($file1) + ->setSharedBy($u3->getUID()) + ->setSharedWith($u5->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_USER) + ->setPermissions(\OCP\Constants::PERMISSION_READ); + $share4 = $this->provider->create($share4); + + $result = $provider->getAccessList([$folder1, $folder2, $file1], false); + + $this->assertCount(4, $result['users']); + $this->assertContains('testShare2', $result['users']); + $this->assertContains('testShare3', $result['users']); + $this->assertContains('testShare4', $result['users']); + $this->assertContains('testShare5', $result['users']); + $this->assertTrue($result['public']); + $this->assertFalse($result['remote']); + + $provider->delete($share1); + $provider->delete($share2); + $provider->delete($share3); + $provider->delete($share4); + + $u1->delete(); + $u2->delete(); + $u3->delete(); + $u4->delete(); + $u5->delete(); + $g1->delete(); + } + + public function testGetAccessListCurrentAccessRequired() { + $userManager = \OC::$server->getUserManager(); + $groupManager = \OC::$server->getGroupManager(); + $rootFolder = \OC::$server->getRootFolder(); + + $provider = new DefaultShareProvider( + $this->dbConn, + $userManager, + $groupManager, + $rootFolder + ); + + $u1 = $userManager->createUser('testShare1', 'test'); + $u2 = $userManager->createUser('testShare2', 'test'); + $u3 = $userManager->createUser('testShare3', 'test'); + $u4 = $userManager->createUser('testShare4', 'test'); + $u5 = $userManager->createUser('testShare5', 'test'); + + $g1 = $groupManager->createGroup('group1'); + $g1->addUser($u3); + $g1->addUser($u4); + + $u1Folder = $rootFolder->getUserFolder($u1->getUID()); + $folder1 = $u1Folder->newFolder('foo'); + $folder2 = $folder1->newFolder('baz'); + $file1 = $folder2->newFile('bar'); + + $shareManager = \OC::$server->getShareManager(); + $share1 = $shareManager->newShare(); + $share1->setNode($folder1) + ->setSharedBy($u1->getUID()) + ->setSharedWith($u2->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_USER) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + $share1 = $this->provider->create($share1); + + $share2 = $shareManager->newShare(); + $share2->setNode($folder2) + ->setSharedBy($u2->getUID()) + ->setSharedWith($g1->getGID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_GROUP) + ->setPermissions(\OCP\Constants::PERMISSION_ALL); + $share2 = $this->provider->create($share2); + + $shareManager->deleteFromSelf($share2, $u4->getUID()); + + $share3 = $shareManager->newShare(); + $share3->setNode($file1) + ->setSharedBy($u3->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_LINK) + ->setPermissions(\OCP\Constants::PERMISSION_READ); + $share3 = $this->provider->create($share3); + + $share4 = $shareManager->newShare(); + $share4->setNode($file1) + ->setSharedBy($u3->getUID()) + ->setSharedWith($u5->getUID()) + ->setShareOwner($u1->getUID()) + ->setShareType(\OCP\Share::SHARE_TYPE_USER) + ->setPermissions(\OCP\Constants::PERMISSION_READ); + $share4 = $this->provider->create($share4); + + $result = $provider->getAccessList([$folder1, $folder2, $file1], true); + + $this->assertCount(3, $result['users']); + $this->assertContains('testShare2', $result['users']); + $this->assertContains('testShare3', $result['users']); + $this->assertContains('testShare5', $result['users']); + $this->assertTrue($result['public']); + $this->assertFalse($result['remote']); + + $provider->delete($share1); + $provider->delete($share2); + $provider->delete($share3); + $provider->delete($share4); + + $u1->delete(); + $u2->delete(); + $u3->delete(); + $u4->delete(); + $u5->delete(); + $g1->delete(); + } } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index abfe2ca9c1..ab42cb4813 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2738,6 +2738,26 @@ class ManagerTest extends \Test\TestCase { } public function testGetAccessList() { + $factory = new DummyFactory2($this->createMock(IServerContainer::class)); + + $manager = new Manager( + $this->logger, + $this->config, + $this->secureRandom, + $this->hasher, + $this->mountManager, + $this->groupManager, + $this->l, + $factory, + $this->userManager, + $this->rootFolder, + $this->eventDispatcher + ); + + $factory->setProvider($this->defaultProvider); + $extraProvider = $this->createMock(IShareProvider::class); + $factory->setSecondProvider($extraProvider); + $owner = $this->createMock(IUser::class); $owner->expects($this->once()) ->method('getUID') @@ -2769,60 +2789,56 @@ class ManagerTest extends \Test\TestCase { $userFolder->method('getPath') ->willReturn('/owner/files'); - $userShare = $this->createMock(IShare::class); - $userShare->method('getShareType') - ->willReturn(\OCP\Share::SHARE_TYPE_USER); - $userShare->method('getSharedWith') - ->willReturn('user1'); - $groupShare = $this->createMock(IShare::class); - $groupShare->method('getShareType') - ->willReturn(\OCP\Share::SHARE_TYPE_GROUP); - $groupShare->method('getSharedWith') - ->willReturn('group1'); - $publicShare = $this->createMock(IShare::class); - $publicShare->method('getShareType') - ->willReturn(\OCP\Share::SHARE_TYPE_LINK); - $remoteShare = $this->createMock(IShare::class); - $remoteShare->method('getShareType') - ->willReturn(\OCP\Share::SHARE_TYPE_REMOTE); - $this->userManager->method('userExists') - ->with($this->equalTo('user1')) + ->with($this->equalTo('owner')) ->willReturn(true); - $user2 = $this->createMock(IUser::class); - $user2->method('getUID') - ->willReturn('user2'); - $group1 = $this->createMock(IGroup::class); - $this->groupManager->method('get') - ->with($this->equalTo('group1')) - ->willReturn($group1); - $group1->method('getUsers') - ->willReturn([$user2]); + $this->defaultProvider->method('getAccessList') + ->with( + $this->equalTo([$file, $folder]), + true + ) + ->willReturn([ + 'users' => [ + 'user1', + 'user2', + 'user3', + ], + 'remote' => false, + 'public' => true, + ]); - $this->defaultProvider->expects($this->any()) - ->method('getSharesByPath') - ->will($this->returnCallback(function(Node $path) use ($file, $folder, $userShare, $groupShare, $publicShare, $remoteShare) { - if ($path === $file) { - return [$userShare, $publicShare]; - } else if ($path === $folder) { - return [$groupShare, $remoteShare]; - } else { - return []; - } - })); + $extraProvider->method('getAccessList') + ->with( + $this->equalTo([$file, $folder]), + true + ) + ->willReturn([ + 'users' => [ + 'user3', + 'user4', + 'user5', + ], + 'remote' => true, + 'public' => false, + ]); $this->rootFolder->method('getUserFolder') ->with($this->equalTo('owner')) ->willReturn($userFolder); $expected = [ - 'users' => ['owner', 'user1', 'user2'], + 'users' => ['owner', 'user1', 'user2', 'user3', 'user4', 'user5'], 'public' => true, 'remote' => true, ]; - $this->assertEquals($expected, $this->manager->getAccessList($node)); + $result = $manager->getAccessList($node, true, true); + + $this->assertSame($expected['public'], $result['public']); + $this->assertSame($expected['remote'], $result['remote']); + $this->assertSame(array_values($expected['users']), array_values($result['users'])); + } } From 12afd7d1d5bc2a85d04815964bf7fb37e77466d6 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 4 Jan 2017 08:59:43 +0100 Subject: [PATCH 08/29] Add mail element to access list * Each provider just returns what they have so adding an element won't require changing everything * Added tests Signed-off-by: Roeland Jago Douma --- .../lib/FederatedShareProvider.php | 2 +- .../tests/FederatedShareProviderTest.php | 28 +++++++++++++++++++ apps/sharebymail/lib/ShareByMailProvider.php | 22 ++++++++++++++- lib/private/Share20/DefaultShareProvider.php | 2 +- lib/private/Share20/Manager.php | 14 ++++++++-- .../lib/Share20/DefaultShareProviderTest.php | 2 -- tests/lib/Share20/ManagerTest.php | 2 -- 7 files changed, 62 insertions(+), 10 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index b54f8a69fd..29aa594df5 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -997,6 +997,6 @@ class FederatedShareProvider implements IShareProvider { $remote = $cursor->fetch() !== false; $cursor->closeCursor(); - return ['users' => [], 'remote' => $remote, 'public' => false]; + return ['remote' => $remote]; } } diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index d9bc9a7e2a..7d571b2082 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -788,4 +788,32 @@ class FederatedShareProviderTest extends \Test\TestCase { $u1->delete(); $u2->delete(); } + + public function testGetAccessList() { + $userManager = \OC::$server->getUserManager(); + $rootFolder = \OC::$server->getRootFolder(); + + $u1 = $userManager->createUser('testFed', md5(time())); + + $folder1 = $rootFolder->getUserFolder($u1->getUID())->newFolder('foo'); + $file1 = $folder1->newFile('bar1'); + + $this->tokenHandler->method('generateToken')->willReturn('token'); + $this->notifications + ->method('sendRemoteShare') + ->willReturn(true); + + $share1 = $this->shareManager->newShare(); + $share1->setSharedWith('user@server.com') + ->setSharedBy($u1->getUID()) + ->setShareOwner($u1->getUID()) + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1); + $this->provider->create($share1); + + $result = $this->provider->getAccessList([$file1], true); + + $this->assertSame(['remote' => true], $result); + $u1->delete(); + } } diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index c367130487..14ed12749b 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -835,7 +835,27 @@ class ShareByMailProvider implements IShareProvider { } public function getAccessList($nodes, $currentAccess) { - return ['users' => [], 'remote' => false, 'public' => false]; + $ids = []; + foreach ($nodes as $node) { + $ids[] = $node->getId(); + } + + $qb = $this->dbConnection->getQueryBuilder(); + $qb->select('share_with') + ->from('share') + ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_EMAIL))) + ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) + )) + ->setMaxResults(1); + $cursor = $qb->execute(); + + $mail = $cursor->fetch() !== false; + $cursor->closeCursor(); + + return ['mail' => $mail]; } } diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 93e8cfc9d7..b8f46a1f4d 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1138,6 +1138,6 @@ class DefaultShareProvider implements IShareProvider { return $count > 0; }); - return ['users' => array_keys($users), 'public' => $link, 'remote' => false]; + return ['users' => array_keys($users), 'public' => $link]; } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 096d05fbec..f1ce66f008 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1236,9 +1236,17 @@ class Manager implements IManager { foreach ($providers as $provider) { $tmp = $provider->getAccessList($nodes, $currentAccess); - $al['users'] = array_merge($al['users'], $tmp['users']); - $al['public'] = $al['public'] || $tmp['public']; - $al['remote'] = $al['remote'] || $tmp['remote']; + foreach ($tmp as $k => $v) { + if (isset($al[$k])) { + if (is_array($al[$k])) { + $al[$k] = array_merge($al[$k], $v); + } else { + $al[$k] = $al[$k] || $v; + } + } else { + $al[$k] = $v; + } + } } $al['users'] = array_unique($al['users']); diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index ccc9f0f511..a7f5555efa 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -2508,7 +2508,6 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertContains('testShare4', $result['users']); $this->assertContains('testShare5', $result['users']); $this->assertTrue($result['public']); - $this->assertFalse($result['remote']); $provider->delete($share1); $provider->delete($share2); @@ -2595,7 +2594,6 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertContains('testShare3', $result['users']); $this->assertContains('testShare5', $result['users']); $this->assertTrue($result['public']); - $this->assertFalse($result['remote']); $provider->delete($share1); $provider->delete($share2); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index ab42cb4813..c1a0eaf5bb 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2804,7 +2804,6 @@ class ManagerTest extends \Test\TestCase { 'user2', 'user3', ], - 'remote' => false, 'public' => true, ]); @@ -2820,7 +2819,6 @@ class ManagerTest extends \Test\TestCase { 'user5', ], 'remote' => true, - 'public' => false, ]); $this->rootFolder->method('getUserFolder') From 4437e00f162f74b6a8aad4ac5c1376099af0c194 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Wed, 4 Jan 2017 09:13:08 +0100 Subject: [PATCH 09/29] Add shareHelper test Signed-off-by: Roeland Jago Douma --- tests/lib/Share20/ShareHelperTests.php | 94 ++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 tests/lib/Share20/ShareHelperTests.php diff --git a/tests/lib/Share20/ShareHelperTests.php b/tests/lib/Share20/ShareHelperTests.php new file mode 100644 index 0000000000..b6c737cf44 --- /dev/null +++ b/tests/lib/Share20/ShareHelperTests.php @@ -0,0 +1,94 @@ + + * + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace Test\Share20; + +use OC\Share20\ShareHelper; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\Files\Node; +use OCP\Files\NotFoundException; +use Test\TestCase; + +class ShareHelperTests extends TestCase { + + /** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */ + private $rootFolder; + + /** @var ShareHelper */ + private $helper; + + public function setUp() { + parent::setUp(); + + $this->rootFolder = $this->createMock(IRootFolder::class); + + $this->helper = new ShareHelper($this->rootFolder); + } + + /** + * uid1 - Exists with valid node + * uid2 - Does not exist + * uid3 - Exists but no valid node + * uid4 - Exists with valid node + */ + public function testGetPathsForAccessList() { + /** @var Folder[]|\PHPUnit_Framework_MockObject_MockObject[] $userFolder */ + $userFolder = [ + 'uid1' => $this->createMock(Folder::class), + 'uid3' => $this->createMock(Folder::class), + 'uid4' => $this->createMock(Folder::class), + ]; + + $this->rootFolder->method('getUserFolder') + ->willReturnCallback(function($uid) use ($userFolder) { + if (isset($userFolder[$uid])) { + return $userFolder[$uid]; + } + throw new NotFoundException(); + }); + + /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->createMock(Node::class); + $node->method('getId') + ->willReturn(42); + + $userFolder['uid1']->method('getById') + ->with(42) + ->willReturn([$node]); + $userFolder['uid3']->method('getById') + ->with(42) + ->willReturn([]); + $userFolder['uid4']->method('getById') + ->with(42) + ->willReturn([$node]); + + $expects = [ + 'uid1' => [$node], + 'uid4' => [$node], + ]; + + $result = $this->helper->getPathsForAccessList($node, ['uid1', 'uid2', 'uid3', 'uid4']); + + $this->assertSame($expects, $result); + } +} From 0c2dc3bc8cdca46afc2bc08da4a985610093f2af Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 20 Mar 2017 14:22:06 +0100 Subject: [PATCH 10/29] Fix comments Signed-off-by: Roeland Jago Douma --- .../DependencyInjection/DIContainer.php | 5 +++ lib/private/Encryption/File.php | 2 +- lib/private/Server.php | 8 ++++ lib/public/Share/IManager.php | 5 ++- lib/public/Share/IShareHelper.php | 44 +++++++++++++++++++ 5 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 lib/public/Share/IShareHelper.php diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 4fb13b09ae..45a971f125 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -57,6 +57,7 @@ use OCP\IRequest; use OCP\IServerContainer; use OCP\IUserSession; use OCP\RichObjectStrings\IValidator; +use OCP\Share\IShareHelper; use OCP\Util; class DIContainer extends SimpleContainer implements IAppContainer { @@ -169,6 +170,10 @@ class DIContainer extends SimpleContainer implements IAppContainer { ); }); + $this->registerService(IShareHelper::class, function (SimpleContainer $c) { + return $c->query(IShareHelper::class); + }); + /** * App Framework APIs diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index 329fb12b83..3b6a87ef51 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -101,7 +101,7 @@ class File implements \OCP\Encryption\IFile { // Find out who, if anyone, is sharing the file if ($file !== null) { $resultForFile = $this->shareManager->getAccessList($file, false); - $userIds = \array_merge($userIds, $resultForFile['users']); + $userIds = array_merge($userIds, $resultForFile['users']); $public = $resultForFile['public'] || $resultForFile['remote'] || $public; } diff --git a/lib/private/Server.php b/lib/private/Server.php index c38675934c..068c89dd8e 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -93,6 +93,7 @@ use OC\Security\CredentialsManager; use OC\Security\SecureRandom; use OC\Security\TrustedDomainHelper; use OC\Session\CryptoWrapper; +use OC\Share20\ShareHelper; use OC\Tagging\TagMapper; use OCA\Theming\ThemingDefaults; use OCP\App\IAppManager; @@ -106,6 +107,7 @@ use OCP\IServerContainer; use OCP\ITempManager; use OCP\RichObjectStrings\IValidator; use OCP\Security\IContentSecurityPolicyManager; +use OCP\Share\IShareHelper; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -977,6 +979,12 @@ class Server extends ServerContainer implements IServerContainer { $this->registerService(\OCP\ISession::class, function(SimpleContainer $c) { return $c->query(\OCP\IUserSession::class)->getSession(); }); + + $this->registerService(IShareHelper::class, function(Server $c) { + return new ShareHelper( + $c->getLazyRootFolder() + ); + }); } /** diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 45e0e9d80c..26c3f2b42e 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -216,10 +216,11 @@ interface IManager { * * @param \OCP\Files\Node $path * @param bool $recursive Should we check all parent folders as well + * @param bool $currentAccess Should the user have currently access to the file * @return array - * @since 9.2.0 + * @since 12 */ - public function getAccessList(\OCP\Files\Node $path, $recursive = true); + public function getAccessList(\OCP\Files\Node $path, $recursive = true, $currentAccess = false); /** * Instantiates a new share object. This is to be passed to diff --git a/lib/public/Share/IShareHelper.php b/lib/public/Share/IShareHelper.php new file mode 100644 index 0000000000..5e99f86832 --- /dev/null +++ b/lib/public/Share/IShareHelper.php @@ -0,0 +1,44 @@ + + * + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCP\Share; + +use OCP\Files\Node; + +/** + * Interface IShareHelper + * + * @package OCP\Share + * @since 12 + */ +interface IShareHelper { + + /** + * If a user has access to a file + * + * @param Node $node + * @param array $users Array of userIds + * @return array Mapping $uid to an array of nodes + * @since 12 + */ + public function getPathsForAccessList(Node $node, $users); +} From 91e650791de6dbb320e5163f626c754d42e0bc35 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Mar 2017 16:06:31 +0200 Subject: [PATCH 11/29] Return the paths for the users without setting them all up Signed-off-by: Joas Schilling --- .../lib/FederatedShareProvider.php | 13 +- lib/private/Encryption/File.php | 4 +- lib/private/Share20/DefaultShareProvider.php | 54 ++++++-- lib/private/Share20/Manager.php | 16 ++- lib/private/Share20/ShareHelper.php | 122 ++++++++++++++---- lib/public/Share/IManager.php | 5 +- lib/public/Share/IShareHelper.php | 7 +- lib/public/Share/IShareProvider.php | 7 +- 8 files changed, 172 insertions(+), 56 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 29aa594df5..9e8d9fd9e4 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -983,18 +983,23 @@ class FederatedShareProvider implements IShareProvider { } $qb = $this->dbConnection->getQueryBuilder(); - $qb->select('share_with') + $qb->select('share_with', 'file_source', 'file_target') ->from('share') ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_REMOTE))) ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) ->andWhere($qb->expr()->orX( $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )) - ->setMaxResults(1); + )); $cursor = $qb->execute(); - $remote = $cursor->fetch() !== false; + $remote = []; + while ($row = $cursor->fetch()) { + $remote[$row['share_with']] = [ + 'node_id' => $row['file_source'], + 'node_path' => $row['file_target'], + ]; + } $cursor->closeCursor(); return ['remote' => $remote]; diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index 3b6a87ef51..a1fd8e300d 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -95,14 +95,14 @@ class File implements \OCP\Encryption\IFile { $this->cache[$parent] = $resultForParents; } $userIds = \array_merge($userIds, $resultForParents['users']); - $public = $resultForParents['public'] || $resultForParents['remote']; + $public = $resultForParents['public'] || !empty($resultForParents['remote']); // Find out who, if anyone, is sharing the file if ($file !== null) { $resultForFile = $this->shareManager->getAccessList($file, false); $userIds = array_merge($userIds, $resultForFile['users']); - $public = $resultForFile['public'] || $resultForFile['remote'] || $public; + $public = $resultForFile['public'] || !empty($resultForFile['remote']) || $public; } // check if it is a group mount diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index b8f46a1f4d..49a756f2a1 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1092,7 +1092,7 @@ class DefaultShareProvider implements IShareProvider { $or->add($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USERGROUP))); } - $qb->select('share_type', 'share_with') + $qb->select('id', 'parent', 'share_type', 'share_with', 'file_source', 'file_target', 'permissions') ->from('share') ->where( $or @@ -1110,7 +1110,8 @@ class DefaultShareProvider implements IShareProvider { $type = (int)$row['share_type']; if ($type === \OCP\Share::SHARE_TYPE_USER) { $uid = $row['share_with']; - $users[$uid] = isset($users[$uid]) ? $users[$uid] + 1 : 1; + $users[$uid] = isset($users[$uid]) ? $users[$uid] : []; + $users[$uid][$row['id']] = $row; } else if ($type === \OCP\Share::SHARE_TYPE_GROUP) { $gid = $row['share_with']; $group = $this->groupManager->get($gid); @@ -1121,23 +1122,60 @@ class DefaultShareProvider implements IShareProvider { $userList = $group->getUsers(); foreach ($userList as $user) { - $users[$user->getUID()] = isset($users[$user->getUID()]) ? $users[$user->getUID()] + 1 : 1; + $uid = $user->getUID(); + $users[$uid] = isset($users[$uid]) ? $users[$uid] : []; + $users[$uid][$row['id']] = $row; } } else if ($type === \OCP\Share::SHARE_TYPE_LINK) { $link = true; } else if ($type === self::SHARE_TYPE_USERGROUP) { if ($currentAccess === true) { $uid = $row['share_with']; - $users[$uid] = isset($users[$uid]) ? $users[$uid] - 1 : -1; + $users[$uid] = isset($users[$uid]) ? $users[$uid] : []; + $users[$uid][$row['id']] = $row; } } } $cursor->closeCursor(); - $users = array_filter($users, function($count) { - return $count > 0; - }); + $users = array_map([$this, 'filterSharesOfUser'], $users); - return ['users' => array_keys($users), 'public' => $link]; + return ['users' => $users, 'public' => $link]; + } + + /** + * For each user the path with the fewest slashes is returned + * @param array $shares + * @return array + */ + protected function filterSharesOfUser(array $shares) { + // Group shares when the user has a share exception + foreach ($shares as $id => $share) { + $type = (int) $share['share_type']; + $permissions = (int) $share['permissions']; + + if ($type === self::SHARE_TYPE_USERGROUP) { + unset($shares[$share['parent']]); + + if ($permissions === 0) { + unset($shares[$id]); + } + } + } + + $best = []; + $bestDepth = 0; + foreach ($shares as $id => $share) { + $depth = substr_count($share['file_target'], '/'); + if (empty($best) || $depth < $bestDepth) { + $bestDepth = $depth; + $best = [ + 'node_id' => $share['file_source'], + 'node_path' => $share['file_target'], + ]; + } + } + + return $best; } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index f1ce66f008..91fcb6af8f 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1191,9 +1191,10 @@ class Manager implements IManager { * * Then the access list will to '/folder1/folder2/fileA' is: * [ - * users => ['user1', 'user2', 'user4'], + * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], * public => bool - * remote => bool + * mail => bool * ] * * This is required for encryption/activity @@ -1206,7 +1207,7 @@ class Manager implements IManager { public function getAccessList(\OCP\Files\Node $path, $recursive = true, $currentAccess = false) { $owner = $path->getOwner()->getUID(); - $al = ['users' => [], 'public' => false, 'remote' => false]; + $al = ['users' => [], 'remote' => [], 'public' => false]; if (!$this->userManager->userExists($owner)) { return $al; } @@ -1222,7 +1223,12 @@ class Manager implements IManager { /** @var Node[] $nodes */ $nodes = []; - $al['users'][] = $owner; + $ownerPath = $path->getPath(); + list(,,,$ownerPath) = explode('/', $ownerPath, 4); + $al['users'][$owner] = [ + 'node_id' => $path->getId(), + 'node_path' => '/' . $ownerPath, + ]; // Collect all the shares while ($path->getPath() !== $userFolder->getPath()) { @@ -1249,8 +1255,6 @@ class Manager implements IManager { } } - $al['users'] = array_unique($al['users']); - return $al; } diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index a7d46457cd..93b32cef3c 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -22,43 +22,113 @@ */ namespace OC\Share20; -use OCP\Files\IRootFolder; use OCP\Files\Node; -use OCP\Files\NotFoundException; +use OCP\Share\IManager; +use OCP\Share\IShareHelper; -class ShareHelper { - /** @var IRootFolder */ - private $rootFolder; +class ShareHelper implements IShareHelper { - public function __construct(IRootFolder $rootFolder) { - $this->rootFolder = $rootFolder; + /** @var IManager */ + private $shareManager; + + public function __construct(IManager $shareManager) { + $this->shareManager = $shareManager; } /** - * If a user has access to a file - * * @param Node $node - * @param array $users Array of userIds - * @return array Mapping $uid to an array of nodes + * @return array [ users => [Mapping $uid => $path], remotes => [Mapping $cloudId => $path]] */ - public function getPathsForAccessList(Node $node, $users) { - $result = []; + public function getPathsForAccessList(Node $node) { + $result = [ + 'users' => [], + 'remotes' => [], + ]; - foreach ($users as $user) { - try { - $userFolder = $this->rootFolder->getUserFolder($user); - } catch (NotFoundException $e) { - continue; - } - - $nodes = $userFolder->getById($node->getId()); - if ($nodes === []) { - continue; - } - - $result[$user] = $nodes; + $accessList = $this->shareManager->getAccessList($node, true, true); + if (!empty($accessList['users'])) { + $result['users'] = $this->getPathsForUsers($node, $accessList['users']); + } + if (!empty($accessList['remote'])) { + $result['remotes'] = $this->getPathsForRemotes($node, $accessList['remote']); } return $result; } + + protected function getPathsForUsers(Node $node, array $users) { + $byId = $results = []; + foreach ($users as $uid => $info) { + if (!isset($byId[$info['node_id']])) { + $byId[$info['node_id']] = []; + } + $byId[$info['node_id']][$uid] = $info['node_path']; + } + + if (isset($byId[$node->getId()])) { + foreach ($byId[$node->getId()] as $uid => $path) { + $results[$uid] = $path; + } + unset($byId[$node->getId()]); + } + + if (empty($byId)) { + return $results; + } + + $item = $node; + $appendix = '/' . $node->getName(); + while (!empty($byId)) { + $item = $item->getParent(); + + if (!empty($byId[$item->getId()])) { + foreach ($byId[$item->getId()] as $uid => $path) { + $results[$uid] = $path . $appendix; + } + unset($byId[$item->getId()]); + } + + $appendix = '/' . $item->getName() . $appendix; + } + + return $results; + } + + protected function getPathsForRemotes(Node $node, array $remotes) { + $byId = $results = []; + foreach ($remotes as $cloudId => $info) { + if (!isset($byId[$info['node_id']])) { + $byId[$info['node_id']] = []; + } + $byId[$info['node_id']][$cloudId] = $info['node_path']; + } + + if (isset($byId[$node->getId()])) { + foreach ($byId[$node->getId()] as $cloudId => $_) { + $results[$cloudId] = '/' . $node->getName(); + } + unset($byId[$node->getId()]); + } + + if (empty($byId)) { + return $results; + } + + $item = $node; + $path = '/' . $node->getName(); + while (!empty($byId)) { + $item = $item->getParent(); + + if (!empty($byId[$item->getId()])) { + foreach ($byId[$item->getId()] as $uid => $_) { + $results[$uid] = $path; + } + unset($byId[$item->getId()]); + } + + $path = '/' . $item->getName() . $path; + } + + return $results; + } } diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 26c3f2b42e..1cbb27cb61 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -207,9 +207,10 @@ interface IManager { * * Then the access list will to '/folder1/folder2/fileA' is: * [ - * users => ['user1', 'user2', 'user4'], + * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], * public => bool - * remote => bool + * mail => bool * ] * * This is required for encryption/activity diff --git a/lib/public/Share/IShareHelper.php b/lib/public/Share/IShareHelper.php index 5e99f86832..01202cf477 100644 --- a/lib/public/Share/IShareHelper.php +++ b/lib/public/Share/IShareHelper.php @@ -33,12 +33,9 @@ use OCP\Files\Node; interface IShareHelper { /** - * If a user has access to a file - * * @param Node $node - * @param array $users Array of userIds - * @return array Mapping $uid to an array of nodes + * @return array [ users => [Mapping $uid => $path], remotes => [Mapping $cloudId => $path]] * @since 12 */ - public function getPathsForAccessList(Node $node, $users); + public function getPathsForAccessList(Node $node); } diff --git a/lib/public/Share/IShareProvider.php b/lib/public/Share/IShareProvider.php index 2533e81abf..fba432f31e 100644 --- a/lib/public/Share/IShareProvider.php +++ b/lib/public/Share/IShareProvider.php @@ -196,14 +196,15 @@ interface IShareProvider { * Return will look like: * * [ - * users => ['user1', 'user2', 'user4'], + * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * mail => bool * public => bool - * remote => bool * ] * * @param Node[] $nodes The list of nodes to get access for * @param bool $currentAccess If current access is required (like for removed shares that might get revived later) - * @return string[] + * @return array * @since 12 */ public function getAccessList($nodes, $currentAccess); From cf7c3209490c60f03fc90b6a71d5d1e00a9d802e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 27 Mar 2017 23:39:20 +0200 Subject: [PATCH 12/29] Also return the token Signed-off-by: Joas Schilling --- lib/private/Share20/ShareHelper.php | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index 93b32cef3c..b5640d0455 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -100,12 +100,15 @@ class ShareHelper implements IShareHelper { if (!isset($byId[$info['node_id']])) { $byId[$info['node_id']] = []; } - $byId[$info['node_id']][$cloudId] = $info['node_path']; + $byId[$info['node_id']][$cloudId] = $info['token']; } if (isset($byId[$node->getId()])) { - foreach ($byId[$node->getId()] as $cloudId => $_) { - $results[$cloudId] = '/' . $node->getName(); + foreach ($byId[$node->getId()] as $cloudId => $token) { + $results[$cloudId] = [ + 'node_path' => '/' . $node->getName(), + 'token' => $token, + ]; } unset($byId[$node->getId()]); } @@ -120,8 +123,11 @@ class ShareHelper implements IShareHelper { $item = $item->getParent(); if (!empty($byId[$item->getId()])) { - foreach ($byId[$item->getId()] as $uid => $_) { - $results[$uid] = $path; + foreach ($byId[$item->getId()] as $uid => $token) { + $results[$uid] = [ + 'node_path' => $path, + 'token' => $token, + ]; } unset($byId[$item->getId()]); } From 4bcb7d88b5cbb8d2c83176062dc76c6213a13c48 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Mar 2017 10:51:44 +0200 Subject: [PATCH 13/29] Return the token as well Signed-off-by: Joas Schilling --- apps/federatedfilesharing/lib/FederatedShareProvider.php | 3 ++- lib/private/Encryption/File.php | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 9e8d9fd9e4..bf50206a73 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -983,7 +983,7 @@ class FederatedShareProvider implements IShareProvider { } $qb = $this->dbConnection->getQueryBuilder(); - $qb->select('share_with', 'file_source', 'file_target') + $qb->select('share_with', 'token', 'file_source', 'file_target') ->from('share') ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_REMOTE))) ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) @@ -998,6 +998,7 @@ class FederatedShareProvider implements IShareProvider { $remote[$row['share_with']] = [ 'node_id' => $row['file_source'], 'node_path' => $row['file_target'], + 'token' => $row['token'], ]; } $cursor->closeCursor(); diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index a1fd8e300d..f6fd3382cb 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -94,7 +94,7 @@ class File implements \OCP\Encryption\IFile { $resultForParents = $this->shareManager->getAccessList($parentNode); $this->cache[$parent] = $resultForParents; } - $userIds = \array_merge($userIds, $resultForParents['users']); + $userIds = array_merge($userIds, $resultForParents['users']); $public = $resultForParents['public'] || !empty($resultForParents['remote']); From 3c1365c0d12a626e8c2b8e1ab35b105f4b88ad2e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 28 Mar 2017 17:06:26 +0200 Subject: [PATCH 14/29] Fix returned paths for remote shares Signed-off-by: Joas Schilling --- .../lib/FederatedShareProvider.php | 3 +- lib/private/Share20/ShareHelper.php | 29 ++++++------------- lib/public/Share/IManager.php | 2 +- lib/public/Share/IShareHelper.php | 2 +- lib/public/Share/IShareProvider.php | 2 +- 5 files changed, 13 insertions(+), 25 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index bf50206a73..617db4a45a 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -983,7 +983,7 @@ class FederatedShareProvider implements IShareProvider { } $qb = $this->dbConnection->getQueryBuilder(); - $qb->select('share_with', 'token', 'file_source', 'file_target') + $qb->select('share_with', 'token', 'file_source') ->from('share') ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(\OCP\Share::SHARE_TYPE_REMOTE))) ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) @@ -997,7 +997,6 @@ class FederatedShareProvider implements IShareProvider { while ($row = $cursor->fetch()) { $remote[$row['share_with']] = [ 'node_id' => $row['file_source'], - 'node_path' => $row['file_target'], 'token' => $row['token'], ]; } diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index b5640d0455..ba3f64dbc1 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -37,7 +37,7 @@ class ShareHelper implements IShareHelper { /** * @param Node $node - * @return array [ users => [Mapping $uid => $path], remotes => [Mapping $cloudId => $path]] + * @return array [ users => [Mapping $uid => $pathForUser], remotes => [Mapping $cloudId => $pathToMountRoot]] */ public function getPathsForAccessList(Node $node) { $result = [ @@ -103,26 +103,10 @@ class ShareHelper implements IShareHelper { $byId[$info['node_id']][$cloudId] = $info['token']; } - if (isset($byId[$node->getId()])) { - foreach ($byId[$node->getId()] as $cloudId => $token) { - $results[$cloudId] = [ - 'node_path' => '/' . $node->getName(), - 'token' => $token, - ]; - } - unset($byId[$node->getId()]); - } - - if (empty($byId)) { - return $results; - } - $item = $node; - $path = '/' . $node->getName(); while (!empty($byId)) { - $item = $item->getParent(); - if (!empty($byId[$item->getId()])) { + $path = $this->getMountedPath($item); foreach ($byId[$item->getId()] as $uid => $token) { $results[$uid] = [ 'node_path' => $path, @@ -131,10 +115,15 @@ class ShareHelper implements IShareHelper { } unset($byId[$item->getId()]); } - - $path = '/' . $item->getName() . $path; + $item = $item->getParent(); } return $results; } + + protected function getMountedPath(Node $node) { + $path = $node->getPath(); + $sections = explode('/', $path, 4); + return '/' . $sections[3]; + } } diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 1cbb27cb61..5f02e1d980 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -208,7 +208,7 @@ interface IManager { * Then the access list will to '/folder1/folder2/fileA' is: * [ * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], - * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * remote => ['user1' => ['node_id' => 42, 'token' => 'ShareToken'], 'user2' => [...]], * public => bool * mail => bool * ] diff --git a/lib/public/Share/IShareHelper.php b/lib/public/Share/IShareHelper.php index 01202cf477..4ec62830c5 100644 --- a/lib/public/Share/IShareHelper.php +++ b/lib/public/Share/IShareHelper.php @@ -34,7 +34,7 @@ interface IShareHelper { /** * @param Node $node - * @return array [ users => [Mapping $uid => $path], remotes => [Mapping $cloudId => $path]] + * @return array [ users => [Mapping $uid => $pathForUser], remotes => [Mapping $cloudId => $pathToMountRoot]] * @since 12 */ public function getPathsForAccessList(Node $node); diff --git a/lib/public/Share/IShareProvider.php b/lib/public/Share/IShareProvider.php index fba432f31e..bac1a5d87f 100644 --- a/lib/public/Share/IShareProvider.php +++ b/lib/public/Share/IShareProvider.php @@ -197,7 +197,7 @@ interface IShareProvider { * * [ * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], - * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * remote => ['user1' => ['node_id' => 42, 'token' => 'ShareToken'], 'user2' => [...]], * mail => bool * public => bool * ] From 5df727d4ad3a735d35a5b13502c93791c84fa948 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 10 Apr 2017 15:40:30 +0200 Subject: [PATCH 15/29] Fix federated file sharing Signed-off-by: Joas Schilling --- .../tests/FederatedShareProviderTest.php | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index 7d571b2082..788ae47fde 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -53,7 +53,7 @@ class FederatedShareProviderTest extends \Test\TestCase { protected $addressHandler; /** @var Notifications | \PHPUnit_Framework_MockObject_MockObject */ protected $notifications; - /** @var TokenHandler */ + /** @var TokenHandler|\PHPUnit_Framework_MockObject_MockObject */ protected $tokenHandler; /** @var IL10N */ protected $l; @@ -798,8 +798,10 @@ class FederatedShareProviderTest extends \Test\TestCase { $folder1 = $rootFolder->getUserFolder($u1->getUID())->newFolder('foo'); $file1 = $folder1->newFile('bar1'); - $this->tokenHandler->method('generateToken')->willReturn('token'); - $this->notifications + $this->tokenHandler->expects($this->exactly(2)) + ->method('generateToken') + ->willReturnOnConsecutiveCalls('token1', 'token2'); + $this->notifications->expects($this->atLeastOnce()) ->method('sendRemoteShare') ->willReturn(true); @@ -811,9 +813,26 @@ class FederatedShareProviderTest extends \Test\TestCase { ->setNode($file1); $this->provider->create($share1); + $share2 = $this->shareManager->newShare(); + $share2->setSharedWith('foobar@localhost') + ->setSharedBy($u1->getUID()) + ->setShareOwner($u1->getUID()) + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file1); + $this->provider->create($share2); + $result = $this->provider->getAccessList([$file1], true); - $this->assertSame(['remote' => true], $result); + $this->assertEquals(['remote' => [ + 'user@server.com' => [ + 'token' => 'token1', + 'node_id' => $file1->getId(), + ], + 'foobar@localhost' => [ + 'token' => 'token2', + 'node_id' => $file1->getId(), + ], + ]], $result); $u1->delete(); } } From 2fcf334c6aeafaf13f3f1eee6479db93cec2e576 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 10 Apr 2017 16:10:34 +0200 Subject: [PATCH 16/29] Fix tests for ShareHelper Signed-off-by: Joas Schilling --- lib/private/Share20/ShareHelper.php | 15 ++++ tests/lib/Share20/ShareHelperTests.php | 110 +++++++++++++++---------- 2 files changed, 83 insertions(+), 42 deletions(-) diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index ba3f64dbc1..9099200501 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -56,6 +56,11 @@ class ShareHelper implements IShareHelper { return $result; } + /** + * @param Node $node + * @param array[] $users + * @return array + */ protected function getPathsForUsers(Node $node, array $users) { $byId = $results = []; foreach ($users as $uid => $info) { @@ -79,6 +84,7 @@ class ShareHelper implements IShareHelper { $item = $node; $appendix = '/' . $node->getName(); while (!empty($byId)) { + /** @var Node $item */ $item = $item->getParent(); if (!empty($byId[$item->getId()])) { @@ -94,6 +100,11 @@ class ShareHelper implements IShareHelper { return $results; } + /** + * @param Node $node + * @param array[] $remotes + * @return array + */ protected function getPathsForRemotes(Node $node, array $remotes) { $byId = $results = []; foreach ($remotes as $cloudId => $info) { @@ -121,6 +132,10 @@ class ShareHelper implements IShareHelper { return $results; } + /** + * @param Node $node + * @return string + */ protected function getMountedPath(Node $node) { $path = $node->getPath(); $sections = explode('/', $path, 4); diff --git a/tests/lib/Share20/ShareHelperTests.php b/tests/lib/Share20/ShareHelperTests.php index b6c737cf44..a6ca581dbb 100644 --- a/tests/lib/Share20/ShareHelperTests.php +++ b/tests/lib/Share20/ShareHelperTests.php @@ -23,16 +23,14 @@ namespace Test\Share20; use OC\Share20\ShareHelper; -use OCP\Files\Folder; -use OCP\Files\IRootFolder; use OCP\Files\Node; -use OCP\Files\NotFoundException; +use OCP\Share\IManager; use Test\TestCase; class ShareHelperTests extends TestCase { - /** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */ - private $rootFolder; + /** @var IManager|\PHPUnit_Framework_MockObject_MockObject */ + private $manager; /** @var ShareHelper */ private $helper; @@ -40,55 +38,83 @@ class ShareHelperTests extends TestCase { public function setUp() { parent::setUp(); - $this->rootFolder = $this->createMock(IRootFolder::class); + $this->manager = $this->createMock(IManager::class); - $this->helper = new ShareHelper($this->rootFolder); + $this->helper = new ShareHelper($this->manager); + } + + public function dataGetPathsForAccessList() { + return [ + [[], [], false, [], [], false, [ + 'users' => [], + 'remotes' => [], + ]], + [['user1', 'user2'], ['user1' => 'foo', 'user2' => 'bar'], true, [], [], false, [ + 'users' => ['user1' => 'foo', 'user2' => 'bar'], + 'remotes' => [], + ]], + [[], [], false, ['remote1', 'remote2'], ['remote1' => 'qwe', 'remote2' => 'rtz'], true, [ + 'users' => [], + 'remotes' => ['remote1' => 'qwe', 'remote2' => 'rtz'], + ]], + [['user1', 'user2'], ['user1' => 'foo', 'user2' => 'bar'], true, ['remote1', 'remote2'], ['remote1' => 'qwe', 'remote2' => 'rtz'], true, [ + 'users' => ['user1' => 'foo', 'user2' => 'bar'], + 'remotes' => ['remote1' => 'qwe', 'remote2' => 'rtz'], + ]], + ]; } /** - * uid1 - Exists with valid node - * uid2 - Does not exist - * uid3 - Exists but no valid node - * uid4 - Exists with valid node + * @dataProvider dataGetPathsForAccessList */ - public function testGetPathsForAccessList() { - /** @var Folder[]|\PHPUnit_Framework_MockObject_MockObject[] $userFolder */ - $userFolder = [ - 'uid1' => $this->createMock(Folder::class), - 'uid3' => $this->createMock(Folder::class), - 'uid4' => $this->createMock(Folder::class), - ]; - - $this->rootFolder->method('getUserFolder') - ->willReturnCallback(function($uid) use ($userFolder) { - if (isset($userFolder[$uid])) { - return $userFolder[$uid]; - } - throw new NotFoundException(); - }); + public function testGetPathsForAccessList(array $userList, array $userMap, $resolveUsers, array $remoteList, array $remoteMap, $resolveRemotes, array $expected) { + $this->manager->expects($this->once()) + ->method('getAccessList') + ->willReturn([ + 'users' => $userList, + 'remote' => $remoteList, + ]); /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ $node = $this->createMock(Node::class); - $node->method('getId') - ->willReturn(42); + /** @var ShareHelper|\PHPUnit_Framework_MockObject_MockObject $helper */ + $helper = $this->getMockBuilder(ShareHelper::class) + ->setConstructorArgs([$this->manager]) + ->setMethods(['getPathsForUsers', 'getPathsForRemotes']) + ->getMock(); - $userFolder['uid1']->method('getById') - ->with(42) - ->willReturn([$node]); - $userFolder['uid3']->method('getById') - ->with(42) - ->willReturn([]); - $userFolder['uid4']->method('getById') - ->with(42) - ->willReturn([$node]); + $helper->expects($resolveUsers ? $this->once() : $this->never()) + ->method('getPathsForUsers') + ->with($node, $userList) + ->willReturn($userMap); - $expects = [ - 'uid1' => [$node], - 'uid4' => [$node], + $helper->expects($resolveRemotes ? $this->once() : $this->never()) + ->method('getPathsForRemotes') + ->with($node, $remoteList) + ->willReturn($remoteMap); + + $this->assertSame($expected, $helper->getPathsForAccessList($node)); + } + + public function dataGetMountedPath() { + return [ + ['/admin/files/foobar', '/foobar'], + ['/admin/files/foo/bar', '/foo/bar'], ]; + } - $result = $this->helper->getPathsForAccessList($node, ['uid1', 'uid2', 'uid3', 'uid4']); + /** + * @dataProvider dataGetMountedPath + * @param string $path + * @param string $expected + */ + public function testGetMountedPath($path, $expected) { + /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->createMock(Node::class); + $node->expects($this->once()) + ->method('getPath') + ->willReturn($path); - $this->assertSame($expects, $result); + $this->assertSame($expected, self::invokePrivate($this->helper, 'getMountedPath', [$node])); } } From 5b57bb955b6f5289bf80aac7ade760d76ce033c6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 10 Apr 2017 16:22:12 +0200 Subject: [PATCH 17/29] Fix default share provider Signed-off-by: Joas Schilling --- lib/private/Share20/DefaultShareProvider.php | 13 ++++++------ .../lib/Share20/DefaultShareProviderTest.php | 20 +++++++++---------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 49a756f2a1..b28dc56f85 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1128,17 +1128,18 @@ class DefaultShareProvider implements IShareProvider { } } else if ($type === \OCP\Share::SHARE_TYPE_LINK) { $link = true; - } else if ($type === self::SHARE_TYPE_USERGROUP) { - if ($currentAccess === true) { - $uid = $row['share_with']; - $users[$uid] = isset($users[$uid]) ? $users[$uid] : []; - $users[$uid][$row['id']] = $row; - } + } else if ($type === self::SHARE_TYPE_USERGROUP && $currentAccess === true) { + $uid = $row['share_with']; + $users[$uid] = isset($users[$uid]) ? $users[$uid] : []; + $users[$uid][$row['id']] = $row; } } $cursor->closeCursor(); $users = array_map([$this, 'filterSharesOfUser'], $users); + if ($currentAccess === true) { + $users = array_filter($users); + } return ['users' => $users, 'public' => $link]; } diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index a7f5555efa..6c351f1a6d 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -1545,9 +1545,9 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->assertEquals(1, $stmt); $id = $qb->getLastInsertId(); - $user1 = $this->getMock('\OCP\IUser'); + $user1 = $this->createMock(IUser::class); $user1->method('getUID')->willReturn('user1'); - $user2 = $this->getMock('\OCP\IUser'); + $user2 = $this->createMock(IUser::class); $user2->method('getUID')->willReturn('user2'); $this->userManager->method('get')->will($this->returnValueMap([ ['user1', $user1], @@ -1556,7 +1556,7 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->groupManager->method('get')->with('group')->willReturn(null); - $file = $this->getMock('\OCP\Files\File'); + $file = $this->createMock(File::class); $file->method('getId')->willReturn(1); $this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf()); @@ -2503,10 +2503,10 @@ class DefaultShareProviderTest extends \Test\TestCase { $result = $provider->getAccessList([$folder1, $folder2, $file1], false); $this->assertCount(4, $result['users']); - $this->assertContains('testShare2', $result['users']); - $this->assertContains('testShare3', $result['users']); - $this->assertContains('testShare4', $result['users']); - $this->assertContains('testShare5', $result['users']); + $this->assertArrayHasKey('testShare2', $result['users']); + $this->assertArrayHasKey('testShare3', $result['users']); + $this->assertArrayHasKey('testShare4', $result['users']); + $this->assertArrayHasKey('testShare5', $result['users']); $this->assertTrue($result['public']); $provider->delete($share1); @@ -2590,9 +2590,9 @@ class DefaultShareProviderTest extends \Test\TestCase { $result = $provider->getAccessList([$folder1, $folder2, $file1], true); $this->assertCount(3, $result['users']); - $this->assertContains('testShare2', $result['users']); - $this->assertContains('testShare3', $result['users']); - $this->assertContains('testShare5', $result['users']); + $this->assertArrayHasKey('testShare2', $result['users']); + $this->assertArrayHasKey('testShare3', $result['users']); + $this->assertArrayHasKey('testShare5', $result['users']); $this->assertTrue($result['public']); $provider->delete($share1); From 4eeb194ae5f7d52e40a2be318730f0c44eb19ca8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 10 Apr 2017 16:53:58 +0200 Subject: [PATCH 18/29] Fix share manager test Signed-off-by: Joas Schilling --- tests/lib/Share20/ManagerTest.php | 37 ++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index c1a0eaf5bb..42308a9d6a 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -1022,12 +1022,12 @@ class ManagerTest extends \Test\TestCase { public function testUserCreateChecksIdenticalPathSharedViaDeletedGroup() { $share = $this->manager->newShare(); - $sharedWith = $this->getMock('\OCP\IUser'); + $sharedWith = $this->createMock(IUser::class); $sharedWith->method('getUID')->willReturn('sharedWith'); $this->userManager->method('get')->with('sharedWith')->willReturn($sharedWith); - $path = $this->getMock('\OCP\Files\Node'); + $path = $this->createMock(Node::class); $share->setSharedWith('sharedWith') ->setNode($path) @@ -1136,7 +1136,7 @@ class ManagerTest extends \Test\TestCase { public function testGroupCreateChecksShareWithGroupMembersOnlyNullGroup() { $share = $this->manager->newShare(); - $user = $this->getMock('\OCP\IUser'); + $user = $this->createMock(IUser::class); $share->setSharedBy('user')->setSharedWith('group'); $this->groupManager->method('get')->with('group')->willReturn(null); @@ -2611,7 +2611,7 @@ class ManagerTest extends \Test\TestCase { $share->setShareType(\OCP\Share::SHARE_TYPE_GROUP); $share->setSharedWith('shareWith'); - $recipient = $this->getMock('\OCP\IUser'); + $recipient = $this->createMock(IUser::class); $this->groupManager->method('get')->with('shareWith')->willReturn(null); $this->userManager->method('get')->with('recipient')->willReturn($recipient); @@ -2779,6 +2779,8 @@ class ManagerTest extends \Test\TestCase { ->willReturn($folder); $file->method('getPath') ->willReturn('/owner/files/folder/file'); + $file->method('getId') + ->willReturn(23); $folder->method('getParent') ->willReturn($userFolder); $folder->method('getPath') @@ -2800,9 +2802,9 @@ class ManagerTest extends \Test\TestCase { ) ->willReturn([ 'users' => [ - 'user1', - 'user2', - 'user3', + 'user1' => [], + 'user2' => [], + 'user3' => [], ], 'public' => true, ]); @@ -2814,11 +2816,13 @@ class ManagerTest extends \Test\TestCase { ) ->willReturn([ 'users' => [ - 'user3', - 'user4', - 'user5', + 'user3' => [], + 'user4' => [], + 'user5' => [], + ], + 'remote' => [ + 'remote1', ], - 'remote' => true, ]); $this->rootFolder->method('getUserFolder') @@ -2826,9 +2830,16 @@ class ManagerTest extends \Test\TestCase { ->willReturn($userFolder); $expected = [ - 'users' => ['owner', 'user1', 'user2', 'user3', 'user4', 'user5'], + 'users' => [ + 'owner' => [ + 'node_id' => 23, + 'node_path' => '/folder/file' + ] + , 'user1' => [], 'user2' => [], 'user3' => [], 'user4' => [], 'user5' => []], + 'remote' => [ + 'remote1', + ], 'public' => true, - 'remote' => true, ]; $result = $manager->getAccessList($node, true, true); From 6c23a5fa3511e5212c83e8407fa18b12ec975f97 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 12:16:10 +0200 Subject: [PATCH 19/29] Add unit tests for sharebymail provider Signed-off-by: Joas Schilling --- .../tests/ShareByMailProviderTest.php | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 288ebb4bb4..1b215016ee 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -664,4 +664,59 @@ class ShareByMailProviderTest extends TestCase { $u2->delete(); } + public function testGetAccessList() { + $userManager = \OC::$server->getUserManager(); + $rootFolder = \OC::$server->getRootFolder(); + + $provider = $this->getInstance(['sendMailNotification', 'createActivity']); + + $u1 = $userManager->createUser('testFed', md5(time())); + $u2 = $userManager->createUser('testFed2', md5(time())); + + $folder = $rootFolder->getUserFolder($u1->getUID())->newFolder('foo'); + + $share1 = $this->shareManager->newShare(); + $share1->setSharedWith('user@server.com') + ->setSharedBy($u1->getUID()) + ->setShareOwner($u1->getUID()) + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($folder); + $share1 = $provider->create($share1); + + $share2 = $this->shareManager->newShare(); + $share2->setSharedWith('user2@server.com') + ->setSharedBy($u2->getUID()) + ->setShareOwner($u1->getUID()) + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($folder); + $share2 = $provider->create($share2); + + $accessList = $provider->getAccessList([$folder], true); + $this->assertArrayHasKey('mail', $accessList); + $this->assertTrue($accessList['mail']); + $accessList = $provider->getAccessList([$folder], false); + $this->assertArrayHasKey('mail', $accessList); + $this->assertTrue($accessList['mail']); + + $provider->delete($share2); + + $accessList = $provider->getAccessList([$folder], true); + $this->assertArrayHasKey('mail', $accessList); + $this->assertTrue($accessList['mail']); + $accessList = $provider->getAccessList([$folder], false); + $this->assertArrayHasKey('mail', $accessList); + $this->assertTrue($accessList['mail']); + + $provider->delete($share1); + + $accessList = $provider->getAccessList([$folder], true); + $this->assertArrayHasKey('mail', $accessList); + $this->assertFalse($accessList['mail']); + $accessList = $provider->getAccessList([$folder], false); + $this->assertArrayHasKey('mail', $accessList); + $this->assertFalse($accessList['mail']); + + $u1->delete(); + $u2->delete(); + } } From 629b7c0fc38d9a2c33805bfcf4359e263e4ae68f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 12:40:36 +0200 Subject: [PATCH 20/29] Adjust docs and make !$currentAccess simpler Signed-off-by: Joas Schilling --- .../lib/FederatedShareProvider.php | 10 +++++ .../tests/FederatedShareProviderTest.php | 11 +++++- apps/sharebymail/lib/ShareByMailProvider.php | 3 ++ .../tests/ShareByMailProviderTest.php | 7 ++++ lib/private/Encryption/File.php | 4 +- lib/private/Share20/DefaultShareProvider.php | 4 +- lib/private/Share20/Manager.php | 37 ++++++++++++++----- lib/public/Share/IManager.php | 31 ++++++++++++---- lib/public/Share/IShareProvider.php | 8 +--- .../lib/Share20/DefaultShareProviderTest.php | 16 ++++++-- 10 files changed, 99 insertions(+), 32 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 617db4a45a..34e02391e0 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -976,6 +976,9 @@ class FederatedShareProvider implements IShareProvider { return ($result === 'yes'); } + /** + * @inheritdoc + */ public function getAccessList($nodes, $currentAccess) { $ids = []; foreach ($nodes as $node) { @@ -993,6 +996,13 @@ class FederatedShareProvider implements IShareProvider { )); $cursor = $qb->execute(); + if ($currentAccess === false) { + $remote = $cursor->fetch() !== false; + $cursor->closeCursor(); + + return ['remote' => $remote]; + } + $remote = []; while ($row = $cursor->fetch()) { $remote[$row['share_with']] = [ diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index 788ae47fde..e01e02c83b 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -805,6 +805,12 @@ class FederatedShareProviderTest extends \Test\TestCase { ->method('sendRemoteShare') ->willReturn(true); + $result = $this->provider->getAccessList([$file1], true); + $this->assertEquals(['remote' => []], $result); + + $result = $this->provider->getAccessList([$file1], false); + $this->assertEquals(['remote' => false], $result); + $share1 = $this->shareManager->newShare(); $share1->setSharedWith('user@server.com') ->setSharedBy($u1->getUID()) @@ -822,7 +828,6 @@ class FederatedShareProviderTest extends \Test\TestCase { $this->provider->create($share2); $result = $this->provider->getAccessList([$file1], true); - $this->assertEquals(['remote' => [ 'user@server.com' => [ 'token' => 'token1', @@ -833,6 +838,10 @@ class FederatedShareProviderTest extends \Test\TestCase { 'node_id' => $file1->getId(), ], ]], $result); + + $result = $this->provider->getAccessList([$file1], false); + $this->assertEquals(['remote' => true], $result); + $u1->delete(); } } diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 14ed12749b..0f4a8dd87b 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -834,6 +834,9 @@ class ShareByMailProvider implements IShareProvider { return $shares; } + /** + * @inheritdoc + */ public function getAccessList($nodes, $currentAccess) { $ids = []; foreach ($nodes as $node) { diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index 1b215016ee..cd0600d3ae 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -675,6 +675,13 @@ class ShareByMailProviderTest extends TestCase { $folder = $rootFolder->getUserFolder($u1->getUID())->newFolder('foo'); + $accessList = $provider->getAccessList([$folder], true); + $this->assertArrayHasKey('mail', $accessList); + $this->assertFalse($accessList['mail']); + $accessList = $provider->getAccessList([$folder], false); + $this->assertArrayHasKey('mail', $accessList); + $this->assertFalse($accessList['mail']); + $share1 = $this->shareManager->newShare(); $share1->setSharedWith('user@server.com') ->setSharedBy($u1->getUID()) diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index f6fd3382cb..2bc0e014f0 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -95,14 +95,14 @@ class File implements \OCP\Encryption\IFile { $this->cache[$parent] = $resultForParents; } $userIds = array_merge($userIds, $resultForParents['users']); - $public = $resultForParents['public'] || !empty($resultForParents['remote']); + $public = $resultForParents['public'] || $resultForParents['remote']; // Find out who, if anyone, is sharing the file if ($file !== null) { $resultForFile = $this->shareManager->getAccessList($file, false); $userIds = array_merge($userIds, $resultForFile['users']); - $public = $resultForFile['public'] || !empty($resultForFile['remote']) || $public; + $public = $resultForFile['public'] || $resultForFile['remote'] || $public; } // check if it is a group mount diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index b28dc56f85..ed3651df9b 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1136,9 +1136,11 @@ class DefaultShareProvider implements IShareProvider { } $cursor->closeCursor(); - $users = array_map([$this, 'filterSharesOfUser'], $users); if ($currentAccess === true) { + $users = array_map([$this, 'filterSharesOfUser'], $users); $users = array_filter($users); + } else { + $users = array_keys($users); } return ['users' => $users, 'public' => $link]; diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 91fcb6af8f..8b670da544 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1181,18 +1181,33 @@ class Manager implements IManager { * * Consider: * -root - * |-folder1 - * |-folder2 - * |-fileA + * |-folder1 (23) + * |-folder2 (32) + * |-fileA (42) * - * fileA is shared with user1 + * fileA is shared with user1 and user1@server1 * folder2 is shared with group2 (user4 is a member of group2) - * folder1 is shared with user2 + * folder1 is shared with user2 (renamed to "folder (1)") and user2@server2 * - * Then the access list will to '/folder1/folder2/fileA' is: + * Then the access list to '/folder1/folder2/fileA' with $currentAccess is: * [ - * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], - * remote => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], + * users => [ + * 'user1' => ['node_id' => 42, 'node_path' => '/fileA'], + * 'user4' => ['node_id' => 32, 'node_path' => '/folder2'], + * 'user2' => ['node_id' => 23, 'node_path' => '/folder (1)'], + * ], + * remote => [ + * 'user1@server1' => ['node_id' => 42, 'token' => 'SeCr3t'], + * 'user2@server2' => ['node_id' => 23, 'token' => 'FooBaR'], + * ], + * public => bool + * mail => bool + * ] + * + * The access list to '/folder1/folder2/fileA' **without** $currentAccess is: + * [ + * users => ['user1', 'user2', 'user4'], + * remote => bool, * public => bool * mail => bool * ] @@ -1207,7 +1222,11 @@ class Manager implements IManager { public function getAccessList(\OCP\Files\Node $path, $recursive = true, $currentAccess = false) { $owner = $path->getOwner()->getUID(); - $al = ['users' => [], 'remote' => [], 'public' => false]; + if ($currentAccess) { + $al = ['users' => [], 'remote' => [], 'public' => false]; + } else { + $al = ['users' => [], 'remote' => false, 'public' => false]; + } if (!$this->userManager->userExists($owner)) { return $al; } diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 5f02e1d980..31f0480306 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -197,18 +197,33 @@ interface IManager { * * Consider: * -root - * |-folder1 - * |-folder2 - * |-fileA + * |-folder1 (23) + * |-folder2 (32) + * |-fileA (42) * - * fileA is shared with user1 + * fileA is shared with user1 and user1@server1 * folder2 is shared with group2 (user4 is a member of group2) - * folder1 is shared with user2 + * folder1 is shared with user2 (renamed to "folder (1)") and user2@server2 * - * Then the access list will to '/folder1/folder2/fileA' is: + * Then the access list to '/folder1/folder2/fileA' with $currentAccess is: * [ - * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], - * remote => ['user1' => ['node_id' => 42, 'token' => 'ShareToken'], 'user2' => [...]], + * users => [ + * 'user1' => ['node_id' => 42, 'node_path' => '/fileA'], + * 'user4' => ['node_id' => 32, 'node_path' => '/folder2'], + * 'user2' => ['node_id' => 23, 'node_path' => '/folder (1)'], + * ], + * remote => [ + * 'user1@server1' => ['node_id' => 42, 'token' => 'SeCr3t'], + * 'user2@server2' => ['node_id' => 23, 'token' => 'FooBaR'], + * ], + * public => bool + * mail => bool + * ] + * + * The access list to '/folder1/folder2/fileA' **without** $currentAccess is: + * [ + * users => ['user1', 'user2', 'user4'], + * remote => bool, * public => bool * mail => bool * ] diff --git a/lib/public/Share/IShareProvider.php b/lib/public/Share/IShareProvider.php index bac1a5d87f..31808206cf 100644 --- a/lib/public/Share/IShareProvider.php +++ b/lib/public/Share/IShareProvider.php @@ -193,14 +193,8 @@ interface IShareProvider { /** * Get the access list to the array of provided nodes. - * Return will look like: * - * [ - * users => ['user1' => ['node_id' => 42, 'node_path' => '/path'], 'user2' => [...]], - * remote => ['user1' => ['node_id' => 42, 'token' => 'ShareToken'], 'user2' => [...]], - * mail => bool - * public => bool - * ] + * @see IManager::getAccessList() for sample docs * * @param Node[] $nodes The list of nodes to get access for * @param bool $currentAccess If current access is required (like for removed shares that might get revived later) diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 6c351f1a6d..0fa8aa3d0c 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -2462,6 +2462,10 @@ class DefaultShareProviderTest extends \Test\TestCase { $folder2 = $folder1->newFolder('baz'); $file1 = $folder2->newFile('bar'); + $result = $provider->getAccessList([$folder1, $folder2, $file1], false); + $this->assertCount(0, $result['users']); + $this->assertFalse($result['public']); + $shareManager = \OC::$server->getShareManager(); $share1 = $shareManager->newShare(); $share1->setNode($folder1) @@ -2503,10 +2507,10 @@ class DefaultShareProviderTest extends \Test\TestCase { $result = $provider->getAccessList([$folder1, $folder2, $file1], false); $this->assertCount(4, $result['users']); - $this->assertArrayHasKey('testShare2', $result['users']); - $this->assertArrayHasKey('testShare3', $result['users']); - $this->assertArrayHasKey('testShare4', $result['users']); - $this->assertArrayHasKey('testShare5', $result['users']); + $this->assertContains('testShare2', $result['users']); + $this->assertContains('testShare3', $result['users']); + $this->assertContains('testShare4', $result['users']); + $this->assertContains('testShare5', $result['users']); $this->assertTrue($result['public']); $provider->delete($share1); @@ -2549,6 +2553,10 @@ class DefaultShareProviderTest extends \Test\TestCase { $folder2 = $folder1->newFolder('baz'); $file1 = $folder2->newFile('bar'); + $result = $provider->getAccessList([$folder1, $folder2, $file1], false); + $this->assertCount(0, $result['users']); + $this->assertFalse($result['public']); + $shareManager = \OC::$server->getShareManager(); $share1 = $shareManager->newShare(); $share1->setNode($folder1) From 7d416ac1ddb158a18ac1473677773417d59e509c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 14:08:35 +0200 Subject: [PATCH 21/29] Activate the test Signed-off-by: Joas Schilling --- tests/lib/Share20/{ShareHelperTests.php => ShareHelperTest.php} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/lib/Share20/{ShareHelperTests.php => ShareHelperTest.php} (98%) diff --git a/tests/lib/Share20/ShareHelperTests.php b/tests/lib/Share20/ShareHelperTest.php similarity index 98% rename from tests/lib/Share20/ShareHelperTests.php rename to tests/lib/Share20/ShareHelperTest.php index a6ca581dbb..e2cd4ce038 100644 --- a/tests/lib/Share20/ShareHelperTests.php +++ b/tests/lib/Share20/ShareHelperTest.php @@ -27,7 +27,7 @@ use OCP\Files\Node; use OCP\Share\IManager; use Test\TestCase; -class ShareHelperTests extends TestCase { +class ShareHelperTest extends TestCase { /** @var IManager|\PHPUnit_Framework_MockObject_MockObject */ private $manager; From 29f2088a7b83a2346ec440d7ffd4e577558fdd23 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 14:16:17 +0200 Subject: [PATCH 22/29] Catch exceptions and use as many results as possible Signed-off-by: Joas Schilling --- lib/private/Share20/ShareHelper.php | 81 +++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 23 deletions(-) diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index 9099200501..6518663446 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -22,7 +22,10 @@ */ namespace OC\Share20; +use OCP\Files\InvalidPathException; use OCP\Files\Node; +use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; use OCP\Share\IManager; use OCP\Share\IShareHelper; @@ -62,7 +65,11 @@ class ShareHelper implements IShareHelper { * @return array */ protected function getPathsForUsers(Node $node, array $users) { - $byId = $results = []; + /** @var array[] $byId */ + $byId = []; + /** @var array[] $results */ + $results = []; + foreach ($users as $uid => $info) { if (!isset($byId[$info['node_id']])) { $byId[$info['node_id']] = []; @@ -70,11 +77,17 @@ class ShareHelper implements IShareHelper { $byId[$info['node_id']][$uid] = $info['node_path']; } - if (isset($byId[$node->getId()])) { - foreach ($byId[$node->getId()] as $uid => $path) { - $results[$uid] = $path; + try { + if (isset($byId[$node->getId()])) { + foreach ($byId[$node->getId()] as $uid => $path) { + $results[$uid] = $path; + } + unset($byId[$node->getId()]); } - unset($byId[$node->getId()]); + } catch (NotFoundException $e) { + return $results; + } catch (InvalidPathException $e) { + return $results; } if (empty($byId)) { @@ -84,17 +97,25 @@ class ShareHelper implements IShareHelper { $item = $node; $appendix = '/' . $node->getName(); while (!empty($byId)) { - /** @var Node $item */ - $item = $item->getParent(); + try { + /** @var Node $item */ + $item = $item->getParent(); - if (!empty($byId[$item->getId()])) { - foreach ($byId[$item->getId()] as $uid => $path) { - $results[$uid] = $path . $appendix; + if (!empty($byId[$item->getId()])) { + foreach ($byId[$item->getId()] as $uid => $path) { + $results[$uid] = $path . $appendix; + } + unset($byId[$item->getId()]); } - unset($byId[$item->getId()]); - } - $appendix = '/' . $item->getName() . $appendix; + $appendix = '/' . $item->getName() . $appendix; + } catch (NotFoundException $e) { + return $results; + } catch (InvalidPathException $e) { + return $results; + } catch (NotPermittedException $e) { + return $results; + } } return $results; @@ -106,7 +127,11 @@ class ShareHelper implements IShareHelper { * @return array */ protected function getPathsForRemotes(Node $node, array $remotes) { - $byId = $results = []; + /** @var array[] $byId */ + $byId = []; + /** @var array[] $results */ + $results = []; + foreach ($remotes as $cloudId => $info) { if (!isset($byId[$info['node_id']])) { $byId[$info['node_id']] = []; @@ -116,17 +141,27 @@ class ShareHelper implements IShareHelper { $item = $node; while (!empty($byId)) { - if (!empty($byId[$item->getId()])) { - $path = $this->getMountedPath($item); - foreach ($byId[$item->getId()] as $uid => $token) { - $results[$uid] = [ - 'node_path' => $path, - 'token' => $token, - ]; + try { + if (!empty($byId[$item->getId()])) { + $path = $this->getMountedPath($item); + foreach ($byId[$item->getId()] as $uid => $token) { + $results[$uid] = [ + 'node_path' => $path, + 'token' => $token, + ]; + } + unset($byId[$item->getId()]); } - unset($byId[$item->getId()]); + + /** @var Node $item */ + $item = $item->getParent(); + } catch (NotFoundException $e) { + return $results; + } catch (InvalidPathException $e) { + return $results; + } catch (NotPermittedException $e) { + return $results; } - $item = $item->getParent(); } return $results; From e1d54e3b48d68e3d631a6c6828b80ce49f414ea4 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 14:45:02 +0200 Subject: [PATCH 23/29] Add more tests for the share helper Signed-off-by: Joas Schilling --- tests/lib/Share20/ShareHelperTest.php | 111 ++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/tests/lib/Share20/ShareHelperTest.php b/tests/lib/Share20/ShareHelperTest.php index e2cd4ce038..69609f9f72 100644 --- a/tests/lib/Share20/ShareHelperTest.php +++ b/tests/lib/Share20/ShareHelperTest.php @@ -24,6 +24,7 @@ namespace Test\Share20; use OC\Share20\ShareHelper; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\Share\IManager; use Test\TestCase; @@ -96,6 +97,116 @@ class ShareHelperTest extends TestCase { $this->assertSame($expected, $helper->getPathsForAccessList($node)); } + public function dataGetPathsForUsers() { + return [ + [[], [23 => 'TwentyThree', 42 => 'FortyTwo'], []], + [ + [ + 'test1' => ['node_id' => 16, 'node_path' => '/foo'], + 'test2' => ['node_id' => 23, 'node_path' => '/bar'], + 'test3' => ['node_id' => 42, 'node_path' => '/cat'], + 'test4' => ['node_id' => 48, 'node_path' => '/dog'], + ], + [16 => 'SixTeen', 23 => 'TwentyThree', 42 => 'FortyTwo'], + [ + 'test1' => '/foo/TwentyThree/FortyTwo', + 'test2' => '/bar/FortyTwo', + 'test3' => '/cat', + ], + ], + ]; + } + + /** + * @dataProvider dataGetPathsForUsers + * + * @param array $users + * @param array $nodes + * @param array $expected + */ + public function testGetPathsForUsers(array $users, array $nodes, array $expected) { + $lastNode = null; + foreach ($nodes as $nodeId => $nodeName) { + /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->createMock(Node::class); + $node->expects($this->any()) + ->method('getId') + ->willReturn($nodeId); + $node->expects($this->any()) + ->method('getName') + ->willReturn($nodeName); + if ($lastNode === null) { + $node->expects($this->any()) + ->method('getParent') + ->willThrowException(new NotFoundException()); + } else { + $node->expects($this->any()) + ->method('getParent') + ->willReturn($lastNode); + } + $lastNode = $node; + } + + $this->assertEquals($expected, self::invokePrivate($this->helper, 'getPathsForUsers', [$lastNode, $users])); + } + + public function dataGetPathsForRemotes() { + return [ + [[], [23 => 'TwentyThree', 42 => 'FortyTwo'], []], + [ + [ + 'test1' => ['node_id' => 16, 'token' => 't1'], + 'test2' => ['node_id' => 23, 'token' => 't2'], + 'test3' => ['node_id' => 42, 'token' => 't3'], + 'test4' => ['node_id' => 48, 'token' => 't4'], + ], + [ + 16 => '/admin/files/SixTeen', + 23 => '/admin/files/SixTeen/TwentyThree', + 42 => '/admin/files/SixTeen/TwentyThree/FortyTwo', + ], + [ + 'test1' => ['token' => 't1', 'node_path' => '/SixTeen'], + 'test2' => ['token' => 't2', 'node_path' => '/SixTeen/TwentyThree'], + 'test3' => ['token' => 't3', 'node_path' => '/SixTeen/TwentyThree/FortyTwo'], + ], + ], + ]; + } + + /** + * @dataProvider dataGetPathsForRemotes + * + * @param array $remotes + * @param array $nodes + * @param array $expected + */ + public function testGetPathsForRemotes(array $remotes, array $nodes, array $expected) { + $lastNode = null; + foreach ($nodes as $nodeId => $nodePath) { + /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->createMock(Node::class); + $node->expects($this->any()) + ->method('getId') + ->willReturn($nodeId); + $node->expects($this->any()) + ->method('getPath') + ->willReturn($nodePath); + if ($lastNode === null) { + $node->expects($this->any()) + ->method('getParent') + ->willThrowException(new NotFoundException()); + } else { + $node->expects($this->any()) + ->method('getParent') + ->willReturn($lastNode); + } + $lastNode = $node; + } + + $this->assertEquals($expected, self::invokePrivate($this->helper, 'getPathsForRemotes', [$lastNode, $remotes])); + } + public function dataGetMountedPath() { return [ ['/admin/files/foobar', '/foobar'], From f57ef5524902be35fb944aa871279cf7ad9777e1 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Apr 2017 14:45:18 +0200 Subject: [PATCH 24/29] Add samples to the docs Signed-off-by: Joas Schilling --- lib/private/Share20/ShareHelper.php | 38 +++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/lib/private/Share20/ShareHelper.php b/lib/private/Share20/ShareHelper.php index 6518663446..358b4e8026 100644 --- a/lib/private/Share20/ShareHelper.php +++ b/lib/private/Share20/ShareHelper.php @@ -60,6 +60,25 @@ class ShareHelper implements IShareHelper { } /** + * Sample: + * $users = [ + * 'test1' => ['node_id' => 16, 'node_path' => '/foo'], + * 'test2' => ['node_id' => 23, 'node_path' => '/bar'], + * 'test3' => ['node_id' => 42, 'node_path' => '/cat'], + * 'test4' => ['node_id' => 48, 'node_path' => '/dog'], + * ]; + * + * Node tree: + * - SixTeen is the parent of TwentyThree + * - TwentyThree is the parent of FortyTwo + * - FortyEight does not exist + * + * $return = [ + * 'test1' => '/foo/TwentyThree/FortyTwo', + * 'test2' => '/bar/FortyTwo', + * 'test3' => '/cat', + * ], + * * @param Node $node * @param array[] $users * @return array @@ -122,6 +141,25 @@ class ShareHelper implements IShareHelper { } /** + * Sample: + * $remotes = [ + * 'test1' => ['node_id' => 16, 'token' => 't1'], + * 'test2' => ['node_id' => 23, 'token' => 't2'], + * 'test3' => ['node_id' => 42, 'token' => 't3'], + * 'test4' => ['node_id' => 48, 'token' => 't4'], + * ]; + * + * Node tree: + * - SixTeen is the parent of TwentyThree + * - TwentyThree is the parent of FortyTwo + * - FortyEight does not exist + * + * $return = [ + * 'test1' => ['token' => 't1', 'node_path' => '/SixTeen'], + * 'test2' => ['token' => 't2', 'node_path' => '/SixTeen/TwentyThree'], + * 'test3' => ['token' => 't3', 'node_path' => '/SixTeen/TwentyThree/FortyTwo'], + * ], + * * @param Node $node * @param array[] $remotes * @return array From b96297e9cc10e5cb7f71a9268eefb534353ca059 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 13 Apr 2017 12:58:29 +0200 Subject: [PATCH 25/29] Do not set full path if not currentAccess Signed-off-by: Roeland Jago Douma --- lib/private/Share20/Manager.php | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 8b670da544..6e59629153 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1242,12 +1242,17 @@ class Manager implements IManager { /** @var Node[] $nodes */ $nodes = []; - $ownerPath = $path->getPath(); - list(,,,$ownerPath) = explode('/', $ownerPath, 4); - $al['users'][$owner] = [ - 'node_id' => $path->getId(), - 'node_path' => '/' . $ownerPath, - ]; + + if ($currentAccess) { + $ownerPath = $path->getPath(); + list(, , , $ownerPath) = explode('/', $ownerPath, 4); + $al['users'][$owner] = [ + 'node_id' => $path->getId(), + 'node_path' => '/' . $ownerPath, + ]; + } else { + $al['users'][] = $owner; + } // Collect all the shares while ($path->getPath() !== $userFolder->getPath()) { From aef95b9b7d6493f933652521647f8d0d5bb81005 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 13 Apr 2017 13:37:39 +0200 Subject: [PATCH 26/29] Not needed in the DIContainer anymore Signed-off-by: Roeland Jago Douma --- .../AppFramework/DependencyInjection/DIContainer.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 45a971f125..689c82c285 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -57,7 +57,6 @@ use OCP\IRequest; use OCP\IServerContainer; use OCP\IUserSession; use OCP\RichObjectStrings\IValidator; -use OCP\Share\IShareHelper; use OCP\Util; class DIContainer extends SimpleContainer implements IAppContainer { @@ -170,11 +169,6 @@ class DIContainer extends SimpleContainer implements IAppContainer { ); }); - $this->registerService(IShareHelper::class, function (SimpleContainer $c) { - return $c->query(IShareHelper::class); - }); - - /** * App Framework APIs */ From 0f5682321e13295d93097688370569f3275043fa Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 13 Apr 2017 14:52:09 +0200 Subject: [PATCH 27/29] Fix server container registration Signed-off-by: Roeland Jago Douma --- lib/private/Server.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Server.php b/lib/private/Server.php index 068c89dd8e..7b1ea50d5f 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -982,7 +982,7 @@ class Server extends ServerContainer implements IServerContainer { $this->registerService(IShareHelper::class, function(Server $c) { return new ShareHelper( - $c->getLazyRootFolder() + $c->query(\OCP\Share\IManager::class) ); }); } From 6a519abde8198914ab56c2fe61cd88e39e6f48e7 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 13 Apr 2017 14:56:05 +0200 Subject: [PATCH 28/29] Update autoloader Signed-off-by: Roeland Jago Douma --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 3cf8733e41..a6181be072 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -252,6 +252,7 @@ return array( 'OCP\\Share\\IManager' => $baseDir . '/lib/public/Share/IManager.php', 'OCP\\Share\\IProviderFactory' => $baseDir . '/lib/public/Share/IProviderFactory.php', 'OCP\\Share\\IShare' => $baseDir . '/lib/public/Share/IShare.php', + 'OCP\\Share\\IShareHelper' => $baseDir . '/lib/public/Share/IShareHelper.php', 'OCP\\Share\\IShareProvider' => $baseDir . '/lib/public/Share/IShareProvider.php', 'OCP\\Share_Backend' => $baseDir . '/lib/public/Share_Backend.php', 'OCP\\Share_Backend_Collection' => $baseDir . '/lib/public/Share_Backend_Collection.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 4aecf62329..96225f1703 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -282,6 +282,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Share\\IManager' => __DIR__ . '/../../..' . '/lib/public/Share/IManager.php', 'OCP\\Share\\IProviderFactory' => __DIR__ . '/../../..' . '/lib/public/Share/IProviderFactory.php', 'OCP\\Share\\IShare' => __DIR__ . '/../../..' . '/lib/public/Share/IShare.php', + 'OCP\\Share\\IShareHelper' => __DIR__ . '/../../..' . '/lib/public/Share/IShareHelper.php', 'OCP\\Share\\IShareProvider' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProvider.php', 'OCP\\Share_Backend' => __DIR__ . '/../../..' . '/lib/public/Share_Backend.php', 'OCP\\Share_Backend_Collection' => __DIR__ . '/../../..' . '/lib/public/Share_Backend_Collection.php', From cab41118f67e6a4743bac2cfcfa69a4bee28f2a3 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 13 Apr 2017 15:14:30 +0200 Subject: [PATCH 29/29] Mail shares trigger the public key Signed-off-by: Roeland Jago Douma --- apps/sharebymail/lib/ShareByMailProvider.php | 2 +- .../tests/ShareByMailProviderTest.php | 32 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 0f4a8dd87b..83170c5648 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -858,7 +858,7 @@ class ShareByMailProvider implements IShareProvider { $mail = $cursor->fetch() !== false; $cursor->closeCursor(); - return ['mail' => $mail]; + return ['public' => $mail]; } } diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php index cd0600d3ae..13fb5d03bd 100644 --- a/apps/sharebymail/tests/ShareByMailProviderTest.php +++ b/apps/sharebymail/tests/ShareByMailProviderTest.php @@ -676,11 +676,11 @@ class ShareByMailProviderTest extends TestCase { $folder = $rootFolder->getUserFolder($u1->getUID())->newFolder('foo'); $accessList = $provider->getAccessList([$folder], true); - $this->assertArrayHasKey('mail', $accessList); - $this->assertFalse($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertFalse($accessList['public']); $accessList = $provider->getAccessList([$folder], false); - $this->assertArrayHasKey('mail', $accessList); - $this->assertFalse($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertFalse($accessList['public']); $share1 = $this->shareManager->newShare(); $share1->setSharedWith('user@server.com') @@ -699,29 +699,29 @@ class ShareByMailProviderTest extends TestCase { $share2 = $provider->create($share2); $accessList = $provider->getAccessList([$folder], true); - $this->assertArrayHasKey('mail', $accessList); - $this->assertTrue($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertTrue($accessList['public']); $accessList = $provider->getAccessList([$folder], false); - $this->assertArrayHasKey('mail', $accessList); - $this->assertTrue($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertTrue($accessList['public']); $provider->delete($share2); $accessList = $provider->getAccessList([$folder], true); - $this->assertArrayHasKey('mail', $accessList); - $this->assertTrue($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertTrue($accessList['public']); $accessList = $provider->getAccessList([$folder], false); - $this->assertArrayHasKey('mail', $accessList); - $this->assertTrue($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertTrue($accessList['public']); $provider->delete($share1); $accessList = $provider->getAccessList([$folder], true); - $this->assertArrayHasKey('mail', $accessList); - $this->assertFalse($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertFalse($accessList['public']); $accessList = $provider->getAccessList([$folder], false); - $this->assertArrayHasKey('mail', $accessList); - $this->assertFalse($accessList['mail']); + $this->assertArrayHasKey('public', $accessList); + $this->assertFalse($accessList['public']); $u1->delete(); $u2->delete();