From d8f6d8568396816a6bfaca1e01211384e803cf91 Mon Sep 17 00:00:00 2001 From: Frederic Guillot Date: Sun, 11 Sep 2016 16:08:03 -0400 Subject: Add project restrictions for custom roles --- app/Controller/BoardAjaxController.php | 8 +- app/Controller/ColumnMoveRestrictionController.php | 4 +- .../ProjectRoleRestrictionController.php | 96 +++++++++++ app/Controller/TaskSuppressionController.php | 4 +- app/Core/Base.php | 1 + app/Core/Helper.php | 1 + app/Formatter/BoardTaskFormatter.php | 2 +- app/Helper/BoardHelper.php | 22 --- app/Helper/ProjectRoleHelper.php | 130 ++++++++++++++ app/Helper/UserHelper.php | 55 +----- app/Model/ProjectRoleModel.php | 13 +- app/Model/ProjectRoleRestrictionModel.php | 164 ++++++++++++++++++ app/Schema/Mysql.php | 17 +- app/Schema/Postgres.php | 17 +- app/Schema/Sqlite.php | 17 +- app/ServiceProvider/ClassProvider.php | 1 + app/ServiceProvider/HelperProvider.php | 1 + app/Template/project_role/show.php | 21 ++- app/Template/project_role_restriction/create.php | 19 +++ app/Template/project_role_restriction/remove.php | 14 ++ app/Template/task/dropdown.php | 2 +- app/Template/task/sidebar.php | 2 +- tests/units/Helper/BoardHelperTest.php | 98 ----------- tests/units/Helper/ProjectRoleHelperTest.php | 189 +++++++++++++++++++++ tests/units/Helper/UserHelperTest.php | 90 ---------- 25 files changed, 700 insertions(+), 288 deletions(-) create mode 100644 app/Controller/ProjectRoleRestrictionController.php create mode 100644 app/Helper/ProjectRoleHelper.php create mode 100644 app/Model/ProjectRoleRestrictionModel.php create mode 100644 app/Template/project_role_restriction/create.php create mode 100644 app/Template/project_role_restriction/remove.php delete mode 100644 tests/units/Helper/BoardHelperTest.php create mode 100644 tests/units/Helper/ProjectRoleHelperTest.php diff --git a/app/Controller/BoardAjaxController.php b/app/Controller/BoardAjaxController.php index cc3b846e..484ef67d 100644 --- a/app/Controller/BoardAjaxController.php +++ b/app/Controller/BoardAjaxController.php @@ -28,14 +28,8 @@ class BoardAjaxController extends BaseController } $values = $this->request->getJson(); - $canMoveTask = $this->columnMoveRestrictionModel->isAllowed( - $project_id, - $this->helper->user->getProjectUserRole($project_id), - $values['src_column_id'], - $values['dst_column_id'] - ); - if (! $canMoveTask) { + if (! $this->helper->projectRole->canMoveTask($project_id, $values['src_column_id'], $values['dst_column_id'])) { throw new AccessForbiddenException(e("You don't have the permission to move this task")); } diff --git a/app/Controller/ColumnMoveRestrictionController.php b/app/Controller/ColumnMoveRestrictionController.php index 3f1b878f..b12f6b77 100644 --- a/app/Controller/ColumnMoveRestrictionController.php +++ b/app/Controller/ColumnMoveRestrictionController.php @@ -45,14 +45,14 @@ class ColumnMoveRestrictionController extends BaseController list($valid, $errors) = $this->columnMoveRestrictionValidator->validateCreation($values); if ($valid) { - $role_id = $this->columnMoveRestrictionModel->create( + $restriction_id = $this->columnMoveRestrictionModel->create( $project['id'], $values['role_id'], $values['src_column_id'], $values['dst_column_id'] ); - if ($role_id !== false) { + if ($restriction_id !== false) { $this->flash->success(t('The column restriction has been created successfully.')); } else { $this->flash->failure(t('Unable to create this column restriction.')); diff --git a/app/Controller/ProjectRoleRestrictionController.php b/app/Controller/ProjectRoleRestrictionController.php new file mode 100644 index 00000000..4fa9b13b --- /dev/null +++ b/app/Controller/ProjectRoleRestrictionController.php @@ -0,0 +1,96 @@ +getProject(); + $role_id = $this->request->getIntegerParam('role_id'); + $role = $this->projectRoleModel->getById($project['id'], $role_id); + + $this->response->html($this->template->render('project_role_restriction/create', array( + 'project' => $project, + 'role' => $role, + 'values' => $values + array('project_id' => $project['id'], 'role_id' => $role['role_id']), + 'errors' => $errors, + 'restrictions' => $this->projectRoleRestrictionModel->getRules(), + ))); + } + + /** + * Save new restriction + */ + public function save() + { + $project = $this->getProject(); + $values = $this->request->getValues(); + + $restriction_id = $this->projectRoleRestrictionModel->create( + $project['id'], + $values['role_id'], + $values['rule'] + ); + + if ($restriction_id !== false) { + $this->flash->success(t('The project restriction has been created successfully.')); + } else { + $this->flash->failure(t('Unable to create this project restriction.')); + } + + $this->response->redirect($this->helper->url->to('ProjectRoleController', 'show', array('project_id' => $project['id']))); + } + + /** + * Confirm suppression + * + * @access public + */ + public function confirm() + { + $project = $this->getProject(); + $restriction_id = $this->request->getIntegerParam('restriction_id'); + + $this->response->html($this->helper->layout->project('project_role_restriction/remove', array( + 'project' => $project, + 'restriction' => $this->projectRoleRestrictionModel->getById($project['id'], $restriction_id), + 'restrictions' => $this->projectRoleRestrictionModel->getRules(), + ))); + } + + /** + * Remove a restriction + * + * @access public + */ + public function remove() + { + $project = $this->getProject(); + $this->checkCSRFParam(); + $restriction_id = $this->request->getIntegerParam('restriction_id'); + + if ($this->projectRoleRestrictionModel->remove($restriction_id)) { + $this->flash->success(t('Project restriction removed successfully.')); + } else { + $this->flash->failure(t('Unable to remove this restriction.')); + } + + $this->response->redirect($this->helper->url->to('ProjectRoleController', 'show', array('project_id' => $project['id']))); + } +} diff --git a/app/Controller/TaskSuppressionController.php b/app/Controller/TaskSuppressionController.php index 600107c9..019bd97c 100644 --- a/app/Controller/TaskSuppressionController.php +++ b/app/Controller/TaskSuppressionController.php @@ -19,7 +19,7 @@ class TaskSuppressionController extends BaseController { $task = $this->getTask(); - if (! $this->helper->user->canRemoveTask($task)) { + if (! $this->helper->projectRole->canRemoveTask($task)) { throw new AccessForbiddenException(); } @@ -37,7 +37,7 @@ class TaskSuppressionController extends BaseController $task = $this->getTask(); $this->checkCSRFParam(); - if (! $this->helper->user->canRemoveTask($task)) { + if (! $this->helper->projectRole->canRemoveTask($task)) { throw new AccessForbiddenException(); } diff --git a/app/Core/Base.php b/app/Core/Base.php index cadaef72..d6da13f2 100644 --- a/app/Core/Base.php +++ b/app/Core/Base.php @@ -91,6 +91,7 @@ use Pimple\Container; * @property \Kanboard\Model\ProjectNotificationModel $projectNotificationModel * @property \Kanboard\Model\ProjectNotificationTypeModel $projectNotificationTypeModel * @property \Kanboard\Model\ProjectRoleModel $projectRoleModel + * @property \Kanboard\Model\ProjectRoleRestrictionModel $projectRoleRestrictionModel * @property \Kanboard\Model\ProjectTaskDuplicationModel $projectTaskDuplicationModel * @property \Kanboard\Model\ProjectTaskPriorityModel $projectTaskPriorityModel * @property \Kanboard\Model\RememberMeSessionModel $rememberMeSessionModel diff --git a/app/Core/Helper.php b/app/Core/Helper.php index c98b3c5e..b5c560af 100644 --- a/app/Core/Helper.php +++ b/app/Core/Helper.php @@ -26,6 +26,7 @@ use Pimple\Container; * @property \Kanboard\Helper\UrlHelper $url * @property \Kanboard\Helper\UserHelper $user * @property \Kanboard\Helper\LayoutHelper $layout + * @property \Kanboard\Helper\ProjectRoleHelper $projectRole * @property \Kanboard\Helper\ProjectHeaderHelper $projectHeader * @property \Kanboard\Helper\ProjectActivityHelper $projectActivity * @property \Kanboard\Helper\MailHelper $mail diff --git a/app/Formatter/BoardTaskFormatter.php b/app/Formatter/BoardTaskFormatter.php index 5956ae35..cd10f77a 100644 --- a/app/Formatter/BoardTaskFormatter.php +++ b/app/Formatter/BoardTaskFormatter.php @@ -81,7 +81,7 @@ class BoardTaskFormatter extends BaseFormatter implements FormatterInterface array_merge_relation($tasks, $this->tags, 'tags', 'id'); foreach ($tasks as &$task) { - $task['is_draggable'] = $this->helper->board->isDraggable($task); + $task['is_draggable'] = $this->helper->projectRole->isDraggable($task); } return $tasks; diff --git a/app/Helper/BoardHelper.php b/app/Helper/BoardHelper.php index 9e8e78ac..f5df3db2 100644 --- a/app/Helper/BoardHelper.php +++ b/app/Helper/BoardHelper.php @@ -24,26 +24,4 @@ class BoardHelper extends Base { return $this->userMetadataCacheDecorator->get(UserMetadataModel::KEY_BOARD_COLLAPSED.$project_id, 0) == 1; } - - /** - * Return true if the task can be moved by the connected user - * - * @param array $task - * @return bool - */ - public function isDraggable(array $task) - { - if ($task['is_active'] == 1 && $this->helper->user->hasProjectAccess('BoardViewController', 'save', $task['project_id'])) { - $role = $this->helper->user->getProjectUserRole($task['project_id']); - - if ($this->role->isCustomProjectRole($role)) { - $srcColumnIds = $this->columnMoveRestrictionCacheDecorator->getAllSrcColumns($task['project_id'], $role); - return isset($srcColumnIds[$task['column_id']]); - } - - return true; - } - - return false; - } } diff --git a/app/Helper/ProjectRoleHelper.php b/app/Helper/ProjectRoleHelper.php new file mode 100644 index 00000000..34905b52 --- /dev/null +++ b/app/Helper/ProjectRoleHelper.php @@ -0,0 +1,130 @@ +memoryCache->proxy($this->projectUserRoleModel, 'getUserRole', $project_id, $this->userSession->getId()); + } + + /** + * Return true if the task can be moved by the connected user + * + * @param array $task + * @return bool + */ + public function isDraggable(array $task) + { + if ($task['is_active'] == 1 && $this->helper->user->hasProjectAccess('BoardViewController', 'save', $task['project_id'])) { + $role = $this->getProjectUserRole($task['project_id']); + + if ($this->role->isCustomProjectRole($role)) { + $srcColumnIds = $this->columnMoveRestrictionCacheDecorator->getAllSrcColumns($task['project_id'], $role); + return isset($srcColumnIds[$task['column_id']]); + } + + return true; + } + + return false; + } + + /** + * Check if the user can move a task + * + * @param int $project_id + * @param int $src_column_id + * @param int $dst_column_id + * @return bool|int + */ + public function canMoveTask($project_id, $src_column_id, $dst_column_id) + { + $role = $this->getProjectUserRole($project_id); + + if ($this->role->isCustomProjectRole($role)) { + return $this->columnMoveRestrictionModel->isAllowed( + $project_id, + $role, + $src_column_id, + $dst_column_id + ); + } + + return true; + } + + /** + * Return true if the user can remove a task + * + * Regular users can't remove tasks from other people + * + * @public + * @param array $task + * @return bool + */ + public function canRemoveTask(array $task) + { + if (isset($task['creator_id']) && $task['creator_id'] == $this->userSession->getId()) { + return true; + } + + if ($this->userSession->isAdmin() || $this->getProjectUserRole($task['project_id']) === Role::PROJECT_MANAGER) { + return true; + } + + return false; + } + + /** + * Check project access + * + * @param string $controller + * @param string $action + * @param integer $project_id + * @return bool + */ + public function checkProjectAccess($controller, $action, $project_id) + { + if (! $this->userSession->isLogged()) { + return false; + } + + if ($this->userSession->isAdmin()) { + return true; + } + + if (! $this->helper->user->hasAccess($controller, $action)) { + return false; + } + + $role = $this->getProjectUserRole($project_id); + + if ($this->role->isCustomProjectRole($role)) { + $restrictions = $this->projectRoleRestrictionModel->getAllByRole($project_id, $role); + $result = $this->projectRoleRestrictionModel->isAllowed($restrictions, $controller, $action); + $result = $result && $this->projectAuthorization->isAllowed($controller, $action, Role::PROJECT_MEMBER); + } else { + $result = $this->projectAuthorization->isAllowed($controller, $action, $role); + } + + return $result; + } +} diff --git a/app/Helper/UserHelper.php b/app/Helper/UserHelper.php index 17c66616..8c2567b9 100644 --- a/app/Helper/UserHelper.php +++ b/app/Helper/UserHelper.php @@ -3,7 +3,6 @@ namespace Kanboard\Helper; use Kanboard\Core\Base; -use Kanboard\Core\Security\Role; /** * User helpers @@ -133,66 +132,14 @@ class UserHelper extends Base */ public function hasProjectAccess($controller, $action, $project_id) { - if (! $this->userSession->isLogged()) { - return false; - } - - if ($this->userSession->isAdmin()) { - return true; - } - - if (! $this->hasAccess($controller, $action)) { - return false; - } - $key = 'project_access:'.$controller.$action.$project_id; $result = $this->memoryCache->get($key); if ($result === null) { - $role = $this->getProjectUserRole($project_id); - - if ($this->role->isCustomProjectRole($role)) { - $role = Role::PROJECT_MEMBER; - } - - $result = $this->projectAuthorization->isAllowed($controller, $action, $role); + $result = $this->helper->projectRole->checkProjectAccess($controller, $action, $project_id); $this->memoryCache->set($key, $result); } return $result; } - - /** - * Get project role for the current user - * - * @access public - * @param integer $project_id - * @return string - */ - public function getProjectUserRole($project_id) - { - return $this->memoryCache->proxy($this->projectUserRoleModel, 'getUserRole', $project_id, $this->userSession->getId()); - } - - /** - * Return true if the user can remove a task - * - * Regular users can't remove tasks from other people - * - * @public - * @param array $task - * @return bool - */ - public function canRemoveTask(array $task) - { - if (isset($task['creator_id']) && $task['creator_id'] == $this->userSession->getId()) { - return true; - } - - if ($this->userSession->isAdmin() || $this->getProjectUserRole($task['project_id']) === Role::PROJECT_MANAGER) { - return true; - } - - return false; - } } diff --git a/app/Model/ProjectRoleModel.php b/app/Model/ProjectRoleModel.php index 82f22806..ed86d6ed 100644 --- a/app/Model/ProjectRoleModel.php +++ b/app/Model/ProjectRoleModel.php @@ -17,7 +17,7 @@ class ProjectRoleModel extends Base /** * Get list of project roles - * + * * @param int $project_id * @return array */ @@ -70,9 +70,14 @@ class ProjectRoleModel extends Base public function getAllWithRestrictions($project_id) { $roles = $this->getAll($project_id); - $restrictions = $this->columnMoveRestrictionModel->getAll($project_id); - $restrictions = array_column_index($restrictions, 'role_id'); - array_merge_relation($roles, $restrictions, 'restrictions', 'role_id'); + + $column_restrictions = $this->columnMoveRestrictionModel->getAll($project_id); + $column_restrictions = array_column_index($column_restrictions, 'role_id'); + array_merge_relation($roles, $column_restrictions, 'column_restrictions', 'role_id'); + + $project_restrictions = $this->projectRoleRestrictionModel->getAll($project_id); + $project_restrictions = array_column_index($project_restrictions, 'role_id'); + array_merge_relation($roles, $project_restrictions, 'project_restrictions', 'role_id'); return $roles; } diff --git a/app/Model/ProjectRoleRestrictionModel.php b/app/Model/ProjectRoleRestrictionModel.php new file mode 100644 index 00000000..0411838d --- /dev/null +++ b/app/Model/ProjectRoleRestrictionModel.php @@ -0,0 +1,164 @@ + array( + array('controller' => 'TaskCreationController', 'method' => '*'), + ) + ); + + /** + * Get rules + * + * @return array + */ + public function getRules() + { + return array( + self::RULE_TASK_CREATION => t('Task creation is not permitted'), + ); + } + + /** + * Get a single restriction + * + * @param integer $project_id + * @param integer $restriction_id + * @return array|null + */ + public function getById($project_id, $restriction_id) + { + return $this->db + ->table(self::TABLE) + ->eq('project_id', $project_id) + ->eq('restriction_id', $restriction_id) + ->findOne(); + } + + /** + * Get restrictions + * + * @param int $project_id + * @return array + */ + public function getAll($project_id) + { + $rules = $this->getRules(); + $restrictions = $this->db + ->table(self::TABLE) + ->columns( + self::TABLE.'.restriction_id', + self::TABLE.'.project_id', + self::TABLE.'.role_id', + self::TABLE.'.rule' + ) + ->eq(self::TABLE.'.project_id', $project_id) + ->findAll(); + + foreach ($restrictions as &$restriction) { + $restriction['title'] = $rules[$restriction['rule']]; + } + + return $restrictions; + } + + /** + * Get restrictions + * + * @param int $project_id + * @param string $role + * @return array + */ + public function getAllByRole($project_id, $role) + { + $rules = $this->db + ->table(self::TABLE) + ->columns( + self::TABLE.'.restriction_id', + self::TABLE.'.project_id', + self::TABLE.'.role_id', + self::TABLE.'.rule', + 'pr.role' + ) + ->eq(self::TABLE.'.project_id', $project_id) + ->eq('role', $role) + ->left(ProjectRoleModel::TABLE, 'pr', 'role_id', self::TABLE, 'role_id') + ->findAll(); + + foreach ($rules as &$rule) { + $rule['acl'] = $this->ruleMapping[$rule['rule']]; + } + + return $rules; + } + + /** + * Create a new restriction + * + * @param int $project_id + * @param int $role_id + * @param string $rule + * @return bool|int + */ + public function create($project_id, $role_id, $rule) + { + return $this->db->table(self::TABLE) + ->persist(array( + 'project_id' => $project_id, + 'role_id' => $role_id, + 'rule' => $rule, + )); + } + + /** + * Remove a restriction + * + * @param integer $restriction_id + * @return bool + */ + public function remove($restriction_id) + { + return $this->db->table(self::TABLE)->eq('restriction_id', $restriction_id)->remove(); + } + + /** + * Check if the controller/method is allowed + * + * @param array $restrictions + * @param string $controller + * @param string $method + * @return bool + */ + public function isAllowed(array $restrictions, $controller, $method) + { + $controller = strtolower($controller); + $method = strtolower($method); + + foreach ($restrictions as $restriction) { + foreach ($restriction['acl'] as $acl) { + $acl['controller'] = strtolower($acl['controller']); + $acl['method'] = strtolower($acl['method']); + + if ($acl['controller'] === $controller && ($acl['method'] === '*' || $acl['method'] === $method)) { + return false; + } + } + } + + return true; + } +} diff --git a/app/Schema/Mysql.php b/app/Schema/Mysql.php index 11c7f232..0da54597 100644 --- a/app/Schema/Mysql.php +++ b/app/Schema/Mysql.php @@ -6,7 +6,22 @@ use PDO; use Kanboard\Core\Security\Token; use Kanboard\Core\Security\Role; -const VERSION = 113; +const VERSION = 114; + +function version_114(PDO $pdo) +{ + $pdo->exec(" + CREATE TABLE project_role_has_restrictions ( + restriction_id INT NOT NULL AUTO_INCREMENT, + project_id INT NOT NULL, + role_id INT NOT NULL, + rule VARCHAR(255) NOT NULL, + UNIQUE(role_id, rule), + FOREIGN KEY(project_id) REFERENCES projects(id) ON DELETE CASCADE, + FOREIGN KEY(role_id) REFERENCES project_has_roles(role_id) ON DELETE CASCADE + ) ENGINE=InnoDB CHARSET=utf8 + "); +} function version_113(PDO $pdo) { diff --git a/app/Schema/Postgres.php b/app/Schema/Postgres.php index 2d1695d0..56cd9de9 100644 --- a/app/Schema/Postgres.php +++ b/app/Schema/Postgres.php @@ -6,7 +6,22 @@ use PDO; use Kanboard\Core\Security\Token; use Kanboard\Core\Security\Role; -const VERSION = 92; +const VERSION = 93; + +function version_93(PDO $pdo) +{ + $pdo->exec(" + CREATE TABLE project_role_has_restrictions ( + restriction_id SERIAL PRIMARY KEY, + project_id INTEGER NOT NULL, + role_id INTEGER NOT NULL, + rule VARCHAR(255) NOT NULL, + UNIQUE(role_id, rule), + FOREIGN KEY(project_id) REFERENCES projects(id) ON DELETE CASCADE, + FOREIGN KEY(role_id) REFERENCES project_has_roles(role_id) ON DELETE CASCADE + ) + "); +} function version_92(PDO $pdo) { diff --git a/app/Schema/Sqlite.php b/app/Schema/Sqlite.php index ecfa93de..c952d58f 100644 --- a/app/Schema/Sqlite.php +++ b/app/Schema/Sqlite.php @@ -6,7 +6,22 @@ use Kanboard\Core\Security\Token; use Kanboard\Core\Security\Role; use PDO; -const VERSION = 104; +const VERSION = 105; + +function version_105(PDO $pdo) +{ + $pdo->exec(" + CREATE TABLE project_role_has_restrictions ( + restriction_id INTEGER PRIMARY KEY, + project_id INTEGER NOT NULL, + role_id INTEGER NOT NULL, + rule VARCHAR(255) NOT NULL, + UNIQUE(role_id, rule), + FOREIGN KEY(project_id) REFERENCES projects(id) ON DELETE CASCADE, + FOREIGN KEY(role_id) REFERENCES project_has_roles(role_id) ON DELETE CASCADE + ) + "); +} function version_104(PDO $pdo) { diff --git a/app/ServiceProvider/ClassProvider.php b/app/ServiceProvider/ClassProvider.php index 98669e6d..4841d1f0 100644 --- a/app/ServiceProvider/ClassProvider.php +++ b/app/ServiceProvider/ClassProvider.php @@ -57,6 +57,7 @@ class ClassProvider implements ServiceProviderInterface 'ProjectMetadataModel', 'ProjectGroupRoleModel', 'ProjectRoleModel', + 'ProjectRoleRestrictionModel', 'ProjectTaskDuplicationModel', 'ProjectTaskPriorityModel', 'ProjectUserRoleModel', diff --git a/app/ServiceProvider/HelperProvider.php b/app/ServiceProvider/HelperProvider.php index a909e3cf..f4c0db22 100644 --- a/app/ServiceProvider/HelperProvider.php +++ b/app/ServiceProvider/HelperProvider.php @@ -35,6 +35,7 @@ class HelperProvider implements ServiceProviderInterface $container['helper']->register('url', '\Kanboard\Helper\UrlHelper'); $container['helper']->register('user', '\Kanboard\Helper\UserHelper'); $container['helper']->register('avatar', '\Kanboard\Helper\AvatarHelper'); + $container['helper']->register('projectRole', '\Kanboard\Helper\ProjectRoleHelper'); $container['helper']->register('projectHeader', '\Kanboard\Helper\ProjectHeaderHelper'); $container['helper']->register('projectActivity', '\Kanboard\Helper\ProjectActivityHelper'); $container['helper']->register('mail', '\Kanboard\Helper\MailHelper'); diff --git a/app/Template/project_role/show.php b/app/Template/project_role/show.php index 5fbd413b..595416ac 100644 --- a/app/Template/project_role/show.php +++ b/app/Template/project_role/show.php @@ -18,6 +18,10 @@