fix: check node on same storage when uploading chunks

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

Author: salmart-dev

apps/dav/lib/Upload/ChunkingV2Plugin.php

@@ -29,6 +29,7 @@
 use OCP\IConfig;
 use OCP\Lock\ILockingProvider;
 use Sabre\DAV\Exception\BadRequest;
+use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\InsufficientStorage;
 use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\Exception\NotFound;
@@ -107,18 +108,26 @@ public function forbiddenMethod(RequestInterface $request) {
 	 * @param string $path
 	 * @param bool $createIfNotExists
 	 * @return FutureFile|UploadFile|ICollection|INode
+	 * @throws Forbidden if the file already exists, but is not updateable
 	 */
 	private function getUploadFile(string $path, bool $createIfNotExists = false) {
 		try {
 			$actualFile = $this->server->tree->getNodeForPath($path);
-			// Only directly upload to the target file if it is on the same storage
-			// There may be further potential to optimize here by also uploading
-			// to other storages directly. This would require to also carefully pick
-			// the storage/path used in getStorage()
-			if ($actualFile instanceof File && $this->uploadFolder->getStorage()->getId() === $actualFile->getNode()->getStorage()->getId()) {
-				return $actualFile;
+			if ($actualFile instanceof File) {
+				$node = $actualFile->getNode();
+				// check that the node has update permissions
+				if (!$node->isUpdateable()) {
+					throw new Forbidden();
+				}
+				// Only directly upload to the target file if it is on the same storage
+				// There may be further potential to optimize here by also uploading
+				// to other storages directly. This would require to also carefully pick
+				// the storage/path used in getStorage()
+				if ($this->uploadFolder->getStorage()->getId() === $node->getStorage()->getId()) {
+					return $actualFile;
+				}
 			}
-		} catch (NotFound $e) {
+		} catch (NotFound) {
 			// If there is no target file we upload to the upload folder first
 		}
 

build/integration/dav_features/webdav-related.feature

@@ -737,3 +737,23 @@ Feature: webdav-related
 		When As an "user0"
 		And Downloading file "/üäöé/äöü.txt"
 		Then Downloaded content should be the created file
+
+	@s3-multipart
+	Scenario: Cannot overwrite a file the user may not update with new chunking v2
+		Given using new dav path
+		And user "user0" exists
+		And user "user1" exists
+		And As an "user1"
+		And user "user1" created a folder "/testshare"
+		And User "user1" copies file "/welcome.txt" to "/testshare/test.txt"
+		And as "user1" creating a share with
+			| path        | testshare |
+			| shareType   | 0         |
+			| permissions | 5         |
+			| shareWith   | user0     |
+		And user "user0" accepts last share
+		And As an "user0"
+		And user "user0" creates a file locally with "3" x 5 MB chunks
+		When user "user0" creates a new chunking v2 upload with id "chunking-update" and destination "/testshare/test.txt"
+		Then the HTTP status code should be "403"
+		And Downloaded content when downloading file "/testshare/test.txt" with range "bytes=0-6" should be "Welcome"

build/integration/features/bootstrap/WebDav.php

@@ -987,9 +987,17 @@ public function userCreatesANewChunkingv2UploadWithIdAndDestination($user, $id,
 		$this->s3MultipartDestination = $this->getTargetDestination($user, $targetDestination);
 		$this->newUploadId();
 		$destination = '/uploads/' . $user . '/' . $this->getUploadId($id);
-		$this->response = $this->makeDavRequest($user, 'MKCOL', $destination, [
-			'Destination' => $this->s3MultipartDestination,
-		], null, 'uploads');
+		try {
+			$this->response = $this->makeDavRequest($user, 'MKCOL', $destination, [
+				'Destination' => $this->s3MultipartDestination,
+			], null, 'uploads');
+		} catch (\GuzzleHttp\Exception\ServerException $e) {
+			// 5xx responses cause a server exception
+			$this->response = $e->getResponse();
+		} catch (\GuzzleHttp\Exception\ClientException $e) {
+			// 4xx responses cause a client exception
+			$this->response = $e->getResponse();
+		}
 	}
 
 	/**