Merge pull request #15769 from nextcloud/part-file-non-creatable

dont use part files for dav writes when the target folder doesn't have create permissions
nextcloud · Jul 3, 2019 · 18b673e · 18b673e
2 parents c5c14d0 + 3b6df74
commit 18b673e
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 10 deletions.
diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php
@@ -147,7 +147,12 @@ public function put($data) {
 		if ($needsPartFile) {
 			// mark file as partial while uploading (ignored by the scanner)
 			$partFilePath = $this->getPartFileBasePath($this->path) . '.ocTransferId' . rand() . '.part';
-		} else {
+
+			if (!$view->isCreatable($partFilePath) && $view->isUpdatable($this->path)) {
+				$needsPartFile = false;
+			}
+		}
+		if (!$needsPartFile) {
 			// upload file directly as the final path
 			$partFilePath = $this->path;
 
@@ -178,7 +183,7 @@ public function put($data) {
 				}
 
 				$isEOF = false;
-				$wrappedData = CallbackWrapper::wrap($data, null, null, null, null, function($stream) use (&$isEOF) {
+				$wrappedData = CallbackWrapper::wrap($data, null, null, null, null, function ($stream) use (&$isEOF) {
 					$isEOF = feof($stream);
 				});
 

diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php
@@ -28,13 +28,20 @@
 namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
 use OC\Files\Storage\Local;
+use OC\Files\Storage\Temporary;
+use OC\Files\Storage\Wrapper\PermissionsMask;
 use OC\Files\View;
+use OCA\DAV\Connector\Sabre\File;
+use OCP\Constants;
 use OCP\Files\ForbiddenException;
 use OCP\Files\Storage;
 use OCP\IConfig;
 use Test\HookHelper;
 use OC\Files\Filesystem;
 use OCP\Lock\ILockingProvider;
+use Test\TestCase;
+use Test\Traits\MountProviderTrait;
+use Test\Traits\UserTrait;
 
 /**
  * Class File
@@ -43,7 +50,9 @@
  *
  * @package OCA\DAV\Tests\unit\Connector\Sabre
  */
-class FileTest extends \Test\TestCase {
+class FileTest extends TestCase {
+	use MountProviderTrait;
+	use UserTrait;
 
 	/**
 	 * @var string
@@ -61,9 +70,8 @@ public function setUp() {
 
 		\OC_Hook::clear();
 
-		$this->user = $this->getUniqueID('user_');
-		$userManager = \OC::$server->getUserManager();
-		$userManager->createUser($this->user, 'pass');
+		$this->user = 'test_user';
+		$this->createUser($this->user, 'pass');
 
 		$this->loginAsUser($this->user);
 
@@ -79,15 +87,14 @@ public function tearDown() {
 	}
 
 	/**
-	 * @return \PHPUnit_Framework_MockObject_MockObject | Storage
+	 * @return \PHPUnit_Framework_MockObject_MockObject|Storage
 	 */
 	private function getMockStorage() {
 		$storage = $this->getMockBuilder(Storage::class)
 			->disableOriginalConstructor()
 			->getMock();
-		$storage->expects($this->any())
-			->method('getId')
-			->will($this->returnValue('home::someuser'));
+		$storage->method('getId')
+			->willReturn('home::someuser');
 		return $storage;
 	}
 
@@ -1163,4 +1170,35 @@ public function testGetThrowsIfNoPermission() {
 
 		$file->get();
 	}
+
+	public function testSimplePutNoCreatePermissions() {
+		$this->logout();
+
+		$storage = new Temporary([]);
+		$storage->file_put_contents('file.txt', 'old content');
+		$noCreateStorage = new PermissionsMask([
+			'storage'=> $storage,
+			'mask' => Constants::PERMISSION_ALL -  Constants::PERMISSION_CREATE
+		]);
+
+		$this->registerMount($this->user, $noCreateStorage, '/' . $this->user . '/files/root');
+
+		$this->loginAsUser($this->user);
+
+		$view = new View('/' . $this->user . '/files');
+
+		$info = $view->getFileInfo('root/file.txt');
+
+		$file = new File($view, $info);
+
+		// beforeMethod locks
+		$view->lockFile('root/file.txt', ILockingProvider::LOCK_SHARED);
+
+		$file->put($this->getStream('new content'));
+
+		// afterMethod unlocks
+		$view->unlockFile('root/file.txt', ILockingProvider::LOCK_SHARED);
+
+		$this->assertEquals('new content', $view->file_get_contents('root/file.txt'));
+	}
 }