From: Vincent Petry <pvince81@owncloud.com>
Date: Fri, 24 Feb 2017 10:56:29 +0000 (+0100)
Subject: Properly handle missing READ permission
X-Git-Tag: v12.0.0beta1~35^2~4
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=614bd5c29419c9f45d4fa826539c544a6d7c2e26;p=nextcloud-server.git

Properly handle missing READ permission
---

diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php
index 9afa6367685..579dbbabf44 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 1f878df1564..7a8bdb1da75 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 929cd1b0bea..a4f3f363a5f 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 fa39cd77274..7d4583c801e 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 31344b36463..81b7ad27d6a 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 e0dea49c49b..739c8f62540 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 <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 4d8a87b093f..3ca131dbf6f 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()