From 6433797fb7f66448de5f2e28c113db97434ed404 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 24 Apr 2024 07:03:14 +0200 Subject: [PATCH] fix(wrapper): Correctly set the flag whether entry is a directory Signed-off-by: Joas Schilling --- lib/StorageWrapper.php | 36 ++++++++++--------- .../features/bootstrap/FeatureContext.php | 9 +++-- .../Integration/features/bootstrap/WebDav.php | 16 +++++---- tests/Integration/features/mimetypes.feature | 20 +++++++++++ tests/Unit/StorageWrapperTest.php | 9 +++-- 5 files changed, 60 insertions(+), 30 deletions(-) diff --git a/lib/StorageWrapper.php b/lib/StorageWrapper.php index 6973efab..d47a9185 100644 --- a/lib/StorageWrapper.php +++ b/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,7 +317,7 @@ 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); @@ -323,10 +325,10 @@ public function writeStream(string $path, $stream, ?int $size = null): int { 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; diff --git a/tests/Integration/features/bootstrap/FeatureContext.php b/tests/Integration/features/bootstrap/FeatureContext.php index 85dfa9cc..aebd32b7 100644 --- a/tests/Integration/features/bootstrap/FeatureContext.php +++ b/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()); } } diff --git a/tests/Integration/features/bootstrap/WebDav.php b/tests/Integration/features/bootstrap/WebDav.php index a5f2c64f..25424fe3 100644 --- a/tests/Integration/features/bootstrap/WebDav.php +++ b/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 diff --git a/tests/Integration/features/mimetypes.feature b/tests/Integration/features/mimetypes.feature index c9f0ab77..1b4aacc7 100644 --- a/tests/Integration/features/mimetypes.feature +++ b/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" diff --git a/tests/Unit/StorageWrapperTest.php b/tests/Unit/StorageWrapperTest.php index 383b29e2..4477bee0 100644 --- a/tests/Unit/StorageWrapperTest.php +++ b/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 {