diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 924a9dd1cd7..9e324078dad 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\AppFramework\Http; use OCP\Contacts\IManager; use OCP\IGroup; use OCP\IGroupManager; @@ -291,8 +292,15 @@ class Sharees { public function search() { $search = isset($_GET['search']) ? (string) $_GET['search'] : ''; $itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null; - $page = !empty($_GET['page']) ? max(1, (int) $_GET['page']) : 1; - $perPage = !empty($_GET['perPage']) ? max(1, (int) $_GET['perPage']) : 200; + $page = isset($_GET['page']) ? (int) $_GET['page'] : 1; + $perPage = isset($_GET['perPage']) ? (int) $_GET['perPage'] : 200; + + if ($perPage <= 0) { + return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid perPage argument'); + } + if ($page <= 0) { + return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid page'); + } $shareTypes = [ Share::SHARE_TYPE_USER, @@ -348,7 +356,7 @@ class Sharees { protected function searchSharees($search, $itemType, array $shareTypes, $page, $perPage) { // Verify arguments if ($itemType === null) { - return new \OC_OCS_Result(null, 400, 'missing itemType'); + return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Missing itemType'); } // Get users diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index 1e28cb8ed5a..5c5d5b0d309 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -21,10 +21,9 @@ 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; +use OCP\AppFramework\Http; use OCP\Share; class ShareesTest extends TestCase { @@ -652,15 +651,6 @@ class ShareesTest extends TestCase { ], '', false, '', null, [0, 1], 1, 200, false], // Test pagination - [[ - 'page' => 0, - ], '', true, '', null, $allTypes, 1, 200, false], - [[ - 'page' => '0', - ], '', true, '', null, $allTypes, 1, 200, false], - [[ - 'page' => -1, - ], '', true, '', null, $allTypes, 1, 200, false], [[ 'page' => 1, ], '', true, '', null, $allTypes, 1, 200, false], @@ -669,15 +659,6 @@ class ShareesTest extends TestCase { ], '', true, '', null, $allTypes, 10, 200, false], // Test perPage - [[ - 'perPage' => 0, - ], '', true, '', null, $allTypes, 1, 200, false], - [[ - 'perPage' => '0', - ], '', true, '', null, $allTypes, 1, 200, false], - [[ - 'perPage' => -1, - ], '', true, '', null, $allTypes, 1, 1, false], [[ 'perPage' => 1, ], '', true, '', null, $allTypes, 1, 1, false], @@ -758,6 +739,75 @@ class ShareesTest extends TestCase { $_GET = $oldGet; } + public function dataSearchInvalid() { + return [ + // Test invalid pagination + [[ + 'page' => 0, + ], 'Invalid page'], + [[ + 'page' => '0', + ], 'Invalid page'], + [[ + 'page' => -1, + ], 'Invalid page'], + + // Test invalid perPage + [[ + 'perPage' => 0, + ], 'Invalid perPage argument'], + [[ + 'perPage' => '0', + ], 'Invalid perPage argument'], + [[ + 'perPage' => -1, + ], 'Invalid perPage argument'], + ]; + } + + /** + * @dataProvider dataSearchInvalid + * + * @param array $getData + * @param string $message + */ + public function testSearchInvalid($getData, $message) { + $oldGet = $_GET; + $_GET = $getData; + + $config = $this->getMockBuilder('OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + $config->expects($this->never()) + ->method('getAppValue'); + + $sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees') + ->setConstructorArgs([ + $this->groupManager, + $this->userManager, + $this->contactsManager, + $config, + $this->session, + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(), + $this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock() + ]) + ->setMethods(array('searchSharees', 'isRemoteSharingAllowed')) + ->getMock(); + $sharees->expects($this->never()) + ->method('searchSharees'); + $sharees->expects($this->never()) + ->method('isRemoteSharingAllowed'); + + /** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */ + $ocs = $sharees->search(); + $this->assertInstanceOf('\OC_OCS_Result', $ocs); + + $this->assertOCSError($ocs, $message); + + $_GET = $oldGet; + } + public function dataIsRemoteSharingAllowed() { return [ ['file', true], @@ -940,13 +990,7 @@ class ShareesTest extends TestCase { $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'); - $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']); + $this->assertOCSError($ocs, 'Missing itemType'); } public function dataGetPaginationLink() { @@ -992,4 +1036,18 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'isV2')); } + + /** + * @param \OC_OCS_Result $ocs + * @param string $message + */ + protected function assertOCSError(\OC_OCS_Result $ocs, $message) { + $this->assertSame(Http::STATUS_BAD_REQUEST, $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($message, $meta['message']); + } }