Merge pull request #19046 from owncloud/issue-18924-throw-error-on-invalid-page

Throw an error when the page count or perPage setting is invalid

2 files changed, 96 insertions, 30 deletions

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 {
@@ -653,15 +652,6 @@ class ShareesTest extends TestCase {
 
 			// 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],
 			[[
@@ -670,15 +660,6 @@ class ShareesTest extends TestCase {
 
 			// 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']);
+	}
 }