Limit card assignment to users who are participants of the board (#1395)

Limit card assignment to users who are participants of the board

Makefile

@@ -84,7 +84,7 @@ test-unit:
 ifeq (, $(shell which phpunit 2> /dev/null))
 	@echo "No phpunit command available, downloading a copy from the web"
 	mkdir -p $(build_tools_directory)
-	curl -sSL https://phar.phpunit.de/phpunit-5.7.phar -o $(build_tools_directory)/phpunit.phar
+	curl -sSL https://phar.phpunit.de/phpunit-8.2.phar -o $(build_tools_directory)/phpunit.phar
 	php $(build_tools_directory)/phpunit.phar -c tests/phpunit.xml --coverage-clover build/php-unit.coverage.xml
 	php $(build_tools_directory)/phpunit.phar -c tests/phpunit.integration.xml --coverage-clover build/php-integration.coverage.xml
 else

docs/API.md

@@ -651,6 +651,33 @@ The board list endpoint supports setting an `If-Modified-Since` header to limit
 
 ##### 200 Success
 
+```json
+{
+  "id": 3,
+  "participant": {
+    "primaryKey": "admin",
+    "uid": "admin",
+    "displayname": "admin"
+  },
+  "cardId": 1
+}
+```
+
+##### 400 Bad request
+
+```json
+{
+  "status": 400,
+  "message": "The user is already assigned to the card"
+}
+```
+
+The request can fail with a bad request response for the following reasons:
+- Missing or wrongly formatted request parameters
+- The user is already assigned to the card
+- The user is not part of the board
+
+
 ### PUT /boards/{boardId}/stacks/{stackId}/cards/{cardId}/unassignUser - Assign a user to a card
 
 #### Request parameters

lib/Service/CardService.php

@@ -588,10 +588,17 @@ class CardService {
 		$assignments = $this->assignedUsersMapper->find($cardId);
 		foreach ($assignments as $assignment) {
 			if ($assignment->getParticipant() === $userId) {
-				return false;
+				throw new BadRequestException('The user is already assigned to the card');
 			}
 		}
+
 		$card = $this->cardMapper->find($cardId);
+		$boardId = $this->cardMapper->findBoardId($cardId);
+		$boardUsers = array_keys($this->permissionService->findUsers($boardId, true));
+		if (!in_array($userId, $boardUsers)) {
+			throw new BadRequestException('The user is not part of the board');
+		}
+
 
 		if ($userId !== $this->currentUser) {
 			/* Notifyuser about the card assignment */

lib/Service/PermissionService.php

@@ -221,9 +221,9 @@ class PermissionService {
 	 * @param $boardId
 	 * @return array
 	 */
-	public function findUsers($boardId) {
+	public function findUsers($boardId, $refresh = false) {
 		// cache users of a board so we don't query them for every cards
-		if (array_key_exists((string) $boardId, $this->users)) {
+		if (array_key_exists((string) $boardId, $this->users) && !$refresh) {
 			return $this->users[(string) $boardId];
 		}
 		try {

tests/integration/database/AssignedUsersMapperTest.php

@@ -102,34 +102,25 @@ class AssignedUsersMapperTest extends \Test\TestCase {
 		$this->stacks = $stacks;
 	}
 
-	/**
-	 * @covers ::__construct
-	 */
-	public function testConstructor() {
-		//$this->assertAttributeInstanceOf(IDBConnection::class, 'db', $this->assignedUsersMapper);
-		//$this->assertAttributeEquals(AssignedUsers::class, 'entityClass', $this->assignedUsersMapper);
-		//$this->assertAttributeEquals('*PREFIX*deck_assigned_users', 'tableName', $this->assignedUsersMapper);
-	}
-
 	/**
 	 * @covers ::find
 	 */
 	public function testFind() {
 		$uids = [];
 		$this->cardService->assignUser($this->cards[0]->getId(), self::TEST_USER1);
-		$this->cardService->assignUser($this->cards[0]->getId(), self::TEST_USER4);
+		$this->cardService->assignUser($this->cards[0]->getId(), self::TEST_USER2);
 
 		$assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId());
 		foreach ($assignedUsers as $user) {
 			$uids[$user->getParticipant()] = $user;
 		}
 		$this->assertArrayHasKey(self::TEST_USER1, $uids);
-		$this->assertArrayNotHasKey(self::TEST_USER2, $uids);
+		$this->assertArrayHasKey(self::TEST_USER2, $uids);
 		$this->assertArrayNotHasKey(self::TEST_USER3, $uids);
-		$this->assertArrayHasKey(self::TEST_USER4, $uids);
+		$this->assertArrayNotHasKey(self::TEST_USER4, $uids);
 
 		$this->cardService->unassignUser($this->cards[0]->getId(), self::TEST_USER1);
-		$this->cardService->unassignUser($this->cards[0]->getId(), self::TEST_USER4);
+		$this->cardService->unassignUser($this->cards[0]->getId(), self::TEST_USER2);
 	}
 
 	/**

tests/unit/Service/CardServiceTest.php

@@ -25,6 +25,7 @@ namespace OCA\Deck\Service;
 
 
 use OCA\Deck\Activity\ActivityManager;
+use OCA\Deck\BadRequestException;
 use OCA\Deck\Db\AssignedUsers;
 use OCA\Deck\Db\AssignedUsersMapper;
 use OCA\Deck\Db\Card;
@@ -322,6 +323,13 @@ class CardServiceTest extends TestCase {
 		$assignment = new AssignedUsers();
 		$assignment->setCardId(123);
 		$assignment->setParticipant('admin');
+		$this->cardMapper->expects($this->once())
+			->method('findBoardId')
+			->willReturn(1);
+		$this->permissionService->expects($this->once())
+			->method('findUsers')
+			->with(1)
+			->willReturn(['admin' => 'admin', 'user1' => 'user1']);
 		$this->assignedUsersMapper->expects($this->once())
 			->method('insert')
 			->with($assignment)
@@ -330,7 +338,30 @@ class CardServiceTest extends TestCase {
 		$this->assertEquals($assignment, $actual);
 	}
 
+	public function testAssignUserNoParticipant() {
+		$this->expectException(BadRequestException::class);
+		$this->expectExceptionMessage('The user is not part of the board');
+		$assignments = [];
+		$this->assignedUsersMapper->expects($this->once())
+			->method('find')
+			->with(123)
+			->willReturn($assignments);
+		$assignment = new AssignedUsers();
+		$assignment->setCardId(123);
+		$assignment->setParticipant('admin');
+		$this->cardMapper->expects($this->once())
+			->method('findBoardId')
+			->willReturn(1);
+		$this->permissionService->expects($this->once())
+			->method('findUsers')
+			->with(1)
+			->willReturn(['user2' => 'user2', 'user1' => 'user1']);
+		$actual = $this->cardService->assignUser(123, 'admin');
+	}
+
 	public function testAssignUserExisting() {
+		$this->expectException(BadRequestException::class);
+		$this->expectExceptionMessage('The user is already assigned to the card');
 		$assignment = new AssignedUsers();
 		$assignment->setCardId(123);
 		$assignment->setParticipant('admin');
@@ -364,13 +395,8 @@ class CardServiceTest extends TestCase {
 		$this->assertEquals($assignment, $actual);
 	}
 
-	/**
-	 * @expectException \OCA\Deck\NotFoundException
-	 *
-	 *
-	 *
-	 */
 	public function testUnassignUserNotExisting() {
+		$this->expectException(NotFoundException::class);
 		$assignment = new AssignedUsers();
 		$assignment->setCardId(123);
 		$assignment->setParticipant('admin');