fix(wrapper): Correctly set the flag whether entry is a directory

Signed-off-by: Joas Schilling <[email protected]>

lib/StorageWrapper.php

@@ -56,8 +56,8 @@ public function __construct($parameters) {
 	/**
 	 * @throws ForbiddenException
 	 */
-	protected function checkFileAccess(string $path, bool $isDir = false): void {
-		$this->operation->checkFileAccess($this, $path, $isDir);
+	protected function checkFileAccess(string $path, ?bool $isDir = null): void {
+		$this->operation->checkFileAccess($this, $path, is_bool($isDir) ? $isDir : $this->is_dir($path));
 	}
 
 	/*
@@ -165,7 +165,7 @@ public function getPermissions($path) {
 	 * @throws ForbiddenException
 	 */
 	public function file_get_contents($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->file_get_contents($path);
 	}
 
@@ -178,7 +178,7 @@ public function file_get_contents($path) {
 	 * @throws ForbiddenException
 	 */
 	public function file_put_contents($path, $data) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->file_put_contents($path, $data);
 	}
 
@@ -190,7 +190,7 @@ public function file_put_contents($path, $data) {
 	 * @throws ForbiddenException
 	 */
 	public function unlink($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->unlink($path);
 	}
 
@@ -203,8 +203,9 @@ public function unlink($path) {
 	 * @throws ForbiddenException
 	 */
 	public function rename($path1, $path2) {
-		$this->checkFileAccess($path1);
-		$this->checkFileAccess($path2);
+		$isDir = $this->is_dir($path1);
+		$this->checkFileAccess($path1, $isDir);
+		$this->checkFileAccess($path2, $isDir);
 		return $this->storage->rename($path1, $path2);
 	}
 
@@ -217,8 +218,9 @@ public function rename($path1, $path2) {
 	 * @throws ForbiddenException
 	 */
 	public function copy($path1, $path2) {
-		$this->checkFileAccess($path1);
-		$this->checkFileAccess($path2);
+		$isDir = $this->is_dir($path1);
+		$this->checkFileAccess($path1, $isDir);
+		$this->checkFileAccess($path2, $isDir);
 		return $this->storage->copy($path1, $path2);
 	}
 
@@ -231,7 +233,7 @@ public function copy($path1, $path2) {
 	 * @throws ForbiddenException
 	 */
 	public function fopen($path, $mode) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->fopen($path, $mode);
 	}
 
@@ -245,7 +247,7 @@ public function fopen($path, $mode) {
 	 * @throws ForbiddenException
 	 */
 	public function touch($path, $mtime = null) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->touch($path, $mtime);
 	}
 
@@ -274,7 +276,7 @@ public function getCache($path = '', $storage = null) {
 	 * @throws ForbiddenException
 	 */
 	public function getDirectDownload($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->getDirectDownload($path);
 	}
 
@@ -290,7 +292,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 			return $this->copy($sourceInternalPath, $targetInternalPath);
 		}
 
-		$this->checkFileAccess($targetInternalPath);
+		$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
 		return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 
@@ -306,7 +308,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 			return $this->rename($sourceInternalPath, $targetInternalPath);
 		}
 
-		$this->checkFileAccess($targetInternalPath);
+		$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
 		return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 
@@ -315,18 +317,18 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 	 */
 	public function writeStream(string $path, $stream, ?int $size = null): int {
 		if (!$this->isPartFile($path)) {
-			$this->checkFileAccess($path);
+			$this->checkFileAccess($path, false);
 		}
 
 		$result = $this->storage->writeStream($path, $stream, $size);
 		if (!$this->isPartFile($path)) {
 			return $result;
 		}
 
-		// Required for object storage since  part file is not in the storage so we cannot check it before moving it to the storage
+		// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
 		// As an alternative we might be able to check on the cache update/insert/delete though the Cache wrapper
 		try {
-			$this->checkFileAccess($path);
+			$this->checkFileAccess($path, false);
 		} catch (\Exception $e) {
 			$this->storage->unlink($path);
 			throw $e;

tests/Integration/features/bootstrap/FeatureContext.php

@@ -131,11 +131,16 @@ public function userSharesFile(string $sharer, string $file, string $sharee): vo
 	// ChecksumsContext
 	/**
 	 * @Then The webdav response should have a status code :statusCode
-	 * @param int $statusCode
+	 * @param string $statusCode
 	 * @throws \Exception
 	 */
 	public function theWebdavResponseShouldHaveAStatusCode($statusCode) {
-		if ((int)$statusCode !== $this->response->getStatusCode()) {
+		if (str_contains($statusCode, '|')) {
+			$statusCodes = array_map('intval', explode('|', $statusCode));
+		} else {
+			$statusCodes = [(int) $statusCode];
+		}
+		if (!in_array($this->response->getStatusCode(), $statusCodes, true)) {
 			throw new \Exception("Expected $statusCode, got ".$this->response->getStatusCode());
 		}
 	}

tests/Integration/features/bootstrap/WebDav.php

@@ -128,18 +128,19 @@ public function userMovesFile($user, $entry, $fileSource, $fileDestination) {
 	}
 
 	/**
-	 * @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
+	 * @When /^User "([^"]*)" copies (file|folder|entry) "([^"]*)" to "([^"]*)"$/
 	 * @param string $user
 	 * @param string $fileSource
 	 * @param string $fileDestination
 	 */
-	public function userCopiesFileTo($user, $fileSource, $fileDestination) {
+	public function userCopiesFileTo($user, $entry, $fileSource, $fileDestination) {
 		$fullUrl = $this->baseUrl . $this->getDavFilesPath($user);
 		$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();
+		} catch (\GuzzleHttp\Exception\ServerException $e) {
 			$this->response = $e->getResponse();
 		}
 	}
@@ -383,7 +384,9 @@ public function theResponseShouldContainAnEmptyProperty($property) {
 		}
 	}
 
-	/*Returns the elements of a propfind, $folderDepth requires 1 to see elements without children*/
+	/**
+	 * Returns the elements of a propfind, $folderDepth requires 1 to see elements without children
+	 */
 	public function listFolder($user, $path, $folderDepth, $properties = null) {
 		$client = $this->getSabreClient($user);
 		if (!$properties) {
@@ -421,7 +424,7 @@ public function propfindFile(string $user, string $path, string $properties = ''
 	}
 
 	/**
-	 * Returns the elements of a searc command
+	 * Returns the elements of a search command
 	 * @param string $properties properties which needs to be included in the report
 	 * @param string $filterRules filter-rules to choose what needs to appear in the report
 	 */
@@ -517,7 +520,8 @@ public function searchFile(string $user, ?string $properties = null, ?string $sc
 		}
 	}
 
-	/* Returns the elements of a report command
+	/**
+	 * Returns the elements of a report command
 	 * @param string $user
 	 * @param string $path
 	 * @param string $properties properties which needs to be included in the report

tests/Integration/features/mimetypes.feature

@@ -65,3 +65,23 @@
     And The webdav response should have a status code "403"
     And Downloading file "/nc.exe" as "test1"
     And The webdav response should have a status code "404"
+
+  Scenario: Blocking anything but folders still allows to rename folders
+    Given User "test1" uploads file "data/hello" to "/hello"
+    And user "admin" creates global flow with 200
+    | name      | Admin flow                       |
+    | class     | OCA\FilesAccessControl\Operation |
+    | entity    | OCA\WorkflowEngine\Entity\File   |
+    | events    | []                               |
+    | operation | deny                             |
+    | checks-0  | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "httpd/unix-directory"} |
+    Given User "test1" created a folder "/folder"
+    And The webdav response should have a status code "201"
+    When User "test1" moves folder "/folder" to "/folder-renamed"
+    And The webdav response should have a status code "201"
+    When User "test1" copies folder "/folder-renamed" to "/folder-copied"
+    And The webdav response should have a status code "201"
+    When User "test1" moves file "/hello" to "/hello.txt"
+    And The webdav response should have a status code "403"
+    When User "test1" copies file "/hello" to "/hello.txt"
+    And The webdav response should have a status code "403|500"

tests/Unit/StorageWrapperTest.php

@@ -61,23 +61,22 @@ protected function getInstance(array $methods = []) {
 
 	public function dataCheckFileAccess(): array {
 		return [
-			['path1'],
-			['path2'],
+			['path1', true],
+			['path2', false],
 		];
 	}
 
 	/**
 	 * @dataProvider dataCheckFileAccess
-	 * @param string $path
 	 */
-	public function testCheckFileAccess(string $path): void {
+	public function testCheckFileAccess(string $path, bool $isDir): void {
 		$storage = $this->getInstance();
 
 		$this->operation->expects($this->once())
 			->method('checkFileAccess')
 			->with($storage, $path);
 
-		self::invokePrivate($storage, 'checkFileAccess', [$path]);
+		self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
 	}
 
 	public function dataSinglePath(): array {