From d72c0ffbc6c68f02c46d4060996738aabc869a6f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 1 Feb 2016 18:18:17 +0100 Subject: [PATCH] Make sure user has access to file for system tag operations Fixes DAV's SystemTagsObjectTypeCollection to not give access to files where the current user doesn't have access to. --- apps/dav/lib/rootcollection.php | 3 +- apps/dav/lib/systemtag/systemtagplugin.php | 7 +-- .../systemtagsobjecttypecollection.php | 23 ++++++++- .../systemtagsrelationscollection.php | 8 +++- .../tests/unit/systemtag/systemtagplugin.php | 34 ++++++++++++++ .../systemtagsobjecttypecollection.php | 47 +++++++++++++++++-- 6 files changed, 109 insertions(+), 13 deletions(-) diff --git a/apps/dav/lib/rootcollection.php b/apps/dav/lib/rootcollection.php index b7c2995dbad..5be469e7b0c 100644 --- a/apps/dav/lib/rootcollection.php +++ b/apps/dav/lib/rootcollection.php @@ -68,7 +68,8 @@ class RootCollection extends SimpleCollection { \OC::$server->getSystemTagManager(), \OC::$server->getSystemTagObjectMapper(), \OC::$server->getUserSession(), - \OC::$server->getGroupManager() + \OC::$server->getGroupManager(), + \OC::$server->getRootFolder() ); $commentsCollection = new Comments\RootCollection( \OC::$server->getCommentsManager(), diff --git a/apps/dav/lib/systemtag/systemtagplugin.php b/apps/dav/lib/systemtag/systemtagplugin.php index 4636ed428b1..3348b431c47 100644 --- a/apps/dav/lib/systemtag/systemtagplugin.php +++ b/apps/dav/lib/systemtag/systemtagplugin.php @@ -104,12 +104,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { $path = $request->getPath(); // Making sure the node exists - try { - $node = $this->server->tree->getNodeForPath($path); - } catch (NotFound $e) { - return null; - } - + $node = $this->server->tree->getNodeForPath($path); if ($node instanceof SystemTagsByIdCollection || $node instanceof SystemTagsObjectMappingCollection) { $data = $request->getBodyAsString(); diff --git a/apps/dav/lib/systemtag/systemtagsobjecttypecollection.php b/apps/dav/lib/systemtag/systemtagsobjecttypecollection.php index 166e3219bc1..93cc7561928 100644 --- a/apps/dav/lib/systemtag/systemtagsobjecttypecollection.php +++ b/apps/dav/lib/systemtag/systemtagsobjecttypecollection.php @@ -25,12 +25,14 @@ namespace OCA\DAV\SystemTag; use Sabre\DAV\Exception\Forbidden; use Sabre\DAV\Exception\MethodNotAllowed; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\ICollection; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; use OCP\IUserSession; use OCP\IGroupManager; +use OCP\Files\IRootFolder; /** * Collection containing object ids by object type @@ -62,6 +64,11 @@ class SystemTagsObjectTypeCollection implements ICollection { */ private $userSession; + /** + * @var IRootFolder + **/ + protected $fileRoot; + /** * Constructor * @@ -70,19 +77,22 @@ class SystemTagsObjectTypeCollection implements ICollection { * @param ISystemTagObjectMapper $tagMapper * @param IUserSession $userSession * @param IGroupManager $groupManager + * @param IRootFolder $fileRoot */ public function __construct( $objectType, ISystemTagManager $tagManager, ISystemTagObjectMapper $tagMapper, IUserSession $userSession, - IGroupManager $groupManager + IGroupManager $groupManager, + IRootFolder $fileRoot ) { $this->tagManager = $tagManager; $this->tagMapper = $tagMapper; $this->objectType = $objectType; $this->userSession = $userSession; $this->groupManager = $groupManager; + $this->fileRoot = $fileRoot; } /** @@ -116,6 +126,10 @@ class SystemTagsObjectTypeCollection implements ICollection { * @param string $objectId */ function getChild($objectId) { + // make sure the object exists and is reachable + if(!$this->childExists($objectId)) { + throw new NotFound('Entity does not exist or is not available'); + } return new SystemTagsObjectMappingCollection( $objectId, $this->objectType, @@ -134,6 +148,13 @@ class SystemTagsObjectTypeCollection implements ICollection { * @param string $name */ function childExists($name) { + // TODO: make this more abstract + if ($this->objectType === 'files') { + // make sure the object is reachable for the current user + $userId = $this->userSession->getUser()->getUID(); + $nodes = $this->fileRoot->getUserFolder($userId)->getById(intval($name)); + return !empty($nodes); + } return true; } diff --git a/apps/dav/lib/systemtag/systemtagsrelationscollection.php b/apps/dav/lib/systemtag/systemtagsrelationscollection.php index 223aaac93d6..6af4edfc1ab 100644 --- a/apps/dav/lib/systemtag/systemtagsrelationscollection.php +++ b/apps/dav/lib/systemtag/systemtagsrelationscollection.php @@ -28,6 +28,7 @@ use Sabre\DAV\Exception\Forbidden; use Sabre\DAV\SimpleCollection; use OCP\IUserSession; use OCP\IGroupManager; +use OCP\Files\IRootFolder; class SystemTagsRelationsCollection extends SimpleCollection { @@ -38,12 +39,14 @@ class SystemTagsRelationsCollection extends SimpleCollection { * @param ISystemTagObjectMapper $tagMapper * @param IUserSession $userSession * @param IGroupManager $groupManager + * @param IRootFolder $fileRoot */ public function __construct( ISystemTagManager $tagManager, ISystemTagObjectMapper $tagMapper, IUserSession $userSession, - IGroupManager $groupManager + IGroupManager $groupManager, + IRootFolder $fileRoot ) { $children = [ new SystemTagsObjectTypeCollection( @@ -51,7 +54,8 @@ class SystemTagsRelationsCollection extends SimpleCollection { $tagManager, $tagMapper, $userSession, - $groupManager + $groupManager, + $fileRoot ), ]; diff --git a/apps/dav/tests/unit/systemtag/systemtagplugin.php b/apps/dav/tests/unit/systemtag/systemtagplugin.php index 873dd7088a8..b026451701f 100644 --- a/apps/dav/tests/unit/systemtag/systemtagplugin.php +++ b/apps/dav/tests/unit/systemtag/systemtagplugin.php @@ -271,6 +271,40 @@ class SystemTagPlugin extends \Test\TestCase { $this->plugin->httpPost($request, $response); } + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testCreateTagToUnknownNode() { + $systemTag = new SystemTag(1, 'Test', true, false); + + $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagsObjectMappingCollection') + ->disableOriginalConstructor() + ->getMock(); + + $this->tree->expects($this->any()) + ->method('getNodeForPath') + ->will($this->throwException(new \Sabre\DAV\Exception\NotFound())); + + $this->tagManager->expects($this->never()) + ->method('createTag'); + + $node->expects($this->never()) + ->method('createFile'); + + $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') + ->disableOriginalConstructor() + ->getMock(); + $response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface') + ->disableOriginalConstructor() + ->getMock(); + + $request->expects($this->once()) + ->method('getPath') + ->will($this->returnValue('/systemtags-relations/files/12')); + + $this->plugin->httpPost($request, $response); + } + /** * @dataProvider nodeClassProvider * @expectedException Sabre\DAV\Exception\Conflict diff --git a/apps/dav/tests/unit/systemtag/systemtagsobjecttypecollection.php b/apps/dav/tests/unit/systemtag/systemtagsobjecttypecollection.php index e6d94803cc0..1d4264f94f9 100644 --- a/apps/dav/tests/unit/systemtag/systemtagsobjecttypecollection.php +++ b/apps/dav/tests/unit/systemtag/systemtagsobjecttypecollection.php @@ -38,6 +38,11 @@ class SystemTagsObjectTypeCollection extends \Test\TestCase { */ private $tagMapper; + /** + * @var \OCP\Files\Folder + */ + private $userFolder; + protected function setUp() { parent::setUp(); @@ -58,12 +63,21 @@ class SystemTagsObjectTypeCollection extends \Test\TestCase { ->with('testuser') ->will($this->returnValue(true)); + $this->userFolder = $this->getMock('\OCP\Files\Folder'); + + $fileRoot = $this->getMock('\OCP\Files\IRootFolder'); + $fileRoot->expects($this->any()) + ->method('getUserfolder') + ->with('testuser') + ->will($this->returnValue($this->userFolder)); + $this->node = new \OCA\DAV\SystemTag\SystemTagsObjectTypeCollection( 'files', $this->tagManager, $this->tagMapper, $userSession, - $groupManager + $groupManager, + $fileRoot ); } @@ -82,10 +96,25 @@ class SystemTagsObjectTypeCollection extends \Test\TestCase { } public function testGetChild() { - $childNode = $this->node->getChild('files'); + $this->userFolder->expects($this->once()) + ->method('getById') + ->with('555') + ->will($this->returnValue([true])); + $childNode = $this->node->getChild('555'); $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagsObjectMappingCollection', $childNode); - $this->assertEquals('files', $childNode->getName()); + $this->assertEquals('555', $childNode->getName()); + } + + /** + * @expectedException Sabre\DAV\Exception\NotFound + */ + public function testGetChildWithoutAccess() { + $this->userFolder->expects($this->once()) + ->method('getById') + ->with('555') + ->will($this->returnValue([])); + $this->node->getChild('555'); } /** @@ -96,9 +125,21 @@ class SystemTagsObjectTypeCollection extends \Test\TestCase { } public function testChildExists() { + $this->userFolder->expects($this->once()) + ->method('getById') + ->with('123') + ->will($this->returnValue([true])); $this->assertTrue($this->node->childExists('123')); } + public function testChildExistsWithoutAccess() { + $this->userFolder->expects($this->once()) + ->method('getById') + ->with('555') + ->will($this->returnValue([])); + $this->assertFalse($this->node->childExists('555')); + } + /** * @expectedException Sabre\DAV\Exception\Forbidden */ -- 2.39.5