From 98301210a9172e6b0a44bcaf43b4ecf4b31f938e Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sat, 8 Aug 2015 22:15:12 +0200 Subject: [PATCH 01/24] Start of OCS Share API sharees endpoint --- apps/files_sharing/api/sharees.php | 234 ++++++++++++++++++++++++++ apps/files_sharing/appinfo/routes.php | 11 ++ 2 files changed, 245 insertions(+) create mode 100644 apps/files_sharing/api/sharees.php diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php new file mode 100644 index 0000000000..83acdd086c --- /dev/null +++ b/apps/files_sharing/api/sharees.php @@ -0,0 +1,234 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ +namespace OCA\Files_Sharing\API; + +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\IAppConfig; +use OCP\IUserSession; +use OCP\IURLGenerator; + +class Sharees { + + /** @var IGroupManager */ + private $groupManager; + + /** @var IUserManager */ + private $userManager; + + /** @var \OCP\Contacts\IManager */ + private $contactsManager; + + /** @var IAppConfig */ + private $appConfig; + + /** @var IUserSession */ + private $userSession; + + /** @var IURLGenerator */ + private $urlGenerator; + + /** + * @param IGroupManager $groupManager + * @param IUserManager $userManager + * @param \OCP\Contacts\IManager $contactsManager + * @param IAppConfig $appConfig + * @param IUserSession $userSession + */ + public function __construct(IGroupManager $groupManager, + IUserManager $userManager, + \OCP\Contacts\IManager $contactsManager, + IAppConfig $appConfig, + IUserSession $userSession, + IURLGenerator $urlGenerator) { + $this->groupManager = $groupManager; + $this->userManager = $userManager; + $this->contactsManager = $contactsManager; + $this->appConfig = $appConfig; + $this->userSession = $userSession; + $this->urlGenerator = $urlGenerator; + } + + /** + * @param string $search + * @param bool $shareWithGroupOnly + * + * @return array possible sharees + */ + private function getUsers($search, $shareWithGroupOnly) { + $sharees = []; + + $users = []; + if ($shareWithGroupOnly) { + // Search in all the groups this user is part of + $userGroups = $this->groupManager->getUserGroupsIds($this->userSession->getUser()); + foreach ($userGroups as $userGroup) { + $users = array_merge($users, $this->groupManager->displayNamesInGroup($userGroup, $search)); + } + $users = array_unique($users); + } else { + // Search in all users + $users_tmp = $this->userManager->searchDisplayName($search); + + // Put in array that maps uid => displayName + foreach($users_tmp as $user) { + $users[$user->getUID()] = $user->getDisplayName(); + } + } + + + foreach ($users as $uid => $displayName) { + $sharees[] = [ + 'label' => $displayName, + 'value' => [ + 'shareType' => \OCP\Share::SHARE_TYPE_USER, + 'shareWith' => $uid + ] + ]; + } + + return $sharees; + } + + /** + * @param string $search + * @param bool $shareWithGroupOnly + * + * @return array possible sharees + */ + private function getGroups($search, $shareWithGroupOnly) { + $sharees = []; + $groups = $this->groupManager->search($search); + + if ($shareWithGroupOnly) { + // Intersect all the groups that match with the groups this user is a member of + $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); + $groups = array_intersect($groups, $userGroups); + } + + foreach ($groups as $group) { + $sharees[] = [ + 'label' => $group->getGID(), + 'value' => [ + 'shareType' => \OCP\Share::SHARE_TYPE_GROUP, + 'shareWith' => $group->getGID() + ] + ]; + } + + return $sharees; + } + + /** + * @param string $search + * + * @return array possible sharees + */ + private function getRemote($search) { + $sharees = []; + + if (substr_count($search, '@') >= 1) { + $sharees[] = [ + 'label' => $search, + 'value' => [ + 'shareType' => \OCP\Share::SHARE_TYPE_REMOTE, + 'shareWith' => $search + ] + ]; + } + + // Search in contacts + $addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN']); + foreach ($addressBookContacts as $contact) { + if (isset($contact['CLOUD'])) { + foreach ($contact['CLOUD'] as $cloudId) { + $sharees[] = [ + 'label' => $contact['FN'] . ' (' . $cloudId . ')', + 'value' => [ + 'shareType' => \OCP\Share::SHARE_TYPE_REMOTE, + 'shareWith' => $cloudId + ] + ]; + } + } + } + + return $sharees; + } + + public function search($params) { + $search = isset($_GET['search']) ? (string)$_GET['search'] : ''; + $item_type = isset($_GET['item_type']) ? (string)$_GET['item_type'] : null; + $share_type = isset($_GET['share_type']) ? intval($_GET['share_type']) : null; + $page = isset($_GET['page']) ? intval($_GET['page']) : 1; + $per_page = isset($_GET['per_page']) ? intval($_GET['per_page']) : 200; + + // Verify arguments + if ($item_type === null) { + return new \OC_OCS_Result(null, 400, 'missing item_type'); + } + + $shareWithGroupOnly = $this->appConfig->getValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' ? true : false; + + $sharees = []; + // Get users + if ($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_USER) { + $sharees = array_merge($sharees, $this->getUsers($search, $shareWithGroupOnly)); + } + + // Get groups + if ($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_GROUP) { + $sharees = array_merge($sharees, $this->getGroups($search, $shareWithGroupOnly)); + } + + // Get remote + if (($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_REMOTE) && + \OCP\Share::getBackend($item_type)->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE)) { + $sharees = array_merge($sharees, $this->getRemote($search)); + } + + //Pagination + $start = ($page - 1) * $per_page; + $end = $page * $per_page; + $tot = count($sharees); + + $sharees = array_slice($sharees, $start, $per_page); + $response = new \OC_OCS_Result($sharees); + + // FIXME: Do this? + $response->setTotalItems($tot); + $response->setItemsPerPage($per_page); + + // TODO add other link rels + + if ($tot > $end) { + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees?') . + 'search=' . $search . + '&item_type=' . $item_type . + '&share_type=' . $share_type . + '&page=' . ($page + 1) . + '&per_page=' . $per_page; + $response->addHeader('Link', '<' . $url . '> rel="next"'); + } + + return $response; + } +} diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 184ad29bba..498b0eb55e 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -102,3 +102,14 @@ API::register('delete', array('\OCA\Files_Sharing\API\Remote', 'declineShare'), 'files_sharing'); +$sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(), + \OC::$server->getUserManager(), + \OC::$server->getContactsManager(), + \OC::$server->getAppConfig(), + \OC::$server->getUserSession(), + \OC::$server->getURLGenerator()); + +API::register('get', + '/apps/files_sharing/api/v1/sharees', + [$sharees, 'search'], + 'files_sharing', API::USER_AUTH); From 8a5c1e6d4d63b2efc2c3e8acca98043973295863 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sun, 9 Aug 2015 20:47:49 +0200 Subject: [PATCH 02/24] Sort sharees To ensure that pagination is working properly we need to make sure the shares are always in the same order. Sorting is first done by label (catches most instances) If there is a user and a group with the same label we sort by shareType If there are multiple users with the same label we sort those by shareWith --- apps/files_sharing/api/sharees.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 83acdd086c..8146c98b01 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -205,6 +205,24 @@ class Sharees { $sharees = array_merge($sharees, $this->getRemote($search)); } + + // Sort sharees + usort($sharees, function($a, $b) { + $res = strcmp($a['label'], $b['label']); + + // If labels are equal sort by share type + if ($res === 0) { + $res = $a['value']['shareType'] - $b['value']['shareType']; + } + + // If sharetype is equal compare shareWith + if ($res === 0) { + $res = strcmp($a['value']['shareWith'], $b['value']['shareWith']); + } + + return $res; + }); + //Pagination $start = ($page - 1) * $per_page; $end = $page * $per_page; From b2fbecc39fa5d0538a946244e5b85a7145cb4505 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Sun, 9 Aug 2015 21:19:35 +0200 Subject: [PATCH 03/24] Empty skeleton for tests --- apps/files_sharing/tests/sharees.php | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 apps/files_sharing/tests/sharees.php diff --git a/apps/files_sharing/tests/sharees.php b/apps/files_sharing/tests/sharees.php new file mode 100644 index 0000000000..a9e3350ceb --- /dev/null +++ b/apps/files_sharing/tests/sharees.php @@ -0,0 +1,50 @@ + Date: Tue, 11 Aug 2015 14:57:51 +0200 Subject: [PATCH 04/24] Move test file to subdir --- apps/files_sharing/api/sharees.php | 2 +- apps/files_sharing/tests/{ => api}/sharees.php | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename apps/files_sharing/tests/{ => api}/sharees.php (100%) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 8146c98b01..5dd0da55de 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -54,7 +54,7 @@ class Sharees { * @param IUserSession $userSession */ public function __construct(IGroupManager $groupManager, - IUserManager $userManager, + IUserManager $userManager, \OCP\Contacts\IManager $contactsManager, IAppConfig $appConfig, IUserSession $userSession, diff --git a/apps/files_sharing/tests/sharees.php b/apps/files_sharing/tests/api/sharees.php similarity index 100% rename from apps/files_sharing/tests/sharees.php rename to apps/files_sharing/tests/api/sharees.php From be257bc9cc2330d0e67957525cb66646b346a850 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Aug 2015 15:43:44 +0200 Subject: [PATCH 05/24] Add tests for "getUsers()" --- apps/files_sharing/api/sharees.php | 2 +- apps/files_sharing/tests/api/sharees.php | 255 +++++++++++++++++++---- 2 files changed, 219 insertions(+), 38 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 5dd0da55de..6853bd8a26 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -79,7 +79,7 @@ class Sharees { $users = []; if ($shareWithGroupOnly) { // Search in all the groups this user is part of - $userGroups = $this->groupManager->getUserGroupsIds($this->userSession->getUser()); + $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($userGroups as $userGroup) { $users = array_merge($users, $this->groupManager->displayNamesInGroup($userGroup, $search)); } diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index a9e3350ceb..30397ba867 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -1,50 +1,231 @@ + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\Files_Sharing\Tests\API; use OCA\Files_Sharing\API\Sharees; use OCA\Files_sharing\Tests\TestCase; class ShareesTest extends TestCase { + /** @var Sharees */ + protected $sharees; + + /** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $userManager; + + /** @var \OCP\IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $groupManager; + + /** @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject */ + protected $session; + + protected function setUp() { + parent::setUp(); + + $this->userManager = $this->getMockBuilder('OCP\IUserManager') + ->disableOriginalConstructor() + ->getMock(); + + $this->groupManager = $this->getMockBuilder('OCP\IGroupManager') + ->disableOriginalConstructor() + ->getMock(); + + $this->session = $this->getMockBuilder('OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + + $this->sharees = new Sharees( + $this->groupManager, + $this->userManager, + $this->getMockBuilder('OCP\Contacts\IManager')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\IAppConfig')->disableOriginalConstructor()->getMock(), + $this->session, + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + ); + } + + protected function getUserMock($uid, $displayName) { + $user = $this->getMockBuilder('OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + + $user->expects($this->any()) + ->method('getUID') + ->willReturn($uid); + + $user->expects($this->any()) + ->method('getDisplayName') + ->willReturn($displayName); + + return $user; + } + + public function dataGetUsers() { + return [ + ['test', false, [], [], []], + ['test', true, [], [], []], + [ + 'test', + false, + [], + [ + $this->getUserMock('test1', 'Test One'), + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ] + ], + [ + 'test', + false, + [], + [ + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ] + ], + [ + 'test', + true, + ['abc', 'xyz'], + [ + ['abc', 'test', -1, 0, ['test1' => 'Test One']], + ['xyz', 'test', -1, 0, []], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ] + ], + [ + 'test', + true, + ['abc', 'xyz'], + [ + ['abc', 'test', -1, 0, [ + 'test1' => 'Test One', + 'test2' => 'Test Two', + ]], + ['xyz', 'test', -1, 0, [ + 'test1' => 'Test One', + 'test2' => 'Test Two', + ]], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ] + ], + [ + 'test', + true, + ['abc', 'xyz'], + [ + ['abc', 'test', -1, 0, [ + 'test1' => 'Test One', + ]], + ['xyz', 'test', -1, 0, [ + 'test2' => 'Test Two', + ]], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ] + ], + ]; + } /** - * Properly mock all injected elements + * @dataProvider dataGetUsers + * + * @param string $searchTerm + * @param bool $shareWithGroupOnly + * @param array $groupResponse + * @param array $userResponse + * @param array $expected */ - protected function setup() { + public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $expected) { + if (!$shareWithGroupOnly) { + $this->userManager->expects($this->once()) + ->method('searchDisplayName') + ->with($searchTerm) + ->willReturn($userResponse); + } else { + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($this->getUserMock('admin', 'Administrator')); + $this->groupManager->expects($this->once()) + ->method('getUserGroupIds') + ->with($this->anything()) + ->willReturn($groupResponse); + + $this->groupManager->expects($this->exactly(sizeof($groupResponse))) + ->method('displayNamesInGroup') + ->with($this->anything(), $searchTerm) + ->willReturnMap($userResponse); + } + + $users = $this->invokePrivate($this->sharees, 'getUsers', [$searchTerm, $shareWithGroupOnly]); + + $this->assertEquals($expected, $users); } - public function testArguments() { - - } - - public function testsOnlyUsers() { - - } - - public function testOnlyGroups() { - - } - - public function testRemoteAddress() { - - } - - public function testRemoteFromContacts() { - - } - - public function testAll() { - - } - - public function testSorting() { - - } - - public function testPagination() { - - } - - public function testShareWithinGroup() { - - } +// public function testArguments() { +// +// } +// +// public function testsOnlyUsers() { +// +// } +// +// public function testOnlyGroups() { +// +// } +// +// public function testRemoteAddress() { +// +// } +// +// public function testRemoteFromContacts() { +// +// } +// +// public function testAll() { +// +// } +// +// public function testSorting() { +// +// } +// +// public function testPagination() { +// +// } +// +// public function testShareWithinGroup() { +// +// } } From ad450d4f0ed4ebbb5e9e6256750061b0adb950e1 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Aug 2015 16:22:05 +0200 Subject: [PATCH 06/24] Add tests for "getGroups()" --- apps/files_sharing/api/sharees.php | 21 ++++--- apps/files_sharing/tests/api/sharees.php | 72 +++++++++++++++++++++++- 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 6853bd8a26..7a3555e0a5 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -20,6 +20,7 @@ */ namespace OCA\Files_Sharing\API; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IUserManager; use OCP\IAppConfig; @@ -100,8 +101,8 @@ class Sharees { 'label' => $displayName, 'value' => [ 'shareType' => \OCP\Share::SHARE_TYPE_USER, - 'shareWith' => $uid - ] + 'shareWith' => $uid, + ], ]; } @@ -117,20 +118,22 @@ class Sharees { private function getGroups($search, $shareWithGroupOnly) { $sharees = []; $groups = $this->groupManager->search($search); + $groups = array_map(function (IGroup $group) { return $group->getGID(); }, $groups); - if ($shareWithGroupOnly) { + if (!empty($groups) && $shareWithGroupOnly) { // Intersect all the groups that match with the groups this user is a member of $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); + $userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups); $groups = array_intersect($groups, $userGroups); } - foreach ($groups as $group) { + foreach ($groups as $gid) { $sharees[] = [ - 'label' => $group->getGID(), + 'label' => $gid, 'value' => [ 'shareType' => \OCP\Share::SHARE_TYPE_GROUP, - 'shareWith' => $group->getGID() - ] + 'shareWith' => $gid, + ], ]; } @@ -150,8 +153,8 @@ class Sharees { 'label' => $search, 'value' => [ 'shareType' => \OCP\Share::SHARE_TYPE_REMOTE, - 'shareWith' => $search - ] + 'shareWith' => $search, + ], ]; } diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 30397ba867..25074e4e8c 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -78,6 +78,18 @@ class ShareesTest extends TestCase { return $user; } + protected function getGroupMock($gid) { + $group = $this->getMockBuilder('OCP\IGroup') + ->disableOriginalConstructor() + ->getMock(); + + $group->expects($this->any()) + ->method('getGID') + ->willReturn($gid); + + return $group; + } + public function dataGetUsers() { return [ ['test', false, [], [], []], @@ -173,13 +185,14 @@ class ShareesTest extends TestCase { ->with($searchTerm) ->willReturn($userResponse); } else { + $user = $this->getUserMock('admin', 'Administrator'); $this->session->expects($this->any()) ->method('getUser') - ->willReturn($this->getUserMock('admin', 'Administrator')); + ->willReturn($user); $this->groupManager->expects($this->once()) ->method('getUserGroupIds') - ->with($this->anything()) + ->with($user) ->willReturn($groupResponse); $this->groupManager->expects($this->exactly(sizeof($groupResponse))) @@ -193,6 +206,61 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $users); } + public function dataGetGroups() { + return [ + ['test', false, [], [], []], + [ + 'test', false, + [$this->getGroupMock('test1')], + [], + [['label' => 'test1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + ], + ['test', true, [], [], []], + [ + 'test', true, + [ + $this->getGroupMock('test1'), + $this->getGroupMock('test2'), + ], + [$this->getGroupMock('test1')], + [['label' => 'test1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + ], + ]; + } + + /** + * @dataProvider dataGetGroups + * + * @param string $searchTerm + * @param bool $shareWithGroupOnly + * @param array $groupResponse + * @param array $userGroupsResponse + * @param array $expected + */ + public function testGetGroups($searchTerm, $shareWithGroupOnly, $groupResponse, $userGroupsResponse, $expected) { + $this->groupManager->expects($this->once()) + ->method('search') + ->with($searchTerm) + ->willReturn($groupResponse); + + if ($shareWithGroupOnly) { + $user = $this->getUserMock('admin', 'Administrator'); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($user); + + $numGetUserGroupsCalls = empty($groupResponse) ? 0 : 1; + $this->groupManager->expects($this->exactly($numGetUserGroupsCalls)) + ->method('getUserGroups') + ->with($user) + ->willReturn($userGroupsResponse); + } + + $users = $this->invokePrivate($this->sharees, 'getGroups', [$searchTerm, $shareWithGroupOnly]); + + $this->assertEquals($expected, $users); + } + // public function testArguments() { // // } From 16e5c15c283c40fd9c7a3f009f054b825bcadbb7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Aug 2015 16:31:54 +0200 Subject: [PATCH 07/24] Add tests for "getRemote()" --- apps/files_sharing/tests/api/sharees.php | 85 +++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 25074e4e8c..04b8ef5025 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -34,6 +34,9 @@ class ShareesTest extends TestCase { /** @var \OCP\IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ protected $groupManager; + /** @var \OCP\Contacts\IManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $contactsManager; + /** @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject */ protected $session; @@ -48,6 +51,10 @@ class ShareesTest extends TestCase { ->disableOriginalConstructor() ->getMock(); + $this->contactsManager = $this->getMockBuilder('OCP\Contacts\IManager') + ->disableOriginalConstructor() + ->getMock(); + $this->session = $this->getMockBuilder('OCP\IUserSession') ->disableOriginalConstructor() ->getMock(); @@ -55,7 +62,7 @@ class ShareesTest extends TestCase { $this->sharees = new Sharees( $this->groupManager, $this->userManager, - $this->getMockBuilder('OCP\Contacts\IManager')->disableOriginalConstructor()->getMock(), + $this->contactsManager, $this->getMockBuilder('OCP\IAppConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() @@ -261,6 +268,82 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $users); } + public function dataGetRemote() { + return [ + ['test', [], []], + [ + 'test@remote', + [], + [ + ['label' => 'test@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], + ], + ], + [ + 'test', + [ + [ + 'FN' => 'User3 @ Localhost', + ], + [ + 'FN' => 'User2 @ Localhost', + 'CLOUD' => [ + ], + ], + [ + 'FN' => 'User @ Localhost', + 'CLOUD' => [ + 'username@localhost', + ], + ], + ], + [ + ['label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ], + ], + [ + 'test@remote', + [ + [ + 'FN' => 'User3 @ Localhost', + ], + [ + 'FN' => 'User2 @ Localhost', + 'CLOUD' => [ + ], + ], + [ + 'FN' => 'User @ Localhost', + 'CLOUD' => [ + 'username@localhost', + ], + ], + ], + [ + ['label' => 'test@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], + ['label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ], + ], + ]; + } + + /** + * @dataProvider dataGetRemote + * + * @param string $searchTerm + * @param array $contacts + * @param array $expected + */ + public function testGetRemote($searchTerm, $contacts, $expected) { + $this->contactsManager->expects($this->any()) + ->method('search') + ->with($searchTerm, ['CLOUD', 'FN']) + ->willReturn($contacts); + + $users = $this->invokePrivate($this->sharees, 'getRemote', [$searchTerm]); + + $this->assertEquals($expected, $users); + } + // public function testArguments() { // // } From 4b08783946c8b2b1e731b3193e581b59bbbb15f5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Aug 2015 17:24:54 +0200 Subject: [PATCH 08/24] Use SearchResultSorter --- apps/files_sharing/api/sharees.php | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 7a3555e0a5..120df126a7 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -210,21 +210,10 @@ class Sharees { // Sort sharees - usort($sharees, function($a, $b) { - $res = strcmp($a['label'], $b['label']); - - // If labels are equal sort by share type - if ($res === 0) { - $res = $a['value']['shareType'] - $b['value']['shareType']; - } - - // If sharetype is equal compare shareWith - if ($res === 0) { - $res = strcmp($a['value']['shareWith'], $b['value']['shareWith']); - } - - return $res; - }); + $sorter = new \OC\Share\SearchResultSorter($search, + 'label', + \OC::$server->getLogger()); + usort($sharees, array($sorter, 'sort')); //Pagination $start = ($page - 1) * $per_page; @@ -239,7 +228,6 @@ class Sharees { $response->setItemsPerPage($per_page); // TODO add other link rels - if ($tot > $end) { $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees?') . 'search=' . $search . From a66aa1fe02038d70bd567cbcdc486441017d3172 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 11 Aug 2015 18:03:07 +0200 Subject: [PATCH 09/24] Filter the sharees with the existing shares --- apps/files_sharing/api/sharees.php | 92 ++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 29 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 120df126a7..a499b0ec6d 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -178,33 +178,49 @@ class Sharees { } public function search($params) { - $search = isset($_GET['search']) ? (string)$_GET['search'] : ''; - $item_type = isset($_GET['item_type']) ? (string)$_GET['item_type'] : null; - $share_type = isset($_GET['share_type']) ? intval($_GET['share_type']) : null; - $page = isset($_GET['page']) ? intval($_GET['page']) : 1; - $per_page = isset($_GET['per_page']) ? intval($_GET['per_page']) : 200; + $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; + $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; + $existingShares = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; + $shareType = isset($_GET['shareType']) ? intval($_GET['shareType']) : null; + $page = !empty($_GET['page']) ? intval($_GET['page']) : 1; + $perPage = !empty($_GET['limit']) ? intval($_GET['limit']) : 200; + + $sharedUsers = $sharedGroups = []; + if (!empty($existingShares)) { + if (!empty($existingShares[\OCP\Share::SHARE_TYPE_USER]) && + is_array($existingShares[\OCP\Share::SHARE_TYPE_USER])) { + $sharedUsers = $existingShares[\OCP\Share::SHARE_TYPE_USER]; + } + + if (!empty($existingShares[\OCP\Share::SHARE_TYPE_GROUP]) && + is_array($existingShares[\OCP\Share::SHARE_TYPE_GROUP])) { + $sharedGroups = $existingShares[\OCP\Share::SHARE_TYPE_GROUP]; + } + } // Verify arguments - if ($item_type === null) { - return new \OC_OCS_Result(null, 400, 'missing item_type'); + if ($itemType === null) { + return new \OC_OCS_Result(null, 400, 'missing itemType'); } $shareWithGroupOnly = $this->appConfig->getValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' ? true : false; $sharees = []; // Get users - if ($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_USER) { - $sharees = array_merge($sharees, $this->getUsers($search, $shareWithGroupOnly)); + if ($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_USER) { + $potentialSharees = $this->getUsers($search, $shareWithGroupOnly); + $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedUsers)); } // Get groups - if ($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_GROUP) { - $sharees = array_merge($sharees, $this->getGroups($search, $shareWithGroupOnly)); + if ($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_GROUP) { + $potentialSharees = $this->getGroups($search, $shareWithGroupOnly); + $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedGroups)); } // Get remote - if (($share_type === null || $share_type === \OCP\Share::SHARE_TYPE_REMOTE) && - \OCP\Share::getBackend($item_type)->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE)) { + if (($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_REMOTE) && + \OCP\Share::getBackend($itemType)->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE)) { $sharees = array_merge($sharees, $this->getRemote($search)); } @@ -216,28 +232,46 @@ class Sharees { usort($sharees, array($sorter, 'sort')); //Pagination - $start = ($page - 1) * $per_page; - $end = $page * $per_page; - $tot = count($sharees); + $start = ($page - 1) * $perPage; + $end = $page * $perPage; + $total = sizeof($sharees); + + $sharees = array_slice($sharees, $start, $perPage); - $sharees = array_slice($sharees, $start, $per_page); $response = new \OC_OCS_Result($sharees); + $response->setTotalItems($total); + $response->setItemsPerPage($perPage); - // FIXME: Do this? - $response->setTotalItems($tot); - $response->setItemsPerPage($per_page); - - // TODO add other link rels - if ($tot > $end) { - $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees?') . - 'search=' . $search . - '&item_type=' . $item_type . - '&share_type=' . $share_type . - '&page=' . ($page + 1) . - '&per_page=' . $per_page; + if ($total > $end) { + $params = [ + 'search' => $search, + 'itemType' => $itemType, + 'existingShares' => $existingShares, + 'page' => $page + 1, + 'limit' => $perPage, + ]; + if ($shareType !== null) { + $params['shareType'] = $shareType; + } + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?' . http_build_query($params); $response->addHeader('Link', '<' . $url . '> rel="next"'); } return $response; } + + /** + * Filter out already existing shares from a list of potential sharees + * + * @param array $potentialSharees + * @param array $existingSharees + * @return array + */ + private function filterSharees($potentialSharees, $existingSharees) { + $sharees = array_map(function ($sharee) use ($existingSharees) { + return in_array($sharee['value']['shareWith'], $existingSharees) ? null : $sharee; + }, $potentialSharees); + + return array_filter($sharees); + } } From 3f64e9423bf190629f867be20f17444a00257f8a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 12 Aug 2015 14:19:34 +0200 Subject: [PATCH 10/24] Split logic and global usage and add tests for "searchSharees()" --- apps/files_sharing/api/sharees.php | 39 +++- apps/files_sharing/tests/api/sharees.php | 222 +++++++++++++++++++---- 2 files changed, 217 insertions(+), 44 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index a499b0ec6d..573fd8f2ab 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -74,7 +74,7 @@ class Sharees { * * @return array possible sharees */ - private function getUsers($search, $shareWithGroupOnly) { + protected function getUsers($search, $shareWithGroupOnly) { $sharees = []; $users = []; @@ -115,7 +115,7 @@ class Sharees { * * @return array possible sharees */ - private function getGroups($search, $shareWithGroupOnly) { + protected function getGroups($search, $shareWithGroupOnly) { $sharees = []; $groups = $this->groupManager->search($search); $groups = array_map(function (IGroup $group) { return $group->getGID(); }, $groups); @@ -145,7 +145,7 @@ class Sharees { * * @return array possible sharees */ - private function getRemote($search) { + protected function getRemote($search) { $sharees = []; if (substr_count($search, '@') >= 1) { @@ -177,14 +177,37 @@ class Sharees { return $sharees; } + /** + * @param array $params + * @return \OC_OCS_Result + */ public function search($params) { - $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; - $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; - $existingShares = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; + $search = isset($_GET['search']) ? (string)$_GET['search'] : ''; + $itemType = isset($_GET['itemType']) ? (string)$_GET['itemType'] : null; + $existingShares = isset($_GET['existingShares']) ? (array)$_GET['existingShares'] : []; $shareType = isset($_GET['shareType']) ? intval($_GET['shareType']) : null; $page = !empty($_GET['page']) ? intval($_GET['page']) : 1; $perPage = !empty($_GET['limit']) ? intval($_GET['limit']) : 200; + $shareWithGroupOnly = $this->appConfig->getValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + + return $this->searchSharees($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly); + } + + /** + * Testable search function that does not need globals + * + * @param string $search + * @param string $itemType + * @param array $existingShares + * @param int $shareType + * @param int $page + * @param int $perPage + * @param bool $shareWithGroupOnly + * @return \OC_OCS_Result + */ + protected function searchSharees($search, $itemType, array $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + $sharedUsers = $sharedGroups = []; if (!empty($existingShares)) { if (!empty($existingShares[\OCP\Share::SHARE_TYPE_USER]) && @@ -203,8 +226,6 @@ class Sharees { return new \OC_OCS_Result(null, 400, 'missing itemType'); } - $shareWithGroupOnly = $this->appConfig->getValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes' ? true : false; - $sharees = []; // Get users if ($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_USER) { @@ -267,7 +288,7 @@ class Sharees { * @param array $existingSharees * @return array */ - private function filterSharees($potentialSharees, $existingSharees) { + protected function filterSharees($potentialSharees, $existingSharees) { $sharees = array_map(function ($sharee) use ($existingSharees) { return in_array($sharee['value']['shareWith'], $existingSharees) ? null : $sharee; }, $potentialSharees); diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 04b8ef5025..93a8d5c3d7 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -344,39 +344,191 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $users); } -// public function testArguments() { -// -// } -// -// public function testsOnlyUsers() { -// -// } -// -// public function testOnlyGroups() { -// -// } -// -// public function testRemoteAddress() { -// -// } -// -// public function testRemoteFromContacts() { -// -// } -// -// public function testAll() { -// -// } -// -// public function testSorting() { -// -// } -// -// public function testPagination() { -// -// } -// -// public function testShareWithinGroup() { -// -// } + public function dataSearchSharees() { + return [ + ['test', 'folder', [], null, 1, 2, false, [], [], [], [], 0, false], + ['test', 'folder', [0 => ['test1'], 1 => ['test2 group']], null, 1, 2, false, [], [], [], [], 0, false], + // First page with 2 of 3 results + [ + 'test', 'folder', [], null, 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], [ + ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ], [ + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ], 3, true, + ], + // Second page with the 3rd result + [ + 'test', 'folder', [], null, 2, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], [ + ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ], [ + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], [ + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], 3, false, + ], + // Ingnoring already shared user + [ + 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], null, 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], [ + ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ], [ + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], [ + ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], 2, false, + ], + // Share type restricted to user - Only one user + [ + 'test', 'folder', [], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], null, null, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], 1, false, + ], + // Share type restricted to user - Multipage result + [ + 'test', 'folder', [], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ['label' => 'test 3', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test3']], + ], null, null, [ + ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], 3, true, + ], + // Share type restricted to user - Only user already shared + [ + 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], null, null, [], 0, false, + ], + ]; + } + + /** + * @dataProvider dataSearchSharees + * + * @param string $searchTerm + * @param string $itemType + * @param array $existingShares + * @param int $shareType + * @param int $page + * @param int $perPage + * @param bool $shareWithGroupOnly + * @param array $expected + */ + public function testSearchSharees($searchTerm, $itemType, array $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly, + $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $totalItems, $nextLink) { + /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ + $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') + ->setConstructorArgs([ + $this->groupManager, + $this->userManager, + $this->contactsManager, + $this->getMockBuilder('OCP\IAppConfig')->disableOriginalConstructor()->getMock(), + $this->session, + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + ]) + ->setMethods(array('getUsers', 'getGroups', 'getRemote')) + ->getMock(); + $sharees->expects(($mockedUserResult === null) ? $this->never() : $this->once()) + ->method('getUsers') + ->with($searchTerm, $shareWithGroupOnly) + ->willReturn($mockedUserResult); + $sharees->expects(($mockedGroupsResult === null) ? $this->never() : $this->once()) + ->method('getGroups') + ->with($searchTerm, $shareWithGroupOnly) + ->willReturn($mockedGroupsResult); + $sharees->expects(($mockedRemotesResult === null) ? $this->never() : $this->once()) + ->method('getRemote') + ->with($searchTerm) + ->willReturn($mockedRemotesResult); + + /** @var \OC_OCS_Result $ocs */ + $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly]); + + $this->assertEquals($expected, $ocs->getData()); + + // Check number of total results + $meta = $ocs->getMeta(); + $this->assertArrayHasKey('totalitems', $meta); + $this->assertSame($totalItems, $meta['totalitems']); + + // Check if next link is set + if ($nextLink) { + $headers = $ocs->getHeaders(); + $this->assertArrayHasKey('Link', $headers); + $this->assertStringStartsWith('<', $headers['Link']); + $this->assertStringEndsWith('> rel="next"', $headers['Link']); + } + } + + public function testSearchShareesNoItemType() { + /** @var \OC_OCS_Result $ocs */ + $ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], null, 0, 0, false]); + + $this->assertSame(400, $ocs->getStatusCode(), 'Expected status code 400'); + $this->assertSame([], $ocs->getData(), 'Expected that no data is send'); + + $meta = $ocs->getMeta(); + $this->assertNotEmpty($meta); + $this->assertArrayHasKey('message', $meta); + $this->assertSame('missing itemType', $meta['message']); + } + + + public function dataFilterSharees() { + return [ + [[], [], []], + [ + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + [], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + ], + [ + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + ['test1'], + [ + 1 => ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + ], + [ + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + ['test2'], + [ + 0 => ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + ], + ]; + } + + /** + * @dataProvider dataFilterSharees + * + * @param array $potentialSharees + * @param array $existingSharees + * @param array $expectedSharees + */ + public function testFilterSharees($potentialSharees, $existingSharees, $expectedSharees) { + $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'filterSharees', [$potentialSharees, $existingSharees])); + } } From 327c47a9894cb6f8730e5824e3bda1b5e51db9d5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 12 Aug 2015 14:23:48 +0200 Subject: [PATCH 11/24] Do not use deprecated method in new code --- apps/files_sharing/api/sharees.php | 17 ++++++++--------- apps/files_sharing/appinfo/routes.php | 2 +- apps/files_sharing/tests/api/sharees.php | 4 ++-- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 573fd8f2ab..4ae6784c3d 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -23,7 +23,7 @@ namespace OCA\Files_Sharing\API; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUserManager; -use OCP\IAppConfig; +use OCP\IConfig; use OCP\IUserSession; use OCP\IURLGenerator; @@ -38,8 +38,8 @@ class Sharees { /** @var \OCP\Contacts\IManager */ private $contactsManager; - /** @var IAppConfig */ - private $appConfig; + /** @var IConfig */ + private $config; /** @var IUserSession */ private $userSession; @@ -51,19 +51,19 @@ class Sharees { * @param IGroupManager $groupManager * @param IUserManager $userManager * @param \OCP\Contacts\IManager $contactsManager - * @param IAppConfig $appConfig + * @param IConfig $config * @param IUserSession $userSession */ public function __construct(IGroupManager $groupManager, IUserManager $userManager, \OCP\Contacts\IManager $contactsManager, - IAppConfig $appConfig, + IConfig $config, IUserSession $userSession, IURLGenerator $urlGenerator) { $this->groupManager = $groupManager; $this->userManager = $userManager; $this->contactsManager = $contactsManager; - $this->appConfig = $appConfig; + $this->config = $config; $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; } @@ -178,10 +178,9 @@ class Sharees { } /** - * @param array $params * @return \OC_OCS_Result */ - public function search($params) { + public function search() { $search = isset($_GET['search']) ? (string)$_GET['search'] : ''; $itemType = isset($_GET['itemType']) ? (string)$_GET['itemType'] : null; $existingShares = isset($_GET['existingShares']) ? (array)$_GET['existingShares'] : []; @@ -189,7 +188,7 @@ class Sharees { $page = !empty($_GET['page']) ? intval($_GET['page']) : 1; $perPage = !empty($_GET['limit']) ? intval($_GET['limit']) : 200; - $shareWithGroupOnly = $this->appConfig->getValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; return $this->searchSharees($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly); } diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 498b0eb55e..45ea85ff72 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -105,7 +105,7 @@ API::register('delete', $sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(), \OC::$server->getUserManager(), \OC::$server->getContactsManager(), - \OC::$server->getAppConfig(), + \OC::$server->getConfig(), \OC::$server->getUserSession(), \OC::$server->getURLGenerator()); diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 93a8d5c3d7..863dc193ae 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -63,7 +63,7 @@ class ShareesTest extends TestCase { $this->groupManager, $this->userManager, $this->contactsManager, - $this->getMockBuilder('OCP\IAppConfig')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() ); @@ -434,7 +434,7 @@ class ShareesTest extends TestCase { $this->groupManager, $this->userManager, $this->contactsManager, - $this->getMockBuilder('OCP\IAppConfig')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() ]) From 068a81897e0a307b7823aeab48595ed7b6f613b0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 12 Aug 2015 15:03:50 +0200 Subject: [PATCH 12/24] Add tests for "search()" --- apps/files_sharing/api/sharees.php | 12 +- apps/files_sharing/tests/api/sharees.php | 151 ++++++++++++++++++++++- 2 files changed, 156 insertions(+), 7 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 4ae6784c3d..bfde21deb2 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -181,12 +181,12 @@ class Sharees { * @return \OC_OCS_Result */ public function search() { - $search = isset($_GET['search']) ? (string)$_GET['search'] : ''; - $itemType = isset($_GET['itemType']) ? (string)$_GET['itemType'] : null; - $existingShares = isset($_GET['existingShares']) ? (array)$_GET['existingShares'] : []; - $shareType = isset($_GET['shareType']) ? intval($_GET['shareType']) : null; - $page = !empty($_GET['page']) ? intval($_GET['page']) : 1; - $perPage = !empty($_GET['limit']) ? intval($_GET['limit']) : 200; + $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; + $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; + $existingShares = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; + $shareType = isset($_GET['shareType']) && is_numeric($_GET['shareType']) ? (int) $_GET['shareType'] : null; + $page = !empty($_GET['page']) ? (int) $_GET['page'] : 1; + $perPage = !empty($_GET['limit']) ? (int) $_GET['limit'] : 200; $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 863dc193ae..1a25266fe0 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -344,6 +344,154 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $users); } + public function dataSearch() { + return [ + [[], '', '', null, [], null, 1, 200, false], + + // Test itemType + [[ + 'search' => '', + ], '', '', null, [], null, 1, 200, false], + [[ + 'search' => 'foobar', + ], '', 'foobar', null, [], null, 1, 200, false], + [[ + 'search' => 0, + ], '', '0', null, [], null, 1, 200, false], + + // Test itemType + [[ + 'itemType' => '', + ], '', '', '', [], null, 1, 200, false], + [[ + 'itemType' => 'folder', + ], '', '', 'folder', [], null, 1, 200, false], + [[ + 'itemType' => 0, + ], '', '', '0', [], null, 1, 200, false], + + // Test existingShares + [[ + 'existingShares' => [], + ], '', '', null, [], null, 1, 200, false], + [[ + 'existingShares' => [0 => ['test'], 1 => ['foobar']], + ], '', '', null, [0 => ['test'], 1 => ['foobar']], null, 1, 200, false], + + // Test shareType + [[ + ], '', '', null, [], null, 1, 200, false], + [[ + 'shareType' => 0, + ], '', '', null, [], 0, 1, 200, false], + [[ + 'shareType' => '0', + ], '', '', null, [], 0, 1, 200, false], + [[ + 'shareType' => 1, + ], '', '', null, [], 1, 1, 200, false], + [[ + 'shareType' => 10, + ], '', '', null, [], 10, 1, 200, false], + [[ + 'shareType' => 'foobar', + ], '', '', null, [], null, 1, 200, false], + + // Test pagination + [[ + 'page' => 0, + ], '', '', null, [], null, 1, 200, false], + [[ + 'page' => '0', + ], '', '', null, [], null, 1, 200, false], + [[ + 'page' => 1, + ], '', '', null, [], null, 1, 200, false], + [[ + 'page' => 10, + ], '', '', null, [], null, 10, 200, false], + + // Test limit + [[ + 'limit' => 0, + ], '', '', null, [], null, 1, 200, false], + [[ + 'limit' => '0', + ], '', '', null, [], null, 1, 200, false], + [[ + 'limit' => 1, + ], '', '', null, [], null, 1, 1, false], + [[ + 'limit' => 10, + ], '', '', null, [], null, 1, 10, false], + + // Test $shareWithGroupOnly setting + [[], 'no', '', null, [], null, 1, 200, false], + [[], 'yes', '', null, [], null, 1, 200, true], + + ]; + } + + /** + * @dataProvider dataSearch + * + * @param array $getData + * @param string $apiSetting + * @param string $search + * @param string $itemType + * @param array $existingShares + * @param int $shareType + * @param int $page + * @param int $perPage + * @param bool $shareWithGroupOnly + */ + public function testSearch($getData, $apiSetting, $search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + $oldGet = $_GET; + $_GET = $getData; + + $config = $this->getMockBuilder('OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + $config->expects($this->once()) + ->method('getAppValue') + ->with('core', 'shareapi_only_share_with_group_members', 'no') + ->willReturn($apiSetting); + + $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') + ->setConstructorArgs([ + $this->groupManager, + $this->userManager, + $this->contactsManager, + $config, + $this->session, + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + ]) + ->setMethods(array('searchSharees')) + ->getMock(); + $sharees->expects($this->once()) + ->method('searchSharees') + ->with($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) + ->willReturnCallback(function + ($isearch, $iitemType, $iexistingShares, $ishareType, $ipage, $iperPage, $ishareWithGroupOnly) + use ($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + + // We are doing strict comparisons here, so we can differ 0/'' and null on shareType/itemType + $this->assertSame($search, $isearch); + $this->assertSame($itemType, $iitemType); + $this->assertSame($existingShares, $iexistingShares); + $this->assertSame($shareType, $ishareType); + $this->assertSame($page, $ipage); + $this->assertSame($perPage, $iperPage); + $this->assertSame($shareWithGroupOnly, $ishareWithGroupOnly); + return new \OC_OCS_Result([]); + }); + + /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ + $this->assertInstanceOf('\OC_OCS_Result', $sharees->search()); + + $_GET = $oldGet; + } + public function dataSearchSharees() { return [ ['test', 'folder', [], null, 1, 2, false, [], [], [], [], 0, false], @@ -455,6 +603,7 @@ class ShareesTest extends TestCase { /** @var \OC_OCS_Result $ocs */ $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly]); + $this->assertInstanceOf('\OC_OCS_Result', $ocs); $this->assertEquals($expected, $ocs->getData()); @@ -475,6 +624,7 @@ class ShareesTest extends TestCase { public function testSearchShareesNoItemType() { /** @var \OC_OCS_Result $ocs */ $ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], null, 0, 0, false]); + $this->assertInstanceOf('\OC_OCS_Result', $ocs); $this->assertSame(400, $ocs->getStatusCode(), 'Expected status code 400'); $this->assertSame([], $ocs->getData(), 'Expected that no data is send'); @@ -485,7 +635,6 @@ class ShareesTest extends TestCase { $this->assertSame('missing itemType', $meta['message']); } - public function dataFilterSharees() { return [ [[], [], []], From c6ed40c9f891454d8f9ceeb67c45ff3d8ce22537 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 12 Aug 2015 17:05:20 +0200 Subject: [PATCH 13/24] Make shareType an array --- apps/files_sharing/api/sharees.php | 57 +++++++-- apps/files_sharing/tests/api/sharees.php | 145 ++++++++++++++++------- 2 files changed, 144 insertions(+), 58 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index bfde21deb2..de5f9c8cbf 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -184,13 +184,46 @@ class Sharees { $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; $existingShares = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; - $shareType = isset($_GET['shareType']) && is_numeric($_GET['shareType']) ? (int) $_GET['shareType'] : null; - $page = !empty($_GET['page']) ? (int) $_GET['page'] : 1; - $perPage = !empty($_GET['limit']) ? (int) $_GET['limit'] : 200; + $page = !empty($_GET['page']) ? max(1, (int) $_GET['page']) : 1; + $perPage = !empty($_GET['limit']) ? max(1, (int) $_GET['limit']) : 200; + + $shareTypes = [ + \OCP\Share::SHARE_TYPE_USER, + \OCP\Share::SHARE_TYPE_GROUP, + \OCP\Share::SHARE_TYPE_REMOTE, + ]; + if (isset($_GET['shareType']) && is_array($_GET['shareType'])) { + $shareTypes = array_intersect($shareTypes, $_GET['shareType']); + sort($shareTypes); + + } else if (isset($_GET['shareType']) && is_numeric($_GET['shareType'])) { + $shareTypes = array_intersect($shareTypes, [(int) $_GET['shareType']]); + sort($shareTypes); + } + + if (in_array(\OCP\Share::SHARE_TYPE_REMOTE, $shareTypes) && !$this->isRemoteSharingAllowed($itemType)) { + // Remove remote shares from type array, because it is not allowed. + $shareTypes = array_diff($shareTypes, [\OCP\Share::SHARE_TYPE_REMOTE]); + } $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; - return $this->searchSharees($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly); + return $this->searchSharees($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly); + } + + /** + * Method to get out the static call for better testing + * + * @param string $itemType + * @return bool + */ + protected function isRemoteSharingAllowed($itemType) { + try { + $backend = \OCP\Share::getBackend($itemType); + return $backend->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE); + } catch (\Exception $e) { + return false; + } } /** @@ -199,13 +232,13 @@ class Sharees { * @param string $search * @param string $itemType * @param array $existingShares - * @param int $shareType + * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly * @return \OC_OCS_Result */ - protected function searchSharees($search, $itemType, array $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + protected function searchSharees($search, $itemType, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly) { $sharedUsers = $sharedGroups = []; if (!empty($existingShares)) { @@ -227,20 +260,19 @@ class Sharees { $sharees = []; // Get users - if ($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_USER) { + if (in_array(\OCP\Share::SHARE_TYPE_USER, $shareTypes)) { $potentialSharees = $this->getUsers($search, $shareWithGroupOnly); $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedUsers)); } // Get groups - if ($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_GROUP) { + if (in_array(\OCP\Share::SHARE_TYPE_GROUP, $shareTypes)) { $potentialSharees = $this->getGroups($search, $shareWithGroupOnly); $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedGroups)); } // Get remote - if (($shareType === null || $shareType === \OCP\Share::SHARE_TYPE_REMOTE) && - \OCP\Share::getBackend($itemType)->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE)) { + if (in_array(\OCP\Share::SHARE_TYPE_REMOTE, $shareTypes)) { $sharees = array_merge($sharees, $this->getRemote($search)); } @@ -267,12 +299,11 @@ class Sharees { 'search' => $search, 'itemType' => $itemType, 'existingShares' => $existingShares, + 'shareType' => $shareTypes, 'page' => $page + 1, 'limit' => $perPage, ]; - if ($shareType !== null) { - $params['shareType'] = $shareType; - } + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?' . http_build_query($params); $response->addHeader('Link', '<' . $url . '> rel="next"'); } diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 1a25266fe0..ec19036bb8 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -345,89 +345,109 @@ class ShareesTest extends TestCase { } public function dataSearch() { + $allTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE]; + return [ - [[], '', '', null, [], null, 1, 200, false], + [[], '', true, '', null, [], $allTypes, 1, 200, false], // Test itemType [[ 'search' => '', - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'search' => 'foobar', - ], '', 'foobar', null, [], null, 1, 200, false], + ], '', true, 'foobar', null, [], $allTypes, 1, 200, false], [[ 'search' => 0, - ], '', '0', null, [], null, 1, 200, false], + ], '', true, '0', null, [], $allTypes, 1, 200, false], // Test itemType [[ 'itemType' => '', - ], '', '', '', [], null, 1, 200, false], + ], '', true, '', '', [], $allTypes, 1, 200, false], [[ 'itemType' => 'folder', - ], '', '', 'folder', [], null, 1, 200, false], + ], '', true, '', 'folder', [], $allTypes, 1, 200, false], [[ 'itemType' => 0, - ], '', '', '0', [], null, 1, 200, false], + ], '', true, '', '0', [], $allTypes, 1, 200, false], // Test existingShares [[ 'existingShares' => [], - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'existingShares' => [0 => ['test'], 1 => ['foobar']], - ], '', '', null, [0 => ['test'], 1 => ['foobar']], null, 1, 200, false], + ], '', true, '', null, [0 => ['test'], 1 => ['foobar']], $allTypes, 1, 200, false], // Test shareType [[ - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'shareType' => 0, - ], '', '', null, [], 0, 1, 200, false], + ], '', true, '', null, [], [0], 1, 200, false], [[ 'shareType' => '0', - ], '', '', null, [], 0, 1, 200, false], + ], '', true, '', null, [], [0], 1, 200, false], [[ 'shareType' => 1, - ], '', '', null, [], 1, 1, 200, false], + ], '', true, '', null, [], [1], 1, 200, false], [[ - 'shareType' => 10, - ], '', '', null, [], 10, 1, 200, false], + 'shareType' => 12, + ], '', true, '', null, [], [], 1, 200, false], [[ 'shareType' => 'foobar', - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], + [[ + 'shareType' => [0, 1, 2], + ], '', true, '', null, [], [0, 1], 1, 200, false], + [[ + 'shareType' => [0, 1], + ], '', true, '', null, [], [0, 1], 1, 200, false], + [[ + 'shareType' => $allTypes, + ], '', true, '', null, [], $allTypes, 1, 200, false], + [[ + 'shareType' => $allTypes, + ], '', false, '', null, [], [0, 1], 1, 200, false], // Test pagination [[ 'page' => 0, - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'page' => '0', - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], + [[ + 'page' => -1, + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'page' => 1, - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'page' => 10, - ], '', '', null, [], null, 10, 200, false], + ], '', true, '', null, [], $allTypes, 10, 200, false], // Test limit [[ 'limit' => 0, - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], [[ 'limit' => '0', - ], '', '', null, [], null, 1, 200, false], + ], '', true, '', null, [], $allTypes, 1, 200, false], + [[ + 'limit' => -1, + ], '', true, '', null, [], $allTypes, 1, 1, false], [[ 'limit' => 1, - ], '', '', null, [], null, 1, 1, false], + ], '', true, '', null, [], $allTypes, 1, 1, false], [[ 'limit' => 10, - ], '', '', null, [], null, 1, 10, false], + ], '', true, '', null, [], $allTypes, 1, 10, false], // Test $shareWithGroupOnly setting - [[], 'no', '', null, [], null, 1, 200, false], - [[], 'yes', '', null, [], null, 1, 200, true], + [[], 'no', true, '', null, [], $allTypes, 1, 200, false], + [[], 'yes', true, '', null, [], $allTypes, 1, 200, true], ]; } @@ -437,15 +457,16 @@ class ShareesTest extends TestCase { * * @param array $getData * @param string $apiSetting + * @param bool $remoteSharingEnabled * @param string $search * @param string $itemType * @param array $existingShares - * @param int $shareType + * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly */ - public function testSearch($getData, $apiSetting, $search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + public function testSearch($getData, $apiSetting, $remoteSharingEnabled, $search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) { $oldGet = $_GET; $_GET = $getData; @@ -466,25 +487,29 @@ class ShareesTest extends TestCase { $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() ]) - ->setMethods(array('searchSharees')) + ->setMethods(array('searchSharees', 'isRemoteSharingAllowed')) ->getMock(); $sharees->expects($this->once()) ->method('searchSharees') - ->with($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) + ->with($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) ->willReturnCallback(function - ($isearch, $iitemType, $iexistingShares, $ishareType, $ipage, $iperPage, $ishareWithGroupOnly) - use ($search, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly) { + ($isearch, $iitemType, $iexistingShares, $ishareTypes, $ipage, $iperPage, $ishareWithGroupOnly) + use ($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) { // We are doing strict comparisons here, so we can differ 0/'' and null on shareType/itemType $this->assertSame($search, $isearch); $this->assertSame($itemType, $iitemType); $this->assertSame($existingShares, $iexistingShares); - $this->assertSame($shareType, $ishareType); + $this->assertSame($shareTypes, $ishareTypes); $this->assertSame($page, $ipage); $this->assertSame($perPage, $iperPage); $this->assertSame($shareWithGroupOnly, $ishareWithGroupOnly); return new \OC_OCS_Result([]); }); + $sharees->expects($this->any()) + ->method('isRemoteSharingAllowed') + ->with($itemType) + ->willReturn($remoteSharingEnabled); /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ $this->assertInstanceOf('\OC_OCS_Result', $sharees->search()); @@ -492,13 +517,32 @@ class ShareesTest extends TestCase { $_GET = $oldGet; } + public function dataIsRemoteSharingAllowed() { + return [ + ['file', true], + ['folder', true], + ['', false], + ['contacts', false], + ]; + } + + /** + * @dataProvider dataIsRemoteSharingAllowed + * + * @param string $itemType + * @param bool $expected + */ + public function testIsRemoteSharingAllowed($itemType, $expected) { + $this->assertSame($expected, $this->invokePrivate($this->sharees, 'isRemoteSharingAllowed', [$itemType])); + } + public function dataSearchSharees() { return [ - ['test', 'folder', [], null, 1, 2, false, [], [], [], [], 0, false], - ['test', 'folder', [0 => ['test1'], 1 => ['test2 group']], null, 1, 2, false, [], [], [], [], 0, false], + ['test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], + ['test', 'folder', [0 => ['test1'], 1 => ['test2 group']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], // First page with 2 of 3 results [ - 'test', 'folder', [], null, 1, 2, false, [ + 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -511,7 +555,7 @@ class ShareesTest extends TestCase { ], // Second page with the 3rd result [ - 'test', 'folder', [], null, 2, 2, false, [ + 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 2, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -521,9 +565,20 @@ class ShareesTest extends TestCase { ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], ], 3, false, ], + // No groups requested + [ + 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], null, [ + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], [ + ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], 2, false, + ], // Ingnoring already shared user [ - 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], null, 1, 2, false, [ + 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -536,7 +591,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Only one user [ - 'test', 'folder', [], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], null, null, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], @@ -544,7 +599,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Multipage result [ - 'test', 'folder', [], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ['label' => 'test 3', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test3']], @@ -555,7 +610,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Only user already shared [ - 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], \OCP\Share::SHARE_TYPE_USER, 1, 2, false, [ + 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], null, null, [], 0, false, ], @@ -568,13 +623,13 @@ class ShareesTest extends TestCase { * @param string $searchTerm * @param string $itemType * @param array $existingShares - * @param int $shareType + * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly * @param array $expected */ - public function testSearchSharees($searchTerm, $itemType, array $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly, + public function testSearchSharees($searchTerm, $itemType, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly, $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $totalItems, $nextLink) { /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') @@ -602,7 +657,7 @@ class ShareesTest extends TestCase { ->willReturn($mockedRemotesResult); /** @var \OC_OCS_Result $ocs */ - $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $existingShares, $shareType, $page, $perPage, $shareWithGroupOnly]); + $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly]); $this->assertInstanceOf('\OC_OCS_Result', $ocs); $this->assertEquals($expected, $ocs->getData()); @@ -623,7 +678,7 @@ class ShareesTest extends TestCase { public function testSearchShareesNoItemType() { /** @var \OC_OCS_Result $ocs */ - $ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], null, 0, 0, false]); + $ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], [], 0, 0, false]); $this->assertInstanceOf('\OC_OCS_Result', $ocs); $this->assertSame(400, $ocs->getStatusCode(), 'Expected status code 400'); From a0ab7a257858b305434255df2f33ef4b323ac81d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 13 Aug 2015 10:57:08 +0200 Subject: [PATCH 14/24] Add all possible links next, prev, first and last --- apps/files_sharing/api/sharees.php | 54 +++++++++++++++++------- apps/files_sharing/tests/api/sharees.php | 50 +++++++++++++++++++++- 2 files changed, 88 insertions(+), 16 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index de5f9c8cbf..714b4950c5 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -285,7 +285,6 @@ class Sharees { //Pagination $start = ($page - 1) * $perPage; - $end = $page * $perPage; $total = sizeof($sharees); $sharees = array_slice($sharees, $start, $perPage); @@ -294,18 +293,16 @@ class Sharees { $response->setTotalItems($total); $response->setItemsPerPage($perPage); - if ($total > $end) { - $params = [ - 'search' => $search, - 'itemType' => $itemType, - 'existingShares' => $existingShares, - 'shareType' => $shareTypes, - 'page' => $page + 1, - 'limit' => $perPage, - ]; + $links = $this->getPaginationLinks($page, $total, [ + 'search' => $search, + 'itemType' => $itemType, + 'existingShares' => $existingShares, + 'shareType' => $shareTypes, + 'limit' => $perPage, + ]); - $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?' . http_build_query($params); - $response->addHeader('Link', '<' . $url . '> rel="next"'); + if (!empty($links)) { + $response->addHeader('Link', implode(', ', $links)); } return $response; @@ -319,10 +316,37 @@ class Sharees { * @return array */ protected function filterSharees($potentialSharees, $existingSharees) { - $sharees = array_map(function ($sharee) use ($existingSharees) { + $sharees = array_filter($potentialSharees, function ($sharee) use ($existingSharees) { return in_array($sharee['value']['shareWith'], $existingSharees) ? null : $sharee; - }, $potentialSharees); + }); - return array_filter($sharees); + return $sharees; + } + + /** + * Generates a bunch of pagination links for the current page + * + * @param int $page Current page + * @param int $total Number of total items that need to be paginated + * @param array $params Parameters for the URL + * @return array + */ + protected function getPaginationLinks($page, $total, array $params) { + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?'; + + $links = []; + if ($page > 1) { + $params['page'] = 1; + $links[] = '<' . $url . http_build_query($params) . '>; rel="first"'; + $params['page'] = $page - 1; + $links[] = '<' . $url . http_build_query($params) . '>; rel="prev"'; + } + if ($page * $params['limit'] < $total) { + $params['page'] = $page + 1; + $links[] = '<' . $url . http_build_query($params) . '>; rel="next"'; + $params['page'] = ceil($total / $params['limit']); + $links[] = '<' . $url . http_build_query($params) . '>; rel="last"'; + } + return $links; } } diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index ec19036bb8..7efd29e592 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -672,7 +672,7 @@ class ShareesTest extends TestCase { $headers = $ocs->getHeaders(); $this->assertArrayHasKey('Link', $headers); $this->assertStringStartsWith('<', $headers['Link']); - $this->assertStringEndsWith('> rel="next"', $headers['Link']); + $this->assertStringEndsWith('"', $headers['Link']); } } @@ -735,4 +735,52 @@ class ShareesTest extends TestCase { public function testFilterSharees($potentialSharees, $existingSharees, $expectedSharees) { $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'filterSharees', [$potentialSharees, $existingSharees])); } + + public function dataGetPaginationLinks() { + return [ + [1, 1, ['limit' => 2], []], + [1, 3, ['limit' => 2], [ + '; rel="next"', + '; rel="last"', + ]], + [1, 21, ['limit' => 2], [ + '; rel="next"', + '; rel="last"', + ]], + [2, 21, ['limit' => 2], [ + '; rel="first"', + '; rel="prev"', + '; rel="next"', + '; rel="last"', + ]], + [5, 21, ['limit' => 2], [ + '; rel="first"', + '; rel="prev"', + '; rel="next"', + '; rel="last"', + ]], + [10, 21, ['limit' => 2], [ + '; rel="first"', + '; rel="prev"', + '; rel="next"', + '; rel="last"', + ]], + [11, 21, ['limit' => 2], [ + '; rel="first"', + '; rel="prev"', + ]], + ]; + } + + /** + * @dataProvider dataGetPaginationLinks + * + * @param int $page + * @param int $total + * @param array $params + * @param array $expected + */ + public function testGetPaginationLinks($page, $total, $params, $expected) { + $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'getPaginationLinks', [$page, $total, $params])); + } } From 5c4fbf519133cc0e017581a29151911f685a64b0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 13 Aug 2015 11:06:03 +0200 Subject: [PATCH 15/24] Inject the logger as well --- apps/files_sharing/api/sharees.php | 63 ++++++++++++++---------- apps/files_sharing/appinfo/routes.php | 3 +- apps/files_sharing/tests/api/sharees.php | 9 ++-- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 714b4950c5..129a66851a 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -20,12 +20,16 @@ */ namespace OCA\Files_Sharing\API; +use OC\Share\SearchResultSorter; +use OCP\Contacts\IManager; use OCP\IGroup; use OCP\IGroupManager; +use OCP\ILogger; use OCP\IUserManager; use OCP\IConfig; use OCP\IUserSession; use OCP\IURLGenerator; +use OCP\Share; class Sharees { @@ -35,7 +39,7 @@ class Sharees { /** @var IUserManager */ private $userManager; - /** @var \OCP\Contacts\IManager */ + /** @var IManager */ private $contactsManager; /** @var IConfig */ @@ -47,25 +51,32 @@ class Sharees { /** @var IURLGenerator */ private $urlGenerator; + /** @var ILogger */ + private $logger; + /** * @param IGroupManager $groupManager * @param IUserManager $userManager - * @param \OCP\Contacts\IManager $contactsManager + * @param IManager $contactsManager * @param IConfig $config * @param IUserSession $userSession + * @param IURLGenerator $urlGenerator + * @param ILogger $logger */ public function __construct(IGroupManager $groupManager, IUserManager $userManager, - \OCP\Contacts\IManager $contactsManager, + IManager $contactsManager, IConfig $config, IUserSession $userSession, - IURLGenerator $urlGenerator) { + IURLGenerator $urlGenerator, + ILogger $logger) { $this->groupManager = $groupManager; $this->userManager = $userManager; $this->contactsManager = $contactsManager; $this->config = $config; $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; + $this->logger = $logger; } /** @@ -100,7 +111,7 @@ class Sharees { $sharees[] = [ 'label' => $displayName, 'value' => [ - 'shareType' => \OCP\Share::SHARE_TYPE_USER, + 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, ], ]; @@ -131,7 +142,7 @@ class Sharees { $sharees[] = [ 'label' => $gid, 'value' => [ - 'shareType' => \OCP\Share::SHARE_TYPE_GROUP, + 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $gid, ], ]; @@ -152,7 +163,7 @@ class Sharees { $sharees[] = [ 'label' => $search, 'value' => [ - 'shareType' => \OCP\Share::SHARE_TYPE_REMOTE, + 'shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => $search, ], ]; @@ -166,7 +177,7 @@ class Sharees { $sharees[] = [ 'label' => $contact['FN'] . ' (' . $cloudId . ')', 'value' => [ - 'shareType' => \OCP\Share::SHARE_TYPE_REMOTE, + 'shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => $cloudId ] ]; @@ -188,9 +199,9 @@ class Sharees { $perPage = !empty($_GET['limit']) ? max(1, (int) $_GET['limit']) : 200; $shareTypes = [ - \OCP\Share::SHARE_TYPE_USER, - \OCP\Share::SHARE_TYPE_GROUP, - \OCP\Share::SHARE_TYPE_REMOTE, + Share::SHARE_TYPE_USER, + Share::SHARE_TYPE_GROUP, + Share::SHARE_TYPE_REMOTE, ]; if (isset($_GET['shareType']) && is_array($_GET['shareType'])) { $shareTypes = array_intersect($shareTypes, $_GET['shareType']); @@ -201,9 +212,9 @@ class Sharees { sort($shareTypes); } - if (in_array(\OCP\Share::SHARE_TYPE_REMOTE, $shareTypes) && !$this->isRemoteSharingAllowed($itemType)) { + if (in_array(Share::SHARE_TYPE_REMOTE, $shareTypes) && !$this->isRemoteSharingAllowed($itemType)) { // Remove remote shares from type array, because it is not allowed. - $shareTypes = array_diff($shareTypes, [\OCP\Share::SHARE_TYPE_REMOTE]); + $shareTypes = array_diff($shareTypes, [Share::SHARE_TYPE_REMOTE]); } $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; @@ -219,8 +230,8 @@ class Sharees { */ protected function isRemoteSharingAllowed($itemType) { try { - $backend = \OCP\Share::getBackend($itemType); - return $backend->isShareTypeAllowed(\OCP\Share::SHARE_TYPE_REMOTE); + $backend = Share::getBackend($itemType); + return $backend->isShareTypeAllowed(Share::SHARE_TYPE_REMOTE); } catch (\Exception $e) { return false; } @@ -242,14 +253,14 @@ class Sharees { $sharedUsers = $sharedGroups = []; if (!empty($existingShares)) { - if (!empty($existingShares[\OCP\Share::SHARE_TYPE_USER]) && - is_array($existingShares[\OCP\Share::SHARE_TYPE_USER])) { - $sharedUsers = $existingShares[\OCP\Share::SHARE_TYPE_USER]; + if (!empty($existingShares[Share::SHARE_TYPE_USER]) && + is_array($existingShares[Share::SHARE_TYPE_USER])) { + $sharedUsers = $existingShares[Share::SHARE_TYPE_USER]; } - if (!empty($existingShares[\OCP\Share::SHARE_TYPE_GROUP]) && - is_array($existingShares[\OCP\Share::SHARE_TYPE_GROUP])) { - $sharedGroups = $existingShares[\OCP\Share::SHARE_TYPE_GROUP]; + if (!empty($existingShares[Share::SHARE_TYPE_GROUP]) && + is_array($existingShares[Share::SHARE_TYPE_GROUP])) { + $sharedGroups = $existingShares[Share::SHARE_TYPE_GROUP]; } } @@ -260,27 +271,25 @@ class Sharees { $sharees = []; // Get users - if (in_array(\OCP\Share::SHARE_TYPE_USER, $shareTypes)) { + if (in_array(Share::SHARE_TYPE_USER, $shareTypes)) { $potentialSharees = $this->getUsers($search, $shareWithGroupOnly); $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedUsers)); } // Get groups - if (in_array(\OCP\Share::SHARE_TYPE_GROUP, $shareTypes)) { + if (in_array(Share::SHARE_TYPE_GROUP, $shareTypes)) { $potentialSharees = $this->getGroups($search, $shareWithGroupOnly); $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedGroups)); } // Get remote - if (in_array(\OCP\Share::SHARE_TYPE_REMOTE, $shareTypes)) { + if (in_array(Share::SHARE_TYPE_REMOTE, $shareTypes)) { $sharees = array_merge($sharees, $this->getRemote($search)); } // Sort sharees - $sorter = new \OC\Share\SearchResultSorter($search, - 'label', - \OC::$server->getLogger()); + $sorter = new SearchResultSorter($search, 'label', $this->logger); usort($sharees, array($sorter, 'sort')); //Pagination diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 45ea85ff72..2000eab651 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -107,7 +107,8 @@ $sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(), \OC::$server->getContactsManager(), \OC::$server->getConfig(), \OC::$server->getUserSession(), - \OC::$server->getURLGenerator()); + \OC::$server->getURLGenerator(), + \OC::$server->getLogger()); API::register('get', '/apps/files_sharing/api/v1/sharees', diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 7efd29e592..ddce7fddcd 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -65,7 +65,8 @@ class ShareesTest extends TestCase { $this->contactsManager, $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, - $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ); } @@ -485,7 +486,8 @@ class ShareesTest extends TestCase { $this->contactsManager, $config, $this->session, - $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ]) ->setMethods(array('searchSharees', 'isRemoteSharingAllowed')) ->getMock(); @@ -639,7 +641,8 @@ class ShareesTest extends TestCase { $this->contactsManager, $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, - $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ]) ->setMethods(array('getUsers', 'getGroups', 'getRemote')) ->getMock(); From 0227cfff081a79627d038f7e30a1bdece9df4d3a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 13 Aug 2015 10:13:23 +0200 Subject: [PATCH 16/24] Take a list of share IDs instead of the user and group names --- apps/files_sharing/api/sharees.php | 68 +++++++++++--- apps/files_sharing/appinfo/routes.php | 3 +- apps/files_sharing/tests/api/sharees.php | 114 ++++++++++++----------- 3 files changed, 118 insertions(+), 67 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 129a66851a..3e802b3028 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -20,8 +20,10 @@ */ namespace OCA\Files_Sharing\API; +use Doctrine\DBAL\Connection; use OC\Share\SearchResultSorter; use OCP\Contacts\IManager; +use OCP\IDBConnection; use OCP\IGroup; use OCP\IGroupManager; use OCP\ILogger; @@ -54,6 +56,9 @@ class Sharees { /** @var ILogger */ private $logger; + /** @var IDBConnection */ + private $connection; + /** * @param IGroupManager $groupManager * @param IUserManager $userManager @@ -62,6 +67,7 @@ class Sharees { * @param IUserSession $userSession * @param IURLGenerator $urlGenerator * @param ILogger $logger + * @param IDBConnection $connection */ public function __construct(IGroupManager $groupManager, IUserManager $userManager, @@ -69,7 +75,8 @@ class Sharees { IConfig $config, IUserSession $userSession, IURLGenerator $urlGenerator, - ILogger $logger) { + ILogger $logger, + IDBConnection $connection) { $this->groupManager = $groupManager; $this->userManager = $userManager; $this->contactsManager = $contactsManager; @@ -77,6 +84,7 @@ class Sharees { $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; $this->logger = $logger; + $this->connection = $connection; } /** @@ -194,7 +202,7 @@ class Sharees { public function search() { $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; - $existingShares = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; + $shareIds = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; $page = !empty($_GET['page']) ? max(1, (int) $_GET['page']) : 1; $perPage = !empty($_GET['limit']) ? max(1, (int) $_GET['limit']) : 200; @@ -219,7 +227,7 @@ class Sharees { $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; - return $this->searchSharees($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly); + return $this->searchSharees($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly); } /** @@ -242,25 +250,27 @@ class Sharees { * * @param string $search * @param string $itemType - * @param array $existingShares + * @param array $shareIds * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly * @return \OC_OCS_Result */ - protected function searchSharees($search, $itemType, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly) { + protected function searchSharees($search, $itemType, array $shareIds, array $shareTypes, $page, $perPage, $shareWithGroupOnly) { $sharedUsers = $sharedGroups = []; - if (!empty($existingShares)) { - if (!empty($existingShares[Share::SHARE_TYPE_USER]) && - is_array($existingShares[Share::SHARE_TYPE_USER])) { - $sharedUsers = $existingShares[Share::SHARE_TYPE_USER]; + $existingSharees = $this->getShareesForShareIds($shareIds); + + if (!empty($existingSharees)) { + if (!empty($existingSharees[Share::SHARE_TYPE_USER]) && + is_array($existingSharees[Share::SHARE_TYPE_USER])) { + $sharedUsers = $existingSharees[Share::SHARE_TYPE_USER]; } - if (!empty($existingShares[Share::SHARE_TYPE_GROUP]) && - is_array($existingShares[Share::SHARE_TYPE_GROUP])) { - $sharedGroups = $existingShares[Share::SHARE_TYPE_GROUP]; + if (!empty($existingSharees[Share::SHARE_TYPE_GROUP]) && + is_array($existingSharees[Share::SHARE_TYPE_GROUP])) { + $sharedGroups = $existingSharees[Share::SHARE_TYPE_GROUP]; } } @@ -305,7 +315,7 @@ class Sharees { $links = $this->getPaginationLinks($page, $total, [ 'search' => $search, 'itemType' => $itemType, - 'existingShares' => $existingShares, + 'existingShares' => $shareIds, 'shareType' => $shareTypes, 'limit' => $perPage, ]); @@ -332,6 +342,38 @@ class Sharees { return $sharees; } + /** + * Get a list of existing share_with's for the given share IDs (if the current user owns them) + * + * @param array $shareIds + * @return array + */ + protected function getShareesForShareIds($shareIds) { + if (empty($shareIds)) { + return []; + } + + $queryBuilder = $this->connection->getQueryBuilder(); + $exprBuilder = $queryBuilder->expr(); + + $queryBuilder->select(['share_type', 'share_with']) + ->from('share') + ->where($exprBuilder->in('id', $queryBuilder->createParameter('shareIds'))) + ->andWhere($exprBuilder->eq('uid_owner', $queryBuilder->createParameter('user'))) + ->andWhere($exprBuilder->isNull('parent')) + ->setParameter('shareIds', $shareIds, Connection::PARAM_INT_ARRAY) + ->setParameter('user', $this->userSession->getUser()->getUID()); + $query = $queryBuilder->execute(); + + $sharees = []; + while ($row = $query->fetch()) { + $sharees[$row['share_type']][] = $row['share_with']; + } + $query->closeCursor(); + + return $sharees; + } + /** * Generates a bunch of pagination links for the current page * diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 2000eab651..28dc3ab967 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -108,7 +108,8 @@ $sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(), \OC::$server->getConfig(), \OC::$server->getUserSession(), \OC::$server->getURLGenerator(), - \OC::$server->getLogger()); + \OC::$server->getLogger(), + \OC::$server->getDatabaseConnection()); API::register('get', '/apps/files_sharing/api/v1/sharees', diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index ddce7fddcd..24a659eb06 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -66,7 +66,8 @@ class ShareesTest extends TestCase { $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), + \OC::$server->getDatabaseConnection() ); } @@ -349,106 +350,106 @@ class ShareesTest extends TestCase { $allTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE]; return [ - [[], '', true, '', null, [], $allTypes, 1, 200, false], + [[], '', true, [], '', null, $allTypes, 1, 200, false], // Test itemType [[ 'search' => '', - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'search' => 'foobar', - ], '', true, 'foobar', null, [], $allTypes, 1, 200, false], + ], '', true, [], 'foobar', null, $allTypes, 1, 200, false], [[ 'search' => 0, - ], '', true, '0', null, [], $allTypes, 1, 200, false], + ], '', true, [], '0', null, $allTypes, 1, 200, false], // Test itemType [[ 'itemType' => '', - ], '', true, '', '', [], $allTypes, 1, 200, false], + ], '', true, [], '', '', $allTypes, 1, 200, false], [[ 'itemType' => 'folder', - ], '', true, '', 'folder', [], $allTypes, 1, 200, false], + ], '', true, [], '', 'folder', $allTypes, 1, 200, false], [[ 'itemType' => 0, - ], '', true, '', '0', [], $allTypes, 1, 200, false], + ], '', true, [], '', '0', $allTypes, 1, 200, false], // Test existingShares [[ 'existingShares' => [], - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ - 'existingShares' => [0 => ['test'], 1 => ['foobar']], - ], '', true, '', null, [0 => ['test'], 1 => ['foobar']], $allTypes, 1, 200, false], + 'existingShares' => [12, 42], + ], '', true, [12, 42], '', null, $allTypes, 1, 200, false], // Test shareType [[ - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'shareType' => 0, - ], '', true, '', null, [], [0], 1, 200, false], + ], '', true, [], '', null, [0], 1, 200, false], [[ 'shareType' => '0', - ], '', true, '', null, [], [0], 1, 200, false], + ], '', true, [], '', null, [0], 1, 200, false], [[ 'shareType' => 1, - ], '', true, '', null, [], [1], 1, 200, false], + ], '', true, [], '', null, [1], 1, 200, false], [[ 'shareType' => 12, - ], '', true, '', null, [], [], 1, 200, false], + ], '', true, [], '', null, [], 1, 200, false], [[ 'shareType' => 'foobar', - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'shareType' => [0, 1, 2], - ], '', true, '', null, [], [0, 1], 1, 200, false], + ], '', true, [], '', null, [0, 1], 1, 200, false], [[ 'shareType' => [0, 1], - ], '', true, '', null, [], [0, 1], 1, 200, false], + ], '', true, [], '', null, [0, 1], 1, 200, false], [[ 'shareType' => $allTypes, - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'shareType' => $allTypes, - ], '', false, '', null, [], [0, 1], 1, 200, false], + ], '', false, [], '', null, [0, 1], 1, 200, false], // Test pagination [[ 'page' => 0, - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'page' => '0', - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'page' => -1, - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'page' => 1, - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'page' => 10, - ], '', true, '', null, [], $allTypes, 10, 200, false], + ], '', true, [], '', null, $allTypes, 10, 200, false], // Test limit [[ 'limit' => 0, - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'limit' => '0', - ], '', true, '', null, [], $allTypes, 1, 200, false], + ], '', true, [], '', null, $allTypes, 1, 200, false], [[ 'limit' => -1, - ], '', true, '', null, [], $allTypes, 1, 1, false], + ], '', true, [], '', null, $allTypes, 1, 1, false], [[ 'limit' => 1, - ], '', true, '', null, [], $allTypes, 1, 1, false], + ], '', true, [], '', null, $allTypes, 1, 1, false], [[ 'limit' => 10, - ], '', true, '', null, [], $allTypes, 1, 10, false], + ], '', true, [], '', null, $allTypes, 1, 10, false], // Test $shareWithGroupOnly setting - [[], 'no', true, '', null, [], $allTypes, 1, 200, false], - [[], 'yes', true, '', null, [], $allTypes, 1, 200, true], + [[], 'no', true, [], '', null, $allTypes, 1, 200, false], + [[], 'yes', true, [], '', null, $allTypes, 1, 200, true], ]; } @@ -459,15 +460,15 @@ class ShareesTest extends TestCase { * @param array $getData * @param string $apiSetting * @param bool $remoteSharingEnabled + * @param array $shareIds * @param string $search * @param string $itemType - * @param array $existingShares * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly */ - public function testSearch($getData, $apiSetting, $remoteSharingEnabled, $search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) { + public function testSearch($getData, $apiSetting, $remoteSharingEnabled, $shareIds, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly) { $oldGet = $_GET; $_GET = $getData; @@ -487,21 +488,22 @@ class ShareesTest extends TestCase { $config, $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), + \OC::$server->getDatabaseConnection() ]) ->setMethods(array('searchSharees', 'isRemoteSharingAllowed')) ->getMock(); $sharees->expects($this->once()) ->method('searchSharees') - ->with($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) + ->with($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly) ->willReturnCallback(function - ($isearch, $iitemType, $iexistingShares, $ishareTypes, $ipage, $iperPage, $ishareWithGroupOnly) - use ($search, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly) { + ($isearch, $iitemType, $ishareIds, $ishareTypes, $ipage, $iperPage, $ishareWithGroupOnly) + use ($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly) { // We are doing strict comparisons here, so we can differ 0/'' and null on shareType/itemType $this->assertSame($search, $isearch); $this->assertSame($itemType, $iitemType); - $this->assertSame($existingShares, $iexistingShares); + $this->assertSame($shareIds, $ishareIds); $this->assertSame($shareTypes, $ishareTypes); $this->assertSame($page, $ipage); $this->assertSame($perPage, $iperPage); @@ -540,11 +542,11 @@ class ShareesTest extends TestCase { public function dataSearchSharees() { return [ - ['test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], - ['test', 'folder', [0 => ['test1'], 1 => ['test2 group']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], + ['test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], + ['test', 'folder', [1, 2], [0 => ['test1'], 1 => ['test2 group']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], // First page with 2 of 3 results [ - 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -557,7 +559,7 @@ class ShareesTest extends TestCase { ], // Second page with the 3rd result [ - 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 2, 2, false, [ + 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 2, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -569,7 +571,7 @@ class ShareesTest extends TestCase { ], // No groups requested [ - 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], null, [ ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], @@ -580,7 +582,7 @@ class ShareesTest extends TestCase { ], // Ingnoring already shared user [ - 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + 'test', 'folder', [1], [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], @@ -593,7 +595,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Only one user [ - 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ + 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], null, null, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], @@ -601,7 +603,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Multipage result [ - 'test', 'folder', [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ + 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ['label' => 'test 3', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test3']], @@ -612,7 +614,7 @@ class ShareesTest extends TestCase { ], // Share type restricted to user - Only user already shared [ - 'test', 'folder', [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ + 'test', 'folder', [1], [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], null, null, [], 0, false, ], @@ -624,6 +626,7 @@ class ShareesTest extends TestCase { * * @param string $searchTerm * @param string $itemType + * @param array $shareIds * @param array $existingShares * @param array $shareTypes * @param int $page @@ -631,7 +634,7 @@ class ShareesTest extends TestCase { * @param bool $shareWithGroupOnly * @param array $expected */ - public function testSearchSharees($searchTerm, $itemType, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly, + public function testSearchSharees($searchTerm, $itemType, array $shareIds, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly, $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $totalItems, $nextLink) { /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') @@ -642,10 +645,15 @@ class ShareesTest extends TestCase { $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), + \OC::$server->getDatabaseConnection() ]) - ->setMethods(array('getUsers', 'getGroups', 'getRemote')) + ->setMethods(array('getShareesForShareIds', 'getUsers', 'getGroups', 'getRemote')) ->getMock(); + $sharees->expects($this->once()) + ->method('getShareesForShareIds') + ->with($shareIds) + ->willReturn($existingShares); $sharees->expects(($mockedUserResult === null) ? $this->never() : $this->once()) ->method('getUsers') ->with($searchTerm, $shareWithGroupOnly) @@ -660,7 +668,7 @@ class ShareesTest extends TestCase { ->willReturn($mockedRemotesResult); /** @var \OC_OCS_Result $ocs */ - $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $existingShares, $shareTypes, $page, $perPage, $shareWithGroupOnly]); + $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly]); $this->assertInstanceOf('\OC_OCS_Result', $ocs); $this->assertEquals($expected, $ocs->getData()); From 83b88c9a264a46f393df95e6c57793f0300495da Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 17 Aug 2015 12:13:49 +0200 Subject: [PATCH 17/24] Do not return the current user himself --- apps/files_sharing/api/sharees.php | 5 +++ apps/files_sharing/tests/api/sharees.php | 44 +++++++++++++++++++++--- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 3e802b3028..f12677265b 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -116,6 +116,11 @@ class Sharees { foreach ($users as $uid => $displayName) { + if ($uid === $this->userSession->getUser()->getUID()) { + // Skip the current user + continue; + } + $sharees[] = [ 'label' => $displayName, 'value' => [ diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 24a659eb06..9b7a21b063 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -127,6 +127,20 @@ class ShareesTest extends TestCase { ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ] ], + [ + 'test', + false, + [], + [ + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), + $this->getUserMock('admin', 'Should be removed'), + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ] + ], [ 'test', true, @@ -175,6 +189,26 @@ class ShareesTest extends TestCase { ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ] ], + [ + 'test', + true, + ['abc', 'xyz'], + [ + ['abc', 'test', -1, 0, [ + 'test1' => 'Test One', + ]], + ['xyz', 'test', -1, 0, [ + 'test2' => 'Test Two', + ]], + ['admin', 'Should be removed', -1, 0, [ + 'test2' => 'Test Two', + ]], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ] + ], ]; } @@ -188,17 +222,17 @@ class ShareesTest extends TestCase { * @param array $expected */ public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $expected) { + $user = $this->getUserMock('admin', 'Administrator'); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($user); + if (!$shareWithGroupOnly) { $this->userManager->expects($this->once()) ->method('searchDisplayName') ->with($searchTerm) ->willReturn($userResponse); } else { - $user = $this->getUserMock('admin', 'Administrator'); - $this->session->expects($this->any()) - ->method('getUser') - ->willReturn($user); - $this->groupManager->expects($this->once()) ->method('getUserGroupIds') ->with($user) From 6b69e7b1dae71601db8486df4c30ab9d73419694 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 17 Aug 2015 12:43:20 +0200 Subject: [PATCH 18/24] Add tests for "getShareesForShareIds()" --- apps/files_sharing/tests/api/sharees.php | 93 ++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/sharees.php index 9b7a21b063..675cc7bede 100644 --- a/apps/files_sharing/tests/api/sharees.php +++ b/apps/files_sharing/tests/api/sharees.php @@ -21,6 +21,8 @@ namespace OCA\Files_Sharing\Tests\API; +use Doctrine\DBAL\Connection; +use OC\Share\Constants; use OCA\Files_Sharing\API\Sharees; use OCA\Files_sharing\Tests\TestCase; @@ -781,6 +783,97 @@ class ShareesTest extends TestCase { $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'filterSharees', [$potentialSharees, $existingSharees])); } + public function dataGetShareesForShareIds() { + return [ + [[], []], + [[1, 2, 3], [Constants::SHARE_TYPE_USER => ['user1'], Constants::SHARE_TYPE_GROUP => ['group1']]], + ]; + } + + /** + * @dataProvider dataGetShareesForShareIds + * + * @param array $shareIds + * @param array $expectedSharees + */ + public function testGetShareesForShareIds(array $shareIds, array $expectedSharees) { + $owner = $this->getUniqueID('test'); + $shares2delete = []; + + if (!empty($shareIds)) { + $userShare = $this->createShare(Constants::SHARE_TYPE_USER, 'user1', $owner, null); + $shares2delete[] = $userShare; + $shares2delete[] = $this->createShare(Constants::SHARE_TYPE_USER, 'user3', $owner . '2', null); + + $groupShare = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group1', $owner, null); + $shares2delete[] = $groupShare; + $groupShare2 = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group2', $owner . '2', null); + $shares2delete[] = $groupShare2; + + $shares2delete[] = $this->createShare($this->invokePrivate(new Constants(), 'shareTypeGroupUserUnique'), 'user2', $owner, $groupShare); + $shares2delete[] = $this->createShare($this->invokePrivate(new Constants(), 'shareTypeGroupUserUnique'), 'user4', $owner, $groupShare2); + } + + $user = $this->getUserMock($owner, 'Sharee OCS test user'); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($user); + $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'getShareesForShareIds', [$shares2delete])); + + $this->deleteShares($shares2delete); + } + + /** + * @param int $type + * @param string $with + * @param string $owner + * @param int $parent + * @return int + */ + protected function createShare($type, $with, $owner, $parent) { + $connection = \OC::$server->getDatabaseConnection(); + $queryBuilder = $connection->getQueryBuilder(); + $queryBuilder->insert('share') + ->values([ + 'share_type' => $queryBuilder->createParameter('share_type'), + 'share_with' => $queryBuilder->createParameter('share_with'), + 'uid_owner' => $queryBuilder->createParameter('uid_owner'), + 'parent' => $queryBuilder->createParameter('parent'), + 'item_type' => $queryBuilder->expr()->literal('fake'), + 'item_source' => $queryBuilder->expr()->literal(''), + 'item_target' => $queryBuilder->expr()->literal(''), + 'file_source' => $queryBuilder->expr()->literal(0), + 'file_target' => $queryBuilder->expr()->literal(''), + 'permissions' => $queryBuilder->expr()->literal(0), + 'stime' => $queryBuilder->expr()->literal(0), + 'accepted' => $queryBuilder->expr()->literal(0), + 'expiration' => $queryBuilder->expr()->literal(''), + 'token' => $queryBuilder->expr()->literal(''), + 'mail_send' => $queryBuilder->expr()->literal(0), + ]) + ->setParameters([ + 'share_type' => $type, + 'share_with' => $with, + 'uid_owner' => $owner, + 'parent' => $parent, + ]) + ->execute(); + return $connection->lastInsertId('share'); + } + + /** + * @param int[] $shareIds + * @return null + */ + protected function deleteShares(array $shareIds) { + $connection = \OC::$server->getDatabaseConnection(); + $queryBuilder = $connection->getQueryBuilder(); + $queryBuilder->delete('share') + ->where($queryBuilder->expr()->in('id', $queryBuilder->createParameter('ids'))) + ->setParameter('ids', $shareIds, Connection::PARAM_INT_ARRAY) + ->execute(); + } + public function dataGetPaginationLinks() { return [ [1, 1, ['limit' => 2], []], From 937586a3f0260bbc976d13637a708c530c708517 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 17 Aug 2015 12:44:23 +0200 Subject: [PATCH 19/24] Rename file to match the class name --- apps/files_sharing/tests/api/{sharees.php => shareestest.php} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename apps/files_sharing/tests/api/{sharees.php => shareestest.php} (100%) diff --git a/apps/files_sharing/tests/api/sharees.php b/apps/files_sharing/tests/api/shareestest.php similarity index 100% rename from apps/files_sharing/tests/api/sharees.php rename to apps/files_sharing/tests/api/shareestest.php From f4186d3dfccc80c6a819c94ece8883c981a9d013 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 19 Aug 2015 10:14:15 +0200 Subject: [PATCH 20/24] Fix wrong value for datetime field --- apps/files_sharing/tests/api/shareestest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index 675cc7bede..c856b93d0f 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -847,7 +847,7 @@ class ShareesTest extends TestCase { 'permissions' => $queryBuilder->expr()->literal(0), 'stime' => $queryBuilder->expr()->literal(0), 'accepted' => $queryBuilder->expr()->literal(0), - 'expiration' => $queryBuilder->expr()->literal(''), + 'expiration' => $queryBuilder->createParameter('expiration'), 'token' => $queryBuilder->expr()->literal(''), 'mail_send' => $queryBuilder->expr()->literal(0), ]) @@ -857,6 +857,7 @@ class ShareesTest extends TestCase { 'uid_owner' => $owner, 'parent' => $parent, ]) + ->setParameter('expiration', null, 'datetime') ->execute(); return $connection->lastInsertId('share'); } From ac8941f6ac919464b672db5961f14604d681c628 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 19 Aug 2015 12:06:00 +0200 Subject: [PATCH 21/24] Manually query for the last id --- apps/files_sharing/tests/api/shareestest.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index c856b93d0f..3b2e17d9fd 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -859,7 +859,17 @@ class ShareesTest extends TestCase { ]) ->setParameter('expiration', null, 'datetime') ->execute(); - return $connection->lastInsertId('share'); + + $queryBuilder = $connection->getQueryBuilder(); + $query = $queryBuilder->select('id') + ->from('share') + ->orderBy('id', 'DESC') + ->setMaxResults(1) + ->execute(); + $share = $query->fetch(); + $query->closeCursor(); + + return (int) $share['id']; } /** From aa2a894eb0b1e7df6384abc56b0d3a375a062936 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 26 Aug 2015 09:40:20 +0200 Subject: [PATCH 22/24] Fix performance issues of the sharees api --- apps/files_sharing/api/sharees.php | 334 ++++++++++++-------------- apps/files_sharing/appinfo/routes.php | 4 +- 2 files changed, 155 insertions(+), 183 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index f12677265b..4454d647b1 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -20,13 +20,12 @@ */ namespace OCA\Files_Sharing\API; -use Doctrine\DBAL\Connection; -use OC\Share\SearchResultSorter; use OCP\Contacts\IManager; -use OCP\IDBConnection; use OCP\IGroup; use OCP\IGroupManager; use OCP\ILogger; +use OCP\IRequest; +use OCP\IUser; use OCP\IUserManager; use OCP\IConfig; use OCP\IUserSession; @@ -50,14 +49,37 @@ class Sharees { /** @var IUserSession */ private $userSession; + /** @var IRequest */ + private $request; + /** @var IURLGenerator */ private $urlGenerator; /** @var ILogger */ private $logger; - /** @var IDBConnection */ - private $connection; + /** @var bool */ + private $shareWithGroupOnly; + + /** @var int */ + protected $offset = 0; + + /** @var int */ + protected $limit = 10; + + /** @var array */ + protected $result = [ + 'exact' => [ + 'users' => [], + 'groups' => [], + 'remotes' => [], + ], + 'users' => [], + 'groups' => [], + 'remotes' => [], + ]; + + protected $reachedEndFor = []; /** * @param IGroupManager $groupManager @@ -66,8 +88,8 @@ class Sharees { * @param IConfig $config * @param IUserSession $userSession * @param IURLGenerator $urlGenerator + * @param IRequest $request * @param ILogger $logger - * @param IDBConnection $connection */ public function __construct(IGroupManager $groupManager, IUserManager $userManager, @@ -75,8 +97,8 @@ class Sharees { IConfig $config, IUserSession $userSession, IURLGenerator $urlGenerator, - ILogger $logger, - IDBConnection $connection) { + IRequest $request, + ILogger $logger) { $this->groupManager = $groupManager; $this->userManager = $userManager; $this->contactsManager = $contactsManager; @@ -84,67 +106,82 @@ class Sharees { $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; $this->logger = $logger; - $this->connection = $connection; } /** * @param string $search - * @param bool $shareWithGroupOnly - * - * @return array possible sharees */ - protected function getUsers($search, $shareWithGroupOnly) { - $sharees = []; - - $users = []; - if ($shareWithGroupOnly) { + protected function getUsers($search) { + $this->result['users'] = $this->result['exact']['users'] = $users = []; + + if ($this->shareWithGroupOnly) { // Search in all the groups this user is part of $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($userGroups as $userGroup) { - $users = array_merge($users, $this->groupManager->displayNamesInGroup($userGroup, $search)); + $users = $this->groupManager->displayNamesInGroup($userGroup, $search, $this->limit, $this->offset); + foreach ($users as $uid => $userDisplayName) { + $users[$uid] = $userDisplayName; + } } - $users = array_unique($users); } else { // Search in all users - $users_tmp = $this->userManager->searchDisplayName($search); + $usersTmp = $this->userManager->searchDisplayName($search, $this->limit, $this->offset); - // Put in array that maps uid => displayName - foreach($users_tmp as $user) { + foreach ($usersTmp as $user) { $users[$user->getUID()] = $user->getDisplayName(); } } - - foreach ($users as $uid => $displayName) { - if ($uid === $this->userSession->getUser()->getUID()) { - // Skip the current user - continue; - } - - $sharees[] = [ - 'label' => $displayName, - 'value' => [ - 'shareType' => Share::SHARE_TYPE_USER, - 'shareWith' => $uid, - ], - ]; + if (sizeof($users) < $this->limit) { + $this->reachedEndFor[] = 'users'; } - return $sharees; + $foundUserById = false; + foreach ($users as $uid => $userDisplayName) { + if ($uid === $search || $userDisplayName === $search) { + if ($uid === $search) { + $foundUserById = true; + } + $this->result['exact']['users'][] = [ + 'shareWith' => $search, + 'label' => $search, + ]; + } else { + $this->result['users'][] = [ + 'shareWith' => $uid, + 'label' => $userDisplayName, + ]; + } + } + + if ($this->offset === 0 && !$foundUserById) { + // On page one we try if the search result has a direct hit on the + // user id and if so, we add that to the exact match list + $user = $this->userManager->get($search); + if ($user instanceof IUser) { + array_push($this->result['exact']['users'], [ + 'shareWith' => $user->getUID(), + 'label' => $user->getDisplayName(), + ]); + } + } } /** * @param string $search - * @param bool $shareWithGroupOnly - * - * @return array possible sharees */ - protected function getGroups($search, $shareWithGroupOnly) { - $sharees = []; - $groups = $this->groupManager->search($search); + protected function getGroups($search) { + $this->result['groups'] = $this->result['exact']['groups'] = []; + + $groups = $this->groupManager->search($search, $this->limit, $this->offset); $groups = array_map(function (IGroup $group) { return $group->getGID(); }, $groups); - if (!empty($groups) && $shareWithGroupOnly) { + if (sizeof($groups) < $this->limit) { + $this->reachedEndFor[] = 'groups'; + } + + $userGroups = []; + if (!empty($groups) && $this->shareWithGroupOnly) { // Intersect all the groups that match with the groups this user is a member of $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); $userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups); @@ -152,53 +189,68 @@ class Sharees { } foreach ($groups as $gid) { - $sharees[] = [ - 'label' => $gid, - 'value' => [ - 'shareType' => Share::SHARE_TYPE_GROUP, + if ($gid === $search) { + $this->result['exact']['groups'][] = [ + 'shareWith' => $search, + 'label' => $search, + ]; + } else { + $this->result['groups'][] = [ 'shareWith' => $gid, - ], - ]; + 'label' => $gid, + ]; + } } - return $sharees; + if ($this->offset === 0 && empty($this->result['exact']['groups'])) { + // On page one we try if the search result has a direct hit on the + // user id and if so, we add that to the exact match list + $group = $this->groupManager->get($search); + if ($group instanceof IGroup && (!$this->shareWithGroupOnly || array_intersect([$group], $userGroups))) { + array_push($this->result['exact']['users'], [ + 'shareWith' => $group->getGID(), + 'label' => $group->getGID(), + ]); + } + } } /** * @param string $search - * * @return array possible sharees */ protected function getRemote($search) { - $sharees = []; + $this->result['remotes'] = []; - if (substr_count($search, '@') >= 1) { - $sharees[] = [ + if (substr_count($search, '@') >= 1 && $this->offset === 0) { + $this->result['exact']['remotes'][] = [ + 'shareWith' => $search, 'label' => $search, - 'value' => [ - 'shareType' => Share::SHARE_TYPE_REMOTE, - 'shareWith' => $search, - ], ]; } // Search in contacts + //@todo Pagination missing $addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN']); foreach ($addressBookContacts as $contact) { if (isset($contact['CLOUD'])) { foreach ($contact['CLOUD'] as $cloudId) { - $sharees[] = [ - 'label' => $contact['FN'] . ' (' . $cloudId . ')', - 'value' => [ - 'shareType' => Share::SHARE_TYPE_REMOTE, - 'shareWith' => $cloudId - ] - ]; + if ($contact['FN'] === $search || $cloudId === $search) { + $this->result['exact']['remotes'][] = [ + 'shareWith' => $cloudId, + 'label' => $contact['FN'], + ]; + } else { + $this->result['remotes'][] = [ + 'shareWith' => $cloudId, + 'label' => $contact['FN'], + ]; + } } } } - return $sharees; + $this->reachedEndFor[] = 'remotes'; } /** @@ -230,9 +282,11 @@ class Sharees { $shareTypes = array_diff($shareTypes, [Share::SHARE_TYPE_REMOTE]); } - $shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes'; + $this->limit = (int) $perPage; + $this->offset = $perPage * ($page - 1); - return $this->searchSharees($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly); + return $this->searchSharees($search, $itemType, $shareIds, $shareTypes, $page, $perPage); } /** @@ -259,150 +313,68 @@ class Sharees { * @param array $shareTypes * @param int $page * @param int $perPage - * @param bool $shareWithGroupOnly * @return \OC_OCS_Result */ - protected function searchSharees($search, $itemType, array $shareIds, array $shareTypes, $page, $perPage, $shareWithGroupOnly) { - - $sharedUsers = $sharedGroups = []; - $existingSharees = $this->getShareesForShareIds($shareIds); - - if (!empty($existingSharees)) { - if (!empty($existingSharees[Share::SHARE_TYPE_USER]) && - is_array($existingSharees[Share::SHARE_TYPE_USER])) { - $sharedUsers = $existingSharees[Share::SHARE_TYPE_USER]; - } - - if (!empty($existingSharees[Share::SHARE_TYPE_GROUP]) && - is_array($existingSharees[Share::SHARE_TYPE_GROUP])) { - $sharedGroups = $existingSharees[Share::SHARE_TYPE_GROUP]; - } - } - + protected function searchSharees($search, $itemType, array $shareIds, array $shareTypes, $page, $perPage) { // Verify arguments if ($itemType === null) { return new \OC_OCS_Result(null, 400, 'missing itemType'); } - $sharees = []; // Get users if (in_array(Share::SHARE_TYPE_USER, $shareTypes)) { - $potentialSharees = $this->getUsers($search, $shareWithGroupOnly); - $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedUsers)); + $this->getUsers($search); } // Get groups if (in_array(Share::SHARE_TYPE_GROUP, $shareTypes)) { - $potentialSharees = $this->getGroups($search, $shareWithGroupOnly); - $sharees = array_merge($sharees, $this->filterSharees($potentialSharees, $sharedGroups)); + $this->getGroups($search); } // Get remote if (in_array(Share::SHARE_TYPE_REMOTE, $shareTypes)) { - $sharees = array_merge($sharees, $this->getRemote($search)); + $this->getRemote($search); } - - // Sort sharees - $sorter = new SearchResultSorter($search, 'label', $this->logger); - usort($sharees, array($sorter, 'sort')); - - //Pagination - $start = ($page - 1) * $perPage; - $total = sizeof($sharees); - - $sharees = array_slice($sharees, $start, $perPage); - - $response = new \OC_OCS_Result($sharees); - $response->setTotalItems($total); + $response = new \OC_OCS_Result($this->result); $response->setItemsPerPage($perPage); - $links = $this->getPaginationLinks($page, $total, [ - 'search' => $search, - 'itemType' => $itemType, - 'existingShares' => $shareIds, - 'shareType' => $shareTypes, - 'limit' => $perPage, - ]); - - if (!empty($links)) { - $response->addHeader('Link', implode(', ', $links)); + if (sizeof($this->reachedEndFor) < 3) { + $response->addHeader('Link', $this->getPaginationLink($page, [ + 'search' => $search, + 'itemType' => $itemType, + 'existingShares' => $shareIds, + 'shareType' => $shareTypes, + 'limit' => $perPage, + ])); } return $response; } - /** - * Filter out already existing shares from a list of potential sharees - * - * @param array $potentialSharees - * @param array $existingSharees - * @return array - */ - protected function filterSharees($potentialSharees, $existingSharees) { - $sharees = array_filter($potentialSharees, function ($sharee) use ($existingSharees) { - return in_array($sharee['value']['shareWith'], $existingSharees) ? null : $sharee; - }); - - return $sharees; - } - - /** - * Get a list of existing share_with's for the given share IDs (if the current user owns them) - * - * @param array $shareIds - * @return array - */ - protected function getShareesForShareIds($shareIds) { - if (empty($shareIds)) { - return []; - } - - $queryBuilder = $this->connection->getQueryBuilder(); - $exprBuilder = $queryBuilder->expr(); - - $queryBuilder->select(['share_type', 'share_with']) - ->from('share') - ->where($exprBuilder->in('id', $queryBuilder->createParameter('shareIds'))) - ->andWhere($exprBuilder->eq('uid_owner', $queryBuilder->createParameter('user'))) - ->andWhere($exprBuilder->isNull('parent')) - ->setParameter('shareIds', $shareIds, Connection::PARAM_INT_ARRAY) - ->setParameter('user', $this->userSession->getUser()->getUID()); - $query = $queryBuilder->execute(); - - $sharees = []; - while ($row = $query->fetch()) { - $sharees[$row['share_type']][] = $row['share_with']; - } - $query->closeCursor(); - - return $sharees; - } - /** * Generates a bunch of pagination links for the current page * * @param int $page Current page - * @param int $total Number of total items that need to be paginated * @param array $params Parameters for the URL - * @return array + * @return string */ - protected function getPaginationLinks($page, $total, array $params) { - $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?'; + protected function getPaginationLink($page, array $params) { + if ($this->isV2()) { + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v2.php/apps/files_sharing/api/v1/sharees') . '?'; + } else { + $url = $this->urlGenerator->getAbsoluteURL('/ocs/v1.php/apps/files_sharing/api/v1/sharees') . '?'; + } + $params['page'] = $page + 1; + $link = '<' . $url . http_build_query($params) . '>; rel="next"'; - $links = []; - if ($page > 1) { - $params['page'] = 1; - $links[] = '<' . $url . http_build_query($params) . '>; rel="first"'; - $params['page'] = $page - 1; - $links[] = '<' . $url . http_build_query($params) . '>; rel="prev"'; - } - if ($page * $params['limit'] < $total) { - $params['page'] = $page + 1; - $links[] = '<' . $url . http_build_query($params) . '>; rel="next"'; - $params['page'] = ceil($total / $params['limit']); - $links[] = '<' . $url . http_build_query($params) . '>; rel="last"'; - } - return $links; + return $link; + } + + /** + * @return bool + */ + protected function isV2() { + return $this->request->getScriptName() === '/ocs/v2.php'; } } diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 28dc3ab967..375124cb73 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -108,8 +108,8 @@ $sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(), \OC::$server->getConfig(), \OC::$server->getUserSession(), \OC::$server->getURLGenerator(), - \OC::$server->getLogger(), - \OC::$server->getDatabaseConnection()); + \OC::$server->getRequest(), + \OC::$server->getLogger()); API::register('get', '/apps/files_sharing/api/v1/sharees', From 2a6e676048f1f8e521e5b6c189e71e02bb2e5005 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 26 Aug 2015 10:51:26 +0200 Subject: [PATCH 23/24] Adjust tests --- apps/files_sharing/api/sharees.php | 77 ++- apps/files_sharing/tests/api/shareestest.php | 688 ++++++++----------- 2 files changed, 355 insertions(+), 410 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 4454d647b1..807b6b3314 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -35,31 +35,31 @@ use OCP\Share; class Sharees { /** @var IGroupManager */ - private $groupManager; + protected $groupManager; /** @var IUserManager */ - private $userManager; + protected $userManager; /** @var IManager */ - private $contactsManager; + protected $contactsManager; /** @var IConfig */ - private $config; + protected $config; /** @var IUserSession */ - private $userSession; + protected $userSession; /** @var IRequest */ - private $request; + protected $request; /** @var IURLGenerator */ - private $urlGenerator; + protected $urlGenerator; /** @var ILogger */ - private $logger; + protected $logger; /** @var bool */ - private $shareWithGroupOnly; + protected $shareWithGroupOnly = false; /** @var int */ protected $offset = 0; @@ -105,6 +105,7 @@ class Sharees { $this->config = $config; $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; + $this->request = $request; $this->logger = $logger; } @@ -118,8 +119,8 @@ class Sharees { // Search in all the groups this user is part of $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($userGroups as $userGroup) { - $users = $this->groupManager->displayNamesInGroup($userGroup, $search, $this->limit, $this->offset); - foreach ($users as $uid => $userDisplayName) { + $usersTmp = $this->groupManager->displayNamesInGroup($userGroup, $search, $this->limit, $this->offset); + foreach ($usersTmp as $uid => $userDisplayName) { $users[$uid] = $userDisplayName; } } @@ -143,13 +144,19 @@ class Sharees { $foundUserById = true; } $this->result['exact']['users'][] = [ - 'shareWith' => $search, - 'label' => $search, + 'label' => $userDisplayName, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_USER, + 'shareWith' => $uid, + ], ]; } else { $this->result['users'][] = [ - 'shareWith' => $uid, 'label' => $userDisplayName, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_USER, + 'shareWith' => $uid, + ], ]; } } @@ -160,8 +167,11 @@ class Sharees { $user = $this->userManager->get($search); if ($user instanceof IUser) { array_push($this->result['exact']['users'], [ - 'shareWith' => $user->getUID(), 'label' => $user->getDisplayName(), + 'value' => [ + 'shareType' => Share::SHARE_TYPE_USER, + 'shareWith' => $user->getUID(), + ], ]); } } @@ -191,13 +201,19 @@ class Sharees { foreach ($groups as $gid) { if ($gid === $search) { $this->result['exact']['groups'][] = [ - 'shareWith' => $search, 'label' => $search, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_GROUP, + 'shareWith' => $search, + ], ]; } else { $this->result['groups'][] = [ - 'shareWith' => $gid, 'label' => $gid, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_GROUP, + 'shareWith' => $gid, + ], ]; } } @@ -208,8 +224,11 @@ class Sharees { $group = $this->groupManager->get($search); if ($group instanceof IGroup && (!$this->shareWithGroupOnly || array_intersect([$group], $userGroups))) { array_push($this->result['exact']['users'], [ - 'shareWith' => $group->getGID(), 'label' => $group->getGID(), + 'value' => [ + 'shareType' => Share::SHARE_TYPE_GROUP, + 'shareWith' => $group->getGID(), + ], ]); } } @@ -224,8 +243,11 @@ class Sharees { if (substr_count($search, '@') >= 1 && $this->offset === 0) { $this->result['exact']['remotes'][] = [ - 'shareWith' => $search, 'label' => $search, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_REMOTE, + 'shareWith' => $search, + ], ]; } @@ -237,13 +259,19 @@ class Sharees { foreach ($contact['CLOUD'] as $cloudId) { if ($contact['FN'] === $search || $cloudId === $search) { $this->result['exact']['remotes'][] = [ - 'shareWith' => $cloudId, 'label' => $contact['FN'], + 'value' => [ + 'shareType' => Share::SHARE_TYPE_REMOTE, + 'shareWith' => $cloudId, + ], ]; } else { $this->result['remotes'][] = [ - 'shareWith' => $cloudId, 'label' => $contact['FN'], + 'value' => [ + 'shareType' => Share::SHARE_TYPE_REMOTE, + 'shareWith' => $cloudId, + ], ]; } } @@ -259,7 +287,6 @@ class Sharees { public function search() { $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; - $shareIds = isset($_GET['existingShares']) ? (array) $_GET['existingShares'] : []; $page = !empty($_GET['page']) ? max(1, (int) $_GET['page']) : 1; $perPage = !empty($_GET['limit']) ? max(1, (int) $_GET['limit']) : 200; @@ -286,7 +313,7 @@ class Sharees { $this->limit = (int) $perPage; $this->offset = $perPage * ($page - 1); - return $this->searchSharees($search, $itemType, $shareIds, $shareTypes, $page, $perPage); + return $this->searchSharees($search, $itemType, $shareTypes, $page, $perPage); } /** @@ -309,13 +336,12 @@ class Sharees { * * @param string $search * @param string $itemType - * @param array $shareIds * @param array $shareTypes * @param int $page * @param int $perPage * @return \OC_OCS_Result */ - protected function searchSharees($search, $itemType, array $shareIds, array $shareTypes, $page, $perPage) { + protected function searchSharees($search, $itemType, array $shareTypes, $page, $perPage) { // Verify arguments if ($itemType === null) { return new \OC_OCS_Result(null, 400, 'missing itemType'); @@ -343,7 +369,6 @@ class Sharees { $response->addHeader('Link', $this->getPaginationLink($page, [ 'search' => $search, 'itemType' => $itemType, - 'existingShares' => $shareIds, 'shareType' => $shareTypes, 'limit' => $perPage, ])); diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index 3b2e17d9fd..d9f3662334 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -25,6 +25,7 @@ use Doctrine\DBAL\Connection; use OC\Share\Constants; use OCA\Files_Sharing\API\Sharees; use OCA\Files_sharing\Tests\TestCase; +use OCP\Share; class ShareesTest extends TestCase { /** @var Sharees */ @@ -42,6 +43,9 @@ class ShareesTest extends TestCase { /** @var \OCP\IUserSession|\PHPUnit_Framework_MockObject_MockObject */ protected $session; + /** @var \OCP\IRequest|\PHPUnit_Framework_MockObject_MockObject */ + protected $request; + protected function setUp() { parent::setUp(); @@ -61,6 +65,10 @@ class ShareesTest extends TestCase { ->disableOriginalConstructor() ->getMock(); + $this->request = $this->getMockBuilder('OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(); + $this->sharees = new Sharees( $this->groupManager, $this->userManager, @@ -68,8 +76,8 @@ class ShareesTest extends TestCase { $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), - \OC::$server->getDatabaseConnection() + $this->request, + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ); } @@ -103,8 +111,8 @@ class ShareesTest extends TestCase { public function dataGetUsers() { return [ - ['test', false, [], [], []], - ['test', true, [], [], []], + ['test', false, [], [], [], [], true], + ['test', true, [], [], [], [], true], [ 'test', false, @@ -112,9 +120,11 @@ class ShareesTest extends TestCase { [ $this->getUserMock('test1', 'Test One'), ], + [], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ] + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + true, ], [ 'test', @@ -124,92 +134,85 @@ class ShareesTest extends TestCase { $this->getUserMock('test1', 'Test One'), $this->getUserMock('test2', 'Test Two'), ], + [], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ] + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + false, ], [ 'test', false, [], [ + $this->getUserMock('test', 'Test'), $this->getUserMock('test1', 'Test One'), $this->getUserMock('test2', 'Test Two'), - $this->getUserMock('admin', 'Should be removed'), ], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ] + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + false, ], [ 'test', true, ['abc', 'xyz'], [ - ['abc', 'test', -1, 0, ['test1' => 'Test One']], - ['xyz', 'test', -1, 0, []], + ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['xyz', 'test', 2, 0, []], ], + [], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ] + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + true, ], [ 'test', true, ['abc', 'xyz'], [ - ['abc', 'test', -1, 0, [ + ['abc', 'test', 2, 0, [ 'test1' => 'Test One', 'test2' => 'Test Two', ]], - ['xyz', 'test', -1, 0, [ + ['xyz', 'test', 2, 0, [ 'test1' => 'Test One', 'test2' => 'Test Two', ]], ], + [], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ] + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + false, ], [ 'test', true, ['abc', 'xyz'], [ - ['abc', 'test', -1, 0, [ - 'test1' => 'Test One', + ['abc', 'test', 2, 0, [ + 'test' => 'Test One', ]], - ['xyz', 'test', -1, 0, [ + ['xyz', 'test', 2, 0, [ 'test2' => 'Test Two', ]], ], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ] - ], - [ - 'test', - true, - ['abc', 'xyz'], - [ - ['abc', 'test', -1, 0, [ - 'test1' => 'Test One', - ]], - ['xyz', 'test', -1, 0, [ - 'test2' => 'Test Two', - ]], - ['admin', 'Should be removed', -1, 0, [ - 'test2' => 'Test Two', - ]], + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], ], [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ] + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + false, ], ]; } @@ -221,9 +224,15 @@ class ShareesTest extends TestCase { * @param bool $shareWithGroupOnly * @param array $groupResponse * @param array $userResponse + * @param array $exactExpected * @param array $expected + * @param bool $reachedEnd */ - public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $expected) { + public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $exactExpected, $expected, $reachedEnd) { + $this->invokePrivate($this->sharees, 'limit', [2]); + $this->invokePrivate($this->sharees, 'offset', [0]); + $this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]); + $user = $this->getUserMock('admin', 'Administrator'); $this->session->expects($this->any()) ->method('getUser') @@ -232,7 +241,7 @@ class ShareesTest extends TestCase { if (!$shareWithGroupOnly) { $this->userManager->expects($this->once()) ->method('searchDisplayName') - ->with($searchTerm) + ->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturn($userResponse); } else { $this->groupManager->expects($this->once()) @@ -242,25 +251,41 @@ class ShareesTest extends TestCase { $this->groupManager->expects($this->exactly(sizeof($groupResponse))) ->method('displayNamesInGroup') - ->with($this->anything(), $searchTerm) + ->with($this->anything(), $searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturnMap($userResponse); } - $users = $this->invokePrivate($this->sharees, 'getUsers', [$searchTerm, $shareWithGroupOnly]); + $this->invokePrivate($this->sharees, 'getUsers', [$searchTerm]); + $result = $this->invokePrivate($this->sharees, 'result'); - $this->assertEquals($expected, $users); + $this->assertEquals($exactExpected, $result['exact']['users']); + $this->assertEquals($expected, $result['users']); + $this->assertCount((int) $reachedEnd, $this->invokePrivate($this->sharees, 'reachedEndFor')); } public function dataGetGroups() { return [ - ['test', false, [], [], []], + ['test', false, [], [], [], [], true], [ 'test', false, [$this->getGroupMock('test1')], [], - [['label' => 'test1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + [], + [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + true, ], - ['test', true, [], [], []], + [ + 'test', false, + [ + $this->getGroupMock('test'), + $this->getGroupMock('test1'), + ], + [], + [['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']]], + [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + false, + ], + ['test', true, [], [], [], [], true], [ 'test', true, [ @@ -268,7 +293,31 @@ class ShareesTest extends TestCase { $this->getGroupMock('test2'), ], [$this->getGroupMock('test1')], - [['label' => 'test1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + [], + [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + false, + ], + [ + 'test', true, + [ + $this->getGroupMock('test'), + $this->getGroupMock('test1'), + ], + [$this->getGroupMock('test')], + [['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']]], + [], + false, + ], + [ + 'test', true, + [ + $this->getGroupMock('test'), + $this->getGroupMock('test1'), + ], + [$this->getGroupMock('test1')], + [], + [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + false, ], ]; } @@ -280,12 +329,18 @@ class ShareesTest extends TestCase { * @param bool $shareWithGroupOnly * @param array $groupResponse * @param array $userGroupsResponse + * @param array $exactExpected * @param array $expected + * @param bool $reachedEnd */ - public function testGetGroups($searchTerm, $shareWithGroupOnly, $groupResponse, $userGroupsResponse, $expected) { + public function testGetGroups($searchTerm, $shareWithGroupOnly, $groupResponse, $userGroupsResponse, $exactExpected, $expected, $reachedEnd) { + $this->invokePrivate($this->sharees, 'limit', [2]); + $this->invokePrivate($this->sharees, 'offset', [0]); + $this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]); + $this->groupManager->expects($this->once()) ->method('search') - ->with($searchTerm) + ->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturn($groupResponse); if ($shareWithGroupOnly) { @@ -301,20 +356,25 @@ class ShareesTest extends TestCase { ->willReturn($userGroupsResponse); } - $users = $this->invokePrivate($this->sharees, 'getGroups', [$searchTerm, $shareWithGroupOnly]); + $this->invokePrivate($this->sharees, 'getGroups', [$searchTerm]); + $result = $this->invokePrivate($this->sharees, 'result'); - $this->assertEquals($expected, $users); + $this->assertEquals($exactExpected, $result['exact']['groups']); + $this->assertEquals($expected, $result['groups']); + $this->assertCount((int) $reachedEnd, $this->invokePrivate($this->sharees, 'reachedEndFor')); } public function dataGetRemote() { return [ - ['test', [], []], + ['test', [], [], [], true], [ 'test@remote', [], [ - ['label' => 'test@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], + ['label' => 'test@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], ], + [], + true, ], [ 'test', @@ -334,9 +394,11 @@ class ShareesTest extends TestCase { ], ], ], + [], [ - ['label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ['label' => 'User @ Localhost', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], ], + true, ], [ 'test@remote', @@ -357,9 +419,12 @@ class ShareesTest extends TestCase { ], ], [ - ['label' => 'test@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], - ['label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ['label' => 'test@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'test@remote']], ], + [ + ['label' => 'User @ Localhost', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ], + true, ], ]; } @@ -369,123 +434,120 @@ class ShareesTest extends TestCase { * * @param string $searchTerm * @param array $contacts + * @param array $exactExpected * @param array $expected + * @param bool $reachedEnd */ - public function testGetRemote($searchTerm, $contacts, $expected) { + public function testGetRemote($searchTerm, $contacts, $exactExpected, $expected, $reachedEnd) { $this->contactsManager->expects($this->any()) ->method('search') ->with($searchTerm, ['CLOUD', 'FN']) ->willReturn($contacts); - $users = $this->invokePrivate($this->sharees, 'getRemote', [$searchTerm]); + $this->invokePrivate($this->sharees, 'getRemote', [$searchTerm]); + $result = $this->invokePrivate($this->sharees, 'result'); - $this->assertEquals($expected, $users); + $this->assertEquals($exactExpected, $result['exact']['remotes']); + $this->assertEquals($expected, $result['remotes']); + $this->assertCount((int) $reachedEnd, $this->invokePrivate($this->sharees, 'reachedEndFor')); } public function dataSearch() { - $allTypes = [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE]; + $allTypes = [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_REMOTE]; return [ - [[], '', true, [], '', null, $allTypes, 1, 200, false], + [[], '', true, '', null, $allTypes, 1, 200, false], // Test itemType [[ 'search' => '', - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'search' => 'foobar', - ], '', true, [], 'foobar', null, $allTypes, 1, 200, false], + ], '', true, 'foobar', null, $allTypes, 1, 200, false], [[ 'search' => 0, - ], '', true, [], '0', null, $allTypes, 1, 200, false], + ], '', true, '0', null, $allTypes, 1, 200, false], // Test itemType [[ 'itemType' => '', - ], '', true, [], '', '', $allTypes, 1, 200, false], + ], '', true, '', '', $allTypes, 1, 200, false], [[ 'itemType' => 'folder', - ], '', true, [], '', 'folder', $allTypes, 1, 200, false], + ], '', true, '', 'folder', $allTypes, 1, 200, false], [[ 'itemType' => 0, - ], '', true, [], '', '0', $allTypes, 1, 200, false], - - // Test existingShares - [[ - 'existingShares' => [], - ], '', true, [], '', null, $allTypes, 1, 200, false], - [[ - 'existingShares' => [12, 42], - ], '', true, [12, 42], '', null, $allTypes, 1, 200, false], + ], '', true, '', '0', $allTypes, 1, 200, false], // Test shareType [[ - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'shareType' => 0, - ], '', true, [], '', null, [0], 1, 200, false], + ], '', true, '', null, [0], 1, 200, false], [[ 'shareType' => '0', - ], '', true, [], '', null, [0], 1, 200, false], + ], '', true, '', null, [0], 1, 200, false], [[ 'shareType' => 1, - ], '', true, [], '', null, [1], 1, 200, false], + ], '', true, '', null, [1], 1, 200, false], [[ 'shareType' => 12, - ], '', true, [], '', null, [], 1, 200, false], + ], '', true, '', null, [], 1, 200, false], [[ 'shareType' => 'foobar', - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'shareType' => [0, 1, 2], - ], '', true, [], '', null, [0, 1], 1, 200, false], + ], '', true, '', null, [0, 1], 1, 200, false], [[ 'shareType' => [0, 1], - ], '', true, [], '', null, [0, 1], 1, 200, false], + ], '', true, '', null, [0, 1], 1, 200, false], [[ 'shareType' => $allTypes, - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'shareType' => $allTypes, - ], '', false, [], '', null, [0, 1], 1, 200, false], + ], '', false, '', null, [0, 1], 1, 200, false], // Test pagination [[ 'page' => 0, - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'page' => '0', - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'page' => -1, - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'page' => 1, - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'page' => 10, - ], '', true, [], '', null, $allTypes, 10, 200, false], + ], '', true, '', null, $allTypes, 10, 200, false], // Test limit [[ 'limit' => 0, - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'limit' => '0', - ], '', true, [], '', null, $allTypes, 1, 200, false], + ], '', true, '', null, $allTypes, 1, 200, false], [[ 'limit' => -1, - ], '', true, [], '', null, $allTypes, 1, 1, false], + ], '', true, '', null, $allTypes, 1, 1, false], [[ 'limit' => 1, - ], '', true, [], '', null, $allTypes, 1, 1, false], + ], '', true, '', null, $allTypes, 1, 1, false], [[ 'limit' => 10, - ], '', true, [], '', null, $allTypes, 1, 10, false], + ], '', true, '', null, $allTypes, 1, 10, false], // Test $shareWithGroupOnly setting - [[], 'no', true, [], '', null, $allTypes, 1, 200, false], - [[], 'yes', true, [], '', null, $allTypes, 1, 200, true], + [[], 'no', true, '', null, $allTypes, 1, 200, false], + [[], 'yes', true, '', null, $allTypes, 1, 200, true], ]; } @@ -496,7 +558,6 @@ class ShareesTest extends TestCase { * @param array $getData * @param string $apiSetting * @param bool $remoteSharingEnabled - * @param array $shareIds * @param string $search * @param string $itemType * @param array $shareTypes @@ -504,7 +565,7 @@ class ShareesTest extends TestCase { * @param int $perPage * @param bool $shareWithGroupOnly */ - public function testSearch($getData, $apiSetting, $remoteSharingEnabled, $shareIds, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly) { + public function testSearch($getData, $apiSetting, $remoteSharingEnabled, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly) { $oldGet = $_GET; $_GET = $getData; @@ -524,26 +585,24 @@ class ShareesTest extends TestCase { $config, $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), - \OC::$server->getDatabaseConnection() + $this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ]) ->setMethods(array('searchSharees', 'isRemoteSharingAllowed')) ->getMock(); $sharees->expects($this->once()) ->method('searchSharees') - ->with($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly) + ->with($search, $itemType, $shareTypes, $page, $perPage) ->willReturnCallback(function - ($isearch, $iitemType, $ishareIds, $ishareTypes, $ipage, $iperPage, $ishareWithGroupOnly) - use ($search, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly) { + ($isearch, $iitemType, $ishareTypes, $ipage, $iperPage) + use ($search, $itemType, $shareTypes, $page, $perPage) { // We are doing strict comparisons here, so we can differ 0/'' and null on shareType/itemType $this->assertSame($search, $isearch); $this->assertSame($itemType, $iitemType); - $this->assertSame($shareIds, $ishareIds); $this->assertSame($shareTypes, $ishareTypes); $this->assertSame($page, $ipage); $this->assertSame($perPage, $iperPage); - $this->assertSame($shareWithGroupOnly, $ishareWithGroupOnly); return new \OC_OCS_Result([]); }); $sharees->expects($this->any()) @@ -554,6 +613,8 @@ class ShareesTest extends TestCase { /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ $this->assertInstanceOf('\OC_OCS_Result', $sharees->search()); + $this->assertSame($shareWithGroupOnly, $this->invokePrivate($sharees, 'shareWithGroupOnly')); + $_GET = $oldGet; } @@ -578,81 +639,88 @@ class ShareesTest extends TestCase { public function dataSearchSharees() { return [ - ['test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], - ['test', 'folder', [1, 2], [0 => ['test1'], 1 => ['test2 group']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], [], 0, false], - // First page with 2 of 3 results + ['test', 'folder', [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [], + 'groups' => [], + 'remotes' => [], + ], false], + ['test', 'folder', [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_REMOTE], 1, 2, false, [], [], [], + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [], + 'groups' => [], + 'remotes' => [], + ], false], [ - 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + 'test', 'folder', [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_GROUP, Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], [ - ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ['label' => 'testgroup1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], ], [ - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], - ], 3, true, - ], - // Second page with the 3rd result - [ - 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 2, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], [ - ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], - ], [ - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], [ - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], 3, false, + ['label' => 'testz@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + 'groups' => [ + ['label' => 'testgroup1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], + ], + 'remotes' => [ + ['label' => 'testz@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], + ], true, ], // No groups requested [ - 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], null, [ - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], 2, false, - ], - // Ingnoring already shared user - [ - 'test', 'folder', [1], [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_GROUP, \OCP\Share::SHARE_TYPE_REMOTE], 1, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], [ - ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], - ], [ - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], [ - ['label' => 'testgroup1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_GROUP, 'shareWith' => 'testgroup1']], - ['label' => 'testz@remote', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], - ], 2, false, + 'test', 'folder', [Share::SHARE_TYPE_USER, Share::SHARE_TYPE_REMOTE], 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], null, [ + ['label' => 'testz@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + 'groups' => [], + 'remotes' => [ + ['label' => 'testz@remote', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'testz@remote']], + ], + ], false, ], // Share type restricted to user - Only one user [ - 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], null, null, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], 1, false, + 'test', 'folder', [Share::SHARE_TYPE_USER], 1, 2, false, [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], null, null, + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [ + ['label' => 'test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ], + 'groups' => [], + 'remotes' => [], + ], false, ], // Share type restricted to user - Multipage result [ - 'test', 'folder', [], [], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ - ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ['label' => 'test 3', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test3']], - ], null, null, [ - ['label' => 'test 1', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'test 2', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ], 3, true, - ], - // Share type restricted to user - Only user already shared - [ - 'test', 'folder', [1], [\OCP\Share::SHARE_TYPE_USER => ['test1']], [\OCP\Share::SHARE_TYPE_USER], 1, 2, false, [ - ['label' => 'test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], null, null, [], 0, false, + 'test', 'folder', [Share::SHARE_TYPE_USER], 1, 2, false, [ + ['label' => 'test 1', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'test 2', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], null, null, + [ + 'exact' => ['users' => [], 'groups' => [], 'remotes' => []], + 'users' => [ + ['label' => 'test 1', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], + ['label' => 'test 2', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], + ], + 'groups' => [], + 'remotes' => [], + ], true, ], ]; } @@ -662,16 +730,18 @@ class ShareesTest extends TestCase { * * @param string $searchTerm * @param string $itemType - * @param array $shareIds - * @param array $existingShares * @param array $shareTypes * @param int $page * @param int $perPage * @param bool $shareWithGroupOnly + * @param array $mockedUserResult + * @param array $mockedGroupsResult + * @param array $mockedRemotesResult * @param array $expected + * @param bool $nextLink */ - public function testSearchSharees($searchTerm, $itemType, array $shareIds, array $existingShares, array $shareTypes, $page, $perPage, $shareWithGroupOnly, - $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $totalItems, $nextLink) { + public function testSearchSharees($searchTerm, $itemType, array $shareTypes, $page, $perPage, $shareWithGroupOnly, + $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $nextLink) { /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') ->setConstructorArgs([ @@ -681,45 +751,47 @@ class ShareesTest extends TestCase { $this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(), $this->session, $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), - $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(), - \OC::$server->getDatabaseConnection() + $this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() ]) ->setMethods(array('getShareesForShareIds', 'getUsers', 'getGroups', 'getRemote')) ->getMock(); - $sharees->expects($this->once()) - ->method('getShareesForShareIds') - ->with($shareIds) - ->willReturn($existingShares); $sharees->expects(($mockedUserResult === null) ? $this->never() : $this->once()) ->method('getUsers') - ->with($searchTerm, $shareWithGroupOnly) - ->willReturn($mockedUserResult); + ->with($searchTerm) + ->willReturnCallback(function() use ($sharees, $mockedUserResult) { + $result = $this->invokePrivate($sharees, 'result'); + $result['users'] = $mockedUserResult; + $this->invokePrivate($sharees, 'result', [$result]); + }); $sharees->expects(($mockedGroupsResult === null) ? $this->never() : $this->once()) ->method('getGroups') - ->with($searchTerm, $shareWithGroupOnly) - ->willReturn($mockedGroupsResult); + ->with($searchTerm) + ->willReturnCallback(function() use ($sharees, $mockedGroupsResult) { + $result = $this->invokePrivate($sharees, 'result'); + $result['groups'] = $mockedGroupsResult; + $this->invokePrivate($sharees, 'result', [$result]); + }); $sharees->expects(($mockedRemotesResult === null) ? $this->never() : $this->once()) ->method('getRemote') ->with($searchTerm) - ->willReturn($mockedRemotesResult); + ->willReturnCallback(function() use ($sharees, $mockedRemotesResult) { + $result = $this->invokePrivate($sharees, 'result'); + $result['remotes'] = $mockedRemotesResult; + $this->invokePrivate($sharees, 'result', [$result]); + }); /** @var \OC_OCS_Result $ocs */ - $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $shareIds, $shareTypes, $page, $perPage, $shareWithGroupOnly]); + $ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly]); $this->assertInstanceOf('\OC_OCS_Result', $ocs); - $this->assertEquals($expected, $ocs->getData()); - // Check number of total results - $meta = $ocs->getMeta(); - $this->assertArrayHasKey('totalitems', $meta); - $this->assertSame($totalItems, $meta['totalitems']); - // Check if next link is set if ($nextLink) { $headers = $ocs->getHeaders(); $this->assertArrayHasKey('Link', $headers); $this->assertStringStartsWith('<', $headers['Link']); - $this->assertStringEndsWith('"', $headers['Link']); + $this->assertStringEndsWith('>; rel="next"', $headers['Link']); } } @@ -737,199 +809,47 @@ class ShareesTest extends TestCase { $this->assertSame('missing itemType', $meta['message']); } - public function dataFilterSharees() { + public function dataGetPaginationLink() { return [ - [[], [], []], - [ - [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], - [], - [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], - ], - [ - [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ], - ['test1'], - [ - 1 => ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ], - ], - [ - [ - ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ['label' => 'Test Two', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], - ], - ['test2'], - [ - 0 => ['label' => 'Test One', 'value' => ['shareType' => \OCP\Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], - ], - ], + [1, '/ocs/v1.php', ['limit' => 2], '; rel="next"'], + [10, '/ocs/v2.php', ['limit' => 2], '; rel="next"'], ]; } /** - * @dataProvider dataFilterSharees - * - * @param array $potentialSharees - * @param array $existingSharees - * @param array $expectedSharees - */ - public function testFilterSharees($potentialSharees, $existingSharees, $expectedSharees) { - $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'filterSharees', [$potentialSharees, $existingSharees])); - } - - public function dataGetShareesForShareIds() { - return [ - [[], []], - [[1, 2, 3], [Constants::SHARE_TYPE_USER => ['user1'], Constants::SHARE_TYPE_GROUP => ['group1']]], - ]; - } - - /** - * @dataProvider dataGetShareesForShareIds - * - * @param array $shareIds - * @param array $expectedSharees - */ - public function testGetShareesForShareIds(array $shareIds, array $expectedSharees) { - $owner = $this->getUniqueID('test'); - $shares2delete = []; - - if (!empty($shareIds)) { - $userShare = $this->createShare(Constants::SHARE_TYPE_USER, 'user1', $owner, null); - $shares2delete[] = $userShare; - $shares2delete[] = $this->createShare(Constants::SHARE_TYPE_USER, 'user3', $owner . '2', null); - - $groupShare = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group1', $owner, null); - $shares2delete[] = $groupShare; - $groupShare2 = $this->createShare(Constants::SHARE_TYPE_GROUP, 'group2', $owner . '2', null); - $shares2delete[] = $groupShare2; - - $shares2delete[] = $this->createShare($this->invokePrivate(new Constants(), 'shareTypeGroupUserUnique'), 'user2', $owner, $groupShare); - $shares2delete[] = $this->createShare($this->invokePrivate(new Constants(), 'shareTypeGroupUserUnique'), 'user4', $owner, $groupShare2); - } - - $user = $this->getUserMock($owner, 'Sharee OCS test user'); - $this->session->expects($this->any()) - ->method('getUser') - ->willReturn($user); - $this->assertEquals($expectedSharees, $this->invokePrivate($this->sharees, 'getShareesForShareIds', [$shares2delete])); - - $this->deleteShares($shares2delete); - } - - /** - * @param int $type - * @param string $with - * @param string $owner - * @param int $parent - * @return int - */ - protected function createShare($type, $with, $owner, $parent) { - $connection = \OC::$server->getDatabaseConnection(); - $queryBuilder = $connection->getQueryBuilder(); - $queryBuilder->insert('share') - ->values([ - 'share_type' => $queryBuilder->createParameter('share_type'), - 'share_with' => $queryBuilder->createParameter('share_with'), - 'uid_owner' => $queryBuilder->createParameter('uid_owner'), - 'parent' => $queryBuilder->createParameter('parent'), - 'item_type' => $queryBuilder->expr()->literal('fake'), - 'item_source' => $queryBuilder->expr()->literal(''), - 'item_target' => $queryBuilder->expr()->literal(''), - 'file_source' => $queryBuilder->expr()->literal(0), - 'file_target' => $queryBuilder->expr()->literal(''), - 'permissions' => $queryBuilder->expr()->literal(0), - 'stime' => $queryBuilder->expr()->literal(0), - 'accepted' => $queryBuilder->expr()->literal(0), - 'expiration' => $queryBuilder->createParameter('expiration'), - 'token' => $queryBuilder->expr()->literal(''), - 'mail_send' => $queryBuilder->expr()->literal(0), - ]) - ->setParameters([ - 'share_type' => $type, - 'share_with' => $with, - 'uid_owner' => $owner, - 'parent' => $parent, - ]) - ->setParameter('expiration', null, 'datetime') - ->execute(); - - $queryBuilder = $connection->getQueryBuilder(); - $query = $queryBuilder->select('id') - ->from('share') - ->orderBy('id', 'DESC') - ->setMaxResults(1) - ->execute(); - $share = $query->fetch(); - $query->closeCursor(); - - return (int) $share['id']; - } - - /** - * @param int[] $shareIds - * @return null - */ - protected function deleteShares(array $shareIds) { - $connection = \OC::$server->getDatabaseConnection(); - $queryBuilder = $connection->getQueryBuilder(); - $queryBuilder->delete('share') - ->where($queryBuilder->expr()->in('id', $queryBuilder->createParameter('ids'))) - ->setParameter('ids', $shareIds, Connection::PARAM_INT_ARRAY) - ->execute(); - } - - public function dataGetPaginationLinks() { - return [ - [1, 1, ['limit' => 2], []], - [1, 3, ['limit' => 2], [ - '; rel="next"', - '; rel="last"', - ]], - [1, 21, ['limit' => 2], [ - '; rel="next"', - '; rel="last"', - ]], - [2, 21, ['limit' => 2], [ - '; rel="first"', - '; rel="prev"', - '; rel="next"', - '; rel="last"', - ]], - [5, 21, ['limit' => 2], [ - '; rel="first"', - '; rel="prev"', - '; rel="next"', - '; rel="last"', - ]], - [10, 21, ['limit' => 2], [ - '; rel="first"', - '; rel="prev"', - '; rel="next"', - '; rel="last"', - ]], - [11, 21, ['limit' => 2], [ - '; rel="first"', - '; rel="prev"', - ]], - ]; - } - - /** - * @dataProvider dataGetPaginationLinks + * @dataProvider dataGetPaginationLink * * @param int $page - * @param int $total + * @param string $scriptName * @param array $params * @param array $expected */ - public function testGetPaginationLinks($page, $total, $params, $expected) { - $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'getPaginationLinks', [$page, $total, $params])); + public function testGetPaginationLink($page, $scriptName, $params, $expected) { + $this->request->expects($this->once()) + ->method('getScriptName') + ->willReturn($scriptName); + + $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'getPaginationLink', [$page, $params])); + } + + public function dataIsV2() { + return [ + ['/ocs/v1.php', false], + ['/ocs/v2.php', true], + ]; + } + + /** + * @dataProvider dataIsV2 + * + * @param string $scriptName + * @param bool $expected + */ + public function testIsV2($scriptName, $expected) { + $this->request->expects($this->once()) + ->method('getScriptName') + ->willReturn($scriptName); + + $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'isV2')); } } From 199d1dc239a55f9be25b8779122420537f34ecf8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 26 Aug 2015 11:45:11 +0200 Subject: [PATCH 24/24] Bring the coverage back to 100% --- apps/files_sharing/api/sharees.php | 38 +++-- apps/files_sharing/tests/api/shareestest.php | 156 ++++++++++++++++++- 2 files changed, 169 insertions(+), 25 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 807b6b3314..d7eabb9a55 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -139,8 +139,8 @@ class Sharees { $foundUserById = false; foreach ($users as $uid => $userDisplayName) { - if ($uid === $search || $userDisplayName === $search) { - if ($uid === $search) { + if (strtolower($uid) === $search || strtolower($userDisplayName) === $search) { + if (strtolower($uid) === $search) { $foundUserById = true; } $this->result['exact']['users'][] = [ @@ -199,7 +199,7 @@ class Sharees { } foreach ($groups as $gid) { - if ($gid === $search) { + if (strtolower($gid) === $search) { $this->result['exact']['groups'][] = [ 'label' => $search, 'value' => [ @@ -222,8 +222,8 @@ class Sharees { // On page one we try if the search result has a direct hit on the // user id and if so, we add that to the exact match list $group = $this->groupManager->get($search); - if ($group instanceof IGroup && (!$this->shareWithGroupOnly || array_intersect([$group], $userGroups))) { - array_push($this->result['exact']['users'], [ + if ($group instanceof IGroup && (!$this->shareWithGroupOnly || in_array($group->getGID(), $userGroups))) { + array_push($this->result['exact']['groups'], [ 'label' => $group->getGID(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, @@ -241,23 +241,17 @@ class Sharees { protected function getRemote($search) { $this->result['remotes'] = []; - if (substr_count($search, '@') >= 1 && $this->offset === 0) { - $this->result['exact']['remotes'][] = [ - 'label' => $search, - 'value' => [ - 'shareType' => Share::SHARE_TYPE_REMOTE, - 'shareWith' => $search, - ], - ]; - } - // Search in contacts //@todo Pagination missing $addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN']); + $foundRemoteById = false; foreach ($addressBookContacts as $contact) { if (isset($contact['CLOUD'])) { foreach ($contact['CLOUD'] as $cloudId) { - if ($contact['FN'] === $search || $cloudId === $search) { + if (strtolower($contact['FN']) === $search || strtolower($cloudId) === $search) { + if (strtolower($cloudId) === $search) { + $foundRemoteById = true; + } $this->result['exact']['remotes'][] = [ 'label' => $contact['FN'], 'value' => [ @@ -278,6 +272,16 @@ class Sharees { } } + if (!$foundRemoteById && substr_count($search, '@') >= 1 && substr_count($search, ' ') === 0 && $this->offset === 0) { + $this->result['exact']['remotes'][] = [ + 'label' => $search, + 'value' => [ + 'shareType' => Share::SHARE_TYPE_REMOTE, + 'shareWith' => $search, + ], + ]; + } + $this->reachedEndFor[] = 'remotes'; } @@ -313,7 +317,7 @@ class Sharees { $this->limit = (int) $perPage; $this->offset = $perPage * ($page - 1); - return $this->searchSharees($search, $itemType, $shareTypes, $page, $perPage); + return $this->searchSharees(strtolower($search), $itemType, $shareTypes, $page, $perPage); } /** diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index d9f3662334..1e4438acd8 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -111,8 +111,20 @@ class ShareesTest extends TestCase { public function dataGetUsers() { return [ - ['test', false, [], [], [], [], true], - ['test', true, [], [], [], [], true], + ['test', false, [], [], [], [], true, false], + ['test', true, [], [], [], [], true, false], + [ + 'test', false, [], [], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], + ], [], true, $this->getUserMock('test', 'Test') + ], + [ + 'test', true, [], [], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], + ], [], true, $this->getUserMock('test', 'Test') + ], [ 'test', false, @@ -125,6 +137,7 @@ class ShareesTest extends TestCase { ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], true, + false, ], [ 'test', @@ -140,24 +153,26 @@ class ShareesTest extends TestCase { ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ], false, + false, ], [ 'test', false, [], [ - $this->getUserMock('test', 'Test'), + $this->getUserMock('test0', 'Test'), $this->getUserMock('test1', 'Test One'), $this->getUserMock('test2', 'Test Two'), ], [ - ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test0']], ], [ ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ], false, + false, ], [ 'test', @@ -172,6 +187,7 @@ class ShareesTest extends TestCase { ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1']], ], true, + false, ], [ 'test', @@ -193,6 +209,7 @@ class ShareesTest extends TestCase { ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ], false, + false, ], [ 'test', @@ -213,6 +230,7 @@ class ShareesTest extends TestCase { ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2']], ], false, + false, ], ]; } @@ -227,8 +245,9 @@ class ShareesTest extends TestCase { * @param array $exactExpected * @param array $expected * @param bool $reachedEnd + * @param mixed $singleUser */ - public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $exactExpected, $expected, $reachedEnd) { + public function testGetUsers($searchTerm, $shareWithGroupOnly, $groupResponse, $userResponse, $exactExpected, $expected, $reachedEnd, $singleUser) { $this->invokePrivate($this->sharees, 'limit', [2]); $this->invokePrivate($this->sharees, 'offset', [0]); $this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]); @@ -255,6 +274,13 @@ class ShareesTest extends TestCase { ->willReturnMap($userResponse); } + if ($singleUser !== false) { + $this->userManager->expects($this->once()) + ->method('get') + ->with($searchTerm) + ->willReturn($singleUser); + } + $this->invokePrivate($this->sharees, 'getUsers', [$searchTerm]); $result = $this->invokePrivate($this->sharees, 'result'); @@ -265,7 +291,7 @@ class ShareesTest extends TestCase { public function dataGetGroups() { return [ - ['test', false, [], [], [], [], true], + ['test', false, [], [], [], [], true, false], [ 'test', false, [$this->getGroupMock('test1')], @@ -273,6 +299,7 @@ class ShareesTest extends TestCase { [], [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], true, + false, ], [ 'test', false, @@ -284,8 +311,41 @@ class ShareesTest extends TestCase { [['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']]], [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], false, + false, ], - ['test', true, [], [], [], [], true], + [ + 'test', false, + [ + $this->getGroupMock('test0'), + $this->getGroupMock('test1'), + ], + [], + [], + [ + ['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']], + ['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']], + ], + false, + null, + ], + [ + 'test', false, + [ + $this->getGroupMock('test0'), + $this->getGroupMock('test1'), + ], + [], + [ + ['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']], + ], + [ + ['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']], + ['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']], + ], + false, + $this->getGroupMock('test'), + ], + ['test', true, [], [], [], [], true, false], [ 'test', true, [ @@ -296,6 +356,7 @@ class ShareesTest extends TestCase { [], [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], false, + false, ], [ 'test', true, @@ -307,6 +368,7 @@ class ShareesTest extends TestCase { [['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']]], [], false, + false, ], [ 'test', true, @@ -318,6 +380,51 @@ class ShareesTest extends TestCase { [], [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], false, + false, + ], + [ + 'test', true, + [ + $this->getGroupMock('test'), + $this->getGroupMock('test1'), + ], + [$this->getGroupMock('test'), $this->getGroupMock('test0'), $this->getGroupMock('test1')], + [['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']]], + [['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + false, + false, + ], + [ + 'test', true, + [ + $this->getGroupMock('test0'), + $this->getGroupMock('test1'), + ], + [$this->getGroupMock('test'), $this->getGroupMock('test0'), $this->getGroupMock('test1')], + [], + [ + ['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']], + ['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']], + ], + false, + null, + ], + [ + 'test', true, + [ + $this->getGroupMock('test0'), + $this->getGroupMock('test1'), + ], + [$this->getGroupMock('test'), $this->getGroupMock('test0'), $this->getGroupMock('test1')], + [ + ['label' => 'test', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test']], + ], + [ + ['label' => 'test0', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test0']], + ['label' => 'test1', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']], + ], + false, + $this->getGroupMock('test'), ], ]; } @@ -332,8 +439,9 @@ class ShareesTest extends TestCase { * @param array $exactExpected * @param array $expected * @param bool $reachedEnd + * @param mixed $singleGroup */ - public function testGetGroups($searchTerm, $shareWithGroupOnly, $groupResponse, $userGroupsResponse, $exactExpected, $expected, $reachedEnd) { + public function testGetGroups($searchTerm, $shareWithGroupOnly, $groupResponse, $userGroupsResponse, $exactExpected, $expected, $reachedEnd, $singleGroup) { $this->invokePrivate($this->sharees, 'limit', [2]); $this->invokePrivate($this->sharees, 'offset', [0]); $this->invokePrivate($this->sharees, 'shareWithGroupOnly', [$shareWithGroupOnly]); @@ -343,6 +451,13 @@ class ShareesTest extends TestCase { ->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturn($groupResponse); + if ($singleGroup !== false) { + $this->groupManager->expects($this->once()) + ->method('get') + ->with($searchTerm) + ->willReturn($singleGroup); + } + if ($shareWithGroupOnly) { $user = $this->getUserMock('admin', 'Administrator'); $this->session->expects($this->any()) @@ -426,6 +541,31 @@ class ShareesTest extends TestCase { ], true, ], + [ + 'username@localhost', + [ + [ + 'FN' => 'User3 @ Localhost', + ], + [ + 'FN' => 'User2 @ Localhost', + 'CLOUD' => [ + ], + ], + [ + 'FN' => 'User @ Localhost', + 'CLOUD' => [ + 'username@localhost', + ], + ], + ], + [ + ['label' => 'User @ Localhost', 'value' => ['shareType' => Share::SHARE_TYPE_REMOTE, 'shareWith' => 'username@localhost']], + ], + [ + ], + true, + ], ]; }