Skip to content

Commit 2896f1e

Browse files
blizzzblizzz
committed
fixup! fix(SecureView): hide disfunctional *download* files action
1 parent 59c26bf commit 2896f1e

File tree

6 files changed

+132
-85
lines changed

6 files changed

+132
-85
lines changed

‎lib/DAV/SecureViewPlugin.php‎

Lines changed: 14 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,13 @@
99

1010
use OCA\DAV\Connector\Sabre\FilesPlugin;
1111
use OCA\DAV\Connector\Sabre\Node;
12-
use OCA\Richdocuments\AppConfig;
1312
use OCA\Richdocuments\Middleware\WOPIMiddleware;
14-
use OCA\Richdocuments\PermissionManager;
13+
use OCA\Richdocuments\Service\SecureViewService;
1514
use OCA\Richdocuments\Storage\SecureViewWrapper;
1615
use OCP\Files\ForbiddenException;
1716
use OCP\Files\NotFoundException;
18-
use OCP\Files\Storage\ISharedStorage;
19-
use OCP\Files\Storage\IStorage;
17+
use OCP\Files\StorageNotAvailableException;
2018
use OCP\IAppConfig;
21-
use OCP\IUserSession;
2219
use Sabre\DAV\INode;
2320
use Sabre\DAV\PropFind;
2421
use Sabre\DAV\Server;
@@ -27,14 +24,13 @@
2724
class SecureViewPlugin extends ServerPlugin {
2825
public function __construct(
2926
protected WOPIMiddleware $wopiMiddleware,
30-
protected IUserSession $userSession,
31-
protected PermissionManager $permissionManager,
3227
protected IAppConfig $appConfig,
28+
protected SecureViewService $secureViewService,
3329
) {
3430
}
3531

3632
public function initialize(Server $server) {
37-
if ($this->appConfig->getValueString(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no') === 'no') {
33+
if (!$this->secureViewService->isEnabled()) {
3834
return;
3935
}
4036
$server->on('propFind', $this->handleGetProperties(...));
@@ -64,49 +60,19 @@ private function handleGetProperties(PropFind $propFind, INode $node): void {
6460
}
6561

6662
private function isDownloadable(\OCP\Files\Node $node): bool {
67-
try {
68-
$this->checkFileAccess($node->getInternalPath(), $node->getStorage());
63+
$storage = $node->getStorage();
64+
if ($this->wopiMiddleware->isWOPIRequest()
65+
|| $storage === null
66+
|| !$storage->instanceOfStorage(SecureViewWrapper::class)
67+
) {
6968
return true;
70-
} catch (ForbiddenException) {
71-
return false;
72-
}
73-
}
74-
75-
private function checkFileAccess(string $path, IStorage $storage): void {
76-
if ($this->wopiMiddleware->isWOPIRequest() || !$storage->instanceOfStorage(SecureViewWrapper::class)) {
77-
return;
78-
}
79-
80-
if ($this->shouldSecure($path, $storage)) {
81-
throw new ForbiddenException('Download blocked due the secure view policy', false);
82-
}
83-
}
84-
85-
// FIXME: remove duplication by moving into a SecureViewService
86-
private function shouldSecure(string $path, ?IStorage $sourceStorage = null): bool {
87-
if ($sourceStorage !== $this && $sourceStorage !== null) {
88-
$fp = $sourceStorage->fopen($path, 'r');
89-
fclose($fp);
9069
}
9170

92-
$storage = $sourceStorage ?? $this;
93-
$cacheEntry = $storage->getCache()->get($path);
94-
if (!$cacheEntry) {
95-
$parent = dirname($path);
96-
if ($parent === '.') {
97-
$parent = '';
98-
}
99-
$cacheEntry = $storage->getCache()->get($parent);
100-
if (!$cacheEntry) {
101-
throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId()));
102-
}
71+
try {
72+
return !$this->secureViewService->shouldSecure($node->getInternalPath(), $storage);
73+
} catch (StorageNotAvailableException|ForbiddenException|NotFoundException $e) {
74+
// Exceptions cannot be nicely inferred.
75+
return false;
10376
}
104-
105-
$isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class);
106-
107-
$share = $isSharedStorage ? $storage->getShare() : null;
108-
$userId = $this->userSession->getUser()?->getUID();
109-
110-
return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null);
11177
}
11278
}

‎lib/Listener/AddSabrePluginListener.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OCP\EventDispatcher\IEventListener;
1515
use Psr\Container\ContainerInterface;
1616

17+
/** @template-implements IEventListener<SabrePluginAddEvent|BeforeSabrePubliclyLoadedEvent> */
1718
class AddSabrePluginListener implements IEventListener {
1819

1920
public function __construct(

‎lib/Service/SecureViewService.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace OCA\Richdocuments\Service;
9+
10+
use OC\Files\Storage\Wrapper\Wrapper;
11+
use OCA\Richdocuments\AppConfig;
12+
use OCA\Richdocuments\PermissionManager;
13+
use OCP\Files\NotFoundException;
14+
use OCP\Files\Storage\ISharedStorage;
15+
use OCP\Files\Storage\IStorage;
16+
use OCP\IAppConfig;
17+
use OCP\IUserSession;
18+
19+
class SecureViewService {
20+
public function __construct(
21+
protected IUserSession $userSession,
22+
protected PermissionManager $permissionManager,
23+
protected IAppConfig $appConfig,
24+
) {
25+
}
26+
27+
public function isEnabled(): bool {
28+
return $this->appConfig->getValueString(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no') !== 'no';
29+
}
30+
31+
public function shouldSecure(string $path, IStorage|Wrapper $storage, bool $tryOpen = true): bool {
32+
if ($tryOpen) {
33+
$fp = $storage->fopen($path, 'r');
34+
fclose($fp);
35+
}
36+
37+
$cacheEntry = $storage->getCache()->get($path);
38+
if (!$cacheEntry) {
39+
$parent = dirname($path);
40+
if ($parent === '.') {
41+
$parent = '';
42+
}
43+
$cacheEntry = $storage->getCache()->get($parent);
44+
if (!$cacheEntry) {
45+
throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId()));
46+
}
47+
}
48+
49+
$isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class);
50+
/** @noinspection PhpPossiblePolymorphicInvocationInspection */
51+
/** @psalm-suppress UndefinedMethod **/
52+
$share = $isSharedStorage ? $storage->getShare() : null;
53+
$userId = $this->userSession->getUser()?->getUID();
54+
55+
return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null);
56+
}
57+
}

‎lib/Storage/SecureViewWrapper.php‎

Lines changed: 7 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,9 @@
1111
use OC\Files\Storage\Wrapper\Wrapper;
1212
use OCA\Richdocuments\Middleware\WOPIMiddleware;
1313
use OCA\Richdocuments\PermissionManager;
14+
use OCA\Richdocuments\Service\SecureViewService;
1415
use OCP\Files\ForbiddenException;
1516
use OCP\Files\IRootFolder;
16-
use OCP\Files\NotFoundException;
17-
use OCP\Files\Storage\ISharedStorage;
1817
use OCP\Files\Storage\IStorage;
1918
use OCP\IUserSession;
2019
use OCP\Server;
@@ -24,6 +23,7 @@ class SecureViewWrapper extends Wrapper {
2423
private WOPIMiddleware $wopiMiddleware;
2524
private IRootFolder $rootFolder;
2625
private IUserSession $userSession;
26+
private SecureViewService $secureViewService;
2727

2828
private string $mountPoint;
2929

@@ -34,6 +34,7 @@ public function __construct(array $parameters) {
3434
$this->wopiMiddleware = Server::get(WOPIMiddleware::class);
3535
$this->rootFolder = Server::get(IRootFolder::class);
3636
$this->userSession = Server::get(IUserSession::class);
37+
$this->secureViewService = Server::get(SecureViewService::class);
3738

3839
$this->mountPoint = $parameters['mountPoint'];
3940
}
@@ -78,41 +79,15 @@ public function rename(string $source, string $target): bool {
7879
* @throws ForbiddenException
7980
*/
8081
private function checkFileAccess(string $path): void {
81-
if ($this->shouldSecure($path) && !$this->wopiMiddleware->isWOPIRequest()) {
82+
if (!$this->wopiMiddleware->isWOPIRequest() && $this->secureViewService->shouldSecure($path, $this, false)) {
8283
throw new ForbiddenException('Download blocked due the secure view policy', false);
8384
}
8485
}
8586

86-
private function shouldSecure(string $path, ?IStorage $sourceStorage = null): bool {
87-
if ($sourceStorage !== $this && $sourceStorage !== null) {
88-
$fp = $sourceStorage->fopen($path, 'r');
89-
fclose($fp);
90-
}
91-
92-
$storage = $sourceStorage ?? $this;
93-
$cacheEntry = $storage->getCache()->get($path);
94-
if (!$cacheEntry) {
95-
$parent = dirname($path);
96-
if ($parent === '.') {
97-
$parent = '';
98-
}
99-
$cacheEntry = $storage->getCache()->get($parent);
100-
if (!$cacheEntry) {
101-
throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId()));
102-
}
103-
}
104-
105-
$isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class);
106-
107-
$share = $isSharedStorage ? $storage->getShare() : null;
108-
$userId = $this->userSession->getUser()?->getUID();
109-
110-
return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null);
111-
}
112-
113-
11487
private function checkSourceAndTarget(string $source, string $target, ?IStorage $sourceStorage = null): void {
115-
if ($this->shouldSecure($source, $sourceStorage) && !$this->shouldSecure($target)) {
88+
if ($this->secureViewService->shouldSecure($source, $sourceStorage ?? $this, $sourceStorage !== null)
89+
&& !$this->secureViewService->shouldSecure($target, $this)
90+
) {
11691
throw new ForbiddenException('Download blocked due the secure view policy. The source requires secure view that the target cannot offer.', false);
11792
}
11893
}

‎tests/psalm-baseline.xml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,6 @@
88
<code><![CDATA[array_key_exists($key, self::APP_SETTING_TYPES) && self::APP_SETTING_TYPES[$key] === 'array']]></code>
99
</RedundantCondition>
1010
</file>
11-
<file src="lib/AppInfo/Application.php">
12-
<MissingDependency>
13-
<code><![CDATA[SecureViewWrapper|IStorage]]></code>
14-
</MissingDependency>
15-
</file>
1611
<file src="lib/Command/ActivateConfig.php">
1712
<UndefinedClass>
1813
<code><![CDATA[Command]]></code>

‎tests/stub.phpstub‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,18 @@ class OC_User {
1010
public static function setIncognitoMode($status) {}
1111
}
1212

13+
namespace OC\Files\Storage\Wrapper {
14+
class Wrapper {
15+
protected $storage;
16+
17+
public function __construct(array $parameters) {}
18+
public function copy(string $source, string $target): bool {}
19+
public function copyFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {}
20+
public function moveFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {}
21+
public function rename(string $source, string $target): bool {}
22+
}
23+
}
24+
1325
namespace OC\Hooks {
1426
interface Emitter {
1527
/**
@@ -32,6 +44,20 @@ namespace OC\Hooks {
3244
}
3345
}
3446

47+
namespace OCA\DAV\Connector\Sabre {
48+
abstract class Node {
49+
public function getNode(): \OCP\Files\Node;
50+
}
51+
52+
class FilesPlugin {
53+
public const SHARE_HIDE_DOWNLOAD_PROPERTYNAME = 'somePropertyName';
54+
}
55+
}
56+
57+
namespace OCA\DAV\Events {
58+
class SabrePluginAddEvent extends \OCP\EventDispatcher\Event {}
59+
}
60+
3561
namespace OCA\Federation {
3662
class TrustedServers {
3763
public function getServers() {
@@ -147,3 +173,30 @@ namespace OCA\Talk\Events {
147173
}
148174
}
149175
}
176+
177+
namespace Sabre\DAV {
178+
interface INode {}
179+
180+
class PropFind {
181+
/**
182+
* @return string[]
183+
*/
184+
public function getRequestedProperties(): array {
185+
return [];
186+
}
187+
188+
public function get(string $key) {
189+
return null;
190+
}
191+
192+
public function set(string $key, $value) {}
193+
194+
public function handle(string $key, $valueOrCallable): void {}
195+
}
196+
197+
class Server {
198+
public function on(string $eventName, callable $callable): void {}
199+
}
200+
201+
class ServerPlugin {}
202+
}

0 commit comments

Comments
 (0)