Skip to content

Commit 3b27369

Browse files
blizzzblizzz
committed
fix(SecureView): hide disfunctional *download* files action
This is achieved by setting a specific DAV attribute. At the moment there is one handler in dav-apps FilesPlugin and it could overwrite the value with "false". We make sure not to downgrade here and prevent downgrade from dav (possible race condition). Signed-off-by: Arthur Schiwon <[email protected]>
1 parent 91b6659 commit 3b27369

File tree

10 files changed

+328
-38
lines changed

10 files changed

+328
-38
lines changed

‎lib/AppInfo/Application.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,15 @@
88

99
namespace OCA\Richdocuments\AppInfo;
1010

11+
use OCA\DAV\Events\SabrePluginAddEvent;
1112
use OCA\Files_Sharing\Event\ShareLinkAccessedEvent;
1213
use OCA\Richdocuments\AppConfig;
1314
use OCA\Richdocuments\Capabilities;
1415
use OCA\Richdocuments\Conversion\ConversionProvider;
1516
use OCA\Richdocuments\Db\WopiMapper;
1617
use OCA\Richdocuments\Listener\AddContentSecurityPolicyListener;
1718
use OCA\Richdocuments\Listener\AddFeaturePolicyListener;
19+
use OCA\Richdocuments\Listener\AddSabrePluginListener;
1820
use OCA\Richdocuments\Listener\BeforeFetchPreviewListener;
1921
use OCA\Richdocuments\Listener\BeforeGetTemplatesListener;
2022
use OCA\Richdocuments\Listener\BeforeTemplateRenderedListener;
@@ -49,6 +51,7 @@
4951
use OCP\AppFramework\Bootstrap\IBootstrap;
5052
use OCP\AppFramework\Bootstrap\IRegistrationContext;
5153
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
54+
use OCP\BeforeSabrePubliclyLoadedEvent;
5255
use OCP\Collaboration\Reference\RenderReferenceEvent;
5356
use OCP\Collaboration\Resources\LoadAdditionalScriptsEvent;
5457
use OCP\Files\Storage\IStorage;
@@ -86,6 +89,8 @@ public function register(IRegistrationContext $context): void {
8689
$context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class);
8790
$context->registerEventListener(BeforeGetTemplatesEvent::class, BeforeGetTemplatesListener::class);
8891
$context->registerEventListener(OverwritePublicSharePropertiesEvent::class, OverwritePublicSharePropertiesListener::class);
92+
$context->registerEventListener(SabrePluginAddEvent::class, AddSabrePluginListener::class);
93+
$context->registerEventListener(BeforeSabrePubliclyLoadedEvent::class, AddSabrePluginListener::class);
8994
$context->registerReferenceProvider(OfficeTargetReferenceProvider::class);
9095
$context->registerSensitiveMethods(WopiMapper::class, [
9196
'getPathForToken',

‎lib/DAV/SecureViewPlugin.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
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\DAV;
9+
10+
use OCA\DAV\Connector\Sabre\FilesPlugin;
11+
use OCA\DAV\Connector\Sabre\Node;
12+
use OCA\Richdocuments\Middleware\WOPIMiddleware;
13+
use OCA\Richdocuments\Service\SecureViewService;
14+
use OCA\Richdocuments\Storage\SecureViewWrapper;
15+
use OCP\Files\ForbiddenException;
16+
use OCP\Files\NotFoundException;
17+
use OCP\Files\StorageNotAvailableException;
18+
use OCP\IAppConfig;
19+
use Psr\Log\LoggerInterface;
20+
use Sabre\DAV\INode;
21+
use Sabre\DAV\PropFind;
22+
use Sabre\DAV\Server;
23+
use Sabre\DAV\ServerPlugin;
24+
25+
class SecureViewPlugin extends ServerPlugin {
26+
public function __construct(
27+
protected WOPIMiddleware $wopiMiddleware,
28+
protected IAppConfig $appConfig,
29+
protected SecureViewService $secureViewService,
30+
protected LoggerInterface $logger,
31+
) {
32+
}
33+
34+
public function initialize(Server $server) {
35+
if (!$this->secureViewService->isEnabled()) {
36+
return;
37+
}
38+
$server->on('propFind', $this->handleGetProperties(...));
39+
}
40+
41+
private function handleGetProperties(PropFind $propFind, INode $node): void {
42+
if (!$node instanceof Node) {
43+
return;
44+
}
45+
46+
$requestedProperties = $propFind->getRequestedProperties();
47+
if (!in_array(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, $requestedProperties, true)) {
48+
return;
49+
}
50+
$currentValue = $propFind->get(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME);
51+
if ($currentValue === 'true') {
52+
// We won't unhide, hence can return early
53+
return;
54+
}
55+
56+
if (!$this->isDownloadable($node->getNode())) {
57+
// FIXME: coordinate with Files how a better solution looks like. Maybe by setting it only to 'true' by any provider? To avoid overwriting. Or throwing a dedicated event in just this case?
58+
$propFind->set(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true');
59+
// avoid potential race condition with FilesPlugin that may set it to "false"
60+
$propFind->handle(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true');
61+
}
62+
}
63+
64+
private function isDownloadable(\OCP\Files\Node $node): bool {
65+
$storage = $node->getStorage();
66+
if ($this->wopiMiddleware->isWOPIRequest()
67+
|| $storage === null
68+
|| !$storage->instanceOfStorage(SecureViewWrapper::class)
69+
) {
70+
return true;
71+
}
72+
73+
try {
74+
return !$this->secureViewService->shouldSecure($node->getInternalPath(), $storage);
75+
} catch (StorageNotAvailableException|ForbiddenException|NotFoundException $e) {
76+
// Exceptions cannot be nicely inferred.
77+
return false;
78+
} catch (\Throwable $e) {
79+
$this->logger->warning('SecureViewPlugin caught an exception that likely is ignorable. Still preventing download.',
80+
['exception' => $e,]
81+
);
82+
return false;
83+
}
84+
}
85+
}

‎lib/Listener/AddSabrePluginListener.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
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\Listener;
9+
10+
use OCA\DAV\Events\SabrePluginAddEvent;
11+
use OCA\Richdocuments\DAV\SecureViewPlugin;
12+
use OCP\BeforeSabrePubliclyLoadedEvent;
13+
use OCP\EventDispatcher\Event;
14+
use OCP\EventDispatcher\IEventListener;
15+
use Psr\Container\ContainerInterface;
16+
17+
/** @template-implements IEventListener<SabrePluginAddEvent|BeforeSabrePubliclyLoadedEvent> */
18+
class AddSabrePluginListener implements IEventListener {
19+
20+
public function __construct(
21+
protected ContainerInterface $server,
22+
) {
23+
}
24+
25+
public function handle(Event $event): void {
26+
if (
27+
!$event instanceof SabrePluginAddEvent
28+
&& !$event instanceof BeforeSabrePubliclyLoadedEvent
29+
) {
30+
return;
31+
}
32+
$davServer = $event->getServer();
33+
$davServer->addPlugin($this->server->get(SecureViewPlugin::class));
34+
}
35+
}

‎lib/Service/SecureViewService.php‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
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+
/**
32+
* @throws NotFoundException
33+
*/
34+
public function shouldSecure(string $path, IStorage $storage, bool $tryOpen = true): bool {
35+
if ($tryOpen) {
36+
// pity… fopen() does not document any possible Exceptions
37+
$fp = $storage->fopen($path, 'r');
38+
fclose($fp);
39+
}
40+
41+
$cacheEntry = $storage->getCache()->get($path);
42+
if (!$cacheEntry) {
43+
$parent = dirname($path);
44+
if ($parent === '.') {
45+
$parent = '';
46+
}
47+
$cacheEntry = $storage->getCache()->get($parent);
48+
if (!$cacheEntry) {
49+
throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId()));
50+
}
51+
}
52+
53+
$isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class);
54+
/** @noinspection PhpPossiblePolymorphicInvocationInspection */
55+
/** @psalm-suppress UndefinedMethod **/
56+
$share = $isSharedStorage ? $storage->getShare() : null;
57+
$userId = $this->userSession->getUser()?->getUID();
58+
59+
return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null);
60+
}
61+
}

‎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/features/bootstrap/RichDocumentsContext.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ class RichDocumentsContext implements Context {
3030
private $fileIds = [];
3131
/** @var array List of templates fetched for a given file type */
3232
private $templates = [];
33+
private array $directoryListing = [];
3334

3435
/** @BeforeScenario */
3536
public function gatherContexts(BeforeScenarioScope $scope) {
@@ -75,6 +76,47 @@ public function userOpens($user, $file) {
7576
Assert::assertNotEmpty($this->wopiToken);
7677
}
7778

79+
/**
80+
* @When the download button for :path will be visible to :user
81+
*/
82+
public function downloadButtonIsVisible(string $path, string $user): void {
83+
$this->downloadButtonIsNotVisibleOrNot($path, $user, true);
84+
}
85+
86+
/**
87+
* @When the download button for :path will not be visible to :user
88+
*/
89+
public function downloadButtonIsNotVisible(string $path, string $user): void {
90+
$this->downloadButtonIsNotVisibleOrNot($path, $user, false);
91+
}
92+
93+
private function downloadButtonIsNotVisibleOrNot(string $path, string $user, bool $isVisible): void {
94+
$hideDownloadProperty = '{http://nextcloud.org/ns}hide-download';
95+
$this->serverContext->usingWebAsUser($user);
96+
$fileInfo = $this->filesContext->listFolder($path, 0, [$hideDownloadProperty]);
97+
98+
if ($isVisible) {
99+
Assert::assertTrue(!isset($fileInfo[$hideDownloadProperty]) || $fileInfo[$hideDownloadProperty] === 'false');
100+
} else {
101+
Assert::assertTrue(isset($fileInfo[$hideDownloadProperty]), 'property is not set');
102+
Assert::assertTrue($fileInfo[$hideDownloadProperty] === 'true', 'property is not true');
103+
Assert::assertTrue(isset($fileInfo[$hideDownloadProperty]) && $fileInfo[$hideDownloadProperty] === 'true');
104+
}
105+
}
106+
107+
/**
108+
* @When the download button for :path will not be visible in the last link share
109+
*/
110+
public function theDownloadButtonWillNotBeVisibleInLastLinkShare(string $path): void {
111+
$hideDownloadProperty = '{http://nextcloud.org/ns}hide-download';
112+
$this->serverContext->usingWebAsUser();
113+
$shareToken = $this->sharingContext->getLastShareData()['token'];
114+
$davClient = $this->filesContext->getPublicSabreClient($shareToken);
115+
$result = $davClient->propFind($path, ['{http://nextcloud.org/ns}hide-download'], 1);
116+
$fileInfo = $result[array_key_first($result)];
117+
Assert::assertTrue(!isset($fileInfo[$hideDownloadProperty]) || $fileInfo[$hideDownloadProperty] === 'true');
118+
}
119+
78120
public function generateTokenWithApi($user, $fileId, ?string $shareToken = null, ?string $path = null, ?string $guestName = null) {
79121
$this->serverContext->usingWebAsUser($user);
80122
$this->serverContext->sendJSONRequest('POST', '/index.php/apps/richdocuments/token', [

‎tests/features/secure-view.feature‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
Feature: API
2+
3+
Background:
4+
Given user "user1" exists
5+
And user "user2" exists
6+
7+
Scenario: Download button is not shown in public shares
8+
Given as user "user1"
9+
And User "user1" creates a folder "NewFolder"
10+
And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt"
11+
And as "user1" create a share with
12+
| path | /NewFolder |
13+
| shareType | 3 |
14+
Then the download button for "/NewFolder/file.odt" will be visible to "user1"
15+
And the download button for "file.odt" will not be visible in the last link share
16+
17+
Scenario: Download button is not shown in internal read-only shares
18+
Given as user "user1"
19+
And User "user1" creates a folder "NewFolder"
20+
And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt"
21+
And as "user1" create a share with
22+
| path | /NewFolder |
23+
| shareType | 0 |
24+
| shareWith | user2 |
25+
| permissions | 1 |
26+
Then the download button for "/NewFolder/file.odt" will not be visible to "user2"
27+
28+
Scenario: Download button is shown in internal shares
29+
Given as user "user1"
30+
And User "user1" creates a folder "NewFolder"
31+
And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt"
32+
And as "user1" create a share with
33+
| path | /NewFolder |
34+
| shareType | 0 |
35+
| shareWith | user2 |
36+
| permissions | 31 |
37+
Then the download button for "/NewFolder/file.odt" will be visible to "user2"

‎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>

0 commit comments

Comments
 (0)