Merge pull request #255 from nextcloud/dav-permission-check

add some additonal permission checks to the webdav backend

apps/dav/lib/Connector/Sabre/ObjectTree.php

@@ -71,7 +71,7 @@ class ObjectTree extends \Sabre\DAV\Tree {
 	 * is present.
 	 *
 	 * @param string $path chunk file path to convert
-	 * 
+	 *
 	 * @return string path to real file
 	 */
 	private function resolveChunkFile($path) {
@@ -186,9 +186,13 @@ class ObjectTree extends \Sabre\DAV\Tree {
 	 *
 	 * @param string $sourcePath The path to the file which should be moved
 	 * @param string $destinationPath The full destination path, so not just the destination parent node
-	 * @throws \Sabre\DAV\Exception\BadRequest
+	 * @throws FileLocked
-	 * @throws \Sabre\DAV\Exception\ServiceUnavailable
+	 * @throws Forbidden
+	 * @throws InvalidPath
 	 * @throws \Sabre\DAV\Exception\Forbidden
+	 * @throws \Sabre\DAV\Exception\Locked
+	 * @throws \Sabre\DAV\Exception\NotFound
+	 * @throws \Sabre\DAV\Exception\ServiceUnavailable
 	 * @return int
 	 */
 	public function move($sourcePath, $destinationPath) {
@@ -196,6 +200,15 @@ class ObjectTree extends \Sabre\DAV\Tree {
 			throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup');
 		}
 
+		$infoDestination = $this->fileView->getFileInfo(dirname($destinationPath));
+		$infoSource = $this->fileView->getFileInfo($sourcePath);
+		$destinationPermission = $infoDestination && $infoDestination->isUpdateable();
+		$sourcePermission =  $infoSource && $infoSource->isDeletable();
+
+		if (!$destinationPermission || !$sourcePermission) {
+			throw new Forbidden('No permissions to move object.');
+		}
+
 		$targetNodeExists = $this->nodeExists($destinationPath);
 		$sourceNode = $this->getNodeForPath($sourcePath);
 		if ($sourceNode instanceof \Sabre\DAV\ICollection && $targetNodeExists) {
@@ -265,6 +278,13 @@ class ObjectTree extends \Sabre\DAV\Tree {
 	 *
 	 * @param string $source
 	 * @param string $destination
+	 * @throws FileLocked
+	 * @throws Forbidden
+	 * @throws InvalidPath
+	 * @throws \Exception
+	 * @throws \Sabre\DAV\Exception\Forbidden
+	 * @throws \Sabre\DAV\Exception\Locked
+	 * @throws \Sabre\DAV\Exception\NotFound
 	 * @throws \Sabre\DAV\Exception\ServiceUnavailable
 	 * @return void
 	 */
@@ -273,6 +293,12 @@ class ObjectTree extends \Sabre\DAV\Tree {
 			throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup');
 		}
 
+
+		$info = $this->fileView->getFileInfo(dirname($destination));
+		if ($info && !$info->isUpdateable()) {
+			throw new Forbidden('No permissions to copy object.');
+		}
+
 		// this will trigger existence check
 		$this->getNodeForPath($source);
 

apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php

@@ -35,6 +35,7 @@ class TestDoubleFileView extends \OC\Files\View {
 		$this->updatables = $updatables;
 		$this->deletables = $deletables;
 		$this->canRename = $canRename;
+		$this->lockingProvider = \OC::$server->getLockingProvider();
 	}
 
 	public function isUpdatable($path) {
@@ -56,6 +57,11 @@ class TestDoubleFileView extends \OC\Files\View {
 	public function getRelativePath($path) {
 		return $path;
 	}
+
+	public function getFileInfo($path, $includeMountPoints = true) {
+		$objectTreeTest = new ObjectTreeTest();
+		return $objectTreeTest->getFileInfoMock();
+	}
 }
 
 /**
@@ -67,6 +73,20 @@ class TestDoubleFileView extends \OC\Files\View {
  */
 class ObjectTreeTest extends \Test\TestCase {
 
+	public function getFileInfoMock() {
+		$mock = $this->getMock('\OCP\Files\FileInfo');
+		$mock
+			->expects($this->any())
+			->method('isDeletable')
+			->willReturn(true);
+		$mock
+			->expects($this->any())
+			->method('isUpdateable')
+			->willReturn(true);
+
+		return $mock;
+	}
+
 	/**
 	 * @dataProvider moveFailedProvider
 	 * @expectedException \Sabre\DAV\Exception\Forbidden

build/integration/features/bootstrap/WebDav.php

@@ -42,6 +42,7 @@ trait WebDav {
 			$request->setBody($body);
 		}
 
+
 		return $client->send($request);
 	}
 
@@ -70,6 +71,23 @@ trait WebDav {
 		$this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers);
 	}
 
+	/**
+	 * @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
+	 * @param string $user
+	 * @param string $fileSource
+	 * @param string $fileDestination
+	 */
+	public function userCopiesFileTo($user, $fileSource, $fileDestination) {
+		$fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath;
+		$headers['Destination'] = $fullUrl . $fileDestination;
+		try {
+			$this->response = $this->makeDavRequest($user, 'COPY', $fileSource, $headers);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			// 4xx and 5xx responses cause an exception
+			$this->response = $e->getResponse();
+		}
+	}
+
 	/**
 	 * @When /^Downloading file "([^"]*)" with range "([^"]*)"$/
 	 * @param string $fileSource

build/integration/features/webdav-related.feature

@@ -257,3 +257,32 @@ Feature: webdav-related
 		When Downloading file "/welcome.txt" as "userToBeDisabled"
 		Then the HTTP status code should be "503"
 
+	Scenario: Copying files into a folder with edit permissions
+		Given using dav path "remote.php/webdav"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testcopypermissionsAllowed"
+		And as "user1" creating a share with
+			| path | testcopypermissionsAllowed |
+			| shareType | 0 |
+			| permissions | 31 |
+			| shareWith | user0 |
+		And User "user0" uploads file with content "copytest" to "/copytest.txt"
+		When User "user0" copies file "/copytest.txt" to "/testcopypermissionsAllowed/copytest.txt"
+		Then the HTTP status code should be "201"
+
+	Scenario: Copying files into a folder without edit permissions
+		Given using dav path "remote.php/webdav"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testcopypermissionsNotAllowed"
+		And as "user1" creating a share with
+			| path | testcopypermissionsNotAllowed |
+			| shareType | 0 |
+			| permissions | 1 |
+			| shareWith | user0 |
+		And User "user0" uploads file with content "copytest" to "/copytest.txt"
+		When User "user0" copies file "/copytest.txt" to "/testcopypermissionsNotAllowed/copytest.txt"
+		Then the HTTP status code should be "403"