Skip to content

Commit b09a890

Browse files
blizzzblizzz
authored
Merge pull request #5200 from nextcloud/backport/5171/stable31
[stable31] fix(SecureView): hide disfunctional *download* files action
2 parents f1f0936 + 07a9430 commit b09a890

File tree

10 files changed

+405
-38
lines changed

10 files changed

+405
-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;
@@ -43,6 +45,7 @@
4345
use OCP\AppFramework\Bootstrap\IBootstrap;
4446
use OCP\AppFramework\Bootstrap\IRegistrationContext;
4547
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
48+
use OCP\BeforeSabrePubliclyLoadedEvent;
4649
use OCP\Collaboration\Reference\RenderReferenceEvent;
4750
use OCP\Collaboration\Resources\LoadAdditionalScriptsEvent;
4851
use OCP\Files\Storage\IStorage;
@@ -80,6 +83,8 @@ public function register(IRegistrationContext $context): void {
8083
$context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class);
8184
$context->registerEventListener(BeforeGetTemplatesEvent::class, BeforeGetTemplatesListener::class);
8285
$context->registerEventListener(OverwritePublicSharePropertiesEvent::class, OverwritePublicSharePropertiesListener::class);
86+
$context->registerEventListener(SabrePluginAddEvent::class, AddSabrePluginListener::class);
87+
$context->registerEventListener(BeforeSabrePubliclyLoadedEvent::class, AddSabrePluginListener::class);
8388
$context->registerReferenceProvider(OfficeTargetReferenceProvider::class);
8489
$context->registerSensitiveMethods(WopiMapper::class, [
8590
'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: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
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 OCA\Richdocuments\AppConfig;
11+
use OCA\Richdocuments\PermissionManager;
12+
use OCP\Files\NotFoundException;
13+
use OCP\Files\Storage\ISharedStorage;
14+
use OCP\Files\Storage\IStorage;
15+
use OCP\IAppConfig;
16+
use OCP\IUserSession;
17+
18+
class SecureViewService {
19+
public function __construct(
20+
protected IUserSession $userSession,
21+
protected PermissionManager $permissionManager,
22+
protected IAppConfig $appConfig,
23+
) {
24+
}
25+
26+
public function isEnabled(): bool {
27+
return $this->appConfig->getValueString(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no') !== 'no';
28+
}
29+
30+
/**
31+
* @throws NotFoundException
32+
*/
33+
public function shouldSecure(string $path, IStorage $storage, bool $tryOpen = true): bool {
34+
if ($tryOpen) {
35+
// pity… fopen() does not document any possible Exceptions
36+
$fp = $storage->fopen($path, 'r');
37+
fclose($fp);
38+
}
39+
40+
$cacheEntry = $storage->getCache()->get($path);
41+
if (!$cacheEntry) {
42+
$parent = dirname($path);
43+
if ($parent === '.') {
44+
$parent = '';
45+
}
46+
$cacheEntry = $storage->getCache()->get($parent);
47+
if (!$cacheEntry) {
48+
throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId()));
49+
}
50+
}
51+
52+
$isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class);
53+
/** @noinspection PhpPossiblePolymorphicInvocationInspection */
54+
/** @psalm-suppress UndefinedMethod **/
55+
$share = $isSharedStorage ? $storage->getShare() : null;
56+
$userId = $this->userSession->getUser()?->getUID();
57+
58+
return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null);
59+
}
60+
}

‎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: 64 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', [
@@ -248,4 +290,26 @@ public function renameFileTo($user, $file, $newName) {
248290

249291
$this->wopiContext->collaboraRenamesTo($fileId, $newName);
250292
}
293+
294+
/**
295+
* @When admin enables secure view
296+
*/
297+
public function enableSecureView(): void {
298+
$this->serverContext->actAsAdmin(function () {
299+
$watermarkKeysToEnable = [
300+
'watermark_enabled',
301+
'watermark_linkAll',
302+
'watermark_shareRead',
303+
];
304+
305+
foreach ($watermarkKeysToEnable as $configKey) {
306+
$this->serverContext->sendOCSRequest(
307+
'POST',
308+
'apps/provisioning_api/api/v1/config/apps/files/' . $configKey,
309+
['value' => 'yes'],
310+
);
311+
$this->serverContext->assertHttpStatusCode(200);
312+
}
313+
});
314+
}
251315
}

0 commit comments

Comments
 (0)