From 614bd5c29419c9f45d4fa826539c544a6d7c2e26 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 24 Feb 2017 11:56:29 +0100 Subject: [PATCH] Properly handle missing READ permission --- apps/dav/lib/Connector/Sabre/Directory.php | 11 ++++ apps/dav/lib/Connector/Sabre/File.php | 5 ++ apps/dav/lib/Connector/Sabre/FilesPlugin.php | 4 ++ .../unit/Connector/Sabre/DirectoryTest.php | 11 ++-- .../tests/unit/Connector/Sabre/FileTest.php | 19 +++++++ .../unit/Connector/Sabre/FilesPluginTest.php | 55 +++++++++++++++++-- .../Connector/Sabre/FilesReportPluginTest.php | 6 ++ 7 files changed, 100 insertions(+), 11 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 9afa636768..579dbbabf4 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -44,6 +44,7 @@ use Sabre\DAV\INode; use Sabre\DAV\Exception\BadRequest; use OC\Files\Mount\MoveableMount; use Sabre\DAV\IFile; +use Sabre\DAV\Exception\NotFound; class Directory extends \OCA\DAV\Connector\Sabre\Node implements \Sabre\DAV\ICollection, \Sabre\DAV\IQuota, \Sabre\DAV\IMoveTarget { @@ -199,6 +200,11 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node * @throws \Sabre\DAV\Exception\ServiceUnavailable */ public function getChild($name, $info = null) { + if (!$this->info->isReadable()) { + // avoid detecting files through this way + throw new NotFound(); + } + $path = $this->path . '/' . $name; if (is_null($info)) { try { @@ -232,12 +238,17 @@ class Directory extends \OCA\DAV\Connector\Sabre\Node * Returns an array with all the child nodes * * @return \Sabre\DAV\INode[] + * @throws \Sabre\DAV\Exception\Locked + * @throws \OCA\DAV\Connector\Sabre\Exception\Forbidden */ public function getChildren() { if (!is_null($this->dirContent)) { return $this->dirContent; } try { + if (!$this->info->isReadable()) { + throw new Forbidden('No read permissions'); + } $folderContent = $this->fileView->getDirectoryContent($this->path); } catch (LockedException $e) { throw new Locked(); diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 1f878df156..7a8bdb1da7 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -54,6 +54,7 @@ use Sabre\DAV\Exception\Forbidden; use Sabre\DAV\Exception\NotImplemented; use Sabre\DAV\Exception\ServiceUnavailable; use Sabre\DAV\IFile; +use Sabre\DAV\Exception\NotFound; class File extends Node implements IFile { @@ -307,6 +308,10 @@ class File extends Node implements IFile { public function get() { //throw exception if encryption is disabled but files are still encrypted try { + if (!$this->info->isReadable()) { + // do a if the file did not exist + throw new NotFound(); + } $res = $this->fileView->fopen(ltrim($this->path, '/'), 'rb'); if ($res === false) { throw new ServiceUnavailable("Could not open file"); diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index 929cd1b0be..a4f3f363a5 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -286,6 +286,10 @@ class FilesPlugin extends ServerPlugin { $httpRequest = $this->server->httpRequest; if ($node instanceof \OCA\DAV\Connector\Sabre\Node) { + if (!$node->getFileInfo()->isReadable()) { + // avoid detecting files through this means + throw new NotFound(); + } $propFind->handle(self::FILEID_PROPERTYNAME, function() use ($node) { return $node->getFileId(); diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index fa39cd7727..7d4583c801 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -77,12 +77,11 @@ class DirectoryTest extends \Test\TestCase { protected function setUp() { parent::setUp(); - $this->view = $this->getMockBuilder('OC\Files\View') - ->disableOriginalConstructor() - ->getMock(); - $this->info = $this->getMockBuilder('OC\Files\FileInfo') - ->disableOriginalConstructor() - ->getMock(); + $this->view = $this->createMock('OC\Files\View'); + $this->info = $this->createMock('OC\Files\FileInfo'); + $this->info->expects($this->any()) + ->method('isReadable') + ->will($this->returnValue(true)); } private function getDir($path = '/') { diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 31344b3646..81b7ad27d6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -1003,4 +1003,23 @@ class FileTest extends \Test\TestCase { $file->get(); } + + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testGetThrowsIfNoPermission() { + $view = $this->getMockBuilder(View::class) + ->setMethods(['fopen']) + ->getMock(); + $view->expects($this->never()) + ->method('fopen'); + + $info = new FileInfo('/test.txt', $this->getMockStorage(), null, [ + 'permissions' => Constants::PERMISSION_CREATE // no read perm + ], null); + + $file = new File($view, $info); + + $file->get(); + } } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php index e0dea49c49..739c8f6254 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesPluginTest.php @@ -34,6 +34,7 @@ use Sabre\HTTP\ResponseInterface; use Test\TestCase; use OCA\DAV\Upload\FutureFile; use OCA\DAV\Connector\Sabre\Directory; +use OCP\Files\FileInfo; /** * Copyright (c) 2015 Vincent Petry @@ -148,13 +149,15 @@ class FilesPluginTest extends TestCase { $node->expects($this->any()) ->method('getDavPermissions') ->will($this->returnValue('DWCKMSR')); + + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(true); + $node->expects($this->any()) ->method('getFileInfo') - ->will($this->returnValue( - $this->getMockBuilder('\OCP\Files\FileInfo') - ->disableOriginalConstructor() - ->getMock() - )); + ->willReturn($fileInfo); return $node; } @@ -313,6 +316,15 @@ class FilesPluginTest extends TestCase { ->getMock(); $node->expects($this->any())->method('getPath')->willReturn('/'); + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(true); + + $node->expects($this->any()) + ->method('getFileInfo') + ->willReturn($fileInfo); + $propFind = new PropFind( '/', [ @@ -329,6 +341,39 @@ class FilesPluginTest extends TestCase { $this->assertEquals('my_fingerprint', $propFind->get(self::DATA_FINGERPRINT_PROPERTYNAME)); } + /** + * @expectedException \Sabre\DAV\Exception\NotFound + */ + public function testGetPropertiesWhenNoPermission() { + /** @var \OCA\DAV\Connector\Sabre\Directory | \PHPUnit_Framework_MockObject_MockObject $node */ + $node = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory') + ->disableOriginalConstructor() + ->getMock(); + $node->expects($this->any())->method('getPath')->willReturn('/'); + + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->expects($this->any()) + ->method('isReadable') + ->willReturn(false); + + $node->expects($this->any()) + ->method('getFileInfo') + ->willReturn($fileInfo); + + $propFind = new PropFind( + '/test', + [ + self::DATA_FINGERPRINT_PROPERTYNAME, + ], + 0 + ); + + $this->plugin->handleGetProperties( + $propFind, + $node + ); + } + public function testUpdateProps() { $node = $this->createTestNode('\OCA\DAV\Connector\Sabre\File'); diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 4d8a87b093..3ca131dbf6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -34,6 +34,7 @@ use OCP\Files\Folder; use OCP\IGroupManager; use OCP\SystemTag\ISystemTagManager; use OCP\ITags; +use OCP\Files\FileInfo; class FilesReportPluginTest extends \Test\TestCase { /** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */ @@ -349,6 +350,9 @@ class FilesReportPluginTest extends \Test\TestCase { public function testPrepareResponses() { $requestedProps = ['{DAV:}getcontentlength', '{http://owncloud.org/ns}fileid', '{DAV:}resourcetype']; + $fileInfo = $this->createMock(FileInfo::class); + $fileInfo->method('isReadable')->willReturn(true); + $node1 = $this->getMockBuilder('\OCA\DAV\Connector\Sabre\Directory') ->disableOriginalConstructor() ->getMock(); @@ -362,6 +366,7 @@ class FilesReportPluginTest extends \Test\TestCase { $node1->expects($this->any()) ->method('getPath') ->will($this->returnValue('/node1')); + $node1->method('getFileInfo')->willReturn($fileInfo); $node2->expects($this->once()) ->method('getInternalFileId') ->will($this->returnValue('222')); @@ -371,6 +376,7 @@ class FilesReportPluginTest extends \Test\TestCase { $node2->expects($this->any()) ->method('getPath') ->will($this->returnValue('/sub/node2')); + $node2->method('getFileInfo')->willReturn($fileInfo); $config = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor()