From aa8b1b2894637f3e2692e4d0f189c07dcb150e63 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 15 Sep 2015 12:37:09 +0200 Subject: [PATCH 1/2] Throw an error when the page count or perPage setting is invalid --- apps/files_sharing/api/sharees.php | 13 ++- apps/files_sharing/tests/api/shareestest.php | 111 ++++++++++++++----- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index 924a9dd1cd7..f3c91c18dc9 100644 --- a/apps/files_sharing/api/sharees.php +++ b/apps/files_sharing/api/sharees.php @@ -291,8 +291,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, 401, 'Invalid perPage argument'); + } + if ($page <= 0) { + return new \OC_OCS_Result(null, 402, 'Invalid page'); + } $shareTypes = [ Share::SHARE_TYPE_USER, @@ -348,7 +355,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, 400, 'Missing itemType'); } // Get users diff --git a/apps/files_sharing/tests/api/shareestest.php b/apps/files_sharing/tests/api/shareestest.php index 1e28cb8ed5a..65354aef8c6 100644 --- a/apps/files_sharing/tests/api/shareestest.php +++ b/apps/files_sharing/tests/api/shareestest.php @@ -652,15 +652,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 +660,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 +740,76 @@ class ShareesTest extends TestCase { $_GET = $oldGet; } + public function dataSearchInvalid() { + return [ + // Test invalid pagination + [[ + 'page' => 0, + ], 402, 'Invalid page'], + [[ + 'page' => '0', + ], 402, 'Invalid page'], + [[ + 'page' => -1, + ], 402, 'Invalid page'], + + // Test invalid perPage + [[ + 'perPage' => 0, + ], 401, 'Invalid perPage argument'], + [[ + 'perPage' => '0', + ], 401, 'Invalid perPage argument'], + [[ + 'perPage' => -1, + ], 401, 'Invalid perPage argument'], + ]; + } + + /** + * @dataProvider dataSearchInvalid + * + * @param array $getData + * @param int $code + * @param string $message + */ + public function testSearchInvalid($getData, $code, $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, $code, $message); + + $_GET = $oldGet; + } + public function dataIsRemoteSharingAllowed() { return [ ['file', true], @@ -940,13 +992,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, 400, 'Missing itemType'); } public function dataGetPaginationLink() { @@ -992,4 +1038,19 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'isV2')); } + + /** + * @param \OC_OCS_Result $ocs + * @param int $code + * @param string $message + */ + protected function assertOCSError(\OC_OCS_Result $ocs, $code, $message) { + $this->assertSame($code, $ocs->getStatusCode(), 'Expected status code ' . $code); + $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']); + } } From 754850f4732845b752e9b2177b78a2bdbc8ae368 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 15 Sep 2015 15:51:54 +0200 Subject: [PATCH 2/2] Fix status code --- apps/files_sharing/api/sharees.php | 7 ++--- apps/files_sharing/tests/api/shareestest.php | 27 +++++++++----------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/apps/files_sharing/api/sharees.php b/apps/files_sharing/api/sharees.php index f3c91c18dc9..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; @@ -295,10 +296,10 @@ class Sharees { $perPage = isset($_GET['perPage']) ? (int) $_GET['perPage'] : 200; if ($perPage <= 0) { - return new \OC_OCS_Result(null, 401, 'Invalid perPage argument'); + return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid perPage argument'); } if ($page <= 0) { - return new \OC_OCS_Result(null, 402, 'Invalid page'); + return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid page'); } $shareTypes = [ @@ -355,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 65354aef8c6..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 { @@ -745,24 +744,24 @@ class ShareesTest extends TestCase { // Test invalid pagination [[ 'page' => 0, - ], 402, 'Invalid page'], + ], 'Invalid page'], [[ 'page' => '0', - ], 402, 'Invalid page'], + ], 'Invalid page'], [[ 'page' => -1, - ], 402, 'Invalid page'], + ], 'Invalid page'], // Test invalid perPage [[ 'perPage' => 0, - ], 401, 'Invalid perPage argument'], + ], 'Invalid perPage argument'], [[ 'perPage' => '0', - ], 401, 'Invalid perPage argument'], + ], 'Invalid perPage argument'], [[ 'perPage' => -1, - ], 401, 'Invalid perPage argument'], + ], 'Invalid perPage argument'], ]; } @@ -770,10 +769,9 @@ class ShareesTest extends TestCase { * @dataProvider dataSearchInvalid * * @param array $getData - * @param int $code * @param string $message */ - public function testSearchInvalid($getData, $code, $message) { + public function testSearchInvalid($getData, $message) { $oldGet = $_GET; $_GET = $getData; @@ -805,7 +803,7 @@ class ShareesTest extends TestCase { $ocs = $sharees->search(); $this->assertInstanceOf('\OC_OCS_Result', $ocs); - $this->assertOCSError($ocs, $code, $message); + $this->assertOCSError($ocs, $message); $_GET = $oldGet; } @@ -992,7 +990,7 @@ class ShareesTest extends TestCase { $ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], [], 0, 0, false]); $this->assertInstanceOf('\OC_OCS_Result', $ocs); - $this->assertOCSError($ocs, 400, 'Missing itemType'); + $this->assertOCSError($ocs, 'Missing itemType'); } public function dataGetPaginationLink() { @@ -1041,11 +1039,10 @@ class ShareesTest extends TestCase { /** * @param \OC_OCS_Result $ocs - * @param int $code * @param string $message */ - protected function assertOCSError(\OC_OCS_Result $ocs, $code, $message) { - $this->assertSame($code, $ocs->getStatusCode(), 'Expected status code ' . $code); + 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();