diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 4b11155f22..163de3a976 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -8,6 +8,7 @@ namespace OCA\Richdocuments\AppInfo; +use OCA\DAV\Events\SabrePluginAddEvent; use OCA\Files_Sharing\Event\ShareLinkAccessedEvent; use OCA\Richdocuments\AppConfig; use OCA\Richdocuments\Capabilities; @@ -15,6 +16,7 @@ use OCA\Richdocuments\Db\WopiMapper; use OCA\Richdocuments\Listener\AddContentSecurityPolicyListener; use OCA\Richdocuments\Listener\AddFeaturePolicyListener; +use OCA\Richdocuments\Listener\AddSabrePluginListener; use OCA\Richdocuments\Listener\BeforeFetchPreviewListener; use OCA\Richdocuments\Listener\BeforeGetTemplatesListener; use OCA\Richdocuments\Listener\BeforeTemplateRenderedListener; @@ -43,6 +45,7 @@ use OCP\AppFramework\Bootstrap\IBootstrap; use OCP\AppFramework\Bootstrap\IRegistrationContext; use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent; +use OCP\BeforeSabrePubliclyLoadedEvent; use OCP\Collaboration\Reference\RenderReferenceEvent; use OCP\Collaboration\Resources\LoadAdditionalScriptsEvent; use OCP\Files\Storage\IStorage; @@ -80,6 +83,8 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class); $context->registerEventListener(BeforeGetTemplatesEvent::class, BeforeGetTemplatesListener::class); $context->registerEventListener(OverwritePublicSharePropertiesEvent::class, OverwritePublicSharePropertiesListener::class); + $context->registerEventListener(SabrePluginAddEvent::class, AddSabrePluginListener::class); + $context->registerEventListener(BeforeSabrePubliclyLoadedEvent::class, AddSabrePluginListener::class); $context->registerReferenceProvider(OfficeTargetReferenceProvider::class); $context->registerSensitiveMethods(WopiMapper::class, [ 'getPathForToken', diff --git a/lib/DAV/SecureViewPlugin.php b/lib/DAV/SecureViewPlugin.php new file mode 100644 index 0000000000..10f3b66e99 --- /dev/null +++ b/lib/DAV/SecureViewPlugin.php @@ -0,0 +1,85 @@ +secureViewService->isEnabled()) { + return; + } + $server->on('propFind', $this->handleGetProperties(...)); + } + + private function handleGetProperties(PropFind $propFind, INode $node): void { + if (!$node instanceof Node) { + return; + } + + $requestedProperties = $propFind->getRequestedProperties(); + if (!in_array(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, $requestedProperties, true)) { + return; + } + $currentValue = $propFind->get(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME); + if ($currentValue === 'true') { + // We won't unhide, hence can return early + return; + } + + if (!$this->isDownloadable($node->getNode())) { + // 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? + $propFind->set(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true'); + // avoid potential race condition with FilesPlugin that may set it to "false" + $propFind->handle(FilesPlugin::SHARE_HIDE_DOWNLOAD_PROPERTYNAME, 'true'); + } + } + + private function isDownloadable(\OCP\Files\Node $node): bool { + $storage = $node->getStorage(); + if ($this->wopiMiddleware->isWOPIRequest() + || $storage === null + || !$storage->instanceOfStorage(SecureViewWrapper::class) + ) { + return true; + } + + try { + return !$this->secureViewService->shouldSecure($node->getInternalPath(), $storage); + } catch (StorageNotAvailableException|ForbiddenException|NotFoundException $e) { + // Exceptions cannot be nicely inferred. + return false; + } catch (\Throwable $e) { + $this->logger->warning('SecureViewPlugin caught an exception that likely is ignorable. Still preventing download.', + ['exception' => $e,] + ); + return false; + } + } +} diff --git a/lib/Listener/AddSabrePluginListener.php b/lib/Listener/AddSabrePluginListener.php new file mode 100644 index 0000000000..b8ff149e32 --- /dev/null +++ b/lib/Listener/AddSabrePluginListener.php @@ -0,0 +1,35 @@ + */ +class AddSabrePluginListener implements IEventListener { + + public function __construct( + protected ContainerInterface $server, + ) { + } + + public function handle(Event $event): void { + if ( + !$event instanceof SabrePluginAddEvent + && !$event instanceof BeforeSabrePubliclyLoadedEvent + ) { + return; + } + $davServer = $event->getServer(); + $davServer->addPlugin($this->server->get(SecureViewPlugin::class)); + } +} diff --git a/lib/Service/SecureViewService.php b/lib/Service/SecureViewService.php new file mode 100644 index 0000000000..894dc2f2f2 --- /dev/null +++ b/lib/Service/SecureViewService.php @@ -0,0 +1,60 @@ +appConfig->getValueString(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no') !== 'no'; + } + + /** + * @throws NotFoundException + */ + public function shouldSecure(string $path, IStorage $storage, bool $tryOpen = true): bool { + if ($tryOpen) { + // pity… fopen() does not document any possible Exceptions + $fp = $storage->fopen($path, 'r'); + fclose($fp); + } + + $cacheEntry = $storage->getCache()->get($path); + if (!$cacheEntry) { + $parent = dirname($path); + if ($parent === '.') { + $parent = ''; + } + $cacheEntry = $storage->getCache()->get($parent); + if (!$cacheEntry) { + throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId())); + } + } + + $isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class); + /** @noinspection PhpPossiblePolymorphicInvocationInspection */ + /** @psalm-suppress UndefinedMethod **/ + $share = $isSharedStorage ? $storage->getShare() : null; + $userId = $this->userSession->getUser()?->getUID(); + + return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null); + } +} diff --git a/lib/Storage/SecureViewWrapper.php b/lib/Storage/SecureViewWrapper.php index a3ce2c616b..86a72ac083 100644 --- a/lib/Storage/SecureViewWrapper.php +++ b/lib/Storage/SecureViewWrapper.php @@ -11,10 +11,9 @@ use OC\Files\Storage\Wrapper\Wrapper; use OCA\Richdocuments\Middleware\WOPIMiddleware; use OCA\Richdocuments\PermissionManager; +use OCA\Richdocuments\Service\SecureViewService; use OCP\Files\ForbiddenException; use OCP\Files\IRootFolder; -use OCP\Files\NotFoundException; -use OCP\Files\Storage\ISharedStorage; use OCP\Files\Storage\IStorage; use OCP\IUserSession; use OCP\Server; @@ -24,6 +23,7 @@ class SecureViewWrapper extends Wrapper { private WOPIMiddleware $wopiMiddleware; private IRootFolder $rootFolder; private IUserSession $userSession; + private SecureViewService $secureViewService; private string $mountPoint; @@ -34,6 +34,7 @@ public function __construct(array $parameters) { $this->wopiMiddleware = Server::get(WOPIMiddleware::class); $this->rootFolder = Server::get(IRootFolder::class); $this->userSession = Server::get(IUserSession::class); + $this->secureViewService = Server::get(SecureViewService::class); $this->mountPoint = $parameters['mountPoint']; } @@ -78,41 +79,15 @@ public function rename(string $source, string $target): bool { * @throws ForbiddenException */ private function checkFileAccess(string $path): void { - if ($this->shouldSecure($path) && !$this->wopiMiddleware->isWOPIRequest()) { + if (!$this->wopiMiddleware->isWOPIRequest() && $this->secureViewService->shouldSecure($path, $this, false)) { throw new ForbiddenException('Download blocked due the secure view policy', false); } } - private function shouldSecure(string $path, ?IStorage $sourceStorage = null): bool { - if ($sourceStorage !== $this && $sourceStorage !== null) { - $fp = $sourceStorage->fopen($path, 'r'); - fclose($fp); - } - - $storage = $sourceStorage ?? $this; - $cacheEntry = $storage->getCache()->get($path); - if (!$cacheEntry) { - $parent = dirname($path); - if ($parent === '.') { - $parent = ''; - } - $cacheEntry = $storage->getCache()->get($parent); - if (!$cacheEntry) { - throw new NotFoundException(sprintf('Could not find cache entry for path and parent of %s within storage %s ', $path, $storage->getId())); - } - } - - $isSharedStorage = $storage->instanceOfStorage(ISharedStorage::class); - - $share = $isSharedStorage ? $storage->getShare() : null; - $userId = $this->userSession->getUser()?->getUID(); - - return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null); - } - - private function checkSourceAndTarget(string $source, string $target, ?IStorage $sourceStorage = null): void { - if ($this->shouldSecure($source, $sourceStorage) && !$this->shouldSecure($target)) { + if ($this->secureViewService->shouldSecure($source, $sourceStorage ?? $this, $sourceStorage !== null) + && !$this->secureViewService->shouldSecure($target, $this) + ) { throw new ForbiddenException('Download blocked due the secure view policy. The source requires secure view that the target cannot offer.', false); } } diff --git a/tests/features/bootstrap/RichDocumentsContext.php b/tests/features/bootstrap/RichDocumentsContext.php index cd78443ec5..40aaa13dfd 100644 --- a/tests/features/bootstrap/RichDocumentsContext.php +++ b/tests/features/bootstrap/RichDocumentsContext.php @@ -30,6 +30,7 @@ class RichDocumentsContext implements Context { private $fileIds = []; /** @var array List of templates fetched for a given file type */ private $templates = []; + private array $directoryListing = []; /** @BeforeScenario */ public function gatherContexts(BeforeScenarioScope $scope) { @@ -75,6 +76,47 @@ public function userOpens($user, $file) { Assert::assertNotEmpty($this->wopiToken); } + /** + * @When the download button for :path will be visible to :user + */ + public function downloadButtonIsVisible(string $path, string $user): void { + $this->downloadButtonIsNotVisibleOrNot($path, $user, true); + } + + /** + * @When the download button for :path will not be visible to :user + */ + public function downloadButtonIsNotVisible(string $path, string $user): void { + $this->downloadButtonIsNotVisibleOrNot($path, $user, false); + } + + private function downloadButtonIsNotVisibleOrNot(string $path, string $user, bool $isVisible): void { + $hideDownloadProperty = '{http://nextcloud.org/ns}hide-download'; + $this->serverContext->usingWebAsUser($user); + $fileInfo = $this->filesContext->listFolder($path, 0, [$hideDownloadProperty]); + + if ($isVisible) { + Assert::assertTrue(!isset($fileInfo[$hideDownloadProperty]) || $fileInfo[$hideDownloadProperty] === 'false'); + } else { + Assert::assertTrue(isset($fileInfo[$hideDownloadProperty]), 'property is not set'); + Assert::assertTrue($fileInfo[$hideDownloadProperty] === 'true', 'property is not true'); + Assert::assertTrue(isset($fileInfo[$hideDownloadProperty]) && $fileInfo[$hideDownloadProperty] === 'true'); + } + } + + /** + * @When the download button for :path will not be visible in the last link share + */ + public function theDownloadButtonWillNotBeVisibleInLastLinkShare(string $path): void { + $hideDownloadProperty = '{http://nextcloud.org/ns}hide-download'; + $this->serverContext->usingWebAsUser(); + $shareToken = $this->sharingContext->getLastShareData()['token']; + $davClient = $this->filesContext->getPublicSabreClient($shareToken); + $result = $davClient->propFind($path, ['{http://nextcloud.org/ns}hide-download'], 1); + $fileInfo = $result[array_key_first($result)]; + Assert::assertTrue(!isset($fileInfo[$hideDownloadProperty]) || $fileInfo[$hideDownloadProperty] === 'true'); + } + public function generateTokenWithApi($user, $fileId, ?string $shareToken = null, ?string $path = null, ?string $guestName = null) { $this->serverContext->usingWebAsUser($user); $this->serverContext->sendJSONRequest('POST', '/index.php/apps/richdocuments/token', [ @@ -248,4 +290,26 @@ public function renameFileTo($user, $file, $newName) { $this->wopiContext->collaboraRenamesTo($fileId, $newName); } + + /** + * @When admin enables secure view + */ + public function enableSecureView(): void { + $this->serverContext->actAsAdmin(function () { + $watermarkKeysToEnable = [ + 'watermark_enabled', + 'watermark_linkAll', + 'watermark_shareRead', + ]; + + foreach ($watermarkKeysToEnable as $configKey) { + $this->serverContext->sendOCSRequest( + 'POST', + 'apps/provisioning_api/api/v1/config/apps/files/' . $configKey, + ['value' => 'yes'], + ); + $this->serverContext->assertHttpStatusCode(200); + } + }); + } } diff --git a/tests/features/secure-view.feature b/tests/features/secure-view.feature new file mode 100644 index 0000000000..6317bfbb1a --- /dev/null +++ b/tests/features/secure-view.feature @@ -0,0 +1,38 @@ +Feature: API + + Background: + Given user "user1" exists + And user "user2" exists + And admin enables secure view + + Scenario: Download button is not shown in public shares + Given as user "user1" + And User "user1" creates a folder "NewFolder" + And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt" + And as "user1" create a share with + | path | /NewFolder | + | shareType | 3 | + Then the download button for "/NewFolder/file.odt" will be visible to "user1" + And the download button for "file.odt" will not be visible in the last link share + + Scenario: Download button is not shown in internal read-only shares + Given as user "user1" + And User "user1" creates a folder "NewFolder" + And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt" + And as "user1" create a share with + | path | /NewFolder | + | shareType | 0 | + | shareWith | user2 | + | permissions | 1 | + Then the download button for "/NewFolder/file.odt" will not be visible to "user2" + + Scenario: Download button is shown in internal shares + Given as user "user1" + And User "user1" creates a folder "NewFolder" + And User "user1" uploads file "./../emptyTemplates/template.odt" to "/NewFolder/file.odt" + And as "user1" create a share with + | path | /NewFolder | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + Then the download button for "/NewFolder/file.odt" will be visible to "user2" diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index db37df5b10..59e368a73c 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -8,11 +8,6 @@ - - - - - diff --git a/tests/run-integration.sh b/tests/run-integration.sh index 4f09302398..46ee0f715d 100755 --- a/tests/run-integration.sh +++ b/tests/run-integration.sh @@ -55,7 +55,6 @@ $OCC config:system:set allow_local_remote_servers --value true --type bool $OCC config:system:set gs.trustedHosts 0 --value="localhost:$PORT_SERVERA" $OCC config:system:set gs.trustedHosts 1 --value="localhost:$PORT_SERVERB" - vendor/bin/behat $SCENARIO_TO_RUN RESULT=$? diff --git a/tests/stub.phpstub b/tests/stub.phpstub index 07183c3d4e..30bb9627b2 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -10,6 +10,76 @@ class OC_User { public static function setIncognitoMode($status) {} } +namespace OC\Files\Storage\Wrapper { + class Wrapper implements \OCP\Files\Storage\IStorage{ + protected $storage; + + public function __construct(array $parameters) {} + public function copy(string $source, string $target): bool {} + public function copyFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {} + public function moveFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {} + public function rename(string $source, string $target): bool {} + public function getId(): string {}; + public function mkdir(string $path): bool {}; + public function rmdir(string $path): bool {}; + public function opendir(string $path) {}; + public function is_dir(string $path): bool {}; + public function is_file(string $path): bool {}; + public function is_file(string $path): bool {}; + public function stat(string $path): array|false {}; + public function filetype(string $path): string|false {}; + public function filesize(string $path): int|float|false {}; + public function isCreatable(string $path): bool {}; + public function isReadable(string $path): bool {}; + public function isUpdatable(string $path): bool {}; + public function isDeletable(string $path): bool {}; + public function isSharable(string $path): bool {}; + public function getPermissions(string $path): int {}; + public function file_exists(string $path): bool {}; + public function filemtime(string $path): int|false {}; + public function file_get_contents(string $path): string|false {}; + public function file_put_contents(string $path, mixed $data): int|float|false {}; + public function unlink(string $path): bool {}; + public function rename(string $source, string $target): bool {}; + public function copy(string $source, string $target): bool {}; + public function fopen(string $path, string $mode) {}; + public function getMimeType(string $path): string|false {}; + public function hash(string $type, string $path, bool $raw = false): string|false {}; + public function free_space(string $path): int|float|false {}; + public function touch(string $path, ?int $mtime = null): bool {}; + public function getLocalFile(string $path): string|false {}; + public function hasUpdated(string $path, int $time): bool {}; + public function getCache(string $path = , ?IStorage $storage = null): ICache {}; + public function getScanner(string $path = , ?IStorage $storage = null): IScanner {}; + public function getOwner(string $path): string|false {}; + public function getWatcher(string $path = , ?IStorage $storage = null): IWatcher {}; + public function getPropagator(?IStorage $storage = null): IPropagator {}; + public function getUpdater(?IStorage $storage = null): IUpdater {}; + public function getStorageCache(): OCFilesCacheStorage {}; + public function getETag(string $path): string|false {}; + public function test(): bool {}; + public function isLocal(): bool {}; + public function instanceOfStorage(string $class): bool {}; + public function getInstanceOfStorage(string $class): ?IStorage {}; + public function __call(string $method, array $args) {}; + public function getDirectDownload(string $path): array|false {}; + public function getAvailability(): array {}; + public function setAvailability(bool $isAvailable): void {}; + public function verifyPath(string $path, string $fileName): void {}; + public function copyFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {}; + public function moveFromStorage(\OCP\Files\Storage\IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool {}; + public function getMetaData(string $path): ?array {}; + public function acquireLock(string $path, int $type, ILockingProvider $provider): void {}; + public function releaseLock(string $path, int $type, ILockingProvider $provider): void {}; + public function changeLock(string $path, int $type, ILockingProvider $provider): void {}; + public function needsPartFile(): bool {}; + public function writeStream(string $path, $stream, ?int $size = null): int {}; + public function getDirectoryContent(string $directory): Traversable {}; + public function isWrapperOf(IStorage $storage): bool {}; + public function setOwner(?string $user): void {}; + } +} + namespace OC\Hooks { interface Emitter { /** @@ -32,6 +102,20 @@ namespace OC\Hooks { } } +namespace OCA\DAV\Connector\Sabre { + abstract class Node { + public function getNode(): \OCP\Files\Node; + } + + class FilesPlugin { + public const SHARE_HIDE_DOWNLOAD_PROPERTYNAME = 'somePropertyName'; + } +} + +namespace OCA\DAV\Events { + class SabrePluginAddEvent extends \OCP\EventDispatcher\Event {} +} + namespace OCA\Federation { class TrustedServers { public function getServers() { @@ -147,3 +231,30 @@ namespace OCA\Talk\Events { } } } + +namespace Sabre\DAV { + interface INode {} + + class PropFind { + /** + * @return string[] + */ + public function getRequestedProperties(): array { + return []; + } + + public function get(string $key) { + return null; + } + + public function set(string $key, $value) {} + + public function handle(string $key, $valueOrCallable): void {} + } + + class Server { + public function on(string $eventName, callable $callable): void {} + } + + class ServerPlugin {} +}