Skip to content

Commit 6a21e7b

Browse files
nickvergessennickvergessen
authored
Merge pull request #527 from nextcloud/backport/522/stable28
[stable28] fix(wrapper): Correctly handle directories
2 parents 2ae8984 + 1bca31d commit 6a21e7b

File tree

5 files changed

+60
-30
lines changed

5 files changed

+60
-30
lines changed

‎lib/StorageWrapper.php‎

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ public function __construct($parameters) {
5656
/**
5757
* @throws ForbiddenException
5858
*/
59-
protected function checkFileAccess(string $path, bool $isDir = false): void {
60-
$this->operation->checkFileAccess($this, $path, $isDir);
59+
protected function checkFileAccess(string $path, ?bool $isDir = null): void {
60+
$this->operation->checkFileAccess($this, $path, is_bool($isDir) ? $isDir : $this->is_dir($path));
6161
}
6262

6363
/*
@@ -165,7 +165,7 @@ public function getPermissions($path) {
165165
* @throws ForbiddenException
166166
*/
167167
public function file_get_contents($path) {
168-
$this->checkFileAccess($path);
168+
$this->checkFileAccess($path, false);
169169
return $this->storage->file_get_contents($path);
170170
}
171171

@@ -178,7 +178,7 @@ public function file_get_contents($path) {
178178
* @throws ForbiddenException
179179
*/
180180
public function file_put_contents($path, $data) {
181-
$this->checkFileAccess($path);
181+
$this->checkFileAccess($path, false);
182182
return $this->storage->file_put_contents($path, $data);
183183
}
184184

@@ -190,7 +190,7 @@ public function file_put_contents($path, $data) {
190190
* @throws ForbiddenException
191191
*/
192192
public function unlink($path) {
193-
$this->checkFileAccess($path);
193+
$this->checkFileAccess($path, false);
194194
return $this->storage->unlink($path);
195195
}
196196

@@ -203,8 +203,9 @@ public function unlink($path) {
203203
* @throws ForbiddenException
204204
*/
205205
public function rename($path1, $path2) {
206-
$this->checkFileAccess($path1);
207-
$this->checkFileAccess($path2);
206+
$isDir = $this->is_dir($path1);
207+
$this->checkFileAccess($path1, $isDir);
208+
$this->checkFileAccess($path2, $isDir);
208209
return $this->storage->rename($path1, $path2);
209210
}
210211

@@ -217,8 +218,9 @@ public function rename($path1, $path2) {
217218
* @throws ForbiddenException
218219
*/
219220
public function copy($path1, $path2) {
220-
$this->checkFileAccess($path1);
221-
$this->checkFileAccess($path2);
221+
$isDir = $this->is_dir($path1);
222+
$this->checkFileAccess($path1, $isDir);
223+
$this->checkFileAccess($path2, $isDir);
222224
return $this->storage->copy($path1, $path2);
223225
}
224226

@@ -231,7 +233,7 @@ public function copy($path1, $path2) {
231233
* @throws ForbiddenException
232234
*/
233235
public function fopen($path, $mode) {
234-
$this->checkFileAccess($path);
236+
$this->checkFileAccess($path, false);
235237
return $this->storage->fopen($path, $mode);
236238
}
237239

@@ -245,7 +247,7 @@ public function fopen($path, $mode) {
245247
* @throws ForbiddenException
246248
*/
247249
public function touch($path, $mtime = null) {
248-
$this->checkFileAccess($path);
250+
$this->checkFileAccess($path, false);
249251
return $this->storage->touch($path, $mtime);
250252
}
251253

@@ -274,7 +276,7 @@ public function getCache($path = '', $storage = null) {
274276
* @throws ForbiddenException
275277
*/
276278
public function getDirectDownload($path) {
277-
$this->checkFileAccess($path);
279+
$this->checkFileAccess($path, false);
278280
return $this->storage->getDirectDownload($path);
279281
}
280282

@@ -290,7 +292,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
290292
return $this->copy($sourceInternalPath, $targetInternalPath);
291293
}
292294

293-
$this->checkFileAccess($targetInternalPath);
295+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
294296
return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
295297
}
296298

@@ -306,7 +308,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
306308
return $this->rename($sourceInternalPath, $targetInternalPath);
307309
}
308310

309-
$this->checkFileAccess($targetInternalPath);
311+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
310312
return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
311313
}
312314

@@ -315,18 +317,18 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
315317
*/
316318
public function writeStream(string $path, $stream, int $size = null): int {
317319
if (!$this->isPartFile($path)) {
318-
$this->checkFileAccess($path);
320+
$this->checkFileAccess($path, false);
319321
}
320322

321323
$result = $this->storage->writeStream($path, $stream, $size);
322324
if (!$this->isPartFile($path)) {
323325
return $result;
324326
}
325327

326-
// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
328+
// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
327329
// As an alternative we might be able to check on the cache update/insert/delete though the Cache wrapper
328330
try {
329-
$this->checkFileAccess($path);
331+
$this->checkFileAccess($path, false);
330332
} catch (\Exception $e) {
331333
$this->storage->unlink($path);
332334
throw $e;

‎tests/Integration/features/bootstrap/FeatureContext.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,16 @@ public function userSharesFile(string $sharer, string $file, string $sharee): vo
131131
// ChecksumsContext
132132
/**
133133
* @Then The webdav response should have a status code :statusCode
134-
* @param int $statusCode
134+
* @param string $statusCode
135135
* @throws \Exception
136136
*/
137137
public function theWebdavResponseShouldHaveAStatusCode($statusCode) {
138-
if ((int)$statusCode !== $this->response->getStatusCode()) {
138+
if (str_contains($statusCode, '|')) {
139+
$statusCodes = array_map('intval', explode('|', $statusCode));
140+
} else {
141+
$statusCodes = [(int) $statusCode];
142+
}
143+
if (!in_array($this->response->getStatusCode(), $statusCodes, true)) {
139144
throw new \Exception("Expected $statusCode, got ".$this->response->getStatusCode());
140145
}
141146
}

‎tests/Integration/features/bootstrap/WebDav.php‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -128,18 +128,19 @@ public function userMovesFile($user, $entry, $fileSource, $fileDestination) {
128128
}
129129

130130
/**
131-
* @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
131+
* @When /^User "([^"]*)" copies (file|folder|entry) "([^"]*)" to "([^"]*)"$/
132132
* @param string $user
133133
* @param string $fileSource
134134
* @param string $fileDestination
135135
*/
136-
public function userCopiesFileTo($user, $fileSource, $fileDestination) {
136+
public function userCopiesFileTo($user, $entry, $fileSource, $fileDestination) {
137137
$fullUrl = $this->baseUrl . $this->getDavFilesPath($user);
138138
$headers['Destination'] = $fullUrl . $fileDestination;
139139
try {
140140
$this->response = $this->makeDavRequest($user, 'COPY', $fileSource, $headers);
141141
} catch (\GuzzleHttp\Exception\ClientException $e) {
142-
// 4xx and 5xx responses cause an exception
142+
$this->response = $e->getResponse();
143+
} catch (\GuzzleHttp\Exception\ServerException $e) {
143144
$this->response = $e->getResponse();
144145
}
145146
}
@@ -383,7 +384,9 @@ public function theResponseShouldContainAnEmptyProperty($property) {
383384
}
384385
}
385386

386-
/*Returns the elements of a propfind, $folderDepth requires 1 to see elements without children*/
387+
/**
388+
* Returns the elements of a propfind, $folderDepth requires 1 to see elements without children
389+
*/
387390
public function listFolder($user, $path, $folderDepth, $properties = null) {
388391
$client = $this->getSabreClient($user);
389392
if (!$properties) {
@@ -421,7 +424,7 @@ public function propfindFile(string $user, string $path, string $properties = ''
421424
}
422425

423426
/**
424-
* Returns the elements of a searc command
427+
* Returns the elements of a search command
425428
* @param string $properties properties which needs to be included in the report
426429
* @param string $filterRules filter-rules to choose what needs to appear in the report
427430
*/
@@ -517,7 +520,8 @@ public function searchFile(string $user, ?string $properties = null, ?string $sc
517520
}
518521
}
519522

520-
/* Returns the elements of a report command
523+
/**
524+
* Returns the elements of a report command
521525
* @param string $user
522526
* @param string $path
523527
* @param string $properties properties which needs to be included in the report

‎tests/Integration/features/mimetypes.feature‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,23 @@
6565
And The webdav response should have a status code "403"
6666
And Downloading file "/nc.exe" as "test1"
6767
And The webdav response should have a status code "404"
68+
69+
Scenario: Blocking anything but folders still allows to rename folders
70+
Given User "test1" uploads file "data/hello" to "/hello"
71+
And user "admin" creates global flow with 200
72+
| name | Admin flow |
73+
| class | OCA\FilesAccessControl\Operation |
74+
| entity | OCA\WorkflowEngine\Entity\File |
75+
| events | [] |
76+
| operation | deny |
77+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "httpd/unix-directory"} |
78+
Given User "test1" created a folder "/folder"
79+
And The webdav response should have a status code "201"
80+
When User "test1" moves folder "/folder" to "/folder-renamed"
81+
And The webdav response should have a status code "201"
82+
When User "test1" copies folder "/folder-renamed" to "/folder-copied"
83+
And The webdav response should have a status code "201"
84+
When User "test1" moves file "/hello" to "/hello.txt"
85+
And The webdav response should have a status code "403"
86+
When User "test1" copies file "/hello" to "/hello.txt"
87+
And The webdav response should have a status code "403"

‎tests/Unit/StorageWrapperTest.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,23 +61,22 @@ protected function getInstance(array $methods = []) {
6161

6262
public function dataCheckFileAccess(): array {
6363
return [
64-
['path1'],
65-
['path2'],
64+
['path1', true],
65+
['path2', false],
6666
];
6767
}
6868

6969
/**
7070
* @dataProvider dataCheckFileAccess
71-
* @param string $path
7271
*/
73-
public function testCheckFileAccess(string $path): void {
72+
public function testCheckFileAccess(string $path, bool $isDir): void {
7473
$storage = $this->getInstance();
7574

7675
$this->operation->expects($this->once())
7776
->method('checkFileAccess')
7877
->with($storage, $path);
7978

80-
self::invokePrivate($storage, 'checkFileAccess', [$path]);
79+
self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
8180
}
8281

8382
public function dataSinglePath(): array {

0 commit comments

Comments
 (0)