Merge pull request #25448 from owncloud/stable9.1-webdav-copypermissions

[stable9.1] Additional perm check in Webdav

4 files changed, 218 insertions, 6 deletions

diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php
index 9e7d876187d..051272e862f 100644
--- a/apps/dav/lib/Connector/Sabre/ObjectTree.php
+++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php
@@ -283,6 +283,11 @@ class ObjectTree extends \Sabre\DAV\Tree {
 			throw new InvalidPath($ex->getMessage());
 		}
 
+		// Webdav's copy will implicitly do a delete+create, so only create+delete permissions are required
+		if (!$this->fileView->isCreatable($destinationDir)) {
+			throw new \Sabre\DAV\Exception\Forbidden();
+		}
+
 		try {
 			$this->fileView->copy($source, $destination);
 		} catch (StorageNotAvailableException $e) {
diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php
index 4a5e43376c0..b4f0b22dd70 100644
--- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php
@@ -142,6 +142,88 @@ class ObjectTreeTest extends \Test\TestCase {
 		$objectTree->move($source, $destination);
 	}
 
+	public function copyDataProvider() {
+		return [
+			// copy into same dir
+			['a', 'b', ''],
+			// copy into same dir
+			['a/a', 'a/b', 'a'],
+			// copy into another dir
+			['a', 'sub/a', 'sub'],
+		];
+	}
+
+	/**
+	 * @dataProvider copyDataProvider
+	 */
+	public function testCopy($sourcePath, $targetPath, $targetParent) {
+		$view = $this->getMock('\OC\Files\View');
+		$view->expects($this->once())
+			->method('verifyPath')
+			->with($targetParent)
+			->will($this->returnValue(true));
+		$view->expects($this->once())
+			->method('isCreatable')
+			->with($targetParent)
+			->will($this->returnValue(true));
+		$view->expects($this->once())
+			->method('copy')
+			->with($sourcePath, $targetPath)
+			->will($this->returnValue(true));
+
+		$info = new FileInfo('', null, null, array(), null);
+
+		$rootDir = new \OCA\DAV\Connector\Sabre\Directory($view, $info);
+		$objectTree = $this->getMock('\OCA\DAV\Connector\Sabre\ObjectTree',
+			array('nodeExists', 'getNodeForPath'),
+			array($rootDir, $view));
+
+		$objectTree->expects($this->once())
+			->method('getNodeForPath')
+			->with($this->identicalTo($sourcePath))
+			->will($this->returnValue(false));
+
+		/** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */
+		$mountManager = \OC\Files\Filesystem::getMountManager();
+		$objectTree->init($rootDir, $view, $mountManager);
+		$objectTree->copy($sourcePath, $targetPath);
+	}
+
+	/**
+	 * @dataProvider copyDataProvider
+	 * @expectedException \Sabre\DAV\Exception\Forbidden
+	 */
+	public function testCopyFailNotCreatable($sourcePath, $targetPath, $targetParent) {
+		$view = $this->getMock('\OC\Files\View');
+		$view->expects($this->once())
+			->method('verifyPath')
+			->with($targetParent)
+			->will($this->returnValue(true));
+		$view->expects($this->once())
+			->method('isCreatable')
+			->with($targetParent)
+			->will($this->returnValue(false));
+		$view->expects($this->never())
+			->method('copy');
+
+		$info = new FileInfo('', null, null, array(), null);
+
+		$rootDir = new \OCA\DAV\Connector\Sabre\Directory($view, $info);
+		$objectTree = $this->getMock('\OCA\DAV\Connector\Sabre\ObjectTree',
+			array('nodeExists', 'getNodeForPath'),
+			array($rootDir, $view));
+
+		$objectTree->expects($this->once())
+			->method('getNodeForPath')
+			->with($this->identicalTo($sourcePath))
+			->will($this->returnValue(false));
+
+		/** @var $objectTree \OCA\DAV\Connector\Sabre\ObjectTree */
+		$mountManager = \OC\Files\Filesystem::getMountManager();
+		$objectTree->init($rootDir, $view, $mountManager);
+		$objectTree->copy($sourcePath, $targetPath);
+	}
+
 	/**
 	 * @dataProvider nodeForPathProvider
 	 */
diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php
index 0abb8667739..3df37db72bb 100644
--- a/build/integration/features/bootstrap/WebDav.php
+++ b/build/integration/features/bootstrap/WebDav.php
@@ -67,7 +67,27 @@ trait WebDav {
 	public function userMovesFile($user, $fileSource, $fileDestination){
 		$fullUrl = substr($this->baseUrl, 0, -4) . $this->davPath;
 		$headers['Destination'] = $fullUrl . $fileDestination;
-		$this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers);
+		try {
+			$this->response = $this->makeDavRequest($user, "MOVE", $fileSource, $headers);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			$this->response = $e->getResponse();
+		}
+	}
+
+	/**
+	 * @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
+	 * @param string $user
+	 * @param string $fileSource
+	 * @param string $fileDestination
+	 */
+	public function userCopiesFile($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) {
+			$this->response = $e->getResponse();
+		}
 	}
 
 	/**
@@ -142,7 +162,11 @@ trait WebDav {
 	 * @param string $fileName
 	 */
 	public function downloadingFile($fileName) {
-		$this->response = $this->makeDavRequest($this->currentUser, 'GET', $fileName, []);
+		try {
+			$this->response = $this->makeDavRequest($this->currentUser, 'GET', $fileName, []);
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			$this->response = $e->getResponse();
+		}
 	}
 
 	/**
diff --git a/build/integration/features/webdav-related.feature b/build/integration/features/webdav-related.feature
index 06df280ea64..19913416afe 100644
--- a/build/integration/features/webdav-related.feature
+++ b/build/integration/features/webdav-related.feature
@@ -2,12 +2,113 @@ Feature: webdav-related
 	Background:
 		Given using api version "1"
 
-	Scenario: moving a file old way
+	Scenario: Moving a file
 		Given using dav path "remote.php/webdav"
 		And As an "admin"
 		And user "user0" exists
-		When User "user0" moves file "/textfile0.txt" to "/FOLDER/textfile0.txt"
+		And As an "user0"
+		When User "user0" moves file "/welcome.txt" to "/FOLDER/welcome.txt"
 		Then the HTTP status code should be "201"
+		And Downloaded content when downloading file "/FOLDER/welcome.txt" with range "bytes=0-6" should be "Welcome"
+
+	Scenario: Moving and overwriting a file old way
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And As an "user0"
+		When User "user0" moves file "/welcome.txt" to "/textfile0.txt"
+		Then the HTTP status code should be "204"
+		And Downloaded content when downloading file "/textfile0.txt" with range "bytes=0-6" should be "Welcome"
+
+	Scenario: Moving a file to a folder with no permissions
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And as "user1" creating a share with
+		  | path | testshare |
+		  | shareType | 0 |
+		  | permissions | 1 |
+		  | shareWith | user0 |
+		And As an "user0"
+		And User "user0" moves file "/textfile0.txt" to "/testshare/textfile0.txt"
+		And the HTTP status code should be "403"
+		When Downloading file "/testshare/textfile0.txt"
+ 		Then the HTTP status code should be "404"
+
+	Scenario: Moving a file to overwrite a file in a folder with no permissions
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And as "user1" creating a share with
+		  | path | testshare |
+		  | shareType | 0 |
+		  | permissions | 1 |
+		  | shareWith | user0 |
+		And User "user1" copies file "/welcome.txt" to "/testshare/overwritethis.txt"
+		And As an "user0"
+		When User "user0" moves file "/textfile0.txt" to "/testshare/overwritethis.txt"
+		Then the HTTP status code should be "403"
+		And Downloaded content when downloading file "/testshare/overwritethis.txt" with range "bytes=0-6" should be "Welcome"
+
+	Scenario: Copying a file
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And As an "user0"
+		When User "user0" copies file "/welcome.txt" to "/FOLDER/welcome.txt"
+		Then the HTTP status code should be "201"
+		And Downloaded content when downloading file "/FOLDER/welcome.txt" with range "bytes=0-6" should be "Welcome"
+
+	Scenario: Copying and overwriting a file
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And As an "user0"
+		When User "user0" copies file "/welcome.txt" to "/textfile1.txt"
+		Then the HTTP status code should be "204"
+		And Downloaded content when downloading file "/textfile1.txt" with range "bytes=0-6" should be "Welcome"
+
+	Scenario: Copying a file to a folder with no permissions
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And as "user1" creating a share with
+		  | path | testshare |
+		  | shareType | 0 |
+		  | permissions | 1 |
+		  | shareWith | user0 |
+		And As an "user0"
+		When User "user0" copies file "/textfile0.txt" to "/testshare/textfile0.txt"
+		Then the HTTP status code should be "403"
+		And Downloading file "/testshare/textfile0.txt"
+		And the HTTP status code should be "404"
+
+	Scenario: Copying a file to overwrite a file into a folder with no permissions
+		Given using dav path "remote.php/webdav"
+		And As an "admin"
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And as "user1" creating a share with
+		  | path | testshare |
+		  | shareType | 0 |
+		  | permissions | 1 |
+		  | shareWith | user0 |
+		And User "user1" copies file "/welcome.txt" to "/testshare/overwritethis.txt"
+		And As an "user0"
+		When User "user0" copies file "/textfile0.txt" to "/testshare/overwritethis.txt"
+		Then the HTTP status code should be "403"
+		And Downloaded content when downloading file "/testshare/overwritethis.txt" with range "bytes=0-6" should be "Welcome"
 
 	Scenario: download a file with range
 		Given using dav path "remote.php/webdav"
@@ -96,13 +197,13 @@ Feature: webdav-related
 		Given Logging in using web as "admin"
 		When Sending a "GET" to "/remote.php/webdav/welcome.txt" without requesttoken
 		Then Downloaded content should start with "Welcome to your ownCloud account!"
-		Then the HTTP status code should be "200"
+		And the HTTP status code should be "200"
 
 	Scenario: Doing a GET with a web login should work with CSRF token on the old backend
 		Given Logging in using web as "admin"
 		When Sending a "GET" to "/remote.php/webdav/welcome.txt" with requesttoken
 		Then Downloaded content should start with "Welcome to your ownCloud account!"
-		Then the HTTP status code should be "200"
+		And the HTTP status code should be "200"
 
 	Scenario: Doing a PROPFIND with a web login should not work without CSRF token on the old backend
 		Given Logging in using web as "admin"