Merge pull request #4059 from nextcloud/feature/improve-security-level

tests/integration/features/decks.feature

@@ -13,3 +13,22 @@ Feature: decks
       |key|value|
       |title|MyBoard|
       |color|000000|
+
+  Scenario: Fail to create a board with invalid parameters
+    Given acting as user "user0"
+    When creates a board named "This is a very ong name that exceeds the maximum length of a deck board created which is longer than 100 characters" with color "ff0000"
+    Then the response should have a status code 400
+    When creates a board named "Example board" with color "invalid"
+    Then the response should have a status code 400
+
+  Scenario: Fail to create a list with invalid parameters
+    Given acting as user "user0"
+    And creates a board named "MyBoard" with color "000000"
+    When create a stack named "This is a very ong name that exceeds the maximum length of a deck board created which is longer than 100 characters"
+    Then the response should have a status code 400
+
+  Scenario: Fail to create a card with invalid parameters
+    Given acting as user "user0"
+    And creates a board named "MyBoard" with color "000000"
+    And create a stack named "ToDo"
+    When create a card named "This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters"

tests/unit/Service/AssignmentServiceTest.php

@@ -32,6 +32,7 @@ use OCA\Deck\Db\CardMapper;
 use OCA\Deck\Db\ChangeHelper;
 use OCA\Deck\NotFoundException;
 use OCA\Deck\Notification\NotificationHelper;
+use OCA\Deck\Validators\AssignmentServiceValidator;
 use OCP\Activity\IEvent;
 use OCP\EventDispatcher\IEventDispatcher;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -76,6 +77,11 @@ class AssignmentServiceTest extends TestCase {
 	 * @var AssignmentService
 	 */
 	private $assignmentService;
+	/**
+	 * @var  AssignmentServiceValidator
+	 */
+	private $assignmentServiceValidator;
+
 
 	public function setUp(): void {
 		parent::setUp();
@@ -87,6 +93,7 @@ class AssignmentServiceTest extends TestCase {
 		$this->activityManager = $this->createMock(ActivityManager::class);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 		$this->changeHelper = $this->createMock(ChangeHelper::class);
+		$this->assignmentServiceValidator = $this->createMock(AssignmentServiceValidator::class);
 		$this->assignmentService = new AssignmentService(
 			$this->permissionService,
 			$this->cardMapper,
@@ -96,6 +103,7 @@ class AssignmentServiceTest extends TestCase {
 			$this->activityManager,
 			$this->changeHelper,
 			$this->eventDispatcher,
+			$this->assignmentServiceValidator,
 			'admin'
 		);
 	}

tests/unit/Service/AttachmentServiceTest.php

@@ -34,6 +34,7 @@ use OCA\Deck\Db\ChangeHelper;
 use OCA\Deck\InvalidAttachmentType;
 use OCA\Deck\NoPermissionException;
 use OCA\Deck\NotFoundException;
+use OCA\Deck\Validators\AttachmentServiceValidator;
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\IAppContainer;
 use OCP\IL10N;
@@ -89,6 +90,10 @@ class AttachmentServiceTest extends TestCase {
 	 * @var IAttachmentService|MockObject
 	 */
 	private $filesAppServiceImpl;
+	/**
+	 * @var  AttachmentServiceValidator
+	 */
+	private $attachmentServiceValidator;
 
 	/**
 	 * @throws \OCP\AppFramework\QueryException
@@ -126,6 +131,7 @@ class AttachmentServiceTest extends TestCase {
 
 		$this->l10n = $this->createMock(IL10N::class);
 		$this->changeHelper = $this->createMock(ChangeHelper::class);
+		$this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class);
 
 		$this->attachmentService = new AttachmentService(
 			$this->attachmentMapper,
@@ -137,7 +143,8 @@ class AttachmentServiceTest extends TestCase {
 			$this->attachmentCacheHelper,
 			$this->userId,
 			$this->l10n,
-			$this->activityManager
+			$this->activityManager,
+			$this->attachmentServiceValidator
 		);
 	}
 
@@ -163,7 +170,7 @@ class AttachmentServiceTest extends TestCase {
 		$application->expects($this->any())
 			->method('getContainer')
 			->willReturn($appContainer);
-		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager);
+		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
 		$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
 		$this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file'));
 		$this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom')));
@@ -193,7 +200,7 @@ class AttachmentServiceTest extends TestCase {
 			->method('getContainer')
 			->willReturn($appContainer);
 
-		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager);
+		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
 		$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
 		$attachmentService->getService('deck_file_invalid');
 	}

tests/unit/Service/BoardServiceTest.php

@@ -46,6 +46,7 @@ use OCP\IGroupManager;
 use PHPUnit\Framework\MockObject\MockObject;
 use \Test\TestCase;
 use OCP\IURLGenerator;
+use OCA\Deck\Validators\BoardServiceValidator;
 
 class BoardServiceTest extends TestCase {
 
@@ -84,6 +85,8 @@ class BoardServiceTest extends TestCase {
 	private $urlGenerator;
 	/** @var IDBConnection|MockObject */
 	private $connection;
+	/** @var BoardServiceValidator */
+	private $boardServiceValidator;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -104,6 +107,7 @@ class BoardServiceTest extends TestCase {
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->connection = $this->createMock(IDBConnection::class);
+		$this->boardServiceValidator = $this->createMock(BoardServiceValidator::class);
 
 		$this->service = new BoardService(
 			$this->boardMapper,
@@ -123,6 +127,7 @@ class BoardServiceTest extends TestCase {
 			$this->changeHelper,
 			$this->urlGenerator,
 			$this->connection,
+			$this->boardServiceValidator,
 			$this->userId
 		);
 

tests/unit/Service/CardServiceTest.php

@@ -35,6 +35,7 @@ use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\LabelMapper;
 use OCA\Deck\Notification\NotificationHelper;
 use OCA\Deck\StatusException;
+use OCA\Deck\Validators\CardServiceValidator;
 use OCP\Activity\IEvent;
 use OCP\Comments\ICommentsManager;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -84,6 +85,8 @@ class CardServiceTest extends TestCase {
 	private $request;
 	/** @var LoggerInterface|MockObject */
 	private $logger;
+	/** @var CardServiceValidator|MockObject */
+	private $cardServiceValidator;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -104,6 +107,7 @@ class CardServiceTest extends TestCase {
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->request = $this->createMock(IRequest::class);
+		$this->cardServiceValidator = $this->createMock(CardServiceValidator::class);
 
 		$this->logger->expects($this->any())->method('error');
 
@@ -125,6 +129,7 @@ class CardServiceTest extends TestCase {
 			$this->urlGenerator,
 			$this->logger,
 			$this->request,
+			$this->cardServiceValidator,
 			'user1'
 		);
 	}

tests/unit/Service/LabelServiceTest.php

@@ -26,6 +26,7 @@ namespace OCA\Deck\Service;
 use OCA\Deck\Db\ChangeHelper;
 use OCA\Deck\Db\Label;
 use OCA\Deck\Db\LabelMapper;
+use OCA\Deck\Validators\LabelServiceValidator;
 use Test\TestCase;
 
 class LabelServiceTest extends TestCase {
@@ -40,6 +41,8 @@ class LabelServiceTest extends TestCase {
 	private $boardService;
 	/** @var ChangeHelper|\PHPUnit\Framework\MockObject\MockObject */
 	private $changeHelper;
+	/** @var  LabelServiceValidator\MockObject */
+	private $labelServiceValidator;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -49,11 +52,14 @@ class LabelServiceTest extends TestCase {
 			->disableOriginalConstructor()->getMock();
 		$this->boardService = $this->createMock(BoardService::class);
 		$this->changeHelper = $this->createMock(ChangeHelper::class);
+		$this->labelServiceValidator = $this->createMock(LabelServiceValidator::class);
+
 		$this->labelService = new LabelService(
 			$this->labelMapper,
 			$this->permissionService,
 			$this->boardService,
-			$this->changeHelper
+			$this->changeHelper,
+			$this->labelServiceValidator,
 		);
 	}
 

tests/unit/Service/StackServiceTest.php

@@ -33,6 +33,7 @@ use OCA\Deck\Db\Label;
 use OCA\Deck\Db\LabelMapper;
 use OCA\Deck\Db\Stack;
 use OCA\Deck\Db\StackMapper;
+use OCA\Deck\Validators\StackServiceValidator;
 use Psr\Log\LoggerInterface;
 use \Test\TestCase;
 
@@ -70,6 +71,8 @@ class StackServiceTest extends TestCase {
 	private $changeHelper;
 	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
 	private $logger;
+	/** @var StackServiceValidator|\PHPUnit\Framework\MockObject\MockObject */
+	private $stackServiceValidator;
 
 	public function setUp(): void {
 		parent::setUp();
@@ -85,6 +88,7 @@ class StackServiceTest extends TestCase {
 		$this->activityManager = $this->createMock(ActivityManager::class);
 		$this->changeHelper = $this->createMock(ChangeHelper::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->stackServiceValidator = $this->createMock(StackServiceValidator::class);
 
 		$this->stackService = new StackService(
 			$this->stackMapper,
@@ -98,7 +102,8 @@ class StackServiceTest extends TestCase {
 			$this->attachmentService,
 			$this->activityManager,
 			$this->changeHelper,
-			$this->logger
+			$this->logger,
+			$this->stackServiceValidator
 		);
 	}
 

tests/unit/Validators/StackServiceValidatorTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2022 Julius Härtl <jus@bitgrid.net>
+ *
+ * @author Julius Härtl <jus@bitgrid.net>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+namespace OCA\Deck\Validators;
+
+use OCA\Deck\Tests\unit\Validators\ValidatorTestBase;
+
+class StackServiceValidatorTest extends ValidatorTestBase {
+	public function setUp(): void {
+		parent::setUpValidatorTest(StackServiceValidator::class);
+	}
+
+	public function testTitle() {
+		$this->assertPass([
+			'title' => 'Short title',
+		]);
+		$this->assertPass([
+			'title' => str_repeat('A', 100)
+		]);
+		$this->assertFail([
+			'title' => str_repeat('A', 101)
+		]);
+		$this->assertFail([
+			'title' => '',
+		]);
+		$this->assertFail([
+			'title' => null,
+		]);
+	}
+
+	public function testId() {
+		$this->assertPass([ 'id' => 1234 ]);
+		$this->assertPass([ 'id' => '1234' ]);
+		$this->assertFail([ 'id' => 'a1234' ]);
+		$this->assertFail([ 'id' => '' ]);
+		$this->assertFail([ 'id' => null ]);
+	}
+
+	public function testBoardId() {
+		$this->assertPass([ 'boardId' => 1234 ]);
+		$this->assertPass([ 'boardId' => '1234' ]);
+		$this->assertFail([ 'boardId' => 'a1234' ]);
+		$this->assertFail([ 'boardId' => '' ]);
+		$this->assertFail([ 'boardId' => null ]);
+	}
+
+	public function testOrder() {
+		$this->assertPass([ 'order' => 1234 ]);
+		$this->assertPass([ 'order' => '1234' ]);
+		$this->assertFail([ 'order' => 'a1234' ]);
+		$this->assertFail([ 'order' => '' ]);
+		$this->assertFail([ 'order' => null ]);
+	}
+}

tests/unit/Validators/ValidatorTestBase.php

@@ -0,0 +1,57 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2022 Julius Härtl <jus@bitgrid.net>
+ *
+ * @author Julius Härtl <jus@bitgrid.net>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+namespace OCA\Deck\Tests\unit\Validators;
+
+use OCA\Deck\BadRequestException;
+use OCA\Deck\Validators\BaseValidator;
+
+abstract class ValidatorTestBase extends \PHPUnit\Framework\TestCase {
+	protected BaseValidator $validator;
+
+	public function setUpValidatorTest($class = null): void {
+		parent::setUp();
+
+		$this->validator = new $class();
+	}
+
+	protected function assertPass($values) {
+		self::assertTrue($this->check($values));
+	}
+
+	protected function assertFail($values) {
+		self::assertFalse($this->check($values));
+	}
+
+	private function check($values) {
+		try {
+			$this->validator->check($values);
+			return true;
+		} catch (BadRequestException $e) {
+			return false;
+		}
+	}
+}