Skip to content

Commit ab2af70

Browse files
salmart-devsalmart-dev
authored
Merge pull request #963 from nextcloud/feat/extended-acl-support
Add support for granular permissions in operation
2 parents c62814b + 52fad84 commit ab2af70

8 files changed

Lines changed: 485 additions & 38 deletions

File tree

‎lib/CacheWrapper.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ protected function formatCacheEntry($entry) {
4545
$jailedPath = $storage->getJailedPath($path);
4646
$path = $jailedPath ?? $path;
4747
}
48-
$this->operation->checkFileAccess($path, $this->mountPoint, $entry['mimetype'] === 'httpd/unix-directory', $entry);
48+
$permissions = $this->operation->checkFileAccess($path, $this->mountPoint, $entry['mimetype'] === 'httpd/unix-directory', $entry, 0);
49+
if ($permissions !== null) {
50+
$entry['permissions'] &= $permissions;
51+
}
4952
} catch (ForbiddenException) {
5053
$entry['permissions'] &= $this->mask;
5154
}

‎lib/Operation.php‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OC\Files\View;
1515
use OCA\GroupFolders\Mount\GroupMountPoint;
1616
use OCA\WorkflowEngine\Entity\File;
17+
use OCP\Constants;
1718
use OCP\EventDispatcher\Event;
1819
use OCP\Files\Cache\ICacheEntry;
1920
use OCP\Files\ForbiddenException;
@@ -46,19 +47,29 @@ public function __construct(
4647
}
4748

4849
/**
50+
* Checks if the user can access the file according to the configured flows.
51+
*
52+
* Flows with `deny` operation always take precedence and make the check fail.
53+
*
54+
* If only flows with permissions match, requiredPermissions is used to decide
55+
* when the check should fail or succeed. If its value is 0, the function simply
56+
* returns the permissions granted by the flows.
57+
*
4958
* @param array|ICacheEntry|null $cacheEntry
59+
* @param int $requiredPermissions Permissions required for the check
60+
* @return int|null If access is not blocked, the permissions allowed by the operations or null if not relevant.
5061
* @throws ForbiddenException
5162
*/
52-
public function checkFileAccess(string $path, IMountPoint $mountPoint, bool $isDir, $cacheEntry = null): void {
63+
public function checkFileAccess(string $path, IMountPoint $mountPoint, bool $isDir, $cacheEntry = null, int $requiredPermissions = Constants::PERMISSION_ALL): ?int {
5364
if (!$this->isBlockablePath($mountPoint, $path) || $this->isCreatingSkeletonFiles() || $this->nestingLevel !== 0) {
5465
// Allow creating skeletons and theming
5566
// https://github.com/nextcloud/files_accesscontrol/issues/5
5667
// https://github.com/nextcloud/files_accesscontrol/issues/12
57-
return;
68+
return null;
5869
}
5970
$storage = $mountPoint->getStorage();
6071
if ($storage === null) {
61-
return;
72+
return null;
6273
}
6374

6475
$this->nestingLevel++;
@@ -71,16 +82,43 @@ public function checkFileAccess(string $path, IMountPoint $mountPoint, bool $isD
7182
$ruleMatcher->setEntitySubject($this->fileEntity, $node);
7283
}
7384
$ruleMatcher->setOperation($this);
74-
$match = $ruleMatcher->getFlows();
85+
$match = $ruleMatcher->getFlows(false);
7586

7687
$this->nestingLevel--;
7788

7889
if (!empty($match)) {
90+
$isDenied = false;
91+
$computedPermissions = 0;
92+
foreach ($match as $operation) {
93+
$operationString = $operation['operation'];
94+
if ($operationString === 'deny') {
95+
$isDenied = true;
96+
// block file access as if a deny operation is present it should take precedence
97+
break;
98+
}
99+
100+
try {
101+
$parsedOperationParams = json_decode($operationString, true, flags: JSON_THROW_ON_ERROR);
102+
} catch (\JsonException) {
103+
// if we can't decode as JSON ignore...
104+
continue;
105+
}
106+
107+
$computedPermissions |= (int)($parsedOperationParams['permissions'] ?? 0);
108+
}
109+
110+
if (!$isDenied && ($computedPermissions & $requiredPermissions) === $requiredPermissions) {
111+
// enough permissions to perform the operation
112+
return $computedPermissions;
113+
}
114+
79115
$e = new \RuntimeException('Access denied for path ' . $path . ' that is ' . ($isDir ? '' : 'not ') . 'a directory and matches rules: ' . (string)json_encode($match));
80116
$this->logger->debug($e->getMessage(), ['exception' => $e]);
81117
// All Checks of one operation matched: prevent access
82118
throw new ForbiddenException('Access denied by access control', false);
83119
}
120+
121+
return null;
84122
}
85123

86124
protected function isBlockablePath(IMountPoint $mountPoint, string $path): bool {

‎lib/StorageWrapper.php‎

Lines changed: 42 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,11 @@ public function __construct($parameters) {
4040
}
4141

4242
/**
43+
* @see Operation::checkFileAccess()
4344
* @throws ForbiddenException
4445
*/
45-
protected function checkFileAccess(string $path, ?bool $isDir = null): void {
46-
$this->operation->checkFileAccess($path, $this->mount, is_bool($isDir) ? $isDir : $this->is_dir($path));
46+
protected function checkFileAccess(string $path, ?bool $isDir = null, ?int $permissions = null): ?int {
47+
return $this->operation->checkFileAccess($path, $this->mount, is_bool($isDir) ? $isDir : $this->is_dir($path), null, $permissions ?? Constants::PERMISSION_ALL);
4748
}
4849

4950
/*
@@ -59,7 +60,7 @@ protected function checkFileAccess(string $path, ?bool $isDir = null): void {
5960
*/
6061
#[\Override]
6162
public function mkdir($path): bool {
62-
$this->checkFileAccess($path, true);
63+
$this->checkFileAccess($path, true, Constants::PERMISSION_CREATE);
6364
return $this->storage->mkdir($path);
6465
}
6566

@@ -72,7 +73,7 @@ public function mkdir($path): bool {
7273
*/
7374
#[\Override]
7475
public function rmdir($path): bool {
75-
$this->checkFileAccess($path, true);
76+
$this->checkFileAccess($path, true, Constants::PERMISSION_DELETE);
7677
return $this->storage->rmdir($path);
7778
}
7879

@@ -85,7 +86,7 @@ public function rmdir($path): bool {
8586
#[\Override]
8687
public function isCreatable($path): bool {
8788
try {
88-
$this->checkFileAccess($path);
89+
$this->checkFileAccess($path, null, Constants::PERMISSION_CREATE);
8990
} catch (ForbiddenException) {
9091
return false;
9192
}
@@ -101,7 +102,7 @@ public function isCreatable($path): bool {
101102
#[\Override]
102103
public function isReadable($path): bool {
103104
try {
104-
$this->checkFileAccess($path);
105+
$this->checkFileAccess($path, null, Constants::PERMISSION_READ);
105106
} catch (ForbiddenException) {
106107
return false;
107108
}
@@ -117,7 +118,7 @@ public function isReadable($path): bool {
117118
#[\Override]
118119
public function isUpdatable($path): bool {
119120
try {
120-
$this->checkFileAccess($path);
121+
$this->checkFileAccess($path, null, Constants::PERMISSION_UPDATE);
121122
} catch (ForbiddenException) {
122123
return false;
123124
}
@@ -133,7 +134,7 @@ public function isUpdatable($path): bool {
133134
#[\Override]
134135
public function isDeletable($path): bool {
135136
try {
136-
$this->checkFileAccess($path);
137+
$this->checkFileAccess($path, null, Constants::PERMISSION_DELETE);
137138
} catch (ForbiddenException) {
138139
return false;
139140
}
@@ -143,10 +144,16 @@ public function isDeletable($path): bool {
143144
#[\Override]
144145
public function getPermissions($path): int {
145146
try {
146-
$this->checkFileAccess($path);
147+
$permissions = $this->checkFileAccess($path, null, 0);
147148
} catch (ForbiddenException) {
148149
return $this->mask;
149150
}
151+
152+
if ($permissions !== null) {
153+
// override with permissions granted by the operation, if any
154+
return $permissions & $this->storage->getPermissions($path);
155+
}
156+
150157
return $this->storage->getPermissions($path);
151158
}
152159

@@ -159,7 +166,7 @@ public function getPermissions($path): int {
159166
*/
160167
#[\Override]
161168
public function file_get_contents($path): string|false {
162-
$this->checkFileAccess($path, false);
169+
$this->checkFileAccess($path, false, Constants::PERMISSION_READ);
163170
return $this->storage->file_get_contents($path);
164171
}
165172

@@ -173,7 +180,7 @@ public function file_get_contents($path): string|false {
173180
*/
174181
#[\Override]
175182
public function file_put_contents(string $path, mixed $data): int|float|false {
176-
$this->checkFileAccess($path, false);
183+
$this->checkFileAccess($path, false, Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE);
177184
return $this->storage->file_put_contents($path, $data);
178185
}
179186

@@ -186,7 +193,7 @@ public function file_put_contents(string $path, mixed $data): int|float|false {
186193
*/
187194
#[\Override]
188195
public function unlink($path): bool {
189-
$this->checkFileAccess($path, false);
196+
$this->checkFileAccess($path, false, Constants::PERMISSION_DELETE);
190197
return $this->storage->unlink($path);
191198
}
192199

@@ -201,8 +208,8 @@ public function unlink($path): bool {
201208
#[\Override]
202209
public function rename($source, $target): bool {
203210
$isDir = $this->is_dir($source);
204-
$this->checkFileAccess($source, $isDir);
205-
$this->checkFileAccess($target, $isDir);
211+
$this->checkFileAccess($source, $isDir, Constants::PERMISSION_READ | Constants::PERMISSION_DELETE);
212+
$this->checkFileAccess($target, $isDir, Constants::PERMISSION_CREATE);
206213
return $this->storage->rename($source, $target);
207214
}
208215

@@ -217,8 +224,8 @@ public function rename($source, $target): bool {
217224
#[\Override]
218225
public function copy($source, $target): bool {
219226
$isDir = $this->is_dir($source);
220-
$this->checkFileAccess($source, $isDir);
221-
$this->checkFileAccess($target, $isDir);
227+
$this->checkFileAccess($source, $isDir, Constants::PERMISSION_READ);
228+
$this->checkFileAccess($target, $isDir, Constants::PERMISSION_CREATE);
222229
return $this->storage->copy($source, $target);
223230
}
224231

@@ -232,7 +239,18 @@ public function copy($source, $target): bool {
232239
*/
233240
#[\Override]
234241
public function fopen($path, $mode) {
235-
$this->checkFileAccess($path, false);
242+
$hasPlus = str_contains($mode, '+');
243+
$isRead = str_contains($mode, 'r');
244+
$isExclusive = str_contains($mode, 'x');
245+
$isWac = str_contains($mode, 'w') || str_contains($mode, 'a') || str_contains($mode, 'c');
246+
$checkPermissions = match (true) {
247+
$isWac => Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | ($hasPlus ? Constants::PERMISSION_READ : 0),
248+
$isExclusive => Constants::PERMISSION_CREATE | ($hasPlus ? Constants::PERMISSION_READ : 0),
249+
$isRead => Constants::PERMISSION_READ | ($hasPlus ? Constants::PERMISSION_UPDATE : 0),
250+
default => Constants::PERMISSION_ALL,
251+
};
252+
253+
$this->checkFileAccess($path, false, $checkPermissions);
236254
return $this->storage->fopen($path, $mode);
237255
}
238256

@@ -247,7 +265,7 @@ public function fopen($path, $mode) {
247265
*/
248266
#[\Override]
249267
public function touch($path, $mtime = null): bool {
250-
$this->checkFileAccess($path, false);
268+
$this->checkFileAccess($path, false, Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE);
251269
return $this->storage->touch($path, $mtime);
252270
}
253271

@@ -278,7 +296,7 @@ public function getCache($path = '', $storage = null): ICache {
278296
*/
279297
#[\Override]
280298
public function getDirectDownload($path): array|false {
281-
$this->checkFileAccess($path, false);
299+
$this->checkFileAccess($path, false, Constants::PERMISSION_READ);
282300
return $this->storage->getDirectDownload($path);
283301
}
284302

@@ -302,7 +320,7 @@ public function getDirectDownloadById(string $fileId): array|false {
302320
// We would have actually a result, so lets see if the user should be able to access it
303321
$path = $this->getCache()->getPathById((int)$fileId);
304322
if ($path !== null) {
305-
$this->checkFileAccess($path, false);
323+
$this->checkFileAccess($path, false, Constants::PERMISSION_READ);
306324
}
307325

308326
return $data;
@@ -321,7 +339,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
321339
return $this->copy($sourceInternalPath, $targetInternalPath);
322340
}
323341

324-
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
342+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath), Constants::PERMISSION_CREATE);
325343
return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
326344
}
327345

@@ -338,7 +356,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
338356
return $this->rename($sourceInternalPath, $targetInternalPath);
339357
}
340358

341-
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
359+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath), Constants::PERMISSION_CREATE);
342360
return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
343361
}
344362

@@ -348,7 +366,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
348366
#[\Override]
349367
public function writeStream(string $path, $stream, ?int $size = null): int {
350368
if (!$this->isPartFile($path)) {
351-
$this->checkFileAccess($path, false);
369+
$this->checkFileAccess($path, false, Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE);
352370
}
353371

354372
$result = parent::writeStream($path, $stream, $size);
@@ -359,7 +377,7 @@ public function writeStream(string $path, $stream, ?int $size = null): int {
359377
// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
360378
// As an alternative we might be able to check on the cache update/insert/delete though the Cache wrapper
361379
try {
362-
$this->checkFileAccess($path, false);
380+
$this->checkFileAccess($path, false, Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE);
363381
} catch (\Exception $e) {
364382
$this->storage->unlink($path);
365383
throw $e;

‎tests/Integration/features/author.feature‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,21 @@ Feature: Author
1616
Then The webdav response should have a status code "403"
1717
Then User "test1" sees no files in the trashbin
1818

19+
Scenario: Propfind has restrictive permissions when file is blocked
20+
Given User "test1" uploads file "data/textfile.txt" to "/foobar.txt"
21+
And The webdav response should have a status code "201"
22+
And user "admin" creates global flow with 200
23+
| name | Admin flow |
24+
| class | OCA\FilesAccessControl\Operation |
25+
| entity | OCA\WorkflowEngine\Entity\File |
26+
| events | [] |
27+
| operation | deny |
28+
| checks-0 | {"class":"OCA\\\\WorkflowEngine\\\\Check\\\\FileName","operator":"is","value":"foobar.txt"} |
29+
And as user "test1"
30+
When File "foobar.txt" in listing of folder "/" should have prop "oc:permissions" equal to "R"
31+
When Downloading file "/foobar.txt"
32+
Then The webdav response should have a status code "404"
33+
1934
Scenario: Downloading file is blocked
2035
Given User "test1" uploads file "data/textfile.txt" to "/foobar.txt"
2136
And The webdav response should have a status code "201"

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,37 @@ public function downloadedContentShouldBe($content) {
190190
Assert::assertEquals($content, (string)$this->response->getBody());
191191
}
192192

193+
/**
194+
* @Then /^File "([^"]*)" in listing of folder "([^"]*)" should have prop "([^"]*):([^"]*)" equal to "([^"]*)"$/
195+
*/
196+
public function checkPropForFileInFolder($file, $folder, $prefix, $prop, $value): void {
197+
$mappedNs = match ($prefix) {
198+
'oc' => '{http://owncloud.org/ns}',
199+
'nc' => '{http://nextcloud.org/ns}',
200+
'd' => '{DAV:}',
201+
's' => '{http://sabredav.org/ns}',
202+
default => throw new \UnexpectedValueException("Unknown prefix: $prefix")
203+
};
204+
205+
$propKey = "$mappedNs$prop";
206+
$response = $this->listFolder($this->currentUser, $folder, 1, [$propKey]);
207+
208+
$folder = trim($folder, '/');
209+
$file = trim($file, '/');
210+
$filePath = str_replace('//', '/', "/$folder/$file");
211+
$key = '/' . $this->getDavFilesPath($this->currentUser) . $filePath;
212+
if (!array_key_exists($key, $response)) {
213+
Assert::fail("File $file is not in folder $folder");
214+
}
215+
216+
if (!array_key_exists($propKey, $response[$key])) {
217+
Assert::fail("Property $propKey not found in file $file");
218+
}
219+
220+
$property = $response[$key][$propKey];
221+
Assert::assertEquals($value, $property);
222+
}
223+
193224
/**
194225
* @Then /^File "([^"]*)" should have prop "([^"]*):([^"]*)" equal to "([^"]*)"$/
195226
* @param string $file

0 commit comments

Comments
 (0)