diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 320e19578..1e9ff905a 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -43,13 +43,12 @@ class PermissionService { private $logger; private $userId; - public function __construct( - ILogger $logger, - AclMapper $aclMapper, - BoardMapper $boardMapper, - IGroupManager $groupManager, - $userId - ) { + public function __construct(ILogger $logger, + AclMapper $aclMapper, + BoardMapper $boardMapper, + IGroupManager $groupManager, + $userId + ) { $this->aclMapper = $aclMapper; $this->boardMapper = $boardMapper; $this->logger = $logger; @@ -74,21 +73,6 @@ class PermissionService { ]; } - /** - * Check if the current user has specified permissions on a board - * - * @param $boardId - * @param $permission - * @return bool - */ - public function getPermission($boardId, $permission) { - if ($this->userIsBoardOwner($boardId)) { - return true; - } - $acls = $this->aclMapper->findAll($boardId); - return $this->userCan($acls, $permission); - } - /** * check permissions for replacing dark magic middleware * @@ -96,7 +80,7 @@ class PermissionService { * @param $id int unique identifier of the Entity * @param $permission int * @return bool - * @throws NoPermissionException|NotFoundException + * @throws NoPermissionException */ public function checkPermission($mapper, $id, $permission) { try { @@ -106,18 +90,27 @@ class PermissionService { $boardId = $id; } if($boardId === null) { - throw new NotFoundException('No entity found'); + // Throw NoPermission to not leak information about existing entries + throw new NoPermissionException('Permission denied'); } - if (!$this->getPermission($boardId, $permission)) { - $class = new \ReflectionClass($mapper); - $constants = array_flip($class->getConstants()); - throw new NoPermissionException('Permission ' . $constants[$permission] . ' not granted.'); + + if ($this->userIsBoardOwner($boardId)) { + return true; } + $acls = $this->aclMapper->findAll($boardId); + $result = $this->userCan($acls, $permission); + if ($result) { + return true; + } + } catch (DoesNotExistException $exception) { - throw new NotFoundException('Permission denied'); + // Throw NoPermission to not leak information about existing entries + throw new NoPermissionException('Permission denied'); } - return true; - } + + throw new NoPermissionException('Permission denied.'); + + } /** * @param $boardId @@ -139,7 +132,7 @@ class PermissionService { * @param $permission * @return bool */ - public function userCan($acls, $permission) { + public function userCan(array $acls, $permission) { // check for users foreach ($acls as $acl) { if ($acl->getType() === "user" && $acl->getParticipant() === $this->userId) { diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php index a87f612a2..e105edf34 100644 --- a/tests/unit/Service/PermissionServiceTest.php +++ b/tests/unit/Service/PermissionServiceTest.php @@ -27,13 +27,16 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; +use OCA\Deck\Db\IPermissionMapper; +use OCA\Deck\NoPermissionException; use OCP\IGroupManager; use OCP\ILogger; -use PHPUnit_Framework_TestCase; class PermissionServiceTest extends \PHPUnit_Framework_TestCase { + /** @var \PHPUnit_Framework_MockObject_MockObject|PermissionService */ private $service; + /** @var \PHPUnit_Framework_MockObject_MockObject|ILogger */ private $logger; private $aclMapper; private $boardMapper; @@ -113,27 +116,6 @@ class PermissionServiceTest extends \PHPUnit_Framework_TestCase { $this->assertEquals($expected, $this->service->getPermissions(123)); } - public function testGetPermission() { - $board = new Board(); - $board->setOwner('admin'); - $this->boardMapper->expects($this->exactly(4))->method('find')->with(123)->willReturn($board); - $this->assertEquals(true, $this->service->getPermission(123, Acl::PERMISSION_READ)); - $this->assertEquals(true, $this->service->getPermission(123, Acl::PERMISSION_EDIT)); - $this->assertEquals(true, $this->service->getPermission(123, Acl::PERMISSION_MANAGE)); - $this->assertEquals(true, $this->service->getPermission(123, Acl::PERMISSION_SHARE)); - } - - public function testGetPermissionFail() { - $board = new Board(); - $board->setOwner('user1'); - $this->boardMapper->expects($this->exactly(4))->method('find')->with(234)->willReturn($board); - $this->aclMapper->expects($this->exactly(4))->method('findAll')->willReturn([]); - $this->assertEquals(false, $this->service->getPermission(234, Acl::PERMISSION_READ)); - $this->assertEquals(false, $this->service->getPermission(234, Acl::PERMISSION_EDIT)); - $this->assertEquals(false, $this->service->getPermission(234, Acl::PERMISSION_MANAGE)); - $this->assertEquals(false, $this->service->getPermission(234, Acl::PERMISSION_SHARE)); - } - public function testUserIsBoardOwner() { $board = new Board(); $board->setOwner('admin'); @@ -176,12 +158,127 @@ class PermissionServiceTest extends \PHPUnit_Framework_TestCase { $this->assertEquals($canEdit, $this->service->userCan($acls, Acl::PERMISSION_EDIT)); $this->assertEquals($canShare, $this->service->userCan($acls, Acl::PERMISSION_SHARE)); $this->assertEquals($canManage, $this->service->userCan($acls, Acl::PERMISSION_MANAGE)); - - - } + } public function testUserCanFail() { $this->assertFalse($this->service->userCan([], Acl::PERMISSION_EDIT)); } -} \ No newline at end of file + public function dataCheckPermission() { + return [ + // see getAcls() for set permissions + [1, Acl::PERMISSION_READ, true], + [1, Acl::PERMISSION_EDIT, false], + [1, Acl::PERMISSION_MANAGE, false], + [1, Acl::PERMISSION_SHARE, false], + + [2, Acl::PERMISSION_READ, true], + [2, Acl::PERMISSION_EDIT, true], + [2, Acl::PERMISSION_MANAGE, false], + [2, Acl::PERMISSION_SHARE, false], + + [3, Acl::PERMISSION_READ, true], + [3, Acl::PERMISSION_EDIT, false], + [3, Acl::PERMISSION_MANAGE, true], + [3, Acl::PERMISSION_SHARE, false], + + [4, Acl::PERMISSION_READ, true], + [4, Acl::PERMISSION_EDIT, false], + [4, Acl::PERMISSION_MANAGE, false], + [4, Acl::PERMISSION_SHARE, true], + + [null, Acl::PERMISSION_READ, false], + [6, Acl::PERMISSION_READ, false], + + [1, Acl::PERMISSION_READ, true, 'admin'], + [1, Acl::PERMISSION_EDIT, true, 'admin'], + [1, Acl::PERMISSION_MANAGE, true, 'admin'], + [1, Acl::PERMISSION_SHARE, true, 'admin'], + ]; + } + + /** @dataProvider dataCheckPermission */ + public function testCheckPermission($boardId, $permission, $result, $owner='foo') { + // Setup mapper + $mapper = $this->getMockBuilder(IPermissionMapper::class)->getMock(); + + // board owner + $mapper->expects($this->once())->method('findBoardId')->willReturn($boardId); + $board = new Board(); + $board->setId($boardId); + $board->setOwner($owner); + $this->boardMapper->expects($this->any())->method('find')->willReturn($board); + + // acl check + $acls = $this->getAcls($boardId); + $this->aclMapper->expects($this->any())->method('findAll')->willReturn($acls); + + + if($result) { + $actual = $this->service->checkPermission($mapper, 1234, $permission); + $this->assertTrue($actual); + } else { + $this->setExpectedException(NoPermissionException::class); + $this->service->checkPermission($mapper, 1234, $permission); + } + + } + + /** @dataProvider dataCheckPermission */ + public function testCheckPermissionWithoutMapper($boardId, $permission, $result, $owner='foo') { + $mapper = null; + $board = new Board(); + $board->setId($boardId); + $board->setOwner($owner); + $this->boardMapper->expects($this->any())->method('find')->willReturn($board); + $acls = $this->getAcls($boardId); + $this->aclMapper->expects($this->any())->method('findAll')->willReturn($acls); + + if($result) { + $actual = $this->service->checkPermission($mapper, 1234, $permission); + $this->assertTrue($actual); + } else { + $this->setExpectedException(NoPermissionException::class); + $this->service->checkPermission($mapper, 1234, $permission); + } + + } + + public function testCheckPermissionNotFound() { + $mapper = $this->getMockBuilder(IPermissionMapper::class)->getMock(); + $mapper->expects($this->once())->method('findBoardId')->willThrowException(new NoPermissionException(null)); + $this->setExpectedException(NoPermissionException::class); + $this->service->checkPermission($mapper, 1234, Acl::PERMISSION_READ); + } + + private function generateAcl($boardId, $type, $participant, $write, $manage, $share) { + $acl = new Acl(); + $acl->setParticipant($participant); + $acl->setBoardId($boardId); + $acl->setType($type); + $acl->setPermissionWrite($write); + $acl->setPermissionInvite($share); + $acl->setPermissionManage($manage); + return $acl; + } + + private function getAcls($boardId) { + $acls = [ + $this->generateAcl(1, 'user', 'admin', false, false, false), + $this->generateAcl(2, 'user', 'admin', true, false, false), + $this->generateAcl(3, 'user', 'admin', false, true, false), + $this->generateAcl(4, 'user', 'admin', false, false, true), + $this->generateAcl(5, 'group', 'admin', false, false, false), + $this->generateAcl(6, 'user', 'foo', false, false, false) + ]; + $result = []; + foreach ($acls as $acl) { + if($acl->getBoardId() === $boardId) { + $result[] = $acl; + } + } + return $result; + } + + +}