Properly handle missing READ permission

diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php
index 9afa63676856c48f2626955f2be0f720a7f362a9..579dbbabf4435da74b7543e58a1e33ab27dd2c0d 100644 (file)
--- 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 1f878df1564eb5d33edbfdc65bd6b4ac2e7e670a..7a8bdb1da750bd8e5ace7a30f7a4c5a8fbeca9c0 100644 (file)
--- 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 929cd1b0bea2626eb26c0a3c864f31b0e012be48..a4f3f363a5fd39a77b9e1d7a15ed351ddd3dbaf9 100644 (file)
--- 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 fa39cd77274fc8dfddb06139f285b26858a2ae8e..7d4583c801efaa7940f0559e40fc5bd8107d760c 100644 (file)
--- 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 31344b36463d28b265ab5d67edc2d8638549c237..81b7ad27d6a5753e76639e31f77ffe864337a556 100644 (file)
--- 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 e0dea49c49b4d617cb838318ad9f5b337c43cfce..739c8f62540daf215a58fa37a545a07fedec3c18 100644 (file)
--- 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 <pvince81@owncloud.com>
@@ -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 4d8a87b093f28d1a700f64f7138d381292c2be17..3ca131dbf6f6819ad1db4d394aba41dd09af4ad3 100644 (file)
--- 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()