diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index a3be7845d..edcb358cb 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -55,6 +55,10 @@ class BoardController extends Controller { $this->userInfo = $this->getBoardPrerequisites(); } + /** + * TODO: move to boardservice + * @return array + */ private function getBoardPrerequisites() { $groups = $this->groupManager->getUserGroupIds( $this->userManager->get($this->userId) @@ -67,7 +71,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireNoPermission */ public function index() { return $this->boardService->findAll($this->userInfo); @@ -75,7 +78,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $boardId * @return \OCP\AppFramework\Db\Entity */ @@ -85,7 +87,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireNoPermission * @param $title * @param $color * @return \OCP\AppFramework\Db\Entity @@ -96,7 +97,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $id * @param $title * @param $color @@ -108,7 +108,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $boardId * @return \OCP\AppFramework\Db\Entity */ @@ -118,7 +117,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $boardId * @return array|bool * @internal param $userId @@ -135,7 +133,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $boardId * @param $type * @param $participant @@ -150,7 +147,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $id * @param $permissionWrite * @param $permissionInvite @@ -163,7 +159,6 @@ class BoardController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $aclId * @return \OCP\AppFramework\Db\Entity */ diff --git a/lib/Controller/CardController.php b/lib/Controller/CardController.php index f9641eb5a..ed506517f 100644 --- a/lib/Controller/CardController.php +++ b/lib/Controller/CardController.php @@ -42,7 +42,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $cardId * @return \OCP\AppFramework\Db\Entity */ @@ -52,7 +51,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @param $stackId * @param $order @@ -64,7 +62,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @param $title * @return \OCP\AppFramework\Db\Entity @@ -75,7 +72,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $title * @param $stackId * @param $type @@ -88,7 +84,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $id * @param $title * @param $stackId @@ -103,7 +98,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @return \OCP\AppFramework\Db\Entity */ @@ -113,7 +107,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @return \OCP\AppFramework\Db\Entity */ @@ -123,7 +116,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @return \OCP\AppFramework\Db\Entity */ @@ -133,7 +125,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @param $labelId */ @@ -143,7 +134,6 @@ class CardController extends Controller { /** * @NoAdminRequired - * @RequireEditPermission * @param $cardId * @param $labelId */ diff --git a/lib/Controller/LabelController.php b/lib/Controller/LabelController.php index 484324b1f..db805fbbb 100644 --- a/lib/Controller/LabelController.php +++ b/lib/Controller/LabelController.php @@ -44,7 +44,6 @@ class LabelController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $title * @param $color * @param $boardId @@ -56,7 +55,6 @@ class LabelController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $id * @param $title * @param $color @@ -68,7 +66,6 @@ class LabelController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $labelId * @return \OCP\AppFramework\Db\Entity */ diff --git a/lib/Controller/ShareController.php b/lib/Controller/ShareController.php index 1d7fa213c..3a4eabb33 100644 --- a/lib/Controller/ShareController.php +++ b/lib/Controller/ShareController.php @@ -55,7 +55,6 @@ class ShareController extends Controller { /** * @NoAdminRequired - * @RequireNoPermission * @param $search * @return array */ diff --git a/lib/Controller/StackController.php b/lib/Controller/StackController.php index c629f7312..2bb42acaa 100644 --- a/lib/Controller/StackController.php +++ b/lib/Controller/StackController.php @@ -44,7 +44,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $boardId * @return array */ @@ -54,7 +53,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $boardId * @return array */ @@ -64,7 +62,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireReadPermission * @param $boardId * @return */ @@ -74,7 +71,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $title * @param $boardId * @param int $order @@ -86,7 +82,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $id * @param $title * @param $boardId @@ -99,7 +94,6 @@ class StackController extends Controller { /** * @NoAdminRequired - * @RequireManagePermission * @param $stackId * @return \OCP\AppFramework\Db\Entity */ diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index 862049d6c..9d71bb67c 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -5,20 +5,20 @@ * @author Julius Härtl * * @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 . - * + * */ namespace OCA\Deck\Db; @@ -27,104 +27,103 @@ use OCP\AppFramework\Db\Entity; use OCP\IDBConnection; - class CardMapper extends DeckMapper implements IPermissionMapper { - private $labelMapper; + private $labelMapper; - public function __construct(IDBConnection $db, LabelMapper $labelMapper) { - parent::__construct($db, 'deck_cards', '\OCA\Deck\Db\Card'); - $this->labelMapper = $labelMapper; - } - - public function insert(Entity $entity) { - $entity->setCreatedAt(time()); - $entity->setLastModified(time()); - return parent::insert($entity); - } + public function __construct(IDBConnection $db, LabelMapper $labelMapper) { + parent::__construct($db, 'deck_cards', '\OCA\Deck\Db\Card'); + $this->labelMapper = $labelMapper; + } - public function update(Entity $entity) { - $entity->setLastModified(time()); - return parent::update($entity); - } + public function insert(Entity $entity) { + $entity->setCreatedAt(time()); + $entity->setLastModified(time()); + return parent::insert($entity); + } + + public function update(Entity $entity) { + $entity->setLastModified(time()); + return parent::update($entity); + } /** * @param $id * @return Entity if not found */ - public function find($id) { - $sql = 'SELECT * FROM `*PREFIX*deck_cards` ' . - 'WHERE `id` = ?'; - $card = $this->findEntity($sql, [$id]); - $labels = $this->labelMapper->findAssignedLabelsForCard($card->id); - $card->setLabels($labels); - return $card; - } + public function find($id) { + $sql = 'SELECT * FROM `*PREFIX*deck_cards` ' . + 'WHERE `id` = ?'; + $card = $this->findEntity($sql, [$id]); + $labels = $this->labelMapper->findAssignedLabelsForCard($card->id); + $card->setLabels($labels); + return $card; + } - public function findAll($stackId, $limit=null, $offset=null) { - $sql = 'SELECT * FROM `*PREFIX*deck_cards` + public function findAll($stackId, $limit = null, $offset = null) { + $sql = 'SELECT * FROM `*PREFIX*deck_cards` WHERE `stack_id` = ? AND NOT archived ORDER BY `order`'; - $entities = $this->findEntities($sql, [$stackId], $limit, $offset); - return $entities; - } + $entities = $this->findEntities($sql, [$stackId], $limit, $offset); + return $entities; + } - public function findAllArchived($stackId, $limit=null, $offset=null) { - $sql = 'SELECT * FROM `*PREFIX*deck_cards` WHERE `stack_id`=? AND archived ORDER BY `last_modified`'; - $entities = $this->findEntities($sql, [$stackId], $limit, $offset); - return $entities; - } + public function findAllArchived($stackId, $limit = null, $offset = null) { + $sql = 'SELECT * FROM `*PREFIX*deck_cards` WHERE `stack_id`=? AND archived ORDER BY `last_modified`'; + $entities = $this->findEntities($sql, [$stackId], $limit, $offset); + return $entities; + } - public function findAllByStack($stackId, $limit=null, $offset=null) { + public function findAllByStack($stackId, $limit = null, $offset = null) { $sql = 'SELECT id FROM `*PREFIX*deck_cards` WHERE `stack_id` = ?'; $entities = $this->findEntities($sql, [$stackId], $limit, $offset); return $entities; } - public function delete(Entity $entity) { + public function delete(Entity $entity) { // delete assigned labels $this->labelMapper->deleteLabelAssignmentsForCard($entity->getId()); // delete card - return parent::delete($entity); - } + return parent::delete($entity); + } - public function deleteByStack($stackId) { - $cards = $this->findAllByStack($stackId); + public function deleteByStack($stackId) { + $cards = $this->findAllByStack($stackId); foreach ($cards as $card) { $this->delete($card); } } - public function assignLabel($card, $label) { - $sql = 'INSERT INTO `*PREFIX*deck_assigned_labels` (`label_id`,`card_id`) VALUES (?,?)'; - $stmt = $this->db->prepare($sql); - $stmt->bindParam(1, $label, \PDO::PARAM_INT); - $stmt->bindParam(2, $card, \PDO::PARAM_INT); - $stmt->execute(); - } + public function assignLabel($card, $label) { + $sql = 'INSERT INTO `*PREFIX*deck_assigned_labels` (`label_id`,`card_id`) VALUES (?,?)'; + $stmt = $this->db->prepare($sql); + $stmt->bindParam(1, $label, \PDO::PARAM_INT); + $stmt->bindParam(2, $card, \PDO::PARAM_INT); + $stmt->execute(); + } - public function removeLabel($card, $label) { - $sql = 'DELETE FROM `*PREFIX*deck_assigned_labels` WHERE card_id = ? AND label_id = ?'; - $stmt = $this->db->prepare($sql); - $stmt->bindParam(1, $card, \PDO::PARAM_INT); - $stmt->bindParam(2, $label, \PDO::PARAM_INT); - $stmt->execute(); - } + public function removeLabel($card, $label) { + $sql = 'DELETE FROM `*PREFIX*deck_assigned_labels` WHERE card_id = ? AND label_id = ?'; + $stmt = $this->db->prepare($sql); + $stmt->bindParam(1, $card, \PDO::PARAM_INT); + $stmt->bindParam(2, $label, \PDO::PARAM_INT); + $stmt->execute(); + } - public function isOwner($userId, $cardId) { - $sql = 'SELECT owner FROM `*PREFIX*deck_boards` WHERE `id` IN (SELECT board_id FROM `*PREFIX*deck_stacks` WHERE id IN (SELECT stack_id FROM `*PREFIX*deck_cards` WHERE id = ?))'; - $stmt = $this->execute($sql, [$cardId]); - $row = $stmt->fetch(); - return ($row['owner'] === $userId); - } + public function isOwner($userId, $cardId) { + $sql = 'SELECT owner FROM `*PREFIX*deck_boards` WHERE `id` IN (SELECT board_id FROM `*PREFIX*deck_stacks` WHERE id IN (SELECT stack_id FROM `*PREFIX*deck_cards` WHERE id = ?))'; + $stmt = $this->execute($sql, [$cardId]); + $row = $stmt->fetch(); + return ($row['owner'] === $userId); + } - public function findBoardId($cardId) { - $sql = 'SELECT id FROM `*PREFIX*deck_boards` WHERE `id` IN (SELECT board_id FROM `*PREFIX*deck_stacks` WHERE id IN (SELECT stack_id FROM `*PREFIX*deck_cards` WHERE id = ?))'; - $stmt = $this->execute($sql, [$cardId]); - $row = $stmt->fetch(); - return $row['id']; - } + public function findBoardId($cardId) { + $sql = 'SELECT id FROM `*PREFIX*deck_boards` WHERE `id` IN (SELECT board_id FROM `*PREFIX*deck_stacks` WHERE id IN (SELECT stack_id FROM `*PREFIX*deck_cards` WHERE id = ?))'; + $stmt = $this->execute($sql, [$cardId]); + $row = $stmt->fetch(); + return $row['id']; + } } \ No newline at end of file diff --git a/lib/Middleware/SharingMiddleware.php b/lib/Middleware/SharingMiddleware.php index 50b511180..1be873bd9 100644 --- a/lib/Middleware/SharingMiddleware.php +++ b/lib/Middleware/SharingMiddleware.php @@ -68,33 +68,6 @@ class SharingMiddleware extends Middleware { $this->permissionService = $permissionService; } - /** - * All permission checks for controller access - * - * The following method annotations are possible - * - RequireReadPermission - * - RequireEditPermission - * - RequireSharePermission - * - RequireManagePermission - * - RequireNoPermission - * - * Depending on the Controller class we call a corresponding mapper to find the board_id - * With the board_id we can check for ownership/permissions in the acl table - * - * @param \OCP\AppFramework\Controller $controller - * @param string $methodName - * @throws NoPermissionException - */ - public function beforeController($controller, $methodName) { - $userId = null; - if ($this->userSession->getUser()) { - $userId = $this->userSession->getUser()->getUID(); - } - $method = $this->request->getMethod(); - $params = $this->request->getParams(); - $this->checkPermissions($userId, $controller, $method, $params, $methodName); - } - /** * Return JSON error response if the user has no sufficient permission * @@ -114,98 +87,4 @@ class SharingMiddleware extends Middleware { throw $exception; } - /** - * Check permission depending on the route (controller/method) - * - * @param $userId - * @param $controller - * @param $method - * @param $params - * @param $methodName - * @return bool - * @throws NoPermissionException - * @throws \Exception - */ - private function checkPermissions($userId, $controller, $method, $params, $methodName) { - - // no permission checks needed for plain html page or RequireNoPermission - if ($controller instanceof PageController || - $this->reflector->hasAnnotation('RequireNoPermission') - ) { - return true; - } - - $mapper = null; - $id = null; - - if ($controller instanceof BoardController) { - $mapper = $this->container->query('OCA\Deck\Db\BoardMapper'); - $id = $params['boardId']; - } - - if ($controller instanceof StackController) { - if ($method === "GET" || $method === "POST") { - $mapper = $this->container->query('OCA\Deck\Db\BoardMapper'); - $id = $params['boardId']; - } else { - $mapper = $this->container->query('OCA\Deck\Db\StackMapper'); - $id = $params['stackId']; - } - - } - if ($controller instanceof CardController) { - if ($method === "POST" && !preg_match('/Label/', $methodName)) { - $mapper = $this->container->query('OCA\Deck\Db\StackMapper'); - $id = $params['stackId']; - } else { - $mapper = $this->container->query('OCA\Deck\Db\CardMapper'); - $id = $params['cardId']; - } - - } - if ($controller instanceof LabelController) { - if ($method === "GET" || $method === "POST") { - $mapper = $this->container->query('OCA\Deck\Db\BoardMapper'); - $id = $params['boardId']; - } else { - $mapper = $this->container->query('OCA\Deck\Db\LabelMapper'); - $id = $params['labelId']; - } - } - - // check if there is a mapper so we can find the corresponding board for the request - if ($mapper === null) { - throw new \Exception("No mappers specified for permission checks"); - } - - $boardId = $mapper->findBoardId($id); - if (!$boardId) { - throw new NotFoundException("Entity not found"); - } - - if ($this->reflector->hasAnnotation('RequireReadPermission')) { - if (!$this->permissionService->getPermission($boardId, Acl::PERMISSION_READ)) { - throw new NoPermissionException("User " . $userId . " has no permission to read.", $controller, $methodName); - } - } - if ($this->reflector->hasAnnotation('RequireEditPermission')) { - if (!$this->permissionService->getPermission($boardId, Acl::PERMISSION_EDIT)) { - throw new NoPermissionException("User " . $userId . " has no permission to edit.", $controller, $methodName); - } - } - if ($this->reflector->hasAnnotation('RequireSharePermission')) { - if (!$this->permissionService->getPermission($boardId, Acl::PERMISSION_SHARE)) { - throw new NoPermissionException("User " . $userId . " has no permission to share.", $controller, $methodName); - } - } - if ($this->reflector->hasAnnotation('RequireManagePermission')) { - if (!$this->permissionService->getPermission($boardId, Acl::PERMISSION_MANAGE)) { - throw new NoPermissionException("User " . $userId . " has no permission to manage.", $controller, $methodName); - } - } - // all permission checks succeeded - return true; - - } - } \ No newline at end of file diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index f8b81e391..522e24922 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -38,10 +38,10 @@ use OCA\Deck\Db\LabelMapper; class BoardService { private $boardMapper; - private $aclMapper; private $labelMapper; - private $logger; + private $aclMapper; private $l10n; + private $permissionService; public function __construct( BoardMapper $boardMapper, @@ -49,14 +49,13 @@ class BoardService { IL10N $l10n, LabelMapper $labelMapper, AclMapper $aclMapper, - IGroupManager $groupManager + PermissionService $permissionService ) { $this->boardMapper = $boardMapper; $this->labelMapper = $labelMapper; $this->aclMapper = $aclMapper; - $this->logger = $logger; $this->l10n = $l10n; - $this->groupManager = $groupManager; + $this->permissionService = $permissionService; } public function findAll($userInfo) { @@ -67,6 +66,7 @@ class BoardService { } public function find($boardId) { + $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); return $this->boardMapper->find($boardId, true, true); } @@ -97,10 +97,12 @@ class BoardService { } public function delete($id) { + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ); return $this->boardMapper->delete($this->find($id)); } public function update($id, $title, $color) { + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE); $board = $this->find($id); $board->setTitle($title); $board->setColor($color); @@ -109,6 +111,7 @@ class BoardService { public function addAcl($boardId, $type, $participant, $write, $invite, $manage) { + $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_SHARE); $acl = new Acl(); $acl->setBoardId($boardId); $acl->setType($type); @@ -120,6 +123,7 @@ class BoardService { } public function updateAcl($id, $write, $invite, $manage) { + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_SHARE); $acl = $this->aclMapper->find($id); $acl->setPermissionWrite($write); $acl->setPermissionInvite($invite); @@ -128,6 +132,7 @@ class BoardService { } public function deleteAcl($id) { + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_SHARE); $acl = $this->aclMapper->find($id); return $this->aclMapper->delete($acl); } diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 784c6666b..39be7497e 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -25,21 +25,31 @@ namespace OCA\Deck\Service; use OCA\Deck\Db\Card; use OCA\Deck\Db\CardMapper; +use OCA\Deck\Db\Acl; use OCA\Deck\CardArchivedException; +use OCA\Deck\Db\StackMapper; class CardService { private $cardMapper; - public function __construct(CardMapper $cardMapper) { + public function __construct( + CardMapper $cardMapper, + StackMapper $stackMapper, + PermissionService $permissionService + ) { $this->cardMapper = $cardMapper; + $this->stackMapper = $stackMapper; + $this->permissionService = $permissionService; } public function find($cardId) { + $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); return $this->cardMapper->find($cardId); } public function create($title, $stackId, $type, $order, $owner) { + $this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT); $card = new Card(); $card->setTitle($title); $card->setStackId($stackId); @@ -51,10 +61,12 @@ class CardService { } public function delete($id) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); return $this->cardMapper->delete($this->cardMapper->find($id)); } public function update($id, $title, $stackId, $type, $order, $description, $owner) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($id); if($card->getArchived()) { throw new CardArchivedException(); @@ -69,6 +81,7 @@ class CardService { } public function rename($id, $title) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($id); if($card->getArchived()) { throw new CardArchivedException(); @@ -77,6 +90,7 @@ class CardService { return $this->cardMapper->update($card); } public function reorder($id, $stackId, $order) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); $cards = $this->cardMapper->findAll($stackId); $i = 0; foreach ($cards as $card) { @@ -102,18 +116,21 @@ class CardService { } public function archive($id) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($id); $card->setArchived(true); return $this->cardMapper->update($card); } public function unarchive($id) { + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($id); $card->setArchived(false); return $this->cardMapper->update($card); } public function assignLabel($cardId, $labelId) { + $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($cardId); if($card->getArchived()) { throw new CardArchivedException(); @@ -122,6 +139,7 @@ class CardService { } public function removeLabel($cardId, $labelId) { + $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); $card = $this->cardMapper->find($cardId); if($card->getArchived()) { throw new CardArchivedException(); diff --git a/lib/Service/LabelService.php b/lib/Service/LabelService.php index ad6bd11df..e339aabeb 100644 --- a/lib/Service/LabelService.php +++ b/lib/Service/LabelService.php @@ -24,8 +24,7 @@ namespace OCA\Deck\Service; use OCA\Deck\Db\Label; -use OCP\ILogger; -use OCP\IL10N; +use OCA\Deck\Db\Acl; use OCA\Deck\Db\LabelMapper; @@ -34,19 +33,22 @@ class LabelService { private $labelMapper; private $logger; - public function __construct(ILogger $logger, - IL10N $l10n, - LabelMapper $labelMapper) { + public function __construct( + LabelMapper $labelMapper, + PermissionService $permissionService + ) { $this->labelMapper = $labelMapper; - $this->logger = $logger; + $this->permissionService = $permissionService; } public function find($labelId) { + $this->permissionService->checkPermission($this->labelMapper, $labelId, Acl::PERMISSION_READ); $label = $this->labelMapper->find($labelId); return $label; } public function create($title, $color, $boardId) { + $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_MANAGE); $label = new Label(); $label->setTitle($title); $label->setColor($color); @@ -55,10 +57,12 @@ class LabelService { } public function delete($id) { + $this->permissionService->checkPermission($this->labelMapper, $id, Acl::PERMISSION_MANAGE); return $this->labelMapper->delete($this->find($id)); } public function update($id, $title, $color) { + $this->permissionService->checkPermission($this->labelMapper, $id, Acl::PERMISSION_MANAGE); $label = $this->find($id); $label->setTitle($title); $label->setColor($color); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 2dece19c6..6d3981342 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -26,6 +26,11 @@ namespace OCA\Deck\Service; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\BoardMapper; +use OCA\Deck\Db\Entity; +use OCA\Deck\Db\IPermissionMapper; +use OCA\Deck\NoPermissionException; +use OCA\Deck\NotFoundException; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\IGroupManager; use OCP\ILogger; @@ -84,6 +89,36 @@ class PermissionService { return $this->userCan($acls, $permission); } + /** + * check permissions for replacing dark magic middleware + * + * @param $mapper IPermissionMapper|null null if $id is a boardId + * @param $id int unique identifier of the Entity + * @param $permission int + * @return bool + * @throws NoPermissionException|NotFoundException + */ + public function checkPermission($mapper, $id, $permission) { + try { + if($mapper instanceof IPermissionMapper) { + $boardId = $mapper->findBoardId($id); + } else { + $boardId = $id; + } + if($boardId === null) { + throw new NotFoundException('No entity found'); + } + if (!$this->getPermission($boardId, $permission)) { + $class = new \ReflectionClass($mapper); + $constants = array_flip($class->getConstants()); + throw new NoPermissionException('Permission ' . $constants[$permission] . ' not granted.'); + } + } catch (DoesNotExistException $exception) { + throw new NotFoundException('Permission denied'); + } + return true; + } + /** * @param $boardId * @return bool diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php index 5e3d4e138..63ec55370 100644 --- a/lib/Service/StackService.php +++ b/lib/Service/StackService.php @@ -23,6 +23,7 @@ namespace OCA\Deck\Service; +use OCA\Deck\Db\Acl; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\LabelMapper; use OCP\ILogger; @@ -41,17 +42,20 @@ class StackService { private $cardMapper; private $logger; private $labelMapper; + private $permissionService; public function __construct(StackMapper $stackMapper, CardMapper $cardMapper, LabelMapper $labelMapper, ILogger $logger, IL10N $l10n, - ITimeFactory $timeFactory) { + ITimeFactory $timeFactory, PermissionService $permissionService) { $this->stackMapper = $stackMapper; $this->cardMapper = $cardMapper; $this->labelMapper = $labelMapper; $this->logger = $logger; + $this->permissionService = $permissionService; } public function findAll($boardId) { + $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_READ); $stacks = $this->stackMapper->findAll($boardId); $labels = $this->labelMapper->getAssignedLabelsForBoard($boardId); foreach ($stacks as $stackIndex => $stack) { @@ -67,6 +71,7 @@ class StackService { } public function findAllArchived($boardId) { + $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_READ); $stacks = $this->stackMapper->findAll($boardId); $labels = $this->labelMapper->getAssignedLabelsForBoard($boardId); foreach ($stacks as $stackIndex => $stack) { @@ -82,6 +87,7 @@ class StackService { } public function create($title, $boardId, $order) { + $this->permissionService->checkPermission(null, $boardId, Acl::PERMISSION_MANAGE); $stack = new Stack(); $stack->setTitle($title); $stack->setBoardId($boardId); @@ -91,10 +97,12 @@ class StackService { } public function delete($id) { + $this->permissionService->checkPermission($this->stackMapper, $id, Acl::PERMISSION_MANAGE); return $this->stackMapper->delete($this->stackMapper->find($id)); } public function update($id, $title, $boardId, $order) { + $this->permissionService->checkPermission($this->stackMapper, $id, Acl::PERMISSION_MANAGE); $stack = $this->stackMapper->find($id); $stack->setTitle($title); $stack->setBoardId($boardId); diff --git a/tests/unit/Middleware/SharingMiddlewareTest.php b/tests/unit/Middleware/SharingMiddlewareTest.php index 18e12b946..9bf86fc9a 100644 --- a/tests/unit/Middleware/SharingMiddlewareTest.php +++ b/tests/unit/Middleware/SharingMiddlewareTest.php @@ -71,68 +71,6 @@ class SharingMiddlewareTest extends \PHPUnit_Framework_TestCase { ); } - public function dataBeforeController() { - return [ - ['GET', '\OCA\Deck\Controller\PageController', 'index', false, true, 123], - ['GET', '\OCA\Deck\Controller\BoardController', 'index', false, true, 123], - ['GET', '\OCA\Deck\Controller\BoardController', 'read', true, true, 123], - ['GET', '\OCA\Deck\Controller\BoardController', 'read', true, true, 123, NoPermissionException::class], - ['GET', '\OCA\Deck\Controller\BoardController', 'read', false, false, null, NotFoundException::class], - ['GET', '\OCA\Deck\Controller\CardController', 'read', true, true, 123, NoPermissionException::class], - ['POST', '\OCA\Deck\Controller\CardController', 'reorder', true, true, 123, NoPermissionException::class], - ['PUT', '\OCA\Deck\Controller\BoardController', 'update', true, false, 123, NoPermissionException::class], - - ]; - } - - /** - * @dataProvider dataBeforeController - * @param $controllerClass - * @param $methodName - */ - public function testBeforeController($method, $controllerClass, $methodName, $getPermission, $success, $boardId, $exception=null) { - $controller = $this->getMockBuilder($controllerClass) - ->disableOriginalConstructor()->getMock(); - $mapper = $this->getMockBuilder(IPermissionMapper::class) - ->disableOriginalConstructor()->getMock(); - $mapper->expects($this->any())->method('findBoardId')->willReturn($boardId); - $mapper->expects($this->any())->method('isOwner')->willReturn(false); - $user = $this->getMockBuilder(IUser::class) - ->disableOriginalConstructor()->getMock(); - $user->expects($this->once())->method('getUID')->willReturn('user1'); - $controllerInstance = \OC::$server->query($controllerClass); - $this->reflector->reflect($controllerInstance, $methodName); - - $this->container->expects($this->any()) - ->method('query')->willReturn($mapper); - $this->userSession->expects($this->exactly(2))->method('getUser')->willReturn($user); - $this->request->expects($this->once())->method('getMethod')->willReturn($method); - if($getPermission === true) { - $this->permissionService - ->expects($this->once()) - ->method('getPermission') - ->willReturn($getPermission); - } - - if($success) { - $this->sharingMiddleware->beforeController($controller, $methodName); - } else { - try { - $this->sharingMiddleware->beforeController($controller, $methodName); - } catch (\Exception $e) { - $this->assertInstanceOf($exception, $e); - } - } - - } - - public function setUpPermissions() { - $this->permissionService->expects($this->once()) - ->method('getPermission') - ->with(123, Acl::PERMISSION_READ) - ->willReturn(true); - } - public function dataAfterException() { return [ diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index dc3c932ce..36b197cc3 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -57,7 +57,7 @@ class BoardServiceTest extends \PHPUnit_Framework_TestCase { ->disableOriginalConstructor()->getMock(); $this->labelMapper = $this->getMockBuilder(LabelMapper::class) ->disableOriginalConstructor()->getMock(); - $this->groupManager = $this->getMockBuilder(IGroupManager::class) + $this->permissionService = $this->getMockBuilder(PermissionService::class) ->disableOriginalConstructor()->getMock(); $this->service = new BoardService( @@ -66,7 +66,7 @@ class BoardServiceTest extends \PHPUnit_Framework_TestCase { $this->l10n, $this->labelMapper, $this->aclMapper, - $this->groupManager + $this->permissionService ); }