API: check project membership for task operations

5 files changed, 61 insertions, 15 deletions

diff --git a/ChangeLog b/ChangeLog
index b94c7e31..cfc0fa16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -13,6 +13,7 @@ Breaking changes:
     - createLdapUser
     - updateUser
 * Event removed: "session.bootstrap" use "app.boostrap" instead
+* Change arguments for API procedure "updateTask"
 
 New features:
 
diff --git a/app/Api/Task.php b/app/Api/Task.php
index 0dceb209..4a7ee932 100644
--- a/app/Api/Task.php
+++ b/app/Api/Task.php
@@ -71,6 +71,14 @@ class Task extends Base
     {
         $this->checkProjectPermission($project_id);
 
+        if ($owner_id !== 0 && ! $this->projectPermission->isMember($project_id, $owner_id)) {
+            return false;
+        }
+
+        if ($this->userSession->isLogged()) {
+            $creator_id = $this->userSession->getId();
+        }
+
         $values = array(
             'title' => $title,
             'project_id' => $project_id,
@@ -96,20 +104,28 @@ class Task extends Base
         return $valid ? $this->taskCreation->create($values) : false;
     }
 
-    public function updateTask($id, $title = null, $project_id = null, $color_id = null, $owner_id = null,
-                               $creator_id = null, $date_due = null, $description = null, $category_id = null, $score = null,
+    public function updateTask($id, $title = null, $color_id = null, $owner_id = null,
+                               $date_due = null, $description = null, $category_id = null, $score = null,
                                $recurrence_status = null, $recurrence_trigger = null, $recurrence_factor = null,
                                $recurrence_timeframe = null, $recurrence_basedate = null, $reference = null)
     {
         $this->checkTaskPermission($id);
 
+        $project_id = $this->taskFinder->getProjectId($id);
+
+        if ($project_id === 0) {
+            return false;
+        }
+
+        if ($owner_id !== null && ! $this->projectPermission->isMember($project_id, $owner_id)) {
+            return false;
+        }
+
         $values = array(
             'id' => $id,
             'title' => $title,
-            'project_id' => $project_id,
             'color_id' => $color_id,
             'owner_id' => $owner_id,
-            'creator_id' => $creator_id,
             'date_due' => $date_due,
             'description' => $description,
             'category_id' => $category_id,
diff --git a/doc/api-task-procedures.markdown b/doc/api-task-procedures.markdown
index 71d5bd23..f2ca54d1 100644
--- a/doc/api-task-procedures.markdown
+++ b/doc/api-task-procedures.markdown
@@ -392,10 +392,8 @@ Response example:
 - Parameters:
     - **id** (integer, required)
     - **title** (string, optional)
-    - **project_id** (integer, optional)
     - **color_id** (string, optional)
     - **owner_id** (integer, optional)
-    - **creator_id** (integer, optional)
     - **date_due**: ISO8601 format (string, optional)
     - **description** Markdown content (string, optional)
     - **category_id** (integer, optional)
diff --git a/tests/functionals/ApiTest.php b/tests/functionals/ApiTest.php
index 9be22023..fbb703c9 100644
--- a/tests/functionals/ApiTest.php
+++ b/tests/functionals/ApiTest.php
@@ -366,6 +366,33 @@ class Api extends PHPUnit_Framework_TestCase
         $this->assertEquals('Swimlane A', $swimlanes[2]['name']);
     }
 
+    public function testCreateTaskWithWrongMember()
+    {
+        $task = array(
+            'title' => 'Task #1',
+            'color_id' => 'blue',
+            'owner_id' => 1,
+            'project_id' => 1,
+            'column_id' => 2,
+        );
+
+        $task_id = $this->client->createTask($task);
+
+        $this->assertFalse($task_id);
+    }
+
+    public function testGetAllowedUsers()
+    {
+        $users = $this->client->getMembers(1);
+        $this->assertNotFalse($users);
+        $this->assertEquals(array(), $users);
+    }
+
+    public function testAddMember()
+    {
+        $this->assertTrue($this->client->allowUser(1, 1));
+    }
+
     public function testCreateTask()
     {
         $task = array(
@@ -573,20 +600,13 @@ class Api extends PHPUnit_Framework_TestCase
         $this->assertEquals('titi@localhost', $user['email']);
     }
 
-    public function testGetAllowedUsers()
-    {
-        $users = $this->client->getMembers(1);
-        $this->assertNotFalse($users);
-        $this->assertEquals(array(), $users);
-    }
-
     public function testAllowedUser()
     {
         $this->assertTrue($this->client->allowUser(1, 2));
 
         $users = $this->client->getMembers(1);
         $this->assertNotFalse($users);
-        $this->assertEquals(array(2 => 'Titi'), $users);
+        $this->assertEquals(array(1 => 'admin', 2 => 'Titi'), $users);
     }
 
     public function testRevokeUser()
@@ -595,7 +615,7 @@ class Api extends PHPUnit_Framework_TestCase
 
         $users = $this->client->getMembers(1);
         $this->assertNotFalse($users);
-        $this->assertEquals(array(), $users);
+        $this->assertEquals(array(1 => 'admin'), $users);
     }
 
     public function testCreateComment()
diff --git a/tests/functionals/UserApiTest.php b/tests/functionals/UserApiTest.php
index 8a80c706..3c7fc04e 100644
--- a/tests/functionals/UserApiTest.php
+++ b/tests/functionals/UserApiTest.php
@@ -163,6 +163,12 @@ class UserApi extends PHPUnit_Framework_TestCase
         $this->assertEquals(2, $this->admin->createTask('my admin title', 1));
     }
 
+    public function testCreateTaskWithWrongMember()
+    {
+        $this->assertFalse($this->user->createTask(array('title' => 'something', 'project_id' => 2, 'owner_id' => 1)));
+        $this->assertFalse($this->app->createTask(array('title' => 'something', 'project_id' => 1, 'owner_id' => 2)));
+    }
+
     public function testGetTask()
     {
         $task = $this->user->getTask(1);
@@ -218,6 +224,11 @@ class UserApi extends PHPUnit_Framework_TestCase
         $this->assertTrue($this->user->moveTaskPosition(2, 1, 2, 1));
     }
 
+    public function testUpdateTaskWithWrongMember()
+    {
+        $this->assertFalse($this->user->updateTask(array('id' => 1, 'title' => 'new title', 'reference' => 'test', 'owner_id' => 1)));
+    }
+
     public function testUpdateTask()
     {
         $this->assertTrue($this->user->updateTask(array('id' => 1, 'title' => 'new title', 'reference' => 'test', 'owner_id' => 2)));